Skip to content

Fix the issue where only one error is displayed, even when multiple errors are present. #4911

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

Conversation

nozomirin
Copy link
Contributor

@nozomirin nozomirin commented Jan 3, 2025

Resolves #4613

Description

  1. Add InventoryActionError
  2. Let InventoryError aggregate InventoryActionErrors

Type of change

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

How Has This Been Tested?

bundle exec rspec spec/events/inventory_aggregate_spec.rb

Screenshots

@nozomirin nozomirin force-pushed the 4613-fix-multiple-errors-but-only-show-one-on-distribution branch 4 times, most recently from cc3e80c to e7d563d Compare January 3, 2025 15:38
@nozomirin nozomirin force-pushed the 4613-fix-multiple-errors-but-only-show-one-on-distribution branch from e7d563d to 6f80c97 Compare January 3, 2025 15:47
cielf
cielf previously requested changes Jan 6, 2025
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @nozomrin -- Thanks for this!

The multiple errors part works well, but you've missed this part of the issue: "The number in brackets after the item (which is the inventory level for that item/storage location) has disappeared -- it should still be there."

Please address.

@nozomirin
Copy link
Contributor Author

nozomirin commented Jan 11, 2025

Hey @nozomrin -- Thanks for this!

The multiple errors part works well, but you've missed this part of the issue: "The number in brackets after the item (which is the inventory level for that item/storage location) has disappeared -- it should still be there."

Please address.

Sorry, I missed that earlier. The issue has been fixed; please check it.

@cielf cielf requested a review from dorner January 13, 2025 16:55
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

LGTM functionally. over to @dorner for technical comments

@cielf cielf dismissed their stale review January 13, 2025 16:56

Addressed.

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

Overall looks good - some minor suggestions.

super(message)
self.item_id = item_id
self.storage_location_id = storage_location_id
def initialize(message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably just remove this definition entirely since it does nothing but call super.

@@ -0,0 +1,14 @@
import { Controller} from "@hotwired/stimulus"
Copy link
Collaborator

Choose a reason for hiding this comment

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

In general Stimulus controllers should be generic. In this case, the controller is just triggering a change event on load. Can we rename this to reflect that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having said that - what does this have to do with the goal of the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general Stimulus controllers should be generic. In this case, the controller is just triggering a change event on load. Can we rename this to reflect that?

sure.

Having said that - what does this have to do with the goal of the PR?

It is mentioned in the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you point me to where? I don't see anything related to the frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@dorner dorner left a comment

Choose a reason for hiding this comment

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

I'm good with this!

@dorner dorner merged commit cf5c061 into rubyforgood:main Jan 21, 2025
12 checks passed
Copy link
Contributor

@nozomirin: Your PR Fix the issue where only one error is displayed, even when multiple errors are present. is part of today's Human Essentials production release: 2025.01.22.
Thank you very much for your contribution!

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.

Fix behaviour with errors on Distribution
3 participants