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

Created on 8 July 2022, almost 2 years ago
Updated 17 April 2024, 2 days ago

Problem/Motivation

#332796: Add permissions to the update.module to hide warnings was released before the release managers had signed off. The wording of the new permission name and description both need help. See comments #97 and #99 for more.

That feature was also reviewed by the UX team at #3284204: Drupal Usability Meeting 2022-06-10 , leading to some additional concerns / suggestions.

Specific concerns:

  1. "update notifications" is too vague. We want something like "notifications about available site updates".
  2. The description doesn't make it clear how important it is to grant to various admin roles. We could add something like "Ensure that site administrators have this permission so that security updates are applied promptly."

Steps to reproduce

Look at core/modules/update/update.permissions.yml

Proposed resolution

  title: 'View software update notifications'
  description: 'Ensure that site administrators have this permission so that security updates are applied promptly.'

Remaining tasks

  1. Decide on the right wording.
  2. Implement it.
  3. Reviews / refinements.
  4. Add screenshots to the summary (once we have something to screenshot).
  5. Final UX review
  6. Release manager review
  7. RTBC

User interface changes

Before

After

API changes

TBD

Data model changes

Release notes snippet

@todo

🐛 Bug report
Status

Fixed

Version

10.3

Component
Update 

Last updated about 6 hours ago

  • Maintained by
  • 🇺🇸United States @tedbow
  • 🇺🇸United States @dww
Created by

🇺🇸United States dww

Live updates comments and jobs are added and updated live.
  • Usability

    Makes Drupal easier to use. Preferred over UX, D7UX, etc.

  • Regression

    It restores functionality that was present in earlier versions.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates : some issue and comment data are missing.

  • First commit to issue fork.
  • Merge request !7254Improve the label and description → (Closed) created by quietone
  • Status changed to Needs review 20 days ago
  • 🇳🇿New Zealand quietone New Zealand

    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.

  • 🇺🇸United States smustgrave

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

  • 🇺🇸United States nicxvan

    I second 12

  • Status changed to Needs work 17 days ago
  • 🇺🇸United States smustgrave

    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?

  • 🇺🇸United States dww

    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.

  • 🇺🇸United States smustgrave

    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.

  • 🇺🇸United States dww

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

  • 🇺🇸United States smustgrave

    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.

  • Status changed to Needs review 16 days ago
  • 🇳🇿New Zealand quietone New Zealand

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

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    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?
  • 🇺🇸United States dww

    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.

  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    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.

  • 🇺🇸United States dww

    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

  • 🇺🇸United States dww

    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.

  • 🇺🇸United States dww

    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.

  • Pipeline finished with Success
    12 days ago
    Total: 693s
    #140374
  • 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland

    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.

  • 🇺🇸United States smustgrave

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

  • 🇺🇸United States dww

    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.

  • Status changed to RTBC 8 days ago
  • 🇺🇸United States smustgrave

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

  • 🇺🇸United States dww

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

  • 🇳🇿New Zealand quietone New Zealand

    Updated the after screenshot.

    • longwave committed eba77b9b on 10.3.x
      Issue #3295078 by quietone, dww, AaronMcHale, Anybody, benjifisher,...
  • Status changed to Fixed 3 days ago
    • longwave committed ee73d14d on 11.x
      Issue #3295078 by quietone, dww, AaronMcHale, Anybody, benjifisher,...
  • 🇬🇧United Kingdom longwave UK

    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!

Production build https://api.contrib.social 0.62.1