API PULL - Block syncing when notifications are enabled #2326

puntope · 2024-03-20T17:37:06Z

Changes proposed in this Pull Request:

This PR disables Product and Coupon Sync when WC API is authorized.

ℹ️ WC API is authorized when the option gla_wpcom_rest_api_status = approved

Detailed test instructions:

Enabled WC API

Set the option gla_wpcom_rest_api_status as "approved"
Go to the Connection Test page
Verify you see at the end WC STAtus APi is approved.
Run Sync All Products --- You will get "Error submitting products to Google: Manual product sync will not run if the automatic data fetching is enabled. Please review your configuration in Google Listing and Ads settings."
Run Delete All Synced Products --- You will get "Error deleting products from Google: Manual product sync will not run if the automatic data fetching is enabled. Please review your configuration in Google Listing and Ads settings."
Run Clean up All Synced Products --- You will get "Error cleaning up products: Manual product sync will not run if the automatic data fetching is enabled. Please review your configuration in Google Listing and Ads settings."
For the coupons there is no test in the Connection page available. So we can only test that the jobs (delete_coupon and update_coupon) are not scheduled when the coupon is created, updated or deleted. But this was tested already in previous works.

Disabled WC API

Remove the option gla_wpcom_rest_api_status or click on "Disconnect" in the Connection test page under "WPCOM REST API Status" section
Run Sync All Products successfully
Run Delete All Synced Products successfully
Run Clean up All Synced Products successfully
For the coupons there is no test in the Connection page available. So we can only test that the jobs (delete_coupon and update_coupon) are scheduled when the coupon is created, updated or deleted. But this was tested already in previous works.

codecov · 2024-03-20T17:40:41Z

Codecov Report

Attention: Patch coverage is 71.28713% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 61.9%. Comparing base (5365056) to head (ac18037).
Report is 12 commits behind head on add/grant-access-to-google-wpcom-app-in-setting-page.

Additional details and impacted files

@@                                  Coverage Diff                                   @@
##             add/grant-access-to-google-wpcom-app-in-setting-page   #2326   +/-   ##
======================================================================================
  Coverage                                                    61.9%   61.9%           
- Complexity                                                   4398    4415   +17     
======================================================================================
  Files                                                         761     761           
  Lines                                                       22308   22369   +61     
  Branches                                                      533     534    +1     
======================================================================================
+ Hits                                                        13802   13849   +47     
- Misses                                                       8054    8067   +13     
- Partials                                                      452     453    +1

Flag	Coverage Δ
js-unit-tests	`54.1% <ø> (+0.1%)`	⬆️
php-unit-tests	`63.5% <71.3%> (+<0.1%)`	⬆️

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

Files	Coverage Δ
src/Coupon/CouponSyncer.php	`72.5% <100.0%> (+1.2%)`	⬆️
...ternal/DependencyManagement/JobServiceProvider.php	`0.0% <ø> (ø)`
src/Jobs/AbstractProductSyncerBatchedJob.php	`100.0% <100.0%> (ø)`
src/Jobs/AbstractProductSyncerJob.php	`100.0% <100.0%> (ø)`
src/MerchantCenter/MerchantCenterService.php	`94.2% <100.0%> (+0.1%)`	⬆️
src/Product/ProductSyncer.php	`98.8% <100.0%> (+0.1%)`	⬆️
src/API/WP/NotificationsService.php	`90.2% <0.0%> (-4.9%)`	⬇️
src/Jobs/AbstractCouponSyncerJob.php	`0.0% <0.0%> (ø)`
src/Options/Options.php	`62.3% <0.0%> (-2.4%)`	⬇️
src/Coupon/SyncerHooks.php	`74.5% <62.5%> (-3.1%)`	⬇️
... and 1 more

... and 6 files with indirect coverage changes

…nto add/block-push-products

puntope · 2024-03-22T09:56:56Z

Notice I didn't include testing steps for Sync Product.

This is because of an issue in bookings https://github.com/woocommerce/woocommerce-bookings/issues/3784

However, if bookings is not installed it should work perfectly. (Blocked when WPCOM API is authorized, working otherwise)

jorgemd24

Hi @puntope for working on this! It worked as expected however I left some comments about the code that we could discuss a bit more.

jorgemd24 · 2024-03-22T09:43:35Z

src/MerchantCenter/MerchantCenterService.php

@@ -103,7 +103,7 @@ public function is_google_connected(): bool {
 	 * @since 1.13.0
 	 * @return boolean
 	 */
-	public function is_ready_for_syncing(): bool {
+	public function is_ready(): bool {


What do you think if we add a @SInCE indicating that this function was renamed? Otherwise, it seems like is_ready() has been there since 1.13.0. However, I'm not sure if renaming the function to is_ready is necessary. To me, "syncing" implies that my site and the MC will be synchronized, regardless of the method used ("push" or "pull").

Adjusted (I rolled back the renaming)

jorgemd24 · 2024-03-22T09:55:08Z

src/MerchantCenter/MerchantCenterService.php

+	 * @return bool
+	 * @since x.x.x
+	 */
+	public function should_sync(): bool {


Wouldn't it be better to say something like "should_notify"? To me, "sync" is ambiguous as I mentioned earlier; it could refer to either a push or pull method.

I changed it to should_push()

jorgemd24 · 2024-03-22T10:13:58Z

src/Product/SyncerHooks.php

@@ -294,6 +316,25 @@ protected function handle_delete_product( int $product_id ) {
 		}
 	}

+	/**
+	 * Send the notification for product deletion


I'd phrase it as "schedule a job to send the product deletion notification" because it doesn't directly send the notification.

jorgemd24 · 2024-03-22T10:34:08Z

src/Jobs/AbstractProductSyncerJob.php

-	/**
-	 * Get whether Merchant Center is connected and ready for syncing data.
-	 *
-	 * @return bool
-	 */
-	public function is_mc_ready_for_syncing(): bool {
-		return $this->merchant_center->is_ready_for_syncing();
-	}
-


How about we leave this as it is and simply replace $this->merchant_center->is_ready_for_syncing() with something like $this->merchant_center->is_ready_for_syncing() && ! $this->merchant_center->should_notify()? We could apply the same change to the other jobs as well.

I'm towards keeping it like it's. That function seems a bit redundant and has attached a full interface for it.

Besides that, all the is_mc_ready_for_syncing are doing the same in all the classes. So doesn't seem a very useful function.

Is there any reason you found for keeping it?

I don't have any specific reason; I was simply curious why we need to remove it now when we'll be removing all this code in the next stages anyway.

jorgemd24 · 2024-03-22T10:37:48Z

src/Product/SyncerHooks.php

-		$update_by_object = function ( int $product_id, WC_Product $product ) {
-			$this->handle_update_products( [ $product ] );
-		};
-
-		$update_by_id = function ( int $product_id ) {
-			$product = $this->wc->maybe_get_product( $product_id );
-			if ( $product instanceof WC_Product ) {
-				$this->handle_update_products( [ $product ] );
-			}
-		};
-
-		$pre_delete = function ( int $product_id ) {
-			$this->handle_pre_delete_product( $product_id );
-		};
-
-		$delete = function ( int $product_id ) {
-			$this->handle_delete_product( $product_id );
-		};
-


What is the reason for moving these as methods instead of keeping them as anonymous functions? My understanding was that the idea behind using anonymous functions was to make it more difficult for other plugins to unhook these actions.

This is mainly for the testing.

For example for doing

$this->assertFalse( has_action( 'woocommerce_new_coupon', [ $this->syncer_hooks, 'update_by_id' ] ) );

https://github.com/woocommerce/google-listings-and-ads/pull/2326/files#diff-afa7e4fae2bc017bdf473aacdcf84a1720a7f73a22e83979eda07d4696b6cd41R271

jorgemd24 · 2024-03-22T11:11:06Z

src/Product/ProductSyncer.php

+		if ( ! $this->merchant_center->should_sync() ) {
+			do_action(
+				'woocommerce_gla_error',
+				'Cannot sync any products because they are being fetched automatically.',
+				__METHOD__
+			);
+
+			throw new ProductSyncerException(
+				__(
+					'Manual product sync will not run if the automatic data fetching is enabled. Please review your configuration in Google Listing and Ads settings.',
+					'google-listings-and-ads'
+				)
+			);
+		}


I wanted to test these errors, but I noticed that validate_merchant_center_setup is only triggered when the product syncers job is processing the items. So theoretically, we shouldn't encounter this error. However, I think it's a good idea to keep it just in case. While reviewing the code, I saw that for the notification part, we don't have anything similar to "validate_merchant_center_setup". We only check if the notification service is enabled, but not the MC setup. should we add something similar to the notification? Or am I missing something?

google-listings-and-ads/src/Jobs/Notifications/AbstractNotificationJob.php

Line 88 in 1f3a180

return apply_filters( 'woocommerce_gla_notification_job_can_schedule', $this->notifications_service->is_enabled() && parent::can_schedule( $args ), $this->get_job_name(), $args );

I wanted to test these errors, but I noticed that validate_merchant_center_setup is only triggered when the product syncers job is processing the items. So theoretically, we shouldn't encounter this error.

You can trigger it in ConnectionTest Page for example

We only check if the notification service is enabled, but not the MC setup. should we add something similar to the notification? Or am I missing something?

We can do so in a follow up. I added the item to the list.

jorgemd24 · 2024-03-22T11:11:48Z

src/Product/ProductSyncer.php

 			do_action( 'woocommerce_gla_error', 'Cannot sync any products before setting up Google Merchant Center.', __METHOD__ );

 			throw new ProductSyncerException( __( 'Google Merchant Center has not been set up correctly. Please review your configuration.', 'google-listings-and-ads' ) );
 		}
+
+		if ( ! $this->merchant_center->should_sync() ) {


Also what do you think if we add some tests for this?

The tests were already added in

ProductSyncerTest - https://github.com/woocommerce/google-listings-and-ads/pull/2326/files#diff-e1a250ff800d7687a728767bcc4817c6ca40f2fa9e97d9846e4b967ff338ee1fR551

CouponSyncerTest - https://github.com/woocommerce/google-listings-and-ads/pull/2326/files#diff-19117240c30f979181c4332c987f76f2ab19a5fd4b460d3345c8e68d195a1116R149

Do you miss any case?

See my other comments about the tests.

puntope · 2024-03-25T12:27:54Z

Hi @jorgemd24 thanks for the review.

I applied the renaming suggestions and clarified the rest of the comments.

Can you take another look?

Thx

jorgemd24

Thanks @puntope for adjusting the naming, I think it's clearer now. I've left some comments about the tests, and I'm not sure why the package-lock.json is committed in this PR. Anyway, the PR worked as expected so I am approving this PR in advanced! Thanks!

jorgemd24 · 2024-03-25T15:37:38Z

package-lock.json

@@ -6,7 +6,7 @@
 	"packages": {
 		"": {
 			"name": "google-listings-and-ads",
-			"version": "2.6.2",
+			"version": "2.6.3",


Not sure why the package-lock-json has been commited

jorgemd24 · 2024-03-25T15:49:21Z

tests/Unit/Product/ProductSyncerTest.php

@@ -548,6 +548,54 @@ protected function mock_google_service( $successful_products, $failed_products )
 			->willReturnCallback( $callback );
 	}

+	public function test_update_throws_exception_when_mc_is_blocked() {
+		$merchant_center = $this->createMock( MerchantCenterService::class );
+		$merchant_center->expects( $this->any() )


If we want to test the exception when should_push returns false, then we should mock is_ready_for_syncing to return true; otherwise, should_push never gets called. Also, I think it's better to expect an exact number instead of any, as in this case the test would pass without the method being called.

jorgemd24 · 2024-03-25T16:14:15Z

src/Jobs/AbstractProductSyncerJob.php

-	/**
-	 * Get whether Merchant Center is connected and ready for syncing data.
-	 *
-	 * @return bool
-	 */
-	public function is_mc_ready_for_syncing(): bool {
-		return $this->merchant_center->is_ready_for_syncing();
-	}
-


I don't have any specific reason; I was simply curious why we need to remove it now when we'll be removing all this code in the next stages anyway.

jorgemd24 · 2024-03-25T16:15:12Z

src/Product/ProductSyncer.php

 			do_action( 'woocommerce_gla_error', 'Cannot sync any products before setting up Google Merchant Center.', __METHOD__ );

 			throw new ProductSyncerException( __( 'Google Merchant Center has not been set up correctly. Please review your configuration.', 'google-listings-and-ads' ) );
 		}
+
+		if ( ! $this->merchant_center->should_sync() ) {


See my other comments about the tests.

Block syncing when notifications are enabled

fe31b81

github-actions bot added changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement. labels Mar 20, 2024

puntope changed the title ~~Block syncing when notifications are enabled~~ API PULL - Block syncing when notifications are enabled Mar 20, 2024

puntope added 6 commits March 21, 2024 16:32

Merge branch 'add/grant-access-to-google-wpcom-app-in-setting-page' i…

07b12b9

…nto add/block-push-products

Adding tests coverage for Product SyncerHooks

dcade83

Adding tests coverage for Coupon SyncerHooks

8725e17

Adding tests coverage for ProductSyncer

e966e9c

Adding tests coverage for CouponSyncer

9369311

PHPCS

31535f3

puntope self-assigned this Mar 21, 2024

PHPCS Tests

1f3a180

puntope requested a review from a team March 21, 2024 18:44

puntope marked this pull request as ready for review March 21, 2024 18:44

puntope mentioned this pull request Mar 22, 2024

Google API Project #2146

Open

35 tasks

jorgemd24 reviewed Mar 22, 2024

View reviewed changes

puntope added 2 commits March 25, 2024 12:57

Rollback function renaming

aaff1cf

Tweak errors description

31b90fe

puntope requested a review from jorgemd24 March 25, 2024 12:28

jorgemd24 approved these changes Mar 25, 2024

View reviewed changes

puntope added 2 commits March 26, 2024 12:06

Restore package.json

9d965d5

Tweak tests

ac18037

puntope merged commit 7b6fa24 into add/grant-access-to-google-wpcom-app-in-setting-page Mar 26, 2024
13 checks passed

puntope deleted the add/block-push-products branch March 26, 2024 11:48

puntope mentioned this pull request Mar 26, 2024

API PULL - Validate MC connection in notifications #2336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API PULL - Block syncing when notifications are enabled #2326

API PULL - Block syncing when notifications are enabled #2326

puntope commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

puntope commented Mar 22, 2024

jorgemd24 left a comment

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024

puntope Mar 25, 2024

jorgemd24 Mar 25, 2024

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024

puntope Mar 25, 2024

jorgemd24 Mar 22, 2024

puntope Mar 25, 2024 •

edited

Loading

jorgemd24 Mar 25, 2024

puntope commented Mar 25, 2024

jorgemd24 left a comment

jorgemd24 Mar 25, 2024

jorgemd24 Mar 25, 2024

jorgemd24 Mar 25, 2024

jorgemd24 Mar 25, 2024

API PULL - Block syncing when notifications are enabled #2326

API PULL - Block syncing when notifications are enabled #2326

Conversation

puntope commented Mar 20, 2024 • edited Loading

Changes proposed in this Pull Request:

Detailed test instructions:

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

puntope commented Mar 22, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope commented Mar 25, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

puntope Mar 25, 2024 •

edited

Loading