MDL-35984 grade: Recalculate min/max for sum of grades when hidden grades are involved.

grade/report/lib.php

@@ -412,9 +412,9 @@ abstract class grade_report {
      * @param string $courseid the course id
      * @param string $course_item an instance of grade_item
      * @param string $finalgrade the grade for the course_item
-     * @return string The new final grade
+     * @return array[] containing values for 'grade', 'grademax' and 'grademin'
      */
-    protected function blank_hidden_total($courseid, $course_item, $finalgrade) {
+    protected function blank_hidden_total_and_adjust_bounds($courseid, $course_item, $finalgrade) {
         global $CFG, $DB;
         static $hiding_affected = null;//array of items in this course affected by hiding
 
@@ -424,13 +424,16 @@ abstract class grade_report {
         // If we're dealing with multiple courses we need to know when we've moved on to a new course.
         static $previous_courseid = null;
 
+        $grademin = $course_item->grademin;
+        $grademax = $course_item->grademax;
+
         if (!is_array($this->showtotalsifcontainhidden)) {
             debugging('showtotalsifcontainhidden should be an array', DEBUG_DEVELOPER);
             $this->showtotalsifcontainhidden = array($courseid => $this->showtotalsifcontainhidden);
         }
 
         if ($this->showtotalsifcontainhidden[$courseid] == GRADE_REPORT_SHOW_REAL_TOTAL_IF_CONTAINS_HIDDEN) {
-            return $finalgrade;
+            return array('grade' => $finalgrade, 'grademin' => $grademin, 'grademax' => $grademax);
         }
 
         // If we've moved on to another course or user, reload the grades.
@@ -473,6 +476,12 @@ abstract class grade_report {
             } else {
                 //use reprocessed marks that exclude hidden items
                 $finalgrade = $hiding_affected['altered'][$course_item->id];
+                if (!empty($hiding_affected['alteredgrademin'][$course_item->id])) {
+                    $grademin = $hiding_affected['alteredgrademin'][$course_item->id];
+                }
+                if (!empty($hiding_affected['alteredgrademax'][$course_item->id])) {
+                    $grademax = $hiding_affected['alteredgrademax'][$course_item->id];
+                }
             }
         } else if (!empty($hiding_affected['unknown'][$course_item->id])) {
             //not sure whether or not this item depends on a hidden item
@@ -482,10 +491,35 @@ abstract class grade_report {
             } else {
                 //use reprocessed marks that exclude hidden items
                 $finalgrade = $hiding_affected['unknown'][$course_item->id];
+
+                if (!empty($hiding_affected['alteredgrademin'][$course_item->id])) {
+                    $grademin = $hiding_affected['alteredgrademin'][$course_item->id];
+                }
+                if (!empty($hiding_affected['alteredgrademax'][$course_item->id])) {
+                    $grademax = $hiding_affected['alteredgrademax'][$course_item->id];
+                }
             }
         }
 
-        return $finalgrade;
+        return array('grade' => $finalgrade, 'grademin' => $grademin, 'grademax' => $grademax);
+    }
+
+    /**
+     * Optionally blank out course/category totals if they contain any hidden items
+     * @deprecated since Moodle 2.8 - Call blank_hidden_total_and_adjust_bounds instead.
+     * @param string $courseid the course id
+     * @param string $course_item an instance of grade_item
+     * @param string $finalgrade the grade for the course_item
+     * @return string The new final grade
+     */
+    protected function blank_hidden_total($courseid, $course_item, $finalgrade) {
+        // Note it is flawed to call this function directly because
+        // the aggregated grade does not make sense without the updated min and max information.
+
+        debugging('grade_report::blank_hidden_total() is deprecated.
+                   Call grade_report::blank_hidden_total_and_adjust_bounds instead.', DEBUG_DEVELOPER);
+        $result = $this->blank_hidden_total_and_adjust_bounds($courseid, $course_item, $finalgrade);
+        return $result['grade'];
     }
 }
 

grade/report/overview/lib.php

@@ -169,7 +169,15 @@ class grade_report_overview extends grade_report {
                     if ($course_grade->is_hidden()) {
                         $finalgrade = null;
                     } else {
-                        $finalgrade = $this->blank_hidden_total($course->id, $course_item, $finalgrade);
+                        $adjustedgrade = $this->blank_hidden_total_and_adjust_bounds($course->id,
+                                                                                     $course_item,
+                                                                                     $finalgrade);
+
+                        // We temporarily adjust the view of this grade item - because the min and
+                        // max are affected by the hidden values in the aggregation.
+                        $finalgrade = $adjustedgrade['grade'];
+                        $course_item->grademax = $adjustedgrade['grademax'];
+                        $course_item->grademin = $adjustedgrade['grademin'];
                     }
                 }
 

grade/report/user/lib.php

@@ -435,8 +435,17 @@ class grade_report_user extends grade_report {
                             $data['grade']['content'] = '-';
                     } else {
                         $data['grade']['class'] = $class;
-                        $gradeval = $this->blank_hidden_total($this->courseid, $grade_grade->grade_item, $gradeval);
+                        $adjustedgrade = $this->blank_hidden_total_and_adjust_bounds($this->courseid,
-                        $data['grade']['content'] = grade_format_gradevalue($gradeval, $grade_grade->grade_item, true);
+                                                                                     $grade_grade->grade_item,
+                                                                                     $gradeval);
+
+                        // We temporarily adjust the view of this grade item - because the min and
+                        // max are affected by the hidden values in the aggregation.
+                        $grade_grade->grade_item->grademax = $adjustedgrade['grademax'];
+                        $grade_grade->grade_item->grademin = $adjustedgrade['grademin'];
+                        $data['grade']['content'] = grade_format_gradevalue($adjustedgrade['grade'],
+                                                                            $grade_grade->grade_item,
+                                                                            true);
                     }
                     $data['grade']['headers'] = "$header_cat $header_row grade";
                 }

grade/tests/reportlib_test.php

@@ -39,10 +39,10 @@ class grade_report_test extends grade_report {
     }
 
     /**
-     * A wrapper around blank_hidden_total() to allow test code to call it directly
+     * A wrapper around blank_hidden_total_and_adjust_bounds() to allow test code to call it directly
      */
-    public function blank_hidden_total($courseid, $courseitem, $finalgrade) {
+    public function blank_hidden_total_and_adjust_bounds($courseid, $courseitem, $finalgrade) {
-        return parent::blank_hidden_total($courseid, $courseitem, $finalgrade);
+        return parent::blank_hidden_total_and_adjust_bounds($courseid, $courseitem, $finalgrade);
     }
 
     /**
@@ -64,9 +64,9 @@ class grade_report_test extends grade_report {
 class core_grade_reportlib_testcase extends advanced_testcase {
 
     /**
-     * Tests grade_report::blank_hidden_total()
+     * Tests grade_report::blank_hidden_total_and_adjust_bounds()
      */
-    public function test_blank_hidden_total() {
+    public function test_blank_hidden_total_and_adjust_bounds() {
         global $DB;
 
         $this->resetAfterTest(true);
@@ -121,15 +121,24 @@ class core_grade_reportlib_testcase extends advanced_testcase {
 
         // Should return the supplied student total grade regardless of hiding.
         $report->showtotalsifcontainhidden = array($course->id => GRADE_REPORT_SHOW_REAL_TOTAL_IF_CONTAINS_HIDDEN);
-        $this->assertEquals($datagrade + $forumgrade, $report->blank_hidden_total($course->id, $coursegradeitem, $datagrade + $forumgrade));
+        $result = $report->blank_hidden_total_and_adjust_bounds($course->id, $coursegradeitem, $datagrade + $forumgrade);
+        $this->assertEquals(array('grade' => $datagrade + $forumgrade,
+                                  'grademax' => $coursegradeitem->grademax,
+                                  'grademin' => $coursegradeitem->grademin), $result);
 
         // Should blank the student total as course grade depends on a hidden item.
         $report->showtotalsifcontainhidden = array($course->id => GRADE_REPORT_HIDE_TOTAL_IF_CONTAINS_HIDDEN);
-        $this->assertEquals(null, $report->blank_hidden_total($course->id, $coursegradeitem, $datagrade + $forumgrade));
+        $result = $report->blank_hidden_total_and_adjust_bounds($course->id, $coursegradeitem, $datagrade + $forumgrade);
+        $this->assertEquals(array('grade' => null,
+                                  'grademax' => $coursegradeitem->grademax,
+                                  'grademin' => $coursegradeitem->grademin), $result);
 
         // Should return the course total minus the hidden database activity grade.
         $report->showtotalsifcontainhidden = array($course->id => GRADE_REPORT_SHOW_TOTAL_IF_CONTAINS_HIDDEN);
-        $this->assertEquals($forumgrade, $report->blank_hidden_total($course->id, $coursegradeitem, $datagrade + $forumgrade));
+        $result = $report->blank_hidden_total_and_adjust_bounds($course->id, $coursegradeitem, $datagrade + $forumgrade);
+        $this->assertEquals(array('grade' => $forumgrade,
+                                  'grademax' => $coursegradeitem->grademax,
+                                  'grademin' => $coursegradeitem->grademin), $result);
 
         // Note: we cannot simply hide modules and call $report->blank_hidden_total() again.
         // It stores grades in a static variable so $report->blank_hidden_total() will return incorrect totals
@@ -183,15 +192,24 @@ class core_grade_reportlib_testcase extends advanced_testcase {
 
         // Should return the supplied student total grade regardless of hiding.
         $report->showtotalsifcontainhidden = array($course->id => GRADE_REPORT_SHOW_REAL_TOTAL_IF_CONTAINS_HIDDEN);
-        $this->assertEquals($datagrade + $forumgrade, $report->blank_hidden_total($course->id, $coursegradeitem, $datagrade + $forumgrade));
+        $result = $report->blank_hidden_total_and_adjust_bounds($course->id, $coursegradeitem, $datagrade + $forumgrade);
+        $this->assertEquals(array('grade' => $datagrade + $forumgrade,
+                                  'grademax' => $coursegradeitem->grademax,
+                                  'grademin' => $coursegradeitem->grademin), $result);
 
         // Should blank the student total as course grade depends on a hidden item.
         $report->showtotalsifcontainhidden = array($course->id => GRADE_REPORT_HIDE_TOTAL_IF_CONTAINS_HIDDEN);
-        $this->assertEquals(null, $report->blank_hidden_total($course->id, $coursegradeitem, $datagrade + $forumgrade));
+        $result = $report->blank_hidden_total_and_adjust_bounds($course->id, $coursegradeitem, $datagrade + $forumgrade);
+        $this->assertEquals(array('grade' => null,
+                                  'grademax' => $coursegradeitem->grademax,
+                                  'grademin' => $coursegradeitem->grademin), $result);
 
         // Should return the course total minus the hidden activity grades.
         // They are both hidden so should return null.
         $report->showtotalsifcontainhidden = array($course->id => GRADE_REPORT_SHOW_TOTAL_IF_CONTAINS_HIDDEN);
-        $this->assertEquals(null, $report->blank_hidden_total($course->id, $coursegradeitem, $datagrade + $forumgrade));
+        $result = $report->blank_hidden_total_and_adjust_bounds($course->id, $coursegradeitem, $datagrade + $forumgrade);
+        $this->assertEquals(array('grade' => null,
+                                  'grademax' => $coursegradeitem->grademax,
+                                  'grademin' => $coursegradeitem->grademin), $result);
     }
 }

lib/grade/grade_category.php

@@ -612,10 +612,11 @@ class grade_category extends grade_object {
         }
 
         // do the maths
-        $agg_grade = $this->aggregate_values($grade_values, $items);
+        $result = $this->aggregate_values_and_adjust_bounds($grade_values, $items);
+        $agg_grade = $result['grade'];
 
         // recalculate the grade back to requested range
-        $finalgrade = grade_grade::standardise_score($agg_grade, 0, 1, $this->grade_item->grademin, $this->grade_item->grademax);
+        $finalgrade = grade_grade::standardise_score($agg_grade, 0, 1, $result['grademin'], $result['grademax']);
 
         $grade->finalgrade = $this->grade_item->bounded_grade($finalgrade);
 
@@ -628,15 +629,22 @@ class grade_category extends grade_object {
     }
 
     /**
-     * Internal function that calculates the aggregated grade for this grade category
+     * Internal function that calculates the aggregated grade and new min/max for this grade category
      *
      * Must be public as it is used by grade_grade::get_hiding_affected()
      *
      * @param array $grade_values An array of values to be aggregated
      * @param array $items The array of grade_items
-     * @return float The aggregate grade for this grade category
+     * @return array containing values for:
+     *                'grade' => the new calculated grade
+     *                'grademin' => the new calculated min grade for the category
+     *                'grademax' => the new calculated max grade for the category
      */
-    public function aggregate_values($grade_values, $items) {
+    public function aggregate_values_and_adjust_bounds($grade_values, $items) {
+        $category_item = $this->get_grade_item();
+        $grademin = $category_item->grademin;
+        $grademax = $category_item->grademax;
+
         switch ($this->aggregation) {
 
             case GRADE_AGGREGATE_MEDIAN: // Middle point value in the set: ignores frequencies
@@ -752,6 +760,19 @@ class grade_category extends grade_object {
                 }
                 break;
 
+            case GRADE_AGGREGATE_SUM:    // Add up all the items.
+                $num = count($grade_values);
+                $sum = array_sum($grade_values);
+                $agg_grade = $sum / $num;
+                // Excluded items can affect the grademax for this grade_item.
+                $grademin = 0;
+                $grademax = 0;
+                foreach ($grade_values as $itemid => $grade_value) {
+                    $grademin += $items[$itemid]->grademin;
+                    $grademax += $items[$itemid]->grademax;
+                }
+                break;
+
             case GRADE_AGGREGATE_MEAN:    // Arithmetic average of all grade items (if ungraded aggregated, NULL counted as minimum)
             default:
                 $num = count($grade_values);
@@ -760,7 +781,24 @@ class grade_category extends grade_object {
                 break;
         }
 
-        return $agg_grade;
+        return array('grade' => $agg_grade, 'grademin' => $grademin, 'grademax' => $grademax);
+    }
+
+    /**
+     * Internal function that calculates the aggregated grade for this grade category
+     *
+     * Must be public as it is used by grade_grade::get_hiding_affected()
+     *
+     * @deprecated since Moodle 2.8
+     * @param array $grade_values An array of values to be aggregated
+     * @param array $items The array of grade_items
+     * @return float The aggregate grade for this grade category
+     */
+    public function aggregate_values($grade_values, $items) {
+        debugging('grade_category::aggregate_values() is deprecated.
+                   Call grade_category::aggregate_values_and_adjust_bounds() instead.', DEBUG_DEVELOPER);
+        $result = $this->aggregate_values_and_adjust_bounds($grade_values, $items);
+        return $result['grade'];
     }
 
     /**

lib/grade/grade_grade.php

@@ -572,7 +572,10 @@ class grade_grade extends grade_object {
      *
      * @param array $grade_grades all course grades of one user, & used for better internal caching
      * @param array $grade_items array of grade items, & used for better internal caching
-     * @return array
+     * @return array This is an array of 3 arrays:
+     *      unknown => list of item ids that may be affected by hiding (with the calculated grade as the value)
+     *      altered => list of item ids that are definitely affected by hiding (with the calculated grade as the value)
+     *      alteredgrademax => for each item in altered or unknown, the new value of the grademax
      */
     public static function get_hiding_affected(&$grade_grades, &$grade_items) {
         global $CFG;
@@ -585,6 +588,8 @@ class grade_grade extends grade_object {
         $todo = array();
         $unknown = array();  // can not find altered
         $altered = array();  // altered grades
+        $alteredgrademax = array();  // Altered grade max values.
+        $alteredgrademin = array();  // Altered grade min values.
 
         $hiddenfound = false;
         foreach($grade_grades as $itemid=>$unused) {
@@ -604,7 +609,10 @@ class grade_grade extends grade_object {
             }
         }
         if (!$hiddenfound) {
-            return array('unknown'=>array(), 'altered'=>array());
+            return array('unknown' => array(),
+                         'altered' => array(),
+                         'alteredgrademax' => array(),
+                         'alteredgrademin' => array());
         }
 
         $max = count($todo);
@@ -646,6 +654,7 @@ class grade_grade extends grade_object {
                                 if (array_key_exists($itemid, $altered)) {
                                     //nulling an altered precursor
                                     $values[$itemid] = $altered[$itemid];
+                                    unset($values[$itemid]);
                                 } elseif (empty($values[$itemid])) {
                                     $values[$itemid] = $grade_grades[$itemid]->finalgrade;
                                 }
@@ -686,12 +695,18 @@ class grade_grade extends grade_object {
                                 continue;
                             }
 
-                            $agg_grade = $grade_category->aggregate_values($values, $grade_items);
+                            $adjustedgrade = $grade_category->aggregate_values_and_adjust_bounds($values, $grade_items);
 
                             // recalculate the rawgrade back to requested range
-                            $finalgrade = grade_grade::standardise_score($agg_grade, 0, 1, $grade_items[$do]->grademin, $grade_items[$do]->grademax);
+                            $finalgrade = grade_grade::standardise_score($adjustedgrade['grade'],
+                                                                         0,
+                                                                         1,
+                                                                         $adjustedgrade['grademin'],
+                                                                         $adjustedgrade['grademax']);
 
                             $finalgrade = $grade_items[$do]->bounded_grade($finalgrade);
+                            $alteredgrademin[$do] = $adjustedgrade['grademin'];
+                            $alteredgrademax[$do] = $adjustedgrade['grademax'];
 
                             $altered[$do] = $finalgrade;
                             unset($todo[$key]);
@@ -706,7 +721,10 @@ class grade_grade extends grade_object {
             }
         }
 
-        return array('unknown'=>$unknown, 'altered'=>$altered);
+        return array('unknown' => $unknown,
+                     'altered' => $altered,
+                     'alteredgrademax' => $alteredgrademax,
+                     'alteredgrademin' => $alteredgrademin);
     }
 
     /**