From 802e646db3d3df9fbb6cfdcf23d10c0ca8e8e337 Mon Sep 17 00:00:00 2001 From: Kevin Attfield Date: Sat, 17 Feb 2018 22:27:53 -0800 Subject: [PATCH 1/6] Test distribute over line item amounts The calculator works as intended, but the test could be more strict. --- core/spec/models/spree/distributed_amounts_handler_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/spec/models/spree/distributed_amounts_handler_spec.rb b/core/spec/models/spree/distributed_amounts_handler_spec.rb index c66b218ab6f..a94b5676f0d 100644 --- a/core/spec/models/spree/distributed_amounts_handler_spec.rb +++ b/core/spec/models/spree/distributed_amounts_handler_spec.rb @@ -57,9 +57,9 @@ end end - context "and the line items are not equally priced" do + context "and the line items do not have equal subtotal amounts" do let(:line_items_attributes) do - [{ price: 150 }, { price: 50 }, { price: 100 }] + [{ price: 50, quantity: 3 }, { price: 50, quantity: 1 }, { price: 50, quantity: 2 }] end it "distributes the total amount relative to the item's price" do From 8a48a2a356c14f9ff13dd41be5c40ed629575673 Mon Sep 17 00:00:00 2001 From: Kevin Attfield Date: Sat, 17 Feb 2018 22:32:04 -0800 Subject: [PATCH 2/6] Lean on Money#allocate to simplify calculator --- .../spree/distributed_amounts_handler.rb | 40 ++++++++++--------- .../spree/distributed_amounts_handler_spec.rb | 2 +- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/core/app/models/spree/distributed_amounts_handler.rb b/core/app/models/spree/distributed_amounts_handler.rb index 4d09c9da770..6a52968c2ff 100644 --- a/core/app/models/spree/distributed_amounts_handler.rb +++ b/core/app/models/spree/distributed_amounts_handler.rb @@ -19,25 +19,27 @@ def amount # @return [Hash] a hash of line item IDs and their # corresponding weighted adjustments def distributed_amounts - remaining_amount = @total_amount - - @order.line_items.each_with_index.map do |line_item, i| - if i == @order.line_items.length - 1 - # If this is the last line item on the order we want to use the - # remaining preferred amount to ensure our total adjustment is what - # has been set as the preferred amount. - [line_item.id, remaining_amount] - else - # Calculate the weighted amount by getting this line item's share of - # the order's total and multiplying it with the preferred amount. - weighted_amount = ((line_item.amount / @order.item_total) * total_amount).round(2) - - # Subtract this line item's weighted amount from the total. - remaining_amount -= weighted_amount - - [line_item.id, weighted_amount] - end - end.to_h + Hash[line_item_ids.zip(allocated_amounts)] + end + + def line_item_ids + @order.line_items.map(&:id) + end + + def line_item_amounts + @order.line_items.map(&:amount) + end + + def subtotal + line_item_amounts.sum + end + + def weights + line_item_amounts.map { |amount| amount.to_f / subtotal.to_f } + end + + def allocated_amounts + @total_amount.to_money.allocate(weights) end end end diff --git a/core/spec/models/spree/distributed_amounts_handler_spec.rb b/core/spec/models/spree/distributed_amounts_handler_spec.rb index a94b5676f0d..5ce487e9b15 100644 --- a/core/spec/models/spree/distributed_amounts_handler_spec.rb +++ b/core/spec/models/spree/distributed_amounts_handler_spec.rb @@ -50,7 +50,7 @@ described_class.new(order.line_items[1], total_amount).amount, described_class.new(order.line_items[2], total_amount).amount ] - ).to eq( + ).to match_array( [3.33, 3.33, 3.34] ) end From 0601a0a6cb7d5c9cc40a9228bbf171e08f36431c Mon Sep 17 00:00:00 2001 From: Kevin Attfield Date: Sat, 17 Feb 2018 22:32:29 -0800 Subject: [PATCH 3/6] Add spec demonstration actual behaviour Since line_item level promotion adjustments are only applied to elligible line_items, there's a surprising interaction between the distributed amount calculator and the product promotion rule. The calculator distributes the preffered amount among all of the order's line_items but only discounts the order by the portion which was applied to the line_item with the elligible product. --- .../calculator/distributed_amount_spec.rb | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/core/spec/models/spree/calculator/distributed_amount_spec.rb b/core/spec/models/spree/calculator/distributed_amount_spec.rb index d96cc2a8123..9c0c2f92d7f 100644 --- a/core/spec/models/spree/calculator/distributed_amount_spec.rb +++ b/core/spec/models/spree/calculator/distributed_amount_spec.rb @@ -2,6 +2,52 @@ require 'shared_examples/calculator_shared_examples' RSpec.describe Spree::Calculator::DistributedAmount, type: :model do + context 'applied to an order' do + let(:calculator) { Spree::Calculator::DistributedAmount.new } + let(:promotion) { + create :promotion, + name: '15 spread' + } + let(:order) { + create :completed_order_with_promotion, + promotion: promotion, + line_items_attributes: [{ price: 20 }, { price: 30 }, { price: 100 }] + } + + before do + calculator.preferred_amount = 15 + Spree::Promotion::Actions::CreateItemAdjustments.create!(calculator: calculator, promotion: promotion) + order.recalculate + end + + it 'correctly distributes the entire discount' do + expect(order.promo_total).to eq(-15) + expect(order.line_items.map(&:adjustment_total)).to eq([-2, -3, -10]) + end + + context 'with product promotion rule' do + let(:first_product) { order.line_items.first.product } + + before do + rule = Spree::Promotion::Rules::Product.create!( + promotion: promotion, + product_promotion_rules: [ + Spree::ProductPromotionRule.new(product: first_product), + ], + ) + promotion.rules << rule + promotion.save! + order.recalculate + end + + it 'still distributes the entire discount' do + pending('over allocation to elligible line items') + expect(order.promo_total).to eq(-15) + expect(order.line_items.map(&:adjustment_total)).to eq([-15, 0, 0]) + end + end + end + describe "#compute_line_item" do subject { calculator.compute_line_item(order.line_items.first) } From 10f5e80ece7a1f82cd20cc3d1ace7fda4b381088 Mon Sep 17 00:00:00 2001 From: Kevin Attfield Date: Sun, 18 Feb 2018 00:35:01 -0800 Subject: [PATCH 4/6] Distribute entire amount By only giving weight to line_items which are elligible for the promotion, the entire preffered_amount will be distributed. --- .../spree/calculator/distributed_amount.rb | 1 + .../spree/distributed_amounts_handler.rb | 18 +++++++++------- .../calculator/distributed_amount_spec.rb | 3 ++- .../spree/distributed_amounts_handler_spec.rb | 21 ++++++++++--------- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/core/app/models/spree/calculator/distributed_amount.rb b/core/app/models/spree/calculator/distributed_amount.rb index aff818e79a2..f1214955c5a 100644 --- a/core/app/models/spree/calculator/distributed_amount.rb +++ b/core/app/models/spree/calculator/distributed_amount.rb @@ -14,6 +14,7 @@ def compute_line_item(line_item) if line_item && preferred_currency.casecmp(line_item.currency).zero? Spree::DistributedAmountsHandler.new( line_item, + calculable.promotion, preferred_amount ).amount else diff --git a/core/app/models/spree/distributed_amounts_handler.rb b/core/app/models/spree/distributed_amounts_handler.rb index 6a52968c2ff..0c723da91fe 100644 --- a/core/app/models/spree/distributed_amounts_handler.rb +++ b/core/app/models/spree/distributed_amounts_handler.rb @@ -1,10 +1,11 @@ module Spree class DistributedAmountsHandler - attr_reader :line_item, :order, :total_amount + attr_reader :line_item, :order, :promotion, :total_amount - def initialize(line_item, total_amount) + def initialize(line_item, promotion, total_amount) @line_item = line_item @order = line_item.order + @promotion = promotion @total_amount = total_amount end @@ -26,20 +27,23 @@ def line_item_ids @order.line_items.map(&:id) end - def line_item_amounts - @order.line_items.map(&:amount) + def elligible_amounts + @order.line_items.map do |line_item| + elligible = promotion.line_item_actionable?(line_item.order, line_item) + elligible ? line_item.amount : 0 + end end def subtotal - line_item_amounts.sum + elligible_amounts.sum end def weights - line_item_amounts.map { |amount| amount.to_f / subtotal.to_f } + elligible_amounts.map { |amount| amount.to_f / subtotal.to_f } end def allocated_amounts - @total_amount.to_money.allocate(weights) + @total_amount.to_money.allocate(weights).map(&:to_money) end end end diff --git a/core/spec/models/spree/calculator/distributed_amount_spec.rb b/core/spec/models/spree/calculator/distributed_amount_spec.rb index 9c0c2f92d7f..38596e8b44e 100644 --- a/core/spec/models/spree/calculator/distributed_amount_spec.rb +++ b/core/spec/models/spree/calculator/distributed_amount_spec.rb @@ -41,7 +41,6 @@ end it 'still distributes the entire discount' do - pending('over allocation to elligible line items') expect(order.promo_total).to eq(-15) expect(order.line_items.map(&:adjustment_total)).to eq([-15, 0, 0]) end @@ -52,6 +51,7 @@ subject { calculator.compute_line_item(order.line_items.first) } let(:calculator) { Spree::Calculator::DistributedAmount.new } + let(:promotion) { create(:promotion) } let(:order) do FactoryBot.create( @@ -63,6 +63,7 @@ before do calculator.preferred_amount = 15 calculator.preferred_currency = currency + Spree::Promotion::Actions::CreateItemAdjustments.create!(calculator: calculator, promotion: promotion) end context "when the order currency matches the store's currency" do diff --git a/core/spec/models/spree/distributed_amounts_handler_spec.rb b/core/spec/models/spree/distributed_amounts_handler_spec.rb index 5ce487e9b15..a6913f37566 100644 --- a/core/spec/models/spree/distributed_amounts_handler_spec.rb +++ b/core/spec/models/spree/distributed_amounts_handler_spec.rb @@ -7,11 +7,12 @@ line_items_attributes: line_items_attributes ) end + let(:promotion) { FactoryBot.build(:promotion) } describe "#amount" do let(:total_amount) { 15 } - subject { described_class.new(line_item, total_amount).amount } + subject { described_class.new(line_item, promotion, total_amount).amount } context "when there is only one line item" do let(:line_items_attributes) { [{ price: 100 }] } @@ -31,9 +32,9 @@ it "evenly distributes the total amount" do expect( [ - described_class.new(order.line_items[0], total_amount).amount, - described_class.new(order.line_items[1], total_amount).amount, - described_class.new(order.line_items[2], total_amount).amount + described_class.new(order.line_items[0], promotion, total_amount).amount, + described_class.new(order.line_items[1], promotion, total_amount).amount, + described_class.new(order.line_items[2], promotion, total_amount).amount ] ).to eq( [5, 5, 5] @@ -46,9 +47,9 @@ it "applies the remainder of the total amount to the last item" do expect( [ - described_class.new(order.line_items[0], total_amount).amount, - described_class.new(order.line_items[1], total_amount).amount, - described_class.new(order.line_items[2], total_amount).amount + described_class.new(order.line_items[0], promotion, total_amount).amount, + described_class.new(order.line_items[1], promotion, total_amount).amount, + described_class.new(order.line_items[2], promotion, total_amount).amount ] ).to match_array( [3.33, 3.33, 3.34] @@ -65,9 +66,9 @@ it "distributes the total amount relative to the item's price" do expect( [ - described_class.new(order.line_items[0], total_amount).amount, - described_class.new(order.line_items[1], total_amount).amount, - described_class.new(order.line_items[2], total_amount).amount + described_class.new(order.line_items[0], promotion, total_amount).amount, + described_class.new(order.line_items[1], promotion, total_amount).amount, + described_class.new(order.line_items[2], promotion, total_amount).amount ] ).to eq( [7.5, 2.5, 5] From 5e2b124d02c6054c4bbb05adc120f900fa36cc0e Mon Sep 17 00:00:00 2001 From: Kevin Attfield Date: Sun, 18 Feb 2018 08:31:40 -0800 Subject: [PATCH 5/6] Fix deprecation warnings --- core/app/models/spree/distributed_amounts_handler.rb | 4 ++-- core/spec/models/spree/calculator/distributed_amount_spec.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/distributed_amounts_handler.rb b/core/app/models/spree/distributed_amounts_handler.rb index 0c723da91fe..f548796c0cd 100644 --- a/core/app/models/spree/distributed_amounts_handler.rb +++ b/core/app/models/spree/distributed_amounts_handler.rb @@ -9,9 +9,9 @@ def initialize(line_item, promotion, total_amount) @total_amount = total_amount end - # @return [Float] the weighted adjustment for the initialized line item + # @return [BigDecimal] the weighted adjustment for the initialized line item def amount - distributed_amounts[@line_item.id].to_f + distributed_amounts[@line_item.id].to_d end private diff --git a/core/spec/models/spree/calculator/distributed_amount_spec.rb b/core/spec/models/spree/calculator/distributed_amount_spec.rb index 38596e8b44e..edff82ad6dd 100644 --- a/core/spec/models/spree/calculator/distributed_amount_spec.rb +++ b/core/spec/models/spree/calculator/distributed_amount_spec.rb @@ -69,6 +69,7 @@ context "when the order currency matches the store's currency" do let(:currency) { "USD" } it { is_expected.to eq 5 } + it { is_expected.to be_a BigDecimal } end context "when the order currency does not match the store's currency" do From 7b837d5895f7a7a261ee73067f700f8412cde92c Mon Sep 17 00:00:00 2001 From: Kevin Attfield Date: Mon, 19 Feb 2018 14:29:00 -0800 Subject: [PATCH 6/6] Prefer generic calculator handler --- .../spree/calculator/distributed_amount.rb | 22 +++++++++------ .../spree/distributed_amounts_handler.rb | 24 +++++++---------- .../spree/distributed_amounts_handler_spec.rb | 27 ++++++++++--------- 3 files changed, 38 insertions(+), 35 deletions(-) diff --git a/core/app/models/spree/calculator/distributed_amount.rb b/core/app/models/spree/calculator/distributed_amount.rb index f1214955c5a..c42ef3bb7df 100644 --- a/core/app/models/spree/calculator/distributed_amount.rb +++ b/core/app/models/spree/calculator/distributed_amount.rb @@ -11,14 +11,20 @@ class Calculator::DistributedAmount < Calculator preference :currency, :string, default: -> { Spree::Config[:currency] } def compute_line_item(line_item) - if line_item && preferred_currency.casecmp(line_item.currency).zero? - Spree::DistributedAmountsHandler.new( - line_item, - calculable.promotion, - preferred_amount - ).amount - else - 0 + return 0 unless line_item + return 0 unless preferred_currency.casecmp(line_item.currency).zero? + return 0 unless calculable.promotion.line_item_actionable?(line_item.order, line_item) + Spree::DistributedAmountsHandler.new( + actionable_line_items(line_item.order), + preferred_amount + ).amount(line_item) + end + + private + + def actionable_line_items(order) + order.line_items.select do |line_item| + calculable.promotion.line_item_actionable?(order, line_item) end end end diff --git a/core/app/models/spree/distributed_amounts_handler.rb b/core/app/models/spree/distributed_amounts_handler.rb index f548796c0cd..9a5c774b409 100644 --- a/core/app/models/spree/distributed_amounts_handler.rb +++ b/core/app/models/spree/distributed_amounts_handler.rb @@ -1,17 +1,16 @@ module Spree class DistributedAmountsHandler - attr_reader :line_item, :order, :promotion, :total_amount + attr_reader :line_items, :total_amount - def initialize(line_item, promotion, total_amount) - @line_item = line_item - @order = line_item.order - @promotion = promotion + def initialize(line_items, total_amount) + @line_items = line_items @total_amount = total_amount end - # @return [BigDecimal] the weighted adjustment for the initialized line item - def amount - distributed_amounts[@line_item.id].to_d + # @param [LineItem] one of the line_items distributed over + # @return [BigDecimal] the weighted adjustment for this line_item + def amount(line_item) + distributed_amounts[line_item.id].to_d end private @@ -24,14 +23,11 @@ def distributed_amounts end def line_item_ids - @order.line_items.map(&:id) + line_items.map(&:id) end def elligible_amounts - @order.line_items.map do |line_item| - elligible = promotion.line_item_actionable?(line_item.order, line_item) - elligible ? line_item.amount : 0 - end + line_items.map(&:amount) end def subtotal @@ -43,7 +39,7 @@ def weights end def allocated_amounts - @total_amount.to_money.allocate(weights).map(&:to_money) + total_amount.to_money.allocate(weights).map(&:to_money) end end end diff --git a/core/spec/models/spree/distributed_amounts_handler_spec.rb b/core/spec/models/spree/distributed_amounts_handler_spec.rb index a6913f37566..2bda51537a8 100644 --- a/core/spec/models/spree/distributed_amounts_handler_spec.rb +++ b/core/spec/models/spree/distributed_amounts_handler_spec.rb @@ -7,19 +7,20 @@ line_items_attributes: line_items_attributes ) end - let(:promotion) { FactoryBot.build(:promotion) } + + let(:handler) { + described_class.new(order.line_items, total_amount) + } describe "#amount" do let(:total_amount) { 15 } - subject { described_class.new(line_item, promotion, total_amount).amount } - context "when there is only one line item" do let(:line_items_attributes) { [{ price: 100 }] } let(:line_item) { order.line_items.first } it "applies the entire amount to the line item" do - expect(subject).to eq(15) + expect(handler.amount(line_item)).to eq(15) end end @@ -32,9 +33,9 @@ it "evenly distributes the total amount" do expect( [ - described_class.new(order.line_items[0], promotion, total_amount).amount, - described_class.new(order.line_items[1], promotion, total_amount).amount, - described_class.new(order.line_items[2], promotion, total_amount).amount + handler.amount(order.line_items[0]), + handler.amount(order.line_items[1]), + handler.amount(order.line_items[2]) ] ).to eq( [5, 5, 5] @@ -47,9 +48,9 @@ it "applies the remainder of the total amount to the last item" do expect( [ - described_class.new(order.line_items[0], promotion, total_amount).amount, - described_class.new(order.line_items[1], promotion, total_amount).amount, - described_class.new(order.line_items[2], promotion, total_amount).amount + handler.amount(order.line_items[0]), + handler.amount(order.line_items[1]), + handler.amount(order.line_items[2]) ] ).to match_array( [3.33, 3.33, 3.34] @@ -66,9 +67,9 @@ it "distributes the total amount relative to the item's price" do expect( [ - described_class.new(order.line_items[0], promotion, total_amount).amount, - described_class.new(order.line_items[1], promotion, total_amount).amount, - described_class.new(order.line_items[2], promotion, total_amount).amount + handler.amount(order.line_items[0]), + handler.amount(order.line_items[1]), + handler.amount(order.line_items[2]) ] ).to eq( [7.5, 2.5, 5]