WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 6 weeks ago

#39186 assigned defect (bug)

Bulk actions not correctly applied when selecting bulk actions in both the top and bottom bulk actions dropdowns

Reported by: engelen Owned by: engelen
Milestone: 4.9.1 Priority: normal
Severity: normal Version: 4.7
Component: Quick/Bulk Edit Keywords: has-patch needs-unit-tests early
Focuses: administration Cc:

Description

In WP_List_Table objects, the bulk actions dropdown and "Apply" button is displayed twice: once below the table and once above the table. The name attribute of the bulk actions dropdown element is always set "action". This wouldn't be a problem if the top and bottom actions were two in different forms.

However, both dropdown elements are in the posts-filter-form. You can start to see the problem here. Two form elements both have the same name, which yields unexpected behaviour when trying to apply bulk actions.

Take the following use case, for example:

  • Select some posts from the posts list in /wp-admin/edit.php.
  • Select "Edit" from the bulk actions dropdown above the list table.
  • Select "Move to Trash" from the dropdown below the list table.
  • Click the apply button next to the dropdown on the bottom of the list table.
  • The submit request is sent. You would expect the posts to be moved to the trash, but instead, nothing happens.

Screencast of bug: http://recordit.co/EjHAbw2KNr

The solution I see would rename the dropdowns for the top and bottom dropdowns (they already have different names) and name the "Apply" buttons. Then, when one of the buttons is pressed, execute the corresponding bulk action.

In any case, we shouldn't have any two form elements with the same name in a single form.

Attachments (4)

39186.patch (1.0 KB) - added by chris_dev 12 months ago.
Add name attribute to Apply button and check where the action is coming from
39186.diff (3.5 KB) - added by engelen 12 months ago.
Fix bulk actions problems for all object types (including terms) and fix bulk user promotion
39186.2.patch (849 bytes) - added by bhargavbhandari90 12 months ago.
39186.3.patch (7.3 KB) - added by chris_dev 11 months ago.
Hi, I found an issue on the plugin page so I added it up.

Download all attachments as: .zip

Change History (24)

@chris_dev
12 months ago

Add name attribute to Apply button and check where the action is coming from

#1 @chris_dev
12 months ago

  • Keywords has-patch added

@engelen
12 months ago

Fix bulk actions problems for all object types (including terms) and fix bulk user promotion

#2 @engelen
12 months ago

  • Keywords needs-unit-tests added

@chris_dev Thanks, the patch solves the problem for most objects.

Close inspection reveals that WP_Terms_List_Table overrides get_current_action() from WP_List_Table, and handles bulk deletion of terms wrongly. It doesn't check which "Apply" button is clicked when deleting terms, and deletes the selected term if either of them is clicked and either of the dropdowns was set to "Delete". This wasn't as big a problem as it is since 4.7, when custom bulk actions were introduced. As such, it can lead to unintended permantent deletion of terms. Somewhat major, if I were to judge it.

Furthermore, the same problem exists with user promotion on the users overview. Here, the role selection from the bottom dropdown overrides the role selection from the top dropdown, even if the top apply button is clicked.

Patch "39186.diff" addresses builds on the patch of @chris_dev and addresses the other issues. I would suggest to add (more?) unit tests to the bulk actions functionality, as these problems are bound to pop up more from 4.7 onward.

#3 @bhargavbhandari90
12 months ago

Rather than we can do like that, if any of bulk action dropdown is changed then also change other at the same time using jQuery.

Last edited 12 months ago by bhargavbhandari90 (previous) (diff)

#4 @azaozz
12 months ago

  • Milestone changed from Awaiting Review to 4.8

39186.diff looks good. Wondering if it makes sense to also add 39186.2.patch to show the action that will be performed at both places in the UI. Maybe best to add both?

Last edited 12 months ago by azaozz (previous) (diff)

#5 @chris_dev
12 months ago

Thank you all @engelen @bhargavbhandari90

#6 follow-up: @engelen
12 months ago

@azaozz Even though it's nice to have the actions to be performed reflected properly in the UI, I do not think that there is any ambiguity at this point. From a user perspective, the two dropdowns and buttons (at the top and at the bottom of the list table) are separate forms, and they have little to do with each other. Furthermore, with 4.7, many use cases can be considered where the patch would not be enough.

What would happen, for example, if a user would add custom bulk actions with corresponding additional settings added through jQuery? I have already encountered this use case in my own plugin (screencast of a case that would be difficult and strange to copy to the bottom bulk actions: http://recordit.co/JYcCMR71JN).

Concluding: I wouldn't say copying the current action to the bottom/top bulk actions is desirable behaviour.

And lastly: wouldn't this be more suited for a minor release? Especially the bug where you can permanently delete categories without intending to do so seems to warrant that.

#7 in reply to: ↑ 6 @chris_dev
11 months ago

Replying to engelen:

@azaozz Even though it's nice to have the actions to be performed reflected properly in the UI, I do not think that there is any ambiguity at this point. From a user perspective, the two dropdowns and buttons (at the top and at the bottom of the list table) are separate forms, and they have little to do with each other. Furthermore, with 4.7, many use cases can be considered where the patch would not be enough.

What would happen, for example, if a user would add custom bulk actions with corresponding additional settings added through jQuery? I have already encountered this use case in my own plugin (screencast of a case that would be difficult and strange to copy to the bottom bulk actions: http://recordit.co/JYcCMR71JN).

Concluding: I wouldn't say copying the current action to the bottom/top bulk actions is desirable behaviour.

And lastly: wouldn't this be more suited for a minor release? Especially the bug where you can permanently delete categories without intending to do so seems to warrant that.

@engelen I reckon it is more suitable for a minor release

@chris_dev
11 months ago

Hi, I found an issue on the plugin page so I added it up.

#9 @desrosj
8 months ago

I found this ticket when working on #40113 which is related to changing roles for users in the edit site section of the network admin. Like @engelen detailed above with term bulk edit and user roles, this area could have potentially large negative consequences.

On the edit site users dialogue in the network admin, the logic in the patch on #40113 (which was copied over from the single site user edit screen) will always respect the second dropdown field.

Say a person is at the bottom of the page and selects a role. Then they scroll up selecting users, and then select a different role in the dropdown at the top. The bottom dropdown value would be applied to all selected users. This could be very bad, and a huge pain if a large number of users had been selected. Especially if the user selects Administrator in the bottom dropdown, and then changes their mind at the top to a lesser role. All users would be promoted to Administrators.

To reproduce, select a role in the dropdown at the top of the table, and then select a different role in the bottom dropdown. Click change in either row, and the second dropdown is the value respected.

I think for certain core fields (like role change, bulk edit), it would make sense to make these values always match.

This ticket was mentioned in Slack in #core-multisite by jeremyfelt. View the logs.


6 months ago

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


6 months ago

#12 @jbpaul17
6 months ago

  • Keywords early added
  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per discussion in today's 4.8 rc1 bug scrub in #core.

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


4 months ago

#14 @jbpaul17
4 months ago

  • Milestone changed from 4.8.1 to 4.9

Punting to 4.9 per today's bug scrub.

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


6 weeks ago

#16 follow-up: @melchoyce
6 weeks ago

  • Milestone changed from 4.9 to Future Release

#17 in reply to: ↑ 16 ; follow-up: @engelen
6 weeks ago

Replying to melchoyce:
...really?

#18 in reply to: ↑ 17 ; follow-up: @melchoyce
6 weeks ago

Replying to engelen:

Replying to melchoyce:
...really?

Happy to un-punt if someone wants to take ownership and see it to commit.

#19 in reply to: ↑ 18 @engelen
6 weeks ago

Replying to melchoyce:

Replying to engelen:

Replying to melchoyce:
...really?

Happy to un-punt if someone wants to take ownership and see it to commit.

Sure. If you change the milestone to 4.9.1 and make me the owner I'll see to it that this gets the attention it needs.

#20 @melchoyce
6 weeks ago

  • Milestone changed from Future Release to 4.9.1
  • Owner set to engelen
  • Status changed from new to assigned

@engelen All set :)

Note: See TracTickets for help on using tickets.