Disable the manual refunds button unless a WCPay refund has failed

[reykjalin]

Supercedes #710

Fixes #426

Changes proposed in this Pull Request

• Save the refund status in _wcpay_refund_status as a meta value.
• Use the meta value to conditionally disable the manual refund button.

Context: Payments made through WooCommerce Payments must be refunded through WCPay. Only after the refund fails may the funds be refunded manually.

Testing instructions

1. git checkout 426-display-manual-refund-after-failure
2. npx webpack
3. Make a purchase with test card 4000 0000 0000 5126.
4. Navigate to the order screen and click "Refund" to display the refund bar.
5. Make sure that the button for manual refunds is disabled and explains that "Refunds are available only through WooCommerce Payments."
6. Attempt a refund through WCPay.
7. The refund should go through "successfully".
8. Reload the page.
9. Note that there is now a new note explaining that the refund actually failed.
10. Delete the refund by clicking the x button that appears when you hover over the refund entry.
11. Try making another refund.
12. Note that there is now only a manual refund option available.

[dechov]

Code looks good and appears to work well, following provided testing instructions.

[Last step of testing instructions:] Note that there is now only a manual refund option available.

Both options should be available at this point, right? This is what I see:

I kind of think we should overwrite the tooltip with some better messaging even if ! disableManualRefunds since we know that there's no other way to "issue a refund through your payment gateway" after manually refunding, but it's not a blocker for this PR.

Just a thought on the tests: they look fine, but maybe it's a bit better to call has_refund_failed and check that value, instead of bothering with the actual meta value detail, since that's what the behavior depends on anyway – what do you think?

[reykjalin]

[Last step of testing instructions:] Note that there is now only a manual refund option available.

Both options should be available at this point, right? This is what I see:

I don't think so? This is what I see:

Before first refund:

After failed refund + reload:

After deleting the refund and clicking Refund again:

I kind of think we should overwrite the tooltip with some better messaging even if ! disableManualRefunds since we know that there's no other way to "issue a refund through your payment gateway" after manually refunding, but it's not a blocker for this PR.

That's actually supposed to happen, but I forgot to include the JS build instructions in the PR via npx webpack 😬 . I've updated the testing instructions to include this.

However, I do think we should probably change the tooltip after the refund has failed (see third screenshot above ^). I think that could be confusing 🤔

What do you think would be a good message? Maybe just "You will need to manually issue a refund after using this". Or maybe "You will need to manually issue a refund without using WooCommerce Payments after using this"? Might be best to get @LevinMedia to look at this. We could do this in a different issue/PR.

Just a thought on the tests: they look fine, but maybe it's a bit better to call has_refund_failed and check that value

That's an excellent observation, and I love that idea! Fixed in df9ec00.

[dechov]

Oh, I see, I think the webhook listener isn't currently working properly on my local server instance for some reason, so the status stayed 'successful' in my test. Now I see that the 'failed' status leads to the single refund button.

On that note, I'm not fully convinced that can_refund_order should be false if the refund has failed once, since the failure could have theoretically been a fluke and could be followed by a successful second attempt. What do you think?

I did have npm start running, so I did see the one tooltip (from this PR) at first, and I was only referring to also having a tooltip after refund failure (as you mention). I think we should definitely change that, though I'm not sure quite how to word it and it's fine to defer to a later PR.

[dechov]

@LevinMedia Did you have it in mind one way or the other, as for whether the "Refund […] via WooCommerce Payments" button should still appear after a failure (and after the manual refund button is enabled)? If so, should it always show & allow any number of retries, or be limited (e.g. by tracking the number of attempts/faillures)?

As agreed with @reykjalin: if it should disappear, I'll merge as is – if it should stick around, I'll just revert the one can_refund_order change and merge.

[LevinMedia]

@reykjalin @dechov thanks!

Initially I would prefer if the refund manually button didn't appear at all - not even a disabled state. After a refund fails, then it would appear. I think we should persist the refund via WooCommerce Payments button indefinitely, even after a failure.

I'd like to update the refund manually tooltip to read:

Refunding manually requires you reimburse your customer offline via cash, check, etc. The refund amounts entered here will only be used to balance your analytics.

[dechov]

👍 I've made the changes:

• c990025: never hide the gateway refund button
• 8eface0: hide the manual button at first, and move the tooltip to the "after failure" case (with @LevinMedia's new wording – just a minor tweak to it)

Before refund failure:

After refund failure (and deleting the WC refund…):

@reykjalin Hope you don't mind I went ahead with that latter change. And thanks for the smart suggestion to consult David 😄

On another note, I think can probably (identify and) delete the refund when an async refund failure comes in, as is done in the sync refund failure case in WC core [source] – we can open a separate issue for that though, since it's not strictly related to the change in this PR.

[reykjalin]

The changes look good and test well @dechov ! Thank you for jumping on the issue ❤️

I did notice another (potential) issue while testing the new changes; if you retry a refund via WCPay after the first refund failed you get this error:

I'm not sure if this is just an issue with the test card (which is always supposed to fail a refund) or if we have to somehow clear the failed refund before attempting another refund.

I don't think we should fix this in this PR, and instead open a new issue. Fixing the issue (if it even is an issue?) will require some digging into how Stripe refunds work 🤔

Reproduction steps:

(continue after step 4 in PR description)

4. ...
5. Attempt a refund through WCPay; notice it's "successful", but fails via webhook.
6. Reload the page
7. Attempt another refund through WCPay; note that you'll see the alert from the screenshot pop up.

[reykjalin]

@reykjalin Hope you don't mind I went ahead with that latter change. And thanks for the smart suggestion to consult David 😄

It looks great, thank you!

I think can probably (identify and) delete the refund when an async refund failure comes in, as is done in the sync refund failure case in WC core [source] – we can open a separate issue for that though, since it's not strictly related to the change in this PR.

Agreed 💯

[diegocurbelo]

@dechov Changes looks good... just a small detail (or edge case maybe):

If you just click the refund button (without a valid amount) you get an alert with the error "Invalid refund amount", after that both buttons (manually and via WooCommerce Payments) are shown.

---

[dechov]

@diegocurbelo Good catch. I think the root problem there isn't how we're handling the refund failure in this PR, but that it's even going ahead with a refund request when we could easily throw early before the API request if amount is 0. IMO that can be (another) separate issue – I actually thought there already might have been an issue for that already, but I can't find it.

[dechov]

@diegocurbelo pointed out that the tooltip wasn't appearing on the same page load immediately after removing the refund. This was because the refund buttons were re-generated in the DOM.

At @reykjalin's suggestion, I've pushed a change to modify the buttons on the "Refund" button click event instead of initial page load only. This had the side-effect of making the manual refund button not show at all in that case, because that same page load / session is unaware of the refund failure (e.g. the failure order note also is not visible at that point) – I don't think this would be a big deal in practice because most async refund failures would take significantly longer than the test one – does make sense?

[dechov]

Approving based on commits up until the ones I pushed, specifically the way _wcpay_refund_status meta value is set and retrieved and the tests around that, all of which is untouched in the latter changes.

[reykjalin]

I don't think [the manual refund button not showing up until you reload the order page after a refund failed asynchronously] would be a big deal in practice because most async refund failures would take significantly longer than the test one

Agreed — Most of these types of failures probably won't happen instantly like the test refunds do.

---

I've had a look at your changes @dechov and they all look good to me. I can't approve the PR since I'm the one that opened it, but I think we're good to merge 🙂

[diegocurbelo]

LGTM 🚢