msupply-foundation / mobile

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

Migrate data from transact.custom_data to Vax name_note #4636

Open kat-ms opened 2 years ago

kat-ms commented 2 years ago

Is your feature request related to a problem? Please describe.

Child of #4624

Describe the solution you'd like

A clear and concise description of what you want to happen.

Implementation

A description of how the solution would best be implemented in the code base

Describe alternatives you've considered

A clear and concise description of any alternative solutions or features you've considered.

Additional context

Add any other context or screenshots about the feature request here.

kat-ms commented 2 years ago

NB: As of mobile 8.3.0 VaccinationEvent name_note records contain the original transaction record (including custom_data). Much duplication 😆 . But also (in the interests of minimising dev work) means there aren't strictly any changes needed to link the vax name note with the supplemental data name_note (aside from backfilling for historical data)

adamdewey commented 2 years ago

Ohh.... how come this was introduced?

I thought at one point that Josh actually removed this as he said it was a bug - maybe it slipped back in again??

kat-ms commented 2 years ago

Ohh.... how come this was introduced?

I thought at one point that Josh actually removed this as he said it was a bug - maybe it slipped back in again??

Hmm...Are you referring to having the [transact] data inside the vaccination name_note records? That particular work was done as part of https://github.com/openmsupply/mobile/issues/4406 - and was intentional to ensure there was a proper link between the vax record and the transaction (maybe the whole payload wasn't needed... but we erred on the side of caution in case we needed any of the info later).

There was an issue prior that meant a patient's name_note records were being recursively added to vaccination name_note records. We snuck a fix for this with the above issue (well, we just removed nameNotes from the patient object so the recursive nesting wouldn't happen).

It was doing something like this:

Vax Event NameNote:
{
  "refused": "false",
  "itemName": "some vaccine",
  [...] -- bunch of other fields omitted for brevity
  "patient": {
    "firstName": "John"
    [...] -- bunch of other fields omitted for brevity
    "nameNotes": {
       [this name note here] 
    }
  }
}

... I feel like I'm saying name_note way too many times 😕

adamdewey commented 2 years ago

Oh cool, thanks for the explanation Kat :)

Yeah, I must have been getting confused - must have been thinking of the patient name_note being put into the vaccination name_note.

Sweet - well that's great if the transaction ID is already available in the name_notes then!

sworup commented 2 years ago

@adamdewey I don't think we have any migration code. I guess the custom_data are in name_notes, so was this issue created to tackle for legacy database? Either way we are not doing this right now. Moving it to be worked on later.

adamdewey commented 2 years ago

@sworup - yes I think it must have been to clean up legacy data but I can't remember off the top of my head what it was right now 🤔

Requires some investigation, but yes I think it should be fine to do after the upgrade rollout.

I guess we might encounter some issues with editing or viewing their very first vaccination event records, possibly?