From 200eb302256c4ea10e18abdf83d0c23363fdc47a Mon Sep 17 00:00:00 2001 From: Tim Hunt Date: Wed, 12 Jun 2024 12:17:05 +0100 Subject: [PATCH] MDL-82178 quiz graded notification: fix capability check Also improve the scheduled task logging --- ...otify_attempt_manual_grading_completed.php | 20 ++--- ..._attempt_manual_grading_completed_test.php | 77 +++++++++---------- 2 files changed, 43 insertions(+), 54 deletions(-) diff --git a/mod/quiz/classes/task/quiz_notify_attempt_manual_grading_completed.php b/mod/quiz/classes/task/quiz_notify_attempt_manual_grading_completed.php index 8da8f09032891..8c0b7406c39b3 100644 --- a/mod/quiz/classes/task/quiz_notify_attempt_manual_grading_completed.php +++ b/mod/quiz/classes/task/quiz_notify_attempt_manual_grading_completed.php @@ -18,7 +18,7 @@ defined('MOODLE_INTERNAL') || die(); -use context_course; +use context_module; use core_user; use mod_quiz\quiz_attempt; use moodle_recordset; @@ -41,11 +41,7 @@ class quiz_notify_attempt_manual_grading_completed extends \core\task\scheduled_ */ protected $forcedtime = null; - /** - * Get name of schedule task. - * - * @return string - */ + #[\Override] public function get_name(): string { return get_string('notifyattemptsgradedtask', 'mod_quiz'); } @@ -75,10 +71,8 @@ public function set_time_for_testing(int $time): void { $this->forcedtime = $time; } - /** - * Execute sending notification for manual graded attempts. - */ - public function execute() { + #[\Override] + public function execute(): void { global $DB; mtrace('Looking for quiz attempts which may need a graded notification sent...'); @@ -94,11 +88,11 @@ public function execute() { if (!$quiz || $attempt->quiz != $quiz->id) { $quiz = $DB->get_record('quiz', ['id' => $attempt->quiz], '*', MUST_EXIST); $cm = get_coursemodule_from_instance('quiz', $attempt->quiz); + $quizcontext = context_module::instance($cm->id); } if (!$course || $course->id != $quiz->course) { $course = get_course($quiz->course); - $coursecontext = context_course::instance($quiz->course); } $quiz = quiz_update_effective_access($quiz, $attempt->userid); @@ -108,11 +102,13 @@ public function execute() { if ($options->manualcomment == question_display_options::HIDDEN) { // User cannot currently see the feedback, so don't message them. // However, this may change in future, so leave them on the list. + mtrace('Not sending an email because manualcomment review option is not set.'); continue; } - if (!has_capability('mod/quiz:emailnotifyattemptgraded', $coursecontext, $attempt->userid, false)) { + if (!has_capability('mod/quiz:emailnotifyattemptgraded', $quizcontext, $attempt->userid, false)) { // User not eligible to get a notification. Mark them done while doing nothing. + mtrace('Not sending an email because user does not have mod/quiz:emailnotifyattemptgraded capability.'); $DB->set_field('quiz_attempts', 'gradednotificationsenttime', $attempt->timefinish, ['id' => $attempt->id]); continue; } diff --git a/mod/quiz/tests/quiz_notify_attempt_manual_grading_completed_test.php b/mod/quiz/tests/quiz_notify_attempt_manual_grading_completed_test.php index 0ff60cbc727f9..8a47453d41c7b 100644 --- a/mod/quiz/tests/quiz_notify_attempt_manual_grading_completed_test.php +++ b/mod/quiz/tests/quiz_notify_attempt_manual_grading_completed_test.php @@ -25,15 +25,13 @@ namespace mod_quiz; use advanced_testcase; -use context_course; use context_module; +use core\context; use mod_quiz\task\quiz_notify_attempt_manual_grading_completed; use question_engine; -use mod_quiz\quiz_settings; +use question_usage_by_activity; use stdClass; -defined('MOODLE_INTERNAL') || die(); - /** * Class containing unit tests for the quiz notify attempt manual grading completed cron task. @@ -43,35 +41,35 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ class quiz_notify_attempt_manual_grading_completed_test extends advanced_testcase { - /** @var \stdClass $course Test course to contain quiz. */ - protected $course; + /** @var stdClass $course Test course to contain quiz. */ + protected stdClass $course; - /** @var \stdClass $quiz A test quiz. */ - protected $quiz; + /** @var stdClass $quiz A test quiz. */ + protected stdClass $quiz; /** @var context The quiz context. */ - protected $context; + protected context $context; /** @var stdClass The course_module. */ - protected $cm; + protected stdClass $cm; /** @var stdClass The student test. */ - protected $student; + protected stdClass $student; - /** @var stdClass The teacher test. */ - protected $teacher; + /** @var stdClass The student role. */ + protected stdClass $studentrole; /** @var quiz_settings Object containing the quiz settings. */ - protected $quizobj; + protected quiz_settings $quizobj; /** @var question_usage_by_activity The question usage for this quiz attempt. */ - protected $quba; + protected question_usage_by_activity $quba; /** * Standard test setup. * - * Create a course with a quiz and a student and a(n editing) teacher. - * the quiz has a truefalse question and an essay question. + * Create a course with a quiz and a student. + * The quiz has a truefalse question and an essay question. * * Also create some bits of a quiz attempt to be used later. */ @@ -83,31 +81,21 @@ public function setUp(): void { // Setup test data. $this->course = $this->getDataGenerator()->create_course(); - $this->quiz = $this->getDataGenerator()->create_module('quiz', ['course' => $this->course->id]); - $this->context = context_module::instance($this->quiz->cmid); - $this->cm = get_coursemodule_from_instance('quiz', $this->quiz->id); - // Create users. + // Create a user enrolled as a student. $this->student = self::getDataGenerator()->create_user(); - $this->teacher = self::getDataGenerator()->create_user(); - - // Users enrolments. - $studentrole = $DB->get_record('role', ['shortname' => 'student']); - $teacherrole = $DB->get_record('role', ['shortname' => 'editingteacher']); - - // Allow student to receive messages. - $coursecontext = context_course::instance($this->course->id); - assign_capability('mod/quiz:emailnotifyattemptgraded', CAP_ALLOW, $studentrole->id, $coursecontext, true); - - $this->getDataGenerator()->enrol_user($this->student->id, $this->course->id, $studentrole->id); - $this->getDataGenerator()->enrol_user($this->teacher->id, $this->course->id, $teacherrole->id); + $this->studentrole = $DB->get_record('role', ['shortname' => 'student']); + $this->getDataGenerator()->enrol_user($this->student->id, $this->course->id, $this->studentrole->id); // Make a quiz. $quizgenerator = $this->getDataGenerator()->get_plugin_generator('mod_quiz'); $this->quiz = $quizgenerator->create_instance(['course' => $this->course->id, 'questionsperpage' => 0, 'grade' => 100.0, 'sumgrades' => 2]); + $this->context = context_module::instance($this->quiz->cmid); + $this->cm = get_coursemodule_from_instance('quiz', $this->quiz->id); // Create a truefalse question and an essay question. + /** @var \core_question_generator $questiongenerator */ $questiongenerator = $this->getDataGenerator()->get_plugin_generator('core_question'); $cat = $questiongenerator->create_question_category(); $truefalse = $questiongenerator->create_question('truefalse', null, ['category' => $cat->id]); @@ -131,7 +119,7 @@ public function test_get_list_of_attempts_within_conditions() { $timenow = time(); // Create an attempt to be completely graded (one hour ago). - $attempt1 = quiz_create_attempt($this->quizobj, 1, null, $timenow - HOURSECS, false, $this->student->id); + $attempt1 = quiz_create_attempt($this->quizobj, 1, false, $timenow - HOURSECS, false, $this->student->id); quiz_start_new_attempt($this->quizobj, $this->quba, $attempt1, 1, $timenow - HOURSECS); quiz_attempt_save_started($this->quizobj, $this->quba, $attempt1); @@ -149,6 +137,8 @@ public function test_get_list_of_attempts_within_conditions() { $update->id = $attemptobj1->get_attemptid(); $update->timemodified = $timenow; $update->sumgrades = $attemptobj1->get_question_usage()->get_total_mark(); + $update->gradednotificationsenttime = null; // Processfinish detects the notification is not required, so sets this. + // Unset to allow testing of our logic. $DB->update_record('quiz_attempts', $update); $attemptobj1->get_quizobj()->get_grade_calculator()->recompute_final_grade(); @@ -172,7 +162,7 @@ public function test_get_list_of_attempts_without_manual_graded() { $timenow = time(); // Create an attempt which won't be graded (1 hour ago). - $attempt2 = quiz_create_attempt($this->quizobj, 2, null, $timenow - HOURSECS, false, $this->student->id); + $attempt2 = quiz_create_attempt($this->quizobj, 2, false, $timenow - HOURSECS, false, $this->student->id); quiz_start_new_attempt($this->quizobj, $this->quba, $attempt2, 2, $timenow - HOURSECS); quiz_attempt_save_started($this->quizobj, $this->quba, $attempt2); @@ -198,13 +188,13 @@ public function test_notify_manual_grading_completed_task_without_capability() { // Create an attempt for a user without the capability. $timenow = time(); - $attempt = quiz_create_attempt($this->quizobj, 3, null, $timenow, false, $this->teacher->id); + $attempt = quiz_create_attempt($this->quizobj, 3, false, $timenow, false, $this->student->id); quiz_start_new_attempt($this->quizobj, $this->quba, $attempt, 3, $timenow - HOURSECS); quiz_attempt_save_started($this->quizobj, $this->quba, $attempt); // Process some responses and submit. $attemptobj = quiz_attempt::create($attempt->id); - $tosubmit = [2 => ['answer' => 'Answer of teacher.', 'answerformat' => FORMAT_HTML]]; + $tosubmit = [2 => ['answer' => 'Essay answer.', 'answerformat' => FORMAT_HTML]]; $attemptobj->process_submitted_actions($timenow - 30 * MINSECS, false, $tosubmit); $attemptobj->process_finish($timenow - 20 * MINSECS, false); @@ -216,15 +206,16 @@ public function test_notify_manual_grading_completed_task_without_capability() { $update->id = $attemptobj->get_attemptid(); $update->timemodified = $timenow; $update->sumgrades = $attemptobj->get_question_usage()->get_total_mark(); + $update->gradednotificationsenttime = null; // Processfinish detects the notification is not required, so sets this. + // Unset to allow testing of our logic. $DB->update_record('quiz_attempts', $update); $attemptobj->get_quizobj()->get_grade_calculator()->recompute_final_grade(); // Run the quiz notify attempt manual graded task. - ob_start(); + $this->expectOutputRegex("~Not sending an email because user does not have mod/quiz:emailnotifyattemptgraded capability.~"); $task = new quiz_notify_attempt_manual_grading_completed(); $task->set_time_for_testing($timenow + 5 * HOURSECS + 1); $task->execute(); - ob_get_clean(); $attemptobj = quiz_attempt::create($attempt->id); $this->assertEquals($attemptobj->get_attempt()->timefinish, $attemptobj->get_attempt()->gradednotificationsenttime); @@ -236,9 +227,12 @@ public function test_notify_manual_grading_completed_task_without_capability() { public function test_notify_manual_grading_completed_task_with_capability() { global $DB; + // Allow student to receive messages. + assign_capability('mod/quiz:emailnotifyattemptgraded', CAP_ALLOW, $this->studentrole->id, $this->context, true); + // Create an attempt with capability. $timenow = time(); - $attempt = quiz_create_attempt($this->quizobj, 4, null, $timenow, false, $this->student->id); + $attempt = quiz_create_attempt($this->quizobj, 4, false, $timenow, false, $this->student->id); quiz_start_new_attempt($this->quizobj, $this->quba, $attempt, 4, $timenow - HOURSECS); quiz_attempt_save_started($this->quizobj, $this->quba, $attempt); @@ -260,11 +254,10 @@ public function test_notify_manual_grading_completed_task_with_capability() { $attemptobj->get_quizobj()->get_grade_calculator()->recompute_final_grade(); // Run the quiz notify attempt manual graded task. - ob_start(); + $this->expectOutputRegex("~Sending email to user {$this->student->id}~"); $task = new quiz_notify_attempt_manual_grading_completed(); $task->set_time_for_testing($timenow + 5 * HOURSECS + 1); $task->execute(); - ob_get_clean(); $attemptobj = quiz_attempt::create($attempt->id);