Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix failed SCA authentication on subs. order payment not reflected in OFN #7562

Merged

Conversation

sauloperez
Copy link
Contributor

@sauloperez sauloperez commented May 6, 2021

What? Why?

Closes #7509. This validates payment intents against Stripe and displays all the possible errors to the user.

This is how it looks:

Peek 2021-05-06 12-29

What should we test?

The exact scenario described in #7509.

Release notes

Fix Stripe's unauthorized payments not reflected in the order and show any possible error with the payment to the user.
Changelog Category: User facing changes

@sauloperez sauloperez requested a review from andrewpbrett May 6, 2021 10:54
@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #7562 (16cda3d) into master (a7331ef) will decrease coverage by 2.56%.
The diff coverage is 96.87%.

❗ Current head 16cda3d differs from pull request most recent head 9a63f38. Consider uploading reports for the commit 9a63f38 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7562      +/-   ##
==========================================
- Coverage   93.19%   90.62%   -2.57%     
==========================================
  Files         635      635              
  Lines       18115    18227     +112     
==========================================
- Hits        16882    16518     -364     
- Misses       1233     1709     +476     
Impacted Files Coverage Δ
app/services/process_payment_intent.rb 97.05% <96.00%> (-2.95%) ⬇️
app/controllers/spree/orders_controller.rb 86.55% <100.00%> (+0.34%) ⬆️
app/models/spree/payment.rb 99.09% <100.00%> (+0.01%) ⬆️
app/jobs/heartbeat_job.rb 0.00% <0.00%> (-100.00%) ⬇️
app/jobs/bulk_invoice_job.rb 0.00% <0.00%> (-100.00%) ⬇️
app/mailers/producer_mailer.rb 0.00% <0.00%> (-100.00%) ⬇️
app/services/mail_configuration.rb 0.00% <0.00%> (-100.00%) ⬇️
app/components/example_component.rb 0.00% <0.00%> (-100.00%) ⬇️
app/services/bulk_invoice_service.rb 0.00% <0.00%> (-100.00%) ⬇️
app/jobs/subscription_placement_job.rb 0.00% <0.00%> (-98.42%) ⬇️
... and 44 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 a7331ef...9a63f38. Read the comment docs.

@sauloperez sauloperez self-assigned this May 6, 2021
@sauloperez sauloperez changed the title Fix failed sca auth Fix failed SCA auth May 6, 2021
@sauloperez sauloperez force-pushed the fix-failed-sca-auth branch from eabfd5e to 051bb9a Compare May 6, 2021 11:21
sauloperez added 4 commits May 6, 2021 16:19
Now the existing validation is redundant. It's Stripe's API who does
that now. It's up to them to decide what's a valid intent.
This gives us the opportunity to retry the operation in case the
processing fails.
@sauloperez sauloperez force-pushed the fix-failed-sca-auth branch from 051bb9a to 69b91ea Compare May 6, 2021 14:19
@sauloperez sauloperez marked this pull request as ready for review May 6, 2021 14:20
@sauloperez sauloperez changed the title Fix failed SCA auth Fix failed SCA authentication not reflected in OFN May 6, 2021
@sauloperez sauloperez changed the title Fix failed SCA authentication not reflected in OFN Fix failed SCA authentication on subs. order payment not reflected in OFN May 6, 2021
@sauloperez
Copy link
Contributor Author

I couldn't figure out how to ask Codecov to reassess the branch. That's clearly wrong.

@sauloperez
Copy link
Contributor Author

😮 it looks good now. It may handle it eventually 🤷

flash[:error] = "#{I18n.t("payment_could_not_process")}. #{result.error}"
end
@order.reload
end
Copy link
Contributor Author

@sauloperez sauloperez May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to move this to its own purpose-specific controller action in an upcoming PR so we don't mix the two concerns and potentially introduce bugs. This will also make it much easier to improve this further us outlined on #7509 (comment).

if params.key?("payment_intent")
result = ProcessPaymentIntent.new(params["payment_intent"], @order).call!
unless result.ok?
flash[:error] = "#{I18n.t("payment_could_not_process")}. #{result.error}"
Copy link
Contributor

@Matt-Yorkley Matt-Yorkley May 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a path where #ok? returns false but #error is nil (line 34 in the service), right?. Do we want to add a message there instead? Or avoid setting flash[:error] with an empty message? Otherwise I think the user will see a red flash message box but with no message in it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to check but I purposefully left the empty string as the default Result error message so this doesn't happen.

Copy link
Contributor Author

@sauloperez sauloperez May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now. In that case #ok? returns false but #error is an empty string. As we don't pass a specific error message flash[:error] shows a generic The payment could not be completed. This is exactly what I reproduced on https://github.com/openfoodfoundation/openfoodnetwork/pull/7571/files#diff-0a685f6019d68e59a8758de19d720e12dbc646b712de620e7f066226e181417fR162.

I left this open for further improvement. If we need to in the future, we can extract a custom validation class and be more specific. For the purpose of this bugfix it felt too much.

Copy link
Contributor

@andrewpbrett andrewpbrett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks excellent, thanks @sauloperez. The change that Matt mentions will be nice, as well as moving to a specific controller method. But that can come later 👍.

@@ -3,39 +3,72 @@
# When directing a customer to Stripe to authorize the payment, we specify a
# redirect_url that Stripe should return them to. When checking out, it's
# /checkout; for admin payments and subscription payemnts it's the order url.
#
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is crucial 😂 #OCD

@filipefurtad0 filipefurtad0 self-assigned this May 6, 2021
@filipefurtad0 filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels May 6, 2021
@filipefurtad0
Copy link
Contributor

Hey @sauloperez ,

This looks good now. Did a brief sanity check, to see if authenticating the payment still leads to a complete and paid order:

Peek 2021-05-06 20-11

Looks good 👍 After that, I reproduced the scenario leading to the bug, and now (as on the video above) it leads to the expected scenario:

  • customer: the order appears as non paid
  • admin: nothing changes

Peek 2021-05-06 19-58

If we do this the other way around - fail payment, then place a new sub with a valid payment - we get the flash message, but this is rather a previously observed "memory effect" on flash messages... It looks weird, but it's actually a separated issue:

Peek 2021-05-06 20-01

So, in summary, this is good to go 💪
Was great to pair on this! 🙌

The followup issues required (on my to-do list) are:

  • No possibility for the admin to generate a new authentication link to the customer, after failed authentication (proposing bug-s3)
  • The customer has a "green screen", despite failed payment: UI could be improved to display a more visible payment status (UI enhancement, it's not really a bug)
  • "memory effect" on flash messages (proposing bug-s4)

@andrewpbrett andrewpbrett merged commit 415d86b into openfoodfoundation:master May 6, 2021
@sauloperez sauloperez deleted the fix-failed-sca-auth branch May 7, 2021 06:12
@sauloperez
Copy link
Contributor Author

"memory effect" on flash messages (proposing bug-s4)

That is concerning 🤔 I think it deserves to be s3 because it's really misleading for the user.

@Matt-Yorkley
Copy link
Contributor

"memory effect" on flash messages (proposing bug-s4)

We had a similar issue in the past around checkout flash messages: https://github.com/openfoodfoundation/openfoodnetwork/pull/5906/files

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed authentication on Stripe-SCA payment leads to "paid" payment state
4 participants