Skip to content

Commit

Permalink
Merge pull request #329 from brianhogg/2568-fix-membership-auto-enrol…
Browse files Browse the repository at this point in the history 
…l-webhooks

Fix membership auto-enroll webhooks
  • Loading branch information
@ideadude
ideadude authored Jan 17, 2024
2 parents d790ce0 + acb1ec5 commit ba4728d
Show file tree
Hide file tree
Showing 3 changed files with 95 additions and 55 deletions.
5 changes: 5 additions & 0 deletions .changelogs/2568-fix-membership-auto-enroll-webhooks.yml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
significance: patch
type: fixed
links:
- "#2568"
entry: Update the processed flag to use all arguments.
30 changes: 3 additions & 27 deletions includes/models/class-llms-rest-webhook.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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
*
Expand Down Expand Up @@ -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.
Expand All @@ -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
*
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
115 changes: 87 additions & 28 deletions tests/unit-tests/class-llms-rest-test-webhook.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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 ) ) ) );

}

}

0 comments on commit ba4728d

Please sign in to comment.