From d520e3838c4544e2ef44fbf188771481c4375ba0 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Sun, 28 Apr 2024 18:32:28 +0100 Subject: [PATCH 1/5] Removes unused spec related to legacy checkout We can see on the respective controller spec, that having a Stripe SCA payment, with no source does not trigger the error 400, observed on the legacy checkout. --- spec/controllers/checkout_controller_spec.rb | 25 +++++ .../requests/checkout/failed_checkout_spec.rb | 93 ------------------- 2 files changed, 25 insertions(+), 93 deletions(-) delete mode 100644 spec/requests/checkout/failed_checkout_spec.rb diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 7c4a158e4d..80e69d4a14 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -10,6 +10,9 @@ RSpec.describe CheckoutController, type: :controller do let(:exchange) { order_cycle.exchanges.outgoing.first } let(:order) { create(:order_with_line_items, line_items_count: 1, distributor:, order_cycle:) } let(:payment_method) { distributor.payment_methods.first } + let(:stripe_payment_method) { + create(:stripe_sca_payment_method, distributor_ids: [distributor.id], environment: Rails.env) + } let(:shipping_method) { distributor.shipping_methods.first } before do @@ -287,6 +290,7 @@ RSpec.describe CheckoutController, type: :controller do expect(response).to redirect_to checkout_step_path(:summary) expect(order.reload.state).to eq "confirmation" + expect(response.status).to be 302 end describe "with a voucher" do @@ -308,6 +312,27 @@ RSpec.describe CheckoutController, type: :controller do end end + context "with no payment source" do + let(:checkout_params) do + { + order: { + payments_attributes: [ + { payment_method_id: stripe_payment_method.id } + ] + } + } + end + + it "updates and redirects to summary step" do + put(:update, params:) + + expect(response).to redirect_to checkout_step_path(:summary) + expect(order.reload.state).to eq "confirmation" + expect(order.payments.first.source).to eq nil + expect(response.status).to be 302 + end + end + context "with payment fees" do let(:payment_method_with_fee) do create(:payment_method, :flat_rate, amount: "1.23", distributors: [distributor]) diff --git a/spec/requests/checkout/failed_checkout_spec.rb b/spec/requests/checkout/failed_checkout_spec.rb deleted file mode 100644 index f7a767f630..0000000000 --- a/spec/requests/checkout/failed_checkout_spec.rb +++ /dev/null @@ -1,93 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe "checking out an order that initially fails", type: :request do - include ShopWorkflow - - let!(:shop) { create(:enterprise) } - let!(:order_cycle) { create(:simple_order_cycle) } - let!(:exchange) { - create(:exchange, order_cycle:, sender: order_cycle.coordinator, receiver: shop, - incoming: false, pickup_time: "Monday") - } - let!(:address) { create(:address) } - let!(:line_item) { create(:line_item, order:, quantity: 3, price: 5.00) } - let!(:payment_method) { - create(:stripe_sca_payment_method, distributor_ids: [shop.id], environment: Rails.env) - } - let!(:check_payment_method) { - create(:payment_method, distributor_ids: [shop.id], environment: Rails.env) - } - let!(:shipping_method) { create(:shipping_method, distributor_ids: [shop.id]) } - let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method:) } - let!(:order) { - create(:order, shipments: [shipment], distributor: shop, order_cycle:) - } - let(:params) do - { order: { - shipping_method_id: shipping_method.id, - payments_attributes: [{ payment_method_id: payment_method.id }], - bill_address_attributes: address.attributes.slice("firstname", "lastname", "address1", - "address2", "phone", "city", "zipcode", - "state_id", "country_id"), - ship_address_attributes: address.attributes.slice("firstname", "lastname", "address1", - "address2", "phone", "city", "zipcode", - "state_id", "country_id") - } } - end - - before do - order_cycle_distributed_variants = double(:order_cycle_distributed_variants) - allow(OrderCycles::DistributedVariantsService).to receive(:new) - .and_return(order_cycle_distributed_variants) - allow(order_cycle_distributed_variants) - .to receive(:distributes_order_variants?).and_return(true) - - order.reload.update_totals - set_order order - end - - pending "when shipping and payment fees apply" do - let(:calculator) { Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) } - - before do - payment_method.calculator = calculator.dup - payment_method.save! - check_payment_method.calculator = calculator.dup - check_payment_method.save! - shipping_method.calculator = calculator.dup - shipping_method.save! - end - - it "clears shipments and payments before rendering the checkout" do - put update_checkout_path, params:, as: :json - - # Checking out a bogus Stripe Gateway without a source fails at :payment - # Shipments and payments should then be cleared before rendering checkout - expect(response.status).to be 400 - expect(flash[:error]) - .to eq 'Payment could not be processed, please check the details you entered' - order.reload - expect(order.shipments.count).to be 0 - expect(order.payments.count).to be 0 - expect(order.adjustment_total).to eq 0 - - # Add another line item to change the fee totals - create(:line_item, order:, quantity: 3, price: 5.00) - - # Use a check payment method, which should work - params[:order][:payments_attributes][0][:payment_method_id] = check_payment_method.id - - put update_checkout_path, params:, as: :json - - expect(response.status).to be 200 - order.reload - expect(order.total).to eq 36 - expect(order.adjustment_total).to eq 6 - expect(order.item_total).to eq 30 - expect(order.shipments.count).to eq 1 - expect(order.payments.count).to eq 1 - end - end -end From 282acd256b3d1105eb29b7f99c06ed279a83d3e3 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Mon, 29 Apr 2024 19:49:05 +0100 Subject: [PATCH 2/5] Undoes changes: removes unecessary test --- spec/controllers/checkout_controller_spec.rb | 25 -------------------- 1 file changed, 25 deletions(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 80e69d4a14..7c4a158e4d 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -10,9 +10,6 @@ RSpec.describe CheckoutController, type: :controller do let(:exchange) { order_cycle.exchanges.outgoing.first } let(:order) { create(:order_with_line_items, line_items_count: 1, distributor:, order_cycle:) } let(:payment_method) { distributor.payment_methods.first } - let(:stripe_payment_method) { - create(:stripe_sca_payment_method, distributor_ids: [distributor.id], environment: Rails.env) - } let(:shipping_method) { distributor.shipping_methods.first } before do @@ -290,7 +287,6 @@ RSpec.describe CheckoutController, type: :controller do expect(response).to redirect_to checkout_step_path(:summary) expect(order.reload.state).to eq "confirmation" - expect(response.status).to be 302 end describe "with a voucher" do @@ -312,27 +308,6 @@ RSpec.describe CheckoutController, type: :controller do end end - context "with no payment source" do - let(:checkout_params) do - { - order: { - payments_attributes: [ - { payment_method_id: stripe_payment_method.id } - ] - } - } - end - - it "updates and redirects to summary step" do - put(:update, params:) - - expect(response).to redirect_to checkout_step_path(:summary) - expect(order.reload.state).to eq "confirmation" - expect(order.payments.first.source).to eq nil - expect(response.status).to be 302 - end - end - context "with payment fees" do let(:payment_method_with_fee) do create(:payment_method, :flat_rate, amount: "1.23", distributors: [distributor]) From ffdbd0d7d4f6da5efe6423a25f72d9cf15cb4058 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Thu, 2 May 2024 18:00:12 +0100 Subject: [PATCH 3/5] Reproduces Bugsnag error For details see: https://app.bugsnag.com/open-food-network-canada-1/open-food-network-canada/errors/66314b2e78673c00073d2de9?filters[event.since]=30d&filters[error.status]=open&filters[search]=payment_source_class&event_id=66314b2e00e6e45d746f0000 Adds test case for Cash and Stripe payment methods With no source --- spec/controllers/checkout_controller_spec.rb | 56 ++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 7c4a158e4d..b39ffdcacd 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -308,6 +308,62 @@ RSpec.describe CheckoutController, type: :controller do end end + context "with no payment source" do + let(:checkout_params) do + { + order: { + payments_attributes: [ + { + payment_method_id:, + source_attributes: { + first_name: "Jane", + last_name: "Doe", + month: "", + year: "", + cc_type: "", + last_digits: "", + gateway_payment_profile_id: "" + } + } + ] + }, + commit: "Next - Order Summary" + } + end + let(:error_message) { + 'You must implement payment_source_class method for this gateway.' + } + + context "with a cash/check payment method" do + let!(:payment_method_id) { payment_method.id } + + it "updates and redirects to summary step" do + expect { put(:update, params:) }.to raise_error(RuntimeError, error_message) + + # according to Bugsnag, we should get an error 500 + expect(response.status).to be 200 + expect(response).not_to redirect_to checkout_step_path(:summary) + expect(order.reload.state).to eq "payment" + end + end + + context "with a StripeSCA payment method" do + let(:stripe_payment_method) { + create(:stripe_sca_payment_method, distributor_ids: [distributor.id], + environment: Rails.env) + } + let!(:payment_method_id) { stripe_payment_method.id } + + it "updates and redirects to summary step" do + expect { put(:update, params:) }.not_to raise_error(RuntimeError) + + expect(response.status).to eq 422 + expect(flash[:error]).to match "Saving failed, please update the highlighted fields." + expect(order.reload.state).to eq "payment" + end + end + end + context "with payment fees" do let(:payment_method_with_fee) do create(:payment_method, :flat_rate, amount: "1.23", distributors: [distributor]) From 62fefd5d499c7d2a01ddbf4648213eba45744844 Mon Sep 17 00:00:00 2001 From: David Cook Date: Mon, 6 May 2024 12:11:11 +1000 Subject: [PATCH 4/5] Implement required method Most of the time this doesn't get called because source_required: false. But sometimes it [does happen](https://app.bugsnag.com/yaycode/openfoodnetwork-uk/errors/66329690f4b6380007e8a4f8) I have a feeling that source_required? could be moved to the superclass as payment_source_class.present?. But I don't know enough about this area of the system to try it... --- app/models/spree/payment_method/check.rb | 4 ++++ spec/controllers/checkout_controller_spec.rb | 9 ++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/models/spree/payment_method/check.rb b/app/models/spree/payment_method/check.rb index 9f4378830c..839698bebc 100644 --- a/app/models/spree/payment_method/check.rb +++ b/app/models/spree/payment_method/check.rb @@ -25,6 +25,10 @@ module Spree ActiveMerchant::Billing::Response.new(true, "", {}, {}) end + def payment_source_class + nil + end + def source_required? false end diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index b39ffdcacd..4930eb22d5 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -338,12 +338,11 @@ RSpec.describe CheckoutController, type: :controller do let!(:payment_method_id) { payment_method.id } it "updates and redirects to summary step" do - expect { put(:update, params:) }.to raise_error(RuntimeError, error_message) + put(:update, params:) - # according to Bugsnag, we should get an error 500 - expect(response.status).to be 200 - expect(response).not_to redirect_to checkout_step_path(:summary) - expect(order.reload.state).to eq "payment" + expect(response.status).to be 302 + expect(response).to redirect_to checkout_step_path(:summary) + expect(order.reload.state).to eq "confirmation" end end From a1e2d3a93b9db163c3b5985f3249f79a98226f93 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Mon, 6 May 2024 09:52:35 +0100 Subject: [PATCH 5/5] Removes negative expect Removes unused error message --- spec/controllers/checkout_controller_spec.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/spec/controllers/checkout_controller_spec.rb b/spec/controllers/checkout_controller_spec.rb index 4930eb22d5..ea761d876b 100644 --- a/spec/controllers/checkout_controller_spec.rb +++ b/spec/controllers/checkout_controller_spec.rb @@ -330,9 +330,6 @@ RSpec.describe CheckoutController, type: :controller do commit: "Next - Order Summary" } end - let(:error_message) { - 'You must implement payment_source_class method for this gateway.' - } context "with a cash/check payment method" do let!(:payment_method_id) { payment_method.id } @@ -354,8 +351,7 @@ RSpec.describe CheckoutController, type: :controller do let!(:payment_method_id) { stripe_payment_method.id } it "updates and redirects to summary step" do - expect { put(:update, params:) }.not_to raise_error(RuntimeError) - + put(:update, params:) expect(response.status).to eq 422 expect(flash[:error]).to match "Saving failed, please update the highlighted fields." expect(order.reload.state).to eq "payment"