diff --git a/.changelogs/2568-fix-membership-auto-enroll-webhooks.yml b/.changelogs/2568-fix-membership-auto-enroll-webhooks.yml new file mode 100644 index 00000000..d7e77759 --- /dev/null +++ b/.changelogs/2568-fix-membership-auto-enroll-webhooks.yml @@ -0,0 +1,5 @@ +significance: patch +type: fixed +links: + - "#2568" +entry: Update the processed flag to use all arguments. diff --git a/includes/models/class-llms-rest-webhook.php b/includes/models/class-llms-rest-webhook.php index 08d8a9fc..2ee8b916 100644 --- a/includes/models/class-llms-rest-webhook.php +++ b/includes/models/class-llms-rest-webhook.php @@ -20,14 +20,6 @@ */ class LLMS_REST_Webhook extends LLMS_REST_Webhook_Data { - /** - * Store which object IDs this webhook has processed (ie scheduled to be delivered) - * within the current page request. - * - * @var array - */ - protected $processed = array(); - /** * Delivers the webhook * @@ -187,18 +179,6 @@ public function enqueue() { } - /** - * Checks if the specified resource has already been queued for delivery within the current request - * - * Helps avoid duplication of data being sent for topics that have more than one hook defined. - * - * @param array $args Numeric array of arguments from the originating hook. - * @return bool - */ - protected function is_already_processed( $args ) { - return false !== array_search( $args[0], $this->processed, true ); - } - /** * Determine if the current action is valid for the webhook * @@ -348,6 +328,7 @@ protected function is_valid_user_action( $user_id ) { * @since 1.0.0-beta.1 * @since 1.0.0-beta.17 Mark this hook's first argument as processed to ensure it doesn't get processed again within the current request. * And don't rely anymore on the webhook's `pending_delivery` property to achieve the same goal. + * @since [version] Remove the processed flag as the ActionScheduler prevents multiple additions of the same hook. * * @param mixed ...$args Arguments from the hook. * @return int|false Timestamp of the scheduled event when the webhook is successfully scheduled. @@ -359,10 +340,6 @@ public function process_hook( ...$args ) { return false; } - // Mark this hook's first argument as processed to ensure it doesn't get processed again within the current request, - // as it might happen with webhooks with multiple hookes defined in `LLMS_REST_Webhooks::get_hooks()`. - $this->processed[] = $args[0]; - /** * Disable background processing of webhooks by returning a falsy * @@ -422,7 +399,7 @@ public function ping() { * Determines if an originating action qualifies for webhook delivery * * @since 1.0.0-beta.1 - * @since [verison] Drop checking whether the webhook is pending in favor of a check on if is already processed within the current request. + * @since [verison] Removed the "is already processed" check since ActionScheduler prevents duplicates. * * @param array $args Numeric array of arguments from the originating hook. * @return bool @@ -431,8 +408,7 @@ protected function should_deliver( $args ) { $deliver = ( 'active' === $this->get( 'status' ) ) // Must be active. && $this->is_valid_action( $args ) // Valid action. - && $this->is_valid_resource( $args ) // Valid resource. - && ! $this->is_already_processed( $args ); // Not already processed. + && $this->is_valid_resource( $args ); // Valid resource. /** * Skip or hijack webhook delivery scheduling diff --git a/tests/unit-tests/class-llms-rest-test-webhook.php b/tests/unit-tests/class-llms-rest-test-webhook.php index 339e2348..1e94c21f 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -761,6 +761,93 @@ public function test_scheduling_enrollment_created() { } + + /** + * Test scheduling a webhook via multiple hooks, ensuring only one is scheduled for delivery. + * + * @since [version] + * + * @return void + */ + public function test_multiple_hooks_for_single_webhook_only_schedules_once() { + + $webhook = LLMS_REST_API()->webhooks()->create( array( + 'delivery_url' => 'https://mock.tld', + 'topic' => 'membership.deleted', + 'status' => 'active', + ) ); + + $webhook->enqueue(); + + $membership_id = $this->factory->membership->create(); + + wp_trash_post($membership_id); + wp_delete_post($membership_id); + + $this->assertCount(1, as_get_scheduled_actions( array( 'hook' => 'lifterlms_rest_deliver_webhook_async' ) ) ); + } + + /** + * Test scheduling enrollment.created via a membership enrollment with multiple auto enroll courses. + * + * @since [version] + * + * @return void + */ + public function test_scheduling_enrollment_created_through_membership() { + + $webhook = LLMS_REST_API()->webhooks()->create( array( + 'delivery_url' => 'https://mock.tld', + 'topic' => 'enrollment.created', + 'status' => 'active', + ) ); + + $webhook->enqueue(); + + $student_id = $this->factory->student->create(); + $membership_id = $this->factory->membership->create(); + $course_id = $this->factory->course->create( array( 'sections' => 0 ) ); + $second_course_id = $this->factory->course->create( array( 'sections' => 0)); + + $membership = new LLMS_Membership( $membership_id ); + $membership->add_auto_enroll_courses( array( $course_id, $second_course_id ), true ); + + $schedule_args_first_course = array( + 'webhook_id' => $webhook->get( 'id' ), + 'args' => array( $student_id, $course_id ), + ); + $schedule_args_second_course = array( + 'webhook_id' => $webhook->get( 'id' ), + 'args' => array( $student_id, $second_course_id ), + ); + $schedule_args_membership = array( + 'webhook_id' => $webhook->get( 'id' ), + 'args' => array( $student_id, $membership_id ), + ); + + llms_enroll_student( $student_id, $membership_id ); + + $this->assertTrue( as_has_scheduled_action( 'lifterlms_rest_deliver_webhook_async', $schedule_args_first_course, 'llms-webhooks' ) ); + $this->assertTrue( as_has_scheduled_action( 'lifterlms_rest_deliver_webhook_async', $schedule_args_second_course, 'llms-webhooks' ) ); + $this->assertTrue( as_has_scheduled_action( 'lifterlms_rest_deliver_webhook_async', $schedule_args_membership, 'llms-webhooks' ) ); + $this->assertCount(1, as_get_scheduled_actions( array( + 'hook' => 'lifterlms_rest_deliver_webhook_async', + 'args' => $schedule_args_first_course, + 'per_page' => -1 + ) ) ); + $this->assertCount(1, as_get_scheduled_actions( array( + 'hook' => 'lifterlms_rest_deliver_webhook_async', + 'args' => $schedule_args_second_course, + 'per_page' => -1 + ) ) ); + $this->assertCount(1, as_get_scheduled_actions( array( + 'hook' => 'lifterlms_rest_deliver_webhook_async', + 'args' => $schedule_args_membership, + 'per_page' => -1 + ) ) ); + } + + /** * Test ping() on unresolveable urls. * @@ -879,32 +966,4 @@ public function test_should_deliver_status() { } - /** - * Test should_deliver() method with already processed hooks(). - * - * @since 1.0.0-beta.17 - * - * @return void - */ - public function test_should_deliver_already_processed() { - - $webhook = LLMS_REST_API()->webhooks()->create( array( - 'delivery_url' => 'https://mock.tld', - 'topic' => 'student.created', - 'status' => 'active', - ) ); - - $student_id = $this->factory->student->create(); - - // Not processed. - $this->assertTrue( LLMS_Unit_Test_Util::call_method( $webhook, 'should_deliver', array( array( $student_id ) ) ) ); - - // Process the hook. - $webhook->process_hook( $student_id ); - - // Processed. - $this->assertFalse( LLMS_Unit_Test_Util::call_method( $webhook, 'should_deliver', array( array( $student_id ) ) ) ); - - } - }