diff --git a/lib/grade/grade_category.php b/lib/grade/grade_category.php index ca67450c4ab..10656f95c82 100644 --- a/lib/grade/grade_category.php +++ b/lib/grade/grade_category.php @@ -878,22 +878,66 @@ class grade_category extends grade_object { asort($grade_values, SORT_NUMERIC); $dropped = 0; - foreach ($grade_values as $itemid=>$value) { + // If we have fewer grade items available to drop than $this->droplow, use this flag to escape the loop + // May occur because of "extra credit" or if droplow is higher than the number of grade items + $droppedsomething = true; - if ($dropped < $this->droplow) { + while ($dropped < $this->droplow && $droppedsomething) { + $droppedsomething = false; - if ($extraused and $items[$itemid]->aggregationcoef > 0) { - // no drop low for extra credits - - } else { - unset($grade_values[$itemid]); - $dropped++; - } - - } else { - // we have dropped enough + $grade_keys = array_keys($grade_values); + if (count($grade_keys) === 0) { + //We've dropped all grade items break; } + + $originalindex = $founditemid = $foundmax = null; + + // Find the first remaining grade item that is available to be dropped + foreach ($grade_keys as $gradekeyindex=>$gradekey) { + if (!$extraused || $items[$gradekey]->aggregationcoef <= 0) { + // Found a non-extra credit grade item that is eligible to be dropped + $originalindex = $gradekeyindex; + $founditemid = $grade_keys[$originalindex]; + $foundmax = $items[$founditemid]->grademax; + break; + } + } + + if (empty($founditemid)) { + // No grade items available to drop + break; + } + + // Now iterate over the remaining grade items + // We're looking for other grade items with the same grade value but a higher grademax + $i = 1; + while ($originalindex+$i < count($grade_keys)) { + $possibleitemid = $grade_keys[$originalindex+$i]; + if ($grade_values[$founditemid] != $grade_values[$possibleitemid]) { + // The next grade item has a different grade value. Stop looking. + break; + } + + if ($extraused && $items[$possibleitemid]->aggregationcoef > 0) { + // Don't drop extra credit grade items. Continue the search. + continue; + } + + if ($foundmax < $items[$possibleitemid]->grademax) { + // Found a grade item with the same grade value and a higher grademax + $foundmax = $items[$possibleitemid]->grademax; + $founditemid = $possibleitemid; + // Continue searching to see if there is an even higher grademax + } + + $i++; + } + + // Now drop whatever grade item we have found + unset($grade_values[$founditemid]); + $dropped++; + $droppedsomething = true; } } else if (!empty($this->keephigh)) { diff --git a/lib/grade/tests/grade_category_test.php b/lib/grade/tests/grade_category_test.php index 102ef8625c6..727e69867f0 100644 --- a/lib/grade/tests/grade_category_test.php +++ b/lib/grade/tests/grade_category_test.php @@ -406,6 +406,7 @@ class grade_category_testcase extends grade_base_testcase { $items[$this->grade_items[2]->id] = new grade_item($this->grade_items[2], false); $items[$this->grade_items[4]->id] = new grade_item($this->grade_items[4], false); + // Test excluding the lowest 2 out of 4 grades from aggregation with no 0 grades $category = new grade_category(); $category->droplow = 2; $grades = array($this->grade_items[0]->id=>5.374, @@ -417,6 +418,7 @@ class grade_category_testcase extends grade_base_testcase { $this->assertEquals($grades[$this->grade_items[1]->id], 9.4743); $this->assertEquals($grades[$this->grade_items[4]->id], 7.3754); + // Test aggregating only the highest 1 out of 4 grades $category = new grade_category(); $category->keephigh = 1; $category->droplow = 0; @@ -429,25 +431,28 @@ class grade_category_testcase extends grade_base_testcase { $grade = reset($grades); $this->assertEquals(9.4743, $grade); + // Test excluding the lowest 2 out of 4 grades from aggregation with no 0 grades + // An extra credit grade item should be kept even if droplow means it would otherwise be excluded $category = new grade_category(); $category->droplow = 2; $category->aggregation = GRADE_AGGREGATE_SUM; - $items[$this->grade_items[2]->id]->aggregationcoef = 1; + $items[$this->grade_items[2]->id]->aggregationcoef = 1; // Mark grade item 2 as "extra credit" $grades = array($this->grade_items[0]->id=>5.374, $this->grade_items[1]->id=>9.4743, $this->grade_items[2]->id=>2.5474, $this->grade_items[4]->id=>7.3754); - $category->apply_limit_rules($grades, $items); $this->assertEquals(count($grades), 2); $this->assertEquals($grades[$this->grade_items[1]->id], 9.4743); $this->assertEquals($grades[$this->grade_items[2]->id], 2.5474); + // Test only aggregating the highest 1 out of 4 grades + // An extra credit grade item is retained in addition to the highest grade $category = new grade_category(); $category->keephigh = 1; $category->droplow = 0; $category->aggregation = GRADE_AGGREGATE_SUM; - $items[$this->grade_items[2]->id]->aggregationcoef = 1; + $items[$this->grade_items[2]->id]->aggregationcoef = 1; // Mark grade item 2 as "extra credit" $grades = array($this->grade_items[0]->id=>5.374, $this->grade_items[1]->id=>9.4743, $this->grade_items[2]->id=>2.5474, @@ -456,6 +461,61 @@ class grade_category_testcase extends grade_base_testcase { $this->assertEquals(count($grades), 2); $this->assertEquals($grades[$this->grade_items[1]->id], 9.4743); $this->assertEquals($grades[$this->grade_items[2]->id], 2.5474); + + + // Test excluding the lowest 1 out of 4 grades from aggregation with two 0 grades + $items[$this->grade_items[2]->id]->aggregationcoef = 0; // Undo marking grade item 2 as "extra credit" + $category = new grade_category(); + $category->droplow = 1; + $category->aggregation = GRADE_AGGREGATE_WEIGHTED_MEAN2; // simple weighted mean + $grades = array($this->grade_items[0]->id=>0, // 0 out of 110. Should be excluded from aggregation. + $this->grade_items[1]->id=>5, // 5 out of 100 + $this->grade_items[2]->id=>2, // 0 out of 6 + $this->grade_items[4]->id=>0); // 0 out of 100 + $category->apply_limit_rules($grades, $items); + $this->assertEquals(count($grades), 3); + $this->assertEquals($grades[$this->grade_items[1]->id], 5); + $this->assertEquals($grades[$this->grade_items[2]->id], 2); + $this->assertEquals($grades[$this->grade_items[4]->id], 0); + + // Test excluding the lowest 2 out of 4 grades from aggregation with three 0 grades + $category = new grade_category(); + $category->droplow = 2; + $category->aggregation = GRADE_AGGREGATE_WEIGHTED_MEAN2; // simple weighted mean + $grades = array($this->grade_items[0]->id=>0, // 0 out of 110. Should be excluded from aggregation. + $this->grade_items[1]->id=>5, // 5 out of 100 + $this->grade_items[2]->id=>0, // 0 out of 6 + $this->grade_items[4]->id=>0); // 0 out of 100. Should be excluded from aggregation. + $category->apply_limit_rules($grades, $items); + $this->assertEquals(count($grades), 2); + $this->assertEquals($grades[$this->grade_items[1]->id], 5); + $this->assertEquals($grades[$this->grade_items[2]->id], 0); + + // Test excluding the lowest 5 out of 4 grades from aggregation + // Just to check we handle this sensibly + $category = new grade_category(); + $category->droplow = 5; + $category->aggregation = GRADE_AGGREGATE_WEIGHTED_MEAN2; // simple weighted mean + $grades = array($this->grade_items[0]->id=>0, // 0 out of 110. Should be excluded from aggregation. + $this->grade_items[1]->id=>5, // 5 out of 100 + $this->grade_items[2]->id=>6, // 6 out of 6 + $this->grade_items[4]->id=>1);// 1 out of 100. Should be excluded from aggregation. + $category->apply_limit_rules($grades, $items); + $this->assertEquals(count($grades), 0); + + // Test excluding the lowest 4 out of 4 grades from aggregation with one marked as extra credit + $category = new grade_category(); + $category->droplow = 4; + $category->aggregation = GRADE_AGGREGATE_WEIGHTED_MEAN2; // simple weighted mean + $items[$this->grade_items[2]->id]->aggregationcoef = 1; // Mark grade item 2 as "extra credit" + $grades = array($this->grade_items[0]->id=>0, // 0 out of 110. Should be excluded from aggregation. + $this->grade_items[1]->id=>5, // 5 out of 100. Should be excluded from aggregation. + $this->grade_items[2]->id=>6, // 6 out of 6. Extra credit. Should be retained. + $this->grade_items[4]->id=>1);// 1 out of 100. Should be excluded from aggregation. + $category->apply_limit_rules($grades, $items); + $this->assertEquals(count($grades), 1); + $this->assertEquals($grades[$this->grade_items[2]->id], 6); + } /**