search

msupply-foundation / mobile

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

Vaccine module #1353

Closed josh-griffin closed 3 years ago

josh-griffin commented 5 years ago

Description

Vaccine module integration and slight refactor into current develop

Previous epic: https://github.com/sussol/org-issues/issues/23

Context

The Vaccine module was never merged into the code base, and now the current branch is very far behind master. Needs to be redone. It is in production in Solomon Islands and Vanuatu.

Update (13/05):

Vaccines is the main feature for v6. Main feature branch merged into develop to get everything up-to-date and to get a head start on integration testing. All future vaccines work should be based off/merged into develop.

INFORMATION

Vaccine work has been merged into the main develop branch.

Old feature branches:

TAG which is deployed in Vannuatu & Solomon Islands: https://github.com/openmsupply/mobile/commits/v2.2.0-RC4

TASKS

I've written the issues quite vague and non-specific as more specific details may not be needed dependent on the tissue and reviewer. Can provide far greater detail on a specific issue when needed!!

Tasks will be separated between back-end logic and front-end UI to try to avoid as much blocking as possible. Setup issues should likely all be done before others.

Each grouping will most likely need to be done roughly in this order

SETUP

BACKEND

FRONTEND

DESKTOP

TESTING

DOCUMENTATION

beginnings of documentation is here:

Design documents:

To be written 'draft like' as features are completed:

josh-griffin commented 5 years ago

Potential issues:

From PR: #1358

josh-griffin commented 4 years ago

Some things to potentially revist, just some initial thoughts returning back to this work!

andreievg commented 4 years ago

Discussion on 04/03

Q: Why not just merge what we already had (in exactly the same way that we had it), with code changes to fit in with new/existing data table ?

A: It's not a prototype anymore, what we merge into develop would be MVP. We have a chance to reconsider a few of the following (and we have some experience to back it up)

NOTE: we are not thinking of migrating all data for existing prototype sites, they of course should still work (shouldn't need to reconfigure sensors/fridge), but will have different data structure and existing data might be lost (basically we want to minimise effort needed to make existing sites work and are willing to compromise loosing data in prototype sites)

Database Structure and Logic

Q: Same why not keep as is ? A: There is quite a bit of aggregation logic on mobile end that is perhaps excessive. It makes code digestion very difficult. Also existing way might not scale. So requirement here are:

Current State of Things

BlueMeastro tempo disc temperature logger is quite sensitive to ambient temperature, and it's not suspended in liquid, so every time fridge is opened, temperature reading jumps to room temperature quite quickly, to avoid this we configure device to record temperature every 5 minutes and aggregated minimum temperature from 4 reading into one (so one record every 20 minutes).

We do further aggregations for temperature records that are not 'in breach' (every 8 hours take max and min temperature log).

There is another table, sensorLogItemLineJoin, which is a join of each itemLines temperature expose

We don't currently clean up temperature logs (sensorLogs).

We only have two VVM statuses (fail pass, ass oppose to 4 stages of VVM)

VVM Status changes are recorded via transactions.

Suggestions

Prerequisite -> How does UI performs when displaying the same graph but data is aggregated on the go ? (instead of aggregating daily max mins from 8 hour intervals, aggregating a whole month worth of 30mins intervals)

UI Changes

TODO, below is just summary write it out, and who are the stakeholders here, should we ask the actual users ?

Notes:

josh-griffin commented 4 years ago

First aggregation should be every 30 mins ?

Yup. Shouldn't change to much with this alone so can lock it in I reckon.

Have two tables (rather then status) for sensorLogs table ?

Don't think we talked about this but I think it's a good idea. What would you call them, though? 🤣

Have historic VVM status item batch table (duplicate record if batch is split)

Definitely. First thoughts:

image

Have historic location item batch table (duplicate records if batch is split)

Definitely. First thoughts:

image

Above table makes sensorLogItemLineJoin obsolete Wipe temperature log data Renaming tables -> sensorLogs to temperatureLogs ?

Makes sense to me

Bonus:

josh-griffin commented 4 years ago

UI

Vaccine specific stock take (which is the manage stock page in vaccine module, but 'batch grained')

So just making just the one page with rows as per batch. Don't show vaccines in normal stocktake page (when vaccine module enabled)

Breach modal, do we really need affected batches there ?

nope! 🤣

VVM stages, can you choose from list of VVM stages as per the VVM ?

Will be wicked - I don't wanna make svg's though 🤣

Do we want buttons in Vaccine Module that do the same thing as in navigator ?

I don't!

BONUS:

josh-griffin commented 4 years ago

Actions that I can see to take from here:

I think thats it?

I think if there are decisions we are not 100% on, we can make the decision now by taking the path of the least work? i.e. breach modal and item batches -> no batches (less work)?

andreievg commented 4 years ago

Thanks Josh, that list of actionables look pretty complete, we can't forget about the desktop Tupaia push though (i guess that's under Determine if any desktop decisions have been made), it would be good to add that to a list of today's discussions.

josh-griffin commented 4 years ago

UPDATE:

Decisions made:

Backend

UI

Actionables

josh-griffin commented 4 years ago

Schema candidates:

TemperatureLog: image

BreachLog:

image

RawSensorLog:

image

VVM Status: image

Location log: image

josh-griffin commented 4 years ago

Options for calculating the exact quantity of a batch at the time of a temperature log:

EDIT: Unless we have aggregation - I believe we need to do a reverse ledger. The inTime and outTime will help with the query for where exactly a batch was:

Without:

With:

josh-griffin commented 4 years ago

Have done a bit of a brain dump here: https://drive.google.com/file/d/1AGvM3oC1KcOf5ko4U94iDVBz1jZ4tCWo/view?usp=sharing

Will clean it up and put it in a doc

Some more questions:

josh-griffin commented 4 years ago
image image

or with X = 30

image

X = 50

image

X = 50 is about where it starts to slow down quite a bit. Wouldn't really want to go past about X = 75.

josh-griffin commented 4 years ago

@andreievg

Regarding the current plan with locations and vvm status': (Just using VVM status as an example, although it applies similarly for Locations)

So the question: What do you store against the TransactionBatch. A VaccineVialMonitorStatus? If you do, you need a TransactionBatchID and StocktakeBatchID on the VaccineVialMonitorStatus schema. Just create a VaccineVialMonitorStatus for the underlying ItemBatch even before finalizing? Doesn't seem quite right.. Maybe a VVMStatus string field on StocktakeBatch and TransactionBatch? Seems pretty hacky? Could have an additional table.. which stores a VVM Status (i.e. would have 4 system data records for level 1, 2, 3, 4. Then the TransactionBatch is related to that record, and when finalized you can create the log record safely. Essentially this is what would happen with Locations.

Example:

image

Unfortunately this is slightly more complicated than initially discussed, but I think it's a pretty robust and flexible design!

andreievg commented 4 years ago

@joshxg, would status on VaccineVialMontiorStatusLog would also solve the problem ?

josh-griffin commented 4 years ago

@andreievg I dont see how, sorry?

andreievg commented 4 years ago

@joshxg, very true, I should have explained better. Was kind of meaning put some sort of status on VaccineVialMonitorStatusLog to know it's 'active' or 'finalised', (not sure if we need the intermediary table in that case). But while thinking on that front, the itemBathId could be null until transaction/stocktakeBatch is finalised.

Something like this:

Screen Shot 2020-03-23 at 4 07 43 PM

What do you think ? (reduces redundancy ?)

josh-griffin commented 4 years ago

I think this is described in my comment above but you still need a StocktakeBatchID and TransactionBatchID FK's on VaccineVialMonitorStatusLog (I mean sure you can have them without but that is starting to get quite hacky) then you don't really need the level.

But IMO this is a lot of messing around to avoid adding a table. It's very similar to aggregation field on original SensorLog imo.

I think it's more complicated and doesn't reduce redundancy at all - actually with the two extra ID fields increases redundancy. I think on the surface it seems less complicated because there is one less table, but I think it's more complicated because it's not very intuitive and is fighting against a relational data model in a relation database?

andreievg commented 4 years ago

Hmm, now i am confused. Why do you need StocktakeBatchID and TransactionBatchID, if StocktakeBatch and TransactionBatch related to VVMStatusLog via vaccineVialMonitorStatusLog (would be vaccineVialMonitorStatusLogID on server) ? I am not seeing the difference between relating Stocktake/TransactionBatch to VaccineVialMonitorStatus vs VaccineVialMonitorStatusLog

josh-griffin commented 4 years ago

ohh right so the itemBatch is null, until after the status changes then the itemBatch gets assigned. I thought it would be assigned to the transactionBatch or StocktakeBatch and then a new one for the ItemBatch .. so I thought it would need to be assigned to at least one thing.

Ok, so yeah I suppose you don't need a TransactionBatchID or StocktakeBatchID- I still think this is quite a lot more complicated then my solution (I think evidence is us both getting confused!) Once we start getting into this status fields and assigning nulls etc it really doesn't become simpler - if that's the goal of the alternative then I don't think it's worth it?

andreievg commented 4 years ago

I actually misinterpreted your original solution, was just writing to comment further and looked at it again, yeah your solution sounds like a good way to do it.

josh-griffin commented 4 years ago

@andreievg

Couple of things for clarification

Let me know what you think with the following:

  1. Location_type [I think you've expressed concerns about this]
    • Location_type.config field to store { maxBreach: { minTemp, duration, maxTemp }, minBreach: {minTemp, duration, maxTemp } }
    • This record created in upgrade code on the server
    • We might need to add some interface to edit the temperatures and timing
    • Should use a type vaccine_fridge or something?
  2. Item
    • use isVaccine field for vaccine items (upgrade code to set this??)
andreievg commented 4 years ago

For identification of breach I am still not 100% sure when to use object fields vs tables tbh, saying that though it's a lot quicker to just add a config field. Either way i think instead of maxBreach and minBreach it should just be an array of fromTemp, toTemp (or minTemp and maxTemp) <- the reason being we can be flexible if another 'breach' criteria is added (i.e. temp goes way way high or low, duration can be minimal)

So two options I think:

Kind of leaning toward structured approach now (with tables)

Location_type I think I was concerned because l was still thinking about prototype. There was no way to select a location type when fridge was created. And I think there was a search on locations where location type has description with fridge in it (and only those locations are shown as options for vaccine item).

I guess the issue for me is how do we link items with suitable locations. There is a mechanism for this in Desktop right now, but that uses item_store_join.restricted_location_type_id, so needs to be configured per store per item, not ideal in my view, would rather add restrictedLocationTypeID to actual 'item' table ? (would require assigning item a location type after desktop is upgraded, and resyncing items with location type to mobile when mobile is upgraded)

If we add locationType for a fridge in migration code, then when 'isVaccine' is chosen for a vaccine, we can make sure it is auto assigned a locationType

Btw I see no use cases where item_store_join.restricted_location_type_id needs to be used for item (wonder why it's done that way)

Item is Vaccine Sound like isVaccine is a good option, not sure if we can do migration for this, i think it would need to be manually configured by user and of course when mobile is upgraded make sure to resync items with isVaccine = true.

josh-griffin commented 4 years ago

For identification of breach

oo I like the table for breach identification.

The only thing I am thinking of where there might be something to solve is for example determining a hot/cold breach to be able to colour the line on the breach red or blue? The only solution here I think is to hard code a temperature value .. lets say.. 5 - and then if the temperature is under 5, use a blue line, otherwise red.

We couldn't use a code or something as I guess these are user-defined on the server, so we don't know the code? Oooor, we can do some hardcoding and say that a breach config must be a 'hot' or 'cold' breach, maybe allow each config to choose a colour..? [Brings up a very interesting discussion about having tables/records be 'user-defined', but having hard-coded records .. i.e. invad [name]]

Location_type

Before I respond to this I think I need some more info on what you mean by "I guess the issue for me is how do we link items with suitable locations. "

Item is Vaccine

Cool - my reservation with this is just 'when does it stop' .. i.e. do we have isPill, isLiquid, isMaterial, isPlastic .. etc (maybe these are bad examples but hopefully you understand!) .. so I don't know when to 'stop'.. but I think the only way to stop this is to have a itemCategoryZZZ field (another one) which is hardcoded itemCategoryZZZ records..?

andreievg commented 4 years ago

Wow color in the breachConfig ! That's a great idea, I was just thinking we use the Temperature_min and Temperature on location_type to figure it out but I am pretty sure it would work well if we add color to breachConfig. (btw breachConfig can be assigned to the 'breach').

Brings up a very interesting discussion about having tables/records be 'user-defined', but having hard-coded records

Yeah, although in this case it's maybe 'pre-defined' rather then hard-coded ?

Cool - my reservation with this is just 'when does it stop' .. i.e. do we have isPill, isLiquid, isMaterial, isPlastic .. etc (maybe these are bad examples but hopefully you understand!) .. so I don't know when to 'stop'.. but I think the only way to stop this is to have a itemCategoryZZZ field (another one) which is hardcoded itemCategoryZZZ records..?

I guess what we are trying to do is add a behaviour to this particular item. Awhile ago I as thinking of a structure where most of those properties are controlled by a 'tag' and there would be user defined 'tags' and hard coded tags that alter app behaviour. You could also have 'tag' category, where only one tag out of a group that can be attached to an entity (i.e. unit/or form) (but i think that's getting off topic, isVaccine should be sufficient for now)

Before I respond to this I think I need some more info on what you mean by "I guess the issue for me is how do we link items with suitable locations. "

So, a where house can have multiple locations and multiple location types, i.e. in Vanuatu there are currently two location_types ('safe' and 'fridge'). Consider a small warehouse with multiple fridges and a safe and a shelves. We have 'vaccines' that go into fridges only, we have things like 'morphine' that needs to be kept in a safe, and everything else can go in other locations. Usually locations are used to identify where 'things' are in on a rack, so that the picker knows where to look for things or where to place things. So potentially there could be quite a few locations of different 'location_types', so how do we know what list to have in UI when choosing location for a batch ? (in mobile VM prototype we currently look for location with locationType.description contains fridge, that's pretty hacky, so thought if we could associated item with a locationType, then it becomes pretty easy and structure

josh-griffin commented 4 years ago

ok sweet!

what list to have in UI when choosing location for a batch

ahhh ok - I'm a bit slow but I understand now!

Sounds like another need for a bulk config setting interface :trollface: Where are most of the fields for item_store_join even edited? I like the default_location_id field.. that could be super nice.

I think in general it might actually be a bit better to set a lot of details on a per-store basis.. except.. sync and the amount of records it creates.. :( The bright side being (hopefully, I haven't checked) that item_store_join surely only syncs to the sites where the store is? Buut.. on second thought - I suppose item_store_join is store data? so it shouldn't be edited on the server? I was thinking you could just have an object in desktop in the [item]input form and that would update all the item_store_join records.. then you can edit them individually on a store/satellite, if you need. That would give the greatest flexibility, but also breaks 'sync rules' :(

I think it's a really good idea.. should probably try to use the item_store_join way since its there already? Maybe a default_location_type on item could help?

andreievg commented 4 years ago

No sync rules broken btw, even though item_store_join is store specific, they are like preferences, controlled by main server.

So the confusion for me is, item is editable on server, but some things in the item input are actually for currently logged in store:

check out bottom of the screenshot, this is how you set item_store_join.restricted_location_type_id in desktop:

Screen Shot 2020-03-25 at 5 31 24 PM

Sounds like another need for a bulk config setting interface

You are on point, not slow at all.

was thinking you could just have an object in desktop in the [item]input form and that would update all

Great, so if you look at the screenshot, if Restricted to is changed to Restricted to in Store {current store name}: and then add another one Restricted to in All Stores:

Maybe a default_location_type on item could help?

Perhaps should be named default_restricted_location_type ? That would be that one Restricted to in All Stores: ?

If we want to overwrite the default one, then we can set Restricted to in Store.. in item.input, which will set it on item_store_join.

And in UI, when we select location, we first look at item_store_join, then item ?

There is actually very limited places where this is used in mSupply desktop, so easy to change.

josh-griffin commented 4 years ago

All sounds really good to me!

And in UI, when we select location, we first look at item_store_join, then item ?

Just this one: Would it be easier that when you change Restricted to in all stores - that this updates all [item_store_join] for the item? Then, when you make a new item_store_join when you add visibility, the default is to have the item.default_restricted_location_type be set to the new item_store_join?

So I am having trouble communicating the difference so hopefully this all makes sense. But basically from what I understand your suggesting:

I think making it so

This is because I think the use cases are

  1. Change the location type for every store
  2. Change the location type for a single store

For 1 - if you have some stores who have set a location type themselves, then to change it for everyone you have to change it individually for every store if you don't change the item_store_join when you change the all stores location type. For 2 - If you change it for everyone, it means everyone - so to change it again for individual stores you have to go back and change it again.

Hopefully that makes sense? Also there is the added benefit of only ever checking item_store_join, rather than item_store_join then item

andreievg commented 4 years ago

I guess the question is:

Yes -> Then have to do it the way i've described No -> Don't technically need a field on item or another drop down, just "Apply location restriction to all stores" button/checkbox ?

Or we can combine both methods, in that case, two drop downs and a checkbox. What you reckon ?

josh-griffin commented 4 years ago

"Do we want to persist individually configured item_store_join settings when changing the global item one ?"

ugh that's the sentence I was trying to write in my paragraphs 🤣

I think your idea for the checkbox is nice - or a pref? Or just a dialog when changing in the interface..? "Do you want to apply this change to all stores?" Then we can still just use the item_store_join

andreievg commented 4 years ago

I guess the simplest would be a dialog, when value in list box changes.

josh-griffin commented 4 years ago

@andreievg

Just been thinking about doses column again.. we said don’t have it adjust quantity and quantity not to adjust doses or enforce limits etc other than what we have now. I'm thinking if we don't, we will need to show an amount of doses column, won't we?

andreievg commented 4 years ago

@joshxg, how hard would it be to add an on focus change event (on top of on data change)?

andreievg commented 4 years ago

Actually what I was thinking about won't help, am i correct to say that on focus change fires after on button click (if say you change quantity and without exiting the cell press finalise) ?

josh-griffin commented 4 years ago

@andreievg

yup it'll be the same behaviour as what it used to be - button click before on losing focus. I think having two columns which are 'dependent' or 'coupled' is really annoying.. same thing for reasons.. and even the stocktake item/batch quantity columns .. it's not the greatest user experience :(

Trying to think a little outside the box on this one.. the 'easy' solutions are for a 'doses' column to show the item.doses field, or to 'couple' the columns... What about if the quantity column is actually for doses for a vaccine item? So you enter number of doses and it sets doses, but also sets numberOfPacks equal to the minimum number of packs...? For example, 2 doses per vial.. enter 17.. doses is set to 17 and numberOfPacks set to 9.. but only display 17.. so just one column?

Or.. the little bottom modal popup thing that we use on the current stock page (and soon to be requisition page), could show the item.doses rather than showing a column? Or, it could show numberOfPacks for a vaccine item if the normal column was actually for doses for a vaccine item...?

andreievg commented 4 years ago

hmm, I like where you going but the main idea behind doses during issue was to be able to figure out 'open vial wastage', or how many doses were actually used for a vial. I remember our main issue was with minimum doses = numberOfPacks (say 10 doses per quantity), so when you enter quantity = 10, and then enter like 100 doses, but want to change to say 19 doses, it goes all haywire (when zero is removed, it would go to 1, which would be auto set to 10, which is the minimum number of doses for quantity 10).

Maybe we don't worry about minimum doses, just restrict by max ? So then the idea would be: when quantities are changed, change doses to dose per vial * quantity, when doses are changed, restrict max allowed doses per quantity, but don't worry about minimum ?

Most vaccines btw are 1 vial = 1 dose, there are a couple with more then 1, say BCG = 10 doses per vial.

I think having two columns which are 'dependent' or 'coupled' is really annoying

I agree, what you reckon, in ideal world when one tries to finalise and things are missing or incorrect, each 'wrong' thing is highlighted, with maybe exclamation mark you can press (when you press it it tells you what is wrong) ?

josh-griffin commented 4 years ago

Maybe we don't worry about minimum doses, just restrict by max ?

Yeah that'll work too, nice.

I agree, what you reckon, in ideal world when one tries to finalise and things are missing or incorrect, each 'wrong' thing is highlighted, with maybe exclamation mark you can press (when you press it it tells you what is wrong) ?

even showing the indication that something is wrong after losing focus is good, I think!

josh-griffin commented 4 years ago

@andreievg

Do you think auto-applying a vvm status is a good idea? We have added a level field - so maybe applying the lowest level vvm status after batches are synced for a SI, and when adding a StocktakeBatch/TransactionBatch, auto-apply it there?

andreievg commented 4 years ago

@joshxg, sounds good to me.

wlthomson commented 4 years ago

Update:

@joshxg off for the remainder of this week and part-time for a bit, so will go ahead and pick up the remaining mobile issues.

@Chris-Petty going to take care of the main desktop UI: https://github.com/sussol/msupply/issues/6089, https://github.com/sussol/msupply/issues/6116.

To ensure sufficient time and to ensure compatibility, all vaccines work is targetted for desktop v12, as the PR deadline for v11 is too close. This gives plenty of time for development + testing + documentation.

josh-griffin commented 3 years ago

I'm so sick of this issue - I'm gonna close and use #3161 for new additions..!