Skip to content

Commit 926f514

Browse files
danpoltawskidanpoltawski
committed
Revert "MDL-34332 completion: timeenrolled not always set correctly"
This reverts commit 154a72b.
1 parent 92c9921 commit 926f514

File tree

6 files changed

+101
-503
lines changed

6 files changed

+101
-503
lines changed

‎completion/completion_completion.php

+1-51
Original file line numberDiff line numberDiff line change
@@ -169,68 +169,18 @@ public function mark_complete($timecomplete = null) {
169169
* Save course completion status
170170
*
171171
* This method creates a course_completions record if none exists
172-
* and also calculates the timeenrolled date if the record is being
173-
* created
174-
*
175172
* @access private
176173
* @return bool
177174
*/
178175
private function _save() {
179-
// Make sure timeenrolled is not null
180-
if (!$this->timeenrolled) {
176+
if ($this->timeenrolled === null) {
181177
$this->timeenrolled = 0;
182178
}
183179

184180
// Save record
185181
if ($this->id) {
186-
// Update
187182
return $this->update();
188183
} else {
189-
// Create new
190-
if (!$this->timeenrolled) {
191-
global $DB;
192-
193-
// Get earliest current enrolment start date
194-
// This means timeend > now() and timestart < now()
195-
$sql = "
196-
SELECT
197-
ue.timestart
198-
FROM
199-
{user_enrolments} ue
200-
JOIN
201-
{enrol} e
202-
ON (e.id = ue.enrolid AND e.courseid = :courseid)
203-
WHERE
204-
ue.userid = :userid
205-
AND ue.status = :active
206-
AND e.status = :enabled
207-
AND (
208-
ue.timeend = 0
209-
OR ue.timeend > :now
210-
)
211-
AND ue.timeend < :now2
212-
ORDER BY
213-
ue.timestart ASC
214-
";
215-
$params = array(
216-
'enabled' => ENROL_INSTANCE_ENABLED,
217-
'active' => ENROL_USER_ACTIVE,
218-
'userid' => $this->userid,
219-
'courseid' => $this->course,
220-
'now' => time(),
221-
'now2' => time()
222-
);
223-
224-
if ($enrolments = $DB->get_record_sql($sql, $params, IGNORE_MULTIPLE)) {
225-
$this->timeenrolled = $enrolments->timestart;
226-
}
227-
228-
// If no timeenrolled could be found, use current time
229-
if (!$this->timeenrolled) {
230-
$this->timeenrolled = time();
231-
}
232-
}
233-
234184
// Make sure reaggregate field is not null
235185
if (!$this->reaggregate) {
236186
$this->reaggregate = 0;

‎completion/cron.php

+100-28
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,19 @@ function completion_cron() {
4848
* @return void
4949
*/
5050
function completion_cron_mark_started() {
51-
global $DB;
51+
global $CFG, $DB;
5252

5353
if (debugging()) {
5454
mtrace('Marking users as started');
5555
}
5656

57+
if (!empty($CFG->gradebookroles)) {
58+
$roles = ' AND ra.roleid IN ('.$CFG->gradebookroles.')';
59+
} else {
60+
// This causes it to default to everyone (if there is no student role)
61+
$roles = '';
62+
}
63+
5764
/**
5865
* A quick explaination of this horrible looking query
5966
*
@@ -70,53 +77,118 @@ function completion_cron_mark_started() {
7077
* of multiple records for each couse/user in the results
7178
*/
7279
$sql = "
73-
INSERT INTO
74-
{course_completions}
75-
(course, userid, timeenrolled, timestarted, reaggregate)
7680
SELECT
7781
c.id AS course,
78-
ue.userid AS userid,
79-
CASE
80-
WHEN MIN(ue.timestart) <> 0
81-
THEN MIN(ue.timestart)
82-
ELSE ?
83-
END,
84-
0,
85-
?
82+
u.id AS userid,
83+
crc.id AS completionid,
84+
ue.timestart AS timeenrolled,
85+
ue.timecreated
8686
FROM
87+
{user} u
88+
INNER JOIN
8789
{user_enrolments} ue
90+
ON ue.userid = u.id
8891
INNER JOIN
8992
{enrol} e
9093
ON e.id = ue.enrolid
9194
INNER JOIN
9295
{course} c
9396
ON c.id = e.courseid
97+
INNER JOIN
98+
{role_assignments} ra
99+
ON ra.userid = u.id
94100
LEFT JOIN
95101
{course_completions} crc
96102
ON crc.course = c.id
97-
AND crc.userid = ue.userid
103+
AND crc.userid = u.id
98104
WHERE
99105
c.enablecompletion = 1
100-
AND crc.id IS NULL
101-
AND ue.status = ?
102-
AND e.status = ?
106+
AND crc.timeenrolled IS NULL
107+
AND ue.status = 0
108+
AND e.status = 0
109+
AND u.deleted = 0
103110
AND ue.timestart < ?
104111
AND (ue.timeend > ? OR ue.timeend = 0)
105-
GROUP BY
106-
c.id,
107-
ue.userid
112+
$roles
113+
ORDER BY
114+
course,
115+
userid
108116
";
109117

110118
$now = time();
111-
$params = array(
112-
$now,
113-
$now,
114-
ENROL_USER_ACTIVE,
115-
ENROL_INSTANCE_ENABLED,
116-
$now,
117-
$now
118-
);
119-
$affected = $DB->execute($sql, $params, true);
119+
$rs = $DB->get_recordset_sql($sql, array($now, $now, $now, $now));
120+
121+
// Check if result is empty
122+
if (!$rs->valid()) {
123+
$rs->close(); // Not going to iterate (but exit), close rs
124+
return;
125+
}
126+
127+
/**
128+
* An explaination of the following loop
129+
*
130+
* We are essentially doing a group by in the code here (as I can't find
131+
* a decent way of doing it in the sql).
132+
*
133+
* Since there can be multiple enrolment plugins for each course, we can have
134+
* multiple rows for each particpant in the query result. This isn't really
135+
* a problem until you combine it with the fact that the enrolment plugins
136+
* can save the enrol start time in either timestart or timeenrolled.
137+
*
138+
* The purpose of this loop is to find the earliest enrolment start time for
139+
* each participant in each course.
140+
*/
141+
$prev = null;
142+
while ($rs->valid() || $prev) {
143+
144+
$current = $rs->current();
145+
146+
if (!isset($current->course)) {
147+
$current = false;
148+
}
149+
else {
150+
// Not all enrol plugins fill out timestart correctly, so use whichever
151+
// is non-zero
152+
$current->timeenrolled = max($current->timecreated, $current->timeenrolled);
153+
}
154+
155+
// If we are at the last record,
156+
// or we aren't at the first and the record is for a diff user/course
157+
if ($prev &&
158+
(!$rs->valid() ||
159+
($current->course != $prev->course || $current->userid != $prev->userid))) {
160+
161+
$completion = new completion_completion();
162+
$completion->userid = $prev->userid;
163+
$completion->course = $prev->course;
164+
$completion->timeenrolled = (string) $prev->timeenrolled;
165+
$completion->timestarted = 0;
166+
$completion->reaggregate = time();
167+
168+
if ($prev->completionid) {
169+
$completion->id = $prev->completionid;
170+
}
171+
172+
$completion->mark_enrolled();
173+
174+
if (debugging()) {
175+
mtrace('Marked started user '.$prev->userid.' in course '.$prev->course);
176+
}
177+
}
178+
// Else, if this record is for the same user/course
179+
elseif ($prev && $current) {
180+
// Use oldest timeenrolled
181+
$current->timeenrolled = min($current->timeenrolled, $prev->timeenrolled);
182+
}
183+
184+
// Move current record to previous
185+
$prev = $current;
186+
187+
// Move to next record
188+
$rs->next();
189+
}
190+
191+
$rs->close();
120192
}
121193

122194
/**

‎completion/tests/completion_completion_test.php

-120
This file was deleted.

0 commit comments

Comments
 (0)