From a0ef4bb8fe42bbd4106687adc29d27cf48ac6bf5 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Fri, 30 Aug 2024 10:55:25 +0100 Subject: [PATCH] MDL-82913 reportbuilder: simplify select filter for limited options. Where the select filter contains upto two items (making it a binary choice), then we can simplify the filter form elements to always assume: "Equal to [ Option 1 | Option 2 ]". --- admin/tests/behat/browse_users.feature | 7 +- .../datasource/task_logs_test.php | 8 ++- .../classes/local/filters/select.php | 67 ++++++++++++------- reportbuilder/tests/behat/audience.feature | 4 +- 4 files changed, 49 insertions(+), 37 deletions(-) diff --git a/admin/tests/behat/browse_users.feature b/admin/tests/behat/browse_users.feature index b06f508de9d..232209b4215 100644 --- a/admin/tests/behat/browse_users.feature +++ b/admin/tests/behat/browse_users.feature @@ -171,9 +171,7 @@ Feature: An administrator can browse user accounts | User Two | two@example.com | | User Three | three@example.com | And I click on "Filters" "button" - And I set the following fields in the "System role" "core_reportbuilder > Filter" to these values: - | System role operator | Is equal to | - | System role value | Course creator | + And I set the field "System role value" in the "System role" "core_reportbuilder > Filter" to "Course creator" And I click on "Apply" "button" in the "[data-region='report-filters']" "css_element" And I click on "Filters" "button" And I should see "User One" in the "reportbuilder-table" "table" @@ -181,8 +179,7 @@ Feature: An administrator can browse user accounts And I should not see "User Three" in the "reportbuilder-table" "table" And I click on "Filters" "button" And I click on "Reset all" "button" in the "[data-region='report-filters']" "css_element" - And I set the following fields in the "Course role" "core_reportbuilder > Filter" to these values: - | Role name | Teacher | + And I set the field "Role name" in the "Course role" "core_reportbuilder > Filter" to "Teacher" And I click on "Apply" "button" in the "[data-region='report-filters']" "css_element" And I click on "Filters" "button" And I should not see "User One" in the "reportbuilder-table" "table" diff --git a/admin/tests/reportbuilder/datasource/task_logs_test.php b/admin/tests/reportbuilder/datasource/task_logs_test.php index 511cd332836..d9f8b7948a4 100644 --- a/admin/tests/reportbuilder/datasource/task_logs_test.php +++ b/admin/tests/reportbuilder/datasource/task_logs_test.php @@ -21,7 +21,7 @@ namespace core_admin\reportbuilder\datasource; use core\task\database_logger; use core_reportbuilder_generator; use core_reportbuilder_testcase; -use core_reportbuilder\local\filters\{boolean_select, date, duration, number, select, text}; +use core_reportbuilder\local\filters\{date, duration, number, select, text}; use core_reportbuilder\task\send_schedules; defined('MOODLE_INTERNAL') || die(); @@ -141,10 +141,12 @@ class task_logs_test extends core_reportbuilder_testcase { 'task_log:output_operator' => text::IS_EMPTY, ], false], 'Filter result' => ['task_log:result', [ - 'task_log:result_operator' => boolean_select::CHECKED, + 'task_log:result_operator' => select::EQUAL_TO, + 'task_log:result_value' => 0, ], true], 'Filter result (no match)' => ['task_log:result', [ - 'task_log:result_operator' => boolean_select::NOT_CHECKED, + 'task_log:result_operator' => select::EQUAL_TO, + 'task_log:result_value' => 1, ], false], 'Filter time start' => ['task_log:timestart', [ 'task_log:timestart_operator' => date::DATE_RANGE, diff --git a/reportbuilder/classes/local/filters/select.php b/reportbuilder/classes/local/filters/select.php index 5c3a0ea5389..5ecb3f0aa88 100644 --- a/reportbuilder/classes/local/filters/select.php +++ b/reportbuilder/classes/local/filters/select.php @@ -74,20 +74,47 @@ class select extends base { * @param MoodleQuickForm $mform */ public function setup_form(MoodleQuickForm $mform): void { - $elements = []; - $elements['operator'] = $mform->createElement('select', $this->name . '_operator', - get_string('filterfieldoperator', 'core_reportbuilder', $this->get_header()), $this->get_operators()); - - // If a multi-dimensional array is passed, we need to use a different element type. + $operators = $this->get_operators(); $options = $this->get_select_options(); - $element = (count($options) == count($options, COUNT_RECURSIVE) ? 'select' : 'selectgroups'); - $elements['value'] = $mform->createElement($element, $this->name . '_value', - get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()), $options); - $mform->addGroup($elements, $this->name . '_group', $this->get_header(), '', false) - ->setHiddenLabel(true); + // If a multidimensional array is passed, we need to use a different element type. + $optioncountrecursive = count($options, COUNT_RECURSIVE); + $element = (count($options) === $optioncountrecursive ? 'select' : 'selectgroups'); - $mform->hideIf($this->name . '_value', $this->name . '_operator', 'eq', self::ANY_VALUE); + // If operators are unrestricted, and we have upto two options, then simplify the filter to list only those. + if (count($operators) === 3 && $optioncountrecursive <= 2) { + $mform->addElement('hidden', "{$this->name}_operator"); + $mform->setType("{$this->name}_operator", PARAM_INT); + $mform->setConstant("{$this->name}_operator", self::EQUAL_TO); + + $mform->addElement( + $element, + "{$this->name}_value", + get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()), + ['' => $operators[self::ANY_VALUE]] + $options, + )->setHiddenLabel(true); + } else { + $elements = []; + + $elements[] = $mform->createElement( + 'select', + "{$this->name}_operator", + get_string('filterfieldoperator', 'core_reportbuilder', $this->get_header()), + $operators, + ); + + $elements[] = $mform->createElement( + $element, + "{$this->name}_value", + get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()), + $options, + ); + + $mform->addGroup($elements, "{$this->name}_group", $this->get_header(), '', false) + ->setHiddenLabel(true); + + $mform->hideIf("{$this->name}_value", "{$this->name}_operator", 'eq', self::ANY_VALUE); + } } /** @@ -101,15 +128,14 @@ class select extends base { public function get_sql_filter(array $values): array { $name = database::generate_param_name(); - $operator = $values["{$this->name}_operator"] ?? self::ANY_VALUE; - $value = $values["{$this->name}_value"] ?? 0; + $operator = (int) ($values["{$this->name}_operator"] ?? self::ANY_VALUE); + $value = $values["{$this->name}_value"] ?? ''; $fieldsql = $this->filter->get_field_sql(); $params = $this->filter->get_field_params(); // Validate filter form values. - if (!$this->validate_filter_values((int) $operator, $value)) { - // Filter configuration is invalid. Ignore the filter. + if ($value === '') { return ['', []]; } @@ -128,17 +154,6 @@ class select extends base { return [$fieldsql, $params]; } - /** - * Validate filter form values - * - * @param int|null $operator - * @param mixed|null $value - * @return bool - */ - private function validate_filter_values(?int $operator, $value): bool { - return !($operator === null || $value === ''); - } - /** * Return sample filter values * diff --git a/reportbuilder/tests/behat/audience.feature b/reportbuilder/tests/behat/audience.feature index 3f3090913b0..101bbea6373 100644 --- a/reportbuilder/tests/behat/audience.feature +++ b/reportbuilder/tests/behat/audience.feature @@ -127,9 +127,7 @@ Feature: Configure access to reports based on intended audience | User 3 | # Now let's filter them. And I click on "Filters" "button" - And I set the following fields in the "Audience" "core_reportbuilder > Filter" to these values: - | Audience operator | Is equal to | - | Audience value | Site administrators | + And I set the field "Audience value" in the "Audience" "core_reportbuilder > Filter" to "Site administrators" And I click on "Apply" "button" in the "[data-region='report-filters']" "css_element" And the following should exist in the "Report access list" table: | -1- |