Fix membership auto-enroll webhooks

[brianhogg]

Description

Currently there is a check to avoid the same webhook firing multiple times, but it's only checking the first argument or ID. For webhooks like the one that fires for the enrollment.created topic, there's two arguments: the student ID and the product ID (course or membership). Because of this, for an enrollment in a membership with one or more auto enrolled courses, only the webhook for the first course fires.

This PR changes the logic to look at the entire argument array when determining if there is a duplicate webhook. However from testing, it appears that the Action Scheduler will not allow a duplicate entry to be added anyway, so the check can likely be removed entirely.

Fixes https://github.com/gocodebox/lifterlms/issues/2568

How has this been tested?

Automated tests, and manually with the instructions on the ticket.

Screenshots

Types of changes

Bug fix

Checklist:

• [x] This PR requires and contains at least one changelog file. <!-- To create a changelog yml file: npm run dev changelog add -- -i and follow the prompt. See also: https://github.com/gocodebox/lifterlms/blob/trunk/packages/dev/README.md#changelog-add -->
• [x] My code has been tested.
• [x] My code passes all existing automated tests.
• [x] My code follows the LifterLMS Coding & Documentation Standards.

[kimcoleman]

@brianhogg thank you for this code we will run some tests.

Could you rebase this PR to dev branch?

[brianhogg]

@brianhogg thank you for this code we will run some tests.

Could you rebase this PR to dev branch?

@kimcoleman done! :)

[actuallyakash]

hey @brianhogg

Thanks for the code and the tests. Looks good. 👌 Gonna close my PR in favour of this PR.

[brianhogg]

@actuallyakash I've removed the is_already_processed check and an associated test that was only checking if that check was happening in should_deliver.

It was a bit of luck when I made is_already_processed return false to verify the one test would pass, then noticed the new test_multiple_hooks_for_single_webhook_only_schedules_once test was also still passing :)

[actuallyakash]

Hey @brianhogg

@actuallyakash I've removed the is_already_processed check and an associated test that was only checking if that check was happening in should_deliver.

Cool, thanks. Just need to update some doc blocks.

It was a bit of luck when I made is_already_processed return false to verify the one test would pass, then noticed the new test_multiple_hooks_for_single_webhook_only_schedules_once test was also still passing :)

oh, nice :D

Just putting here for discussion, the is_already_processed() check was introduced due to this issue: https://github.com/gocodebox/lifterlms-rest/issues/210

However, the issue was due to the pending_delivery check which stopped the new webhooks from being delivered if the old ones were stuck. The pending_delivery check was removed and is_already_processed was introduced.

From the current PR, we now know that is_already_processed is not useful as "Action Scheduler does not seem to allow scheduling two actions with the same arguments and the same webhook ID".

[brianhogg]

Cool, thanks. Just need to update some doc blocks.

Docblocks updated, let me know if there's anything else or the updates aren't clear :)

Just putting here for discussion, the is_already_processed() check was introduced due to this issue: #210

However, the issue was due to the pending_delivery check which stopped the new webhooks from being delivered if the old ones were stuck. The pending_delivery check was removed and is_already_processed was introduced.

From the current PR, we now know that is_already_processed is not useful as "Action Scheduler does not seem to allow scheduling two actions with the same arguments and the same webhook ID".

Thanks for the background! It doesn't look like this change will cause a regression. There's also now a test to ensure multiple actions can be scheduled for the same webhook in the same request, but with different arguments.