Block Shipping, Products and Coupons when Sync Push is disabled #2810
Conversation
github-actions
bot
added
type: enhancement
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/api-pull-sync-endpoint #2810 +/- ##
==================================================================
- Coverage 67.3% 67.2% -0.1%
- Complexity 4705 4711 +6
==================================================================
Files 481 481
Lines 19713 19746 +33
==================================================================
+ Hits 13267 13275 +8
- Misses 6446 6471 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks @puntope for working on these changes.
As explained in the comments, I think we should follow some of the simple logic as is done for Shipping. As it is right now a product/coupon status will still be changed to pending sync even if push is disabled, we should not allow any changes if it's disabled.
| public function is_enabled_for_datatype( string $data_type ): bool { | ||
| /** @var NotificationsService $notifications_service */ | ||
| $notifications_service = $this->container->get( NotificationsService::class ); | ||
| return $notifications_service->is_push_enabled_for_datatype( $data_type ); | ||
| } |
There was a problem hiding this comment.
So two confusing things about this function:
- If it's actually just a wrapper to call
is_push_enabled_for_datatypethen why doesn't the name also reflect that it's checking if push is enabled? - Push as nothing to do with notifications, so why is it calling a notification service?
Reasoning why I think we shouldn't call the notification service is because we should think of push and pull as two completely independent parts, which can both run together (dark launch), or toggled between the two. We could also decide to later remove one. So making push logic dependent on notifications (which is pull only) doesn't separate the two. Should we have a separate service handling the overlapping logic between the two?
| $this->job_repository->get( UpdateShippingSettings::class )->schedule(); | ||
| if ( $this->merchant_center->is_enabled_for_datatype( NotificationsService::DATATYPE_SHIPPING ) ) { | ||
| $this->job_repository->get( UpdateShippingSettings::class )->schedule(); | ||
| } |
There was a problem hiding this comment.
So the changes here seem pretty straightforward with simple logic:
- If pull is enabled: send a notification
- If push is enabled: schedule a job to start the sync
But when I look at other SyncerHooks I don't see that same clear separation. For example for products I can see that if push is disabled, it will still perform the following steps for a single product (or a batch of products in case it's a variable product):
- Check if the product is sync ready
- Mark the product as pending sync
- If not sync ready prepare delete requests
- Schedule the products to be updated
- Schedule the products to be deleted
Now because can_scheduler will check if push is enabled, step 4 and 5 won't actually schedule the job in action scheduler. But in step 2 we already modified the product.
So I think we should apply the same simple logic like we did for the shipping syncerhooks, and not even attempt to do anything if push is not enabled.
Note
Same logic applies for coupons.
| @@ -62,6 +63,6 @@ public function __construct( | |||
| * @return bool Returns true if the job can be scheduled. | |||
| */ | |||
| public function can_schedule( $args = [] ): bool { | |||
| return ! $this->is_running( $args ) && $this->merchant_center->should_push(); | |||
| return ! $this->is_running( $args ) && $this->merchant_center->is_enabled_for_datatype( NotificationsService::DATATYPE_PRODUCT ); | |||
There was a problem hiding this comment.
As I outlined in the previous comment, we should prevent pushing at an earlier stage. I can however understand we want some logic checking within the syncer jobs, but shouldn't that be for when a job item is being processed.
Here we are only blocking a job from being scheduled. But what happens if we perform the actions in this order:
- Schedule a product update
- Disable product push
- Scheduled actions runs (at this step I'd expect it to bail as push is no longer enabled)
woocommerce_gla_batch_retry_update_products will also be happily triggered again without being blocked if the previous sync failed.
| // Confirm that the Merchant Center account is connected and the user has chosen for the shipping rates to be synced from WooCommerce settings. | ||
| return $this->merchant_center->is_connected() && $this->google_settings->should_get_shipping_rates_from_woocommerce(); | ||
| // Confirm that the Merchant Center account is connected, the user has chosen for the shipping rates to be synced from WooCommerce settings and the Push Sync is enabled for Shipping. | ||
| return $this->merchant_center->is_connected() && $this->google_settings->should_get_shipping_rates_from_woocommerce() && $this->merchant_center->is_enabled_for_datatype( NotificationsService::DATATYPE_SHIPPING ); |
There was a problem hiding this comment.
So here we are making the change in the right location. Because can_sync_shipping will be checked when scheduling the job but also when processing the items of the job. So if a job was already scheduled and the push disabled later it will still prevent the job from being run.
At most we might want to tweak the failure message to indicate it could also be because push is disabled:
Cannot sync shipping settings. Confirm that the merchant center account is connected and the option to automatically sync the shipping settings is selected.
Changes proposed in this Pull Request:
Block Merchant Center Sync in case the Sync is disabled for the datatype
Detailed test instructions:
gla/jobs/update_products/process_item. Run itgla/jobs/update_products/process_item. Run itgla/jobs/update_coupon/process_item. Run itgla/jobs/update_coupon/process_item. Run itgla/jobs/update_shipping_settings/process_itemAdditional details:
PT: pcTzPl-2yG-p2
Changelog entry