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

[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 lastprocessed 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.

[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! :)