Fix errors found in testing #4122
Conversation
|
I don't know if this is specfic to this fix or if it would occur on main, but I was testing a bit using MHM, which appears to have some negative inventory items at this time, and when saving a Donation (of a different item) I got "Error occurred when re-running events: DistributionEvent on 2024-02-20: Could not reduce quantity by 1 - current quantity is 0 for Car Seat - Infant up to 22lbs. w/ handle in Main Warehouse" Sooooo.... we may need to do some data cleanup for this to work? |
|
Sounds right - we might need to identify organizations with negative inventory at the current time and maybe add an adjustment to set it back to zero? This PR tightens up our validations so it makes sense that something that worked before wouldn't now. |
|
Hrrrm. i managed to totally mung the inventory numbers. Magnitude: Took a value that was in the 20000 range down to negatives, and messed up all the numbers. I assumed it was to do with the above error, but no luck with the immediate recreation. I was checking a couple of other things, and going back and forth between flag on and flag off. So this is just a heads up until I recreate. |
|
Regarding the negative inventory -- I'm pretty sure we'll need to remove the old events and start with a fresh snapshot. There are some pretty large differences between the inventory we calculate with events, and what they are currently seeing on this org. |
|
Regarding the exception: I'm not sure adding an inventory adjustment would work. It's failing for something that happened in the past... |
|
Oh -- also on the donation -- I wasn't clear -- the error around the negative inventory is an uncaught error, so they'd get a 500. On the distribution, it is caught and presented as an error. (It would still be mightily confusing, and prevent them doing anything) Edit: purchase is behaving the same as donation. |
|
It does look like this is safe when the read_events flag is disabled though. I have not been able to recreate the problem with the inventory going kerfluey. I was hopping amongst 2 or 3 different branches, maybe that had something to do with it. |
|
I spent awhile testing this with the flag on, and it definitely does what I expect it to. |
There was a problem hiding this comment.
As far as changes - could we make it so that that error regarding the negative inventory is caught on donation and purchase? Please check kit allocation too.
Otherwise, with some trepidation around not being able to recreate that crazy inventory issue (my apologies for not grabbing the events on that), it looks good.
|
Found another problem that is related to the reduction of inventory. If you have that kind of error, at least in donation, the name of the item in question goes away, and you just have the item number. I can write this up as a separate issue if you prefer. (Edit: Looks like purchase is ok)(Further Clarification: Testing on seed, so this is not related to the error with the negative inventory from MGM.)
|
|
@dorner One last thing -- that error is going to be pretty confusing if it ever comes up. What action should the user take if it does -- contact us? Or make an inventory adjustment? Maybe we can give them some guidance. ( I mean the error that comes up if there is negative inventory. I feel like it's a "something happened in the past that we can't get past" situation, rather than a "oh, just adjust the inventory" situation.) |
|
@dorner Whoops --the crash also happens on finalizing audit. Please address. |
|
Pushed a fix for the audit. If the error happens, for now I think they need to contact us. Should I add that to the error message? I feel like it's already pretty wordy. |
|
There really isn't anything else that we really want them to contact us for. It's an exceptional case (which I hope we never actually have once we get events fully implemented). So, yes. Please do have them contact us. |
|
@awwaiid -- can you do the tech review up to this point, please? (Handling the outstanding text change is well within my abilities to confidently o.k.) |
| @@ -82,7 +82,7 @@ def quantity_for(storage_location: nil, item_id: nil) | |||
|
|
|||
| if storage_location | |||
| if item_id | |||
| @inventory.storage_locations[storage_location.to_i].items[item_id.to_i]&.quantity || 0 | |||
| @inventory.storage_locations[storage_location.to_i]&.items&.dig(item_id.to_i)&.quantity || 0 | |||
There was a problem hiding this comment.
Unrelatedly, I learned a while back that javascript's ? operator is short-cutting, so that the first one in a long chain immediately halts the whole sequence. I like the explicitness of Ruby's a bit better, though it ends up more verbose.
|
@cielf added the extra text. |
|
@dorner There are a couple of suggestions from Brock -- could you take a quick look at them and confirm or deny? Thanks. |
Co-authored-by: Brock Wilcox <awwaiid@thelackthereof.org>
|
@cielf done. |
|
@dorner: Your PR |
@cielf this fixes the 2 issues we found so far: