Risk analysis box update by elia · Pull Request #4883 · solidusio/solidus

elia · 2023-01-26T16:58:17Z

Summary

While investigating how an order and its payments are marked as risky as part of the solidus_stripe rewrite I bumped into AVS/CCV codes.

Any modern payment provider and risk analysis tool will probably provide some structured data and a score (with thresholds) as the outcome of a risk analysis.

This is a first step toward distancing ourselves from the ruins of solidus_gateway toward per payment-method and per-order risk analysis tools.

Changes

The original risk banner was showing information for the last payment, despite Spree::Order#is_risky? was based on having any of the payments to be considered risky → now it lists information for all risky payments
The fieldset was changed to an expandable details/summary box → vanilla HTML FTW

before

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.
I have documented new code with YARD.

The following are not always needed (~~cross them out~~ if they are not):

I have added automated tests to cover my changes.
I have attached screenshots to demo visual changes.
I have opened a PR to update the guides.
I have updated the README to account for my changes.

waiting-for-dev

Thanks, @elia; I left some comments to understand it better and a non-blocking suggestion about moving the query up.

elia

@waiting-for-dev did a few changes, mostly pushing details of the risk analysis to the partial that is specific to the payment method, so we can stop assuming all payments will have AVS and CVV informations returned by payment providers.

waiting-for-dev

@elia, it looks cohesive now 👍 Please, see my comment, I'm not sure if there's a line that is commented by error.

kennyadsl

Left a couple of questions, but overall this change works for me, thanks Elia.

kennyadsl · 2023-02-01T21:35:44Z

@elia a rebase is needed here after merging #4891 🙏

There's no point in providing the latest payment from outside when the partial is accessing `@order` directly.

`Order#is_risky?` is currently based on whether a payment is risky, but over time the API could be expanded and allow customization.

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.

Use AR and existing scopes to build the query instead of SQL. Add a spec ensuring failed payments are considered risky.

Match the behavior of Payment.risky.

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.

gsmendoza · 2023-02-03T03:32:36Z

-<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>


As a side note, if you say the translation very fast, it sounds like "spreeesky" 😆

elia self-assigned this Jan 26, 2023

github-actions Bot added changelog:solidus_backend Changes to the solidus_backend gem changelog:solidus_core Changes to the solidus_core gem labels Jan 26, 2023

elia marked this pull request as ready for review January 26, 2023 17:37

elia requested a review from a team as a code owner January 26, 2023 17:37

waiting-for-dev reviewed Jan 27, 2023

View reviewed changes

elia marked this pull request as draft January 27, 2023 11:08

elia force-pushed the elia+kennyadsl/risky-business branch from 4510546 to ed88c13 Compare January 27, 2023 18:53

elia commented Jan 27, 2023

View reviewed changes

Comment thread backend/app/views/spree/admin/orders/_risk_analysis.html.erb Outdated

Comment thread core/app/models/spree/payment.rb

Comment thread core/spec/models/spree/payment_spec.rb Outdated

elia force-pushed the elia+kennyadsl/risky-business branch from ed88c13 to fcb58ac Compare January 27, 2023 19:10

elia marked this pull request as ready for review January 27, 2023 19:39

elia requested a review from waiting-for-dev January 27, 2023 19:40

waiting-for-dev approved these changes Jan 30, 2023

View reviewed changes

Comment thread backend/app/views/spree/admin/payments/_list.html.erb Outdated

kennyadsl reviewed Jan 31, 2023

View reviewed changes

Comment thread backend/app/views/spree/admin/orders/_risk_analysis.html.erb Outdated

Comment thread backend/app/views/spree/admin/payments/_list.html.erb Outdated

elia force-pushed the elia+kennyadsl/risky-business branch from fcb58ac to 384fafc Compare February 1, 2023 09:44

elia requested a review from kennyadsl February 1, 2023 09:48

gsmendoza reviewed Feb 2, 2023

View reviewed changes

Comment thread backend/app/views/spree/admin/orders/_risk_analysis.html.erb Outdated

gsmendoza reviewed Feb 2, 2023

View reviewed changes

Comment thread core/spec/models/spree/payment_spec.rb Outdated

gsmendoza reviewed Feb 2, 2023

View reviewed changes

Comment thread core/spec/models/spree/payment_spec.rb Outdated

elia added 7 commits February 2, 2023 11:44

Fetch the latest payment within the admin risky analysis partial

3f4d56f

There's no point in providing the latest payment from outside when the partial is accessing `@order` directly.

Only use Order#is_risky? to render the admin risk analysis partial

32ec90e

`Order#is_risky?` is currently based on whether a payment is risky, but over time the API could be expanded and allow customization.

Remove references to solidus_gateway, now archived

28628d7

Fix Payment#is_cvv_risky?

ad17b62

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.

Refactor Payment.risky

e633805

Use AR and existing scopes to build the query instead of SQL. Add a spec ensuring failed payments are considered risky.

Add Payment#risky?

bca3790

Match the behavior of Payment.risky.

elia force-pushed the elia+kennyadsl/risky-business branch from 384fafc to 8f8a331 Compare February 2, 2023 10:45

elia requested a review from gsmendoza February 2, 2023 15:43

gsmendoza reviewed Feb 3, 2023

View reviewed changes

gsmendoza approved these changes Feb 3, 2023

View reviewed changes

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

kennyadsl merged commit 1050ca0 into solidusio:master Feb 3, 2023

kennyadsl deleted the elia+kennyadsl/risky-business branch February 3, 2023 16:25

Uh oh!

Conversation

elia commented Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Checklist

Uh oh!

waiting-for-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waiting-for-dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kennyadsl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kennyadsl commented Feb 1, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsmendoza Feb 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elia commented Jan 26, 2023 •

edited

Loading