7272 ensure patients are visible on oms central #7338

lache-melvin · 2025-04-15T06:08:30Z

👩🏻‍💻 What does this PR do?

Adds push of name/name_store_join to OMS Central, and additional "add patient visibility to central" call to the /name-store-join endpoint

Reasoning outlined in a README

💌 Any notes for the reviewer?

I did test all of the cases below, and it took a hot minute. Given I've done a full manual test and so would QA, maybe is ok for this to be a code review only idk lol

All cases working except "Patient B" below - a patient that is visible in multiple OMS stores already, and that already has vaccination records. Upgrade to 2.7 makes the vax card accessible in all the stores where patient is visible. But remote site is already at latest changelog cursor before upgrading, so OMS central doesn't think there are any new vaccination records that it needs to send out.

Reinitialising the remote site (after upgrading and a sync) would resolve this, as initialisation is from changelog 0. Given low likelihood of vaccination records existing, maybe this is actually an ok solution? Add to release notes/support upgrade info to check for this case and reinitialise if needed?

🧪 Testing

Dear QA: I am sorry but you'll need to reinitialise for this one 😢

📃 Documentation

Part of an epic: documentation will be completed for the feature as a whole
No documentation required: no user facing changes or a bug fix which isn't a change in behaviour
These areas should be updated or checked:
1.
2.

📃 Reviewer Checklist

The PR Reviewer(s) should fill out this section before approving the PR

Breaking Changes

No Breaking Changes in the Graphql API
Technically some Breaking Changes but not expected to impact any integrations

Issue Review

All requirements in original issue have been covered
A follow up issue(s) have been created to cover additional requirements

Tests Pass

Postgres
SQLite
Frontend

server/service/src/programs/patient/README.md

lache-melvin · 2025-04-16T00:27:23Z

server/repository/src/migrations/v2_07_00/add_patient_link_id_to_vaccination.rs

+                INSERT INTO changelog (table_name, record_id, row_action, store_id, name_link_id)
+                    SELECT 'vaccination', id, 'UPSERT', store_id, patient_link_id
+                    FROM vaccination
+                    WHERE patient_link_id <> '';


Hi @andreievg - lol sorry we've gone through this a few times, and I know low likelihood of vaccination records existing. But not inserting name_link_id with changelog here meant that if had existing vaccinations were created on OMS Central, they'd never end up with a changelog with name_link_id populated (that was only happening after remote site push).

These would then not sync out if patient visibility added. Best to insert changelog correctly with name_link_id, only where name_link_id does exist. Am I missing an edge case where this could error?

Hmm i am not sure tbh, i feel like in this case it's better to just WHERE patient_link_id IN (SELECT id from name_link_id), to be safe and reduce use case thinking

server/service/src/apis/api_on_central/patient_name_store_join.rs

…ites

lache-melvin · 2025-04-16T02:31:02Z

server/repository/src/migrations/v2_07_00/add_patient_link_id_to_vaccination.rs

+      Note! patient_link_id is required field, so if <2.7 sites sync vaccination records to 2.7+ OMS Central, they will
+      fail to integrate. They will re-sync and integrate when remote site upgrades, but records would be lost if
+      remote site reinitialises before upgrading. Considering this low likelihood, as feature not officially rolled out
+      before 2.7.


should this be added to release notes or something

lache-melvin · 2025-04-16T02:42:16Z

server/service/src/apis/api_on_central/patient_name_store_join.rs

+    ctx: &ServiceContext,
+    name_id: &str,
+) -> Result<(), CentralApiError> {
+    let timeout = Duration::from_secs(30);


Not sure how long I should set for timeout?

We've added wait for processors logic, I think that might be better? https://github.com/msupply-foundation/open-msupply/pull/1706/files

It would be different for the synchroniser_driver though, the trigger would also get a channel to report on finish, does that sounds like too much work ? (if you think it is, i'll have a good look at what you have here)

Btw for the patient_id thing in trigger I think we could have kept existing trigger and add another one trigger_with_patient_id.

And if we implement the waiting I would do create another trigger_with_await (or something like that)

Thanks, created issue - #7435

…-central

andreievg · 2025-04-16T21:53:21Z

server/service/src/programs/patient/search_central.rs

@@ -20,10 +21,13 @@ use crate::{

 use super::PatientSearch;

-#[derive(Debug)]
+#[derive(Error, Debug)]


andreievg · 2025-04-17T03:07:16Z

server/service/src/apis/api_on_central/patient_name_store_join.rs

+        // More robust to check patient record has been received
+        if !sync_status.is_syncing {
+            // If sync finished but integration of patient failed, will break after timeout
+            if check_patient_exists(&ctx.connection, &name_id)?.is_some() {


That's cool

andreievg · 2025-04-17T03:08:27Z

server/service/src/apis/api_on_central/patient_name_store_join.rs

+                break;
+            }
+        }
+        debug!("Patient not yet found, awaiting sync completion...");


Every 500ms ?

lol ever the optimist

andreievg · 2025-04-17T03:11:45Z

server/service/src/apis/api_on_central/patient_name_store_join.rs

+    Ok(())
+}
+
+impl From<GetActiveStoresOnSiteError> for CentralApiError {


At some point we agreed that ? make it hard to trace the conversion, so should only convert error for common things. Not requesting change now, but I would do a helper method in CentralApiError (that accept same thing as format_error)

Thanks, ended up replacing with map_error in the next PR, adding the processor 🙏

andreievg

Looks great, thanks @lache-melvin, we can do the await for sync refactor after we get 2.7RC (with this change) build out for testing : )

lache-melvin added 4 commits April 15, 2025 15:31

add patient sync readme

5698207

add changelogs for name/name_store_join on migration

ec6845b

add name to vaccination pull deps

b2ac973

add patient to store from central name_store_join method

09b8958

github-actions bot added this to the v2.7.0 milestone Apr 15, 2025

github-actions bot added enhancement New feature or request sync Team Ruru 🦉 Roxy, Noel, Chris, Aimee, Laché Severity: High Bugs breaking core functionality or with no/unacceptable workaround. High impact. Next patch release feature: vaccine card deployment gotchas labels Apr 15, 2025

lache-melvin commented Apr 15, 2025

View reviewed changes

server/service/src/programs/patient/README.md Outdated Show resolved Hide resolved

lache-melvin added 2 commits April 16, 2025 12:11

update readme

86e13c4

update comments

09682a8

lache-melvin commented Apr 16, 2025

View reviewed changes

server/service/src/apis/api_on_central/patient_name_store_join.rs Outdated Show resolved Hide resolved

lache-melvin added 2 commits April 16, 2025 14:05

add timeout and patient lookup to patient name store join endpoint

2ece264

flag failed integration of vax records before 2.7 upgrade of remote s…

121197e

…ites

lache-melvin marked this pull request as ready for review April 16, 2025 02:15

github-actions bot added the Severity: Hotfix Extreme user impact. Can't finalise invoices. Needs patch release and installation immediately. label Apr 16, 2025

lache-melvin commented Apr 16, 2025

View reviewed changes

lache-melvin removed the Severity: Hotfix Extreme user impact. Can't finalise invoices. Needs patch release and installation immediately. label Apr 16, 2025

lache-melvin commented Apr 16, 2025

View reviewed changes

lache-melvin added 2 commits April 16, 2025 15:09

Merge branch 'v2.7.0-RC' into 7272-ensure-patients-are-visible-on-oms…

f4b829c

…-central

add logs

77dfcba

andreievg self-assigned this Apr 16, 2025

Merge branch 'v2.7.0-RC' into 7272-ensure-patients-are-visible-on-oms…

92aea63

…-central

andreievg reviewed Apr 16, 2025

View reviewed changes

andreievg reviewed Apr 17, 2025

View reviewed changes

andreievg approved these changes Apr 17, 2025

View reviewed changes

lache-melvin mentioned this pull request Apr 22, 2025

Implement a "wait_for_sync" helper #7435

Open

pr fixes

442ac8d

lache-melvin merged commit 6c29d21 into v2.7.0-RC Apr 22, 2025
2 checks passed

lache-melvin deleted the 7272-ensure-patients-are-visible-on-oms-central branch April 22, 2025 00:57

lache-melvin mentioned this pull request Apr 22, 2025

Make patient data shareable when patient created in OG #7272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

7272 ensure patients are visible on oms central #7338

7272 ensure patients are visible on oms central #7338

lache-melvin commented Apr 15, 2025 •

edited

Loading

lache-melvin Apr 16, 2025

andreievg Apr 16, 2025

lache-melvin Apr 16, 2025

lache-melvin Apr 16, 2025

andreievg Apr 16, 2025

andreievg Apr 16, 2025 •

edited

Loading

lache-melvin Apr 22, 2025

andreievg Apr 16, 2025

andreievg Apr 17, 2025

andreievg Apr 17, 2025

lache-melvin Apr 22, 2025

andreievg Apr 17, 2025

lache-melvin Apr 22, 2025

andreievg left a comment •

edited

Loading

7272 ensure patients are visible on oms central #7338

7272 ensure patients are visible on oms central #7338

Conversation

lache-melvin commented Apr 15, 2025 • edited Loading

👩🏻‍💻 What does this PR do?

💌 Any notes for the reviewer?

🧪 Testing

📃 Documentation

📃 Reviewer Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreievg Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreievg left a comment • edited Loading

Choose a reason for hiding this comment

lache-melvin commented Apr 15, 2025 •

edited

Loading

andreievg Apr 16, 2025 •

edited

Loading

andreievg left a comment •

edited

Loading