Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When we find a spam post, a node admin can delete it but needs a user admin to ban the user. And it takes too many clicks. Esp for book pages. This here adds a checkbox to the node delete form -- one click block.
Comment | File | Size | Author |
---|---|---|---|
#35 | 0001-Issue-146076-Added-Block-the-author-while-deleting-c.patch | 3.96 KB | marvil07 |
#29 | block_author_on_delete_0.patch | 5.49 KB | swentel |
#26 | block_author_on_delete.patch | 4.55 KB | David Strauss |
#25 | block_author_on_delete.patch | 4.6 KB | David Strauss |
#22 | block-users.patch | 3.65 KB | Jody Lynn |
Comments
Comment #1
webchick+100 billion. This would make the workflow for managing drupal.org SO much easier... we could allow people in the site maintainer role to delete content AND users without giving them permissions to view user email addresses and such.
This is a re-roll with a couple small re-wording changes (mentions the user's name when asking whether to delete, drupal_set_message()s that they've been banned, checkbox changed to "Also ban name?").
Comment #2
catchplease, please let this get in.
Comment #3
webchickThis patch adds in the ability to delete authors when deleting comments as well.
Also a couple changes: check first to see if the author's user ID is > 1, as we can't ban the Anonymous user, and we don't want to ban uid 1. :P
Comment #4
webchickdww rightly pointed out that other places in the UI, "ban" is called "block" and this should be consistent. I personally find the word "ban" a much better word, and more consistent with what other software uses, but that's a separate argument for a separate patch. :)
Comment #5
dwwas you can probably tell, i've already looked at earlier versions of this patch and gave my input. ;) that said, the latest is definitely RTBC:
let's commit this to D6. once a core committer confirms this is good, i'd be in favor of a backport to D5 that i could apply to d.o. this would be a huge time saver for the infra team if all the content maintainers could use this to block spam users themselves.
Comment #6
AjK CreditAttribution: AjK commentedSide note: In IRC #drupal it was suggested that this patch be done as a contrib module rather than backport to D5. However, due to the fact that comment.module uses $_POST['confirm'] to capture the confirm form button press in an if() you can't bind an additional submit handler for the comment delete confirm_form() handler as that side steps the entire fAPI submit process (which means you can't capture the "delete" button being pressed).
It's ok for nodes as node.module uses node_delete_confirm_submit() in the normal sense and it's possible to bind an additional submit handler.
So, in it's current form, to get all this functionality into D5 requires a core patch rather than a contrib. Contrib can provide a node solution just not a comment one also. Sorry about that ;)
Comment #7
AjK CreditAttribution: AjK commentedOK, for the record I have a solution for #6 above so I now have a module which does this. Contact me via my contact tab for details if needed.
Comment #8
Gurpartap Singh CreditAttribution: Gurpartap Singh commentedIt seems more of drupal.org's needs than a worth feature.. contrib module would be better place that's my opinion.
Comment #9
catchif it's in a contrib module I'll download it, but it'd be a shame not to have this in core given it's a small amount of code, and so many spammers around ( some bots appear to be programmed to target drupal sites given some forum discussions here), anything which saves a few seconds (although this adds up to hours a year) dealing with that is great.
Comment #10
RobRoy CreditAttribution: RobRoy commentedI agree that this reads more 'contrib' than 'core' even though it would be useful on drupal.org. Since it is perm based though, we can essentially limit who sees this option so I don't have any huge issues with this going into core.
Comment #11
Dries CreditAttribution: Dries commentedWouldn't the integration of the actions module, deprecate this patch, or not really?
Comment #12
jvandyk CreditAttribution: jvandyk commentedNot really. Not any more than the "Make node sticky" action deprecates the Sticky checkbox. An action is a way to do something programmatically that can also be done manually.
Plus, what is being talked about here is manual banning of a user. An action that does this would not be helpful without condition support in actions, which is something that is probably Drupal 7ish.
So, +1 for this patch. But. The patch should also do a DELETE from {sessions} WHERE uid = %d IMO; blocked users should find themselves logged out.
Comment #13
dww@jvandyk: see core's user_save() function:
;)
so, there you have it. Mr. Actions doesn't think this is particularly action-able, everyone wants the patch on d.o, it's so tiny that doing this as contrib seems silly, it could be useful for many sites -- not just d.o, it's been reviewed a bunch, tested, it's secure, etc.
therefore, #4 is RTBC.
thanks,
-derek
Comment #14
David StraussIs there a reason for this to be in core? Couldn't a contrib module use hook_form_alter to add an option to the node deletion confirmation and capture the result in hook_init (or wherever) to ban the author, if requested?
Comment #15
David StraussIf my suggested implementation is too awkward, perhaps we should go forward with certain elements of the Deletion API to allow convenient additions to deletion forms.
Another option for contrib would be developing a sort of vandalism control center that adds a link to the deletion confirmation page to a more complete form offering tools to wipe out the user and his or her posts and comments.
Comment #16
moshe weitzman CreditAttribution: moshe weitzman commentedit should be in core because it is a common operation for a community site, which is what we offer at drupal.org. yes it could be done in contrib, but everyone working here wants to improve the experience for people right out of the box.
Comment #17
asimmonds CreditAttribution: asimmonds commentedI reckon this a big usability improvement for social networking sites, hopefully it could still be considered for 6.x
I've updated the patch from #4 to current HEAD
Comment #18
asimmonds CreditAttribution: asimmonds commentedRe-rolled for HEAD
Comment #19
Freso CreditAttribution: Freso commentedNot all of the patch applies, and none cleanly:
So I re-rolled and tested the new patch, though not thoroughly, and am convinced this still works. So if someone would care to give this a more thorough review... ? Or should this be marked for D7 by now?
Comment #21
catchNo longer applies. Much as I love this patch, if it was going to get into D6, it would've happened by now.
Comment #22
Jody LynnRerolled.
Comment #23
swentel CreditAttribution: swentel commentedStill applies, great feature, works as intented, let's get this in !
Comment #24
Dries CreditAttribution: Dries commentedThere are some minor code-style issues in the patch (i.e. tabs instead of spaces).
I'm not sure we need a new permission for this. There is already a permission called "Administer users" with the following description: "Manage or block users, and manage their role assignments.". I think we should stick with the existing permission and keep things simple.
Personally, I like to _delete_ spammers rather than blocking them. I guess blocking them would be a useful first step so I'm supportive of this patch.
Comment #25
David StraussHere's a revised patch with all the code style issues I could find by visual inspection fixed.
It appears that user_perm() has style issues in CVS, and I've updated this patch to fix them as well as add the "block users" permission item.
I believe that we should add a new permission for "block users" so that content administrators can have the ability to block spammers when they delete content without also needing to be full site admins (or capable of taking such control). You don't necessarily want content administrators to have full user administration capabilities. Compared to the capabilities provided by "administer users", the capabilities provided by "block users" are easy to reverse if they're abused, and there's no ability to escalate your privileges with it.
I have extended every case of user_access('block users') to accept the broader "administer users" permission.
Comment #26
David StraussTiny update to restore the array initialization strangely removed in the patch.
Comment #27
PanchoI second David's argumentation why we should add a new permission for this. See also webchicks first comment on this - that's an obvious use case.
Comment #29
swentel CreditAttribution: swentel commentedChasing HEAD and requesting testbot review.
Comment #30
catchI agree with the extra permission for the same reason we separated edit and delete permissions for nodes - blocking is non-destructive and easily reversed so there are plenty of use cases for giving it separately. Visually the code looks fine, so RTBC.
Comment #31
sunI would highly suggest to rethink the implementation of this patch after #8: Let users cancel their accounts has landed. Slight changes to this patch would allow moderators/administrators to cancel the user account (i.e. not only block the user) by invoking the new cancel account process. Having all cancel account options would be extremely helpful in cases where you encounter malicious posts or spam, i.e. when you know that you could also simply and immediately delete the user and all his contents.
Also, this patch does not allow to skip the notification mail that is automatically sent by user_save() upon blocking an account (if the notification is enabled on admin/user/settings), which is also covered in the new cancel account process.
Comment #32
David StraussSetting to CNW now that #8: Let users cancel their accounts has landed.
Comment #33
sunComment #34
vimalramaka CreditAttribution: vimalramaka commented#22: block-users.patch queued for re-testing.
Comment #35
marvil07 CreditAttribution: marvil07 commentedNon-reworked patch, just a re-roll on tip of 8.x to let apply again.
Comment #36
marvil07 CreditAttribution: marvil07 commentedSo, I finally read the related patch about cancel accounts, and now I understand the CNW
Comment #37
NancyDruFive years since this was proposed and two years since it was moved to 8.x. Isn't it time to commit or kill?
Comment #38
chx CreditAttribution: chx commented