From e33644dfa9aefb33567d00d7ce66f4c1a2fef2d3 Mon Sep 17 00:00:00 2001 From: benjamin wil Date: Fri, 11 Oct 2024 13:28:52 -0700 Subject: [PATCH 01/20] Add `db-query-matchers` gem https://github.com/sds/db-query-matchers Co-authored-by: Adam Mueller Co-authored-by: Andrew Stewart Co-authored-by: Harmony Evangelina Co-authored-by: Jared Norman Co-authored-by: Nick Van Doorn Co-authored-by: Noah Silvera Co-authored-by: Senem Soy Co-authored-by: Sofia Besenski Co-authored-by: Tom Van Manen --- Gemfile | 2 +- core/spec/rails_helper.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 7f69dd3a643..921dbf3848e 100644 --- a/Gemfile +++ b/Gemfile @@ -21,7 +21,7 @@ gem 'pg', '~> 1.0', require: false if dbs.match?(/all|postgres/) gem 'fast_sqlite', require: false if dbs.match?(/all|sqlite/) gem 'sqlite3', '~> 1.4', require: false if dbs.match?(/all|sqlite/) - +gem 'db-query-matchers' gem 'database_cleaner', '~> 2.0', require: false gem 'rspec-activemodel-mocks', '~> 1.1', require: false gem 'rspec-rails', '~> 6.0.3', require: false diff --git a/core/spec/rails_helper.rb b/core/spec/rails_helper.rb index f3b6e161509..c135d3893f4 100644 --- a/core/spec/rails_helper.rb +++ b/core/spec/rails_helper.rb @@ -13,6 +13,7 @@ require 'rspec/rails' require 'rspec-activemodel-mocks' require 'database_cleaner' +require 'db-query-matchers' Dir["./spec/support/**/*.rb"].sort.each { |f| require f } From 96c9fe774e9ded4b05be45a3721f26b73a6077c2 Mon Sep 17 00:00:00 2001 From: benjamin wil Date: Fri, 11 Oct 2024 13:43:22 -0700 Subject: [PATCH 02/20] Copy existing `OrderUpdater` implementation In subsequent commits we'll ensure that this can update orders in memory, without persisting changes using manipulative DB queries. Co-authored-by: Adam Mueller Co-authored-by: Andrew Stewart Co-authored-by: Harmony Evangelina Co-authored-by: Jared Norman Co-authored-by: Nick Van Doorn Co-authored-by: Noah Silvera Co-authored-by: Senem Soy Co-authored-by: Sofia Besenski Co-authored-by: Tom Van Manen --- .../models/spree/in_memory_order_updater.rb | 242 +++++++++++ .../spree/in_memory_order_updater_spec.rb | 376 ++++++++++++++++++ 2 files changed, 618 insertions(+) create mode 100644 core/app/models/spree/in_memory_order_updater.rb create mode 100644 core/spec/models/spree/in_memory_order_updater_spec.rb diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb new file mode 100644 index 00000000000..41ac249a3ae --- /dev/null +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -0,0 +1,242 @@ +# frozen_string_literal: true + +module Spree + class InMemoryOrderUpdater + attr_reader :order + delegate :payments, :line_items, :adjustments, :all_adjustments, :shipments, :quantity, to: :order + + def initialize(order) + @order = order + end + + # This is a multi-purpose method for processing logic related to changes in the Order. + # It is meant to be called from various observers so that the Order is aware of changes + # that affect totals and other values stored in the Order. + # + # This method should never do anything to the Order that results in a save call on the + # object with callbacks (otherwise you will end up in an infinite recursion as the + # associations try to save and then in turn try to call +update!+ again.) + def recalculate + order.transaction do + update_item_count + update_shipment_amounts + update_totals + if order.completed? + update_payment_state + update_shipments + update_shipment_state + end + Spree::Bus.publish :order_recalculated, order: order + persist_totals + end + end + alias_method :update, :recalculate + deprecate update: :recalculate, deprecator: Spree.deprecator + + # Updates the +shipment_state+ attribute according to the following logic: + # + # shipped when all Shipments are in the "shipped" state + # partial when at least one Shipment has a state of "shipped" and there is another Shipment with a state other than "shipped" + # or there are InventoryUnits associated with the order that have a state of "sold" but are not associated with a Shipment. + # ready when all Shipments are in the "ready" state + # backorder when there is backordered inventory associated with an order + # pending when all Shipments are in the "pending" state + # + # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention. + def update_shipment_state + log_state_change('shipment') do + order.shipment_state = determine_shipment_state + end + + order.shipment_state + end + + # Updates the +payment_state+ attribute according to the following logic: + # + # paid when +payment_total+ is equal to +total+ + # balance_due when +payment_total+ is less than +total+ + # credit_owed when +payment_total+ is greater than +total+ + # failed when most recent payment is in the failed state + # void when the order has been canceled and the payment total is 0 + # + # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention. + def update_payment_state + log_state_change('payment') do + order.payment_state = determine_payment_state + end + + order.payment_state + end + + private + + def determine_payment_state + if payments.present? && payments.valid.empty? && order.outstanding_balance != 0 + 'failed' + elsif order.state == 'canceled' && order.payment_total.zero? + 'void' + elsif order.outstanding_balance > 0 + 'balance_due' + elsif order.outstanding_balance < 0 + 'credit_owed' + else + # outstanding_balance == 0 + 'paid' + end + end + + def determine_shipment_state + if order.backordered? + 'backorder' + else + # get all the shipment states for this order + shipment_states = shipments.states + if shipment_states.size > 1 + # multiple shiment states means it's most likely partially shipped + 'partial' + else + # will return nil if no shipments are found + shipment_states.first + end + end + end + + # This will update and select the best promotion adjustment, update tax + # adjustments, update cancellation adjustments, and then update the total + # fields (promo_total, included_tax_total, additional_tax_total, and + # adjustment_total) on the item. + # @return [void] + def recalculate_adjustments + # Promotion adjustments must be applied first, then tax adjustments. + # This fits the criteria for VAT tax as outlined here: + # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 + # It also fits the criteria for sales tax as outlined here: + # http://www.boe.ca.gov/formspubs/pub113/ + update_promotions + update_taxes + update_item_totals + end + + # Updates the following Order total values: + # + # +payment_total+ The total value of all finalized Payments (NOTE: non-finalized Payments are excluded) + # +item_total+ The total value of all LineItems + # +adjustment_total+ The total value of all adjustments (promotions, credits, etc.) + # +promo_total+ The total value of all promotion adjustments + # +total+ The so-called "order total." This is equivalent to +item_total+ plus +adjustment_total+. + def update_totals + update_payment_total + update_item_total + update_shipment_total + update_adjustment_total + end + + def update_shipment_amounts + shipments.each(&:update_amounts) + end + + # give each of the shipments a chance to update themselves + def update_shipments + shipments.each(&:update_state) + end + + def update_payment_total + order.payment_total = payments.completed.includes(:refunds).sum { |payment| payment.amount - payment.refunds.sum(:amount) } + end + + def update_shipment_total + order.shipment_total = shipments.to_a.sum(&:cost) + update_order_total + end + + def update_order_total + order.total = order.item_total + order.shipment_total + order.adjustment_total + end + + def update_adjustment_total + recalculate_adjustments + + all_items = line_items + shipments + order_tax_adjustments = adjustments.select(&:tax?) + + order.adjustment_total = all_items.sum(&:adjustment_total) + adjustments.sum(&:amount) + order.included_tax_total = all_items.sum(&:included_tax_total) + order_tax_adjustments.select(&:included?).sum(&:amount) + order.additional_tax_total = all_items.sum(&:additional_tax_total) + order_tax_adjustments.reject(&:included?).sum(&:amount) + + update_order_total + end + + def update_item_count + order.item_count = line_items.to_a.sum(&:quantity) + end + + def update_item_total + order.item_total = line_items.to_a.sum(&:amount) + update_order_total + end + + def persist_totals + order.save! + end + + def log_state_change(name) + state = "#{name}_state" + old_state = order.public_send(state) + yield + new_state = order.public_send(state) + if old_state != new_state + order.state_changes.new( + previous_state: old_state, + next_state: new_state, + name: name, + user_id: order.user_id + ) + end + end + + def update_promotions + Spree::Config.promotions.order_adjuster_class.new(order).call + end + + def update_taxes + Spree::Config.tax_adjuster_class.new(order).adjust! + + [*line_items, *shipments].each do |item| + tax_adjustments = item.adjustments.select(&:tax?) + # Tax adjustments come in not one but *two* exciting flavours: + # Included & additional + + # Included tax adjustments are those which are included in the price. + # These ones should not affect the eventual total price. + # + # Additional tax adjustments are the opposite, affecting the final total. + item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) + item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) + end + end + + def update_cancellations + end + deprecate :update_cancellations, deprecator: Spree.deprecator + + def update_item_totals + [*line_items, *shipments].each do |item| + # The cancellation_total isn't persisted anywhere but is included in + # the adjustment_total + item.adjustment_total = item.adjustments. + 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 + end + end + end +end diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb new file mode 100644 index 00000000000..98cfb111ee2 --- /dev/null +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -0,0 +1,376 @@ +# frozen_string_literal: true + +require 'rails_helper' + +module Spree + RSpec.describe InMemoryOrderUpdater, type: :model do + let!(:store) { create :store } + let(:order) { Spree::Order.create } + let(:updater) { described_class.new(order) } + + context "order totals" do + before do + 2.times do + create(:line_item, order: order, price: 10) + end + end + + context 'with refund' do + it "updates payment totals" do + create(:payment_with_refund, order: order, amount: 33.25, refund_amount: 3) + updater.recalculate + expect(order.payment_total).to eq(30.25) + end + end + + it "update item total" do + expect { + updater.recalculate + }.to change { order.item_total }.to 20 + end + + it "update shipment total" do + create(:shipment, order: order, cost: 10) + expect { + updater.recalculate + }.to change { order.shipment_total }.to 10 + end + + context 'with a source-less line item adjustment' do + let(:line_item) { create(:line_item, order: order, price: 10) } + before do + create(:adjustment, source: nil, adjustable: line_item, order: order, amount: -5) + end + + it "updates the line item total" do + expect { updater.recalculate }.to change { line_item.reload.adjustment_total }.from(0).to(-5) + end + end + + it "update order adjustments" do + create(:adjustment, adjustable: order, order: order, source: nil, amount: 10) + + expect { + updater.recalculate + }.to change { + order.adjustment_total + }.from(0).to(10) + end + end + + describe '#recalculate_adjustments ' do + describe 'promotion recalculation' do + it "calls the Promotion Adjustments Recalculator" do + adjuster = double(:call) + expect(Spree::Config.promotions.order_adjuster_class).to receive(:new).and_return(adjuster) + expect(adjuster).to receive(:call) + order.recalculate + end + end + + describe 'tax recalculation' do + let!(:ship_address) { create(:address) } + let!(:tax_zone) { create(:global_zone) } # will include the above address + let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_categories: [tax_category]) } + + let(:order) do + create( + :order_with_line_items, + line_items_attributes: [{ price: 10, variant: variant }], + ship_address: ship_address, + ) + end + let(:line_item) { order.line_items[0] } + + let(:variant) { create(:variant, tax_category: tax_category) } + let(:tax_category) { create(:tax_category) } + + context 'when the item quantity has changed' do + before do + line_item.update!(quantity: 2) + end + + it 'updates the promotion amount' do + expect { + order.recalculate + }.to change { + line_item.additional_tax_total + }.from(1).to(2) + end + end + + context 'with a custom tax_calculator_class' do + let(:custom_calculator_class) { double } + let(:custom_calculator_instance) { double } + + before do + order # generate this first so we can expect it + stub_spree_preferences(tax_calculator_class: custom_calculator_class) + + allow(custom_calculator_class).to receive(:new).and_return(custom_calculator_instance) + allow(custom_calculator_instance).to receive(:calculate).and_return( + Spree::Tax::OrderTax.new( + order_id: order.id, + order_taxes: [ + Spree::Tax::ItemTax.new( + label: "Delivery Fee", + tax_rate: tax_rate, + amount: 2.60, + included_in_price: false + ) + ], + line_item_taxes: [ + Spree::Tax::ItemTax.new( + item_id: line_item.id, + label: "Item Tax", + tax_rate: tax_rate, + amount: 1.40, + included_in_price: false + ) + ], + shipment_taxes: [] + ) + ) + end + + it 'uses the configured class' do + order.recalculate + + expect(custom_calculator_class).to have_received(:new).with(order).at_least(:once) + expect(custom_calculator_instance).to have_received(:calculate).at_least(:once) + end + + it 'updates the aggregate columns' do + expect { + order.recalculate + }.to change { order.reload.additional_tax_total }.to(4.00) + .and change { order.reload.adjustment_total }.to(4.00) + end + end + end + end + + context "updating shipment state" do + before do + allow(order).to receive_messages backordered?: false + end + + it "is backordered" do + allow(order).to receive_messages backordered?: true + updater.update_shipment_state + + expect(order.shipment_state).to eq('backorder') + end + + it "is nil" do + updater.update_shipment_state + expect(order.shipment_state).to be_nil + end + + ["shipped", "ready", "pending"].each do |state| + it "is #{state}" do + create(:shipment, order: order, state: state) + updater.update_shipment_state + expect(order.shipment_state).to eq(state) + end + end + + it "is partial" do + create(:shipment, order: order, state: 'pending') + create(:shipment, order: order, state: 'ready') + updater.update_shipment_state + expect(order.shipment_state).to eq('partial') + end + end + + context "updating payment state" do + let(:order) { build(:order) } + let(:updater) { order.recalculator } + before { allow(order).to receive(:refund_total).and_return(0) } + + context 'no valid payments with non-zero order total' do + it "is failed" do + create(:payment, order: order, state: 'invalid') + order.total = 1 + order.payment_total = 0 + + updater.update_payment_state + expect(order.payment_state).to eq('failed') + end + end + + context 'invalid payments are present but order total is zero' do + it 'is paid' do + order.payments << Spree::Payment.new(state: 'invalid') + order.total = 0 + order.payment_total = 0 + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'paid' + end + end + + context "payment total is greater than order total" do + it "is credit_owed" do + order.payment_total = 2 + order.total = 1 + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "order total is greater than payment total" do + it "is balance_due" do + order.payment_total = 1 + order.total = 2 + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'balance_due' + end + end + + context "order total equals payment total" do + it "is paid" do + order.payment_total = 30 + order.total = 30 + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'paid' + end + end + + context "order is canceled" do + before do + order.state = 'canceled' + end + + context "and is still unpaid" do + it "is void" do + order.payment_total = 0 + order.total = 30 + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'void' + end + end + + context "and is paid" do + it "is credit_owed" do + order.payment_total = 30 + order.total = 30 + create(:payment, order: order, state: 'completed', amount: 30) + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "and payment is refunded" do + it "is void" do + order.payment_total = 0 + order.total = 30 + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'void' + end + end + end + end + + context "completed order" do + before { allow(order).to receive_messages completed?: true } + + it "updates payment state" do + expect(updater).to receive(:update_payment_state) + updater.recalculate + end + + it "updates shipment state" do + expect(updater).to receive(:update_shipment_state) + updater.recalculate + end + + context 'with a shipment' do + before { create(:shipment, order: order) } + let(:shipment){ order.shipments[0] } + + it "updates each shipment" do + expect(shipment).to receive(:update_state) + updater.recalculate + end + + it "updates the shipment amount" do + expect(shipment).to receive(:update_amounts) + updater.recalculate + end + end + end + + context "incompleted order" do + before { allow(order).to receive_messages completed?: false } + + it "doesnt update payment state" do + expect(updater).not_to receive(:update_payment_state) + updater.recalculate + end + + it "doesnt update shipment state" do + expect(updater).not_to receive(:update_shipment_state) + updater.recalculate + end + + it "doesnt update each shipment" do + shipment = stub_model(Spree::Shipment) + order.shipments = [shipment] + allow(order.shipments).to receive_messages(states: [], ready: [], pending: [], shipped: []) + allow(updater).to receive(:update_totals) # Otherwise this gets called and causes a scene + expect(updater).not_to receive(:update_shipments) + updater.recalculate + end + end + + context "with item with no adjustment and incorrect totals" do + let!(:line_item) { create(:line_item, order: order, price: 10) } + + it "updates the totals" do + line_item.update!(adjustment_total: 100) + expect { + order.recalculate + }.to change { line_item.reload.adjustment_total }.from(100).to(0) + end + end + + context "with 'order_recalculated' event subscription" do + let(:item) { spy('object') } + let(:bus) { Spree::Bus } + + let!(:subscription) do + bus.subscribe :order_recalculated do + item.do_something + end + end + + after { bus.unsubscribe subscription } + + it "fires the 'order_recalculated' event" do + order.recalculate + + expect(item).to have_received(:do_something) + end + end + + context "with invalid associated objects" do + let(:order) { Spree::Order.create(ship_address: Spree::Address.new) } + subject { updater.recalculate } + + it "raises because of the invalid object" do + expect { subject }.to raise_error(ActiveRecord::RecordInvalid) + end + end + end +end From 711db241a350dcbc940d3447de9e8aaf2a01df8c Mon Sep 17 00:00:00 2001 From: benjamin wil Date: Fri, 11 Oct 2024 13:56:49 -0700 Subject: [PATCH 03/20] Add `persist` flag to `#recalculate` We want our new in-memory order updater to be able to persist or not persist changes to the order record. WORK IN PROGRESS This is a first step in ensuring we don't need to write to the database using the order updater. Clearly we have more work to do to ensure this functions like the existing updater. Co-authored-by: Adam Mueller Co-authored-by: Andrew Stewart Co-authored-by: Harmony Evangelina Co-authored-by: Jared Norman Co-authored-by: Nick Van Doorn Co-authored-by: Noah Silvera Co-authored-by: Senem Soy Co-authored-by: Sofia Besenski Co-authored-by: Tom Van Manen --- .../models/spree/in_memory_order_updater.rb | 5 +- .../spree/in_memory_order_updater_spec.rb | 46 +++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index 41ac249a3ae..5f5255a169f 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -16,7 +16,7 @@ def initialize(order) # This method should never do anything to the Order that results in a save call on the # object with callbacks (otherwise you will end up in an infinite recursion as the # associations try to save and then in turn try to call +update!+ again.) - def recalculate + def recalculate(persist: true) order.transaction do update_item_count update_shipment_amounts @@ -27,7 +27,8 @@ def recalculate update_shipment_state end Spree::Bus.publish :order_recalculated, order: order - persist_totals + + persist_totals if persist end end alias_method :update, :recalculate diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index 98cfb111ee2..2e9020825dd 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -8,6 +8,52 @@ module Spree let(:order) { Spree::Order.create } let(:updater) { described_class.new(order) } + describe "#recalculate" do + subject { updater.recalculate(persist:) } + + let(:new_store) { create(:store) } + + context "when the persist flag is set to 'false'" do + let(:persist) { false } + + it "does not persist changes to order" do + order.store = new_store + + expect { + subject + }.not_to make_database_queries(manipulative: true) + + expect(order.store).to eq new_store + expect(order.reload.store).not_to eq new_store + end + + it "does not persist changes to the item count" do + order.line_items << build(:line_item) + + expect { + subject + }.not_to make_database_queries(manipulative: true) + + expect(order.item_count).to eq 1 + expect(order.reload.item_count).to eq 0 + end + end + + context "when the persist flag is set to 'true'" do + let(:persist) { true } + + it "persists any changes to order" do + order.store = new_store + + expect { + subject + }.to make_database_queries(manipulative: true) + + expect(order.reload.store).to eq new_store + end + end + end + context "order totals" do before do 2.times do From 42cca44a3bebe44252af652f4997752828559660 Mon Sep 17 00:00:00 2001 From: Noah Silvera Date: Fri, 25 Oct 2024 16:47:25 -0400 Subject: [PATCH 04/20] Add describe block to Shipment#update_amounts test --- core/spec/models/spree/shipment_spec.rb | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 384413166d6..6578a288967 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -507,14 +507,23 @@ end end - context "updates cost when selected shipping rate is present" do - let(:shipment) { create(:shipment) } - before { shipment.selected_shipping_rate.update!(cost: 5) } + describe "#update_amounts" do + let(:shipment) { create(:shipment, cost: 3) } - it "updates shipment totals" do - expect { - shipment.update_amounts - }.to change { shipment.cost }.to(5) + context 'when the selected shipping rate cost is different than the current shipment cost' do + before { shipment.selected_shipping_rate.update!(cost: 5) } + + it "updates the shipments cost" do + expect { + shipment.update_amounts + }.to change { shipment.reload.cost }.to(5) + end + + it 'changes the updated_at column' do + expect { + shipment.update_amounts + }.to change { shipment.reload.updated_at } + end end end From c5a41b874ca9e4e483e48ac46ea49c400772c517 Mon Sep 17 00:00:00 2001 From: Noah Silvera Date: Fri, 25 Oct 2024 17:13:33 -0400 Subject: [PATCH 05/20] Conditionally persist Shipment#update_amounts changes This is in service of supporting the InMemoryOrderUpdater's goal to not do database writes. --- core/app/models/spree/shipment.rb | 4 +-- core/spec/models/spree/shipment_spec.rb | 33 +++++++++++++++++++------ 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/core/app/models/spree/shipment.rb b/core/app/models/spree/shipment.rb index 6808518448d..310d74fb327 100644 --- a/core/app/models/spree/shipment.rb +++ b/core/app/models/spree/shipment.rb @@ -258,10 +258,10 @@ def tracking_url @tracking_url ||= shipping_method.build_tracking_url(tracking) end - def update_amounts + def update_amounts(persist: true) if selected_shipping_rate self.cost = selected_shipping_rate.cost - if changed? + if changed? && persist update_columns( cost:, updated_at: Time.current diff --git a/core/spec/models/spree/shipment_spec.rb b/core/spec/models/spree/shipment_spec.rb index 6578a288967..cbbefe292b8 100644 --- a/core/spec/models/spree/shipment_spec.rb +++ b/core/spec/models/spree/shipment_spec.rb @@ -508,22 +508,41 @@ end describe "#update_amounts" do - let(:shipment) { create(:shipment, cost: 3) } + subject { shipment.update_amounts(persist: persist) } + + let(:persist) { true } + let(:shipment) { create(:shipment, cost: 1) } context 'when the selected shipping rate cost is different than the current shipment cost' do - before { shipment.selected_shipping_rate.update!(cost: 5) } + before { shipment.selected_shipping_rate.update!(cost: 999) } - it "updates the shipments cost" do + it "changes and persists the shipments cost" do expect { - shipment.update_amounts - }.to change { shipment.reload.cost }.to(5) + subject + }.to change { shipment.reload.cost }.to(999) end - it 'changes the updated_at column' do + it 'changes and persists the updated_at column' do expect { - shipment.update_amounts + subject }.to change { shipment.reload.updated_at } end + + context 'when `persist: false` is passed' do + let(:persist) { false } + + it 'does not perform any database writes' do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + + it "changes but does not persist the shipments cost" do + subject + expect(shipment.cost).to eq 999 + expect(shipment.reload.cost).to eq 1 + end + end end end From a7ab737783e5b07dd823ae0d19b2827ab6426e95 Mon Sep 17 00:00:00 2001 From: Noah Silvera Date: Fri, 25 Oct 2024 17:18:06 -0400 Subject: [PATCH 06/20] Preventing InMemoryOrderUpdater#update_shipment_amounts from making database writes We have prevented write calls to update the cost and `updated_at` of a shipment, as well as allowed us to conditionally persist item totals, by passing down the `persist` argument to that method. Co-authored-by: Adam Mueller Co-authored-by: Andrew Stewart Co-authored-by: Harmony Evangelina Co-authored-by: Kendra Riga Co-authored-by: Jared Norman Co-authored-by: Tom Van Manen Co-authored-by: Chris Todorov Co-authored-by: Sofia Besenski Co-authored-by: Senem Soy Co-authored-by: Benjamin Willems --- .../models/spree/in_memory_order_updater.rb | 24 +++++++++---------- .../spree/in_memory_order_updater_spec.rb | 24 +++++++++++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index 5f5255a169f..d0566b886de 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -19,8 +19,8 @@ def initialize(order) def recalculate(persist: true) order.transaction do update_item_count - update_shipment_amounts - update_totals + update_shipment_amounts(persist:) + update_totals(persist:) if order.completed? update_payment_state update_shipments @@ -107,7 +107,7 @@ def determine_shipment_state # fields (promo_total, included_tax_total, additional_tax_total, and # adjustment_total) on the item. # @return [void] - def recalculate_adjustments + def recalculate_adjustments(persist:) # Promotion adjustments must be applied first, then tax adjustments. # This fits the criteria for VAT tax as outlined here: # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 @@ -115,7 +115,7 @@ def recalculate_adjustments # http://www.boe.ca.gov/formspubs/pub113/ update_promotions update_taxes - update_item_totals + update_item_totals(persist:) end # Updates the following Order total values: @@ -125,15 +125,15 @@ def recalculate_adjustments # +adjustment_total+ The total value of all adjustments (promotions, credits, etc.) # +promo_total+ The total value of all promotion adjustments # +total+ The so-called "order total." This is equivalent to +item_total+ plus +adjustment_total+. - def update_totals + def update_totals(persist:) update_payment_total update_item_total update_shipment_total - update_adjustment_total + update_adjustment_total(persist:) end - def update_shipment_amounts - shipments.each(&:update_amounts) + def update_shipment_amounts(persist:) + shipments.each { _1.update_amounts(persist:) } end # give each of the shipments a chance to update themselves @@ -154,8 +154,8 @@ def update_order_total order.total = order.item_total + order.shipment_total + order.adjustment_total end - def update_adjustment_total - recalculate_adjustments + def update_adjustment_total(persist:) + recalculate_adjustments(persist:) all_items = line_items + shipments order_tax_adjustments = adjustments.select(&:tax?) @@ -220,7 +220,7 @@ def update_cancellations end deprecate :update_cancellations, deprecator: Spree.deprecator - def update_item_totals + def update_item_totals(persist:) [*line_items, *shipments].each do |item| # The cancellation_total isn't persisted anywhere but is included in # the adjustment_total @@ -228,7 +228,7 @@ def update_item_totals reject(&:included?). sum(&:amount) - if item.changed? + if persist && item.changed? item.update_columns( promo_total: item.promo_total, included_tax_total: item.included_tax_total, diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index 2e9020825dd..0bdb503a9bd 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -37,6 +37,30 @@ module Spree expect(order.item_count).to eq 1 expect(order.reload.item_count).to eq 0 end + + context 'when a shipment is attached to the order' do + let(:shipment) { create(:shipment) } + + before do + order.shipments << shipment + end + + it 'does not make manipulative database queries' do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + + context 'when the shipment has a selected shipping rate' do + let(:shipment) { create(:shipment, shipping_rates: [build(:shipping_rate, selected: true)]) } + + it 'does not make manipulative database queries' do + expect { + subject + }.not_to make_database_queries(manipulative: true) + end + end + end end context "when the persist flag is set to 'true'" do From 3b74feec11a9bc8ebed1f16e5ad5ce5d727bc2f1 Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Fri, 8 Nov 2024 13:54:54 -0800 Subject: [PATCH 07/20] Rename method that recalculates shipment state Update implies that we are persisting the change in Rails, which this method does not do. Co-authored-by: Adam Mueller Co-authored-by: Senem Soy Co-authored-by: Andrew Stewart Co-authored-by: Kendra Riga Co-authored-by: Sofia Besenski Co-authored-by: Benjamin Willems --- core/app/models/spree/in_memory_order_updater.rb | 8 +++++--- .../models/spree/in_memory_order_updater_spec.rb | 12 ++++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index d0566b886de..00d98ef1f67 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -24,7 +24,7 @@ def recalculate(persist: true) if order.completed? update_payment_state update_shipments - update_shipment_state + recalculate_shipment_state end Spree::Bus.publish :order_recalculated, order: order @@ -34,7 +34,7 @@ def recalculate(persist: true) alias_method :update, :recalculate deprecate update: :recalculate, deprecator: Spree.deprecator - # Updates the +shipment_state+ attribute according to the following logic: + # Recalculates the +shipment_state+ attribute according to the following logic: # # shipped when all Shipments are in the "shipped" state # partial when at least one Shipment has a state of "shipped" and there is another Shipment with a state other than "shipped" @@ -44,13 +44,15 @@ def recalculate(persist: true) # pending when all Shipments are in the "pending" state # # The +shipment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention. - def update_shipment_state + def recalculate_shipment_state log_state_change('shipment') do order.shipment_state = determine_shipment_state end order.shipment_state end + alias_method :update_shipment_state, :recalculate_shipment_state + deprecate update_shipment_state: :recalculate_shipment_state, deprecator: Spree.deprecator # Updates the +payment_state+ attribute according to the following logic: # diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index 0bdb503a9bd..00d2e21dfd2 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -227,20 +227,20 @@ module Spree it "is backordered" do allow(order).to receive_messages backordered?: true - updater.update_shipment_state + updater.recalculate_shipment_state expect(order.shipment_state).to eq('backorder') end it "is nil" do - updater.update_shipment_state + updater.recalculate_shipment_state expect(order.shipment_state).to be_nil end ["shipped", "ready", "pending"].each do |state| it "is #{state}" do create(:shipment, order: order, state: state) - updater.update_shipment_state + updater.recalculate_shipment_state expect(order.shipment_state).to eq(state) end end @@ -248,7 +248,7 @@ module Spree it "is partial" do create(:shipment, order: order, state: 'pending') create(:shipment, order: order, state: 'ready') - updater.update_shipment_state + updater.recalculate_shipment_state expect(order.shipment_state).to eq('partial') end end @@ -361,7 +361,7 @@ module Spree end it "updates shipment state" do - expect(updater).to receive(:update_shipment_state) + expect(updater).to receive(:recalculate_shipment_state) updater.recalculate end @@ -390,7 +390,7 @@ module Spree end it "doesnt update shipment state" do - expect(updater).not_to receive(:update_shipment_state) + expect(updater).not_to receive(:recalculate_shipment_state) updater.recalculate end From 5735a9775d6300147f9cf6c1ea407178bddc3f59 Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Fri, 8 Nov 2024 14:08:41 -0800 Subject: [PATCH 08/20] Rename method that recalculates payment state Update implies that we are persisting the change in Rails, which this method does not do. Co-authored-by: Adam Mueller Co-authored-by: Andrew Stewart Co-authored-by: Benjamin Willems Co-authored-by: Senem Soy Co-authored-by: Sofia Besenski Co-authored-by: Kendra Riga --- .../models/spree/in_memory_order_updater.rb | 8 ++++--- .../spree/in_memory_order_updater_spec.rb | 21 +++++++++---------- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index 00d98ef1f67..e0770ba59f5 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -22,7 +22,7 @@ def recalculate(persist: true) update_shipment_amounts(persist:) update_totals(persist:) if order.completed? - update_payment_state + recalculate_payment_state update_shipments recalculate_shipment_state end @@ -54,7 +54,7 @@ def recalculate_shipment_state alias_method :update_shipment_state, :recalculate_shipment_state deprecate update_shipment_state: :recalculate_shipment_state, deprecator: Spree.deprecator - # Updates the +payment_state+ attribute according to the following logic: + # Recalculates the +payment_state+ attribute according to the following logic: # # paid when +payment_total+ is equal to +total+ # balance_due when +payment_total+ is less than +total+ @@ -63,13 +63,15 @@ def recalculate_shipment_state # void when the order has been canceled and the payment total is 0 # # The +payment_state+ value helps with reporting, etc. since it provides a quick and easy way to locate Orders needing attention. - def update_payment_state + def recalculate_payment_state log_state_change('payment') do order.payment_state = determine_payment_state end order.payment_state end + alias_method :update_payment_state, :recalculate_shipment_state + deprecate update_payment_state: :recalculate_shipment_state, deprecator: Spree.deprecator private diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index 00d2e21dfd2..ebfb8aef1c4 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -255,7 +255,6 @@ module Spree context "updating payment state" do let(:order) { build(:order) } - let(:updater) { order.recalculator } before { allow(order).to receive(:refund_total).and_return(0) } context 'no valid payments with non-zero order total' do @@ -264,7 +263,7 @@ module Spree order.total = 1 order.payment_total = 0 - updater.update_payment_state + updater.recalculate_payment_state expect(order.payment_state).to eq('failed') end end @@ -276,7 +275,7 @@ module Spree order.payment_total = 0 expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'paid' end end @@ -287,7 +286,7 @@ module Spree order.total = 1 expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'credit_owed' end end @@ -298,7 +297,7 @@ module Spree order.total = 2 expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'balance_due' end end @@ -309,7 +308,7 @@ module Spree order.total = 30 expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'paid' end end @@ -324,7 +323,7 @@ module Spree order.payment_total = 0 order.total = 30 expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'void' end end @@ -335,7 +334,7 @@ module Spree order.total = 30 create(:payment, order: order, state: 'completed', amount: 30) expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'credit_owed' end end @@ -345,7 +344,7 @@ module Spree order.payment_total = 0 order.total = 30 expect { - updater.update_payment_state + updater.recalculate_payment_state }.to change { order.payment_state }.to 'void' end end @@ -356,7 +355,7 @@ module Spree before { allow(order).to receive_messages completed?: true } it "updates payment state" do - expect(updater).to receive(:update_payment_state) + expect(updater).to receive(:recalculate_payment_state) updater.recalculate end @@ -385,7 +384,7 @@ module Spree before { allow(order).to receive_messages completed?: false } it "doesnt update payment state" do - expect(updater).not_to receive(:update_payment_state) + expect(updater).not_to receive(:recalculate_payment_state) updater.recalculate end From e45c4a000b0c0ffb919decf9d2cf468399f1ce82 Mon Sep 17 00:00:00 2001 From: Chris Todorov Date: Fri, 8 Nov 2024 14:25:33 -0800 Subject: [PATCH 09/20] Add TODO so we know what to do --- TODO.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 TODO.md diff --git a/TODO.md b/TODO.md new file mode 100644 index 00000000000..7bf3c16ae02 --- /dev/null +++ b/TODO.md @@ -0,0 +1,10 @@ +In-Memory Order Updater TODO +=== + +- [ ] Finish renaming methods that don't persist ever +- [ ] Consider Sofia's recommendation to break this class into POROs to simplify testing +- [ ] Address FIXME on renaming `recalculate_adjustments`? +- [ ] Add test coverage for `update_item_total` when line item totals change +- [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation) +- [ ] Handle persistence in `update_promotions` +- [ ] Handle persistence in `update_taxes` From d31236836efeb36242ddfc4d42aa3d84c1f6df03 Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 13:20:11 -0800 Subject: [PATCH 10/20] Rename update_ private methods These methods don't persist so it's more accurate to say that they recalculate the total instead of saying that they update it. Co-Authored-By: Adam Mueller Co-Authored-By: Benjamin Willems Co-Authored-By: Andrew Stewart Co-Authored-By: Harmony Bouvier Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: Chris Todorov Co-Authored-By: Tom Van Manen Co-Authored-By: Noah Silvera --- .../models/spree/in_memory_order_updater.rb | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index e0770ba59f5..fd8e761f285 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -18,7 +18,7 @@ def initialize(order) # associations try to save and then in turn try to call +update!+ again.) def recalculate(persist: true) order.transaction do - update_item_count + recalculate_item_count update_shipment_amounts(persist:) update_totals(persist:) if order.completed? @@ -130,9 +130,9 @@ def recalculate_adjustments(persist:) # +promo_total+ The total value of all promotion adjustments # +total+ The so-called "order total." This is equivalent to +item_total+ plus +adjustment_total+. def update_totals(persist:) - update_payment_total - update_item_total - update_shipment_total + recalculate_payment_total + recalculate_item_total + recalculate_shipment_total update_adjustment_total(persist:) end @@ -145,16 +145,16 @@ def update_shipments shipments.each(&:update_state) end - def update_payment_total + def recalculate_payment_total order.payment_total = payments.completed.includes(:refunds).sum { |payment| payment.amount - payment.refunds.sum(:amount) } end - def update_shipment_total + def recalculate_shipment_total order.shipment_total = shipments.to_a.sum(&:cost) - update_order_total + recalculate_order_total end - def update_order_total + def recalculate_order_total order.total = order.item_total + order.shipment_total + order.adjustment_total end @@ -168,16 +168,16 @@ def update_adjustment_total(persist:) order.included_tax_total = all_items.sum(&:included_tax_total) + order_tax_adjustments.select(&:included?).sum(&:amount) order.additional_tax_total = all_items.sum(&:additional_tax_total) + order_tax_adjustments.reject(&:included?).sum(&:amount) - update_order_total + recalculate_order_total end - def update_item_count + def recalculate_item_count order.item_count = line_items.to_a.sum(&:quantity) end - def update_item_total + def recalculate_item_total order.item_total = line_items.to_a.sum(&:amount) - update_order_total + recalculate_order_total end def persist_totals From 51bd06c31576407778dec4e9f8dd9eb482c9e634 Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 13:33:05 -0800 Subject: [PATCH 11/20] Rename recalculate_adjustments We want all the methods that might persist data to be called update_ instead of recalculate to be clear that they hit the database. Co-Authored-By: Adam Mueller Co-Authored-By: Benjamin Willems Co-Authored-By: Andrew Stewart Co-Authored-By: Harmony Bouvier Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: Chris Todorov Co-Authored-By: Tom Van Manen Co-Authored-By: Noah Silvera --- core/app/models/spree/in_memory_order_updater.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index fd8e761f285..cf35a2ce80d 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -111,7 +111,7 @@ def determine_shipment_state # fields (promo_total, included_tax_total, additional_tax_total, and # adjustment_total) on the item. # @return [void] - def recalculate_adjustments(persist:) + def update_adjustments(persist:) # Promotion adjustments must be applied first, then tax adjustments. # This fits the criteria for VAT tax as outlined here: # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 @@ -159,7 +159,7 @@ def recalculate_order_total end def update_adjustment_total(persist:) - recalculate_adjustments(persist:) + update_adjustments(persist:) all_items = line_items + shipments order_tax_adjustments = adjustments.select(&:tax?) From c3673d0b0637b720e73b20a696123ac118aeb62b Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 13:35:07 -0800 Subject: [PATCH 12/20] Update TODO --- TODO.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/TODO.md b/TODO.md index 7bf3c16ae02..34e9d98f558 100644 --- a/TODO.md +++ b/TODO.md @@ -1,9 +1,7 @@ In-Memory Order Updater TODO === -- [ ] Finish renaming methods that don't persist ever - [ ] Consider Sofia's recommendation to break this class into POROs to simplify testing -- [ ] Address FIXME on renaming `recalculate_adjustments`? - [ ] Add test coverage for `update_item_total` when line item totals change - [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation) - [ ] Handle persistence in `update_promotions` From 9385968aac701bc3def1531b073694fb3b80aad6 Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 13:40:16 -0800 Subject: [PATCH 13/20] Remove describe block for private method This is calling the recalculate method not update_adjustments. Co-Authored-By: Adam Mueller Co-Authored-By: Benjamin Willems Co-Authored-By: Andrew Stewart Co-Authored-By: Harmony Bouvier Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: Chris Todorov Co-Authored-By: Tom Van Manen Co-Authored-By: Noah Silvera --- .../spree/in_memory_order_updater_spec.rb | 150 +++++++++--------- 1 file changed, 74 insertions(+), 76 deletions(-) diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index ebfb8aef1c4..91ce3451bbb 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -128,94 +128,92 @@ module Spree end end - describe '#recalculate_adjustments ' do - describe 'promotion recalculation' do - it "calls the Promotion Adjustments Recalculator" do - adjuster = double(:call) - expect(Spree::Config.promotions.order_adjuster_class).to receive(:new).and_return(adjuster) - expect(adjuster).to receive(:call) - order.recalculate - end + describe 'promotion recalculation' do + it "calls the Promotion Adjustments Recalculator" do + adjuster = double(:call) + expect(Spree::Config.promotions.order_adjuster_class).to receive(:new).and_return(adjuster) + expect(adjuster).to receive(:call) + order.recalculate end + end - describe 'tax recalculation' do - let!(:ship_address) { create(:address) } - let!(:tax_zone) { create(:global_zone) } # will include the above address - let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_categories: [tax_category]) } - - let(:order) do - create( - :order_with_line_items, - line_items_attributes: [{ price: 10, variant: variant }], - ship_address: ship_address, - ) - end - let(:line_item) { order.line_items[0] } + describe 'tax recalculation' do + let!(:ship_address) { create(:address) } + let!(:tax_zone) { create(:global_zone) } # will include the above address + let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_categories: [tax_category]) } + + let(:order) do + create( + :order_with_line_items, + line_items_attributes: [{ price: 10, variant: variant }], + ship_address: ship_address, + ) + end + let(:line_item) { order.line_items[0] } - let(:variant) { create(:variant, tax_category: tax_category) } - let(:tax_category) { create(:tax_category) } + let(:variant) { create(:variant, tax_category: tax_category) } + let(:tax_category) { create(:tax_category) } - context 'when the item quantity has changed' do - before do - line_item.update!(quantity: 2) - end + context 'when the item quantity has changed' do + before do + line_item.update!(quantity: 2) + end - it 'updates the promotion amount' do - expect { - order.recalculate - }.to change { - line_item.additional_tax_total - }.from(1).to(2) - end + it 'updates the promotion amount' do + expect { + order.recalculate + }.to change { + line_item.additional_tax_total + }.from(1).to(2) end + end - context 'with a custom tax_calculator_class' do - let(:custom_calculator_class) { double } - let(:custom_calculator_instance) { double } + context 'with a custom tax_calculator_class' do + let(:custom_calculator_class) { double } + let(:custom_calculator_instance) { double } - before do - order # generate this first so we can expect it - stub_spree_preferences(tax_calculator_class: custom_calculator_class) - - allow(custom_calculator_class).to receive(:new).and_return(custom_calculator_instance) - allow(custom_calculator_instance).to receive(:calculate).and_return( - Spree::Tax::OrderTax.new( - order_id: order.id, - order_taxes: [ - Spree::Tax::ItemTax.new( - label: "Delivery Fee", - tax_rate: tax_rate, - amount: 2.60, - included_in_price: false - ) - ], - line_item_taxes: [ - Spree::Tax::ItemTax.new( - item_id: line_item.id, - label: "Item Tax", - tax_rate: tax_rate, - amount: 1.40, - included_in_price: false - ) - ], - shipment_taxes: [] - ) + before do + order # generate this first so we can expect it + stub_spree_preferences(tax_calculator_class: custom_calculator_class) + + allow(custom_calculator_class).to receive(:new).and_return(custom_calculator_instance) + allow(custom_calculator_instance).to receive(:calculate).and_return( + Spree::Tax::OrderTax.new( + order_id: order.id, + order_taxes: [ + Spree::Tax::ItemTax.new( + label: "Delivery Fee", + tax_rate: tax_rate, + amount: 2.60, + included_in_price: false + ) + ], + line_item_taxes: [ + Spree::Tax::ItemTax.new( + item_id: line_item.id, + label: "Item Tax", + tax_rate: tax_rate, + amount: 1.40, + included_in_price: false + ) + ], + shipment_taxes: [] ) - end + ) + end - it 'uses the configured class' do - order.recalculate + it 'uses the configured class' do + order.recalculate - expect(custom_calculator_class).to have_received(:new).with(order).at_least(:once) - expect(custom_calculator_instance).to have_received(:calculate).at_least(:once) - end + expect(custom_calculator_class).to have_received(:new).with(order).at_least(:once) + expect(custom_calculator_instance).to have_received(:calculate).at_least(:once) + end - it 'updates the aggregate columns' do - expect { - order.recalculate - }.to change { order.reload.additional_tax_total }.to(4.00) - .and change { order.reload.adjustment_total }.to(4.00) - end + it 'updates the aggregate columns' do + expect { + order.recalculate + }.to change { order.reload.additional_tax_total }.to(4.00) + .and change { order.reload.adjustment_total }.to(4.00) end end end From 0b8e26d95bede1ee972ec8cb7b74a84ac0d14542 Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 13:51:24 -0800 Subject: [PATCH 14/20] Reorder private methods This puts all the update and recalculate methods together. Co-Authored-By: Adam Mueller Co-Authored-By: Benjamin Willems Co-Authored-By: Andrew Stewart Co-Authored-By: Harmony Bouvier Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: Chris Todorov Co-Authored-By: Tom Van Manen Co-Authored-By: Noah Silvera --- .../models/spree/in_memory_order_updater.rb | 92 +++++++++---------- 1 file changed, 46 insertions(+), 46 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index cf35a2ce80d..ed2bf81430c 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -140,24 +140,6 @@ def update_shipment_amounts(persist:) shipments.each { _1.update_amounts(persist:) } end - # give each of the shipments a chance to update themselves - def update_shipments - shipments.each(&:update_state) - end - - def recalculate_payment_total - order.payment_total = payments.completed.includes(:refunds).sum { |payment| payment.amount - payment.refunds.sum(:amount) } - end - - def recalculate_shipment_total - order.shipment_total = shipments.to_a.sum(&:cost) - recalculate_order_total - end - - def recalculate_order_total - order.total = order.item_total + order.shipment_total + order.adjustment_total - end - def update_adjustment_total(persist:) update_adjustments(persist:) @@ -171,34 +153,6 @@ def update_adjustment_total(persist:) recalculate_order_total end - def recalculate_item_count - order.item_count = line_items.to_a.sum(&:quantity) - end - - def recalculate_item_total - order.item_total = line_items.to_a.sum(&:amount) - recalculate_order_total - end - - def persist_totals - order.save! - end - - def log_state_change(name) - state = "#{name}_state" - old_state = order.public_send(state) - yield - new_state = order.public_send(state) - if old_state != new_state - order.state_changes.new( - previous_state: old_state, - next_state: new_state, - name: name, - user_id: order.user_id - ) - end - end - def update_promotions Spree::Config.promotions.order_adjuster_class.new(order).call end @@ -243,5 +197,51 @@ def update_item_totals(persist:) end end end + + # give each of the shipments a chance to update themselves + def update_shipments + shipments.each(&:update_state) + end + + def recalculate_payment_total + order.payment_total = payments.completed.includes(:refunds).sum { |payment| payment.amount - payment.refunds.sum(:amount) } + end + + def recalculate_shipment_total + order.shipment_total = shipments.to_a.sum(&:cost) + recalculate_order_total + end + + def recalculate_order_total + order.total = order.item_total + order.shipment_total + order.adjustment_total + end + + def recalculate_item_count + order.item_count = line_items.to_a.sum(&:quantity) + end + + def recalculate_item_total + order.item_total = line_items.to_a.sum(&:amount) + recalculate_order_total + end + + def persist_totals + order.save! + end + + def log_state_change(name) + state = "#{name}_state" + old_state = order.public_send(state) + yield + new_state = order.public_send(state) + if old_state != new_state + order.state_changes.new( + previous_state: old_state, + next_state: new_state, + name: name, + user_id: order.user_id + ) + end + end end end From fbf96ffec7ad18d3dbf6a7a26564b684082c345d Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 14:22:01 -0800 Subject: [PATCH 15/20] Pull out the item total updater We want to start breaking out some of the complex logic of the in memory updater into smaller more focused classes. Co-Authored-By: Adam Mueller Co-Authored-By: Benjamin Willems Co-Authored-By: Andrew Stewart Co-Authored-By: Harmony Bouvier Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: Chris Todorov Co-Authored-By: Tom Van Manen Co-Authored-By: Noah Silvera --- .../models/spree/in_memory_order_updater.rb | 6 +----- core/app/models/spree/item_total_updater.rb | 9 +++++++++ .../models/spree/item_total_updater_spec.rb | 18 ++++++++++++++++++ 3 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 core/app/models/spree/item_total_updater.rb create mode 100644 core/spec/models/spree/item_total_updater_spec.rb diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index ed2bf81430c..fc59a8a29f1 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -180,11 +180,7 @@ def update_cancellations def update_item_totals(persist:) [*line_items, *shipments].each do |item| - # The cancellation_total isn't persisted anywhere but is included in - # the adjustment_total - item.adjustment_total = item.adjustments. - reject(&:included?). - sum(&:amount) + Spree::ItemTotalUpdater.recalculate(item) if persist && item.changed? item.update_columns( diff --git a/core/app/models/spree/item_total_updater.rb b/core/app/models/spree/item_total_updater.rb new file mode 100644 index 00000000000..c062f7a1ac9 --- /dev/null +++ b/core/app/models/spree/item_total_updater.rb @@ -0,0 +1,9 @@ +class Spree::ItemTotalUpdater + class << self + def recalculate(item) + item.adjustment_total = item.adjustments + .reject(&:included?) + .sum(&:amount) + end + end +end diff --git a/core/spec/models/spree/item_total_updater_spec.rb b/core/spec/models/spree/item_total_updater_spec.rb new file mode 100644 index 00000000000..180772e778c --- /dev/null +++ b/core/spec/models/spree/item_total_updater_spec.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::ItemTotalUpdater do + describe ".recalculate" do + subject { described_class.recalculate(item) } + + let(:item) { create :line_item, adjustments: [adjustment] } + let(:adjustment) { create :adjustment, amount: 1} + + it "sets the adjustment total on the item" do + expect { subject } + .to change { item.adjustment_total } + .from(0).to(1) + end + end +end From 06e3a2ad0785c710ccfabfe2a8d97aa3782a4130 Mon Sep 17 00:00:00 2001 From: Alistair Norman Date: Fri, 22 Nov 2024 14:25:22 -0800 Subject: [PATCH 16/20] Add spec creation to todo --- TODO.md | 1 + 1 file changed, 1 insertion(+) diff --git a/TODO.md b/TODO.md index 34e9d98f558..5dcf61e09bb 100644 --- a/TODO.md +++ b/TODO.md @@ -1,6 +1,7 @@ In-Memory Order Updater TODO === +- [ ] Add additional cases to item_total_updater_spec (doesn't currently account for included adjustments) - [ ] Consider Sofia's recommendation to break this class into POROs to simplify testing - [ ] Add test coverage for `update_item_total` when line item totals change - [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation) From 7e760702f2a3d726607950c1e938d6d3dabcf2b8 Mon Sep 17 00:00:00 2001 From: Sofia Besenski Date: Fri, 6 Dec 2024 14:24:19 -0800 Subject: [PATCH 17/20] Introduce new InMemoryOrderAdjuster class for promotions This is just a stub for now, but we want to eventually introduce a class to handle running the promotion adjustments in memory. Co-authored-by: Adam Mueller Co-authored-by: Andrew Stewart Co-authored-by: Harmony Evangelina Co-authored-by: Kendra Riga Co-authored-by: Jared Norman Co-authored-by: Tom Van Manen Co-authored-by: Senem Soy Co-authored-by: Benjamin Willems --- .../models/spree/in_memory_order_updater.rb | 18 +++++++++++++++--- .../spree/in_memory_order_updater_spec.rb | 11 +++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index fc59a8a29f1..9624bff84d9 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -117,7 +117,7 @@ def update_adjustments(persist:) # http://www.hmrc.gov.uk/vat/managing/charging/discounts-etc.htm#1 # It also fits the criteria for sales tax as outlined here: # http://www.boe.ca.gov/formspubs/pub113/ - update_promotions + update_promotions(persist:) update_taxes update_item_totals(persist:) end @@ -153,8 +153,12 @@ def update_adjustment_total(persist:) recalculate_order_total end - def update_promotions - Spree::Config.promotions.order_adjuster_class.new(order).call + def update_promotions(persist:) + if persist + Spree::Config.promotions.order_adjuster_class + else + InMemoryOrderAdjuster + end.new(order).call end def update_taxes @@ -239,5 +243,13 @@ def log_state_change(name) ) end end + + class InMemoryOrderAdjuster + def initialize(order) + end + + def call + end + end end end diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index 91ce3451bbb..9794ca8995f 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -166,6 +166,17 @@ module Spree line_item.additional_tax_total }.from(1).to(2) end + + context "when recalculating the order in memory" do + it "raises an error" do + order_adjuster = double + allow(order_adjuster).to receive(:call) { raise NotImplementedError } + allow(Spree::InMemoryOrderUpdater::InMemoryOrderAdjuster).to receive(:new).and_return(order_adjuster) + + expect{described_class.new(order).recalculate(persist: false)} + .to raise_error(NotImplementedError) + end + end end context 'with a custom tax_calculator_class' do From 27e49885f308f5f728e3037c7c0ba3ac3bc73f20 Mon Sep 17 00:00:00 2001 From: Andrew Stewart Date: Fri, 20 Dec 2024 13:09:38 -0800 Subject: [PATCH 18/20] Fixup linter errors in InMemoryOrderUpdater Co-Authored-By: Jared Norman Co-Authored-By: benjamin wil Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski --- core/app/models/spree/in_memory_order_updater.rb | 8 +++++--- core/app/models/spree/item_total_updater.rb | 2 ++ core/spec/models/spree/in_memory_order_updater_spec.rb | 2 +- core/spec/models/spree/item_total_updater_spec.rb | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index 9624bff84d9..b91f0e30376 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -3,6 +3,7 @@ module Spree class InMemoryOrderUpdater attr_reader :order + delegate :payments, :line_items, :adjustments, :all_adjustments, :shipments, :quantity, to: :order def initialize(order) @@ -26,7 +27,8 @@ def recalculate(persist: true) update_shipments recalculate_shipment_state end - Spree::Bus.publish :order_recalculated, order: order + + Spree::Bus.publish(:order_recalculated, order:) persist_totals if persist end @@ -238,8 +240,8 @@ def log_state_change(name) order.state_changes.new( previous_state: old_state, next_state: new_state, - name: name, - user_id: order.user_id + user_id: order.user_id, + name: ) end end diff --git a/core/app/models/spree/item_total_updater.rb b/core/app/models/spree/item_total_updater.rb index c062f7a1ac9..97d20843c6e 100644 --- a/core/app/models/spree/item_total_updater.rb +++ b/core/app/models/spree/item_total_updater.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + class Spree::ItemTotalUpdater class << self def recalculate(item) diff --git a/core/spec/models/spree/in_memory_order_updater_spec.rb b/core/spec/models/spree/in_memory_order_updater_spec.rb index 9794ca8995f..f754d603301 100644 --- a/core/spec/models/spree/in_memory_order_updater_spec.rb +++ b/core/spec/models/spree/in_memory_order_updater_spec.rb @@ -173,7 +173,7 @@ module Spree allow(order_adjuster).to receive(:call) { raise NotImplementedError } allow(Spree::InMemoryOrderUpdater::InMemoryOrderAdjuster).to receive(:new).and_return(order_adjuster) - expect{described_class.new(order).recalculate(persist: false)} + expect{ described_class.new(order).recalculate(persist: false) } .to raise_error(NotImplementedError) end end diff --git a/core/spec/models/spree/item_total_updater_spec.rb b/core/spec/models/spree/item_total_updater_spec.rb index 180772e778c..092e17138e0 100644 --- a/core/spec/models/spree/item_total_updater_spec.rb +++ b/core/spec/models/spree/item_total_updater_spec.rb @@ -7,7 +7,7 @@ subject { described_class.recalculate(item) } let(:item) { create :line_item, adjustments: [adjustment] } - let(:adjustment) { create :adjustment, amount: 1} + let(:adjustment) { create :adjustment, amount: 1 } it "sets the adjustment total on the item" do expect { subject } From 1593c06bbea4a37973c266df506049bb218ef089 Mon Sep 17 00:00:00 2001 From: Andrew Stewart Date: Fri, 20 Dec 2024 13:34:06 -0800 Subject: [PATCH 19/20] Rename Spree::ItemTotalUpdater to Spree::ItemTotal Co-Authored-By: Harmony Evangelina Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: benjamin wil --- core/app/models/spree/item_total.rb | 15 ++++++++++++ core/app/models/spree/item_total_updater.rb | 11 --------- core/spec/models/spree/item_total_spec.rb | 23 +++++++++++++++++++ .../models/spree/item_total_updater_spec.rb | 18 --------------- 4 files changed, 38 insertions(+), 29 deletions(-) create mode 100644 core/app/models/spree/item_total.rb delete mode 100644 core/app/models/spree/item_total_updater.rb create mode 100644 core/spec/models/spree/item_total_spec.rb delete mode 100644 core/spec/models/spree/item_total_updater_spec.rb diff --git a/core/app/models/spree/item_total.rb b/core/app/models/spree/item_total.rb new file mode 100644 index 00000000000..8d13ccbeacf --- /dev/null +++ b/core/app/models/spree/item_total.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class Spree::ItemTotal + def initialize(item) + @item = item + end + + def recalculate! + item.adjustment_total = item.adjustments.reject(&:included?).sum(&:amount) + end + + private + + attr_reader :item +end diff --git a/core/app/models/spree/item_total_updater.rb b/core/app/models/spree/item_total_updater.rb deleted file mode 100644 index 97d20843c6e..00000000000 --- a/core/app/models/spree/item_total_updater.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -class Spree::ItemTotalUpdater - class << self - def recalculate(item) - item.adjustment_total = item.adjustments - .reject(&:included?) - .sum(&:amount) - end - end -end diff --git a/core/spec/models/spree/item_total_spec.rb b/core/spec/models/spree/item_total_spec.rb new file mode 100644 index 00000000000..9d71cf9352e --- /dev/null +++ b/core/spec/models/spree/item_total_spec.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Spree::ItemTotal do + describe "#recalculate!" do + subject { described_class.new(item).recalculate! } + + let(:item) { create :line_item, adjustments: [adjustment] } + let(:adjustment) { create :adjustment, amount: 19.99 } + + it "sets the adjustment total on the item" do + expect { subject } + .to change { item.adjustment_total } + .from(0).to(19.99) + end + + it "does not factor in included adjustments" do + adjustment.update!(included: true) + expect { subject }.not_to change { item.adjustment_total }.from(0) + end + end +end diff --git a/core/spec/models/spree/item_total_updater_spec.rb b/core/spec/models/spree/item_total_updater_spec.rb deleted file mode 100644 index 092e17138e0..00000000000 --- a/core/spec/models/spree/item_total_updater_spec.rb +++ /dev/null @@ -1,18 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe Spree::ItemTotalUpdater do - describe ".recalculate" do - subject { described_class.recalculate(item) } - - let(:item) { create :line_item, adjustments: [adjustment] } - let(:adjustment) { create :adjustment, amount: 1 } - - it "sets the adjustment total on the item" do - expect { subject } - .to change { item.adjustment_total } - .from(0).to(1) - end - end -end From 4a77350fb9e71268a465b041cccb825f8cacf678 Mon Sep 17 00:00:00 2001 From: Andrew Stewart Date: Fri, 20 Dec 2024 13:34:06 -0800 Subject: [PATCH 20/20] Refactor item-total recalculation into Spree::ItemTotal This change more clearly splits up the logic for recalculating shipment/item adjustment totals from the logic for persisting updated values to the database. Continuing to colocating logic for recalculating item totals in one PORO makes it easier to reason about and helps to simplify the higher-level order updater. Co-Authored-By: Harmony Evangelina Co-Authored-By: Jared Norman Co-Authored-By: Kendra Riga Co-Authored-By: Sofia Besenski Co-Authored-By: benjamin wil --- TODO.md | 2 +- .../models/spree/in_memory_order_updater.rb | 49 ++++++++----------- core/app/models/spree/item_total.rb | 9 ++++ core/spec/models/spree/item_total_spec.rb | 46 +++++++++++++---- 4 files changed, 68 insertions(+), 38 deletions(-) diff --git a/TODO.md b/TODO.md index 5dcf61e09bb..f83e2e289b9 100644 --- a/TODO.md +++ b/TODO.md @@ -1,7 +1,7 @@ In-Memory Order Updater TODO === -- [ ] Add additional cases to item_total_updater_spec (doesn't currently account for included adjustments) +- [x] Add additional cases to item_total_updater_spec (doesn't currently account for included adjustments) - [ ] Consider Sofia's recommendation to break this class into POROs to simplify testing - [ ] Add test coverage for `update_item_total` when line item totals change - [ ] Test coverage to ensure state changes aren't persisted (if someone changes current implementation) diff --git a/core/app/models/spree/in_memory_order_updater.rb b/core/app/models/spree/in_memory_order_updater.rb index b91f0e30376..963257bdd1c 100644 --- a/core/app/models/spree/in_memory_order_updater.rb +++ b/core/app/models/spree/in_memory_order_updater.rb @@ -120,8 +120,9 @@ def update_adjustments(persist:) # It also fits the criteria for sales tax as outlined here: # http://www.boe.ca.gov/formspubs/pub113/ update_promotions(persist:) - update_taxes - update_item_totals(persist:) + update_tax_adjustments + recalculate_item_totals + persist_changes_to_items if persist end # Updates the following Order total values: @@ -163,40 +164,32 @@ def update_promotions(persist:) end.new(order).call end - def update_taxes + # TODO: split implementation based on 'persist' + def update_tax_adjustments Spree::Config.tax_adjuster_class.new(order).adjust! - - [*line_items, *shipments].each do |item| - tax_adjustments = item.adjustments.select(&:tax?) - # Tax adjustments come in not one but *two* exciting flavours: - # Included & additional - - # Included tax adjustments are those which are included in the price. - # These ones should not affect the eventual total price. - # - # Additional tax adjustments are the opposite, affecting the final total. - item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) - item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) - end end def update_cancellations end deprecate :update_cancellations, deprecator: Spree.deprecator - def update_item_totals(persist:) + def recalculate_item_totals [*line_items, *shipments].each do |item| - Spree::ItemTotalUpdater.recalculate(item) - - if persist && 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 + Spree::ItemTotal.new(item).recalculate! + end + end + + def persist_item_changes + [*line_items, *shipments].each do |item| + next unless 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 end diff --git a/core/app/models/spree/item_total.rb b/core/app/models/spree/item_total.rb index 8d13ccbeacf..3ef666e39e8 100644 --- a/core/app/models/spree/item_total.rb +++ b/core/app/models/spree/item_total.rb @@ -6,6 +6,15 @@ def initialize(item) end def recalculate! + tax_adjustments = item.adjustments.select(&:tax?) + + # Included tax adjustments are those which are included in the price. + # These ones should not affect the eventual total price. + # + # Additional tax adjustments are the opposite, affecting the final total. + item.included_tax_total = tax_adjustments.select(&:included?).sum(&:amount) + item.additional_tax_total = tax_adjustments.reject(&:included?).sum(&:amount) + item.adjustment_total = item.adjustments.reject(&:included?).sum(&:amount) end diff --git a/core/spec/models/spree/item_total_spec.rb b/core/spec/models/spree/item_total_spec.rb index 9d71cf9352e..1f6a2433a64 100644 --- a/core/spec/models/spree/item_total_spec.rb +++ b/core/spec/models/spree/item_total_spec.rb @@ -6,18 +6,46 @@ describe "#recalculate!" do subject { described_class.new(item).recalculate! } - let(:item) { create :line_item, adjustments: [adjustment] } - let(:adjustment) { create :adjustment, amount: 19.99 } + let(:item) { create :line_item, adjustments: } - it "sets the adjustment total on the item" do - expect { subject } - .to change { item.adjustment_total } - .from(0).to(19.99) + let(:tax_rate) { create(:tax_rate) } + + let(:arbitrary_adjustment) { create :adjustment, amount: 1, source: nil } + let(:included_tax_adjustment) { create :adjustment, amount: 2, source: tax_rate, included: true } + let(:additional_tax_adjustment) { create :adjustment, amount: 3, source: tax_rate, included: false } + + context "with multiple types of adjustments" do + let(:adjustments) { [arbitrary_adjustment, included_tax_adjustment, additional_tax_adjustment] } + + it "updates item totals" do + expect { + subject + }.to change(item, :adjustment_total).from(0).to(4). + and change { item.included_tax_total }.from(0).to(2). + and change { item.additional_tax_total }.from(0).to(3) + end end - it "does not factor in included adjustments" do - adjustment.update!(included: true) - expect { subject }.not_to change { item.adjustment_total }.from(0) + context "with only an arbitrary adjustment" do + let(:adjustments) { [arbitrary_adjustment] } + + it "updates the adjustment total" do + expect { + subject + }.to change { item.adjustment_total }.from(0).to(1) + end + end + + context "with only tax adjustments" do + let(:adjustments) { [included_tax_adjustment, additional_tax_adjustment] } + + it "updates the adjustment total" do + expect { + subject + }.to change { item.adjustment_total }.from(0).to(3). + and change { item.included_tax_total }.from(0).to(2). + and change { item.additional_tax_total }.from(0).to(3) + end end end end