From 808a335c906eaa8443afebd7cdf467c2cebe4e2d Mon Sep 17 00:00:00 2001 From: Neill Magill Date: Thu, 4 Apr 2019 11:31:49 +0100 Subject: [PATCH] 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 c2290d46982..68ae50f256e 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 4292a4cfce4..4308e94f03c 100644 --- a/mod/assign/locallib.php +++ b/mod/assign/locallib.php @@ -2090,6 +2090,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. @@ -2112,6 +2139,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'; @@ -2158,7 +2187,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 @@ -2216,12 +2245,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; } /** @@ -2310,7 +2345,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; @@ -2425,7 +2460,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 0e4713174de..e27894ec591 100644 --- a/mod/assign/tests/locallib_test.php +++ b/mod/assign/tests/locallib_test.php @@ -1054,6 +1054,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();