diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index 9650b7fff17..49376288a78 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2919,5 +2919,13 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2014011000.01); } + if ($oldversion < 2014011701.00) { + // Fix gradebook sortorder duplicates. + upgrade_grade_item_fix_sortorder(); + + // Main savepoint reached. + upgrade_main_savepoint(true, 2014011701.00); + } + return true; } diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php index 7e9be492643..0ff743f8f2e 100644 --- a/lib/tests/upgradelib_test.php +++ b/lib/tests/upgradelib_test.php @@ -42,4 +42,111 @@ class core_upgradelib_testcase extends advanced_testcase { // if there aren't any old files in the codebase. $this->assertFalse(upgrade_stale_php_files_present()); } + + /** + * Test the {@link upgrade_grade_item_fix_sortorder() function with + * faked duplicate sortorder data. + */ + public function test_upgrade_grade_item_fix_sortorder() { + global $DB; + + $this->resetAfterTest(true); + + // The purpose of this test is to make sure that after upgrade script + // there is no duplicates in the field grade_items.sortorder (for each course) + // and the result of query "SELECT id FROM grade_items WHERE courseid=? ORDER BY sortorder, id" does not change. + $sequencesql = 'SELECT id FROM {grade_items} WHERE courseid=? ORDER BY sortorder, id'; + + // Each set is used for filling the db with fake data and will be representing the result of query: + // "SELECT sortorder from {grade_items} WHERE courseid=? ORDER BY id". + $testsets = array( + // Items that need no action. + array(1,2,3), + array(5,6,7), + array(7,6,1,3,2,5), + // Items with sortorder duplicates + array(1,2,2,3,3,4,5), + // Only one sortorder duplicate. + array(1,1), + array(3,3), + // Non-sequential sortorders with one or multiple duplicates. + array(3,3,7,5,6,6,9,10,8,3), + array(7,7,3), + array(3,4,5,3,5,4,7,1) + ); + $origsequences = array(); + + // Generate the data and remember the initial sequence or items. + foreach ($testsets as $testset) { + $course = $this->getDataGenerator()->create_course(); + foreach ($testset as $sortorder) { + $this->insert_fake_grade_item_sortorder($course->id, $sortorder); + } + $DB->get_records('grade_items'); + $origsequences[$course->id] = $DB->get_fieldset_sql($sequencesql, array($course->id)); + } + + $duplicatedetectionsql = "SELECT courseid, sortorder + FROM {grade_items} + GROUP BY courseid, sortorder + HAVING COUNT(id) > 1"; + + // Verify there are duplicates before we start the fix. + $dupes = $DB->record_exists_sql($duplicatedetectionsql); + $this->assertTrue($dupes); + + // Do the work. + upgrade_grade_item_fix_sortorder(); + + // Verify that no duplicates are left in the database. + $dupes = $DB->record_exists_sql($duplicatedetectionsql); + $this->assertFalse($dupes); + + // Verify that sequences are exactly the same as they were before upgrade script. + $idx = 0; + foreach ($origsequences as $courseid => $origsequence) { + if (count(($testsets[$idx])) == count(array_unique($testsets[$idx]))) { + // If there were no duplicates for this course verify that sortorders are not modified. + $newsortorders = $DB->get_fieldset_sql("SELECT sortorder from {grade_items} WHERE courseid=? ORDER BY id", array($courseid)); + $this->assertEquals($testsets[$idx], $newsortorders); + } + $newsequence = $DB->get_fieldset_sql($sequencesql, array($courseid)); + $this->assertEquals($origsequence, $newsequence, + "Sequences do not match for test set $idx : ".join(',', $testsets[$idx])); + $idx++; + } + } + + /** + * Populate some fake grade items into the database with specified + * sortorder and course id. + * + * NOTE: This function doesn't make much attempt to respect the + * gradebook internals, its simply used to fake some data for + * testing the upgradelib function. Please don't use it for other + * purposes. + * + * @param int $courseid id of course + * @param int $sortorder numeric sorting order of item + * @return stdClass grade item object from the database. + */ + private function insert_fake_grade_item_sortorder($courseid, $sortorder) { + global $DB, $CFG; + require_once($CFG->libdir.'/gradelib.php'); + + $item = new stdClass(); + $item->courseid = $courseid; + $item->sortorder = $sortorder; + $item->gradetype = GRADE_TYPE_VALUE; + $item->grademin = 30; + $item->grademax = 110; + $item->itemnumber = 1; + $item->iteminfo = ''; + $item->timecreated = time(); + $item->timemodified = time(); + + $item->id = $DB->insert_record('grade_items', $item); + + return $DB->get_record('grade_items', array('id' => $item->id)); + } } diff --git a/lib/upgradelib.php b/lib/upgradelib.php index 0ec6e24b0a2..24563fdd69f 100644 --- a/lib/upgradelib.php +++ b/lib/upgradelib.php @@ -2049,3 +2049,42 @@ function upgrade_rename_old_backup_files_using_shortname() { @rename($dir . '/' . $file, $dir . '/' . $newname); } } + +/** + * Detect duplicate grade item sortorders and resort the + * items to remove them. + */ +function upgrade_grade_item_fix_sortorder() { + global $DB; + + // The simple way to fix these sortorder duplicates would be simply to resort each + // affected course. But in order to reduce the impact of this upgrade step we're trying + // to do it more efficiently by doing a series of update statements rather than updating + // every single grade item in affected courses. + + $transaction = $DB->start_delegated_transaction(); + + $sql = "SELECT g1.id, g1.courseid, g1.sortorder + FROM {grade_items} g1 + JOIN {grade_items} g2 ON g1.courseid = g2.courseid + WHERE g1.sortorder = g2.sortorder AND g1.id != g2.id + ORDER BY g1.courseid ASC, g1.sortorder DESC, g1.id DESC"; + + // Get all duplicates in course order, highest sort order, and higest id first so that we can make space at the + // bottom higher end of the sort orders and work down by id. + $rs = $DB->get_recordset_sql($sql); + + foreach($rs as $duplicate) { + $DB->execute("UPDATE {grade_items} + SET sortorder = sortorder + 1 + WHERE courseid = :courseid AND + (sortorder > :sortorder OR (sortorder = :sortorder2 AND id > :id))", + array('courseid' => $duplicate->courseid, + 'sortorder' => $duplicate->sortorder, + 'sortorder2' => $duplicate->sortorder, + 'id' => $duplicate->id)); + } + $rs->close(); + + $transaction->allow_commit(); +} diff --git a/version.php b/version.php index 86888464f8b..be8cb942217 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2014011700.00; // YYYYMMDD = weekly release date of this DEV branch. +$version = 2014011701.00; // YYYYMMDD = weekly release date of this DEV branch. // RR = release increments - 00 in DEV branches. // .XX = incremental changes.