Separate "Administer comments and comment settings" into two distinct permissions.

[jenlampton]

Administer comments - should provide access to the admin/content/comment page
Administer comment settings - should provide access to the comment settings vertical tab on nodes

[robbiethegeek]

Pull request submitted backdrop/backdrop#478

[quicksketch]

This didn't get into 1.0.0. We could put this into 1.1.x I think, but it's a little dicey. Is this an API change or not? We need to come up with criteria for what's acceptable.

[jenlampton]

whoops, didn't mean to close.

[ghost]

Would it be enough to simply provide an update hook thing (can't think of the proper name for hook_update_N() implementations) that sets the value of the new permission (administer comment settings) to the same value as the old one (administer comments)? Then we can still do this now but we're not breaking anything either...

[ghost]

Hmm, seems @quicksketch already suggested this: backdrop/backdrop#478 (comment)

[klonos]

can't think of the proper name for

No, you're right 👍 ...these are called "update hooks".

[jenlampton]

Is this an API change or not? We need to come up with criteria for what's acceptable.

I think as long as the change is backwards-compatible, it can go into 1.x. If adding an update hook makes sites behave exactly the same before this change as after, then it should be able to go into 1.x.

[ghost]

Here's a PR: backdrop/backdrop#3370

[indigoxela]

Several related tests are currently failing, so back to "needs work".

[ghost]

Thanks, all fixed.

[jenlampton]

This PR is "upgrading" permissions on existing sites, so functionality won't be lost per se, since any user role that previously had the administer comments permission will now have that same permission + administer comment settings. That's great 👍🏼

If a contrib or custom module was previously relying on the administer comments permission to do things that now require administer comment settings, then things might not work as expected. It also depends on the logic that the module has in place around that permission 🤔

@klonos I'm not sure the use-case you brought up here is a valid one. The only way this could be a problem is if you have people who have permission to administer settings, without administering comments themselves. Not only is this likely to be a very rare scenario, but because the settings-only permission did not exist before this change, the liklihood of that affecting existing sites is zero. (so, this is not a BC issue at all)

For new sites, it is possible that someone could add this settings-only permission and get themselves into a pickle, but it's perhaps more likely that the contrib project would update the permission it requires before then.

What's done in this PR is the way we've handled all permission updates in the past. If we want to add a backwards-compatible permission system to 2.x -- and I think that's a reasonable discussion to have! -- but let's not block this useful feature by doing it here :) Can you please open a new issue? (If you haven't already)

[jenlampton]

I will remove the good first issue tag, since I don't think there is anything for a new contributor to do right now.

@stpaultim this issue both needs code review and testing. And since the code change is minimal, those are both realistic for first time contributors. I'm going to add the label back since we have a shortage of people helping to test issues.

[quicksketch]

This looks good to me. We now have precedent for adding/splitting permissions and have done the same thing in a few other places, such as:

• [D8] Create a new 'Assign user roles' permission #313
• [D8][UX] Add Better Taxonomy permissions #382

There was a fair amount of permissions additions added throughout File module's history too.

[quicksketch]

One more comment @BWPanda if you could take a look: https://github.com/backdrop/backdrop/pull/3370/files#r825147313

[ghost]

Done.

[stpaultim]

I tested the Pull Request and it seems to work as expected.

I created a TEST account and gave them editor role.

When I gave them Administer comments permission they were able to access to the admin/content/comment page.
When I gave them Administer comments settings permission they were able to access to the comment settings vertical tab on nodes.

It all seems to work as expected.

(I did this as part of an OpenForce2022 demo)

[quicksketch]

Thanks folks! I have merged backdrop/backdrop#3370 into 1.x for 1.22.0! I don't think this qualifies as either a bug or UX improvement, so I have not pulled it back into 1.21.x. Huge thanks for knocking out this very old issue!