L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Revert "MDL-34332 completion: timeenrolled not always set correctly"

Browse Source 
This reverts commit 154a72b002.
This commit is contained in:
Dan Poltawski
2012-11-01 12:02:49 +08:00
parent 92c99211af
commit 926f51436b
6 changed files with 101 additions and 503 deletions
Download Patch File Download Diff File Expand all files Collapse all files
completion/completion_completion.php
+1 -51
View File
@@ -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;
completion/cron.php
+100 -28
View File
@@ -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();
}
/**
completion/tests/completion_completion_test.php
-120
View File
@@ -1,120 +0,0 @@
<?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 <[email protected]>
* @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)));
}
}
completion/tests/cron_test.php
-101
View File
@@ -1,101 +0,0 @@
<?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 <[email protected]>
* @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)));
}
}
completion/tests/lib.php
-143
View File
@@ -1,143 +0,0 @@
<?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 <[email protected]>
* @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;
}
}
lib/phpunit/classes/data_generator.php
-60
View File
@@ -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;
}
}