Improvement: Align actions column on flavours and smart menus setting…

…s pages, solves #381
moodle-an-hochschulen · Sep 1, 2023 · 4ea7fae · 4ea7fae
1 parent 3e92fd0
commit 4ea7fae
Show file tree

Hide file tree

Showing 10 changed files with 144 additions and 108 deletions.
diff --git a/CHANGES.md b/CHANGES.md
@@ -6,6 +6,7 @@ Changes
 
 ### Unreleased
 
+* 2023-08-30 - Improvement: Align actions column on flavours and smart menus settings pages, solves #381.
 * 2023-08-19 - Improvement: Fix more mustache linter warnings, solves #360.
 * 2023-08-02 - Improvement: Add 'aboutus', 'offers', 'page1', 'page2' and 'page3' static pages, solves #351.
 * 2023-08-19 - Bugfix: Fix unparsable example JSON in Mustache template, solves #348.

diff --git a/classes/table/flavours_overview.php b/classes/table/flavours_overview.php
@@ -69,6 +69,7 @@ public function __construct() {
         $this->pageable(false); // Having a pageable table would be nice, but we will keep it simple for now.
         $this->define_columns($columns);
         $this->define_headers($headers);
+        $this->define_header_column('title');
 
         // Initialize values for the updown feature.
         $this->count = 0;
@@ -97,9 +98,9 @@ public function col_updown($data) {
         if ($this->count > 0) {
             // Add the up icon.
             $updown .= \html_writer::link($actionurl->out(false,
-                            array('action' => 'up', 'id' => $data->id)),
-                            $OUTPUT->pix_icon('t/up', get_string('up'), 'moodle',
-                                    array('class' => 'iconsmall')), array('class' => 'sort-flavour-up-action'));
+                    array('action' => 'up', 'id' => $data->id, 'sesskey' => sesskey())),
+                    $OUTPUT->pix_icon('t/up', get_string('up'), 'moodle',
+                            array('class' => 'iconsmall')), array('class' => 'sort-flavour-up-action'));
 
             // Otherwise, just add a spacer.
         } else {
@@ -111,9 +112,9 @@ public function col_updown($data) {
             // Add the down icon.
             $updown .= '&nbsp;';
             $updown .= \html_writer::link($actionurl->out(false,
-                            array('action' => 'down', 'id' => $data->id)),
-                            $OUTPUT->pix_icon('t/down', get_string('down'), 'moodle',
-                                    array('class' => 'iconsmall')), array('class' => 'sort-flavour-down-action'));
+                    array('action' => 'down', 'id' => $data->id, 'sesskey' => sesskey())),
+                    $OUTPUT->pix_icon('t/down', get_string('down'), 'moodle',
+                            array('class' => 'iconsmall')), array('class' => 'sort-flavour-down-action'));
 
             // Otherwise, just add a spacer.
         } else {
@@ -166,17 +167,46 @@ public function col_appliesto($data) {
     public function col_actions($data) {
         global $OUTPUT;
 
-        // Compose and return the action buttons.
-        return
-            $OUTPUT->single_button(
-                    new \moodle_url('/theme/boost_union/flavours/preview.php', ['id' => $data->id]),
-                    get_string('flavourspreview', 'theme_boost_union'), 'get').
-            $OUTPUT->single_button(
-                    new \moodle_url('/theme/boost_union/flavours/edit.php', ['action' => 'edit', 'id' => $data->id]),
-                    get_string('flavoursedit', 'theme_boost_union'), 'get').
-            $OUTPUT->single_button(
-                    new \moodle_url('/theme/boost_union/flavours/edit.php', ['action' => 'delete', 'id' => $data->id]),
-                    get_string('flavoursdelete', 'theme_boost_union'), 'get');
+        // Initialize actions.
+        $actions = array();
+
+        // Preview.
+        $actions[] = array(
+                'url' => new \moodle_url('/theme/boost_union/flavours/preview.php', array('id' => $data->id)),
+                'icon' => new \pix_icon('i/search', get_string('flavoursedit', 'theme_boost_union')),
+                'attributes' => array('class' => 'action-preview')
+        );
+
+        // Edit.
+        $actions[] = array(
+                'url' => new \moodle_url('/theme/boost_union/flavours/edit.php',
+                        array('action' => 'edit', 'id' => $data->id, 'sesskey' => sesskey())),
+                'icon' => new \pix_icon('t/edit', get_string('flavoursedit', 'theme_boost_union')),
+                'attributes' => array('class' => 'action-edit')
+        );
+
+        // Delete.
+        $actions[] = array(
+                'url' => new \moodle_url('/theme/boost_union/flavours/edit.php',
+                        array('action' => 'delete', 'id' => $data->id, 'sesskey' => sesskey())),
+                'icon' => new \pix_icon('t/delete', get_string('flavourspreview', 'theme_boost_union')),
+                'attributes' => array('class' => 'action-delete')
+        );
+
+        // Compose action icons for all actions.
+        $actionshtml = array();
+        foreach ($actions as $action) {
+            $action['attributes']['role'] = 'button';
+            $actionshtml[] = $OUTPUT->action_icon(
+                $action['url'],
+                $action['icon'],
+                ($action['confirm'] ?? null),
+                $action['attributes']
+            );
+        }
+
+        // Return all actions.
+        return \html_writer::span(join('', $actionshtml), 'flavours-actions');
     }
 
     /**

diff --git a/flavours/edit.php b/flavours/edit.php
@@ -40,6 +40,7 @@
 
 // Access checks.
 require_login();
+require_sesskey();
 require_capability('theme/boost_union:configure', $context);
 
 // Prepare the page.

diff --git a/flavours/overview.php b/flavours/overview.php
@@ -36,67 +36,71 @@
 require_once($CFG->libdir.'/adminlib.php');
 
 // Get parameters.
-$action = optional_param('action', '', PARAM_TEXT);
-$flavourid = optional_param('id', '', PARAM_INT);
+$action = optional_param('action', null, PARAM_TEXT);
+$flavourid = optional_param('id', null, PARAM_INT);
 
 // Get system context.
 $context = context_system::instance();
 
+// Access checks.
+admin_externalpage_setup('theme_boost_union_flavours');
+
 // Prepare the page (to make sure that all necessary information is already set even if we just handle the actions as a start).
 $PAGE->set_context($context);
 $PAGE->set_url(new moodle_url('/theme/boost_union/flavours/overview.php'));
 $PAGE->set_cacheable(false);
 
-// Process sort action.
-if ($action && $flavourid) {
+// Process actions.
+if ($action !== null && confirm_sesskey()) {
+    // Every action is based on a flavour, thus the flavour ID param has to exist.
+    $flavourid = required_param('id', PARAM_INT);
+
+    // The actions might be done with more than one DB statements which should have a monolithic effect, so we use a transaction.
+    $transaction = $DB->start_delegated_transaction();
+
     // Sort 'up' action.
-    if ($action == 'up') {
-        $currentflavour = $DB->get_record('theme_boost_union_flavours', array('id' => $flavourid));
-        $prevflavour = $DB->get_record('theme_boost_union_flavours', array('sort' => $currentflavour->sort - 1));
-        if ($prevflavour) {
-            // The sorting is done with two DB statements which should have a monolithic effect,
-            // so we use a transaction.
-            $transaction = $DB->start_delegated_transaction();
-            $DB->set_field('theme_boost_union_flavours', 'sort', $prevflavour->sort,
-                    array('id' => $currentflavour->id));
-            $DB->set_field('theme_boost_union_flavours', 'sort', $currentflavour->sort,
-                    array('id' => $prevflavour->id));
-            $transaction->allow_commit();
-
-            // Purge the flavours cache as the users might get other flavours which apply after the sorting.
-            // We would have preferred using cache_helper::purge_by_definition, but this just purges the session cache
-            // of the current user and not for all users.
-            cache_helper::purge_by_event('theme_boost_union_flavours_resorted');
-        }
-
-        // Sort 'down' action.
-    } else if ($action = "down") {
-        $currentflavour = $DB->get_record('theme_boost_union_flavours', array('id' => $flavourid));
-        $nextflavour = $DB->get_record('theme_boost_union_flavours', array('sort' => $currentflavour->sort + 1));
-        if ($nextflavour) {
-            // The sorting is done with two DB statements which should have a monolithic effect,
-            // so we use a transaction.
-            $transaction = $DB->start_delegated_transaction();
-            $DB->set_field('theme_boost_union_flavours', 'sort', $nextflavour->sort,
-                    array('id' => $currentflavour->id));
-            $DB->set_field('theme_boost_union_flavours', 'sort', $currentflavour->sort,
-                    array('id' => $nextflavour->id));
-            $transaction->allow_commit();
-
-            // Purge the flavours cache as the users might get other flavours which apply after the sorting.
-            // We would have preferred using cache_helper::purge_by_definition, but this just purges the session cache
-            // of the current user and not for all users.
-            cache_helper::purge_by_event('theme_boost_union_flavours_resorted');
-        }
+    switch ($action) {
+        case 'up':
+            // Move the flavour upwards.
+            $currentflavour = $DB->get_record('theme_boost_union_flavours', array('id' => $flavourid));
+            $prevflavour = $DB->get_record('theme_boost_union_flavours', array('sort' => $currentflavour->sort - 1));
+            if ($prevflavour) {
+                $DB->set_field('theme_boost_union_flavours', 'sort', $prevflavour->sort,
+                        array('id' => $currentflavour->id));
+                $DB->set_field('theme_boost_union_flavours', 'sort', $currentflavour->sort,
+                        array('id' => $prevflavour->id));
+
+                // Purge the flavours cache as the users might get other flavours which apply after the sorting.
+                // We would have preferred using cache_helper::purge_by_definition, but this just purges the session cache
+                // of the current user and not for all users.
+                cache_helper::purge_by_event('theme_boost_union_flavours_resorted');
+            }
+            break;
+        case 'down':
+            // Move the flavour downwards.
+            $currentflavour = $DB->get_record('theme_boost_union_flavours', array('id' => $flavourid));
+            $nextflavour = $DB->get_record('theme_boost_union_flavours', array('sort' => $currentflavour->sort + 1));
+            if ($nextflavour) {
+                $DB->set_field('theme_boost_union_flavours', 'sort', $nextflavour->sort,
+                        array('id' => $currentflavour->id));
+                $DB->set_field('theme_boost_union_flavours', 'sort', $currentflavour->sort,
+                        array('id' => $nextflavour->id));
+
+                // Purge the flavours cache as the users might get other flavours which apply after the sorting.
+                // We would have preferred using cache_helper::purge_by_definition, but this just purges the session cache
+                // of the current user and not for all users.
+                cache_helper::purge_by_event('theme_boost_union_flavours_resorted');
+            }
+            break;
     }
 
+    // Allow to update the changes to database.
+    $transaction->allow_commit();
+
     // Redirect to the same page.
     redirect($PAGE->url);
 }
 
-// Access checks.
-admin_externalpage_setup('theme_boost_union_flavours');
-
 // Further prepare the page.
 $PAGE->set_title(theme_boost_union_get_externaladminpage_title(get_string('flavoursflavours', 'theme_boost_union')));
 $PAGE->set_heading(theme_boost_union_get_externaladminpage_heading());
@@ -115,7 +119,7 @@
 // Prepare 'Create flavours' button.
 $createbutton = $OUTPUT->box_start();
 $createbutton .= $OUTPUT->single_button(
-        new \moodle_url('/theme/boost_union/flavours/edit.php', ['action' => 'create']),
+        new \moodle_url('/theme/boost_union/flavours/edit.php', array('action' => 'create', 'sesskey' => sesskey())),
         get_string('flavourscreateflavour', 'theme_boost_union'), 'get');
 $createbutton .= $OUTPUT->box_end();
 

diff --git a/smartmenus/items.php b/smartmenus/items.php
@@ -32,7 +32,7 @@
 require_once($CFG->libdir.'/adminlib.php');
 
 // Get parameters.
-$action = optional_param('action', null, PARAM_ALPHAEXT);
+$action = optional_param('action', null, PARAM_TEXT);
 $menuid = optional_param('menu', null, PARAM_INT);
 $id = optional_param('id', null, PARAM_INT);
 
@@ -92,23 +92,23 @@
                 \core\notification::success(get_string('smartmenusmenudeleted', 'theme_boost_union'));
             }
             break;
-        case "down":
+        case 'down':
             // Move the item downwards.
             $item->move_downward();
             break;
-        case "up":
+        case 'up':
             // Move the item upwards.
             $item->move_upward();
             break;
-        case "copy":
+        case 'copy':
             // Duplicate the item.
             $item->duplicate();
             break;
-        case "hide":
+        case 'hide':
             // Hide the item from menu.
             $item->update_field('visible', false);
             break;
-        case "show":
+        case 'show':
             // Show the item in menu.
             $item->update_field('visible', true);
             break;

diff --git a/smartmenus/menus.php b/smartmenus/menus.php
@@ -32,7 +32,7 @@
 require_once($CFG->libdir.'/adminlib.php');
 
 // Get parameters.
-$action = optional_param('action', null, PARAM_ALPHAEXT);
+$action = optional_param('action', null, PARAM_TEXT);
 $menuid = optional_param('id', null, PARAM_INT);
 
 // Get system context.
@@ -66,23 +66,23 @@
                 \core\notification::success(get_string('smartmenusmenudeleted', 'theme_boost_union'));
             }
             break;
-        case "down":
+        case 'down':
             // Move the menu downwards.
             $menu->move_downward();
             break;
-        case "up":
+        case 'up':
             // Move the menu upwards.
             $menu->move_upward();
             break;
-        case "copy":
+        case 'copy':
             // Duplicate the menu and its items.
             $menu->duplicate();
             break;
-        case "hide":
+        case 'hide':
             // Disable the menu visibility.
             $menu->update_visible(false);
             break;
-        case "show":
+        case 'show':
             // Enable the menu visibility.
             $menu->update_visible(true);
             break;

diff --git a/tests/behat/theme_boost_union_flavourssettings_application.feature b/tests/behat/theme_boost_union_flavourssettings_application.feature
@@ -41,7 +41,7 @@ Feature: Configuring the theme_boost_union plugin on the "Flavours" page, applyi
     And I should see "Create flavour" in the "#page-header h1" "css_element"
     And I set the field "Title" to "My shiny new flavour"
     And I click on "Save changes" "button"
-    And I click on "Preview" "button" in the "#region-main table" "css_element"
+    And I click on ".action-preview" "css_element" in the "My shiny new flavour" "table_row"
     # Unfortunately, we can't test for the particular flavour ID, we can just check that the class is there.
     Then the "class" attribute of "body" "css_element" should contain "flavour-"
 
@@ -145,7 +145,7 @@ Feature: Configuring the theme_boost_union plugin on the "Flavours" page, applyi
     And I click on "Save changes" "button"
     And I should see "Flavours" in the "#region-main h2" "css_element"
     And I should see "Course categories" in the "Cat 1 flavour" "table_row"
-    And I click on "Edit" "button" in the "#region-main table" "css_element"
+    And I click on ".action-edit" "css_element" in the "Cat 1 flavour" "table_row"
     And I expand all fieldsets
     And I select "No" from the "Apply to course categories" singleselect
     And I click on "Save changes" "button"
@@ -210,7 +210,7 @@ Feature: Configuring the theme_boost_union plugin on the "Flavours" page, applyi
     And I click on "Save changes" "button"
     And I should see "Flavours" in the "#region-main h2" "css_element"
     And I should see "Cohorts" in the "Cohort 1 flavour" "table_row"
-    And I click on "Edit" "button" in the "#region-main table" "css_element"
+    And I click on ".action-edit" "css_element" in the "Cohort 1 flavour" "table_row"
     And I expand all fieldsets
     And I select "No" from the "Apply to cohorts" singleselect
     And I click on "Save changes" "button"

diff --git a/tests/behat/theme_boost_union_flavourssettings_caching.feature b/tests/behat/theme_boost_union_flavourssettings_caching.feature
@@ -92,7 +92,7 @@ Feature: Configuring the theme_boost_union plugin on the "Flavours" page, cachin
     And I am on "Course 1" course homepage
     And I should not see "Course 1" in the "#page-header .page-header-headings" "css_element"
     And I navigate to "Appearance > Themes > Boost Union > Flavours" in site administration
-    And I click on "Edit" "button" in the "#region-main table" "css_element"
+    And I click on ".action-edit" "css_element" in the "Non-effective flavour" "table_row"
     And I click on "span.badge" "css_element" in the "#fitem_id_applytocategories_ids .form-autocomplete-selection" "css_element"
     And I click on ".form-autocomplete-downarrow" "css_element" in the "#fitem_id_applytocategories_ids" "css_element"
     And I click on "Cat 2" item in the autocomplete list
@@ -147,7 +147,7 @@ Feature: Configuring the theme_boost_union plugin on the "Flavours" page, cachin
     And I am on "Course 1" course homepage
     And I should not see "Course 1" in the "#page-header .page-header-headings" "css_element"
     And I navigate to "Appearance > Themes > Boost Union > Flavours" in site administration
-    And I click on "Delete" "button" in the "Effective flavour" "table_row"
+    And I click on ".action-delete" "css_element" in the "Effective flavour" "table_row"
     And I click on "Delete" "button"
     And I am on "Course 1" course homepage
     Then I should see "Course 1" in the "#page-header .page-header-headings" "css_element"