Permalink
Browse files

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.
  • Loading branch information...
1 parent 9c1c9d8 commit 15fac639562216bb64970c7dd4ff783139fd8c57 @danpoltawski danpoltawski committed with marinaglancy Jan 14, 2014
Showing with 228 additions and 1 deletion.
  1. +8 −0 lib/db/upgrade.php
  2. +184 −0 lib/tests/upgradelib_test.php
  3. +35 −0 lib/upgradelib.php
  4. +1 −1 version.php
View
@@ -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;
}
@@ -42,4 +42,188 @@ public function test_upgrade_stale_php_files_present() {
// 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));
+ }
}
View
@@ -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();
+}
View
@@ -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.

0 comments on commit 15fac63

Please sign in to comment.