diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php index 19d5686a0a2..bc2a5e20fdb 100644 --- a/reportbuilder/classes/local/aggregation/base.php +++ b/reportbuilder/classes/local/aggregation/base.php @@ -81,9 +81,14 @@ abstract class base { * * @param string[] $sqlfields * @param string $delimeter + * @param string $coalescechar * @return string */ - final protected static function get_column_fields_concat(array $sqlfields, string $delimeter = ','): string { + final protected static function get_column_fields_concat( + array $sqlfields, + string $delimeter = ',', + string $coalescechar = ' ' + ): string { global $DB; // We need to ensure all values are char. @@ -95,8 +100,8 @@ abstract class base { $sqlfield = $DB->sql_cast_to_char($sqlfield); } - // Coalesce all the SQL fields, to remove all nulls. - $concatfields[] = "COALESCE({$sqlfield}, ' ')"; + // Coalesce all the SQL fields. Ensure cross-DB compatibility, and that we always get string data back. + $concatfields[] = "COALESCE({$sqlfield}, '{$coalescechar}')"; $concatfields[] = "'{$delimeter}'"; } diff --git a/reportbuilder/classes/local/aggregation/groupconcat.php b/reportbuilder/classes/local/aggregation/groupconcat.php index e3572a9eba7..4cd07b47aba 100644 --- a/reportbuilder/classes/local/aggregation/groupconcat.php +++ b/reportbuilder/classes/local/aggregation/groupconcat.php @@ -34,6 +34,9 @@ class groupconcat extends base { /** @var string Character to use as a delimeter between column fields */ protected const COLUMN_FIELD_DELIMETER = '<|>'; + /** @var string Character to use a null coalesce value */ + protected const COLUMN_NULL_COALESCE = '<^>'; + /** @var string Character to use as a delimeter between field values */ protected const FIELD_VALUE_DELIMETER = '<,>'; @@ -79,7 +82,7 @@ class groupconcat extends base { return parent::get_column_field_sql($sqlfields); } - return self::get_column_fields_concat($sqlfields, self::COLUMN_FIELD_DELIMETER); + return self::get_column_fields_concat($sqlfields, self::COLUMN_FIELD_DELIMETER, self::COLUMN_NULL_COALESCE); } /** @@ -124,7 +127,11 @@ class groupconcat extends base { continue; } - $originalvalues = array_combine($valuenames, $valuedata); + // Re-construct original values, also ensuring any nulls contained within are restored. + $originalvalues = array_map(static function(string $value): ?string { + return $value === self::COLUMN_NULL_COALESCE ? null : $value; + }, array_combine($valuenames, $valuedata)); + $originalvalue = column::get_default_value($originalvalues, $columntype); // Once we've re-constructed each value, we can apply callbacks to it. diff --git a/reportbuilder/tests/local/aggregation/groupconcat_test.php b/reportbuilder/tests/local/aggregation/groupconcat_test.php index e162d09cf81..c339e626c16 100644 --- a/reportbuilder/tests/local/aggregation/groupconcat_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcat_test.php @@ -18,6 +18,8 @@ declare(strict_types=1); namespace core_reportbuilder\local\aggregation; +use core_badges_generator; +use core_badges\reportbuilder\datasource\badges; use core_reportbuilder_testcase; use core_reportbuilder_generator; use core_user\reportbuilder\datasource\users; @@ -141,4 +143,53 @@ class groupconcat_test extends core_reportbuilder_testcase { ], ], $content); } + + /** + * Test aggregation when applied to column with callback that expects/handles null values + */ + public function test_datasource_aggregate_column_callback_with_null(): void { + $this->resetAfterTest(); + $this->setAdminUser(); + + $userone = $this->getDataGenerator()->create_user(['description' => 'First user']); + $usertwo = $this->getDataGenerator()->create_user(['description' => 'Second user']); + + /** @var core_badges_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_badges'); + + // Create course badge, issue to both users. + $badgeone = $generator->create_badge(['name' => 'First badge']); + $badgeone->issue($userone->id, true); + $badgeone->issue($usertwo->id, true); + + // Create second badge, without issuing to anyone. + $badgetwo = $generator->create_badge(['name' => 'Second badge']); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Badges', 'source' => badges::class, 'default' => 0]); + + // First column, sorted. + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'badge:name']) + ->set('sortenabled', true) + ->update(); + + // This is the column we'll aggregate. + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:description']) + ->set('aggregation', groupconcat::get_class_name()) + ->update(); + + // Assert description column was aggregated, with callbacks accounting for null values. + $content = $this->get_custom_report_content($report->get('id')); + $this->assertEquals([ + [ + 'c0_name' => $badgeone->name, + 'c1_description' => "{$userone->description}, {$usertwo->description}", + ], + [ + 'c0_name' => $badgetwo->name, + 'c1_description' => '', + ], + ], $content); + } }