From 1f55fff3937e0d038c11af481c4e3349675a0ffc Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Mon, 10 Dec 2018 09:27:36 +0000 Subject: [PATCH 1/3] MDL-64336 assign: Submissions should be visible while frozen Before this change if a student visited an assignment that is frozen they would only see the title and description even if they had made a submission to it. After the change they will be able to see the status of their submission and any feedback and grades they have recived. It will also make the Moodle app recognise that submission should not happen because the assignment is frozen. Tests based on ones created by Andrew Nicols --- mod/assign/externallib.php | 5 +++-- mod/assign/locallib.php | 11 ++++++++--- mod/assign/tests/locallib_test.php | 19 +++++++++++++++++++ 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/mod/assign/externallib.php b/mod/assign/externallib.php index 51886299d59..dd022601ad9 100644 --- a/mod/assign/externallib.php +++ b/mod/assign/externallib.php @@ -2375,7 +2375,8 @@ class mod_assign_external extends external_api { } // Retrieve the rest of the renderable objects. - if (has_capability('mod/assign:submit', $assign->get_context(), $user)) { + $cansubmit = has_capability('mod/assign:submit', $context, $user, false); + if ($cansubmit || $assign->get_user_submission($user->id, false) !== false) { $lastattempt = $assign->get_assign_submission_status_renderable($user, true); } @@ -2431,7 +2432,7 @@ class mod_assign_external extends external_api { } // Can edit its own submission? - $lastattempt->caneditowner = $assign->submissions_open($user->id) && $assign->is_any_submission_plugin_enabled(); + $lastattempt->caneditowner = $cansubmit && $assign->submissions_open($user->id) && $assign->is_any_submission_plugin_enabled(); $result['lastattempt'] = $lastattempt; } diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index c86f7b5b0a3..2c56a03e27c 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -4868,7 +4868,7 @@ class assign { if (has_any_capability(array('mod/assign:viewgrades', 'mod/assign:grade'), $this->context)) { return true; } - if ($userid == $USER->id && has_capability('mod/assign:submit', $this->context)) { + if ($userid == $USER->id) { return true; } return false; @@ -5441,8 +5441,9 @@ class assign { $o = ''; if ($this->can_view_submission($user->id)) { - - if (has_capability('mod/assign:submit', $this->get_context(), $user, false)) { + $cansubmit = has_capability('mod/assign:submit', $this->get_context(), $user, false); + if ($cansubmit || $this->get_user_submission($user->id, false) !== false) { + // The user can submit, or has a submission. $submissionstatus = $this->get_assign_submission_status_renderable($user, $showlinks); $o .= $this->get_renderer()->render($submissionstatus); } @@ -5471,6 +5472,10 @@ class assign { * @return bool */ protected function show_submit_button($submission = null, $teamsubmission = null, $userid = null) { + if (!has_capability('mod/assign:submit', $this->get_context(), $userid, false)) { + // The user does not have the capability to submit. + return false; + } if ($teamsubmission) { if ($teamsubmission->status === ASSIGN_SUBMISSION_STATUS_SUBMITTED) { // The assignment submission has been completed. diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 9cd16757fad..7bc534c2a1f 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -2345,6 +2345,25 @@ class mod_assign_locallib_testcase extends advanced_testcase { $output = $assign->view_student_summary($student, true); $this->assertDoesNotMatchRegularExpression('/Feedback/', $output, 'Do not show feedback if the grade is hidden in the gradebook'); + + // Freeze the context. + $this->setAdminUser(); + $context = $assign->get_context(); + $CFG->contextlocking = true; + $context->set_locked(true); + + // No feedback should be available because the grade is hidden. + $this->setUser($student); + $output = $assign->view_student_summary($student, true); + $this->assertDoesNotMatchRegularExpression('/Feedback/', $output, 'Do not show feedback if the grade is hidden in the gradebook'); + + // Show the feedback again - it should still be visible even in a frozen context. + $this->setUser($teacher); + $gradeitem->set_hidden(0, false); + + $this->setUser($student); + $output = $assign->view_student_summary($student, true); + $this->assertMatchesRegularExpression('/Feedback/', $output, 'Show feedback if there is a grade'); } public function test_show_student_summary_with_feedback() { From 06a69802d69dd3f9f7418d24c5483b74964c356b Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Thu, 4 Apr 2019 11:31:49 +0100 Subject: [PATCH 2/3] MDL-64336 assign: Teachers should be able to see all submissions Before this change a teacher would be able to see users listed if: * They have an active enrolment and can submit * They have an an inactive enrolment for a role that can submit After this change they will additonally be able to see users listed: * That have an active enrolment and have submitted * That have an inactive enrolment and have submitted This means that if an assignment has it's context frozen all users that have made some form of submission will still be listed. It will also apply if the submission capability is removed from a user. If a user's enrolment is deleted they will not be listed. The submission and grading counts have also been updated so they will reflect the new rules. --- mod/assign/externallib.php | 3 +- mod/assign/locallib.php | 45 +++++++++++++++-- .../tests/locallib_participants_test.php | 37 ++++++++++++++ mod/assign/tests/locallib_test.php | 50 +++++++++++++++++++ 4 files changed, 129 insertions(+), 6 deletions(-) diff --git a/mod/assign/externallib.php b/mod/assign/externallib.php index dd022601ad9..f6c041b9635 100644 --- a/mod/assign/externallib.php +++ b/mod/assign/externallib.php @@ -2432,7 +2432,8 @@ class mod_assign_external extends external_api { } // Can edit its own submission? - $lastattempt->caneditowner = $cansubmit && $assign->submissions_open($user->id) && $assign->is_any_submission_plugin_enabled(); + $lastattempt->caneditowner = $cansubmit && $assign->submissions_open($user->id) + && $assign->is_any_submission_plugin_enabled(); $result['lastattempt'] = $lastattempt; } diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 2c56a03e27c..5a69747940c 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -2091,6 +2091,33 @@ class assign { return $result; } + /** + * Returns array with sql code and parameters returning all ids of users who have submitted an assignment. + * + * @param int $group The group that the query is for. + * @return array list($sql, $params) + */ + protected function get_submitted_sql($group = 0) { + // We need to guarentee unique table names. + static $i = 0; + $i++; + $prefix = 'sa' . $i . '_'; + $params = [ + "{$prefix}assignment" => (int) $this->get_instance()->id, + "{$prefix}status" => ASSIGN_SUBMISSION_STATUS_NEW, + ]; + $capjoin = get_enrolled_with_capabilities_join($this->context, $prefix, '', $group, $this->show_only_active_users()); + $params += $capjoin->params; + $sql = "SELECT {$prefix}s.userid + FROM {assign_submission} {$prefix}s + JOIN {user} {$prefix}u ON {$prefix}u.id = {$prefix}s.userid + $capjoin->joins + WHERE {$prefix}s.assignment = :{$prefix}assignment + AND {$prefix}s.status <> :{$prefix}status + AND $capjoin->wheres"; + return array($sql, $params); + } + /** * Load a list of users enrolled in the current course with the specified permission and group. * 0 for no group. @@ -2113,6 +2140,8 @@ class assign { if (!isset($this->participants[$key])) { list($esql, $params) = get_enrolled_sql($this->context, 'mod/assign:submit', $currentgroup, $this->show_only_active_users()); + list($ssql, $sparams) = $this->get_submitted_sql($currentgroup); + $params += $sparams; $fields = 'u.*'; $orderby = 'u.lastname, u.firstname, u.id'; @@ -2159,7 +2188,7 @@ class assign { $sql = "SELECT $fields FROM {user} u - JOIN ($esql) je ON je.id = u.id + JOIN ($esql UNION $ssql) je ON je.id = u.id $additionaljoins WHERE u.deleted = 0 $additionalfilters @@ -2217,12 +2246,18 @@ class assign { return null; } - if (!is_enrolled($this->context, $participant, 'mod/assign:submit', $this->show_only_active_users())) { + if (!is_enrolled($this->context, $participant, '', $this->show_only_active_users())) { return null; } $result = $this->get_submission_info_for_participants(array($participant->id => $participant)); - return $result[$participant->id]; + + $submissioninfo = $result[$participant->id]; + if (!$submissioninfo->submitted && !has_capability('mod/assign:submit', $this->context, $userid)) { + return null; + } + + return $submissioninfo; } /** @@ -2311,7 +2346,7 @@ class assign { if ($currentgroup === null) { $currentgroup = groups_get_activity_group($this->get_course_module(), true); } - list($esql, $params) = get_enrolled_sql($this->get_context(), 'mod/assign:submit', $currentgroup, true); + list($esql, $params) = get_enrolled_sql($this->get_context(), '', $currentgroup, true); $params['assignid'] = $this->get_instance()->id; $params['submitted'] = ASSIGN_SUBMISSION_STATUS_SUBMITTED; @@ -2426,7 +2461,7 @@ class assign { if ($currentgroup === null) { $currentgroup = groups_get_activity_group($this->get_course_module(), true); } - list($esql, $params) = get_enrolled_sql($this->get_context(), 'mod/assign:submit', $currentgroup, true); + list($esql, $params) = get_enrolled_sql($this->get_context(), '', $currentgroup, true); $params['assignid'] = $this->get_instance()->id; $params['assignid2'] = $this->get_instance()->id; diff --git a/mod/assign/tests/locallib_participants_test.php b/mod/assign/tests/locallib_participants_test.php index 7121383bd7a..84ee80a1b7e 100644 --- a/mod/assign/tests/locallib_participants_test.php +++ b/mod/assign/tests/locallib_participants_test.php @@ -28,8 +28,11 @@ defined('MOODLE_INTERNAL') || die(); global $CFG; require_once(__DIR__ . '/../locallib.php'); +require_once($CFG->dirroot . '/mod/assign/tests/generator.php'); class mod_assign_locallib_participants extends advanced_testcase { + use mod_assign_test_generator; + public function test_list_participants_blind_marking() { global $DB; $this->resetAfterTest(true); @@ -118,6 +121,40 @@ class mod_assign_locallib_participants extends advanced_testcase { $this->assertEquals($newkeys, array_keys($table->rawdata)); } + /** + * Tests that users who have a submission, but can no longer submit are listed. + */ + public function test_list_participants_can_no_longer_submit() { + global $DB; + $this->resetAfterTest(true); + // Create a role that will prevent users submitting. + $role = self::getDataGenerator()->create_role(); + assign_capability('mod/assign:submit', CAP_PROHIBIT, $role, context_system::instance()); + // Create the test data. + $course = self::getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + $assign = $this->create_instance($course); + self::getDataGenerator()->create_and_enrol($course, 'teacher'); + $student1 = self::getDataGenerator()->create_and_enrol($course, 'student'); + $student2 = self::getDataGenerator()->create_and_enrol($course, 'student'); + $cannotsubmit1 = self::getDataGenerator()->create_and_enrol($course, 'student'); + $cannotsubmit2 = self::getDataGenerator()->create_and_enrol($course, 'student'); + // Create submissions for some users. + $this->add_submission($student1, $assign); + $this->submit_for_grading($student1, $assign); + $this->add_submission($cannotsubmit1, $assign); + $this->submit_for_grading($cannotsubmit1, $assign); + // Remove the capability to submit from some users. + role_assign($role, $cannotsubmit1->id, $coursecontext); + role_assign($role, $cannotsubmit2->id, $coursecontext); + // Everything is setup for the test now. + $participants = $assign->list_participants(null, true); + self::assertCount(3, $participants); + self::assertArrayHasKey($student1->id, $participants); + self::assertArrayHasKey($student2->id, $participants); + self::assertArrayHasKey($cannotsubmit1->id, $participants); + } + public function helper_add_submission($assign, $user, $data, $type) { global $USER; diff --git a/mod/assign/tests/locallib_test.php b/mod/assign/tests/locallib_test.php index 7bc534c2a1f..a661b9376b2 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -1065,6 +1065,56 @@ class mod_assign_locallib_testcase extends advanced_testcase { $this->assertFalse($participant->grantedextension); } + /** + * Tests that if a student with no submission who can no longer submit is not a participant. + */ + public function test_get_participant_with_no_submission_no_capability() { + global $DB; + $this->resetAfterTest(); + $course = self::getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + $assign = $this->create_instance($course); + $teacher = self::getDataGenerator()->create_and_enrol($course, 'teacher'); + $student = self::getDataGenerator()->create_and_enrol($course, 'student'); + + // Remove the students capability to submit. + $role = $DB->get_field('role', 'id', ['shortname' => 'student']); + assign_capability('mod/assign:submit', CAP_PROHIBIT, $role, $coursecontext); + + $participant = $assign->get_participant($student->id); + + self::assertNull($participant); + } + + /** + * Tests that if a student that has submitted but can no longer submit is a participant. + */ + public function test_get_participant_with_submission_no_capability() { + global $DB; + $this->resetAfterTest(); + $course = self::getDataGenerator()->create_course(); + $coursecontext = context_course::instance($course->id); + $assign = $this->create_instance($course); + $teacher = self::getDataGenerator()->create_and_enrol($course, 'teacher'); + $student = self::getDataGenerator()->create_and_enrol($course, 'student'); + + // Simulate a submission. + $this->add_submission($student, $assign); + $this->submit_for_grading($student, $assign); + + // Remove the students capability to submit. + $role = $DB->get_field('role', 'id', ['shortname' => 'student']); + assign_capability('mod/assign:submit', CAP_PROHIBIT, $role, $coursecontext); + + $participant = $assign->get_participant($student->id); + + self::assertNotNull($participant); + self::assertEquals($student->id, $participant->id); + self::assertTrue($participant->submitted); + self::assertTrue($participant->requiregrading); + self::assertFalse($participant->grantedextension); + } + public function test_get_participant_with_graded_submission() { $this->resetAfterTest(); $course = $this->getDataGenerator()->create_course(); From bce4c28577d11d5ab56c00132a74b4a210808916 Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Thu, 4 Apr 2019 11:33:31 +0100 Subject: [PATCH 3/3] MDL-64336 assign: Add missing PHP doc --- mod/assign/locallib.php | 1 + 1 file changed, 1 insertion(+) diff --git a/mod/assign/locallib.php b/mod/assign/locallib.php index 5a69747940c..5fdb19d6760 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -2125,6 +2125,7 @@ class assign { * * @param int $currentgroup * @param bool $idsonly + * @param bool $tablesort * @return array List of user records */ public function list_participants($currentgroup, $idsonly, $tablesort = false) {