From b014a4b944ad5c26ba0d5900b1a00800ef08d046 Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Wed, 14 Feb 2024 15:31:08 +0000 Subject: [PATCH] MDL-80943 reportbuilder: select-type customfields should be text. The column type determines the available aggregation options present for the column. Custom fields of type "select" should be considered as text fields for this purpose, because their stored value represents the index to their available options, rather than having any distinct meaning of it's own for display. --- lib/db/upgrade.php | 21 ++++++ .../classes/local/helpers/custom_fields.php | 4 ++ .../local/helpers/custom_fields_test.php | 64 +++++++++++++------ version.php | 2 +- 4 files changed, 70 insertions(+), 21 deletions(-) diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php index b5174c75ffd..d67dbaf6a81 100644 --- a/lib/db/upgrade.php +++ b/lib/db/upgrade.php @@ -3330,5 +3330,26 @@ privatefiles,moodle|/user/files.php'; upgrade_main_savepoint(true, 2023042405.06); } + if ($oldversion < 2023042406.05) { + + // Get all "select" custom field shortnames. + $fieldshortnames = $DB->get_fieldset_select('customfield_field', 'shortname', 'type = :type', ['type' => 'select']); + + // Ensure any used in custom reports columns are not using integer type aggregation. + foreach ($fieldshortnames as $fieldshortname) { + $DB->execute(" + UPDATE {reportbuilder_column} + SET aggregation = NULL + WHERE " . $DB->sql_like('uniqueidentifier', ':uniqueidentifier', false) . " + AND aggregation IN ('avg', 'max', 'min', 'sum') + ", [ + 'uniqueidentifier' => '%' . $DB->sql_like_escape(":customfield_{$fieldshortname}"), + ]); + } + + // Main savepoint reached. + upgrade_main_savepoint(true, 2023042406.05); + } + return true; } diff --git a/reportbuilder/classes/local/helpers/custom_fields.php b/reportbuilder/classes/local/helpers/custom_fields.php index 932fd55e149..16a0b8971a8 100644 --- a/reportbuilder/classes/local/helpers/custom_fields.php +++ b/reportbuilder/classes/local/helpers/custom_fields.php @@ -180,6 +180,10 @@ class custom_fields { return column::TYPE_TIMESTAMP; } + if ($field->get('type') === 'select') { + return column::TYPE_TEXT; + } + if ($datafield === 'intvalue') { return column::TYPE_INTEGER; } diff --git a/reportbuilder/tests/local/helpers/custom_fields_test.php b/reportbuilder/tests/local/helpers/custom_fields_test.php index 0266ebb6272..ad35136bad0 100644 --- a/reportbuilder/tests/local/helpers/custom_fields_test.php +++ b/reportbuilder/tests/local/helpers/custom_fields_test.php @@ -97,16 +97,23 @@ class custom_fields_test extends core_reportbuilder_testcase { $this->assertCount(5, $columns); $this->assertContainsOnlyInstancesOf(column::class, $columns); - [$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() - ]); + // Column titles. + $this->assertEquals( + ['Text', 'Textarea', 'Checkbox', 'Date', 'Select'], + array_map(fn(column $column) => $column->get_title(), $columns) + ); - $this->assertEquals(column::TYPE_TEXT, $column0->get_type()); - $this->assertEquals('course', $column0->get_entity_name()); - $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $column0->get_joins()[0]); - // Column of type TEXT is sortable. - $this->assertTrue($column0->get_is_sortable()); + // Column types. + $this->assertEquals( + [column::TYPE_TEXT, column::TYPE_LONGTEXT, column::TYPE_BOOLEAN, column::TYPE_TIMESTAMP, column::TYPE_TEXT], + array_map(fn(column $column) => $column->get_type(), $columns) + ); + + // Column sortable. + $this->assertEquals( + [true, false, true, true, true], + array_map(fn(column $column) => $column->get_is_sortable(), $columns) + ); } /** @@ -116,12 +123,23 @@ class custom_fields_test extends core_reportbuilder_testcase { $this->resetAfterTest(); $customfields = $this->generate_customfields(); - $columns = $customfields->get_columns(); - $this->assertCount(1, ($columns[0])->get_joins()); - $customfields->add_join('JOIN {test} t ON t.id = id'); + // By default, we always join on the customfield data table. $columns = $customfields->get_columns(); - $this->assertCount(2, ($columns[0])->get_joins()); + $joins = $columns[0]->get_joins(); + + $this->assertCount(1, $joins); + $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $joins[0]); + + // Add additional join. + $customfields->add_join('JOIN {test} t ON t.id = id'); + + $columns = $customfields->get_columns(); + $joins = $columns[0]->get_joins(); + + $this->assertCount(2, $joins); + $this->assertEquals('JOIN {test} t ON t.id = id', $joins[0]); + $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $joins[1]); } /** @@ -131,12 +149,17 @@ class custom_fields_test extends core_reportbuilder_testcase { $this->resetAfterTest(); $customfields = $this->generate_customfields(); - $columns = $customfields->get_columns(); - $this->assertCount(1, ($columns[0])->get_joins()); + // Add additional joins. $customfields->add_joins(['JOIN {test} t ON t.id = id', 'JOIN {test2} t2 ON t2.id = id']); + $columns = $customfields->get_columns(); - $this->assertCount(3, ($columns[0])->get_joins()); + $joins = $columns[0]->get_joins(); + + $this->assertCount(3, $joins); + $this->assertEquals('JOIN {test} t ON t.id = id', $joins[0]); + $this->assertEquals('JOIN {test2} t2 ON t2.id = id', $joins[1]); + $this->assertStringStartsWith('LEFT JOIN {customfield_data}', $joins[2]); } /** @@ -151,10 +174,11 @@ class custom_fields_test extends core_reportbuilder_testcase { $this->assertCount(5, $filters); $this->assertContainsOnlyInstancesOf(filter::class, $filters); - [$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() - ]); + // Filter titles. + $this->assertEquals( + ['Text', 'Textarea', 'Checkbox', 'Date', 'Select'], + array_map(fn(filter $filter) => $filter->get_header(), $filters) + ); } /** diff --git a/version.php b/version.php index 29cd1bc3c64..5a8b11d1642 100644 --- a/version.php +++ b/version.php @@ -29,7 +29,7 @@ defined('MOODLE_INTERNAL') || die(); -$version = 2023042406.04; // 20230424 = branching date YYYYMMDD - do not modify! +$version = 2023042406.05; // 20230424 = branching date YYYYMMDD - do not modify! // RR = release increments - 00 in DEV branches. // .XX = incremental changes. $release = '4.2.6+ (Build: 20240305)'; // Human-friendly version name