msupply-foundation / mobile

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

Stocktake, adjusts based on snapshot quantity difference not on real quantity difference #638

Closed andreievg closed 6 years ago

andreievg commented 6 years ago

Stock-takes by their nature need to be done synchronously to other actions like receiving/issuing stock. This is explained in training but is not restricted in the app. Sound like we need to handle the cases where users issues/receives stock while stocktake is in progress, to minimise a chance of ledger issues. Currently when stocktake is finalise we create an inventory adjustment based on difference between snapshot quantity, not actual quantity. This can result in ledger discrepancy. i.e.

Item A -> Starting Quantity: 1000

So our actual quantity = 900 but our ledger reads: Starting Quantity = 1000 Transaction Line A -100 Transaction Line B -100 Expected Quantity = 800

Even though Quantity is correct, ledger is not.

This is how this example looks in mSupply Desktop Item->Stock->Ledger:

screen shot 2018-03-26 at 4 42 40 pm

Prior to https://github.com/sussol/mobile/pull/634 with some trickery it was possible to finalise an a stocktake that show a negative ledger.

Solution:

@GaryWilletts @ujwalSussol @Chris-Petty @craigdrown, which one do you prefer ?

andreievg commented 6 years ago

A correction (while talking to @ujwalSussol), he explained, and I tested. mSupply Desktop adjusts stocktake based on 'difference' between snapshot and counted quantity. (and as explained above mobile sets real quantity = counted quantity). Scenario:

This is makes sense if stocktake counted/actual quantity is entered before the customer invoice (i.e. most places when doing stocktake, a printout is given to the store-man, who does a 'snapshot' stocktake, and data entry may 'lag' behind stocktake's finalisation).

Makes a little less sense when we assume that at the time of finalisation, counted quantity is the actual, real quantity of the stock. (which maybe what happens in mobile ?)

I am happy with replicating mSupply logic of adjustments.

ujwalSussol commented 6 years ago

Replicating mSupply desktop logic is probably the best approach. This way our documentation will be the same….

On 26 Mar 2018, at 18:07, Andrei notifications@github.com wrote:

A correction (while talking to @ujwalSussol https://github.com/ujwalSussol), he explained, and I tested. mSupply Desktop adjusts stocktake based on 'difference' between snapshot and counted quantity. (and as explained above mobile sets real quantity = counted quantity). Scenario:

Stock adjusted in stocktake, -100 Customer invoice is created, -100 Stocktake is finalised, total reduction of real quantity = -200 This is makes sense if stocktake counted/actual quantity is entered before the customer invoice (i.e. most places when doing stocktake, a printout is given to the store-man, who does a 'snapshot' stocktake, and data entry may 'lag' behind stocktake's finalisation).

Makes a little less sense when we assume that at the time of finalisation, counted quantity is the actual, real quantity of the stock. (which maybe what happens in mobile ?)

I am happy with replicating mSupply logic of adjustments.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sussol/mobile/issues/638#issuecomment-376147100, or mute the thread https://github.com/notifications/unsubscribe-auth/AGc26iGXIDFdLQYW7QLXPneB_fcghbRKks5tiN2fgaJpZM4S67xZ.

Chris-Petty commented 6 years ago

It's a notorious problem with stocktake. When using paper you have to make sure to include anything that has been picked for outgoing in your counted quantity (if picked before you counted).

I think places get around this by doing a massive full stocktake outside of normal work hours (at least in my experience).

Applying the difference from counted - actual stock on hand isn't any good either, the ledger problems are just positive values rather than negative.

Updating snapshot quantities to real quantity suffers the same issue.

For the record, mobile does apply the difference as you describe desktop does. Desktop even throws the same sort of error if you try to make reducing inventory adjustments when a customer invoice has reduced stock to 0 already. Problem with mobile was that you could wiggle around it by abusing a bug. image

The code in mobile is interesting though. StocktakeBatch.js

  finalise(database) {
    const isAddition = this.countedTotalQuantity > this.snapshotTotalQuantity;
    const inventoryAdjustment = isAddition ? this.stocktake.getAdditions(database)
                                            : this.stocktake.getReductions(database);
    // Adjust inventory
    this.itemBatch.batch = this.batch;
    // Review: The item batch is edited directly, changing the number of packs
    this.itemBatch.numberOfPacks = this.countedNumberOfPacks;
    this.itemBatch.expiryDate = this.expiryDate;
    // Create TransactionItem, TransactionBatch to store inventory adjustment in this Stocktake
    const item = this.itemBatch.item;
    const transactionItem = createRecord(database, 'TransactionItem', inventoryAdjustment, item);
    const transactionBatch = createRecord(database, 'TransactionBatch',
                                          transactionItem, this.itemBatch);
    // Review: The corresponding transaction batch is made using difference, but the
    // transaction (inventory adjustment) is created with confirmed state. This is means
    // that the difference isn't applied to stock, that was directly done above. I am not
    // sure why it's done this way. This is why ledger gets a negative value but 
    // mobile sets it's stock to the counted quantity
    transactionBatch.numberOfPacks = Math.abs(this.snapshotTotalQuantity
                                              - this.countedTotalQuantity);

    database.save('ItemBatch', this.itemBatch);
    database.save('TransactionBatch', transactionBatch);
  }

If you haven't, read the "Review:" comments I made in the code above. Desktop does appear to do it similarly. The inventory adjustments are never moved to status "finalised". They can be deleted separately from the stocktake which seems really naff to me. Why not have the inventory adjustment as "suggested" when starting finalisation of the stocktake and once the stocktake finalising code has completed populating the adjustment, confirm or finalise it to apply adjustments? That way in mobile we can stop editing the stock value directly (setting it to counted). At least then we'll actually show negative stock when when a bug allowed it!

As implied that doesn't actually fix the fact that the applied difference could give a negative stock value (if you wiggle round the check for changes into negative stock like you could in mobile).

For the current logic the only half decent way I can think is for customer invoices, supplier invoices and inventory adjustments to adjust snapshot quantity (making it no longer a snapshot...) or adjust the counted quantity of unfinalised stocktakes. A customer invoice would have to decrease the former or increase the latter. Old unfinalised stocktakes will still become a full blown mess at any rate. The meaning of counted quantity vs snapshot quantity for a given start date is lost, but for the final date is correct. That said, you can add items after you created the stocktake in mobile, so it already wasn't solid for the start date.

Chris-Petty commented 6 years ago

@GaryWilletts I detect has 2 cents given a comment on cerb.

@sussol/msupplyadmin in general probably have some wisdom on the matter.

GaryWilletts commented 6 years ago

Aye, I do! Changing the snapshot is a bad idea. The assumed way of working in mSupply desktop is that you stop all normal warehouse activity after the snapshot is taken, make the physical count, finalise the stocktake then resume normal warehouse activities. On average, this will be easier in mobile stores because they're smaller. But whichever way you work it, you have to assume the operators are behaving one way or another, you can't read their minds to know what's been counted. So best to keep it simple and prescribe what most people expect and understand - no transactions while the stock is being counted after the snapshot is taken.

andreievg commented 6 years ago

@Chris-Petty I think I confused you re how mSupply desktop applies the adjustments, it doesn't actually look at the counted quantity vs actual quantity, it looks at counted quantity vs snapshot quantity and tried to apply the difference to actual quantity.

andreievg commented 6 years ago

I am quite keen to reduce user error / confusion that results from stocktake (not by telling user not to do something, but by removing the possibility of that something being done).

For desktop I can only see one issue, when stocktake is finalised, the counted quantity is not necessary what the quantity will be adjusted to.. This maybe confusing for the user

Mobile, as outlined above will always adjust quantity to be counted quantity on finalisation, but mobile may create inventory adjustments that lead to ledger discrepancies (even thought the counted/actual quantity should stay correct)

Chris-Petty commented 6 years ago

Are you still arguing your case for in your original comment?

In mobile it can only make ledger issues if you abuse that bug we know about right? I agree it'd be better if mobile and desktop did the same by applying difference. Going negative stock should be blocked, but if we do (through a bug that allows it) I think mobile should show it.

Not sure if silly idea: What if stocktake had a "Refresh" button. It'd have a modal that comes up that warns/confirms. It'd look at past transactions since the creation date of the stocktake and adjust snapshot and counted quantities (don't touch "Not counted") by the same amount. Bring the creation date forward. Though if you've left your stocktake floating around like that to begin with, normal practice would be just to delete it as far as I know.

GaryWilletts commented 6 years ago

@Chris-Petty be sure, the idea is silly ;-) I foresee a world of pain if you allow stocktakes to be updated like that. You're right, normal procedure is to delete the stocktake and start again.

@andreievg when you say:

For desktop I can only see one issue, when stocktake is finalised, the counted quantity is not necessary what the quantity will be adjusted to.. This maybe confusing for the user

under what circumstances will the quantity the stock has been adjusted to not match the counted quantity?

andreievg commented 6 years ago

@GaryWilletts If there is a reduction or addition transactions while the stocktake is in progress, when the stocktake is 'finalised' -> adjusted quantity != counted quantity. (for an example please read the first note of this issue)

@Chris-Petty I like the idea of refresh button, I think the solution we discussed yesterday is similar 'auto-refresh'.

Anytime a stocktake is opened, we check snapshot quantity vs actual quantity for every stocktake line, if snapshot quantity is different, then we readjust it and set counted quantity field to 'not counted'. Also a modal will pop up, listing items which have been 'refreshed', maybe even a message warning that while stocktake is in progress, one should not make/edit transactions with items in the stocktake.

Benefits:

Chris-Petty commented 6 years ago

Ok I've been testing in mobile and have come to some conclusions. Little bit weird.

Just to be clear "Stock on Hand"/"Actual Stock" will be referred to as SOH.

Case one Make a stocktake for itemA (increasing) and then customer invoice

  1. Stocktake for itemA, snapshot 1000, counted 1500, diff +500
  2. Issue 500 in customer invoice
  3. finalise stocktake Once synced, desktop reconciles to say 1000 on ledger (definitely the correct value), SOH is 1500 (uh oh). image

Case two - Make a stocktake for itemA (decreasing) and then customer invoice (this is the one Andrei originally described)

  1. Stocktake for itemA, snapshot 1500, counted 1000, diff -500
  2. Issue 500 in customer invoice
  3. finalise stocktake Once synced, desktop reconciles to say 500 on ledger (definitely the correct value), SOH is 1000 (uh oh). image

Case three - Make a stocktake for itemA and then supplier invoice Short story is this works fine. No ledger problems. The first 3 transaction in above picture resulted correctly in 1500. Doesn't matter if the stocktake increased or reduced stock for itemA, works fine.

Mobile is doing a bad between customer invoice and stocktake. Pending me investigating exactly what happens on the mobile side: It should match the ledger by applying the difference (like desktop). This is ideal because it means previous counts in a stocktake are mostly fine except the one exception... If the difference applied to SOH will result in negative ledger, it currently just blocks the user on finalising. It's a major headache to correct these. Throwing out a whole stocktake to correct a few lines would be a waste. We discussed yesterday either blocking making new customer invoices all together while there is an unfinalised stocktake, but that's gross. "Refreshing" or resetting lines which snapshot no longer matches SOH means that this is more spread out, but unnecessary if we correctly apply the difference. TL;DR Resetting lines that aren't counted yet and the lines that will have a difference applied that goes to negative stock will conserve the most amount of work, and keep the ledger and SOH as correct as possible. @andreievg did I miss something

@GaryWilletts, @craigdrown @adrianwb didn't complain when we talked about refreshing/resetting lines yesterday. I will note that mobile does give us a bit of flexibility to do this because there is only ever one point of data entry for , the tablet. This wouldn't work with desktop due to the multi client environment and tendency to print out stocktake sheets.

Actions

  1. I'm going to poke around customer invoices and stocktake. Probably make stocktake correctly apply difference. Will have to test that supplier invoices keep playing ball.
  2. When opening a unfinalised stocktake, quickly check all lines won't cause negative stock. If they do, reset them and inform user. Update snapshot for all uncounted lines. (Will do this one when all debate is settled)
GaryWilletts commented 6 years ago

@andreievg thanks, just checking I was understanding what you were saying - I was :-)

@Chris-Petty I'm coming round to the idea of refreshing the snapshot in mobile but I'm still not convinced. Let's see what your investigations show. Someone has to stand up for tradition ;-)

andreievg commented 6 years ago

Sorry to be adding comments to this discussion this late. But while doing a review of https://github.com/sussol/mobile/pull/655/ I've realised that our discussion didn't fully end, and we still have to agree on something.

My suggestion was to reset snapshot and counted quantity (without an option, but with a message) for all items where snapshot quantity != actual quantity (current available quantity), and do this every time a stock take is opened, with a message saying something along those lines: Snapshot quantities for these items have changed (due to stock issued or received while this stocktake is in progress), both Counted Quantity and Snapshot quantity will be reset: Item A, Item B, Item C..

This way we are completely locking the user out of making 'error' or something they will perceive as errors. There is a scenario where there might be a question, will give an example at the very bottom of this comment.

@Chris-Petty, I am happy with replicating the adjustment logic as per mSupply. But I am still worried about these scenarios:

Would it not be better if the user does not ask us questions, and or thinks that our system is erroneously adjusting the stock ?

There is a potential confusion for the user as per my suggestion. They can ask a question

I am still holding strong to resetting all snapshot quantities / counted quantities. Thus all the scenarios. I think the question for us is, what do we think is right ? And what customer thinks is right ? I think with the solution I am suggestion, the confusion between the two is minimised.

Chris-Petty commented 6 years ago

Your first 2 examples assume that they press the cancel button, not heeding the warning given to them. The blunt way to resolve that would be to not allow it to be cancelled. It's up to us if we want the new functionality/behaviour as optional. Counted items that are fine have their work preserved but the risk of error on other lines is resolved.

If we do not adjust items with already counted quantity...

Your suggestion of resetting any lines of snapshot quantity != stock on hand does mitigate this, but I argue that slightly better training on stocktake would enable them to do less rework (only recounting lines that would apply into negative stock). Common practice is rolling stocktake or full stocktake, diverging from that is training anyway. That said, mobile docs are suuuuper basic - assuming user has knowledge of stocktaking kinda basic.

Just a note: The current implementation in #655 will prompt every time they look at the stocktake until it's finalised. Could also prompt on editing every line (don't think it'll be much overhead).

andreievg commented 6 years ago

@Chris-Petty

I argue that slightly better training on stocktake would enable them to do less rework (only recounting lines that would apply into negative stock). Common practice is rolling stocktake or full stocktake, diverging from that is training anyway. That said, mobile docs are suuuuper basic - assuming user has knowledge of stocktaking kinda basic.

So far training has not helped (I'll telegram you examples shortly), imagine 100+ mobile sites, doing stocktake every month, with possibility of erroneous adjustments. That's a lot of support emails, we can solve this issue for mobile once and for all by reseting as i've suggested, why not do it ? By the way with this suggestion the app teaches the user, if they have done something they shouldn't have (stock movements) while a stocktake is in progress, they will need to recount those items, now they have an incentive, not to issue/receive stock when stocktake is in progress. Mobile sites have small warehouses, and recounting a couple of items is not a big task.

Why allow a possibility of erroneous adjustments ?

Just a note: The current implementation in #655 will prompt every time they look at the stocktake until it's finalised. Could also prompt on editing every line (don't think it'll be much overhead).

With no cancel button and resetting snapshot and counted quantity for all items that had stock movements since stocktake started, we don't need to prompt on every line edit.

If we are in disagreement here still by the end of the day, we should ask for some mSupply guru opinions. (it's too much of a blocker atm)

Chris-Petty commented 6 years ago

I agree with making it non optional. I can get on board with resetting all stocktake lines that have had movement (i.e. including those that wouldn't result in negative stock). It should lead to less support for us and possibly a little more rework for them (though they should learn to avoid doing invoices while a stocktake is in progress as you've reiterated).

@sussol/managers can interject, but I'll move forward with that until then.

GaryWilletts commented 6 years ago

The same issue applies to mSupply desktop and we should keep the two consistent. Whichever way it's done, the user has to know because it makes a difference to what they count in a stocktake.

Other options are to disallow confirming an invoice with an item included in an unfinalised stocktake or to warn the user if they confirm an invoice with an item in an unfinalised stocktake.

@craigdrown @craig you have an opinion?

Chris-Petty commented 6 years ago

Mobile customer invoices are confirmed by default, so that's a bit of a paradigm shift on that platform @GaryWilletts . An equivalent would be not allowing confirmed invoices to be edited, though.

GaryWilletts commented 6 years ago

Ah, I'd forgotten that. We just need to make a decision about the way to go and it's not entirely clear which way is best, hence our long discussions. Personally I'd rather prevent someone editing a confirmed customer invoice in mobile if there's an unfinalised stocktake with the edited item on it (or putting the item on a new customer invoice if it's in an unfinalised stocktake). It prevents stock issues and trains the user to use correct stocktaking practice at the same time. Is the check expensive in mobile?

andreievg commented 6 years ago

@GaryWilletts, this type of check is expensive in terms of keeping the code clean. If we do this type of check then it will have to be for Custom Invoices, Supplier Invoices, Customer Requisition (as it generates CI automatically) and for Other Stocktakes.

Can you please describe which part of the described solution you don't like or disagree with. Described solution being reseting the items in stocktake that have changed quantity (stock issued or received while stocktake is in progress)

Please keep in mind that this solution is aimed at mobile not desktop, where the scale of everything is smaller.

GaryWilletts commented 6 years ago

I'm not in disagreement, just trying to think round the wider issues. It's a pretty radical change and mobile and desktop will be working differently - important we do it the right way.

The expensive argument doesn't work I think because your proposed change is more expensive in all ways - you have to have the same checking code as if you were warning/stopping the user but you additionally have to check and update all suggested stocktakes.

I'm OK with updating the snapshot, just making sure it's properly thought through. Think of me as the grit that leads to the pearl ;-)

ujwalSussol commented 6 years ago

Current mSupply desktop logic :

example 1 :

Snapshot = 600 stock take = 250 [ diff 600-250 = 350 ]

user issues = 100 So actual stock is : 600 - 100 = 500

Now stock take is finalised : 500 [current stock level] - 350 [stocktake difference] = 150

example 2:

Snapshot = 600 stock take = 250 [ diff 600-250 = 350 ]

user issues = 400 So actual stock is : 600 - 400 = 200

Now stock take is finalised : 200 [current stock level] - 350 [stocktake difference] = -150 Here the calculation shows that the stock will go negative if allowed... so mSupply disallows...

Instead it used to do : 200 [current stock level] - 200 [stocktake difference] = 0 .... Now we show the excel sheet which has created complication to the user.... Previously the inventory adjustment would be adjusted to bring the stock to zero.... If absolutely no stock was available, mSupply even adjusted the stock take to zero. This was before the excel option..... Now we are having to bring back this feature as our users can't handle the excel sheet.

andreievg commented 6 years ago

@GaryWilletts

The expensive argument doesn't work I think because your proposed change is more expensive in all ways - you have to have the same checking code as if you were warning/stopping the user but you additionally have to check and update all suggested stocktakes.

In mobile there is only ever a single page interface (no multi window) so we can quite easily adjust snapshot quantity for items where snapshot quantity differs from available quantity. This is done when stocktake is opened, so not expensive at all

I'm not in disagreement, just trying to think round the wider issues. It's a pretty radical change and mobile and desktop will be working differently - important we do it the right way.

Yeah, there will be differences in how mobile and desktop handles stock-takes. Mobile already differs in a few ways from desktop, I think the differences were driven by trying to simplify Mobiles interface and operations. I believe this new change to stocktake reset is the simplest way to avoid user error, user confusion (leading to support tickets).

On a side note, it look like one good difference, the requisition modal, is being applied to desktop now.

andreievg commented 6 years ago

@ujwalSussol @GaryWilletts @craigdrown For mSupply Desktop, in my view the ideal solution will be to change our stocktake table + stock take line edit.

For stocktake table, change Enter Quantity to Snapshot Counted Quantity and add Finalised Quantity (the quantity stock level will be adjusted to after stocktake is finalised). Make the row red if Finalised Quantity is below zero.

And when stocktake row is double clicked, can add extra information. Finalised Quantity, Current Quantity (current actual or available quantity of the item, may be different to snapshot quantity) and Adjustment

We can even include a little formula here (maybe our interface can be in the shape of this formula)

This way the user will know what's actually going on with stocktake adjustments.

Chris-Petty commented 6 years ago

Obligatory "If we had a proper UX team and user testing/feedback system... "

Chris-Petty commented 6 years ago

@ujwalSussol can I ask why there was the move to the excel option to start with?

The approach that you're bringing back is pretty simple but... is it gross?

What I've implemented on mobile

andreievg commented 6 years ago

I've talked to Chris a little bit, it seems Snapshot Counted Quantity is not the best name for a column. I guess Counted Quantity will be sufficient (I was trying to say that 'this column is what you've counted, but may differ to actual quantity at the warehouse, as there could have been some stock movements since you've counted it'). And again there may be better suggestions for Finalised Quantity -> quantity that mSupply will adjust the stock to once inventory adjustment is created.

andreievg commented 6 years ago

I've moved mSupply desktop related discussion to: https://bugs.msupply.org.nz/view.php?id=8811

ujwalSussol commented 6 years ago

@Chris-Petty : I think we had one client that noticed mSupply making stock take adjustment and ledger adjustment due to lack of stock.... So the excel approach was to stop the process and to allow the users to logically do the maths.... As we have noticed lately this was asking too much of the users.....

So the easy option for desktop is to use bring the previous approach back but to have logs mention lack of stock for stock take, so mSupply made stock take and ledger adjustment.

For mobile : What every way you have done is fine for now.... as long as we don't have ledger problems.

Chris-Petty commented 6 years ago

Apparently I sleepily closed this issue and commented a half comment haha