MDL-64320 analytics: Validate discrete indicators separately

analytics/classes/local/indicator/base.php

@@ -169,10 +169,7 @@ abstract class base extends \core_analytics\calculable {
 
             if (!is_null($calculatedvalue)) {
                 $notnulls[$sampleid] = $sampleid;
-                if ($calculatedvalue > self::MAX_VALUE || $calculatedvalue < self::MIN_VALUE) {
-                    throw new \coding_exception('Calculated values should be higher than ' . self::MIN_VALUE .
-                        ' and lower than ' . self::MAX_VALUE . ' ' . $calculatedvalue . ' received');
-                }
+                $this->validate_calculated_value($calculatedvalue);
             }
 
             $calculations[$sampleid] = $calculatedvalue;
@@ -182,4 +179,19 @@ abstract class base extends \core_analytics\calculable {
 
         return array($features, $newcalculations, $notnulls);
     }
+
+    /**
+     * Validates the calculated value.
+     *
+     * @throws \coding_exception
+     * @param float $calculatedvalue
+     * @return true
+     */
+    protected function validate_calculated_value($calculatedvalue) {
+        if ($calculatedvalue > self::MAX_VALUE || $calculatedvalue < self::MIN_VALUE) {
+            throw new \coding_exception('Calculated values should be higher than ' . self::MIN_VALUE .
+                ' and lower than ' . self::MAX_VALUE . ' ' . $calculatedvalue . ' received');
+        }
+        return true;
+    }
 }

analytics/classes/local/indicator/binary.php

@@ -41,9 +41,7 @@ abstract class binary extends discrete {
      * @return array
      */
     public static final function get_classes() {
-        // It does not really matter, all \core_analytics\local\indicator\discrete get_classes calls have been overwriten as we
-        // only need 1 column here.
-        return array(0);
+        return [-1, 1];
     }
 
     /**

analytics/classes/local/indicator/discrete.php

@@ -52,8 +52,7 @@ abstract class discrete extends base {
     public static function get_feature_headers() {
         $fullclassname = '\\' . get_called_class();
 
-        $headers = array($fullclassname);
-        foreach (self::get_classes() as $class) {
+        foreach (static::get_classes() as $class) {
             $headers[] = $fullclassname . '/' . $class;
         }
 
@@ -116,26 +115,45 @@ abstract class discrete extends base {
      */
     protected function to_features($calculatedvalues) {
 
-        $classes = self::get_classes();
+        $classes = static::get_classes();
 
         foreach ($calculatedvalues as $sampleid => $calculatedvalue) {
 
-            $classindex = array_search($calculatedvalue, $classes, true);
+            // Using intval as it may come as a float from the db.
+            $classindex = array_search(intval($calculatedvalue), $classes, true);
 
-            if (!$classindex) {
-                throw new \coding_exception(get_class($this) . ' calculated "' . $calculatedvalue .
-                    '" which is not one of its defined classes (' . json_encode($classes) . ')');
+            if ($classindex === false && !is_null($calculatedvalue)) {
+                throw new \coding_exception(get_class($this) . ' calculated value "' . $calculatedvalue .
+                    '" is not one of its defined classes (' . json_encode($classes) . ')');
             }
 
             // We transform the calculated value into multiple features, one for each of the possible classes.
             $features = array_fill(0, count($classes), 0);
 
             // 1 to the selected value.
-            $features[$classindex] = 1;
+            if (!is_null($calculatedvalue)) {
+                $features[$classindex] = 1;
+            }
 
             $calculatedvalues[$sampleid] = $features;
         }
 
         return $calculatedvalues;
     }
+
+    /**
+     * Validates the calculated value.
+     *
+     * @param float $calculatedvalue
+     * @return true
+     */
+    protected function validate_calculated_value($calculatedvalue) {
+
+        // Using intval as it may come as a float from the db.
+        if (!in_array(intval($calculatedvalue), static::get_classes())) {
+            throw new \coding_exception(get_class($this) . ' calculated value "' . $calculatedvalue .
+                '" is not one of its defined classes (' . json_encode(static::get_classes()) . ')');
+        }
+        return true;
+    }
 }

analytics/tests/fixtures/test_indicator_discrete.php

@@ -0,0 +1,90 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Test indicator.
+ *
+ * @package   core_analytics
+ * @copyright 2019 David Monllao {@link http://www.davidmonllao.com}
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Test indicator.
+ *
+ * @package   core_analytics
+ * @copyright 2019 David Monllao {@link http://www.davidmonllao.com}
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class test_indicator_discrete extends \core_analytics\local\indicator\discrete {
+
+    /**
+     * Returns the name.
+     *
+     * If there is a corresponding '_help' string this will be shown as well.
+     *
+     * @return \lang_string
+     */
+    public static function get_name() : \lang_string {
+        // Using a string that exists and contains a corresponding '_help' string.
+        return new \lang_string('allowstealthmodules');
+    }
+
+    /**
+     * The different classes this discrete indicator provides.
+     * @return [type] [description]
+     */
+    protected static function get_classes() {
+        return [0, 1, 2, 3, 4];
+    }
+
+    /**
+     * Just for testing.
+     *
+     * @param  float $value
+     * @param  string $subtype
+     * @return string
+     */
+    public function get_calculation_outcome($value, $subtype = false) {
+        return self::OUTCOME_OK;
+    }
+
+    /**
+     * Custom indicator calculated value display as otherwise we would display meaningless numbers to users.
+     *
+     * @param  float  $value
+     * @param  string $subtype
+     * @return string
+     */
+    public function get_display_value($value, $subtype = false) {
+        return $value;
+    }
+
+    /**
+     * calculate_sample
+     *
+     * @param int $sampleid
+     * @param string $sampleorigin
+     * @param int $starttime
+     * @param int $endtime
+     * @return float
+     */
+    protected function calculate_sample($sampleid, $sampleorigin, $starttime = false, $endtime = false) {
+        return 4;
+    }
+}

analytics/tests/fixtures/test_indicator_random.php

@@ -31,7 +31,7 @@ defined('MOODLE_INTERNAL') || die();
  * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com}
  * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class test_indicator_random extends \core_analytics\local\indicator\binary {
+class test_indicator_random extends \core_analytics\local\indicator\linear {
 
     /**
      * Returns a lang_string object representing the name for the indicator.

analytics/tests/indicator_test.php

@@ -0,0 +1,99 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Unit tests for the indicator API.
+ *
+ * @package   core_analytics
+ * @copyright 2019 David Monllaó {@link http://www.davidmonllao.com}
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+require_once(__DIR__ . '/fixtures/test_indicator_max.php');
+require_once(__DIR__ . '/fixtures/test_indicator_discrete.php');
+require_once(__DIR__ . '/fixtures/test_indicator_min.php');
+
+/**
+ * Unit tests for the model.
+ *
+ * @package   core_analytics
+ * @copyright 2017 David Monllaó {@link http://www.davidmonllao.com}
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class analytics_indicator_testcase extends advanced_testcase {
+
+    /**
+     * test_validate_calculated_value
+     *
+     * @param string $indicatorclass
+     * @param array $returnedvalue
+     * @dataProvider validate_calculated_value
+     * @return null
+     */
+    public function test_validate_calculated_value($indicatorclass, $returnedvalue) {
+        $indicator = new $indicatorclass();
+        list($values, $unused) = $indicator->calculate([1], 'notrelevanthere');
+        $this->assertEquals($returnedvalue, $values[0]);
+    }
+
+    /**
+     * Data provider for test_validate_calculated_value
+     *
+     * @return array
+     */
+    public function validate_calculated_value() {
+        return [
+            'max' => ['test_indicator_max', [1]],
+            'min' => ['test_indicator_min', [-1]],
+            'discrete' => ['test_indicator_discrete', [0, 0, 0, 0, 1]],
+        ];
+    }
+
+    /**
+     * test_validate_calculated_value_exceptions
+     *
+     * @param string $indicatorclass
+     * @param string $willreturn
+     * @dataProvider validate_calculated_value_exceptions
+     * @expectedException \coding_exception
+     * @return null
+     */
+    public function test_validate_calculated_value_exceptions($indicatorclass, $willreturn) {
+
+        $indicator = new $indicatorclass();
+        $indicatormock = $this->getMockBuilder(get_class($indicator))
+            ->setMethods(['calculate_sample'])
+            ->getMock();
+        $indicatormock->method('calculate_sample')->willReturn($willreturn);
+        list($values, $unused) = $indicatormock->calculate([1], 'notrelevanthere');
+
+    }
+
+    /**
+     * Data provider for test_validate_calculated_value_exceptions
+     *
+     * @return array
+     */
+    public function validate_calculated_value_exceptions() {
+        return [
+            'max' => ['test_indicator_max', 2],
+            'min' => ['test_indicator_min', -2],
+            'discrete' => ['test_indicator_discrete', 7],
+        ];
+    }
+}

analytics/tests/prediction_test.php

@@ -452,7 +452,7 @@ class core_analytics_prediction_testcase extends advanced_testcase {
         $indicator = $this->getMockBuilder('test_indicator_max')->setMethods(['calculate_sample'])->getMock();
         $indicator->expects($this->never())->method('calculate_sample');
 
-        $existingcalcs = array(111 => 1, 222 => 0.5);
+        $existingcalcs = array(111 => 1, 222 => -1);
         $sampleids = array(111 => 111, 222 => 222);
         list($values, $unused) = $indicator->calculate($sampleids, $sampleorigin, $starttime, $endtime, $existingcalcs);
     }