Permalink
Browse files

MDL-34332 completion: timeenrolled not always set correctly

  • Loading branch information...
1 parent c92d6f4 commit 154a72b0024761f8fe57ede5aa1b2255a1c2babd Aaron Barnes committed Jul 16, 2012
@@ -165,18 +165,68 @@ public function mark_complete($timecomplete = null) {
* 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;
View
@@ -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);
}
/**
@@ -0,0 +1,120 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle. If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Course completion completion_completion unit tests
+ *
+ * @package core
+ * @category phpunit
+ * @copyright 2012 Aaron Barnes <aaronb@catalyst.net.nz>
+ * @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)));
+ }
+}
Oops, something went wrong.

0 comments on commit 154a72b

Please sign in to comment.