Risk analysis box update

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

backend/app/views/spree/admin/orders/_risk_analysis.html.erb

@@ -1,50 +1,26 @@
-<fieldset class="no-border-bottom" id="risk_analysis">
-  <legend><%= "#{t('spree.risk_analysis')}: #{t('spree.not') unless @order.is_risky?} #{t('spree.risky')}" %></legend>
+<details id="risk_analysis">
+  <summary><%= t('spree.risk_analysis') %>: <%= t('spree.risky') %></summary>
+
   <table>
     <thead>
+      <th><%= t('spree.payment') %></th>
       <th><%= t('spree.risk') %></th>
       <th><%= t('spree.status') %></th>
     </thead>
-    <tbody id="risk-analysis" data-hook="order_details_adjustments" class="with-border">
-      <tr class="">
-        <td><strong>
-          <%= t('spree.failed_payment_attempts') %>:
-        </strong></td>
-        <td>
-          <span class="pill pill-<%= @order.payments.failed.count > 0 ? 'warning' : 'complete' %>">
-            <%= t(
-              'spree.payments_failed_count',
-              count: @order.payments.failed.count
-            ) %>
-          </span>
-        </td>
-      </tr>
-
-      <tr>
-        <td><strong><%= t('spree.avs_response') %>:</strong></td>
-        <td>
-          <span class="pill pill-<%= latest_payment.is_avs_risky? ? 'warning' : 'complete' %>">
-            <% if latest_payment.is_avs_risky? %>
-              <%= avs_response_code[latest_payment.avs_response] %>
-            <% else %>
-              <%= t('spree.success') %>
-            <% end %>
-          </span>
-        </td>
-      </tr>
-
-      <tr>
-        <td><strong><%= t('spree.cvv_response') %>:</strong></td>
-        <td>
-          <span class="pill pill-<%= latest_payment.is_cvv_risky? ? 'warning' : 'complete' %>">
-            <% if latest_payment.is_cvv_risky? %>
-              <%= cvv_response_code[latest_payment.cvv_response_code] %>
-            <% else %>
-              <%= t('spree.success') %>
-            <% end %>
-          </span>
-        </td>
-      </tr>
+    <tbody id="risk-analysis" data-hook="order_details_adjustments"  class="with-border">
+      <% order.payments.risky.each do |payment| %>
+        <tr class="">
+          <td><%= link_to payment.number, admin_order_payment_path(order, payment) %></td>
+          <td>
+            <span class="pill pill-warning"><%= t('spree.risky') %></span>
+          </td>
+          <td>
+            <span class="pill pill-<%= payment.state %>">
+              <%= t(payment.state, scope: 'spree.payment_states') %>
+            </span>
+          </td>
+        </tr>
+      <% end %>
     </tbody>
   </table>
-</fieldset>
+</details>

backend/app/views/spree/admin/orders/cart.html.erb

@@ -15,8 +15,8 @@
   <%= render partial: 'spree/shared/error_messages', locals: { target: @order } %>
 </div>
 
-<% 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 %>
 
 <div data-hook="admin_order_edit_form">

backend/app/views/spree/admin/orders/edit.html.erb

@@ -14,8 +14,8 @@
   <%= render partial: 'spree/shared/error_messages', locals: { target: @order } %>
 </div>
 
-<% 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 %>
 
 <div data-hook="admin_order_edit_sub_header"></div>

backend/app/views/spree/admin/payments/_list.html.erb

@@ -22,7 +22,10 @@
   <tbody>
     <% payments.each do |payment| %>
       <tr id="<%= dom_id(payment) %>" data-hook="payments_row" class="payment vertical-middle" data-payment-id="<%= payment.id %>">
-        <td><%= link_to payment.number, spree.admin_order_payment_path(@order, payment) %></td>
+        <td>
+          <%= 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) %>
+        </td>
         <td><%= l(payment.created_at, format: :short) %></td>
         <td><%= payment.payment_method.name %></td>
         <td><%= payment.transaction_id %></td>

backend/app/views/spree/admin/payments/source_views/_gateway.html.erb

@@ -15,10 +15,53 @@
 
         <dt><%= Spree::CreditCard.human_attribute_name(:expiration) %>:</dt>
         <dd><%= payment.source.month %>/<%= payment.source.year %></dd>
+
+        <% if payment.source.address %>
+          <dt><%= Spree::CreditCard.human_attribute_name(:source_address) %>:</dt>
+          <dd><%= render partial: 'spree/admin/shared/address', locals: {address: payment.source.address} %></dd>
+        <% end %>
+      </dl>
+    </div>
+
+    <div class="col-2">
+    </div>
+
+    <div class="col-4">
+      <dl>
+        <dt><%= Spree::CreditCard.human_attribute_name(:code) %>:</dt>
+        <dd><%= payment.number %></dd>
+
+        <dt><%= t('spree.risk_analysis') %></dt>
+        <dd>
+          <span class="pill pill-<%= payment.risky? ? 'error' : 'complete' %>">
+            <%= "#{t('spree.not') unless payment.risky?} #{t('spree.risky').downcase}".capitalize %>
+          </span>
+        </dd>
+
+        <dt><%= t('spree.status')%></dt>
+        <dd><span class="pill pill-<%= payment.state %>"><%= t(payment.state, scope: 'spree.payment_states') %></span></dd>
+
+
+        <dt><%= Spree::CreditCard.human_attribute_name(:avs_response) %>:</dt>
+        <dd>
+          <%= 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],
+          ) %>
+        </dd>
+
+        <dt><%= Spree::CreditCard.human_attribute_name(:cvv_response_code) %>:</dt>
+        <dd>
+          <%= 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],
+          ) %>
+        </dd>
       </dl>
-      <% if payment.source.address %>
-        <%= render partial: 'spree/admin/shared/address', locals: {address: payment.source.address} %>
-      <% end %>
     </div>
   </div>
 </fieldset>

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

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
 

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

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