From e3620081f2e34fb33ce9134fdd968508e1a009f8 Mon Sep 17 00:00:00 2001 From: Damyon Wiese Date: Fri, 29 Aug 2014 09:48:44 +0800 Subject: [PATCH] MDL-46997 Grades: Fix aggregation when hiding is excluded and items have nested dependencies Includes a unit test for the dependency flattening function. --- grade/report/user/lib.php | 36 ++++++++----- lib/grade/grade_grade.php | 78 ++++++++++++++++++++++++++-- lib/grade/tests/fixtures/lib.php | 10 ++++ lib/grade/tests/grade_grade_test.php | 37 +++++++++++++ 4 files changed, 145 insertions(+), 16 deletions(-) diff --git a/grade/report/user/lib.php b/grade/report/user/lib.php index 5d96c975f3a..d63feb6ee3e 100644 --- a/grade/report/user/lib.php +++ b/grade/report/user/lib.php @@ -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); diff --git a/lib/grade/grade_grade.php b/lib/grade/grade_grade.php index b06d44ffb93..bc14ff89425 100644 --- a/lib/grade/grade_grade.php +++ b/lib/grade/grade_grade.php @@ -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]); diff --git a/lib/grade/tests/fixtures/lib.php b/lib/grade/tests/fixtures/lib.php index cc3dbb0f010..3e197263a18 100644 --- a/lib/grade/tests/fixtures/lib.php +++ b/lib/grade/tests/fixtures/lib.php @@ -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); + } +} + diff --git a/lib/grade/tests/grade_grade_test.php b/lib/grade/tests/grade_grade_test.php index 81685809158..656a5961697 100644 --- a/lib/grade/tests/grade_grade_test.php +++ b/lib/grade/tests/grade_grade_test.php @@ -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); + } }