From 20df5c23e809d688ed1039ac7364609c9b71dd75 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 30 Oct 2024 14:01:04 +1100 Subject: [PATCH 01/10] Remove before save callback to update shipping fees It should be handled in the controller, it's currently handled in `Spree::OrderContents#remove`. As long as we don't manually remove line item from an order we should be good. Currently it gets trigerred each time the order is saved, which seems to happen mutiple time when we finalize an order. It's a bit useless to recalculated the fees over and over Context, it was added here : https://github.com/openfoodfoundation/openfoodnetwork/commit/217eda8362d30f64d5bd16e360a8361f3309be27 --- app/models/spree/order.rb | 1 - spec/models/spree/order_spec.rb | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 81bee889d3..a365693d0e 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -106,7 +106,6 @@ module Spree before_validation :clone_billing_address, if: :use_billing? before_validation :ensure_customer - before_save :update_shipping_fees!, if: :complete? before_save :update_payment_fees!, if: :complete? before_create :link_by_email diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 00a0cdb621..d7b226aed5 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1239,7 +1239,7 @@ RSpec.describe Spree::Order do expect(order.shipment.included_tax_total).to eq 1.2 end - context "removing line_items" do + xcontext "removing line_items" do it "updates shipping and transaction fees" do order.line_items.first.update_attribute(:quantity, 0) order.save From f9bd72034143373cbce7b2fc9bb6c8f2917132a1 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 10 Dec 2024 12:16:37 +1100 Subject: [PATCH 02/10] Add spec for #update_shipping_fees! And update related specs --- spec/models/spree/order_spec.rb | 104 ++++++++++++++++---------------- 1 file changed, 51 insertions(+), 53 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index d7b226aed5..e70458b759 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1211,76 +1211,73 @@ RSpec.describe Spree::Order do end end - describe "a completed order with shipping and transaction fees" do - let(:distributor) { create(:distributor_enterprise_with_tax) } - let(:zone) { create(:zone_with_member) } - let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25, included_in_price: true, zone:) } - let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } + describe "#update_shipping_fees!" do + let(:distributor) { create(:distributor_enterprise) } let(:order) { - create(:completed_order_with_fees, distributor:, shipping_fee:, - payment_fee:, - shipping_tax_category:) + create(:completed_order_with_fees, distributor:, shipping_fee:, payment_fee: 0) + } + let(:shipping_fee) { 5 } + + it "does nothing if shipment is shipped" do + # An order need to be paid before we can ship a shipment + create(:payment, amount: order.total, order:, state: "completed") + + shipment = order.shipments.first + shipment.ship + + expect(shipment).not_to receive(:save) + + order.update_shipping_fees! + end + + it "saves the each shipment" do + order.shipments << create(:shipment, order:) + order.shipments.each do |shipment| + expect(shipment).to receive(:save) + end + + order.update_shipping_fees! + end + + context "with shipment including a shipping fee" do + it "updates shipping fee" do + # Manually udate line item quantity, in a normal scenario we would use + # order.contents method, which takes care of updating shipments + order.line_items.first.update(quantity: 2) + + order.update_shipping_fees! + + item_num = order.line_items.sum(&:quantity) + expect(order.reload.adjustment_total).to eq(item_num * shipping_fee) + end + end + end + + describe "a completed order with transaction fees" do + let(:distributor) { create(:distributor_enterprise_with_tax) } + let(:order) { + create(:completed_order_with_fees, distributor:, shipping_fee: 0, payment_fee:) } - let(:shipping_fee) { 3 } let(:payment_fee) { 5 } let(:item_num) { order.line_items.length } - let(:expected_fees) { item_num * (shipping_fee + payment_fee) } + let(:expected_fees) { item_num * payment_fee } before do order.reload order.create_tax_charge! # Sanity check the fees - expect(order.all_adjustments.length).to eq 3 - expect(order.shipment_adjustments.length).to eq 2 + expect(order.all_adjustments.length).to eq 2 expect(item_num).to eq 2 expect(order.adjustment_total).to eq expected_fees - expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 1.2 - expect(order.shipment.included_tax_total).to eq 1.2 end - xcontext "removing line_items" do - it "updates shipping and transaction fees" do + context "removing line_items" do + it "updates transaction fees" do order.line_items.first.update_attribute(:quantity, 0) order.save - expect(order.adjustment_total).to eq expected_fees - shipping_fee - payment_fee - expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 0.6 - expect(order.shipment.included_tax_total).to eq 0.6 - end - - context "when finalized fee adjustments exist on the order" do - before do - order.all_adjustments.each(&:finalize!) - order.reload - end - - it "does not attempt to update such adjustments" do - order.update(line_items_attributes: [{ id: order.line_items.first.id, quantity: 0 }]) - - # Check if fees got updated - order.reload - - expect(order.adjustment_total).to eq expected_fees - expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 1.2 - expect(order.shipment.included_tax_total).to eq 1.2 - end - end - end - - context "changing the shipping method to one without fees" do - let(:shipping_method) { - create(:shipping_method, calculator: Calculator::FlatRate.new(preferred_amount: 0)) - } - - it "updates shipping fees" do - order.shipments = [create(:shipment_with, :shipping_method, - shipping_method:)] - order.save - - expect(order.adjustment_total).to eq expected_fees - (item_num * shipping_fee) - expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 0 - expect(order.shipment.included_tax_total).to eq 0 + expect(order.adjustment_total).to eq expected_fees - payment_fee end end @@ -1296,6 +1293,7 @@ RSpec.describe Spree::Order do # Check if fees got updated order.reload + expect(order.adjustment_total).to eq expected_fees - (item_num * payment_fee) end end From 9e7e40a5a8b47948dbeb0d58588a057bcb205f25 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 10 Dec 2024 12:17:37 +1100 Subject: [PATCH 03/10] Update spec to properly test shipping fee update --- spec/models/spree/order_contents_spec.rb | 82 ++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/spec/models/spree/order_contents_spec.rb b/spec/models/spree/order_contents_spec.rb index d3a55fcf8b..19788fbeb8 100644 --- a/spec/models/spree/order_contents_spec.rb +++ b/spec/models/spree/order_contents_spec.rb @@ -38,6 +38,27 @@ RSpec.describe Spree::OrderContents do expect(order.item_total.to_f).to eq 19.99 expect(order.total.to_f).to eq 19.99 end + + context "with a completed order" do + let!(:order) { create(:completed_order_with_totals) } + + it "updates shipping fees" do + expect(order).to receive(:update_shipping_fees!) + + subject.add(variant, 1) + end + end + + context "when passing a shipment" do + let!(:order) { create(:order_with_line_items) } + + it "updates shipping fees" do + shipment = order.shipments.first + expect(order).to receive(:update_shipping_fees!) + + subject.add(variant, 1, shipment) + end + end end context "#remove" do @@ -86,6 +107,27 @@ RSpec.describe Spree::OrderContents do expect(order.item_total.to_f).to eq 19.99 expect(order.total.to_f).to eq 19.99 end + + context "with a completed order" do + let!(:order) { create(:completed_order_with_totals) } + + it "updates shipping fees" do + expect(order).to receive(:update_shipping_fees!) + + subject.remove(order.line_items.first.variant, 1) + end + end + + context "when passing a shipment" do + let!(:order) { create(:order_with_line_items) } + + it "updates shipping fees" do + shipment = order.reload.shipments.first + expect(order).to receive(:update_shipping_fees!) + + subject.remove(order.line_items.first.variant, 1, shipment) + end + end end context "#update_cart" do @@ -126,6 +168,16 @@ RSpec.describe Spree::OrderContents do expect(subject.order).to receive(:ensure_updated_shipments) subject.update_cart params end + + context "with a completed order" do + let!(:order) { create(:completed_order_with_totals) } + + it "updates shipping fees" do + expect(order).to receive(:update_shipping_fees!) + + subject.update_cart params + end + end end describe "#update_item" do @@ -163,6 +215,16 @@ RSpec.describe Spree::OrderContents do subject.update_item(line_item, { quantity: 3 }) end + + context "with a completed order" do + let!(:order) { create(:completed_order_with_totals) } + + it "updates shipping fees" do + expect(order).to receive(:update_shipping_fees!) + + subject.update_item(order.line_items.first, { quantity: 3 }) + end + end end describe "#update_or_create" do @@ -181,6 +243,16 @@ RSpec.describe Spree::OrderContents do subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) end + + context "with completed order" do + let!(:order) { create(:completed_order_with_totals) } + + it "updates shipping fees" do + expect(order).to receive(:update_shipping_fees!) + + subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) + end + end end describe "updating" do @@ -198,6 +270,16 @@ RSpec.describe Spree::OrderContents do subject.update_or_create(variant, { quantity: 3, max_quantity: 4 }) end + + context "with completed order" do + let!(:order) { create(:completed_order_with_totals) } + + it "updates shipping fees" do + expect(order).to receive(:update_shipping_fees!) + + subject.update_or_create(variant, { quantity: 3, max_quantity: 4 }) + end + end end end end From a8d1d0c591fae73a4d2933f16a723f975fcfda6e Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 10 Dec 2024 12:18:52 +1100 Subject: [PATCH 04/10] Update spec to properly update line items on an order User Order::Contents#update_item to update line item on an order, it ensures the order is properly updated --- spec/system/admin/orders_spec.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index fd306cebbc..29a60f319c 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -452,14 +452,15 @@ RSpec.describe ' context "orders with different order totals" do before do - Spree::LineItem.where(order_id: order2.id).first.update!(quantity: 5) - Spree::LineItem.where(order_id: order3.id).first.update!(quantity: 4) - Spree::LineItem.where(order_id: order4.id).first.update!(quantity: 3) - Spree::LineItem.where(order_id: order5.id).first.update!(quantity: 2) - order2.save - order3.save - order4.save - order5.save + order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first, + { quantity: 5 }) + order3.contents.update_item(Spree::LineItem.where(order_id: order3.id).first, + { quantity: 4 }) + order4.contents.update_item(Spree::LineItem.where(order_id: order4.id).first, + { quantity: 3 }) + order5.contents.update_item(Spree::LineItem.where(order_id: order5.id).first, + { quantity: 2 }) + login_as_admin visit spree.admin_orders_path end From 1afa7fe5c0594b1e0b8e8c48cceef4e712892fe4 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 11 Dec 2024 09:29:32 +1100 Subject: [PATCH 05/10] Per review, small improvment --- spec/system/admin/orders_spec.rb | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/spec/system/admin/orders_spec.rb b/spec/system/admin/orders_spec.rb index 29a60f319c..e672166a6c 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -452,14 +452,10 @@ RSpec.describe ' context "orders with different order totals" do before do - order2.contents.update_item(Spree::LineItem.where(order_id: order2.id).first, - { quantity: 5 }) - order3.contents.update_item(Spree::LineItem.where(order_id: order3.id).first, - { quantity: 4 }) - order4.contents.update_item(Spree::LineItem.where(order_id: order4.id).first, - { quantity: 3 }) - order5.contents.update_item(Spree::LineItem.where(order_id: order5.id).first, - { quantity: 2 }) + order2.contents.update_item(Spree::LineItem.find_by(order_id: order2.id), { quantity: 5 }) + order3.contents.update_item(Spree::LineItem.find_by(order_id: order3.id), { quantity: 4 }) + order4.contents.update_item(Spree::LineItem.find_by(order_id: order4.id), { quantity: 3 }) + order5.contents.update_item(Spree::LineItem.find_by(order_id: order5.id), { quantity: 2 }) login_as_admin visit spree.admin_orders_path From c71ae35685e563389480f526ef71018c0cf3530f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 20 Jan 2025 16:07:32 +1100 Subject: [PATCH 06/10] Fix payment_total calculation For payment that complete during the checkout (Paypal, Stripe) the amount was recorded twice against `order.payment_total`. This is because the `payment_total` gets updated in an afer_save Payment callback when a payment is completed, and then once more when we process payment in `Spree::Order#process_each_payment`. This is an existing issue on master, but it was hidden by the `update_shipping_fees!` callback, it trigerred an update of the order's total, which then updated `order.payment_total` with the correct value. Now that we removed the callback, the bug showed up. Note, I updated the stripe specs for consistency even though they are currently disabled. --- app/models/spree/order.rb | 4 ---- app/views/spree/orders/_totals_footer.html.haml | 2 +- spec/models/spree/order_spec.rb | 9 --------- spec/system/consumer/shopping/checkout_paypal_spec.rb | 1 + spec/system/consumer/shopping/checkout_stripe_spec.rb | 2 ++ 5 files changed, 4 insertions(+), 14 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index a365693d0e..17ffe51700 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -673,10 +673,6 @@ module Spree break if payment_total >= total yield payment - - if payment.completed? - self.payment_total += payment.amount - end end end diff --git a/app/views/spree/orders/_totals_footer.html.haml b/app/views/spree/orders/_totals_footer.html.haml index 7319869eb9..223524e676 100644 --- a/app/views/spree/orders/_totals_footer.html.haml +++ b/app/views/spree/orders/_totals_footer.html.haml @@ -29,7 +29,7 @@ %td.text-right{colspan: "3"} %strong = t :order_amount_paid - %td.text-right.total + %td.text-right.total{id: "amount-paid"} %strong = order.display_payment_total.to_html - if order.outstanding_balance.positive? diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index e70458b759..f10c1f4f80 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -294,15 +294,6 @@ RSpec.describe Spree::Order do expect(payment).to receive(:process!) expect(order.process_payments!).to be_truthy end - - it "stores the payment total on the order" do - allow(payment).to receive(:process!) - allow(payment).to receive(:completed?).and_return(true) - - order.process_payments! - - expect(order.payment_total).to eq(payment.amount) - end end context "when a payment raises a GatewayError" do diff --git a/spec/system/consumer/shopping/checkout_paypal_spec.rb b/spec/system/consumer/shopping/checkout_paypal_spec.rb index 98947d099c..93f2fdbabe 100644 --- a/spec/system/consumer/shopping/checkout_paypal_spec.rb +++ b/spec/system/consumer/shopping/checkout_paypal_spec.rb @@ -69,6 +69,7 @@ RSpec.describe "Check out with Paypal" do click_on "Complete order" expect(page).to have_content "Your order has been processed successfully" + expect(page.find("#amount-paid").text).to have_content "$19.99" expect(order.reload.state).to eq "complete" expect(order.payments.count).to eq 1 diff --git a/spec/system/consumer/shopping/checkout_stripe_spec.rb b/spec/system/consumer/shopping/checkout_stripe_spec.rb index 7758f582db..f1974fd5af 100644 --- a/spec/system/consumer/shopping/checkout_stripe_spec.rb +++ b/spec/system/consumer/shopping/checkout_stripe_spec.rb @@ -71,6 +71,8 @@ RSpec.describe "Check out with Stripe" do checkout_with_stripe expect(page).to have_content "Confirmed" + expect(page.find("#amount-paid").text).to have_content "$19.99" + expect(order.reload.completed?).to eq true expect(order.payments.first.state).to eq "completed" end From 17e7f7d26d0d60bf592af6c5e2f2e8de2e40a7c6 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 20 Jan 2025 16:16:00 +1100 Subject: [PATCH 07/10] Small linting fix --- spec/models/spree/order_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index f10c1f4f80..3b2bcf1735 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -280,17 +280,17 @@ RSpec.describe Spree::Order do end end - context "#process_payments!" do + describe "#process_payments!" do let(:payment) { build(:payment) } before { allow(order).to receive_messages pending_payments: [payment], total: 10 } - it "should return false if no pending_payments available" do + it "returns false if no pending_payments available" do allow(order).to receive_messages pending_payments: [] expect(order.process_payments!).to be_falsy end context "when the processing is sucessful" do - it "should process the payments" do + it "processes the payments" do expect(payment).to receive(:process!) expect(order.process_payments!).to be_truthy end @@ -299,12 +299,12 @@ RSpec.describe Spree::Order do context "when a payment raises a GatewayError" do before { expect(payment).to receive(:process!).and_raise(Spree::Core::GatewayError) } - it "should return true when configured to allow checkout on gateway failures" do + it "returns true when configured to allow checkout on gateway failures" do Spree::Config.set allow_checkout_on_gateway_error: true expect(order.process_payments!).to be_truthy end - it "should return false when not configured to allow checkout on gateway failures" do + it "returns false when not configured to allow checkout on gateway failures" do Spree::Config.set allow_checkout_on_gateway_error: false expect(order.process_payments!).to be_falsy end From d7ae91c23ed4ac841bf5934d2dbb3935fe44fe62 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 20 Jan 2025 16:21:19 +1100 Subject: [PATCH 08/10] Per review, specify the actual expected value --- spec/models/spree/order_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 3b2bcf1735..7ef37bb628 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1238,8 +1238,7 @@ RSpec.describe Spree::Order do order.update_shipping_fees! - item_num = order.line_items.sum(&:quantity) - expect(order.reload.adjustment_total).to eq(item_num * shipping_fee) + expect(order.reload.adjustment_total).to eq(15) # 3 items * 5 end end end From 0ca164a3542532f366b9358997dc6c34cf8ca16e Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 21 Jan 2025 10:51:08 +1100 Subject: [PATCH 09/10] Fix spec Use a completed payment, instead of trying to complete a payment --- spec/controllers/admin/customers_controller_spec.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/spec/controllers/admin/customers_controller_spec.rb b/spec/controllers/admin/customers_controller_spec.rb index d8d464fcb0..3353dcb447 100644 --- a/spec/controllers/admin/customers_controller_spec.rb +++ b/spec/controllers/admin/customers_controller_spec.rb @@ -83,13 +83,9 @@ module Admin let!(:variant) { create(:variant, price: 10.0) } before do - allow_any_instance_of(Spree::Payment).to receive(:completed?).and_return(true) - order.contents.add(variant) - order.payments << create(:payment, order:, amount: order.total) - order.reload + order.payments << create(:payment, :completed, order:, amount: order.total) - order.process_payments! order.update_attribute(:state, 'canceled') end From fcd366cc061acf3c2b99ca1854b9d570b027b8ed Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 21 Jan 2025 10:52:32 +1100 Subject: [PATCH 10/10] Fix spec Payment needs to be linked to the order, in order for the payment callback to update `order.payment_total` --- spec/models/spree/order/payment_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/models/spree/order/payment_spec.rb b/spec/models/spree/order/payment_spec.rb index 2e3af6e953..a3527849ba 100644 --- a/spec/models/spree/order/payment_spec.rb +++ b/spec/models/spree/order/payment_spec.rb @@ -15,13 +15,13 @@ module Spree create(:credit_card) } let(:payment1) { - create(:payment, amount: 50, payment_method:, source:, response_code: "12345") + create(:payment, order:, amount: 50, payment_method:, source:, response_code: "12345") } let(:payment2) { - create(:payment, amount: 50, payment_method:, source:, response_code: "12345") + create(:payment, order:, amount: 50, payment_method:, source:, response_code: "12345") } let(:payment3) { - create(:payment, amount: 50, payment_method:, source:, response_code: "12345") + create(:payment, order:, amount: 50, payment_method:, source:, response_code: "12345") } let(:failed_payment) { create(:payment, amount: 50, state: 'failed', payment_method:, source:,