From 75f2d4a20eac3dc0fdb22663f3ce1640ac4f1945 Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Wed, 15 Nov 2023 08:04:33 -0500 Subject: [PATCH 1/8] Failing test that each course webhook, along with membership webhook, are enqueued --- .../class-llms-rest-test-webhook.php | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/tests/unit-tests/class-llms-rest-test-webhook.php b/tests/unit-tests/class-llms-rest-test-webhook.php index 339e2348..df653fda 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -761,6 +761,69 @@ public function test_scheduling_enrollment_created() { } + /** + * Test scheduling enrollment.created via a membership enrollment with multiple auto enroll courses + * + * @since Unknown + * + * @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. * From fbd873be7af176b3450e180ec3ad56357d871f0e Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Wed, 15 Nov 2023 08:05:22 -0500 Subject: [PATCH 2/8] Test that only one webhook is queued to send for a topic that has multiple hooks --- .../unit-tests/class-llms-rest-test-webhook.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/unit-tests/class-llms-rest-test-webhook.php b/tests/unit-tests/class-llms-rest-test-webhook.php index df653fda..d05fde40 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -761,6 +761,23 @@ public function test_scheduling_enrollment_created() { } + 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 * From 105a2dcbb87582bd8b55c124c8684d9873f10e7b Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Wed, 15 Nov 2023 09:48:51 -0500 Subject: [PATCH 3/8] Formatting, getting tests to pass. It looks like as_next_scheduled_action() already de-duplicates entries so is_already_processed() appears to be unneeded. --- includes/models/class-llms-rest-webhook.php | 4 ++-- tests/unit-tests/class-llms-rest-test-webhook.php | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/includes/models/class-llms-rest-webhook.php b/includes/models/class-llms-rest-webhook.php index 08d8a9fc..4f4ed53b 100644 --- a/includes/models/class-llms-rest-webhook.php +++ b/includes/models/class-llms-rest-webhook.php @@ -196,7 +196,7 @@ public function enqueue() { * @return bool */ protected function is_already_processed( $args ) { - return false !== array_search( $args[0], $this->processed, true ); + return false !== array_search( $args, $this->processed, true ); } /** @@ -361,7 +361,7 @@ public function process_hook( ...$args ) { // 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]; + $this->processed[] = $args; /** * Disable background processing of webhooks by returning a falsy diff --git a/tests/unit-tests/class-llms-rest-test-webhook.php b/tests/unit-tests/class-llms-rest-test-webhook.php index d05fde40..33b5ede7 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -775,7 +775,7 @@ public function test_multiple_hooks_for_single_webhook_only_schedules_once() { wp_trash_post($membership_id); wp_delete_post($membership_id); - $this->assertCount(1, as_get_scheduled_actions( array( 'hook' => 'lifterlms_rest_deliver_webhook_async'))); + $this->assertCount(1, as_get_scheduled_actions( array( 'hook' => 'lifterlms_rest_deliver_webhook_async' ) ) ); } /** From 01efe63c6d1e9c2efb57811533430f6d0fd3eb61 Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Wed, 15 Nov 2023 16:35:54 -0500 Subject: [PATCH 4/8] Additional formatting to match other tests --- tests/unit-tests/class-llms-rest-test-webhook.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/unit-tests/class-llms-rest-test-webhook.php b/tests/unit-tests/class-llms-rest-test-webhook.php index 33b5ede7..a5fc9f81 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -762,6 +762,7 @@ public function test_scheduling_enrollment_created() { } 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', @@ -795,9 +796,9 @@ public function test_scheduling_enrollment_created_through_membership() { $webhook->enqueue(); - $student_id = $this->factory->student->create(); - $membership_id = $this->factory->membership->create(); - $course_id = $this->factory->course->create( array( 'sections' => 0 ) ); + $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 ); @@ -807,12 +808,10 @@ public function test_scheduling_enrollment_created_through_membership() { '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 ), From 0266fab1fe533f7003fbe0ef0e51102ff750bca9 Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Wed, 15 Nov 2023 16:53:13 -0500 Subject: [PATCH 5/8] Adding changelog --- .changelogs/2568-fix-membership-auto-enroll-webhooks.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changelogs/2568-fix-membership-auto-enroll-webhooks.yml 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. From 0ca4aedb5c61c26d68245801b101e26f3e00c415 Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Wed, 15 Nov 2023 17:03:46 -0500 Subject: [PATCH 6/8] Additional docs --- includes/models/class-llms-rest-webhook.php | 3 +++ tests/unit-tests/class-llms-rest-test-webhook.php | 12 ++++++++++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/includes/models/class-llms-rest-webhook.php b/includes/models/class-llms-rest-webhook.php index 4f4ed53b..e91ad274 100644 --- a/includes/models/class-llms-rest-webhook.php +++ b/includes/models/class-llms-rest-webhook.php @@ -192,6 +192,8 @@ public function enqueue() { * * Helps avoid duplication of data being sent for topics that have more than one hook defined. * + * @since [version] Verify the processed flag with all arguments of `$args`. + * * @param array $args Numeric array of arguments from the originating hook. * @return bool */ @@ -348,6 +350,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] Updated the processed flag to use all arguments of `$args`. * * @param mixed ...$args Arguments from the hook. * @return int|false Timestamp of the scheduled event when the webhook is successfully scheduled. diff --git a/tests/unit-tests/class-llms-rest-test-webhook.php b/tests/unit-tests/class-llms-rest-test-webhook.php index a5fc9f81..5131807f 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -761,6 +761,14 @@ 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( @@ -780,9 +788,9 @@ public function test_multiple_hooks_for_single_webhook_only_schedules_once() { } /** - * Test scheduling enrollment.created via a membership enrollment with multiple auto enroll courses + * Test scheduling enrollment.created via a membership enrollment with multiple auto enroll courses. * - * @since Unknown + * @since [version] * * @return void */ From d2567d06f0ce19e54125abe97ebc237f1f5a2491 Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Fri, 17 Nov 2023 06:01:40 -0500 Subject: [PATCH 7/8] Removing processed check since ActionScheduler already handles removing duplicates in the same request --- includes/models/class-llms-rest-webhook.php | 30 +------------------ .../class-llms-rest-test-webhook.php | 28 ----------------- 2 files changed, 1 insertion(+), 57 deletions(-) diff --git a/includes/models/class-llms-rest-webhook.php b/includes/models/class-llms-rest-webhook.php index e91ad274..a81cefb5 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,20 +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. - * - * @since [version] Verify the processed flag with all arguments of `$args`. - * - * @param array $args Numeric array of arguments from the originating hook. - * @return bool - */ - protected function is_already_processed( $args ) { - return false !== array_search( $args, $this->processed, true ); - } - /** * Determine if the current action is valid for the webhook * @@ -362,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; - /** * Disable background processing of webhooks by returning a falsy * @@ -425,7 +399,6 @@ 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. * * @param array $args Numeric array of arguments from the originating hook. * @return bool @@ -434,8 +407,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 5131807f..1e94c21f 100644 --- a/tests/unit-tests/class-llms-rest-test-webhook.php +++ b/tests/unit-tests/class-llms-rest-test-webhook.php @@ -966,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 ) ) ) ); - - } - } From acb1ec53b6f925dc3bcb4ec5417dd21830b2f56b Mon Sep 17 00:00:00 2001 From: Brian Hogg Date: Tue, 21 Nov 2023 08:39:52 -0500 Subject: [PATCH 8/8] Additional docblock changes --- includes/models/class-llms-rest-webhook.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/includes/models/class-llms-rest-webhook.php b/includes/models/class-llms-rest-webhook.php index a81cefb5..2ee8b916 100644 --- a/includes/models/class-llms-rest-webhook.php +++ b/includes/models/class-llms-rest-webhook.php @@ -328,7 +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] Updated the processed flag to use all arguments of `$args`. + * @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. @@ -399,6 +399,7 @@ public function ping() { * Determines if an originating action qualifies for webhook delivery * * @since 1.0.0-beta.1 + * @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