Problem/Motivation

The revisions tab on a node lists all of the revisions for that entity. The latest revision is instantly marked as "Current revision", when it may not be the default or published revision.

Proposed resolution

Update NodeController::revisionOverview to show "Current revision for the default revision rather than the latest revision

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

timmillwood created an issue. See original summary.

timmillwood’s picture

Assigned: timmillwood » Unassigned
Status: Active » Needs review
FileSize
1.7 KB

Initial fix.

timmillwood’s picture

Issue tags: +Needs usability review
alexpott’s picture

Issue tags: +Needs tests
timmillwood’s picture

Issue summary: View changes
FileSize
160.46 KB
dawehner’s picture

timmillwood’s picture

Slightly tweaked the table to add the revision id to the row class, this was really only to allow the test to work.

Status: Needs review » Needs work

The last submitted patch, 7: the_latest_revisions-2761409-7.patch, failed testing.

timmillwood’s picture

hrmmm.. this worked locally.

hgoto’s picture

Status: Needs work » Needs review
FileSize
4.27 KB
3.3 KB

Great work! I tested the patch #7 in my local environment and it worked also for me with PHP 5.6. I don't understand what's wrong with it...

Please let me suggest some fix to the patch #7 (though this is not related to the test failure).

For the test case testNodeRevisionsTab:

  • Changed the test case name to testNodeRevisionsTabWithDefaultRevision() for more accuracy.
  • Removed unnecessary set of the node title and body. (drupalCreateNode() sets them.)
  • Replaced the hard-coded node id 1 to $node_id.
  • Changed the assertions method a little.

As for NodeController class code:

  • Removed the number class only for test. (This was introduced with the patch #7, as timmillwood told.)
  • Instead, added a class node-revision-table to the revisions table itself.

I'm sorry, I still don't understand the reason that the auto test for #7 failed clearly...

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Well that worked better, thanks @hgoto!

hgoto’s picture

@timmillwood, thank you for your reaction.

Now I guess that the failure on the patch #7 was caused by the hard-coded node id. In my local environment I ran that test alone and that's why I didn't notice those errors.

yoroy’s picture

show "Current revision for the default revision rather than the latest revision".

Which is basically just common sense no? Seems fine, not sure this needs explicit usability review to move forward, but by all means, go ahead :-)

timmillwood’s picture

Thanks @yoroy, I mainly wanted to make sure I wasn't adding any bad UX working or practices.

Great to see you're happy with this fix.

On to the commit!

catch’s picture

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -219,16 +219,17 @@ public function revisionOverview(NodeInterface $node) {
+              'title' => $vid < $node->getRevisionId() ? $this->t('Revert') : $this->t('Set as current revision'),

Would be good if the test coverage also handled this logic where the text is different based on forward revision or not.

Additionally I'm not sure about the language here. This UI copies the revision to a new one, then makes that the default. So neither reverting nor 'set as current' is really correct. This is an improvement though, so maybe a follow-up?

Also should the test coverage extend WebTestCase - I guess we'll convert this test to phpunit later, so maybe that's OK rather than making a new test class that'd have to be merged back.

catch’s picture

Title: The latest revisions isn't always the current revision » The latest revision isn't always the current revision
yoroy’s picture

"Copy as current revision"
"Copy to current revision"
"Set a duplicate as the current revision"…

These are all wonky right? I worry more correct wording will make the button label harder to understand.

timmillwood’s picture

"obvert"?

I'm not sure there is a better wording.

I think we need a follow up issue to discuss what is done when the button is clicked, as well as the wording of the button label.

  • catch committed ec4bfd9 on 8.2.x
    Issue #2761409 by timmillwood, hgoto: The latest revision isn't always...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Opened #2764805: Clarify concepts (and implementation) around reverting revisions and publishing forward revisions to discuss that more. Committed/pushed to 8.2.x - I think it's fine to add a method to the existing test and convert wholesale later on.

catch’s picture

Issue tags: +String change in 8.2.0
jhedstrom’s picture

catch credited Jaesin.

catch’s picture

catch’s picture

Added credit for jhedstom and Jaesin.

Status: Fixed » Closed (fixed)

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