From fccc728fcce547f8deb5a5701a85f05bbe5dbac1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Monlla=C3=B3?= Date: Wed, 3 Apr 2019 19:46:03 +0200 Subject: [PATCH] MDL-64783 analytics: Performance improvements - Removed redundant query to analytics_predict_samples - Analysers API now uses recordsets to iterate through the analysable elements. They take the last analysed time into account. - New method for targets so there is no need to always update the last analysis time. Useful for lightweight targets. --- .../classes/output/invalid_analysables.php | 10 +- analytics/classes/analysable.php | 4 - analytics/classes/analysis.php | 116 ++++++------- analytics/classes/course.php | 16 +- analytics/classes/local/analyser/base.php | 95 ++++++++++- .../classes/local/analyser/by_course.php | 43 +++-- analytics/classes/local/analyser/sitewide.php | 11 +- analytics/classes/local/target/base.php | 13 ++ .../classes/local/time_splitting/base.php | 7 + analytics/classes/manager.php | 16 +- analytics/classes/user.php | 14 +- analytics/tests/fixtures/test_analysis.php | 4 +- analytics/tests/model_test.php | 9 +- analytics/upgrade.txt | 44 +++-- lib/classes/analytics/analyser/users.php | 38 +++-- lib/tests/analysers_test.php | 96 ++++++++++- lib/tests/fixtures/deprecated_analyser.php | 158 ++++++++++++++++++ .../target/upcoming_activities_due.php | 17 +- 18 files changed, 553 insertions(+), 158 deletions(-) create mode 100644 lib/tests/fixtures/deprecated_analyser.php diff --git a/admin/tool/analytics/classes/output/invalid_analysables.php b/admin/tool/analytics/classes/output/invalid_analysables.php index 0bb39025747..dbdaa00ef41 100644 --- a/admin/tool/analytics/classes/output/invalid_analysables.php +++ b/admin/tool/analytics/classes/output/invalid_analysables.php @@ -76,13 +76,17 @@ class invalid_analysables implements \renderable, \templatable { $offset = $this->page * $this->perpage; - $analysables = $this->model->get_analyser(['notimesplitting' => true])->get_analysables(); + $analysables = $this->model->get_analyser(['notimesplitting' => true])->get_analysables_iterator(); $skipped = 0; $enoughresults = false; $morepages = false; $results = array(); - foreach ($analysables as $key => $analysable) { + foreach ($analysables as $analysable) { + + if (!$analysable) { + continue; + } $validtraining = $this->model->get_target()->is_valid_analysable($analysable, true); if ($validtraining === true) { @@ -117,8 +121,6 @@ class invalid_analysables implements \renderable, \templatable { $morepages = true; break; } - - unset($analysables[$key]); } // Prepare the context object. diff --git a/analytics/classes/analysable.php b/analytics/classes/analysable.php index e9dcaae46de..97faf596e8e 100644 --- a/analytics/classes/analysable.php +++ b/analytics/classes/analysable.php @@ -29,10 +29,6 @@ defined('MOODLE_INTERNAL') || die(); /** * Any element analysers can analyse. * - * Analysers get_analysers method return all analysable elements in the site; - * it is important that analysable elements implement lazy loading to avoid - * big memory footprints. See \core_analytics\course example. - * * @package core_analytics * @copyright 2016 David Monllao {@link http://www.davidmonllao.com} * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later diff --git a/analytics/classes/analysis.php b/analytics/classes/analysis.php index e37491ed736..764b4ee48f8 100644 --- a/analytics/classes/analysis.php +++ b/analytics/classes/analysis.php @@ -83,15 +83,27 @@ class analysis { $filesbytimesplitting = array(); - list($analysables, $processedanalysables) = $this->get_sorted_analysables(); + $alreadyprocessedanalysables = $this->get_processed_analysables(); - $inittime = time(); - foreach ($analysables as $key => $analysable) { + if ($this->includetarget) { + $action = 'training'; + } else { + $action = 'prediction'; + } + $analysables = $this->analyser->get_analysables_iterator($action); + + $inittime = microtime(true); + foreach ($analysables as $analysable) { + $processed = false; + + if (!$analysable) { + continue; + } $analysableresults = $this->process_analysable($analysable); if ($analysableresults) { - $success = $this->result->add_analysable_results($analysableresults); - if (!$success) { + $processed = $this->result->add_analysable_results($analysableresults); + if (!$processed) { $errors = array(); foreach ($analysableresults as $timesplittingid => $result) { $str = ''; @@ -110,51 +122,18 @@ class analysis { } // Updated regardless of how well the analysis went. - $this->update_analysable_analysed_time($processedanalysables, $analysable->get_id()); + if ($this->analyser->get_target()->always_update_analysis_time() || $processed) { + $this->update_analysable_analysed_time($alreadyprocessedanalysables, $analysable->get_id()); + } // Apply time limit. if (!$options['evaluation']) { - $timespent = time() - $inittime; + $timespent = microtime(true) - $inittime; if ($modeltimelimit <= $timespent) { break; } } - - unset($analysables[$key]); } - - return true; - } - - /** - * Returns the list of analysables sorted in processing priority order. - * - * It will first return analysables that have never been analysed before - * and it will continue with the ones we have already seen by timeanalysed DESC - * order. - * - * @return array(0 => \core_analytics\analysable[], 1 => \stdClass[]) - */ - protected function get_sorted_analysables(): array { - - $analysables = $this->analyser->get_analysables(); - - // Get the list of analysables that have been already processed. - $processedanalysables = $this->get_processed_analysables(); - - // We want to start processing analysables we have not yet processed and later continue - // with analysables that we already processed. - $unseen = array_diff_key($analysables, $processedanalysables); - - // Var $processed first as we want to respect its timeanalysed DESC order so analysables that - // have recently been processed are on the bottom of the stack. - $seen = array_intersect_key($processedanalysables, $analysables); - array_walk($seen, function(&$value, $analysableid) use ($analysables) { - // We replace the analytics_used_analysables record by the analysable object. - $value = $analysables[$analysableid]; - }); - - return array($unseen + $seen, $processedanalysables); } /** @@ -294,7 +273,7 @@ class analysis { // Only when processing data for predictions. if (!$this->includetarget) { // We also filter out samples and ranges that have already been used for predictions. - $this->filter_out_prediction_samples_and_ranges($sampleids, $ranges, $timesplitting); + $predictsamplesrecord = $this->filter_out_prediction_samples_and_ranges($sampleids, $ranges, $timesplitting); } if (count($sampleids) === 0) { @@ -365,7 +344,9 @@ class analysis { if ($this->includetarget) { $this->save_train_samples($sampleids, $timesplitting); } else { - $this->save_prediction_samples($sampleids, $ranges, $timesplitting); + // The variable $predictsamplesrecord will always be set as filter_out_prediction_samples_and_ranges + // will always be called before it (no evaluation mode and no includetarget). + $this->save_prediction_samples($sampleids, $ranges, $timesplitting, $predictsamplesrecord); } } @@ -736,21 +717,17 @@ class analysis { * @param int[] $sampleids * @param array $ranges * @param \core_analytics\local\time_splitting\base $timesplitting - * @return null + * @return \stdClass|null The analytics_predict_samples record or null */ protected function filter_out_prediction_samples_and_ranges(array &$sampleids, array &$ranges, \core_analytics\local\time_splitting\base $timesplitting) { - global $DB; if (count($ranges) > 1) { throw new \coding_exception('$ranges argument should only contain one range'); } $rangeindex = key($ranges); - - $params = array('modelid' => $this->analyser->get_modelid(), 'analysableid' => $timesplitting->get_analysable()->get_id(), - 'timesplitting' => $timesplitting->get_id(), 'rangeindex' => $rangeindex); - $predictedrange = $DB->get_record('analytics_predict_samples', $params); + $predictedrange = $this->get_predict_samples_record($timesplitting, $rangeindex); if (!$predictedrange) { // Nothing to filter out. @@ -767,6 +744,18 @@ class analysis { // Replace the list of samples by the one excluding samples that already got predictions at this range. $sampleids = $missingsamples; + + return $predictedrange; + } + + private function get_predict_samples_record(\core_analytics\local\time_splitting\base $timesplitting, int $rangeindex) { + global $DB; + + $params = array('modelid' => $this->analyser->get_modelid(), 'analysableid' => $timesplitting->get_analysable()->get_id(), + 'timesplitting' => $timesplitting->get_id(), 'rangeindex' => $rangeindex); + $predictedrange = $DB->get_record('analytics_predict_samples', $params); + + return $predictedrange; } /** @@ -796,10 +785,11 @@ class analysis { * @param int[] $sampleids * @param array $ranges * @param \core_analytics\local\time_splitting\base $timesplitting + * @param ?\stdClass $predictsamplesrecord The existing record or null if there is no record yet. * @return null */ protected function save_prediction_samples(array $sampleids, array $ranges, - \core_analytics\local\time_splitting\base $timesplitting) { + \core_analytics\local\time_splitting\base $timesplitting, ?\stdClass $predictsamplesrecord) { global $DB; if (count($ranges) > 1) { @@ -808,20 +798,18 @@ class analysis { $rangeindex = key($ranges); - $params = array('modelid' => $this->analyser->get_modelid(), 'analysableid' => $timesplitting->get_analysable()->get_id(), - 'timesplitting' => $timesplitting->get_id(), 'rangeindex' => $rangeindex); - if ($predictionrange = $DB->get_record('analytics_predict_samples', $params)) { + if ($predictsamplesrecord) { // Append the new samples used for prediction. - $prevsamples = json_decode($predictionrange->sampleids, true); - $predictionrange->sampleids = json_encode($prevsamples + $sampleids); - $predictionrange->timemodified = time(); - $DB->update_record('analytics_predict_samples', $predictionrange); + $predictsamplesrecord->sampleids = json_encode($predictsamplesrecord->sampleids + $sampleids); + $predictsamplesrecord->timemodified = time(); + $DB->update_record('analytics_predict_samples', $predictsamplesrecord); } else { - $predictionrange = (object)$params; - $predictionrange->sampleids = json_encode($sampleids); - $predictionrange->timecreated = time(); - $predictionrange->timemodified = $predictionrange->timecreated; - $DB->insert_record('analytics_predict_samples', $predictionrange); + $predictsamplesrecord = (object)['modelid' => $this->analyser->get_modelid(), 'analysableid' => $timesplitting->get_analysable()->get_id(), + 'timesplitting' => $timesplitting->get_id(), 'rangeindex' => $rangeindex]; + $predictsamplesrecord->sampleids = json_encode($sampleids); + $predictsamplesrecord->timecreated = time(); + $predictsamplesrecord->timemodified = $predictsamplesrecord->timecreated; + $DB->insert_record('analytics_predict_samples', $predictsamplesrecord); } } @@ -898,4 +886,4 @@ class analysis { return (int)$bulkinsert; } -} \ No newline at end of file +} diff --git a/analytics/classes/course.php b/analytics/classes/course.php index 919f36bd690..d20154cd49e 100644 --- a/analytics/classes/course.php +++ b/analytics/classes/course.php @@ -132,12 +132,11 @@ class course implements \core_analytics\analysable { * Use self::instance() instead to get cached copies of the course. Instances obtained * through this constructor will not be cached. * - * Lazy load of course data, students and teachers. - * - * @param int|\stdClass $course Course id + * @param int|\stdClass $course Course id or mdl_course record + * @param ?\context $context * @return void */ - public function __construct($course) { + public function __construct($course, ?\context $context = null) { if (is_scalar($course)) { $this->course = new \stdClass(); @@ -145,6 +144,10 @@ class course implements \core_analytics\analysable { } else { $this->course = $course; } + + if (!is_null($context)) { + $this->coursecontext = $context; + } } /** @@ -153,9 +156,10 @@ class course implements \core_analytics\analysable { * Lazy load of course data, students and teachers. * * @param int|\stdClass $course Course object or course id + * @param ?\context $context * @return \core_analytics\course */ - public static function instance($course) { + public static function instance($course, ?\context $context = null) { $courseid = $course; if (!is_scalar($courseid)) { @@ -166,7 +170,7 @@ class course implements \core_analytics\analysable { return self::$cachedinstance; } - $cachedinstance = new \core_analytics\course($course); + $cachedinstance = new \core_analytics\course($course, $context); self::$cachedinstance = $cachedinstance; self::$cachedid = (int)$courseid; return self::$cachedinstance; diff --git a/analytics/classes/local/analyser/base.php b/analytics/classes/local/analyser/base.php index b1799349795..ce20aa78f99 100644 --- a/analytics/classes/local/analyser/base.php +++ b/analytics/classes/local/analyser/base.php @@ -110,10 +110,34 @@ abstract class base { * * \core_analytics\local\analyser\by_course and \core_analytics\local\analyser\sitewide are implementing * this method returning site courses (by_course) and the whole system (sitewide) as analysables. - * + * @throws \coding_exception * @return \core_analytics\analysable[] Array of analysable elements using the analysable id as array key. */ - abstract public function get_analysables(); + public function get_analysables() { + // This function should only be called from get_analysables_iterator and we keep it here until php 4.1 + // for backwards compatibility. + throw new \coding_exception('This method is deprecated in favour of get_analysables_iterator.'); + } + + /** + * Returns the list of analysable elements available on the site. + * + * A relatively complex SQL query should be set so that we take into account which analysable elements + * have already been processed and the order in which they have been processed. Helper methods are available + * to ease to implementation of get_analysables_iterator: get_iterator_sql and order_sql. + * + * @param ?string $action 'prediction', 'training' or null if no specific action needed. + * @return \Iterator + */ + public function get_analysables_iterator(?string $action = null) { + + debugging('Please overwrite get_analysables_iterator with your own implementation, we only keep this default + implementation for backwards compatibility purposes with get_analysables(). note that $action param will + be ignored so the analysable elements will be processed using get_analysables order, regardless of the + last time they were processed.'); + + return new \ArrayIterator($this->get_analysables()); + } /** * This function returns this analysable list of samples. @@ -376,4 +400,71 @@ abstract class base { public static function one_sample_per_analysable() { return false; } + + /** + * Get the sql of a default implementaion of the iterator. + * + * This method only works for analysers that return analysable elements which ids map to a context instance ids. + * + * @param string $tablename The name of the table + * @param int $contextlevel The context level of the analysable + * @param string|null $action + * @param string|null $tablealias The table alias + * @return array [0] => sql and [1] => params array + */ + protected function get_iterator_sql(string $tablename, int $contextlevel, ?string $action = null, ?string $tablealias = null) { + + if (!$tablealias) { + $tablealias = 'analysable'; + } + + $params = ['contextlevel' => $contextlevel, 'modelid' => $this->get_modelid()]; + $select = $tablealias . '.*, ' . \context_helper::get_preload_record_columns_sql('ctx'); + + // We add the action filter on ON instead of on WHERE because otherwise records are not returned if there are existing + // records for another action or model. + $usedanalysablesjoin = ' LEFT JOIN {analytics_used_analysables} aua ON ' . $tablealias . '.id = aua.analysableid AND ' . + '(aua.modelid = :modelid OR aua.modelid IS NULL)'; + + if ($action) { + $usedanalysablesjoin .= " AND aua.action = :action"; + $params = $params + ['action' => $action]; + } + + // Adding the 1 = 1 just to have the WHERE part so that all further conditions added by callers can be + // appended to $sql with and ' AND'. + $sql = 'SELECT ' . $select . ' + FROM {' . $tablename . '} ' . $tablealias . ' + ' . $usedanalysablesjoin . ' + JOIN {context} ctx ON (ctx.contextlevel = :contextlevel AND ctx.instanceid = ' . $tablealias . '.id) + WHERE 1 = 1'; + + return [$sql, $params]; + } + + /** + * Returns the order by clause. + * + * @param string|null $fieldname The field name + * @param string $order 'ASC' or 'DESC' + * @param string|null $tablealias The table alias of the field + * @return string + */ + protected function order_sql(?string $fieldname = null, string $order = 'ASC', ?string $tablealias = null) { + + if (!$tablealias) { + $tablealias = 'analysable'; + } + + if ($order != 'ASC' && $order != 'DESC') { + throw new \coding_exception('The order can only be ASC or DESC'); + } + + $ordersql = ' ORDER BY (CASE WHEN aua.timeanalysed IS NULL THEN 0 ELSE aua.timeanalysed END) ASC'; + if ($fieldname) { + $ordersql .= ', ' . $tablealias . '.' . $fieldname .' ' . $order; + } + + return $ordersql; + } } diff --git a/analytics/classes/local/analyser/by_course.php b/analytics/classes/local/analyser/by_course.php index bbda69a1abf..7794cbe132f 100644 --- a/analytics/classes/local/analyser/by_course.php +++ b/analytics/classes/local/analyser/by_course.php @@ -38,34 +38,43 @@ abstract class by_course extends base { /** * Return the list of courses to analyse. * - * @return \core_analytics\course[] + * @param ?string $action 'prediction', 'training' or null if no specific action needed. + * @return \Iterator */ - public function get_analysables() { + public function get_analysables_iterator(?string $action = null) { + global $DB; - // Default to all system courses. + list($sql, $params) = $this->get_iterator_sql('course', CONTEXT_COURSE, $action, 'c'); + + // This will be updated to filter by context as part of MDL-64739. if (!empty($this->options['filter'])) { $courses = array(); foreach ($this->options['filter'] as $courseid) { $courses[$courseid] = new \stdClass(); $courses[$courseid]->id = $courseid; } - } else { - // Iterate through all potentially valid courses. - $courses = get_courses('all', 'c.sortorder ASC', 'c.id'); - } - unset($courses[SITEID]); - $analysables = array(); - foreach ($courses as $course) { - // Skip the frontpage course. - $analysable = \core_analytics\course::instance($course->id); - $analysables[$analysable->get_id()] = $analysable; + list($coursesql, $courseparams) = $DB->get_in_or_equal($courses, SQL_PARAMS_NAMED); + $sql .= " AND c.id IN $coursesql"; + $params = $params + $courseparams; } - if (empty($analysables)) { - $this->log[] = get_string('nocourses', 'analytics'); + $ordersql = $this->order_sql('sortorder', 'ASC', 'c'); + + $recordset = $DB->get_recordset_sql($sql . $ordersql, $params); + + if (!$recordset->valid()) { + $this->add_log(get_string('nocourses', 'analytics')); + return []; } - return $analysables; + return new \core\dml\recordset_walk($recordset, function($record) { + + if ($record->id == SITEID) { + return false; + } + $context = \context_helper::preload_from_record($record); + return \core_analytics\course::instance($record, $context); + }); } -} +} \ No newline at end of file diff --git a/analytics/classes/local/analyser/sitewide.php b/analytics/classes/local/analyser/sitewide.php index 075b930e2fb..da7c475c414 100644 --- a/analytics/classes/local/analyser/sitewide.php +++ b/analytics/classes/local/analyser/sitewide.php @@ -36,12 +36,13 @@ defined('MOODLE_INTERNAL') || die(); abstract class sitewide extends base { /** - * Returns one single analysable element, the site. + * Return the list of courses to analyse. * - * @return \core_analytics\analysable[] + * @param ?string $action 'prediction', 'training' or null if no specific action needed. + * @return \Iterator */ - public function get_analysables() { - $analysable = new \core_analytics\site(); - return array(SYSCONTEXTID => $analysable); + public function get_analysables_iterator(?string $action = null) { + // We can safely ignore $action as we have 1 single analysable element in this analyser. + return new \ArrayIterator([new \core_analytics\site()]); } } diff --git a/analytics/classes/local/target/base.php b/analytics/classes/local/target/base.php index f41bf4d769c..686d455e7f8 100644 --- a/analytics/classes/local/target/base.php +++ b/analytics/classes/local/target/base.php @@ -104,6 +104,19 @@ abstract class base extends \core_analytics\calculable { return false; } + /** + * Update the last analysis time on analysable processed or always. + * + * If you overwrite this method to return false the last analysis time + * will only be recorded in DB when the element successfully analysed. You can + * safely return false for lightweight targets. + * + * @return bool + */ + public function always_update_analysis_time(): bool { + return true; + } + /** * Suggested actions for a user. * diff --git a/analytics/classes/local/time_splitting/base.php b/analytics/classes/local/time_splitting/base.php index 3b4c03c0ca2..3eef63c04cd 100644 --- a/analytics/classes/local/time_splitting/base.php +++ b/analytics/classes/local/time_splitting/base.php @@ -262,6 +262,13 @@ abstract class base { /** * Whether to cache or not the indicator calculations. + * + * Indicator calculations are stored to be reused across models. The calculations + * are indexed by the calculation start and end time, and these times depend on the + * time-splitting method. You should overwrite this method and return false if the time + * frames generated by your time-splitting method are unique and / or can hardly be + * reused by further models. + * * @return bool */ public function cache_indicator_calculations(): bool { diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php index 861fadd9523..3e5bbb8d3e9 100644 --- a/analytics/classes/manager.php +++ b/analytics/classes/manager.php @@ -565,15 +565,19 @@ class manager { $models = self::get_all_models(); foreach ($models as $model) { $analyser = $model->get_analyser(array('notimesplitting' => true)); - $analysables = $analyser->get_analysables(); - if (!$analysables) { + $analysables = $analyser->get_analysables_iterator(); + + $analysableids = []; + foreach ($analysables as $analysable) { + if (!$analysable) { + continue; + } + $analysableids[] = $analysable->get_id(); + } + if (empty($analysableids)) { continue; } - $analysableids = array_map(function($analysable) { - return $analysable->get_id(); - }, $analysables); - list($notinsql, $params) = $DB->get_in_or_equal($analysableids, SQL_PARAMS_NAMED, 'param', false); $params['modelid'] = $model->get_id(); diff --git a/analytics/classes/user.php b/analytics/classes/user.php index f562a090afd..8751e02ce13 100644 --- a/analytics/classes/user.php +++ b/analytics/classes/user.php @@ -70,12 +70,11 @@ class user implements \core_analytics\analysable { * Use self::instance() instead to get cached copies of the class. Instances obtained * through this constructor will not be cached. * - * Lazy load of the analysable data. - * * @param int|\stdClass $user User id + * @param ?\context $context * @return void */ - public function __construct($user) { + public function __construct($user, ?\context $context = null) { if (is_scalar($user)) { $this->user = new \stdClass(); @@ -83,6 +82,10 @@ class user implements \core_analytics\analysable { } else { $this->user = $user; } + + if (!is_null($context)) { + $this->usercontext = $context; + } } /** @@ -91,9 +94,10 @@ class user implements \core_analytics\analysable { * Lazy load of analysable data. * * @param int|\stdClass $user User object or user id + * @param ?\context $context * @return \core_analytics\user */ - public static function instance($user) { + public static function instance($user, ?\context $context = null) { $userid = $user; if (!is_scalar($userid)) { @@ -104,7 +108,7 @@ class user implements \core_analytics\analysable { return self::$cachedinstance; } - $cachedinstance = new \core_analytics\user($user); + $cachedinstance = new \core_analytics\user($user, $context); self::$cachedinstance = $cachedinstance; self::$cachedid = (int)$userid; return self::$cachedinstance; diff --git a/analytics/tests/fixtures/test_analysis.php b/analytics/tests/fixtures/test_analysis.php index 4b6e63cd976..6af04d1f2ea 100644 --- a/analytics/tests/fixtures/test_analysis.php +++ b/analytics/tests/fixtures/test_analysis.php @@ -40,8 +40,8 @@ class test_analysis extends \core_analytics\analysis { * @return array */ public function process_analysable(\core_analytics\analysable $analysable): array { - // A bit more than 1 second. - usleep(550000); + // Half a second. + usleep(500000); return parent::process_analysable($analysable); } } diff --git a/analytics/tests/model_test.php b/analytics/tests/model_test.php index ee37485d1a0..97a7e3b2ed3 100644 --- a/analytics/tests/model_test.php +++ b/analytics/tests/model_test.php @@ -307,7 +307,7 @@ class analytics_model_testcase extends advanced_testcase { $result = new \core_analytics\local\analysis\result_array(1, false, []); $analysis = new test_analysis($analyser, false, $result); - // Each analysable element takes 0.55 secs, so the max (and likely) number of analysable + // Each analysable element takes 0.5 secs minimum (test_analysis), so the max (and likely) number of analysable // elements that will be processed is 2. $analysis->run(); $params = array('modelid' => 1, 'action' => 'prediction'); @@ -329,13 +329,16 @@ class analytics_model_testcase extends advanced_testcase { $last = $courses[$analysed->analysableid]; } + // No time limit now to process the rest. + set_config('modeltimelimit', 1000, 'analytics'); + $analysis->run(); - $this->assertGreaterThanOrEqual(5, $DB->count_records('analytics_used_analysables', $params)); + $this->assertEquals(5, $DB->count_records('analytics_used_analysables', $params)); // New analysable elements are immediately pulled. $this->getDataGenerator()->create_course(); $analysis->run(); - $this->assertGreaterThanOrEqual(6, $DB->count_records('analytics_used_analysables', $params)); + $this->assertEquals(6, $DB->count_records('analytics_used_analysables', $params)); // Training and prediction data do not get mixed. $result = new \core_analytics\local\analysis\result_array(1, false, []); diff --git a/analytics/upgrade.txt b/analytics/upgrade.txt index 9f4f10b2a00..12aa5d9f23c 100644 --- a/analytics/upgrade.txt +++ b/analytics/upgrade.txt @@ -13,24 +13,36 @@ information provided here is intended especially for developers. has been replaced with automatic update of models provided by the core moodle component. There is no need to call this method explicitly any more. Instead, adding new models can be achieved by updating the lib/db/analytics.php file and bumping the core version. -* \core_analytics\local\time_splitting\base::append_rangeindex and - \core_analytics\local\time_splitting\base::infer_sample_info are now marked as final and can not - be overwritten. * \core_analytics\model::execute_prediction_callbacks now returns an array with both sample's contexts and the prediction records. -* \core_analytics\local\analyser\base::get_most_recent_prediction_range has been moved to - \core_analytics\local\time_splitting\base::get_most_recent_prediction_range and it is not overwritable - by time splitting methods. -* Time splitting methods can now re-implement include_range_info_in_training_data() and - get_training_ranges() methods. They can be used to create time splitting methods with a pre-defined - number of ranges. -* Analysers can overwrite a new one_sample_per_analysable method if the analysables they use only have - one sample. The insights generated by models will then include the suggested actions in - the notification. -* The visibility of target's ignored_predicted_classes method must now be public. -* The visibility of analyser's get_all_samples method must now be public. -* Target's prediction_actions() has now a 3rd parameter $isinsightuser. This parameter is true - when we are listing actions for the user that will receives the insight. +* Time splitting methods: + * \core_analytics\local\time_splitting\base::append_rangeindex and + \core_analytics\local\time_splitting\base::infer_sample_info are now marked as final and can not + be overwritten. + * Can now overwrite include_range_info_in_training_data() and + get_training_ranges() methods. They can be used to create time splitting methods with a pre-defined + number of ranges. + * Can now overwrite cache_indicator_calculations(). You should return false if the time frames generated + by your time-splitting method are unique and / or can hardly be reused by further models. + * \core_analytics\local\analyser\base::get_most_recent_prediction_range has been moved to + \core_analytics\local\time_splitting\base::get_most_recent_prediction_range and it is not overwritable + by time splitting methods. +* Targets: + * The visibility of the following methods must now be public: ignored_predicted_classes() + and get_insights_users() + * Prediction_actions() has now a 3rd parameter $isinsightuser. This parameter is true + when we are listing actions for the user that will receives the insight. + * Can now implement a always_update_analysis_time() method so analysable elements' timeanalysed is + only updated when analysable elements have been successfully evaluated. It is useful for lightweight targets. + * Can not implement two new methods to tune the insights generated by the model: get_insight_subject() + and get_insight_context_url(). +* Analysers: + * The visibility of get_all_samples() method must now be public. + * get_analysables() method has been deprecated in favour of a new get_analysables_interator() + for performance reasons. + * Can overwrite a new one_sample_per_analysable() method if the analysables they use only have + one sample. The insights generated by models will then include the suggested actions in + the notification. === 3.5 === diff --git a/lib/classes/analytics/analyser/users.php b/lib/classes/analytics/analyser/users.php index 6b5c4e4c4e1..422951d59aa 100644 --- a/lib/classes/analytics/analyser/users.php +++ b/lib/classes/analytics/analyser/users.php @@ -38,32 +38,36 @@ class users extends \core_analytics\local\analyser\base { /** * The site users are the analysable elements returned by this analyser. * - * @return \core_analytics\analysable[] + * @param ?string $action 'prediction', 'training' or null if no specific action needed. + * @return \Iterator */ - public function get_analysables() { + public function get_analysables_iterator(?string $action = null) { global $DB, $CFG; $siteadmins = explode(',', $CFG->siteadmins); - $params = ['deleted' => 0, 'confirmed' => 1, 'suspended' => 0]; - $users = $DB->get_records('user', $params, 'timecreated', 'id'); - $analysables = array(); - foreach ($users as $user) { + list($sql, $params) = $this->get_iterator_sql('user', CONTEXT_USER, $action, 'u'); - if (in_array($user->id, $siteadmins) || isguestuser($user->id)) { + $sql .= " AND u.deleted = :deleted AND u.confirmed = :confirmed AND u.suspended = :suspended"; + $params = $params + ['deleted' => 0, 'confirmed' => 1, 'suspended' => 0]; + + $ordersql = $this->order_sql('timecreated', 'ASC', 'u'); + + $recordset = $DB->get_recordset_sql($sql, $params); + if (!$recordset->valid()) { + $this->add_log(get_string('nousersfound')); + } + + return new \core\dml\recordset_walk($recordset, function($record) use ($siteadmins) { + + if (in_array($record->id, $siteadmins) || isguestuser($record->id)) { // Skip admins and the guest user. - continue; + return false; } - $analysable = \core_analytics\user::instance($user->id); - $analysables[$analysable->get_id()] = $analysable; - } - - if (empty($analysables)) { - $this->log[] = get_string('nousersfound'); - } - - return $analysables; + $context = \context_helper::preload_from_record($record); + return \core_analytics\user::instance($record, $context); + }); } /** diff --git a/lib/tests/analysers_test.php b/lib/tests/analysers_test.php index 4c28a0685e2..f52ab0f9636 100644 --- a/lib/tests/analysers_test.php +++ b/lib/tests/analysers_test.php @@ -25,7 +25,9 @@ defined('MOODLE_INTERNAL') || die(); +require_once(__DIR__ . '/../../analytics/tests/fixtures/test_target_course_level_shortname.php'); require_once(__DIR__ . '/../../analytics/tests/fixtures/test_target_shortname.php'); +require_once(__DIR__ . '/fixtures/deprecated_analyser.php'); require_once(__DIR__ . '/../../lib/enrollib.php'); /** @@ -119,7 +121,7 @@ class core_analytics_analysers_testcase extends advanced_testcase { } /** - * test_site_courses_analyser + * test_student_enrolments_analyser * * @return void */ @@ -175,4 +177,96 @@ class core_analytics_analysers_testcase extends advanced_testcase { $this->assertEquals($prevsampledata['course']->shortname, $samplesdata[$sampleid]['course']->shortname); $this->assertEquals($prevsampledata['user']->firstname, $samplesdata[$sampleid]['user']->firstname); } + + /** + * test_deprecated_analyser + * + * @return void + */ + public function test_deprecated_analyser() { + + $target = new test_target_shortname(); + $analyser = new deprecated_analyser(1, $target, [], [], []); + + $analysables = $analyser->get_analysables_iterator(); + $this->assertDebuggingCalled(); + } + + /** + * test_get_analysables_iterator description + * + * @return null + */ + public function test_get_analysables_iterator() { + global $DB; + + $this->resetAfterTest(true); + + $courses = array(); + for ($i = 0; $i < 2; $i++) { + $course = $this->getDataGenerator()->create_course(); + $analysable = new \core_analytics\course($course); + $courses[$analysable->get_id()] = $course; + } + + // Check that the analysis performs as expected. + $modelid = 1; + $includetarget = false; + + $target = new test_target_course_level_shortname(); + $analyser = new \core\analytics\analyser\courses($modelid, $target, [], [], []); + + $result = new \core_analytics\local\analysis\result_array($modelid, $includetarget, []); + $analysis = new \core_analytics\analysis($analyser, $includetarget, $result); + $analysis->run(); + $params = array('modelid' => $modelid, 'action' => 'prediction'); + $this->assertEquals(2, $DB->count_records('analytics_used_analysables', $params)); + + // Check that the previous records do not conflict with the includetarget == false ones. + $includetarget = true; + + $target = new test_target_course_level_shortname(); + $analyser = new \core\analytics\analyser\courses($modelid, $target, [], [], []); + + $result = new \core_analytics\local\analysis\result_array($modelid, $includetarget, []); + $analysis = new \core_analytics\analysis($analyser, $includetarget, $result); + $analysis->run(); + $params = array('modelid' => $modelid, 'action' => 'prediction'); + $this->assertEquals(2, $DB->count_records('analytics_used_analysables', $params)); + $params = array('modelid' => $modelid, 'action' => 'training'); + $this->assertEquals(2, $DB->count_records('analytics_used_analysables', $params)); + $params = array('modelid' => $modelid); + $this->assertEquals(4, $DB->count_records('analytics_used_analysables', $params)); + + // Check that other models' records do not conflict with previous records. + $prevmodelid = 1; + $modelid = 2; + $includetarget = false; + + $target = new test_target_course_level_shortname(); + $analyser = new \core\analytics\analyser\courses($modelid, $target, [], [], []); + + $result = new \core_analytics\local\analysis\result_array($modelid, $includetarget, []); + $analysis = new \core_analytics\analysis($analyser, $includetarget, $result); + $analysis->run(); + $params = array('modelid' => $prevmodelid); + $this->assertEquals(4, $DB->count_records('analytics_used_analysables', $params)); + $params = array('modelid' => $modelid, 'action' => 'prediction'); + $this->assertEquals(2, $DB->count_records('analytics_used_analysables', $params)); + $this->assertEquals(6, $DB->count_records('analytics_used_analysables')); + + $includetarget = true; + + $target = new test_target_course_level_shortname(); + $analyser = new \core\analytics\analyser\courses($modelid, $target, [], [], []); + + $result = new \core_analytics\local\analysis\result_array($modelid, $includetarget, []); + $analysis = new \core_analytics\analysis($analyser, $includetarget, $result); + $analysis->run(); + $params = array('modelid' => $prevmodelid); + $this->assertEquals(4, $DB->count_records('analytics_used_analysables', $params)); + $params = array('modelid' => $modelid, 'action' => 'training'); + $this->assertEquals(2, $DB->count_records('analytics_used_analysables', $params)); + $this->assertEquals(8, $DB->count_records('analytics_used_analysables')); + } } diff --git a/lib/tests/fixtures/deprecated_analyser.php b/lib/tests/fixtures/deprecated_analyser.php new file mode 100644 index 00000000000..33e7594e130 --- /dev/null +++ b/lib/tests/fixtures/deprecated_analyser.php @@ -0,0 +1,158 @@ +. + +/** + * Deprecated analyser for testing purposes. + * + * @package core_analytics + * @copyright 2019 David Monllao {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ + +defined('MOODLE_INTERNAL') || die(); + +/** + * Deprecated analyser for testing purposes. + * + * @package core_analytics + * @copyright 2019 David Monllao {@link http://www.davidmonllao.com} + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + */ +class deprecated_analyser extends \core_analytics\local\analyser\base { + + /** + * Implementation of a deprecated method. + * + * It should be called by get_analysables_iterator, which triggers a debugging message. + * @return \core_analytics\analysable[] + */ + public function get_analysables() { + $analysable = new \core_analytics\site(); + return [SYSCONTEXTID => $analysable]; + } + + /** + * Samples origin is course table. + * + * @return string + */ + public function get_samples_origin() { + return 'user'; + } + + /** + * Returns the sample analysable + * + * @param int $sampleid + * @return \core_analytics\analysable + */ + public function get_sample_analysable($sampleid) { + return new \core_analytics\site(); + } + + /** + * Data this analyer samples provide. + * + * @return string[] + */ + protected function provided_sample_data() { + return array('user'); + } + + /** + * Returns the sample context. + * + * @param int $sampleid + * @return \context + */ + public function sample_access_context($sampleid) { + return \context_system::instance(); + } + + /** + * Returns all site courses. + * + * @param \core_analytics\analysable $site + * @return array + */ + public function get_all_samples(\core_analytics\analysable $site) { + global $DB; + + $users = $DB->get_records('user'); + $userids = array_keys($users); + $sampleids = array_combine($userids, $userids); + + $users = array_map(function($user) { + return array('user' => $user); + }, $users); + + return array($sampleids, $users); + } + + /** + * Return all complete samples data from sample ids. + * + * @param int[] $sampleids + * @return array + */ + public function get_samples($sampleids) { + global $DB; + + list($userssql, $params) = $DB->get_in_or_equal($sampleids, SQL_PARAMS_NAMED); + $users = $DB->get_records_select('user', "id {$userssql}", $params); + $userids = array_keys($users); + $sampleids = array_combine($userids, $userids); + + $users = array_map(function($user) { + return array('user' => $user); + }, $users); + + return array($sampleids, $users); + } + + /** + * Returns the description of a sample. + * + * @param int $sampleid + * @param int $contextid + * @param array $sampledata + * @return array array(string, \renderable) + */ + public function sample_description($sampleid, $contextid, $sampledata) { + $description = fullname($samplesdata['user']); + $userimage = new \pix_icon('i/user', get_string('user')); + return array($description, $userimage); + } + + /** + * We need to delete associated data if a user requests his data to be deleted. + * + * @return bool + */ + public function processes_user_data() { + return true; + } + + /** + * Join the samples origin table with the user id table. + * + * @param string $sampletablealias + * @return string + */ + public function join_sample_user($sampletablealias) { + return "JOIN {user} u ON u.id = {$sampletablealias}.sampleid"; + } +} \ No newline at end of file diff --git a/user/classes/analytics/target/upcoming_activities_due.php b/user/classes/analytics/target/upcoming_activities_due.php index 459f1386d17..911d30196bf 100644 --- a/user/classes/analytics/target/upcoming_activities_due.php +++ b/user/classes/analytics/target/upcoming_activities_due.php @@ -46,6 +46,14 @@ class upcoming_activities_due extends \core_analytics\local\target\binary { return true; } + /** + * Only update last analysis time when analysables are processed. + * @return bool + */ + public function always_update_analysis_time(): bool { + return false; + } + /** * Returns the name. * @@ -100,18 +108,15 @@ class upcoming_activities_due extends \core_analytics\local\target\binary { } /** - * Discard users without courses. + * All users are ok. * * @param \core_analytics\analysable $analysable * @param mixed $fortraining * @return true|string */ public function is_valid_analysable(\core_analytics\analysable $analysable, $fortraining = true) { - // We can skip this checking if we want to save db reads. - $courses = enrol_get_all_users_courses($analysable->get_id(), true, 'id'); - if (!$courses) { - return get_string('nocourses'); - } + // The calendar API used by \core_course\analytics\indicator\activities_due is already checking + // if the user has any courses. return true; }