MDL-64783 analytics: Performance improvements
authorDavid MonllaĆ³ <davidm@moodle.com>
Wed, 3 Apr 2019 17:46:03 +0000 (19:46 +0200)
committerEloy Lafuente (stronk7) <stronk7@moodle.org>
Mon, 8 Apr 2019 22:29:57 +0000 (00:29 +0200)
- 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.

18 files changed:
admin/tool/analytics/classes/output/invalid_analysables.php
analytics/classes/analysable.php
analytics/classes/analysis.php
analytics/classes/course.php
analytics/classes/local/analyser/base.php
analytics/classes/local/analyser/by_course.php
analytics/classes/local/analyser/sitewide.php
analytics/classes/local/target/base.php
analytics/classes/local/time_splitting/base.php
analytics/classes/manager.php
analytics/classes/user.php
analytics/tests/fixtures/test_analysis.php
analytics/tests/model_test.php
analytics/upgrade.txt
lib/classes/analytics/analyser/users.php
lib/tests/analysers_test.php
lib/tests/fixtures/deprecated_analyser.php [new file with mode: 0644]
user/classes/analytics/target/upcoming_activities_due.php

index 0bb3902..dbdaa00 100644 (file)
@@ -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.
index e9dcaae..97faf59 100644 (file)
@@ -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
index e37491e..764b4ee 100644 (file)
@@ -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
+}
index 919f36b..d20154c 100644 (file)
@@ -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;
index b179934..ce20aa7 100644 (file)
@@ -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;
+    }
 }
index bbda69a..7794cbe 100644 (file)
@@ -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
index 075b930..da7c475 100644 (file)
@@ -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()]);
     }
 }
index f41bf4d..686d455 100644 (file)
@@ -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.
      *
index 3b4c03c..3eef63c 100644 (file)
@@ -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 {
index 861fadd..3e5bbb8 100644 (file)
@@ -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();
 
index f562a09..8751e02 100644 (file)
@@ -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;
index 4b6e63c..6af04d1 100644 (file)
@@ -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);
     }
 }
index ee37485..97a7e3b 100644 (file)
@@ -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, []);
index 9f4f10b..12aa5d9 100644 (file)
@@ -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 ===
 
index 6b5c4e4..422951d 100644 (file)
@@ -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)) {
-                // Skip admins and the guest user.
-                continue;
-            }
-            $analysable = \core_analytics\user::instance($user->id);
-            $analysables[$analysable->get_id()] = $analysable;
-        }
+        $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');
 
-        if (empty($analysables)) {
-            $this->log[] = get_string('nousersfound');
+        $recordset = $DB->get_recordset_sql($sql, $params);
+        if (!$recordset->valid()) {
+            $this->add_log(get_string('nousersfound'));
         }
 
-        return $analysables;
+        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.
+                return false;
+            }
+            $context = \context_helper::preload_from_record($record);
+            return \core_analytics\user::instance($record, $context);
+        });
     }
 
     /**
index 4c28a06..f52ab0f 100644 (file)
@@ -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 (file)
index 0000000..33e7594
--- /dev/null
@@ -0,0 +1,158 @@
+<?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/>.
+
+/**
+ * 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
index 459f138..911d301 100644 (file)
@@ -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;
     }