From 15fac639562216bb64970c7dd4ff783139fd8c57 Mon Sep 17 00:00:00 2001 From: Dan Poltawski Date: Tue, 14 Jan 2014 14:53:03 +0800 Subject: [PATCH 1/2] MDL-41062 gradebook: remove sortorder duplicates * Upgrade function to remove duplicates from the grade item duplicates column. Duplicates were causing sorting to fail in some cases. * Add some unit tests which simulate sort order duplicate data and verify that they have been removed. --- lib/db/upgrade.php | 8 ++ lib/tests/upgradelib_test.php | 184 ++++++++++++++++++++++++++++++++++ lib/upgradelib.php | 35 +++++++ version.php | 2 +- 4 files changed, 228 insertions(+), 1 deletion(-) diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index ee5afb4f50a..74f6ac16a2c 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -2903,5 +2903,13 @@ function xmldb_main_upgrade($oldversion) { upgrade_main_savepoint(true, 2013111800.08); } + if ($oldversion < 2013111801.02) { + // Fix gradebook sortorder duplicates. + upgrade_grade_item_fix_sortorder(); + + // Main savepoint reached. + upgrade_main_savepoint(true, 2013111801.02); + } + return true; } diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php index 7e9be492643..2cd498f3f1e 100644 --- a/lib/tests/upgradelib_test.php +++ b/lib/tests/upgradelib_test.php @@ -42,4 +42,188 @@ 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); + + // Create fake items in course1, with sortorder duplicates. + $course1 = $this->getDataGenerator()->create_course(); + $course1item = array(); + $course1item[0] = $this->insert_fake_grade_item_sortorder($course1->id, 1); + + $course1item[1] = $this->insert_fake_grade_item_sortorder($course1->id, 2); + $course1item[2] = $this->insert_fake_grade_item_sortorder($course1->id, 2); + + $course1item[3] = $this->insert_fake_grade_item_sortorder($course1->id, 3); + $course1item[4] = $this->insert_fake_grade_item_sortorder($course1->id, 3); + + $course1item[5] = $this->insert_fake_grade_item_sortorder($course1->id, 4); + $course1item[6] = $this->insert_fake_grade_item_sortorder($course1->id, 5); + + // Create fake items in course2 which need no action. + $course2 = $this->getDataGenerator()->create_course(); + $course2item = array(); + $course2item[0] = $this->insert_fake_grade_item_sortorder($course2->id, 1); + $course2item[1] = $this->insert_fake_grade_item_sortorder($course2->id, 2); + $course2item[2] = $this->insert_fake_grade_item_sortorder($course2->id, 3); + + // Create a new course which only has sortorder duplicates. + $course3 = $this->getDataGenerator()->create_course(); + $course3item = array(); + $course3item[0] = $this->insert_fake_grade_item_sortorder($course3->id, 1); + $course3item[1] = $this->insert_fake_grade_item_sortorder($course3->id, 1); + + // A course with non-sequential sortorders and duplicates. + $course4 = $this->getDataGenerator()->create_course(); + $course4item = array(); + $course4item[0] = $this->insert_fake_grade_item_sortorder($course4->id, 3); + $course4item[1] = $this->insert_fake_grade_item_sortorder($course4->id, 3); + + $course4item[2] = $this->insert_fake_grade_item_sortorder($course4->id, 5); + $course4item[3] = $this->insert_fake_grade_item_sortorder($course4->id, 6); + $course4item[4] = $this->insert_fake_grade_item_sortorder($course4->id, 6); + + $course4item[5] = $this->insert_fake_grade_item_sortorder($course4->id, 9); + $course4item[6] = $this->insert_fake_grade_item_sortorder($course4->id, 10); + // Create some items with non-sequential id and sortorder relationship. + $course4item[7] = $this->insert_fake_grade_item_sortorder($course4->id, 7); + $course4item[8] = $this->insert_fake_grade_item_sortorder($course4->id, 8); + + $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); + + // Load all grade items for ease. + $afterfixgradeitems = $DB->get_records('grade_items'); + + // Verify that the duplicate sortorders have been removed from course1. + $this->assertNotEquals($afterfixgradeitems[$course1item[1]->id]->sortorder, + $afterfixgradeitems[$course1item[2]->id]->sortorder); + $this->assertNotEquals($afterfixgradeitems[$course1item[3]->id]->sortorder, + $afterfixgradeitems[$course1item[4]->id]->sortorder); + // Verify that the order has been respected in course1. + $this->assertGreaterThan($afterfixgradeitems[$course1item[0]->id]->sortorder, + $afterfixgradeitems[$course1item[1]->id]->sortorder); + $this->assertGreaterThan($afterfixgradeitems[$course1item[2]->id]->sortorder, + $afterfixgradeitems[$course1item[3]->id]->sortorder); + $this->assertGreaterThan($afterfixgradeitems[$course1item[3]->id]->sortorder, + $afterfixgradeitems[$course1item[5]->id]->sortorder); + $this->assertGreaterThan($afterfixgradeitems[$course1item[5]->id]->sortorder, + $afterfixgradeitems[$course1item[6]->id]->sortorder); + + // Verify that no other fields have been modified in course1. + foreach ($course1item as $originalitem) { + $newitem = $afterfixgradeitems[$originalitem->id]; + + // Ignore changes to sortorder. + unset($originalitem->sortorder); + unset($newitem->sortorder); + + $this->assertEquals($originalitem, $newitem); + } + + // Verify that course2 items are completely unmodified. + foreach ($course2item as $originalitem) { + $newitem = $afterfixgradeitems[$originalitem->id]; + $this->assertEquals($originalitem, $newitem); + } + + // Verify that the duplicates in course3 have been removed. + $this->assertNotEquals($afterfixgradeitems[$course3item[0]->id]->sortorder, + $afterfixgradeitems[$course3item[1]->id]->sortorder); + + // Verify that no other fields in course3 have been modified. + foreach ($course3item as $originalitem) { + $newitem = $afterfixgradeitems[$originalitem->id]; + + // Ignore changes to sortorder. + unset($originalitem->sortorder); + unset($newitem->sortorder); + + $this->assertEquals($originalitem, $newitem); + } + + // Verify that the duplicates in course4 have been removed. + $this->assertNotEquals($afterfixgradeitems[$course4item[0]->id]->sortorder, + $afterfixgradeitems[$course4item[1]->id]->sortorder); + $this->assertNotEquals($afterfixgradeitems[$course4item[3]->id]->sortorder, + $afterfixgradeitems[$course4item[4]->id]->sortorder); + + // Verify that the order has been respected in course4. + $this->assertGreaterThan($afterfixgradeitems[$course4item[1]->id]->sortorder, + $afterfixgradeitems[$course4item[2]->id]->sortorder, "2 grater than 1"); + $this->assertGreaterThan($afterfixgradeitems[$course4item[4]->id]->sortorder, + $afterfixgradeitems[$course4item[5]->id]->sortorder); + $this->assertGreaterThan($afterfixgradeitems[$course4item[5]->id]->sortorder, + $afterfixgradeitems[$course4item[6]->id]->sortorder); + + // Check the items created with non-sequential id and sortorder relationship + // are converted correclty. + $this->assertGreaterThan($afterfixgradeitems[$course4item[7]->id]->sortorder, + $afterfixgradeitems[$course4item[5]->id]->sortorder); + $this->assertGreaterThan($afterfixgradeitems[$course4item[7]->id]->sortorder, + $afterfixgradeitems[$course4item[8]->id]->sortorder); + + // Verify that no other fields in course4 have been modified. + foreach ($course4item as $originalitem) { + $newitem = $afterfixgradeitems[$originalitem->id]; + + // Ignore changes to sortorder. + unset($originalitem->sortorder); + unset($newitem->sortorder); + + $this->assertEquals($originalitem, $newitem); + } + } + + /** + * 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 7f22c4d1a2e..db283559d2b 100644 --- a/lib/upgradelib.php +++ b/lib/upgradelib.php @@ -2049,3 +2049,38 @@ 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 AND id > :id", + array('courseid' => $duplicate->courseid, 'sortorder' =>$duplicate->sortorder, 'id' => $duplicate->id)); + } + $rs->close(); + + $transaction->allow_commit(); +} diff --git a/version.php b/version.php index 2106932a939..e6cbcf9bb99 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2013111801.01; // 20131118 = branching date YYYYMMDD - do not modify! +$version = 2013111801.02; // 20131118 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches. // .XX = incremental changes. From 8de910013682260e7ee72c772aa1374b02f44e72 Mon Sep 17 00:00:00 2001 From: Marina Glancy Date: Tue, 14 Jan 2014 14:54:00 +0800 Subject: [PATCH 2/2] MDL-41062 gradebook: corrections to grade_item upgrade script, more tests --- lib/tests/upgradelib_test.php | 163 +++++++++------------------------- lib/upgradelib.php | 8 +- 2 files changed, 49 insertions(+), 122 deletions(-) diff --git a/lib/tests/upgradelib_test.php b/lib/tests/upgradelib_test.php index 2cd498f3f1e..0ff743f8f2e 100644 --- a/lib/tests/upgradelib_test.php +++ b/lib/tests/upgradelib_test.php @@ -52,48 +52,39 @@ class core_upgradelib_testcase extends advanced_testcase { $this->resetAfterTest(true); - // Create fake items in course1, with sortorder duplicates. - $course1 = $this->getDataGenerator()->create_course(); - $course1item = array(); - $course1item[0] = $this->insert_fake_grade_item_sortorder($course1->id, 1); + // 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'; - $course1item[1] = $this->insert_fake_grade_item_sortorder($course1->id, 2); - $course1item[2] = $this->insert_fake_grade_item_sortorder($course1->id, 2); + // 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(); - $course1item[3] = $this->insert_fake_grade_item_sortorder($course1->id, 3); - $course1item[4] = $this->insert_fake_grade_item_sortorder($course1->id, 3); - - $course1item[5] = $this->insert_fake_grade_item_sortorder($course1->id, 4); - $course1item[6] = $this->insert_fake_grade_item_sortorder($course1->id, 5); - - // Create fake items in course2 which need no action. - $course2 = $this->getDataGenerator()->create_course(); - $course2item = array(); - $course2item[0] = $this->insert_fake_grade_item_sortorder($course2->id, 1); - $course2item[1] = $this->insert_fake_grade_item_sortorder($course2->id, 2); - $course2item[2] = $this->insert_fake_grade_item_sortorder($course2->id, 3); - - // Create a new course which only has sortorder duplicates. - $course3 = $this->getDataGenerator()->create_course(); - $course3item = array(); - $course3item[0] = $this->insert_fake_grade_item_sortorder($course3->id, 1); - $course3item[1] = $this->insert_fake_grade_item_sortorder($course3->id, 1); - - // A course with non-sequential sortorders and duplicates. - $course4 = $this->getDataGenerator()->create_course(); - $course4item = array(); - $course4item[0] = $this->insert_fake_grade_item_sortorder($course4->id, 3); - $course4item[1] = $this->insert_fake_grade_item_sortorder($course4->id, 3); - - $course4item[2] = $this->insert_fake_grade_item_sortorder($course4->id, 5); - $course4item[3] = $this->insert_fake_grade_item_sortorder($course4->id, 6); - $course4item[4] = $this->insert_fake_grade_item_sortorder($course4->id, 6); - - $course4item[5] = $this->insert_fake_grade_item_sortorder($course4->id, 9); - $course4item[6] = $this->insert_fake_grade_item_sortorder($course4->id, 10); - // Create some items with non-sequential id and sortorder relationship. - $course4item[7] = $this->insert_fake_grade_item_sortorder($course4->id, 7); - $course4item[8] = $this->insert_fake_grade_item_sortorder($course4->id, 8); + // 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} @@ -111,86 +102,18 @@ class core_upgradelib_testcase extends advanced_testcase { $dupes = $DB->record_exists_sql($duplicatedetectionsql); $this->assertFalse($dupes); - // Load all grade items for ease. - $afterfixgradeitems = $DB->get_records('grade_items'); - - // Verify that the duplicate sortorders have been removed from course1. - $this->assertNotEquals($afterfixgradeitems[$course1item[1]->id]->sortorder, - $afterfixgradeitems[$course1item[2]->id]->sortorder); - $this->assertNotEquals($afterfixgradeitems[$course1item[3]->id]->sortorder, - $afterfixgradeitems[$course1item[4]->id]->sortorder); - // Verify that the order has been respected in course1. - $this->assertGreaterThan($afterfixgradeitems[$course1item[0]->id]->sortorder, - $afterfixgradeitems[$course1item[1]->id]->sortorder); - $this->assertGreaterThan($afterfixgradeitems[$course1item[2]->id]->sortorder, - $afterfixgradeitems[$course1item[3]->id]->sortorder); - $this->assertGreaterThan($afterfixgradeitems[$course1item[3]->id]->sortorder, - $afterfixgradeitems[$course1item[5]->id]->sortorder); - $this->assertGreaterThan($afterfixgradeitems[$course1item[5]->id]->sortorder, - $afterfixgradeitems[$course1item[6]->id]->sortorder); - - // Verify that no other fields have been modified in course1. - foreach ($course1item as $originalitem) { - $newitem = $afterfixgradeitems[$originalitem->id]; - - // Ignore changes to sortorder. - unset($originalitem->sortorder); - unset($newitem->sortorder); - - $this->assertEquals($originalitem, $newitem); - } - - // Verify that course2 items are completely unmodified. - foreach ($course2item as $originalitem) { - $newitem = $afterfixgradeitems[$originalitem->id]; - $this->assertEquals($originalitem, $newitem); - } - - // Verify that the duplicates in course3 have been removed. - $this->assertNotEquals($afterfixgradeitems[$course3item[0]->id]->sortorder, - $afterfixgradeitems[$course3item[1]->id]->sortorder); - - // Verify that no other fields in course3 have been modified. - foreach ($course3item as $originalitem) { - $newitem = $afterfixgradeitems[$originalitem->id]; - - // Ignore changes to sortorder. - unset($originalitem->sortorder); - unset($newitem->sortorder); - - $this->assertEquals($originalitem, $newitem); - } - - // Verify that the duplicates in course4 have been removed. - $this->assertNotEquals($afterfixgradeitems[$course4item[0]->id]->sortorder, - $afterfixgradeitems[$course4item[1]->id]->sortorder); - $this->assertNotEquals($afterfixgradeitems[$course4item[3]->id]->sortorder, - $afterfixgradeitems[$course4item[4]->id]->sortorder); - - // Verify that the order has been respected in course4. - $this->assertGreaterThan($afterfixgradeitems[$course4item[1]->id]->sortorder, - $afterfixgradeitems[$course4item[2]->id]->sortorder, "2 grater than 1"); - $this->assertGreaterThan($afterfixgradeitems[$course4item[4]->id]->sortorder, - $afterfixgradeitems[$course4item[5]->id]->sortorder); - $this->assertGreaterThan($afterfixgradeitems[$course4item[5]->id]->sortorder, - $afterfixgradeitems[$course4item[6]->id]->sortorder); - - // Check the items created with non-sequential id and sortorder relationship - // are converted correclty. - $this->assertGreaterThan($afterfixgradeitems[$course4item[7]->id]->sortorder, - $afterfixgradeitems[$course4item[5]->id]->sortorder); - $this->assertGreaterThan($afterfixgradeitems[$course4item[7]->id]->sortorder, - $afterfixgradeitems[$course4item[8]->id]->sortorder); - - // Verify that no other fields in course4 have been modified. - foreach ($course4item as $originalitem) { - $newitem = $afterfixgradeitems[$originalitem->id]; - - // Ignore changes to sortorder. - unset($originalitem->sortorder); - unset($newitem->sortorder); - - $this->assertEquals($originalitem, $newitem); + // 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++; } } diff --git a/lib/upgradelib.php b/lib/upgradelib.php index db283559d2b..b5844a67dd0 100644 --- a/lib/upgradelib.php +++ b/lib/upgradelib.php @@ -2077,8 +2077,12 @@ function upgrade_grade_item_fix_sortorder() { foreach($rs as $duplicate) { $DB->execute("UPDATE {grade_items} SET sortorder = sortorder + 1 - WHERE courseid = :courseid AND sortorder >= :sortorder AND id > :id", - array('courseid' => $duplicate->courseid, 'sortorder' =>$duplicate->sortorder, 'id' => $duplicate->id)); + 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();