human-essentials
human-essentials copied to clipboard
rubyforgood
prevent multiple imports of inventory
Resolves #3687
Description
Added a check that the storage location doesn't already have items before importing the inventory. This prevents double imports and makes sure that, whenever an import is made, items can only be added. Added the corresponding error, handled it in the controller to show it to the user Disable the button after click by using the existing ui helper "submit button"
Added the replace inventory option, used only for the inventory csv import. With the option enabled, imports replace existing quantities instead of just adding. It's not great code as it adds to the already overly complex increase inventory function,there's probably a refactor to do and change the logging, in a separate issue.
Type of change
- Bug fix (non-breaking change which fixes an issue)
How Has This Been Tested?
Added tests in the model
Screenshots
the new error:
if you click on the button to import, you can see the disabled state, with text "in progress"
Hey @fchatterji - this isn't what was asked for in the ticket. We'd like the import to still work, but to adjust the levels of inventory to match what's in the import, rather than blindly adding them.
@dorner ok, what I did was prevent importing when there are already items in the inventory. So whenever an import is made, it is made on an empty inventory. So the inventory after import will always match the content of the import (it cannot add if there is nothing before adding, if you have 10 in the import you'll always have 10 in the inventory at the end).
But yeah right now the import inventory method in storage location still uses an "adjustment", so if there were values in the inventory it would add to them. So I guess you want me to remove this?
So the idea here is to have the ultimate inventory match what's in the import, regardless of what's in the inventory right now. For example:
Current inventory:
1 item1, 2 item2
Import:
2 item1, 3 item2
This would result in an increase of 1 to item1 and an increase of 1 to item2.
Hey, so i adjusted the PR for a new review. As discussed on slack, i left the inventory already has items error.
@cielf can you take a look at this? I'm still a bit fuzzy on what the issue is asking compared to what's being done in the PR.
Am looking at it, and it is complicated... g .. Sorry it has taken me so long -- I kept handling the easier ones.
This issue was born because people were managing to import inventory twice, even though they weren't supposed to, and it was doubling the inventory.
So we wanted import to effectively replace rather than add.
The issue also has a bunch of measures to prevent this happening in the first place. Not sure why we had both.
@fchatterji Sorry for the delay and the confusion. We discussed and decided that your original approach which focused on preventing imports from happening if the storage location had any inventory was the correct thing to do - replacing existing inventory is a red herring and we do not want or need that.
Really sorry for the back and forth here. Hope this is still workable!
I also updated https://github.com/rubyforgood/human-essentials/issues/3687 to reflect the clarified thinking!
@dorner No worries, i understand. So what remains to be done on this ticket? Just removing the split_difference?
@fchatterji there's been no movement on this in a very long time - are you still around?
This was replaced by https://github.com/rubyforgood/human-essentials/pull/4300 I think! Thank you for the work @fchatterji, parts of it went into the merged PR.