Skip to content
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

Fix membership auto-enroll webhooks #329

Merged

Conversation

brianhogg
Copy link
Contributor

@brianhogg brianhogg commented Nov 15, 2023

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 gocodebox/lifterlms#2568

How has this been tested?

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

Screenshots

Captura de pantalla 2023-11-15 a las 4 34 07 p  m Captura de pantalla 2023-11-15 a las 4 34 14 p  m Captura de pantalla 2023-11-15 a las 4 34 21 p  m Captura de pantalla 2023-11-15 a las 4 34 28 p  m

Types of changes

Bug fix

Checklist:

  • This PR requires and contains at least one changelog file.
  • My code has been tested.
  • My code passes all existing automated tests.
  • My code follows the LifterLMS Coding & Documentation Standards.

Sorry, something went wrong.

@kimcoleman
Copy link
Member

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

Could you rebase this PR to dev branch?

@brianhogg brianhogg force-pushed the 2568-fix-membership-auto-enroll-webhooks branch from 545c004 to 0ca4aed Compare November 16, 2023 16:35
@brianhogg brianhogg changed the base branch from trunk to dev November 16, 2023 16:36
@brianhogg
Copy link
Contributor Author

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

Could you rebase this PR to dev branch?

@kimcoleman done! :)

@actuallyakash
Copy link
Collaborator

hey @brianhogg

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

@brianhogg
Copy link
Contributor Author

@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
Copy link
Collaborator

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: #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
Copy link
Contributor Author

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.

@ideadude ideadude merged commit ba4728d into gocodebox:dev Jan 17, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
4 participants