Fix Distribution#new,#create,#edit to only show active items #4848
Conversation
coalest
force-pushed
the
3988-inactive-items-should-not-appear-in-item-selection-drop-downs-for-distributions
branch
from
e7d9bf3 to
9908943
Compare
| @@ -46,7 +46,6 @@ class Distribution < ApplicationRecord | |||
|
|
|||
| enum state: { scheduled: 5, complete: 10 } | |||
| enum delivery_method: { pick_up: 0, delivery: 1, shipped: 2 } | |||
| scope :active, -> { joins(:line_items).joins(:items).where(items: { active: true }) } | |||
There was a problem hiding this comment.
I can remove this change if preferred as it's unrelated to this PR. But it looks like a piece of dead code—I can't find it being used anywhere.
There was a problem hiding this comment.
It's a weird scope anyway... it would only exclude distributions where all the items are inactive, which I can't imagine being a common case.
|
@coalest -- I haven't dug in as yet -- does this cover off the "gotcha" mentioned in the comments -- i.e. that there could be inactive items in distributions that are being edited? (I expect its in the rare-but-could-happen category). |
|
@cielf Whoops, I missed that comment about inactive items wanted to appear when editing distributions. What about the |
|
@cielf Regarding showing inactive items in a distribution. If a distribution has inactive items then it is not editable right? But we want to allow users to be able to edit a previous distribution that didn't have inactive items to now have inactive items? |
|
I remembered that after I'd walked away from my machine. (It's a realtively recent change.) No -- we don't want them to be able to add new inactive items to a distribution. |
|
Ok, so then there isn't really a gotcha here anymore? We never want to show inactive items in the dropdown when creating a new distribution or editing an old distribution? |
|
Right. Stopping people from editting old distributions with inactive items rendered the gotcha moot. |
|
I guess the current flow for a user wants to add inactive items to an old distribution:
Which I guess is ok. Because it's probably not a thing that happens often. And we would rather prevent issues for most used behaviors. |
|
So I will inactive items visible in the item dropdown filter on the distributions index page, because the issue didn't say anything about changing that? |
coalest
force-pushed
the
3988-inactive-items-should-not-appear-in-item-selection-drop-downs-for-distributions
branch
from
9908943 to
654d0d4
Compare
coalest
changed the title
|
Technically more than that. That's the workflow if you want to edit a distribution that happens to have inactive items (for some other reason). All inactive items by rule have zero inventory, so if you actually needed to add inactive items to a distribution, you'd have to add the inventory. But if you're doing that, the item really isn't inactive, anyway... |
|
I think they should be able to see inactive items in the filter, because they might be looking to export the information about them. |
|
In that case, this PR is now ready for review. (I force pushed and reverted the change I had made to the index page) |
|
LGTM functionally |
|
@coalest: Your PR |
Resolves #3988
Description
Previously we were showing inactive items in the item dropdown for distributions in some situations. When we render the page, inactive items were shown, but after the ajax call to fetch inventory quantities (like when a storage location is selected) the inactive items were removed.
After this change, inactive items are never shown.
Type of change
How Has This Been Tested?
Added new request specs and tested manually.
An easy way to test this manually is to disable Javascript in the browser (I used ublock origin for ad blocking and it provides an easy way to toggle javascript on and off).
Otherwise if you try to edit a distribution currently, the ajax call to update the item dropdown might occur too fast for you to see the inactive items.
If testing manually, the
/distributions,/distributions/new(both before clicking save and after clicking save with validation errors), and/distributions/editpages all had this issue.