-
Notifications
You must be signed in to change notification settings - Fork 22
Block Shipping, Products and Coupons when Sync Push is disabled #2810
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
base: feature/api-pull-sync-endpoint
Are you sure you want to change the base?
Changes from all commits
1fb79ec
e87db7e
369f761
7d1bf31
06f5486
fb7babc
07d34cd
573cc78
1ca445b
89dff2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
|
||
use Automattic\WooCommerce\GoogleListingsAndAds\ActionScheduler\ActionSchedulerInterface; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\API\Google\Settings as GoogleSettings; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\API\WP\NotificationsService; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\MerchantCenter\MerchantCenterService; | ||
|
||
defined( 'ABSPATH' ) || exit; | ||
|
@@ -21,6 +22,7 @@ | |
* @since 2.1.0 | ||
*/ | ||
class UpdateShippingSettings extends AbstractActionSchedulerJob { | ||
|
||
/** | ||
* @var MerchantCenterService | ||
*/ | ||
|
@@ -97,7 +99,7 @@ public function schedule( array $args = [] ) { | |
* @return bool | ||
*/ | ||
protected function can_sync_shipping(): bool { | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here we are making the change in the right location. Because At most we might want to tweak the failure message to indicate it could also be because push is disabled:
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,17 +126,28 @@ public function is_ready_for_syncing(): bool { | |
} | ||
|
||
/** | ||
* Whether we should push data into MC. Only if: | ||
* - MC is ready for syncing {@see is_ready_for_syncing} | ||
* - Notifications Service is not enabled | ||
* Whether we should push data into MC. Only is MC is ready for syncing. | ||
* | ||
* @see is_ready_for_syncing | ||
* @return bool | ||
* @since 2.8.0 | ||
*/ | ||
public function should_push(): bool { | ||
return $this->is_ready_for_syncing(); | ||
} | ||
|
||
/** | ||
* Whether push is enabled for a specific data type. | ||
* | ||
* @param string $data_type The data type to check. | ||
* @return bool | ||
* @since x.x.x | ||
*/ | ||
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 ); | ||
} | ||
Comment on lines
+146
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So two confusing things about this function:
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? |
||
|
||
/** | ||
* Get whether the country is supported by the Merchant Center. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -142,7 +142,9 @@ protected function handle_update_shipping_settings() { | |
$this->job_repository->get( ShippingNotificationJob::class )->schedule( [ 'topic' => NotificationsService::TOPIC_SHIPPING_UPDATED ] ); | ||
} | ||
|
||
$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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the changes here seem pretty straightforward with simple logic:
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):
Now because 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. |
||
|
||
$this->already_scheduled = true; | ||
} | ||
|
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.
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:
woocommerce_gla_batch_retry_update_products
will also be happily triggered again without being blocked if the previous sync failed.