The loop in "email_certificate_task.php" that says it removes users already emailed appears redundant

[JackAidley]

I've been looking at this task because it is taking a long time even when nothing is happening - hopefully that'll be resolved by #596 - but while looking I noticed that this loop:

// Remove all the users who have already been emailed.
foreach ($issuedusers as $key => $issueduser) {
    if ($issueduser->emailed) {
        unset($issuedusers[$key]);
    }
}

Appears to do nothing since, as far as I can tell, the emailed flag is only ever set in the code a few lines above where it is set to 0:

// Add them to the array so we email them.
$enroluser->issueid = $issueid;
$enroluser->emailed = 0;
$issuedusers[] = $enroluser;

And so can never have a truthy value. I'm unclear whether this is actually causing a bug or whether it's just redundant.

[mdjnelson]

I had to think about this one for a while to understand why I put that there. Turns out it is needed.

This is needed because we get all the issued users in $issuedusers = $DB->get_records_sql($sql, ['customcertid' => $customcert->id]);. It then gets all the enrolled users in the course, and if they can see the certificate adds them to that list. The loop you pointed out removes the ones that are already emailed that were retrieved from the DB call I listed. We can not filter by email in the get_records_sql call because we want all users. If we just got the people who werent emailed then it would keep emailing users who already have it because they are in the enrolled users call.

[JackAidley]

Ah gotcha. Sorry I missed that on reading the code. One thought though, wouldn't it be better to screen out the issuedusers with emailed set to non-zero in the SQL query that returns them instead of fetching unwanted records and then screening them out? I don't suppose it's particularly performance critical either way.

[mdjnelson]

The problem with that is that the $issuedusers is used to check if the user has already been issued.