From 21202090f7e344d2f4d2612ee47926a3302e2941 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Monlla=C3=B3?= Date: Thu, 22 Aug 2019 15:49:41 +0800 Subject: [PATCH 1/3] MDL-66091 report_insights: Usability improvements - More feedback provided for the user once they click on useful/notuseful from the email. - Replace the indicator calculations table headers for the table caption and replace the text by just "Indicators" - Replace "System" for the site name for insights generated at system level - Replace "$modelname prediction" for "$modelname" in report/insights/prediction.php heading MDL-66091 --- analytics/classes/insights_generator.php | 3 ++- report/insights/done.php | 17 +++++++++++++++-- report/insights/insights.php | 6 +++++- report/insights/lang/en/report_insights.php | 7 ++----- report/insights/prediction.php | 6 +++++- .../insights/templates/insight_details.mustache | 16 +++++----------- 6 files changed, 34 insertions(+), 21 deletions(-) diff --git a/analytics/classes/insights_generator.php b/analytics/classes/insights_generator.php index 24fdfaf1d8d..6dad5ebcba9 100644 --- a/analytics/classes/insights_generator.php +++ b/analytics/classes/insights_generator.php @@ -195,7 +195,8 @@ class insights_generator { $actionurl = $action->get_url(); if (!$actionurl->get_param('forwardurl')) { - $actiondoneurl = new \moodle_url('/report/insights/done.php'); + $params = ['actionvisiblename' => $action->get_text(), 'target' => '_blank']; + $actiondoneurl = new \moodle_url('/report/insights/done.php', $params); // Set the forward url to the 'done' script. $actionurl->param('forwardurl', $actiondoneurl->out(false)); } diff --git a/report/insights/done.php b/report/insights/done.php index d64d15120b4..4878ef0092d 100644 --- a/report/insights/done.php +++ b/report/insights/done.php @@ -26,5 +26,18 @@ require_once(__DIR__ . '/../../config.php'); require_login(); -$url = new \moodle_url('/'); -redirect($url, get_string('actionsaved', 'report_insights'), null, \core\output\notification::NOTIFY_SUCCESS); +$actionvisiblename = required_param('actionvisiblename', PARAM_NOTAGS); + +$PAGE->set_pagelayout('popup'); +$PAGE->set_context(\context_system::instance()); +$PAGE->set_title(get_site()->fullname); +$PAGE->set_url(new \moodle_url('/report/insights/done.php')); + +echo $OUTPUT->header(); + +$notification = new \core\output\notification(get_string('actionsaved', 'report_insights', $actionvisiblename), + \core\output\notification::NOTIFY_SUCCESS); +$notification->set_show_closebutton(false); +echo $OUTPUT->render($notification); + +echo $OUTPUT->footer(); diff --git a/report/insights/insights.php b/report/insights/insights.php index c45aaf9e79b..2aaab8b05f3 100644 --- a/report/insights/insights.php +++ b/report/insights/insights.php @@ -103,8 +103,12 @@ if (!$model->uses_insights()) { exit(0); } +if ($context->id == SYSCONTEXTID) { + $PAGE->set_heading(get_site()->shortname); +} else { + $PAGE->set_heading($insightinfo->contextname); +} $PAGE->set_title($insightinfo->insightname); -$PAGE->set_heading($insightinfo->contextname); // Some models generate one single prediction per context. We can directly show the prediction details in this case. if ($model->get_analyser()::one_sample_per_analysable()) { diff --git a/report/insights/lang/en/report_insights.php b/report/insights/lang/en/report_insights.php index 646a6e6694d..db7c32e2651 100644 --- a/report/insights/lang/en/report_insights.php +++ b/report/insights/lang/en/report_insights.php @@ -22,11 +22,9 @@ * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -$string['actionsaved'] = 'Your action has been saved.'; -$string['calculatedvalue'] = 'Calculated value'; +$string['actionsaved'] = 'Your feedback of \'{$a}\' has been saved.'; $string['disabledmodel'] = 'Sorry, this model has been disabled by the administrator'; -$string['indicator'] = 'Indicator'; -$string['insightprediction'] = '{$a} prediction'; +$string['indicators'] = 'Indicators'; $string['insight'] = 'Insight'; $string['insights'] = 'Insights'; $string['justpredictions'] = 'Please note that the following insights are only predictions. It is not possible to predict the future with any certainty. The insights are provided so that action can be taken as necessary to prevent any negative predictions becoming reality.'; @@ -39,7 +37,6 @@ $string['outcomeverypositive'] = 'Very positive outcome'; $string['outcomeverynegative'] = 'Very negative outcome'; $string['pluginname'] = 'Insights'; $string['prediction'] = 'Prediction'; -$string['predictioncalculations'] = 'Indicator calculations'; $string['predictiondetails'] = 'Prediction details'; $string['nodetailsavailable'] = 'No prediction details are relevant.'; $string['timecreated'] = 'Time predicted'; diff --git a/report/insights/prediction.php b/report/insights/prediction.php index bb67a028487..6db5e394bc6 100644 --- a/report/insights/prediction.php +++ b/report/insights/prediction.php @@ -71,8 +71,12 @@ if (!$model->uses_insights()) { exit(0); } +if ($context->id == SYSCONTEXTID) { + $PAGE->set_heading(get_site()->shortname); +} else { + $PAGE->set_heading($insightinfo->contextname); +} $PAGE->set_title($insightinfo->insightname); -$PAGE->set_heading($insightinfo->contextname); echo $OUTPUT->header(); diff --git a/report/insights/templates/insight_details.mustache b/report/insights/templates/insight_details.mustache index ad61ef1b94a..33164dd933e 100644 --- a/report/insights/templates/insight_details.mustache +++ b/report/insights/templates/insight_details.mustache @@ -73,7 +73,7 @@ } }} -

{{#str}}insightprediction, report_insights, {{insightname}} {{/str}}

+

{{insightname}}

{{#showpredicionheading}} - + {{#timerange}} - + {{/timerange}}
@@ -101,29 +101,23 @@ {{#str}}predictiondetails, report_insights{{/str}}
{{#str}}timecreated, report_insights{{/str}}{{#str}}timecreated, report_insights{{/str}} {{timecreated}}
{{#str}}timerange, report_insights{{/str}}{{#str}}timerange, report_insights{{/str}} {{.}}
- - - - - - - + {{#calculations}} - + {{/calculations}} From 486e797c5f2cb5aeb17eb9e0b386595d92cc5c8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Monlla=C3=B3?= Date: Thu, 22 Aug 2019 15:50:59 +0800 Subject: [PATCH 2/3] MDL-66091 analytics: Targets choose if there should be a report or not --- analytics/classes/insights_generator.php | 6 +++- analytics/classes/local/target/base.php | 11 ++++++- analytics/classes/manager.php | 5 +++- analytics/classes/model.php | 29 ++++++++++--------- .../insight_info_message_prediction.mustache | 5 ++-- lang/en/calendar.php | 1 + report/insights/insights.php | 20 +++++++++++++ .../target/upcoming_activities_due.php | 13 +++++++-- 8 files changed, 70 insertions(+), 20 deletions(-) diff --git a/analytics/classes/insights_generator.php b/analytics/classes/insights_generator.php index 6dad5ebcba9..cb24ccdc251 100644 --- a/analytics/classes/insights_generator.php +++ b/analytics/classes/insights_generator.php @@ -193,19 +193,23 @@ class insights_generator { $insighturl = null; foreach ($predictionactions as $action) { $actionurl = $action->get_url(); + $opentoblank = false; if (!$actionurl->get_param('forwardurl')) { $params = ['actionvisiblename' => $action->get_text(), 'target' => '_blank']; $actiondoneurl = new \moodle_url('/report/insights/done.php', $params); // Set the forward url to the 'done' script. $actionurl->param('forwardurl', $actiondoneurl->out(false)); + + $opentoblank = true; } if (empty($insighturl)) { // We use the primary action url as insight url so we log that the user followed the provided link. $insighturl = $action->get_url(); } - $actiondata = (object)['url' => $action->get_url()->out(false), 'text' => $action->get_text()]; + $actiondata = (object)['url' => $action->get_url()->out(false), 'text' => $action->get_text(), + 'opentoblank' => $opentoblank]; $fullmessageplaintext .= get_string('insightinfomessageaction', 'analytics', $actiondata) . PHP_EOL; $messageactions[] = $actiondata; } diff --git a/analytics/classes/local/target/base.php b/analytics/classes/local/target/base.php index daf27246b45..0db27aa6d40 100644 --- a/analytics/classes/local/target/base.php +++ b/analytics/classes/local/target/base.php @@ -105,6 +105,15 @@ abstract class base extends \core_analytics\calculable { return true; } + /** + * Should the insights of this model be linked from reports? + * + * @return bool + */ + public function link_insights_report(): bool { + return true; + } + /** * Based on facts (processed by machine learning backends) by default. * @@ -147,7 +156,7 @@ abstract class base extends \core_analytics\calculable { $actions = array(); - if ($includedetailsaction) { + if ($this->link_insights_report() && $includedetailsaction) { $predictionurl = new \moodle_url('/report/insights/prediction.php', array('id' => $predictionid)); $detailstext = $this->get_view_details_text(); diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php index 03b02e9399a..a5553d56197 100644 --- a/analytics/classes/manager.php +++ b/analytics/classes/manager.php @@ -484,6 +484,9 @@ class manager { /** * Returns the models with insights at the provided context. * + * Note that this method is used for display purposes. It filters out models whose insights + * are not linked from the reports page. + * * @param \context $context * @return \core_analytics\model[] */ @@ -494,7 +497,7 @@ class manager { $models = self::get_all_models(true, true, $context); foreach ($models as $key => $model) { // Check that it not only have predictions but also generates insights from them. - if (!$model->uses_insights()) { + if (!$model->uses_insights() || !$model->get_target()->link_insights_report()) { unset($models[$key]); } } diff --git a/analytics/classes/model.php b/analytics/classes/model.php index e5ca153c6d4..83ccb923e4d 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -963,19 +963,22 @@ class model { $this->get_target()->generate_insight_notifications($this->model->id, $samplecontexts, $predictions); - // Update cache. - $cache = \cache::make('core', 'contextwithinsights'); - foreach ($samplecontexts as $context) { - $modelids = $cache->get($context->id); - if (!$modelids) { - // The cache is empty, but we don't know if it is empty because there are no insights - // in this context or because cache/s have been purged, we need to be conservative and - // "pay" 1 db read to fill up the cache. - $models = \core_analytics\manager::get_models_with_insights($context); - $cache->set($context->id, array_keys($models)); - } else if (!in_array($this->get_id(), $modelids)) { - array_push($modelids, $this->get_id()); - $cache->set($context->id, $modelids); + if ($this->get_target()->link_insights_report()) { + + // Update cache. + $cache = \cache::make('core', 'contextwithinsights'); + foreach ($samplecontexts as $context) { + $modelids = $cache->get($context->id); + if (!$modelids) { + // The cache is empty, but we don't know if it is empty because there are no insights + // in this context or because cache/s have been purged, we need to be conservative and + // "pay" 1 db read to fill up the cache. + $models = \core_analytics\manager::get_models_with_insights($context); + $cache->set($context->id, array_keys($models)); + } else if (!in_array($this->get_id(), $modelids)) { + array_push($modelids, $this->get_id()); + $cache->set($context->id, $modelids); + } } } } diff --git a/analytics/templates/insight_info_message_prediction.mustache b/analytics/templates/insight_info_message_prediction.mustache index 9896031bf6d..7bd76de7c0d 100644 --- a/analytics/templates/insight_info_message_prediction.mustache +++ b/analytics/templates/insight_info_message_prediction.mustache @@ -33,7 +33,8 @@ "text": "Moodle" }, { "url": "https://en.wikipedia.org/wiki/Noodle", - "text": "Noodle" + "text": "Noodle", + "opentoblank": 1 } ] } @@ -65,5 +66,5 @@ body:not(.dir-ltr):not(.dir-rtl) .btn-insight {
{{#actions}} - {{text}} + {{text}} {{/actions}} diff --git a/lang/en/calendar.php b/lang/en/calendar.php index 9a7982ef762..2e31f24bb36 100644 --- a/lang/en/calendar.php +++ b/lang/en/calendar.php @@ -261,6 +261,7 @@ $string['urlforical'] = 'URL for iCalendar export, for subscribing to calendar'; $string['user'] = 'User'; $string['userevent'] = 'User event'; $string['userevents'] = 'User events'; +$string['viewupcomingactivitiesdue'] = 'View the upcoming activities due'; $string['wed'] = 'Wed'; $string['wednesday'] = 'Wednesday'; $string['weekly'] = 'Weekly'; diff --git a/report/insights/insights.php b/report/insights/insights.php index 2aaab8b05f3..5dc9dfc0361 100644 --- a/report/insights/insights.php +++ b/report/insights/insights.php @@ -40,10 +40,24 @@ if ($context->contextlevel < CONTEXT_COURSE) { // Only for higher levels than course. $PAGE->set_context($context); } + \core_analytics\manager::check_can_list_insights($context); // Get all models that are enabled, trained and have predictions at this context. $othermodels = \core_analytics\manager::get_all_models(true, true, $context); +array_filter($othermodels, function($model) use ($context) { + + // Discard insights that are not linked unless you are a manager. + if (!$model->get_target()->link_insights_report()) { + try { + \core_analytics\manager::check_can_manage_models(); + } catch (\required_capability_exception $e) { + return false; + } + } + return true; +}); + if (!$modelid && count($othermodels)) { // Autoselect the only available model. $model = reset($othermodels); @@ -89,6 +103,12 @@ if (!$modelid) { $model = new \core_analytics\model($modelid); +if (!$model->get_target()->link_insights_report()) { + + // Only manager access if this target does not link the insights report. + \core_analytics\manager::check_can_manage_models(); +} + $insightinfo = new stdClass(); $insightinfo->contextname = $context->get_context_name(); $insightinfo->insightname = $model->get_target()->get_name(); diff --git a/user/classes/analytics/target/upcoming_activities_due.php b/user/classes/analytics/target/upcoming_activities_due.php index b97ecbe1d44..bd3fe9edc02 100644 --- a/user/classes/analytics/target/upcoming_activities_due.php +++ b/user/classes/analytics/target/upcoming_activities_due.php @@ -160,6 +160,15 @@ class upcoming_activities_due extends \core_analytics\local\target\binary { return 0; } + /** + * No need to link to the insights report in this case. + * + * @return bool + */ + public function link_insights_report(): bool { + return false; + } + /** * Adds a view upcoming events action. * @@ -180,9 +189,9 @@ class upcoming_activities_due extends \core_analytics\local\target\binary { // We force a lookahead of 30 days so we are sure that the upcoming activities due are shown. $url = new \moodle_url('/calendar/view.php', ['view' => 'upcoming', 'lookahead' => '30']); - $pix = new \pix_icon('i/calendar', get_string('upcomingevents', 'calendar')); + $pix = new \pix_icon('i/calendar', get_string('viewupcomingactivitiesdue', 'calendar')); $action = new \core_analytics\prediction_action('viewupcoming', $prediction, - $url, $pix, get_string('upcomingevents', 'calendar')); + $url, $pix, get_string('viewupcomingactivitiesdue', 'calendar')); return array_merge([$action], $parentactions); } From 93e71c712ddee02fb049a5cfe72ff0f57d689c42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Monlla=C3=B3?= Date: Tue, 17 Sep 2019 07:32:53 +0800 Subject: [PATCH 3/3] MDL-66091 report_insights: Unify contextwithinsights cache sets --- analytics/classes/manager.php | 39 +++++++++++++++++++++++++++++++++++ analytics/classes/model.php | 13 +----------- lib/deprecatedlib.php | 17 +++++++++++++++ report/insights/lib.php | 25 +++------------------- 4 files changed, 60 insertions(+), 34 deletions(-) diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php index a5553d56197..699d22b7727 100644 --- a/analytics/classes/manager.php +++ b/analytics/classes/manager.php @@ -504,6 +504,45 @@ class manager { return $models; } + /** + * Returns the models that generated insights in the provided context. It can also be used to add new models to the context. + * + * Note that if you use this function with $newmodelid is the caller responsibility to ensure that the + * provided model id generated insights for the provided context. + * + * @throws \coding_exception + * @param \context $context + * @param int|null $newmodelid A new model to add to the list of models with insights in the provided context. + * @return int[] + */ + public static function cached_models_with_insights(\context $context, int $newmodelid = null) { + + $cache = \cache::make('core', 'contextwithinsights'); + $modelids = $cache->get($context->id); + if ($modelids === false) { + // The cache is empty, but we don't know if it is empty because there are no insights + // in this context or because cache/s have been purged, we need to be conservative and + // "pay" 1 db read to fill up the cache. + + $models = \core_analytics\manager::get_models_with_insights($context); + + if ($newmodelid && empty($models[$newmodelid])) { + throw new \coding_exception('The provided modelid ' . $newmodelid . ' did not generate any insights'); + } + + $modelids = array_keys($models); + $cache->set($context->id, $modelids); + + } else if ($newmodelid && !in_array($newmodelid, $modelids)) { + // We add the context we got as an argument to the cache. + + array_push($modelids, $newmodelid); + $cache->set($context->id, $modelids); + } + + return $modelids; + } + /** * Returns a prediction * diff --git a/analytics/classes/model.php b/analytics/classes/model.php index 83ccb923e4d..3678d97adba 100644 --- a/analytics/classes/model.php +++ b/analytics/classes/model.php @@ -966,19 +966,8 @@ class model { if ($this->get_target()->link_insights_report()) { // Update cache. - $cache = \cache::make('core', 'contextwithinsights'); foreach ($samplecontexts as $context) { - $modelids = $cache->get($context->id); - if (!$modelids) { - // The cache is empty, but we don't know if it is empty because there are no insights - // in this context or because cache/s have been purged, we need to be conservative and - // "pay" 1 db read to fill up the cache. - $models = \core_analytics\manager::get_models_with_insights($context); - $cache->set($context->id, array_keys($models)); - } else if (!in_array($this->get_id(), $modelids)) { - array_push($modelids, $this->get_id()); - $cache->set($context->id, $modelids); - } + \core_analytics\manager::cached_models_with_insights($context, $this->get_id()); } } } diff --git a/lib/deprecatedlib.php b/lib/deprecatedlib.php index afc91d429f0..e8335e730ff 100644 --- a/lib/deprecatedlib.php +++ b/lib/deprecatedlib.php @@ -3449,3 +3449,20 @@ function get_courses_page($categoryid="all", $sort="c.sortorder ASC", $fields="c $rs->close(); return $visiblecourses; } + +/** + * Returns the models that generated insights in the provided context. + * + * @deprecated since Moodle 3.8 MDL-66091 - please do not use this function any more. + * @todo MDL-65799 This will be deleted in Moodle 4.2 + * @see \core_analytics\manager::cached_models_with_insights + * @param \context $context + * @return int[] + */ +function report_insights_context_insights(\context $context) { + + debugging('report_insights_context_insights is deprecated. Please use ' . + '\core_analytics\manager::cached_models_with_insights instead', DEBUG_DEVELOPER); + + return \core_analytics\manager::cached_models_with_insights($context); +} diff --git a/report/insights/lib.php b/report/insights/lib.php index 97c9ecb86e4..eb7b4f42139 100644 --- a/report/insights/lib.php +++ b/report/insights/lib.php @@ -36,7 +36,7 @@ function report_insights_extend_navigation_course($navigation, $course, $context if (has_capability('moodle/analytics:listinsights', $context)) { - $modelids = report_insights_context_insights($context); + $modelids = \core_analytics\manager::cached_models_with_insights($context); if (!empty($modelids)) { $url = new moodle_url('/report/insights/insights.php', array('contextid' => $context->id)); $node = navigation_node::create(get_string('insights', 'report_insights'), $url, navigation_node::TYPE_SETTING, @@ -61,7 +61,7 @@ function report_insights_myprofile_navigation(core_user\output\myprofile\tree $t $context = \context_user::instance($user->id); if (\core_analytics\manager::check_can_list_insights($context, true)) { - $modelids = report_insights_context_insights($context); + $modelids = \core_analytics\manager::cached_models_with_insights($context); if (!empty($modelids)) { $url = new moodle_url('/report/insights/insights.php', array('contextid' => $context->id)); $node = new core_user\output\myprofile\node('reports', 'insights', get_string('insights', 'report_insights'), @@ -82,7 +82,7 @@ function report_insights_extend_navigation_category_settings($navigation, $conte if (has_capability('moodle/analytics:listinsights', $context)) { - $modelids = report_insights_context_insights($context); + $modelids = \core_analytics\manager::cached_models_with_insights($context); if (!empty($modelids)) { $url = new moodle_url('/report/insights/insights.php', array('contextid' => $context->id)); @@ -99,22 +99,3 @@ function report_insights_extend_navigation_category_settings($navigation, $conte } } } - -/** - * Returns the models that generated insights in the provided context. - * - * @param \context $context - * @return int[] - */ -function report_insights_context_insights(\context $context) { - - $cache = \cache::make('core', 'contextwithinsights'); - $modelids = $cache->get($context->id); - if ($modelids === false) { - // They will be full unless a model has been cleared. - $models = \core_analytics\manager::get_models_with_insights($context); - $modelids = array_keys($models); - $cache->set($context->id, $modelids); - } - return $modelids; -}
{{#str}}predictioncalculations, report_insights{{/str}}
{{#str}}indicator, report_insights{{/str}}{{#str}}calculatedvalue, report_insights{{/str}}
{{#str}}indicators, report_insights{{/str}}
{{name}}{{name}} {{#outcomeicon}}{{> core/pix_icon}}{{/outcomeicon}} {{displayvalue}}