diff --git a/analytics/classes/local/indicator/base.php b/analytics/classes/local/indicator/base.php index 5cd88b9cdba..4a0d576fefe 100644 --- a/analytics/classes/local/indicator/base.php +++ b/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; + } } diff --git a/analytics/classes/local/indicator/binary.php b/analytics/classes/local/indicator/binary.php index a7309961d69..2e4d4a9d6d9 100644 --- a/analytics/classes/local/indicator/binary.php +++ b/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]; } /** diff --git a/analytics/classes/local/indicator/discrete.php b/analytics/classes/local/indicator/discrete.php index 8a8aa6699d2..b04b4c6a5f4 100644 --- a/analytics/classes/local/indicator/discrete.php +++ b/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; + } } diff --git a/analytics/tests/fixtures/test_indicator_discrete.php b/analytics/tests/fixtures/test_indicator_discrete.php new file mode 100644 index 00000000000..983b6554922 --- /dev/null +++ b/analytics/tests/fixtures/test_indicator_discrete.php @@ -0,0 +1,90 @@ +. + +/** + * 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; + } +} diff --git a/analytics/tests/fixtures/test_indicator_random.php b/analytics/tests/fixtures/test_indicator_random.php index 8b934dda8db..19106a682ed 100644 --- a/analytics/tests/fixtures/test_indicator_random.php +++ b/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. diff --git a/analytics/tests/indicator_test.php b/analytics/tests/indicator_test.php new file mode 100644 index 00000000000..add3f3d4914 --- /dev/null +++ b/analytics/tests/indicator_test.php @@ -0,0 +1,99 @@ +. + +/** + * 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], + ]; + } +} diff --git a/analytics/tests/prediction_test.php b/analytics/tests/prediction_test.php index 34d82f7ab3d..7d74868013e 100644 --- a/analytics/tests/prediction_test.php +++ b/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); }