-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Scheduled notifications using meta handler #2202
Scheduled notifications using meta handler #2202
Conversation
Thanks for presenting these two strategies as separate PR's it's helpful to see how it works. Personally I like the initial approach in this PR more, but I think we should do some changes to support the benefits of both PR's, while reducing the cons of both approaches. For example these are some of my preferences:
I'm also not super clear on the notification meta which is set for the product. As far as I understand this is to prevent multiple updates from being scheduled shortly after each other. But looking at the code it seems that this status is never completely cleared. So if I have an updated trigger then the product meta will get set to PENDING_UPDATED, and then later once the notification is sent it will be changed to UPDATED, but what happens after that? Does it stay the status UPDATED, so if we legitimately update the product a second time it won't be able to send an update notification? Similar if I update a product and then right away delete it, based on the code it looks like it would block the delete notification from sending because an update notification was still pending. Another thing I noticed is the way it's setup in the SyncerHooks. Even though we don't trigger
If scheduled tasks are all executed in consecutive order (and each task runs without failures or retries), then this wouldn't happen, but that's not something we can guarantee. Hence I think this would be another argument why dropping the batches would be a good idea. At the time when we call Let me know what you think about these suggestions, happy to discuss them further. |
Agree with this, I would prefer to keep this independent from the rest of the code, one for the reason that @mikkamp mentioned earlier, that eventually we will remove all the code related to the Product Syncing and second I think the other solution could cause issues with the current implementations creating more friction in the migration process.
I don't really see this as a disadvantages. It actually would helps us break down the logic and makes it easier to read.
Yeah, I think so too. Sending all the variations seems unnecessary. A more efficient approach would be to include the variation_id so that Google can fetch the specific item. But for now, I guess we can just send the parent id, and Google will have to fetch the complete product. I played around with
And then the
This way, we could use the same job for different topics, regardless if it's a product, coupon, shipping zone, and so on. Another benefit of using |
Thanks, @mikkamp and @jorgemd24 For all the suggestions. I also think the approach followed in this PR is cleaner. So I will close the other one and continue working in this direction.
Yes, the metas are for reducing duplications and inconsistencies, for example, if a product has not states "CREATED", "UPDATED" or "PENDING_UPDATE" we don't trigger "PENDING_DELETE" because the product was not notified creation yet. So google would receive The status UPDATED allows you to call "deletes" and "updates" in further changes. |
Regarding batches, if we are not going to introduce batches we don't really need jobs... we can just call the notify function I guess. And reduce quite a lot the logic. However, I thought a product with let's say 20 variants... would be too much (20 requests to WPCOM) That's why I introduced batched jobs. |
Another alternative, if using batches, is to have a single NotificationJob and depending on the state, it calls different topics. For example, if it has PENDING_UPDATE... it will call topic |
If it's ok to send just the parent, then I guess we don't need jobs. |
I think the benefit of having the job is that the request will happen in the background and not during updating/creating/deleting the products. Also, what would happen if you do a bulk update of products without having a job? Would we make all the requests at once? |
Since it is a single request. I still don't see the need for the job.
For the bulk update, the way WordPress works is that it will trigger the hook one time for each product. So, at the end, for 1000 products, it will trigger 1000 jobs. The good thing about using jobs in batches is that we can omit the scheduled job call, and just update the metas. Then we can have a single recurring batched job notifying products in the background based on those metas. |
From my understanding, that's similar to how it works now: We made a single request/job for each product updated. Instead, if we don't use jobs, those 1000 requests will be executed during the bulk update. With jobs, these requests can be spread out over an interval since the AS will execute them when possible. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feature/google-api-project #2202 +/- ##
==============================================================
- Coverage 58.6% 58.5% -0.1%
- Complexity 4211 4248 +37
==============================================================
Files 456 458 +2
Lines 16827 16919 +92
==============================================================
+ Hits 9864 9906 +42
- Misses 6963 7013 +50
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Hi @jorgemd24 and @mikkamp I updated this PR adding a job scheduler without batches. Note: After discussing with @mikkamp non published products are not notified (except for deletion if they go from publish to draft for example) |
Notice there is a problem with PHP 7.3 I'm not sure whats going on and I'm investigating why tests are failing there |
Thanks for all the refactoring it's definitely going in the right direction. I had an initial look and added some more clarification. But I didn't have a chance to fully test the code yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @puntope, for the improvements! Overall, it's working well, but I still have some concerns about relying on previous notification updates as It could lead to easily going out of sync and sending incorrect notifications, such as deleting products in the MC. Also see my comment about existing merchants using the GL&A plugin.
Additionally, I left a comment about the woocommerce_gla_is_ready_to_notify
filter because I'm not sure if it's producing the expected result.
I am happy to approve this PR so we can set the initial code, and then we can address the other comments in further PRs. Let me know what you think.
src/Google/NotificationsService.php
Outdated
@@ -95,7 +115,7 @@ private function notification_error( int $item_id, string $topic, string $error | |||
* @param array $args | |||
* @return array|\WP_Error | |||
*/ | |||
protected function do_request( $args ) { | |||
protected function do_request( array $args ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing return type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here 08efcce
|
||
if ( $this->notifications_service->notify( $item, $topic ) ) { | ||
$this->set_status( $item, $this->get_after_notification_status( $topic ) ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted here d93c7b7
} | ||
|
||
/** | ||
* @param WC_Product $product |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a PHPDoc explaining this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted here cbb00a2
* | ||
* @return bool | ||
*/ | ||
public function has_notified_creation( WC_Product $product ): bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we don't take in consideration the previous statuses of (PUSH based) MC for sending the Notifications
Imagine a merchant using the plugin and having products already synced in the MC before the notification system was implemented. Then, they start using the new version of the plugin, which includes the notification system. For that merchant, we'll end up sending the wrong notifications because the initial metadata isn't set.
Another potential concern is that if one of the topic's metadata fields isn't set, we'll begin sending incorrect notifications, leading to an out-of-sync situation. I believe relying on previous notification states seems a bit risky.
* @param bool $value The current filter value. | ||
* @param WC_Product $product The product for the notification. | ||
*/ | ||
return apply_filters( 'woocommerce_gla_is_ready_to_notify', $is_ready, $product ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I could use this filter to stop notifications for a specific product, say 123, but instead of stopping them, it's triggering the product.delete
notification for the product in the MC. Is this expected behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe yes. If a product has notified creation and is not anymore ready for notify, it should be deleted.
public function should_trigger_delete_notification( WC_Product $product ): bool {
return ! $this->is_ready_to_notify( $product ) && $this->has_notified_creation( $product );
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining a scenario where I have my product in the MC, and I want to keep it and manage it myself. So, I would use this filter to stop the notifications. WDYT?
* @since x.x.x | ||
* | ||
* @param bool $value The current filter value. By default, it is the result of `$this->can_schedule` function. | ||
* @param array $args The arguments for the schedule function. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add that $args
is composed of item_id
and topic
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added here 677e4ec
Thanks @jorgemd24 for the helpful insights I moved the discussion in the p2 pcTzPl-1OX-p2#comment-3036 I added some code for now, supporting already synced products in MC. So, in case after discussion we decide we don't, I will remove it. |
Thanks @jorgemd24 for the reviews. I adjusted the code based on your comments and answered your questions. |
I added some tests for the Job here 65dff81 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some code for now, supporting already synced products in MC. So, in case after discussion we decide we don't, I will remove it.
Thanks, @puntope, for the adjustments. However, now when I trash a product and then restore it, I get the following:
- When I trash the product, I get the delete notification as expected.
- However, when I restore it, I get the update notification instead of the create notification. This happens because the product is still "synced" and still has the
gla_google_ids
in the database.
For now, I think it's better to leave it as it was until we decide what to do with existing products.
Another concern that comes to mind is what happens for a store with a high volume of jobs. The job will take some time to be executed, and the status can be incorrect. Similar to the earlier scenario, the following will happen:
- Trash the product.
- Now the status will be "pending_delete" -> eventually we will send a delete notification.
- Restore the product while the status is still "pending_delete".
- You get a "product.update" notification job. I would expect a create notification.
So, I think this PR is a good initial approach. However, I have my concerns about the status and the metadata. I'm approving this PR so we can address these discussions in a different PR based on the discussion that are in the P2.
@@ -5,12 +5,13 @@ | |||
|
|||
use Automattic\WooCommerce\GoogleListingsAndAds\Google\NotificationsService; | |||
use Automattic\WooCommerce\GoogleListingsAndAds\Tests\Framework\UnitTest; | |||
use WC_Helper_Product; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused WC_Helper_Product
* | ||
* @param array $args Arguments with the item id and the topic | ||
*/ | ||
protected function process_items( array $args ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing return types, I haven't checked all the files specifically, but I see a mix of functions with types and return types, and others without.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
: void is the default return type so seems not necessary. However I added them if you think is more convinient
* @param bool $value The current filter value. | ||
* @param WC_Product $product The product for the notification. | ||
*/ | ||
return apply_filters( 'woocommerce_gla_is_ready_to_notify', $is_ready, $product ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining a scenario where I have my product in the MC, and I want to keep it and manage it myself. So, I would use this filter to stop the notifications. WDYT?
Thanks @jorgemd24 I missed that case. If we follow the approach of considering the current MC as a parameter for the notifications. Then on deletion, we should also delete those metas. Or at least tweak the functions for doing it in another way.
That's right, If the notification was under PENDING_DELETE it didn't send the notification for delete yet. In the previous approach, in case the product changes again it would change the status to PENDING_UPDATE (because delete was never called). However, now it doesn't work that way as we process the items right away, so I tweaked the function to not use PENDING_DELETE as status for the |
For that right now, you can use DONT_SYNC_AND_SHOW and create the product yourself in MC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll go ahead and approve this as we can continue tweaking in followup PR's to get the status right.
The only comment I have is to also call the is_ready_to_notify
function from ProductNotificationJob::process_items
, and stop early if it's false. Since it could take a while before the notification is actually sent.
Also can we just get the PHPCS checks to pass so we end up with a bit cleaner code:
Changes proposed in this Pull Request:
This PR adds some work for scheduling the Notifications in batches.
The strategy of this PR is to set up some product metas and then filter them in a Notification Job.
For that this PR:
ProductHelper.php
with some helper functions regarding Notifications.ProductMetaHandler.php
creating some metas for the notifications.NotificationStatus.php
to hold the Statuses for the Notification metasProductRepository.php
adding some functions to filter the products by Notifications meta.SyncerHooks.php
in order to trigger the Notification Jobs.NotificationsService.php
adding some functions and dependencies.The way this works is that on Update Product we check if the Notifications are enabled. (For now using a filter). If so, instead of sync to the Merchant we updated some product metas marking a product as PENDING_CREATE, PENDING_DELETE or PENDING_UPDATE. Then we schedule the Jobs that filter those products based on the metas in batches. Finally, we process those items and update their metas to CREATED, UPDATED, DELETED.
The advantages of this strategy are:
Detailed test instructions:
No need to test in detail the PR. Just offering me some feedback about the strategy. Final PR will come afterwards.
Here are some test to see how it works
gla/jobs/notifications//process_item
with one item (the product ID just created) and product.created as topicgla/jobs/notifications/process_item
with one item (the product ID just created) and product.update as topicgla/jobs/notifications/process_item
with one item (the product ID just created) and product.delete as topicgla/jobs/notifications/process_item
with one item (the product ID just restored) and product.create as topicgla/jobs/notifications/process_item
with one item (the product ID just restored) and product.delete as topicgla/jobs/notifications/process_item
with one item (the product ID just restored) and product.create as topicgla/jobs/notifications/process_item
with one item (the product ID just restored) and product.delete as topicgla/jobs/notifications//process_item
with one item (the product ID just created) and product.created as topicAdditional details:
Changelog entry