diff --git a/mod/assign/batchsetmarkingworkflowstateform.php b/mod/assign/batchsetmarkingworkflowstateform.php index cfa561cef60..91cb7eb3e1f 100644 --- a/mod/assign/batchsetmarkingworkflowstateform.php +++ b/mod/assign/batchsetmarkingworkflowstateform.php @@ -49,6 +49,11 @@ class mod_assign_batch_set_marking_workflow_state_form extends moodleform { $options = $params['markingworkflowstates']; $mform->addElement('select', 'markingworkflowstate', get_string('markingworkflowstate', 'assign'), $options); + // Don't allow notification to be sent until in "Released" state. + $mform->addElement('selectyesno', 'sendstudentnotifications', get_string('sendstudentnotifications', 'assign')); + $mform->setDefault('sendstudentnotifications', 0); + $mform->disabledIf('sendstudentnotifications', 'markingworkflowstate', 'neq', ASSIGN_MARKING_WORKFLOW_STATE_RELEASED); + $mform->addElement('hidden', 'id'); $mform->setType('id', PARAM_INT); $mform->addElement('hidden', 'action', 'setbatchmarkingworkflowstate'); @@ -59,5 +64,24 @@ class mod_assign_batch_set_marking_workflow_state_form extends moodleform { } + /** + * Validate the submitted form data. + * + * @param array $data array of ("fieldname"=>value) of submitted data + * @param array $files array of uploaded files "element_name"=>tmp_file_path + * @return array of "element_name"=>"error_description" if there are errors + */ + public function validation($data, $files) { + $errors = parent::validation($data, $files); + + // As the implementation of this feature exists currently, no user will see a validation + // failure from this form, but this check ensures the form won't validate if someone + // manipulates the 'sendstudentnotifications' field's disabled attribute client-side. + if (!empty($data['sendstudentnotifications']) && $data['markingworkflowstate'] != ASSIGN_MARKING_WORKFLOW_STATE_RELEASED) { + $errors['sendstudentnotifications'] = get_string('studentnotificationworkflowstateerror', 'assign'); + } + + return $errors; + } } diff --git a/mod/assign/gradeform.php b/mod/assign/gradeform.php index 751d220b2cb..2aab572bab6 100644 --- a/mod/assign/gradeform.php +++ b/mod/assign/gradeform.php @@ -77,6 +77,12 @@ class mod_assign_grade_form extends moodleform { global $DB; $errors = parent::validation($data, $files); $instance = $this->assignment->get_instance(); + + if ($instance->markingworkflow && !empty($data['sendstudentnotifications']) && + $data['workflowstate'] != ASSIGN_MARKING_WORKFLOW_STATE_RELEASED) { + $errors['sendstudentnotifications'] = get_string('studentnotificationworkflowstateerror', 'assign'); + } + // Advanced grading. if (!array_key_exists('grade', $data)) { return $errors; diff --git a/mod/assign/lang/en/assign.php b/mod/assign/lang/en/assign.php index 6652b1839fc..da89afce19c 100644 --- a/mod/assign/lang/en/assign.php +++ b/mod/assign/lang/en/assign.php @@ -353,6 +353,7 @@ $string['setmarkerallocationforlog'] = 'Set marking allocation : (id={$a->id}, f $string['settings'] = 'Assignment settings'; $string['showrecentsubmissions'] = 'Show recent submissions'; $string['status'] = 'Status'; +$string['studentnotificationworkflowstateerror'] = 'Marking workflow state must be \'Released\' to notify students.'; $string['submissioncopiedtext'] = 'You have made a copy of your previous assignment submission for \'{$a->assignment}\' diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 3fb608acf14..c47d466424d 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -1643,8 +1643,12 @@ class assign { $timenow = time(); $lastcron = $DB->get_field('modules', 'lastcron', array('name' => 'assign')); - // Collect all submissions from the past 24 hours that require mailing. - // Submissions are excluded if the assignment is hidden in the gradebook. + // Collect all submissions that require mailing. + // Submissions are included if all are true: + // - The assignment is visible in the gradebook. + // - No previous notification has been sent. + // - If marking workflow is not enabled, the grade was updated in the past 24 hours, or + // if marking workflow is enabled, the workflow state is at 'released'. $sql = "SELECT g.id as gradeid, a.course, a.name, a.blindmarking, a.revealidentities, g.*, g.timemodified as lastmodified, cm.id as cmid FROM {assign} a @@ -1653,12 +1657,16 @@ class assign { JOIN {course_modules} cm ON cm.course = a.course AND cm.instance = a.id JOIN {modules} md ON md.id = cm.module AND md.name = 'assign' JOIN {grade_items} gri ON gri.iteminstance = a.id AND gri.courseid = a.course AND gri.itemmodule = md.name - WHERE g.timemodified >= :yesterday AND - g.timemodified <= :today AND + WHERE ((a.markingworkflow = 0 AND g.timemodified >= :yesterday AND g.timemodified <= :today) OR + (a.markingworkflow = 1 AND uf.workflowstate = :wfreleased)) AND uf.mailed = 0 AND gri.hidden = 0 ORDER BY a.course, cm.id"; - $params = array('yesterday' => $yesterday, 'today' => $timenow); + $params = array( + 'yesterday' => $yesterday, + 'today' => $timenow, + 'wfreleased' => ASSIGN_MARKING_WORKFLOW_STATE_RELEASED, + ); $submissions = $DB->get_records_sql($sql, $params); if (!empty($submissions)) { @@ -6081,10 +6089,15 @@ class assign { // Get assignment visibility information for student. $modinfo = get_fast_modinfo($settings->course, $userid); $cm = $modinfo->get_cm($this->get_course_module()->id); - // Don't allow notification to be sent if student can't access assignment. + + // Don't allow notification to be sent if the student can't access the assignment, + // or until in "Released" state if using marking workflow. if (!$cm->uservisible) { $mform->setDefault('sendstudentnotifications', 0); $mform->freeze('sendstudentnotifications'); + } else if ($this->get_instance()->markingworkflow) { + $mform->setDefault('sendstudentnotifications', 0); + $mform->disabledIf('sendstudentnotifications', 'workflowstate', 'neq', ASSIGN_MARKING_WORKFLOW_STATE_RELEASED); } else { $mform->setDefault('sendstudentnotifications', $this->get_instance()->sendstudentnotifications); } @@ -6360,6 +6373,16 @@ class assign { $flags->workflowstate = $state; + // Clear the mailed flag if notification is requested, the student hasn't been + // notified previously, the student can access the assignment, and the state + // is "Released". + $modinfo = get_fast_modinfo($this->course, $userid); + $cm = $modinfo->get_cm($this->get_course_module()->id); + if ($formdata->sendstudentnotifications && $flags->mailed != 1 && + $cm->uservisible && $state == ASSIGN_MARKING_WORKFLOW_STATE_RELEASED) { + $flags->mailed = 0; + } + $gradingdisabled = $this->grading_disabled($userid); // Will not apply update if user does not have permission to assign this workflow state. diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 71ed77faf7c..4cdd77f9182 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -889,6 +889,44 @@ class mod_assign_locallib_testcase extends mod_assign_base_testcase { $this->assertEquals($assign->get_instance()->name, $messages[0]->contexturlname); } + /** + * Test delivery of grade notifications as controlled by marking workflow. + */ + public function test_markingworkflow_cron() { + // First run cron so there are no messages waiting to be sent (from other tests). + cron_setup_user(); + assign::cron(); + + // Now create an assignment with marking workflow enabled. + $this->setUser($this->editingteachers[0]); + $assign = $this->create_instance(array('sendstudentnotifications' => 1, 'markingworkflow' => 1)); + + // Simulate adding a grade. + $this->setUser($this->teachers[0]); + $data = new stdClass(); + $data->grade = '50.0'; + + // This student will not receive notification. + $data->workflowstate = ASSIGN_MARKING_WORKFLOW_STATE_READYFORRELEASE; + $assign->testable_apply_grade_to_user($data, $this->students[0]->id, 0); + + // This student will receive notification. + $data->workflowstate = ASSIGN_MARKING_WORKFLOW_STATE_RELEASED; + $assign->testable_apply_grade_to_user($data, $this->students[1]->id, 0); + + // Now run cron and see that one message was sent. + $this->preventResetByRollback(); + $sink = $this->redirectMessages(); + cron_setup_user(); + $this->expectOutputRegex('/Done processing 1 assignment submissions/'); + assign::cron(); + + $messages = $sink->get_messages(); + $this->assertEquals(1, count($messages)); + $this->assertEquals($messages[0]->useridto, $this->students[1]->id); + $this->assertEquals($assign->get_instance()->name, $messages[0]->contexturlname); + } + public function test_is_graded() { $this->setUser($this->editingteachers[0]); $assign = $this->create_instance();