Skip to content

Commit

Permalink
MDL-49257 grades: fix bugs with extra credit weights
Browse files Browse the repository at this point in the history 
When we adjust the weights in natural grading (setup screen), the extra credit items
should not depend on other overrides. If extra credit item's weight was overriden, it stays as it is.
Otherwise it is calculated as itemgrademax / totalgrademax (total excludes extra credit items)
  • Loading branch information
@marinaglancy
marinaglancy committed Jun 23, 2015
1 parent deb3d5e commit 156d048
Show file tree
Hide file tree
Showing 7 changed files with 167 additions and 4 deletions.
6 changes: 6 additions & 0 deletions backup/moodle2/restore_stepslib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,12 @@ protected function gradebook_calculation_freeze() {
$gradebookcalculationsfreeze = get_config('core', 'gradebook_calculations_freeze_' . $this->get_courseid());
preg_match('/(\d{8})/', $this->get_task()->get_info()->moodle_release, $matches);
$backupbuild = (int)$matches[1];

// Extra credits need adjustments only for backups made between 2.8 release (20141110) and the fix release (20150619).
if (!$gradebookcalculationsfreeze && $backupbuild >= 20141110 && $backupbuild < 20150619) {
require_once($CFG->libdir . '/db/upgradelib.php');
upgrade_extra_credit_weightoverride($this->get_courseid());
}
}
}

Expand Down
1 change: 1 addition & 0 deletions lib/db/install.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ function xmldb_main_install() {
'filterall' => 0, // setting page, so have to be initialised here.
'texteditors' => 'atto,tinymce,textarea',
'upgrade_minmaxgradestepignored' => 1, // New installs should not run this upgrade step.
'upgrade_extracreditweightsstepignored' => 1, // New installs should not run this upgrade step.
);
foreach($defaults as $key => $value) {
set_config($key, $value);
Expand Down
20 changes: 20 additions & 0 deletions lib/db/upgrade.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4399,5 +4399,25 @@ function xmldb_main_upgrade($oldversion) {
upgrade_main_savepoint(true, 2015060400.02);
}

if ($oldversion < 2015061900.00) {
// MDL-49257. Changed the algorithm of calculating automatic weights of extra credit items.

// Before the change, in case when grade category (in "Natural" agg. method) had items with
// overridden weights, the automatic weight of extra credit items was illogical.
// In order to prevent grades changes after the upgrade we need to freeze gradebook calculation
// for the affected courses.

// This script in included in each major version upgrade process so make sure we don't run it twice.
if (empty($CFG->upgrade_extracreditweightsstepignored)) {
upgrade_extra_credit_weightoverride();

// To skip running the same script on the upgrade to the next major release.
set_config('upgrade_extracreditweightsstepignored', 1);
}

// Main savepoint reached.
upgrade_main_savepoint(true, 2015061900.00);
}

return true;
}
35 changes: 35 additions & 0 deletions lib/db/upgradelib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -529,4 +529,39 @@ function upgrade_mimetypes($filetypes) {
array($extension)
);
}
}

/**
* Marks all courses with changes in extra credit weight calculation
*
* Used during upgrade and in course restore process
*
* This upgrade script is needed because we changed the algorithm for calculating the automatic weights of extra
* credit items and want to prevent changes in the existing student grades.
*
* @param int $onlycourseid
*/
function upgrade_extra_credit_weightoverride($onlycourseid = 0) {
global $DB;

// Find all courses that have categories in Natural aggregation method where there is at least one extra credit
// item and at least one item with overridden weight.
$courses = $DB->get_fieldset_sql(
"SELECT DISTINCT gc.courseid
FROM {grade_categories} gc
INNER JOIN {grade_items} gi ON gc.id = gi.categoryid AND gi.weightoverride = :weightoverriden
INNER JOIN {grade_items} gie ON gc.id = gie.categoryid AND gie.aggregationcoef = :extracredit
WHERE gc.aggregation = :naturalaggmethod" . ($onlycourseid ? " AND gc.courseid = :onlycourseid" : ''),
array('naturalaggmethod' => 13,
'weightoverriden' => 1,
'extracredit' => 1,
'onlycourseid' => $onlycourseid,
)
);
foreach ($courses as $courseid) {
$gradebookfreeze = get_config('core', 'gradebook_calculations_freeze_' . $courseid);
if (!$gradebookfreeze) {
set_config('gradebook_calculations_freeze_' . $courseid, 20150619);
}
}
}
44 changes: 41 additions & 3 deletions lib/grade/grade_category.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1166,6 +1166,12 @@ public function aggregate_values_and_adjust_bounds($grade_values,
$this->load_grade_item();
$num = count($grade_values);
$sum = 0;

// This setting indicates if we should use algorithm prior to MDL-49257 fix for calculating extra credit weights.
// Even though old algorith has bugs in it, we need to preserve existing grades.
$gradebookcalculationfreeze = (int)get_config('core', 'gradebook_calculations_freeze_' . $this->courseid);
$oldextracreditcalculation = $gradebookcalculationfreeze && ($gradebookcalculationfreeze <= 20150619);

$sumweights = 0;
$grademin = 0;
$grademax = 0;
Expand Down Expand Up @@ -1205,7 +1211,11 @@ public function aggregate_values_and_adjust_bounds($grade_values,
$userweights[$itemid] = 0;
continue;
}
$userweights[$itemid] = $items[$itemid]->aggregationcoef2 / $sumweights;
$userweights[$itemid] = $sumweights ? ($items[$itemid]->aggregationcoef2 / $sumweights) : 0;
if (!$oldextracreditcalculation && isset($extracredititems[$itemid])) {
// Extra credit items do not affect totals.
continue;
}
$totaloverriddenweight += $userweights[$itemid];
$usergrademax = $items[$itemid]->grademax;
if (isset($grademaxoverrides[$itemid])) {
Expand All @@ -1216,9 +1226,9 @@ public function aggregate_values_and_adjust_bounds($grade_values,
}
$nonoverriddenpoints = $grademax - $totaloverriddengrademax;

// Then we need to recalculate the automatic weights.
// Then we need to recalculate the automatic weights except for extra credit items.
foreach ($grade_values as $itemid => $gradevalue) {
if (!$items[$itemid]->weightoverride) {
if (!$items[$itemid]->weightoverride && ($oldextracreditcalculation || !isset($extracredititems[$itemid]))) {
$usergrademax = $items[$itemid]->grademax;
if (isset($grademaxoverrides[$itemid])) {
$usergrademax = $grademaxoverrides[$itemid];
Expand All @@ -1236,6 +1246,19 @@ public function aggregate_values_and_adjust_bounds($grade_values,
}
}

// Now when we finally know the grademax we can adjust the automatic weights of extra credit items.
if (!$oldextracreditcalculation) {
foreach ($grade_values as $itemid => $gradevalue) {
if (!$items[$itemid]->weightoverride && isset($extracredititems[$itemid])) {
$usergrademax = $items[$itemid]->grademax;
if (isset($grademaxoverrides[$itemid])) {
$usergrademax = $grademaxoverrides[$itemid];
}
$userweights[$itemid] = $grademax ? ($usergrademax / $grademax) : 0;
}
}
}

// We can use our freshly corrected weights below.
foreach ($grade_values as $itemid => $gradevalue) {
if (isset($extracredititems[$itemid])) {
Expand Down Expand Up @@ -1517,6 +1540,11 @@ private function auto_update_weights() {

$totalnonoverriddengrademax = $totalgrademax - $totaloverriddengrademax;

// This setting indicates if we should use algorithm prior to MDL-49257 fix for calculating extra credit weights.
// Even though old algorith has bugs in it, we need to preserve existing grades.
$gradebookcalculationfreeze = (int)get_config('core', 'gradebook_calculations_freeze_' . $this->courseid);
$oldextracreditcalculation = $gradebookcalculationfreeze && ($gradebookcalculationfreeze <= 20150619);

reset($children);
foreach ($children as $sortorder => $child) {
$gradeitem = null;
Expand All @@ -1539,6 +1567,16 @@ private function auto_update_weights() {
continue;
}

if (!$oldextracreditcalculation && $gradeitem->aggregationcoef > 0) {
// For an item with extra credit ignore other weigths and overrides.
// Do not change anything at all if it's weight was already overridden.
if (!$gradeitem->weightoverride) {
$gradeitem->aggregationcoef2 = $totalgrademax ? ($gradeitem->grademax / $totalgrademax) : 0;
$gradeitem->update();
}
continue;
}

if (!$gradeitem->weightoverride) {
// Calculations with a grade maximum of zero will cause problems. Just set the weight to zero.
if ($totaloverriddenweight >= 1 || $totalnonoverriddengrademax == 0 || $gradeitem->grademax == 0) {
Expand Down
63 changes: 63 additions & 0 deletions lib/tests/upgradelib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -482,4 +482,67 @@ public function test_upgrade_minmaxgrade() {
// Restore value.
$CFG->grade_minmaxtouse = $initialminmax;
}

public function test_upgrade_extra_credit_weightoverride() {
global $DB, $CFG;

$this->resetAfterTest(true);

$c = array();
$a = array();
$gi = array();
for ($i=0; $i<5; $i++) {
$c[$i] = $this->getDataGenerator()->create_course();
$a[$i] = array();
$gi[$i] = array();
for ($j=0;$j<3;$j++) {
$a[$i][$j] = $this->getDataGenerator()->create_module('assign', array('course' => $c[$i], 'grade' => 100));
$giparams = array('itemtype' => 'mod', 'itemmodule' => 'assign', 'iteminstance' => $a[$i][$j]->id,
'courseid' => $c[$i]->id, 'itemnumber' => 0);
$gi[$i][$j] = grade_item::fetch($giparams);
}
}

// Case 1: Course $c[0] has aggregation method different from natural.
$coursecategory = grade_category::fetch_course_category($c[0]->id);
$coursecategory->aggregation = GRADE_AGGREGATE_WEIGHTED_MEAN;
$coursecategory->update();
$gi[0][1]->aggregationcoef = 1;
$gi[0][1]->update();
$gi[0][2]->weightoverride = 1;
$gi[0][2]->update();

// Case 2: Course $c[1] has neither extra credits nor overrides

// Case 3: Course $c[2] has extra credits but no overrides
$gi[2][1]->aggregationcoef = 1;
$gi[2][1]->update();

// Case 4: Course $c[3] has no extra credits and has overrides
$gi[3][2]->weightoverride = 1;
$gi[3][2]->update();

// Case 5: Course $c[4] has both extra credits and overrides
$gi[4][1]->aggregationcoef = 1;
$gi[4][1]->update();
$gi[4][2]->weightoverride = 1;
$gi[4][2]->update();

// Run the upgrade script and make sure only course $c[4] was marked as needed to be fixed.
upgrade_extra_credit_weightoverride();

$this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $c[0]->id}));
$this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $c[1]->id}));
$this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $c[2]->id}));
$this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $c[3]->id}));
$this->assertEquals(20150619, $CFG->{'gradebook_calculations_freeze_' . $c[4]->id});

set_config('gradebook_calculations_freeze_' . $c[4]->id, null);

// Run the upgrade script for a single course only.
upgrade_extra_credit_weightoverride($c[0]->id);
$this->assertTrue(empty($CFG->{'gradebook_calculations_freeze_' . $c[0]->id}));
upgrade_extra_credit_weightoverride($c[4]->id);
$this->assertEquals(20150619, $CFG->{'gradebook_calculations_freeze_' . $c[4]->id});
}
}
2 changes: 1 addition & 1 deletion version.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

defined('MOODLE_INTERNAL') || die();

$version = 2015061800.00; // YYYYMMDD = weekly release date of this DEV branch.
$version = 2015061900.00; // YYYYMMDD = weekly release date of this DEV branch.
// RR = release increments - 00 in DEV branches.
// .XX = incremental changes.

Expand Down

0 comments on commit 156d048

Please sign in to comment.