Stripe stock check #7779

Matt-Yorkley · 2021-06-11T14:38:45Z

What? Why?

Related to #7692

Handles Stripe payments when checkout fails due to stock issues (frontoffice).

This can occur when stock is reduced after the user is redirected to Stripe and before they are redirected back. The stock is insufficient, the order is not complete, the user is bounced back to the cart, but a pending Stripe payment is left on the order.

This explicitly voids the leftover payment when the checkout fails in this case, so it's not sitting there in the Stripe account. Also improves user feedback so the user is informed the payment they just authorized has been canceled instead of captured.

What should we test?

Checkout with Stripe where the user is redirected for authorization and the stock runs out before they have finished authorizing.

Release notes

Changelog Category: Technical changes

codecov · 2021-06-11T14:48:18Z

Codecov Report

Merging #7779 (eaebe53) into master (9795734) will decrease coverage by 78.01%.
The diff coverage is 14.28%.

❗ Current head eaebe53 differs from pull request most recent head 8e1631b. Consider uploading reports for the commit 8e1631b to get more accurate results

@@             Coverage Diff             @@
##           master    #7779       +/-   ##
===========================================
- Coverage   93.27%   15.25%   -78.02%     
===========================================
  Files         637      568       -69     
  Lines       18136    22858     +4722     
===========================================
- Hits        16916     3488    -13428     
- Misses       1220    19370    +18150

Impacted Files	Coverage Δ
app/controllers/checkout_controller.rb	`0.00% <0.00%> (-98.50%)`	⬇️
app/controllers/locales_controller.rb	`0.00% <0.00%> (ø)`
app/controllers/spree/user_passwords_controller.rb	`0.00% <0.00%> (-60.87%)`	⬇️
lib/open_food_network/i18n_config.rb	`91.66% <50.00%> (-8.34%)`	⬇️
app/models/spree/payment.rb	`60.50% <100.00%> (-38.65%)`	⬇️
app/jobs/job_logger.rb	`0.00% <0.00%> (-100.00%)`	⬇️
app/jobs/heartbeat_job.rb	`0.00% <0.00%> (-100.00%)`	⬇️
app/models/exchange_fee.rb	`0.00% <0.00%> (-100.00%)`	⬇️
app/services/unit_price.rb	`0.00% <0.00%> (-100.00%)`	⬇️
app/models/order_balance.rb	`0.00% <0.00%> (-100.00%)`	⬇️
... and 583 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9795734...8e1631b. Read the comment docs.

andrewpbrett

This looks great, we just need a small update based on #7708 being merged.

andrewpbrett · 2021-06-22T21:21:17Z

app/controllers/checkout_controller.rb

@@ -102,7 +102,10 @@ def valid_order_line_items?
        distributes_order_variants?(@order)
  end

-  def redirect_to_cart_path
+  def handle_invalid_stock


Nice renaming 👍

andrewpbrett · 2021-06-22T21:29:48Z

app/controllers/checkout_controller.rb

+  def cancel_incomplete_payments
+    # The checkout could not complete due to stock running out. We void any pending (incomplete)
+    # Stripe payments here as the order will need to be changed and resubmitted (or abandoned).
+    @order.payments.pending.each(&:void!)


Now that #7708 is merged, I think the payment will be in the requires_authorization state in OFN. Since this code is being called in the case where the authorization succeeded, we should probably move the payment to the complete state (to reflect the successful authorization) and then call void on it because the stock is no longer available. We'll want to rebase before testing as well.

mkllnk

Nice!

I'm moving it back to in-dev for rebasing and conflict resolution.

This can occur when stock is reduced after the user is redirected to Stripe and before they are redirected back. The stock is insufficient, the order is not complete, the user is bounced back to the cart, but a *pending* Stripe payment is left on the order.

…ue to stock changes

Matt-Yorkley · 2021-07-09T12:34:10Z

I've rebased and added a couple of commits here to allow voiding payments in requires_authorization state. Moving back to code review.

filipefurtad0 · 2021-07-11T16:05:27Z

(edited)

Hey Matt,

Indeed... Before staging this PR it is possible to reproduce the case in which the stock is reduced while the customer awaits for bank authorization. In this case, the customer is redirected to the /cart page, with the usual "out of stock" error message:

We should note, that the transaction fee is charged, although the stock was set to zero, in the BO. Also, we can see on Stripe that the payment actually gets authorized, although not yet captured (right side, pic below).

As an admin, we can check this order - taking the order number from the Stripe payment - and verify it was left in a Payment state (left side, pic below):

The error message is displayed when one tries to access the Payments tab.

After staging this PR:

Repeating the same steps, the customer is redirected as well to the /cart page, but sees a message indicating that the payment has been cancelled:

However, two issues I'm not sure about:

i) it seems that irrespective of emptying the cart before proceeding with another order, following payment attempts are adding up the respective transaction fees:

ii) These payments are authorized - but not captured, as before - and seem to always be associated to the same order number:

So, I think we could merge this as neither i) or ii) are introduced here, but I'm not sure if it is within scope of this PR to prevent both these from occurring - adding the feedback needed label.

Otherwise we can end up with duplicate transaction fees for voided payments.

Matt-Yorkley · 2021-07-12T07:30:25Z

Thanks @filipefurtad0 I added an extra commit so now those transaction fees on canceled payment should be marked as ineligible and should not appear in the order total.

filipefurtad0 · 2021-07-13T12:10:44Z

Hey @Matt-Yorkley ,

That's great, after this PR and going through the same scenario as before:

add stock to cart
checkout with Stripe SCA
wait till the "Authorization screen" is displayed on the UI (customer)
set available stock to zero (admin)
Click Complete Authentication (1) (customer) -> redirection to /cart page; banner about cancelled payment appears
resolve the stock conflict (for ex., by adding new stock - as admin)
Proceed to checkout again and click Place Order
When prompted, click Complete Authentication (2) (customer) -> redirection to Order confirmation page; transaction fee only charged once 🎉

So far so good 👍

However, from the notes on this PR:

This explicitly voids the leftover payment when the checkout fails in this case, so it's not sitting there in the Stripe account.

We still can see both payments on Stripe, (1) and (2):

The first payment attempt (1) above appears in Stripe as an authorized, but Uncaptured payment;
The second and successful payment attempt appears in Stripe as a Succeeded payment;

Should we merge and address this second bit in a separate PR?

filipefurtad0 · 2021-07-13T12:20:23Z

Should we merge and address this second bit in a separate PR?

We'll go with this approach, as discussed via DM 👍

Matt-Yorkley · 2021-07-13T12:46:30Z

Merging (following discussion in Slack) 👍

Matt-Yorkley self-assigned this Jun 11, 2021

Matt-Yorkley marked this pull request as ready for review June 14, 2021 18:53

andrewpbrett reviewed Jun 22, 2021

View reviewed changes

mkllnk approved these changes Jul 9, 2021

View reviewed changes

Matt-Yorkley added 3 commits July 9, 2021 13:13

Use Order::Contents service in test setup

2cd6b05

Add test for insufficient stock behavior after Stripe redirect

a199f80

Matt-Yorkley force-pushed the stripe-stock-check branch from 9575d3c to a199f80 Compare July 9, 2021 12:14

Matt-Yorkley added 4 commits July 9, 2021 13:24

Allow payments in requires_authorization state to be voided

b1db06a

Add :incomplete scope in Spree::Payment

b4a9a6f

Tidy up :valid scope in Spree::Payment

2b8b690

Use :incomplete scope when voiding payments that can't be processed d…

e6d9545

…ue to stock changes

andrewpbrett approved these changes Jul 10, 2021

View reviewed changes

andrewpbrett mentioned this pull request Jul 10, 2021

Payment registered in Stripe when OFN declared a problem #7692

Closed

filipefurtad0 self-assigned this Jul 11, 2021

filipefurtad0 added pr-staged-au staging.openfoodnetwork.org.au pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-au staging.openfoodnetwork.org.au labels Jul 11, 2021

filipefurtad0 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jul 11, 2021

Set adjustments associated with voided payments to ineligible.

8e1631b

Otherwise we can end up with duplicate transaction fees for voided payments.

Matt-Yorkley force-pushed the stripe-stock-check branch from 83980a6 to 8e1631b Compare July 12, 2021 07:28

filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed feedback-needed labels Jul 13, 2021

filipefurtad0 removed the pr-staged-uk staging.openfoodnetwork.org.uk label Jul 13, 2021

filipefurtad0 mentioned this pull request Jul 13, 2021

Payment attempts on orders with stock restrictions appear as authorized and "uncaptured" on Stripe #7922

Closed

Matt-Yorkley merged commit 0d16be3 into openfoodfoundation:master Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stripe stock check #7779

Stripe stock check #7779

Matt-Yorkley commented Jun 11, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021 •

edited

Loading

andrewpbrett left a comment

andrewpbrett Jun 22, 2021

andrewpbrett Jun 22, 2021

mkllnk left a comment

Matt-Yorkley commented Jul 9, 2021

filipefurtad0 commented Jul 11, 2021 •

edited

Loading

Matt-Yorkley commented Jul 12, 2021

filipefurtad0 commented Jul 13, 2021

filipefurtad0 commented Jul 13, 2021

Matt-Yorkley commented Jul 13, 2021

Stripe stock check #7779

Stripe stock check #7779

Conversation

Matt-Yorkley commented Jun 11, 2021 • edited Loading

What? Why?

What should we test?

Release notes

codecov bot commented Jun 11, 2021 • edited Loading

Codecov Report

andrewpbrett left a comment

Choose a reason for hiding this comment

andrewpbrett Jun 22, 2021

Choose a reason for hiding this comment

andrewpbrett Jun 22, 2021

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

Matt-Yorkley commented Jul 9, 2021

filipefurtad0 commented Jul 11, 2021 • edited Loading

Matt-Yorkley commented Jul 12, 2021

filipefurtad0 commented Jul 13, 2021

filipefurtad0 commented Jul 13, 2021

Matt-Yorkley commented Jul 13, 2021

Matt-Yorkley commented Jun 11, 2021 •

edited

Loading

codecov bot commented Jun 11, 2021 •

edited

Loading

filipefurtad0 commented Jul 11, 2021 •

edited

Loading