human-essentials icon indicating copy to clipboard operation
human-essentials copied to clipboard

Fix errors found in testing

Open dorner opened this issue 2 years ago • 10 comments

@cielf this fixes the 2 issues we found so far:

  • Crash after creating a storage location
  • Not failing when reducing inventory via an edit

dorner avatar Feb 18 '24 21:02 dorner

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?

cielf avatar Feb 21 '24 22:02 cielf

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.

dorner avatar Feb 22 '24 01:02 dorner

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.

cielf avatar Feb 22 '24 12:02 cielf

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.

cielf avatar Feb 22 '24 12:02 cielf

Regarding the exception: I'm not sure adding an inventory adjustment would work. It's failing for something that happened in the past...

cielf avatar Feb 22 '24 13:02 cielf

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.

cielf avatar Feb 23 '24 15:02 cielf

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.

cielf avatar Feb 23 '24 15:02 cielf

I spent awhile testing this with the flag on, and it definitely does what I expect it to.

cielf avatar Feb 23 '24 17:02 cielf

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.)

Screenshot 2024-02-25 at 8 40 16 AM

cielf avatar Feb 25 '24 13:02 cielf

@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.)

cielf avatar Feb 26 '24 20:02 cielf

@dorner Whoops --the crash also happens on finalizing audit. Please address.

cielf avatar Mar 01 '24 17:03 cielf

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.

dorner avatar Mar 01 '24 20:03 dorner

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.

cielf avatar Mar 01 '24 22:03 cielf

@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.)

cielf avatar Mar 01 '24 22:03 cielf

@cielf added the extra text.

dorner avatar Mar 03 '24 02:03 dorner

@dorner There are a couple of suggestions from Brock -- could you take a quick look at them and confirm or deny? Thanks.

cielf avatar Mar 03 '24 22:03 cielf

@cielf done.

dorner avatar Mar 04 '24 01:03 dorner

@dorner: Your PR Fix errors found in testing is part of today's Human Essentials production release: 2024.03.17. Thank you very much for your contribution!

github-actions[bot] avatar Mar 17 '24 14:03 github-actions[bot]