"Post comments without approval" permission name is completely misleading

I can confirm, there is also something else weird going on. If you are able to post the first comment, the next time you can only reply on the existing comment, not add a new one.

I gave this a once over, and it appears to me that it's an issue with the links that show up underneath the node.

On a fresh install I made a node, and gave anonymous users the permission to post comments without permission, I'm able to post the first comment, like #1 says, and can only post additional comments by going straight to the url: /comment/reply/1#comment-form since the "reply" link doesn't appear.

I tracked it down to a bug in Garland's CSS where the comments div would slide over the 'links' div. That's why the 'add a comment' link is visible when a node has no comments.

I've made a patch that fixes this in style.css.

Okay.

The CSS change is indeed a seperate issue. The HTML is all there. The code works. It's just the CSS that obscures the 'add a comment' link making it only possible to reply to an existing comment.

I've taken another look at the original issue. The code only defines user_access('post comments') in the comment.module ignoring the 'post comments without approval' permission. i've changed it and tested it. Looks like it works for me. I can post a comment as an anonymous user with the 'post a comment without approval' permission set.

Attached is a patch.

Weird. I *did* change the status to 'needs review'.

I was always under the impression this permission meant 'bypass approval for comments' - and required 'post comments' permissions as well. It's probably better this way though.

Doesn't seem like this would only be limited to anonymous users - also it's something we could write a test for.

The thought occurred to me that those two permissions are a bit confusing. 'Post comments' actually stands for two things:

1. able to make a comment
2. you're comment needs to be approved by someone with 'administer comment' permission before it is published.

Whereas 'Post comments without approval' means:

1. able to make a comment
2. you're comment will be published right away

The problem is that both are in fact mutually exclusive. When you make a module, you can code in such a way that certain permissions take precedence over others. That's what's happening here: If you give a role the permission 'Post comments without permission' the 'Post comments' permission will be ignored. Problem is that there is no way to show this behavior in a clear way to the end user when he configures permissions. A good alternative would be to uncheck and grey out the 'Post comments' permission if the 'Post comments without approval' is checked for a role.

Two permissions for the very same action is a 100% no-go. Permissions are cumulative. Having two permissions that do more or less the same will increase and NOT reduce confusion.

Change the permission name and/or clarify the description instead.

And, this is not critical.

It will be good to consider #593670: User who cannot post comments without approval optionally should be notified in advance. while fixing this issue.

delete me please...wrong post

Yup. this is not a bug in permission logic, but confusion caused by a poorly named permission. (Creating a ux issue that appears to be a bug)

If you look at comment_permission(), the permission “post comments” ('machine name') is given the title “Post comments with approval”. This change that was made from 6->7. (Along with the ability to give permissions titles, the 'machine name' doesn't change between versions.)

It is listed next to the permission “Post comments without approval” and that's where the confusion starts.

“Post comments with approval” acts as a "Master Switch" to allow the posting of comments by users in a role. A user who has the permission “post comments without approval” and does not have the permission “Post comments with approval” is NOT able to post comments, although the permission title implies they can.

The permissions don't function according to the way the permission titles read. They need to be changed to match the logic that hasn't changed from D6.

In my opinion, the patch in #6 (which needs a reroll by now and would probably make more sense changing the 'machine name' of the 'post comments' permission) makes the comment system function the way an average user would be inclined to use those permissions. Users would be able to “Post comments without approval”

Since that idea was rejected, here's a patch that changes the title of “Post comments with approval” to “Post comments” (as it was in previous versions of Drupal). And adds some descriptions to these permissions, hopefully explaining to a site administrator how they function, and avoiding any confusion.

It will involve a string change, but hopefully will help a new user to understand these permissions.

#18: comment-438224-18.patch queued for re-testing.

test bot liked it this time.

I like the proposed change in #18. I've changed the machine name of 'post comments' to 'post comments with approval' and made changes in my own patch (#6) accordingly.

However.

I've noticed that not only 'post comments with approval' originally acts on 'post comments without approval' as a master switch, there's also an issue with the 'access comments' permission.

I've tried this:

1. rerolled my original changes in #6 where I decouple 'post comments' and 'post comments without approval' to use a different, more semantic machine/human readable name.

2. assigned the 'post comments with approval' permission to the Anonymous user without checking the 'access comments' permission.

The end result: you'll see a link 'add a comment' in the full node view, but the 'add a comment' form itself isn't displayed. This is due to a check on 'access comments' on line 689 in comment.module.

Logically: 'access comments' and and 'post comments with approval' are not mutually exclusive. If you want to enable the latter, you have to enable the former.

As far as I can see: Rather then changes the naming of 1 or 2 permissions to clarify their usage, the actually way in which these permissions act on the comment system is subject to debate. I for one would try to decouple those permissions.

The current permission system in Drupal just doesn't support this kind of complex behaviour in a decent, usable fashion. The trouble to get it right here just shows the limitations of the current permission system in Drupal.

Rather then keep fighting the original issue here, I would advise closing this one and opening a new issue in which we extend the scope and put all permissions in the comment module under review.

Noticed these two issues:

#52151: Create permission dependencies ("post comments without approval" should set "access comments")
#364159: Enable 'access comments' permission for anonymous users by default.

Looks like this is a cluster of issues which can probably be solved by a common solution.

Or...those are separate issues, and the patch in 18 can fix (clarify) the bug reported by the OP.

Tested and reviewed proposed patch in 18. Works as described. This is a UX improvement for sure and is quite trivial. Suggest commit as well. Without hierarchical permissions (nogo in D7) this is a sane improvement.

Works, but I'd suggest to rename the title of 'Post comments without approval' to 'No approval needed' as the old name still suggest that checking this option alone should work. You have to read the description first to understand the working. See my patch (didn't see this thread...) for my suggestion.

#782980: Comment permissions naming not clear

Hm. I like the patch in #782980: Comment permissions naming not clear

Getting the descriptions right makes most sense at the moment.

I don't care what color we paint the bike shed.

I just stumbled on this problem in alpha 5 and must say it is really confusing and even if the text strings are frozen for D7 an exception has to be made for this.

As sun says in #12 the to post comments... conflicts with each other. The naming in the patch mentioned in #30 is for me the wrong logic. For me its more logical to set the roles that shall have approval, not those who shouldent.

Better would be to simply change them to something like:

- Post comments
- Comment approval needed

That would make it very clear, avoid confusion and make it easier to manage.

even if the text strings are frozen

They aren't.

fwiw #799998: "post comments" permission does not work without "access comments" addresses the issue described in #23 and could use some reviewers.

#18: comment-438224-18.patch queued for re-testing.

Please fix this....
Caused me a headache this morning...

Patch in #18

+++ modules/comment/comment.module	2 Apr 2010 02:19:55 -0000
@@ -384,10 +384,12 @@ function comment_permission() {
-      'title' => t('Post comments with approval'),
+      'title' => t('Post comments'),
+      'description' => t('Users may comment on site content.'),

I think the main fix for this issue already is the removal of "with approval".

The added permission description only repeats the title and serves no value of its own. It can be removed.

+++ modules/comment/comment.module	2 Apr 2010 02:19:55 -0000
@@ -384,10 +384,12 @@ function comment_permission() {
       'title' => t('Post comments without approval'),
+      'description' => t('Users who have the “Post comments” permission enabled can post comments without administrator moderation.'),

1) We use simple double-quotes only.

2) This can be shortened. Perhaps this way, but probably better:

"Users can %post-comments without moderation approval."

or

"New comments posted do not go into moderation."

or similar.

Powered by Dreditor.

This still doesn't solve the problem. It is not clear to me that, if I enable 'Post comments without approval', I should have the 'Post comments' checked as well. I think we need to change the title of the second checkbox, or think of a really clear description.

+++ modules/comment/comment.module	18 Jul 2010 19:12:08 -0000
@@ -388,10 +388,11 @@ function comment_permission() {
+      'title' => t('Post comments'),
...
       'title' => t('Post comments without approval'),
+      'description' => t('New comments posted do not go into moderation.'),

Alright. We have two permissions A and B.

Permission B depends on A and does nothing on its own.

Permission A technically does something unnatural by default - any other permission of this kind would not lead to unpublished content.

Permission B alters the default behavior of the process surrounding permission A to be more like similar processes.

--

Technically, one could say that entirely flipping this complete thing upside-down, disregarding what we previously had and already know, could and potentially would solve the problem space correctly:

1) Make "Post comments" what "create article content" would do: just create that post, published, and

2) Have a "Unpublish new posts" "permission" (all of this being permissions technically is an abuse on its own...), which actually performs the naturally unexpected additional action of unpublishing all new comments of users that were not granted this permission.

That, however, would imply a user permission with negated meaning: It basically "disallows" you to post published comments. If that remotely makes sense.
Furthermore, a negated permission does not map to the mental model of all other permissions.

In general, however, I have to insist on this permission being an abuse of permissions. If actions and trigger/rules module would be more mature in core, then this would clearly be a conditionally executed action for new comments:
if ($comment->uid == 0) then $comment->status = unpublished

--

We need to resolve this in the already given direction. But, perhaps, we can draw the line up to the point, instead of building associations to "approval" and "moderation"?

Permission A: Post comments

Permission B: Publish new comments

(alternatively: Publish posted comments [to make the relationship totally clear])

Thoughts?

My opinion is that the logic is wrong if I have to set two permissions for letting user publish comments without approval.

Default should be that when "Create comments" are ticked for a role they do not need approval, i.e. they are published directly.

The second permission should be "Moderate all comments" and only if that is ticked comments will be placed in the approval queue before being published.

@xmacinfo: Any feedback on the suggestions in #42? Idea being to specifically avoid any needless association with "approval" or "moderation", but instead, keep it up to the point of "publish".

AFAICS, we have the following suggestions now:

1) "Post comments" + "Publish new comments"

2) "Post comments" + "Publish posted comments"

3) "Create comments" + "Publish comments without approval"

--

While I would support a standardization on "create xyz" permissions, I think that in this case, the new name would also have to change the internal permission name. Thus, 3) does not entirely work for me.

Bad: All of the permission B names could be understood totally entirely different: The user role is granted "moderation access" to publish (any) comments. :(

If we don't find a proper resolution in the next days, then I'd say we simply do this change (rollback/revert to D6):

+++ modules/comment/comment.module 2 Apr 2010 02:19:55 -0000
@@ -384,10 +384,12 @@ function comment_permission() {
-      'title' => t('Post comments with approval'),
+      'title' => t('Post comments'),

--

Throwing a new option into the mix:

4) "Post unpublished comments" + "Have comments published" :-P

Even D6's permissions were more accurate than D7's are currently.

I like something along the lines of catch's suggestion from #8:
"Post comments" + "Bypass Approval for Comments"
or
"Post comments" + "Bypass Comment Moderation"

wow, didn't see catch's suggestion in #8. Why was it ignored for such a long time?

#48 absolutely makes sense. No idea which of both suggestions is better though. My immediate reactions:

"Bypass approval for comments": Is pretty precise already. I wonder whether it could be shortened to "Bypass comment approval".

"Bypass comment moderation": Makes me think of an administrative comment moderation page, which I could skip. Not as good as aforementioned suggestion.

Please bear in mind that we are using "bypass" in the context of global superuser permissions currently. We should try to not break that mental model too hard.

I totally agree with the above solution. "Bypass comment approval" sounds clear and does what it says.

Ok We agreed now let's make a patch ;)

Here we go.

I'm glad to rtbc this :D

+1 on the patch

+1 Go! Go! Go!

#55: drupal.comment-approval.55.patch queued for re-testing.

Although badly needed, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

Huge +1 to #55 being RTBC. I was just recently nailed by this confusing permission and reminded how misleading the names are. Patch still applies with minor fuzz and the bot is happy.

Granted, it's a bit too bad that the machine names aren't changed, too. But, that's probably a much-too-far-reaching change at this stage in D7. If/when this lands, we should either set it back to a D8 issue to do a more thorough cleaning (for DX) or we should open a new D8 issue for that cleanup.

Relevant meta data. ;)

Thank you. This was completely confusing, even for me.

Hm. If we're going to change this, we need to change the machine name too, or else we just introduce another WTF.

As for D7 or D8, not sure. Feels more D8ish to me, since this code has been like that for eons.

Done.

This is #55 plus the results of:

find . -type f | xargs perl -pi.bak -e 's/post comments without approval/bypass comment approval/g;'

What do you say, bot? ;)

Hah, nice x-post. ;) Here's the interdiff. Looks like #74 caught a few that Dave missed.

And yeah, I'd consider this a major WTF, sure. ;)

Interesting, and the interdiff shows that dave was more careful than find in a few cases -- the D6 DBs. So, those need to go. But, there were a few things my patch got right that Dave's didn't. Rerolling now. I guess we also want a DB update to migrate the perms...

Here's the better patch. Still missing a DB update... stay tuned.

Well, here's the patch with the DB update. I haven't actually seen this update work yet, since a new D7 test site with latest HEAD is *still* running update.php from a D6 test site DB. :/ It's stuck on "Updating taxonomy module". I'll let that keep running for now. Might be a sign of another beta blocker. :( But, anyway, here's the (hopefully final) patch for this permission rename. If I get impatient, I might kill this update.php run and try again with a more trivial D6 DB just to make sure user_update_7014() is working as expected.

Okay, I gave up on that attempted update.php. Tried again with a more trivial D6 test site and I can confirm user_update_7014() did the right thing.

Thanks, justinrandell!

BTW, in case anyone missed it, the failed update.php led me to this: #930708: taxonomy_update_7005() can go on an infinite search

Bypass is security lingo and doesn't make it more clear.

Bypass isn't lingo, it's a commonly used English word for "going past" something - usually roads that cut around towns, or heart operations. Also we already have at least one other permission using 'Bypass'.

The point is that this permission doesn't actually let you post comments. All it does is let you avoid, bypass, go-around, be-free-from, liberate-yourself-from, etc, comment moderation. Renaming it makes it a lot more clear.

- When a highway goes around a city, they call it a "bypass" -- hardly security lingo.
- We already use "bypass" in the form of "bypass node access".
- Please don't leave us with a permission called "post comments without approval" which doesn't let you post comments.

Sorry, but words are all about context - in this context its most definitely lingo. To me the permission " bypass node access" is a really poor example, since thats even far more technical.

The bad thing about the actual sentence, is not "without approval" its the "post ..." part. Even going for something like "Allow comments without approval" would be better.

Anyway I am not keeping this from being committed, but its not an optimal choice of words (also bypass is a hard to translate word).

Sorry? What happened here?

The entire point of human-readable permission labels is that they can be changed and tweaked for UX *and* properly translated.

Do you think that this is the only permission label that does not use the same string as the internal permission name?

Do you think that any of the permission labels remotely use the same words as the internal permission name when being translated into German?

#55 was and still is RTBC.

@sun: What happened here is two-fold:

1) I pointed out that end users are an important group, but not the only group, who are confused by this permission name. If we're going to fix the UI, we should fix the underlying code, so developers aren't also confused. Makes the code more readable and less prone to bugs. Consistency++. Hence the patches from me and Dave Reid.

2) Bojhan is complaining that "bypass" is jargon, and that we shouldn't name a permission using that verb. So, by his argument, #55 isn't RTBC, either.

I've tried to point out that while #2 is a semi-legitimate concern, I think "bypass comment moderation" is by far the lesser evil than "post comments without approval" since the perm doesn't let you post comments.

If we "win" the debate that "bypass comment moderation", while not ideal, is better than the UX WTF we have now, we should go ahead and commit #79 to keep the code and the UI more consistent. Sure, the UI strings radically change if your site is in German, but the code is written in English, and should match the default English UI text.

Ok, well its a weird permission anyway - it doesn't do anything on its own. I still think its a suboptimal choice of words, because the "bypass" has to trigger two mental models :

1. Side-stepping, an approval process (unless this process is seen people don't know what this does).
2. The permission does nothing on its own, you need to grant "Post comments" permission to post - to create the mental connection.

Its probably optimistic that any choice of words will convey both, but bypass in my understanding does most for 2 and little for 1. Hence my arguing for rethinking this line of wording, however shouldn't keep it from going in as it is an improvement.

Again, there is absolutely zero need to "keep internal permission names" aligned. The whole idea of human-readable permission labels is that they DO NOT necessarily match.

This is not the only permission label that does not match the internal permission name. And *rightfully* so. There is no need to do an API change here.

We definitely do not want to do API changes, just because a permission could use a better label, punctuation, or wording. If you keep on believing that human-readable labels and internal names need to match, then you did not understand the entire point of human-readable permission labels. It makes absolutely no sense at all.

Simply consider that #55 or #79 would have landed already. Now the UX team comes and (rightfully or not) says "That label is ugly and makes no sense!". So we change APIs for that?

Hell, no!

Do we change menu router item paths, just because a link title changes? And what about a node type that's named "page", but uses "Basic page" as label? I can go on and on with lots of other examples that invalidate the demand for an API change here.

We introduced human-readable permission labels exactly for the reason to be able to improve UX of the product, without having to touch and break underlying functionality.

Dead simple.

I completely forgot that we had this permission in d6 as well, so I agree we shouldn't change the permission's 'machine name' and only fix the title. We'll fix the bigger WTF and it will only remain a smaller DXWTF, which is fine for now.

#79 is not an API change. Unless you have the completely bizarre standard that adding user_update_7014() is an "API change". No function signatures are touched. No behavior is modified.

Furthermore, I object to the the spirit behind #90. The idea that "The whole idea of human-readable permission labels is that they DO NOT necessarily match." is dangerous and misleading. That's not the idea. The point is that we want consistent internal machine names (all lower case, not translated, etc), and nicely formatted human-readable labels (which can be translated, etc). That does not mean freedom for wildly divergent labels and names.

The major bug is the evil name/label for this permission (which is a weird permission that should be re-thought for D8, no question). I'm not arguing that a divergent label and machine name is the major bug. But, if we're going to fix the major UX bug, we should not introduce a normal DX bug that the name and the label have basically nothing in common, and all the code still reads as if this permission lets you post comments.

To clarify:
If you consider #79 an API change and just want the label change, #55 is RTBC.
If you're webchick and you requested this change, #79 is RTBC.
;)

Just to make sure that everyone knows how many API changes would have to be done in follow-up issues to this one, when following the bogus idea of keeping machine names identical to human-readable labels:

@sun we get the point, or at least I do - dont worry :P

I don't really understand that objection. I don't disagree that other permissions (particularly around access/view) have mismatched permission machine names to human-readable names. We should probably clean that mess up in D8.

But until this patch, "post comments without approval" was not one of those permissions. The name and machine name matched exactly. Changing it so that they now mis-match is explicitly introducing a WTF. Especially considering that the old permission name and the new one mean different things, which is the entire point of this issue!

Anyway, I see no compelling case to commit this to D7 given that there is so much contention here around what to do, and the extent with which to do it. Let's address this in D8, and deal with 'bypass node access' in the same way. Sorry, folks... :\

Re: "when following the bogus idea of keeping machine names identical to human-readable labels"

@sun: please don't put words in my mouth. I never said "identical". I'm advocating against "wildly divergent labels and names" and in favor of labels and code that are at least closely related. In every arrow in your picture the difference is minor -- the addition of "new" here, s/node/content/ there, etc. But "Bypass comment moderation" vs. "post comments without approval" is more than a cosmetic divergence. It's a totally different verb that implies totally different behavior. That qualifies as "wildly divergent" in my book.

About the only reason I care is that I think it's possible there will be bugs introduced if the machine name continues to be misleading and wrong, even if the humans get a better label in the admin UI. You can't really make that argument about the difference between "access user profiles" vs. "View user profiles", for example. No one writing code is going to misunderstand what if (user_access('access user profiles')) is supposed to allow, even if the UI uses "View" for the verb, not "access". However, if we just commit #55 to fix the label, I'll bet there will still be bugs caused by this:

  if (user_access('post comments without approval')) {
    // Do something assuming the user can post comments.
  }
}

p.s. *sigh* D8? :( Alas. I don't think a sun vs. dww smackdown over hair splitting really qualifies as "so much contention here around what to do". The important point that everyone (including even Bojhan now, per IRC) agrees is that calling this permission "Post comments without approval" is a major UX bug. This whole permission is weird and needs to be fixed, but at the very least, can we at least commit #55 to help end users? I'll withdraw my objections over the "wildly divergent label and name" if we can at least fix the UI WTF...

Please let's not forget that more important than any machine name is that 'post comments without approval' by itself doesn't allow a user to post comments.

In 7 the user sees:
Post comments with approval
post comments without approval

(Hmm, one or the other)

In 6 the user sees:
Post comments
post comments without approval

(Hmm, I want one, but also the other, i guess?)

In both versions this a possible source of a WTF moment. The change in the D7 user facing text creates a much larger WTF moment than a machine name ever could. A definite minus to D7 usability.

*Addition: Please note the machine names (currently in head) don't change from how they were in D5 (and probably before). They have the same functionality as they had then. Changing the user facing string IMHO is more important, since IT is the only part that changed from 6->7 increasing WTF vakye.

The recent discussion does not affect whether this goes into D7 or not. It only affects how. dww raises a valid point about not fixing developer beliefs by only changing the label, but then again, I'd say that the API bug is not new.

So please choose either #55 or #79, as you wish, but bumping to D8 is not an option, as this is the most weird thing we've ever seen, which not even hardcore developers understand and have to look up in code, and thus a major bug that has to be fixed for D7. The only other option would be to revert both permissions to D6, but I suspect that this move would not be supported by anyone.

In IRC yoroy suggested "skip" as an alternative to "bypass". Posted here for consideration. This is basically #79 with the new verb.

after extensive discussion on irc +1 on #101 from me

Skip is a good word.

So with #101 we get:

Post comments
&
Skip comment approval

Good!

here's #101 but with the help text at admin/help/comment fixed to match. Sorry my find/perl missed this originally.

Removed the double negative in the help text and further improved based on IRC review by yoroy and arianek...

As promised, D8 followup issue:
#931774: Comment moderation/approval is a giant steaming pile of WTF [meta]
;)

patch from 106 reviewed, looks good to go. RTBC'ing

dww, tha_sun, arianek and I discussed this in IRC this evening.

I initially asked why vordude's comment at #98 didn't prove that what's in D7 right now is actually fine. Comments are either posted with approval, or they're not. Simple, right?

NO, actually. vordude's comment goes to prove that this is a big WTF (which also confused me for a good couple of minutes or so) because the permissions don't work like that at all. You need both of them in order to post without approval. Stupid but true. And since this permission name change was done in D7 (sorry, can't find the issue but I'm sure it's cross-linked up above somewhere), this does qualify as a bug that's on the table to fix here.

#106 contains the consensus of the folks in the room, and it looks like xmacinfo has given independent review of it as well. The only problem I was able to find in the earlier patches is they forgot to update the text at admin/help/comment, which is fixed in the later patch.

Therefore, committed #106 to HEAD. Great work, folks! Sorry for not initially understanding.

Tagging as "API change", since the "post comments without approval" permission machine name has now been switched to 'skip comment approval' which might affect some odd-ball contributed modules/tests that are checking for this.

Hm, I find myself agreeing with @sun a bit here - the "post comments without approval" machine name (and any confusion associated with it) looks like it's been around since at least Drupal 4.6. Doesn't seem like we had to change that just because we were fixing some new confusion added via the human-readable names introduced in Drupal 7?

In any case, it's small and easy to deal with, as API changes go. But API changes should be included in the D7 module upgrade guide, so marking as "needs work" for that. (I think a fair number of comment-related modules probably do reuse or check access to this permission.)

Certainly agreed this change needs a doc in the upgrade guide. I can't fix that right now, but if no one gets to it in the next day or so, I'll do it.

This should probably be announced.

It did break one of the tests in Examples, so I imagine it has broken other things.

Only an API change announcement/documentation (?) seems to be left, so demoting to normal.

This was announced in http://lists.drupal.org/pipermail/development/2010-October/036462.html. Probably needs an update in the update guide. It's beyond me how anybody could ever grok the update guide at this point though :-)

fwiw i've already updated the handbook's comment module D7 section accordingly http://drupal.org/node/777676

@rfay good lord, things i need to wrap my head around before christmas... maybe we can sprint on this in dec ;)

changing tagging scheme for issues that need to be mentioned in update guide

Update docs added finally:
http://drupal.org/update/modules/6/7#comment_perms

RE #2: Fixed and included in Drupal 7.0 (http://drupal.org/node/438224#comment-3531250). What is this change good for? Or is there something wrong with drupal.org (comment 2 has a date of today)?

@BarisW: Sadly, people can edit their comments, leading to all sorts of confusion for everyone else (especially for people subscribed via email). The only thing wrong with drupal.org is that I was discouraged from ever implementing and configuring #306132: Add a permission that prevents users from editing issue followup comments. ;)