From ec7ce8f2049f62937fc32c0611fe64fcb1c8ef4e Mon Sep 17 00:00:00 2001 From: Jun Pataleta Date: Thu, 27 Feb 2020 16:48:32 +0800 Subject: [PATCH] MDL-56287 gradereport_history: Show users from groups that can be viewed --- grade/report/history/classes/helper.php | 30 ++++- .../history/classes/output/tablelog.php | 21 +++- grade/report/history/index.php | 14 +++ grade/report/history/tests/report_test.php | 109 ++++++++++++++++++ 4 files changed, 171 insertions(+), 3 deletions(-) diff --git a/grade/report/history/classes/helper.php b/grade/report/history/classes/helper.php index b0a3ff95862..0a50fb80d55 100644 --- a/grade/report/history/classes/helper.php +++ b/grade/report/history/classes/helper.php @@ -128,6 +128,7 @@ class helper { * @return array sql and params list */ protected static function get_users_sql_and_params($context, $search = '', $count = false) { + global $DB, $USER; // Fields we need from the user table. $extrafields = get_extra_user_fields($context); @@ -147,13 +148,38 @@ class helper { $select = "SELECT DISTINCT $ufields "; $orderby = " ORDER BY u.lastname ASC, u.firstname ASC"; } + + $groupjoinsql = ''; + $groupwheresql = ''; + $courseid = $context->instanceid; + $course = $DB->get_record('course', ['id' => $courseid]); + $groupmode = groups_get_course_groupmode($course); + + // 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'); + $groupjoinsql = " JOIN {groups_members} gm ON gm.userid = u.id "; + $groupwheresql = " AND gm.groupid $insql "; + $params = array_merge($params, $inparams); + } + $sql = "$select FROM {user} u JOIN {grade_grades_history} ggh ON u.id = ggh.userid JOIN {grade_items} gi ON gi.id = ggh.itemid - WHERE $filtersql gi.courseid = :courseid"; + $groupjoinsql + WHERE $filtersql gi.courseid = :courseid $groupwheresql"; $sql .= $orderby; - $params['courseid'] = $context->instanceid; + $params['courseid'] = $courseid; return array($sql, $params); } diff --git a/grade/report/history/classes/output/tablelog.php b/grade/report/history/classes/output/tablelog.php index 4d9c9fcaf01..9ff4748e748 100644 --- a/grade/report/history/classes/output/tablelog.php +++ b/grade/report/history/classes/output/tablelog.php @@ -63,6 +63,9 @@ 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. @@ -99,6 +102,8 @@ 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); @@ -379,6 +384,8 @@ 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, '; @@ -424,12 +431,24 @@ 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"; + WHERE $where $groupwheresql"; // 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 127e106aadf..d58bb64d26c 100644 --- a/grade/report/history/index.php +++ b/grade/report/history/index.php @@ -43,6 +43,19 @@ $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(); @@ -78,6 +91,7 @@ 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 7a3959d2a4a..4775e769e62 100644 --- a/grade/report/history/tests/report_test.php +++ b/grade/report/history/tests/report_test.php @@ -219,6 +219,115 @@ class gradereport_history_report_testcase extends advanced_testcase { $this->assertEquals(1, \gradereport_history\helper::get_users_count($c1ctx, 'c')); } + /** + * Data provider method for \gradereport_history_report_testcase::test_get_users_with_groups() + */ + public function get_users_provider() { + return [ + 'Visible groups, non-editing teacher, not in any group' => [ + VISIBLEGROUPS, 'teacher', ['g1', 'g2'], false, ['s1', 's2', 's3', 's4'] + ], + 'Visible groups, non-editing teacher' => [ + VISIBLEGROUPS, 'teacher', [], false, ['s1', 's2', 's3', 's4'] + ], + 'Visible groups, editing teacher' => [ + VISIBLEGROUPS, 'editingteacher', ['g1', 'g2'], false, ['s1', 's2', 's3', 's4'] + ], + 'Separate groups, non-editing teacher' => [ + SEPARATEGROUPS, 'teacher', ['g1', 'g2'], false, ['s1', 's2'] + ], + 'Separate groups, non-editing teacher, not in any group' => [ + SEPARATEGROUPS, 'teacher', [], true, [] + ], + 'Separate groups, editing teacher' => [ + SEPARATEGROUPS, 'editingteacher', ['g1', 'g2'], false, ['s1', 's2', 's3', 's4'] + ], + ]; + } + + /** + * Test for helper::get_users() with course group mode set. + * + * @dataProvider get_users_provider + * @param $groupmode + * @param $teacherrole + * @param $teachergroups + * @param $exceptionexpected + * @param $expectedusers + */ + public function test_get_users_with_groups($groupmode, $teacherrole, $teachergroups, $exceptionexpected, $expectedusers) { + global $DB; + $this->resetAfterTest(); + + $generator = $this->getDataGenerator(); + + // Create a test course. + $course = $generator->create_course(['groupmode' => $groupmode]); + + // Create an assignment module. + $assign = $generator->create_module('assign', ['course' => $course]); + + // Fetch roles. + $role = $DB->get_record('role', ['shortname' => $teacherrole], '*', MUST_EXIST); + $studentrole = $DB->get_record('role', ['shortname' => 'student'], '*', MUST_EXIST); + + // Create users. + $t1 = $generator->create_user(['username' => 't1', 'email' => 't1@example.com']); + $s1 = $generator->create_user(['username' => 's1', 'email' => 's1@example.com']); + $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']); + + // Enrol users. + $generator->enrol_user($t1->id, $course->id, $role->id); + $generator->enrol_user($s1->id, $course->id, $studentrole->id); + $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); + + // 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']); + + // Add teacher to the assigned groups. + foreach ($teachergroups as $groupname) { + $group = $groups[$groupname]; + $generator->create_group_member(['groupid' => $group->id, 'userid' => $t1->id]); + } + + // Add students to groups. + $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]); + + // Creating grade history for the students. + $gi = grade_item::fetch(['iteminstance' => $assign->id, 'itemtype' => 'mod', 'itemmodule' => 'assign']); + $this->create_grade_history(['itemid' => $gi->id, 'userid' => $s1->id]); + $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]); + + // 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. + $this->assertCount(count($expectedusers), $users); + foreach ($users as $user) { + // Confirm that each user returned is in the list of expected users. + $this->assertTrue(in_array($user->username, $expectedusers)); + } + } + /** * Test the get graders helper method. */