Disable inactivating item with inventory and don't show inactive item…

[dorner]

…s on storage location.

Might have some failing tests here as I updated the inventory_for to stop showing zero-inventory items, so might need to fix other expectations.

[cielf]

@dorner Hrm. That's not quite what I expected. The storage location should show active items with 0 inventory (if it's ever had them -- I'm not sure if we currently show inventory levels in an SL if it's never had the items) just not inactive ones.

[dorner]

It should still show them on the storage location page because it explicitly includes them in the query.

[cielf]

It's still letting me inactivate with non-zero inventory, too.

[cielf]

After inactivating a couple, I got this on signing in again: View::Inventory::ViewInventoryItem#name delegated to db_item.name, but db_item is nil: #<View::Inventory::ViewInventoryItem item_id=1 quantity=0 storage_location_id=1 db_item=nil>" Edit: I'll see if i can recreate this.

[cielf]

It should still show them on the storage location page because it explicitly includes them in the query.

Well, I did an inventory adjustment of AB L/XL down to 0, and it disappeared from the SL page. Checked the current behaviour on the production branch -- it doesn't disappear.

[dorner]

Hmm... not sure what happened here then. Will take this back.

[cielf]

Please make this one your first priority, coding-wise, as it will hold up the release. Danke.

[dorner]

@cielf Pushed a fix to show the 0-inventory items. I haven't been able to recreate the other two things you mentioned.

[cielf]

Ignore the comment from 2:28 or so. Didn't have the very latest. Am trying to recreate again.

[cielf]

Well, the first part is definitely still happening on my local, without read_events:
Login as org_admin1@example.com
Click Inventory, then Storage Location, them the 'view' button beside Adult Briefs L/XL. Confirm that it is not zero.
Click items & Inventory, then click the 'Delete' beside Adult Briefs L/XL. Click OK. It says that item has been removed, and it does not appear in the list anymore.
We shouldn't according to my understanding, be able to delete that item, because it has inventory.

[cielf]

I'm getting the same results with read_events enabled.

[dorner]

@cielf deleting an item is not the same as making it inactive. There is a checkbox in the "edit" page that does that.

[cielf]

Hrm. Um - but it shows up again if I click 'Also include inactive items" So what is the difference? (We may have a terminology inconsistency)

[cielf]

Also - on the read_events side, after deleting the item, I got the error I mentioned (View::Inventory::ViewInventoryItem#name delegated to db_item.name, but db_item is nil: #<View::Inventory::ViewInventoryItem item_id=1 quantity=699 storage_location_id=1 db_item=nil>) If I tried to view the storage location after the above steps.

[dorner]

... Item.destroy is redefined to instead deactivate it. 🤦 so we have to copy that check to that place. I'd rather fix it so that doesn't happen, tbh, and have that handled in a delete service.

Thanks - will look again tomorrow.

[cielf]

The delete calls item.destroy , which is overriden to call deactivate if the item has history
From item.rb:

def destroy
if has_history?
deactivate
else
super
end
end

[dorner]

This is a footgun just waiting to go off. Looks like there is already an issue because one code path will deactivate an associated kit and the other won't.

@cielf any issues with removing one or the other paths? I think it'll be simpler to require users to click the "Delete" button to deactivate the item, and remove the checkbox from the update path. We can update the wording on the item to "Deactivate" if it has history. Thoughts?

[cielf]

If we're only to keep one, I would keep the button in the list. I like the idea of changing the wording.

We can probably 'sell' it as a simplification.

[dorner]

@cielf finally got this done

Item page will show "Delete" if it has no history at all, "Deactivate" if it has history but no inventory/kits, and a disabled "Deactivate" button if it has either of those and hence can't even be deactivated. Removed the "Item Is Active?" field from the edit page.

Not sure if some tests will fail, will wait to see.

[dorner]

Finally good to go here I think!

[cielf]

From functional test: If you have an active item that been in a storage location but is now 0 there, the storage location should show in the list in the item view (Inventory | Items & Inventory | view the item in question). It isn't appearing if read_events is enabled.
I'm willing to let this go forward with this issue in place and fix it for next time.

[cielf]

From functional test. Still getting this, with read_events enabled, viewing a storage location after having deactivated an item. FWIW, the item was deactivated with read_events off (I think) but that is certainly in the "could happen" category.

(Sigh) And I can make it disappear. Will recreate from fresh seed.

[cielf]

Here is a shortish path to the error:
1/ fully enable read_events
2/ login as org_admin1
3/ Inventory Adjustments — put Adult Briefs L/XL to 0 for both storage locations
4/ Items & Inventory — Deactivate AB L/XL
5/ Storage Locations
and I get the error

If I restore the item, the error goes away.

[cielf]

So. It looks good to me, functionally, when read_events is not enabled, but is not ready for prime time when read_events is enabled.

FWIW, This sequence was not in my event source testing list. So this problem may have already been there.

[cielf]

I checked against the production branch, though, and I didn't get the error. That was unexpected. EDIT: Seems ok on main too!?

[cielf]

@dorner It appears to me that StorageLocation.items_inventoried is doing things with all items, but trips over its shoelaces on the inactive item. Given the removal of the filter on Storage Locations, I posit that items_inventoried should only be looking at active items. Does that make sense? (or is that the wrong place to fix this)

EDIT: Nope -- it's also dying in much the same way on the dashboard, so either the place to fix it is further 'down' the chain, or we need to fix it in multiple places.

[dorner]

I think I was being dumb with the View::Inventory class. It's OK for it to have inactive items in its cache because it only uses it to populate the db_item for the in-memory item. As long as we have it, it should be populated. Taking an item that used to be active and making it inactive doesn't change all the previous history that had that item in it, and we should be able to view that.

[awwaiid]

I think I was looking for this the other day for another spec, heh

[awwaiid]

I walked through and also did some interactive testing as well, focusing on the non-event branches. Looks like you just fixed the event-branch issue, and the fix makes sense. All of this look good to me.

[awwaiid]

We reviewed the event-based part together and it looks good/safe.

[github-actions]

@dorner: Your PR Disable inactivating item with inventory and don't show inactive item… is part of today's Human Essentials production release: 2024.03.17.
Thank you very much for your contribution!