diff --git a/admin/tool/analytics/classes/output/form/edit_model.php b/admin/tool/analytics/classes/output/form/edit_model.php index b529d50932f..c897b5c81bc 100644 --- a/admin/tool/analytics/classes/output/form/edit_model.php +++ b/admin/tool/analytics/classes/output/form/edit_model.php @@ -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']) { diff --git a/admin/tool/analytics/classes/output/models_list.php b/admin/tool/analytics/classes/output/models_list.php index b45993348d6..63648a985d6 100644 --- a/admin/tool/analytics/classes/output/models_list.php +++ b/admin/tool/analytics/classes/output/models_list.php @@ -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; + } } diff --git a/admin/tool/analytics/lang/en/tool_analytics.php b/admin/tool/analytics/lang/en/tool_analytics.php index b1fa75d924f..7041b83b4c7 100644 --- a/admin/tool/analytics/lang/en/tool_analytics.php +++ b/admin/tool/analytics/lang/en/tool_analytics.php @@ -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'; diff --git a/admin/tool/analytics/model.php b/admin/tool/analytics/model.php index ba49d7ea2b2..653ab82db1e 100644 --- a/admin/tool/analytics/model.php +++ b/admin/tool/analytics/model.php @@ -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); diff --git a/analytics/classes/local/target/base.php b/analytics/classes/local/target/base.php index ad17226c3dc..daf27246b45 100644 --- a/analytics/classes/local/target/base.php +++ b/analytics/classes/local/target/base.php @@ -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. * diff --git a/analytics/classes/model.php b/analytics/classes/model.php index 93d653eccf0..195c00e6ac0 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -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. * diff --git a/analytics/tests/fixtures/test_static_target_shortname.php b/analytics/tests/fixtures/test_static_target_shortname.php index 59a90bb0ec8..8ce97c1d6cc 100644 --- a/analytics/tests/fixtures/test_static_target_shortname.php +++ b/analytics/tests/fixtures/test_static_target_shortname.php @@ -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. * diff --git a/analytics/tests/fixtures/test_target_site_users.php b/analytics/tests/fixtures/test_target_site_users.php index e78cbf584c0..342f5169645 100644 --- a/analytics/tests/fixtures/test_target_site_users.php +++ b/analytics/tests/fixtures/test_target_site_users.php @@ -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 * diff --git a/analytics/upgrade.txt b/analytics/upgrade.txt index c8853c26059..3a12570b4d4 100644 --- a/analytics/upgrade.txt +++ b/analytics/upgrade.txt @@ -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 === diff --git a/lang/en/analytics.php b/lang/en/analytics.php index 47a160ba758..cbeceea407b 100644 --- a/lang/en/analytics.php +++ b/lang/en/analytics.php @@ -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.';