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
44 stars 21 forks source link

[API Pull] Auth Flow #2424

Closed puntope closed 2 months ago

puntope commented 2 months ago

Changes proposed in this Pull Request:

Detailed test instructions:

  1. Go to Settings and verify you see "We will soon transition to a new and improved method for synchronizing product data with Google -- Get early access" banner
  2. Click on Get early access (see the button loading)
  3. You will be redirected to auth flow
  4. Complete the auth flow
  5. You will be redirected back to the store
  6. You see "Google has been granted access to fetch your product data." in green on the Google MC card
  7. Click on "Disable product data fetch"
  8. Auth is disabled and you see the banner again

Additional details:

ianlin commented 2 months ago

after authorizing the WPCOM App, it seems Google is redirecting to the wrong URL.

Had a look at it and I found they tried to redirect us to this URL:

https://shoppingdataintegration.googleapis.com/v1/credentials/partners/WooCommerce/https%3A%2F%2Ftest-site.example.com%2Fwp-admin%2Fadmin.php%3Fpage%3Dwc-admin%26path%3D%2Fgoogle%2Fsettings%3Fnonce=XXXXX&google_wpcom_app_status=approved

We should ask them to remove https://shoppingdataintegration.googleapis.com/v1/credentials/partners/WooCommerce/ so they could redirect to the correct URL.

However I also noticed there were something wrong in the merchant site's URL:

The URL decoded version of the merchant site's URL is:

https://test-site.example.com/wp-admin/admin.php?page=wc-admin&path=/google/settings?nonce=XXXXX&google_wpcom_app_status=approved

where it added a question mark ? before nonce parameter which it would cause the page not found. We should also ask them the replace that ? with & so it would return to the merchant site correctly.

puntope commented 2 months ago

Thanks, @ianlin and @jorgemd24 for your suggestions and investigation. They fixed the error from Google and now seems to work. Also, I addressed the review comments. Can you test again?

jorgemd24 commented 2 months ago

@puntope, I tested it, and it is now redirecting me back to my site. Is there anything else we need to wait for, or can I proceed with the review? Also, I noticed some PHPCS errors.

puntope commented 2 months ago

@puntope, I tested it, and it is now redirecting me back to my site. Is there anything else we need to wait for, or can I proceed with the review? Also, I noticed some PHPCS errors.

We need to wait for a fix from google. I will ping you

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 64.70588% with 18 lines in your changes missing coverage. Please review.

Project coverage is 64.3%. Comparing base (aec6d57) to head (e42f728). Report is 481 commits behind head on feature/google-api-project.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424/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/2424?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) ```diff @@ Coverage Diff @@ ## feature/google-api-project #2424 +/- ## ============================================================== + Coverage 64.2% 64.3% +0.1% - Complexity 4484 4549 +65 ============================================================== Files 471 794 +323 Lines 18862 22795 +3933 Branches 0 1219 +1219 ============================================================== + Hits 12117 14665 +2548 - Misses 6745 7963 +1218 - Partials 0 167 +167 ``` | [Flag](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424/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/2424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `63.5% <ø> (?)` | | | [php-unit-tests](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | `64.6% <64.7%> (+0.3%)` | :arrow_up: | 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/2424?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce) | Coverage Δ | | |---|---|---| | [src/API/Google/Middleware.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424?src=pr&el=tree&filepath=src%2FAPI%2FGoogle%2FMiddleware.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0FQSS9Hb29nbGUvTWlkZGxld2FyZS5waHA=) | `95.0% <100.0%> (+8.2%)` | :arrow_up: | | [src/MerchantCenter/AccountService.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424?src=pr&el=tree&filepath=src%2FMerchantCenter%2FAccountService.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL01lcmNoYW50Q2VudGVyL0FjY291bnRTZXJ2aWNlLnBocA==) | `98.3% <100.0%> (+2.1%)` | :arrow_up: | | [...rc/API/Site/Controllers/RestAPI/AuthController.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424?src=pr&el=tree&filepath=src%2FAPI%2FSite%2FControllers%2FRestAPI%2FAuthController.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0FQSS9TaXRlL0NvbnRyb2xsZXJzL1Jlc3RBUEkvQXV0aENvbnRyb2xsZXIucGhw) | `92.4% <0.0%> (-1.0%)` | :arrow_down: | | [src/API/WP/OAuthService.php](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424?src=pr&el=tree&filepath=src%2FAPI%2FWP%2FOAuthService.php&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce#diff-c3JjL0FQSS9XUC9PQXV0aFNlcnZpY2UucGhw) | `66.7% <33.3%> (-14.0%)` | :arrow_down: | ... and [512 files with indirect coverage changes](https://app.codecov.io/gh/woocommerce/google-listings-and-ads/pull/2424/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=woocommerce)
puntope commented 2 months ago

@jorgemd24 @ianlin This seems to be ready for another look. Google deployed their last changes

jorgemd24 commented 2 months ago

Also, what do you think about setting the default value of this filter to true? Or do we want the merchant to enable this manually?

https://github.com/woocommerce/google-listings-and-ads/blob/ec1e862ee00e0112765b7e8bee76300dadc168b6/src/API/WP/NotificationsService.php#L176

jorgemd24 commented 2 months ago

One more thought: how will Google return any errors through the OAuth flow, and how are we going to handle them?

puntope commented 2 months ago

Or do we want the merchant to enable this manually?

If nothing has changed for now I think the first version requires setting the filter. But not sure if that changed after dark launch discussion

puntope commented 2 months ago

One more thought: how will Google return any errors through the OAuth flow, and how are we going to handle them?

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

puntope commented 2 months ago

Thanks @jorgemd24. Can you take another look ?

puntope commented 2 months ago

Thanks for the adjustments, LGTM 👍

If nothing has changed for now I think the first version requires setting the filter. But not sure if that changed after dark launch discussion

Okay I will ask them, for me it looks a bit redundant.

Sounds good. @jorgemd24 Go ahead and we can remove if so.

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

If so, I can work on a PR to display those errors to the user.

@jorgemd24 We do display a notice on the card banner and allow granting again. Is this what you mean?

p.s You can also check the tracking topic if you are looking for issues.

ianlin commented 2 months ago

One more thought: how will Google return any errors through the OAuth flow, and how are we going to handle them?

The discussion between @ianlin and Google was that they would return google_wpcom_app_status=error. Can you confirm @ianlin ?

@puntope @jorgemd24 Yeah correct, they will just set the query param to google_wpcom_app_status=error without what error it is.