Invalid item inventory

[dorner]

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]

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

[cielf]

But wait! What about the historical trends graph?

[cielf]

But wait! What about the historical trends graph?

I don't think they use inventory.

[cielf]

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

[cielf]

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

[dorner]

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

[dorner]

1. 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.
2. Request comes in with inactive item - this shouldn't be possible already: https://github.com/rubyforgood/human-essentials/blob/56ac4f21d2bce94f6c4b2073091b95184065397f/app/services/partner_fetch_requestable_items_service.rb#L9-L9
3. Same answer as 1 above.

[cielf]

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]

While working on https://github.com/rubyforgood/human-essentials/pull/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]

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]

I'll make some updates on Friday hopefully.

[dorner]

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

[dorner]

Fix pushed as per discussion @cielf

[cielf]

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]

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

[cielf]

Over to @awwaiid, then!

[github-actions[bot]]

@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!