Skip to content

Commit 7a871e9

Browse files
mdjnelsonmdjnelson
committed
Ensure certificates are issued and emailed only once when emailing is enabled (#671)
1 parent 14e91a8 commit 7a871e9

File tree

3 files changed

+103
-24
lines changed

3 files changed

+103
-24
lines changed

‎classes/task/email_certificate_task.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,12 @@ public function execute() {
5050
global $DB;
5151

5252
$customdata = $this->get_custom_data();
53+
if (empty($customdata) || empty($customdata->issueid) || empty($customdata->customcertid)) {
54+
return;
55+
}
5356

54-
$issueid = $customdata->issueid;
55-
$customcertid = $customdata->customcertid;
57+
$issueid = (int)$customdata->issueid;
58+
$customcertid = (int)$customdata->customcertid;
5659
$sql = "SELECT c.*, ct.id as templateid, ct.name as templatename, ct.contextid, co.id as courseid,
5760
co.fullname as coursefullname, co.shortname as courseshortname
5861
FROM {customcert} c
@@ -61,6 +64,9 @@ public function execute() {
6164
WHERE c.id = :id";
6265

6366
$customcert = $DB->get_record_sql($sql, ['id' => $customcertid]);
67+
if (!$customcert) {
68+
return;
69+
}
6470

6571
// The renderers used for sending emails.
6672
$page = new \moodle_page();
@@ -93,6 +99,9 @@ public function execute() {
9399
WHERE ci.customcertid = :customcertid
94100
AND ci.id = :issueid";
95101
$user = $DB->get_record_sql($sql, ['customcertid' => $customcertid, 'issueid' => $issueid]);
102+
if (!$user) {
103+
return;
104+
}
96105

97106
// Create a directory to store the PDF we will be sending.
98107
$tempdir = make_temp_directory('certificate/attachment');

‎classes/task/issue_certificates_task.php‎

Lines changed: 24 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ public function execute() {
119119
// Get the context.
120120
$context = \context::instance_by_id($customcert->contextid);
121121

122-
// Get a list of all the issues.
122+
// Get a list of all the issues that are already emailed (skip these users).
123123
$sql = "SELECT u.id
124124
FROM {customcert_issues} ci
125125
JOIN {user} u
@@ -134,57 +134,59 @@ public function execute() {
134134
// Get the context of the Custom Certificate module.
135135
$cmcontext = \context_module::instance($cm->id);
136136

137-
// Now, get a list of users who can view and issue the certificate but have not yet.
138137
// Get users with the mod/customcert:receiveissue capability in the Custom Certificate module context.
139138
$userswithissue = get_users_by_capability($cmcontext, 'mod/customcert:receiveissue');
140139
// Get users with mod/customcert:view capability.
141140
$userswithview = get_users_by_capability($cmcontext, 'mod/customcert:view');
142-
// Users with both mod/customcert:view and mod/customcert:receiveissue cabapilities.
141+
// Users with both mod/customcert:view and mod/customcert:receiveissue capabilities.
143142
$userswithissueview = array_intersect_key($userswithissue, $userswithview);
144143

145-
// Filter the remaining users by determining whether they can actually see the CM or not
146-
// (Note: filter_user_list only takes into account those availability condition which actually implement
147-
// this function, so the second check with get_fast_modinfo must be still performed - but we can reduce the
148-
// size of the users list here already).
144+
// Filter remaining users by availability conditions.
149145
$infomodule = new \core_availability\info_module($cm);
150146
$filteredusers = $infomodule->filter_user_list($userswithissueview);
151147

152148
foreach ($filteredusers as $filtereduser) {
153-
// Check if the user has already been issued and emailed.
149+
// Skip if the user has already been issued and emailed.
154150
if (in_array($filtereduser->id, array_keys((array)$issuedusers))) {
155151
continue;
156152
}
157153

158-
// Don't want to issue to teachers.
154+
// Don't want to issue to teachers/managers.
159155
if (in_array($filtereduser->id, array_keys((array)$userswithmanage))) {
160156
continue;
161157
}
162158

163-
// Now check if the certificate is not visible to the current user.
164-
$cm = get_fast_modinfo($customcert->courseid, $filtereduser->id)->instances['customcert'][$customcert->id];
165-
if (!$cm->uservisible) {
159+
// Check whether the CM is visible to this user.
160+
$usercm = get_fast_modinfo($customcert->courseid, $filtereduser->id)->instances['customcert'][$customcert->id];
161+
if (!$usercm->uservisible) {
166162
continue;
167163
}
168164

169-
// Check that they have passed the required time.
165+
// Check required time (if any).
170166
if (!empty($customcert->requiredtime)) {
171167
if (\mod_customcert\certificate::get_course_time($customcert->courseid,
172168
$filtereduser->id) < ($customcert->requiredtime * 60)) {
173169
continue;
174170
}
175171
}
176172

177-
// Ensure the cert hasn't already been issued, e.g via the UI (view.php) - a race condition.
173+
// Ensure the cert hasn't already been issued; if not, issue it now.
178174
$issue = $DB->get_record('customcert_issues',
179-
['userid' => $filtereduser->id, 'customcertid' => $customcert->id], 'id, emailed');
180-
181-
// Ok, issue them the certificate.
182-
$issueid = empty($issue) ?
183-
\mod_customcert\certificate::issue_certificate($customcert->id, $filtereduser->id) : $issue->id;
175+
['userid' => $filtereduser->id, 'customcertid' => $customcert->id],
176+
'id, emailed');
177+
178+
$issueid = null;
179+
$emailed = 0;
180+
if (!empty($issue)) {
181+
$issueid = (int)$issue->id;
182+
$emailed = (int)$issue->emailed;
183+
} else {
184+
$issueid = \mod_customcert\certificate::issue_certificate($customcert->id, $filtereduser->id);
185+
$emailed = 0;
186+
}
184187

185-
// Validate issueid and one last check for emailed.
186-
if (!empty($issueid) && empty($issue->emailed)) {
187-
// We create a new adhoc task to send the email.
188+
// If we have an issue and it has not been emailed yet, send it now.
189+
if (!empty($issueid) && $emailed === 0) {
188190
$task = new \mod_customcert\task\email_certificate_task();
189191
$task->set_custom_data(['issueid' => $issueid, 'customcertid' => $customcert->id]);
190192
$useadhoc = get_config('customcert', 'useadhoc');

‎tests/email_certificate_task_test.php‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -869,4 +869,72 @@ public function test_issue_certificates_task_creates_issue_when_none_exist(): vo
869869
$this->assertEquals($student->email, $emails[0]->to, 'Email recipient is incorrect.');
870870
}
871871

872+
/**
873+
* The email_certificate_task should no-op (not fatal) when custom data is missing.
874+
*
875+
* @covers \mod_customcert\task\email_certificate_task
876+
*/
877+
public function test_email_adhoc_task_ignores_missing_customdata(): void {
878+
// No setup; call the adhoc task with no custom data.
879+
$sink = $this->redirectEmails();
880+
881+
$task = new \mod_customcert\task\email_certificate_task();
882+
// Intentionally DO NOT call set_custom_data().
883+
$task->execute();
884+
885+
// Should not throw; should not send any email.
886+
$emails = $sink->get_messages();
887+
$this->assertCount(0, $emails);
888+
}
889+
890+
/**
891+
* The email_certificate_task should no-op when customcert id is invalid.
892+
*
893+
* @covers \mod_customcert\task\email_certificate_task
894+
*/
895+
public function test_email_adhoc_task_invalid_customcertid(): void {
896+
$sink = $this->redirectEmails();
897+
898+
$task = new \mod_customcert\task\email_certificate_task();
899+
900+
// Point to bogus ids; both should be integers but not exist.
901+
$task->set_custom_data((object)['issueid' => 999999, 'customcertid' => 999998]);
902+
$task->execute();
903+
904+
$emails = $sink->get_messages();
905+
$this->assertCount(0, $emails);
906+
}
907+
908+
/**
909+
* The email_certificate_task should no-op when issue id is invalid (even if customcert exists).
910+
*
911+
* @covers \mod_customcert\task\email_certificate_task
912+
*/
913+
public function test_email_adhoc_task_invalid_issueid_with_valid_customcert(): void {
914+
global $DB;
915+
916+
// Minimal valid customcert (with one element) so the customcert lookup succeeds.
917+
$course = $this->getDataGenerator()->create_course();
918+
$customcert = $this->getDataGenerator()->create_module('customcert', ['course' => $course->id]);
919+
920+
// Make the template valid.
921+
$template = new \stdClass();
922+
$template->id = $customcert->templateid;
923+
$template->name = 'T';
924+
$template->contextid = \context_course::instance($course->id)->id;
925+
$template = new \mod_customcert\template($template);
926+
$pageid = $template->add_page();
927+
$DB->insert_record('customcert_elements', (object)['pageid' => $pageid, 'name' => 'E']);
928+
929+
$sink = $this->redirectEmails();
930+
931+
$task = new \mod_customcert\task\email_certificate_task();
932+
933+
// Valid customcertid, but bogus issueid.
934+
$task->set_custom_data((object)['issueid' => 123456789, 'customcertid' => $customcert->id]);
935+
$task->execute();
936+
937+
$emails = $sink->get_messages();
938+
$this->assertCount(0, $emails);
939+
}
872940
}

0 commit comments

Comments
 (0)