From 5232d3f2c9eeafa53c48adcf6fba3f5a59007672 Mon Sep 17 00:00:00 2001
From: Damyon Wiese <damyon@moodle.com>
Date: Mon, 4 Aug 2014 15:03:26 +0800
Subject: [PATCH] MDL-35984 grade: Recalculate min/max for sum of grades when
hidden grades are involved.
---
grade/report/lib.php | 42 +++++++++++++++++++++++++---
grade/report/overview/lib.php | 10 ++++++-
grade/report/user/lib.php | 13 +++++++--
grade/tests/reportlib_test.php | 40 +++++++++++++++++++--------
lib/grade/grade_category.php | 50 ++++++++++++++++++++++++++++++----
lib/grade/grade_grade.php | 28 +++++++++++++++----
6 files changed, 154 insertions(+), 29 deletions(-)
diff --git a/grade/report/lib.php b/grade/report/lib.php
index 3e757cd1084..29ebe66ad78 100644
--- a/grade/report/lib.php
+++ b/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'];
}
}
diff --git a/grade/report/overview/lib.php b/grade/report/overview/lib.php
index 8c279f7a891..90ca93def3e 100644
--- a/grade/report/overview/lib.php
+++ b/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'];
}
}
diff --git a/grade/report/user/lib.php b/grade/report/user/lib.php
index bdbc83cc90d..930a69375aa 100644
--- a/grade/report/user/lib.php
+++ b/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);
- $data['grade']['content'] = grade_format_gradevalue($gradeval, $grade_grade->grade_item, true);
+ $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'],
+ $grade_grade->grade_item,
+ true);
}
$data['grade']['headers'] = "$header_cat $header_row grade";
}
diff --git a/grade/tests/reportlib_test.php b/grade/tests/reportlib_test.php
index dac85f9da31..4ff5240a79d 100644
--- a/grade/tests/reportlib_test.php
+++ b/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) {
- return parent::blank_hidden_total($courseid, $courseitem, $finalgrade);
+ public function blank_hidden_total_and_adjust_bounds($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);
}
}
diff --git a/lib/grade/grade_category.php b/lib/grade/grade_category.php
index d5a9e6d72fa..6c23d3188f1 100644
--- a/lib/grade/grade_category.php
+++ b/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'];
}
/**
diff --git a/lib/grade/grade_grade.php b/lib/grade/grade_grade.php
index 8763fdd3a7a..fcc4098c9a3 100644
--- a/lib/grade/grade_grade.php
+++ b/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);
}
/**