Problem/Motivation

We currently track the revision uid (editor) separately from the node uid (author), but this information is not displayed on the node revision overview page.

This is a regression from the way it worked in Drupal 7.

Proposed resolution

Display revision_uid so that the editor instead of the initial author is properly displayed.

Remaining tasks

Reviews.

User interface changes

The node's editor would display, if different from the author, on the revisions overview page.

API changes

CommentFileSizeAuthor
#21 interdiff-2495297-17-21.txt724 bytessnehi
#21 2495297-21.patch2.41 KBsnehi
#17 2495297-17.patch2.41 KBjhedstrom
#17 2495297-17-TEST-ONLY.patch1.71 KBjhedstrom
#17 interdiff.txt1.39 KBjhedstrom
#16 2495297-16.patch2.16 KBjhedstrom
#16 interdiff.txt498 bytesjhedstrom
#15 interdiff.txt1.7 KBjhedstrom
#15 2495297-15.patch2.4 KBjhedstrom
#15 2495297-15-TEST-ONLY.patch1.7 KBjhedstrom
#13 2495297-13.patch716 bytesjhedstrom
#4 node-revision-overview-show-editor-2495297-04.patch3.48 KBjhedstrom
#4 interdiff.txt3.03 KBjhedstrom
#4 Revisions_for_An_article___Site-Install.png44.73 KBjhedstrom
#2 Revisions_for_An_article___Site-Install.png41.56 KBjhedstrom
#2 node-revision-overview-show-editor-2495297-02.patch1.79 KBjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Assigned: Unassigned » jhedstrom
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests, +Needs usability review
FileSize
node-revision-overview-show-editor-2495297-02.patch1.79 KB
Revisions_for_An_article___Site-Install.png41.56 KB

This patch changes the UI to display as pictured in the summary.

Tagging for usability review.

I think the tests can be added more easily once #1528028: Add tests for reverting revisions where revision_uid and uid differ lands.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Status: Needs review » Needs work

Oops, the screenshot indicates an obvious bug in the patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
Revisions_for_An_article___Site-Install.png44.73 KB
interdiff.txt3.03 KB
node-revision-overview-show-editor-2495297-04.patch3.48 KB

This patch resolves that issue.

The last submitted patch, 2: node-revision-overview-show-editor-2495297-02.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: node-revision-overview-show-editor-2495297-04.patch, failed testing.

Status: Needs work » Needs review

jhedstrom queued 4: node-revision-overview-show-editor-2495297-04.patch for re-testing.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Assigned: jhedstrom » Unassigned
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Related issues: +#2490136: Enable revisions by default
shwetaneelsharma’s picture

shwetaneelsharma CreditAttribution: shwetaneelsharma at Axelerant commented
Status: Needs review » Needs work

node-revision-overview-show-editor-2495297-04.patch does not apply any more. Changing the status to needs work.

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented
Issue tags: -Needs usability review

What? shouldn't it always show who edited. Thats much more relevant?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented

What? shouldn't it always show who edited. Thats much more relevant?

Probably. The author would always be the editor of the first revision, so no information would be removed or lost if we only displayed the editor's name.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +Regression, +rc target triage
FileSize
2495297-13.patch716 bytes

I just tested this in D7, and this is actually a regression. This patch implements Bohjan's idea in #11, which is actually how it already worked in D7.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Title: Display revision_uid editor name if different from uid on node revision overview page » [regression] Display revision_uid editor name if different from uid on node revision overview page
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
Issue tags: -Needs tests
FileSize
2495297-15-TEST-ONLY.patch1.7 KB
2495297-15.patch2.4 KB
interdiff.txt1.7 KB

Here's a test.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
FileSize
interdiff.txt498 bytes
2495297-16.patch2.16 KB

Oops, unused use statement.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 commented
FileSize
interdiff.txt1.39 KB
2495297-17-TEST-ONLY.patch1.71 KB
2495297-17.patch2.41 KB

Oops, the test was looking for the original author in both instances.

The last submitted patch, 17: 2495297-17-TEST-ONLY.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Issue summary: View changes

We have a test, so let's remove that from the list of todos

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -187,7 +187,7 @@ public function revisionOverview(NodeInterface $node) {
           '#theme' => 'username',
-          '#account' => $revision->uid->entity,
+          '#account' => $revision->revision_uid->entity,

What about using $revision->getRevisionAuthor() here?

snehi’s picture

snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented
Assigned: Unassigned » snehi
snehi’s picture

snehi CreditAttribution: snehi as a volunteer and at Publicis Sapient for Publicis Sapient commented
FileSize
2495297-21.patch2.41 KB
interdiff-2495297-17-21.txt724 bytes

Please review.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thank you

The last submitted patch, 17: 2495297-17-TEST-ONLY.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Issue tags: -rc target triage +rc target

Discussed with @effulgentsia. Looking at https://www.drupal.org/files/issues/Revisions_for_An_article___Site-Inst... makes it clear that the behavior in HEAD is not intended. Given that this is hiding information but that it would also be questionable to change what's seen in this UI during a patch release, marking as an RC target.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed a56fa00 on 8.0.x 
    Issue #2495297 by jhedstrom, snehi: [regression] Display revision_uid...

  • catch committed a56fa00 on 8.1.x 
    Issue #2495297 by jhedstrom, snehi: [regression] Display revision_uid...

Status: Fixed » Closed (fixed)

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