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

Add a stock locations filter configurable class #3116

Merged

Conversation

kennyadsl
Copy link
Member

Description

This class is helpful to filter stock locations that need to be taken into account by the SimpleCoordinator while generating shipments.

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)

@kennyadsl kennyadsl self-assigned this Feb 20, 2019
Copy link
Contributor

@aitbw aitbw left a comment

Choose a reason for hiding this comment

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

Great job as usual, @kennyadsl 👏

core/app/models/spree/stock/simple_coordinator.rb Outdated Show resolved Hide resolved
core/app/models/spree/stock/simple_coordinator.rb Outdated Show resolved Hide resolved
@kennyadsl kennyadsl force-pushed the kennyadsl/stock-locations-filter branch from 84829f4 to beb537a Compare February 21, 2019 14:53
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Awesome 👏

Could we advise our users to use the config/initializers/spree file instead of having some configuration in application.rb?

This class is helpful to filter stock locations that need to
be taken into account by the SimpleCoordinator while generating
shipments.
I think it's better to use the plural for locations and singular for
sorter since we always use just one sorter class.
Reflecting the file name, stock locations sorter is preferred to
stock location sorters.

Also, there are some broken link to GitHub that have been fixed.
There's no need to keep that item in the menu since it's a
technical implementation detail and could be confusing for someone
who is not familiar with how the sorter works. We are linking
stock locations sorter and filter into the Split Shipment page, which
is the context where those class make sense to be mentioned.
This is how most of the users are doing so we should suggest this
approach.
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Great work! Guides look awesome too! 👍

@jacobherrington jacobherrington merged commit 1a1eae6 into solidusio:master Mar 1, 2019
@aitbw aitbw deleted the kennyadsl/stock-locations-filter branch March 1, 2019 20:30
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.

None yet

4 participants