Problem/Motivation

While there is no way through the core UI (unless #1776796: Provide a better UX for creating, editing & managing draft revisions. lands) to produce a revision that is more recent than the current node revision, it is possible to do through code, and contrib will certainly do this as workflow modules start to appear for Drupal 8.

The current revision overview form makes little sense to 'Revert' to a newer version. Rather, it seems preferable to change the text for revisions that are more recent than the current version (tentatively 'Set current').

Making this change in the node module will save contrib from having to alter the text of these buttons (which is a non-trivial task, given the way the page currently works).

Proposed resolution

Update the node revision overview page accordingly, and also the confirm revert form.

Remaining tasks

User interface changes

also, the confirm form will read

'Are you sure you want to set the current version to %revision-date?'

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it will allow contrib to provide forward-revisioning of nodes
Issue priority Not critical
Prioritized changes The main goal of this issue is usability and DX
CommentFileSizeAuthor
#29 interdiff-2429153_11-29.txt7.01 KBJaesin
#29 2429153-29.patch7.16 KBJaesin
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,515 pass(es). View
#13 node-revision-text-2429153-13.patch6.81 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,429 pass(es). View
#13 interdiff.txt951 bytesjhedstrom
#11 node-revision-text-2429153-11.patch6.8 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,429 pass(es). View
#11 interdiff.txt1.51 KBjhedstrom
#9 node-revision-text-2429153-09.patch6.8 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,423 pass(es), 6 fail(s), and 0 exception(s). View
#9 interdiff.txt1.05 KBjhedstrom
#6 node-revision-text-2429153-06.patch6.01 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,377 pass(es). View
#3 node-revision-text-2429153-03.patch6.04 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch node-revision-text-2429153-03.patch. Unable to apply patch. See the log in the details link for more information. View
#3 interdiff.txt5.52 KBjhedstrom
#1 node-revision-text-2429153-01.patch2.7 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,191 pass(es). View
Revisions_for_Third___Site-Install.png127.13 KBjhedstrom
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jhedstrom’s picture

Status: Active » Needs review
Parent issue: » #1776796: Provide a better UX for creating, editing & managing draft revisions.
FileSize
2.7 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,191 pass(es). View
jhedstrom’s picture

Issue tags: +Usability, +Needs tests

This is testable.

Adding a usability tag too, since without, one gets to revert to things that have never been current, which is confusing.

jhedstrom’s picture

Issue tags: -Needs tests
FileSize
5.52 KB
6.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch node-revision-text-2429153-03.patch. Unable to apply patch. See the log in the details link for more information. View

This adds tests, and changes the button text to 'Publish' if the node is currently published, otherwise it uses 'Set current'.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs beta evaluation

RTBC, looks good to me, but needs a beta evaluation.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: node-revision-text-2429153-03.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,377 pass(es). View

Re-rolled #3.

jhedstrom’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs beta evaluation

Adding a beta phase eval, and moving back to RTBC as per #4.

hass’s picture

Status: Reviewed & tested by the community » Needs work

"current revision" translatable string need to be ucfirst.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.05 KB
6.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,423 pass(es), 6 fail(s), and 0 exception(s). View

re: #8, that wasn't added as part of this patch, but makes sense to fix it while we're here.

Status: Needs review » Needs work

The last submitted patch, 9: node-revision-text-2429153-09.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
6.8 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,429 pass(es). View

Needed to make the corresponding case-change in the tests.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, nits look good.

jhedstrom’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
951 bytes
6.81 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,429 pass(es). View

Oops, this was still using the now-deprecated String class.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC assuming #13 goes green.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs usability review

Tagging to a get a usability review - to my eyes the change looks great but it'd be awesome to get confirmation from a UX guru.

miro_dietiker’s picture

I'm not a UX guru, but here my user thoughts.

I don't see a way to create a "noncurrent" revision in the current UI.
So the status you propose about "Publish" a revision can only happen through content creation in code.
This doesn't make sense for me in core like this. Either i can create a future revision and deal with it, or i don't need the future revision functionality in core at all.
When using the core UI, always the latest revision is current.

Then i did some DB tweaking to reproduce what you propose.
Reverting (or publishing a revision) creates a new revision on top of everything.
With "Publishing" or "Set current revision", i would expect to have any revision enabled as active, without creating a new revision at all. It should just switch the active revision.

jhedstrom’s picture

i would expect to have any revision enabled as active, without creating a new revision at all. It should just switch which one is the active.

This patch only tweaks the labels, that actual behavior is lower level. AFAIK, currently 'reverting' to an older version creates a new revision of the older revision as the latest.

miro_dietiker’s picture

Exactly, and that's why i reported that switching the labels creates different user expectations.

I suggest you update the title and the issue summary as the current proposal to "Publish" a revision changes the perspective.
IMHO: If the user expectation is confirmed to be different, then processes possibly need to change as part of this issue.

jstoller’s picture

Part of Drupal Core's job is to enable great things to be done in Contrib. While robust support for content moderation and workflows is still embarrassingly absent from Core, there are a number of Contrib modules trying to fill in that gaping hole. The ability to save save forward revisions at an API level was added in #218755: Support revisions in different states and is a huge win in this regard. Though Drupal's UI does not make use of this capability out of the box, its inclusion will prevent modules like Workbench Moderation from having to employ risky hacks for their basic functionality.

Given that Core enables the saving of forward revisions (programmatically, if not through the UI), it seems perfectly reasonable to me that any representation of revisions in the UI should take that possibility into account. This is especially important in cases where changing Core's behavior is particularly difficult (as is apparently the case with this page). We should try to avoid the need for more crazy hacks.

As a user (and a UX guy), my expectation is that I should have the option to make any forward revision the "current" revision. Doing this should never create a new revision, or in any way move that revision ahead of other forward revisions. It should just set that forward revision as the current revision. I would expect that this option was the default option for the most recent revision (if it is a forward revision). Additionally, I should be able to revert to any revision older than the most recent revision. Doing so should create a new revision, copying over the content from the old revision.

miro_dietiker’s picture

As a user...

Thanks jstoller, that's exactly what i tried to describe in #16.

Fabianx’s picture

#19, #20:

The comments are good, but are unfortunately out of scope here. Core does not support setting any revision to published, that is not how forward revision support works. (and does not work properly in workbench D7 either, we use drafty in cps module for that).

The only sane way to publish is to load a revision and save that as a new published one. (currently)

A forward revision in that is technically not different than a "revert".

You might not like that behavior, but it is very difficult (and not implement in core) to just write the base + field tables without changing the revision tables.
Obscure bugs happen (e.g. field_collection in D7) if you don't do it that way ... And again it is out of scope here.

All that forward revision support added - if I understand the code correctly - is to write a non-standard revision, which means write a revision without changing the base entity and field tables.

The opposite was not implemented - unless I am mistaken.

--

That said: Can we get back to the issue of the UX improvement and maybe open a follow-up to support publishing / setting current in the way you described?

A possibility for a contrib module would be to remove the older revision after writing the newly published revision. (to lessen the confusion)

miro_dietiker’s picture

A possibility for a contrib module would be to remove the older revision...

This would be very dangerous. No one knows what revisions are "in use". Sometimes references point to revisions and core is not aware of that.
At this point, the core revision concept is too limited and contrib that goes into this direction might break any other contrib.

Looked again at the proposal/code and yes, i think it's OK combined with the followups proposed.

jstoller’s picture

It was my understanding that you could load a forward revision and save the entity, setting $newRevision = FALSE and $isDefaultRevision = TRUE, which would have the effect of making that revision the default (a.k.a. "current") revision, without creating a new revision or changing the content in any way. Is that not the case? Or, does it only work with the most recent revision? If so, I'll post a follow up issue to fix that oversight.

In any case, "Revert" is markedly different than "Set current." If the button is functionally the same as a revert, then it is a revert and we should continue to call it "Revert." I can see several use cases where you would want to revert to a forward revision (assuming there were multiple forward revisions on a node), so this does actually make sense. The one place where it does not make any sense to revert is on the most recent revision. In that case you certainly should be able to "Set current" and as I said, I'm pretty sure the API allows for the expected behavior, but I could be wrong.

Fabianx’s picture

#23: You are right that might even work by chance. The revert form currently statically sets ->revision = 1 and isDefaultRevision =1 for the node, so anything done on a forward revision will be a new revision.

--

On the ->revision = 0, ->isDefaultRevision=1 case:

However there is zero test coverage for that case in core (as far as I can see) and at least that should be provided - else this could break any time.

So if you want to rely on that behavior, lets at least add some test coverage (which is unfrozen at this point of beta anyway).

jstoller’s picture

jhedstrom’s picture

Note that without this patch, the UI simply uses the text 'Revert' throughout, making even less sense for a version that has not ever been the current revision.

jstoller’s picture

Imagine that I am using a contrib module that supports saving draft revisions. I have a "current" published version of my node, but I've been working on some changes, so I also have six forward revisions representing more recent drafts. Now I find that the last few changes I made were going down the wrong path and I need to revert my draft to my third forward revision.

In this scenario, I am creating a new revision that is an exact copy of an older revision, which is a textbook revert. The revision I'm reverting to has never been published (or "current"), but that's largely irrelevant. Furthermore, there's nothing saying that the new revision I make will be "current." In the above example I'm reverting my draft to an older draft, but the end result should still be a draft revision, leaving my current "current" revision unchanged.

jhedstrom’s picture

So this seems to have stalled.

Jaesin’s picture

FileSize
7.16 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,515 pass(es). View
7.01 KB

Re-roll. Sorry for the crappy interdiff.

Status: Needs review » Needs work

The last submitted patch, 29: 2429153-29.patch, failed testing.

edurenye queued 29: 2429153-29.patch for re-testing.

edurenye’s picture

Status: Needs work » Needs review
Jaesin’s picture

I had a conversation with our UX specialist about this issue and I walked away with the following ideas:

Why not just use "Activate revision" (Same for all) or something similar as the operation verb because the operation is the same either way.

I think adding more specific language on the confirmation form makes sense. And possibly even a warning if the published state is going to change like. Notice: "This action will publish currently unpublished content." and Notice: "This action will un-publish currently published content.".

Thirdly (if that's a word). How about adding a status column to the revision table so you would have an indicator of the status of the revision before clicking the "Activate Revision" button.

Lastly, we should rename the "Current revision" label to "Active revision" because the most current (from the perspective of time) revision would be the most "forward" and that would always be at the top of the list.

jstoller’s picture

Jaesin’s picture

You can test out this concept here:

https://marvelapp.com/1ab9a89#8112143

jstoller’s picture

@Jaesin: Looking at your example, I have no concept of what an "active" revision is. As shown, the "active" revision is neither the published revision (in fact it's the only revision that isn't published), or the most recent revision. I think most user's instincts when seeing this button will be to assume that activating a revision would publish it. Importantly, nothing about the term "activate" implies that a new revision will be created.

I think we need to take a step back and consider what actually happens when one pushes that button. If I understand correctly then, in all cases, pushing the button makes a new copy of that revision and places it at the top of the list. Assuming no content moderation modules are in play, this new revision will have the same publication status as the old revision being copied.

At this point things get a little fuzzy for me. If content moderation is employed and the entity in question already has a published revision, then my expectation is that the new revision will be a forward revision, in some sort of draft state, allowing further modification prior to publication. Is that correct, or will the new revision still take on the state of the old revision being reverted? Does this button always make the new revision the default (a.k.a. "current") revision, or, in the case of content moderation, will it be a forward revision?

So, the one consistent action here is that the revision who's button is pushed is copied. Thus, we could rename all the buttons "Copy revision," which would be accurate. In my mind, this is probably the best option if content moderation is employed AND the new revision is always a forward revision. However, "copy" does not do a good job of conveying the impact of your action on the publication status of your copy in the absence of content moderation. In that case, "revert" seems the better choice. Maybe its me, but "revert" just sounds like it has the potential to be more destructive than "copy." "Copy" sounds too nice and safe.

Now I think I've come full circle. Out of the box, "revert" seems like it's the most descriptive verb in every case. Even if there are forward revisions, "revert" works in every case except the most recent forward revision. However, Core doesn't support saving forward revisions in the UI and it seems to me that any module implementing forward revisions will likely need to completely override this form to deal with all the intricacies involved. I hate to say it, but at this point I think we should consider pushing all this to contrib. The more important issue, I think, is to make sure contrib can override this page when a more full-featured workflow is implemented.

Jaesin’s picture

What if there was just a "clone" button and instead of taking you to a confirmation page, it took you to a page to edit the unsaved clone based off the selected revision? Once it's saved, it's active, current and default.

That way we don't have to pretend the revision page actually let's us manage revisions and that can be left to contrib.

jstoller’s picture

That would be great! In most use cases I think opening an edit page would provide the best user experience. It lets users make small edits prior to saving and they are able to decide for themselves if the revision should be published. But cloning and reverting are somewhat different actions, so I'm not sure if this level of change is on the table.

Bojhan’s picture

Issue tags: -Needs usability review

I am not really sure, the latest is not really "current". What other alternatives to "set current" did we consider?

Bojhan’s picture

Issue tags: -Needs usability review

I am not really sure, the latest is not really "current". What other alternatives to "set current" did we consider?

dawehner’s picture

Active could be also a good name ...

dixon_’s picture

I like the word "default" which is also used on the API level at the moment. E.g. the "default" revision is what being loaded/shown/visible/published unless you don't ask for a specific revision. That makes sense to me.

"Active" seems too vague and imply that other revisions would be "inactive" or "deactivated" which they really aren't.

Jaesin’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Closed (duplicate)