From 0ba1e78bcf9c0c8f752d9347ab045c06d5acc50f Mon Sep 17 00:00:00 2001 From: Paul Holden Date: Wed, 28 Aug 2024 12:32:51 +0100 Subject: [PATCH] MDL-82938 reportbuilder: update column editor sorting icons. Clarify language strings for the same; remove some duplication from accompanying tests. --- lang/en/reportbuilder.php | 4 +-- ...custom_report_columns_sorting_exporter.php | 15 +++++--- .../local/dynamictabs/editor.mustache | 8 +++-- .../templates/local/settings/area.mustache | 8 +++-- .../templates/local/settings/sorting.mustache | 8 +++-- .../tests/behat/columnsortingeditor.feature | 6 ++-- .../tests/external/columns/add_test.php | 8 ++--- .../tests/external/columns/delete_test.php | 8 ++--- .../external/columns/sort/reorder_test.php | 15 +++----- .../external/columns/sort/toggle_test.php | 21 ++++------- ...m_report_columns_sorting_exporter_test.php | 36 +++++++++++++------ 11 files changed, 72 insertions(+), 65 deletions(-) diff --git a/lang/en/reportbuilder.php b/lang/en/reportbuilder.php index aeba45202b0..e57310a6a64 100644 --- a/lang/en/reportbuilder.php +++ b/lang/en/reportbuilder.php @@ -64,8 +64,8 @@ $string['columnadded'] = 'Added column \'{$a}\''; $string['columnaggregated'] = 'Aggregated column \'{$a}\''; $string['columndeleted'] = 'Deleted column \'{$a}\''; $string['columnmoved'] = 'Moved column \'{$a}\''; -$string['columnsortdirectionasc'] = 'Sort column \'{$a}\' ascending'; -$string['columnsortdirectiondesc'] = 'Sort column \'{$a}\' descending'; +$string['columnsortdirectionasc'] = 'Change initial sorting for column \'{$a}\' to ascending'; +$string['columnsortdirectiondesc'] = 'Change initial sorting for column \'{$a}\' to descending'; $string['columnsortdisable'] = 'Disable initial sorting for column \'{$a}\''; $string['columnsortenable'] = 'Enable initial sorting for column \'{$a}\''; $string['columnsortupdated'] = 'Updated sorting for column \'{$a}\''; diff --git a/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php b/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php index 2ba1a8a93b6..795ed26625e 100644 --- a/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php +++ b/reportbuilder/classes/external/custom_report_columns_sorting_exporter.php @@ -102,12 +102,16 @@ class custom_report_columns_sorting_exporter extends exporter { $columntitle = $column->get_title(); $columnheading = $persistent->get_formatted_heading($report->get_context()); - $columnsortascending = ($persistent->get('sortdirection') == SORT_ASC); $sortenabledtitle = $persistent->get('sortenabled') ? 'columnsortdisable' : 'columnsortenable'; - $sortdirectiontitle = $columnsortascending ? 'columnsortdirectiondesc' : 'columnsortdirectionasc'; - $icon = $columnsortascending ? 't/uplong' : 't/downlong'; - $sorticon = new pix_icon($icon, get_string($sortdirectiontitle, 'core_reportbuilder', $columntitle)); + // The icon should reflect the current sort order, with prompt to invert the order. + if ($persistent->get('sortdirection') === SORT_ASC) { + $sorticon = 't/sort_asc'; + $sorticonstr = 'columnsortdirectiondesc'; + } else { + $sorticon = 't/sort_desc'; + $sorticonstr = 'columnsortdirectionasc'; + } return [ 'id' => $persistent->get('id'), @@ -116,7 +120,8 @@ class custom_report_columns_sorting_exporter extends exporter { 'sortdirection' => $persistent->get('sortdirection'), 'sortenabled' => $persistent->get('sortenabled'), 'sortorder' => $persistent->get('sortorder'), - 'sorticon' => $sorticon->export_for_pix(), + 'sorticon' => (new pix_icon($sorticon, get_string($sorticonstr, 'core_reportbuilder', $columntitle))) + ->export_for_pix(), 'movetitle' => get_string('movesorting', 'core_reportbuilder', $columntitle), 'sortenabledtitle' => get_string($sortenabledtitle, 'core_reportbuilder', $columntitle), ]; diff --git a/reportbuilder/templates/local/dynamictabs/editor.mustache b/reportbuilder/templates/local/dynamictabs/editor.mustache index 8f04f4d30eb..000d76eb121 100644 --- a/reportbuilder/templates/local/dynamictabs/editor.mustache +++ b/reportbuilder/templates/local/dynamictabs/editor.mustache @@ -77,12 +77,14 @@ "title": "Email address", "sortdirection": "4", "sortenabled": true, + "sortenabledtitle": "Disable initial sorting for column 'Email address'", "sortorder": 1, "sorticon": [{ - "key": "t/uplong", - "component": "core", - "title": "Sort column 'Email address' ascending" + "key": "t/sort_asc", + "component": "moodle", + "title": "Change initial sorting for column 'Email address' to descending" }], + "movetitle": "Move sorting for column 'Email address'", "heading": "Email address" }] }], diff --git a/reportbuilder/templates/local/settings/area.mustache b/reportbuilder/templates/local/settings/area.mustache index f958cc71e2a..809551c3a66 100644 --- a/reportbuilder/templates/local/settings/area.mustache +++ b/reportbuilder/templates/local/settings/area.mustache @@ -60,12 +60,14 @@ "title": "Email address", "sortdirection": "4", "sortenabled": true, + "sortenabledtitle": "Disable initial sorting for column 'Email address'", "sortorder": 1, "sorticon": [{ - "key": "t/uplong", - "component": "core", - "title": "Sort column 'Email address' ascending" + "key": "t/sort_asc", + "component": "moodle", + "title": "Change initial sorting for column 'Email address' to descending" }], + "movetitle": "Move sorting for column 'Email address'", "heading": "Email address" }] }], diff --git a/reportbuilder/templates/local/settings/sorting.mustache b/reportbuilder/templates/local/settings/sorting.mustache index 3f2f81bcbb2..5d4f003552b 100644 --- a/reportbuilder/templates/local/settings/sorting.mustache +++ b/reportbuilder/templates/local/settings/sorting.mustache @@ -28,12 +28,14 @@ "title": "Email address", "sortdirection": "4", "sortenabled": true, + "sortenabledtitle": "Disable initial sorting for column 'Email address'", "sortorder": 1, "sorticon": [{ - "key": "t/uplong", - "component": "core", - "title": "Sort column 'Email address' ascending" + "key": "t/sort_asc", + "component": "moodle", + "title": "Change initial sorting for column 'Email address' to descending" }], + "movetitle": "Move sorting for column 'Email address'", "heading": "Email address" }] }] diff --git a/reportbuilder/tests/behat/columnsortingeditor.feature b/reportbuilder/tests/behat/columnsortingeditor.feature index 1ab93f14fb5..e07eedcdd8f 100644 --- a/reportbuilder/tests/behat/columnsortingeditor.feature +++ b/reportbuilder/tests/behat/columnsortingeditor.feature @@ -36,10 +36,10 @@ Feature: Manage custom report columns sorting Given I change window size to "large" And I click on "Show/hide 'Sorting'" "button" When I click on "Enable initial sorting for column 'Last name'" "checkbox" - And I click on "Sort column 'Last name' descending" "button" + And I click on "Change initial sorting for column 'Last name' to descending" "button" Then I should see "Updated sorting for column 'Last name'" And "user01" "table_row" should appear before "user02" "table_row" - And I click on "Sort column 'Last name' ascending" "button" + And I click on "Change initial sorting for column 'Last name' to ascending" "button" And I should see "Updated sorting for column 'Last name'" And "user02" "table_row" should appear before "user01" "table_row" @@ -63,7 +63,7 @@ Feature: Manage custom report columns sorting # 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 click on "Change initial sorting for column 'Full name' to 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" diff --git a/reportbuilder/tests/external/columns/add_test.php b/reportbuilder/tests/external/columns/add_test.php index 6070772fb83..1fd7fcb6e67 100644 --- a/reportbuilder/tests/external/columns/add_test.php +++ b/reportbuilder/tests/external/columns/add_test.php @@ -38,7 +38,7 @@ require_once("{$CFG->dirroot}/webservice/tests/helpers.php"); * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class add_test extends externallib_advanced_testcase { +final class add_test extends externallib_advanced_testcase { /** * Text execute method @@ -62,15 +62,13 @@ class add_test extends externallib_advanced_testcase { $this->assertTrue($result['hassortablecolumns']); $this->assertCount(1, $result['sortablecolumns']); + $sortablecolumn = reset($result['sortablecolumns']); $this->assertEquals('Full name', $sortablecolumn['title']); $this->assertEquals(SORT_ASC, $sortablecolumn['sortdirection']); $this->assertEquals(0, $sortablecolumn['sortenabled']); $this->assertEquals(1, $sortablecolumn['sortorder']); - $this->assertEquals('t/uplong', $sortablecolumn['sorticon']['key']); - $this->assertEquals('moodle', $sortablecolumn['sorticon']['component']); - $str = get_string('columnsortdirectiondesc', 'core_reportbuilder', 'Full name'); - $this->assertEquals($str, $sortablecolumn['sorticon']['title']); + $this->assertArrayHasKey('sorticon', $sortablecolumn); // Assert report columns. $columns = column::get_records(['reportid' => $report->get('id')]); diff --git a/reportbuilder/tests/external/columns/delete_test.php b/reportbuilder/tests/external/columns/delete_test.php index 08312f29b6a..d77d888b579 100644 --- a/reportbuilder/tests/external/columns/delete_test.php +++ b/reportbuilder/tests/external/columns/delete_test.php @@ -38,7 +38,7 @@ require_once("{$CFG->dirroot}/webservice/tests/helpers.php"); * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class delete_test extends externallib_advanced_testcase { +final class delete_test extends externallib_advanced_testcase { /** * Text execute method @@ -66,15 +66,13 @@ class delete_test extends externallib_advanced_testcase { $this->assertTrue($result['hassortablecolumns']); $this->assertCount(1, $result['sortablecolumns']); + $sortablecolumn = reset($result['sortablecolumns']); $this->assertEquals('Email address', $sortablecolumn['title']); $this->assertEquals(SORT_ASC, $sortablecolumn['sortdirection']); $this->assertEquals(0, $sortablecolumn['sortenabled']); $this->assertEquals(2, $sortablecolumn['sortorder']); - $this->assertEquals('t/uplong', $sortablecolumn['sorticon']['key']); - $this->assertEquals('moodle', $sortablecolumn['sorticon']['component']); - $str = get_string('columnsortdirectiondesc', 'core_reportbuilder', 'Email address'); - $this->assertEquals($str, $sortablecolumn['sorticon']['title']); + $this->assertArrayHasKey('sorticon', $sortablecolumn); // Assert report columns. $columns = column::get_records(['reportid' => $report->get('id')]); diff --git a/reportbuilder/tests/external/columns/sort/reorder_test.php b/reportbuilder/tests/external/columns/sort/reorder_test.php index 54822407640..3b224d8930b 100644 --- a/reportbuilder/tests/external/columns/sort/reorder_test.php +++ b/reportbuilder/tests/external/columns/sort/reorder_test.php @@ -38,7 +38,7 @@ require_once("{$CFG->dirroot}/webservice/tests/helpers.php"); * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class reorder_test extends externallib_advanced_testcase { +final class reorder_test extends externallib_advanced_testcase { /** * Text execute method @@ -63,20 +63,13 @@ class reorder_test extends externallib_advanced_testcase { $this->assertTrue($result['hassortablecolumns']); $this->assertCount(4, $result['sortablecolumns']); - $columnid = $columncity->get('id'); - $sortablecolumn = array_filter($result['sortablecolumns'], function(array $column) use($columnid) { - return $column['id'] == $columnid; - }); - $sortablecolumn = reset($sortablecolumn); - $this->assertEquals($columnid, $sortablecolumn['id']); + + $sortablecolumn = $result['sortablecolumns'][1]; $this->assertEquals('City/town', $sortablecolumn['title']); $this->assertEquals(SORT_ASC, $sortablecolumn['sortdirection']); $this->assertEquals(0, $sortablecolumn['sortenabled']); $this->assertEquals(2, $sortablecolumn['sortorder']); - $this->assertEquals('t/uplong', $sortablecolumn['sorticon']['key']); - $this->assertEquals('moodle', $sortablecolumn['sorticon']['component']); - $str = get_string('columnsortdirectiondesc', 'core_reportbuilder', 'City/town'); - $this->assertEquals($str, $sortablecolumn['sorticon']['title']); + $this->assertArrayHasKey('sorticon', $sortablecolumn); // Assert report column sort order. $columns = column::get_records(['reportid' => $report->get('id')], 'sortorder'); diff --git a/reportbuilder/tests/external/columns/sort/toggle_test.php b/reportbuilder/tests/external/columns/sort/toggle_test.php index c9c11b8124c..48bb3b9d22b 100644 --- a/reportbuilder/tests/external/columns/sort/toggle_test.php +++ b/reportbuilder/tests/external/columns/sort/toggle_test.php @@ -38,7 +38,7 @@ require_once("{$CFG->dirroot}/webservice/tests/helpers.php"); * @copyright 2021 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class toggle_test extends externallib_advanced_testcase { +final class toggle_test extends externallib_advanced_testcase { /** * Text execute method @@ -49,7 +49,7 @@ class toggle_test extends externallib_advanced_testcase { /** @var core_reportbuilder_generator $generator */ $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); - $report = $generator->create_report(['name' => 'My report', 'source' => users::class]); + $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); $column = $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:email']); // Toggle sort descending. @@ -57,21 +57,14 @@ class toggle_test extends externallib_advanced_testcase { $result = external_api::clean_returnvalue(toggle::execute_returns(), $result); $this->assertTrue($result['hassortablecolumns']); - $this->assertCount(4, $result['sortablecolumns']); - $columnid = $column->get('id'); - $sortablecolumn = array_filter($result['sortablecolumns'], function(array $column) use($columnid) { - return $column['id'] == $columnid; - }); - $sortablecolumn = reset($sortablecolumn); - $this->assertEquals($columnid, $sortablecolumn['id']); + $this->assertCount(1, $result['sortablecolumns']); + + $sortablecolumn = reset($result['sortablecolumns']); $this->assertEquals('Email address', $sortablecolumn['title']); $this->assertEquals(SORT_DESC, $sortablecolumn['sortdirection']); $this->assertEquals(1, $sortablecolumn['sortenabled']); - $this->assertEquals(4, $sortablecolumn['sortorder']); - $this->assertEquals('t/downlong', $sortablecolumn['sorticon']['key']); - $this->assertEquals('moodle', $sortablecolumn['sorticon']['component']); - $str = get_string('columnsortdirectionasc', 'core_reportbuilder', 'Email address'); - $this->assertEquals($str, $sortablecolumn['sorticon']['title']); + $this->assertEquals(1, $sortablecolumn['sortorder']); + $this->assertArrayHasKey('sorticon', $sortablecolumn); // Confirm column was updated. $columnupdated = new column($column->get('id')); diff --git a/reportbuilder/tests/external/custom_report_columns_sorting_exporter_test.php b/reportbuilder/tests/external/custom_report_columns_sorting_exporter_test.php index 925b00b3051..0f98b83e982 100644 --- a/reportbuilder/tests/external/custom_report_columns_sorting_exporter_test.php +++ b/reportbuilder/tests/external/custom_report_columns_sorting_exporter_test.php @@ -21,7 +21,6 @@ namespace core_reportbuilder\external; use advanced_testcase; use core_reportbuilder_generator; use core_reportbuilder\manager; -use core_reportbuilder\local\helpers\report; use core_user\reportbuilder\datasource\users; /** @@ -32,7 +31,7 @@ use core_user\reportbuilder\datasource\users; * @copyright 2022 Paul Holden * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later */ -class custom_report_columns_sorting_exporter_test extends advanced_testcase { +final class custom_report_columns_sorting_exporter_test extends advanced_testcase { /** * Test exported data structure @@ -46,13 +45,20 @@ class custom_report_columns_sorting_exporter_test extends advanced_testcase { $generator = $this->getDataGenerator()->get_plugin_generator('core_reportbuilder'); $report = $generator->create_report(['name' => 'My report', 'source' => users::class, 'default' => false]); - // Add a couple of columns. - $columnfullname = $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:fullname']); - $columnemail = $generator->create_column(['reportid' => $report->get('id'), 'uniqueidentifier' => 'user:email']); - - // Enable sorting on the email column, move it to first place. - report::toggle_report_column_sorting($report->get('id'), $columnemail->get('id'), true, SORT_DESC); - report::reorder_report_column_sorting($report->get('id'), $columnemail->get('id'), 1); + // Add a couple of columns (enable sorting on the email column, move it to first place.). + $columnfullname = $generator->create_column([ + 'reportid' => $report->get('id'), + 'uniqueidentifier' => 'user:fullname', + 'sortenabled' => false, + 'sortorder' => 2, + ]); + $columnemail = $generator->create_column([ + 'reportid' => $report->get('id'), + 'uniqueidentifier' => 'user:email', + 'sortenabled' => true, + 'sortdirection' => SORT_DESC, + 'sortorder' => 1, + ]); $reportinstance = manager::get_report_from_persistent($report); @@ -70,7 +76,11 @@ class custom_report_columns_sorting_exporter_test extends advanced_testcase { $this->assertEquals(1, $sortcolumnemail['sortorder']); $this->assertEquals(SORT_DESC, $sortcolumnemail['sortdirection']); $this->assertEquals('Disable initial sorting for column \'Email address\'', $sortcolumnemail['sortenabledtitle']); - $this->assertEquals('Sort column \'Email address\' ascending', $sortcolumnemail['sorticon']['title']); + $this->assertEquals([ + 'key' => 't/sort_desc', + 'component' => 'moodle', + 'title' => 'Change initial sorting for column \'Email address\' to ascending', + ], $sortcolumnemail['sorticon']); // Fullname column. $this->assertEquals($columnfullname->get('id'), $sortcolumnfullname['id']); @@ -79,7 +89,11 @@ class custom_report_columns_sorting_exporter_test extends advanced_testcase { $this->assertEquals(2, $sortcolumnfullname['sortorder']); $this->assertEquals(SORT_ASC, $sortcolumnfullname['sortdirection']); $this->assertEquals('Enable initial sorting for column \'Full name\'', $sortcolumnfullname['sortenabledtitle']); - $this->assertEquals('Sort column \'Full name\' descending', $sortcolumnfullname['sorticon']['title']); + $this->assertEquals([ + 'key' => 't/sort_asc', + 'component' => 'moodle', + 'title' => 'Change initial sorting for column \'Full name\' to descending', + ], $sortcolumnfullname['sorticon']); $this->assertNotEmpty($export->helpicon); }