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 cdfbe77fe2c..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,50 +1,26 @@ -
- <%= "#{t('spree.risk_analysis')}: #{t('spree.not') unless @order.is_risky?} #{t('spree.risky')}" %> +
+ <%= t('spree.risk_analysis') %>: <%= 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 06f02b5fa8f..7ef68278c0c 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', latest_payment: @order.payments.reorder("created_at DESC").first %> +<% if @order.is_risky? %> + <%= 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 b6be4af31e3..1337e8d999b 100644 --- a/backend/app/views/spree/admin/orders/edit.html.erb +++ b/backend/app/views/spree/admin/orders/edit.html.erb @@ -14,8 +14,8 @@ <%= render partial: 'spree/shared/error_messages', locals: { target: @order } %>
-<% if @order.payments.exists? && @order.is_risky? %> - <%= render 'spree/admin/orders/risk_analysis', latest_payment: @order.payments.reorder("created_at DESC").first %> +<% if @order.is_risky? %> + <%= 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 %>
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.rb b/core/app/models/spree/payment.rb index 0cdce2c1b9f..07aba3ef6b9 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) } @@ -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) @@ -137,7 +142,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/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 diff --git a/core/spec/models/spree/payment_spec.rb b/core/spec/models/spree/payment_spec.rb index e38a7a832ff..fc1c1b8a45f 100644 --- a/core/spec/models/spree/payment_spec.rb +++ b/core/spec/models/spree/payment_spec.rb @@ -46,14 +46,29 @@ ) 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') } - it 'should not return successful responses' do - expect(subject.class.risky.to_a).to match_array([payment_3, payment_4]) + 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 + + 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 @@ -1207,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) @@ -1231,26 +1246,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