From 0f923405cb3f4fc09e8a5b9b2ad6bd713100c2a5 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 22 Jul 2024 15:48:21 +1000 Subject: [PATCH 1/5] Attempt to spec for error It didn't catch the error I was looking for. I'm not sure if it is a valid use case, but it still seems helpful to add coverage for current functionality. --- spec/controllers/checkout_controller_spec.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index acef705a0c..a322f9b3bf 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -328,6 +328,23 @@ RSpec.describe CheckoutController, type: :controller do expect_cable_ready_redirect(response) end end + + context "with existing invalid payments" do + let(:invalid_payments) { [ + create(:payment, state: :failed), + create(:payment, state: :void), + ] } + + before do + order.payments = invalid_payments + end + + it "deletes invalid payments" do + expect{ + put(:update, params:) + }.to change { order.payments.to_a }.from(invalid_payments) + end + end end context "with no payment source" do From 25eb00f69c533f49439418e0511c977cff7b4318 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 22 Jul 2024 15:48:21 +1000 Subject: [PATCH 2/5] Spec for #12693 I tried to build it within context "with existing invalid payments", but couldn't get it to work so created a different one. --- spec/controllers/checkout_controller_spec.rb | 35 ++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index a322f9b3bf..e444bd9730 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -330,10 +330,12 @@ RSpec.describe CheckoutController, type: :controller do end context "with existing invalid payments" do - let(:invalid_payments) { [ + let(:invalid_payments) { + [ create(:payment, state: :failed), create(:payment, state: :void), - ] } + ] + } before do order.payments = invalid_payments @@ -345,6 +347,35 @@ RSpec.describe CheckoutController, type: :controller do }.to change { order.payments.to_a }.from(invalid_payments) end end + + context "with different payment method previously chosen" do + let(:other_payment_method) { build(:payment_method, distributors: [distributor]) } + let(:other_payment) { + build(:payment, amount: order.total, payment_method: other_payment_method) + } + + before do + order.payments = [other_payment] + end + + context "and order is in an earlier state" do + # This revealed obscure bug #12693. If you progress to order summary, go back to payment + # method, then open delivery details in a new tab (or hover over the link with Turbo + # enabled), then submit new payment details, this happens. + + before do + order.back_to_address + end + + it "deletes invalid (old) payments" do + pending "#12693 ActiveRecord::RecordNotFound: Couldn't find Spree::Payment" + + put(:update, params:) + order.payments.reload + expect(order.payments).not_to include other_payment + end + end + end end context "with no payment source" do From f37742d84a5c3946ff1c502408e1b99db4a1ec99 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 6 Mar 2025 17:19:49 +1100 Subject: [PATCH 3/5] Prevent exception when payment has been cleared Note that in the real world, this avoids a crash, but still requires the user to click the button two more times before it will work, with no hints as to why. So not a great help. --- app/services/orders/workflow_service.rb | 5 ++++- spec/controllers/checkout_controller_spec.rb | 2 -- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/services/orders/workflow_service.rb b/app/services/orders/workflow_service.rb index 99eec1bd17..f0e84fbe18 100644 --- a/app/services/orders/workflow_service.rb +++ b/app/services/orders/workflow_service.rb @@ -92,7 +92,10 @@ module Orders # Verifies if the in-memory payment state is different from the one stored in the database # This is be done without reloading the payment so that in-memory data is not changed def different_from_db_payment_state?(in_memory_payment_state, payment_id) - in_memory_payment_state != Spree::Payment.find(payment_id).state + # Re-load payment from the DB (unless it was cleared by clear_invalid_payments) + db_payment = Spree::Payment.find_by(id: payment_id) + + db_payment.present? && in_memory_payment_state != db_payment.state end end end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index e444bd9730..4adbffa9df 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -368,8 +368,6 @@ RSpec.describe CheckoutController, type: :controller do end it "deletes invalid (old) payments" do - pending "#12693 ActiveRecord::RecordNotFound: Couldn't find Spree::Payment" - put(:update, params:) order.payments.reload expect(order.payments).not_to include other_payment From 55cbe51592654a2b13420247fc352fda82c782d9 Mon Sep 17 00:00:00 2001 From: David Cook Date: Tue, 11 Mar 2025 16:18:07 +1100 Subject: [PATCH 4/5] Prevent Turbo pre-fetch which changes cart state --- app/views/checkout/_tabs.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/checkout/_tabs.html.haml b/app/views/checkout/_tabs.html.haml index e4c027c895..6042e190da 100644 --- a/app/views/checkout/_tabs.html.haml +++ b/app/views/checkout/_tabs.html.haml @@ -1,4 +1,5 @@ -.flex +-# Prevent Turbo pre-fetch which changes cart state +.flex{'data-turbo': "false"} .columns.three.text-center.checkout-tab{"class": [("selected" if checkout_step?(:details)), ("success" unless checkout_step?(:details))]} %div %span.checkout-tab-number From b5065de7d9cc2024e626f9d790554c3ff788398d Mon Sep 17 00:00:00 2001 From: David Cook Date: Wed, 12 Mar 2025 10:01:45 +1100 Subject: [PATCH 5/5] Update app/views/checkout/_tabs.html.haml Co-authored-by: Gaetan Craig-Riou <40413322+rioug@users.noreply.github.com> --- app/views/checkout/_tabs.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/checkout/_tabs.html.haml b/app/views/checkout/_tabs.html.haml index 6042e190da..8ec938cc9d 100644 --- a/app/views/checkout/_tabs.html.haml +++ b/app/views/checkout/_tabs.html.haml @@ -1,5 +1,5 @@ -# Prevent Turbo pre-fetch which changes cart state -.flex{'data-turbo': "false"} +.flex{'data-turbo-prefetch': "false"} .columns.three.text-center.checkout-tab{"class": [("selected" if checkout_step?(:details)), ("success" unless checkout_step?(:details))]} %div %span.checkout-tab-number