Invalid item inventory #3982

dorner · 2023-12-22T16:50:10Z

As per discussion with @cielf .

Currently, we allow items to be turned inactive even when it has inventory in a storage location, or is included in an active kit. Both of those things shouldn't be allowed, because it puts the system in an unknown state (what does it mean to have inventory of an item that isn't valid? Or to distribute kits that have invalid items in them?)

This PR makes a number of changes to make this flow stricter.

First, a migration to reactivate any inactive items that are currently in active kits.
Second, a migration to delete any inventory of inactive items.
Remove options to show inactive inventory (e.g. on the storage location or audit page).
Do not allow an item to be deactivated if it's in an active kit or in storage location inventory.
Do not allow a kit to be reactivated if it contains an inactive item (the user will be prompted to reactivate the item first).

This will help with the event sourcing project as we will not need to track the active status of items for querying them on an inventory basis.

cielf · 2023-12-22T19:21:05Z

Noting for @awwaiid 's benefit that we are going to have to communicate to the affected banks when this goes out

cielf · 2023-12-22T19:22:47Z

But wait! What about the historical trends graph?

cielf · 2023-12-22T19:36:40Z

But wait! What about the historical trends graph?

I don't think they use inventory.

cielf · 2023-12-22T19:37:59Z

However there is another edge case I just thought of -- reclaiming distributions. Might never happen, but..

cielf · 2023-12-22T19:38:51Z

And, of course, what happens if someone edits a donation or purchase that has an inactive item in it. Le Sigh

dorner · 2023-12-24T14:45:55Z

I think if we're talking event-based, neither of those should be an issue. Editing something updates it at the time it was done - so if there are items that are now inactive, they may not have been inactive at the time of the donation etc. and they should show up on the edit page and in the event. Same with reclaiming (aka destroying) a distribution - we fire a new event that just says "this distribution never happened".

cielf · 2023-12-29T14:57:43Z

app/controllers/items_controller.rb

+      return
+    end
+
+    if @item.save


Why did we change from update to save?

update both sets the parameters and saves in a single function. In this case we're setting the parameters early (to check if the active parameter changed) and then saving it after our check.

cielf · 2023-12-29T15:00:17Z

app/controllers/kits_controller.rb

+      @kit.reactivate
+      redirect_back(fallback_location: dashboard_path, notice: "Kit has been reactivated!")
+    else
+      redirect_back(fallback_location: dashboard_path, alert: "Cannot reactivate kit - it has inactive item! Please reactivate the items first.")


tiny grammar thing -- s.b. "it has inactive items!"

cielf

Couple more things to think about, if we haven't already (I just woke up, so the memory is not completely online yet)
What happens if editing a dist, and decreasing the # of the inactive item?
What happens if a request comes in with an inactive item?
What happens if a purchase/donation is edited and the amount on an inactive item is changed?

dorner · 2024-01-01T01:29:16Z

Editing a distribution and decreasing the inactive item - this should work as the user expects and decrease the amount of that item, since at the time the distribution was made the item wasn't inactive. The user should be able to make whatever changes necessary at that time. Having said that, since the item is inactive now, it won't show up on any inventory reports anyway, so practically speaking it won't make much difference.
Request comes in with inactive item - this shouldn't be possible already:

human-essentials/app/services/partner_fetch_requestable_items_service.rb

Line 9 in 56ac4f2

partner.requestable_items.active.visible
Same answer as 1 above.

cielf · 2024-01-02T20:00:40Z

Yeah, all of this really hinges on the idea that editing a distribution is fixing a clerical error. If it actually represents real life changes, then it's potentially a different matter.

rowenwillabus · 2024-01-03T04:40:06Z

While working on #3998 I noticed that inactive items are being reactivated upon editing distribution. I found that to be odd and may need some help understanding that.

dorner · 2024-01-03T14:11:59Z

Ah, I knew I saw something weird. Yeah, it's very odd that we automatically make an item active whenever e.g. editing a distribution to change the amount of an item that's now inactive. It's like we kind of knew that inactive items were bad in a storage location, but also kind of didn't?

In any case we probably should not be doing this without any notification to the user. This might be why I saw some organizations with three items with almost identical names, all with different quantities.

dorner · 2024-01-03T14:12:15Z

I'll make some updates on Friday hopefully.

dorner · 2024-01-21T15:10:12Z

Per discussion on Jan. 21 - we should prevent changing the inventory values for inactive items, by editing any Itemizable (i.e. if the edit would change the values of an item that is now inactive, it should fail).

# Conflicts: # db/schema.rb

dorner · 2024-01-22T00:55:55Z

Fix pushed as per discussion @cielf

cielf · 2024-01-31T16:42:27Z

Hey @dorner. Here's a thought: Regarding the migration making inactive items that are in (active) kits active, I suggest that we should make them not visible to partners. Reasoning: If they are inactive, the bank is definitely not offering them to the partners, and this would be the path of least surprise.

cielf · 2024-02-27T16:03:02Z

@dorner I think the status on this one is that I'm expecting a change regarding the above comment, then to have @awwaiid do the tech review and I'll do a last kick at the can.

# Conflicts: # app/controllers/items_controller.rb # app/controllers/storage_locations_controller.rb # app/queries/items_by_storage_collection_and_quantity_query.rb # app/views/distributions/_form.html.erb # spec/models/storage_location_spec.rb # spec/services/itemizable_update_service_spec.rb

cielf · 2024-03-01T22:17:57Z

Over to @awwaiid, then!

awwaiid · 2024-03-02T16:03:05Z

app/models/storage_location.rb

@@ -142,7 +142,6 @@ def increase_inventory(itemizable_array)
    # This is, at least for now, how we log changes to the inventory made in this call
    log = {}
    # Iterate through each of the line-items in the moving box
-    Item.reactivate(itemizable_array.map { |item_hash| item_hash[:item_id] })


awwaiid · 2024-03-02T16:06:39Z

db/migrate/20231222140700_remove_invalid_inventory.rb

@@ -0,0 +1,23 @@
+class RemoveInvalidInventory < ActiveRecord::Migration[7.0]


Certainly the scariest thing here. Looks good.

awwaiid

This all look great and I'm happy for the cleanup. I'm not merging it immediately until I confirm the timing.

github-actions · 2024-03-17T14:58:23Z

@dorner: Your PR Invalid item inventory is part of today's Human Essentials production release: 2024.03.17.
Thank you very much for your contribution!

dorner added 2 commits December 22, 2023 11:01

Do not allow inactive inventory

aa883c3

Reactivate inactive kit items

2da4128

dorner requested a review from cielf December 22, 2023 16:50

dorner added 2 commits December 22, 2023 11:51

Lint fixes

ad49a35

Remove invalid test

35de3dd

cielf reviewed Dec 29, 2023

View reviewed changes

cielf requested a review from awwaiid December 29, 2023 15:06

fix typo

c3bc7b9

cielf reviewed Dec 30, 2023

View reviewed changes

dorner added 2 commits January 21, 2024 19:48

Merge branch 'main' into invalid-item-inventory

a198897

# Conflicts: # db/schema.rb

Do not reactivate / allow inactive items in updates

ff0031b

dorner added 3 commits March 1, 2024 15:42

Change migration to make items invisible to partners

7ff074d

missed a line

db8ba7d

dorner added 2 commits March 1, 2024 15:55

lint + spec fixes

8097efd

more lint fix

50c7573

awwaiid reviewed Mar 2, 2024

View reviewed changes

awwaiid approved these changes Mar 2, 2024

View reviewed changes

awwaiid merged commit 6059f8e into main Mar 3, 2024
20 checks passed

awwaiid deleted the invalid-item-inventory branch March 3, 2024 15:58

jimmyli97 mentioned this pull request Aug 2, 2024

Improve synchronization of items and their kits -- active status #3348

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalid item inventory #3982

Invalid item inventory #3982

dorner commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023 •

edited

Loading

dorner commented Dec 24, 2023

cielf Dec 29, 2023

dorner Dec 29, 2023

cielf Dec 29, 2023

cielf left a comment

dorner commented Jan 1, 2024

cielf commented Jan 2, 2024

rowenwillabus commented Jan 3, 2024

dorner commented Jan 3, 2024

dorner commented Jan 3, 2024

dorner commented Jan 21, 2024

dorner commented Jan 22, 2024

cielf commented Jan 31, 2024 •

edited

Loading

cielf commented Feb 27, 2024

cielf commented Mar 1, 2024

awwaiid Mar 2, 2024

awwaiid Mar 2, 2024

awwaiid left a comment

github-actions bot commented Mar 17, 2024

		@@ -0,0 +1,23 @@
		class RemoveInvalidInventory < ActiveRecord::Migration[7.0]

Invalid item inventory #3982

Invalid item inventory #3982

Conversation

dorner commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023

cielf commented Dec 22, 2023 • edited Loading

dorner commented Dec 24, 2023

cielf Dec 29, 2023

Choose a reason for hiding this comment

dorner Dec 29, 2023

Choose a reason for hiding this comment

cielf Dec 29, 2023

Choose a reason for hiding this comment

cielf left a comment

Choose a reason for hiding this comment

dorner commented Jan 1, 2024

cielf commented Jan 2, 2024

rowenwillabus commented Jan 3, 2024

dorner commented Jan 3, 2024

dorner commented Jan 3, 2024

dorner commented Jan 21, 2024

dorner commented Jan 22, 2024

cielf commented Jan 31, 2024 • edited Loading

cielf commented Feb 27, 2024

cielf commented Mar 1, 2024

awwaiid Mar 2, 2024

Choose a reason for hiding this comment

awwaiid Mar 2, 2024

Choose a reason for hiding this comment

awwaiid left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 17, 2024

cielf commented Dec 22, 2023 •

edited

Loading

cielf commented Jan 31, 2024 •

edited

Loading