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

Track (negative) stock for on-demand products and overrides #12726

Merged
merged 7 commits into from
Aug 20, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 89 additions & 0 deletions spec/models/spree/variant_stock_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# 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 "ignores stock when on demand" do
variant.on_demand = true

expect { variant.move(-2) }.not_to change { variant.on_hand }
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)
Copy link
Collaborator

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 the override 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, so hub_variant.on_hand actually returns the override.count_on_hand

Copy link
Member

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!

.and change { variant.on_hand }.by(0)
end

it "ignores stock when on demand" do
override.update!(on_demand: true, count_on_hand: nil)

expect {
hub_variant.move(-3)
override.reload
}
.not_to change {
[
override.count_on_hand,
hub_variant.on_hand,
variant.on_hand,
]
}
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
end
end
end