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

Scheduled task related performance related improvement and minor changes #155

Closed

Conversation

weilai-irl
Copy link

@weilai-irl weilai-irl commented May 28, 2024

Hi Catalyst team,

We recent made some changes to the plugin with the main aim to improve the performance of the scheduled task.

The performance issue
The performance issue was raised from a Moodle site that we maintain, which have about 30 mod_reengagement activities in different courses, many of which are legacy courses that have been hidden, or moved to hidden course categories. Each run of the scheduled task now takes over 15 minutes to run, and the logic to mark completion and send emails are only reached after the logic to create in-progress records, at the end of the scheduled task. On some courses, mod_reengagement activities are used to allow users to access subsequent activities following a delay after another activity is completed, and the duration is set to be relatively short period, e.g. 30 minutes or 1 hour. Long scheduled task running time effectively adds a lot of extra time to the configured duration, resulting in confusion to users.

The solution
We have made two changes to improve this:

  1. The current logic in the scheduled task to (1) mark completion and (2) send emails have been moved to two ad-hoc tasks. The ad-hoc tasks are created with nextruntime of the expected completion time / email time, so will be triggered by the Moodle cronjob separate from the scheduled task.
  2. Two settings have been added to the plugin to allow site admins to control whether to process mod_reengagement activities in hidden courses, and whether (parent) category visibility needs to be considered. In our case, this alone hugely reduces the scheduled task running time.

Bug fix in completion time / email time display
This is related to issue #151. When a student access the plugin view.php page, the completion time / email time displayed is fetched by finding a random in progress record of the user, without limiting the in-progress record to the current mod_reengagement activity only. The pull request contains the fix to this issue.

Other changes
The PR also contains a small change to add an option to reset data belonging to the mod_reengagement plugin when the course is reset. I noticed that the reset function was actually implemented, but the option didn't exist, so the function would never be triggered.

Please review the changes, and let me know if you have any questions.

Regards,
Lai

…sks, add control over mod_reengagement instances to process according to course visibility, and add support to course reset
@danmarsden
Copy link
Member

@weilai-irl I note your patch here reverts: d358026 but there's also a decent chunk to review here so it might sit for a while until we get a chance to review it properly!

thanks for the PR!

@weilai-irl
Copy link
Author

Hi @danmarsden

I have made changes to add back the missing commit and and address the issues found in the checks. Please feel free to run the checks again to see if all pass.

Regards,
Lai

@weilai-irl
Copy link
Author

@danmarsden

May I check if there has been any progress in reviewing these changes? Is there anything that I can do to moving things forward?

Regards,
Lai

@danmarsden
Copy link
Member

Thansk @weilai-irl, we've been rewriting this a bit as part of #174 and #164 but hopefully that will land soon - closing this one off for now in favour for that work but once that lands feel free to let us know if you think anything else is missed.

@danmarsden danmarsden closed this Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants