MDL-56287 gradereport_history: Separate groups queries improved

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(get_course($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');
+            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);

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().

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);
 

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" .
     }
 
     /**