search

woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
45 stars 21 forks source link

Fix Issue disconnecting wpcom api #2515

Closed jorgemd24 closed 1 month ago

jorgemd24 commented 1 month ago

Changes proposed in this Pull Request:

Closes # .

This PR addresses an issue when attempting to disconnect all accounts on the settings page. The problem occurs when the user hasn't granted access to the new Pull mechanism, but we still try to revoke the wpcom token. Since there’s no token to revoke, WPCOM responds with an error.

I’m wondering if we should adjust this behavior in WPCOM so that if there’s no token to revoke, it simply returns without throwing an error. I’d be happy to hear thoughts on this.

Screenshots:

Detailed test instructions:

  1. Check out the develop branch and open the dev console.
  2. Navigate to the settings page.
  3. Do not grant access to the new API Pull mechanism.
  4. Click "Disconnect all accounts."
  5. See the error in the dev console from the REST API authorization endpoint, because there is not aWPCOM token to revoke.
  6. Refresh the page and go through the onboarding process again.
  7. Switch to this branch.
  8. Repeat the steps and notice that you can now disconnect all your accounts without any issues.

Additional details:

Changelog entry

Fix - Disconnecting all accounts when WPCOM connection is not granted.

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 64.7%. Comparing base (76d15ab) to head (007190f). Report is 27 commits behind head on develop.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515/graphs/tree.svg?width=650&height=150&src=pr&token=UROWUPF1LX&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## develop #2515 +/- ## ============================================ + Coverage 63.5% 64.7% +1.2% - Complexity 0 4580 +4580 ============================================ Files 322 801 +479 Lines 5043 22974 +17931 Branches 1220 1229 +9 ============================================ + Hits 3204 14873 +11669 - Misses 1672 7934 +6262 Partials 167 167 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [js-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `63.8% <100.0%> (+0.3%)` | :arrow_up: | | [php-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `65.0% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [js/src/data/actions.js](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515?src=pr&el=tree&filepath=js%2Fsrc%2Fdata%2Factions.js&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-anMvc3JjL2RhdGEvYWN0aW9ucy5qcw==) | `7.9% <100.0%> (+6.3%)` | :arrow_up: | ... and [480 files with indirect coverage changes](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2515/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)
mikkamp commented 1 month ago

I’m wondering if we should adjust this behavior in WPCOM so that if there’s no token to revoke, it simply returns without throwing an error. I’d be happy to hear thoughts on this.

I can't find any documentation about revoking a token, but based on the documentation about validating a token it seems it's common to return an error when the token is not valid (or possibly doesn't exist).

So I'd suggest to keep expecting an error, and just handle it accordingly in the client so they can still proceed.

jorgemd24 commented 1 month ago

I can't find any documentation about revoking a token, but based on the documentation about validating a token it seems it's common to return an error when the token is not valid (or possibly doesn't exist).

Thanks for your input! We implemented the endpoint in WPCOM for revoking the GFW token, so it’s great to know we're aligned with the other responses in WPCOM.

jorgemd24 commented 1 month ago

Hi @puntope, thanks for the suggestion! I’ve updated the PR to handle the error on the client side instead of the server. I believe it’s better to attempt revoking the token regardless of the WPCOM Auth status in the DB (just to be safe), and as @mikkamp mentioned, this keeps us aligned with the other endpoints. Could you take another look? Thanks!