Merge branch 'MDL-73706' of git://github.com/paulholden/moodle

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
      *

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
      *

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);
     }
 
     /**

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);
+    }
 }