Don't charge duplicate fees for pending SCA payments #6951
Conversation
Here we mark an adjustment as ineligible if there SCA auth is required on a payment. Then when the customer completes SCA authorization, we process the payment intent and mark the adjustment as eligible again. Fixes openfoodfoundation#6683
|
LGTM, but it probably needs some specs. |
| before do | ||
| allow(order).to receive(:deliver_order_confirmation_email) | ||
| payment.adjustment.update_attribute(:eligible, false) | ||
| end | ||
|
|
||
| it "completes the payment" do | ||
| service.call! | ||
| payment.reload | ||
| expect(payment.state).to eq("completed") | ||
| expect(payment.cvv_response_message).to be nil | ||
| expect(payment.adjustment.eligible).to be true |
There was a problem hiding this comment.
I was originally doing
expect(payment).to have_received(:reinstate_adjustment_eligibility!)
but it wasn't working as expected - it said it wasn't receiving the message, even though it was.
So I went with this approach, which is a more useful test, although it does require reaching into the adjustment implementation just a little bit.
| @@ -126,7 +126,9 @@ def payment_source | |||
| end | |||
|
|
|||
| def ensure_correct_adjustment | |||
| revoke_adjustment_eligibility if ['failed', 'invalid'].include?(state) | |||
| if ['failed', 'invalid'].include?(state) || authorization_action_required? | |||
There was a problem hiding this comment.
The extra complexity this conditional has and I think it'd be good the extract it, give it an abstract name, but I don't have any suggestion so I guess it's not a big deal. We'll see when a third condition comes into play.
|
Hey @andrewpbrett , This is a tricky one... It looks like a step in the right direction, but I'm not sure it's ready for merging. It looks as if this PR introduces the transaction (updated) fees on the orders, after the payments take place, in both back- and front office. In the videos below we have the backoffice in the left side, and the customer's window on the right side. After a transaction (updated) fee of 3 eur is set to stripe-SCA payment method, and:
Notice, how the order is marked as "not paid" (non reglé), although the payment was successful.
In this case, the whole payment is charged (12 eur + enterprise fee of 3 eur) but the fee is not included in the order, so it appears as credit owed. For comparison and clarity, compare the same order before staging this PR - the transaction fee is included in the order:
This needs another go, I think. I hope this is helpful feedback - if there is any aspect you feel needs clarification please don't hesitate to reach out @andrewpbrett . |
|
Thanks @filipefurtad0 - I can see why this PR wouldn't address enterprise fees that are a flat fee per order. I'm a little confused why it's actually removing them entirely however. I'll do some more digging and see if it warrants a separate issue. |
|
@filipefurtad0 I took a look at this, and I think if I follow the steps you did, I get the same result, even on
Let me know if I misunderstood what you did or if that isn't a valid test in this case. |
|
Hey @andrewpbrett , I realize this was perhaps not clear, from my testing notes before - thank you for reaching out! There is a spot on the previous notes, where "enterprise" is written but where "transaction" is meant. Sorry, this is corrected now. The detailed steps you describe rather probe how/when enterprise fees included in the backoffice orders - these steps actually reproduce bug #5567. So, to clarify: this specific PR would introduce (at the time of testing) a regression in which transaction fees would not be included in the order total, when checking out through the shopfront. This would be a considerable regression for the shopfront checkouts: it would render all Stripe orders placed though the shopfront in a "balance due" state. Maybe we need to cover this somehow with a feature spec. Please note however, that this is known bug, for orders placed in the BO - see #5574. So, the focus here is really on transaction fees, and on the steps to reproduce, on issue #6683 - currently in master. Using Stripe-SCA, in the backoffice:
I hope this helps. |
|
Fixed in #6965 |
What? Why?
Close #6683
Here we mark an adjustment as ineligible if there SCA auth is required on a payment. Then when the customer completes SCA authorization, we process the payment intent and mark the adjustment as eligible again.
What should we test?
Creating a payment that requires SCA authorization should not add a transaction fee to the order until the payment is authorized by the customer.
Release notes
Fixed a bug where back office payments would create duplicate transaction fees each time a payment failed or needed authorization.
Changelog Category: User facing changes
Dependencies
Documentation updates