From d9f05b81d0e10458a4b2174b8514ab232a2070ef Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Thu, 7 Mar 2024 18:41:23 +0000 Subject: [PATCH] MDL-81168 reportbuilder: better float input support in filters. Support localised float values in filter forms, so that locales that use different decimal separators work as expected. Note lack of current support in the date filter. --- reportbuilder/classes/local/filters/date.php | 3 ++ .../classes/local/filters/duration.php | 24 ++++++------ .../classes/local/filters/filesize.php | 10 +++-- .../classes/local/filters/number.php | 37 ++++++++++++------- .../tests/local/filters/duration_test.php | 12 +++--- .../tests/local/filters/filesize_test.php | 8 ++-- .../tests/local/filters/number_test.php | 29 ++++++++------- reportbuilder/upgrade.txt | 4 ++ 8 files changed, 75 insertions(+), 52 deletions(-) diff --git a/reportbuilder/classes/local/filters/date.php b/reportbuilder/classes/local/filters/date.php index 6886cd1547b..f1f12346144 100644 --- a/reportbuilder/classes/local/filters/date.php +++ b/reportbuilder/classes/local/filters/date.php @@ -111,6 +111,9 @@ class date extends base { /** * Setup form * + * Note that we cannot support float inputs in this filter currently, because decimals are not supported when calculating + * relative timeframes according to {@link https://www.php.net/manual/en/datetime.formats.php} + * * @param MoodleQuickForm $mform */ public function setup_form(MoodleQuickForm $mform): void { diff --git a/reportbuilder/classes/local/filters/duration.php b/reportbuilder/classes/local/filters/duration.php index 7fbf75f15c6..2a37198e513 100644 --- a/reportbuilder/classes/local/filters/duration.php +++ b/reportbuilder/classes/local/filters/duration.php @@ -25,7 +25,7 @@ use core_reportbuilder\local\helpers\database; /** * Duration report filter * - * This filter accepts a number of seconds to perform filtering on + * This filter accepts a number of seconds to perform filtering on (note that the value will be cast to float prior to comparison) * * @package core_reportbuilder * @copyright 2021 Paul Holden @@ -77,7 +77,7 @@ class duration extends base { $valuelabel = get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()); $elements[] = $mform->createElement('text', "{$this->name}_value", $valuelabel, ['size' => 3]); - $mform->setType("{$this->name}_value", PARAM_FLOAT); + $mform->setType("{$this->name}_value", PARAM_LOCALISEDFLOAT); $mform->setDefault("{$this->name}_value", 0); $mform->hideIf("{$this->name}_value", "{$this->name}_operator", 'eq', self::DURATION_ANY); @@ -107,27 +107,25 @@ class duration extends base { * @return array */ public function get_sql_filter(array $values): array { + global $DB; + $fieldsql = $this->filter->get_field_sql(); $params = $this->filter->get_field_params(); + $operator = (int) ($values["{$this->name}_operator"] ?? self::DURATION_ANY); + $durationvalue = unformat_float($values["{$this->name}_value"] ?? 0); $durationunit = (int) ($values["{$this->name}_unit"] ?? 0); - $operator = $values["{$this->name}_operator"] ?? self::DURATION_ANY; + $paramduration = database::generate_param_name(); + $params[$paramduration] = $durationvalue * $durationunit; + switch ($operator) { case self::DURATION_MAXIMUM: - $paramduration = database::generate_param_name(); - - $sql = "{$fieldsql} <= :{$paramduration}"; - $params[$paramduration] = $durationvalue * $durationunit; - + $sql = $DB->sql_cast_char2real("({$fieldsql})") . " <= :{$paramduration}"; break; case self::DURATION_MINIMUM: - $paramduration = database::generate_param_name(); - - $sql = "{$fieldsql} >= :{$paramduration}"; - $params[$paramduration] = $durationvalue * $durationunit; - + $sql = $DB->sql_cast_char2real("({$fieldsql})") . " >= :{$paramduration}"; break; default: // Invalid or inactive filter. diff --git a/reportbuilder/classes/local/filters/filesize.php b/reportbuilder/classes/local/filters/filesize.php index 57c66ba10ed..9f78bd26b60 100644 --- a/reportbuilder/classes/local/filters/filesize.php +++ b/reportbuilder/classes/local/filters/filesize.php @@ -25,6 +25,8 @@ use MoodleQuickForm; /** * Filesize report filter * + * This filter accepts a number value to perform filtering on (note that the value will be cast to float prior to comparison) + * * @package core_reportbuilder * @copyright 2023 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -82,7 +84,7 @@ class filesize extends base { // Value selector. $elements[] = $mform->createElement('text', "{$this->name}_value1", get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()), ['size' => 4]); - $mform->setType("{$this->name}_value1", PARAM_FLOAT); + $mform->setType("{$this->name}_value1", PARAM_LOCALISEDFLOAT); $mform->setDefault("{$this->name}_value1", 1); $mform->hideIf("{$this->name}_value1", "{$this->name}_operator", 'eq', self::ANY_VALUE); @@ -111,6 +113,8 @@ class filesize extends base { * @return array */ public function get_sql_filter(array $values): array { + global $DB; + $fieldsql = $this->filter->get_field_sql(); $params = $this->filter->get_field_params(); @@ -124,10 +128,10 @@ class filesize extends base { switch ($operator) { case self::LESS_THAN: - $sql = "{$fieldsql} < :{$paramfilesize}"; + $sql = $DB->sql_cast_char2real("({$fieldsql})") . " < :{$paramfilesize}"; break; case self::GREATER_THAN: - $sql = "{$fieldsql} > :{$paramfilesize}"; + $sql = $DB->sql_cast_char2real("({$fieldsql})") . " > :{$paramfilesize}"; break; default: // Invalid or inactive filter. diff --git a/reportbuilder/classes/local/filters/number.php b/reportbuilder/classes/local/filters/number.php index 98e26a07690..e69825ad5c3 100644 --- a/reportbuilder/classes/local/filters/number.php +++ b/reportbuilder/classes/local/filters/number.php @@ -23,6 +23,8 @@ use core_reportbuilder\local\helpers\database; /** * Number report filter * + * This filter accepts a number value to perform filtering on (note that the value will be cast to float prior to comparison) + * * @package core_reportbuilder * @copyright 2021 David Matamoros * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later @@ -91,14 +93,14 @@ class number extends base { $objs['text'] = $mform->createElement('text', $this->name . '_value1', get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()), ['size' => 3]); - $mform->setType($this->name . '_value1', PARAM_INT); + $mform->setType($this->name . '_value1', PARAM_LOCALISEDFLOAT); $mform->setDefault($this->name . '_value1', 0); $mform->hideIf($this->name . '_value1', $this->name . '_operator', 'in', [self::ANY_VALUE, self::IS_NOT_EMPTY, self::IS_EMPTY]); $objs['text2'] = $mform->createElement('text', $this->name . '_value2', - get_string('filterfieldto', 'reportbuilder', $this->get_header()), ['size' => 3]); - $mform->setType($this->name . '_value2', PARAM_INT); + get_string('filterfieldto', 'core_reportbuilder', $this->get_header()), ['size' => 3]); + $mform->setType($this->name . '_value2', PARAM_LOCALISEDFLOAT); $mform->setDefault($this->name . '_value2', 0); $mform->hideIf($this->name . '_value2', $this->name . '_operator', 'noteq', self::RANGE); @@ -113,10 +115,17 @@ class number extends base { * @return array array of two elements - SQL query and named parameters */ public function get_sql_filter(array $values): array { + global $DB; + $operator = (int) ($values["{$this->name}_operator"] ?? self::ANY_VALUE); - $value1 = $values["{$this->name}_value1"] ?? null; - $value2 = $values["{$this->name}_value2"] ?? null; + $value1 = $value2 = null; + if (array_key_exists("{$this->name}_value1", $values)) { + $value1 = unformat_float($values["{$this->name}_value1"]); + } + if (array_key_exists("{$this->name}_value2", $values)) { + $value2 = unformat_float($values["{$this->name}_value2"]); + } // Validate filter form values. if (!$this->validate_filter_values($operator, $value1, $value2)) { @@ -139,27 +148,27 @@ class number extends base { $res = "COALESCE({$fieldsql}, 0) = 0"; break; case self::LESS_THAN: - $res = "{$fieldsql} < :{$param}"; + $res = $DB->sql_cast_char2real("({$fieldsql})") . " < :{$param}"; $params[$param] = $value1; break; case self::GREATER_THAN: - $res = "{$fieldsql} > :{$param}"; + $res = $DB->sql_cast_char2real("({$fieldsql})") . " > :{$param}"; $params[$param] = $value1; break; case self::EQUAL_TO: - $res = "{$fieldsql} = :{$param}"; + $res = $DB->sql_cast_char2real("({$fieldsql})") . " = :{$param}"; $params[$param] = $value1; break; case self::EQUAL_OR_LESS_THAN: - $res = "{$fieldsql} <= :{$param}"; + $res = $DB->sql_cast_char2real("({$fieldsql})") . " <= :{$param}"; $params[$param] = $value1; break; case self::EQUAL_OR_GREATER_THAN: - $res = "{$fieldsql} >= :{$param}"; + $res = $DB->sql_cast_char2real("({$fieldsql})") . " >= :{$param}"; $params[$param] = $value1; break; case self::RANGE: - $res = "{$fieldsql} BETWEEN :{$param} AND :{$param2}"; + $res = $DB->sql_cast_char2real("({$fieldsql})") . " BETWEEN :{$param} AND :{$param2}"; $params[$param] = $value1; $params[$param2] = $value2; break; @@ -174,11 +183,11 @@ class number extends base { * Validate filter form values * * @param int $operator - * @param int|null $value1 - * @param int|null $value2 + * @param float|null $value1 + * @param float|null $value2 * @return bool */ - private function validate_filter_values(int $operator, ?int $value1, ?int $value2): bool { + private function validate_filter_values(int $operator, ?float $value1, ?float $value2): bool { // Check that for any of these operators value1 can not be null. $requirescomparisonvalue = [ self::LESS_THAN, diff --git a/reportbuilder/tests/local/filters/duration_test.php b/reportbuilder/tests/local/filters/duration_test.php index ee903b52787..6a58ea6913f 100644 --- a/reportbuilder/tests/local/filters/duration_test.php +++ b/reportbuilder/tests/local/filters/duration_test.php @@ -31,14 +31,14 @@ use core_reportbuilder\local\report\filter; * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class duration_test extends advanced_testcase { +final class duration_test extends advanced_testcase { /** * Data provider for {@see test_get_sql_filter} * * @return array */ - public function get_sql_filter_provider(): array { + public static function get_sql_filter_provider(): array { return [ 'Any duration' => [duration::DURATION_ANY, true], @@ -53,11 +53,11 @@ class duration_test extends advanced_testcase { 'Maximum minutes match' => [duration::DURATION_MAXIMUM, true, 150, MINSECS], 'Maximum hours non-match (float)' => - [duration::DURATION_MAXIMUM, false, 0.5, HOURSECS], + [duration::DURATION_MAXIMUM, false, 0.777, HOURSECS], 'Maximum hours non-match' => [duration::DURATION_MAXIMUM, false, 1, HOURSECS], 'Maximum hours match (float)' => - [duration::DURATION_MAXIMUM, true, 2.5, HOURSECS], + [duration::DURATION_MAXIMUM, true, 2.333, HOURSECS], 'Maximum hours match' => [duration::DURATION_MAXIMUM, true, 3, HOURSECS], @@ -71,11 +71,11 @@ class duration_test extends advanced_testcase { 'Minimum minutes non-match' => [duration::DURATION_MINIMUM, false, 150, MINSECS], 'Minimum hours match (float)' => - [duration::DURATION_MINIMUM, true, 0.5, HOURSECS], + [duration::DURATION_MINIMUM, true, 0.777, HOURSECS], 'Minimum hours match' => [duration::DURATION_MINIMUM, true, 1, HOURSECS], 'Minimum hours non-match (float)' => - [duration::DURATION_MINIMUM, false, 2.5, HOURSECS], + [duration::DURATION_MINIMUM, false, 2.333, HOURSECS], 'Minimum hours non-match' => [duration::DURATION_MINIMUM, false, 3, HOURSECS], ]; diff --git a/reportbuilder/tests/local/filters/filesize_test.php b/reportbuilder/tests/local/filters/filesize_test.php index 47bced2f62d..aebfac087b0 100644 --- a/reportbuilder/tests/local/filters/filesize_test.php +++ b/reportbuilder/tests/local/filters/filesize_test.php @@ -31,7 +31,7 @@ use lang_string; * @copyright 2023 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class filesize_test extends advanced_testcase { +final class filesize_test extends advanced_testcase { /** * Data provider for {@see test_get_sql_filter} @@ -44,12 +44,14 @@ class filesize_test extends advanced_testcase { [filesize::LESS_THAN, false, 10, filesize::SIZE_UNIT_BYTE], [filesize::LESS_THAN, false, 10, filesize::SIZE_UNIT_KILOBYTE], - [filesize::LESS_THAN, true, 10, filesize::SIZE_UNIT_MEGABYTE], + [filesize::LESS_THAN, false, 1.7, filesize::SIZE_UNIT_MEGABYTE], + [filesize::LESS_THAN, true, 2.3, filesize::SIZE_UNIT_MEGABYTE], [filesize::LESS_THAN, true, 10, filesize::SIZE_UNIT_GIGABYTE], [filesize::GREATER_THAN, true, 10, filesize::SIZE_UNIT_BYTE], [filesize::GREATER_THAN, true, 10, filesize::SIZE_UNIT_KILOBYTE], - [filesize::GREATER_THAN, false, 10, filesize::SIZE_UNIT_MEGABYTE], + [filesize::GREATER_THAN, true, 1.7, filesize::SIZE_UNIT_MEGABYTE], + [filesize::GREATER_THAN, false, 2.3, filesize::SIZE_UNIT_MEGABYTE], [filesize::GREATER_THAN, false, 10, filesize::SIZE_UNIT_GIGABYTE], ]; } diff --git a/reportbuilder/tests/local/filters/number_test.php b/reportbuilder/tests/local/filters/number_test.php index 963fee2d4d1..4240533025a 100644 --- a/reportbuilder/tests/local/filters/number_test.php +++ b/reportbuilder/tests/local/filters/number_test.php @@ -31,36 +31,39 @@ use core_reportbuilder\local\report\filter; * @copyright 2021 David Matamoros * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class number_test extends advanced_testcase { +final class number_test extends advanced_testcase { /** * Data provider for {@see test_get_sql_filter_simple} * * @return array[] */ - public function get_sql_filter_simple_provider(): array { + public static function get_sql_filter_simple_provider(): array { return [ [number::ANY_VALUE, null, null, true], [number::IS_NOT_EMPTY, null, null, true], [number::IS_EMPTY, null, null, false], - [number::LESS_THAN, 1, null, false], + [number::LESS_THAN, 122.5, null, false], [number::LESS_THAN, 123, null, false], - [number::LESS_THAN, 124, null, true], - [number::GREATER_THAN, 1, null, true], + [number::LESS_THAN, 123.5, null, true], + [number::GREATER_THAN, 122.5, null, true], [number::GREATER_THAN, 123, null, false], - [number::GREATER_THAN, 124, null, false], + [number::GREATER_THAN, 123.5, null, false], + [number::EQUAL_TO, 122, null, false], [number::EQUAL_TO, 123, null, true], [number::EQUAL_TO, 124, null, false], - [number::EQUAL_OR_LESS_THAN, 124, null, true], - [number::EQUAL_OR_LESS_THAN, 123, null, true], [number::EQUAL_OR_LESS_THAN, 122, null, false], + [number::EQUAL_OR_LESS_THAN, 123, null, true], + [number::EQUAL_OR_LESS_THAN, 124, null, true], [number::EQUAL_OR_GREATER_THAN, 122, null, true], [number::EQUAL_OR_GREATER_THAN, 123, null, true], [number::EQUAL_OR_GREATER_THAN, 124, null, false], + [number::RANGE, 121, 122, false], [number::RANGE, 122, 124, true], - [number::RANGE, 124, 125, false], [number::RANGE, 122, 123, true], + [number::RANGE, 122.5, 123.5, true], [number::RANGE, 123, 124, true], + [number::RANGE, 124, 125, false], ]; } @@ -68,13 +71,13 @@ class number_test extends advanced_testcase { * Test getting filter SQL * * @param int $operator - * @param int|null $value1 - * @param int|null $value2 + * @param float|null $value1 + * @param float|null $value2 * @param bool $expectmatch * * @dataProvider get_sql_filter_simple_provider */ - public function test_get_sql_filter_simple(int $operator, ?int $value1, ?int $value2, bool $expectmatch): void { + public function test_get_sql_filter_simple(int $operator, ?float $value1, ?float $value2, bool $expectmatch): void { global $DB; $this->resetAfterTest(); @@ -111,7 +114,7 @@ class number_test extends advanced_testcase { * * @return array[] */ - public function get_sql_filter_invalid_provider(): array { + public static function get_sql_filter_invalid_provider(): array { return [ [number::LESS_THAN], [number::GREATER_THAN], diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index 3e158489546..49be7ef495c 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -10,6 +10,10 @@ Information provided here is intended especially for developers. - `audience::get_all_audiences_menu_types` - `report::get_available_columns` * The base datasource `add_all_from_entities` method accepts optional parameter to specify which entities to add elements from +* In order to better support float values in filter forms, the following filter types now cast given SQL prior to comparison: + - `duration` + - `filesize` + - `number` === 4.4 ===