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.
This commit is contained in:
Dan Poltawski
committed by
Marina Glancy
Marina Glancy
parent
6370d316fd
commit
4468527db9
@@ -2321,6 +2321,13 @@ function xmldb_main_upgrade($oldversion) {
|
||||
upgrade_main_savepoint(true, 2013051403.09);
|
||||
}
|
||||
|
||||
if ($oldversion < 2013051404.02) {
|
||||
// Fix gradebook sortorder duplicates.
|
||||
upgrade_grade_item_fix_sortorder();
|
||||
|
||||
// Main savepoint reached.
|
||||
upgrade_main_savepoint(true, 2013051404.02);
|
||||
}
|
||||
|
||||
return true;
|
||||
}
|
||||
|
||||
@@ -42,4 +42,188 @@ class upgradelib_test 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));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
+1
-1
@@ -29,7 +29,7 @@
|
||||
|
||||
defined('MOODLE_INTERNAL') || die();
|
||||
|
||||
$version = 2013051404.01; // 20130514 = branching date YYYYMMDD - do not modify!
|
||||
$version = 2013051404.02; // 20130514 = branching date YYYYMMDD - do not modify!
|
||||
// RR = release increments - 00 in DEV branches
|
||||
// .XX = incremental changes
|
||||
|
||||
|
||||
Reference in New Issue
Block a user