Fix the label and description of the new "view update notifications" permission

I've made an MR with the suggested change but I have not checked for test failures because this needs a decision on the text first. So, I am setting this to NR to discuss the text.

For what it’s worth I’m still a fan of my suggestion in #12 for the description.

I second 12

Seems to have two +1's for the #12.

Haven't seen much discussion around the change but reason I'm against "Ensure that site administrators have this permission" is that sounds more instructive vs descriptive. The permissions description, least to me, should just mention what the permission does but no recommendations who should have them. If we went that route then we would have to update dozens of descriptions with suggestions, example "administer content types" we could have to recommend site builders have it.

If we just update title/description do we still need a CR?

From the summary, under "specific concerns":

The description doesn't make it clear how important it is to grant to various admin roles.

True, we don't currently make recommendations about who should have which permissions. But this is sort of a weird edge-case perm, and I (and the UX team) wanted to make sure that it doesn't get ignored. Yes, 'administrator' role (if any) would automatically have all perms, including this one. That was part of the motivation of splitting this out and allowing site builders to turn this off for not-quite-root admins that can't do anything with the results. But if a site doesn't have an 'administrator' role, and is blazing their own path, I'm in favor of giving them a little nudge about it.

A totally different, weird, probably bad idea to address this would be to have a status report warning if there are no roles on the site that have this perm. But that seems likely to generate more requests for "how do I turn of this stupid warning?", and will require a bunch more code in here than adding a recommendation to the description.

Wonder if we need a new option like "restrict access: true" which generates "Warning: Give to trusted roles only; this permission has security implications." to permissions automatically.

@smustgrave re #23: I don't understand. Are you proposing something like recommend access: true to automatically add the last sentence? That seems like over-engineering. Until we have at least 3 perms that need this, we should just 1-off it before trying to generalize anything. The sentence won't necessarily be the same for every perm, either. In this case, it's "Ensure that site administrators have this permission so that security updates are applied promptly." or something. But another perm where we really want admins to have it might have other reasons, etc. We'd need tests for the new feature, CR, docs, etc, etc. I just want to add 1 more sentence to this description and be done. 😂

Gotcha, won't be the blocker but still seems odd to make the recommendation in a permission. Think a better description should just say what the it does. Leave it to the site builder to determine the role it needs.

Setting back to NR because we need to discuss the text. Also, I pinged in #ux about this.

Re #12. I'm not sure about that description, to me it's just saying the same thing the title says but using more words.

How about:
title: View software update notifications
description: Displays software update notifications for users who do not have the Administer software updates permission, users with that permission always see these notifications.

Here's my rational:

1. Using the term "software updates" cogitatively links it to the "administer software updates" permission.
2. In the description we state the key difference between the two permissions.
3. We reassure the administrator that if you have the ability to administer updates you will always see the notifications, which is something we said in the previous issue would be good idea. Is this actually how it works though, if not it probably should work that way?

Re: #27: Thanks for taking another look. I don't have time to come up with better text right now, so I'm ignoring all that for the moment. 😅 But to directly address your question in point #3:

No, that's not how it (currently) works. The messages are only shown to people with the view permission:

function update_page_top() {
  /** @var \Drupal\Core\Routing\AdminContext $admin_context */
  $admin_context = \Drupal::service('router.admin_context');
  $route_match = \Drupal::routeMatch();
  if ($admin_context->isAdminRoute($route_match->getRouteObject()) && \Drupal::currentUser()->hasPermission('view update notifications')) {
     ...
  }
}

(Side note: that function would be a good candidate to invert the logic check there and early return if the conditions aren't met, instead of indenting the whole thing an extra 2 spaces). 🤓

Arguably how you assumed it works would make more sense -- if you have administer software updates you should automatically see the warnings. But part of the justification for splitting this into a separate perm in the first place ( #332796: Add permissions to the update.module to hide warnings → ) is that sites wanted to hide these messages from certain admins and my hard-coded assumptions about who should see them weren't true. So I'm a little reluctant to backtrack on this and force the messages to appear for anyone without that explicit perm.

Arguably how you assumed it works would make more sense -- if you have administer software updates you should automatically see the warnings. [...]

Hmmmmm....... yeah I can see the rational for wanting to have the ability to perform software updates, without getting notified about them. But then, if you are the administrator, then presumably you cannot "turn off" these messages because you have all permissions.

I wonder if it would have been better to just made this an option on the user profile for those who have the permission.

So in that case it would work like:

1. If you are have the "administer software updates" permission you see the messages by default;
2. And, if you have the "view update notifications" permission then you also see the messages by default;
3. However, you can individually choose to turn off these messages using a new option by editing your user profile, and this option is only visible if you have either of those permissions.

The idea that you would not see the messages if you have the administer permission doesn't sit right with me because it goes against the established permission hierarchy pattern that users have come to expect in Drupal. That being, where those who have the "administer X" permission generally have access to all of the things, and the more specific permissions like "view X entity" and "edit X entity" provide more granular control for those who don't have the "administer X" permission.

Originally, the messages were shown to anyone with administer site configuration. But not all of those folks would have administer software updates, so some people seeing the messages couldn't do anything about them, which was the source of the trouble. It's pretty hard to imagine a scenario where someone with administer software updates should not be seeing these warnings at the top of admin pages. So I'd be fine with changing the behavior. As you point out, there's tons of precedent in core to have an 'administer X' perm that's a superset of individual '$verb X' perms...

I only worry about scope management. 😅 If it's best for the UI, happy to change both the label/description, *and* the behavior in this 1 issue. That does seem reasonable in this case, and the changes should be pretty small. But I'd love confirmation from a core committer that this approach is okay.

Thanks,
-Derek

p.s. Forgot to mention, but I'm strongly opposed to adding a per-user option for this. That's all sorts of complication for a very, very tiny fraction of sites that might want the flexibility. This is all about who should have access to see what, which is squarely in the domain of permissions and roles, and adding a whole other way to control who sees these seems very problematic on multiple fronts.

I pinged about this in #bugsmash to get input on the scope. Multiple folks chimed in to say that changing the behavior should happen in a separate issue, since it's likely to lead to more churn and will slow down this issue. Let's focus on updating this for how it works now, and if we want to change how it works, do it later.

Meanwhile, rebased the MR to the latest 11.x, and updated the label to be 'View software update notifications' per @AaronMcHale in #27 (which I think is a good improvement).

Leaving the description as "Ensure that site administrators have this permission so that security updates are applied promptly." since 1. that was the UX team's recommendation and 2 I agree that's better than the alternatives.

Added this as the proposed resolution in the summary.

Also, removed 'Update tests' from remaining tasks, since it looks like we're not changing the machine name of the permission at all, and therefore, no tests are impacted. We (thankfully) do not have tests to verify that the permission label and description match update.permissions.yml.

Thanks @dww, that all makes perfect sense to me :)

So yeah I'm in favor of just get this in as-is, and then we can do behavior change and any further labeling/description changes in a follow-up.

Since we are just updating description and test do we still need release notes, change records, and release manager sign off?

Re: #34: Part of the initial impetus for this issue, as described in the summary, is that #332796: Add permissions to the update.module to hide warnings → happened without release manager approval, and they didn't love how it was labeled/described in the first place. That's why I put release manager review as a remaining task. I doubt this needs a change record or release note, so I'll remove those tags for now, but that's up to the RMs to restore the tags if they desire.

Re:#29 and later: 😂 #3295081: Show Update manager /admin/* UI warnings to anyone with "administer software updates" permission, too → I opened that one moments after submitting this one.

With the tags removed and since this is just description and title change believe it’s good

Cool thanks. Now that it’s ready, officially tagging for RM sign off. 😅

Updated the after screenshot.

Looks great to me, the wording is clearer and the description is useful instead of just using more words to repeat the label :)

Not eligible for backport to 10.2.x as this is a translatable string change.

Committed and pushed ee73d14d8e to 11.x and eba77b9b3f to 10.3.x. Thanks!

longwave → closed merge request !7254

Automatically closed - issue fixed for 2 weeks with no activity.