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 | Owned by: | 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)
Change History (28)
@
8 years ago
Fix bulk actions problems for all object types (including terms) and fix bulk user promotion
#2
@
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
@
8 years ago
Rather than we can do like that, if any of bulk action dropdown is changed then also change other.
#4
@
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?
#6
follow-up:
↓ 7
@
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
@
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
#9
@
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
@
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
This ticket was mentioned in Slack in #core by melchoyce. View the logs.
7 years ago
#20
@
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 :)
#22
@
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
@
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.
Add name attribute to Apply button and check where the action is coming from