Node revision revert/delete permission descriptions are wrong

While these might be technically wrong, is there actually a way to actually do this via the UI with just those permissions? As far as I know you need to be able to view revisions to be able to delete them in practice.

Well that's also a bug, there's been an issue open to resolve it since just after the delete permission was added in 2007 #196758: Having delete as a button on the node edit form means certain users don't have access to it when they should. So we might make it consistent with the delete permission, but we could equally adjust the node delete permissions description to make it consistent with this one (or change both).

I think there's actually some other ways to get to the delete operation now like contextual links and admin/content maybe, but there's no such mechanism for revisions, so it's not entirely equivalent either.

Moving back to CNR and tagging 'needs usability review'.

So we might make it consistent with the delete permission, but we could equally adjust the node delete permissions description to make it consistent with this one (or change both).

I'm losing track of which refers to what here :)

This issue is still tagged for a usability review (@yoroy did not have enough information above to provide the review).

I confirmed that nodes can be deleted without permission to edit the node or to view revisions, and similarly, revisions can can be deleted without permission to edit. In both cases, visiting the link directly or a similar operation is allowed. So, it's misleading to have this description on only the revision permissions while the similar content permissions have none. I've attached a screenshot to illustrate.

Edit: What does appear to be true is that you must be able to delete the given content in order to be able to delete a revision of it.

I think the questions in scope for this issue are:

1. Is the updated text sufficiently clear for site builders to understand what additional permissions are needed in order to delete the revisions?
2. Should we add a similar description to the content delete permissions in addition to the revision delete permissions?
3. Or, should we instead simply remove all four of these descriptions, since they are not entirely correct either in HEAD or in the patch, and since reducing the textual load actually makes it easier for users to use a page?
4. Edit: Or, should we come up with a different wording that indicates revisions can only be deleted if the content can be deleted (even without permission to view or edit)?

Edit: incorporating above for clarity.

I have made a path as per the suggestion. it's my first patch :)

Adding Patch as per instructions in #19.

Thanks!!

Hi Jhodgdon,

I have done the changes.

Please review,

Thanks!!

Hi Jhodgdon,

Any updates??

Thanks!!

I am reviewing this but its difficult to understand all the complications. Sorry, that these reviews are sometimes slow - allocating required time for complex issues is not easy.

The challenges I see:

• We should not use "nodes", in the Human Interface Guidelines we indicate viable alternatives.
• Using the word "also" seems conversational, is this needed to convey the message?
• Overall can we cut words to make it more scannable? It looks like a lot of condition/repetition now.

I am unable to really evaluate how it compares to the triangulation of permissions :) But each permission seems to clearly indicate which other permissions are required.

Would anyone want to provide a permission without doing the underlying step (e.g. not granting edit the content item when they want people to revert a revision)?

Overall it feels very close, and would not constitute another usability review before commit.

"also" seems necessary to me since you have to have both permissions, not just the "edit" permission.

I see no reason to prevent this patch from going through. If someone has the actual phrase that is perfect, then they should specifically put it here and we can go through the review process at that time.

However, at this time I see it as good enough. Someone coming up with an improved way of saying it can go through the review process at that time.

Rebuilt patch.

Rebuilt patch (without my email this time ... sigh).

Is there any way to delete 2678178-29.patch ? Thanks.

Reuploading new patch for 8.3.x

Reuploading new patch for 8.3.x

Looks like you might have had some tabs creep in there on line 66. The indentation is incorrect.

Patch to fix indentation as discussed on #37.

The patch passed.

A usability review was done in #26, and the patch content has not changed since then, noting three points

• Remove the word 'node'.
• Concern on the use of 'also'.
• Make it more scannable.

The word 'node' has been removed. Using 'also' seems necessary as said by @ftyre76. There is no work done on making it more scannable. But even so, the usability review stated that these would not constitute another usability review before commit.

Way back in #23, jhodgdon, was ready to RTBC but after a usability to review. Since that has happened this should be good to go.

Finally, since I don't usually work with permissions I installed the patch and tried it. I found the new text an improvement.

+++ b/core/modules/node/node.permissions.yml
@@ -18,12 +18,13 @@ view own unpublished content:
+  description: 'To view a revision, <em>you also need permission to view the content item.</em>'
...
-  description: 'Role requires permission <em>view revisions</em> and <em>edit rights</em> for nodes in question or <em>administer nodes</em>.'
+  description: 'To revert a revision, <em>you also need permission to edit the content item.</em>'
...
-  description: 'Role requires permission to <em>view revisions</em> and <em>delete rights</em> for nodes in question or <em>administer nodes</em>.'
+  description: 'To delete a revision, <em>you also need permission to delete the content item.</em>'

So it does not really make sense use italics here. The original use of italics was to denote the labels of the permissions used in the UI. Let's remove them now that they are no longer labels.

Thanks!

Removed <em> as per #43

@tameeshb, thanks for making those fixes.

+++ b/core/modules/node/src/NodePermissions.php
+++ b/core/modules/node/src/NodePermissions.php
@@ -62,14 +62,15 @@ protected function buildPermissions(NodeType $type) {

@@ -62,14 +62,15 @@ protected function buildPermissions(NodeType $type) {
       ),
       "view $type_id revisions" => array(
         'title' => $this->t('%type_name: View revisions', $type_params),
+        'description' => t('To view a revision, <em>you also need permission to view the content item.</em>'),
       ),
       "revert $type_id revisions" => array(
         'title' => $this->t('%type_name: Revert revisions', $type_params),
-        'description' => t('Role requires permission <em>view revisions</em> and <em>edit rights</em> for nodes in question, or <em>administer nodes</em>.'),
+        'description' => t('To revert a revision, <em>you also need permission to edit the content item.</em>'),
       ),
       "delete $type_id revisions" => array(
         'title' => $this->t('%type_name: Delete revisions', $type_params),
-        'description' => $this->t('Role requires permission to <em>view revisions</em> and <em>delete rights</em> for nodes in question, or <em>administer nodes</em>.'),
+        'description' => $this->t('To delete a revision, <em>you also need permission to delete the content item.</em>'),

Still <em> here.

Removed tags.

Ah, right. I see.

Alright, this ended up in a good place I think. Thanks everyone! Committed to 8.4.x.