Use remote-site-status to check the WPCOM Auth status #2547

puntope · 2024-08-20T11:30:44Z

Changes proposed in this Pull Request:

Context: p1724064639654099/1722414955.111429-slack-C02BB3F30TG

When authorizing Google to fetch data from the Shop as well as generating a token we set an option gla_wpcom_rest_api_status as approved. If the token is revoked outside the extension, the option will still be approved and the notifications will be sent.

This PR handles that and checks the status of the token using the WPCOM REST API.

Screenshots:

Detailed test instructions:

Disable fetch data from Settings or disconnect it from the Connection Test page
Grant access from scratch still works.
You can send notifications in the Connection test page for example
Now disconnect WPCOM Access from Connection Test page
You CANNOT send notifications anymore
Set gla_wpcom_rest_api_status as approved to simulate a granted access that has no token.
You CANNOT send notifications anymore
Go to settings. You see the warning notice in the MC Card
Grant access again
Now You can send notifications again in the Connection test page

Additional details:

ℹ️ I was thinking about removing gla_wpcom_rest_api_status and only relying on the transient + WPCOM REST API. status endpoint for considering the WPCOM API as ready. I would appreciate thoughts on this.

Changelog entry

Tweak - Use remote-site-status to check the WPCOM Auth status

codecov · 2024-08-20T11:34:37Z

Codecov Report

Attention: Patch coverage is 56.25000% with 14 lines in your changes missing coverage. Please review.

Project coverage is 65.0%. Comparing base (cc158ab) to head (fd5d8aa).
Report is 115 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/MerchantCenter/AccountService.php	54.2%	11 Missing ⚠️
src/ConnectionTest.php	0.0%	2 Missing ⚠️
...ernal/DependencyManagement/CoreServiceProvider.php	0.0%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #2547     +/-   ##
===========================================
- Coverage       65.0%   65.0%   -0.0%     
- Complexity      4585    4602     +17     
===========================================
  Files            475     475             
  Lines          17898   17932     +34     
===========================================
+ Hits           11640   11660     +20     
- Misses          6258    6272     +14

Flag	Coverage Δ
php-unit-tests	`65.0% <56.2%> (-<0.1%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/API/WP/NotificationsService.php	`95.2% <100.0%> (+0.1%)`	⬆️
src/Settings/SyncerHooks.php	`100.0% <100.0%> (ø)`
...ernal/DependencyManagement/CoreServiceProvider.php	`0.0% <0.0%> (ø)`
src/ConnectionTest.php	`0.0% <0.0%> (ø)`
src/MerchantCenter/AccountService.php	`94.4% <54.2%> (-3.9%)`	⬇️

... and 3 files with indirect coverage changes

jorgemd24

Thanks, @puntope, for working on this. It works as you described in the PR, but I’ve left some concerns about how we’re handling the errors returned by the remote status endpoint.

jorgemd24 · 2024-08-21T08:11:04Z

src/MerchantCenter/AccountService.php

+			if ( is_wp_error( $integration_remote_request_response ) ) {
+				$this->delete_wpcom_api_status_transient();
+				return false;
+			}


If the token isn't healthy or if the user disconnect Jetpack, does that mean we'll still make a request every time , even though we know the token isn't valid? Shouldn't we store that status in the transient as well?

I also noticed that the SyncerHooks class is registered every time the plugin is initialized, which means we could end up calling this endpoint repeatedly without end.

google-listings-and-ads/src/Infrastructure/GoogleListingsAndAdsPlugin.php

Line 91 in e8e51f3

$this->maybe_register_services();

google-listings-and-ads/src/Settings/SyncerHooks.php

Lines 82 to 85 in e8e51f3

public function register(): void {

if ( ! $this->notifications_service->is_ready() ) {

return;

}

Thanks for the changes, but I still see this issue. If the response is wp_error, we will keep querying the endpoint. Wouldn't it be better to update the transient with the status error instead?

jorgemd24 · 2024-08-21T08:12:52Z

tests/Unit/API/WP/NotificationsServiceTest.php

@@ -5,6 +5,8 @@

 use Automattic\WooCommerce\Admin\RemoteInboxNotifications\TransformerService;
 use Automattic\WooCommerce\GoogleListingsAndAds\API\WP\NotificationsService;
+use Automattic\WooCommerce\GoogleListingsAndAds\API\WP\OAuthService;


unused dependency

jorgemd24 · 2024-08-21T08:56:24Z

src/MerchantCenter/AccountService.php

+
+			$status = json_decode( wp_remote_retrieve_body( $integration_remote_request_response ), true ) ?? [];
+
+			if ( isset( $status['error'] ) ) {


The response from the remote status endpoints returns errors instead of error, should it be error?

puntope · 2024-08-22T15:17:15Z

Thanks for your useful review @jorgemd24 I addressed your comments so now I believe this is ready for a new round

jorgemd24

Thanks for the changes, @puntope! However I’m still seeing some possible issues—see my comments. Thanks!

jorgemd24 · 2024-08-30T10:45:29Z

src/MerchantCenter/AccountService.php

@@ -227,6 +229,11 @@ public function get_connected_status(): array {
 		$id                    = $this->options->get_merchant_id();
 		$wpcom_rest_api_status = $this->options->get( OptionsInterface::WPCOM_REST_API_STATUS );

+		// If token is revoked outside the extension. Set the status as error to force the merchant to grant access again.
+		if ( $wpcom_rest_api_status === 'approved' && ! $this->is_wpcom_api_status_healthy() ) {


What do you think if we update the WPCOM_REST_API_STATUS accordingly? Or Maybe is better just to delete the option so then the user can start the onboarding again.

src/API/WP/NotificationsService.php

src/MerchantCenter/AccountService.php

jorgemd24 · 2024-08-30T11:36:22Z

src/MerchantCenter/AccountService.php

+			if ( is_wp_error( $integration_remote_request_response ) ) {
+				$this->delete_wpcom_api_status_transient();
+				return false;
+			}


Thanks for the changes, but I still see this issue. If the response is wp_error, we will keep querying the endpoint. Wouldn't it be better to update the transient with the status error instead?

puntope · 2024-09-02T14:22:42Z

Hi @jorgemd24 for the review. I updated again the code with your suggestions:

New tests here: https://github.com/woocommerce/google-listings-and-ads/pull/2547/files#diff-f34e5f2ef622a3c8104b654486ebfe6d8e6343a5c0a0c17437ff42d77c6d2fe3R226-R236
Set the Option as error if the status is not healthy https://github.com/woocommerce/google-listings-and-ads/pull/2547/files#diff-c3732a2e44ad6802f55b880fa6023ec164cc9862b1520e96f9c3198918fc8d3bR235
Set the status as ['is_healthy' => false] in case is wp_error or json_decode is null. https://github.com/woocommerce/google-listings-and-ads/pull/2547/files#diff-c3732a2e44ad6802f55b880fa6023ec164cc9862b1520e96f9c3198918fc8d3bR666-R670

Can we do a new round of review?

Thanks

jorgemd24

Thanks for the adjustments, @puntope! One last thing: if the token isn't valid, shouldn't we reset the gla_wpcom_rest_api_status option? Currently, if I disconnect the GFW app, it doesn't send the notification, but the status still shows as "approved."

puntope · 2024-09-04T06:26:50Z

if the token isn't valid, shouldn't we reset the gla_wpcom_rest_api_status option? Currently, if I disconnect the GFW app, it doesn't send the notification, but the status still shows as "approved."

Hi @jorgemd24 Thanks for the review. Can you elaborate the steps for this use case? As far as I understand, When the token is invalid we set the transient and option as "error". (See AccountService::get_connected_status and AccuntService::is_wpcom_api_status_healthy)

jorgemd24

Hi @jorgemd24 Thanks for the review. Can you elaborate the steps for this use case? As far as I understand, When the token is invalid we set the transient and option as "error". (See AccountService::get_connected_status and AccuntService::is_wpcom_api_status_healthy)

Thanks for the clarification! I think this happened because I had the transient set to healthy. Then, I disconnected the Google App and refreshed the settings page, but since the transient was still set to "healthy" it appeared as if it was approved. But now I realize this was just a coincidence when I was doing the review and that the issue is temporary. So, I think we can move forward with this PR!

Use remote-site-status to check the WPCOM Auth status

19c069c

github-actions bot added the changelog: tweak Small change, that isn't actually very important. label Aug 20, 2024

puntope self-assigned this Aug 20, 2024

puntope requested a review from a team August 20, 2024 11:31

puntope marked this pull request as ready for review August 20, 2024 11:32

PHPCS

e8e51f3

jorgemd24 reviewed Aug 21, 2024

View reviewed changes

puntope added 2 commits August 22, 2024 18:56

Remove unused dependency

38254c1

Prevent loop and excessive request.

857e074

puntope requested a review from jorgemd24 August 22, 2024 15:21

jorgemd24 reviewed Aug 30, 2024

View reviewed changes

puntope added 4 commits September 2, 2024 17:56

Add test for is_Ready without health check

f126caa

Update option when there is error.

90ed2a4

Add more tests

ec9e32f

Add more tests

fd5d8aa

puntope requested a review from jorgemd24 September 2, 2024 14:22

jorgemd24 reviewed Sep 3, 2024

View reviewed changes

jorgemd24 approved these changes Sep 4, 2024

View reviewed changes

puntope merged commit 7210b14 into develop Sep 5, 2024
12 checks passed

puntope deleted the tweak/check-wpcom-api-auth branch September 5, 2024 07:42

martynmjones mentioned this pull request Sep 5, 2024

Release 2.8.5 #2586

Merged

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use remote-site-status to check the WPCOM Auth status #2547

Use remote-site-status to check the WPCOM Auth status #2547

puntope commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading

jorgemd24 left a comment

jorgemd24 Aug 21, 2024

jorgemd24 Aug 30, 2024

jorgemd24 Aug 21, 2024

jorgemd24 Aug 21, 2024

puntope commented Aug 22, 2024

jorgemd24 left a comment

jorgemd24 Aug 30, 2024

jorgemd24 Aug 30, 2024

puntope commented Sep 2, 2024

jorgemd24 left a comment

puntope commented Sep 4, 2024

jorgemd24 left a comment

	public function register(): void {
	if ( ! $this->notifications_service->is_ready() ) {
	return;
	}


		$status = json_decode( wp_remote_retrieve_body( $integration_remote_request_response ), true ) ?? [];

		if ( isset( $status['error'] ) ) {

Use remote-site-status to check the WPCOM Auth status #2547

Use remote-site-status to check the WPCOM Auth status #2547

Conversation

puntope commented Aug 20, 2024 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Aug 20, 2024 • edited Loading

Codecov Report

jorgemd24 left a comment

Choose a reason for hiding this comment

jorgemd24 Aug 21, 2024

Choose a reason for hiding this comment

jorgemd24 Aug 30, 2024

Choose a reason for hiding this comment

jorgemd24 Aug 21, 2024

Choose a reason for hiding this comment

jorgemd24 Aug 21, 2024

Choose a reason for hiding this comment

puntope commented Aug 22, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

jorgemd24 Aug 30, 2024

Choose a reason for hiding this comment

jorgemd24 Aug 30, 2024

Choose a reason for hiding this comment

puntope commented Sep 2, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

puntope commented Sep 4, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

puntope commented Aug 20, 2024 •

edited

Loading

codecov bot commented Aug 20, 2024 •

edited

Loading