Make WordPress Core

Opened 8 years ago

Last modified 22 months 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's profile engelen Owned by: engelen's profile engelen
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Quick/Bulk Edit Keywords: has-patch needs-unit-tests early needs-testing
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 8 years ago.
Add name attribute to Apply button and check where the action is coming from
39186.diff (3.5 KB) - added by engelen 8 years ago.
Fix bulk actions problems for all object types (including terms) and fix bulk user promotion
39186.2.patch (849 bytes) - added by bhargavbhandari90 8 years ago.
39186.3.patch (7.3 KB) - added by chris_dev 8 years ago.
Hi, I found an issue on the plugin page so I added it up.

Download all attachments as: .zip

Change History (28)

@chris_dev
8 years ago

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

#1 @chris_dev
8 years ago

  • Keywords has-patch added

@engelen
8 years ago

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

#2 @engelen
8 years 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
8 years ago

Rather than we can do like that, if any of bulk action dropdown is changed then also change other.

Version 0, edited 8 years ago by bhargavbhandari90 (next)

#4 @azaozz
8 years 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 8 years ago by azaozz (previous) (diff)

#5 @chris_dev
8 years ago

Thank you all @engelen @bhargavbhandari90

#6 follow-up: @engelen
8 years 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
8 years 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
8 years ago

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

#8 @desrosj
8 years ago

Related #40113

#9 @desrosj
8 years 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.


8 years ago

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


8 years ago

#12 @jbpaul17
8 years 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.


7 years ago

#14 @jbpaul17
7 years 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.


7 years ago

#16 follow-up: @melchoyce
7 years ago

  • Milestone changed from 4.9 to Future Release

#17 in reply to: ↑ 16 ; follow-up: @engelen
7 years ago

Replying to melchoyce:
...really?

#18 in reply to: ↑ 17 ; follow-up: @melchoyce
7 years 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
7 years 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
7 years ago

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

@engelen All set :)

#21 @johnbillion
7 years ago

  • Milestone changed from 4.9.1 to 5.0

#22 @peterwilsoncc
6 years ago

  • Milestone changed from 5.0 to Future Release

Switching milestone due to the focus on the new editor (Gutenberg) for WordPress 5.0.

#23 @oglekler
22 months ago

  • Keywords needs-testing added

I managed to reproduce this issue only by changing value of button select by request, otherwise it is handling correctly because changes of both selects now synchronized. But JS script is still picking up a value from the first selector, and it isn't correct behavior for the script. And this patch is only applied to backend. I assume that JS script which is making this action needs to be tested and possibly JS behavior to be improved via additional ticket.

Last edited 22 months ago by oglekler (previous) (diff)

This ticket was mentioned in Slack in #core-test by oglekler. View the logs.


22 months ago

Note: See TracTickets for help on using tickets.