diff --git a/lib/completionlib.php b/lib/completionlib.php index 4c8467bd0b9..cce41d0d922 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -935,13 +935,20 @@ class completion_info { function internal_set_data($cm, $data) { global $USER, $SESSION, $DB; - if ($data->id) { - // Has real (nonzero) id meaning that a database row exists - $DB->update_record('course_modules_completion', $data); - } else { + $transaction = $DB->start_delegated_transaction(); + if (!$data->id) { + // Check there isn't really a row + $data->id = $DB->get_field('course_modules_completion', 'id', + array('coursemoduleid'=>$data->coursemoduleid, 'userid'=>$data->userid)); + } + if (!$data->id) { // Didn't exist before, needs creating $data->id = $DB->insert_record('course_modules_completion', $data); + } else { + // Has real (nonzero) id meaning that a database row exists, update + $DB->update_record('course_modules_completion', $data); } + $transaction->allow_commit(); if ($data->userid == $USER->id) { $SESSION->completioncache[$cm->course][$cm->id] = $data; diff --git a/lib/db/install.xml b/lib/db/install.xml index 27b3c1e5b19..c83310c103f 100644 --- a/lib/db/install.xml +++ b/lib/db/install.xml @@ -367,8 +367,8 @@ - - + + diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index b47810c0c2c..54bd086443e 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -6062,6 +6062,60 @@ WHERE gradeitemid IS NOT NULL AND grademax IS NOT NULL"); upgrade_main_savepoint(true, 2011022100.01); } + if ($oldversion < 2011033003.09) { + // Completion system has issue in which possible duplicate rows are + // added to the course_modules_completion table. This change deletes + // the older version of duplicate rows and replaces an index with a + // unique one so it won't happen again. + + // This would have been a single query but because MySQL is a PoS + // and can't do subqueries in DELETE, I have made it into two. The + // system is unlikely to run out of memory as only IDs are stored in + // the array. + + // Find all rows cmc1 where there is another row cmc2 with the + // same user id and the same coursemoduleid, but a higher id (=> newer, + // meaning that cmc1 is an older row). + $rs = $DB->get_recordset_sql(" +SELECT DISTINCT + cmc1.id +FROM + {course_modules_completion} cmc1 + JOIN {course_modules_completion} cmc2 + ON cmc2.userid = cmc1.userid + AND cmc2.coursemoduleid = cmc1.coursemoduleid + AND cmc2.id > cmc1.id"); + $deleteids = array(); + foreach ($rs as $row) { + $deleteids[] = $row->id; + } + $rs->close(); + // Note: SELECT part performance tested on table with ~7m + // rows of which ~15k match, only took 30 seconds so probably okay. + + // Delete all those rows + $DB->delete_records_list('course_modules_completion', 'id', $deleteids); + + // Define index userid (not unique) to be dropped form course_modules_completion + $table = new xmldb_table('course_modules_completion'); + $index = new xmldb_index('userid', XMLDB_INDEX_NOTUNIQUE, array('userid')); + + // Conditionally launch drop index userid + if ($dbman->index_exists($table, $index)) { + $dbman->drop_index($table, $index); + } + + // Define index userid-coursemoduleid (unique) to be added to course_modules_completion + $index = new xmldb_index('userid-coursemoduleid', XMLDB_INDEX_UNIQUE, + array('userid', 'coursemoduleid')); + + // Conditionally launch add index userid-coursemoduleid + if (!$dbman->index_exists($table, $index)) { + $dbman->add_index($table, $index); + } + + upgrade_main_savepoint(true, 2011033003.09); + } return true; } diff --git a/lib/simpletest/testcompletionlib.php b/lib/simpletest/testcompletionlib.php index 66fc9cb7e52..f4288fd9eb0 100644 --- a/lib/simpletest/testcompletionlib.php +++ b/lib/simpletest/testcompletionlib.php @@ -6,6 +6,7 @@ require_once($CFG->libdir.'/completionlib.php'); global $DB; Mock::generate(get_class($DB), 'mock_database'); +Mock::generate('moodle_transaction', 'mock_transaction'); Mock::generatePartial('completion_info','completion_cutdown', array('delete_all_state','get_tracked_users','update_state', @@ -452,24 +453,40 @@ WHERE function test_internal_set_data() { global $DB,$SESSION; - $cm=(object)array('course'=>42,'id'=>13); - $c=new completion_info((object)array('id'=>42)); + $cm = (object)array('course' => 42,'id' => 13); + $c = new completion_info((object)array('id' => 42)); // 1) Test with new data - $data=(object)array('id'=>0,'userid'=>314159); - $DB->setReturnValueAt(0,'insert_record',4); - $DB->expectAt(0,'insert_record',array('course_modules_completion',$data)); - $c->internal_set_data($cm,$data); - $this->assertEqual(4,$data->id); - $this->assertEqual(array(42=>array(13=>$data)),$SESSION->completioncache); + $data = (object)array('id'=>0, 'userid' => 314159, 'coursemoduleid' => 99); + $DB->setReturnValueAt(0, 'start_delegated_transaction', new mock_transaction()); + $DB->setReturnValueAt(0, 'insert_record', 4); + $DB->expectAt(0, 'get_field', array('course_modules_completion', 'id', + array('coursemoduleid' => 99, 'userid' => 314159))); + $DB->expectAt(0, 'insert_record', array('course_modules_completion', $data)); + $c->internal_set_data($cm, $data); + $this->assertEqual(4, $data->id); + $this->assertEqual(array(42 => array(13 => $data)), $SESSION->completioncache); // 2) Test with existing data and for different user (not cached) unset($SESSION->completioncache); - $d2=(object)array('id'=>7,'userid'=>17); - $DB->expectAt(0,'update_record',array('course_modules_completion',$d2)); - $c->internal_set_data($cm,$d2); + $d2 = (object)array('id' => 7, 'userid' => 17, 'coursemoduleid' => 66); + $DB->setReturnValueAt(1, 'start_delegated_transaction', new mock_transaction()); + $DB->expectAt(0,'update_record', array('course_modules_completion', $d2)); + $c->internal_set_data($cm, $d2); $this->assertFalse(isset($SESSION->completioncache)); + // 3) Test where it THINKS the data is new (from cache) but actually + // in the database it has been set since + // 1) Test with new data + $data = (object)array('id'=>0, 'userid' => 314159, 'coursemoduleid' => 99); + $DB->setReturnValueAt(2, 'start_delegated_transaction', new mock_transaction()); + $DB->setReturnValueAt(1, 'get_field', 13); + $DB->expectAt(1, 'get_field', array('course_modules_completion', 'id', + array('coursemoduleid' => 99, 'userid' => 314159))); + $d3 = (object)array('id' => 13, 'userid' => 314159, 'coursemoduleid' => 99); + $DB->expectAt(1,'update_record', array('course_modules_completion', $d3)); + $c->internal_set_data($cm, $data); + $DB->tally(); } diff --git a/version.php b/version.php index 06757b0563e..175573d1920 100644 --- a/version.php +++ b/version.php @@ -30,7 +30,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2011033003.08; // 20110330 = branching date YYYYMMDD - do not modify! +$version = 2011033003.09; // 20110330 = branching date YYYYMMDD - do not modify! // RR = release version - do not change in weeklies // .XX = incremental changes