Make more stock classes configurable #4395
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
stewart
approved these changes
May 30, 2022
spaghetticode
approved these changes
May 31, 2022
kennyadsl
approved these changes
May 31, 2022
jarednorman
force-pushed
the
jared/likes-configurable-classes
branch
from
2483b46 to
bcb06e3
Compare
|
Only change here is that I applied @kennyadsl's suggestion for stubbing the preferences. |
There was a problem hiding this comment.
Thanks, @jarednorman. It's always nice having more flexibility. I left some suggestions.
adammathys
approved these changes
Jun 1, 2022
jarednorman
force-pushed
the
jared/likes-configurable-classes
branch
from
bcb06e3 to
3300cf9
Compare
jarednorman
changed the title
jarednorman
force-pushed
the
jared/likes-configurable-classes
branch
from
3300cf9 to
0b4eebb
Compare
|
I've updated the description and added two additional commits to hook into the other two spots where |
|
@waiting-for-dev can we merge this? |
|
Needs documentation update. |
|
I've opened up a change request in Git Book to explain the use of these endpoints. |
jarednorman
force-pushed
the
jared/likes-configurable-classes
branch
from
3ce8dc4 to
b41f7ce
Compare
|
In writing the documentation I discovered that the commit that made the inventory validator configurable actually hadn't; it had added the configuration option, but it had neither used it, nor even attempted to assert that it the configured value was used. I've now fixed that and rebased this. Assuming tests pass I believe this should be ready to merge after it receives reviews. |
This adds the inventory unit builder (Spree::Stock::InventoryUnitBuilder by default) to the Spree::Config.stock configuration, so that it can be overridden. This is desirable for any store that doesn't have a one-to-one mapping between purchased variants and the fulfilled inventory units. The extension solidus_product_assembly heavily overrides this class to accomplish this, so with this change it will be able to stop decorating Spree::Stock::InventoryUnitBuilder and instead provide its own. I also reworked the stock configuration specs a bit. I fixed the repeated typo, simplified the structure slightly, and ensured they don't leak constants anymore.
This makes the Spree::Stock::AvailabilityValidator swappable. I need to justify using `send` in that test: this method is called by the order state machine, itself a swappable component. I could imagine making it a public method, but I don't want to increase the surface area of Spree::Order's API. Instead, I think it's useful to test this method in isolation (i.e. separately from the state machine) with the assumption that it is part of the API that the configured state machine will call. I've not added tests for the change to the checkout controller in the frontend... because it's too hard. The configured class already gets called a bunch just in setting up these tests, so trying to write a test that cares that it gets called with the right arguments at that particular moment did not seem worth the effort given we're deprecated frontend. Hopefully we can do something better in solidus_starter_frontend, where we also need to care about whether we're on a Solidus version that supports this configuration option.
Similar to the two parent commits, I'm making more of the stock-related classes in Solidus configurable to better support functionality like solidus_product_assembly provides. Also like the previous commit, I've chosen to unit test a private method on Spree::Order because I think it warrants being tested independently of the order state machine (the only place that calls this method).
This ensures that even if the specs fail, they don't leak constants.
jarednorman
force-pushed
the
jared/likes-configurable-classes
branch
from
b41f7ce to
244c5c0
Compare
|
I've implemented @waiting-for-dev's remaining suggestion now. |
waiting-for-dev
approved these changes
Jul 8, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This makes three additional stock classes configurable, so that they can be overridden by extensions like
solidus_product_assemblyor any stores that need similar functionality:Spree::Stock::InventoryUnitBuilderSpree::Stock::InventoryValidatorSpree::Stock::AvailabilityValidatorI think there's probably more (breaking) work to be done to improve how this system works and better support the use case of having line items that map to inventory units with different quantities or variants, but this is a good first step.
Checklist: