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

delayed engagements may still fire after engagement has been deleted #290

Open
thomasplevy opened this issue Jan 30, 2017 · 11 comments · May be fixed by #1884
Open

delayed engagements may still fire after engagement has been deleted #290

thomasplevy opened this issue Jan 30, 2017 · 11 comments · May be fixed by #1884

Comments

@thomasplevy
Copy link
Member

@thomasplevy thomasplevy commented Jan 30, 2017

Recreation steps:

  1. Create an engagement (eg email) with a delay of 1 or more days -- try lesson completion
  2. Trigger engagement via a student (eg student completes the lesson)
  3. Delete the engagement
  4. Email will still send

Fix:

When the engagement post is deleted the scheduled actions should be removed

@toyinogun
Copy link

@toyinogun toyinogun commented Aug 30, 2021

@toyinogun
Copy link

@toyinogun toyinogun commented Aug 30, 2021

@gocodebox/engineering is there a way to temporarily stop this or we should rather wait for a fix?

@thomasplevy
Copy link
Member Author

@thomasplevy thomasplevy commented Aug 30, 2021

@toyinogunseinde I don't have a hidden solution in my pocket. This is a bug that we haven't addressed yet. Sorry.

@thomasplevy thomasplevy added this to Awaiting Triage in Triage via automation Aug 30, 2021
@thomasplevy thomasplevy removed this from Awaiting Triage in Triage Aug 30, 2021
@thomasplevy thomasplevy added this to To do in Active via automation Aug 30, 2021
@thomasplevy thomasplevy moved this from To do to In progress in Active Aug 31, 2021
@thomasplevy thomasplevy self-assigned this Aug 31, 2021
@thomasplevy thomasplevy moved this from In progress to To do in Active Aug 31, 2021
@thomasplevy thomasplevy removed their assignment Aug 31, 2021
@thomasplevy thomasplevy moved this from To do to In progress in Active Sep 1, 2021
@thomasplevy thomasplevy self-assigned this Sep 1, 2021
@thomasplevy thomasplevy moved this from In progress to To do in Active Sep 1, 2021
@toyinogun
Copy link

@toyinogun toyinogun commented Sep 2, 2021

@hubert0504
Copy link

@hubert0504 hubert0504 commented Sep 9, 2021

Hi, guys, any idea when this will be sorted. I got one of our clients affected by this bug. This seems to be a major oversight by the Lifter LMS.

@thomasplevy
Copy link
Member Author

@thomasplevy thomasplevy commented Sep 9, 2021

@hubert0504 it's in progress. Unfortunately it's a complicated oversight to walk back from. Progress updates will be relaid here or on related pull requests

@hubert0504
Copy link

@hubert0504 hubert0504 commented Oct 15, 2021

Hi @thomasplevy, our client is becoming frustrated with this issue. Is it possible to redirect those specific emails (maybe using id or subject line) to another dummy email address using hooks or filters?

@thomasplevy
Copy link
Member Author

@thomasplevy thomasplevy commented Oct 15, 2021

Hey @hubert0504

I appreciate you and your client's frustrations and I do apologize that this bug continues to exist.

The creative thinking here would be useful for a workaround except that it suffers from the same issue as the core issue suffers from: the triggered event (to send an email, in this case) is not aware of the engagement post type that was used to create it (which is now deleted).

The solution is for the scheduled action to become aware of the engagement post type id (it should be passed as an additional argument to the scheduled action) so that when the engagement is deleted scheduled actions created from that post can be unscheduled...

We've run into a few compounding issues while working on this... You can see this halted PR: #1771

I started working on adding a way to pass the additional arguments and then, after quite a bit of refactoring, realized that it's impossible (not entirely, but difficult) to cancel scheduled actions based on one value passed to the arguments. You can cancel based on a combination of all of them but not a single one.

I've been sitting on this problem hoping that someone (or my subconcious) will present a better solution. I've taken another stab at it this morning prompted by your reminder that this is still ongoing.

I've come up with a solution but an issue will still remain that existing actions that are scheduled at this moment (or at the time of a bugfix) will still not have the required information available to them.

I hope to have what I'm working on completed and ready for code review next week which, assuming there's no issues I'm missing, should have a release next week. But, you're still going to need to either accept that existing actions will still send or you'll have to clean them up manually.

I've tried to come up with a way to fix these without the information we need (the post ID) but there's a potential to unschedule events that shouldn't be unscheduled as a result.

The scheduled actions have access to the user id, the related post id (say the course or quiz that was completed to trigger the engagement) and the id of the email that's supposed to send. We could use the related post id and the email's id to lookup the engagement but there's two problems here:

  1. The IDs could also be used on a separate engagement which should not be deleted. It's conceivable that the same trigger could be used to send multiple emails. Maybe the same reminder gets sent every week for 3 weeks after a course is completed (or something...). We could try to use the delay to work backwards but the delay is not available to the action. There's no record kept by the WP scheduled action system of when the action was scheduled, just when it's supposed to trigger.

  2. Assuming we decide the likelihood of false-positives from the above scenario, we couldn't even really do it because if the engagement has already been deleted we won't be able to use the information we have to look up an engagement. We could look for any action that doesn't have an engagement with the specified post id and email id has been deleted, but it's conceivable that they weren't deleted and they were just changed... Maybe the site owner changed the related email and they want a new one to go out moving forward but they did not desire for existing ones to be deleted...

Both the above scenarios worry about some weird niche side effects and we could probably just cancel anything and move on without anyone complaining... but... if we don't consider these things it's going to be worse for everyone involved to create chaos while trying to fix something.... So it's better to be safe, I think, and not try to create a migration based on incomplete information.

So I think the best thing for you to do would be to manually audit your existing scheduled actions and cancel those that should be cancelled. You'll have to review them yourself and determine i[f they should be deleted or not. I know that's time consuming but it's the best I can recommend. I'm willing to be you won't be affected by either the above two issues so you can probably easily cancel all actions for post id and email id combination of the engagement(s) you've already deleted.

If you're not aware, you can use this plugin to find (and cancel) scheduled actions: https://wordpress.org/plugins/wp-crontrol/

You'll want to look for the following hooks: lifterlms_engagement_send_email, lifterlms_engagement_award_achievement, and lifterlms_engagement_send_email

@hubert0504
Copy link

@hubert0504 hubert0504 commented Oct 17, 2021

Hey @hubert0504

I appreciate you and your client's frustrations and I do apologize that this bug continues to exist.

The creative thinking here would be useful for a workaround except that it suffers from the same issue as the core issue suffers from: the triggered event (to send an email, in this case) is not aware of the engagement post type that was used to create it (which is now deleted).

The solution is for the scheduled action to become aware of the engagement post type id (it should be passed as an additional argument to the scheduled action) so that when the engagement is deleted scheduled actions created from that post can be unscheduled...

We've run into a few compounding issues while working on this... You can see this halted PR: #1771

I started working on adding a way to pass the additional arguments and then, after quite a bit of refactoring, realized that it's impossible (not entirely, but difficult) to cancel scheduled actions based on one value passed to the arguments. You can cancel based on a combination of all of them but not a single one.

I've been sitting on this problem hoping that someone (or my subconcious) will present a better solution. I've taken another stab at it this morning prompted by your reminder that this is still ongoing.

I've come up with a solution but an issue will still remain that existing actions that are scheduled at this moment (or at the time of a bugfix) will still not have the required information available to them.

I hope to have what I'm working on completed and ready for code review next week which, assuming there's no issues I'm missing, should have a release next week. But, you're still going to need to either accept that existing actions will still send or you'll have to clean them up manually.

I've tried to come up with a way to fix these without the information we need (the post ID) but there's a potential to unschedule events that shouldn't be unscheduled as a result.

The scheduled actions have access to the user id, the related post id (say the course or quiz that was completed to trigger the engagement) and the id of the email that's supposed to send. We could use the related post id and the email's id to lookup the engagement but there's two problems here:

  1. The IDs could also be used on a separate engagement which should not be deleted. It's conceivable that the same trigger could be used to send multiple emails. Maybe the same reminder gets sent every week for 3 weeks after a course is completed (or something...). We could try to use the delay to work backwards but the delay is not available to the action. There's no record kept by the WP scheduled action system of when the action was scheduled, just when it's supposed to trigger.
  2. Assuming we decide the likelihood of false-positives from the above scenario, we couldn't even really do it because if the engagement has already been deleted we won't be able to use the information we have to look up an engagement. We could look for any action that doesn't have an engagement with the specified post id and email id has been deleted, but it's conceivable that they weren't deleted and they were just changed... Maybe the site owner changed the related email and they want a new one to go out moving forward but they did not desire for existing ones to be deleted...

Both the above scenarios worry about some weird niche side effects and we could probably just cancel anything and move on without anyone complaining... but... if we don't consider these things it's going to be worse for everyone involved to create chaos while trying to fix something.... So it's better to be safe, I think, and not try to create a migration based on incomplete information.

So I think the best thing for you to do would be to manually audit your existing scheduled actions and cancel those that should be cancelled. You'll have to review them yourself and determine i[f they should be deleted or not. I know that's time consuming but it's the best I can recommend. I'm willing to be you won't be affected by either the above two issues so you can probably easily cancel all actions for post id and email id combination of the engagement(s) you've already deleted.

If you're not aware, you can use this plugin to find (and cancel) scheduled actions: https://wordpress.org/plugins/wp-crontrol/

You'll want to look for the following hooks: lifterlms_engagement_send_email, lifterlms_engagement_award_achievement, and lifterlms_engagement_send_email

Hi @thomasplevy , thanks for the detailed information. I really appreciate it. I will try to audit the scheduled actions and try to remove them manually. Thanks again mate.

@thomasplevy thomasplevy moved this from To do to Ready for Review in Active Nov 18, 2021
@thomasplevy thomasplevy moved this from Ready for Review to In progress in Active Nov 18, 2021
@thomasplevy thomasplevy moved this from In progress to Reviewer approved in Active Nov 22, 2021
@kelliott14900
Copy link

@kelliott14900 kelliott14900 commented Dec 6, 2021

@thomasplevy Do you have an update on when this fix will be rolled out? We've got a pretty big issue with this bug on one of our client sites as well thats causing tons of emails to go out each day which are inaccurate but we've got no way of stopping them as the engagements were deleted. Thank you so much for all of your efforts to resolve this. They are much appreciated.

@thomasplevy
Copy link
Member Author

@thomasplevy thomasplevy commented Dec 6, 2021

@kelliott14900 This issue will be resolved in LifterLMS 6.0, slated for alpha release within the next two weeks. Various PRs are currently open, the "hub" pr (which will be shifted to the development branch for version 6.0, is in PR #1771

This issue is fixed on that working branch but it's not ready for production usage as of yet. We're completing adjacent features and continuing to test our fixes and improvements before it's released.

If you have any other questions let us know

@thomasplevy thomasplevy linked a pull request that will close this issue Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Active
  
Reviewer approved
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants