diff --git a/core/app/models/spree/order_updater.rb b/core/app/models/spree/order_updater.rb index f3b4f69906c..2a5ba57803f 100644 --- a/core/app/models/spree/order_updater.rb +++ b/core/app/models/spree/order_updater.rb @@ -168,7 +168,7 @@ def recalculate_order_total def update_adjustment_total update_adjustments - all_items = line_items + shipments + all_items = (line_items + shipments).reject(&:marked_for_destruction?) # Ignore any adjustments that have been marked for destruction in our # calculations. They'll get removed when/if we persist the order. valid_adjustments = adjustments.reject(&:marked_for_destruction?) @@ -232,12 +232,11 @@ def recalculate_item_totals next unless item.changed? - item.update_columns( + item.assign_attributes( promo_total: item.promo_total, included_tax_total: item.included_tax_total, additional_tax_total: item.additional_tax_total, - adjustment_total: item.adjustment_total, - updated_at: Time.current, + adjustment_total: item.adjustment_total ) end end diff --git a/legacy_promotions/app/patches/models/solidus_legacy_promotions/spree_order_updater_patch.rb b/legacy_promotions/app/patches/models/solidus_legacy_promotions/spree_order_updater_patch.rb index 02b7e2f74ef..06f5dd02e62 100644 --- a/legacy_promotions/app/patches/models/solidus_legacy_promotions/spree_order_updater_patch.rb +++ b/legacy_promotions/app/patches/models/solidus_legacy_promotions/spree_order_updater_patch.rb @@ -5,7 +5,7 @@ module SpreeOrderUpdaterPatch def update_adjustment_total update_adjustments - all_items = line_items + shipments + all_items = (line_items + shipments).reject(&:marked_for_destruction?) order_tax_adjustments = adjustments.select(&:eligible?).select(&:tax?) order.adjustment_total = all_items.sum(&:adjustment_total) + adjustments.select(&:eligible?).sum(&:amount) @@ -25,20 +25,20 @@ def recalculate_item_totals # Core doesn't have "eligible" adjustments anymore, so we need to # override the adjustment_total calculation to exclude them for legacy # promotions. - item.adjustment_total = item.adjustments. - select(&:eligible?). - reject(&:included?). - sum(&:amount) - - if item.changed? - item.update_columns( - promo_total: item.promo_total, - included_tax_total: item.included_tax_total, - additional_tax_total: item.additional_tax_total, - adjustment_total: item.adjustment_total, - updated_at: Time.current, - ) - end + item.adjustment_total = item.adjustments.select { |adjustment| + adjustment.eligible? && + !adjustment.marked_for_destruction? && + !adjustment.included? + }.sum(&:amount) + + next unless item.changed? + + item.assign_attributes( + promo_total: item.promo_total, + included_tax_total: item.included_tax_total, + additional_tax_total: item.additional_tax_total, + adjustment_total: item.adjustment_total + ) end end Spree::OrderUpdater.prepend self diff --git a/legacy_promotions/spec/models/spree/promotion/actions/create_adjustment_spec.rb b/legacy_promotions/spec/models/spree/promotion/actions/create_adjustment_spec.rb index 9079f28c7ab..3e9cf436efe 100644 --- a/legacy_promotions/spec/models/spree/promotion/actions/create_adjustment_spec.rb +++ b/legacy_promotions/spec/models/spree/promotion/actions/create_adjustment_spec.rb @@ -126,4 +126,42 @@ is_expected.to eq(Spree::Config.promotions.calculators[described_class.to_s]) } end + + describe "#compute_amount" do + subject { action.compute_amount(order) } + + before do + promotion.promotion_actions = [action] + action.calculator = Spree::Calculator::FlatRate.new(preferred_amount:) + end + + let(:preferred_amount) { 50 } + + context "when the adjustable is actionable" do + it "calls compute on the calculator" do + expect(action.calculator).to receive(:compute).with(order).and_call_original + subject + end + + it "doesn't persist anything to the database" do + allow(action.calculator).to receive(:compute).with(order).and_call_original + + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + + context "calculator returns amount greater than order total" do + let(:preferred_amount) { 300 } + + before do + allow(order).to receive_messages(item_total: 50, ship_total: 50) + end + + it "does not exceed it" do + expect(subject).to eql(-100) + end + end + end + end end diff --git a/legacy_promotions/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb b/legacy_promotions/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb index 5c3ef9bcef8..a4d30fa25f8 100644 --- a/legacy_promotions/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb +++ b/legacy_promotions/spec/models/spree/promotion/actions/create_item_adjustments_spec.rb @@ -92,13 +92,23 @@ module Spree end end - context "#compute_amount" do + describe "#compute_amount" do + subject { action.compute_amount(line_item) } + before { promotion.promotion_actions = [action] } context "when the adjustable is actionable" do it "calls compute on the calculator" do expect(action.calculator).to receive(:compute).with(line_item).and_call_original - action.compute_amount(line_item) + subject + end + + it "doesn't persist anything to the database" do + allow(action.calculator).to receive(:compute).with(line_item).and_call_original + + expect { + subject + }.not_to make_database_queries(manipulative: true) end context "calculator returns amount greater than item total" do @@ -108,7 +118,7 @@ module Spree end it "does not exceed it" do - expect(action.compute_amount(line_item)).to eql(-100) + expect(subject).to eql(-100) end end end diff --git a/legacy_promotions/spec/rails_helper.rb b/legacy_promotions/spec/rails_helper.rb index d015f3daaa6..6b1e170f72e 100644 --- a/legacy_promotions/spec/rails_helper.rb +++ b/legacy_promotions/spec/rails_helper.rb @@ -27,6 +27,7 @@ require 'rspec/rails' require 'rspec-activemodel-mocks' require 'database_cleaner' +require 'db-query-matchers' Dir["./spec/support/**/*.rb"].sort.each { |f| require f } diff --git a/promotions/app/models/solidus_promotions/benefits/create_discounted_item.rb b/promotions/app/models/solidus_promotions/benefits/create_discounted_item.rb index 1732898bc48..dba339619d3 100644 --- a/promotions/app/models/solidus_promotions/benefits/create_discounted_item.rb +++ b/promotions/app/models/solidus_promotions/benefits/create_discounted_item.rb @@ -9,14 +9,13 @@ class CreateDiscountedItem < Benefit preference :necessary_quantity, :integer, default: 1 def perform(order) - line_item = find_item(order) || create_item(order) + line_item = find_item(order) || build_item(order) set_quantity(line_item, determine_item_quantity(order)) line_item.current_discounts << discount(line_item) end def remove_from(order) - line_item = find_item(order) - order.line_items.destroy(line_item) + find_item(order)&.mark_for_destruction end private @@ -25,8 +24,8 @@ def find_item(order) order.line_items.detect { |line_item| line_item.managed_by_order_benefit == self } end - def create_item(order) - order.line_items.create!(quantity: determine_item_quantity(order), variant: variant, managed_by_order_benefit: self) + def build_item(order) + order.line_items.build(quantity: determine_item_quantity(order), variant: variant, managed_by_order_benefit: self) end def determine_item_quantity(order) diff --git a/promotions/app/models/solidus_promotions/order_adjuster.rb b/promotions/app/models/solidus_promotions/order_adjuster.rb index 08316463dbb..295e9284e1d 100644 --- a/promotions/app/models/solidus_promotions/order_adjuster.rb +++ b/promotions/app/models/solidus_promotions/order_adjuster.rb @@ -25,10 +25,12 @@ def call order.reset_current_discounts unless dry_run - # Since automations might have added a line item, we need to recalculate item total and item count here. - order.item_total = order.line_items.sum(&:amount) - order.item_count = order.line_items.sum(&:quantity) - order.promo_total = (order.line_items + order.shipments).sum(&:promo_total) + # Since automations might have added a line item, we need to recalculate + # item total and item count here. + line_items = order.line_items.reject(&:marked_for_destruction?) + order.item_total = line_items.sum(&:amount) + order.item_count = line_items.sum(&:quantity) + order.promo_total = (line_items + order.shipments).sum(&:promo_total) end order end diff --git a/promotions/app/models/solidus_promotions/order_adjuster/persist_discounted_order.rb b/promotions/app/models/solidus_promotions/order_adjuster/persist_discounted_order.rb index 6d05a29afc7..cb460d5cd69 100644 --- a/promotions/app/models/solidus_promotions/order_adjuster/persist_discounted_order.rb +++ b/promotions/app/models/solidus_promotions/order_adjuster/persist_discounted_order.rb @@ -34,13 +34,14 @@ def call attr_reader :order - # Walk through the discounts for an item and update adjustments for it. Once - # all of the discounts have been added as adjustments, remove any old promotion - # adjustments that weren't touched. + # Walk through the discounts for an item and update adjustments for it. + # Once all of the discounts have been added as adjustments, mark any old + # promotion adjustments that weren't touched for destruction. # # @private # @param [#adjustments] item a {Spree::LineItem} or {Spree::Shipment} - # @param [Array] item_discounts a list of calculated discounts for an item + # @param [Array] item_discounts a list of + # calculated discounts for an item # @return [void] def update_adjustments(item, item_discounts) promotion_adjustments = item.adjustments.select(&:promotion?) @@ -48,19 +49,19 @@ def update_adjustments(item, item_discounts) active_adjustments = item_discounts.map do |item_discount| update_adjustment(item, item_discount) end - item.update(promo_total: active_adjustments.sum(&:amount)) + item.promo_total = active_adjustments.sum(&:amount) # Remove any promotion adjustments tied to promotion benefits which no longer match. unmatched_adjustments = promotion_adjustments - active_adjustments - item.adjustments.destroy(unmatched_adjustments) + unmatched_adjustments.each(&:mark_for_destruction) end - # Update or create a new promotion adjustment on an item. + # Update or build a new promotion adjustment on an item. # # @private # @param [#adjustments] item a {Spree::LineItem} or {Spree::Shipment} # @param [SolidusPromotions::ItemDiscount] discount_item calculated discounts for an item - # @return [Spree::Adjustment] the created or updated promotion adjustment + # @return [Spree::Adjustment] the new or updated promotion adjustment def update_adjustment(item, discount_item) adjustment = item.adjustments.detect do |item_adjustment| item_adjustment.source == discount_item.source @@ -71,7 +72,7 @@ def update_adjustment(item, discount_item) order_id: item.is_a?(Spree::Order) ? item.id : item.order_id, label: discount_item.label ) - adjustment.update!(amount: discount_item.amount) + adjustment.amount = discount_item.amount adjustment end end diff --git a/promotions/spec/models/solidus_promotions/benefit_spec.rb b/promotions/spec/models/solidus_promotions/benefit_spec.rb index 188e374c488..43214d79aa6 100644 --- a/promotions/spec/models/solidus_promotions/benefit_spec.rb +++ b/promotions/spec/models/solidus_promotions/benefit_spec.rb @@ -122,6 +122,25 @@ def compute_line_item(_line_item, _options) = 1 end end + describe "#compute_amount" do + subject { benefit.compute_amount(discountable) } + + let(:variant) { create(:variant) } + let(:order) { create(:order) } + let(:discountable) { Spree::LineItem.new(order: order, variant: variant, price: 10, quantity: 1) } + let(:promotion) { SolidusPromotions::Promotion.new(customer_label: "20 Perzent off") } + let(:calculator) { SolidusPromotions::Calculators::Percent.new(preferred_percent: 20) } + let(:benefit) { described_class.new(promotion: promotion, calculator: calculator) } + + it "doesn't save anything to the database" do + discountable + + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + end + describe ".original_promotion_action" do let(:spree_promotion) { create :promotion, :with_adjustable_action } let(:spree_promotion_action) { spree_promotion.actions.first } diff --git a/promotions/spec/models/solidus_promotions/benefits/adjust_line_item_quantity_groups_spec.rb b/promotions/spec/models/solidus_promotions/benefits/adjust_line_item_quantity_groups_spec.rb index d3a6630e230..060f5c0bf7d 100644 --- a/promotions/spec/models/solidus_promotions/benefits/adjust_line_item_quantity_groups_spec.rb +++ b/promotions/spec/models/solidus_promotions/benefits/adjust_line_item_quantity_groups_spec.rb @@ -50,6 +50,14 @@ context "and an item with a quantity of 3" do let(:quantity) { 3 } it { is_expected.to eq(-10) } + + it "doesn't save anything to the database" do + line_item + + expect { + subject + }.not_to make_database_queries(manipulative: true) + end end context "and an item with a quantity of 4" do diff --git a/promotions/spec/models/solidus_promotions/benefits/create_discounted_item_spec.rb b/promotions/spec/models/solidus_promotions/benefits/create_discounted_item_spec.rb index 2972505b7c1..5f158632d50 100644 --- a/promotions/spec/models/solidus_promotions/benefits/create_discounted_item_spec.rb +++ b/promotions/spec/models/solidus_promotions/benefits/create_discounted_item_spec.rb @@ -5,16 +5,18 @@ RSpec.describe SolidusPromotions::Benefits::CreateDiscountedItem do it { is_expected.to respond_to(:preferred_variant_id) } + let!(:benefit) { SolidusPromotions::Benefits::CreateDiscountedItem.new(preferred_variant_id: goodie.id, calculator: hundred_percent, promotion: promotion) } + let(:hundred_percent) { SolidusPromotions::Calculators::Percent.new(preferred_percent: 100) } + let(:promotion) { create(:solidus_promotion) } + let(:goodie) { create(:variant) } + describe "#perform" do - let(:order) { create(:order_with_line_items) } - let(:promotion) { create(:solidus_promotion) } - let(:benefit) { SolidusPromotions::Benefits::CreateDiscountedItem.new(preferred_variant_id: goodie.id, calculator: hundred_percent, promotion: promotion) } - let(:hundred_percent) { SolidusPromotions::Calculators::Percent.new(preferred_percent: 100) } - let(:goodie) { create(:variant) } + let!(:order) { create(:order_with_line_items) } + subject { benefit.perform(order) } it "creates a line item with a hundred percent discount" do - expect { subject }.to change { order.line_items.count }.by(1) + expect { subject }.to change { order.line_items.size }.by(1) created_item = order.line_items.detect { |line_item| line_item.managed_by_order_benefit == benefit } expect(created_item.discountable_amount).to be_zero end @@ -22,5 +24,46 @@ it "never calls the order recalculator" do expect(order).not_to receive(:recalculate) end + + it "does not persist changes to order" do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + end + + describe "remove_from" do + let!(:order) { create(:order_with_line_items) } + + subject { benefit.remove_from(order) } + + context "with an item not on the order" do + it "does not modify the order" do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + end + + context "with an item present on the order" do + before do + benefit.perform(order) + order.save! + end + + it "marks the the line item for destruction" do + expect { subject }.to change { + order.line_items.select(&:marked_for_destruction?).count + }.by(1) + + expect { order.save }.to change(order.line_items, :count).by(-1) + end + + it "does not make manipulative database queries" do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + end end end diff --git a/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb b/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb index c19771c8fca..a11863aa8c5 100644 --- a/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb +++ b/promotions/spec/models/solidus_promotions/order_adjuster_spec.rb @@ -3,16 +3,17 @@ require "rails_helper" RSpec.describe SolidusPromotions::OrderAdjuster, type: :model do - subject(:discounter) { described_class.new(order) } + subject(:order_adjuster) { described_class.new(order, dry_run_promotion: dry_run_promotion) } + let!(:order) { line_item.order } + let(:dry_run_promotion) { nil } let(:line_item) { create(:line_item) } - let(:order) { line_item.order } let(:promotion) { create(:solidus_promotion, apply_automatically: true) } let(:calculator) { SolidusPromotions::Calculators::Percent.new(preferred_percent: 10) } context "adding discounted line items" do let(:variant) { create(:variant, price: 100) } - let(:benefit) do + let!(:benefit) do SolidusPromotions::Benefits::CreateDiscountedItem.create( promotion: promotion, calculator: calculator, @@ -21,18 +22,37 @@ end let(:adjustable) { order } - subject do - benefit - discounter.call + context "when not doing a dry-run of a promotion" do + subject do + order_adjuster.call + end + + it "builds a line item of the given variant with a discount adjustment corresponding to the calculator", :aggregate_failures do + expect { + subject + }.to change { order.line_items.length }.by(1) + + expect(order.line_items.last).not_to be_persisted + expect(order.line_items.last.variant).to eq(variant) + expect(order.line_items.last.adjustments.first.amount).to eq(-10) + expect(order.line_items.last.adjustments.first.source).to eq(benefit) + end end - it "creates a line item of the given variant with a discount adjustment corresponding to the calculator" do - expect { - subject - }.to change { order.line_items.count }.by(1) + context 'when on a dry run' do + let(:dry_run_promotion) { promotion } + + subject do + order_adjuster.call + end + + it "does not create any line items" do + expect { subject }.not_to change(Spree::LineItem, :count) + end - expect(order.line_items.last.variant).to eq(variant) - expect(order.line_items.last.adjustments.promotion.first&.amount).to eq(-10) + it "does not create any adjustments" do + expect { subject }.not_to change(Spree::Adjustment, :count) + end end end @@ -44,7 +64,7 @@ subject do benefit - discounter.call + order_adjuster.call end context "promotion with conditionless benefit" do @@ -116,11 +136,12 @@ let(:old_promotion_benefit) { create(:promotion, :with_adjustable_action, apply_automatically: false).actions.first } let!(:adjustment) { create(:adjustment, source: old_promotion_benefit, adjustable: line_item) } - it "removes the old adjustment from the line item" do + it "marks the old adjustment for destruction" do adjustable.reload expect { subject - }.to change { adjustable.reload.adjustments.length }.by(-1) + }.to change { adjustable.adjustments.first.marked_for_destruction? } + .from(false).to(true) end end end @@ -170,7 +191,7 @@ subject do promotion - discounter.call + order_adjuster.call end it "creates shipping rate discounts" do @@ -184,7 +205,7 @@ it "will not create an adjustment on the shipping rate" do expect do subject - end.not_to change { order.shipments.first.shipping_rates.first.discounts.count } + end.not_to change { order.shipments.first.shipping_rates.first.discounts.length } end end end @@ -199,7 +220,7 @@ expect do promotion subject.call - end.to change { order.shipments.first.adjustments.count } + end.to change { order.shipments.first.adjustments.length } end context "if the promo is eligible but the calculcator returns 0" do @@ -210,7 +231,7 @@ expect do promotion subject.call - end.not_to change { order.shipments.first.adjustments.count } + end.not_to change { order.shipments.first.adjustments.length } end end end diff --git a/promotions/spec/rails_helper.rb b/promotions/spec/rails_helper.rb index c233338de47..6cc5ff61fa4 100644 --- a/promotions/spec/rails_helper.rb +++ b/promotions/spec/rails_helper.rb @@ -5,6 +5,7 @@ require "spec_helper" require "solidus_legacy_promotions" +require 'db-query-matchers' # SOLIDUS DUMMY APP require "spree/testing_support/dummy_app"