diff --git a/backup/moodle2/restore_stepslib.php b/backup/moodle2/restore_stepslib.php index ec11ffdc484..78de7b50695 100644 --- a/backup/moodle2/restore_stepslib.php +++ b/backup/moodle2/restore_stepslib.php @@ -480,6 +480,12 @@ class restore_gradebook_structure_step extends restore_structure_step { $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()); + } } } diff --git a/lib/db/install.php b/lib/db/install.php index 0d8e13297e0..04538bfb106 100644 --- a/lib/db/install.php +++ b/lib/db/install.php @@ -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); diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index fe2f44adb1b..d11e0187795 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -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; } diff --git a/lib/db/upgradelib.php b/lib/db/upgradelib.php index 81b92b5ef46..23af19e1486 100644 --- a/lib/db/upgradelib.php +++ b/lib/db/upgradelib.php @@ -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); + } + } } \ No newline at end of file diff --git a/lib/grade/grade_category.php b/lib/grade/grade_category.php index 3830b7a8b2c..0fde1008a63 100644 --- a/lib/grade/grade_category.php +++ b/lib/grade/grade_category.php @@ -1166,6 +1166,12 @@ class grade_category extends grade_object { $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; @@ -1205,7 +1211,11 @@ class grade_category extends grade_object { $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])) { @@ -1216,9 +1226,9 @@ class grade_category extends grade_object { } $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]; @@ -1236,6 +1246,19 @@ class grade_category extends grade_object { } } + // 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])) { @@ -1517,6 +1540,11 @@ class grade_category extends grade_object { $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; @@ -1539,6 +1567,16 @@ class grade_category extends grade_object { 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) { diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php index debdffd6db0..3a1d7241dc2 100644 --- a/lib/tests/upgradelib_test.php +++ b/lib/tests/upgradelib_test.php @@ -482,4 +482,67 @@ class core_upgradelib_testcase extends advanced_testcase { // 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}); + } } diff --git a/version.php b/version.php index 8503ad3c5bc..74fe9368558 100644 --- a/version.php +++ b/version.php @@ -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.