MDL-67636 gradebook: Prevent exposing hidden grade in locked category
authorJohn Yao <johnyao@catalyst-au.net>
Thu, 19 Nov 2020 01:36:32 +0000 (12:36 +1100)
committerJohn Yao <johnyao@catalyst-au.net>
Thu, 19 Nov 2020 03:33:26 +0000 (14:33 +1100)
grade/tests/behat/grade_hidden_items_locked_category.feature [new file with mode: 0644]
lib/grade/grade_grade.php
lib/grade/grade_item.php
lib/grade/tests/grade_grade_test.php

diff --git a/grade/tests/behat/grade_hidden_items_locked_category.feature b/grade/tests/behat/grade_hidden_items_locked_category.feature
new file mode 100644 (file)
index 0000000..6cc1154
--- /dev/null
@@ -0,0 +1,92 @@
+@core @core_grades
+Feature: Hidden grade items should be hidden when grade category is locked, but should be visible in overridden category
+  In order to verify existing grades items display as expected
+  As an teacher
+  I need to modify grade items and grade categories
+  I need to ensure existing grades display in an expected manner
+
+  Background:
+    Given the following "courses" exist:
+      | fullname | shortname | category | groupmode |
+      | Course 1 | C1 | 0 | 1 |
+    And the following "users" exist:
+      | username | firstname | lastname | email | idnumber |
+      | teacher1 | Teacher | 1 | teacher1@example.com | t1 |
+      | student1 | Student | 1 | student1@example.com | s1 |
+    And the following "course enrolments" exist:
+      | user | course | role |
+      | teacher1 | C1 | editingteacher |
+      | student1 | C1 | student |
+    And I log in as "admin"
+    And I am on "Course 1" course homepage
+    And I navigate to "Setup > Gradebook setup" in the course gradebook
+    And I press "Add category"
+    And I set the following fields to these values:
+      | Category name | Test locked category |
+    And I press "Save changes"
+    And I press "Add grade item"
+    And I set the following fields to these values:
+      | Item name | Hidden item |
+      | Hidden | 1 |
+      | Grade category | Test locked category |
+    And I press "Save changes"
+    And I log out
+    And I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I navigate to "View > Grader report" in the course gradebook
+    And I turn editing mode on
+    And I give the grade "50.00" to the user "Student 1" for the grade item "Hidden item"
+    And I press "Save changes"
+    And I navigate to "Setup > Gradebook setup" in the course gradebook
+    And I set the following settings for grade item "Test locked category":
+      | Locked | 1 |
+    And I press "Save changes"
+    And I log out
+
+  Scenario: Hidden grade items in locked category is hidden for teacher
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I navigate to "View > User report" in the course gradebook
+    And I select "Myself" from the "View report as" singleselect
+    When I select "Student 1" from the "Select all or one user" singleselect
+    Then the following should exist in the "user-grade" table:
+      | Grade item | Calculated weight | Grade | Range | Percentage | Contribution to course total |
+      | Test locked category total | 100.00 % | 50.00 | 0–100 | 50.00 % | - |
+      | Course total | - | 50.00 | 0–100 | 50.00 % | - |
+
+  Scenario: Hidden grade items in locked category is hidden for student
+    Given I log in as "student1"
+    And I am on "Course 1" course homepage
+    When I navigate to "User report" in the course gradebook
+    Then the following should exist in the "user-grade" table:
+      | Grade item | Calculated weight | Grade | Range | Percentage | Contribution to course total |
+      | Test locked category total | 100.00 % | - | 0–100 | - | - |
+      | Course total | - | - | 0–100 | - | - |
+    And I should not see "Hidden item"
+
+  Scenario: Hidden grade items in overridden category should show
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I navigate to "Setup > Gradebook setup" in the course gradebook
+    And I press "Add category"
+    And I set the following fields to these values:
+      | Category name | Test overridden category B|
+    And I press "Save changes"
+    And I press "Add grade item"
+    And I set the following fields to these values:
+      | Item name | Cat b item |
+      | Grade category | Test overridden category B |
+    And I press "Save changes"
+    When I navigate to "View > Grader report" in the course gradebook
+    And I turn editing mode on
+    And I give the grade "50.00" to the user "Student 1" for the grade item "Test overridden category B total"
+    And I press "Save changes"
+    And I log out
+    And I log in as "student1"
+    And I am on "Course 1" course homepage
+    And I navigate to "User report" in the course gradebook
+    Then the following should exist in the "user-grade" table:
+      | Grade item | Calculated weight | Grade | Range | Percentage | Contribution to course total |
+      | Test locked category total | 50.00 % | - | 0–100 | - | - |
+      | Test overridden category B total | 50.00 % | 50.00 | 0–100 | 50.00 % | - |
+      | Course total | - | - | 0–200 | - | - |
index 13d2ad5..27062ab 100644 (file)
@@ -780,13 +780,15 @@ class grade_grade extends grade_object {
             $dependson[$grade_grade->itemid] = $grade_items[$grade_grade->itemid]->depends_on();
             if ($grade_grade->is_excluded()) {
                 //nothing to do, aggregation is ok
+                continue;
             } else if ($grade_grade->is_hidden()) {
                 $hiddenfound = true;
                 $altered[$grade_grade->itemid] = null;
                 $alteredaggregationstatus[$grade_grade->itemid] = 'dropped';
                 $alteredaggregationweight[$grade_grade->itemid] = 0;
-            } else if ($grade_grade->is_locked() or $grade_grade->is_overridden()) {
-                // no need to recalculate locked or overridden grades
+            } else if ($grade_grade->is_overridden()) {
+                // No need to recalculate overridden grades.
+                continue;
             } else {
                 if (!empty($dependson[$grade_grade->itemid])) {
                     $dependencydepth[$grade_grade->itemid] = 1;
@@ -846,9 +848,11 @@ class grade_grade extends grade_object {
                     } else {
                         // depends on altered grades - we should try to recalculate if possible
                         if ($grade_items[$do]->is_calculated() or
-                            (!$grade_items[$do]->is_category_item() and !$grade_items[$do]->is_course_item())
+                            (!$grade_items[$do]->is_category_item() and !$grade_items[$do]->is_course_item()) or
+                            ($grade_items[$do]->is_category_item() and $grade_items[$do]->is_locked())
                         ) {
                             // This is a grade item that is not a category or course and has been affected by grade hiding.
+                            // Or a grade item that is a category and it is locked.
                             // I guess this means it is a calculation that needs to be recalculated.
                             $unknown[$do] = $grade_grades[$do]->finalgrade;
                             unset($todo[$key]);
index 511eb8b..245469b 100644 (file)
@@ -1658,7 +1658,7 @@ class grade_item extends grade_object {
             return $this->dependson_cache;
         }
 
-        if ($this->is_locked()) {
+        if ($this->is_locked() && !$this->is_category_item()) {
             // locked items do not need to be regraded
             $this->dependson_cache = array();
             return $this->dependson_cache;
index 5a003f5..c9723d9 100644 (file)
@@ -570,4 +570,103 @@ class core_grade_grade_testcase extends grade_base_testcase {
         $gg = grade_grade::fetch(array('userid' => $u2->id, 'itemid' => $gi->id));
         $this->assertEmpty($gg);
     }
+
+    /**
+     * Tests get_hiding_affected by locked category and overridden grades.
+     */
+    public function test_category_get_hiding_affected() {
+        $generator = $this->getDataGenerator();
+
+        // Create the data we need for the tests.
+        $course1 = $generator->create_course();
+        $user1 = $generator->create_and_enrol($course1, 'student');
+        $assignment2 = $generator->create_module('assign', ['course' => $course1->id]);
+
+        // Create a category item.
+        $gradecategory = new grade_category(array('courseid' => $course1->id, 'fullname' => 'test'), false);
+        $gradecategoryid = $gradecategory->insert();
+
+        // Create one hidden grade item.
+        $gradeitem1a = new grade_item($generator->create_grade_item(
+            [
+                'courseid' => $course1->id,
+                'itemtype' => 'mod',
+                'itemmodule' => 'assign',
+                'iteminstance' => $assignment2->id,
+                'categoryid' => $gradecategoryid,
+                'hidden' => 1,
+            ]
+        ), false);
+        grade_update('mod/assign', $gradeitem1a->courseid, $gradeitem1a->itemtype, $gradeitem1a->itemmodule, $gradeitem1a->iteminstance,
+        $gradeitem1a->itemnumber, ['userid' => $user1->id]);
+
+        // Get category grade item.
+        $gradeitem = $gradecategory->get_grade_item();
+        // Reset needsupdate to allow set_locked.
+        $gradeitem->needsupdate = 0;
+        $gradeitem->update();
+        // Lock category grade item.
+        $gradeitem->set_locked(1);
+
+        $hidingaffectedlocked = $this->call_get_hiding_affected($course1, $user1);
+        // Since locked category now should be recalculated.
+        // The number of unknown items is 2, this includes category item and course item.
+        $this->assertEquals(2, count($hidingaffectedlocked['unknown']));
+
+        // Unlock category.
+        $gradeitem->set_locked(0);
+        $hidingaffectedunlocked = $this->call_get_hiding_affected($course1, $user1);
+        // When category unlocked, hidden item should exist in altered items.
+        $this->assertTrue(in_array($gradeitem1a->id, array_keys($hidingaffectedunlocked['altered'])));
+
+        // This creates all the grade_grades we need.
+        grade_regrade_final_grades($course1->id);
+
+        // Set grade override.
+        $gradegrade = grade_grade::fetch([
+            'userid' => $user1->id,
+            'itemid' => $gradeitem->id,
+        ]);
+        // Set override grade grade, and check that grade submission has been overridden.
+        $gradegrade->set_overridden(true);
+        $this->assertEquals(true, $gradegrade->is_overridden());
+        $hidingaffectedoverridden = $this->call_get_hiding_affected($course1, $user1);
+        // No need to recalculate overridden grades.
+        $this->assertTrue(in_array($gradegrade->itemid, array_keys($hidingaffectedoverridden['alteredaggregationstatus'])));
+        $this->assertEquals('used', $hidingaffectedoverridden['alteredaggregationstatus'][$gradegrade->itemid]);
+    }
+
+    /**
+     * Call get_hiding_affected().
+     * @param stdClass $course The course object
+     * @param stdClass $user The student object
+     * @return array
+     */
+    private function call_get_hiding_affected($course, $user) {
+        global $DB;
+
+        $items = grade_item::fetch_all(array('courseid' => $course->id));
+        $grades = array();
+        $sql = "SELECT g.*
+                  FROM {grade_grades} g
+                  JOIN {grade_items} gi ON gi.id = g.itemid
+                 WHERE g.userid = :userid AND gi.courseid = :courseid";
+        if ($gradesrecords = $DB->get_records_sql($sql, ['userid' => $user->id, 'courseid' => $course->id])) {
+            foreach ($gradesrecords as $grade) {
+                $grades[$grade->itemid] = new grade_grade($grade, false);
+            }
+            unset($gradesrecords);
+        }
+        foreach ($items as $itemid => $gradeitem) {
+            if (!isset($grades[$itemid])) {
+                $gradegrade = new grade_grade();
+                $gradegrade->userid = $user->id;
+                $gradegrade->itemid = $gradeitem->id;
+                $grades[$itemid] = $gradegrade;
+            }
+            $gradeitem->grade_item = $gradeitem;
+        }
+
+        return grade_grade::get_hiding_affected($grades, $items);
+    }
 }