From 2ffda63da8c37e5755026f1672bc12a1a90bc60a Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Wed, 25 Jan 2023 12:21:53 +0000 Subject: [PATCH] MDL-75359 reportbuilder: fixes for custom/user profile field elements. Ensures cross-DB compatibility for all types of both custom and user profile fields, specifically when they're filtered and/or aggregated. Implement stress testing of both via appropriate report sources. --- .../reportbuilder/datasource/courses_test.php | 8 +-- .../classes/local/helpers/custom_fields.php | 50 ++++++++-------- .../local/helpers/user_profile_fields.php | 38 +++++------- .../local/helpers/custom_fields_test.php | 59 ++++++++++++++++--- .../helpers/user_profile_fields_test.php | 27 +++++++++ .../reportbuilder/datasource/users_test.php | 3 +- 6 files changed, 121 insertions(+), 64 deletions(-) diff --git a/course/tests/reportbuilder/datasource/courses_test.php b/course/tests/reportbuilder/datasource/courses_test.php index f1a5758c2dd..a089db49117 100644 --- a/course/tests/reportbuilder/datasource/courses_test.php +++ b/course/tests/reportbuilder/datasource/courses_test.php @@ -19,7 +19,6 @@ declare(strict_types=1); namespace core_course\reportbuilder\datasource; use context_course; -use core_customfield_generator; use core_reportbuilder_testcase; use core_reportbuilder_generator; use core_reportbuilder\local\filters\boolean_select; @@ -426,13 +425,8 @@ class courses_test extends core_reportbuilder_testcase { $this->resetAfterTest(); - /** @var core_customfield_generator $generator */ - $generator = $this->getDataGenerator()->get_plugin_generator('core_customfield'); - $customfieldcategory = $generator->create_category(); - $generator->create_field(['categoryid' => $customfieldcategory->get('id'), 'shortname' => 'hi']); - $category = $this->getDataGenerator()->create_category(); - $course = $this->getDataGenerator()->create_course(['category' => $category->id, 'customfield_hi' => 'Hello']); + $course = $this->getDataGenerator()->create_course(['category' => $category->id]); $this->datasource_stress_test_columns(courses::class); $this->datasource_stress_test_columns_aggregation(courses::class); diff --git a/reportbuilder/classes/local/helpers/custom_fields.php b/reportbuilder/classes/local/helpers/custom_fields.php index 12079461cc1..616d114f80a 100644 --- a/reportbuilder/classes/local/helpers/custom_fields.php +++ b/reportbuilder/classes/local/helpers/custom_fields.php @@ -142,6 +142,8 @@ class custom_fields { * @return column[] */ public function get_columns(): array { + global $DB; + $columns = []; $categorieswithfields = $this->handler->get_categories_with_fields(); @@ -151,34 +153,40 @@ class custom_fields { $customdatatablealias = $this->get_table_alias($field); $datacontroller = data_controller::create(0, null, $field); + $datafield = $datacontroller->datafield(); + $datafieldsql = "{$customdatatablealias}.{$datafield}"; + + // Long text fields should be cast for Oracle, for aggregation support. + $columntype = $this->get_column_type($field, $datafield); + if ($columntype === column::TYPE_LONGTEXT && $DB->get_dbfamily() === 'oracle') { + $datafieldsql = $DB->sql_order_by_text($datafieldsql, 1024); + } // Select enough fields to re-create and format each custom field instance value. - $selectfields = "{$customdatatablealias}.{$datafield}, {$customdatatablealias}.id, - {$customdatatablealias}.contextid"; + $selectfields = "{$customdatatablealias}.id, {$customdatatablealias}.contextid"; if ($datafield === 'value') { // We will take the format into account when displaying the individual values. $selectfields .= ", {$customdatatablealias}.valueformat"; } - $columntype = $this->get_column_type($field, $datafield); - - $newcolumn = (new column( + $columns[] = (new column( 'customfield_' . $field->get('shortname'), new lang_string('customfieldcolumn', 'core_reportbuilder', $field->get_formatted_name()), $this->entityname )) ->add_joins($this->get_joins()) ->add_join($this->get_table_join($field)) + ->add_field($datafieldsql, $datafield) ->add_fields($selectfields) ->set_type($columntype) ->set_is_sortable($columntype !== column::TYPE_LONGTEXT) - ->add_callback([$this, 'customfield_value'], $field) + ->add_callback(static function($value, stdClass $row, field_controller $field): string { + return (string) data_controller::create(0, $row, $field)->export_value(); + }, $field) // Important. If the handler implements can_view() function, it will be called with parameter $instanceid=0. // This means that per-instance access validation will be ignored. ->set_is_available($this->handler->can_view($field, 0)); - - $columns[] = $newcolumn; } } return $columns; @@ -223,6 +231,8 @@ class custom_fields { * @return filter[] */ public function get_filters(): array { + global $DB; + $filters = []; $categorieswithfields = $this->handler->get_categories_with_fields(); @@ -232,15 +242,20 @@ class custom_fields { $customdatatablealias = $this->get_table_alias($field); $datacontroller = data_controller::create(0, null, $field); - $datafield = $datacontroller->datafield(); - $typeclass = $this->get_filter_class_type($datacontroller); + $datafield = $datacontroller->datafield(); + $datafieldsql = "{$customdatatablealias}.{$datafield}"; + if ($datafield === 'value') { + $datafieldsql = $DB->sql_cast_to_char($datafieldsql); + } + + $typeclass = $this->get_filter_class_type($datacontroller); $filter = (new filter( $typeclass, 'customfield_' . $field->get('shortname'), new lang_string('customfieldcolumn', 'core_reportbuilder', $field->get_formatted_name()), $this->entityname, - "{$customdatatablealias}.{$datafield}" + $datafieldsql )) ->add_joins($this->get_joins()) ->add_join($this->get_table_join($field)); @@ -298,17 +313,4 @@ class custom_fields { return $classtype; } - - /** - * Format for custom fields value. We get the correct custom field value using export_value method. - * - * @param mixed $value Current value. - * @param stdClass $row Full row. - * @param field_controller $field Field controller object. - * @return mixed|null - */ - public function customfield_value($value, stdClass $row, field_controller $field) { - $data = data_controller::create(0, (object)$row, $field); - return $data->export_value(); - } } diff --git a/reportbuilder/classes/local/helpers/user_profile_fields.php b/reportbuilder/classes/local/helpers/user_profile_fields.php index c6160ff4ba7..6e5d419fd1d 100644 --- a/reportbuilder/classes/local/helpers/user_profile_fields.php +++ b/reportbuilder/classes/local/helpers/user_profile_fields.php @@ -157,13 +157,17 @@ class user_profile_fields { $columns = []; foreach ($this->userprofilefields as $profilefield) { $columntype = $this->get_user_field_type($profilefield->field->datatype); - $columnfieldsql = $this->get_table_alias($profilefield) . '.data'; - if ($DB->get_dbfamily() === 'oracle') { + + // Numeric (checkbox/time) fields should be cast, as should all fields for Oracle, for aggregation support. + if ($columntype === column::TYPE_BOOLEAN || $columntype === column::TYPE_TIMESTAMP) { + $columnfieldsql = "CASE WHEN {$columnfieldsql} IS NULL THEN NULL ELSE " . + $DB->sql_cast_char2int($columnfieldsql, true) . " END"; + } else if ($DB->get_dbfamily() === 'oracle') { $columnfieldsql = $DB->sql_order_by_text($columnfieldsql, 1024); } - $column = (new column( + $columns[] = (new column( 'profilefield_' . core_text::strtolower($profilefield->field->shortname), new lang_string('customfieldcolumn', 'core_reportbuilder', format_string($profilefield->field->name, true, @@ -175,9 +179,15 @@ class user_profile_fields { ->add_field($columnfieldsql, 'data') ->set_type($columntype) ->set_is_sortable($columntype !== column::TYPE_LONGTEXT) - ->add_callback([$this, 'format_profile_field'], $profilefield); + ->add_callback(static function($value, stdClass $row, profile_field_base $field): string { + // Special handling of checkboxes, we want to display their boolean state rather than the input element itself. + if (is_a($field, 'profile_field_checkbox')) { + return format::boolean_as_text($value); + } - $columns[] = $column; + $field->data = $value; + return (string) $field->display_data(); + }, $profilefield); } return $columns; @@ -274,22 +284,4 @@ class user_profile_fields { } return $customfieldtype; } - - /** - * Formatter for a profile field. It formats the field according to its type. - * - * @param mixed $value - * @param stdClass $row - * @param profile_field_base $field - * @return string - */ - public static function format_profile_field($value, stdClass $row, profile_field_base $field): string { - // Special handling of checkboxes, we want to display their boolean state rather than the input element itself. - if (is_a($field, 'profile_field_checkbox')) { - return format::boolean_as_text($value); - } - - $field->data = $value; - return (string) $field->display_data(); - } } diff --git a/reportbuilder/tests/local/helpers/custom_fields_test.php b/reportbuilder/tests/local/helpers/custom_fields_test.php index d4c4f904b96..ae38b8ddb59 100644 --- a/reportbuilder/tests/local/helpers/custom_fields_test.php +++ b/reportbuilder/tests/local/helpers/custom_fields_test.php @@ -64,6 +64,9 @@ class custom_fields_test extends core_reportbuilder_testcase { $generator->create_field( ['categoryid' => $category->get('id'), 'type' => 'text', 'name' => 'Text', 'shortname' => 'text']); + $generator->create_field( + ['categoryid' => $category->get('id'), 'type' => 'textarea', 'name' => 'Textarea', 'shortname' => 'textarea']); + $generator->create_field( ['categoryid' => $category->get('id'), 'type' => 'checkbox', 'name' => 'Checkbox', 'shortname' => 'checkbox']); @@ -91,12 +94,13 @@ class custom_fields_test extends core_reportbuilder_testcase { $customfields = $this->generate_customfields(); $columns = $customfields->get_columns(); - $this->assertCount(4, $columns); + $this->assertCount(5, $columns); $this->assertContainsOnlyInstancesOf(column::class, $columns); - [$column0, $column1, $column2, $column3] = $columns; - $this->assertEqualsCanonicalizing(['Text', 'Checkbox', 'Date', 'Select'], - [$column0->get_title(), $column1->get_title(), $column2->get_title(), $column3->get_title()]); + [$column0, $column1, $column2, $column3, $column4] = $columns; + $this->assertEqualsCanonicalizing(['Text', 'Textarea', 'Checkbox', 'Date', 'Select'], [ + $column0->get_title(), $column1->get_title(), $column2->get_title(), $column3->get_title(), $column4->get_title() + ]); $this->assertEquals(column::TYPE_TEXT, $column0->get_type()); $this->assertEquals('course', $column0->get_entity_name()); @@ -144,12 +148,13 @@ class custom_fields_test extends core_reportbuilder_testcase { $customfields = $this->generate_customfields(); $filters = $customfields->get_filters(); - $this->assertCount(4, $filters); + $this->assertCount(5, $filters); $this->assertContainsOnlyInstancesOf(filter::class, $filters); - [$filter0, $filter1, $filter2, $filter3] = $filters; - $this->assertEqualsCanonicalizing(['Text', 'Checkbox', 'Date', 'Select'], - [$filter0->get_header(), $filter1->get_header(), $filter2->get_header(), $filter3->get_header()]); + [$filter0, $filter1, $filter2, $filter3, $filter4] = $filters; + $this->assertEqualsCanonicalizing(['Text', 'Textarea', 'Checkbox', 'Date', 'Select'], [ + $filter0->get_header(), $filter1->get_header(), $filter2->get_header(), $filter3->get_header(), $filter4->get_header() + ]); } /** @@ -162,6 +167,7 @@ class custom_fields_test extends core_reportbuilder_testcase { $course = $this->getDataGenerator()->create_course(['customfields' => [ ['shortname' => 'text', 'value' => 'Hello'], + ['shortname' => 'textarea_editor', 'value' => ['text' => 'Goodbye', 'format' => FORMAT_MOODLE]], ['shortname' => 'checkbox', 'value' => true], ['shortname' => 'date', 'value' => 1669852800], ['shortname' => 'select', 'value' => 2], @@ -174,6 +180,7 @@ class custom_fields_test extends core_reportbuilder_testcase { // Add user profile field columns to the report. $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'course:fullname']); $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'course:customfield_text']); + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'course:customfield_textarea']); $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'course:customfield_checkbox']); $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'course:customfield_date']); $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'course:customfield_select']); @@ -183,6 +190,7 @@ class custom_fields_test extends core_reportbuilder_testcase { $this->assertEquals([ $course->fullname, 'Hello', + '
Goodbye
', 'Yes', userdate(1669852800), 'Dog' @@ -204,6 +212,14 @@ class custom_fields_test extends core_reportbuilder_testcase { 'course:customfield_text_operator' => text::IS_EQUAL_TO, 'course:customfield_text_value' => 'Goodbye', ], false], + 'Filter by textarea custom field' => ['course:customfield_textarea', [ + 'course:customfield_textarea_operator' => text::IS_EQUAL_TO, + 'course:customfield_textarea_value' => 'Goodbye', + ], true], + 'Filter by textarea custom field (no match)' => ['course:customfield_textarea', [ + 'course:customfield_textarea_operator' => text::IS_EQUAL_TO, + 'course:customfield_textarea_value' => 'Hello', + ], false], 'Filter by checkbox custom field' => ['course:customfield_checkbox', [ 'course:customfield_checkbox_operator' => boolean_select::CHECKED, ], true], @@ -245,6 +261,7 @@ class custom_fields_test extends core_reportbuilder_testcase { $course = $this->getDataGenerator()->create_course(['customfields' => [ ['shortname' => 'text', 'value' => 'Hello'], + ['shortname' => 'textarea_editor', 'value' => ['text' => 'Goodbye', 'format' => FORMAT_MOODLE]], ['shortname' => 'checkbox', 'value' => true], ['shortname' => 'date', 'value' => 1669852800], ['shortname' => 'select', 'value' => 2], @@ -268,5 +285,31 @@ class custom_fields_test extends core_reportbuilder_testcase { $this->assertEmpty($content); } } + + /** + * Stress test course datasource using custom fields + * + * In order to execute this test PHPUNIT_LONGTEST should be defined as true in phpunit.xml or directly in config.php + */ + public function test_stress_datasource(): void { + if (!PHPUNIT_LONGTEST) { + $this->markTestSkipped('PHPUNIT_LONGTEST is not defined'); + } + + $this->resetAfterTest(); + + $this->generate_customfields(); + $course = $this->getDataGenerator()->create_course(['customfields' => [ + ['shortname' => 'text', 'value' => 'Hello'], + ['shortname' => 'textarea_editor', 'value' => ['text' => 'Goodbye', 'format' => FORMAT_MOODLE]], + ['shortname' => 'checkbox', 'value' => true], + ['shortname' => 'date', 'value' => 1669852800], + ['shortname' => 'select', 'value' => 2], + ]]); + + $this->datasource_stress_test_columns(courses::class); + $this->datasource_stress_test_columns_aggregation(courses::class); + $this->datasource_stress_test_conditions(courses::class, 'course:idnumber'); + } } diff --git a/reportbuilder/tests/local/helpers/user_profile_fields_test.php b/reportbuilder/tests/local/helpers/user_profile_fields_test.php index 9ec960ca5a9..44599cd499c 100644 --- a/reportbuilder/tests/local/helpers/user_profile_fields_test.php +++ b/reportbuilder/tests/local/helpers/user_profile_fields_test.php @@ -350,4 +350,31 @@ class user_profile_fields_test extends core_reportbuilder_testcase { $this->assertCount(1, $content); $this->assertEquals($expectmatchuser, reset($content[0])); } + + /** + * Stress test user datasource using profile fields + * + * In order to execute this test PHPUNIT_LONGTEST should be defined as true in phpunit.xml or directly in config.php + */ + public function test_stress_datasource(): void { + if (!PHPUNIT_LONGTEST) { + $this->markTestSkipped('PHPUNIT_LONGTEST is not defined'); + } + + $this->resetAfterTest(); + + $userprofilefields = $this->generate_userprofilefields(); + $user = $this->getDataGenerator()->create_user([ + 'profile_field_checkbox' => true, + 'profile_field_datetime' => '2021-12-09', + 'profile_field_menu' => 'Dog', + 'profile_field_Social' => '12345', + 'profile_field_text' => 'Hello', + 'profile_field_textarea' => 'Goodbye', + ]); + + $this->datasource_stress_test_columns(users::class); + $this->datasource_stress_test_columns_aggregation(users::class); + $this->datasource_stress_test_conditions(users::class, 'user:idnumber'); + } } diff --git a/user/tests/reportbuilder/datasource/users_test.php b/user/tests/reportbuilder/datasource/users_test.php index dc75418bbbe..774894e3986 100644 --- a/user/tests/reportbuilder/datasource/users_test.php +++ b/user/tests/reportbuilder/datasource/users_test.php @@ -441,8 +441,7 @@ class users_test extends core_reportbuilder_testcase { $this->resetAfterTest(); - $this->getDataGenerator()->create_custom_profile_field(['datatype' => 'text', 'name' => 'Hi', 'shortname' => 'hi']); - $user = $this->getDataGenerator()->create_user(['profile_field_hi' => 'Hello']); + $user = $this->getDataGenerator()->create_user(); $this->datasource_stress_test_columns(users::class); $this->datasource_stress_test_columns_aggregation(users::class);