From eb8050d61d5d49123c12d45454369cbcbe89dff4 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 25 Sep 2024 15:58:28 +1000 Subject: [PATCH 1/7] Add spec reproducing the bug --- spec/system/consumer/checkout/summary_spec.rb | 43 ++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index a15b800528..f1b67ea33a 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -10,6 +10,7 @@ RSpec.describe "As a consumer, I want to checkout my order" do include StripeStubs include PaypalHelper include AuthenticationHelper + include UIComponentHelper let!(:zone) { create(:zone_with_member) } let(:supplier) { create(:supplier_enterprise) } @@ -48,7 +49,6 @@ RSpec.describe "As a consumer, I want to checkout my order" do before do login_as(user) - visit checkout_path end context "summary step" do @@ -311,6 +311,47 @@ RSpec.describe "As a consumer, I want to checkout my order" do end end + context "when updating cart after summary step" do + let(:order) { + create(:order_ready_for_payment, distributor:) + } + let!(:payment_with_fee) { + create( + :payment_method, + distributors: [distributor], + name: "Payment with Fee", description: "Payment with fee", + calculator: Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 0.1) + ) + } + + it "calculated the correct order total" do + pending + visit checkout_step_path(:payment) + expect(page).to have_checked_field "Payment with Fee" + + click_button "Next - Order summary" + expect(page).to have_title "Checkout Summary - Open Food Network" + + # Back to the shop + click_link "Shopping @ #{distributor.name}" + expect(page).to have_content(distributor.name) + + # Add item to cart + within_variant(order.line_items.first.variant) do + click_button increase_quantity_symbol + end + wait_for_cart + + # Checkout + toggle_cart + click_link "Checkout" + + # Check summary page total + expect(page).to have_title "Checkout Summary - Open Food Network" + expect(page).to have_selector("#order_total", text: 20.02) + end + end + context "with previous open orders" do let(:order) { create(:order_ready_for_confirmation, distributor:, From fafd86a2db38f1da5cb67e14c749e4b05700e2e4 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 30 Sep 2024 16:04:44 +1000 Subject: [PATCH 2/7] Revert change made in https://github.com/openfoodfoundation/openfoodnetwork/pull/12538 Although the change fix the issue in the back office scenario, it has the side effect of getting the order total out of sync. Updating a payment adjustment need to be followed by udpating the order total and payment amount to keep everything in sync. --- app/models/spree/payment.rb | 1 - spec/system/admin/order_spec.rb | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index e10920d4b8..6479694df9 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -155,7 +155,6 @@ module Spree if adjustment adjustment.originator = payment_method adjustment.label = adjustment_label - adjustment.amount = payment_method.compute_amount(self) adjustment.save elsif !processing_refund? && payment_method.present? payment_method.create_adjustment(adjustment_label, self, true) diff --git a/spec/system/admin/order_spec.rb b/spec/system/admin/order_spec.rb index 685e1174cb..43d0745723 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -180,6 +180,7 @@ RSpec.describe ' let(:order_with_fees) { create(:completed_order_with_fees, user:, distributor:, order_cycle: ) } it 'recalculates transaction fee' do + pending login_as_admin visit spree.edit_admin_order_path(order_with_fees) From 03dbd54b259ba1f29e580b965540bb50ca602ed1 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 30 Sep 2024 16:15:59 +1000 Subject: [PATCH 3/7] Fix order updater to update payment fees The order updater did not take into account payment fees on pending payment. --- .../order_management/order/updater.rb | 19 ++++++++++++++++++- .../order_management/order/updater_spec.rb | 15 ++++++++++++++- spec/system/consumer/checkout/summary_spec.rb | 6 +++--- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index 97b77ace28..d0ee5abd59 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -240,7 +240,24 @@ module OrderManagement return unless order.state.in? ["payment", "confirmation", "complete"] return unless order.pending_payments.any? - order.pending_payments.first.update_attribute :amount, order.total + # Update payment tax fees if needed + payment = order.pending_payments.first + new_amount = payment.payment_method.compute_amount(payment) + if new_amount != payment.adjustment.amount + update_payment_adjustment(payment.adjustment, new_amount) + end + + # Update payment with correct amount + payment.update_attribute :amount, order.total + end + + def update_payment_adjustment(adjustment, amount) + adjustment.update_attribute(:amount, amount) + + # Update order total to take into account updated payment fees + update_adjustment_total + update_order_total + persist_totals end end end diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index 4b1eb0689e..458bb785ce 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -121,7 +121,7 @@ module OrderManagement updater.update end - context "whith pending payments" do + context "with pending payments" do let(:order) { create(:completed_order_with_totals) } it "updates pending payments" do @@ -183,6 +183,19 @@ module OrderManagement expect { updater.update }.to change { payment.reload.amount }.from(10).to(20) end + + it "updates pending payments fees" do + calculator = build(:calculator_flat_percent_per_item, preferred_flat_percent: 10) + payment_method = create(:payment_method, name: "Percentage cash", calculator:) + payment = create(:payment, payment_method:, order:, amount: order.total) + + # update order so the order total will change + update_order_quantity(order) + order.payments.reload + + expect { updater.update }.to change { payment.reload.amount }.from(10).to(22) + .and change { payment.reload.adjustment.amount }.from(1).to(2) + end end context "with order in cart" do diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index f1b67ea33a..22d1e7cdbd 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -320,12 +320,11 @@ RSpec.describe "As a consumer, I want to checkout my order" do :payment_method, distributors: [distributor], name: "Payment with Fee", description: "Payment with fee", - calculator: Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 0.1) + calculator: Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) ) } it "calculated the correct order total" do - pending visit checkout_step_path(:payment) expect(page).to have_checked_field "Payment with Fee" @@ -348,7 +347,8 @@ RSpec.describe "As a consumer, I want to checkout my order" do # Check summary page total expect(page).to have_title "Checkout Summary - Open Food Network" - expect(page).to have_selector("#order_total", text: 20.02) + expect(page).to have_selector("#order_total", text: 22.00) + expect(order.reload.payments.first.amount).to eql(22.00) end end From a74cf970839ba4fa0c38f53435aef9beb6c5291c Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Mon, 30 Sep 2024 16:49:19 +1000 Subject: [PATCH 4/7] Fix spec when adding a product with transaction fee Previous iteration did not actually check the payment fee had been updated. It also checks the order total get correctly updated. Spec is passing, so fixing the order updater also fix this bug : https://github.com/openfoodfoundation/openfoodnetwork/issues/12512 --- spec/system/admin/order_spec.rb | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/spec/system/admin/order_spec.rb b/spec/system/admin/order_spec.rb index 43d0745723..61a056e9c7 100644 --- a/spec/system/admin/order_spec.rb +++ b/spec/system/admin/order_spec.rb @@ -179,20 +179,22 @@ RSpec.describe ' context "When adding a product on an order with transaction fee" do let(:order_with_fees) { create(:completed_order_with_fees, user:, distributor:, order_cycle: ) } - it 'recalculates transaction fee' do - pending + it "recalculates transaction fee and order total" do login_as_admin visit spree.edit_admin_order_path(order_with_fees) - adjustment_for_transaction_fee = order_with_fees.all_adjustments.payment_fee.eligible.first - transaction_fee = adjustment_for_transaction_fee.amount + # Fee is $5 per item and we have two line items + expect(page).to have_css("#order_adjustments", text: 10.00) + expect(page).to have_css(".order-total", text: 36.00) - expect(page.find("#order_adjustments").text).to have_content(transaction_fee) + expect { + select2_select product.name, from: 'add_variant_id', search: true + find('button.add_variant').click + }.to change { order_with_fees.payments.first.adjustment.amount }.from(10.00).to(15.00) + .and change { order_with_fees.reload.total }.from(36.00).to(63.99) - select2_select product.name, from: 'add_variant_id', search: true - find('button.add_variant').click - expect(page).to have_css("#order_adjustments", - text: adjustment_for_transaction_fee.reload.amount) + expect(page).to have_css("#order_adjustments", text: 15.00) + expect(page).to have_css(".order-total", text: 63.99) end end From d01d312b4ff32b3aa6bd794171458f81d67becd4 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 1 Oct 2024 10:22:47 +1000 Subject: [PATCH 5/7] Fix updating pending payment Check if payment actually have an adjustment before trying to update it --- .../order_management/order/updater.rb | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index d0ee5abd59..c31e2b929d 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -240,19 +240,25 @@ module OrderManagement return unless order.state.in? ["payment", "confirmation", "complete"] return unless order.pending_payments.any? + @payment = order.pending_payments.first + return update_payment if @payment.adjustment.nil? + # Update payment tax fees if needed - payment = order.pending_payments.first - new_amount = payment.payment_method.compute_amount(payment) - if new_amount != payment.adjustment.amount - update_payment_adjustment(payment.adjustment, new_amount) + new_amount = @payment.payment_method.compute_amount(@payment) + if new_amount != @payment.adjustment.amount + update_payment_adjustment(new_amount) end - # Update payment with correct amount - payment.update_attribute :amount, order.total + update_payment end - def update_payment_adjustment(adjustment, amount) - adjustment.update_attribute(:amount, amount) + def update_payment + # Update payment with correct amount + @payment.update_attribute :amount, order.total + end + + def update_payment_adjustment(amount) + @payment.adjustment.update_attribute(:amount, amount) # Update order total to take into account updated payment fees update_adjustment_total From b2b6847882e61acae81ca8d7a50d52ffb0b38a79 Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Tue, 1 Oct 2024 10:38:33 +1000 Subject: [PATCH 6/7] Fix test data The future is now ! :D --- spec/controllers/spree/credit_cards_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/spree/credit_cards_controller_spec.rb b/spec/controllers/spree/credit_cards_controller_spec.rb index c5163163ed..639127676e 100644 --- a/spec/controllers/spree/credit_cards_controller_spec.rb +++ b/spec/controllers/spree/credit_cards_controller_spec.rb @@ -26,7 +26,7 @@ RSpec.describe Spree::CreditCardsController, type: :controller do { format: :json, exp_month: 9, - exp_year: 2024, + exp_year: 1.year.from_now.year, last4: 4242, token: token['id'], cc_type: "visa" From aa5feb66053a7054958528feb1dc76f20d417b9f Mon Sep 17 00:00:00 2001 From: Gaetan Craig-Riou Date: Wed, 2 Oct 2024 09:33:02 +1000 Subject: [PATCH 7/7] Remove system spec It's covered by unit test of order updater --- spec/system/consumer/checkout/summary_spec.rb | 41 ------------------- 1 file changed, 41 deletions(-) diff --git a/spec/system/consumer/checkout/summary_spec.rb b/spec/system/consumer/checkout/summary_spec.rb index 22d1e7cdbd..21c29e9316 100644 --- a/spec/system/consumer/checkout/summary_spec.rb +++ b/spec/system/consumer/checkout/summary_spec.rb @@ -311,47 +311,6 @@ RSpec.describe "As a consumer, I want to checkout my order" do end end - context "when updating cart after summary step" do - let(:order) { - create(:order_ready_for_payment, distributor:) - } - let!(:payment_with_fee) { - create( - :payment_method, - distributors: [distributor], - name: "Payment with Fee", description: "Payment with fee", - calculator: Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) - ) - } - - it "calculated the correct order total" do - visit checkout_step_path(:payment) - expect(page).to have_checked_field "Payment with Fee" - - click_button "Next - Order summary" - expect(page).to have_title "Checkout Summary - Open Food Network" - - # Back to the shop - click_link "Shopping @ #{distributor.name}" - expect(page).to have_content(distributor.name) - - # Add item to cart - within_variant(order.line_items.first.variant) do - click_button increase_quantity_symbol - end - wait_for_cart - - # Checkout - toggle_cart - click_link "Checkout" - - # Check summary page total - expect(page).to have_title "Checkout Summary - Open Food Network" - expect(page).to have_selector("#order_total", text: 22.00) - expect(order.reload.payments.first.amount).to eql(22.00) - end - end - context "with previous open orders" do let(:order) { create(:order_ready_for_confirmation, distributor:,