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,