From eb2e261df4c71a9fc0e422de0da9dcbc2cd0af1b Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Fri, 28 Jan 2022 11:09:56 +0000 Subject: [PATCH] MDL-73706 reportbuilder: fix count for columns with multiple fields. When using the "Count distinct" aggregation type on a column that selects multiple fields, we should account for each of them in the returned SQL. Move helper method to facilitate this to the base aggregation class so it can be re-used between all types. --- .../classes/local/aggregation/base.php | 32 +++++++++++++++++++ .../local/aggregation/countdistinct.php | 14 ++++++++ .../classes/local/aggregation/groupconcat.php | 23 +------------ .../local/aggregation/countdistinct_test.php | 26 +++++++++++++++ 4 files changed, 73 insertions(+), 22 deletions(-) diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php index ffb74d2d87e..f175133970b 100644 --- a/reportbuilder/classes/local/aggregation/base.php +++ b/reportbuilder/classes/local/aggregation/base.php @@ -76,6 +76,38 @@ abstract class base { return reset($sqlfields); } + /** + * Helper method for concatenating given fields for a column, so they are suitable for aggregation + * + * @param string[] $sqlfields + * @param string $delimeter + * @return string + */ + final protected static function get_column_fields_concat(array $sqlfields, string $delimeter = ','): string { + global $DB; + + $concatfields = []; + foreach ($sqlfields as $sqlfield) { + + // We need to ensure all values are char (this ought to be done in the DML drivers, see MDL-72184). + switch ($DB->get_dbfamily()) { + case 'postgres' : + $sqlfield = "CAST({$sqlfield} AS VARCHAR)"; + break; + case 'oracle' : + $sqlfield = "TO_CHAR({$sqlfield})"; + break; + } + + // Coalesce all the SQL fields, to remove all nulls. + $concatfields[] = "COALESCE({$sqlfield}, ' ')"; + $concatfields[] = "'{$delimeter}'"; + } + + // Slice off the last delimeter. + return $DB->sql_concat(...array_slice($concatfields, 0, -1)); + } + /** * Return the aggregated field SQL * diff --git a/reportbuilder/classes/local/aggregation/countdistinct.php b/reportbuilder/classes/local/aggregation/countdistinct.php index 7d936e0ebc8..ffcd20011e5 100644 --- a/reportbuilder/classes/local/aggregation/countdistinct.php +++ b/reportbuilder/classes/local/aggregation/countdistinct.php @@ -49,6 +49,20 @@ class countdistinct extends base { return true; } + /** + * Override base method to ensure all SQL fields are concatenated together if there are multiple + * + * @param array $sqlfields + * @return string + */ + public static function get_column_field_sql(array $sqlfields): string { + if (count($sqlfields) === 1) { + return parent::get_column_field_sql($sqlfields); + } + + return self::get_column_fields_concat($sqlfields); + } + /** * Return the aggregated field SQL * diff --git a/reportbuilder/classes/local/aggregation/groupconcat.php b/reportbuilder/classes/local/aggregation/groupconcat.php index 2c1412623fe..9d7daec241e 100644 --- a/reportbuilder/classes/local/aggregation/groupconcat.php +++ b/reportbuilder/classes/local/aggregation/groupconcat.php @@ -75,32 +75,11 @@ class groupconcat extends base { * @return string */ public static function get_column_field_sql(array $sqlfields): string { - global $DB; - if (count($sqlfields) === 1) { return parent::get_column_field_sql($sqlfields); } - // Coalesce all the SQL fields, to remove all nulls. - $concatfields = []; - foreach ($sqlfields as $sqlfield) { - - // We need to ensure all values are char (this ought to be done in the DML drivers, see MDL-72184). - switch ($DB->get_dbfamily()) { - case 'postgres' : - $sqlfield = "CAST({$sqlfield} AS VARCHAR)"; - break; - case 'oracle' : - $sqlfield = "TO_CHAR({$sqlfield})"; - break; - } - - $concatfields[] = "COALESCE({$sqlfield}, ' ')"; - $concatfields[] = "'" . self::COLUMN_FIELD_DELIMETER . "'"; - } - - // Slice off the last delimeter. - return $DB->sql_concat(...array_slice($concatfields, 0, -1)); + return self::get_column_fields_concat($sqlfields, self::COLUMN_FIELD_DELIMETER); } /** diff --git a/reportbuilder/tests/local/aggregation/countdistinct_test.php b/reportbuilder/tests/local/aggregation/countdistinct_test.php index 201f7fca23a..bd46883f370 100644 --- a/reportbuilder/tests/local/aggregation/countdistinct_test.php +++ b/reportbuilder/tests/local/aggregation/countdistinct_test.php @@ -75,4 +75,30 @@ class countdistinct_test extends core_reportbuilder_testcase { ], ], $content); } + + /** + * Test aggregation when applied to column with multiple fields + */ + public function test_column_aggregation_multiple_fields(): void { + $this->resetAfterTest(); + + // Create a user with the same firstname as existing admin. + $this->getDataGenerator()->create_user(['firstname' => 'Admin', 'lastname' => 'Test']); + + /** @var core_reportbuilder_generator $generator */ + $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); + $report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]); + + // This is the column we'll aggregate. + $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:fullname']) + ->set('aggregation', countdistinct::get_class_name()) + ->update(); + + $content = $this->get_custom_report_content($report->get('id')); + $this->assertCount(1, $content); + + // There are two distinct fullnames ("Admin User" & "Admin Test"). + $countdistinct = reset($content[0]); + $this->assertEquals(2, $countdistinct); + } }