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

Replace Stock::Coordinator with Stock::SimpleCoordinator #2199

Merged
merged 7 commits into from
Sep 12, 2017

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Sep 1, 2017

This PR introduces Spree::Stock::SimpleCoordinator, which is intended to fix the issues with and replace:

  • Spree::Stock::Coordinator
  • Spree::Stock::Adjuster
  • Spree::Stock::Packer
  • Spree::Stock::Prioritizer

This solves several issues and bugs with the existing Coordinator, where stock coordination would fail despite actually having enough stock. It should also be faster than the existing Coordinator by making use of the new Stock::Availability class.

This fixes #583 and #2122

It is also intended to contain all the business logic for how stock is packaged in the SimpleCoordinator class itself (vs split across Packer/Adjuster/Prioritizer). This should make it a better starting point for stores which want to implement their own coordinator class.

Spree::StockQuantities

This introduces StockQuantities, a value class representing counts for a number of variants.

This class provides set-like operations to make to make it easier to deal with and reason about stock.

Stock::Availability

This commit adds Spree::Stock::Availability, a class to manage checking stock availability efficiently for a set of Variants and StockLocations.

This is similar to the existing Spree::Stock::Quantifier, but more efficient by checking multiple variants at once. This needs only two queries to check stock for any number of variants and stock_locations
(though passing fewer will have faster queries).

This would be a good candidate to be a pluggable class. It could in theory be overridden to provide oft-requested features like stock reservations and querying other sources (like 3PL or ERP). I haven't done this at this time because the interface isn't the best suited to be generic and instead is just the most convenient for the one place its used now. To do this the class would also have to include
unstocking and restocking.

@jhawthorn jhawthorn force-pushed the simple_stock_coordinator branch from 768a5d8 to 12deb68 Compare September 1, 2017 20:08
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@solidusio solidusio deleted a comment from houndci-bot Sep 1, 2017
@jhawthorn jhawthorn force-pushed the simple_stock_coordinator branch from a37c32c to ec7b241 Compare September 1, 2017 22:30
@jhawthorn jhawthorn force-pushed the simple_stock_coordinator branch from ec7b241 to d0f3bd4 Compare September 5, 2017 18:36
@jhawthorn jhawthorn changed the title Simple stock coordinator Replace Stock::Coordinator with Stock::SimpleCoordinator Sep 5, 2017
@jhawthorn jhawthorn force-pushed the simple_stock_coordinator branch from d0f3bd4 to 0f0a97c Compare September 5, 2017 23:25
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

This much better than what we had previously. I would like the old stuff to still be available via an extension (legacy_stock_system) . Great work!

end
end

# Subtracks another StockQuantities from this one
Copy link
Contributor

Choose a reason for hiding this comment

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

Subtracts (typo)

Copy link
Member

@gmacdougall gmacdougall 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 John!

This spec previously created two line_items, and relied on the order
they came back from the DB (and only testing the first). I believe the
second line item was not intended by the original author of this spec.

This commit removes the second line item and makes the spec consistent
regardless of DB ordering.
This commit introduces StockQuantities, a value class representing
counts for a number of variants.

This class provides set-like operations to make to make it easier to
deal with and reason about stock.
This commit adds Spree::Stock::Availability, a class to manage checking
stock availability efficiently for a set of Variants and StockLocations.

This is similar to the existing Spree::Stock::Quantifier, but more
efficient by checking multiple variants at once. This needs only two
queries to check stock for any number of variants and stock_locations
(though passing fewer will have faster queries).

This would be a good candidate to be a pluggable class. It could in
theory be overridden to provide oft-requested features like
stock reservations and querying other sources (like 3PL or ERP). I
haven't done this at this time because the interface isn't the best
suited to be generic and instead is just the most convenient for the
one place its used now. To do this the class would also have to include
unstocking and restocking.
This class is intended to replace Spree::Stock::Coordinator as well as its helper classes:
  Stock::Adjuster
  Stock::Packer
  Stock::Prioritizer

This solves several issues and bugs with the existing Coordinator, where
stock coordination would fail despite actually having enough stock. It
should also be faster than the existing Coordinator by making use of the
new Stock::Availability class.

My hope is that this class is simple enough that it could be copied in
to stores and used as a starting point to customizations.
This commit removes
 * Spree::Stock::Coordinator
 * Spree::Stock::Packer
 * Spree::Stock::Prioritizer
 * Spree::Stock::Adjuster

as well as specs and factories
These specs failed with the old Stock::Coordinator
@jhawthorn jhawthorn force-pushed the simple_stock_coordinator branch from 0f0a97c to 332e216 Compare September 12, 2017 20:57
@jhawthorn
Copy link
Contributor Author

@swcraig is working on the solidus_legacy_stock_system gem. I think this can be merged before that's 100% finished.

@jhawthorn jhawthorn merged commit 0d9c043 into solidusio:master Sep 12, 2017
benjaminwil added a commit to benjaminwil/solidus that referenced this pull request Nov 1, 2017
The way split shipments works has changed significantly in Solidus
v2.4.0. See solidusio#2199 for more information.
benjaminwil added a commit to benjaminwil/solidus that referenced this pull request Nov 24, 2017
This article revises the documentation for split shipment management
in Solidus, which changed as of Solidus v2.4.0. See solidusio#2199 for more
information.
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.

Packaging fails if multiple stock locations are required to fulfill quantity
3 participants