MDL-46997 Grades: Fix aggregation when hiding is excluded and items have nested depen...
authorDamyon Wiese <damyon@moodle.com>
Fri, 29 Aug 2014 01:48:44 +0000 (09:48 +0800)
committerDamyon Wiese <damyon@moodle.com>
Tue, 9 Sep 2014 09:11:59 +0000 (17:11 +0800)
Includes a unit test for the dependency flattening function.

grade/report/user/lib.php
lib/grade/grade_grade.php
lib/grade/tests/fixtures/lib.php
lib/grade/tests/grade_grade_test.php

index 966951b..695ecf3 100644 (file)
@@ -433,6 +433,18 @@ class grade_report_user extends grade_report {
 
                 /// Actual Grade
                 $gradeval = $grade_grade->finalgrade;
+                if (!$this->canviewhidden) {
+                    /// Virtual Grade (may be calculated excluding hidden items etc).
+                    $adjustedgrade = $this->blank_hidden_total_and_adjust_bounds($this->courseid,
+                                                                                 $grade_grade->grade_item,
+                                                                                 $gradeval);
+                    $gradeval = $adjustedgrade['grade'];
+
+                    // 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'];
+                }
 
                 if ($this->showfeedback) {
                     // Copy $class before appending itemcenter as feedback should not be centered
@@ -460,20 +472,17 @@ class grade_report_user extends grade_report {
                         $data['grade']['class'] = $class;
                         $data['grade']['content'] = get_string('submittedon', 'grades', userdate($grade_grade->get_datesubmitted(), get_string('strftimedatetimeshort')));
 
-                    } elseif ($grade_grade->is_hidden()) {
-                            $data['grade']['class'] = $class.' dimmed_text';
-                            $data['grade']['content'] = '-';
+                    } else if ($grade_grade->is_hidden()) {
+                        $data['grade']['class'] = $class.' dimmed_text';
+                        $data['grade']['content'] = '-';
+                        if ($this->canviewhidden) {
+                            $data['grade']['content'] = grade_format_gradevalue($gradeval,
+                                                                                $grade_grade->grade_item,
+                                                                                true);
+                        }
                     } else {
                         $data['grade']['class'] = $class;
-                        $adjustedgrade = $this->blank_hidden_total_and_adjust_bounds($this->courseid,
-                                                                                     $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'],
+                        $data['grade']['content'] = grade_format_gradevalue($gradeval,
                                                                             $grade_grade->grade_item,
                                                                             true);
                     }
@@ -495,6 +504,9 @@ class grade_report_user extends grade_report {
                     } else if ($grade_grade->is_hidden()) {
                         $data['percentage']['class'] = $class.' dimmed_text';
                         $data['percentage']['content'] = '-';
+                        if ($this->canviewhidden) {
+                            $data['percentage']['content'] = grade_format_gradevalue($gradeval, $grade_grade->grade_item, true, GRADE_DISPLAY_TYPE_PERCENTAGE);
+                        }
                     } else {
                         $data['percentage']['class'] = $class;
                         $data['percentage']['content'] = grade_format_gradevalue($gradeval, $grade_grade->grade_item, true, GRADE_DISPLAY_TYPE_PERCENTAGE);
index fcc4098..e9c6ce4 100644 (file)
@@ -565,6 +565,46 @@ class grade_grade extends grade_object {
         return $standardised_value;
     }
 
+    /**
+     * Given an array like this:
+     * $a = array(1=>array(2, 3),
+     *            2=>array(4),
+     *            3=>array(1),
+     *            4=>array())
+     * this function fully resolves the dependencies so each value will be an array of
+     * the all items this item depends on and their dependencies (and their dependencies...).
+     * It should not explode if there are circular dependencies.
+     * The dependency depth array will list the number of branches in the tree above each leaf.
+     *
+     * @param array $dependson Array to flatten
+     * @param array $dependencydepth Array of itemids => depth. Initially these should be all set to 1.
+     * @return array Flattened array
+     */
+    protected static function flatten_dependencies_array(&$dependson, &$dependencydepth) {
+        // Flatten the nested dependencies - this will handle recursion bombs because it removes duplicates.
+        $somethingchanged = true;
+        while ($somethingchanged) {
+            $somethingchanged = false;
+
+            foreach ($dependson as $itemid => $depends) {
+                // Make a copy so we can tell if it changed.
+                $before = $dependson[$itemid];
+                foreach ($depends as $subitemid => $subdepends) {
+                    $dependson[$itemid] = array_unique(array_merge($depends, $dependson[$subdepends]));
+                    sort($dependson[$itemid], SORT_NUMERIC);
+                }
+                if ($before != $dependson[$itemid]) {
+                    $somethingchanged = true;
+                    if (!isset($dependencydepth[$itemid])) {
+                        $dependencydepth[$itemid] = 1;
+                    } else {
+                        $dependencydepth[$itemid]++;
+                    }
+                }
+            }
+        }
+    }
+
     /**
      * Return array of grade item ids that are either hidden or indirectly depend
      * on hidden grades, excluded grades are not returned.
@@ -590,10 +630,13 @@ class grade_grade extends grade_object {
         $altered = array();  // altered grades
         $alteredgrademax = array();  // Altered grade max values.
         $alteredgrademin = array();  // Altered grade min values.
+        $dependencydepth = array();
 
         $hiddenfound = false;
         foreach($grade_grades as $itemid=>$unused) {
             $grade_grade =& $grade_grades[$itemid];
+            // We need the immediate dependencies of all every grade_item so we can calculate nested dependencies.
+            $dependson[$grade_grade->itemid] = $grade_items[$grade_grade->itemid]->depends_on();
             if ($grade_grade->is_excluded()) {
                 //nothing to do, aggregation is ok
             } else if ($grade_grade->is_hidden()) {
@@ -602,18 +645,24 @@ class grade_grade extends grade_object {
             } else if ($grade_grade->is_locked() or $grade_grade->is_overridden()) {
                 // no need to recalculate locked or overridden grades
             } else {
-                $dependson[$grade_grade->itemid] = $grade_items[$grade_grade->itemid]->depends_on();
                 if (!empty($dependson[$grade_grade->itemid])) {
+                    $dependencydepth[$grade_grade->itemid] = 1;
                     $todo[] = $grade_grade->itemid;
                 }
             }
         }
+
+        // Flatten the dependency tree and count number of branches to each leaf.
+        self::flatten_dependencies_array($dependson, $dependencydepth);
+
         if (!$hiddenfound) {
             return array('unknown' => array(),
                          'altered' => array(),
                          'alteredgrademax' => array(),
                          'alteredgrademin' => array());
         }
+        // We need to resort the todo list by the dependency depth. This guarantees we process the leaves, then the branches.
+        array_multisort($dependencydepth, $todo);
 
         $max = count($todo);
         $hidden_precursors = null;
@@ -641,20 +690,28 @@ class grade_grade extends grade_object {
                         if ($grade_items[$do]->is_calculated() or
                             (!$grade_items[$do]->is_category_item() and !$grade_items[$do]->is_course_item())
                         ) {
+                            // This is a grade item that is not a category or course and has been affected by grade hiding.
+                            // I guess this means it is a calculation that needs to be recalculated.
                             $unknown[$do] = $do;
                             unset($todo[$key]);
                             $found = true;
                             continue;
 
                         } else {
+                            // This is a grade category (or course).
                             $grade_category = $grade_items[$do]->load_item_category();
 
+                            // Build a new list of the grades in this category.
                             $values = array();
-                            foreach ($dependson[$do] as $itemid) {
+                            $immediatedepends = $grade_items[$do]->depends_on();
+                            foreach ($immediatedepends as $itemid) {
                                 if (array_key_exists($itemid, $altered)) {
                                     //nulling an altered precursor
                                     $values[$itemid] = $altered[$itemid];
-                                    unset($values[$itemid]);
+                                    if (is_null($values[$itemid])) {
+                                        // This means this was a hidden grade item removed from the result.
+                                        unset($values[$itemid]);
+                                    }
                                 } elseif (empty($values[$itemid])) {
                                     $values[$itemid] = $grade_grades[$itemid]->finalgrade;
                                 }
@@ -665,7 +722,16 @@ class grade_grade extends grade_object {
                                     unset($values[$itemid]);
                                     continue;
                                 }
-                                $values[$itemid] = grade_grade::standardise_score($value, $grade_items[$itemid]->grademin, $grade_items[$itemid]->grademax, 0, 1);
+                                // The grade min/max may have been altered by hiding.
+                                $grademin = $grade_items[$itemid]->grademin;
+                                if (isset($alteredgrademin[$itemid])) {
+                                    $grademin = $alteredgrademin[$itemid];
+                                }
+                                $grademax = $grade_items[$itemid]->grademax;
+                                if (isset($alteredgrademax[$itemid])) {
+                                    $grademax = $alteredgrademax[$itemid];
+                                }
+                                $values[$itemid] = grade_grade::standardise_score($value, $grademin, $grademax, 0, 1);
                             }
 
                             if ($grade_category->aggregateonlygraded) {
@@ -707,6 +773,10 @@ class grade_grade extends grade_object {
                             $finalgrade = $grade_items[$do]->bounded_grade($finalgrade);
                             $alteredgrademin[$do] = $adjustedgrade['grademin'];
                             $alteredgrademax[$do] = $adjustedgrade['grademax'];
+                            // We need to muck with the "in-memory" grade_items records so
+                            // that subsequent calculations will use the adjusted grademin and grademax.
+                            $grade_items[$do]->grademin = $adjustedgrade['grademin'];
+                            $grade_items[$do]->grademax = $adjustedgrade['grademax'];
 
                             $altered[$do] = $finalgrade;
                             unset($todo[$key]);
index 52613b5..372c46a 100644 (file)
@@ -973,3 +973,13 @@ abstract class grade_base_testcase extends advanced_testcase {
         $this->grade_items[17] = $grade_item;
     }
 }
+
+/**
+ * Allow calling protected method.
+ */
+class test_grade_grade_flatten_dependencies_array extends grade_grade {
+    public static function test_flatten_dependencies_array(&$a,&$b) {
+        return self::flatten_dependencies_array($a, $b);
+    }
+}
+
index 8168580..656a596 100644 (file)
@@ -193,4 +193,41 @@ class core_grade_grade_testcase extends grade_base_testcase {
         $grade->hidden = time()+666;
         $this->assertTrue($grade->is_hidden());
     }
+
+    public function test_flatten_dependencies() {
+        // First test a simple normal case.
+        $a = array(1 => array(2, 3), 2 => array(), 3 => array(4), 4 => array());
+        $b = array();
+        $expecteda = array(1 => array(2, 3, 4), 2 => array(), 3 => array(4), 4 => array());
+        $expectedb = array(1 => 1);
+
+        test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
+        $this->assertSame($expecteda, $a);
+        $this->assertSame($expectedb, $b);
+
+        // Edge case - empty arrays.
+        $a = $b = $expecteda = $expectedb = array();
+
+        test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
+        $this->assertSame($expecteda, $a);
+        $this->assertSame($expectedb, $b);
+
+        // Circular dependency.
+        $a = array(1 => array(2), 2 => array(3), 3 => array(1));
+        $b = array();
+        $expecteda = array(1 => array(1, 2, 3), 2 => array(1, 2, 3), 3 => array(1, 2, 3));
+
+        test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
+        $this->assertSame($expecteda, $a);
+        // Note - we don't test the depth when we got circular dependencies - the main thing we wanted to test was that there was
+        // no ka-boom. The result would be hard to understand and doesn't matter.
+
+        // Circular dependency 2.
+        $a = array(1 => array(2), 2 => array(3), 3 => array(4), 4 => array(2, 1));
+        $b = array();
+        $expecteda = array(1 => array(1, 2, 3, 4), 2 => array(1, 2, 3, 4), 3 => array(1, 2, 3, 4), 4 => array(1, 2, 3, 4));
+
+        test_grade_grade_flatten_dependencies_array::test_flatten_dependencies_array($a, $b);
+        $this->assertSame($expecteda, $a);
+    }
 }