-
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
Changes from all commits
ce8b86f
d8cebdb
832d0dc
94c4fb6
0386c74
51b729f
619123f
d85d7ed
a60ac81
c47b7fa
b716805
d4c2d37
545cd22
8ee54d1
fd11304
3d9b4bc
ba7ed8f
b3b0735
8940067
a1eaad6
a1d5f22
d3a641f
b9af9f1
82795a7
99f5b9e
08efcce
d93c7b7
cbb00a2
677e4ec
6315209
de3e24c
65dff81
ac3f6cc
75e4c96
3056459
2bb00ca
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 |
---|---|---|
@@ -0,0 +1,133 @@ | ||
<?php | ||
declare(strict_types=1); | ||
|
||
namespace Automattic\WooCommerce\GoogleListingsAndAds\Jobs\Notifications; | ||
|
||
use Automattic\WooCommerce\GoogleListingsAndAds\ActionScheduler\ActionSchedulerInterface; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Google\NotificationsService; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Jobs\AbstractActionSchedulerJob; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Jobs\ActionSchedulerJobMonitor; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Jobs\JobInterface; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductHelper; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Value\NotificationStatus; | ||
|
||
|
||
defined( 'ABSPATH' ) || exit; | ||
|
||
/** | ||
* Class ProductNotificationJob | ||
* Class for all the Product Notifications Jobs | ||
* | ||
* @since x.x.x | ||
* @package Automattic\WooCommerce\GoogleListingsAndAds\Jobs\Notifications | ||
*/ | ||
class ProductNotificationJob extends AbstractActionSchedulerJob implements JobInterface { | ||
|
||
/** | ||
* @var NotificationsService $notifications_service | ||
*/ | ||
protected $notifications_service; | ||
|
||
/** | ||
* @var ProductHelper $product_helper | ||
*/ | ||
protected $product_helper; | ||
|
||
/** | ||
* Notifications Jobs constructor. | ||
* | ||
* @param ActionSchedulerInterface $action_scheduler | ||
* @param ActionSchedulerJobMonitor $monitor | ||
* @param NotificationsService $notifications_service | ||
* @param ProductHelper $product_helper | ||
*/ | ||
public function __construct( | ||
ActionSchedulerInterface $action_scheduler, | ||
ActionSchedulerJobMonitor $monitor, | ||
NotificationsService $notifications_service, | ||
ProductHelper $product_helper | ||
) { | ||
$this->notifications_service = $notifications_service; | ||
$this->product_helper = $product_helper; | ||
parent::__construct( $action_scheduler, $monitor ); | ||
} | ||
|
||
/** | ||
* Get the job name | ||
* | ||
* @return string | ||
*/ | ||
public function get_name(): string { | ||
return 'notifications/products'; | ||
} | ||
|
||
|
||
/** | ||
* Logic when processing the items | ||
* | ||
* @param array $args Arguments with the item id and the topic | ||
*/ | ||
protected function process_items( array $args ): void { | ||
if ( ! isset( $args[0] ) || ! isset( $args[1] ) ) { | ||
return; | ||
} | ||
|
||
$item = $args[0]; | ||
$topic = $args[1]; | ||
|
||
if ( $this->notifications_service->notify( $item, $topic ) ) { | ||
$this->set_status( $item, $this->get_after_notification_status( $topic ) ); | ||
} | ||
} | ||
|
||
/** | ||
* Schedule the Product Notification Job | ||
* | ||
* @param array $args | ||
*/ | ||
public function schedule( array $args = [] ): void { | ||
/** | ||
* Allow users to disable the notification job schedule. | ||
* | ||
* @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 with the item id and the topic. | ||
*/ | ||
$can_schedule = apply_filters( 'woocommerce_gla_product_notification_job_can_schedule', $this->can_schedule( [ $args ] ), $args ); | ||
|
||
if ( $can_schedule ) { | ||
$this->action_scheduler->schedule_immediate( | ||
$this->get_process_item_hook(), | ||
[ $args ] | ||
); | ||
} | ||
} | ||
|
||
/** | ||
* Set the notification status for a product. | ||
* | ||
* @param int $product_id | ||
* @param string $status | ||
*/ | ||
protected function set_status( int $product_id, string $status ): void { | ||
$product = $this->product_helper->get_wc_product( $product_id ); | ||
$this->product_helper->set_notification_status( $product, $status ); | ||
} | ||
|
||
/** | ||
* Get the Notification Status after the notification happens | ||
* | ||
* @param string $topic | ||
* @return string | ||
*/ | ||
protected function get_after_notification_status( string $topic ): string { | ||
if ( str_contains( $topic, '.create' ) ) { | ||
return NotificationStatus::NOTIFICATION_CREATED; | ||
} elseif ( str_contains( $topic, '.delete' ) ) { | ||
return NotificationStatus::NOTIFICATION_DELETED; | ||
} else { | ||
return NotificationStatus::NOTIFICATION_UPDATED; | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
use Automattic\WooCommerce\GoogleListingsAndAds\PluginHelper; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Proxies\WC; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Value\ChannelVisibility; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Value\NotificationStatus; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Value\SyncStatus; | ||
use Automattic\WooCommerce\GoogleListingsAndAds\Vendor\Google\Service\ShoppingContent\Product as GoogleProduct; | ||
use WC_Product; | ||
|
@@ -332,6 +333,95 @@ public function is_product_synced( WC_Product $product ): bool { | |
return ! empty( $synced_at ) && ! empty( $google_ids ); | ||
} | ||
|
||
/** | ||
* Indicates if a product is ready for sending Notifications. | ||
* A product is ready to send notifications if DONT_SYNC_AND_SHOW is not enabled and the post status is publish. | ||
* | ||
* @param WC_Product $product | ||
* | ||
* @return bool | ||
*/ | ||
public function is_ready_to_notify( WC_Product $product ): bool { | ||
$is_ready = ChannelVisibility::DONT_SYNC_AND_SHOW !== $this->get_channel_visibility( $product ) && | ||
$product->get_status() === 'publish' && | ||
in_array( $product->get_type(), ProductSyncer::get_supported_product_types(), true ); | ||
|
||
/** | ||
* Allow users to filter if a product is ready to notify. | ||
* | ||
* @since x.x.x | ||
* | ||
* @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 ); | ||
} | ||
|
||
/** | ||
* Indicates if a product is ready for sending a create Notification. | ||
* A product is ready to send create notifications if is ready to notify and has not sent create notification yet. | ||
* | ||
* @param WC_Product $product | ||
* | ||
* @return bool | ||
*/ | ||
public function should_trigger_create_notification( WC_Product $product ): bool { | ||
return $this->is_ready_to_notify( $product ) && ! $this->has_notified_creation( $product ); | ||
} | ||
|
||
/** | ||
* Indicates if a product is ready for sending an update Notification. | ||
* A product is ready to send update notifications if is ready to notify and has sent create notification already. | ||
* | ||
* @param WC_Product $product | ||
* | ||
* @return bool | ||
*/ | ||
public function should_trigger_update_notification( WC_Product $product ): bool { | ||
return $this->is_ready_to_notify( $product ) && $this->has_notified_creation( $product ); | ||
} | ||
|
||
/** | ||
* Indicates if a product is ready for sending a delete Notification. | ||
* A product is ready to send delete notifications if it is not ready to notify and has sent create notification already. | ||
* | ||
* @param WC_Product $product | ||
* | ||
* @return bool | ||
*/ | ||
public function should_trigger_delete_notification( WC_Product $product ): bool { | ||
return ! $this->is_ready_to_notify( $product ) && $this->has_notified_creation( $product ); | ||
} | ||
|
||
/** | ||
* Indicates if a product was already notified about its creation. | ||
* Notice we consider synced products in MC as notified for creation. | ||
* | ||
* @param WC_Product $product | ||
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. Can we add a PHPDoc explaining this function? 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. 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 commentThe reason will be displayed to describe this comment to others. Learn more. I got a bit confused with all the statuses but what I understand is that these statuses indicate whether the product has been created/synced, helping us, for example, to avoid sending an update/delete topic if the product has not been notified yet. Also, I believe we need to clarify that the creation/delete topics are related to the MC status. Sometimes, I found myself mixing the concepts of creation/delete with creating/deleting products in WC, whereas in the MC case, it could be just changing the channel visibility from "syncable" to "not syncable." or vice-versa. Moreover, I'm trying to understand why we need to track this status. For instance:
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.
Correct. For example. Imagine this scenario:
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.
I assume we don't take in consideration the previous statuses of (PUSH based) MC for sending the Notifications 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. I will move that question to the P2 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.
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. |
||
$valid_has_notified_creation_statuses = [ | ||
NotificationStatus::NOTIFICATION_PENDING_CREATE, | ||
NotificationStatus::NOTIFICATION_CREATED, | ||
NotificationStatus::NOTIFICATION_UPDATED, | ||
NotificationStatus::NOTIFICATION_PENDING_UPDATE | ||
]; | ||
|
||
return in_array( $this->meta_handler->get_notification_status( $product ), $valid_has_notified_creation_statuses, true ) || $this->is_product_synced( $product ); | ||
} | ||
|
||
/** | ||
* Set the notification status for a WooCommerce product. | ||
* | ||
* @param WC_Product $product | ||
* @param string $status | ||
*/ | ||
public function set_notification_status( WC_Product $product, $status ): void { | ||
$this->meta_handler->update_notification_status( $product, $status ); | ||
} | ||
|
||
/** | ||
* @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.
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.
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?