Fix #6964: don't try to complete a failed payment

[andrewpbrett]

What? Why?

Closes #6964

Here we only complete a payment if it's in a state where it can be completed, and list those states explicitly

What should we test?

Choosing not to authorize an SCA payment should not result in a snail.

This might be pretty difficult to write an automated test for since we're testing what happens when the customer interacts with the Stripe website.

Release notes

I don't think this is happening in production

Changelog Category: Technical changes

Dependencies

Documentation updates

[sauloperez]

This might be pretty difficult to write an automated test for since we're testing what happens when the customer interacts with the Stripe website.

but we do know the exact Stripe request we receive when that happens, don't we? We would need to stub that out and we'd be reproducing the same scenario.

[sauloperez]

I would personally make the effort to extract a method out of this and find a name for this so 1) we don't mix levels of abstraction in #call! 2) we make it easier for the next dev to understand why pending and processing states and not others.

[andrewpbrett]

Good catch, I'm not sure I didn't just use the can_complete? method in the first place.

[sauloperez]

We should test this new behavior.

EDIT nice! the test is failing

[filipefurtad0]

Hey @andrewpbrett ,

thanks for addressing this.

Before this PR we had the following scenario:

i) placing an order in the BO (admin)
ii) using the email received per email to proceed with authentication (customer)
iii) clicking "Fail Authentication" leads to the bug as below (left, admin; right, customer):

After this PR:
iii) does not trigger a snail anymore, and looks like this:

Also, please not that I was not able to reproduce bug #6683 after this PR 🎉
Neither by refusing authentication nor by using an invalid card like 4000008260003178.

We can see, that the (3 eur) transaction fee do not pile up, despite the failed payment attempts:

The fact that the order is still in the balance due state relates to bug #5574.

It would be good to have a spec covering us from the re-occurrence of #6683. Should I open a tech-debt for this?

Awesome, let's get this merged 🎉

[sauloperez]

It would be good to have a spec covering us from the re-occurrence of #6683. Should I open a tech-debt for this?

Yes, I think it's worth exploring why no test failed after we introduced this regression.