From acc654e8ce36a11bc4e8eec052165ec2238e6071 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 14 Jan 2025 21:06:54 +0000 Subject: [PATCH] MDL-84213 reportbuilder: add course "do not force" field options. Where course entity select elements theme, language and calendar are defined we should prepend with "Do not force" in order to match the interface when editing the same fields. The select filter has been updated to ensure it supports empty values when switched to simplified version (a0ef4bb8) as well as improving validation to ensure only present options can be used for filtering. --- .upgradenotes/MDL-82913-2025011510021468.yml | 8 +++++++ .upgradenotes/MDL-84213-2025011422342259.yml | 7 ++++++ .../reportbuilder/datasource/roles_test.php | 2 +- .../reportbuilder/datasource/cohorts_test.php | 6 ++--- .../datasource/competencies_test.php | 4 ++-- .../datasource/categories_test.php | 2 +- .../reportbuilder/datasource/courses_test.php | 11 +++++---- .../classes/local/entities/course.php | 8 +++---- reportbuilder/classes/local/entities/user.php | 2 +- .../classes/local/filters/select.php | 23 +++++++++++++++---- .../tests/local/filters/select_test.php | 15 ++++++++---- .../local/helpers/custom_fields_test.php | 2 +- .../helpers/user_profile_fields_test.php | 3 ++- .../reportbuilder/datasource/users_test.php | 4 ++-- 14 files changed, 67 insertions(+), 30 deletions(-) create mode 100644 .upgradenotes/MDL-82913-2025011510021468.yml create mode 100644 .upgradenotes/MDL-84213-2025011422342259.yml diff --git a/.upgradenotes/MDL-82913-2025011510021468.yml b/.upgradenotes/MDL-82913-2025011510021468.yml new file mode 100644 index 00000000000..8062c6e4562 --- /dev/null +++ b/.upgradenotes/MDL-82913-2025011510021468.yml @@ -0,0 +1,8 @@ +issueNumber: MDL-82913 +notes: + core_reportbuilder: + - message: >- + When the `select` filter contains upto two options only then the + operator field is removed, switching to a simpler value selection field + only (this may affect your Behat scenarios) + type: changed diff --git a/.upgradenotes/MDL-84213-2025011422342259.yml b/.upgradenotes/MDL-84213-2025011422342259.yml new file mode 100644 index 00000000000..2b6c79e4b7b --- /dev/null +++ b/.upgradenotes/MDL-84213-2025011422342259.yml @@ -0,0 +1,7 @@ +issueNumber: MDL-84213 +notes: + core_reportbuilder: + - message: >- + The `select` filter type is now stricter in it's filtering, in that it + will now discard values that aren't present in available filter options + type: changed diff --git a/admin/roles/tests/reportbuilder/datasource/roles_test.php b/admin/roles/tests/reportbuilder/datasource/roles_test.php index d327e4c9762..06422006d59 100644 --- a/admin/roles/tests/reportbuilder/datasource/roles_test.php +++ b/admin/roles/tests/reportbuilder/datasource/roles_test.php @@ -149,7 +149,7 @@ final class roles_test extends core_reportbuilder_testcase { ], true], 'Filter role name (no match)' => ['role:name', [ 'role:name_operator' => select::EQUAL_TO, - 'role:name_value' => -1, + 'role:name_value' => $DB->get_field('role', 'id', ['shortname' => 'teacher']), ], false], 'Filter role archetype' => ['role:archetype', [ 'role:archetype_operator' => select::EQUAL_TO, diff --git a/cohort/tests/reportbuilder/datasource/cohorts_test.php b/cohort/tests/reportbuilder/datasource/cohorts_test.php index d3a8e4e8b17..b9c202b91cb 100644 --- a/cohort/tests/reportbuilder/datasource/cohorts_test.php +++ b/cohort/tests/reportbuilder/datasource/cohorts_test.php @@ -149,9 +149,9 @@ final class cohorts_test extends core_reportbuilder_testcase { 'cohort:context_operator' => select::EQUAL_TO, 'cohort:context_value' => system::instance()->id, ], true], - 'Filter content (no match)' => ['cohort:context', [ - 'cohort:context_operator' => select::EQUAL_TO, - 'cohort:context_value' => -1, + 'Filter context (no match)' => ['cohort:context', [ + 'cohort:context_operator' => select::NOT_EQUAL_TO, + 'cohort:context_value' => system::instance()->id, ], false], 'Filter name' => ['cohort:name', [ 'cohort:name_operator' => text::IS_EQUAL_TO, diff --git a/competency/tests/reportbuilder/datasource/competencies_test.php b/competency/tests/reportbuilder/datasource/competencies_test.php index ccd3aa9d9f0..0adb9a56636 100644 --- a/competency/tests/reportbuilder/datasource/competencies_test.php +++ b/competency/tests/reportbuilder/datasource/competencies_test.php @@ -187,8 +187,8 @@ final class competencies_test extends core_reportbuilder_testcase { 'framework:scale_value' => '', ], true], 'Framework scale (no match)' => ['framework:scale', [ - 'framework:scale_operator' => select::EQUAL_TO, - 'framework:scale_value' => -1, + 'framework:scale_operator' => select::NOT_EQUAL_TO, + 'framework:scale_value' => '', ], false], 'Framework visible' => ['framework:visible', [ 'framework:visible_operator' => boolean_select::CHECKED, diff --git a/course/tests/reportbuilder/datasource/categories_test.php b/course/tests/reportbuilder/datasource/categories_test.php index 2d57dc84699..996b07283b4 100644 --- a/course/tests/reportbuilder/datasource/categories_test.php +++ b/course/tests/reportbuilder/datasource/categories_test.php @@ -194,7 +194,7 @@ final class categories_test extends core_reportbuilder_testcase { ], true], 'Filter role (no match)' => ['role:name', [ 'role:name_operator' => select::EQUAL_TO, - 'role:name_value' => -1, + 'role:name_value' => $DB->get_field('role', 'id', ['shortname' => 'teacher']), ], false], // User. diff --git a/course/tests/reportbuilder/datasource/courses_test.php b/course/tests/reportbuilder/datasource/courses_test.php index 1a6ea203313..96ec7d00461 100644 --- a/course/tests/reportbuilder/datasource/courses_test.php +++ b/course/tests/reportbuilder/datasource/courses_test.php @@ -82,6 +82,7 @@ final class courses_test extends core_reportbuilder_testcase { 'category' => $category->id, 'fullname' => 'Cats', 'summary' => 'Course description', + 'lang' => 'en', 'theme' => 'boost', 'tags' => ['Horses'], ]); @@ -161,8 +162,8 @@ final class courses_test extends core_reportbuilder_testcase { $this->assertEquals('Yes', $coursevisible); $this->assertEquals('No groups', $coursegroupmode); $this->assertEquals('No', $coursegroupmodeforce); - $this->assertEmpty($courselang); - $this->assertEmpty($coursecalendar); + $this->assertEquals('English ‎(en)‎', $courselang); + $this->assertEquals('Do not force', $coursecalendar); $this->assertEquals('Boost', $coursetheme); $this->assertEquals('No', $coursecompletion); $this->assertEmpty($coursedownload); @@ -273,7 +274,7 @@ final class courses_test extends core_reportbuilder_testcase { ], true], 'Filter course format (no match)' => ['course:format', [ 'course:format_operator' => select::EQUAL_TO, - 'course:format_value' => 'weekly', + 'course:format_value' => 'weeks', ], false], 'Filter course startdate' => ['course:startdate', [ 'course:startdate_operator' => date::DATE_RANGE, @@ -315,7 +316,7 @@ final class courses_test extends core_reportbuilder_testcase { ], true], 'Filter course lang (no match)' => ['course:lang', [ 'course:lang_operator' => select::EQUAL_TO, - 'course:lang_value' => 'de', + 'course:lang_value' => '', ], false], 'Filter course calendartype' => ['course:calendartype', [ 'course:calendartype_operator' => select::EQUAL_TO, @@ -323,7 +324,7 @@ final class courses_test extends core_reportbuilder_testcase { ], true], 'Filter course calendartype (no match)' => ['course:calendartype', [ 'course:calendartype_operator' => select::EQUAL_TO, - 'course:calendartype_value' => 'hijri', + 'course:calendartype_value' => '', ], false], 'Filter course theme' => ['course:theme', [ 'course:theme_operator' => select::EQUAL_TO, diff --git a/reportbuilder/classes/local/entities/course.php b/reportbuilder/classes/local/entities/course.php index a21ea0a51f4..df658c44c50 100644 --- a/reportbuilder/classes/local/entities/course.php +++ b/reportbuilder/classes/local/entities/course.php @@ -409,7 +409,7 @@ class course extends base { * @return array */ public static function get_options_for_theme(): array { - return array_map( + return ['' => get_string('forceno')] + array_map( fn(theme_config $theme) => $theme->get_theme_name(), get_list_of_themes(), ); @@ -421,7 +421,7 @@ class course extends base { * @return array */ public static function get_options_for_lang(): array { - return get_string_manager()->get_list_of_translations(); + return ['' => get_string('forceno')] + get_string_manager()->get_list_of_translations(); } /** @@ -430,7 +430,7 @@ class course extends base { * @return array */ public static function get_options_for_calendartype(): array { - return \core_calendar\type_factory::get_list_of_calendar_types(); + return ['' => get_string('forceno')] + \core_calendar\type_factory::get_list_of_calendar_types(); } /** @@ -452,7 +452,7 @@ class course extends base { // If the column has corresponding filter, determine the value from its options. $options = $this->get_options_for($fieldname); - if ($options !== null && array_key_exists($value, $options)) { + if ($options !== null && $value !== null && array_key_exists($value, $options)) { return $options[$value]; } diff --git a/reportbuilder/classes/local/entities/user.php b/reportbuilder/classes/local/entities/user.php index 7e35f4ea5a0..5db1401830f 100644 --- a/reportbuilder/classes/local/entities/user.php +++ b/reportbuilder/classes/local/entities/user.php @@ -364,7 +364,7 @@ class user extends base { // If the column has corresponding filter, determine the value from its options. $options = $this->get_options_for($fieldname); - if ($options !== null && array_key_exists($value, $options)) { + if ($options !== null && $value !== null && array_key_exists($value, $options)) { return $options[$value]; } diff --git a/reportbuilder/classes/local/filters/select.php b/reportbuilder/classes/local/filters/select.php index 5ecb3f0aa88..c42d4be60fd 100644 --- a/reportbuilder/classes/local/filters/select.php +++ b/reportbuilder/classes/local/filters/select.php @@ -44,6 +44,9 @@ class select extends base { /** @var int Not equal to */ public const NOT_EQUAL_TO = 2; + /** @var int Value to indicate "Any value" for the simplified filter options */ + private const OPTION_ANY_VALUE = -124567; + /** * Returns an array of comparison operators * @@ -65,7 +68,13 @@ class select extends base { * @return array */ protected function get_select_options(): array { - return (array) $this->filter->get_options(); + static $options = []; + + if (!array_key_exists($this->name, $options)) { + $options[$this->name] = (array) $this->filter->get_options(); + } + + return $options[$this->name]; } /** @@ -91,7 +100,7 @@ class select extends base { $element, "{$this->name}_value", get_string('filterfieldvalue', 'core_reportbuilder', $this->get_header()), - ['' => $operators[self::ANY_VALUE]] + $options, + [self::OPTION_ANY_VALUE => $operators[self::ANY_VALUE]] + $options, )->setHiddenLabel(true); } else { $elements = []; @@ -129,13 +138,19 @@ class select extends base { $name = database::generate_param_name(); $operator = (int) ($values["{$this->name}_operator"] ?? self::ANY_VALUE); - $value = $values["{$this->name}_value"] ?? ''; + $value = (string) ($values["{$this->name}_value"] ?? self::OPTION_ANY_VALUE); $fieldsql = $this->filter->get_field_sql(); $params = $this->filter->get_field_params(); + // Get available options, if multidimensional then flatten the array. + $options = $this->get_select_options(); + if (count($options) !== count($options, COUNT_RECURSIVE)) { + $options = array_merge(...array_values($options)); + } + // Validate filter form values. - if ($value === '') { + if ($operator === self::ANY_VALUE || !array_key_exists($value, $options)) { return ['', []]; } diff --git a/reportbuilder/tests/local/filters/select_test.php b/reportbuilder/tests/local/filters/select_test.php index 6bbbc6b9a42..ed34079b1f7 100644 --- a/reportbuilder/tests/local/filters/select_test.php +++ b/reportbuilder/tests/local/filters/select_test.php @@ -43,8 +43,12 @@ final class select_test extends advanced_testcase { [select::ANY_VALUE, null, true], [select::EQUAL_TO, 'starwars', true], [select::EQUAL_TO, 'mandalorian', false], + [select::EQUAL_TO, '', false], + [select::EQUAL_TO, 'invalid', true], [select::NOT_EQUAL_TO, 'starwars', false], [select::NOT_EQUAL_TO, 'mandalorian', true], + [select::NOT_EQUAL_TO, '', true], + [select::NOT_EQUAL_TO, 'invalid', true], ]; } @@ -64,11 +68,11 @@ final class select_test extends advanced_testcase { $course1 = $this->getDataGenerator()->create_course([ 'fullname' => "May the course be with you", - 'shortname' => 'starwars', + 'idnumber' => 'starwars', ]); $course2 = $this->getDataGenerator()->create_course([ 'fullname' => "This is the course", - 'shortname' => 'mandalorian', + 'idnumber' => '', ]); $filter = (new filter( @@ -76,10 +80,11 @@ final class select_test extends advanced_testcase { 'test', new lang_string('course'), 'testentity', - 'shortname' + 'idnumber' ))->set_options([ - $course1->shortname => $course1->fullname, - $course2->shortname => $course2->fullname, + $course1->idnumber => $course1->fullname, + $course2->idnumber => $course2->fullname, + 'mandalorian' => 'This is not the course you are looking for', ]); // Create instance of our filter, passing given operator. diff --git a/reportbuilder/tests/local/helpers/custom_fields_test.php b/reportbuilder/tests/local/helpers/custom_fields_test.php index 1be2aee68ec..c5c5258603d 100644 --- a/reportbuilder/tests/local/helpers/custom_fields_test.php +++ b/reportbuilder/tests/local/helpers/custom_fields_test.php @@ -70,7 +70,7 @@ final class custom_fields_test extends core_reportbuilder_testcase { $generator->create_field( ['categoryid' => $category->get('id'), 'type' => 'select', 'name' => 'Select', 'shortname' => 'select', - 'configdata' => ['options' => "Cat\nDog", 'defaultvalue' => 'Cat']]); + 'configdata' => ['options' => "Cat\nDog\nFish", 'defaultvalue' => 'Cat']]); $generator->create_field( ['categoryid' => $category->get('id'), 'type' => 'number', 'name' => 'Number', 'shortname' => 'number', diff --git a/reportbuilder/tests/local/helpers/user_profile_fields_test.php b/reportbuilder/tests/local/helpers/user_profile_fields_test.php index 2060320e2e5..8986bea2b87 100644 --- a/reportbuilder/tests/local/helpers/user_profile_fields_test.php +++ b/reportbuilder/tests/local/helpers/user_profile_fields_test.php @@ -50,7 +50,8 @@ final class user_profile_fields_test extends core_reportbuilder_testcase { 'defaultdata' => 0, 'visible' => PROFILE_VISIBLE_NONE]); $this->getDataGenerator()->create_custom_profile_field([ - 'shortname' => 'menu', 'name' => 'Menu field', 'datatype' => 'menu', 'param1' => "Cat\nDog", 'defaultdata' => 'Cat']); + 'shortname' => 'menu', 'name' => 'Menu field', 'datatype' => 'menu', 'param1' => "Cat\nDog\nFish", + 'defaultdata' => 'Cat']); $this->getDataGenerator()->create_custom_profile_field([ 'shortname' => 'Social', 'name' => 'msn', 'datatype' => 'social', 'param1' => 'msn']); diff --git a/user/tests/reportbuilder/datasource/users_test.php b/user/tests/reportbuilder/datasource/users_test.php index 6a9cd0ec053..0c2a38820c2 100644 --- a/user/tests/reportbuilder/datasource/users_test.php +++ b/user/tests/reportbuilder/datasource/users_test.php @@ -386,8 +386,8 @@ final class users_test extends core_reportbuilder_testcase { 'user:lang_value' => 'en', ], true], 'Filter lang (no match)' => ['user:lang', [ - 'user:lang_operator' => select::EQUAL_TO, - 'user:lang_value' => 'de', + 'user:lang_operator' => select::NOT_EQUAL_TO, + 'user:lang_value' => 'en', ], false], 'Filter timezone' => ['user:timezone', [ 'user:timezone_operator' => select::EQUAL_TO, -- 2.43.0