-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix membership auto-enroll webhooks #329
Conversation
@brianhogg thank you for this code we will run some tests. Could you rebase this PR to dev branch? |
…tion() already de-duplicates entries so is_already_processed() appears to be unneeded.
545c004
to
0ca4aed
Compare
@kimcoleman done! :) |
hey @brianhogg Thanks for the code and the tests. Looks good. 👌 |
…ng duplicates in the same request
@actuallyakash I've removed the It was a bit of luck when I made |
Hey @brianhogg
Cool, thanks. Just need to update some doc blocks.
oh, nice :D Just putting here for discussion, the However, the issue was due to the From the current PR, we now know that |
Docblocks updated, let me know if there's anything else or the updates aren't clear :)
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. |
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
Types of changes
Bug fix
Checklist: