From 74a582c502ec715fda3f4589dc90960b2e6528e9 Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Thu, 18 Apr 2019 15:33:16 +0200 Subject: [PATCH] MDL-65381 grade_report: fix empty separate groups own grades access On separate groups' courses, users without groups could not access their own grade items. --- grade/report/user/classes/external/user.php | 28 +++++---- grade/report/user/tests/externallib_test.php | 62 +++++++++++++++----- 2 files changed, 64 insertions(+), 26 deletions(-) diff --git a/grade/report/user/classes/external/user.php b/grade/report/user/classes/external/user.php index f5281a8b658..c29c999c69b 100644 --- a/grade/report/user/classes/external/user.php +++ b/grade/report/user/classes/external/user.php @@ -88,7 +88,8 @@ class user extends external_api { core_user::require_active_user($user); // Check if we can view the user group (if any). // When userid == 0, we are retrieving all the users, we'll check then if a groupid is required. - if (!groups_user_groups_visible($course, $user->id)) { + // User are always in their own group, also when they don't have groups. + if ($userid != $USER->id && !groups_user_groups_visible($course, $user->id)) { throw new moodle_exception('notingroup'); } } @@ -107,21 +108,24 @@ class user extends external_api { throw new moodle_exception('nopermissiontoviewgrades', 'error'); } - if (!empty($groupid)) { - // Determine is the group is visible to user. - if (!groups_group_visible($groupid, $course)) { - throw new moodle_exception('notingroup'); - } - } else { - // Check to see if groups are being used here. - if ($groupmode = groups_get_course_groupmode($course)) { - $groupid = groups_get_course_group($course); - // Determine is the group is visible to user (this is particullary for the group 0). + // User are always in their own group, also when they don't have groups. + if ($userid != $USER->id) { + if (!empty($groupid)) { + // Determine if the group is visible to user. if (!groups_group_visible($groupid, $course)) { throw new moodle_exception('notingroup'); } } else { - $groupid = 0; + // Check to see if groups are being used here. + if ($groupmode = groups_get_course_groupmode($course)) { + $groupid = groups_get_course_group($course); + // Determine if the group is visible to the user (this is particularly for group 0). + if (!groups_group_visible($groupid, $course)) { + throw new moodle_exception('notingroup'); + } + } else { + $groupid = 0; + } } } diff --git a/grade/report/user/tests/externallib_test.php b/grade/report/user/tests/externallib_test.php index 698a4308829..0c82e6bc10d 100644 --- a/grade/report/user/tests/externallib_test.php +++ b/grade/report/user/tests/externallib_test.php @@ -42,7 +42,7 @@ class externallib_test extends externallib_advanced_testcase { * @param int $s2grade Student 2 grade * @return array Course and users instances */ - private function load_data(int $s1grade, int $s2grade): array { + private function load_data(int $s1grade, int $s2grade, int $s3grade): array { global $DB; $course = $this->getDataGenerator()->create_course(['groupmode' => SEPARATEGROUPS, 'groupmodeforce' => 1]); @@ -54,6 +54,10 @@ class externallib_test extends externallib_advanced_testcase { $student2 = $this->getDataGenerator()->create_user(); $this->getDataGenerator()->enrol_user($student2->id, $course->id, $studentrole->id); + // Student 3 is in no groups. + $student3 = $this->getDataGenerator()->create_user(); + $this->getDataGenerator()->enrol_user($student3->id, $course->id, $studentrole->id); + $teacherrole = $DB->get_record('role', ['shortname' => 'teacher']); $teacher = $this->getDataGenerator()->create_user(); $this->getDataGenerator()->enrol_user($teacher->id, $course->id, $teacherrole->id); @@ -74,7 +78,8 @@ class externallib_test extends externallib_advanced_testcase { $student1grade = ['userid' => $student1->id, 'rawgrade' => $s1grade, 'idnumber' => 'testidnumber1']; $student2grade = ['userid' => $student2->id, 'rawgrade' => $s2grade, 'idnumber' => 'testidnumber2']; - $studentgrades = [$student1->id => $student1grade, $student2->id => $student2grade]; + $student3grade = ['userid' => $student3->id, 'rawgrade' => $s3grade, 'idnumber' => 'testidnumber3']; + $studentgrades = [$student1->id => $student1grade, $student2->id => $student2grade, $student3->id => $student3grade]; assign_grade_item_update($assignment, $studentgrades); return [ @@ -82,6 +87,7 @@ class externallib_test extends externallib_advanced_testcase { $teacher, $student1, $student2, + $student3, $assignment ]; } @@ -95,8 +101,9 @@ class externallib_test extends externallib_advanced_testcase { $s1grade = 80; $s2grade = 60; + $s3grade = 50; - list($course, $teacher, $student1, $student2, $assignment) = $this->load_data($s1grade, $s2grade); + list($course, $teacher, $student1, $student2, $student3, $assignment) = $this->load_data($s1grade, $s2grade, $s3grade); // A teacher must see all student grades (in their group only). $this->setUser($teacher); @@ -129,8 +136,9 @@ class externallib_test extends externallib_advanced_testcase { $s1grade = 80; $s2grade = 60; + $s3grade = 50; - list($course, $teacher, $student1, $student2, $assignment) = $this->load_data($s1grade, $s2grade); + list($course, $teacher, $student1, $student2, $student3, $assignment) = $this->load_data($s1grade, $s2grade, $s3grade); // A user can see his own grades. $this->setUser($student1); @@ -144,6 +152,22 @@ class externallib_test extends externallib_advanced_testcase { $student1returnedgrade = (int) $studentgrade['tables'][0]['tabledata'][2]['grade']['content']; $this->assertEquals($s1grade, $student1returnedgrade); + // A user can see his own even when in no groups. + $this->setUser($student3); + $studentgrade = user_external::get_grades_table($course->id, $student3->id); + $studentgrade = external_api::clean_returnvalue(user_external::get_grades_table_returns(), $studentgrade); + + // No warnings returned. + $this->assertTrue(count($studentgrade['warnings']) == 0); + + $this->assertTrue(count($studentgrade['tables']) == 1); + $student3returnedgrade = (int) $studentgrade['tables'][0]['tabledata'][2]['grade']['content']; + $this->assertEquals($s3grade, $student3returnedgrade); + + // Expect exception when user is not indicated. + $this->setUser($student3); + $this->expectException(\required_capability_exception::class); + user_external::get_grades_table($course->id); } /** @@ -156,8 +180,9 @@ class externallib_test extends externallib_advanced_testcase { $s1grade = 80; $s2grade = 60; + $s3grade = 50; - list($course, $teacher, $student1, $student2, $assignment) = $this->load_data($s1grade, $s2grade); + list($course, $teacher, $student1, $student2, $student3, $assignment) = $this->load_data($s1grade, $s2grade, $s3grade); $this->setUser($student2); @@ -179,7 +204,8 @@ class externallib_test extends externallib_advanced_testcase { $s1grade = 80; $s2grade = 60; - list($course, $teacher, $student1, $student2, $assignment) = $this->load_data($s1grade, $s2grade); + $s3grade = 50; + list($course, $teacher, $student1, $student2, $student3, $assignment) = $this->load_data($s1grade, $s2grade, $s3grade); // Redirect events to the sink, so we can recover them later. $sink = $this->redirectEvents(); @@ -226,8 +252,9 @@ class externallib_test extends externallib_advanced_testcase { $s1grade = 80; $s2grade = 60; + $s3grade = 50; - list($course, $teacher, $student1, $student2, $assignment) = $this->load_data($s1grade, $s2grade); + list($course, $teacher, $student1, $student2, $student3, $assignment) = $this->load_data($s1grade, $s2grade, $s3grade); // A teacher must see all student grades (in their group only). $this->setUser($teacher); @@ -277,8 +304,10 @@ class externallib_test extends externallib_advanced_testcase { $this->assertFalse($studentgrades['usergrades'][0]['gradeitems'][0]['gradeisoverridden']); $this->assertEquals('B-', $studentgrades['usergrades'][0]['gradeitems'][0]['lettergradeformatted']); $this->assertEquals(1, $studentgrades['usergrades'][0]['gradeitems'][0]['rank']); - $this->assertEquals(2, $studentgrades['usergrades'][0]['gradeitems'][0]['numusers']); - $this->assertEquals(70, $studentgrades['usergrades'][0]['gradeitems'][0]['averageformatted']); + $this->assertEquals(3, $studentgrades['usergrades'][0]['gradeitems'][0]['numusers']); + $this->assertEquals( + round(array_sum([$s1grade, $s2grade, $s3grade]) / 3, 2), + $studentgrades['usergrades'][0]['gradeitems'][0]['averageformatted']); // Course grades. $this->assertEquals('course', $studentgrades['usergrades'][0]['gradeitems'][1]['itemtype']); @@ -297,8 +326,10 @@ class externallib_test extends externallib_advanced_testcase { $this->assertFalse($studentgrades['usergrades'][0]['gradeitems'][1]['gradeisoverridden']); $this->assertEquals('B-', $studentgrades['usergrades'][0]['gradeitems'][1]['lettergradeformatted']); $this->assertEquals(1, $studentgrades['usergrades'][0]['gradeitems'][1]['rank']); - $this->assertEquals(2, $studentgrades['usergrades'][0]['gradeitems'][1]['numusers']); - $this->assertEquals(70, $studentgrades['usergrades'][0]['gradeitems'][1]['averageformatted']); + $this->assertEquals(3, $studentgrades['usergrades'][0]['gradeitems'][1]['numusers']); + $this->assertEquals( + round(array_sum([$s1grade, $s2grade, $s3grade]) / 3, 2), + $studentgrades['usergrades'][0]['gradeitems'][1]['averageformatted']); // Now, override and lock a grade. $gradegrade = \grade_grade::fetch(['itemid' => $studentgrades['usergrades'][0]['gradeitems'][0]['id'], @@ -325,8 +356,9 @@ class externallib_test extends externallib_advanced_testcase { $s1grade = 80; $s2grade = 60; + $s3grade = 50; - list($course, $teacher, $student1, $student2, $assignment) = $this->load_data($s1grade, $s2grade); + list($course, $teacher, $student1, $student2, $student3, $assignment) = $this->load_data($s1grade, $s2grade, $s3grade); grade_set_setting($course->id, 'report_user_showrank', 1); grade_set_setting($course->id, 'report_user_showpercentage', 1); @@ -374,8 +406,10 @@ class externallib_test extends externallib_advanced_testcase { $this->assertNull($studentgrades['usergrades'][0]['gradeitems'][0]['gradeisoverridden']); $this->assertEquals('B-', $studentgrades['usergrades'][0]['gradeitems'][0]['lettergradeformatted']); $this->assertEquals(1, $studentgrades['usergrades'][0]['gradeitems'][0]['rank']); - $this->assertEquals(2, $studentgrades['usergrades'][0]['gradeitems'][0]['numusers']); - $this->assertEquals(70, $studentgrades['usergrades'][0]['gradeitems'][0]['averageformatted']); + $this->assertEquals(3, $studentgrades['usergrades'][0]['gradeitems'][0]['numusers']); + $this->assertEquals( + round(array_sum([$s1grade, $s2grade, $s3grade]) / 3, 2), + $studentgrades['usergrades'][0]['gradeitems'][0]['averageformatted']); // Check that the idnumber for assignment grades is equal to the cmid. $this->assertEquals((string) $studentgrades['usergrades'][0]['gradeitems'][0]['cmid'],