Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3988 Omit inactive items from distributions#new dropdown #3998

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rowenwillabus
Copy link
Contributor

Description

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Created a system test to verify the changes.
  • Changes also verified manually

To reproduce

  1. Sign in as org_admin
  2. Go into Inventory | Inventory Items and delete Adult Large/XL
  3. Go to Distributions
  4. New Distribution
  5. Fill in the storage location
  6. Check list of items: Adult Large/XL will be omitted.

@rowenwillabus rowenwillabus force-pushed the 3988-omit-inactive-items-from-distributions-dropdown branch from 2dd16b7 to ad01f3d Compare December 25, 2023 15:54
@rowenwillabus rowenwillabus reopened this Dec 25, 2023
@dorner
Copy link
Collaborator

dorner commented Dec 25, 2023

@rowenwillabus this looks good! Ideally we'd also have this work when editing a distribution and adding a new item to it (i.e. it shouldn't remove any existing inactive items, but you shouldn't be able to select new items for the same distribution that are inactive). @cielf does that sound right?

Were you able to verify for the other itemizables (donations, transfers, audits, purchases) that they don't currently show inactive items?

@rowenwillabus
Copy link
Contributor Author

@dorner The gotcha regarding the edit was mentioned by @cielf: #3988 (comment) Initially, I shared the same train of thought (editing a distribution, to add a new item, shouldn't allow an inactive item). However, that'll punish users for making a mistake.

Scenario:

  1. There is a distribution of two items Adult Briefs (Large/X-Large) and Bed Pads (Cloth)
  2. Organization user creates a distribution and mistakenly adds 1 item
  • the error is realized two days later, both items are now inactive *

The Organization user will then be prevented from making the correction.

I'm happy to make the change, do let me know.

@cielf
Copy link
Collaborator

cielf commented Dec 25, 2023

@dorner -- That does sound right. Shouldn't be able to add inactive items.

We do need to consider the issue of what happens if someone reduces the amount distributed on an inactive item, whether it is on a line-item basis or a distribution reclaim.

One possibility is that we should display an error, and make the user make the item active to do the change (though only if they are changing the value or reclaiming). It is going to be a uncommon case, I think.

@cielf
Copy link
Collaborator

cielf commented Dec 25, 2023

And, because it's a pretty uncommon case, I don't really mind making the user work around the situation. (1571 out of 11154 items are inactive, but once an item is inactive, it usually stays that way.)

@rowenwillabus rowenwillabus marked this pull request as draft December 26, 2023 00:28
@rowenwillabus
Copy link
Contributor Author

@cielf @dorner The issue stated "inactive" with an explanation on how to make an item inactive. Should we expand that to omit inactive (by status change) AND items with no inventory (zero count)?

If you try to distribute an item that has 0 in inventory, it displays Sorry, we weren't able to save the distribution. Validation failed: Inventory 1T Diapers is not available at this storage location. My thought is to remove such from the dropdown.

Do let me know. I'll wrap this up tomorrow.

@cielf
Copy link
Collaborator

cielf commented Dec 31, 2023

What would the effect of that be on a case where we are creating a distribution from a request that has an item that has 0 in inventory? Will they still initially see the item (that has 0)?

@rowenwillabus
Copy link
Contributor Author

rowenwillabus commented Dec 31, 2023

They wouldn't see that. They would only see items that can be added to the distribution. If they pick an item without any inventory and attempt to move forward, the validation blocks them. My proposal is to only show paths that can be travelled.

See screencast of what happens now:
Screencast from 12-31-2023 06:45:25 AM.webm

edit
I haven't fully explored the UX of creating a distribution from a request. Permit me an hour to respond to that.

@rowenwillabus
Copy link
Contributor Author

What would the effect of that be on a case where we are creating a distribution from a request that has an item that has 0 in inventory? Will they still initially see the item (that has 0)? - @cielf

No. They wouldn't. The item will not be displayed in the list. This is the same effect as a user requesting an item that then becomes inactive. However, I do see the utility in leaving it to be displayed as the user switches between storage locations. Whichever way, I believe it the new and edit experience should be similar, whether the distribution is from a request or not.

You have more context, happy to solve any way you suggest.

@dorner
Copy link
Collaborator

dorner commented Dec 31, 2023

@rowenwillabus - it's a great idea, but I'd actually like to put a kibosh on any inspection of current inventory for these pages. Especially for the edit one - you don't want to know if there is inventory now, but if there was inventory at the time of distribution. We don't have a good way of doing this right now. We are in the process of reimplementing inventory entirely using event sourcing, so it will be handled on submit once that's done.

@rowenwillabus
Copy link
Contributor Author

rowenwillabus commented Dec 31, 2023

@dorner Thanks for mentioning that as I went down a rabbit hole and found some ways in which we can improve the process. E.g a request may have multiple distributions as all items requested may not come from a single storage location. Please loop me in on that work, happy to help.

I'll keep this PR focused on the inactive items affecting the new and edit methods. Thank you both for your time. Will submit changes shortly to close this.

@cielf
Copy link
Collaborator

cielf commented Dec 31, 2023

"E.g a request may have multiple distributions as all items requested may not come from a single storage location"
In practice, that's not a thing I've seen happening. And if it did happen, you would just create another distribution, not linked to the request, for the second storage location. It's a rare enough case that having that as a workaround is good.

@cielf
Copy link
Collaborator

cielf commented Jan 26, 2024

@rowenwillabus Hi Rowen - what's the status on this PR? Are you able to move forward with it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inactive items should not appear in item selection drop downs for distributions.
3 participants