Skip to content

Commit 1050ca0

Browse files
kennyadslkennyadsl
authored
Merge pull request #4883 from nebulab/elia+kennyadsl/risky-business
Risk analysis box update
2 parents 2dcd90e + 8f8a331 commit 1050ca0

File tree

10 files changed

+109
-80
lines changed

10 files changed

+109
-80
lines changed

‎backend/app/controllers/spree/admin/payments_controller.rb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ class PaymentsController < Spree::Admin::BaseController
1111
before_action :load_data
1212
before_action :require_bill_address, only: [:index]
1313

14+
helper ::Spree::Admin::OrdersHelper
15+
1416
respond_to :html
1517

1618
def index

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

Lines changed: 19 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1,50 +1,26 @@
1-
<fieldset class="no-border-bottom" id="risk_analysis">
2-
<legend><%= "#{t('spree.risk_analysis')}: #{t('spree.not') unless @order.is_risky?} #{t('spree.risky')}" %></legend>
1+
<details id="risk_analysis">
2+
<summary><%= t('spree.risk_analysis') %>: <%= t('spree.risky') %></summary>
3+
34
<table>
45
<thead>
6+
<th><%= t('spree.payment') %></th>
57
<th><%= t('spree.risk') %></th>
68
<th><%= t('spree.status') %></th>
79
</thead>
8-
<tbody id="risk-analysis" data-hook="order_details_adjustments" class="with-border">
9-
<tr class="">
10-
<td><strong>
11-
<%= t('spree.failed_payment_attempts') %>:
12-
</strong></td>
13-
<td>
14-
<span class="pill pill-<%= @order.payments.failed.count > 0 ? 'warning' : 'complete' %>">
15-
<%= t(
16-
'spree.payments_failed_count',
17-
count: @order.payments.failed.count
18-
) %>
19-
</span>
20-
</td>
21-
</tr>
22-
23-
<tr>
24-
<td><strong><%= t('spree.avs_response') %>:</strong></td>
25-
<td>
26-
<span class="pill pill-<%= latest_payment.is_avs_risky? ? 'warning' : 'complete' %>">
27-
<% if latest_payment.is_avs_risky? %>
28-
<%= avs_response_code[latest_payment.avs_response] %>
29-
<% else %>
30-
<%= t('spree.success') %>
31-
<% end %>
32-
</span>
33-
</td>
34-
</tr>
35-
36-
<tr>
37-
<td><strong><%= t('spree.cvv_response') %>:</strong></td>
38-
<td>
39-
<span class="pill pill-<%= latest_payment.is_cvv_risky? ? 'warning' : 'complete' %>">
40-
<% if latest_payment.is_cvv_risky? %>
41-
<%= cvv_response_code[latest_payment.cvv_response_code] %>
42-
<% else %>
43-
<%= t('spree.success') %>
44-
<% end %>
45-
</span>
46-
</td>
47-
</tr>
10+
<tbody id="risk-analysis" data-hook="order_details_adjustments" class="with-border">
11+
<% order.payments.risky.each do |payment| %>
12+
<tr class="">
13+
<td><%= link_to payment.number, admin_order_payment_path(order, payment) %></td>
14+
<td>
15+
<span class="pill pill-warning"><%= t('spree.risky') %></span>
16+
</td>
17+
<td>
18+
<span class="pill pill-<%= payment.state %>">
19+
<%= t(payment.state, scope: 'spree.payment_states') %>
20+
</span>
21+
</td>
22+
</tr>
23+
<% end %>
4824
</tbody>
4925
</table>
50-
</fieldset>
26+
</details>

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@
1515
<%= render partial: 'spree/shared/error_messages', locals: { target: @order } %>
1616
</div>
1717

18-
<% if @order.payments.exists? && @order.is_risky? %>
19-
<%= render 'spree/admin/orders/risk_analysis', latest_payment: @order.payments.reorder("created_at DESC").first %>
18+
<% if @order.is_risky? %>
19+
<%= render 'spree/admin/orders/risk_analysis', order: @order %>
2020
<% end %>
2121

2222
<div data-hook="admin_order_edit_form">

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@
1414
<%= render partial: 'spree/shared/error_messages', locals: { target: @order } %>
1515
</div>
1616

17-
<% if @order.payments.exists? && @order.is_risky? %>
18-
<%= render 'spree/admin/orders/risk_analysis', latest_payment: @order.payments.reorder("created_at DESC").first %>
17+
<% if @order.is_risky? %>
18+
<%= render 'spree/admin/orders/risk_analysis', order: @order %>
1919
<% end %>
2020

2121
<div data-hook="admin_order_edit_sub_header"></div>

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@
2222
<tbody>
2323
<% payments.each do |payment| %>
2424
<tr id="<%= dom_id(payment) %>" data-hook="payments_row" class="payment vertical-middle" data-payment-id="<%= payment.id %>">
25-
<td><%= link_to payment.number, spree.admin_order_payment_path(@order, payment) %></td>
25+
<td>
26+
<%= tag :i, class: "fa fa-warning red", title: t('spree.risky') if payment.risky? %>
27+
<%= link_to payment.number, spree.admin_order_payment_path(@order, payment) %>
28+
</td>
2629
<td><%= l(payment.created_at, format: :short) %></td>
2730
<td><%= payment.payment_method.name %></td>
2831
<td><%= payment.transaction_id %></td>

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

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,53 @@
1515

1616
<dt><%= Spree::CreditCard.human_attribute_name(:expiration) %>:</dt>
1717
<dd><%= payment.source.month %>/<%= payment.source.year %></dd>
18+
19+
<% if payment.source.address %>
20+
<dt><%= Spree::CreditCard.human_attribute_name(:source_address) %>:</dt>
21+
<dd><%= render partial: 'spree/admin/shared/address', locals: {address: payment.source.address} %></dd>
22+
<% end %>
23+
</dl>
24+
</div>
25+
26+
<div class="col-2">
27+
</div>
28+
29+
<div class="col-4">
30+
<dl>
31+
<dt><%= Spree::CreditCard.human_attribute_name(:code) %>:</dt>
32+
<dd><%= payment.number %></dd>
33+
34+
<dt><%= t('spree.risk_analysis') %></dt>
35+
<dd>
36+
<span class="pill pill-<%= payment.risky? ? 'error' : 'complete' %>">
37+
<%= "#{t('spree.not') unless payment.risky?} #{t('spree.risky').downcase}".capitalize %>
38+
</span>
39+
</dd>
40+
41+
<dt><%= t('spree.status')%></dt>
42+
<dd><span class="pill pill-<%= payment.state %>"><%= t(payment.state, scope: 'spree.payment_states') %></span></dd>
43+
44+
45+
<dt><%= Spree::CreditCard.human_attribute_name(:avs_response) %>:</dt>
46+
<dd>
47+
<%= content_tag(
48+
:span,
49+
payment.is_avs_risky? ? t('spree.failure') : t('spree.success'),
50+
class: "pill pill-#{payment.is_avs_risky? ? 'warning' : 'complete'}",
51+
title: avs_response_code[payment.avs_response],
52+
) %>
53+
</dd>
54+
55+
<dt><%= Spree::CreditCard.human_attribute_name(:cvv_response_code) %>:</dt>
56+
<dd>
57+
<%= content_tag(
58+
:span,
59+
payment.is_cvv_risky? ? t('spree.failure') : t('spree.success'),
60+
class: "pill pill-#{payment.is_cvv_risky? ? 'warning' : 'complete'}",
61+
title: cvv_response_code[payment.cvv_response_code],
62+
) %>
63+
</dd>
1864
</dl>
19-
<% if payment.source.address %>
20-
<%= render partial: 'spree/admin/shared/address', locals: {address: payment.source.address} %>
21-
<% end %>
2265
</div>
2366
</div>
2467
</fieldset>

‎core/README.md‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@ source (e.g. `Spree::CreditCard`) using a specific payment method (e.g
4040
`Solidus::Gateway::Braintree`).
4141
* `Spree::PaymentMethod` - A base class which is used for implementing payment methods.
4242
* `Spree::PaymentMethod::CreditCard` - An implementation of a `Spree::PaymentMethod` for credit card payments.
43-
See https://github.com/solidusio/solidus_gateway/ for officially supported payment method implementations.
4443
* `Spree::CreditCard` - The `source` of a `Spree::Payment` using `Spree::PaymentMethod::CreditCard` as payment method.
4544

4645
## Inventory sub-system

‎core/app/models/spree/payment.rb‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ class Payment < Spree::Base
5454
scope :processing, -> { with_state('processing') }
5555
scope :failed, -> { with_state('failed') }
5656

57-
scope :risky, -> { where("avs_response IN (?) OR (cvv_response_code IS NOT NULL and cvv_response_code != 'M') OR state = 'failed'", RISKY_AVS_CODES) }
57+
scope :risky, -> { failed.or(where(avs_response: RISKY_AVS_CODES)).or(where.not(cvv_response_code: [nil, '', 'M'])) }
5858
scope :valid, -> { where.not(state: %w(failed invalid void)) }
5959

6060
scope :store_credits, -> { where(source_type: Spree::StoreCredit.to_s) }
@@ -127,6 +127,11 @@ def payment_source
127127
res || payment_method
128128
end
129129

130+
# @return [Boolean] true when this payment is risky
131+
def risky?
132+
is_avs_risky? || is_cvv_risky? || state == 'failed'
133+
end
134+
130135
# @return [Boolean] true when this payment is risky based on address
131136
def is_avs_risky?
132137
return false if avs_response.blank? || NON_RISKY_AVS_CODES.include?(avs_response)
@@ -137,7 +142,6 @@ def is_avs_risky?
137142
def is_cvv_risky?
138143
return false if cvv_response_code == "M"
139144
return false if cvv_response_code.nil?
140-
return false if cvv_response_message.present?
141145
true
142146
end
143147

‎core/app/models/spree/payment_method/credit_card.rb‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,6 @@ module Spree
44
# An implementation of a `Spree::PaymentMethod` for credit card payments.
55
#
66
# It's a good candidate as base class for other credit card based payment methods.
7-
#
8-
# See https://github.com/solidusio/solidus_gateway/ for
9-
# officially supported payment method implementations.
10-
#
117
class PaymentMethod::CreditCard < PaymentMethod
128
def payment_source_class
139
Spree::CreditCard

‎core/spec/models/spree/payment_spec.rb‎

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -46,14 +46,29 @@
4646
)
4747
end
4848

49-
context '.risky' do
49+
context 'risk analysis' do
5050
let!(:payment_1) { create(:payment, avs_response: 'Y', cvv_response_code: 'M', cvv_response_message: 'Match') }
5151
let!(:payment_2) { create(:payment, avs_response: 'Y', cvv_response_code: 'M', cvv_response_message: '') }
5252
let!(:payment_3) { create(:payment, avs_response: 'A', cvv_response_code: 'M', cvv_response_message: 'Match') }
5353
let!(:payment_4) { create(:payment, avs_response: 'Y', cvv_response_code: 'N', cvv_response_message: 'No Match') }
54+
let!(:payment_5) { create(:payment, avs_response: 'Y', cvv_response_code: 'M', cvv_response_message: '', state: 'failed') }
5455

55-
it 'should not return successful responses' do
56-
expect(subject.class.risky.to_a).to match_array([payment_3, payment_4])
56+
describe '.risky' do
57+
it 'fetches only risky payments' do
58+
expect(subject.class.risky.to_a).to match_array([payment_3, payment_4, payment_5])
59+
end
60+
end
61+
62+
context '#risky?' do
63+
it 'is true for risky payments' do
64+
aggregate_failures do
65+
expect(payment_1).not_to be_risky
66+
expect(payment_2).not_to be_risky
67+
expect(payment_3).to be_risky
68+
expect(payment_4).to be_risky
69+
expect(payment_5).to be_risky
70+
end
71+
end
5772
end
5873
end
5974

@@ -1207,7 +1222,7 @@
12071222
end
12081223
end
12091224

1210-
describe "is_avs_risky?" do
1225+
describe "#is_avs_risky?" do
12111226
it "returns false if avs_response included in NON_RISKY_AVS_CODES" do
12121227
('A'..'Z').reject{ |x| subject.class::RISKY_AVS_CODES.include?(x) }.to_a.each do |char|
12131228
payment.update_attribute(:avs_response, char)
@@ -1231,26 +1246,17 @@
12311246
end
12321247
end
12331248

1234-
describe "is_cvv_risky?" do
1235-
it "returns false if cvv_response_code == 'M'" do
1236-
payment.update_attribute(:cvv_response_code, "M")
1237-
expect(payment.is_cvv_risky?).to eq(false)
1238-
end
1239-
1240-
it "returns false if cvv_response_code == nil" do
1241-
payment.update_attribute(:cvv_response_code, nil)
1242-
expect(payment.is_cvv_risky?).to eq(false)
1243-
end
1244-
1245-
it "returns false if cvv_response_message == ''" do
1246-
payment.update_attribute(:cvv_response_message, '')
1247-
expect(payment.is_cvv_risky?).to eq(false)
1249+
describe "#is_cvv_risky?" do
1250+
['M', nil].each do |char|
1251+
it "returns false if cvv_response_code is #{char.inspect}" do
1252+
payment.cvv_response_code = char
1253+
expect(payment.is_cvv_risky?).to eq(false)
1254+
end
12481255
end
12491256

1250-
it "returns true if cvv_response_code == [A-Z], omitting D" do
1251-
# should use cvv_response_code helper
1252-
(%w{N P S U} << "").each do |char|
1253-
payment.update_attribute(:cvv_response_code, char)
1257+
['', *('A'...'M'), *('N'..'Z')].each do |char|
1258+
it "returns true if cvv_response_code is #{char.inspect} (not 'M' or nil)" do
1259+
payment.cvv_response_code = char
12541260
expect(payment.is_cvv_risky?).to eq(true)
12551261
end
12561262
end

0 commit comments

Comments
 (0)