From 846c5859100f10cbfc22f166d3bb121fbcbcc46b Mon Sep 17 00:00:00 2001 From: Simey Lameze Date: Fri, 5 Apr 2019 13:56:48 +0800 Subject: [PATCH 1/1] MDL-65276 core: fix CiBoT complains --- completion/criteria/completion_criteria.php | 4 +- lib/classes/task/completion_daily_task.php | 143 ++++++--------- lib/classes/task/completion_regular_task.php | 173 ++++++++++--------- lib/completionlib.php | 2 +- 4 files changed, 143 insertions(+), 179 deletions(-) diff --git a/completion/criteria/completion_criteria.php b/completion/criteria/completion_criteria.php index bec45a2f5c9..baa3977ead3 100644 --- a/completion/criteria/completion_criteria.php +++ b/completion/criteria/completion_criteria.php @@ -77,7 +77,9 @@ define('COMPLETION_CRITERIA_TYPE_ROLE', 7); define('COMPLETION_CRITERIA_TYPE_COURSE', 8); /** - * Criteria type constant to class name mapping + * Criteria type constant to class name mapping. + * + * This global variable would be improved if it was implemented as a cache. */ global $COMPLETION_CRITERIA_TYPES; $COMPLETION_CRITERIA_TYPES = array( diff --git a/lib/classes/task/completion_daily_task.php b/lib/classes/task/completion_daily_task.php index 4c770b6460f..2457200d3ad 100644 --- a/lib/classes/task/completion_daily_task.php +++ b/lib/classes/task/completion_daily_task.php @@ -52,103 +52,66 @@ class completion_daily_task extends scheduled_task { if (debugging()) { mtrace('Marking users as started'); } + + // This causes it to default to everyone (if there is no student role). + $sqlroles = ''; 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 = ''; + $sqlroles = ' AND ra.roleid IN (' . $CFG->gradebookroles.')'; } - /** - * A quick explaination of this horrible looking query - * - * It's purpose is to locate all the active participants - * of a course with course completion enabled. - * - * We also only want the users with no course_completions - * record as this functions job is to create the missing - * ones :) - * - * We want to record the user's enrolment start time for the - * course. This gets tricky because there can be multiple - * enrolment plugins active in a course, hence the possibility - * of multiple records for each couse/user in the results - */ - $sql = " - SELECT - c.id AS course, - 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 = u.id - WHERE - c.enablecompletion = 1 - 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) - $roles - ORDER BY - course, - userid - "; + + // It's purpose is to locate all the active participants of a course with course completion enabled. + // We also only want the users with no course_completions record as this functions job is to create + // the missing ones :) + // We want to record the user's enrolment start time for the course. This gets tricky because there can be + // multiple enrolment plugins active in a course, hence the possibility of multiple records for each + // couse/user in the results. + $sql = "SELECT c.id AS course, 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 = u.id + WHERE c.enablecompletion = 1 + 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) + $sqlroles + ORDER BY course, userid"; $now = time(); - $rs = $DB->get_recordset_sql($sql, array($now, $now, $now, $now)); - // Check if result is empty + $rs = $DB->get_recordset_sql($sql, [$now, $now, $now, $now]); + + // Check if result is empty. if (!$rs->valid()) { - $rs->close(); // Not going to iterate (but exit), close rs + // Not going to iterate (but exit), close rs. + $rs->close(); 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. - */ + + // 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 participant 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 the 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 + } 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() || + + // 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 = new \completion_completion(); $completion->userid = $prev->userid; $completion->course = $prev->course; $completion->timeenrolled = (string) $prev->timeenrolled; @@ -158,22 +121,20 @@ class completion_daily_task extends scheduled_task { $completion->id = $prev->completionid; } $completion->mark_enrolled(); + if (debugging()) { - mtrace('Marked started user '.$prev->userid.' in course '.$prev->course); + 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 + } else if ($prev && $current) { + // Else, if this record is for the same user/course use oldest timeenrolled. $current->timeenrolled = min($current->timeenrolled, $prev->timeenrolled); } - // Move current record to previous + // Move current record to previous. $prev = $current; - // Move to next record + // Move to next record. $rs->next(); } $rs->close(); } } - } diff --git a/lib/classes/task/completion_regular_task.php b/lib/classes/task/completion_regular_task.php index 3edea829c19..40d45728e15 100644 --- a/lib/classes/task/completion_regular_task.php +++ b/lib/classes/task/completion_regular_task.php @@ -51,10 +51,11 @@ class completion_regular_task extends scheduled_task { // Process each criteria type. foreach ($COMPLETION_CRITERIA_TYPES as $type) { - $object = 'completion_criteria_'.$type; - require_once $CFG->dirroot.'/completion/criteria/'.$object.'.php'; + $object = 'completion_criteria_' . $type; + require_once($CFG->dirroot . '/completion/criteria/' . $object . '.php'); + $class = new $object(); - // Run the criteria type's cron method, if it has one + // Run the criteria type's cron method, if it has one. if (method_exists($class, 'cron')) { if (debugging()) { mtrace('Running '.$object.'->cron()'); @@ -66,132 +67,132 @@ class completion_regular_task extends scheduled_task { if (debugging()) { mtrace('Aggregating completions'); } - // Save time started + + // Save time started. $timestarted = time(); - // Grab all criteria and their associated criteria completions - $sql = ' - SELECT DISTINCT - c.id AS course, - cr.id AS criteriaid, - crc.userid AS userid, - cr.criteriatype AS criteriatype, - cc.timecompleted AS timecompleted - FROM - {course_completion_criteria} cr - INNER JOIN - {course} c - ON cr.course = c.id - INNER JOIN - {course_completions} crc - ON crc.course = c.id - LEFT JOIN - {course_completion_crit_compl} cc - ON cc.criteriaid = cr.id - AND crc.userid = cc.userid - WHERE - c.enablecompletion = 1 - AND crc.timecompleted IS NULL - AND crc.reaggregate > 0 - AND crc.reaggregate < :timestarted - ORDER BY - course, - userid - '; - $rs = $DB->get_recordset_sql($sql, array('timestarted' => $timestarted)); - // Check if result is empty + + // Grab all criteria and their associated criteria completions. + $sql = 'SELECT DISTINCT c.id AS course, cr.id AS criteriaid, crc.userid AS userid, + cr.criteriatype AS criteriatype, cc.timecompleted AS timecompleted + FROM {course_completion_criteria} cr + INNER JOIN {course} c ON cr.course = c.id + INNER JOIN {course_completions} crc ON crc.course = c.id + LEFT JOIN {course_completion_crit_compl} cc ON cc.criteriaid = cr.id AND crc.userid = cc.userid + WHERE c.enablecompletion = 1 + AND crc.timecompleted IS NULL + AND crc.reaggregate > 0 + AND crc.reaggregate < :timestarted + ORDER BY course, userid'; + $rs = $DB->get_recordset_sql($sql, ['timestarted' => $timestarted]); + + // Check if result is empty. if (!$rs->valid()) { - $rs->close(); // Not going to iterate (but exit), close rs + $rs->close(); return; } - $current_user = null; - $current_course = null; - $completions = array(); + + $currentuser = null; + $currentcourse = null; + $completions = []; while (1) { - // Grab records for current user/course + // Grab records for current user/course. foreach ($rs as $record) { - // If we are still grabbing the same users completions - if ($record->userid === $current_user && $record->course === $current_course) { + // If we are still grabbing the same users completions. + if ($record->userid === $currentuser && $record->course === $currentcourse) { $completions[$record->criteriaid] = $record; } else { break; } } - // Aggregate + + // Aggregate. if (!empty($completions)) { if (debugging()) { - mtrace('Aggregating completions for user '.$current_user.' in course '.$current_course); + mtrace('Aggregating completions for user ' . $currentuser . ' in course ' . $currentcourse); } - // Get course info object - $info = new \completion_info((object)array('id' => $current_course)); - // Setup aggregation + + // Get course info object. + $info = new \completion_info((object)['id' => $currentcourse]); + + // Setup aggregation. $overall = $info->get_aggregation_method(); $activity = $info->get_aggregation_method(COMPLETION_CRITERIA_TYPE_ACTIVITY); $prerequisite = $info->get_aggregation_method(COMPLETION_CRITERIA_TYPE_COURSE); $role = $info->get_aggregation_method(COMPLETION_CRITERIA_TYPE_ROLE); - $overall_status = null; - $activity_status = null; - $prerequisite_status = null; - $role_status = null; - // Get latest timecompleted + + $overallstatus = null; + $activitystatus = null; + $prerequisitestatus = null; + $rolestatus = null; + + // Get latest timecompleted. $timecompleted = null; - // Check each of the criteria + + // Check each of the criteria. foreach ($completions as $params) { $timecompleted = max($timecompleted, $params->timecompleted); $completion = new \completion_criteria_completion((array)$params, false); - // Handle aggregation special cases + + // Handle aggregation special cases. if ($params->criteriatype == COMPLETION_CRITERIA_TYPE_ACTIVITY) { - completion_cron_aggregate($activity, $completion->is_complete(), $activity_status); + completion_cron_aggregate($activity, $completion->is_complete(), $activitystatus); } else if ($params->criteriatype == COMPLETION_CRITERIA_TYPE_COURSE) { - completion_cron_aggregate($prerequisite, $completion->is_complete(), $prerequisite_status); + completion_cron_aggregate($prerequisite, $completion->is_complete(), $prerequisitestatus); } else if ($params->criteriatype == COMPLETION_CRITERIA_TYPE_ROLE) { - completion_cron_aggregate($role, $completion->is_complete(), $role_status); + completion_cron_aggregate($role, $completion->is_complete(), $rolestatus); } else { - completion_cron_aggregate($overall, $completion->is_complete(), $overall_status); + completion_cron_aggregate($overall, $completion->is_complete(), $overallstatus); } } - // Include role criteria aggregation in overall aggregation - if ($role_status !== null) { - completion_cron_aggregate($overall, $role_status, $overall_status); + + // Include role criteria aggregation in overall aggregation. + if ($rolestatus !== null) { + completion_cron_aggregate($overall, $rolestatus, $overallstatus); } - // Include activity criteria aggregation in overall aggregation - if ($activity_status !== null) { - completion_cron_aggregate($overall, $activity_status, $overall_status); + + // Include activity criteria aggregation in overall aggregation. + if ($activitystatus !== null) { + completion_cron_aggregate($overall, $activitystatus, $overallstatus); } - // Include prerequisite criteria aggregation in overall aggregation - if ($prerequisite_status !== null) { - completion_cron_aggregate($overall, $prerequisite_status, $overall_status); + + // Include prerequisite criteria aggregation in overall aggregation. + if ($prerequisitestatus !== null) { + completion_cron_aggregate($overall, $prerequisitestatus, $overallstatus); } - // If aggregation status is true, mark course complete for user - if ($overall_status) { + + // If aggregation status is true, mark course complete for user. + if ($overallstatus) { if (debugging()) { mtrace('Marking complete'); } - $ccompletion = new \completion_completion(array('course' => $params->course, 'userid' => $params->userid)); + + $ccompletion = new \completion_completion([ + 'course' => $params->course, + 'userid' => $params->userid + ]); $ccompletion->mark_complete($timecompleted); } } - // If this is the end of the recordset, break the loop + + // If this is the end of the recordset, break the loop. if (!$rs->valid()) { $rs->close(); break; } - // New/next user, update user details, reset completions - $current_user = $record->userid; - $current_course = $record->course; - $completions = array(); + + // New/next user, update user details, reset completions. + $currentuser = $record->userid; + $currentcourse = $record->course; + $completions = []; $completions[$record->criteriaid] = $record; } - // Mark all users as aggregated - $sql = " - UPDATE - {course_completions} - SET - reaggregate = 0 - WHERE - reaggregate < :timestarted - AND reaggregate > 0 - "; - $DB->execute($sql, array('timestarted' => $timestarted)); + + // Mark all users as aggregated. + $sql = "UPDATE {course_completions} + SET reaggregate = 0 + WHERE reaggregate < :timestarted + AND reaggregate > 0"; + $DB->execute($sql, ['timestarted' => $timestarted]); } } diff --git a/lib/completionlib.php b/lib/completionlib.php index a84e3f6690e..1253ac1ea96 100644 --- a/lib/completionlib.php +++ b/lib/completionlib.php @@ -1384,7 +1384,7 @@ function completion_cron_aggregate($method, $data, &$state) { } else { $state = false; } - } elseif ($method == COMPLETION_AGGREGATION_ANY) { + } else if ($method == COMPLETION_AGGREGATION_ANY) { if ($data) { $state = true; } else if (!$data && $state === null) { -- 2.17.1