L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

MDL-73493 reportbuilder: correct handling of empty string in filters.

Browse Source 
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 <carlos.castillo@moodle.com>
This commit is contained in:
Paul Holden
2022-01-21 11:27:28 +00:00
parent 3d8620854a
commit 628541b5e1
4 changed files with 136 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
reportbuilder/classes/local/filters/text.php
+7 -5
View File
@@ -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.
reportbuilder/classes/local/helpers/user_profile_fields.php
+16 -5
View File
@@ -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} " .
reportbuilder/tests/local/entities/user_test.php
+57 -2
View File
@@ -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}
*
reportbuilder/tests/local/filters/text_test.php
+56 -2
View File
@@ -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);
}
}
}