Lifetime calculation in days not months #5226
Conversation
jniles
force-pushed
the
5185-fix-lifetime-calculation-in-days-not-months
branch
from
816aef7 to
8c0367f
Compare
jniles
force-pushed
the
5185-fix-lifetime-calculation-in-days-not-months
branch
2 times, most recently
from
25f1699 to
9185cf3
Compare
|
@mbayopanda I think this is finally ready for a review. Apologies for taking so long. @jmcameron if you want to take a look at this as well, I would appreciate it. It's pretty dense though, so don't worry if you feel out of your depth taking a look. |
There was a problem hiding this comment.
A minor suggestion. We have several all-caps variables (eg, IS_IN_RISK_EXPIRATION, S_RISK, S_LOT_LIFETIME, etc). In general, we usually reserve all-caps-with-underscores for constants or i18n language tags, not variables/flags. I suggest making these lower-case (with underscores or camel case, as appropriate). And maybe choosing more descriptive names as part of the change?
Question: Since the CMM is calculated by consumption over N months, it will not change very fast when stock consumption increases due to some short-term period with high usage. However, the usage timeline depends on CMM and will not properly reflect how the usage pattern may need to change if the anomalous usage pattern continues. We may want to consider how to address that. I can imagine this scenario: There is a flare up of Cholera locally. The hospital dispenses meds for that at a higher than usual rate. The admins start to wonder, when will we run out? If they use the current code (as nice as it is), they may get a rosy/misleading picture. It may be useful to provide a way to do these calculations with a more instantaneous estimate of the CMM to get a more accurate picture. This could be done by providing a way to let the CMM period be temporarily overridden in the query.
| let inventoriesWithLotsProcessed = computeLotIndicators(inventoriesWithManagementData); | ||
|
|
||
| if (_status) { | ||
| inventoriesWithLotsProcessed = inventoriesWithLotsProcessed.filter(row => row.status === _status); | ||
| inventoriesWithLotsProcessed = inventoriesWithLotsProcessed.filter(lot => lot.status === _status); |
There was a problem hiding this comment.
I like these variable name changes. Makes it more readable!
| const inventoryLots = _.groupBy(depotInventories, 'inventory_uuid'); | ||
|
|
||
| _.map(inventoryLots, (lots) => { | ||
| _.forEach(inventoryLots, (lots) => { | ||
|
|
There was a problem hiding this comment.
Typo on line 713/732: hight -> higher
server/controllers/stock/core.js
Outdated
| @@ -714,32 +733,80 @@ function computeLotIndicators(inventories) { | |||
| orderedInventoryLots = _.orderBy(orderedInventoryLots, 'lifetime', 'asc'); | |||
|
|
|||
| // compute the lot coefficient | |||
server/controllers/stock/core.js
Outdated
| // add to the running LotLifetimes so that the next product will be used after it. | ||
| runningLotLifetimes += lot.lifetime_lot; | ||
|
|
||
| // the usuable quantity remaining is the minimum of the stock actually available |
server/controllers/stock/core.js
Outdated
| * Computes the indicators on stock lots. Sets the following flags: | ||
| * 1) expired - if the expiration date is in the past | ||
| * 2) exhausted - if the quantity is 0. | ||
| * 3) near_expiration - if the expiration is before |
There was a problem hiding this comment.
Whoops this also is an incomplete thought.
jniles
force-pushed
the
5185-fix-lifetime-calculation-in-days-not-months
branch
from
5bbb763 to
8724179
Compare
|
@jmcameron thanks for you review.
I agree with this suggestion. I'll let @mbayopanda decide if this should be implemented in this PR or not. I worried that changing too many things at once may make it harder to follow. Happy to do that here or in a subsequent PR that would be more focused on renaming rather than introducing new logic.
I'm a bit torn about this, so I think we need to ask our pharmacist team. I think the point of CMM is to smooth out these peaks/troughs when you do your ordering. The user can always update the "number of months" used in the stock settings, so a smart user could gain this information currently. It might be interesting to have a graduated CMM report with columns for 24 month CMM, 12 month CMM, 6 month CMM, 3 month CMM. That way, you could see the trend line over a large period and judge for yourself when ordering. I don't think the CMM calculation architecture is currently designed for this scenario, but we could always refactor it. |
There was a problem hiding this comment.
@jniles having a lifespan of lots in days is giving our calculation accuracy, most of our flags (expired, risk of expiration, risk of stock out, stock out, etc.) coloration have an obvious understanding now than before.
My concern is about the lot lifespan by lot, by reading the registry we cannot understand (in an obvious way) how this column is calculated; there is no reason to have this column in the registry if the user can't understand how it is calculated.
In the previous implementation, this column had a reason because it was helping us to understand some calculations just by reading the registry.
If our clients need that column we must have that in a way where they understand easily where it comes from and the usage just by reading the registry
If our clients cannot understand that, we must not have this column in the registry.
Or, we can add in the registry other columns for CMM in days and MS in days which can help to understand some calculations made per day; with this option, our registry will become bigger (horizontally) and less comfortable for users. With this option also if the user can't understand the columns displayed there is no reason to have these columns
| lot.exhausted = false; | ||
| lot.expired = false; | ||
| lot.near_expiration = false; | ||
| lot.at_risk = false; |
jniles
force-pushed
the
5185-fix-lifetime-calculation-in-days-not-months
branch
from
8724179 to
accc12a
Compare
|
@jmcameron I've switched the base branch to the Here is what the final version looks like: |
Bumps [delay](https://github.com/sindresorhus/delay) from 4.4.1 to 5.0.0. - [Release notes](https://github.com/sindresorhus/delay/releases) - [Commits](sindresorhus/delay@v4.4.1...v5.0.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [karma](https://github.com/karma-runner/karma) from 6.0.3 to 6.0.4. - [Release notes](https://github.com/karma-runner/karma/releases) - [Changelog](https://github.com/karma-runner/karma/blob/master/CHANGELOG.md) - [Commits](karma-runner/karma@v6.0.3...v6.0.4) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Bumps [angular-ui-grid](https://github.com/angular-ui/ui-grid) from 4.9.1 to 4.10.0. - [Release notes](https://github.com/angular-ui/ui-grid/releases) - [Changelog](https://github.com/angular-ui/ui-grid/blob/master/CHANGELOG.md) - [Commits](angular-ui/ui-grid@v4.9.1...v4.10.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>
Fixes the color scheme of the stock lots registry. Now the expired lots show up as red always, instead of as "at risk of expiration". Closes Third-Culture-Software#5185.
This commit adds all four stock states to the key at the bottom of the registry of the lots in stock. It also adds totals to the registry.
Moves the lot flag calculations to the server to make it cleaner on the client.
Updates the risk calculation to properly compute the risk by quantity and time period. Also cleans up the client so that we render in red the quantities at risk.
Updates the tests for stock now that they take into account the consumption schedule.
Cleans up the comments based on @jmcameron's feedback.
Removes many of the registry columns from the stock lot registry so that they are not as distracting to the user. Closes Third-Culture-Software#5311.
jniles
force-pushed
the
5185-fix-lifetime-calculation-in-days-not-months
branch
from
accc12a to
c1083b8
Compare
|
bors r+ |
|
Build succeeded: |
This pull request attempts to clean up our inventory/lot flag calculations so that we have a uniform way of presenting the stock out, stock expiration, at risk of expiration, and at risk of stock out states across the client.
Here is what it looks like:
We've standardized around four "flags":
I've updated many of the calculations so they take place on the server will commented documentation.
Consumption Schedule
This PR implements what I'll call the "consumption schedule" for lots. Basically, the lifetimes of each lot are calculated as if the enterprise was optimally using the lots within their depot. So, the depot will use the lot that will expire soonest, then the next soonest, etc. This means that a lot may be in stock, but will not be used until much later.
This may produce some misleading figures in the registry, which is why it may be important to show some of these calculations to the client. I'll propose an issue about that.
Bug Fixes
This PR also fixes bugs in the coloration of the registry and adds a final aggregation to the bottom of the registry for all four flags.