From 3f4d56f210eb8744f521f9d7d5207a084d2d7ab0 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 26 Jan 2023 12:51:10 +0100 Subject: [PATCH 1/7] Fetch the latest payment within the admin risky analysis partial There's no point in providing the latest payment from outside when the partial is accessing `@order` directly. --- backend/app/views/spree/admin/orders/_risk_analysis.html.erb | 2 ++ backend/app/views/spree/admin/orders/cart.html.erb | 2 +- backend/app/views/spree/admin/orders/edit.html.erb | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/backend/app/views/spree/admin/orders/_risk_analysis.html.erb b/backend/app/views/spree/admin/orders/_risk_analysis.html.erb index cdfbe77fe2c..c4a255ece6e 100644 --- a/backend/app/views/spree/admin/orders/_risk_analysis.html.erb +++ b/backend/app/views/spree/admin/orders/_risk_analysis.html.erb @@ -1,3 +1,5 @@ +<% latest_payment = @order.payments.reorder("created_at DESC").first %> +
<%= "#{t('spree.risk_analysis')}: #{t('spree.not') unless @order.is_risky?} #{t('spree.risky')}" %> diff --git a/backend/app/views/spree/admin/orders/cart.html.erb b/backend/app/views/spree/admin/orders/cart.html.erb index 06f02b5fa8f..93be609cf92 100644 --- a/backend/app/views/spree/admin/orders/cart.html.erb +++ b/backend/app/views/spree/admin/orders/cart.html.erb @@ -16,7 +16,7 @@ <% if @order.payments.exists? && @order.is_risky? %> - <%= render 'spree/admin/orders/risk_analysis', latest_payment: @order.payments.reorder("created_at DESC").first %> + <%= render 'spree/admin/orders/risk_analysis' %> <% end %>
diff --git a/backend/app/views/spree/admin/orders/edit.html.erb b/backend/app/views/spree/admin/orders/edit.html.erb index b6be4af31e3..5ab7a2fe31b 100644 --- a/backend/app/views/spree/admin/orders/edit.html.erb +++ b/backend/app/views/spree/admin/orders/edit.html.erb @@ -15,7 +15,7 @@
<% if @order.payments.exists? && @order.is_risky? %> - <%= render 'spree/admin/orders/risk_analysis', latest_payment: @order.payments.reorder("created_at DESC").first %> + <%= render 'spree/admin/orders/risk_analysis' %> <% end %>
From 32ec90e85bcbcb4e2b6ea7bf3582dba51bf81377 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 26 Jan 2023 12:54:01 +0100 Subject: [PATCH 2/7] Only use `Order#is_risky?` to render the admin risk analysis partial `Order#is_risky?` is currently based on whether a payment is risky, but over time the API could be expanded and allow customization. --- backend/app/views/spree/admin/orders/cart.html.erb | 4 ++-- backend/app/views/spree/admin/orders/edit.html.erb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/backend/app/views/spree/admin/orders/cart.html.erb b/backend/app/views/spree/admin/orders/cart.html.erb index 93be609cf92..42484a29121 100644 --- a/backend/app/views/spree/admin/orders/cart.html.erb +++ b/backend/app/views/spree/admin/orders/cart.html.erb @@ -15,8 +15,8 @@ <%= render partial: 'spree/shared/error_messages', locals: { target: @order } %> -<% if @order.payments.exists? && @order.is_risky? %> - <%= render 'spree/admin/orders/risk_analysis' %> +<% if @order.is_risky? %> + <%= render 'spree/admin/orders/risk_analysis' %> <% end %>
diff --git a/backend/app/views/spree/admin/orders/edit.html.erb b/backend/app/views/spree/admin/orders/edit.html.erb index 5ab7a2fe31b..1e4fc9fe672 100644 --- a/backend/app/views/spree/admin/orders/edit.html.erb +++ b/backend/app/views/spree/admin/orders/edit.html.erb @@ -14,7 +14,7 @@ <%= render partial: 'spree/shared/error_messages', locals: { target: @order } %>
-<% if @order.payments.exists? && @order.is_risky? %> +<% if @order.is_risky? %> <%= render 'spree/admin/orders/risk_analysis' %> <% end %> From 28628d7ce02b3e8e4907536f340e9a07c5afbf27 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 26 Jan 2023 17:51:14 +0100 Subject: [PATCH 3/7] Remove references to solidus_gateway, now archived --- core/README.md | 1 - core/app/models/spree/payment_method/credit_card.rb | 4 ---- 2 files changed, 5 deletions(-) diff --git a/core/README.md b/core/README.md index d625b2fd92e..5442b660404 100644 --- a/core/README.md +++ b/core/README.md @@ -40,7 +40,6 @@ source (e.g. `Spree::CreditCard`) using a specific payment method (e.g `Solidus::Gateway::Braintree`). * `Spree::PaymentMethod` - A base class which is used for implementing payment methods. * `Spree::PaymentMethod::CreditCard` - An implementation of a `Spree::PaymentMethod` for credit card payments. -See https://github.com/solidusio/solidus_gateway/ for officially supported payment method implementations. * `Spree::CreditCard` - The `source` of a `Spree::Payment` using `Spree::PaymentMethod::CreditCard` as payment method. ## Inventory sub-system diff --git a/core/app/models/spree/payment_method/credit_card.rb b/core/app/models/spree/payment_method/credit_card.rb index 78e6d2c069a..6e268cf1664 100644 --- a/core/app/models/spree/payment_method/credit_card.rb +++ b/core/app/models/spree/payment_method/credit_card.rb @@ -4,10 +4,6 @@ module Spree # An implementation of a `Spree::PaymentMethod` for credit card payments. # # It's a good candidate as base class for other credit card based payment methods. - # - # See https://github.com/solidusio/solidus_gateway/ for - # officially supported payment method implementations. - # class PaymentMethod::CreditCard < PaymentMethod def payment_source_class Spree::CreditCard From ad17b622200073265db9245e626d5f92cb910385 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Fri, 27 Jan 2023 19:29:23 +0100 Subject: [PATCH 4/7] Fix Payment#is_cvv_risky? Should not be false when a CVV message is present; that's just a field to collect descriptions from the payment gateway, the code is what matters This makes it consistent with both Payment.risky and OrdersHelper#cvv_response_code. --- core/app/models/spree/payment.rb | 1 - core/spec/models/spree/payment_spec.rb | 27 +++++++++----------------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/core/app/models/spree/payment.rb b/core/app/models/spree/payment.rb index 0cdce2c1b9f..a9b4f9d4846 100644 --- a/core/app/models/spree/payment.rb +++ b/core/app/models/spree/payment.rb @@ -137,7 +137,6 @@ def is_avs_risky? def is_cvv_risky? return false if cvv_response_code == "M" return false if cvv_response_code.nil? - return false if cvv_response_message.present? true end diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index e38a7a832ff..f406ee641df 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -1231,26 +1231,17 @@ end end - describe "is_cvv_risky?" do - it "returns false if cvv_response_code == 'M'" do - payment.update_attribute(:cvv_response_code, "M") - expect(payment.is_cvv_risky?).to eq(false) - end - - it "returns false if cvv_response_code == nil" do - payment.update_attribute(:cvv_response_code, nil) - expect(payment.is_cvv_risky?).to eq(false) - end - - it "returns false if cvv_response_message == ''" do - payment.update_attribute(:cvv_response_message, '') - expect(payment.is_cvv_risky?).to eq(false) + describe "#is_cvv_risky?" do + ['M', nil].each do |char| + it "returns false if cvv_response_code is #{char.inspect}" do + payment.cvv_response_code = char + expect(payment.is_cvv_risky?).to eq(false) + end end - it "returns true if cvv_response_code == [A-Z], omitting D" do - # should use cvv_response_code helper - (%w{N P S U} << "").each do |char| - payment.update_attribute(:cvv_response_code, char) + ['', *('A'...'M'), *('N'..'Z')].each do |char| + it "returns true if cvv_response_code is #{char.inspect} (not 'M' or nil)" do + payment.cvv_response_code = char expect(payment.is_cvv_risky?).to eq(true) end end From e633805f98a1164f4aa5cae501fb3333cb17df59 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Fri, 27 Jan 2023 19:32:39 +0100 Subject: [PATCH 5/7] Refactor Payment.risky Use AR and existing scopes to build the query instead of SQL. Add a spec ensuring failed payments are considered risky. --- core/app/models/spree/payment.rb | 2 +- core/spec/models/spree/payment_spec.rb | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/core/app/models/spree/payment.rb b/core/app/models/spree/payment.rb index a9b4f9d4846..e7227247f5e 100644 --- a/core/app/models/spree/payment.rb +++ b/core/app/models/spree/payment.rb @@ -54,7 +54,7 @@ class Payment < Spree::Base scope :processing, -> { with_state('processing') } 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 :risky, -> { failed.or(where(avs_response: RISKY_AVS_CODES)).or(where.not(cvv_response_code: [nil, '', 'M'])) } scope :valid, -> { where.not(state: %w(failed invalid void)) } scope :store_credits, -> { where(source_type: Spree::StoreCredit.to_s) } diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index f406ee641df..bb6024d98ff 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -46,14 +46,19 @@ ) end - context '.risky' do + context 'risk analysis' do let!(:payment_1) { create(:payment, avs_response: 'Y', cvv_response_code: 'M', cvv_response_message: 'Match') } let!(:payment_2) { create(:payment, avs_response: 'Y', cvv_response_code: 'M', cvv_response_message: '') } let!(:payment_3) { create(:payment, avs_response: 'A', cvv_response_code: 'M', cvv_response_message: 'Match') } let!(:payment_4) { create(:payment, avs_response: 'Y', cvv_response_code: 'N', cvv_response_message: 'No Match') } + let!(:payment_5) { create(:payment, avs_response: 'Y', cvv_response_code: 'M', cvv_response_message: '', state: 'failed') } + + describe '.risky' do + it 'fetches only risky payments' do + expect(subject.class.risky.to_a).to match_array([payment_3, payment_4, payment_5]) + end + end - it 'should not return successful responses' do - expect(subject.class.risky.to_a).to match_array([payment_3, payment_4]) end end From bca379054f5b38f62641ce8db83597298ce93325 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Fri, 27 Jan 2023 19:33:01 +0100 Subject: [PATCH 6/7] Add Payment#risky? Match the behavior of Payment.risky. --- core/app/models/spree/payment.rb | 5 +++++ core/spec/models/spree/payment_spec.rb | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/core/app/models/spree/payment.rb b/core/app/models/spree/payment.rb index e7227247f5e..07aba3ef6b9 100644 --- a/core/app/models/spree/payment.rb +++ b/core/app/models/spree/payment.rb @@ -127,6 +127,11 @@ def payment_source res || payment_method end + # @return [Boolean] true when this payment is risky + def risky? + is_avs_risky? || is_cvv_risky? || state == 'failed' + end + # @return [Boolean] true when this payment is risky based on address def is_avs_risky? return false if avs_response.blank? || NON_RISKY_AVS_CODES.include?(avs_response) diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index bb6024d98ff..fc1c1b8a45f 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -59,6 +59,16 @@ end end + context '#risky?' do + it 'is true for risky payments' do + aggregate_failures do + expect(payment_1).not_to be_risky + expect(payment_2).not_to be_risky + expect(payment_3).to be_risky + expect(payment_4).to be_risky + expect(payment_5).to be_risky + end + end end end @@ -1212,7 +1222,7 @@ end end - describe "is_avs_risky?" do + describe "#is_avs_risky?" do it "returns false if avs_response included in NON_RISKY_AVS_CODES" do ('A'..'Z').reject{ |x| subject.class::RISKY_AVS_CODES.include?(x) }.to_a.each do |char| payment.update_attribute(:avs_response, char) From 8f8a331be1f0cc8d0169ae1a92e0b46d0c6fcac5 Mon Sep 17 00:00:00 2001 From: Elia Schito Date: Thu, 26 Jan 2023 17:50:29 +0100 Subject: [PATCH 7/7] Push the risk analysis details to the payment partial AVS and CVV are a legacy coming from an age in which everything could be done with ActiveMerchant and PCI compliance didn't exist. Moving away from specific risk checks paves the way for payment-method specific risky checks and display, although the underlying tables didn't change. --- .../spree/admin/payments_controller.rb | 2 + .../admin/orders/_risk_analysis.html.erb | 62 ++++++------------- .../views/spree/admin/orders/cart.html.erb | 2 +- .../views/spree/admin/orders/edit.html.erb | 2 +- .../views/spree/admin/payments/_list.html.erb | 5 +- .../payments/source_views/_gateway.html.erb | 49 ++++++++++++++- 6 files changed, 72 insertions(+), 50 deletions(-) diff --git a/backend/app/controllers/spree/admin/payments_controller.rb b/backend/app/controllers/spree/admin/payments_controller.rb index eda6ca65665..d8d83e0bf2c 100644 --- a/backend/app/controllers/spree/admin/payments_controller.rb +++ b/backend/app/controllers/spree/admin/payments_controller.rb @@ -11,6 +11,8 @@ class PaymentsController < Spree::Admin::BaseController before_action :load_data before_action :require_bill_address, only: [:index] + helper ::Spree::Admin::OrdersHelper + respond_to :html def index diff --git a/backend/app/views/spree/admin/orders/_risk_analysis.html.erb b/backend/app/views/spree/admin/orders/_risk_analysis.html.erb index c4a255ece6e..97b7124400b 100644 --- a/backend/app/views/spree/admin/orders/_risk_analysis.html.erb +++ b/backend/app/views/spree/admin/orders/_risk_analysis.html.erb @@ -1,52 +1,26 @@ -<% latest_payment = @order.payments.reorder("created_at DESC").first %> +
+ <%= t('spree.risk_analysis') %>: <%= t('spree.risky') %> -
- <%= "#{t('spree.risk_analysis')}: #{t('spree.not') unless @order.is_risky?} #{t('spree.risky')}" %>
+ - - - - - - - - - - - - - - - + + <% order.payments.risky.each do |payment| %> + + + + + + <% end %>
<%= t('spree.payment') %> <%= t('spree.risk') %> <%= t('spree.status') %>
- <%= t('spree.failed_payment_attempts') %>: - - - <%= t( - 'spree.payments_failed_count', - count: @order.payments.failed.count - ) %> - -
<%= t('spree.avs_response') %>: - - <% if latest_payment.is_avs_risky? %> - <%= avs_response_code[latest_payment.avs_response] %> - <% else %> - <%= t('spree.success') %> - <% end %> - -
<%= t('spree.cvv_response') %>: - - <% if latest_payment.is_cvv_risky? %> - <%= cvv_response_code[latest_payment.cvv_response_code] %> - <% else %> - <%= t('spree.success') %> - <% end %> - -
<%= link_to payment.number, admin_order_payment_path(order, payment) %> + <%= t('spree.risky') %> + + + <%= t(payment.state, scope: 'spree.payment_states') %> + +
-
+ diff --git a/backend/app/views/spree/admin/orders/cart.html.erb b/backend/app/views/spree/admin/orders/cart.html.erb index 42484a29121..7ef68278c0c 100644 --- a/backend/app/views/spree/admin/orders/cart.html.erb +++ b/backend/app/views/spree/admin/orders/cart.html.erb @@ -16,7 +16,7 @@ <% if @order.is_risky? %> - <%= render 'spree/admin/orders/risk_analysis' %> + <%= render 'spree/admin/orders/risk_analysis', order: @order %> <% end %>
diff --git a/backend/app/views/spree/admin/orders/edit.html.erb b/backend/app/views/spree/admin/orders/edit.html.erb index 1e4fc9fe672..1337e8d999b 100644 --- a/backend/app/views/spree/admin/orders/edit.html.erb +++ b/backend/app/views/spree/admin/orders/edit.html.erb @@ -15,7 +15,7 @@
<% if @order.is_risky? %> - <%= render 'spree/admin/orders/risk_analysis' %> + <%= render 'spree/admin/orders/risk_analysis', order: @order %> <% end %>
diff --git a/backend/app/views/spree/admin/payments/_list.html.erb b/backend/app/views/spree/admin/payments/_list.html.erb index d4fb62bf0ea..3c5828d7deb 100644 --- a/backend/app/views/spree/admin/payments/_list.html.erb +++ b/backend/app/views/spree/admin/payments/_list.html.erb @@ -22,7 +22,10 @@ <% payments.each do |payment| %> - <%= link_to payment.number, spree.admin_order_payment_path(@order, payment) %> + + <%= tag :i, class: "fa fa-warning red", title: t('spree.risky') if payment.risky? %> + <%= link_to payment.number, spree.admin_order_payment_path(@order, payment) %> + <%= l(payment.created_at, format: :short) %> <%= payment.payment_method.name %> <%= payment.transaction_id %> diff --git a/backend/app/views/spree/admin/payments/source_views/_gateway.html.erb b/backend/app/views/spree/admin/payments/source_views/_gateway.html.erb index b797d3ba13e..67c2db99756 100644 --- a/backend/app/views/spree/admin/payments/source_views/_gateway.html.erb +++ b/backend/app/views/spree/admin/payments/source_views/_gateway.html.erb @@ -15,10 +15,53 @@
<%= Spree::CreditCard.human_attribute_name(:expiration) %>:
<%= payment.source.month %>/<%= payment.source.year %>
+ + <% if payment.source.address %> +
<%= Spree::CreditCard.human_attribute_name(:source_address) %>:
+
<%= render partial: 'spree/admin/shared/address', locals: {address: payment.source.address} %>
+ <% end %> + + + +
+
+ +
+
+
<%= Spree::CreditCard.human_attribute_name(:code) %>:
+
<%= payment.number %>
+ +
<%= t('spree.risk_analysis') %>
+
+ + <%= "#{t('spree.not') unless payment.risky?} #{t('spree.risky').downcase}".capitalize %> + +
+ +
<%= t('spree.status')%>
+
<%= t(payment.state, scope: 'spree.payment_states') %>
+ + +
<%= Spree::CreditCard.human_attribute_name(:avs_response) %>:
+
+ <%= content_tag( + :span, + payment.is_avs_risky? ? t('spree.failure') : t('spree.success'), + class: "pill pill-#{payment.is_avs_risky? ? 'warning' : 'complete'}", + title: avs_response_code[payment.avs_response], + ) %> +
+ +
<%= Spree::CreditCard.human_attribute_name(:cvv_response_code) %>:
+
+ <%= content_tag( + :span, + payment.is_cvv_risky? ? t('spree.failure') : t('spree.success'), + class: "pill pill-#{payment.is_cvv_risky? ? 'warning' : 'complete'}", + title: cvv_response_code[payment.cvv_response_code], + ) %> +
- <% if payment.source.address %> - <%= render partial: 'spree/admin/shared/address', locals: {address: payment.source.address} %> - <% end %>