From 7324de23201b4e981e6b4283a5d28d570b3cbfa8 Mon Sep 17 00:00:00 2001 From: David Woloszyn Date: Fri, 20 Dec 2024 16:04:51 +1100 Subject: [PATCH] MDL-84042 mod_assign: Fixed notification helper submission check mod_assign_generator now accepts a status when creating a submission. --- mod/assign/classes/notification_helper.php | 12 ++-- mod/assign/tests/generator/lib.php | 5 ++ mod/assign/tests/notification_helper_test.php | 62 +++++++++++++++---- 3 files changed, 64 insertions(+), 15 deletions(-) diff --git a/mod/assign/classes/notification_helper.php b/mod/assign/classes/notification_helper.php index 7523d27d27c..8a863c40296 100644 --- a/mod/assign/classes/notification_helper.php +++ b/mod/assign/classes/notification_helper.php @@ -220,7 +220,8 @@ class notification_helper { foreach ($users as $key => $user) { // Check if the user has submitted already. - if ($assignmentobj->get_user_submission($user->id, false)) { + $submission = $assignmentobj->get_user_submission($user->id, false); + if ($submission && $submission->status === ASSIGN_SUBMISSION_STATUS_SUBMITTED) { unset($users[$key]); continue; } @@ -320,7 +321,8 @@ class notification_helper { } // Check if the user has submitted already. - if ($assignmentobj->get_user_submission($userid, false)) { + $submission = $assignmentobj->get_user_submission($userid, false); + if ($submission && $submission->status === ASSIGN_SUBMISSION_STATUS_SUBMITTED) { return; } @@ -400,7 +402,8 @@ class notification_helper { } // Check if the user has submitted already. - if ($assignmentobj->get_user_submission($userid, false)) { + $submission = $assignmentobj->get_user_submission($userid, false); + if ($submission && $submission->status === ASSIGN_SUBMISSION_STATUS_SUBMITTED) { return; } @@ -471,7 +474,8 @@ class notification_helper { $assignmentobj = self::get_assignment_data($assignment->id); // Check if the user has submitted already. - if ($assignmentobj->get_user_submission($userid, false)) { + $submission = $assignmentobj->get_user_submission($userid, false); + if ($submission && $submission->status === ASSIGN_SUBMISSION_STATUS_SUBMITTED) { continue; } diff --git a/mod/assign/tests/generator/lib.php b/mod/assign/tests/generator/lib.php index 21d1b87bd58..8dc881bd9f6 100644 --- a/mod/assign/tests/generator/lib.php +++ b/mod/assign/tests/generator/lib.php @@ -126,9 +126,14 @@ class mod_assign_generator extends testing_module_generator { if (array_key_exists($pluginname, $data)) { $plugingenerator = $this->datagenerator->get_plugin_generator("assignsubmission_{$pluginname}"); $plugingenerator->add_submission_data($submission, $assign, $data); + $plugin->enable(); } } + if (isset($data['status'])) { + $submission->status = $data['status']; + } + $assign->save_submission($submission, $notices); $this->set_user($currentuser); diff --git a/mod/assign/tests/notification_helper_test.php b/mod/assign/tests/notification_helper_test.php index fc9487319e9..db454379f8e 100644 --- a/mod/assign/tests/notification_helper_test.php +++ b/mod/assign/tests/notification_helper_test.php @@ -106,6 +106,7 @@ final class notification_helper_test extends \advanced_testcase { $assignment = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate, + 'submissiondrafts' => 0, ]); // User1 will have a user override, giving them an extra 1 hour for 'duedate'. @@ -140,6 +141,7 @@ final class notification_helper_test extends \advanced_testcase { 'cmid' => $assignment->cmid, 'status' => 'submitted', 'timemodified' => $clock->time(), + 'onlinetext' => 'Some text', ]); // There should be 3 users with the teacher excluded. @@ -171,6 +173,7 @@ final class notification_helper_test extends \advanced_testcase { $assignment = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate, + 'submissiondrafts' => 0, ]); $clock->bump(5); @@ -184,15 +187,14 @@ final class notification_helper_test extends \advanced_testcase { $duedate = $assignmentobj->get_instance($user1->id)->duedate; // Get the notifications that should have been created during the adhoc task. - $this->assertCount(1, $sink->get_messages_by_component('mod_assign')); + $messages = $sink->get_messages_by_component('mod_assign'); + $this->assertCount(1, $messages); // Check the subject matches. - $messages = $sink->get_messages_by_component('mod_assign'); $message = reset($messages); $stringparams = [ 'duedate' => userdate($duedate), 'assignmentname' => $assignment->name, - 'type' => $helper::TYPE_DUE_SOON, ]; $expectedsubject = get_string('assignmentduesoonsubject', 'mod_assign', $stringparams); $this->assertEquals($expectedsubject, $message->subject); @@ -216,7 +218,16 @@ final class notification_helper_test extends \advanced_testcase { $this->run_due_soon_notification_helper_tasks(); // There should be a new notification because the 'duedate' has been updated. - $this->assertCount(1, $sink->get_messages_by_component('mod_assign')); + $messages = $sink->get_messages_by_component('mod_assign'); + $this->assertCount(1, $messages); + $message = reset($messages); + $stringparams = [ + 'duedate' => userdate($updatedata->duedate), + 'assignmentname' => $assignment->name, + ]; + $expectedsubject = get_string('assignmentduesoonsubject', 'mod_assign', $stringparams); + $this->assertEquals($expectedsubject, $message->subject); + // Clear sink. $sink->clear(); @@ -232,14 +243,18 @@ final class notification_helper_test extends \advanced_testcase { 'cmid' => $assignment->cmid, 'status' => 'submitted', 'timemodified' => $clock->time(), + 'onlinetext' => 'Some text', ]); $clock->bump(5); // Run the tasks again. $this->run_due_soon_notification_helper_tasks(); - // No new notification should have been sent. - $this->assertEmpty($sink->get_messages_by_component('mod_assign')); + // There should only be one notifcation for submission (not for due soon). + $messages = $sink->get_messages_by_component('mod_assign'); + $this->assertCount(1, $messages); + $expectedsubject = get_string('submissionreceiptsmall', 'mod_assign', ['assignment' => $assignment->name]); + $this->assertEquals($expectedsubject, reset($messages)->subject); // Clear sink. $sink->clear(); @@ -321,6 +336,7 @@ final class notification_helper_test extends \advanced_testcase { $assignment = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate, + 'submissiondrafts' => 0, ]); // User1 will have a user override, giving them an extra minute for 'duedate'. @@ -356,6 +372,7 @@ final class notification_helper_test extends \advanced_testcase { 'cmid' => $assignment->cmid, 'status' => 'submitted', 'timemodified' => $clock->time(), + 'onlinetext' => 'Some text', ]); // User6 will have a cut-off date override that has already lapsed, excluding them from the results. @@ -398,6 +415,7 @@ final class notification_helper_test extends \advanced_testcase { 'course' => $course->id, 'duedate' => $duedate, 'cutoffdate' => $cutoffdate, + 'submissiondrafts' => 0, ]); $clock->bump(5); @@ -435,7 +453,11 @@ final class notification_helper_test extends \advanced_testcase { $this->run_overdue_notification_helper_tasks(); // There should be a new notification because the 'duedate' has been updated. - $this->assertCount(1, $sink->get_messages_by_component('mod_assign')); + $messages = $sink->get_messages_by_component('mod_assign'); + $this->assertCount(1, $messages); + $message = reset($messages); + $expectedsubject = get_string('assignmentoverduesubject', 'mod_assign', ['assignmentname' => $assignment->name]); + $this->assertEquals($expectedsubject, $message->subject); // Let's modify the 'cut-off date'. $updatedata = new \stdClass(); @@ -450,7 +472,10 @@ final class notification_helper_test extends \advanced_testcase { $this->run_overdue_notification_helper_tasks(); // There should be a new notification because the 'cut-off date' has been updated. - $this->assertCount(1, $sink->get_messages_by_component('mod_assign')); + $messages = $sink->get_messages_by_component('mod_assign'); + $message = reset($messages); + $expectedsubject = get_string('assignmentoverduesubject', 'mod_assign', ['assignmentname' => $assignment->name]); + $this->assertEquals($expectedsubject, $message->subject); // Let's modify the 'duedate' one more time. $updatedata = new \stdClass(); @@ -464,6 +489,7 @@ final class notification_helper_test extends \advanced_testcase { 'cmid' => $assignment->cmid, 'status' => 'submitted', 'timemodified' => $clock->time(), + 'onlinetext' => 'Some text', ]); // Clear sink. @@ -524,6 +550,7 @@ final class notification_helper_test extends \advanced_testcase { $assignment = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate, + 'submissiondrafts' => 0, ]); // User1 will have a user override, giving them an extra 1 day for 'duedate', excluding them from the results. @@ -551,6 +578,7 @@ final class notification_helper_test extends \advanced_testcase { 'cmid' => $assignment->cmid, 'status' => 'submitted', 'timemodified' => $clock->time(), + 'onlinetext' => 'Some text', ]); // There should be 1 user with the teacher excluded. @@ -581,16 +609,19 @@ final class notification_helper_test extends \advanced_testcase { $assignment1 = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate1, + 'submissiondrafts' => 0, ]); $duedate2 = $clock->time() + WEEKSECS; $assignment2 = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate2, + 'submissiondrafts' => 0, ]); $duedate3 = $clock->time() + WEEKSECS + DAYSECS; $assignment3 = $assignmentgenerator->create_instance([ 'course' => $course->id, 'duedate' => $duedate3, + 'submissiondrafts' => 0, ]); $clock->bump(5); @@ -598,10 +629,10 @@ final class notification_helper_test extends \advanced_testcase { $this->run_due_digest_notification_helper_tasks(); // Get the notifications that should have been created during the adhoc task. - $this->assertCount(1, $sink->get_messages_by_component('mod_assign')); + $messages = $sink->get_messages_by_component('mod_assign'); + $this->assertCount(1, $messages); // Check the message for the expected assignments. - $messages = $sink->get_messages_by_component('mod_assign'); $message = reset($messages); $this->assertStringContainsString($assignment1->name, $message->fullmessagehtml); $this->assertStringContainsString($assignment2->name, $message->fullmessagehtml); @@ -611,6 +642,10 @@ final class notification_helper_test extends \advanced_testcase { $formatteddate = userdate($duedate1, get_string('strftimedaydate', 'langconfig')); $this->assertStringContainsString($formatteddate, $message->fullmessagehtml); + // Check the subject matches. + $expectedsubject = get_string('assignmentduedigestsubject', 'mod_assign', $message->subject); + $this->assertEquals($expectedsubject, $message->subject); + // Clear sink. $sink->clear(); @@ -639,6 +674,7 @@ final class notification_helper_test extends \advanced_testcase { 'cmid' => $assignment2->cmid, 'status' => 'submitted', 'timemodified' => $clock->time(), + 'onlinetext' => 'Some text', ]); $clock->bump(5); @@ -646,7 +682,11 @@ final class notification_helper_test extends \advanced_testcase { $this->run_due_digest_notification_helper_tasks(); // There are no assignments left to report, so no notification should have been sent. - $this->assertEmpty($sink->get_messages_by_component('mod_assign')); + // There should only be one notifcation for submission. + $messages = $sink->get_messages_by_component('mod_assign'); + $this->assertCount(1, $messages); + $expectedsubject = get_string('submissionreceiptsmall', 'mod_assign', ['assignment' => $assignment2->name]); + $this->assertEquals($expectedsubject, reset($messages)->subject); // Clear sink. $sink->clear();