MDL-84537 reportbuilder: define optional 'separator' aggregation value.

For the group concatenation types, allow callers to define the text to
display between aggregated items. Use this to change the separator for
formatted tag names to be a space character.

.upgradenotes/MDL-84537-2025021417393998.yml

@@ -0,0 +1,8 @@
+issueNumber: MDL-84537
+notes:
+  core_reportbuilder:
+    - message: >-
+        The `groupconcat[distinct]` aggregation types support optional
+        `'separator'` value to specify the text to display between aggregated
+        items
+      type: improved

reportbuilder/classes/local/aggregation/groupconcat.php

@@ -25,6 +25,9 @@ use core_reportbuilder\local\report\column;
 /**
  * Column group concatenation aggregation type
  *
+ * The value used for the separator between aggregated items can be specified by passing the 'separator' option
+ * via {@see column::set_aggregation} or {@see column::set_aggregation_options} methods
+ *
  * @package     core_reportbuilder
  * @copyright   2021 Paul Holden <paulh@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
@@ -133,7 +136,11 @@ class groupconcat extends base {
             $formattedvalues[] = parent::format_value($originalvalue, $originalvalues, $callbacks, $columntype);
         }
 
-        $listseparator = get_string('listsep', 'langconfig') . ' ';
-        return implode($listseparator, $formattedvalues);
+        // Determine separator based on passed options, defaulting to language pack list separator.
+        $separator = array_key_exists('separator', $this->options)
+            ? (string) $this->options['separator']
+            : get_string('listsep', 'langconfig') . ' ';
+
+        return implode($separator, $formattedvalues);
     }
 }

reportbuilder/classes/local/aggregation/groupconcatdistinct.php

@@ -24,6 +24,9 @@ use core_reportbuilder\local\helpers\database;
 /**
  * Column group concatenation distinct aggregation type
  *
+ * The value used for the separator between aggregated items can be specified by passing the 'separator' option
+ * via {@see column::set_aggregation} or {@see column::set_aggregation_options} methods
+ *
  * @package     core_reportbuilder
  * @copyright   2021 Paul Holden <paulh@moodle.com>
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later

reportbuilder/tests/behat/customreports.feature

@@ -1,4 +1,4 @@
-@core_reportbuilder @javascript
+@core @core_reportbuilder @javascript
 Feature: Manage custom reports
   In order to manage custom reports
   As an admin
@@ -96,8 +96,8 @@ Feature: Manage custom reports
     And I click on "Save" "button" in the "New report" "dialogue"
     And I click on "Close 'Manager report' editor" "button"
     And the following should exist in the "Reports list" table:
-      | Name           | Tags     | Report source |
-      | Manager report | Cat, Dog | Users         |
+      | Name           | Tags    | Report source |
+      | Manager report | Cat Dog | Users         |
     # Manager can edit their own report, but not those of other users.
     And I set the field "Edit report name" in the "Manager report" "table_row" to "Manager report (renamed)"
     Then the "Edit report content" item should exist in the "Actions" action menu of the "Manager report (renamed)" "table_row"
@@ -148,8 +148,8 @@ Feature: Manage custom reports
     And I click on "Save" "button" in the "Edit report details" "dialogue"
     Then I should see "Report updated"
     And the following should exist in the "Reports list" table:
-      | Name              | Tags     | Report source |
-      | My renamed report | Cat, Dog | Users         |
+      | Name              | Tags    | Report source |
+      | My renamed report | Cat Dog | Users         |
 
   Scenario Outline: Filter custom reports
     Given the following "core_reportbuilder > Reports" exist:
@@ -159,9 +159,9 @@ Feature: Manage custom reports
     And I log in as "admin"
     When I navigate to "Reports > Report builder > Custom reports" in site administration
     And the following should exist in the "Reports list" table:
-      | Name       | Tags     | Report source |
-      | My users   | Cat, Dog | Users         |
-      | My courses |          | Courses       |
+      | Name       | Tags    | Report source |
+      | My users   | Cat Dog | Users         |
+      | My courses |         | Courses       |
     And I click on "Filters" "button"
     And I set the following fields in the "<filter>" "core_reportbuilder > Filter" to these values:
       | <filter> operator | Is equal to |
@@ -169,8 +169,8 @@ Feature: Manage custom reports
     And I click on "Apply" "button" in the "[data-region='report-filters']" "css_element"
     Then I should see "Filters applied"
     And the following should exist in the "Reports list" table:
-      | Name     | Tags     | Report source |
-      | My users | Cat, Dog | Users         |
+      | Name     | Tags    | Report source |
+      | My users | Cat Dog | Users         |
     And I should not see "My courses" in the "Reports list" "table"
     Examples:
       | filter        | value    |

reportbuilder/tests/external/system_report_data_exporter_test.php

@@ -71,7 +71,7 @@ final class system_report_data_exporter_test extends advanced_testcase {
 
         $this->assertStringContainsString('My second report', $name);
         $this->assertEquals(users::get_name(), $source);
-        $this->assertMatchesRegularExpression('/cat.*, .*dog/', $tags);
+        $this->assertMatchesRegularExpression('/cat.*dog/', $tags);
         $this->assertNotEmpty($timecreated);
         $this->assertNotEmpty($timemodified);
         $this->assertEquals('Admin User', $modifiedby);

reportbuilder/tests/external/systemreports/retrieve_test.php

@@ -75,7 +75,7 @@ final class retrieve_test extends externallib_advanced_testcase {
 
         $this->assertStringContainsString('My second report', $name);
         $this->assertEquals(users::get_name(), $source);
-        $this->assertMatchesRegularExpression('/cat.*, .*dog/', $tags);
+        $this->assertMatchesRegularExpression('/cat.*dog/', $tags);
         $this->assertNotEmpty($timecreated);
         $this->assertNotEmpty($timemodified);
         $this->assertEquals('Admin User', $modifiedby);

reportbuilder/tests/local/aggregation/groupconcat_test.php

@@ -70,6 +70,44 @@ final class groupconcat_test extends core_reportbuilder_testcase {
         ], array_map('array_values', $content));
     }
 
+    /**
+     * Test aggregation with custom separator option when applied to column
+     */
+    public function test_column_aggregation_separator_option(): void {
+        $this->resetAfterTest();
+
+        // Test subjects.
+        $this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Banana']);
+        $this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Apple']);
+        $this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Banana']);
+
+        /** @var core_reportbuilder_generator $generator */
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder');
+        $report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]);
+
+        // Report columns, aggregated/sorted by user lastname.
+        $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:firstname']);
+        $generator->create_column([
+            'reportid' => $report->get('id'),
+            'uniqueidentifier' => 'user:lastname',
+            'aggregation' => groupconcat::get_class_name(),
+            'sortenabled' => 1,
+            'sortdirection' => SORT_ASC,
+        ]);
+
+        // Set aggregation option for separator.
+        $instance = manager::get_report_from_persistent($report);
+        $instance->get_column('user:lastname')
+            ->set_aggregation_options(groupconcat::get_class_name(), ['separator' => '<br />']);
+
+        // Assert lastname column was aggregated, with defined separator between each item.
+        $content = $this->get_custom_report_content($report->get('id'));
+        $this->assertEquals([
+            ['Bob', 'Apple<br />Banana<br />Banana'],
+            ['Admin', 'User'],
+        ], array_map('array_values', $content));
+    }
+
     /**
      * Test aggregation when applied to column with multiple fields
      */
@@ -134,15 +172,9 @@ final class groupconcat_test extends core_reportbuilder_testcase {
         // Assert confirmed column was aggregated, and sorted predictably with callback applied.
         $content = $this->get_custom_report_content($report->get('id'));
         $this->assertEquals([
-            [
-                'c0_firstname' => 'Admin',
-                'c1_confirmed' => 'Yes (groupconcat)',
-            ],
-            [
-                'c0_firstname' => 'Bob',
-                'c1_confirmed' => 'No (groupconcat), Yes (groupconcat), Yes (groupconcat)',
-            ],
-        ], $content);
+            ['Admin', 'Yes (groupconcat)'],
+            ['Bob', 'No (groupconcat), Yes (groupconcat), Yes (groupconcat)'],
+        ], array_map('array_values', $content));
     }
 
     /**
@@ -183,14 +215,8 @@ final class groupconcat_test extends core_reportbuilder_testcase {
         // 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);
+            [$badgeone->name, "{$userone->description}, {$usertwo->description}"],
+            [$badgetwo->name, ''],
+        ], array_map('array_values', $content));
     }
 }

reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php

@@ -81,6 +81,44 @@ final class groupconcatdistinct_test extends core_reportbuilder_testcase {
         ], array_map('array_values', $content));
     }
 
+    /**
+     * Test aggregation with custom separator option when applied to column
+     */
+    public function test_column_aggregation_separator_option(): void {
+        $this->resetAfterTest();
+
+        // Test subjects.
+        $this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Banana']);
+        $this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Apple']);
+        $this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Banana']);
+
+        /** @var core_reportbuilder_generator $generator */
+        $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder');
+        $report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]);
+
+        // Report columns, aggregated/sorted by user lastname.
+        $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:firstname']);
+        $generator->create_column([
+            'reportid' => $report->get('id'),
+            'uniqueidentifier' => 'user:lastname',
+            'aggregation' => groupconcatdistinct::get_class_name(),
+            'sortenabled' => 1,
+            'sortdirection' => SORT_ASC,
+        ]);
+
+        // Set aggregation option for separator.
+        $instance = manager::get_report_from_persistent($report);
+        $instance->get_column('user:lastname')
+            ->set_aggregation_options(groupconcatdistinct::get_class_name(), ['separator' => '<br />']);
+
+        // Assert lastname column was aggregated, with defined separator between each item.
+        $content = $this->get_custom_report_content($report->get('id'));
+        $this->assertEquals([
+            ['Bob', 'Apple<br />Banana'],
+            ['Admin', 'User'],
+        ], array_map('array_values', $content));
+    }
+
     /**
      * Test aggregation when applied to column with multiple fields
      */
@@ -145,14 +183,8 @@ final class groupconcatdistinct_test extends core_reportbuilder_testcase {
         // Assert confirmed column was aggregated, and sorted predictably with callback applied.
         $content = $this->get_custom_report_content($report->get('id'));
         $this->assertEquals([
-            [
-                'c0_firstname' => 'Admin',
-                'c1_confirmed' => 'Yes (groupconcatdistinct)',
-            ],
-            [
-                'c0_firstname' => 'Bob',
-                'c1_confirmed' => 'No (groupconcatdistinct), Yes (groupconcatdistinct)',
-            ],
-        ], $content);
+            ['Admin', 'Yes (groupconcatdistinct)'],
+            ['Bob', 'No (groupconcatdistinct), Yes (groupconcatdistinct)'],
+        ], array_map('array_values', $content));
     }
 }

tag/classes/reportbuilder/local/entities/tag.php

@@ -113,6 +113,8 @@ class tag extends base {
             ->add_joins($this->get_joins())
             ->add_fields("{$tagalias}.rawname, {$tagalias}.name, {$tagalias}.flag, {$tagalias}.isstandard")
             ->set_is_sortable(true)
+            ->set_aggregation_options('groupconcat', ['separator' => ' '])
+            ->set_aggregation_options('groupconcatdistinct', ['separator' => ' '])
             ->add_callback(static function($rawname, stdClass $tag): string {
                 if ($rawname === null) {
                     return '';