Skip to content

Commit

Permalink
MDL-41062 gradebook: remove sortorder duplicates
Browse files Browse the repository at this point in the history 
* 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
@danpoltawski @marinaglancy
danpoltawski authored and marinaglancy committed Jan 17, 2014
1 parent f05e25d commit 2a9d7a4
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 1 deletion.
8 changes: 8 additions & 0 deletions lib/db/upgrade.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
184 changes: 184 additions & 0 deletions lib/tests/upgradelib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
35 changes: 35 additions & 0 deletions lib/upgradelib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
2 changes: 1 addition & 1 deletion version.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down

0 comments on commit 2a9d7a4

Please sign in to comment.