Skip to content

Commit 723bbec

Browse files
mdjnelsonmdjnelson
committed
Ensure certificates are issued and emailed only once when emailing is enabled (#671)
1 parent 44e1ef4 commit 723bbec

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
@@ -125,7 +125,7 @@ public function execute() {
125125
// Get the context.
126126
$context = \context::instance_by_id($customcert->contextid);
127127

128-
// Get a list of all the issues.
128+
// Get a list of all the issues that are already emailed (skip these users).
129129
$sql = "SELECT u.id
130130
FROM {customcert_issues} ci
131131
JOIN {user} u
@@ -140,57 +140,59 @@ public function execute() {
140140
// Get the context of the Custom Certificate module.
141141
$cmcontext = \context_module::instance($cm->id);
142142

143-
// Now, get a list of users who can view and issue the certificate but have not yet.
144143
// Get users with the mod/customcert:receiveissue capability in the Custom Certificate module context.
145144
$userswithissue = get_users_by_capability($cmcontext, 'mod/customcert:receiveissue');
146145
// Get users with mod/customcert:view capability.
147146
$userswithview = get_users_by_capability($cmcontext, 'mod/customcert:view');
148-
// Users with both mod/customcert:view and mod/customcert:receiveissue cabapilities.
147+
// Users with both mod/customcert:view and mod/customcert:receiveissue capabilities.
149148
$userswithissueview = array_intersect_key($userswithissue, $userswithview);
150149

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

158154
foreach ($filteredusers as $filtereduser) {
159-
// Check if the user has already been issued and emailed.
155+
// Skip if the user has already been issued and emailed.
160156
if (in_array($filtereduser->id, array_keys((array)$issuedusers))) {
161157
continue;
162158
}
163159

164-
// Don't want to issue to teachers.
160+
// Don't want to issue to teachers/managers.
165161
if (in_array($filtereduser->id, array_keys((array)$userswithmanage))) {
166162
continue;
167163
}
168164

169-
// Now check if the certificate is not visible to the current user.
170-
$cm = get_fast_modinfo($customcert->courseid, $filtereduser->id)->instances['customcert'][$customcert->id];
171-
if (!$cm->uservisible) {
165+
// Check whether the CM is visible to this user.
166+
$usercm = get_fast_modinfo($customcert->courseid, $filtereduser->id)->instances['customcert'][$customcert->id];
167+
if (!$usercm->uservisible) {
172168
continue;
173169
}
174170

175-
// Check that they have passed the required time.
171+
// Check required time (if any).
176172
if (!empty($customcert->requiredtime)) {
177173
if (\mod_customcert\certificate::get_course_time($customcert->courseid,
178174
$filtereduser->id) < ($customcert->requiredtime * 60)) {
179175
continue;
180176
}
181177
}
182178

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

191-
// Validate issueid and one last check for emailed.
192-
if (!empty($issueid) && empty($issue->emailed)) {
193-
// We create a new adhoc task to send the email.
194+
// If we have an issue and it has not been emailed yet, send it now.
195+
if (!empty($issueid) && $emailed === 0) {
194196
$task = new \mod_customcert\task\email_certificate_task();
195197
$task->set_custom_data(['issueid' => $issueid, 'customcertid' => $customcert->id]);
196198
$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)