search

msupply-foundation / mobile

Open source mobile app for medical inventory control
http://msupply.org.nz/mobile
Other
43 stars 27 forks source link

(Kiribati) Merged patient vaccinations not displaying in Patient Vaccination History #4484

Closed adamdewey closed 2 years ago

adamdewey commented 2 years ago

Describe the bug

Kiribati have patients that have been entered into the system twice through human-error.

Each of the patient records has had a vaccination event recorded against them.

Kiribati now want to merge the patients records in mSupply Desktop to eliminate one of the patient records but retain both vaccination events.

It appears that the vaccination event is correctly transferred from the eliminated patient record to the preserved patient record in mSupply Desktop, but the Patient Vaccination History in Mobile does not display the transferred vaccination event.

image

image

image

(Seems that there's some timing discrepancy between the server and tablets here as date is off by 1 day)

image

image

To reproduce

Steps to reproduce the behaviour:

  1. Create a new patient record in Mobile
  2. Administer a vaccination
  3. Create another new patient record in Mobile
  4. Administer another vaccination
  5. Merge the patient records in Deskop
  6. Observe that Desktop shows both vaccination events but Mobile only displays one.

Expected behaviour

Both the original vaccination event and the transferred vaccination event to be displayed in the Patient Vaccination History page.

Proposed Solution

Leave if you don't know how to fix/implement. Edit this issue description and explain here if you know the best path of implementing the fix within the codebase.

Version and device info

Additional context

Add any other context about the problem here.

josh-griffin commented 2 years ago

Thanks @adamdewey 😁

I'm not 100% sure on all of the issues, but I would start with merging properly:

Merging is done here: https://github.com/openmsupply/mobile/blob/a43354062a621a9d51a741bf52eba226c8e2a725/src/sync/incomingSyncUtils.js#L1290

It has not been changed since we started having patients in mobile

Should be quite easy to implement - the merge logic itself isn't very complicated, though the mergeRecords code is, in my opinion, confusing to work with

kat-ms commented 2 years ago

Merging is done here:

https://github.com/openmsupply/mobile/blob/a43354062a621a9d51a741bf52eba226c8e2a725/src/sync/incomingSyncUtils.js#L1290

It has not been changed since we started having patients in mobile

Should be quite easy to implement - the merge logic itself isn't very complicated, though the mergeRecords code is, in my opinion, confusing to work with

Thanks @joshxg yeah I think the merge process is missing an update of name notes (and maybe some other stuff too). If I clear the app data and resync it pulls the merged records and history fine from Desktop... will have a look 👀

josh-griffin commented 2 years ago

@katherine-sussol thanks

I believe this might take a little bit of desktop looking into also. Merging isss a littttle bit complicated..

Some info which might cause confusion or help:

adamdewey commented 2 years ago

Oh interesting.. I helpfully didn't include which store I was in in the screenshots above when I did the merge...

Hmm, I would have thought I would have gone into the same store I created the patient in, but you never know with this guy

adamdewey commented 2 years ago

But also, thinking about it we do need to be able to merge cross-stores in case different teams (stores) added patient records multiple times (because they couldn't find the original record due to slight spelling variation etc)

josh-griffin commented 2 years ago

I suspect so. Duplicates have always been a pretty big concern, we're just not handling them very well .. :)

Technically, I believe you can transfer the 'home store' of a patient, which means you can still do the merge

adamdewey commented 2 years ago

Ah ok, as long as there's some kind of way to do it then that's all good :)

kat-ms commented 2 years ago

(I think) a small tweak addresses the issues with vaccinations (and ADRs also it seems) not being migrated - if we add the NameNote and ADR name fields here: https://github.com/openmsupply/mobile/blob/a43354062a621a9d51a741bf52eba226c8e2a725/src/sync/incomingSyncUtils.js#L1248-L1259

Unfortunately there's still a bit of weird behaviour going on. I think it's pre-existing as it exists in the current release but if you're trying to do something with a patient (e.g. dispense something to them) and they get deleted due a merge (or I guess even if it's deleted on the central server? I haven't tested that yet), the app will crash because the record being edited has suddenly disappeared 🤣 . Trying to see what to do about this but it's also sort of edge case-ey.

There's also several other links to the Name table that aren't quite accounted for yet (e.g. NameTagJoin, NameStoreJoin), reviewing those too 🤔

Chris-Petty commented 2 years ago

@joshxg @katherine-sussol I'm wondering how exactly the vaccination records are structured and how they play into merging (just so I can wrap my head around the desktop side).

  1. There is a single name note that contains JSON data of all the vaccines for a patient?
  2. After merge does it merge the name_notes (or should it?), or does the patient just end up with their vaccination data across 2 name_notes?

Just judging from Adams photos and my guesses, I guess there are indeed two name_notes on the patient and mobile just flattens the data across them when showing in UI?

josh-griffin commented 2 years ago

I don't remember - I can look into it soon.

Are you looking at doing some work in this area for desktop? Kat has a PR that needs reviewing - seems it would be very useful if you could review the PR?

Chris-Petty commented 2 years ago

I don't remember - I can look into it soon.

Are you looking at doing some work in this area for desktop? Kat has a PR that needs reviewing - seems it would be very useful if you could review the PR?

@adamdewey nudged me on server side scripts to manage merging a whole bunch of patients as specified from a spreadsheet, but figured that the fixing of merging would be a prerequisite 😁 ! So just checking in here to see what's up. I'm going to avoid reviewing mobile side of this- I've had no involvement for a long long time (no idea about the merging implementation). Generally high priority focus on site refactoring on server but can manage making sure the desktop side of this well defined and got people on it!

kat-ms commented 2 years ago

@Chris-Petty The name notes are currently separate, so every time there is a new vaccine -> new name note gets created. The vaccine history on mobile pulls all name notes with patient event with code of 'vaccination' (and linked to selected patient)

The PR is pretty small (given the merging framework already existed in mobile and seems to work mostly as expected): Before deleting the merged patient all name_note records linked to the to-be-deleted patient get updated to be linked to the to-be-kept patient record (and it does the same for some other linked records that were missed previously).

Chris-Petty commented 2 years ago

@Chris-Petty The name notes are currently separate, so every time there is a new vaccine -> new name note gets created. The vaccine history on mobile pulls all name notes with patient event with code of 'vaccination' (and linked to selected patient)

Interesting- In my memory there was a time that it was just one name_note? I recall like an accidental recursive thing that every time a new vaccine happened it took the old json and nested it in the new json (which would grow exponentially I think?)? Was that changed to individual notes for each event? Is my memory completely off? My only involvement was a blip of helping Anil write import scripts months ago after all.

What I'm hearing is that it seeeeems merging on the desktop side is OK? I think I understand enough here to double check. At a glance it seems added to the right place to work, which seems confirmed by details in Adams issue! So long as the merge doesn't need to take the 2 notes and munge the JSON into one, I think we're good! (Clarity on that is basically the itch I was trying to get to here)

The PR is pretty small (given the merging framework already existed in mobile and seems to work mostly as expected): Before deleting the merged patient all name_note records linked to the to-be-deleted patient get updated to be linked to the to-be-kept patient record (and it does the same for some other linked records that were missed previously).

I take that it's pretty small, that's good. But given the experience and ownership of code here (as far even as good protips earlier on this issue) I think it makes much more sense for @joshxg to pickup the review- probably half time and of better quality than I can do.

kat-ms commented 2 years ago

Interesting- In my memory there was a time that it was just one name_note? I recall like an accidental recursive thing that every time a new vaccine happened it took the old json and nested it in the new json (which would grow exponentially I think?)? Was that changed to individual notes for each event? Is my memory completely off? My only involvement was a blip of helping Anil write import scripts months ago after all.

Oh, hm I think you are remembering the name_note recursion correctly, it was removed with this commit, which went into 8.3.0: 419443d.

The original implementation always did create separate name_note records from the looks of things though (so I guess there was a period where we had multiple name_note records and unintentional recursion... 🤔 ): https://github.com/openmsupply/mobile/pull/4303

I am pretty sure there's nothing in mobile trying to do a recursive lookup of patient name notes 😆 (I can't comment with the same certainty about other areas)

kat-ms commented 2 years ago

Closed by #4497. Tangentially related issue logged here: https://github.com/openmsupply/mobile/issues/4503

ArpitaSussol commented 2 years ago

Tested in v8.3.1-rc0

Testing

Steps to reproduce or otherwise test the changes of this PR: