Handle credit validation errors #5780

sauloperez · 2020-07-17T15:39:25Z

What? Why?

Note this is based on #5806.

I'm not particularly happy about having to come down to the attr_accessor trick to skip a callback but this prevents this fix from growing exponentially on LOCs and avoids the risk of making the wrong assumptions. What I like about it is that skip_source_validation is explicit enough to pave the way for extracting specific per use-case service objects in the future.

What should we test?

We need to cover all payment scenarios:

Issue a partial credit with each supported payment gateway
Issue a void operation with each supported payment gateway
Issue a capture operation with each supported payment gateway
Place a regular order as a consumer
Place an order as an admin

When performing these actions as admin, the payment total should be accurate, taking into account any payment fees.

Release notes

Handle credit and other payment actions whose credit card is now expired. Stripe kept track of them and we weren't
Changelog Category: Fixed

luisramos0

nice, go! 🚀

app/models/spree/payment.rb

spec/models/spree/payment_spec.rb

luisramos0

Awesome, this is good to go.

I left some comments, the main one is about the code comment, basically I think the "chain of validations" is not related to the skip_source_validation attribute but to the invalidate_old_payments, so the comment about that would need to go there?

spec/models/spree/payment_spec.rb

app/models/spree/payment.rb

sauloperez · 2020-07-22T15:06:30Z

@luisramos0 I addressed your comments plus a commit that handles #refund!.

luisramos0

perfect 👌

Matt-Yorkley · 2020-07-23T10:58:19Z

app/models/spree/payment.rb

    def invalidate_old_payments
-      order.payments.with_state('checkout').where("id != ?", id).each(&:invalidate!)
+      order.payments.with_state('checkout').where("id != ?", id).each do |payment|


I think we can start using #not in Rails 4 for these "where not equals" AR scopes. Not blocking, just mentioning it...

wooow! I totally forgot Rails 4.0 introduced #not!

sauloperez · 2020-07-23T14:29:03Z

As expected, changing from #invalidate! to #update_column doesn't play with the order total calculation in the build. The order total is 10$ higher than expected 🙈 #callbackHell.

The issue is that since we now skip validations and callbacks the after_save :update_order and after_save :ensure_correct_adjustment are not executed and thus, the order doesn't update its totals. But there could be other things the order updater does and other callbacks that we're missing.

sauloperez · 2020-07-23T15:41:06Z

I thought this would take days but I think I managed to solve it. I also addressed your comment @Matt-Yorkley . ProTip: You need a where because what comes out of an association is a #ActiveRecord::AssociationRelation and not a WhereChain. I guess the latter replaces ActiveRecord::Relation in Rails 4.

luisramos0

lgtm it's test ready I think

filipefurtad0 · 2020-07-23T17:23:53Z

Hi @sauloperez and @luisramos0,

The PR introduces an error 500 on /admin/enterprises/<hub_name>/edit

Please see:
https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1595523810421200

sauloperez · 2020-07-23T18:18:56Z

Got it. The migration https://github.com/openfoodfoundation/openfoodnetwork/blob/master/db/migrate/20200616162646_move_all_calculators_outside_the_spree_namespace.rb was applied in that DB but this branch's code still has the calculators within the Spree namespace.

After that, we can TDD a second one that also handles validation errors.

This basically catches ActiveRecord::RecordInvalid caused by an invalid credit record, for instance, but also other situations we haven't forseen.

Given the importance of this code, it doesn't bring me much confidence. Apparently, this specs where using a non-existent state by mistake and this went unnoticed because the payment creation was failing silently in payment/processing.rb. This unearthed the fact that our `#ensure_correct_adjustment` needs the order to be persisted to succeed.

No reason to use a callback when custom validation methods can be defined.

The original payment may not be valid because its credit card may be expired. Stripe gives this as a valid scenario returning a success and we should do too. When creating the credit payment we end up validating all sources in a chain as follows. ``` Payment being persisted -> source payment -> original credit card. ``` The source payment was valid when created (It would not be persisted otherwise) but its source card may now be expired, and that's legit. There was also an issue with the `#invalidate_old_payments` callback. It was causing the original payment to be validated again and thus the credit payment failed to be persisted due to the original credit card being expired. Switching this callback to use `#update_column` skips validations and so we don't validate the source payment. We only care about the state there, so it should be fine.

Switching from `#invalidate` to `#update_column` skipped both validations and callbacks and thus, `#ensure_correct_adjustments` was no longer called for older payments.

sauloperez · 2020-07-23T18:26:05Z

I forced push to get the latest changes of #5806. This should put both FR staging's DB and the calculators' code in sync

sauloperez · 2020-07-23T18:43:29Z

@filipefurtad0 it's solved now. https://staging.openfoodfrance.org/admin/enterprises/fricando-hub/edit works.

filipefurtad0 · 2020-07-24T13:33:53Z

Hey @sauloperez ,

I've reversed the order, and numbered the test cases:

i) Place regular orders, as a consumer
ii) Place orders as admin
iii) Issue a partial credit with each supported payment gateway
iv) Issue a void operation with each supported payment gateway
v) Issue a capture operation with each supported payment gateway

There is quite a lot going on here, so let's go, vaya vayita! :-)

Set different enterprise fees, shipping fees, transaction fees. The shop charges GST. Placed 6 orders, with different payment methods in the shopfront. All payment methods have the same transaction fee.

i) Placing 6 orders, with these payment methods:

Paypal
Cash
Stripe-SCA (paid with 3D card)
Stripe-SCA (paid with regular)
Stripe-Connect
Pin payments

The value of the orders should be and is the same, as checked on confirmation e-mails and under /orders.

ii) Placing the same 6 orders, on the backoffice:

Paypal - No Payment Via Admin Backend
Cash - OK
Stripe-SCA (paid with 3D card) - not supported in the Backoffice
Stripe-SCA (paid with regular) - OK
Stripe-Connect - OK
Pin payments – not supported in the back office - Pin payments not working in backoffice orders #5790

Issue “Backoffice orders: "Balance due" state after payments with a transaction fee #5574” was observed; Used the described workaround and obtained the same values as those obtained in shopfront orders.

iii) Issue a partial credit with each supported payment gateway

Removed 5 items and recalculated fees. Proceeded to Payments:

Paypal - issuing credit is not supported

Cash - issuing credit is not supported

Stripe-SCA (paid with 3D card) - Issuing the credit works. Known issue [When crediting an order the transaction fee is applied when it shouldn't #3490](When crediting an order the transaction fee is applied when it shouldn't)

Refund appears correctly in Stripe Dashboard:

Stripe-SCA (paid with regular) - same as 3)
Stripe-Connect - same as 3)
Pin payments – issuing is not supported:

iv) Issue a void operation with each supported payment gateway:

Paypal - No Payment Via Admin Backend - not supported.
Cash - "Voiding payment does not remove transaction fee Voiding payment does not remove transaction fee #3584"
Stripe-SCA (paid with 3D card) - If one tries to void the payments on the previously edited order, one gets the errors:

voiding the refund:

voiding the initial payment:

Voiding a Stripe payment from an order which was not edited works fine.

Stripe-SCA (paid with regular) - same as 3)
Stripe-Connect - Voiding the initial payment from the order works. Attempting to void the refund, after refunding the whole payment, results in this error:

On Stripe dashboard it can be seen, that a partially refunded order goes from this state:

Into:

Voiding Pin payments – is not supported in the backoffice:

v) Issue a capture operation with each supported payment gateway

I am not 100% sure about this operation @sauloperez . Do you mean this (below) the tick, under /orders? I thought this was only possible for cash payments...

Cash - Capturing payment works

So in summary:

I think there is still an issue with refunds on Stripe-SCA. It should be possible to refund the initial amount, without getting the error. This is perhaps worth opening a new issue.
for Stripe Connect (for which the issue was opened) all looks good.
The PR touches several previously known issues. See epic 5580.

I would say this is ready to go, but I'm especially grateful for feedback here. This is quite extensive and touches many corners. Moving to ready to go, but adding the feedback label. Thank you!

luisramos0 · 2020-07-24T13:48:33Z

Hey @filipefurtad0 thanks a lot for the testing.
The testing looks good. Re the StripeSCA, I think the refund worked well:

I think you tried to void the refund and then void the original payment of an order with a partial refund. I think it's good it failed. After you issue a partial refund, foer example, payed 100 and refunded 50, none of these two movements should be voided. I think all the user can do after this is to issue another partial refund, or collect another payment.

I think what also needs to be tested here are the checkout retrial scenarios. I will have a look at #5816 and #5818 so we can continue the conversation there.

So, I will merge this PR now and look at #5816 and #5818 with and without this PR to see if anything has changed 👍
Thanks Filipe!

filipefurtad0 · 2020-07-24T15:36:00Z

Hey @luisramos0,

Yes, the partial refund works well for both Stripe methods. The subsequent full refund fails for Stripe-SCA (test cases iv) 3 and 4) but works nicely for Stripe-Connect (see test case iv) 5).

If not allowing for a full refund after a partial one is the expected behavior than all is good think, as eventually Stripe-Connect will be discontinued.

I checked anyway for this difference between the two payment methods and it is not introduced by this PR and it does not seem to relate to transaction fees.

luisramos0 · 2020-07-24T19:42:39Z

oh, nice finding. I didnt know that would work in StripeConnect. That's basically lost functionality from StripeConnect to StripeSCA. We should create a specific issue for it and then decide what to do.

The functitonality is "full refund after partial refund", and I think you mean the user presses VOID on the original payment, right?
I wonder if VOID on the partial refund also works on StripeConnect. Voiding a partial refund should work in StripeSCA, I think that could be another separate issue to be looked at.
These are straight forward things to do in StripeSCA, when we have the issues, we can decide if we do them now or later.

filipefurtad0 · 2020-07-27T14:52:02Z

Hi @luisramos0 ,

The functitonality is "full refund after partial refund", and I think you mean the user presses VOID on > the original payment, right?

Yes, that's it.

I wonder if VOID on the partial refund also works on StripeConnect. Voiding a partial refund should work in StripeSCA, I think
that could be another separate issue to be looked at.

Voiding partial refunds do not work in neither Stripe-Connect or -SCA: opened issue #5830

luisramos0 · 2020-07-27T15:01:03Z

Hello Filipe! Thanks for confirming that 👍

sauloperez · 2020-07-28T13:44:19Z

Thanks a lot for this in-depth testing @filipefurtad0 ! We are now in a much better position to fix these things 💪

sauloperez self-assigned this Jul 17, 2020

sauloperez force-pushed the handle-credit-validation-errors branch from aba9a79 to e619080 Compare July 17, 2020 15:44

luisramos0 reviewed Jul 20, 2020

View reviewed changes

app/models/spree/payment.rb Outdated Show resolved Hide resolved

spec/models/spree/payment_spec.rb Outdated Show resolved Hide resolved

sauloperez force-pushed the handle-credit-validation-errors branch from 68903e7 to 899c498 Compare July 22, 2020 12:53

sauloperez marked this pull request as ready for review July 22, 2020 12:58

luisramos0 approved these changes Jul 22, 2020

View reviewed changes

spec/models/spree/payment_spec.rb Show resolved Hide resolved

app/models/spree/payment.rb Show resolved Hide resolved

app/models/spree/payment.rb Outdated Show resolved Hide resolved

app/models/spree/payment.rb Outdated Show resolved Hide resolved

luisramos0 approved these changes Jul 22, 2020

View reviewed changes

sauloperez changed the base branch from master to bring-in-payment-model July 22, 2020 15:20

Matt-Yorkley approved these changes Jul 23, 2020

View reviewed changes

filipefurtad0 self-assigned this Jul 23, 2020

filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 23, 2020

luisramos0 approved these changes Jul 23, 2020

View reviewed changes

sauloperez added 13 commits July 23, 2020 20:24

Test the payment controller handles GatewayError

26ed601

After that, we can TDD a second one that also handles validation errors.

Handle all errors when dealing with payment event

59da07d

This basically catches ActiveRecord::RecordInvalid caused by an invalid credit record, for instance, but also other situations we haven't forseen.

DRY specs and fix rubocop failures

73b1b1f

Replace spec's syntax to RSpec 3

1c02647

Replace before_validation with custom validation

f2b28a1

No reason to use a callback when custom validation methods can be defined.

Handle #refund! as we do with #credit!

c0f72f8

Clarify method documentation

813459e

Add missing attribute to skip source validation

4d9fbb6

Fix simple Rubocop issues

e6943ce

Recalculate adjustments when invalidating payments

357037e

Switching from `#invalidate` to `#update_column` skipped both validations and callbacks and thus, `#ensure_correct_adjustments` was no longer called for older payments.

Replace literal with AR's 4 #not

97f551a

sauloperez force-pushed the handle-credit-validation-errors branch from 8b30858 to 97f551a Compare July 23, 2020 18:24

filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-fr staging.coopcircuits.fr labels Jul 23, 2020

filipefurtad0 added the feedback-needed label Jul 24, 2020

luisramos0 removed feedback-needed pr-staged-uk staging.openfoodnetwork.org.uk labels Jul 24, 2020

luisramos0 merged commit d93c168 into openfoodfoundation:bring-in-payment-model Jul 24, 2020

luisramos0 mentioned this pull request Jul 24, 2020

Bring in payment model #5806

Merged

filipefurtad0 mentioned this pull request Jul 27, 2020

Voiding an initial payment (i.e. a full refund) after partially refunding the order is not possible with Stripe-SCA #5829

Closed

RachL mentioned this pull request Jul 27, 2020

Release v3.2.0 Feebonaci #5823

Closed

7 tasks

sauloperez deleted the handle-credit-validation-errors branch July 28, 2020 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle credit validation errors #5780

Handle credit validation errors #5780

sauloperez commented Jul 17, 2020 •

edited

Loading

luisramos0 left a comment

luisramos0 left a comment

sauloperez commented Jul 22, 2020

luisramos0 left a comment •

edited

Loading

Matt-Yorkley Jul 23, 2020

sauloperez Jul 23, 2020

sauloperez commented Jul 23, 2020 •

edited

Loading

sauloperez commented Jul 23, 2020 •

edited

Loading

luisramos0 left a comment

filipefurtad0 commented Jul 23, 2020 •

edited

Loading

sauloperez commented Jul 23, 2020

sauloperez commented Jul 23, 2020 •

edited

Loading

sauloperez commented Jul 23, 2020 •

edited

Loading

filipefurtad0 commented Jul 24, 2020

luisramos0 commented Jul 24, 2020 •

edited

Loading

filipefurtad0 commented Jul 24, 2020

luisramos0 commented Jul 24, 2020 •

edited

Loading

filipefurtad0 commented Jul 27, 2020 •

edited

Loading

luisramos0 commented Jul 27, 2020

sauloperez commented Jul 28, 2020 •

edited

Loading

Handle credit validation errors #5780

Handle credit validation errors #5780

Conversation

sauloperez commented Jul 17, 2020 • edited Loading

What? Why?

What should we test?

Release notes

luisramos0 left a comment

Choose a reason for hiding this comment

luisramos0 left a comment

Choose a reason for hiding this comment

sauloperez commented Jul 22, 2020

luisramos0 left a comment • edited Loading

Choose a reason for hiding this comment

Matt-Yorkley Jul 23, 2020

Choose a reason for hiding this comment

sauloperez Jul 23, 2020

Choose a reason for hiding this comment

sauloperez commented Jul 23, 2020 • edited Loading

sauloperez commented Jul 23, 2020 • edited Loading

luisramos0 left a comment

Choose a reason for hiding this comment

filipefurtad0 commented Jul 23, 2020 • edited Loading

sauloperez commented Jul 23, 2020

sauloperez commented Jul 23, 2020 • edited Loading

sauloperez commented Jul 23, 2020 • edited Loading

filipefurtad0 commented Jul 24, 2020

luisramos0 commented Jul 24, 2020 • edited Loading

filipefurtad0 commented Jul 24, 2020

luisramos0 commented Jul 24, 2020 • edited Loading

filipefurtad0 commented Jul 27, 2020 • edited Loading

luisramos0 commented Jul 27, 2020

sauloperez commented Jul 28, 2020 • edited Loading

sauloperez commented Jul 17, 2020 •

edited

Loading

luisramos0 left a comment •

edited

Loading

sauloperez commented Jul 23, 2020 •

edited

Loading

sauloperez commented Jul 23, 2020 •

edited

Loading

filipefurtad0 commented Jul 23, 2020 •

edited

Loading

sauloperez commented Jul 23, 2020 •

edited

Loading

sauloperez commented Jul 23, 2020 •

edited

Loading

luisramos0 commented Jul 24, 2020 •

edited

Loading

luisramos0 commented Jul 24, 2020 •

edited

Loading

filipefurtad0 commented Jul 27, 2020 •

edited

Loading

sauloperez commented Jul 28, 2020 •

edited

Loading