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

Adding credit from UX team review and parent issue.

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.

@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. 😂

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.

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.

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 Needs followup I opened that one moments after submitting this one.

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