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.

Sign up for GitHub

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

Jump to bottom

Alternative solution to handle forwarded "account.updated" webhook #612

Merged
merged 4 commits into from
Apr 28, 2020
Merged

Alternative solution to handle forwarded "account.updated" webhook #612

merged 4 commits into from
Apr 28, 2020

Conversation

vbelolapotkov
Copy link
Collaborator

@vbelolapotkov vbelolapotkov commented Apr 23, 2020 •
edited by luizreis
Loading

Fixes #512
Alternative to #598 .
Pros:

  • the same flow/endpoint is used for refreshing account data.
  • more simple handler both on the site and server.

Cons:

  • one more request to the API is made in the response to the webhook.

Changes proposed in this Pull Request

  • Handle the hook by refetching account data from the server.
  • Log webhooks when debugging enabled.

Testing instructions

  1. Run unit tests with ./vendor/bin/phpunit.

  2. Setup testing environment:

    • Checkout server branch add/179-2-forward-account-updated-webhook.
    • If running server locally in docker Apply dev tools#4 to your local site. This allows the simple HTTP request to work without proper Jetpack authentication.
    • Forward Stripe webhooks to the server with CLI.
    • If running the server in a sandbox launch HTTP tunnel to the URL used during Jetpack activation (associated with blog id).

  3. Testing steps:

    • In WP Admin go to Payments > Settings. Check account settings.
    • Go to Stripe Dashboard > Connect > Connected Account and update account settings. E.g. payout schedule.
    • Reload account settings. Payout schedule should be updated.
    • Go to WC Admin > Status > Logs. Check account.updated webhook is logged.

account-updated-webhook

  • Tested on mobile (or does not apply)

@vbelolapotkov vbelolapotkov changed the title Handle forwarded "account.updated" webhook: alternative solution Alternative solution to handle forwarded "account.updated" webhook Apr 23, 2020
@allendav
Copy link
Contributor

"one more request to the API is made in the response to the webhook."

Meaning that the plugin will fetch the account (which it does every 2 hours anyways), right?

@vbelolapotkov
Copy link
Collaborator Author

Right.

@vbelolapotkov
Copy link
Collaborator Author

vbelolapotkov commented Apr 27, 2020 •
edited by luizreis
Loading

update: changes required to address the server 232#issuecomment-619123773.

@vbelolapotkov
Update webhook handler method
a653bd7 
Make only `event_type` prop required in the event body. This allows
processing events without `data` prop. Handlers for specific event types
still may require additional props, e.g. `data.object` is required for
handling `charge.refund.updated` event.
marcinbot
marcinbot previously approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@marcinbot marcinbot left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! :shipit: when ready (I don't think we need to wait for the server to be deployed)

@marcinbot marcinbot self-requested a review April 27, 2020 14:07
@marcinbot
Copy link
Contributor

Just noticed there are some failing unit tests

@marcinbot marcinbot dismissed their stale review April 27, 2020 14:09

unit tests

@vbelolapotkov
Copy link
Collaborator Author

vbelolapotkov commented Apr 27, 2020 •
edited
Loading

Just noticed there are some failing unit tests

Thanks @marcinbot ! Yeah, those tests were failing because we've made event.data and event.data.object optional 🙂Fixed tests in 30fd86e

marcinbot
marcinbot approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@marcinbot marcinbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it should be ready to merge :)

@vbelolapotkov vbelolapotkov merged commit f1cdb2e into master Apr 28, 2020
@vbelolapotkov vbelolapotkov deleted the add/512-2-handle-account-updated-webhook branch April 28, 2020 12:36
@vbelolapotkov vbelolapotkov mentioned this pull request Apr 28, 2020
Closed
1 task
@marcinbot marcinbot mentioned this pull request Apr 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcinbot marcinbot marcinbot approved these changes

Assignees
No one assigned
Labels
pr: ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle forwarded "account updated" webhook
3 participants
@vbelolapotkov @allendav @marcinbot