From 154a72b0024761f8fe57ede5aa1b2255a1c2babd Mon Sep 17 00:00:00 2001 From: Aaron Barnes Date: Mon, 16 Jul 2012 14:49:39 +1200 Subject: [PATCH] MDL-34332 completion: timeenrolled not always set correctly --- completion/completion_completion.php | 52 ++++++- completion/cron.php | 128 ++++------------ .../tests/completion_completion_test.php | 120 +++++++++++++++ completion/tests/cron_test.php | 101 +++++++++++++ completion/tests/lib.php | 143 ++++++++++++++++++ lib/phpunit/classes/data_generator.php | 60 ++++++++ 6 files changed, 503 insertions(+), 101 deletions(-) create mode 100644 completion/tests/completion_completion_test.php create mode 100644 completion/tests/cron_test.php create mode 100644 completion/tests/lib.php diff --git a/completion/completion_completion.php b/completion/completion_completion.php index e8e80197bdf..27ef1894e98 100644 --- a/completion/completion_completion.php +++ b/completion/completion_completion.php @@ -165,18 +165,68 @@ class completion_completion extends data_object { * Save course completion status * * This method creates a course_completions record if none exists + * and also calculates the timeenrolled date if the record is being + * created + * * @access private * @return bool */ private function _save() { - if ($this->timeenrolled === null) { + // Make sure timeenrolled is not null + if (!$this->timeenrolled) { $this->timeenrolled = 0; } // Save record if ($this->id) { + // Update return $this->update(); } else { + // Create new + if (!$this->timeenrolled) { + global $DB; + + // Get earliest current enrolment start date + // This means timeend > now() and timestart < now() + $sql = " + SELECT + ue.timestart + FROM + {user_enrolments} ue + JOIN + {enrol} e + ON (e.id = ue.enrolid AND e.courseid = :courseid) + WHERE + ue.userid = :userid + AND ue.status = :active + AND e.status = :enabled + AND ( + ue.timeend = 0 + OR ue.timeend > :now + ) + AND ue.timeend < :now2 + ORDER BY + ue.timestart ASC + "; + $params = array( + 'enabled' => ENROL_INSTANCE_ENABLED, + 'active' => ENROL_USER_ACTIVE, + 'userid' => $this->userid, + 'courseid' => $this->course, + 'now' => time(), + 'now2' => time() + ); + + if ($enrolments = $DB->get_record_sql($sql, $params, IGNORE_MULTIPLE)) { + $this->timeenrolled = $enrolments->timestart; + } + + // If no timeenrolled could be found, use current time + if (!$this->timeenrolled) { + $this->timeenrolled = time(); + } + } + // Make sure reaggregate field is not null if (!$this->reaggregate) { $this->reaggregate = 0; diff --git a/completion/cron.php b/completion/cron.php index 23e682c84c6..cc595aefe58 100644 --- a/completion/cron.php +++ b/completion/cron.php @@ -48,19 +48,12 @@ function completion_cron() { * @return void */ function completion_cron_mark_started() { - global $CFG, $DB; + global $DB; if (debugging()) { mtrace('Marking users as started'); } - if (!empty($CFG->gradebookroles)) { - $roles = ' AND ra.roleid IN ('.$CFG->gradebookroles.')'; - } else { - // This causes it to default to everyone (if there is no student role) - $roles = ''; - } - /** * A quick explaination of this horrible looking query * @@ -77,118 +70,53 @@ function completion_cron_mark_started() { * of multiple records for each couse/user in the results */ $sql = " + INSERT INTO + {course_completions} + (course, userid, timeenrolled, timestarted, reaggregate) SELECT c.id AS course, - u.id AS userid, - crc.id AS completionid, - ue.timestart AS timeenrolled, - ue.timecreated + ue.userid AS userid, + CASE + WHEN MIN(ue.timestart) <> 0 + THEN MIN(ue.timestart) + ELSE ? + END, + 0, + ? FROM - {user} u - INNER JOIN {user_enrolments} ue - ON ue.userid = u.id INNER JOIN {enrol} e ON e.id = ue.enrolid INNER JOIN {course} c ON c.id = e.courseid - INNER JOIN - {role_assignments} ra - ON ra.userid = u.id LEFT JOIN {course_completions} crc ON crc.course = c.id - AND crc.userid = u.id + AND crc.userid = ue.userid WHERE c.enablecompletion = 1 - AND crc.timeenrolled IS NULL - AND ue.status = 0 - AND e.status = 0 - AND u.deleted = 0 + AND crc.id IS NULL + AND ue.status = ? + AND e.status = ? AND ue.timestart < ? AND (ue.timeend > ? OR ue.timeend = 0) - $roles - ORDER BY - course, - userid + GROUP BY + c.id, + ue.userid "; $now = time(); - $rs = $DB->get_recordset_sql($sql, array($now, $now, $now, $now)); - - // Check if result is empty - if (!$rs->valid()) { - $rs->close(); // Not going to iterate (but exit), close rs - return; - } - - /** - * An explaination of the following loop - * - * We are essentially doing a group by in the code here (as I can't find - * a decent way of doing it in the sql). - * - * Since there can be multiple enrolment plugins for each course, we can have - * multiple rows for each particpant in the query result. This isn't really - * a problem until you combine it with the fact that the enrolment plugins - * can save the enrol start time in either timestart or timeenrolled. - * - * The purpose of this loop is to find the earliest enrolment start time for - * each participant in each course. - */ - $prev = null; - while ($rs->valid() || $prev) { - - $current = $rs->current(); - - if (!isset($current->course)) { - $current = false; - } - else { - // Not all enrol plugins fill out timestart correctly, so use whichever - // is non-zero - $current->timeenrolled = max($current->timecreated, $current->timeenrolled); - } - - // If we are at the last record, - // or we aren't at the first and the record is for a diff user/course - if ($prev && - (!$rs->valid() || - ($current->course != $prev->course || $current->userid != $prev->userid))) { - - $completion = new completion_completion(); - $completion->userid = $prev->userid; - $completion->course = $prev->course; - $completion->timeenrolled = (string) $prev->timeenrolled; - $completion->timestarted = 0; - $completion->reaggregate = time(); - - if ($prev->completionid) { - $completion->id = $prev->completionid; - } - - $completion->mark_enrolled(); - - if (debugging()) { - mtrace('Marked started user '.$prev->userid.' in course '.$prev->course); - } - } - // Else, if this record is for the same user/course - elseif ($prev && $current) { - // Use oldest timeenrolled - $current->timeenrolled = min($current->timeenrolled, $prev->timeenrolled); - } - - // Move current record to previous - $prev = $current; - - // Move to next record - $rs->next(); - } - - $rs->close(); + $params = array( + $now, + $now, + ENROL_USER_ACTIVE, + ENROL_INSTANCE_ENABLED, + $now, + $now + ); + $affected = $DB->execute($sql, $params, true); } /** diff --git a/completion/tests/completion_completion_test.php b/completion/tests/completion_completion_test.php new file mode 100644 index 00000000000..c116077959b --- /dev/null +++ b/completion/tests/completion_completion_test.php @@ -0,0 +1,120 @@ +. + +/** + * Course completion completion_completion unit tests + * + * @package core + * @category phpunit + * @copyright 2012 Aaron Barnes + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once("{$CFG->dirroot}/completion/tests/lib.php"); + +class completion_completion_testcase extends completion_testcase { + + /** + * Things this test needs to be checking for: + * - No users are missed! + * - The function handles already started users correctly + * e.g. does not alter the record in any way + * - The handling of users with multiple enrolments in the same course + * e.g. the lowest non-zero timestart from an active enrolment is used as timeenrolled + * - The correct times are used for users enrolled in multiple courses + * - Users who have no current enrolments are not handled + * - Ignore courses with completion disabled + */ + public function test_completion_completion_save() { + global $CFG, $DB; + + $this->_create_complex_testdata(); + + $course1 = $this->_testcourses[1]; + $course2 = $this->_testcourses[2]; + $course3 = $this->_testcourses[3]; + + $user1 = $this->_testusers[1]; + $user2 = $this->_testusers[2]; + $user3 = $this->_testusers[3]; + $user4 = $this->_testusers[4]; + + $now = $this->_testdates['now']; + $past = $this->_testdates['past']; + $future = $this->_testdates['future']; + + // Run functionality to test + require_once("{$CFG->dirroot}/completion/completion_completion.php"); + $completions = array( + $course1->id => array( + $user1->id, $user2->id, $user3->id, $user4->id + ), + $course2->id => array( + $user1->id, $user2->id, $user3->id, $user4->id + ) + ); + + foreach ($completions as $course => $users) { + foreach ($users as $user) { + $params = array( + 'userid' => $user, + 'course' => $course + ); + $completion = new completion_completion($params, true); + $completion->mark_inprogress(); + } + } + + // Load all records for these courses in course_completions + // Return results indexed by userid (which will not hide duplicates due to their being a unique index on that and the course columns) + $cc1 = $DB->get_records('course_completions', array('course' => $course1->id), '', 'userid, *'); + $cc2 = $DB->get_records('course_completions', array('course' => $course2->id), '', 'userid, *'); + $cc3 = $DB->get_records('course_completions', array('course' => $course3->id), '', 'userid, *'); + + // Test results + // Check correct number of records + $this->assertEquals(4, $DB->count_records('course_completions', array('course' => $course1->id))); + $this->assertEquals(4, $DB->count_records('course_completions', array('course' => $course2->id))); + + // All users should be mark started in course1 + $this->assertEquals($past-2, $cc1[$user2->id]->timeenrolled); + $this->assertGreaterThanOrEqual($now, $cc1[$user4->id]->timeenrolled); + $this->assertLessThan($now + 60, $cc1[$user4->id]->timeenrolled); + + // User1 should have a timeenrolled in course1 of $past-5 (due to multiple enrolments) + $this->assertEquals($past-5, $cc1[$user1->id]->timeenrolled); + + // User3 should have a timeenrolled in course1 of $past-2 (due to multiple enrolments) + $this->assertEquals($past-2, $cc1[$user3->id]->timeenrolled); + + // User 2 was not enrolled in course2 (nothing current) so timeenrolled should be current time + $this->assertGreaterThanOrEqual($now, $cc2[$user2->id]->timeenrolled); + $this->assertLessThan($now + 60, $cc2[$user2->id]->timeenrolled); + + // Add some enrolment to course2 with different times to check for bugs + $this->assertEquals($past-10, $cc2[$user1->id]->timeenrolled); + $this->assertEquals($past-15, $cc2[$user3->id]->timeenrolled); + + // Add enrolment in course2 for user4 (who will be already started) + $this->assertEquals($past-50, $cc2[$user4->id]->timeenrolled); + + // Check no records in course with completion disabled + $this->assertEquals(0, $DB->count_records('course_completions', array('course' => $course3->id))); + } +} diff --git a/completion/tests/cron_test.php b/completion/tests/cron_test.php new file mode 100644 index 00000000000..8e11fbb371c --- /dev/null +++ b/completion/tests/cron_test.php @@ -0,0 +1,101 @@ +. + +/** + * Course completion cron unit tests + * + * @package core + * @category phpunit + * @copyright 2012 Aaron Barnes + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +global $CFG; +require_once("{$CFG->dirroot}/completion/tests/lib.php"); + +class completioncron_testcase extends completion_testcase { + + /** + * Things this test needs to be checking for: + * - No users are missed! + * - The function handles already started users correctly + * e.g. does not alter the record in any way + * - The handling of users with multiple enrolments in the same course + * e.g. the lowest non-zero timestart from an active enrolment is used as timeenrolled + * - The correct times are used for users enrolled in multiple courses + * - Users who have no current enrolments are not handled + * - Ignore courses with completion disabled + */ + public function test_completion_cron_mark_started() { + global $CFG, $DB; + + $this->_create_complex_testdata(); + + $course1 = $this->_testcourses[1]; + $course2 = $this->_testcourses[2]; + $course3 = $this->_testcourses[3]; + + $user1 = $this->_testusers[1]; + $user2 = $this->_testusers[2]; + $user3 = $this->_testusers[3]; + $user4 = $this->_testusers[4]; + + $now = $this->_testdates['now']; + $past = $this->_testdates['past']; + $future = $this->_testdates['future']; + + // Run cron function to test + require_once("{$CFG->dirroot}/completion/cron.php"); + completion_cron_mark_started(); + + // Load all records for these courses in course_completions + // Return results indexed by userid (which will not hide duplicates due to their being a unique index on that and the course columns) + $cc1 = $DB->get_records('course_completions', array('course' => $course1->id), '', 'userid, *'); + $cc2 = $DB->get_records('course_completions', array('course' => $course2->id), '', 'userid, *'); + $cc3 = $DB->get_records('course_completions', array('course' => $course3->id), '', 'userid, *'); + + // Test results + // Check correct number of records + $this->assertEquals(4, $DB->count_records('course_completions', array('course' => $course1->id))); + $this->assertEquals(3, $DB->count_records('course_completions', array('course' => $course2->id))); + + // All users should be mark started in course1 + $this->assertEquals($past-2, $cc1[$user2->id]->timeenrolled); + $this->assertGreaterThanOrEqual($now, $cc1[$user4->id]->timeenrolled); + $this->assertLessThan($now + 60, $cc1[$user4->id]->timeenrolled); + + // User1 should have a timeenrolled in course1 of $past-5 (due to multiple enrolments) + $this->assertEquals($past-5, $cc1[$user1->id]->timeenrolled); + + // User3 should have a timeenrolled in course1 of $past-2 (due to multiple enrolments) + $this->assertEquals($past-2, $cc1[$user3->id]->timeenrolled); + + // User 2 should not be mark as started in course2 at all (nothing current) + $this->assertEquals(false, isset($cc2[$user2->id])); + + // Add some enrolment to course2 with different times to check for bugs + $this->assertEquals($past-10, $cc2[$user1->id]->timeenrolled); + $this->assertEquals($past-15, $cc2[$user3->id]->timeenrolled); + + // Add enrolment in course2 for user4 (who will be already started) + $this->assertEquals($past-50, $cc2[$user4->id]->timeenrolled); + + // Check no records in course with completion disabled + $this->assertEquals(0, $DB->count_records('course_completions', array('course' => $course3->id))); + } +} diff --git a/completion/tests/lib.php b/completion/tests/lib.php new file mode 100644 index 00000000000..db17ab6ba91 --- /dev/null +++ b/completion/tests/lib.php @@ -0,0 +1,143 @@ +. + +/** + * Course completion unit test helper + * + * @package core + * @category phpunit + * @copyright 2012 Aaron Barnes + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +abstract class completion_testcase extends advanced_testcase { + + /** + * Test data + */ + protected $_testcourses = array(); + protected $_testusers = array(); + protected $_testdates = array(); + + protected function _create_complex_testdata() { + global $DB; + + $this->resetAfterTest(true); + + $gen = $this->getDataGenerator(); + + // Setup a couple of courses + $courseparams = array('enablecompletion' => 1, 'completionstartonenrol' => 1); + $course1 = $gen->create_course($courseparams); + $course2 = $gen->create_course($courseparams); + $course3 = $gen->create_course(); + + // Make all enrolment plugins enabled + $DB->execute( + " + UPDATE + {enrol} + SET + status = ? + WHERE + courseid IN (?, ?, ?) + ", + array( + ENROL_INSTANCE_ENABLED, + $course1->id, $course2->id, $course3->id + ) + ); + + // Setup some users + $user1 = $gen->create_user(); + $user2 = $gen->create_user(); + $user3 = $gen->create_user(); + $user4 = $gen->create_user(); + + // Enrol the users + $now = time(); + $past = $now - (60*60*24); + $future = $now + (60*60*24); + + $enrolments = array( + // All users should be mark started in course1 + array($course1->id, $user1->id, $past, 0, null), + array($course1->id, $user2->id, $past-2, 0, null), + array($course1->id, $user3->id, 0, 0, null), + array($course1->id, $user4->id, 0, 0, null), + // User1 should have a timeenrolled in course1 of $past-5 (due to multiple enrolments) + array($course1->id, $user1->id, $past-5, 0, 'self'), + // User3 should have a timeenrolled in course1 of $past-2 (due to multiple enrolments) + array($course1->id, $user3->id, $past-2, 0, 'self'), + array($course1->id, $user3->id, $past-100, $past, null), // in the past + array($course1->id, $user3->id, $future, $future+100, null), // in the future + // User 2 should not be mark as started in course2 at all (nothing current) + array($course2->id, $user2->id, $future, 0, null), + array($course2->id, $user2->id, 0, $past, null), + // Add some enrolment to course2 with different times to check for bugs + array($course2->id, $user1->id, $past-10, 0, null), + array($course2->id, $user3->id, $past-15, 0, null), + // Add enrolment in course2 for user4 (who will be already started) + array($course2->id, $user4->id, $past-13, 0, null), + // Add enrolment in course3 even though completion is not enabled + array($course3->id, $user1->id, 0, 0, null), + // Add multiple enrolments of same type! + ); + + foreach ($enrolments as $enrol) { + $enrol = array( + 'courseid' => $enrol[0], + 'userid' => $enrol[1], + 'timestart' => $enrol[2], + 'timeend' => $enrol[3], + 'plugin' => $enrol[4] + ); + if (!$gen->create_enrolment($enrol)) { + throw new coding_exception('error creating enrolments in test_completion_cron_mark_started()'); + } + } + + // Delete all old records in case they were missed + $DB->delete_records('course_completions', array('course' => $course1->id)); + $DB->delete_records('course_completions', array('course' => $course2->id)); + $DB->delete_records('course_completions', array('course' => $course3->id)); + + // Create course_completions record for user4 in course2 + $params = array( + 'course' => $course2->id, + 'userid' => $user4->id, + 'timeenrolled' => $past-50, + 'reaggregate' => 0 + ); + $DB->insert_record('course_completions', $params); + + + $this->_testcourses[1] = $course1; + $this->_testcourses[2] = $course2; + $this->_testcourses[3] = $course3; + + $this->_testusers[1] = $user1; + $this->_testusers[2] = $user2; + $this->_testusers[3] = $user3; + $this->_testusers[4] = $user4; + + $this->_testdates['now'] = $now; + $this->_testdates['past'] = $past; + $this->_testdates['future'] = $future; + } +} diff --git a/lib/phpunit/classes/data_generator.php b/lib/phpunit/classes/data_generator.php index 48a459f1d9f..5bcf0d9208b 100644 --- a/lib/phpunit/classes/data_generator.php +++ b/lib/phpunit/classes/data_generator.php @@ -541,4 +541,64 @@ EOD; return $DB->get_record('scale', array('id'=>$id), '*', MUST_EXIST); } + + /** + * Create a test enrolment + * @param array|stdClass $record + * @return boolean + */ + function create_enrolment($record = null) { + global $DB, $CFG; + require_once("{$CFG->libdir}/enrollib.php"); + + $record = (array)$record; + + if (empty($record['courseid'])) { + throw new coding_exception('courseid must be present in phpunit_util::create_enrolment() $record'); + } + + if (empty($record['userid'])) { + throw new coding_exception('userid must be present in phpunit_util::create_enrolment() $record'); + } + + if (empty($record['roleid'])) { + $record['roleid'] = null; + } + + if (empty($record['timestart'])) { + $record['timestart'] = 0; + } + + if (empty($record['timeend'])) { + $record['timeend'] = 0; + } + + if (empty($record['plugin'])) { + $record['plugin'] = 'manual'; + } + + if (!enrol_is_enabled($record['plugin'])) { + return false; + } + + if (!$enrol = enrol_get_plugin($record['plugin'])) { + return false; + } + + $params = array( + 'enrol' => $record['plugin'], + 'courseid' => $record['courseid'], + 'status' => ENROL_INSTANCE_ENABLED + ); + + if (!$instances = $DB->get_records('enrol', $params, 'sortorder,id ASC')) { + return false; + } + + $instance = reset($instances); + + $enrol->enrol_user($instance, $record['userid'], $record['roleid'], $record['timestart'], $record['timeend']); + + return true; + } } -- 2.17.1