From a73d989a657865f38ebee7cac34d807abaf68c9c Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Tue, 25 Jan 2022 14:46:56 +0000 Subject: [PATCH] MDL-73682 reportbuilder: allow columns to define how they're sorted. --- reportbuilder/classes/local/entities/user.php | 6 ++- reportbuilder/classes/local/report/column.php | 35 +++++++++++++- .../classes/table/base_report_table.php | 28 +++++++++++ .../tests/behat/columnsortingeditor.feature | 15 ++++++ .../tests/local/report/column_test.php | 48 +++++++++++++++++++ 5 files changed, 129 insertions(+), 3 deletions(-) diff --git a/reportbuilder/classes/local/entities/user.php b/reportbuilder/classes/local/entities/user.php index 14feaa3b5e5..f690cc2dd1d 100644 --- a/reportbuilder/classes/local/entities/user.php +++ b/reportbuilder/classes/local/entities/user.php @@ -111,6 +111,8 @@ class user extends base { $usertablealias = $this->get_table_alias('user'); $fullnameselect = self::get_name_fields_select($usertablealias); + $fullnamesort = explode(', ', $fullnameselect); + $userpictureselect = fields::for_userpic()->get_sql($usertablealias, false, '', '', false)->selects; $viewfullnames = has_capability('moodle/site:viewfullnames', context_system::instance()); @@ -123,7 +125,7 @@ class user extends base { ->add_joins($this->get_joins()) ->add_fields($fullnameselect) ->set_type(column::TYPE_TEXT) - ->set_is_sortable($this->is_sortable('fullname')) + ->set_is_sortable($this->is_sortable('fullname'), $fullnamesort) ->add_callback(static function(?string $value, stdClass $row) use ($viewfullnames): string { if ($value === null) { return ''; @@ -154,7 +156,7 @@ class user extends base { ->add_fields($fullnameselect) ->add_field("{$usertablealias}.id") ->set_type(column::TYPE_TEXT) - ->set_is_sortable($this->is_sortable($fullnamefield)) + ->set_is_sortable($this->is_sortable($fullnamefield), $fullnamesort) ->add_callback(static function(?string $value, stdClass $row) use ($fullnamefield, $viewfullnames): string { global $OUTPUT; diff --git a/reportbuilder/classes/local/report/column.php b/reportbuilder/classes/local/report/column.php index 980f5cf4549..0722aadf7c4 100644 --- a/reportbuilder/classes/local/report/column.php +++ b/reportbuilder/classes/local/report/column.php @@ -94,6 +94,9 @@ final class column { /** @var bool $issortable Used to indicate if a column is sortable */ private $issortable = false; + /** @var array $sortfields Fields to sort the column by */ + private $sortfields = []; + /** @var array $attributes */ private $attributes = []; @@ -567,10 +570,13 @@ final class column { * Sets the column as sortable * * @param bool $issortable + * @param array $sortfields Define the fields that should be used when the column is sorted, typically a subset of the fields + * selected for the column, via {@see add_field}. If omitted then the first selected field is used * @return self */ - public function set_is_sortable(bool $issortable): self { + public function set_is_sortable(bool $issortable, array $sortfields = []): self { $this->issortable = $issortable; + $this->sortfields = $sortfields; return $this; } @@ -589,6 +595,33 @@ final class column { return $this->issortable; } + /** + * Return fields to use for sorting of the column, where available the field aliases will be returned + * + * @return array + */ + public function get_sort_fields(): array { + $fieldsalias = $this->get_fields_sql_alias(); + + return array_map(static function(string $sortfield) use ($fieldsalias): string { + + // Check whether sortfield refers to a defined field alias. + if (array_key_exists($sortfield, $fieldsalias)) { + return $fieldsalias[$sortfield]['alias']; + } + + // Check whether sortfield refers to field SQL. + foreach ($fieldsalias as $field) { + if (strcasecmp($sortfield, $field['sql']) === 0) { + $sortfield = $field['alias']; + break; + } + } + + return $sortfield; + }, $this->sortfields); + } + /** * Extract all values from given row for this column * diff --git a/reportbuilder/classes/table/base_report_table.php b/reportbuilder/classes/table/base_report_table.php index 4d2b190e198..28c94c88be9 100644 --- a/reportbuilder/classes/table/base_report_table.php +++ b/reportbuilder/classes/table/base_report_table.php @@ -175,6 +175,34 @@ abstract class base_report_table extends table_sql implements dynamic, renderabl } } + /** + * Override parent method of the same, to ensure that any columns with custom sort fields are accounted for + * + * @return string + */ + public function get_sql_sort() { + $columnsbyalias = $this->report->get_active_columns_by_alias(); + $columnsortby = []; + + // Iterate over all sorted report columns, replace with columns own fields if applicable. + foreach ($this->get_sort_columns() as $alias => $order) { + $column = $columnsbyalias[$alias] ?? null; + + // If the column is not being aggregated and defines custom sort fields, then use them. + if ($column && !$column->get_aggregation() && + ($sortfields = $column->get_sort_fields())) { + + foreach ($sortfields as $sortfield) { + $columnsortby[$sortfield] = $order; + } + } else { + $columnsortby[$alias] = $order; + } + } + + return static::construct_order_by($columnsortby); + } + /** * Set the export class to use when downloading reports (TODO: consider applying to all tables, MDL-72058) * diff --git a/reportbuilder/tests/behat/columnsortingeditor.feature b/reportbuilder/tests/behat/columnsortingeditor.feature index 8e70eaddd78..1c64a7342d1 100644 --- a/reportbuilder/tests/behat/columnsortingeditor.feature +++ b/reportbuilder/tests/behat/columnsortingeditor.feature @@ -17,6 +17,7 @@ Feature: Manage custom report columns sorting | username | firstname | lastname | email | | user01 | Alice | Zebra | user01@example.com | | user02 | Zoe | Aardvark | user02@example.com | + | user03 | Alice | Badger | user03@example.com | And I am on the "My report" "reportbuilder > Editor" page logged in as "admin" Scenario: Toggle column sorting in report @@ -53,6 +54,20 @@ Feature: Manage custom report columns sorting And "First name" "text" should appear before "Surname" "text" in the "#settingssorting" "css_element" And "user01" "table_row" should appear before "user02" "table_row" + Scenario: Change column sorting for column sorted by multiple fields + Given I change window size to "large" + And I click on "Add column 'Full name'" "link" + And I click on "Show/hide 'Sorting'" "button" + When I click on "Enable sorting for column 'Full name'" "checkbox" + Then I should see "Updated sorting for column 'Full name'" + # User1 = Alice Zebra; User2=Zoe Aardvark; User3 = Alice Badger. + And "user03" "table_row" should appear before "user01" "table_row" + And "user01" "table_row" should appear before "user02" "table_row" + And I click on "Sort column 'Full name' descending" "button" + And I should see "Updated sorting for column 'Full name'" + And "user02" "table_row" should appear before "user01" "table_row" + And "user01" "table_row" should appear before "user03" "table_row" + Scenario: Configured report sorting is always applied when editing Given I change window size to "large" And I click on "Show/hide 'Sorting'" "button" diff --git a/reportbuilder/tests/local/report/column_test.php b/reportbuilder/tests/local/report/column_test.php index 3cbe8d974e9..4442747fd54 100644 --- a/reportbuilder/tests/local/report/column_test.php +++ b/reportbuilder/tests/local/report/column_test.php @@ -255,6 +255,17 @@ class column_test extends advanced_testcase { $this->assertEquals('c1_foo', $column->get_column_alias()); } + /** + * Test column alias with a field containing an alias + */ + public function test_column_alias_with_field_alias(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_field('COALESCE(t.foo, t.bar)', 'lionel'); + + $this->assertEquals('c1_lionel', $column->get_column_alias()); + } + /** * Test alias of column without any fields throws exception */ @@ -441,6 +452,43 @@ class column_test extends advanced_testcase { $this->assertTrue($column->get_is_sortable()); } + /** + * Test retrieving sort fields + */ + public function test_get_sortfields(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_fields('t.foo, t.bar, t.baz') + ->set_is_sortable(true, ['t.baz', 't.bar']); + + $this->assertEquals(['c1_baz', 'c1_bar'], $column->get_sort_fields()); + } + + /** + * Test retrieving sort fields when an aliased field is set as sortable + */ + public function test_get_sortfields_with_field_alias(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_field('t.foo') + ->add_field('COALESCE(t.foo, t.bar)', 'lionel') + ->set_is_sortable(true, ['lionel']); + + $this->assertEquals(['c1_lionel'], $column->get_sort_fields()); + } + + /** + * Test retrieving sort fields when an unknown field is set as sortable + */ + public function test_get_sortfields_unknown_field(): void { + $column = $this->create_column('test') + ->set_index(1) + ->add_fields('t.foo') + ->set_is_sortable(true, ['t.baz']); + + $this->assertEquals(['t.baz'], $column->get_sort_fields()); + } + /** * Test is available */