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.

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

Handle forwarded "account.updated" webhook #598

Closed
wants to merge 5 commits into from

Conversation

vbelolapotkov
Copy link
Collaborator

@vbelolapotkov vbelolapotkov commented Apr 17, 2020

Fixes #512
Based on webhooks forwarding introduced in #411

Changes proposed in this Pull Request

  • Handle the hook by updating transient account cache when necessary.
  • Log webhooks when debugging enabled.

Questions:

  1. Should we set cache if it's empty or expired?
  2. Are there any other cases when we should not update cached account except account id mismatch? E.g. live/test mode mismatch?

Testing instructions

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

  2. Setup testing environment:

    • Apply server#225
    • 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)

jrodger added 3 commits April 6, 2020 16:29
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.
@vbelolapotkov vbelolapotkov changed the title Handle account updated webhook Handle forwarded "account.updated" webhook Apr 17, 2020
array(
'methods' => WP_REST_Server::CREATABLE,
'callback' => array( $this, 'handle_webhook' ),
'permission_callback' => array( $this, 'check_permission' ),
Copy link
Contributor

Choose a reason for hiding this comment

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

check_permission will be interesting. How will you do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to #411 (comment), the existing check should be enough because it will log in the connection owner via the signature in the request (see also paJDYF-wq-p2).

@allendav
Copy link
Contributor

Looks like you're including a refactor to make it more unit testable too? That's awesome, but if this PR gets much larger let's do that as a separate PR

@vbelolapotkov vbelolapotkov force-pushed the add/512-handle-account-updated-webhook branch from 831c326 to d3e0817 Compare April 20, 2020 02:02
@vbelolapotkov vbelolapotkov changed the base branch from master to add/handling-for-failed-refunds April 20, 2020 02:22
@vbelolapotkov
Copy link
Collaborator Author

Looks like you're including a refactor to make it more unit testable too? That's awesome, but if this PR gets much larger let's do that as a separate PR

@allendav the PR is based on #411. I've changed the merge base to emphasize the changes by this specific PR.

@jrodger jrodger force-pushed the add/handling-for-failed-refunds branch from 1d1ee4d to e2fcaff Compare April 21, 2020 10:25
@vbelolapotkov vbelolapotkov changed the base branch from add/handling-for-failed-refunds to master April 23, 2020 13:04
@vbelolapotkov
Copy link
Collaborator Author

Replaced with #612

@frosso frosso deleted the add/512-handle-account-updated-webhook branch January 16, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle forwarded "account updated" webhook
4 participants