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 97fc4a0b596..8c578fd7d03 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -11,6 +11,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 ===