API PULL - Prevent notification errors for deleted products #2356

puntope · 2024-04-09T09:53:53Z

Changes proposed in this Pull Request:

When we send notifications we take as granted that the product or coupon are available in DB when handling its metas. This is not always true as the Job and some intermediate functions can run after the Product or Coupon is permanently deleted.

When this happens, the notifications meta handler or some other functions will throw some exceptions.

This PR fixes that ensuring that the item is available. Introducing the next changes:

Check the item is available on Update and Create Topics. Boil in case it isn't.
Run the delete Notification right away (without AS job) when the product doesn't exist.
Consider NON-existent products as already Notified products. This could trigger (very rarely, I wasn't able to reproduce it yet) unnecessarily DELETE notifications when we permanently delete a product that was never notified. But I think even if this may happen it would be better than missing a needed notification.

Detailed test instructions:

Follow the steps in https://github.com/woocommerce/google-listings-and-ads/wiki/API-PULL-Feature-(Beta)#enabling-the-feature and https://github.com/woocommerce/google-listings-and-ads/wiki/API-PULL-Feature-(Beta)#enabling-the-feature for enabling and granting access to Notifications feature.
Create a Product - See the gla/jobs/notifications/products/process_item job is scheduled with CREATE topic.
Run it. See the Notification happens in the Logs
Update the product - See the gla/jobs/notifications/products/process_item job is scheduled with UPDATE topic.
Run it. See the Notification happens in the Logs
Trash the product - See the gla/jobs/notifications/products/process_item job is scheduled with DELETE topic.
Run it. See the Notification happens in the Logs
Create a Product - See the gla/jobs/notifications/products/process_item job is scheduled with CREATE topic.
Don't run it.
Delete the product via wp_delete_post( {post_id}, true );
Run the job from step 8. See the Notification DOESNT happen in the Logs. NO exception happens.
Repeat steps 2-4. Don't run the UPDATE topic Job
Delete the product via wp_delete_post( {post_id}, true );
Run the update job from step. See the Notification DOESNT happen in the Logs. NO exception happens.
Repeat steps 2-3 and 6. Don't run the DELETE topic Job
Delete the product via wp_delete_post( {post_id}, true );
Now run the job. See the Notification YES happens in the Logs. NO exception happens.
Repeat all the steps for Coupons.

Additional details

I considered as well having some kind of table/options to keep the Notification metas so we don't depend anymore on the item itself. But that would require a big re-structure of the Notifications so this may be discussed for phase 2.

codecov · 2024-04-09T09:58:12Z

Codecov Report

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

Project coverage is 64.2%. Comparing base (aec6d57) to head (50a8dd5).
Report is 79 commits behind head on feature/google-api-project.

❗ Current head 50a8dd5 differs from pull request most recent head 0280bde. Consider uploading reports for the commit 0280bde to get more accurate results

Additional details and impacted files

@@                      Coverage Diff                       @@
##             feature/google-api-project   #2356     +/-   ##
==============================================================
- Coverage                          64.2%   64.2%   -0.0%     
- Complexity                         4484    4486      +2     
==============================================================
  Files                               471     471             
  Lines                             18862   18879     +17     
==============================================================
+ Hits                              12117   12120      +3     
- Misses                             6745    6759     +14

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

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

Files	Coverage Δ
...Jobs/Notifications/HelperNotificationInterface.php	`0.0% <ø> (ø)`
src/Coupon/CouponHelper.php	`68.8% <71.4%> (+1.4%)`	⬆️
...Jobs/Notifications/AbstractItemNotificationJob.php	`89.5% <66.7%> (-10.5%)`	⬇️
src/Product/ProductHelper.php	`90.7% <71.4%> (-1.4%)`	⬇️
src/Coupon/SyncerHooks.php	`73.2% <58.3%> (-1.3%)`	⬇️
src/Product/SyncerHooks.php	`81.5% <53.8%> (-1.6%)`	⬇️

... and 2 files with indirect coverage changes

…tion-erorrs-deleted-products # Conflicts: # src/Jobs/Notifications/AbstractItemNotificationJob.php

ianlin

Thanks for handling the case when products/coupons no longer exist before sending notifications. I noticed that when I try to delete a product, the notification coupon.delete would be triggered as well:

DEBUG Automattic\WooCommerce\GoogleListingsAndAds\API\WP\NotificationsService::notify Notification - Item ID: 143 - Topic: coupon.delete - Data []

ianlin · 2024-04-12T02:53:49Z

src/Coupon/CouponHelper.php

+		try {
+			$coupon = $this->get_wc_coupon( $coupon_id );
+		} catch ( InvalidValue $e ) {
+			return true; // Sent true for forcing delete notification as the product doesn't exist anymore.


Should be coupon in the comment.

ianlin · 2024-04-12T02:54:07Z

src/Coupon/SyncerHooks.php

@@ -299,11 +299,17 @@ protected function handle_delete_coupon( int $coupon_id ) {
 	protected function maybe_send_delete_notification( int $coupon_id ): void {
 		$coupon = $this->wc->maybe_get_coupon( $coupon_id );

-		if ( $coupon instanceof WC_Coupon && $this->coupon_helper->should_trigger_delete_notification( $coupon ) ) {
+		if ( is_null( $coupon ) ) {
+			// In case product is not anymore in DB we send the notification directly.


Should be coupon in the comment.

ianlin · 2024-04-12T03:22:25Z

src/Coupon/SyncerHooks.php

@@ -299,11 +299,17 @@ protected function handle_delete_coupon( int $coupon_id ) {
 	protected function maybe_send_delete_notification( int $coupon_id ): void {
 		$coupon = $this->wc->maybe_get_coupon( $coupon_id );

-		if ( $coupon instanceof WC_Coupon && $this->coupon_helper->should_trigger_delete_notification( $coupon ) ) {
+		if ( is_null( $coupon ) ) {


I guess since we are also listening to actions trashed_post or deleted_post for triggering delete coupons function, this line would cause the coupon.delete notification being triggered when a product is trashed.

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

Lines 134 to 135 in 61fe02d

add_action( 'trashed_post', [ $this, 'delete_by_id' ], 90 );

add_action( 'deleted_post', [ $this, 'delete_by_id' ], 90 );

Thanks @ianlin. That is expected.

When the item is trashed is still available in the DB and it won't be null and we schedule the deletion Notification. In case the item is not anymore in DB we notify the deletion directly.

Yeah, I understand in this PR it triggers the deletion notifications straightaway. But my question was a bit different: when I tried to delete a product, the coupon deletion notification would be triggered as well. It did not happen prior to this PR.

…tion-erorrs-deleted-products

Check if product is still available.

50a8dd5

puntope requested a review from a team April 9, 2024 09:53

puntope self-assigned this Apr 9, 2024

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

puntope marked this pull request as ready for review April 9, 2024 09:57

puntope added 2 commits April 10, 2024 15:20

Merge branch 'feature/google-api-project' into tweak/prevent-notifica…

e4ebb6c

…tion-erorrs-deleted-products # Conflicts: # src/Jobs/Notifications/AbstractItemNotificationJob.php

Add Data in direct notify calls

61fe02d

ianlin reviewed Apr 12, 2024

View reviewed changes

puntope added 2 commits April 23, 2024 16:48

Merge branch 'feature/google-api-project' into tweak/prevent-notifica…

de1f30f

…tion-erorrs-deleted-products

Update typo in docs

0280bde

Base automatically changed from feature/google-api-project to develop July 26, 2024 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API PULL - Prevent notification errors for deleted products #2356

API PULL - Prevent notification errors for deleted products #2356

puntope commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

ianlin left a comment •

edited

Loading

ianlin Apr 12, 2024

ianlin Apr 12, 2024

ianlin Apr 12, 2024

puntope Apr 13, 2024

ianlin Apr 15, 2024

	add_action( 'trashed_post', [ $this, 'delete_by_id' ], 90 );
	add_action( 'deleted_post', [ $this, 'delete_by_id' ], 90 );

API PULL - Prevent notification errors for deleted products #2356

Are you sure you want to change the base?

API PULL - Prevent notification errors for deleted products #2356

Conversation

puntope commented Apr 9, 2024 • edited Loading

Changes proposed in this Pull Request:

Detailed test instructions:

Additional details

codecov bot commented Apr 9, 2024 • edited Loading

Codecov Report

ianlin left a comment • edited Loading

Choose a reason for hiding this comment

ianlin Apr 12, 2024

Choose a reason for hiding this comment

ianlin Apr 12, 2024

Choose a reason for hiding this comment

ianlin Apr 12, 2024

Choose a reason for hiding this comment

puntope Apr 13, 2024

Choose a reason for hiding this comment

ianlin Apr 15, 2024

Choose a reason for hiding this comment

puntope commented Apr 9, 2024 •

edited

Loading

codecov bot commented Apr 9, 2024 •

edited

Loading

ianlin left a comment •

edited

Loading