From 156d048659c47dc7dd6e06ccdd4e7e2baa71bb24 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Fri, 19 Jun 2015 13:17:23 +0800 Subject: [PATCH] MDL-49257 grades: fix bugs with extra credit weights 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) --- backup/moodle2/restore_stepslib.php | 6 +++ lib/db/install.php | 1 + lib/db/upgrade.php | 20 +++++++++ lib/db/upgradelib.php | 35 ++++++++++++++++ lib/grade/grade_category.php | 44 ++++++++++++++++++-- lib/tests/upgradelib_test.php | 63 +++++++++++++++++++++++++++++ version.php | 2 +- 7 files changed, 167 insertions(+), 4 deletions(-) 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.