Handle failed refunds #411

jrodger · 2020-02-06T14:25:35Z

Fixes #312

Changes proposed in this Pull Request

Adds a REST endpoint for receiving webhooks forwarded from WP.com
Initial action is simply adding an order note when we get an update regarding a failed refund.

Local Environment Testing instructions

Apply server PR https://github.com/Automattic/woocommerce-payments-server/pull/159 to your local server and forward Stripe webhooks to it using the CLI: ./local/bin/listen-to-webhooks.sh
Check that the mocked details in functions.php - wcpay_server_remote_request match your local setup. This replaces the Jetpack call with a simple HTTP request.
Apply dev tools PR https://github.com/Automattic/woocommerce-payments-dev-tools/pull/4 to your local site. This allows the simple HTTP request to work without proper Jetpack authentication.
Make a purchase with test card 4000 0000 0000 5126 (docs)
Refund the purchase in wp-admin
The refund will succeed, but then asynchronously fail. Wait for the refund to fail, refresh the page and ensure an order note is added indicating that the refund failed

Sandbox Testing instructions

Apply server PR https://github.com/Automattic/woocommerce-payments-server/pull/159 to your sandbox and forward Stripe webhooks to it using the CLI
Make a purchase with test card 4000 0000 0000 5126 (docs)
Refund the purchase in wp-admin
The refund will succeed, but then asynchronously fail. Wait for the refund to fail, refresh the page and ensure an order note is added indicating that the refund failed

vbelolapotkov · 2020-02-13T13:47:33Z

@jrodger tried twice but wasn't able to reproduce the test (see screenshots below). Despite the server is running the correct branch I'm not sure the webhook reached my local site. Tried to set a breakpoint in webhook-controller->handle_webhook but never reached it. What's the best way to troubleshoot it?

P.s. Tried with localhost and ngrok, no difference.

jrodger · 2020-02-13T16:55:47Z

🤔 We can see that the refund is failing from your first screenshot, so that's good.

That failure triggers a charge.refund.updated webhook event from Stripe to WP.com. Can you see that in your Stripe CLI? What status is WP.com returning for that event?

If that's returning 200 then we'll need to dig a little deeper. Can you try putting something like error_log( print_r( $response_array, true ) ); at the bottom of server's handle_webhook function? You can then see if anything gets logged out using something like tail --lines=100 /tmp/php-errors on your sandbox. Maybe the response code it's returning will give us a clue, or if nothing is logged then it suggests we're erroring out earlier somewhere.

jrodger · 2020-02-13T17:05:38Z

Forgot to mention, please don't spend too long on this if there are other PRs needing review that are required for beta.

vbelolapotkov · 2020-02-14T14:11:54Z

That failure triggers a charge.refund.updated webhook event from Stripe to WP.com. Can you see that in your Stripe CLI? What status is WP.com returning for that event?

Here is the link to refund_update event:
https://dashboard.stripe.com/acct_1FzKmRFL7IKwCJxf/test/events/evt_1GBhy6FL7IKwCJxfNUgwK4Wk

According to the server response was 200 OK

If that's returning 200 then we'll need to dig a little deeper.

Will try again on Monday.

vbelolapotkov

@jrodger I was able to set up stripe CLI and reproduce testing instruction with webhooks forwarded to the sandbox. Well done!

Some general thoughts:

What about tests for webhook controller class?
As @RadoslavGeorgiev noticed we have a successful confirmation note first and a failure note afterwards. So it's probably a good idea to update existing messaging for order notes to avoid confusion in case of refund failure. E.g. Refund request was successfully sent as an alternative to existing success message.

includes/admin/class-wc-rest-payments-webhook-controller.php

vbelolapotkov · 2020-02-17T18:46:47Z

includes/class-wc-payments-db.php

+		if ( $order_id ) {
+			return wc_get_order( $order_id );
+		}
+		return false;


Just curious is returning false is better than returning NULL? AFAIK NULL is intended for informing about missing value.

I much prefer having a single return type, so yeah, I prefer returning null to indicate a missing value. However, I thought I'd leave this as is since I was just moving it from another class.

It's only used in one other place, so we could take the opportunity to change it now?

It's only used in one other place

Actually there are two. And this one leads to an error if false or null is returned. IMO we should fix it either here on in another PR.

so we could take the opportunity to change it now

Now I see that wc_get_order can return a boolean value, and forcing it to return null without explicit benefit feels weird. So let's leave it as is now.

includes/admin/class-wc-rest-payments-webhook-controller.php

jrodger · 2020-04-07T15:46:48Z

FYI - I've updated the testing instructions so that both the local and sandbox configurations can be tested.

vbelolapotkov

Great progress @jrodger, thank you ! 🎉 I've tested it both in sandbox mode and with a local server in docker. Changes work as expected. Left some note regarding the changes!

includes/admin/class-wc-rest-payments-webhook-controller.php

tests/admin/test-class-wc-rest-payments-webhook.php

This logic is required by other parts of the code base, so extracting it out allows it to be shared. The unit tests have been left as is for now, but this also allows for database access to be mocked more easily.

The initial implementation just handles refund updated events and adds an order note in the event of a failure.

This rule is generating some false positives. We can try turning it on again if it gets fixed in a later PHPCS version. Alternatively we could start relying on a static code analyser for more robust checks on how we're using exceptions.

jrodger · 2020-04-21T10:27:06Z

Thanks for the detailed review @vbelolapotkov. I think I've addressed all your comments now. A lot of my comments boil down to "let's do this later", absolutely feel free to push back on any of those!

As @RadoslavGeorgiev noticed we have a successful confirmation note first and a failure note afterwards. So it's probably a good idea to update existing messaging for order notes to avoid confusion in case of refund failure. E.g. Refund request was successfully sent as an alternative to existing success message.

I've raised #601 to discuss this point because the more I thought about it the more work it seemed.

vbelolapotkov

Gave it a final round of tests, works as expected! Thank you @jrodger !

jrodger added the pr: needs review label Feb 6, 2020

vbelolapotkov added the pr: needs author reply label Feb 13, 2020

vbelolapotkov reviewed Feb 18, 2020

View reviewed changes

dwainm reviewed Feb 24, 2020

View reviewed changes

includes/admin/class-wc-rest-payments-webhook-controller.php Outdated Show resolved Hide resolved

v18 removed the pr: needs review label Mar 1, 2020

jrodger force-pushed the add/handling-for-failed-refunds branch 2 times, most recently from 59ebee2 to 1d1ee4d Compare April 6, 2020 15:29

jrodger added pr: in progress pr: needs review and removed pr: needs author reply pr: in progress labels Apr 6, 2020

vbelolapotkov self-assigned this Apr 8, 2020

vbelolapotkov requested changes Apr 9, 2020

View reviewed changes

vbelolapotkov added pr: needs author reply and removed pr: needs review labels Apr 9, 2020

This was referenced Apr 17, 2020

Add registration method for webhook handlers processing events from the WCPay server #599

Closed

Refine failed refund notes #601

Open

dechov mentioned this pull request Apr 17, 2020

Handle forwarded "account.updated" webhook #598

Closed

1 task

jrodger added 7 commits April 21, 2020 10:53

Extract DB access from Stripe API client class

7067cd0

This logic is required by other parts of the code base, so extracting it out allows it to be shared. The unit tests have been left as is for now, but this also allows for database access to be mocked more easily.

Add REST endpoint for handling webhooks from WP.com

ee9c7e2

The initial implementation just handles refund updated events and adds an order note in the event of a failure.

Fix whitespace in phpcs.xml.dist

51719b4

Clean up webhook controller test class

0c02b1e

Tweak naming of read_property function

3b48437

Make exception message translatable

3468b38

Escape HTML in translation string

e2fcaff

jrodger force-pushed the add/handling-for-failed-refunds branch from 1d1ee4d to e2fcaff Compare April 21, 2020 10:25

jrodger added pr: needs review and removed pr: needs author reply labels Apr 21, 2020

vbelolapotkov approved these changes Apr 21, 2020

View reviewed changes

vbelolapotkov approved these changes Apr 21, 2020 •

edited

Loading

View reviewed changes

vbelolapotkov added pr: ready to merge and removed pr: needs review labels Apr 22, 2020

Rename WC_Payments_Rest_Exception to WC_Payments_Rest_Request_Exception

cefcaf1

jrodger merged commit 6e19910 into master Apr 22, 2020

frosso deleted the add/handling-for-failed-refunds branch January 16, 2024 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle failed refunds #411

Handle failed refunds #411

jrodger commented Feb 6, 2020 •

edited

Loading

vbelolapotkov commented Feb 13, 2020

jrodger commented Feb 13, 2020

jrodger commented Feb 13, 2020

vbelolapotkov commented Feb 14, 2020

vbelolapotkov left a comment

vbelolapotkov Feb 17, 2020

jrodger Apr 7, 2020

vbelolapotkov Apr 21, 2020

jrodger commented Apr 7, 2020

vbelolapotkov left a comment

jrodger commented Apr 21, 2020

vbelolapotkov left a comment •

edited

Loading

Handle failed refunds #411

Handle failed refunds #411

Conversation

jrodger commented Feb 6, 2020 • edited Loading

Changes proposed in this Pull Request

Local Environment Testing instructions

Sandbox Testing instructions

vbelolapotkov commented Feb 13, 2020

jrodger commented Feb 13, 2020

jrodger commented Feb 13, 2020

vbelolapotkov commented Feb 14, 2020

vbelolapotkov left a comment

Choose a reason for hiding this comment

vbelolapotkov Feb 17, 2020

Choose a reason for hiding this comment

jrodger Apr 7, 2020

Choose a reason for hiding this comment

vbelolapotkov Apr 21, 2020

Choose a reason for hiding this comment

jrodger commented Apr 7, 2020

vbelolapotkov left a comment

Choose a reason for hiding this comment

jrodger commented Apr 21, 2020

vbelolapotkov left a comment • edited Loading

Choose a reason for hiding this comment

jrodger commented Feb 6, 2020 •

edited

Loading

vbelolapotkov left a comment •

edited

Loading