From 5eb0bddba3be9ac22a8b1a3fa92fa91a0b4bedb9 Mon Sep 17 00:00:00 2001 From: andrea longhi Date: Thu, 21 Feb 2019 17:53:09 +0100 Subject: [PATCH 1/3] Do not allow successful checkout when order has only a void payment When a payment has been marked for some reason as `void`, the customer should not be able to complete the checkout process successfully. The customer is now redirected to the payment page, where they can add another payment to the order in order to complete it successfully. --- core/app/models/spree/payment.rb | 2 +- core/spec/models/spree/payment_spec.rb | 12 ++++++++++++ frontend/spec/features/checkout_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/payment.rb b/core/app/models/spree/payment.rb index 1b602fd64d2..d9889db7517 100644 --- a/core/app/models/spree/payment.rb +++ b/core/app/models/spree/payment.rb @@ -58,7 +58,7 @@ class Payment < Spree::Base scope :failed, -> { with_state('failed') } scope :risky, -> { where("avs_response IN (?) OR (cvv_response_code IS NOT NULL and cvv_response_code != 'M') OR state = 'failed'", RISKY_AVS_CODES) } - scope :valid, -> { where.not(state: %w(failed invalid)) } + scope :valid, -> { where.not(state: %w(failed invalid void)) } scope :store_credits, -> { where(source_type: Spree::StoreCredit.to_s) } scope :not_store_credits, -> { where(arel_table[:source_type].not_eq(Spree::StoreCredit.to_s).or(arel_table[:source_type].eq(nil))) } diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index 8fe2bdfc8a9..fcfd67dafab 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -1255,4 +1255,16 @@ end end end + + describe '::valid scope' do + before do + create :payment, state: :void + create :payment, state: :failed + create :payment, state: :invalid + end + + it 'does not include void, failed and invalid payments' do + expect(described_class.valid).to be_empty + end + end end diff --git a/frontend/spec/features/checkout_spec.rb b/frontend/spec/features/checkout_spec.rb index 6d1897a79d0..448a3662e3a 100644 --- a/frontend/spec/features/checkout_spec.rb +++ b/frontend/spec/features/checkout_spec.rb @@ -211,6 +211,26 @@ end end + context "when order has only a void payment" do + let(:order) { Spree::TestingSupport::OrderWalkthrough.up_to(:payment) } + + before do + user = create(:user) + order.user = user + order.recalculate + + allow_any_instance_of(Spree::CheckoutController).to receive_messages(current_order: order) + allow_any_instance_of(Spree::CheckoutController).to receive_messages(try_spree_current_user: user) + end + + it "does not allow successful order submission" do + visit spree.checkout_path + order.payments.first.update state: :void + click_button 'Place Order' + expect(page).to have_current_path spree.checkout_state_path(:payment) + end + end + # Regression test for https://github.com/spree/spree/issues/2694 and https://github.com/spree/spree/issues/4117 context "doesn't allow bad credit card numbers" do let!(:payment_method) { create(:credit_card_payment_method) } From c064bc1fc2f7c0378dde5127f4b2eba37eb0613a Mon Sep 17 00:00:00 2001 From: andrea longhi Date: Tue, 26 Feb 2019 18:28:11 +0100 Subject: [PATCH 2/3] Fix failing spec, add spec to expose `void` behavior When a single payment exists for the order, and the payment is marked as void, then the order payment state changes to `Failed`. When there are multiple pending payments for the order, if only one is marked as void the order payment state is still `Balance Due`. If all the payments are marked as void then the order payment state changes to `Failed`. --- .../features/admin/orders/payments_spec.rb | 59 ++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/backend/spec/features/admin/orders/payments_spec.rb b/backend/spec/features/admin/orders/payments_spec.rb index 9e82ce66eea..e449b490c8d 100644 --- a/backend/spec/features/admin/orders/payments_spec.rb +++ b/backend/spec/features/admin/orders/payments_spec.rb @@ -62,6 +62,63 @@ expect(page).to have_content 'my cc address' end + context 'when there are multiple pending payments', :js do + context 'while marking all payments as void' do + let(:card_payment_method) { create(:credit_card_payment_method) } + + let!(:payment) do + create( + :payment, + order: order, + amount: order.outstanding_balance, + payment_method: card_payment_method, + state: :pending + ) + end + + let!(:second_payment) do + create( + :payment, + order: order, + amount: order.outstanding_balance, + payment_method: card_payment_method, + state: :pending + ) + end + + it 'updates the order payment state correctly at each iteration' do + visit current_path + expect(page).to have_css('#payment_status', text: 'Balance due') + + within '#payments' do + expect(page).to have_selector('.pill-pending', count: 2) + within "#payment_#{payment.id}" do + find('.fa-void').click + end + end + + expect(page).to have_css('#payment_status', text: 'Balance due') + + within '#payments' do + expect(page).to have_selector('.pill-pending', count: 1) + within "#payment_#{payment.id}" do + expect(page).to have_selector('.pill-void', count: 1) + end + end + + within "#payment_#{second_payment.id}" do + find('.fa-void').click + end + + within '#payments' do + expect(page).not_to have_selector('.pill-pending') + expect(page).to have_selector('.pill-void', count: 2) + end + expect(page).to have_css('#payment_status', text: 'Failed') + end + end + end + it 'lists, updates and creates payments for an order', js: true do within_row(1) do expect(column_text(3)).to eq('Credit Card') @@ -70,7 +127,7 @@ end click_icon :void - expect(page).to have_css('#payment_status', text: 'Balance due') + expect(page).to have_css('#payment_status', text: 'Failed') expect(page).to have_content('Payment Updated') within_row(1) do From e663b24cd70d6d33274a1fa9d27c98e930898d7b Mon Sep 17 00:00:00 2001 From: andrea longhi Date: Tue, 26 Feb 2019 18:43:12 +0100 Subject: [PATCH 3/3] DRY up specs by extracting `create_payment` method Utility method `create_payment` was extracted in order to DRY up the code a bit. --- .../features/admin/orders/payments_spec.rb | 47 ++++++++----------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/backend/spec/features/admin/orders/payments_spec.rb b/backend/spec/features/admin/orders/payments_spec.rb index e449b490c8d..6b24696d188 100644 --- a/backend/spec/features/admin/orders/payments_spec.rb +++ b/backend/spec/features/admin/orders/payments_spec.rb @@ -5,17 +5,24 @@ describe 'Payments', type: :feature do stub_authorization! - context "with a pre-existing payment" do - let!(:payment) do - create(:payment, - order: order, - amount: order.outstanding_balance, + let(:state) { 'checkout' } + + def create_payment(opts = {}) + create( + :payment, + { + order: order, + amount: order.outstanding_balance, payment_method: create(:credit_card_payment_method), - state: state) - end + state: state + }.merge(opts) + ) + end + + context "with a pre-existing payment" do + let!(:payment) { create_payment } let(:order) { create(:completed_order_with_totals, number: 'R100', line_items_price: 50) } - let(:state) { 'checkout' } before do visit "/admin/orders/#{order.number}/payments" @@ -24,12 +31,7 @@ # Regression tests for https://github.com/spree/spree/issues/1453 context 'with a check payment', js: true do let(:order) { create(:completed_order_with_totals, number: 'R100') } - let!(:payment) do - create(:payment, - order: order, - amount: order.outstanding_balance, - payment_method: create(:check_payment_method, available_to_admin: true)) # Check - end + let!(:payment) { create_payment(payment_method: create(:check_payment_method, available_to_admin: true)) } it 'capturing a check payment from a new order' do click_icon(:capture) @@ -67,20 +69,14 @@ let(:card_payment_method) { create(:credit_card_payment_method) } let!(:payment) do - create( - :payment, - order: order, - amount: order.outstanding_balance, + create_payment( payment_method: card_payment_method, state: :pending ) end let!(:second_payment) do - create( - :payment, - order: order, - amount: order.outstanding_balance, + create_payment( payment_method: card_payment_method, state: :pending ) @@ -268,12 +264,7 @@ context 'with a soft-deleted payment method' do let(:order) { create(:completed_order_with_totals, line_items_count: 1) } let!(:payment_method) { create(:check_payment_method) } - let!(:payment) do - create(:payment, - order: order, - amount: order.outstanding_balance, - payment_method: payment_method) - end + let!(:payment) { create_payment(payment_method: payment_method) } before do payment_method.discard