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

Make charges update method update the first pending payment #5798

Merged

Conversation

luisramos0
Copy link
Contributor

@luisramos0 luisramos0 commented Jul 21, 2020

What? Why?

Closes #5774

In the order workflow there's a step where the fees are recalculated, charge_shipping_and_payment_fees! (there are multiple places where the fees are updated, this is one of them see_no_evil). In this method we are updating the payment amount based on the order total, but the process is selecting the first payment of the order (not the first pending payment) which can result in updating the total in a past failed payment, not the payment that will be processed.
For some reason in rails 3 the first payment was the pending one and now in rails 4 the first one is coming up as one of the previous/failed payments. The amount of the previous/failed payment is updated and the pending payment that will be charged now keeps the amount without fees and thus the issue reported.

This PR updates the code to look at pending payments only heavy_check_mark
I could replicate the problem locally with live AUS data and I could verify the PR fixes the issue. This has been deployed in AUS live as v3.1.2 and it has fixed the issue.

There are auto specs missing (I didnt have time to add them this week). I think we can create an issue to add them next week.

What should we test?

Try to replicate the issue. I am not sure exactly how (I couldnt replicate this with sample data, only with live aus data with a broken stripe setup) but it's all about making the first checkout attempt fail and then try again and see that the payment in the second checkout attempt doesn't include fees.
Sorry, not very specific instructions.

Release notes

Changelog Category: Fixed
Several checkout attempts will now always charge all the fees of the order.

Updating the first overall payment could select a failed payment and ignore the pending payment that is about to be processed
@luisramos0 luisramos0 self-assigned this Jul 21, 2020
@filipefurtad0 filipefurtad0 self-assigned this Jul 23, 2020
@filipefurtad0
Copy link
Contributor

Hi @luisramos0 ,

Reproduced the issue for an orders of 144 eur order total, including

  • a 6 Eur Stripe-SCA transaction fee
  • a 5 Eur shipping fee

Steps to reproduce:

  1. attempt to pay with 4000000000000069 - Charge is declined with an expired_card code.
  2. pay with 5200828282828210 Mastercard (debit)
  • Stripe Dashboard:

image

  • /orders page (Admin):

image

Conclusion 1: with Stripe-SCA payments, the shipping fee is changed, but the transaction fee is missing (6 eur)

Repeated the same steps as above, for Stripe-Connect:

Reproduced the issue for an orders of 144 eur order total, including

  • a 6 Eur Stripe-Connect transaction fee
  • a 5 Eur shipping fee

Steps to reproduce (as before):

  1. attempt to pay with 4000000000000069 - Charge is declined with an expired_card code.
  2. pay with 5200828282828210 Mastercard (debit)
  • Stripe Dashboard

image

  • /orders page (Admin):

image

Conclusion 2: with Stripe-Connect payments, neither the shipping fee (5 eur) nor the transaction fee is changed (6 eur).

I'm staging the PR now and checking results.

@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 23, 2020
@filipefurtad0
Copy link
Contributor

I repeated the test cases above, after staging this PR, and get these results:

Test case 1) stripe SCA

image

image

Test case 2) stripe Connect

image

image

The same outcome as reported here is being observed:

  • For Stripe-SCA the transaction fee is not being charged, after a failed payment attempt. This is verified for both "simple" cards or cards which require 3D authentication.
  • For Stripe-Connect all seems to be fixed, after this PR.

I think this should be in the next release, but still needs to be fixed for Stripe-SCA still. Thoughts?

@filipefurtad0 filipefurtad0 added feedback-needed and removed pr-staged-fr staging.coopcircuits.fr labels Jul 23, 2020
@luisramos0
Copy link
Contributor Author

yeah, let's merge and get it to the release and open a new issue for stripeSCA.

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.

Stripe Payment processed with incorrect amount
4 participants