-
-
Notifications
You must be signed in to change notification settings - Fork 719
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
Track (negative) stock for on-demand products and overrides #12726
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
90fdf59
Test current stock logic on shipment level
mkllnk 675b7fe
Test stock logic on variant level
mkllnk e9f8936
Remove validation of positive stock when on demand
mkllnk a1887bd
Update stock levels of on-demand items
mkllnk cd8dc41
Update stock specs and add pending cases
mkllnk b6c4079
Allow on-demand VariantOverride to track stock
mkllnk 2201d2e
VariantOverride with on_demand now overriding stock
mkllnk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
# frozen_string_literal: false | ||
|
||
require 'spec_helper' | ||
|
||
RSpec.describe Spree::Variant do | ||
# This method is defined in app/models/concerns/variant_stock.rb. | ||
# There is a separate spec for that concern but here I want to test | ||
# the interplay of Spree::Variant and VariantOverride. | ||
# | ||
# A variant can be scoped to a hub which means that all stock methods | ||
# like this one get overridden. Future calls to `variant.move` are then | ||
# handled by the ScopeVariantToHub module which may call the | ||
# VariantOverride. | ||
describe "#move" do | ||
subject(:variant) { create(:variant, on_hand: 5) } | ||
|
||
it "changes stock" do | ||
expect { variant.move(-2) }.to change { variant.on_hand }.from(5).to(3) | ||
end | ||
|
||
it "reduces stock even when on demand" do | ||
variant.on_demand = true | ||
|
||
expect { variant.move(-2) }.to change { variant.on_hand }.from(5).to(3) | ||
end | ||
|
||
it "rejects negative stock" do | ||
expect { variant.move(-7) }.to raise_error( | ||
ActiveRecord::RecordInvalid, | ||
"Validation failed: Count on hand must be greater than or equal to 0" | ||
) | ||
end | ||
|
||
describe "with VariantOverride" do | ||
subject(:hub_variant) { | ||
Spree::Variant.find(variant.id).tap { |v| scoper.scope(v) } | ||
} | ||
let(:override) { | ||
VariantOverride.create!( | ||
variant:, | ||
hub: create(:distributor_enterprise), | ||
count_on_hand: 7, | ||
on_demand: false, | ||
) | ||
} | ||
let(:scoper) { OpenFoodNetwork::ScopeVariantToHub.new(override.hub) } | ||
|
||
it "changes stock only on the variant override" do | ||
expect { | ||
hub_variant.move(-3) | ||
override.reload | ||
} | ||
.to change { override.count_on_hand }.from(7).to(4) | ||
.and change { hub_variant.on_hand }.from(7).to(4) | ||
.and change { variant.on_hand }.by(0) | ||
end | ||
|
||
it "reduces stock when on demand" do | ||
override.update!(on_demand: true, count_on_hand: 7) | ||
|
||
expect { | ||
hub_variant.move(-3) | ||
override.reload | ||
} | ||
.to change { override.count_on_hand }.from(7).to(4) | ||
.and change { hub_variant.on_hand }.from(7).to(4) | ||
.and change { variant.on_hand }.by(0) | ||
end | ||
|
||
it "doesn't prevent negative stock" do | ||
# VariantOverride relies on other stock checks during checkout. :-( | ||
expect { | ||
hub_variant.move(-8) | ||
override.reload | ||
} | ||
.to change { override.count_on_hand }.from(7).to(-1) | ||
.and change { hub_variant.on_hand }.from(7).to(-1) | ||
.and change { variant.on_hand }.by(0) | ||
|
||
# The update didn't run validations and now it's invalid: | ||
expect(override).not_to be_valid | ||
end | ||
|
||
it "doesn't fail on negative stock when on demand" do | ||
override.update!(on_demand: true, count_on_hand: nil) | ||
|
||
expect { | ||
hub_variant.move(-8) | ||
override.reload | ||
} | ||
.to change { override.count_on_hand }.from(nil).to(-8) | ||
.and change { hub_variant.on_hand }.from(nil).to(-8) | ||
.and change { variant.on_hand }.by(0) | ||
end | ||
end | ||
end | ||
end |
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec description implies that only theoverride
stock should be updated, is this meant to change as well ?Took me a minute to understand, but if I am not mistaken because of
OpenFoodNetwork::ScopeVariantToHub
the variant override is "linked" to the variant, sohub_variant.on_hand
actually returns theoverride.count_on_hand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation seems very strange and error-prone to me.. I'm glad that we at least have a spec that demonstrates it now!