diff --git a/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php b/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php index e2b8b96f753..7eaef089b93 100644 --- a/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php +++ b/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php @@ -94,7 +94,9 @@ class custom_report_columns_sorting_exporter extends exporter { $reportid = $report->get_report_persistent()->get('id'); $activecolumns = column::get_records(['reportid' => $reportid], 'sortorder'); $sortablecolumns = array_filter($activecolumns, function(column $persistent) use($report) { - $column = $report->get_column($persistent->get('uniqueidentifier')); + $column = $report->get_column($persistent->get('uniqueidentifier')) + ->set_aggregation($persistent->get('aggregation')); + return $column && $column->get_is_sortable(); }); diff --git a/reportbuilder/classes/local/aggregation/base.php b/reportbuilder/classes/local/aggregation/base.php index 828514c6b3d..ffb74d2d87e 100644 --- a/reportbuilder/classes/local/aggregation/base.php +++ b/reportbuilder/classes/local/aggregation/base.php @@ -56,6 +56,26 @@ abstract class base { */ abstract public static function compatible(int $columntype): bool; + /** + * Whether the aggregation is sortable, by default return the sortable status of the column itself + * + * @param bool $columnsortable + * @return bool + */ + public static function sortable(bool $columnsortable): bool { + 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 * @@ -66,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; + } } diff --git a/reportbuilder/classes/local/aggregation/groupconcat.php b/reportbuilder/classes/local/aggregation/groupconcat.php index 4367aa1abad..0e490c47689 100644 --- a/reportbuilder/classes/local/aggregation/groupconcat.php +++ b/reportbuilder/classes/local/aggregation/groupconcat.php @@ -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); } } diff --git a/reportbuilder/classes/local/aggregation/groupconcatdistinct.php b/reportbuilder/classes/local/aggregation/groupconcatdistinct.php index 831a9175bf1..dd3f0963815 100644 --- a/reportbuilder/classes/local/aggregation/groupconcatdistinct.php +++ b/reportbuilder/classes/local/aggregation/groupconcatdistinct.php @@ -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 * @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; - } } diff --git a/reportbuilder/classes/local/aggregation/max.php b/reportbuilder/classes/local/aggregation/max.php index c801b9101e8..6dd1dcc6806 100644 --- a/reportbuilder/classes/local/aggregation/max.php +++ b/reportbuilder/classes/local/aggregation/max.php @@ -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; - } } diff --git a/reportbuilder/classes/local/aggregation/min.php b/reportbuilder/classes/local/aggregation/min.php index efde0020068..91608d7ad1c 100644 --- a/reportbuilder/classes/local/aggregation/min.php +++ b/reportbuilder/classes/local/aggregation/min.php @@ -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; - } } diff --git a/reportbuilder/classes/local/entities/user.php b/reportbuilder/classes/local/entities/user.php index 39c3b6dd224..3e8c1f7ea56 100644 --- a/reportbuilder/classes/local/entities/user.php +++ b/reportbuilder/classes/local/entities/user.php @@ -128,6 +128,12 @@ class user extends base { return ''; } + // Ensure we populate all required name properties. + $namefields = fields::get_name_fields(); + foreach ($namefields as $namefield) { + $row->{$namefield} = $row->{$namefield} ?? ''; + } + return fullname($row, $viewfullnames); }); @@ -155,6 +161,12 @@ class user extends base { return ''; } + // Ensure we populate all required name properties. + $namefields = fields::get_name_fields(); + foreach ($namefields as $namefield) { + $row->{$namefield} = $row->{$namefield} ?? ''; + } + if ($fullnamefield === 'fullnamewithlink') { return html_writer::link(new moodle_url('/user/profile.php', ['id' => $row->id]), fullname($row, $viewfullnames)); @@ -190,7 +202,9 @@ class user extends base { ->add_fields($userpictureselect) ->set_type(column::TYPE_INTEGER) ->set_is_sortable($this->is_sortable('picture')) - ->add_callback(static function (int $value, stdClass $row): string { + // It doesn't make sense to offer integer aggregation methods for this column. + ->set_disabled_aggregation(['avg', 'max', 'min', 'sum']) + ->add_callback(static function ($value, stdClass $row): string { global $OUTPUT; return !empty($row->id) ? $OUTPUT->user_picture($row, ['link' => false, 'alttext' => false]) : ''; @@ -264,17 +278,29 @@ class user extends base { /** * Returns a SQL statement to select all user fields necessary for fullname() function * + * Note the implementation here is similar to {@see fields::get_sql_fullname} but without concatenation + * * @param string $usertablealias * @return string */ public static function get_name_fields_select(string $usertablealias = 'u'): string { + + $namefields = fields::get_name_fields(true); + + // Create a dummy user object containing all name fields. + $dummyuser = (object) array_combine($namefields, $namefields); + $dummyfullname = fullname($dummyuser, true); + + // Extract any name fields from the fullname format in the order that they appear. + $matchednames = array_values(order_in_string($namefields, $dummyfullname)); + $userfields = array_map(static function(string $userfield) use ($usertablealias): string { if (!empty($usertablealias)) { $userfield = "{$usertablealias}.{$userfield}"; } return $userfield; - }, fields::get_name_fields(true)); + }, $matchednames); return implode(', ', $userfields); } diff --git a/reportbuilder/classes/local/helpers/aggregation.php b/reportbuilder/classes/local/helpers/aggregation.php index 8eaa9878440..162b622a84a 100644 --- a/reportbuilder/classes/local/helpers/aggregation.php +++ b/reportbuilder/classes/local/helpers/aggregation.php @@ -52,24 +52,33 @@ class aggregation { return class_exists($aggregationclass) && is_subclass_of($aggregationclass, base::class); } + /** + * Return list of all available/valid aggregation types + * + * @return base[] + */ + public static function get_aggregations(): array { + $classes = core_component::get_component_classes_in_namespace('core_reportbuilder', 'local\\aggregation'); + + return array_filter(array_keys($classes), static function(string $class): bool { + return static::valid($class); + }); + } + /** * Get available aggregation types for given column type * * @param int $columntype - * @param array $exclude List of types to exclude, if omitted then include all available types + * @param array $exclude List of types to exclude, e.g. ['min', 'sum'] * @return string[] Aggregation types indexed by [shortname => name] */ public static function get_column_aggregations(int $columntype, array $exclude = []): array { $types = []; - $classes = core_component::get_component_classes_in_namespace('core_reportbuilder', 'local\\aggregation'); - foreach ($classes as $class => $path) { - /** @var base $aggregationclass */ - $aggregationclass = $class; - if (static::valid($aggregationclass) && $aggregationclass::compatible($columntype) && - !in_array($aggregationclass::get_class_name(), $exclude)) { - - $types[$aggregationclass::get_class_name()] = (string) $aggregationclass::get_name(); + $classes = static::get_aggregations(); + foreach ($classes as $class) { + if ($class::compatible($columntype) && !in_array($class::get_class_name(), $exclude)) { + $types[$class::get_class_name()] = (string) $class::get_name(); } } diff --git a/reportbuilder/classes/local/helpers/database.php b/reportbuilder/classes/local/helpers/database.php index e391504a44f..9bb3d082feb 100644 --- a/reportbuilder/classes/local/helpers/database.php +++ b/reportbuilder/classes/local/helpers/database.php @@ -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('/(?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; + } } diff --git a/reportbuilder/classes/local/helpers/format.php b/reportbuilder/classes/local/helpers/format.php index 8eae420a6b8..8a0a2971764 100644 --- a/reportbuilder/classes/local/helpers/format.php +++ b/reportbuilder/classes/local/helpers/format.php @@ -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 based on Alberto Lara Hernández 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); } } diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index 0a43f2a308f..75f991f9c09 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -76,12 +76,18 @@ final class column { /** @var array $params */ private $params = []; + /** @var string $groupbysql */ + private $groupbysql; + /** @var array[] $callbacks Array of [callable, additionalarguments] */ private $callbacks = []; /** @var base|null $aggregation Aggregation type to apply to column */ private $aggregation = null; + /** @var array $disabledaggregation Aggregation types explicitly disabled */ + private $disabledaggregation = []; + /** @var bool $issortable Used to indicate if a column is sortable */ private $issortable = false; @@ -369,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']}"; @@ -382,23 +393,6 @@ final class column { return array_values($fields); } - /** - * Return suitable SQL fragment for grouping by the column fields (during aggregation) - * - * @return array - */ - public function get_groupby_sql(): array { - global $DB; - - $fieldsalias = $this->get_fields_sql_alias(); - - // Note that we can reference field aliases in GROUP BY only in MySQL/Postgres. - $usealias = in_array($DB->get_dbfamily(), ['mysql', 'postgres']); - $columnname = $usealias ? 'alias' : 'sql'; - - return array_column($fieldsalias, $columnname); - } - /** * Return column parameters, prefixed by the current index to allow the column to be added multiple times to a report * @@ -429,6 +423,39 @@ final class column { return reset($fields)['alias']; } + /** + * Define suitable SQL fragment for grouping by the columns fields. This will be returned from {@see get_groupby_sql} if set + * + * @param string $groupbysql + * @return self + */ + public function set_groupby_sql(string $groupbysql): self { + $this->groupbysql = $groupbysql; + return $this; + } + + /** + * Return suitable SQL fragment for grouping by the column fields (during aggregation) + * + * @return array + */ + public function get_groupby_sql(): array { + global $DB; + + // Return defined value if it's already been set during column definition. + if (!empty($this->groupbysql)) { + return [$this->groupbysql]; + } + + $fieldsalias = $this->get_fields_sql_alias(); + + // Note that we can reference field aliases in GROUP BY only in MySQL/Postgres. + $usealias = in_array($DB->get_dbfamily(), ['mysql', 'postgres']); + $columnname = $usealias ? 'alias' : 'sql'; + + return array_column($fieldsalias, $columnname); + } + /** * Adds column callback (in the case there are multiple, they will be applied one after another) * @@ -488,6 +515,41 @@ final class column { return $this->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 + * + * @param array $disabledaggregation Array of types, e.g. ['min', 'sum'] + * @return self + */ + public function set_disabled_aggregation(array $disabledaggregation): self { + $this->disabledaggregation = $disabledaggregation; + return $this; + } + + /** + * Disable all aggregation methods for the column, for instance when current database can't aggregate fields that contain + * sub-queries + * + * @return self + */ + public function set_disabled_aggregation_all(): self { + $aggregationnames = array_map(static function(string $aggregation): string { + return $aggregation::get_class_name(); + }, aggregation::get_aggregations()); + + return $this->set_disabled_aggregation($aggregationnames); + } + + /** + * Return those aggregations methods explicitly disabled for the column + * + * @return array + */ + public function get_disabled_aggregation(): array { + return $this->disabledaggregation; + } + /** * Sets the column as sortable * @@ -505,6 +567,12 @@ final class column { * @return bool */ public function get_is_sortable(): bool { + + // Defer sortable status to aggregation type if column is being aggregated. + if (!empty($this->aggregation)) { + return $this->aggregation::sortable($this->issortable); + } + return $this->issortable; } @@ -517,8 +585,9 @@ final class column { private function get_values(array $row): array { $values = []; + // During aggregation we only get a single alias back, subsequent aliases won't exist. foreach ($this->get_fields_sql_alias() as $alias => $field) { - $values[$alias] = $row[$field['alias']]; + $values[$alias] = $row[$field['alias']] ?? null; } return $values; @@ -565,7 +634,8 @@ final class column { $value = $this->aggregation::format_value($value, $values, $this->callbacks); } else { foreach ($this->callbacks as $callback) { - $value = ($callback[0])($value, (object) $values, $callback[1]); + [$callable, $arguments] = $callback; + $value = ($callable)($value, (object) $values, $arguments); } } diff --git a/reportbuilder/classes/output/column_aggregation_editable.php b/reportbuilder/classes/output/column_aggregation_editable.php index c2bf76eb99d..2631e72292c 100644 --- a/reportbuilder/classes/output/column_aggregation_editable.php +++ b/reportbuilder/classes/output/column_aggregation_editable.php @@ -53,10 +53,14 @@ class column_aggregation_editable extends inplace_editable { $currentvalue = (string) $column->get('aggregation'); + $editlabel = get_string('aggregatecolumn', 'core_reportbuilder', $columninstance->get_title()); parent::__construct('core_reportbuilder', 'columnaggregation', $column->get('id'), $editable, null, $currentvalue, - get_string('aggregatecolumn', 'core_reportbuilder', $columninstance->get_title())); + $editlabel, $editlabel); + + // List of available aggregation methods for the column type, minus any specifically disabled. + $options = aggregation::get_column_aggregations($columninstance->get_type(), + $columninstance->get_disabled_aggregation()); - $options = aggregation::get_column_aggregations($columninstance->get_type()); $this->set_type_select(['' => get_string('aggregationnone', 'core_reportbuilder')] + $options); } diff --git a/reportbuilder/classes/table/custom_report_table.php b/reportbuilder/classes/table/custom_report_table.php index 909d7d163f5..92de896cec3 100644 --- a/reportbuilder/classes/table/custom_report_table.php +++ b/reportbuilder/classes/table/custom_report_table.php @@ -155,7 +155,7 @@ class custom_report_table extends base_report_table { foreach ($instances as $instance) { $column = $columns[$instance->get('id')] ?? null; - if ($column !== null && $column->get_is_available()) { + if ($column !== null && $column->get_is_sortable()) { $sortcolumns[$column->get_column_alias()] = $instance->get('sortdirection'); } } diff --git a/reportbuilder/tests/behat/behat_reportbuilder.php b/reportbuilder/tests/behat/behat_reportbuilder.php index f869a044b0f..2c9c6a88401 100644 --- a/reportbuilder/tests/behat/behat_reportbuilder.php +++ b/reportbuilder/tests/behat/behat_reportbuilder.php @@ -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)]); + } } diff --git a/reportbuilder/tests/behat/columnaggregationeditor.feature b/reportbuilder/tests/behat/columnaggregationeditor.feature new file mode 100644 index 00000000000..a029d68005c --- /dev/null +++ b/reportbuilder/tests/behat/columnaggregationeditor.feature @@ -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 "" + Then I should see "Aggregated column 'First name'" + And I should see "" 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 "" + Then I should see "Aggregated column 'Full name'" + And I should see "" 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 "" + Then I should see "Aggregated column 'Last access'" + And I should see "" 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 "" + Then I should see "Aggregated column 'Confirmed'" + And I should see "" 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 | diff --git a/reportbuilder/tests/local/aggregation/groupconcat_test.php b/reportbuilder/tests/local/aggregation/groupconcat_test.php index e8dde3931cb..f10ac6c85ba 100644 --- a/reportbuilder/tests/local/aggregation/groupconcat_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcat_test.php @@ -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); } } diff --git a/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php b/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php index 1f930720afd..680a5e37127 100644 --- a/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php +++ b/reportbuilder/tests/local/aggregation/groupconcatdistinct_test.php @@ -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); } } diff --git a/reportbuilder/tests/local/entities/user_test.php b/reportbuilder/tests/local/entities/user_test.php index 4ba2762da67..48107aad052 100644 --- a/reportbuilder/tests/local/entities/user_test.php +++ b/reportbuilder/tests/local/entities/user_test.php @@ -252,23 +252,38 @@ class user_testcase extends advanced_testcase { } /** - * Tests the helper method for selecting all of a users' name fields + * Data provider for {@see test_get_name_fields_select} + * + * @return array */ - public function test_get_name_fields_select(): void { + public function get_name_fields_select_provider(): array { + return [ + ['firstname lastname', ['firstname', 'lastname']], + ['firstname middlename lastname', ['firstname', 'middlename', 'lastname']], + ['alternatename lastname firstname', ['alternatename', 'lastname', 'firstname']], + ]; + } + + /** + * Tests the helper method for selecting all of a users' name fields + * + * @param string $fullnamedisplay + * @param string[] $expecteduserfields + * + * @dataProvider get_name_fields_select_provider + */ + public function test_get_name_fields_select(string $fullnamedisplay, array $expecteduserfields): void { global $DB; + $this->resetAfterTest(true); + + set_config('alternativefullnameformat', $fullnamedisplay); + $fields = user::get_name_fields_select('u'); $user = $DB->get_record_sql("SELECT {$fields} FROM {user} u WHERE username = :username", ['username' => 'admin']); // Ensure we received back all name fields. - $this->assertEqualsCanonicalizing([ - 'firstname', - 'lastname', - 'firstnamephonetic', - 'lastnamephonetic', - 'middlename', - 'alternatename', - ], array_keys((array) $user)); + $this->assertEquals($expecteduserfields, array_keys((array) $user)); } /** diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index 692472c40d9..99f7d117172 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -264,6 +264,39 @@ class column_test extends advanced_testcase { $column->add_field($column->get_column_alias()); } + /** + * Test setting column group by SQL + */ + public function test_set_groupby_sql(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_field('COALESCE(t.foo, t.bar)', 'lionel') + ->set_groupby_sql('t.id'); + + $this->assertEquals(['t.id'], $column->get_groupby_sql()); + } + + /** + * Test getting default column group by SQL + */ + public function test_get_groupby_sql(): void { + global $DB; + + $column = $this->create_column('test') + ->set_index(1) + ->add_fields('t.foo, t.bar'); + + // The behaviour of this method differs due to DB limitations. + $usealias = in_array($DB->get_dbfamily(), ['mysql', 'postgres']); + if ($usealias) { + $expected = ['c1_foo', 'c1_bar']; + } else { + $expected = ['t.foo', 't.bar']; + } + + $this->assertEquals($expected, $column->get_groupby_sql()); + } + /** * Data provider for {@see test_format_value} *