Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#46872 closed defect (bug) (fixed)

Marry the Bulk Actions and Change Role controls on list tables via Javascript to avoid conflicts in submission when each has a unique selection.

Reported by: clayray's profile clayray Owned by: johnbillion's profile johnbillion
Milestone: 5.7 Priority: normal
Severity: normal Version: 2.8
Component: Quick/Bulk Edit Keywords: has-patch early has-dev-note
Focuses: ui, administration Cc:

Description (last modified by garrett-eclipse)

I have selected a number of plugins to deactivate. Then I choose Deactivate from the dropdown BELOW the plugins list and then click Apply, I am taken to an "Updates" screen where the plugins get updated.
This does not happen if I use the dropdown and apply button ABOVE the plugins list.
(By "trunk" below, presumably the current version 5.1.1 is meant, since 5.1.1 isn't otherwise selectable via the Dropdown)

Original Title: Bulk Actions underneath plugins list perform Update when Deactivate chosen

Attachments (7)

46872.diff (989 bytes) - added by garrett-eclipse 5 years ago.
Initial working patch to apply the doaction[2] string as the name attribute on the submit input. This will allow us to know which button was clicked.
46872.2.diff (948 bytes) - added by garrett-eclipse 4 years ago.
Updated to avoid breaking installs/activations from the plugin repository.
46872.3.diff (8.2 KB) - added by garrett-eclipse 4 years ago.
Initial patch of alternate JS marriage of controls.
46872.4.changeroles.diff (11.5 KB) - added by garrett-eclipse 4 years ago.
Update patch to cover Change Roles dual selectors on Users screen.
7e363db528c2f95c44435751b89f4a67.gif (584.2 KB) - added by garrett-eclipse 4 years ago.
Married Selectors for Bulk Actions and Change Roles.
46872.5.diff (11.5 KB) - added by Hareesh Pillai 3 years ago.
Adding the patch refreshed against trunk.
46872.4.diff (9.9 KB) - added by johnbillion 3 years ago.

Download all attachments as: .zip

Change History (47)

#1 @subrataemfluence
5 years ago

  • Keywords reporter-feedback added

I did a fresh 5.2 beta install and tried to reproduce the issue by first activating the bundled plugins Akismeta and Hello Dolly and then Bulk deactivating. I could not get into any issue.

If possible please provide the exact steps you are using in order to reproduce the issue.

Thank you!

#2 @SergeyBiryukov
5 years ago

  • Focuses ui administration added

#3 @garrett-eclipse
5 years ago

  • Keywords needs-patch added; reporter-feedback removed

Hi @clayray,

Correct me if my assumption is wrong but was the top select possibly set to 'Update' when you used the bottom one?

I believe I've reproduced your issue in trunk with one clarifying nuance, setting the top Bulk Select action to Update and then going to use the bottom action.

This seems to force the update action on undesireable plugins, even ones which don't have any available updates. Not sure if that overwrites them with what's on wp.org but if so that could replace customized code unnecessarily (not that you should customize plugins directly...).

@subrataemfluence with this nuance in mind can you retest and double confirm for me. Simply add one step prior to your existing tests;

  1. Change the top 'Bulk Action' selector to 'Update'

Testing with other options set in the top Bulk Action selector it seems you can unintentionally trigger all of them. This means you can accidentally trigger Delete action (thankfully there's a confirm page).

#4 @garrett-eclipse
5 years ago

  • Keywords dev-feedback added

Looking into the code it appears this issues comes from the parent WP_List_Table class in wp-admin/includes/class-wp-list-table.php here;
https://github.com/WordPress/WordPress/blob/7a617078fa7674f2062379b961daba412875ab9e/wp-admin/includes/class-wp-list-table.php#L492-L498

<?php
if ( isset( $_REQUEST['action'] ) && -1 != $_REQUEST['action'] ) {
        return $_REQUEST['action'];
}
if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
        return $_REQUEST['action2'];
}

So if the first bulk selector is set it's action takes precedence irregardless of the secondary action.

There's a few avenues we can take to resolve this.

  1. Tie the two selectors together so changes to one apply to the other meaning they'll always submit the same value.
  2. Depending on which Apply is used unset the other bulk action selector so it's value being submitted is -1 allowing the second action to act.
  3. Introduce a hidden input which gets the currently acted upon selectors value mapped to it just before submit.

Adding dev-feedback to get input on the above.

#5 @garrett-eclipse
5 years ago

  • Component changed from Plugins to Quick/Bulk Edit

Moving to 'Quick/Bulk Edit' as the issue pertains to all List Tables and not just Plugins.

#6 @subrataemfluence
5 years ago

@garrett-eclipse I should have been more elaborate when tested. You are right. Your way allowed me to reproduce the issue.

My proposal is that if we can rewrite the current function in the following way, it will solve the issue.

<?php
public function current_action() {
  if ( isset( $_REQUEST['filter_action'] ) && ! empty( $_REQUEST['filter_action'] ) ) {
    return false;
  }
                
  $action1 = '';
  $action2 = '';
                
  if ( isset( $_REQUEST['action'] ) && -1 != $_REQUEST['action'] ) {
    $action1 = $_REQUEST['action'];
  }

  if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
    $action2 = $_REQUEST['action2'];
  }
                
  if( $action1 === $action2 ) {
    return $action1;
  }
                
  return false;
}

However, we need to have a proper AdminNotice, something like

You have selected different actions. Operation cannot be performed.

#7 @subrataemfluence
5 years ago

There is a logical issue in my above code. Please disregard. I am looking into it.

#8 @subrataemfluence
5 years ago

Modified code:

<?php
public function current_action() {
     if ( isset( $_REQUEST['filter_action'] ) && ! empty( $_REQUEST['filter_action'] ) ) {
          return false;
     }
        
     if ( isset( $_REQUEST['action'] ) && -1 != $_REQUEST['action'] ) {
          if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
               if( $_REQUEST['action'] === $_REQUEST['action2'] ) {
                    return $_REQUEST['action'];
               }
                
               return false;
          }
            
          return $_REQUEST['action'];
     }
        
     if ( isset( $_REQUEST['action2'] ) && -1 != $_REQUEST['action2'] ) {
          return $_REQUEST['action2'];
     }
        
     return false;
}

@garrett-eclipse
5 years ago

Initial working patch to apply the doaction[2] string as the name attribute on the submit input. This will allow us to know which button was clicked.

#9 @garrett-eclipse
5 years ago

  • Keywords has-patch needs-testing added; needs-patch dev-feedback removed

Thanks @subrataemfluence I appreciate the logic suggestions for for current_action but I feel we can accomplish this even simpler by applying a unique name attribute to the submit inputs as that will then make available to the $_REQUEST object which submit button was actually used.

With that in mind can you test my simple patch 46872.diff.

It should also avoid submitting action1 when action2 isn't present but the doaction2 button is clicked.

Thoughts?

#10 @SergeyBiryukov
5 years ago

  • Version changed from trunk to 2.8

Introduced in [11474]. Related: #28555

@garrett-eclipse
4 years ago

Updated to avoid breaking installs/activations from the plugin repository.

#11 @garrett-eclipse
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to garrett-eclipse
  • Status changed from new to accepted

After some testing I found my original patch broke activations from the plugin repository and the link 'Activate' in the plugin actions links. Definitely not optimal. In 46872.2.diff I've addressed this issue moving the check for the second action first and removing the rigidness of the second check in order to allow non-bulk actions.
Moving into 5.5 for testing and review.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#13 follow-up: @whyisjake
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In [48134]:

Quick/Bulk Edit: Ensure the proper actions is triggered when using the bulk updater.

If a user selects the top option, then chooses a different option, the top selection takes precedence. This update gives a new name to the bottom action, ensuring the proper update is carried out.

Fixes #46872.

Props clayray, garrett-eclipse, subrataemfluence.

#14 in reply to: ↑ 13 @subrataemfluence
4 years ago

Thanks!
Replying to whyisjake:

In [48134]:

Quick/Bulk Edit: Ensure the proper actions is triggered when using the bulk updater.

If a user selects the top option, then chooses a different option, the top selection takes precedence. This update gives a new name to the bottom action, ensuring the proper update is carried out.

Fixes #46872.

Props clayray, garrett-eclipse, subrataemfluence.

#15 @SergeyBiryukov
4 years ago

In 48866:

Quick/Bulk Edit: Revert [48134] to address the bottom "Bulk actions" dropdown not functioning properly on Posts and Users list tables.

A better solution for the original issue will be explored in a future release.

Props audrasjb, garrett-eclipse, webzunft, Krstarica, chunkysteveo, SergeyBiryukov.
Fixes #50882, #50998. See #46872.

#16 @SergeyBiryukov
4 years ago

In 48867:

Quick/Bulk Edit: Revert [48134] to address the bottom "Bulk actions" dropdown not functioning properly on Posts and Users list tables.

A better solution for the original issue will be explored in a future release.

Props audrasjb, garrett-eclipse, webzunft, Krstarica, chunkysteveo, SergeyBiryukov.
Merges [48866] to the 5.5 branch.
Fixes #50882, #50998. See #46872.

#17 @SergeyBiryukov
4 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Milestone changed from 5.5 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening per the commits above, as [48867] was reverted due to a regression in #50882 and #50998.

During the 5.5.1 bug scrub on Slack, @johnbillion and @garrett-eclipse suggested introducing some JS to sync the chosen options, then we can probably get rid of checking $_REQUEST['action2'].

Version 0, edited 4 years ago by SergeyBiryukov (next)

@garrett-eclipse
4 years ago

Initial patch of alternate JS marriage of controls.

#18 @garrett-eclipse
4 years ago

  • Keywords has-patch needs-testing needs-dev-note added; needs-patch removed
  • Milestone changed from Future Release to 5.6

If at first you don't succeed, Try, try, try again.

I've uploaded an initial patch 46872.3.diff to take the alternate approach of marrying the bottom bulk action controls with the top ones and removing the submission of action2 ensuring we're always working off action without confusion.

As this is a JS approach I've disabled the bottom controls on .no-js. I left them in the dom instead of injecting the controls as it felt less impactful.

Testing through the list tables in core (Posts, Pages, Media, Comments, Plugins, Users, Export and Erasure) I did find a few nuances requiring me to stop propagation in order to avoid duplicate confirm alerts as well as string some action2 handling in the PHP.

Do note there remains some references to action2 in core as they're unrelated to this specific change and would break their function. These currently are;

  1. Handling the multisite confirm action it uses action2 along with action to pass it's request through via URL (not bulk actions). This is in; /wp-admin/includes/class-wp-ms-sites-list-table.php & /wp-admin/network/sites.php
  2. Some PHPUnit testing framework for action hook testing found in; /tests/phpunit/includes/utils.php & /tests/phpunit/tests/actions.php
  3. Some references in the @wordpress npm package; @wordpress/data/src/namespace-store/test/index.js and @wordpress/data/src/test/registry.js

This is working pretty nicely for me right now, would love some review/testing if you have time @johnbillion @SergeyBiryukov @whyisjake

P.S. For now I only hide the bottom bulk actions but left the bottom actions container visible for plugins use. We may want to look at making this work agnostically so any bottom control is married to it's primary counterpart.

#19 @garrett-eclipse
4 years ago

  • Keywords dev-feedback added

@garrett-eclipse
4 years ago

Update patch to cover Change Roles dual selectors on Users screen.

@garrett-eclipse
4 years ago

Married Selectors for Bulk Actions and Change Roles.

#20 @garrett-eclipse
4 years ago

Doing a bit more testing it felt odd to leave the Change Role bottom action untouched so 46872.4.changeroles.diff updates the patch to also marry the top+bottom Change Role controls on the Users table.

#21 @garrett-eclipse
4 years ago

  • Description modified (diff)
  • Summary changed from Bulk Actions underneath plugins list perform Update when Deactivate chosen to Marry the Bulk Actions and Change Role controls on list tables via Javascript to avoid conflicts in submission when each has a unique selection.

#22 @garrett-eclipse
4 years ago

  • Keywords early added

Let's address this early in order to raise exposure and back-compat issues that can arise with plugins when we change UI conventions.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by francina. View the logs.


3 years ago

#26 @johnbillion
3 years ago

I wonder if #19278 could solve this in a different way. Switch to one bulk action dropdown and use an optgroup for the change role options.

#27 @johnbillion
3 years ago

Ah no, this connection in JS would still be needed regardless.

#28 @johnbillion
3 years ago

  • Milestone changed from 5.6 to Future Release

Punting as this is an early.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

#30 @hellofromTonya
3 years ago

  • Milestone changed from Future Release to 5.7

Moving into 5.7 milestone per scrub and John's note above.

#31 @pbiron
3 years ago

Did some initial testing of 46872.4.changeroles.diff today. Everything seems to work as expected. I'll do some more testing tomorrow.

Also, while the patch still applies, it could use a refresh, because the line numbers no longer line up with trunk.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


3 years ago

This ticket was mentioned in Slack in #core by metalandcoffee. View the logs.


3 years ago

@Hareesh Pillai
3 years ago

Adding the patch refreshed against trunk.

#34 @johnbillion
3 years ago

  • Owner changed from garrett-eclipse to johnbillion
  • Status changed from reopened to accepted

@johnbillion
3 years ago

#35 @johnbillion
3 years ago

This is looking good. In 46872.4.diff I have:

  • Fixed a PHP syntax error in wp-admin/network/sites.php
  • Converted the IIFE into a function that accepts selectors, to remove the duplication between the "Bulk actions" and "Change roles" functionality
  • Reverted the removal of action2 from the remove_query_arg() call before doing a redirect, just in case a plugin is still doing things with action2

I tested this on all screens that contain list tables, on single site and Multisite, with JS enabled and disabled. All working nicely.

#36 @johnbillion
3 years ago

  • Keywords needs-testing dev-feedback removed

#37 @johnbillion
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 49944:

Quick/Bulk Edit: By the power vested in me, I hereby declare the top bulk actions and the bottom bulk actions joined forever in MatrimonyScript.

This joyous marriage means that users will no longer find a selected top bulk action on a list table unexpectedly being applied instead of their selected bottom bulk action. The top and bottom controls for changing user roles are equally wedded forever too.

Props clayray, subrataemfluence, garrett-eclipse, pbiron, hareesh-pillai

Fixes #46872

#38 @hellofromTonya
3 years ago

Please provide need more information for testers to manually test the patch (including at Test Scrubs):

  • What are the steps to reproduce the problem?
  • What are the steps to test?
  • Are there any testing dependencies such as a plugin or script?
  • What is the expected behavior after applying the patch?
Last edited 3 years ago by hellofromTonya (previous) (diff)

#39 @hellofromTonya
3 years ago

  • Keywords needs-testing-info added

#40 @audrasjb
3 years ago

  • Keywords has-dev-note added; needs-dev-note needs-testing-info removed
Note: See TracTickets for help on using tickets.