prevent multiple imports of inventory #3696
Conversation
|
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. |
|
ok gotcha, the PR is ready for review |
| @@ -79,6 +79,9 @@ def import_inventory | |||
| flash[:notice] = "Inventory imported successfully!" | |||
| redirect_back(fallback_location: storage_locations_path(organization_id: current_organization)) | |||
| end | |||
| rescue Errors::InventoryAlreadyHasItems => e | |||
There was a problem hiding this comment.
This shouldn't be a thing. In general they will have items and that's not an error.
There was a problem hiding this comment.
Ok, i hadn't understood that! Note that on the frontend, the button to import inventory only appears if there is nothing in the current inventory
<%= modal_button_to("#csvImportModal", { text: "Import Baseline Inventory", icon: "upload" }) if @storage_location.items.empty? %>
Which is why i got confused, to me the ticket was about not making it possible to import while there was items. We should have this button visible at all times then no?
app/models/storage_location.rb
Outdated
| @@ -145,7 +148,8 @@ def increase_inventory(itemizable_array) | |||
| # Locate the storage box for the item, or create a new storage box for it | |||
| inventory_item = inventory_items.find_or_create_by!(item_id: item_hash[:item_id]) | |||
| # Increase the quantity-on-record for that item | |||
| new_quantity = inventory_item.quantity + item_hash[:quantity].to_i | |||
| new_quantity = item_hash[:quantity].to_i | |||
| new_quantity += inventory_item.quantity unless replace_inventory | |||
| inventory_item.update!(quantity: new_quantity) | |||
| # Record in the log that this has occurred | |||
| log[item_hash[:item_id]] = "+#{item_hash[:quantity]}" | |||
There was a problem hiding this comment.
Note that this log is now incorrect - we didn't add this quantity, we added (new_quantity - old_quantity).
I think I'd prefer being more explicit here - rather than setting the inventory to the given value, we should calculate the difference and do an increase_inventory or decrease_inventory to the value we've calculated. I think there would be some edge cases that would bite us if we don't do it this way.
| context "if the inventory already has items" do | ||
| let(:storage_location_with_items) { create(:storage_location, :with_items, item_quantity: 10, organization_id: @organization.id) } | ||
|
|
||
| it "raises the inventory already has items error" do |
fchatterji
force-pushed
the
prevent_double_import_of_inventory
branch
from
f48e19d
to
39103ce
Compare
|
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. |
| adjustment.storage_location.increase_inventory(adjustment) | ||
|
|
||
| increasing_adjustment, _decreasing_adjustment = adjustment.split_difference | ||
|
|
There was a problem hiding this comment.
I don't recommend using adjustment.split_difference. We've recently discovered that it has the side effect of changing the quantities on the line items attached to the adjustment to be positive.
|
@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 #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? |
|
Yes, and please revert the PR back to the original approach. |
|
@fchatterji there's been no movement on this in a very long time - are you still around? |
|
This was replaced by #4300 I think! Thank you for the work @fchatterji, parts of it went into the merged PR. |
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
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"