From 926f51436bae84e3d3adfb373563bb2e2f5a05ec Mon Sep 17 00:00:00 2001 From: Dan Poltawski Date: Thu, 1 Nov 2012 12:02:49 +0800 Subject: [PATCH] Revert "MDL-34332 completion: timeenrolled not always set correctly" This reverts commit 154a72b0024761f8fe57ede5aa1b2255a1c2babd. --- 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, 101 insertions(+), 503 deletions(-) delete mode 100644 completion/tests/completion_completion_test.php delete mode 100644 completion/tests/cron_test.php delete mode 100644 completion/tests/lib.php diff --git a/completion/completion_completion.php b/completion/completion_completion.php index d0b42a3a51e..dab34d8b453 100644 --- a/completion/completion_completion.php +++ b/completion/completion_completion.php @@ -169,68 +169,18 @@ 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() { - // Make sure timeenrolled is not null - if (!$this->timeenrolled) { + if ($this->timeenrolled === null) { $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 cc595aefe58..23e682c84c6 100644 --- a/completion/cron.php +++ b/completion/cron.php @@ -48,12 +48,19 @@ function completion_cron() { * @return void */ function completion_cron_mark_started() { - global $DB; + global $CFG, $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 * @@ -70,53 +77,118 @@ 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, - ue.userid AS userid, - CASE - WHEN MIN(ue.timestart) <> 0 - THEN MIN(ue.timestart) - ELSE ? - END, - 0, - ? + u.id AS userid, + crc.id AS completionid, + ue.timestart AS timeenrolled, + ue.timecreated 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 = ue.userid + AND crc.userid = u.id WHERE c.enablecompletion = 1 - AND crc.id IS NULL - AND ue.status = ? - AND e.status = ? + AND crc.timeenrolled IS NULL + AND ue.status = 0 + AND e.status = 0 + AND u.deleted = 0 AND ue.timestart < ? AND (ue.timeend > ? OR ue.timeend = 0) - GROUP BY - c.id, - ue.userid + $roles + ORDER BY + course, + userid "; $now = time(); - $params = array( - $now, - $now, - ENROL_USER_ACTIVE, - ENROL_INSTANCE_ENABLED, - $now, - $now - ); - $affected = $DB->execute($sql, $params, true); + $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(); } /** diff --git a/completion/tests/completion_completion_test.php b/completion/tests/completion_completion_test.php deleted file mode 100644 index c116077959b..00000000000 --- a/completion/tests/completion_completion_test.php +++ /dev/null @@ -1,120 +0,0 @@ -. - -/** - * 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 deleted file mode 100644 index 8e11fbb371c..00000000000 --- a/completion/tests/cron_test.php +++ /dev/null @@ -1,101 +0,0 @@ -. - -/** - * 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 deleted file mode 100644 index db17ab6ba91..00000000000 --- a/completion/tests/lib.php +++ /dev/null @@ -1,143 +0,0 @@ -. - -/** - * 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 1bb8716e7fd..93f28f9cf7c 100644 --- a/lib/phpunit/classes/data_generator.php +++ b/lib/phpunit/classes/data_generator.php @@ -566,64 +566,4 @@ 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; - } }