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

Fix inventory quantity errors to be tied to organization, not storage location (Fixes #4647) #4804

Merged
merged 8 commits into from
Dec 10, 2024

Conversation

McEileen
Copy link
Contributor

Resolves #4647

Description

  • Errors for item minimum level are now tied to organization quantity, not storage location quantity.
  • Errors for item minimum level now display as warnings (black on yellow), rather than as errors (white on red).
  • Errors for item recommended level are now tied to organization quantity, not storage location quantity.
  • I added tests to distributions_controller_spec and inventory_check_service_spec to confirm the changes.
  • When a distribution triggers both a minimum level error and a recommended level error, both error messages display. The minimum error message displays first.
  • When both the minimum and recommended level errors display, they display on the same line. I'm not sure if this is desired, or if they should display on separate lines? I will include a screenshot of this scenario. If they should display on separate lines, I'm happy to make the change.
  • In order to test the scenario where both a minimum and recommended level error occur in one distribution, I added a new trait, with_items_mixed, to the storage_locations factory. I contemplated updating the existing with_items trait to support more than one type of item. I decided against that, because I didn't want to alter everywhere that already used the with_items trait. I am curious to hear feedback about this decision and the new trait.

Type of change

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

How Has This Been Tested?

  • In both the inventory_check_service_spec and distributions_controller_spec, I added automated tests for the bug. Before implementing a solution, I verified that the tests failed.
  • I manually tested by following the "recreation" steps in Errors re inventory levels falling below a given level should be based on whole bank, not just current storage location. #4647.
  • I tested the scenario with a minimum quantity error and a recommended quantity error by creating a distribution that distributes two different items. Due to the distribution, one item falls below the minimum quantity, and one item falls below the recommended quantity. I confirmed that both the minimum quantity and recommended quantity error will display in this scenario.

Screenshots

Error message for distribution that causes both minimum level and recommended level error

4647InventoryLevelErrorsUpdated

@McEileen McEileen changed the title WIP: Fix inventory quantity errors to be tied to organization, not storage location (Fixes #4647) Fix inventory quantity errors to be tied to organization, not storage location (Fixes #4647) Nov 27, 2024
@cielf
Copy link
Collaborator

cielf commented Nov 28, 2024

Hey @McEileen I took it out for a spin, and it seems to be working well. I do like the idea of putting the recommended level message on a new line, though. Please do that.

@McEileen
Copy link
Contributor Author

Sure thing, @cielf. I made the requested change in commit c198965.

@cielf cielf requested a review from dorner November 30, 2024 22:34
@cielf
Copy link
Collaborator

cielf commented Nov 30, 2024

@dorner - can you give this a technical once-over? Thx.

Comment on lines 319 to 326
if inventory_check_result.minimum_alert.present? && inventory_check_result.recommended_alert.present?
flash[:alert] = inventory_check_result.minimum_alert
flash[:alert] += "\n"
flash[:alert] += inventory_check_result.recommended_alert
elsif inventory_check_result.minimum_alert.present?
flash[:alert] = inventory_check_result.minimum_alert
elsif inventory_check_result.recommended_alert.present?
flash[:alert] = inventory_check_result.recommended_alert
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if inventory_check_result.minimum_alert.present? && inventory_check_result.recommended_alert.present?
flash[:alert] = inventory_check_result.minimum_alert
flash[:alert] += "\n"
flash[:alert] += inventory_check_result.recommended_alert
elsif inventory_check_result.minimum_alert.present?
flash[:alert] = inventory_check_result.minimum_alert
elsif inventory_check_result.recommended_alert.present?
flash[:alert] = inventory_check_result.recommended_alert
alerts = [inventory_check_result.minimum_alert, inventory_check_result.recommended_alert]
merged_alert = alerts.compact.join("\n")
flash[:alert] = merged_alert if merged_alert.present?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really lovely refactor. Thanks for suggesting it. See 6d00cbc.

@@ -59,5 +59,38 @@
end
end
end

trait :with_items_mixed do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in one place and is pretty complex. I'd much rather just have that one spec create this data rather than adding it to the factory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. See 6d00cbc.

@dorner dorner merged commit 931c6c8 into rubyforgood:main Dec 10, 2024
12 checks passed
@dorner
Copy link
Collaborator

dorner commented Dec 10, 2024

Thanks!

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