From 99b3ce966929abea7ac00f515e3814c6a2312d47 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Tue, 3 Mar 2020 18:30:06 +0100 Subject: [PATCH] MDL-56287 gradereport_history: Separate groups queries improved --- grade/report/history/classes/helper.php | 28 ++++--- .../history/classes/output/tablelog.php | 33 +++----- grade/report/history/index.php | 14 ---- grade/report/history/tests/report_test.php | 76 +++++++++++++++---- 4 files changed, 92 insertions(+), 59 deletions(-) diff --git a/grade/report/history/classes/helper.php b/grade/report/history/classes/helper.php index 0a50fb80d55..00f4c6d429f 100644 --- a/grade/report/history/classes/helper.php +++ b/grade/report/history/classes/helper.php @@ -152,21 +152,16 @@ class helper { $groupjoinsql = ''; $groupwheresql = ''; $courseid = $context->instanceid; - $course = $DB->get_record('course', ['id' => $courseid]); - $groupmode = groups_get_course_groupmode($course); + $groupmode = groups_get_course_groupmode(get_course($courseid)); // We're only interested in separate groups mode because it's the only group mode that requires the user to be a member of // specific group(s), except when they have the 'moodle/site:accessallgroups' capability. if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $context)) { // Fetch the groups that the user can see. $groups = groups_get_all_groups($courseid, $USER->id, 0, 'g.id'); - if (empty($groups)) { - // The user's not in any group and they don't have the capability to access all groups. So throw an error. - throw new \moodle_exception('notingroup'); - } // Add join condition to include users that only belong to the same group as the user. - list($insql, $inparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED, 'gid'); + list($insql, $inparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED, 'gid', true, 0); $groupjoinsql = " JOIN {groups_members} gm ON gm.userid = u.id "; $groupwheresql = " AND gm.groupid $insql "; $params = array_merge($params, $inparams); @@ -192,18 +187,31 @@ class helper { * @return array list of graders. */ public static function get_graders($courseid) { - global $DB; + global $DB, $USER; + + $groupjoinsql = $groupwheresql = ''; + $inparams = []; + $groupmode = groups_get_course_groupmode(get_course($courseid)); + if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', \context_course::instance($courseid))) { + // Fetch the groups that the user can see. + $groups = groups_get_all_groups($courseid, $USER->id, 0, 'g.id'); + // Add join condition to include users that only belong to the same group as the user. + list($insql, $inparams) = $DB->get_in_or_equal(array_keys($groups), SQL_PARAMS_NAMED, 'gid', true, 0); + $groupjoinsql = " JOIN {groups_members} gm ON gm.userid = u.id "; + $groupwheresql = " AND gm.groupid $insql "; + } $ufields = get_all_user_name_fields(true, 'u'); $sql = "SELECT u.id, $ufields FROM {user} u JOIN {grade_grades_history} ggh ON ggh.usermodified = u.id JOIN {grade_items} gi ON gi.id = ggh.itemid - WHERE gi.courseid = :courseid + $groupjoinsql + WHERE gi.courseid = :courseid $groupwheresql GROUP BY u.id, $ufields ORDER BY u.lastname ASC, u.firstname ASC"; - $graders = $DB->get_records_sql($sql, array('courseid' => $courseid)); + $graders = $DB->get_records_sql($sql, array('courseid' => $courseid) + $inparams); $return = array(0 => get_string('allgraders', 'gradereport_history')); foreach ($graders as $grader) { $return[$grader->id] = fullname($grader); diff --git a/grade/report/history/classes/output/tablelog.php b/grade/report/history/classes/output/tablelog.php index 9ff4748e748..5cc89df88f1 100644 --- a/grade/report/history/classes/output/tablelog.php +++ b/grade/report/history/classes/output/tablelog.php @@ -63,9 +63,6 @@ class tablelog extends \table_sql implements \renderable { */ protected $cms; - /** @var array Only fetch users belonging to these groups if defined. */ - protected $groups; - /** * @var int The default number of decimal points to use in this course * when a grade item does not itself define the number of decimal points. @@ -102,8 +99,6 @@ class tablelog extends \table_sql implements \renderable { $this->courseid = $this->context->instanceid; $this->pagesize = $perpage; $this->page = $page; - $this->groups = $filters['groups']; - unset($filters['groups']); $this->filters = (object)$filters; $this->gradeitems = \grade_item::fetch_all(array('courseid' => $this->courseid)); $this->cms = get_fast_modinfo($this->courseid); @@ -342,7 +337,7 @@ class tablelog extends \table_sql implements \renderable { * @return array containing sql to use and an array of params. */ protected function get_filters_sql_and_params() { - global $DB; + global $DB, $USER; $coursecontext = $this->context; $filter = 'gi.courseid = :courseid'; @@ -373,6 +368,16 @@ class tablelog extends \table_sql implements \renderable { $params += array('grader' => $this->filters->grader); } + // If the course is separate group mode and the current user is not allowed to see all groups make sure + // that we display only users from the same groups as current user. + $groupmode = get_course($coursecontext->instanceid)->groupmode; + if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $coursecontext)) { + $groupids = array_column(groups_get_all_groups($coursecontext->instanceid, $USER->id, 0, 'g.id'), 'id'); + list($gsql, $gparams) = $DB->get_in_or_equal($groupids, SQL_PARAMS_NAMED, 'gmuparam', true, 0); + $filter .= " AND EXISTS (SELECT 1 FROM {groups_members} gmu WHERE gmu.userid=ggh.userid AND gmu.groupid $gsql)"; + $params += $gparams; + } + return array($filter, $params); } @@ -384,8 +389,6 @@ class tablelog extends \table_sql implements \renderable { * @return array containing sql to use and an array of params. */ protected function get_sql_and_params($count = false) { - global $DB; - $fields = 'ggh.id, ggh.timemodified, ggh.itemid, ggh.userid, ggh.finalgrade, ggh.usermodified, ggh.source, ggh.overridden, ggh.locked, ggh.excluded, ggh.feedback, ggh.feedbackformat, gi.itemtype, gi.itemmodule, gi.iteminstance, gi.itemnumber, '; @@ -431,24 +434,12 @@ class tablelog extends \table_sql implements \renderable { list($where, $params) = $this->get_filters_sql_and_params(); - $groupjoinsql = ''; - $groupwheresql = ''; - // Generate group filters when necessary. - if ($this->groups) { - // Add join condition to include users that only belong to the same group as the user. - list($insql, $inparams) = $DB->get_in_or_equal(array_keys($this->groups), SQL_PARAMS_NAMED, 'gid'); - $groupjoinsql = " JOIN {groups_members} gm ON gm.userid = u.id "; - $groupwheresql = " AND gm.groupid $insql "; - $params = array_merge($params, $inparams); - } - $sql = "SELECT $select FROM {grade_grades_history} ggh JOIN {grade_items} gi ON gi.id = ggh.itemid JOIN {user} u ON u.id = ggh.userid - $groupjoinsql LEFT JOIN {user} ug ON ug.id = ggh.usermodified - WHERE $where $groupwheresql"; + WHERE $where"; // As prevgrade is a dynamic field, we need to wrap the query. This is the only filtering // that should be defined outside the method self::get_filters_sql_and_params(). diff --git a/grade/report/history/index.php b/grade/report/history/index.php index d58bb64d26c..127e106aadf 100644 --- a/grade/report/history/index.php +++ b/grade/report/history/index.php @@ -43,19 +43,6 @@ $context = context_course::instance($course->id); require_capability('gradereport/history:view', $context); require_capability('moodle/grade:viewall', $context); -$groupmode = groups_get_course_groupmode($course); -$groups = []; -// We're only interested in separate groups mode because it's the only group mode that requires the user to be a member of -// specific group(s), except when they have the 'moodle/site:accessallgroups' capability. -if ($groupmode == SEPARATEGROUPS && !has_capability('moodle/site:accessallgroups', $context)) { - // Fetch the groups that the user can see. - $groups = groups_get_all_groups($courseid, $USER->id, 0, 'g.id'); - if (empty($groups)) { - // The user's not in any group and they don't have the capability to access all groups. So throw an error. - throw new \moodle_exception('notingroup'); - } -} - // Last selected report session tracking. if (!isset($USER->grade_last_report)) { $USER->grade_last_report = array(); @@ -91,7 +78,6 @@ if ($data = $mform->get_data()) { 'revisedonly' => optional_param('revisedonly', 0, PARAM_INT), ); } -$filters['groups'] = $groups; $table = new \gradereport_history\output\tablelog('gradereport_history', $context, $url, $filters, $download, $page); diff --git a/grade/report/history/tests/report_test.php b/grade/report/history/tests/report_test.php index 4775e769e62..7a58ae1363e 100644 --- a/grade/report/history/tests/report_test.php +++ b/grade/report/history/tests/report_test.php @@ -53,6 +53,12 @@ class gradereport_history_report_testcase extends advanced_testcase { $u5 = $this->getDataGenerator()->create_user(); $grader1 = $this->getDataGenerator()->create_user(); $grader2 = $this->getDataGenerator()->create_user(); + self::getDataGenerator()->enrol_user($grader1->id, $c1->id, 'teacher'); + self::getDataGenerator()->enrol_user($grader2->id, $c1->id, 'teacher'); + self::getDataGenerator()->enrol_user($u2->id, $c1->id, 'student'); + self::getDataGenerator()->enrol_user($u3->id, $c1->id, 'student'); + self::getDataGenerator()->enrol_user($u4->id, $c1->id, 'student'); + self::getDataGenerator()->enrol_user($u5->id, $c1->id, 'student'); // Modules. $c1m1 = $this->getDataGenerator()->create_module('assign', array('course' => $c1)); @@ -65,6 +71,8 @@ class gradereport_history_report_testcase extends advanced_testcase { $giparams = array('itemtype' => 'mod', 'itemmodule' => 'assign'); $grades = array(); + $this->setUser($grader1); + $gi = grade_item::fetch($giparams + array('iteminstance' => $c1m1->id)); $grades['c1m1u1'] = $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u1->id, 'timemodified' => time() - 3600)); @@ -163,6 +171,20 @@ class gradereport_history_report_testcase extends advanced_testcase { $this->assertEquals(null, $results[$grades['c2m2u1a']->id]->prevgrade); $this->assertEquals($grades['c2m2u1a']->finalgrade, $results[$grades['c2m2u1c']->id]->prevgrade); $this->assertEquals($grades['c2m2u1c']->finalgrade, $results[$grades['c2m2u1e']->id]->prevgrade); + + // Put course in separate groups mode, add grader1 and two students to the same group. + $c1->groupmode = SEPARATEGROUPS; + update_course($c1); + $this->assertFalse(has_capability('moodle/site:accessallgroups', \context_course::instance($c1->id))); + $g1 = self::getDataGenerator()->create_group(['courseid' => $c1->id, 'name' => 'g1']); + self::getDataGenerator()->create_group_member(['groupid' => $g1->id, 'userid' => $grader1->id]); + self::getDataGenerator()->create_group_member(['groupid' => $g1->id, 'userid' => $u1->id]); + self::getDataGenerator()->create_group_member(['groupid' => $g1->id, 'userid' => $u2->id]); + $this->assertEquals(2, $this->get_tablelog_results($c1ctx, array(), true)); + + // Grader2 is not in any groups. + $this->setUser($grader2); + $this->assertEquals(0, $this->get_tablelog_results($c1ctx, array(), true)); } /** @@ -225,22 +247,25 @@ class gradereport_history_report_testcase extends advanced_testcase { public function get_users_provider() { return [ 'Visible groups, non-editing teacher, not in any group' => [ - VISIBLEGROUPS, 'teacher', ['g1', 'g2'], false, ['s1', 's2', 's3', 's4'] + VISIBLEGROUPS, 'teacher', ['g1', 'g2'], ['s1', 's2', 's3', 's4', 's5'] ], 'Visible groups, non-editing teacher' => [ - VISIBLEGROUPS, 'teacher', [], false, ['s1', 's2', 's3', 's4'] + VISIBLEGROUPS, 'teacher', [], ['s1', 's2', 's3', 's4', 's5'] ], 'Visible groups, editing teacher' => [ - VISIBLEGROUPS, 'editingteacher', ['g1', 'g2'], false, ['s1', 's2', 's3', 's4'] + VISIBLEGROUPS, 'editingteacher', ['g1', 'g2'], ['s1', 's2', 's3', 's4', 's5'] ], 'Separate groups, non-editing teacher' => [ - SEPARATEGROUPS, 'teacher', ['g1', 'g2'], false, ['s1', 's2'] + SEPARATEGROUPS, 'teacher', ['g1', 'g2'], ['s1', 's2'] ], 'Separate groups, non-editing teacher, not in any group' => [ - SEPARATEGROUPS, 'teacher', [], true, [] + SEPARATEGROUPS, 'teacher', [], [] + ], + 'Separate groups, non-editing teacher and student share two groups' => [ + SEPARATEGROUPS, 'teacher', ['g4', 'g5'], ['s5'] ], 'Separate groups, editing teacher' => [ - SEPARATEGROUPS, 'editingteacher', ['g1', 'g2'], false, ['s1', 's2', 's3', 's4'] + SEPARATEGROUPS, 'editingteacher', ['g1', 'g2'], ['s1', 's2', 's3', 's4', 's5'] ], ]; } @@ -252,10 +277,9 @@ class gradereport_history_report_testcase extends advanced_testcase { * @param $groupmode * @param $teacherrole * @param $teachergroups - * @param $exceptionexpected * @param $expectedusers */ - public function test_get_users_with_groups($groupmode, $teacherrole, $teachergroups, $exceptionexpected, $expectedusers) { + public function test_get_users_with_groups($groupmode, $teacherrole, $teachergroups, $expectedusers) { global $DB; $this->resetAfterTest(); @@ -277,6 +301,7 @@ class gradereport_history_report_testcase extends advanced_testcase { $s2 = $generator->create_user(['username' => 's2', 'email' => 's2@example.com']); $s3 = $generator->create_user(['username' => 's3', 'email' => 's3@example.com']); $s4 = $generator->create_user(['username' => 's4', 'email' => 's4@example.com']); + $s5 = $generator->create_user(['username' => 's5', 'email' => 's5@example.com']); // Enrol users. $generator->enrol_user($t1->id, $course->id, $role->id); @@ -284,12 +309,15 @@ class gradereport_history_report_testcase extends advanced_testcase { $generator->enrol_user($s2->id, $course->id, $studentrole->id); $generator->enrol_user($s3->id, $course->id, $studentrole->id); $generator->enrol_user($s4->id, $course->id, $studentrole->id); + $generator->enrol_user($s5->id, $course->id, $studentrole->id); // Create groups. $groups = []; $groups['g1'] = $generator->create_group(['courseid' => $course->id, 'name' => 'g1']); $groups['g2'] = $generator->create_group(['courseid' => $course->id, 'name' => 'g2']); $groups['g3'] = $generator->create_group(['courseid' => $course->id, 'name' => 'g3']); + $groups['g4'] = $generator->create_group(['courseid' => $course->id, 'name' => 'g4']); + $groups['g5'] = $generator->create_group(['courseid' => $course->id, 'name' => 'g5']); // Add teacher to the assigned groups. foreach ($teachergroups as $groupname) { @@ -301,6 +329,8 @@ class gradereport_history_report_testcase extends advanced_testcase { $generator->create_group_member(['groupid' => $groups['g1']->id, 'userid' => $s1->id]); $generator->create_group_member(['groupid' => $groups['g2']->id, 'userid' => $s2->id]); $generator->create_group_member(['groupid' => $groups['g3']->id, 'userid' => $s3->id]); + $generator->create_group_member(['groupid' => $groups['g4']->id, 'userid' => $s5->id]); + $generator->create_group_member(['groupid' => $groups['g5']->id, 'userid' => $s5->id]); // Creating grade history for the students. $gi = grade_item::fetch(['iteminstance' => $assign->id, 'itemtype' => 'mod', 'itemmodule' => 'assign']); @@ -308,16 +338,11 @@ class gradereport_history_report_testcase extends advanced_testcase { $this->create_grade_history(['itemid' => $gi->id, 'userid' => $s2->id]); $this->create_grade_history(['itemid' => $gi->id, 'userid' => $s3->id]); $this->create_grade_history(['itemid' => $gi->id, 'userid' => $s4->id]); + $this->create_grade_history(['itemid' => $gi->id, 'userid' => $s5->id]); // Log in as the teacher. $this->setUser($t1); - // Declare when we expect to get an exception. - if ($exceptionexpected) { - $this->expectException(moodle_exception::class); - $this->expectExceptionMessage('notingroup'); - } - // Fetch the users. $users = \gradereport_history\helper::get_users(context_course::instance($course->id)); // Confirm that the number of users fetched is the same as the count of expected users. @@ -337,9 +362,11 @@ class gradereport_history_report_testcase extends advanced_testcase { // Making the setup. $c1 = $this->getDataGenerator()->create_course(); $c2 = $this->getDataGenerator()->create_course(); + $c3 = $this->getDataGenerator()->create_course(['groupmode' => SEPARATEGROUPS]); $c1m1 = $this->getDataGenerator()->create_module('assign', array('course' => $c1)); $c2m1 = $this->getDataGenerator()->create_module('assign', array('course' => $c2)); + $c3m1 = $this->getDataGenerator()->create_module('assign', array('course' => $c3)); // Users. $u1 = $this->getDataGenerator()->create_user(array('firstname' => 'Eric', 'lastname' => 'Cartman')); @@ -347,6 +374,12 @@ class gradereport_history_report_testcase extends advanced_testcase { $u3 = $this->getDataGenerator()->create_user(array('firstname' => 'Kyle', 'lastname' => 'Broflovski')); $u4 = $this->getDataGenerator()->create_user(array('firstname' => 'Kenny', 'lastname' => 'McCormick')); + foreach ([$c1, $c2, $c3] as $course) { + foreach ([$u1, $u2, $u3, $u4] as $user) { + self::getDataGenerator()->enrol_user($user->id, $course->id, 'student'); + } + } + // Creating grade history for some users. $gi = grade_item::fetch(array('iteminstance' => $c1m1->id, 'itemtype' => 'mod', 'itemmodule' => 'assign')); $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u1->id, 'usermodified' => $u1->id)); @@ -356,6 +389,10 @@ class gradereport_history_report_testcase extends advanced_testcase { $gi = grade_item::fetch(array('iteminstance' => $c2m1->id, 'itemtype' => 'mod', 'itemmodule' => 'assign')); $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u1->id, 'usermodified' => $u4->id)); + $gi = grade_item::fetch(array('iteminstance' => $c3m1->id, 'itemtype' => 'mod', 'itemmodule' => 'assign')); + $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u1->id, 'usermodified' => $u1->id)); + $this->create_grade_history(array('itemid' => $gi->id, 'userid' => $u2->id, 'usermodified' => $u2->id)); + // Checking fetching some users. $graders = \gradereport_history\helper::get_graders($c1->id); $this->assertCount(4, $graders); // Including "all graders" . @@ -365,6 +402,17 @@ class gradereport_history_report_testcase extends advanced_testcase { $graders = \gradereport_history\helper::get_graders($c2->id); $this->assertCount(2, $graders); // Including "all graders" . $this->assertArrayHasKey($u4->id, $graders); + + // Third course is in separate groups mode. Only graders from the same group will be returned. + $g = self::getDataGenerator()->create_group(['courseid' => $course->id, 'name' => 'g1']); + self::getDataGenerator()->create_group_member(['groupid' => $g->id, 'userid' => $u1->id]); + self::getDataGenerator()->create_group_member(['groupid' => $g->id, 'userid' => $u2->id]); + $this->setUser($u1); + $graders = \gradereport_history\helper::get_graders($c3->id); + $this->assertCount(3, $graders); // Including "all graders" . + $this->setUser($u3); + $graders = \gradereport_history\helper::get_graders($c3->id); + $this->assertCount(1, $graders); // Including "all graders" . } /**