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

4219 Fixed Order of items in inventory adjustment #4380

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

iamronakgupta
Copy link

Resolves #4219

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.

I believe the appropriate place to make the change for the order of items for the inventory adjustments is the adjustments controller, not the storage location controller.

There is code there that appears to be alphabetizing the items -- but I believe it is not alphabetizing them by item name, so it ends up being in the order they were added.

Please also provide tests for your changes.

@iamronakgupta
Copy link
Author

I believe the appropriate place to make the change for the order of items for the inventory adjustments is the adjustments controller, not the storage location controller.

There is code there that appears to be alphabetizing the items -- but I believe it is not alphabetizing them by item name, so it ends up being in the order they were added.

Please also provide tests for your changes.

@cielf Thank you for the review.

After checking from logs, I find out that when we changes storage location from drop down, there is an ajax call happens GET /storage_locations/3/inventory.json?include_omitted_items=true which calls inventory action from storage_locations_controller and that's where items changes their order.
So I think the issue is in storage_locations_controller and specifically in inventory action.
Whats your thought?

@@ -160,6 +160,7 @@ def inventory
.active

@inventory_items += include_omitted_items(@inventory_items.collect(&:item_id)) if params[:include_omitted_items] == "true"
@inventory_items.alphabetized
Copy link
Collaborator

@dorner dorner May 24, 2024

Choose a reason for hiding this comment

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

I think you meant

@inventory_items = @inventory_items.alphabetized

...but even that won't work. alphabetized does an order on the database columns, but at this point @inventory_items isn't an ActiveRecord query, it's an array. It's probably easiest just to do @inventory_items.sort_by! { |ii| ii.item.name } here.

Copy link
Author

Choose a reason for hiding this comment

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

@dorner Yes, I got it now.

But i think @inventory_items.sort_by { |inventory_item| inventory_item.item.name } will work instead of @inventory_items.sort_by!(&:name) as inventory_item record doesn't have name

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.

Looks good, but can you write or update a test verifying that the data is alphabetized? E.g. create inventory items in reverse alphabetical order and verify that they are displayed in proper order. You can use a request test for this.

@iamronakgupta
Copy link
Author

Reference

@dorner yes I am working on this.

Just to confirm, I am working on storage_location_requests_spec here

Am I working at right place?

@cielf
Copy link
Collaborator

cielf commented May 25, 2024

@iamronakgupta I fear that you are "barking up the wrong tree" -- working to solve the problem in the wrong place.

For clarity, here is the result when I manually tested it just now. You can see that the items dropdown in the Inventory Adjustments screen is not showing the items in alphabetical order.

Screenshot 2024-05-25 at 8 41 22 AM

The changes you have made so far would affect a different screen -- the storage locations view.

@iamronakgupta
Copy link
Author

iamronakgupta commented May 26, 2024

@iamronakgupta I fear that you are "barking up the wrong tree" -- working to solve the problem in the wrong place.

For clarity, here is the result when I manually tested it just now. You can see that the items dropdown in the Inventory Adjustments screen is not showing the items in alphabetical order.

Screenshot 2024-05-25 at 8 41 22 AM The changes you have made so far would affect a different screen -- the storage locations view.

@cielf I think this is working because order and sort_by method puts words with lowercase at the bottom and uppercase at top

@cielf
Copy link
Collaborator

cielf commented May 26, 2024

Yup. Looks like that is what it is doing, And it's what it was doing.

IMO, it should be a case insensitive sort because humans sort case insensitively - but that would currently make it inconsistent with the rest of the system.

@iamronakgupta
Copy link
Author

Yup. Looks like that is what it is doing, And it's what it was doing.

IMO, it should be a case insensitive sort because humans sort case insensitively - but that would currently make it inconsistent with the rest of the system.

@cielf So, how should I implement it? with case sensitive or insensitive?

@cielf
Copy link
Collaborator

cielf commented May 27, 2024

With case sensitive. We'd want to make all the sorts case insensitive at the same time.

@dorner
Copy link
Collaborator

dorner commented Aug 16, 2024

@iamronakgupta this has been sitting for a while - are you planning on picking it back up?

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.

Order of items in inventory adjustment should be alphabetical
3 participants