MDL-57791 analytics: Changes after review

- Split model::predict in parts
- JS promises updated according to eslint-plugin-promise
- New API methods replacing direct DB queries
- Reduce insights nav link display cost
- Increase time limit as well as memory for big processes
- Move prediction action event to core
- Dataset write locking and others
- Refine last time range end time
- Removed dodgy splitting method id to int
- Replace admin_setting_predictor output_html overwrite for write_setting overwrite
- New APIs for access control
- Discard invalid samples also during prediction

admin/settings/analytics.php

@@ -45,7 +45,7 @@
         $logmanager = get_log_manager();
         $readers = $logmanager->get_readers('core\log\sql_reader');
         $options = array();
-        $defaultreader = false;
+        $defaultreader = null;
         foreach ($readers as $plugin => $reader) {
             if (!$reader->is_logging()) {
                 continue;

admin/tool/models/amd/src/log_info.js

@@ -7,7 +7,7 @@
  /**
   * @module tool_models/log_info
   */
-define(['jquery', 'core/str', 'core/modal_factory'], function($, str, ModalFactory) {
+define(['jquery', 'core/str', 'core/modal_factory', 'core/notification'], function($, str, ModalFactory, Notification) {
 
     return {
 
@@ -20,19 +20,21 @@ define(['jquery', 'core/str', 'core/modal_factory'], function($, str, ModalFacto
         loadInfo : function(id, info) {
 
             var link = $('[data-model-log-id="' + id + '"]');
-            str.get_string('loginfo', 'tool_models').done(function(langString) {
+            str.get_string('loginfo', 'tool_models').then(function(langString) {
 
                 var bodyInfo = $("<ul>");
                 for (var i in info) {
                     bodyInfo.append("<li>" + info[i] + "</li>");
                 }
                 bodyInfo.append("</ul>");
-                ModalFactory.create({
+
+                return ModalFactory.create({
                     title: langString,
                     body: bodyInfo.html(),
                     large: true,
                 }, link);
-            });
+
+            }).catch(Notification.exception);
         }
     };
 });

admin/tool/models/classes/analytics/target/course_dropout.php

@@ -46,10 +46,10 @@ public static function get_name() {
         return get_string('target:coursedropout', 'tool_models');
     }
 
-    public function prediction_actions(\core_analytics\prediction $prediction) {
+    public function prediction_actions(\core_analytics\prediction $prediction, $includedetailsaction = false) {
         global $USER;
 
-        $actions = parent::prediction_actions($prediction);
+        $actions = parent::prediction_actions($prediction, $includedetailsaction);
 
         $sampledata = $prediction->get_sample_data();
         $studentid = $sampledata['user']->id;
@@ -140,13 +140,14 @@ public function is_valid_analysable(\core_analytics\analysable $course, $fortrai
     }
 
     /**
-     * is_valid_sample
+     * All student enrolments are valid.
      *
      * @param int $sampleid
      * @param \core_analytics\analysable $course
+     * @param bool $fortraining
      * @return bool
      */
-    public function is_valid_sample($sampleid, \core_analytics\analysable $course) {
+    public function is_valid_sample($sampleid, \core_analytics\analysable $course, $fortraining = true) {
         return true;
     }
 

admin/tool/models/classes/analytics/target/no_teaching.php

@@ -48,10 +48,10 @@ public static function get_name() {
         return get_string('target:noteachingactivity', 'tool_models');
     }
 
-    public function prediction_actions(\core_analytics\prediction $prediction) {
+    public function prediction_actions(\core_analytics\prediction $prediction, $includedetailsaction = false) {
         global $USER;
 
-        // No need to call the parent as the only default action is view details and this target only have 1 feature.
+        // No need to call the parent as the parent's action is view details and this target only have 1 feature.
         $actions = array();
 
         $sampledata = $prediction->get_sample_data();
@@ -110,9 +110,10 @@ public function is_valid_analysable(\core_analytics\analysable $analysable, $for
      *
      * @param mixed $sampleid
      * @param \core_analytics\analysable $analysable
+     * @param bool $fortraining
      * @return void
      */
-    public function is_valid_sample($sampleid, \core_analytics\analysable $analysable) {
+    public function is_valid_sample($sampleid, \core_analytics\analysable $analysable, $fortraining = true) {
 
         $course = $this->retrieve('course', $sampleid);
 

admin/tool/models/classes/output/model_logs.php

@@ -180,8 +180,6 @@ public function col_usermodified($log) {
      * @param bool $useinitialsbar do you want to use the initials bar.
      */
     public function query_db($pagesize, $useinitialsbar = true) {
-        global $DB;
-
         $total = count($this->model->get_logs());
         $this->pagesize($pagesize, $total);
         $this->rawdata = $this->model->get_logs($this->get_page_start(), $this->get_page_size());

admin/tool/models/classes/task/predict_models.php

@@ -39,7 +39,7 @@ public function get_name() {
     }
 
     public function execute() {
-        global $DB, $OUTPUT, $PAGE;
+        global $OUTPUT, $PAGE;
 
         $models = \core_analytics\manager::get_all_models(true, true);
         if (!$models) {

admin/tool/models/classes/task/train_models.php

@@ -39,7 +39,7 @@ public function get_name() {
     }
 
     public function execute() {
-        global $DB, $OUTPUT, $PAGE;
+        global $OUTPUT, $PAGE;
 
         $models = \core_analytics\manager::get_all_models(true);
         if (!$models) {

admin/tool/models/cli/enable_model.php

@@ -63,8 +63,7 @@
 // We need admin permissions.
 \core\session\manager::set_user(get_admin());
 
-$modelobj = $DB->get_record('analytics_models', array('id' => $options['modelid']), '*', MUST_EXIST);
-$model = new \core_analytics\model($modelobj);
+$model = new \core_analytics\model($options['modelid']);
 
 // Evaluate its suitability to predict accurately.
 $model->enable($options['timesplitting']);

admin/tool/models/cli/evaluate_model.php

@@ -75,8 +75,7 @@
 // We need admin permissions.
 \core\session\manager::set_user(get_admin());
 
-$modelobj = $DB->get_record('analytics_models', array('id' => $options['modelid']), '*', MUST_EXIST);
-$model = new \core_analytics\model($modelobj);
+$model = new \core_analytics\model($options['modelid']);
 
 mtrace(get_string('analysingsitedata', 'tool_models'));
 

admin/tool/models/model.php

@@ -30,9 +30,9 @@
 $context = context_system::instance();
 
 require_login();
-require_capability('moodle/analytics:managemodels', $context);
 
 $model = new \core_analytics\model($id);
+\core_analytics\manager::check_can_manage_models();
 
 $params = array('id' => $id, 'action' => $action);
 $url = new \moodle_url('/admin/tool/models/model.php', $params);

analytics/classes/admin_setting_predictor.php

@@ -30,28 +30,26 @@
 class admin_setting_predictor extends \admin_setting_configselect {
 
     /**
-     * Builds HTML to display the control.
+     * Save a setting
      *
-     * The main purpose of this is to display a warning if the selected predictions processor is not ready.
-
-     * @param string $data Unused
-     * @param string $query
-     * @return string HTML
+     * @param string $data
+     * @return string empty of error string
      */
-    public function output_html($data, $query='') {
-        global $CFG, $OUTPUT;
-
-        $html = '';
+    public function write_setting($data) {
+        if (!$this->load_choices() or empty($this->choices)) {
+            return '';
+        }
+        if (!array_key_exists($data, $this->choices)) {
+            return ''; // ignore it
+        }
 
         // Calling it here without checking if it is ready because we check it below and show it as a controlled case.
         $selectedprocessor = \core_analytics\manager::get_predictions_processor($data, false);
-
         $isready = $selectedprocessor->is_ready();
         if ($isready !== true) {
-            $html .= $OUTPUT->notification(get_string('errorprocessornotready', 'analytics', $isready));
+            return get_string('errorprocessornotready', 'analytics', $isready);
         }
 
-        $html .= parent::output_html($data, $query);
-        return $html;
+        return ($this->config_write($this->name, $data) ? '' : get_string('errorsetting', 'admin'));
     }
 }

analytics/classes/calculable.php

@@ -130,7 +130,8 @@ protected function retrieve($elementname, $sampleid) {
     /**
      * Returns the number of weeks a time range contains.
      *
-     * Useful for calculations that depend on the time range duration.
+     * Useful for calculations that depend on the time range duration. Note that it returns
+     * a float, rounding the float may lead to inaccurate results.
      *
      * @param int $starttime
      * @param int $endtime
@@ -141,9 +142,14 @@ protected function get_time_range_weeks_number($starttime, $endtime) {
             throw new \coding_exception('End time timestamp should be greater than start time.');
         }
 
-        $diff = $endtime - $starttime;
+        $starttimedt = new \DateTime();
+        $starttimedt->setTimestamp($starttime);
+        $starttimedt->setTimezone(\DateTimeZone::UTC);
+        $endtimedt = new \DateTime();
+        $endtimedt->setTimestamp($endtime);
+        $endtimedt->setTimezone(\DateTimeZone::UTC);
 
-        // No need to be strict about DST here.
+        $diff = $endtimedt->getTimestamp() - $starttimedt->getTimestamp();
         return $diff / WEEKSECS;
     }
 

analytics/classes/course.php

@@ -443,9 +443,6 @@ protected function completed_by(\cm_info $activity, $starttime, $endtime) {
             return false;
         }
 
-        // TODO Use course_modules_completion's timemodified + COMPLETION_COMPLETE* to discard
-        // activities that have already been completed.
-
         // We skip activities that were not yet visible or their 'until' was not in this $starttime - $endtime range.
         if ($activity->availability) {
             $info = new \core_availability\info_module($activity);
@@ -485,7 +482,6 @@ protected function completed_by(\cm_info $activity, $starttime, $endtime) {
             }
         }
 
-        // TODO Think about activities in sectionnum 0.
         if ($activity->sectionnum == 0) {
             return false;
         }
@@ -533,8 +529,6 @@ protected function availability_completed_by(\core_availability\info $info, $sta
         $dateconditions = $info->get_availability_tree()->get_all_children('\availability_date\condition');
         foreach ($dateconditions as $condition) {
             // Availability API does not allow us to check from / to dates nicely, we need to be naughty.
-            // TODO Would be nice to expand \availability_date\condition API for this calling a save that
-            // does not save is weird.
             $conditiondata = $condition->save();
 
             if ($conditiondata->d === \availability_date\condition::DIRECTION_FROM &&

analytics/classes/dataset_manager.php

@@ -86,18 +86,21 @@ public function __construct($modelid, $analysableid, $timesplittingid, $evaluati
     /**
      * Mark the analysable as being analysed.
      *
-     * @return void
+     * @return bool Could we get the lock or not.
      */
     public function init_process() {
         $lockkey = 'modelid:' . $this->modelid . '-analysableid:' . $this->analysableid .
-            '-timesplitting:' . self::convert_to_int($this->timesplittingid) . '-includetarget:' . (int)$this->includetarget;
+            '-timesplitting:' . self::clean_time_splitting_id($this->timesplittingid) . '-includetarget:' . (int)$this->includetarget;
 
         // Large timeout as processes may be quite long.
         $lockfactory = \core\lock\lock_config::get_lock_factory('core_analytics');
-        $this->lock = $lockfactory->get_lock($lockkey, WEEKSECS);
 
-        // We release the lock if there is an error during the process.
-        \core_shutdown_manager::register_function(array($this, 'release_lock'), array($this->lock));
+        // If it is not ready in 10 secs skip this model + analysable + timesplittingmethod combination
+        // it will attempt it again during next cron run.
+        if (!$this->lock = $lockfactory->get_lock($lockkey, 10)) {
+            return false;
+        }
+        return true;
     }
 
     /**
@@ -115,7 +118,7 @@ public function store($data) {
             'filearea' => self::get_filearea($this->includetarget),
             'itemid' => $this->modelid,
             'contextid' => \context_system::instance()->id,
-            'filepath' => '/analysable/' . $this->analysableid . '/' . self::convert_to_int($this->timesplittingid) . '/',
+            'filepath' => '/analysable/' . $this->analysableid . '/' . self::clean_time_splitting_id($this->timesplittingid) . '/',
             'filename' => self::get_filename($this->evaluation)
         ];
 
@@ -127,6 +130,10 @@ public function store($data) {
         // Write all this stuff to a tmp file.
         $filepath = make_request_directory() . DIRECTORY_SEPARATOR . $filerecord['filename'];
         $fh = fopen($filepath, 'w+');
+        if (!$fh) {
+            $this->close_process();
+            throw new \moodle_exception('errorcannotwritedataset', 'analytics', '', $tmpfilepath);
+        }
         foreach ($data as $line) {
             fputcsv($fh, $line);
         }
@@ -144,10 +151,6 @@ public function close_process() {
         $this->lock->release();
     }
 
-    public function release_lock(\core\lock\lock $lock) {
-        $lock->release();
-    }
-
     /**
      * Returns the previous evaluation file.
      *
@@ -162,7 +165,7 @@ public static function get_previous_evaluation_file($modelid, $timesplittingid)
         $fs = get_file_storage();
         // Evaluation data is always labelled.
         return $fs->get_file(\context_system::instance()->id, 'analytics', self::LABELLED_FILEAREA, $modelid,
-            '/timesplitting/' . self::convert_to_int($timesplittingid) . '/', self::EVALUATION_FILENAME);
+            '/timesplitting/' . self::clean_time_splitting_id($timesplittingid) . '/', self::EVALUATION_FILENAME);
     }
 
     public static function delete_previous_evaluation_file($modelid, $timesplittingid) {
@@ -183,7 +186,7 @@ public static function get_evaluation_analysable_file($modelid, $analysableid, $
         // Always evaluation.csv and labelled as it is an evaluation file.
         $filearea = self::get_filearea(true);
         $filename = self::get_filename(true);
-        $filepath = '/analysable/' . $analysableid . '/' . self::convert_to_int($timesplittingid) . '/';
+        $filepath = '/analysable/' . $analysableid . '/' . self::clean_time_splitting_id($timesplittingid) . '/';
         return $fs->get_file(\context_system::instance()->id, 'analytics', $filearea, $modelid, $filepath, $filename);
     }
 
@@ -235,6 +238,9 @@ public static function merge_datasets(array $files, $modelid, $timesplittingid,
 
         // Start writing to the merge file.
         $wh = fopen($tmpfilepath, 'w');
+        if (!$wh) {
+            throw new \moodle_exception('errorcannotwritedataset', 'analytics', '', $tmpfilepath);
+        }
 
         fputcsv($wh, $varnames);
         fputcsv($wh, $values);
@@ -262,7 +268,7 @@ public static function merge_datasets(array $files, $modelid, $timesplittingid,
             'filearea' => self::get_filearea($includetarget),
             'itemid' => $modelid,
             'contextid' => \context_system::instance()->id,
-            'filepath' => '/timesplitting/' . self::convert_to_int($timesplittingid) . '/',
+            'filepath' => '/timesplitting/' . self::clean_time_splitting_id($timesplittingid) . '/',
             'filename' => self::get_filename($evaluation)
         ];
 
@@ -315,17 +321,14 @@ public static function clear_model_files($modelid) {
     }
 
     /**
-     * I know it is not very orthodox...
+     * Remove all possibly problematic chars from the time splitting method id (id = its full class name).
      *
-     * @param string $string
-     * @return int
+     * @param string $timesplittingid
+     * @return string
      */
-    protected static function convert_to_int($string) {
-        $sum = 0;
-        for ($i = 0; $i < strlen($string); $i++) {
-            $sum += ord($string[$i]);
-        }
-        return $sum;
+    protected static function clean_time_splitting_id($timesplittingid) {
+        $timesplittingid = str_replace('\\', '-', $timesplittingid);
+        return clean_param($timesplittingid, PARAM_ALPHANUMEXT);
     }
 
     protected static function get_filename($evaluation) {