L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'MDL-84537' of https://github.com/paulholden/moodle

Browse Source
This commit is contained in:
Sara Arjona
2025-03-10 16:59:11 +01:00
parent 0d71bbbd96 89738625b4
commit 8e1248930f
19 changed files with 205 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.upgradenotes/MDL-84537-2025021416101901.yml
+18
View File
@@ -0,0 +1,18 @@
issueNumber: MDL-84537
notes:
core_reportbuilder:
- message: >-
Aggregation types can access passed options set via the base class
constructor in the `$this->options[]` class property. As such, their
`format_value` method is no longer static and is always called from an
instantiated class instance
type: changed
- message: >-
New `$options` argument added to the
`column::set_aggregation` method for system reports, to set aggregation
type-specific options
Report entities can call new `column::set_aggregation_options` to
achieve the same
type: changed
.upgradenotes/MDL-84537-2025021417393998.yml
+8
View File
@@ -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/datasource.php
+3 -1
View File
@@ -134,11 +134,13 @@ abstract class datasource extends base {
" {$instance->get_is_deprecated_message()}", DEBUG_DEVELOPER);
}
$columnaggregation = $column->get('aggregation');
// We should clone the report column to ensure if it's added twice to a report, each operates independently.
$this->activecolumns['values'][] = clone $instance
->set_index($index)
->set_persistent($column)
->set_aggregation($column->get('aggregation'));
->set_aggregation($columnaggregation, $instance->get_aggregation_options($columnaggregation));
}
}
reportbuilder/classes/local/aggregation/avg.php
+1 -1
View File
@@ -85,7 +85,7 @@ class avg extends base {
* @param int $columntype
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks, int $columntype) {
public function format_value($value, array $values, array $callbacks, int $columntype) {
if (reset($values) === null) {
return null;
}
reportbuilder/classes/local/aggregation/base.php
+13 -1
View File
@@ -30,6 +30,18 @@ use core_reportbuilder\local\report\column;
*/
abstract class base {
/**
* Constructor
*
* @param array $options Aggregation type specific options
*/
public function __construct(
/** @var array Aggregation type specific options */
protected readonly array $options = [],
) {
}
/**
* Return the class name of the aggregation type
*
@@ -153,7 +165,7 @@ abstract class base {
* @param int $columntype The original type of the column, to ensure it is preserved for callbacks
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks, int $columntype) {
public function format_value($value, array $values, array $callbacks, int $columntype) {
foreach ($callbacks as $callback) {
[$callable, $arguments] = $callback;
$value = ($callable)($value, (object) $values, $arguments, static::get_class_name());
reportbuilder/classes/local/aggregation/count.php
+1 -1
View File
@@ -79,7 +79,7 @@ class count extends base {
* @param int $columntype
* @return int
*/
public static function format_value($value, array $values, array $callbacks, int $columntype): int {
public function format_value($value, array $values, array $callbacks, int $columntype): int {
return (int) reset($values);
}
}
reportbuilder/classes/local/aggregation/countdistinct.php
+1 -1
View File
@@ -93,7 +93,7 @@ class countdistinct extends base {
* @param int $columntype
* @return int
*/
public static function format_value($value, array $values, array $callbacks, int $columntype): int {
public function format_value($value, array $values, array $callbacks, int $columntype): int {
return (int) reset($values);
}
}
reportbuilder/classes/local/aggregation/date.php
+1 -1
View File
@@ -93,7 +93,7 @@ class date extends base {
* @param int $columntype
* @return string
*/
public static function format_value($value, array $values, array $callbacks, int $columntype): string {
public function format_value($value, array $values, array $callbacks, int $columntype): string {
return format::userdate($value, (object) [], get_string('strftimedaydate', 'core_langconfig'));
}
}
reportbuilder/classes/local/aggregation/groupconcat.php
+10 -3
View File
@@ -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 <[email protected]>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
@@ -100,7 +103,7 @@ class groupconcat extends base {
* @param int $columntype
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks, int $columntype) {
public function format_value($value, array $values, array $callbacks, int $columntype) {
$firstvalue = reset($values);
if ($firstvalue === null) {
return '';
@@ -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
+3
View File
@@ -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 <[email protected]>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
reportbuilder/classes/local/aggregation/percent.php
+1 -1
View File
@@ -82,7 +82,7 @@ class percent extends base {
* @param int $columntype
* @return string
*/
public static function format_value($value, array $values, array $callbacks, int $columntype): string {
public function format_value($value, array $values, array $callbacks, int $columntype): string {
if (reset($values) === null) {
return '';
}
reportbuilder/classes/local/aggregation/sum.php
+1 -1
View File
@@ -88,7 +88,7 @@ class sum extends base {
* @param int $columntype
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks, int $columntype) {
public function format_value($value, array $values, array $callbacks, int $columntype) {
$firstvalue = reset($values);
if ($firstvalue === null) {
return null;
reportbuilder/classes/local/report/column.php
+45 -13
View File
@@ -76,7 +76,10 @@ final class column {
private $callbacks = [];
/** @var base|null $aggregation Aggregation type to apply to column */
private $aggregation = null;
private base|null $aggregation = null;
/** @var array[] $aggregationoptions Aggregation type options */
private array $aggregationoptions = [];
/** @var array $disabledaggregation Aggregation types explicitly disabled */
private $disabledaggregation = [];
@@ -358,7 +361,7 @@ final class column {
public function get_fields(): array {
$fieldsalias = $this->get_fields_sql_alias();
if (!empty($this->aggregation)) {
if ($this->aggregation !== null) {
$fieldsaliassql = array_column($fieldsalias, 'sql');
$field = reset($fieldsalias);
@@ -383,7 +386,7 @@ final class column {
* @throws coding_exception
*/
private function get_field_aggregation_sql(array $sqlfields): string {
if (empty($this->aggregation)) {
if ($this->aggregation === null) {
throw new coding_exception('Column aggregation is undefined');
}
@@ -447,7 +450,7 @@ final class column {
// To ensure cross-platform support for column aggregation, where the aggregation should also be grouped, we need
// to generate SQL from column fields and use it to generate aggregation SQL.
if (!empty($this->aggregation) && $this->aggregation::column_groupby()) {
if ($this->aggregation !== null && $this->aggregation::column_groupby()) {
if ($usealias) {
$this->set_groupby_sql($this->get_column_alias());
} else {
@@ -504,18 +507,25 @@ final class column {
* Set column aggregation type
*
* @param string|null $aggregation Type of aggregation, e.g. 'sum', 'count', etc
* @param array|null $options Aggregation type options
* @return self
* @throws coding_exception For invalid aggregation type, or one that is incompatible with column type
*/
public function set_aggregation(?string $aggregation): self {
if (!empty($aggregation)) {
$aggregation = aggregation::get_full_classpath($aggregation);
if (!aggregation::valid($aggregation) || !$aggregation::compatible($this->get_type())) {
public function set_aggregation(?string $aggregation, ?array $options = null): self {
if ((string) $aggregation !== '') {
// Convert aggregation to full class instance for internal storage.
$aggregationclasspath = aggregation::get_full_classpath($aggregation);
if (!aggregation::valid($aggregationclasspath) || !$aggregationclasspath::compatible($this->get_type())) {
throw new coding_exception('Invalid column aggregation', $aggregation);
}
$options ??= $this->get_aggregation_options($aggregation);
$this->aggregation = new $aggregationclasspath($options);
} else {
$this->aggregation = null;
}
$this->aggregation = $aggregation;
return $this;
}
@@ -524,10 +534,32 @@ final class column {
*
* @return base|null
*/
public function get_aggregation(): ?string {
public function get_aggregation(): ?base {
return $this->aggregation;
}
/**
* Set options for the given aggregation type
*
* @param string $aggregation Type of aggregation, e.g. 'sum', 'count', etc
* @param array $options Aggregation type options
* @return self
*/
public function set_aggregation_options(string $aggregation, array $options): self {
$this->aggregationoptions[$aggregation] = $options;
return $this;
}
/**
* Get options for the given aggregation type
*
* @param string|null $aggregation Type of aggregation, e.g. 'sum', 'count', etc
* @return array
*/
public function get_aggregation_options(?string $aggregation): array {
return $this->aggregationoptions[$aggregation] ?? [];
}
/**
* Set disabled aggregation methods for the column. Typically only those methods suitable for the current column type are
* available: {@see aggregation::get_column_aggregations}, however in some cases we may want to disable specific methods
@@ -585,7 +617,7 @@ final class column {
public function get_is_sortable(): bool {
// Defer sortable status to aggregation type if column is being aggregated.
if (!empty($this->aggregation)) {
if ($this->aggregation !== null) {
return $this->aggregation::sortable($this->issortable);
}
@@ -673,9 +705,9 @@ final class column {
$values = $this->get_values($row);
// If column is being aggregated then defer formatting to them, otherwise loop through all column callbacks.
if (!empty($this->aggregation)) {
if ($this->aggregation !== null) {
$value = self::get_default_value($values, $this->aggregation::get_column_type($this->get_type()));
$value = $this->aggregation::format_value($value, $values, $this->callbacks, $this->get_type());
$value = $this->aggregation->format_value($value, $values, $this->callbacks, $this->get_type());
} else {
$value = self::get_default_value($values, $this->get_type());
foreach ($this->callbacks as $callback) {
reportbuilder/tests/behat/customreports.feature
+10 -10
View File
@@ -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
Vendored
+1 -1
View File
@@ -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
Vendored
+1 -1
View File
@@ -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
+44 -18
View File
@@ -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
+41 -9
View File
@@ -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
+2
View File
@@ -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 '';