From 4a6e4d4c6d70959425f8ba8ac812b6a238c7f854 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 30 Oct 2024 15:53:15 +1100 Subject: [PATCH 1/7] Ensure shipment is updated when using `update_or_create` `Spree::OrderContents#update_or_create` is used to update the cart when on the /shop page. If you start an order and proceed to the "Order summary" step, and then decide to update your order by using the shop link next to the cart, such update wouldn't update the shipment. This result in the order page in the backoffice displaying the wrong data, and more importantly, in the stock not being updated. So now we ensure shipment will be updated, which result in the checkout flow being restarted, thus making sure the shipment is updated. --- app/models/spree/order_contents.rb | 1 + spec/models/spree/order_contents_spec.rb | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/app/models/spree/order_contents.rb b/app/models/spree/order_contents.rb index 6c8e150e02c..e295d3cc6cb 100644 --- a/app/models/spree/order_contents.rb +++ b/app/models/spree/order_contents.rb @@ -38,6 +38,7 @@ def update_or_create(variant, attributes) line_item.price = variant.price order.line_items << line_item end + update_shipment order.reload line_item diff --git a/spec/models/spree/order_contents_spec.rb b/spec/models/spree/order_contents_spec.rb index b31d685ea38..9f7ec83221f 100644 --- a/spec/models/spree/order_contents_spec.rb +++ b/spec/models/spree/order_contents_spec.rb @@ -166,6 +166,12 @@ end describe "#update_or_create" do + it "ensures shipments are updated" do + expect(order).to receive(:ensure_updated_shipments) + + subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) + end + describe "creating" do it "creates a new line item with given attributes" do subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) From cedf040b478e161fe7c2ce0dcf3397c8e5d73861 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 4 Dec 2024 22:15:56 +1100 Subject: [PATCH 2/7] Per review, test on create and update --- spec/models/spree/order_contents_spec.rb | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/spec/models/spree/order_contents_spec.rb b/spec/models/spree/order_contents_spec.rb index 9f7ec83221f..d3a55fcf8b8 100644 --- a/spec/models/spree/order_contents_spec.rb +++ b/spec/models/spree/order_contents_spec.rb @@ -166,12 +166,6 @@ end describe "#update_or_create" do - it "ensures shipments are updated" do - expect(order).to receive(:ensure_updated_shipments) - - subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) - end - describe "creating" do it "creates a new line item with given attributes" do subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) @@ -181,6 +175,12 @@ expect(line_item.max_quantity).to eq 3 expect(line_item.price).to eq variant.price end + + it "ensures shipments are updated" do + expect(order).to receive(:ensure_updated_shipments) + + subject.update_or_create(variant, { quantity: 2, max_quantity: 3 }) + end end describe "updating" do @@ -192,6 +192,12 @@ expect(line_item.reload.quantity).to eq 3 expect(line_item.max_quantity).to eq 4 end + + it "ensures shipments are updated" do + expect(order).to receive(:ensure_updated_shipments) + + subject.update_or_create(variant, { quantity: 3, max_quantity: 4 }) + end end end end From 9f658568ad0db2a6c37e10fb0ea602acdba7002a Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 30 Oct 2024 14:01:04 +1100 Subject: [PATCH 3/7] 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 81bee889d3d..a365693d0e0 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -106,7 +106,6 @@ def states 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 00a0cdb6214..d7b226aed5b 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1239,7 +1239,7 @@ 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 1cec5a1b743f6f86a47251cf1c1d3df5bd6f0970 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 10 Dec 2024 12:16:37 +1100 Subject: [PATCH 4/7] Add spec for #update_shipping_fees! And update related specs --- spec/models/spree/order_spec.rb | 102 ++++++++++++++++---------------- 1 file changed, 50 insertions(+), 52 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index d7b226aed5b..e70458b7595 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1211,76 +1211,73 @@ end end - describe "a completed order with shipping and transaction fees" do + describe "#update_shipping_fees!" do + let(:distributor) { create(:distributor_enterprise) } + let(:order) { + 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(: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]) } let(:order) { - create(:completed_order_with_fees, distributor:, shipping_fee:, - payment_fee:, - shipping_tax_category:) + 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 @@ # Check if fees got updated order.reload + expect(order.adjustment_total).to eq expected_fees - (item_num * payment_fee) end end From c4b2c53ac3b025f17801a928c79fb24926cf883d Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 10 Dec 2024 12:17:37 +1100 Subject: [PATCH 5/7] 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 d3a55fcf8b8..19788fbeb81 100644 --- a/spec/models/spree/order_contents_spec.rb +++ b/spec/models/spree/order_contents_spec.rb @@ -38,6 +38,27 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 677f5e928d1ceeb115f724a9e653d5e8719481bd Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 10 Dec 2024 12:18:52 +1100 Subject: [PATCH 6/7] 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 fd306cebbc7..29a60f319cd 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -452,14 +452,15 @@ 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 d7216aea445374237ce94e309e91664bbf37012e Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 11 Dec 2024 09:29:32 +1100 Subject: [PATCH 7/7] 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 29a60f319cd..e672166a6c7 100644 --- a/spec/system/admin/orders_spec.rb +++ b/spec/system/admin/orders_spec.rb @@ -452,14 +452,10 @@ 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