Skip to content

fix(Sync): Order Problem#5767

Closed
igbanam wants to merge 4 commits intomasterfrom
igbanam/SIMPLEBACK-52/sync-order-problem
Closed

fix(Sync): Order Problem#5767
igbanam wants to merge 4 commits intomasterfrom
igbanam/SIMPLEBACK-52/sync-order-problem

Conversation

@igbanam
Copy link
Contributor

@igbanam igbanam commented Feb 11, 2026

Story card: SIMPLEBACK-52

Because

Patients with CVD Risks cannot sync

This addresses

Removing the foreign key to allow loose association

Test instructions

n/a

The error we see with CVD Risks is because its introduction assumes the
association which holds at the application layer also holds at the
logical layer. While this assumption is intuitive, it goes against how
sync was designed. The association in the logical layer is a loose
association which allows for orphan associations. This allows for some
late-binding after sync is completed. This is necessary because — for
some reason!!! (and this is the flaw) — the ID the server should use in
persisting a resource is generated on the client side.

Fixes SIMPLEBACK-52
@igbanam igbanam self-assigned this Feb 11, 2026
@igbanam igbanam requested a review from a team February 11, 2026 09:15
CREATE TABLE public.cvd_risks (
id uuid DEFAULT gen_random_uuid() NOT NULL,
risk_score character varying,
patient_id uuid NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just remove the constraint no? This is removing the column patient_id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. The migration only removes the constraint. But because I run a different version of PostgreSQL locally. db:migrate needs manual cleanup. I'll fix this soon

('20251215113615'),
('20251219061210'),
('20260128094448');
('20260105123000'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already added in the master. Probably just a rebasing issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased again and this is still here. I guess if it's in master, and the PR is mergeable, it should be okay.

@igbanam
Copy link
Contributor Author

igbanam commented Feb 11, 2026

I've fixed the DB structure, @aagrawalrtsl

@igbanam igbanam requested a review from aagrawalrtsl February 11, 2026 15:49

CREATE INDEX index_cvd_risks_on_patient_id ON public.cvd_risks USING btree (patient_id);


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can leave this index added.
thoughts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Technically. But where else is the link between CVD Risks and Patients?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are using cvd_risks for statin reports in metabase. Having this index would affect that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's odd that the Rails migration took this away. Or maybe this is an effect of the state of structure.sql which causes migrations to need manual clean-ups.

@igbanam
Copy link
Contributor Author

igbanam commented Feb 12, 2026

I'll create a new PR for this.

@igbanam igbanam closed this Feb 12, 2026
@igbanam igbanam deleted the igbanam/SIMPLEBACK-52/sync-order-problem branch February 12, 2026 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants