MDL-65633 tool_analytics: Notification for invalid analysis intervals
authorDavid Monllaó <davidm@moodle.com>
Sun, 26 May 2019 15:15:22 +0000 (17:15 +0200)
committerDavid Monllaó <davidm@moodle.com>
Thu, 18 Jul 2019 16:38:13 +0000 (18:38 +0200)
admin/tool/analytics/classes/output/form/edit_model.php
admin/tool/analytics/classes/output/models_list.php
admin/tool/analytics/lang/en/tool_analytics.php
admin/tool/analytics/model.php
analytics/classes/local/target/base.php
analytics/classes/model.php
analytics/tests/fixtures/test_static_target_shortname.php
analytics/tests/fixtures/test_target_site_users.php
analytics/upgrade.txt
lang/en/analytics.php

index b529d50..c897b5c 100644 (file)
@@ -65,6 +65,15 @@ class edit_model extends \moodleform {
             $mform->addRule('target', get_string('required'), 'required', null, 'client');
         }
 
+        if (!empty($this->_customdata['targetname']) && !empty($this->_customdata['targetclass'])) {
+            $mform->addElement('static', 'targetname', get_string('target', 'tool_analytics'), $this->_customdata['targetname']);
+            $mform->addElement('hidden', 'target',
+                \tool_analytics\output\helper::class_to_option($this->_customdata['targetclass']));
+            // We won't update the model's target so no worries about its format (we can't use PARAM_ALPHANUMEXT
+            // because of class_to_option).
+            $mform->setType('target', PARAM_TEXT);
+        }
+
         // Indicators.
         if (!$this->_customdata['staticmodel']) {
             $indicators = array();
@@ -81,6 +90,13 @@ class edit_model extends \moodleform {
         }
 
         // Time-splitting methods.
+        if (!empty($this->_customdata['invalidcurrenttimesplitting'])) {
+            $mform->addElement('html', $OUTPUT->notification(
+                get_string('invalidcurrenttimesplitting', 'tool_analytics'),
+                \core\output\notification::NOTIFY_WARNING)
+            );
+        }
+
         $timesplittings = array('' => '');
         foreach ($this->_customdata['timesplittings'] as $classname => $timesplitting) {
             $optionname = \tool_analytics\output\helper::class_to_option($classname);
@@ -131,10 +147,17 @@ class edit_model extends \moodleform {
         $errors = parent::validation($data, $files);
 
         if (!empty($data['timesplitting'])) {
-            $realtimesplitting = \tool_analytics\output\helper::option_to_class($data['timesplitting']);
-            if (\core_analytics\manager::is_valid($realtimesplitting, '\core_analytics\local\time_splitting\base') === false) {
+            $timesplittingclass = \tool_analytics\output\helper::option_to_class($data['timesplitting']);
+            if (\core_analytics\manager::is_valid($timesplittingclass, '\core_analytics\local\time_splitting\base') === false) {
                 $errors['timesplitting'] = get_string('errorinvalidtimesplitting', 'analytics');
             }
+
+            $targetclass = \tool_analytics\output\helper::option_to_class($data['target']);
+            $timesplitting = \core_analytics\manager::get_time_splitting($timesplittingclass);
+            $target = \core_analytics\manager::get_target($targetclass);
+            if (!$target->can_use_timesplitting($timesplitting)) {
+                $errors['timesplitting'] = get_string('invalidtimesplitting', 'tool_analytics');
+            }
         }
 
         if (!$this->_customdata['staticmodel']) {
index b459933..63648a9 100644 (file)
@@ -96,6 +96,10 @@ class models_list implements \renderable, \templatable {
             $onlycli = 1;
         }
 
+        // Evaluation options.
+        $timesplittingsforevaluation = \core_analytics\manager::get_time_splitting_methods_for_evaluation(true);
+
+        $misconfiguredmodels = [];
         $data->models = array();
         foreach ($this->models as $model) {
             $modeldata = $model->export($output);
@@ -112,6 +116,10 @@ class models_list implements \renderable, \templatable {
                     describe its purpose.", DEBUG_DEVELOPER);
             }
 
+            if ($model->invalid_timesplitting_selected()) {
+                $misconfiguredmodels[$model->get_id()] = $model->get_name();
+            }
+
             // Check if there is a help icon for the indicators to show.
             if (!empty($modeldata->indicators)) {
                 $indicators = array();
@@ -206,20 +214,7 @@ class models_list implements \renderable, \templatable {
                 $actionid = 'evaluate-' . $model->get_id();
 
                 // Evaluation options.
-                $modeltimesplittingmethods = [
-                    ['id' => 'all', 'text' => get_string('alltimesplittingmethods', 'tool_analytics')],
-                ];
-                $potentialtimesplittingmethods = $model->get_potential_timesplittings();
-                foreach (\core_analytics\manager::get_time_splitting_methods_for_evaluation(true) as $timesplitting) {
-                    if (empty($potentialtimesplittingmethods[$timesplitting->get_id()])) {
-                        // This time-splitting method can not be used for this model.
-                        continue;
-                    }
-                    $modeltimesplittingmethods[] = [
-                        'id' => \tool_analytics\output\helper::class_to_option($timesplitting->get_id()),
-                        'text' => $timesplitting->get_name()->out(),
-                    ];
-                }
+                $modeltimesplittingmethods = $this->timesplittings_options_for_evaluation($model, $timesplittingsforevaluation);
 
                 // Include the current time-splitting method as the default selection method the model already have one.
                 if ($model->get_model_obj()->timesplitting) {
@@ -338,10 +333,10 @@ class models_list implements \renderable, \templatable {
             $data->models[] = $modeldata;
         }
 
+        $data->warnings = [];
+        $data->infos = [];
         if (!$onlycli) {
-            $data->warnings = array(
-                (object)array('message' => get_string('bettercli', 'tool_analytics'), 'closebutton' => true)
-            );
+            $data->warnings[] = (object)array('message' => get_string('bettercli', 'tool_analytics'), 'closebutton' => true);
         } else {
             $url = new \moodle_url('/admin/settings.php', array('section' => 'analyticssettings'),
                 'id_s_analytics_onlycli');
@@ -350,12 +345,43 @@ class models_list implements \renderable, \templatable {
             if (is_siteadmin()) {
                 $langstrid = 'clievaluationandpredictions';
             }
-            $data->infos = array(
-                (object)array('message' => get_string($langstrid, 'tool_analytics', $url->out()),
-                    'closebutton' => true)
-            );
+            $data->infos[] = (object)array('message' => get_string($langstrid, 'tool_analytics', $url->out()),
+                'closebutton' => true);
+        }
+
+        if ($misconfiguredmodels) {
+            $warningstr = get_string('invalidtimesplittinginmodels', 'tool_analytics', implode(', ', $misconfiguredmodels));
+            $data->warnings[] = (object)array('message' => $warningstr, 'closebutton' => true);
         }
 
         return $data;
     }
+
+    /**
+     * Returns the list of time splitting methods that are available for evaluation.
+     *
+     * @param  \core_analytics\model $model
+     * @param  array                 $timesplittingsforevaluation
+     * @return array
+     */
+    private function timesplittings_options_for_evaluation(\core_analytics\model $model,
+            array $timesplittingsforevaluation): array {
+
+        $modeltimesplittingmethods = [
+            ['id' => 'all', 'text' => get_string('alltimesplittingmethods', 'tool_analytics')],
+        ];
+        $potentialtimesplittingmethods = $model->get_potential_timesplittings();
+        foreach ($timesplittingsforevaluation as $timesplitting) {
+            if (empty($potentialtimesplittingmethods[$timesplitting->get_id()])) {
+                // This time-splitting method can not be used for this model.
+                continue;
+            }
+            $modeltimesplittingmethods[] = [
+                'id' => \tool_analytics\output\helper::class_to_option($timesplitting->get_id()),
+                'text' => $timesplitting->get_name()->out(),
+            ];
+        }
+
+        return $modeltimesplittingmethods;
+    }
 }
index b1fa75d..7041b83 100644 (file)
@@ -98,7 +98,10 @@ $string['insights'] = 'Insights';
 $string['invalidanalysables'] = 'Invalid site elements';
 $string['invalidanalysablesinfo'] = 'This page lists analysable elements that can\'t be used by this prediction model. The listed elements can\'t be used either to train the prediction model nor can the prediction model obtain predictions for them.';
 $string['invalidanalysablestable'] = 'Invalid site analysable elements table';
-$string['invalidindicatorsremoved'] = 'A new model has been added. Indicators that do not work with the selected target have been automatically removed.';
+$string['invalidcurrenttimesplitting'] = 'The current analysis interval is invalid for the target of this model. Please select a different interval.';
+$string['invalidindicatorsremoved'] = 'A new model has been added. Indicators that don\'t work with the selected target have been automatically removed.';
+$string['invalidtimesplitting'] = 'The selected analysis interval is invalid for the selected target.';
+$string['invalidtimesplittinginmodels'] = 'The analysis interval used by some of the models is invalid. Please select a different interval for the following models: {$a}';
 $string['invalidprediction'] = 'Invalid to get predictions';
 $string['invalidtraining'] = 'Invalid to train the model';
 $string['loginfo'] = 'Log extra info';
index ba49d7e..653ab82 100644 (file)
@@ -113,12 +113,18 @@ switch ($action) {
     case 'edit':
         confirm_sesskey();
 
+        $invalidcurrenttimesplitting = $model->invalid_timesplitting_selected();
+        $potentialtimesplittings = $model->get_potential_timesplittings();
+
         $customdata = array(
             'id' => $model->get_id(),
             'trainedmodel' => $model->is_trained(),
             'staticmodel' => $model->is_static(),
+            'invalidcurrenttimesplitting' => (!empty($invalidcurrenttimesplitting)),
+            'targetclass' => $model->get_target()->get_id(),
+            'targetname' => $model->get_target()->get_name(),
             'indicators' => $model->get_potential_indicators(),
-            'timesplittings' => $model->get_potential_timesplittings(),
+            'timesplittings' => $potentialtimesplittings,
             'predictionprocessors' => \core_analytics\manager::get_all_prediction_processors()
         );
         $mform = new \tool_analytics\output\form\edit_model(null, $customdata);
index ad17226..daf2724 100644 (file)
@@ -84,6 +84,16 @@ abstract class base extends \core_analytics\calculable {
      */
     abstract protected function calculate_sample($sampleid, \core_analytics\analysable $analysable, $starttime = false, $endtime = false);
 
+    /**
+     * Can the provided time-splitting method be used on this target?.
+     *
+     * Time-splitting methods not matching the target requirements will not be selectable by models based on this target.
+     *
+     * @param  \core_analytics\local\time_splitting\base $timesplitting
+     * @return bool
+     */
+    abstract public function can_use_timesplitting(\core_analytics\local\time_splitting\base $timesplitting): bool;
+
     /**
      * Is this target generating insights?
      *
@@ -104,18 +114,6 @@ abstract class base extends \core_analytics\calculable {
         return false;
     }
 
-    /**
-     * Can the provided time-splitting method be used on this target?.
-     *
-     * Time-splitting methods not matching the target requirements will not be selectable by models based on this target.
-     *
-     * @param  \core_analytics\local\time_splitting\base $timesplitting
-     * @return bool
-     */
-    public function can_use_timesplitting(\core_analytics\local\time_splitting\base $timesplitting): bool {
-        return true;
-    }
-
     /**
      * Update the last analysis time on analysable processed or always.
      *
index 93d653e..195c00e 100644 (file)
@@ -1799,6 +1799,26 @@ class model {
             has_capability('moodle/analytics:managemodels', \context_system::instance()), $displayname, $this->model->name);
     }
 
+    /**
+     * Returns true if the time-splitting method used by this model is invalid for this model.
+     * @return  bool
+     */
+    public function invalid_timesplitting_selected(): bool {
+        $currenttimesplitting = $this->model->timesplitting;
+        if (empty($currenttimesplitting)) {
+            // Not set is different from invalid. This function is used to identify invalid
+            // time-splittings.
+            return false;
+        }
+
+        $potentialtimesplittings = $this->get_potential_timesplittings();
+        if ($currenttimesplitting && empty($potentialtimesplittings[$currenttimesplitting])) {
+            return true;
+        }
+
+        return false;
+    }
+
     /**
      * Adds the id from {analytics_predictions} db table to the prediction \stdClass objects.
      *
index 59a90bb..8ce97c1 100644 (file)
@@ -61,6 +61,16 @@ class test_static_target_shortname extends test_target_shortname {
         return true;
     }
 
+    /**
+     * Everything yep, this is just for testing.
+     *
+     * @param  \core_analytics\local\time_splitting\base $timesplitting
+     * @return bool
+     */
+    public function can_use_timesplitting(\core_analytics\local\time_splitting\base $timesplitting): bool {
+        return true;
+    }
+
     /**
      * Different analyser just to test a different one.
      *
index e78cbf5..342f516 100644 (file)
@@ -65,6 +65,16 @@ class test_target_site_users extends \core_analytics\local\target\binary {
         return 'test_site_users_analyser';
     }
 
+    /**
+     * Everything yep, this is just for testing.
+     *
+     * @param  \core_analytics\local\time_splitting\base $timesplitting
+     * @return bool
+     */
+    public function can_use_timesplitting(\core_analytics\local\time_splitting\base $timesplitting): bool {
+        return true;
+    }
+
     /**
      * classes_description
      *
index c8853c2..3a12570 100644 (file)
@@ -6,7 +6,7 @@ information provided here is intended especially for developers.
 * "Time-splitting method" have been replaced by "Analysis interval" for the language strings that are
   displayed in the Moodle UI. The name of several time-splitting methods have been updated according
   to the new description of the field.
-* A new target::can_use_timesplitting method can be used to discard time-splitting methods that can not
+* A new target::can_use_timesplitting method must be implemented to discard time-splitting methods that can not
   be used on a target.
 
 === 3.7 ===
index 47a160b..cbeceea 100644 (file)
@@ -37,7 +37,7 @@ $string['defaultpredictoroption'] = 'Default processor ({$a})';
 $string['disabledmodel'] = 'Disabled model';
 $string['erroralreadypredict'] = 'File {$a} has already been used to generate predictions.';
 $string['errorcannotreaddataset'] = 'Dataset file {$a} cannot be read.';
-$string['errorcannotusetimesplitting'] = 'The provided analysis interval can not be used on this model.';
+$string['errorcannotusetimesplitting'] = 'The provided analysis interval can\'t be used on this model.';
 $string['errorcannotwritedataset'] = 'Dataset file {$a} cannot be written.';
 $string['errorexportmodelresult'] = 'The machine learning model cannot be exported.';
 $string['errorimport'] = 'Error importing the provided JSON file.';