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

MDL-72663 reportbuilder: improvements to column aggregation methods.

Browse Source 
The group concatenation types now consistently sort the returned values
and support aggregating columns containing multiple fields and those
with defined callbacks.
This commit is contained in:
Paul Holden
2021-10-22 16:17:31 +01:00
parent 16a65f73e7
commit 5ec86bf501
12 changed files with 384 additions and 89 deletions
Download Patch File Download Diff File Expand all files Collapse all files
reportbuilder/classes/local/aggregation/base.php
+23 -3
View File
@@ -66,6 +66,16 @@ abstract class base {
return $columnsortable;
}
/**
* Return SQL suitable for using within {@see get_field_sql} for column fields, by default just the first one
*
* @param string[] $sqlfields
* @return string
*/
public static function get_column_field_sql(array $sqlfields): string {
return reset($sqlfields);
}
/**
* Return the aggregated field SQL
*
@@ -76,12 +86,22 @@ abstract class base {
abstract public static function get_field_sql(string $field, int $columntype): string;
/**
* Return formatted value for column when applying aggregation
* Return formatted value for column when applying aggregation, by default executing all callbacks on the value
*
* Should be overridden in child classes that need to format the column value differently (e.g. 'sum' would just show
* a numeric count value)
*
* @param mixed $value
* @param array $values
* @param array $callbacks
* @param array $callbacks Array of column callbacks, {@see column::add_callback} for definition
* @return mixed
*/
abstract public static function format_value($value, array $values, array $callbacks);
public static function format_value($value, array $values, array $callbacks) {
foreach ($callbacks as $callback) {
[$callable, $arguments] = $callback;
$value = ($callable)($value, (object) $values, $arguments);
}
return $value;
}
}
reportbuilder/classes/local/aggregation/groupconcat.php
+70 -3
View File
@@ -19,6 +19,7 @@ declare(strict_types=1);
namespace core_reportbuilder\local\aggregation;
use lang_string;
use core_reportbuilder\local\helpers\database;
use core_reportbuilder\local\report\column;
/**
@@ -30,6 +31,9 @@ use core_reportbuilder\local\report\column;
*/
class groupconcat extends base {
/** @var string Character to use as a delimeter between column fields */
private const DELIMETER = '|';
/**
* Return aggregation name
*
@@ -51,6 +55,51 @@ class groupconcat extends base {
]);
}
/**
* We cannot sort this aggregation type
*
* @param bool $columnsortable
* @return bool
*/
public static function sortable(bool $columnsortable): bool {
return false;
}
/**
* 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 {
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::DELIMETER . "'";
}
// Slice off the last delimeter.
return $DB->sql_concat(...array_slice($concatfields, 0, -1));
}
/**
* Return the aggregated field SQL
*
@@ -61,11 +110,14 @@ class groupconcat extends base {
public static function get_field_sql(string $field, int $columntype): string {
global $DB;
return $DB->sql_group_concat($field);
$fieldsort = database::sql_group_concat_sort($field);
return $DB->sql_group_concat($field, ', ', $fieldsort);
}
/**
* Return formatted value for column when applying aggregation
* Return formatted value for column when applying aggregation, note we need to split apart the concatenated string
* and apply callbacks to each concatenated value separately
*
* @param mixed $value
* @param array $values
@@ -73,6 +125,21 @@ class groupconcat extends base {
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks) {
return $value;
$formattedvalues = [];
// Store original names of all values that would be present without aggregation.
$valuenames = array_keys($values);
$values = explode(', ', (string) reset($values));
// Loop over each extracted value from the concatenated string.
foreach ($values as $value) {
$originalvalue = array_combine($valuenames, explode(self::DELIMETER, $value));
$originalfirstvalue = reset($originalvalue);
// Once we've re-constructed each value, we can apply callbacks to it.
$formattedvalues[] = parent::format_value($originalfirstvalue, $originalvalue, $callbacks);
}
return implode(', ', $formattedvalues);
}
}
reportbuilder/classes/local/aggregation/groupconcatdistinct.php
+10 -19
View File
@@ -19,7 +19,7 @@ declare(strict_types=1);
namespace core_reportbuilder\local\aggregation;
use lang_string;
use core_reportbuilder\local\report\column;
use core_reportbuilder\local\helpers\database;
/**
* Column group concatenation distinct aggregation type
@@ -28,7 +28,7 @@ use core_reportbuilder\local\report\column;
* @copyright 2021 Paul Holden <[email protected]>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/
class groupconcatdistinct extends base {
class groupconcatdistinct extends groupconcat {
/**
* Return aggregation name
@@ -53,9 +53,7 @@ class groupconcatdistinct extends base {
'postgres',
]);
return $dbsupportedtype && !in_array($columntype, [
column::TYPE_TIMESTAMP,
]);
return $dbsupportedtype && parent::compatible($columntype);
}
/**
@@ -69,22 +67,15 @@ class groupconcatdistinct extends base {
global $DB;
// DB limitations mean we only support MySQL and Postgres, and each handle it differently.
$fieldsort = database::sql_group_concat_sort($field);
if ($DB->get_dbfamily() === 'postgres') {
return "STRING_AGG(DISTINCT CAST({$field} AS VARCHAR), ', ')";
if ($fieldsort !== '') {
$fieldsort = "ORDER BY {$fieldsort}";
}
return "STRING_AGG(DISTINCT CAST({$field} AS VARCHAR), ', ' {$fieldsort})";
} else {
return $DB->sql_group_concat("DISTINCT {$field}");
return $DB->sql_group_concat("DISTINCT {$field}", ', ', $fieldsort);
}
}
/**
* Return formatted value for column when applying aggregation
*
* @param mixed $value
* @param array $values
* @param array $callbacks
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks) {
return $value;
}
}
reportbuilder/classes/local/aggregation/max.php
-16
View File
@@ -64,20 +64,4 @@ class max extends base {
public static function get_field_sql(string $field, int $columntype): string {
return "MAX({$field})";
}
/**
* Return formatted value for column when applying aggregation
*
* @param mixed $value
* @param array $values
* @param array $callbacks
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks) {
foreach ($callbacks as $callback) {
$value = ($callback[0])($value, (object) $values, $callback[1]);
}
return $value;
}
}
reportbuilder/classes/local/aggregation/min.php
-16
View File
@@ -64,20 +64,4 @@ class min extends base {
public static function get_field_sql(string $field, int $columntype): string {
return "MIN({$field})";
}
/**
* Return formatted value for column when applying aggregation
*
* @param mixed $value
* @param array $values
* @param array $callbacks
* @return mixed
*/
public static function format_value($value, array $values, array $callbacks) {
foreach ($callbacks as $callback) {
$value = ($callback[0])($value, (object) $values, $callback[1]);
}
return $value;
}
}
reportbuilder/classes/local/helpers/database.php
+40
View File
@@ -19,6 +19,7 @@ declare(strict_types=1);
namespace core_reportbuilder\local\helpers;
use coding_exception;
use core_text;
/**
* Helper functions for DB manipulations
@@ -74,4 +75,43 @@ class database {
return true;
}
/**
* Generate SQL expression for sorting group concatenated fields
*
* @param string $field The original field or SQL expression
* @param string|null $sort A valid SQL ORDER BY to sort the concatenated fields, if omitted then $field will be used
* @return string
*/
public static function sql_group_concat_sort(string $field, string $sort = null): string {
global $DB;
// Fallback to sorting by the specified field, unless it contains parameters which would be duplicated.
if ($sort === null && !preg_match('/[:?$]/', $field)) {
$fieldsort = $field;
} else {
$fieldsort = $sort;
}
// Nothing to sort by.
if ($fieldsort === null) {
return '';
}
// If the sort specifies a direction, we need to handle that differently in Postgres.
if ($DB->get_dbfamily() === 'postgres') {
$fieldsortdirection = '';
preg_match('/(?<direction>ASC|DESC)?$/i', $fieldsort, $matches);
if (array_key_exists('direction', $matches)) {
$fieldsortdirection = $matches['direction'];
$fieldsort = core_text::substr($fieldsort, 0, -(core_text::strlen($fieldsortdirection)));
}
// Cast sort, stick the direction on the end.
$fieldsort = "CAST({$fieldsort} AS VARCHAR) {$fieldsortdirection}";
}
return $fieldsort;
}
}
reportbuilder/classes/local/helpers/format.php
+10 -7
View File
@@ -21,7 +21,10 @@ namespace core_reportbuilder\local\helpers;
use stdClass;
/**
* Class containing helper methods for format columns data as callbacks.
* Class containing helper methods for formatting column data via callbacks
*
* Note that type hints for each $value argument are avoided to allow for these callbacks to be executed when columns are
* aggregated using one of the "Group concatenation" methods, where the value is typically stringified
*
* @package core_reportbuilder
* @copyright 2021 Sara Arjona <[email protected]> based on Alberto Lara Hernández <[email protected]> code.
@@ -37,8 +40,8 @@ class format {
* @param string|null $format Format string for strftime
* @return string
*/
public static function userdate(int $value, stdClass $row, ?string $format = null): string {
return $value ? userdate($value, $format) : '';
public static function userdate($value, stdClass $row, ?string $format = null): string {
return $value ? userdate((int) $value, $format) : '';
}
/**
@@ -47,8 +50,8 @@ class format {
* @param bool $value
* @return string
*/
public static function boolean_as_text(bool $value): string {
return $value ? get_string('yes') : get_string('no');
public static function boolean_as_text($value): string {
return (bool) $value ? get_string('yes') : get_string('no');
}
/**
@@ -57,7 +60,7 @@ class format {
* @param float $value
* @return string
*/
public static function percent(float $value): string {
return sprintf('%.1f', $value) . '%';
public static function percent($value): string {
return sprintf('%.1f%%', (float) $value);
}
}
reportbuilder/classes/local/report/column.php
+7 -2
View File
@@ -375,10 +375,15 @@ final class column {
public function get_fields(): array {
$fieldsalias = $this->get_fields_sql_alias();
// We aggregate the first field only.
if (!empty($this->aggregation)) {
$fieldsaliassql = array_column($fieldsalias, 'sql');
$field = reset($fieldsalias);
$fields = [$this->aggregation::get_field_sql($field['sql'], $this->get_type()) . " AS {$field['alias']}"];
// If aggregating the column, generate SQL from column fields and use it to generate aggregation SQL.
$columnfieldsql = $this->aggregation::get_column_field_sql($fieldsaliassql);
$aggregationfieldsql = $this->aggregation::get_field_sql($columnfieldsql, $this->get_type());
$fields = ["{$aggregationfieldsql} AS {$field['alias']}"];
} else {
$fields = array_map(static function(array $field): string {
return "{$field['sql']} AS {$field['alias']}";
reportbuilder/tests/behat/behat_reportbuilder.php
+28
View File
@@ -19,7 +19,9 @@ declare(strict_types=1);
// NOTE: no MOODLE_INTERNAL test here, this file may be required by behat before including /config.php.
require_once(__DIR__ . '/../../../lib/behat/behat_base.php');
use core_reportbuilder\local\aggregation\groupconcatdistinct;
use core_reportbuilder\local\models\report;
use core_reportbuilder\local\report\column;
/**
* Behat step definitions for Report builder
@@ -71,4 +73,30 @@ class behat_reportbuilder extends behat_base {
]),
];
}
/**
* Set aggregation for given column in report editor (proxied so we can skip if aggregation type not available)
*
* @When I set the :column column aggregation to :aggregation
*
* @param string $column
* @param string $aggregation
*
* @throws \Moodle\BehatExtension\Exception\SkippedException
*/
public function i_set_the_column_aggregation_to(string $column, string $aggregation): void {
// Skip if aggregation type unavailable.
$aggregationgroupconcatdistinct = (string) groupconcatdistinct::get_name();
if ($aggregation === $aggregationgroupconcatdistinct && !groupconcatdistinct::compatible(column::TYPE_TEXT)) {
throw new \Moodle\BehatExtension\Exception\SkippedException("{$aggregationgroupconcatdistinct} not available");
}
$editlabel = get_string('aggregatecolumn', 'core_reportbuilder', $column);
// Work around for MDL-72696, which treats all inplace_editable elements as text boxes and causes unpredictable
// behaviour in Chrome when it types characters into an inplace_editable of type select (it selects the wrong option).
$this->execute('behat_general::i_click_on', [$this->escape($editlabel), 'link']);
$this->execute('behat_forms::i_set_the_field_to', [$this->escape($editlabel), $this->escape($aggregation)]);
}
}
reportbuilder/tests/behat/columnaggregationeditor.feature
+96
View File
@@ -0,0 +1,96 @@
@core_reportbuilder @javascript
Feature: Manage custom report columns aggregation
In order to manage the aggregation for columns of custom reports
As an admin
I need to select an aggregation for columns
Background:
Given the following "users" exist:
| username | firstname | lastname | email | confirmed | lastaccess |
| user01 | Bill | Richie | user01@example.com | 1 | ##2 days ago## |
| user02 | Ben | Richie | user02@example.com | 1 | ##3 days ago## |
| user03 | Bill | Richie | user03@example.com | 0 | ##3 days ago## |
Scenario Outline: Aggregate a text column
Given the following "core_reportbuilder > Reports" exist:
| name | source | default |
| My report | core_user\reportbuilder\datasource\users | 0 |
And the following "core_reportbuilder > Columns" exist:
| report | uniqueidentifier |
| My report | user:lastname |
| My report | user:firstname |
And I am on the "My report" "reportbuilder > Editor" page logged in as "admin"
And I change window size to "large"
When I set the "First name" column aggregation to "<aggregation>"
Then I should see "Aggregated column 'First name'"
And I should see "<output>" in the "Richie" "table_row"
Examples:
| aggregation | output |
| Comma separated distinct values | Ben, Bill |
| Comma separated values | Ben, Bill, Bill |
| Count | 3 |
| Count distinct | 2 |
Scenario Outline: Aggregate a text column containing multiple fields
Given the following "core_reportbuilder > Reports" exist:
| name | source | default |
| My report | core_user\reportbuilder\datasource\users | 0 |
And the following "core_reportbuilder > Columns" exist:
| report | uniqueidentifier |
| My report | user:lastname |
| My report | user:fullname |
And I am on the "My report" "reportbuilder > Editor" page logged in as "admin"
And I change window size to "large"
When I set the "Full name" column aggregation to "<aggregation>"
Then I should see "Aggregated column 'Full name'"
And I should see "<output>" in the "Richie" "table_row"
Examples:
| aggregation | output |
| Comma separated distinct values | Ben Richie, Bill Richie |
| Comma separated values | Ben Richie, Bill Richie, Bill Richie |
| Count | 3 |
| Count distinct | 2 |
Scenario Outline: Aggregate a time column
Given the following "core_reportbuilder > Reports" exist:
| name | source | default |
| My report | core_user\reportbuilder\datasource\users | 0 |
And the following "core_reportbuilder > Columns" exist:
| report | uniqueidentifier |
| My report | user:lastname |
| My report | user:lastaccess |
And I am on the "My report" "reportbuilder > Editor" page logged in as "admin"
And I change window size to "large"
When I set the "Last access" column aggregation to "<aggregation>"
Then I should see "Aggregated column 'Last access'"
And I should see "<output>" in the "Richie" "table_row"
Examples:
| aggregation | output |
| Count | 3 |
| Count distinct | 2 |
| Maximum | ##2 days ago##%A, %d %B %Y## |
| Minimum | ##3 days ago##%A, %d %B %Y## |
Scenario Outline: Aggregate a boolean column
Given the following "core_reportbuilder > Reports" exist:
| name | source | default |
| My report | core_user\reportbuilder\datasource\users | 0 |
And the following "core_reportbuilder > Columns" exist:
| report | uniqueidentifier |
| My report | user:lastname |
| My report | user:confirmed |
And I am on the "My report" "reportbuilder > Editor" page logged in as "admin"
And I change window size to "large"
When I set the "Confirmed" column aggregation to "<aggregation>"
Then I should see "Aggregated column 'Confirmed'"
And I should see "<output>" in the "Richie" "table_row"
Examples:
| aggregation | output |
| Comma separated distinct values | No, Yes |
| Comma separated values | No, Yes, Yes |
| Count | 3 |
| Count distinct | 2 |
| Maximum | Yes |
| Minimum | No |
| Percentage | 66.7% |
| Sum | 2 |
reportbuilder/tests/local/aggregation/groupconcat_test.php
+50 -12
View File
@@ -45,8 +45,8 @@ class groupconcat_test extends core_reportbuilder_testcase {
$this->resetAfterTest();
// Test subjects.
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Apple']);
$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 */
@@ -63,18 +63,56 @@ class groupconcat_test extends core_reportbuilder_testcase {
->set('aggregation', groupconcat::get_class_name())
->update();
[$firstrow, $secondrow] = $this->get_custom_report_content($report->get('id'));
// Assert lastname column was aggregated, and sorted predictably.
$content = $this->get_custom_report_content($report->get('id'));
$this->assertEquals([
'c0_firstname' => 'Admin',
'c1_lastname' => 'User',
], $firstrow);
[
'c0_firstname' => 'Admin',
'c1_lastname' => 'User',
],
[
'c0_firstname' => 'Bob',
'c1_lastname' => 'Apple, Banana, Banana',
],
], $content);
}
// Currently the aggregated field sort order is undefined.
$this->assertEquals('Bob', $secondrow['c0_firstname']);
$this->assertEqualsCanonicalizing([
'Apple',
'Banana',
'Banana',
], explode(', ', $secondrow['c1_lastname']));
/**
* Test aggregation when applied to column with callback
*/
public function test_column_aggregation_with_callback(): void {
$this->resetAfterTest();
// Test subjects.
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'confirmed' => 1]);
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'confirmed' => 0]);
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'confirmed' => 1]);
/** @var core_reportbuilder_generator $generator */
$generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder');
$report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]);
// First column, sorted.
$generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:firstname'])
->set('sortenabled', true)
->update();
// This is the column we'll aggregate.
$generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:confirmed'])
->set('aggregation', groupconcat::get_class_name())
->update();
// 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',
],
[
'c0_firstname' => 'Bob',
'c1_confirmed' => 'No, Yes, Yes',
],
], $content);
}
}
reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php
+50 -11
View File
@@ -57,8 +57,8 @@ class groupconcatdistinct_test extends core_reportbuilder_testcase {
$this->resetAfterTest();
// Test subjects.
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'lastname' => 'Apple']);
$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 */
@@ -75,17 +75,56 @@ class groupconcatdistinct_test extends core_reportbuilder_testcase {
->set('aggregation', groupconcatdistinct::get_class_name())
->update();
[$firstrow, $secondrow] = $this->get_custom_report_content($report->get('id'));
// Assert lastname column was aggregated, and sorted predictably.
$content = $this->get_custom_report_content($report->get('id'));
$this->assertEquals([
'c0_firstname' => 'Admin',
'c1_lastname' => 'User',
], $firstrow);
[
'c0_firstname' => 'Admin',
'c1_lastname' => 'User',
],
[
'c0_firstname' => 'Bob',
'c1_lastname' => 'Apple, Banana',
],
], $content);
}
// Currently the aggregated field sort order is undefined.
$this->assertEquals('Bob', $secondrow['c0_firstname']);
$this->assertEqualsCanonicalizing([
'Apple',
'Banana',
], explode(', ', $secondrow['c1_lastname']));
/**
* Test aggregation when applied to column with callback
*/
public function test_column_aggregation_with_callback(): void {
$this->resetAfterTest();
// Test subjects.
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'confirmed' => 1]);
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'confirmed' => 0]);
$this->getDataGenerator()->create_user(['firstname' => 'Bob', 'confirmed' => 1]);
/** @var core_reportbuilder_generator $generator */
$generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder');
$report = $generator->create_report(['name' => 'Users', 'source' => users::class, 'default' => 0]);
// First column, sorted.
$generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:firstname'])
->set('sortenabled', true)
->update();
// This is the column we'll aggregate.
$generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:confirmed'])
->set('aggregation', groupconcatdistinct::get_class_name())
->update();
// 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',
],
[
'c0_firstname' => 'Bob',
'c1_confirmed' => 'No, Yes',
],
], $content);
}
}