[API Pull] Tweak Variations Notifications #2450

puntope · 2024-07-03T14:12:21Z

Changes proposed in this Pull Request:

This PR tweaks the logic for notifications when handling variations. In particular:

FIXED: We were sending create notifications for variations when the parent was not published still
FIXED: We were only sending delete notifications for the parent when a variation was deleted.
FIXED: SOme unhandled exceptions when we try to access. a product that is not in the database anymore.

Detailed test instructions:

Checkout PR
Test that creation/update/deletion of simple product works as always
Create a Variable Product and generate variations for It
Go to Woo - Status - Scheduled actions - That step should not schedule any notifications
Asign price to some of the variations and publish the product
Go to Woo - Status - Scheduled actions - See scheduled notifications create for the variations with price + the variable product
Run the actions and see if they are logged in the gla log under Woo - Status - logs
Edit one of the variations without price
It doesn't schedule or log anything as it was not created yet
Remove one of the variations without price
It doesn't schedule or log anything as it was not created yet
Edit one of the variations with price
Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product. Run them
Remove the variation with price
Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product and one delete notification in the logs (directly without schedule) for the one you deleted with offer_id
Set the variable product as DONT SYNC AND SHOW
Go to Woo - Status - Scheduled actions - See scheduled notifications delete for the variations with price + the variable product
Before running it. Go back and set the variable product as SYNC AND SHOW
Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product
Run the delete notifications of the step 17
See in the logs they are not logged (a filter prevented it, because we set is as visible in step 18)
Run the tasks from step 19
See them logged in the log
Try to delete products and variations in different ways to see if everything is handled properly.

Additional details:

Changelog entry

src/Jobs/Notifications/AbstractItemNotificationJob.php

src/Product/ProductHelper.php

src/Product/SyncerHooks.php

codecov · 2024-07-03T14:16:25Z

Codecov Report

Attention: Patch coverage is 80.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 64.7%. Comparing base (7ff8a9e) to head (f03a407).
Report is 33 commits behind head on feature/google-api-project.

Additional details and impacted files

@@                      Coverage Diff                       @@
##             feature/google-api-project   #2450     +/-   ##
==============================================================
+ Coverage                          64.6%   64.7%   +0.1%     
- Complexity                         4552    4560      +8     
==============================================================
  Files                               473     473             
  Lines                             17757   17769     +12     
==============================================================
+ Hits                              11478   11501     +23     
+ Misses                             6279    6268     -11

Flag	Coverage Δ
php-unit-tests	`64.7% <80.0%> (+0.1%)`	⬆️

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

Files	Coverage Δ
src/Jobs/Notifications/ProductNotificationJob.php	`100.0% <100.0%> (ø)`
src/Product/ProductHelper.php	`92.1% <100.0%> (+0.2%)`	⬆️
src/Product/SyncerHooks.php	`90.0% <90.5%> (+5.3%)`	⬆️
...Jobs/Notifications/AbstractItemNotificationJob.php	`88.4% <37.5%> (-11.6%)`	⬇️

... and 3 files with indirect coverage changes

puntope · 2024-07-03T14:16:46Z

src/Product/SyncerHooks.php

-			);
+
+		if ( $product instanceof WC_Product && $this->product_helper->has_notified_creation( $product ) ) {
+			$this->notifications_service->notify( NotificationsService::TOPIC_PRODUCT_DELETED, $product_id, [ 'offer_id' => $this->product_helper->get_offer_id( $product_id ) ] );


For post deletions, it is better to send the notification directly.

Why is it better to send the notification directly instead of scheduling the job?

Because otherwise, it can be the case that the item is not in the database (for example, the user trash the product and deletes it from the trash before the action runs). In this cases an exception might happen or we cannot get anymore the product props.

Thanks for the explanation. However, this could be problematic because if you delete multiple products or a variable product with many variations (say 30), we'll end up making all the requests simultaneously. From the merchant's perspective, it will take a while to delete a product with multiple variations because all the requests need to be completed. Also, we could reach the API rate limit.

As far as I can tell, we only need to notify the product ID, topic, and offer_id, so can't we just schedule the job to notify that? Or do we need the actual product?

jorgemd24

Hi @puntope, thanks for fixing this. However, I found some issues:

When I trash a simple product, we don't scheduled the DELETE topic and it is aligned with your comment here: [API Pull] Tweak Variations Notifications #2450 (comment) but when I restore the simple product, we send an UPDATE topic instead of a CREATE topic.

Edit one of the variations without price
It doesn't schedule or log anything as it was not created yet
Remove one of the variations without price
It doesn't schedule or log anything as it was not created yet
Edit one of the variations with price

Not sure if I'm doing this right or understanding this part, but I see notifications scheduled for all the variations with prices, as well as for the parent.

Edit one of the variations with price
Go to Woo - Status - Scheduled actions - See scheduled notifications update for the variations with price + the variable product. Run them

I am unsure if this is ideal because we only updated one variation. Will Google make X (number of variations + parent product) REST API requests? It seems unnecessary to make so many requests. For example, if a product has 30 variations and only 1 is updated, we'll schedule 30 + 1 jobs, and the merchant will receive 31 API requests?

Also, I left some comments about the code, and it doesn't seem like we've added tests to check which variations need to be notified.

jorgemd24 · 2024-07-04T09:56:03Z

src/Jobs/Notifications/AbstractItemNotificationJob.php

-			$this->handle_notified( $topic, $item );
+		try {
+			if ( $this->can_process( $item, $topic ) && $this->notifications_service->notify( $topic, $item, $data ) ) {
+				$this->set_status( $item, $this->get_after_notification_status( $topic ) );


Should we update the PHPDocs to set_status and the other functions it calls to indicate that this method can throw an "InvalidValue" exception? Otherwise, it's hard to know which methods might throw the exception. From what I can see, the exception will be thrown here:

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

Lines 70 to 72 in 1b92296

protected function get_item( int $item_id ) {

return $this->helper->get_wc_product( $item_id );

}

jorgemd24 · 2024-07-04T09:58:40Z

src/Product/ProductHelper.php

+	 *
+	 * @return string The offer id
+	 */
+	public function get_offer_id( $product_id ) {


The rest of the file includes argument types. Should we add the type for $product_id as well?

Adjusted here 9e041f5

jorgemd24 · 2024-07-04T10:03:52Z

src/Product/SyncerHooks.php

-					'topic'   => NotificationsService::TOPIC_PRODUCT_DELETED,
-				]
-			);
+			$this->schedule_delete_notification( $product );


Not needed for this PR but maybe we could have similar methods for schedule_update_notificationand schedule_create_notification

For now, we are only calling those in that place. schedule_delete_notification was refactored to avoid code duplication.

jorgemd24 · 2024-07-04T11:24:40Z

src/Product/SyncerHooks.php

-			);
+
+		if ( $product instanceof WC_Product && $this->product_helper->has_notified_creation( $product ) ) {
+			$this->notifications_service->notify( NotificationsService::TOPIC_PRODUCT_DELETED, $product_id, [ 'offer_id' => $this->product_helper->get_offer_id( $product_id ) ] );


Why is it better to send the notification directly instead of scheduling the job?

puntope · 2024-07-05T08:06:48Z

but when I restore the simple product, we send an UPDATE topic instead of a CREATE topic.

Good catch. Fixed here. 413c7c5

puntope · 2024-07-05T11:34:09Z

I am unsure if this is ideal because we only updated one variation. Will Google make X (number of variations + parent product) REST API requests? It seems unnecessary to make so many requests. For example, if a product has 30 variations and only 1 is updated, we'll schedule 30 + 1 jobs, and the merchant will receive 31 API requests?

Seems like when we did activate the MC Push again we forgot to handle that
https://github.com/woocommerce/google-listings-and-ads/blob/develop/src/Product/SyncerHooks.php#L180

I did some tweaks for it also. Now should be trigger per variation and parent only.

See ff8468e

puntope · 2024-07-05T12:37:39Z

Thanks @jorgemd24 for all your feedback. I applied some changes. Can you take another look?

jorgemd24

Hi @puntope, thanks for the adjustments. It is working well and I'm getting the right results. However, I have one concern about making the request to the Notification endpoint directly instead of scheduling the job when deleting the item. See my comment.

Let me know if you would prefer to address this in this PR or a follow-up PR.

Thanks!

jorgemd24 · 2024-07-07T12:45:22Z

src/Product/SyncerHooks.php

-		if ( $this->notifications_service->is_ready() ) {
-			$this->handle_update_product_notification( $products[0] );
-		}
-
 		foreach ( $products as $product ) {


At this point, everything should be an instance of WC_Product. However, as an extra precaution, should we add a check like this before continuing with the loop: if (!$product instanceof WC_Product) continue;? I meant in these lines:

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

Lines 213 to 223 in 96f16b9

protected function handle_update_products( array $products, $notify = true ) {

$products_to_update = [];

$products_to_delete = [];

foreach ( $products as $product ) {

$product_id = $product->get_id();

// Avoid to handle variations directly. We handle them from the parent.

if ( $this->notifications_service->is_ready() && $notify ) {

$this->handle_update_product_notification( $product );

}

jorgemd24 · 2024-07-07T13:01:00Z

src/Product/SyncerHooks.php

-			);
+
+		if ( $product instanceof WC_Product && $this->product_helper->has_notified_creation( $product ) ) {
+			$this->notifications_service->notify( NotificationsService::TOPIC_PRODUCT_DELETED, $product_id, [ 'offer_id' => $this->product_helper->get_offer_id( $product_id ) ] );


Thanks for the explanation. However, this could be problematic because if you delete multiple products or a variable product with many variations (say 30), we'll end up making all the requests simultaneously. From the merchant's perspective, it will take a while to delete a product with multiple variations because all the requests need to be completed. Also, we could reach the API rate limit.

As far as I can tell, we only need to notify the product ID, topic, and offer_id, so can't we just schedule the job to notify that? Or do we need the actual product?

puntope · 2024-07-08T07:35:21Z

As far as I can tell, we only need to notify the product ID, topic, and offer_id, so can't we just schedule the job to notify that? Or do we need the actual product?

The logic know uses get_item that needs the product from DB. We can refactor that in future iterations. Since DarkL is next week I would suggest to have it working even if the performance is not the optimal.

p.s I also added the check for WC_Product. This is ready for a new round.

jorgemd24

Thanks for the adjustments is looking good.

The logic know uses get_item that needs the product from DB. We can refactor that in future iterations. Since DarkL is next week I would suggest to have it working even if the performance is not the optimal.

Yes I think we should give priority to this part as it can degrade the performance when deleting multiple products.

puntope added 4 commits July 3, 2024 15:05

Handle Product Variations creation and deletion

bbb4b6c

Tweak tests for get_offer_id

66a96f0

Test for variations notifications

2ad5d0e

PHPCS

1b92296

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

puntope self-assigned this Jul 3, 2024

puntope commented Jul 3, 2024

View reviewed changes

src/Jobs/Notifications/AbstractItemNotificationJob.php Show resolved Hide resolved

puntope commented Jul 3, 2024

View reviewed changes

src/Product/ProductHelper.php Show resolved Hide resolved

puntope commented Jul 3, 2024

View reviewed changes

src/Product/SyncerHooks.php Outdated Show resolved Hide resolved

puntope commented Jul 3, 2024

View reviewed changes

puntope marked this pull request as ready for review July 3, 2024 14:17

puntope requested a review from a team July 3, 2024 14:17

jorgemd24 reviewed Jul 4, 2024

View reviewed changes

Delete metas after notifying deletion

413c7c5

Tweak logic for varitions

ff8468e

puntope added 3 commits July 5, 2024 15:35

PHPCS

9ee9675

Add parameter definition

9e041f5

Add docs for exceptions

96f16b9

puntope requested a review from jorgemd24 July 5, 2024 12:37

jorgemd24 reviewed Jul 7, 2024

View reviewed changes

Check if product is WC_Product

f03a407

puntope requested a review from jorgemd24 July 8, 2024 07:35

jorgemd24 approved these changes Jul 8, 2024

View reviewed changes

puntope merged commit 0ba7fcd into feature/google-api-project Jul 8, 2024
12 checks passed

puntope deleted the tweak/notifications-variations branch July 8, 2024 12:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API Pull] Tweak Variations Notifications #2450

[API Pull] Tweak Variations Notifications #2450

puntope commented Jul 3, 2024

codecov bot commented Jul 3, 2024 •

edited

Loading

puntope Jul 3, 2024

jorgemd24 Jul 4, 2024

puntope Jul 5, 2024

jorgemd24 Jul 7, 2024

jorgemd24 left a comment

jorgemd24 Jul 4, 2024

jorgemd24 Jul 4, 2024

puntope Jul 5, 2024

jorgemd24 Jul 4, 2024

puntope Jul 5, 2024

jorgemd24 Jul 4, 2024

puntope commented Jul 5, 2024

puntope commented Jul 5, 2024

puntope commented Jul 5, 2024

jorgemd24 left a comment

jorgemd24 Jul 7, 2024 •

edited

Loading

jorgemd24 Jul 7, 2024

puntope commented Jul 8, 2024

jorgemd24 left a comment

	protected function get_item( int $item_id ) {
	return $this->helper->get_wc_product( $item_id );
	}

	protected function handle_update_products( array $products, $notify = true ) {
	$products_to_update = [];
	$products_to_delete = [];

	foreach ( $products as $product ) {
	$product_id = $product->get_id();

	// Avoid to handle variations directly. We handle them from the parent.
	if ( $this->notifications_service->is_ready() && $notify ) {
	$this->handle_update_product_notification( $product );
	}

[API Pull] Tweak Variations Notifications #2450

[API Pull] Tweak Variations Notifications #2450

Conversation

puntope commented Jul 3, 2024

Changes proposed in this Pull Request:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Jul 3, 2024 • edited Loading

Codecov Report

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

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

puntope commented Jul 5, 2024

puntope commented Jul 5, 2024

puntope commented Jul 5, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

jorgemd24 Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope commented Jul 8, 2024

jorgemd24 left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 3, 2024 •

edited

Loading

jorgemd24 Jul 7, 2024 •

edited

Loading