Improvement to the speed of the emailing certificate task (#531)

[mdjnelson]

No description provided.

[mdjnelson]

@mohamedmohamedatia can we not use the task_log table to determine when it was last run instead of introducing another table? It would be a lot nicer and easier to maintain than us implementing our own logic. Then you just need to check the issued times and only get the issues that were done after the last time the task was run.

Other notes -

1. Do not use array(), please use the short syntax [].
2. Align the settings to the left of the file. Make it consistent with the rest of the file.
3. There is no need for language strings to have capitals in all the words. The string Include Certificates in Not Visible Courses should be Include certificates in not visible courses. Also in this case not visible can be changed to hidden.
4. $fastmoduleinfo = get_fast_modinfo($customcert->courseid, $enroluser->id)->instances['customcert'][$customcert->id]; - $enroluser is not declared.
5. get_context_instance() is deprecated(), please use context_xxxx::instance() instead.

I think the concept is good, doing the emailing in batches but I think we can use the existing table to do this. I am still unsure about the other settings. The other big flaw with this approach if I am correct is that we rely on a count which is not a good indicator. If we run the task and we process 50 users, then we unenrol them all and enrol 50 new users these new users will not be emailed as we are storing 50. Correct me if I am wrong.

Thanks, look forward to your reply.

[mohamedmohamedatia]

@mdjnelson

Apologies for the delay in responding. I missed reading the notification email and just noticed it now. Thank you for your valuable feedback.

I'll review and implement your suggestions promptly and will reply soon.

Best regards,

[mohamedmohamedatia]

@mdjnelson
I appreciate your input and I'd like to clarify a few points about the changes I've implemented.

Before my edits, the task was indeed handling two primary functions: issuing certificates and sending emails. It loops through all certificates on the system to check if new users could issue them.

I introduced a new feature that allows administrators to determine how many certificates to process during each run. By default, if set to 0, it will check all certificates.

I introduced a table to store the starting position for certificate processing each run. For example, if there are 1000 certificates and the admin sets 100 certificates per run, after the first run, it stores 100 as the last_processed field. Subsequent runs begin from position 101, I am not sure if the task_log table alone can achieve this, I will wait for further direction from your side if there is a better way to store the start position.

Regarding Other notes – I created a branch named Notes#610 and all notes are done; should I merge now?

Regarding your concern about the count, it's for looping over certificates, not users. It determines the batch size for processing certificates, not users for issuance, as highlighted above.

Regarding users, instead of iterating over all enrolled users, the loop now targets users with the capability "issue certificate".
For instance, if a certificate is restricted to a certain role, such as "teacher," only users with that role will be included in the loop. Similarly, if the certificate is tied to completing a particular activity, only users who have completed that activity will be considered.

Additionally, I added the ability for admins to exclude certificates from hidden courses and categories, vital in our university where old terms are often hidden. This ensures active term certificates are only processed by default, with the option to include hidden courses if needed. Also, admins can exclude old courses that ended in the past over a determined period from processing.

Thanks, look forward to your reply.

[mdjnelson]

@mohamedmohamedatia you can just add a commit onto the same branch instead of creating a new pull request or branch and I can see them here. I'll address the rest of this review when I am next working on the project.

[mdjnelson]

Thanks @mohamedmohamedatia. Can you squash these commits into one and rebase this on the latest version of MOODLE_402_STABLE. Right now there are conflicts. Thanks!

[mohamedmohamedatia]

Hi Mark,
Thank you for your guidance. I've squashed the commits as requested and rebased the branch onto the latest version of MOODLE_402_STABLE

It was my first time squashing commits, but I followed online tutorials to ensure I did it correctly. As a result, we now have two succinct commits:

1. Configurable settings for task efficiency
2. Optimize email certificate task by reducing database reads/writes

Please let me know if there's anything else I can assist you with.
Thanks again for your direction!

[mdjnelson]

Hi @mohamedmohamedatia, did you pull down the latest updates of the customcert version 4.2 and then do the rebase as I still get the errorr "This branch has conflicts that must be resolved" on GitHub? Also these aren't exactly succinct since the second commit contains fixes for the first commit. I would prefer them separated, or just have one commit (easiest solution).

[mdjnelson]

@mohamedmohamedatia, there are still conflicts. You will need to git pull the MOODLE_402_STABLE, then checkout your branch and do a rebase on it. See https://github.com/mdjnelson/moodle-mod_customcert/commits/MOODLE_402_STABLE/ - this is the latest.

[mohamedmohamedatia]

Hi Mark,
I've successfully addressed the conflicts and combined the changes into a single commit. is everything OK now?

[mdjnelson]

Awesome work @mohamedmohamedatia. When I get time ill give it another review. :)

[mdjnelson]

Hi @mohamedmohamedatia can you fix this issues raised by the automated tests? :)

[mohamedmohamedatia]

Hi Mark, Noted, will address them soon.

[mohamedmohamedatia]

Hi Mark,
I fixed the test issues. All tests now pass. Squashed commits into one.
Results: OK (7 tests, 29 assertions).

[mdjnelson]

Thanks @mohamedmohamedatia,

I am currently busy with work outside of Moodle but when I get a chance ill give it an extensive review.

Regards,

Mark

[mdjnelson]

Looking at this now. Just going to comment that this will solve #531 so it links in GitHub.

[mdjnelson]

Thanks for working on this @mohamedmohamedatia and your perseverance.

I have made several comments in the diff.

Also, the git commit message should not be so long.

I suggest -

Optimise email certificate task. (#531)
    
By reducing database reads/writes and introducing
configurable settings for task efficiency.

Also the version in version.php needs to be 2023042409. I will sort this out when I backport it to version 4.1 as it will need a different version so don't worry about that.

[ericmerrill]

I don't want to step on any toes, but I (or someone on my team) could review this, and update code with Mark's suggestions if @mohamedmohamedatia won't have time soon to work on this. We have several clients that have been having performance problems with this.

[mdjnelson]

Go ahead Eric. Thanks! :)

[ojnadjarm]

Hi @mdjnelson, I'm Oscar I work for Moodle US and I working on this, so I went deep into this issue and I think we have 2 pain points here.

1. The main query, brings ALL the certificates that have the emailing active, no matter how old the certificate is or if still active. This should not be an issue for 100 certificates but with site with 1000+ or event 10000+ this doesn't scale. We are maybe loopping on thousands of certificates that probably don't need to be processed. So Excluding hidden courses and old courses directly on the query it will help a lot because we do a lot of queries per certificate, I think even more than running the query by a batches. We should still mantain that posibility because if there are a lot of active certificates It will be needed, also and I added a filter not only for inactive courses but also for courses that doesn't have an enddate but the last issued certificate is older than what is configured by the user.(Let say the lastest issued certificate is 2 years old, we probably should ignore that certificate). Also I don't think we need to create a table, we were just using 1 value and 1 row, it fit pretty well as a plugin config just for the backend.
2. Issuing the certificates and emailing on the schedule task could be a really heavy load when we are doing a bulk issuing. I separate the issues from the emailing, It was not only creating the issues, but creating the PDF and sending the email, too much work for a single task. I added a new shechedule task that will only issue the certificate and if an email is requested it will created an adhoc task that will be the one on creating and emailing the certificate. This will be enable by a setting. If not enable it will do it live. This is the PR I create to our repo. Let me know what do you think if you are ok with it we can create a PR to the main repo.
   https://github.com/moodleus/moodle-mod_customcert/pull/1/files

[mdjnelson]

Let me look at this properly tomorrow. I appreciate you taking your time to work on this and contribute. No harm in doing a PR now so we can start a discussion there.

[mdjnelson]

I also noticed from this fork https://github.com/Claud10R/moodle-mod_customcert that @Claud10R created some commits to work on this. Would be good to see if perhaps we can make this even more efficient. Waiting on PR before reviewing. :)

[ojnadjarm]

Hi, awesome I'll review it CLaud's commits.

[mohamedmohamedatia]

Hi Mark,

I apologize for the delay in my response. I was on an extended leave for two months and was unable to review emails during that period.

I have reviewed your comments and made the necessary changes. I’ve also added my responses to each of your comments. Please note that while I haven’t completely halted the processing of hidden courses, I’ve enabled administrators to make a selection based on their operational policies. The default setting will not process hidden courses.

The default configuration now processes courses with an end date no older than one year. However, administrators can adjust this setting to zero if they wish to include all courses. Courses without an end date are considered open and will always be processed, such as self-study courses.

I will review #632 at a later time, but I suggest we proceed with #610 for now and consider #632 as future enhancements. Please let me know if you have any suggestions for alternative scenarios.

Best regards,

[mdjnelson]

Ignore the failures for now as it seems to be because of moodlehq/moodle-plugin-ci#309.

[mohamedmohamedatia]

No worries, I’ll ignore the failures for now since it seems to be related to moodlehq/moodle-plugin-ci#309. I ran the tests on my side, and everything passed just fine!

[mdjnelson]

I have cherry-picked this (with some minor changes) in to 4.4, 4.3, 4.2 and 4.1. I have not released it as a version in the plugins DB though.

@mohamedmohamedatia just to confirm that if this processes a particular course that it does not ignore it when it does the batching? New students could still qualify for the certificate after its been processed.

[mohamedmohamedatia]

Hi Mark Great news, Thank you for cherry-picking the changes into the different versions!, I can confirm that after a course is processed, it is not excluded in future batches. This ensures that any new students who meet the criteria after the initial processing will still qualify for the certificate.

[mdjnelson]

Awesome, closing. :) Thanks @mohamedmohamedatia for all your hard work, it's appreciated.

[mohamedmohamedatia]

Thanks @mdjnelson ! It’s been great working on this plugin. I appreciate the chance to contribute!

[mdjnelson]

Hi @mohamedmohamedatia, it seems this introduced an urgent bug that needs fixing. Can you take a look at https://moodle.org/mod/forum/discuss.php?d=461260 and see if you can spot the issue?

[mdjnelson]

The following code was removed -

// Now check if the certificate is not visible to the current user.
$cm = get_fast_modinfo($customcert->courseid, $enroluser->id)->instances['customcert'][$customcert->id];
if (!$cm->uservisible) {
    continue;
}

[mdjnelson]

I created a fix you can find at 94cfd4b. I am not sure why $infomodule->filter_user_list($userswithissueview) did not work, strange. I have created a unit test now so we wont run into this issue again (hopefully).