MDL-57791 analytics: Fixes during integration review

This commit includes the following changes:
- cibot complains fixes
- removed randomly failing test
- fixed course_dropout return
- other minor fixes

diff --git a/admin/tool/models/amd/build/log_info.min.js b/admin/tool/models/amd/build/log_info.min.js
index c4d0a9e..5b09246 100644 (file)
Binary files a/admin/tool/models/amd/build/log_info.min.js and b/admin/tool/models/amd/build/log_info.min.js differ

diff --git a/admin/tool/models/amd/src/log_info.js b/admin/tool/models/amd/src/log_info.js
index 2b89be9..f14e11d 100644 (file)
--- a/admin/tool/models/amd/src/log_info.js
+++ b/admin/tool/models/amd/src/log_info.js
@@ -39,9 +39,9 @@ define(['jquery', 'core/str', 'core/modal_factory', 'core/notification'], functi
             str.get_string('loginfo', 'tool_models').then(function(langString) {
 
                 var bodyInfo = $("<ul>");
-                for (var i in info) {
-                    bodyInfo.append("<li>" + info[i] + "</li>");
-                }
+                info.forEach(function(item) {
+                    bodyInfo.append('<li>' + item + '</li>');
+                });
                 bodyInfo.append("</ul>");
 
                 return ModalFactory.create({

diff --git a/admin/tool/models/classes/output/renderer.php b/admin/tool/models/classes/output/renderer.php
index 6f1455b..04d1f1e 100644 (file)
--- a/admin/tool/models/classes/output/renderer.php
+++ b/admin/tool/models/classes/output/renderer.php
@@ -42,7 +42,7 @@ class renderer extends plugin_renderer_base {
     /**
      * Defer to template.
      *
-     * @param \tool_models\output\models_list $templatable
+     * @param \tool_models\output\models_list $modelslist
      * @return string HTML
      */
     protected function render_models_list(\tool_models\output\models_list $modelslist) {

diff --git a/analytics/classes/calculable.php b/analytics/classes/calculable.php
index cda4fc3..a3e3720 100644 (file)
--- a/analytics/classes/calculable.php
+++ b/analytics/classes/calculable.php
@@ -126,9 +126,7 @@ abstract class calculable {
      * @param string|false $subtype
      * @return int
      */
-    public function get_calculation_outcome($value, $subtype = false) {
-        throw new \coding_exception('Please overwrite get_calculation_outcome method');
-    }
+    abstract public function get_calculation_outcome($value, $subtype = false);
 
     /**
      * Retrieve the specified element associated to $sampleid.

diff --git a/analytics/classes/local/target/base.php b/analytics/classes/local/target/base.php
index 688e230..466255e 100644 (file)
--- a/analytics/classes/local/target/base.php
+++ b/analytics/classes/local/target/base.php
@@ -181,7 +181,7 @@ abstract class base extends \core_analytics\calculable {
 
                 $message->fullmessage = get_string('insightinfomessage', 'analytics', $insighturl->out());
                 $message->fullmessageformat = FORMAT_PLAIN;
-                $message->fullmessagehtml = get_string('insightinfomessage', 'analytics', $insighturl->out());
+                $message->fullmessagehtml = get_string('insightinfomessagehtml', 'analytics', $insighturl->out());
                 $message->smallmessage = get_string('insightinfomessage', 'analytics', $insighturl->out());
                 $message->contexturl = $insighturl->out(false);
 

diff --git a/analytics/classes/manager.php b/analytics/classes/manager.php
index 2c1cfd2..0574a9e 100644 (file)
--- a/analytics/classes/manager.php
+++ b/analytics/classes/manager.php
@@ -100,7 +100,7 @@ class manager {
                 $conditions[] = 'am.trained = :trained';
                 $params['trained'] = 1;
             }
-            $sql .= ' WHERE ' . implode(' AND ', $conditions);
+            $sql .= ' WHERE ' . implode(' AND ', $conditions) . ' ORDER BY am.timemodified DESC';
         }
         $modelobjs = $DB->get_records_sql($sql, $params);
 
@@ -390,7 +390,7 @@ class manager {
      */
     public static function add_builtin_models() {
 
-        $target = \core_analytics\manager::get_target('\core\analytics\target\course_dropout');
+        $target = self::get_target('\core\analytics\target\course_dropout');
 
         // Community of inquiry indicators.
         $coiindicators = array(
@@ -439,7 +439,7 @@ class manager {
         );
         $indicators = array();
         foreach ($coiindicators as $coiindicator) {
-            $indicator = \core_analytics\manager::get_indicator($coiindicator);
+            $indicator = self::get_indicator($coiindicator);
             $indicators[$indicator->get_id()] = $indicator;
         }
         if (!\core_analytics\model::exists($target, $indicators)) {
@@ -447,9 +447,9 @@ class manager {
         }
 
         // No teaching model.
-        $target = \core_analytics\manager::get_target('\core\analytics\target\no_teaching');
+        $target = self::get_target('\core\analytics\target\no_teaching');
         $timesplittingmethod = '\core\analytics\time_splitting\single_range';
-        $noteacher = \core_analytics\manager::get_indicator('\core_course\analytics\indicator\no_teacher');
+        $noteacher = self::get_indicator('\core_course\analytics\indicator\no_teacher');
         $indicators = array($noteacher->get_id() => $noteacher);
         if (!\core_analytics\model::exists($target, $indicators)) {
             \core_analytics\model::create($target, $indicators, $timesplittingmethod);

diff --git a/analytics/classes/model.php b/analytics/classes/model.php
index 11191ed..fecb10a 100644 (file)
--- a/analytics/classes/model.php
+++ b/analytics/classes/model.php
@@ -1066,7 +1066,6 @@ class model {
 
         list($unused, $samplesdata) = $this->get_analyser()->get_samples($sampleids);
 
-
         $current = 0;
 
         if ($page !== false) {

diff --git a/analytics/tests/fixtures/test_static_target_shortname.php b/analytics/tests/fixtures/test_static_target_shortname.php
index 9a662ae..015cbcc 100644 (file)
--- a/analytics/tests/fixtures/test_static_target_shortname.php
+++ b/analytics/tests/fixtures/test_static_target_shortname.php
@@ -22,10 +22,10 @@
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 
-require_once(__DIR__ . '/test_target_shortname.php');
-
 defined('MOODLE_INTERNAL') || die();
 
+require_once(__DIR__ . '/test_target_shortname.php');
+
 /**
  * Test static target.
  *

diff --git a/analytics/tests/prediction_test.php b/analytics/tests/prediction_test.php
index fd15197..f335a15 100644 (file)
--- a/analytics/tests/prediction_test.php
+++ b/analytics/tests/prediction_test.php
@@ -280,19 +280,7 @@ class core_analytics_prediction_testcase extends advanced_testcase {
      */
     public function provider_ml_test_evaluation() {
 
-        $notenoughandlowscore = \core_analytics\model::EVALUATE_NOT_ENOUGH_DATA + \core_analytics\model::EVALUATE_LOW_SCORE;
         $cases = array(
-            'bad-and-no-enough-data' => array(
-                'modelquality' => 'random',
-                'ncourses' => 5,
-                'expectedresults' => array(
-                    // The course duration is too much to be processed by in weekly basis.
-                    '\core\analytics\time_splitting\weekly' => \core_analytics\model::NO_DATASET,
-                    // 10 samples is not enough to process anything.
-                    '\core\analytics\time_splitting\single_range' => $notenoughandlowscore,
-                    '\core\analytics\time_splitting\quarters' => $notenoughandlowscore,
-                )
-            ),
             'bad' => array(
                 'modelquality' => 'random',
                 'ncourses' => 50,

diff --git a/lang/en/analytics.php b/lang/en/analytics.php
index 4f1b68d..fc368aa 100644 (file)
--- a/lang/en/analytics.php
+++ b/lang/en/analytics.php
@@ -53,7 +53,8 @@ $string['errorunknownaction'] = 'Unknown action';
 $string['eventpredictionactionstarted'] = 'Prediction action started';
 $string['insightmessagesubject'] = 'New insight for "{$a->contextname}": {$a->insightname}';
 $string['insightinfo'] = '{$a->insightname} - {$a->contextname}';
-$string['insightinfomessage'] = 'There are some insights you may find useful. Check out {$a}';
+$string['insightinfomessage'] = 'The system generated some insights for you: {$a}';
+$string['insightinfomessagehtml'] = 'The system generated some insights for you: <a href="{$a}">{$a}</a>.';
 $string['invalidtimesplitting'] = 'Model with id {$a} needs a time splitting method before it can be used to train';
 $string['invalidanalysablefortimesplitting'] = 'It can not be analysed using {$a} time splitting method';
 $string['modeloutputdir'] = 'Models output directory';

diff --git a/lib/classes/analytics/target/course_dropout.php b/lib/classes/analytics/target/course_dropout.php
index 80c8890..8a021eb 100644 (file)
--- a/lib/classes/analytics/target/course_dropout.php
+++ b/lib/classes/analytics/target/course_dropout.php
@@ -198,7 +198,7 @@ class course_dropout extends \core_analytics\local\target\binary {
      * @param \core_analytics\analysable $course
      * @param int $starttime
      * @param int $endtime
-     * @return float
+     * @return float 0 -> not at risk, 1 -> at risk
      */
     protected function calculate_sample($sampleid, \core_analytics\analysable $course, $starttime = false, $endtime = false) {
 
@@ -226,8 +226,8 @@ class course_dropout extends \core_analytics\local\target\binary {
         $params = array('userid' => $userenrol->userid, 'courseid' => $course->get_id(), 'limit' => $limit);
         $nlogs = $logstore->get_events_select_count($select, $params);
         if ($nlogs == 0) {
-            return 0;
+            return 1;
         }
-        return 1;
+        return 0;
     }
 }

diff --git a/lib/tests/indicators_test.php b/lib/tests/indicators_test.php
index 73f623d..13461d0 100644 (file)
--- a/lib/tests/indicators_test.php
+++ b/lib/tests/indicators_test.php
@@ -18,7 +18,7 @@
  * Unit tests for core indicators.
  *
  * @package   core
- * @category  phpunit
+ * @category  analytics
  * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com}
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
@@ -33,7 +33,7 @@ require_once(__DIR__ . '/../../lib/enrollib.php');
  * Unit tests for core indicators.
  *
  * @package   core
- * @category  phpunit
+ * @category  analytics
  * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com}
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */

diff --git a/lib/tests/time_splittings_test.php b/lib/tests/time_splittings_test.php
index 8210245..fe99b83 100644 (file)
--- a/lib/tests/time_splittings_test.php
+++ b/lib/tests/time_splittings_test.php
@@ -18,7 +18,7 @@
  * Unit tests for core time splitting methods.
  *
  * @package   core
- * @category  phpunit
+ * @category  analytics
  * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com}
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
@@ -32,7 +32,7 @@ require_once(__DIR__ . '/../../lib/enrollib.php');
  * Unit tests for core time splitting methods.
  *
  * @package   core
- * @category  phpunit
+ * @category  analytics
  * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com}
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */