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 solidus stock locations sorting #3954

Merged

Conversation

ikraamg
Copy link
Contributor

@ikraamg ikraamg commented Feb 23, 2021

  • This might need backporting as this issue has been here for a couple of versions.

Description

There are two challenges that this PR aims to solve:

  1. Fixing the sorting of stock locations:

The order of the stock locations that is returned from the configured
sorter is lost in the SimpleCoordinator. This occurs in
the Stock::Availiability class when creating the #stock_item_scope.

Previously, the #sort_availability method
was used to order the stock locations, but this was
lost during the refactor to allow extending the default Solidus
allocation logic.

This re-orders the stock locations for all methods
called by the stock allocators. Namely on_hand_by_stock_location_id
and backorderable_by_stock_location_id

  1. Sort stock locations by position after default. (Fix: Sorting Stock Locations #3601)

This is useful because the Admin UI allows the switching of position through a drag and drop interface.
Currently, this interface is not used. It would be more intuitive to have the default stock location used first,
then select the next stock location based on its position column value. For more info, see this issue.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@ikraamg ikraamg changed the title [WIP] Fix solidus stock locations sorting Fix solidus stock locations sorting Feb 23, 2021
@ikraamg ikraamg force-pushed the ikraamg/fix-stock-location-sorting branch from 86caae0 to e5e3a32 Compare February 28, 2021 16:58
@ikraamg ikraamg force-pushed the ikraamg/fix-stock-location-sorting branch from e5e3a32 to d9735eb Compare March 26, 2021 16:51
@ikraamg ikraamg force-pushed the ikraamg/fix-stock-location-sorting branch from d9735eb to 7e3993b Compare March 26, 2021 17:18
@ikraamg ikraamg marked this pull request as ready for review March 26, 2021 17:19
@ikraamg
Copy link
Contributor Author

ikraamg commented Mar 27, 2021

Hey @kennyadsl 👋I thought I would bring the first commit in this PR to your attention, incase you think it's important for the Solidus 3 release.

@ikraamg
Copy link
Contributor Author

ikraamg commented Apr 15, 2021

Hey @aldesantis 👋.

I noticed that you had initially worked on the configurable location sorter before it was affected by a refactor.

When you have some time, could you please have a look at this fix?

Copy link
Member

@aldesantis aldesantis left a comment

Choose a reason for hiding this comment

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

@ikraamg I think this looks good, thank you!

@ikraamg ikraamg force-pushed the ikraamg/fix-stock-location-sorting branch from 7e3993b to 18289fc Compare April 16, 2021 13:38
ikraamg added 2 commits April 16, 2021 15:45
The order of the stock locations that is returned from the configured
sorter is lost in the SimpleCoordinator.

Previously, the  #sort_availability
method was used to re-order the stock locations but this was
lost during the refactor to allow extending the default Solidus
allocation logic.

This commit re-orders the stock locations for all methods
called by the stock allocators.
This is useful because the Admin UI allows the updating of
position value through a drag and drop interface.
A more intuitive and user customisable approach.
@ikraamg ikraamg force-pushed the ikraamg/fix-stock-location-sorting branch from 18289fc to f70d648 Compare April 16, 2021 13:46
@kennyadsl kennyadsl merged commit 41fba9a into solidusio:master May 5, 2021
@kennyadsl kennyadsl deleted the ikraamg/fix-stock-location-sorting branch May 5, 2021 09:14
@kennyadsl kennyadsl added Important Change changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels May 5, 2021
@kennyadsl
Copy link
Member

@ikraamg Thanks! Can I ask you a little help with the CHANGELOG entry for this change? I'd like to understand what people upgrading to the next version that contains this change need to know during the upgrade process. As far as I got this change can impact how stocks are allocated during checkout so I want to be sure to provide clear instructions about how to be sure they don't have any negative drawback from this. Thanks!

@ikraamg
Copy link
Contributor Author

ikraamg commented May 5, 2021

Sure @kennyadsl 👍

The first commit allows for custom stock allocators to work as expected. In other words, before this fix, Solidus technically ignored any custom allocators that stores had configured.

The second commit changes the default solidus stock allocation logic (no custom allocator) by allocating stock first from the warehouse with the default boolean set to true. Thereafter in order of position integer. The position can be changed with the drag and drop interface in the admin panel.

Previously, the default behaviour would have been default first, then in order of name, so the admin drag and drop interface was ignored.

In this image, the stock location named "default" will be used first then the rest by top to bottom: New York, London, Madrid.

Screenshot 2021-05-05 at 13 11 44

@ikraamg Thanks! Can I ask you a little help with the CHANGELOG entry for this change? I'd like to understand what people upgrading to the next version that contains this change need to know during the upgrade process. As far as I got this change can impact how stocks are allocated during checkout so I want to be sure to provide clear instructions about how to be sure they don't have any negative drawback from this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sorting Stock Locations
5 participants