diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php index 9cb1167aa6a..77409aa35f0 100644 --- a/reportbuilder/classes/local/aggregation/base.php +++ b/reportbuilder/classes/local/aggregation/base.php @@ -133,7 +133,7 @@ abstract class base { public static function format_value($value, array $values, array $callbacks, int $columntype) { foreach ($callbacks as $callback) { [$callable, $arguments] = $callback; - $value = ($callable)($value, (object) $values, $arguments); + $value = ($callable)($value, (object) $values, $arguments, static::get_class_name()); } return $value; diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index 5db3f8dd2c4..3db57db8ba5 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -473,18 +473,20 @@ final class column { } /** - * Adds column callback (in the case there are multiple, they will be applied one after another) + * Adds column callback (in the case there are multiple, they will be called iteratively - the result of each passed + * along to the next in the chain) * * The callback should implement the following signature (where $value is the first column field, $row is all column - * fields, and $additionalarguments are those passed on from this method): + * fields, $additionalarguments are those passed to this method, and $aggregation indicates the current aggregation type + * being applied to the column): + * + * function($value, stdClass $row, $additionalarguments, ?string $aggregation): string * * The type of the $value parameter passed to the callback is determined by calling {@see set_type}, this type is preserved - * if the column is part of a report source and is being aggregated. - * For entities that can to be left joined to a report, the first argument to their column callbacks must be nullable. + * if the column is part of a report source and is being aggregated. For entities that can be left joined to a report, the + * first argument of the callback must be nullable (as it should also be if the first column field is itself nullable). * - * function($value, stdClass $row[, $additionalarguments]): string - * - * @param callable $callable function that takes arguments ($value, \stdClass $row, $additionalarguments) + * @param callable $callable * @param mixed $additionalarguments * @return self */ @@ -687,7 +689,7 @@ final class column { } else { foreach ($this->callbacks as $callback) { [$callable, $arguments] = $callback; - $value = ($callable)($value, (object) $values, $arguments); + $value = ($callable)($value, (object) $values, $arguments, null); } } diff --git a/reportbuilder/tests/local/aggregation/avg_test.php b/reportbuilder/tests/local/aggregation/avg_test.php index 661c2129d5a..86123c3c7de 100644 --- a/reportbuilder/tests/local/aggregation/avg_test.php +++ b/reportbuilder/tests/local/aggregation/avg_test.php @@ -23,6 +23,7 @@ use core_reportbuilder_generator; use core_reportbuilder\manager; use core_reportbuilder\local\report\column; use core_user\reportbuilder\datasource\users; +use stdClass; defined('MOODLE_INTERNAL') || die(); @@ -100,19 +101,20 @@ class avg_test extends core_reportbuilder_testcase { $instance = manager::get_report_from_persistent($report); $instance->get_column('user:suspended') ->set_type(column::TYPE_FLOAT) - ->set_callback(static function(float $value): string { - return number_format($value, 1) . ' suspended'; + ->set_callback(static function(float $value, stdClass $row, $arguments, ?string $aggregation): string { + // Simple callback to return the given value, and append aggregation type. + return number_format($value, 1) . " ({$aggregation})"; }); $content = $this->get_custom_report_content($report->get('id')); $this->assertEquals([ [ 'c0_firstname' => 'Admin', - 'c1_suspended' => '0.0 suspended', + 'c1_suspended' => '0.0 (avg)', ], [ 'c0_firstname' => 'Bob', - 'c1_suspended' => '0.5 suspended', + 'c1_suspended' => '0.5 (avg)', ], ], $content); } diff --git a/reportbuilder/tests/local/aggregation/groupconcat_test.php b/reportbuilder/tests/local/aggregation/groupconcat_test.php index f945eab16e9..94b6e9643e7 100644 --- a/reportbuilder/tests/local/aggregation/groupconcat_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcat_test.php @@ -22,7 +22,9 @@ use core_badges_generator; use core_badges\reportbuilder\datasource\badges; use core_reportbuilder_testcase; use core_reportbuilder_generator; +use core_reportbuilder\manager; use core_user\reportbuilder\datasource\users; +use stdClass; defined('MOODLE_INTERNAL') || die(); @@ -132,16 +134,24 @@ class groupconcat_test extends core_reportbuilder_testcase { 'aggregation' => groupconcat::get_class_name(), ]); + // Add callback to format the column. + $instance = manager::get_report_from_persistent($report); + $instance->get_column('user:confirmed') + ->add_callback(static function(string $value, stdClass $row, $arguments, ?string $aggregation): string { + // Simple callback to return the given value, and append aggregation type. + return "{$value} ({$aggregation})"; + }); + // 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', + 'c1_confirmed' => 'Yes (groupconcat)', ], [ 'c0_firstname' => 'Bob', - 'c1_confirmed' => 'No, Yes, Yes', + 'c1_confirmed' => 'No (groupconcat), Yes (groupconcat), Yes (groupconcat)', ], ], $content); } diff --git a/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php b/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php index f68459010b0..0477ba8a423 100644 --- a/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php @@ -20,8 +20,10 @@ namespace core_reportbuilder\local\aggregation; use core_reportbuilder_testcase; use core_reportbuilder_generator; +use core_reportbuilder\manager; use core_reportbuilder\local\report\column; use core_user\reportbuilder\datasource\users; +use stdClass; defined('MOODLE_INTERNAL') || die(); @@ -142,16 +144,24 @@ class groupconcatdistinct_test extends core_reportbuilder_testcase { 'aggregation' => groupconcatdistinct::get_class_name(), ]); + // Add callback to format the column. + $instance = manager::get_report_from_persistent($report); + $instance->get_column('user:confirmed') + ->add_callback(static function(string $value, stdClass $row, $arguments, ?string $aggregation): string { + // Simple callback to return the given value, and append aggregation type. + return "{$value} ({$aggregation})"; + }); + // 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', + 'c1_confirmed' => 'Yes (groupconcatdistinct)', ], [ 'c0_firstname' => 'Bob', - 'c1_confirmed' => 'No, Yes', + 'c1_confirmed' => 'No (groupconcatdistinct), Yes (groupconcatdistinct)', ], ], $content); } diff --git a/reportbuilder/tests/local/aggregation/sum_test.php b/reportbuilder/tests/local/aggregation/sum_test.php index 56b4fb7efd0..c8b8aaa467e 100644 --- a/reportbuilder/tests/local/aggregation/sum_test.php +++ b/reportbuilder/tests/local/aggregation/sum_test.php @@ -23,6 +23,7 @@ use core_reportbuilder_generator; use core_reportbuilder\manager; use core_reportbuilder\local\report\column; use core_user\reportbuilder\datasource\users; +use stdClass; defined('MOODLE_INTERNAL') || die(); @@ -101,19 +102,20 @@ class sum_test extends core_reportbuilder_testcase { $instance = manager::get_report_from_persistent($report); $instance->get_column('user:suspended') ->set_type(column::TYPE_INTEGER) - ->set_callback(static function(int $value): string { - return "{$value} suspended"; + ->set_callback(static function(int $value, stdClass $row, $arguments, ?string $aggregation): string { + // Simple callback to return the given value, and append aggregation type. + return "{$value} ({$aggregation})"; }); $content = $this->get_custom_report_content($report->get('id')); $this->assertEquals([ [ 'c0_firstname' => 'Admin', - 'c1_suspended' => '0 suspended', + 'c1_suspended' => '0 (sum)', ], [ 'c0_firstname' => 'Bob', - 'c1_suspended' => '2 suspended', + 'c1_suspended' => '2 (sum)', ], ], $content); } diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index 721b1a7b3b8..c9483005f3a 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -419,6 +419,22 @@ class column_test extends advanced_testcase { ])); } + /** + * Test that column value with callback (where aggregation is not set) is returned + */ + public function test_format_value_callback_aggregation(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_field('t.foo') + ->set_type(column::TYPE_INTEGER) + ->add_callback(static function(int $value, stdClass $values, $argument, ?string $aggregation): string { + // Simple callback to return the given value, and append type of aggregation parameter. + return "{$value} " . gettype($aggregation); + }); + + $this->assertEquals("42 NULL", $column->format_value(['c1_foo' => 42])); + } + /** * Test adding multiple callbacks to a column */ diff --git a/reportbuilder/upgrade.txt b/reportbuilder/upgrade.txt index da502fc9e4d..6d8b50f7a73 100644 --- a/reportbuilder/upgrade.txt +++ b/reportbuilder/upgrade.txt @@ -4,6 +4,8 @@ Information provided here is intended especially for developers. === 4.2 === * New method `set_checkbox_toggleall` in system report class to allow reports to easily create checkbox toggle columns +* Column callbacks are now passed a fourth argument to indicate the aggregation type currently being applied, which allows + for columns to define how the aggregated data is displayed * New methods `[add|get]_attributes` added to report base class, for including custom attributes in report container HTML * The following attributes can be added to custom reports in order to control card view display (via the `add_attributes` method): - `data-force-card` to force cards view