MDL-34332 completion: timeenrolled not always set correctly
authorAaron Barnes <aaronb@catalyst.net.nz>
Mon, 16 Jul 2012 02:49:39 +0000 (14:49 +1200)
committerAaron Barnes <aaronb@catalyst.net.nz>
Mon, 20 Aug 2012 04:51:51 +0000 (16:51 +1200)
completion/completion_completion.php
completion/cron.php
completion/tests/completion_completion_test.php [new file with mode: 0644]
completion/tests/cron_test.php [new file with mode: 0644]
completion/tests/lib.php [new file with mode: 0644]
lib/phpunit/classes/data_generator.php

index e8e8019..27ef189 100644 (file)
@@ -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;
index 23e682c..cc595ae 100644 (file)
@@ -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 (file)
index 0000000..c116077
--- /dev/null
@@ -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)));
+    }
+}
diff --git a/completion/tests/cron_test.php b/completion/tests/cron_test.php
new file mode 100644 (file)
index 0000000..8e11fbb
--- /dev/null
@@ -0,0 +1,101 @@
+<?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 cron 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 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 (file)
index 0000000..db17ab6
--- /dev/null
@@ -0,0 +1,143 @@
+<?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 unit test helper
+ *
+ * @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();
+
+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;
+    }
+}
index 48a459f..5bcf0d9 100644 (file)
@@ -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;
+    }
 }