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

Several small refactors to promotions code #3416

Merged
merged 4 commits into from
Nov 20, 2019
Merged
Changes from all commits
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
4 changes: 0 additions & 4 deletions core/app/models/spree/shipping_calculator.rb
Original file line number Diff line number Diff line change
@@ -2,10 +2,6 @@

module Spree
class ShippingCalculator < Calculator
def compute_shipment(_shipment)
raise NotImplementedError, "Please implement 'compute_shipment(shipment)' in your calculator: #{self.class.name}"
end

def compute_package(_package)
raise NotImplementedError, "Please implement 'compute_package(package)' in your calculator: #{self.class.name}"
end
Original file line number Diff line number Diff line change
@@ -25,7 +25,7 @@ module Calculator::Shipping
)
end

subject { FlatPercentItemTotal.new(preferred_flat_percent: 10) }
subject { described_class.new(preferred_flat_percent: 10) }

it "should round result correctly" do
expect(subject.compute(package)).to eq(4.04)
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@
module Spree
module Calculator::Shipping
RSpec.describe FlatRate, type: :model do
subject { Calculator::Shipping::FlatRate.new(preferred_amount: 4.00) }
subject { described_class.new(preferred_amount: 4.00) }

it_behaves_like 'a calculator with a description'

4 changes: 2 additions & 2 deletions core/spec/models/spree/calculator/shipping/flexi_rate_spec.rb
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ module Calculator::Shipping
build(:stock_package, variants_contents: { variant1 => 4, variant2 => 6 })
end

let(:subject) { FlexiRate.new }
let(:subject) { described_class.new }

context "compute" do
it "should compute amount correctly when all fees are 0" do
@@ -46,7 +46,7 @@ module Calculator::Shipping
end

it "should allow creation of new object with all the attributes" do
FlexiRate.new(preferred_first_item: 1,
described_class.new(preferred_first_item: 1,
preferred_additional_item: 1,
preferred_max_items: 1)
end
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ module Calculator::Shipping
build(:stock_package, variants_contents: { variant1 => 5, variant2 => 3 })
end

subject { PerItem.new(preferred_amount: 10) }
subject { described_class.new(preferred_amount: 10) }

it "correctly calculates per item shipping" do
expect(subject.compute(package).to_f).to eq(80) # 5 x 10 + 3 x 10
39 changes: 24 additions & 15 deletions core/spec/models/spree/calculator/shipping/price_sack_spec.rb
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@

RSpec.describe Spree::Calculator::Shipping::PriceSack, type: :model do
let(:calculator) do
calculator = Spree::Calculator::PriceSack.new
calculator = described_class.new
calculator.preferred_minimal_amount = 5
calculator.preferred_normal_amount = 10
calculator.preferred_discount_amount = 1
@@ -14,22 +14,31 @@

it_behaves_like 'a calculator with a description'

let(:order) { stub_model(Spree::Order) }
let(:shipment) { stub_model(Spree::Shipment, amount: 10) }
describe '#compute' do
subject { calculator.compute(package) }
let(:package) { build(:stock_package, variants_contents: { build(:variant) => 1 }) }

# Regression test for https://github.com/spree/spree/issues/714 and https://github.com/spree/spree/issues/739
it "computes with an order object" do
calculator.compute(order)
end
before do
# This hack is due to our factories not being so smart to understand
# that they should create line items with the price of the associated
# variant by default.
allow_any_instance_of(Spree::Stock::ContentItem).to receive(:price) { amount }
Copy link
Member

Choose a reason for hiding this comment

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

Do you think it would make sense (be possible, simple, and useful for other scenarios) to change the factory in order to avoid this stubbing?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had the same thought but I think we have to change the line item price having a dynamic price, based on the variant price. I think that this would break a lot of specs in stores that are relying on the static price of line items in our core factories, and that's why I think this is the best trade-off.

I'm open to making an attempt to create an alternative line_item factory with a dynamic price and trying to deprecate the current one. Do you think it's worth it?

Copy link
Member

@spaghetticode spaghetticode Oct 31, 2019

Choose a reason for hiding this comment

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

I think it is the normal scenario for line items to have prices bound to the ones of their variants... I would consider a price difference to be more of an edge case, so I think that factory could be later changed to reflect the more regular use case.
But of course I would not consider this an urgent matter at all. We may just open an issue and see how it goes from there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, let me do that!

end

# Regression test for https://github.com/spree/spree/issues/1156
it "computes with a shipment object" do
calculator.compute(shipment)
end
context 'when price < minimal amount' do
let(:amount) { 2 }

it "returns the discounted amount" do
expect(subject).to eq(10)
end
end

context 'when price > minimal amount' do
let(:amount) { 6 }

# Regression test for https://github.com/spree/spree/issues/2055
it "computes the correct amount" do
expect(calculator.compute(2)).to eq(calculator.preferred_normal_amount)
expect(calculator.compute(6)).to eq(calculator.preferred_discount_amount)
it "returns the discounted amount" do
expect(subject).to eq(1)
end
end
end
end
4 changes: 2 additions & 2 deletions core/spec/models/spree/returns_calculator_spec.rb
Original file line number Diff line number Diff line change
@@ -5,9 +5,9 @@
module Spree
RSpec.describe ReturnsCalculator, type: :model do
let(:return_item) { build(:return_item) }
subject { ReturnsCalculator.new }
subject { described_class.new }

it 'compute_shipment must be overridden' do
it 'compute must be overridden' do
expect {
subject.compute(return_item)
}.to raise_error NotImplementedError
14 changes: 1 addition & 13 deletions core/spec/models/spree/shipping_calculator_spec.rb
Original file line number Diff line number Diff line change
@@ -21,25 +21,13 @@ module Spree
)
end

subject { ShippingCalculator.new }

it 'computes with a shipment' do
shipment = mock_model(Spree::Shipment)
expect(subject).to receive(:compute_shipment).with(shipment)
subject.compute(shipment)
end
subject { described_class.new }

it 'computes with a package' do
expect(subject).to receive(:compute_package).with(package)
subject.compute(package)
end

it 'compute_shipment must be overridden' do
expect {
subject.compute_shipment(shipment)
}.to raise_error NameError
end

it 'compute_package must be overridden' do
expect {
subject.compute_package(package)