Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
When you have to moderate many spam comments the typical workflow is as follows:
- Go to
admin/content/comment/approval - Mass select comments (e.g. all on the page)
- Select Delete the selected comments in the dropdown
- Click Update
- On the confirmation page flag them e.g. as spam and finally click on Delete comments button to complete the action
You'll now get redirected to admin/content/comment. From an usability standpoint, this is very prone to errors as you might not have realized you're back to the approved comments pane and can then repeat the operation to delete legit comments this time instead of continuing to delete spam comments.
Proposed resolution
Change the unapproved comments deletion confirmation page to redirect to admin/content/comment/approval as long as we have unapproved comments.
Remaining tasks
Write a patch.
User interface changes
Confirmation page after mass deleting of unapproved comments now redirects to admin/content/comment/approval if the number of unapproved comments is superior to 0
API changes
None.
Data model changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | interdiff.txt | 548 bytes | eporama |
| #10 | mass_comment_moderation-2680133-10.patch | 506 bytes | eporama |
| #7 | mass_comment_moderation-2680133-7.patch | 526 bytes | gnosis |
Comments
Comment #3
gnosis CreditAttribution: gnosis at Socha Dev commentedI will work on this.
My intention is to make the redirect back to /admin/content/comment/approval in all cases, as this will display the "nothing here" copy on the "unapproved comments" list. The ensures the moderator that he/she has finished the queue.
Comment #4
gnosis CreditAttribution: gnosis at Socha Dev commentedComment #5
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedGnosis, now that nola2016 is in the books, did we make any progress on this? I don't want to step on toes if that's the case.
Comment #6
gnosis CreditAttribution: gnosis at Socha Dev commentedYes, I did solve the issue on Friday, but had to run to catch plane. I'm back at the office now and should be able to post a patch today.
Comment #7
gnosis CreditAttribution: gnosis at Socha Dev commentedThis issue actually occurs not just in the redirects, but also in the Cancel buttons. It happens whether you are on the "Published Comments" tab (
comment.admin) or on the "Unapproved Comments" tab (comment.admin_approval), because these share the same code. That is,\Drupal\comment\Form\ConfirmDeleteMultipleis used for the bulk op either way.Currently, the method
getCancelUrl()is hard-coded to return routecomment.admin, which is the "Published Comments" tab. The confirm submit handler completes by redirecting togetCancelUrl(). The result is that regardless of what tab you're on, and whether you complete the action or not, you're always going to end up back atcomment.admin.The solution in this patch is to change
getCancelUrl()to return whichever route the user was already on rather than hard-code it. I have tested this and it solves the problem for both the Cancel buttons and the submit redirect: the user ends up back where they were instead of forced to the default task/tab.Comment #9
gnosis CreditAttribution: gnosis at Socha Dev commentedHm. Looks like my solution fails because there is no HTTP request in the test context. It seems like it should be possible to insert a condition to check for it and return the previous hard-coded route if no request exists, but I don't know if that's acceptable.
I'm not sure how this usability issue could be resolved without some way to know which task/tab the user was on when they initiated the bulk op.
Oddly, when I run the phpunit group comment on my local, with this patch, all tests pass. Not sure what the difference is there. But it does make some sense that no request object would be present in the test env.
Comment #10
eporama CreditAttribution: eporama as a volunteer and at Acquia commentedI went through the same process and originally wanted to simply remove the call to
setRedirectUrl()completely withinsubmitForm, but that led to a lot of&d=admin/comment/approvalquerystrings.I think that it would work to invoke
routeMatch()instead ofrequest().I have attached a new patch and interdiff for testing.
Comment #11
anavarreIf I'm not mistaken, I think we should use dependency injection if at all possible in class files.
Comment #12
gnosis CreditAttribution: gnosis at Socha Dev commentedPatch in #10 seems perfect to me. Tested in the GUI and it works as expected; it does the same thing as my original patch, but in a better way that passes automated tests.
Comment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.
Tagging for needs tests.
Will also tend to agree with #11 that dependency injection is best practice.
Moving back to NW for those 2 things.