From 628541b5e17dec30a1794eb7666347d785df3bb3 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Fri, 21 Jan 2022 11:27:28 +0000 Subject: [PATCH] MDL-73493 reportbuilder: correct handling of empty string in filters. This is specifically for Oracle, which treats empty strings and NULL in an inconsistent manner unless passed as query parameters. Increase test coverage of the same. Co-authored-by: Carlos Castillo --- reportbuilder/classes/local/filters/text.php | 12 ++-- .../local/helpers/user_profile_fields.php | 21 +++++-- .../tests/local/entities/user_test.php | 59 ++++++++++++++++++- .../tests/local/filters/text_test.php | 58 +++++++++++++++++- 4 files changed, 136 insertions(+), 14 deletions(-) diff --git a/reportbuilder/classes/local/filters/text.php b/reportbuilder/classes/local/filters/text.php index b00db726625..d41e7995a36 100644 --- a/reportbuilder/classes/local/filters/text.php +++ b/reportbuilder/classes/local/filters/text.php @@ -155,13 +155,15 @@ class text extends base { $value = $DB->sql_like_escape($value); $params[$name] = "%$value"; break; - case self::IS_EMPTY: // Note we also account for field not existing here. - $res = "COALESCE({$fieldsql}, '') = :{$name}"; - $params[$name] = ''; + case self::IS_EMPTY: + $paramempty = database::generate_param_name(); + $res = "COALESCE({$fieldsql}, :{$paramempty}) = :{$name}"; + $params[$paramempty] = $params[$name] = ''; break; case self::IS_NOT_EMPTY: - $res = "COALESCE({$fieldsql}, '') != :{$name}"; - $params[$name] = ''; + $paramempty = database::generate_param_name(); + $res = "COALESCE({$fieldsql}, :{$paramempty}) != :{$name}"; + $params[$paramempty] = $params[$name] = ''; break; default: // Filter configuration is invalid. Ignore the filter. diff --git a/reportbuilder/classes/local/helpers/user_profile_fields.php b/reportbuilder/classes/local/helpers/user_profile_fields.php index 31d5b0d18c3..be1e04bb495 100644 --- a/reportbuilder/classes/local/helpers/user_profile_fields.php +++ b/reportbuilder/classes/local/helpers/user_profile_fields.php @@ -23,6 +23,7 @@ use core_reportbuilder\local\filters\boolean_select; use core_reportbuilder\local\filters\date; use core_reportbuilder\local\filters\select; use core_reportbuilder\local\filters\text; +use core_reportbuilder\local\helpers\database; use core_reportbuilder\local\report\column; use core_reportbuilder\local\report\filter; use lang_string; @@ -155,25 +156,34 @@ class user_profile_fields { foreach ($this->userprofilefields as $profilefield) { $userinfotablealias = database::generate_alias(); $field = "{$userinfotablealias}.data"; + $params = []; switch ($profilefield->field->datatype) { case 'checkbox': $classname = boolean_select::class; - $field = $DB->sql_cast_char2int("COALESCE({$field}, 0)"); + $fieldsql = "COALESCE(" . $DB->sql_cast_char2int($field, true) . ", 0)"; break; case 'datetime': $classname = date::class; - $field = $DB->sql_cast_char2int($field); + $fieldsql = $DB->sql_cast_char2int($field); break; case 'menu': $classname = select::class; - $field = "COALESCE({$field}, '')"; + + $emptyparam = database::generate_param_name(); + $fieldsql = "COALESCE(" . $DB->sql_compare_text($field, 255) . ", :{$emptyparam})"; + $params[$emptyparam] = ''; + break; case 'text': case 'textarea': default: - $field = $DB->sql_compare_text("COALESCE({$field}, '')", 255); $classname = text::class; + + $emptyparam = database::generate_param_name(); + $fieldsql = "COALESCE(" . $DB->sql_compare_text($field, 255) . ", :{$emptyparam})"; + $params[$emptyparam] = ''; + break; } @@ -184,7 +194,8 @@ class user_profile_fields { format_string($profilefield->field->name, true, ['escape' => false, 'context' => context_system::instance()])), $this->entityname, - $field + $fieldsql, + $params )) ->add_joins($this->get_joins()) ->add_join("LEFT JOIN {user_info_data} {$userinfotablealias} " . diff --git a/reportbuilder/tests/local/entities/user_test.php b/reportbuilder/tests/local/entities/user_test.php index 122e9189883..879b65891e9 100644 --- a/reportbuilder/tests/local/entities/user_test.php +++ b/reportbuilder/tests/local/entities/user_test.php @@ -19,12 +19,13 @@ declare(strict_types=1); namespace core_reportbuilder\local\entities; use advanced_testcase; -use core_reportbuilder\local\filters\boolean_select; -use core_reportbuilder\local\filters\date; use moodle_url; use core_reportbuilder\manager; use core_reportbuilder\testable_system_report_table; use core_reportbuilder\user_entity_report; +use core_reportbuilder\local\filters\boolean_select; +use core_reportbuilder\local\filters\date; +use core_reportbuilder\local\filters\select; use core_reportbuilder\local\filters\text; use core_reportbuilder\local\helpers\user_filter_manager; @@ -251,6 +252,60 @@ class user_test extends advanced_testcase { ], array_column($tablerows, 'fullname')); } + /** + * Data provider for {@see test_userprofilefield_filter_empty} + * + * @return array + */ + public function userprofilefield_filter_empty_provider(): array { + return [ + ['checkbox', 1, boolean_select::NOT_CHECKED], + ['text', 'Hello', text::IS_EMPTY], + ['text', 'Hello', text::IS_NOT_EQUAL_TO], + ['text', 'Hello', text::DOES_NOT_CONTAIN], + ['menu', 'one', select::NOT_EQUAL_TO, "one\ntwo"], + ]; + } + + /** + * Test filtering report by a user profile field with negated operators (contains the "empty" value appropriate to the field + * type, or is not set/null) + * + * @param string $datatype + * @param mixed $userfieldvalue + * @param int $operator + * @param string $datatypeparam1 + * + * @dataProvider userprofilefield_filter_empty_provider + */ + public function test_userprofilefield_filter_empty(string $datatype, $userfieldvalue, int $operator, + string $datatypeparam1 = ''): void { + + $this->resetAfterTest(); + + $this->getDataGenerator()->create_custom_profile_field([ + 'datatype' => $datatype, + 'shortname' => 'test', + 'name' => 'My test field', + 'param1' => $datatypeparam1, + ]); + + // At this point, the custom profile field was created after the admin account, so the value will be not set/null. + $filtervalues = [ + 'user:profilefield_test_operator' => $operator, + 'user:profilefield_test_value' => $userfieldvalue, + ]; + + // Create a user who does have the field set. + $user = $this->getDataGenerator()->create_user(['profile_field_test' => $userfieldvalue]); + + $rows = $this->get_report_table_rows($filtervalues); + + $usernames = array_column($rows, 'username'); + $this->assertContains('admin', $usernames); + $this->assertNotContains($user->username, $usernames); + } + /** * Data provider for {@see test_get_name_fields_select} * diff --git a/reportbuilder/tests/local/filters/text_test.php b/reportbuilder/tests/local/filters/text_test.php index 2ea6e0c3723..9a9a5f0d017 100644 --- a/reportbuilder/tests/local/filters/text_test.php +++ b/reportbuilder/tests/local/filters/text_test.php @@ -53,8 +53,6 @@ class text_test extends advanced_testcase { [text::STARTS_WITH, 'sunlight', false], [text::ENDS_WITH, 'looking for?', true], [text::ENDS_WITH, 'your heart', false], - [text::IS_EMPTY, null, false], - [text::IS_NOT_EMPTY, null, true], ]; } @@ -97,4 +95,60 @@ class text_test extends advanced_testcase { $this->assertNotContains($course->fullname, $fullnames); } } + + /** + * Data provider for {@see test_get_sql_filter_empty} + * + * @return array + */ + public function get_sql_filter_empty_provider(): array { + return [ + [text::IS_EMPTY, null, true], + [text::IS_EMPTY, '', true], + [text::IS_EMPTY, 'hola', false], + [text::IS_NOT_EMPTY, null, false], + [text::IS_NOT_EMPTY, '', false], + [text::IS_NOT_EMPTY, 'hola', true], + ]; + } + + /** + * Test getting filter SQL using the {@see text::IS_EMPTY} and {@see text::IS_NOT_EMPTY} operators + * + * @param int $operator + * @param string|null $profilefieldvalue + * @param bool $expectmatch + * + * @dataProvider get_sql_filter_empty_provider + */ + public function test_get_sql_filter_empty(int $operator, ?string $profilefieldvalue, bool $expectmatch): void { + global $DB; + + $this->resetAfterTest(); + + // We are using the user.moodlenetprofile field because it is nullable. + $user = $this->getDataGenerator()->create_user([ + 'moodlenetprofile' => $profilefieldvalue, + ]); + + $filter = new filter( + text::class, + 'test', + new lang_string('user'), + 'testentity', + 'moodlenetprofile' + ); + + // Create instance of our filter, passing given operator. + [$select, $params] = text::create($filter)->get_sql_filter([ + $filter->get_unique_identifier() . '_operator' => $operator, + ]); + + $usernames = $DB->get_fieldset_select('user', 'username', $select, $params); + if ($expectmatch) { + $this->assertContains($user->username, $usernames); + } else { + $this->assertNotContains($user->username, $usernames); + } + } } -- 2.43.0