Problem/Motivation

This is a follow-up for #218755: Support revisions in different states, which added API support for content moderation in Core. The related UX/UI implementation was left to this issue to solve.

This also aims to solve confusion around our existing UI for saving/publishing nodes (see #1123696: Does 'Save' publish my content? for details), while implementing the new "save as draft" functionality. The physical separation between the 'Save' button and the 'Publish' checkbox make it difficult for users to understand what will happen when they press the button.

Proposed resolution

Initial requirements include:

  • After clicking the 'Save' button, you should always see the text you just edited.
  • There should be a visual indication of the state of the revision you are viewing.
  • When you click 'Edit', you should edit the text of your most recent revision by default, and there should be a visual indication this is happening, but you should be given the option of editing the currently published revision instead (or any other revision).

Remaining tasks

  1. Settle on resolution
  2. Implement changes
  3. Update documentation

User interface changes

Under discussion...

API changes

A number of methods must be added to entities, to support the concept of an edit revision. See comment #74 for details.

References

Related modules

Original report by yoroy

Follow-up for #218755: Support revisions in different states:

You should see the text you just edited after you click the save button, and it should be given some sort of visual affordance for "draft mode." When you click "Edit," you should edit the text of your most recent revision, along with a visual affordance to let you know this is happening, and give you the option of editing the currently published revision instead.

http://drupal.org/node/218755#comment-6441358

Files: 
CommentFileSizeAuthor
#176 provide_a_better_ux_for-1776796-176.patch22.59 KBgrndlvl
#176 interdiff-1776796-169-176.txt16.58 KBgrndlvl
#169 1776796-169.patch16.97 KBJaesin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#169 interdiff-1776796_165-169.txt3.36 KBJaesin
#165 provide_a_better_ux_for-1776796-165.patch16.77 KBgoogletorp
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
#154 save-as-draft-1776796-154.patch21.53 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch save-as-draft-1776796-154.patch. Unable to apply patch. See the log in the details link for more information. View
#154 interdiff.txt2.01 KBjhedstrom
#153 save-as-draft-1776796-153.patch20.73 KBjhedstrom
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,313 pass(es). View
#153 interdiff.txt6.94 KBjhedstrom
#150 save-as-draft-1776796-150.patch17.92 KBjhedstrom
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,102 pass(es), 1 fail(s), and 0 exception(s). View
#150 interdiff.txt34.73 KBjhedstrom
#134 save-as-draft-1776796-134.patch79.94 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 50,381 pass(es), 729 fail(s), and 437 exception(s). View
#130 save-as-draft-1776796-130.patch77.47 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 49,726 pass(es), 788 fail(s), and 478 exception(s). View
#130 interdiff.txt462 byteseffulgentsia
#128 save-as-draft-1776796-128.patch77.47 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install. View
#115 save-as-draft-1776796-115.patch78.63 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 49,113 pass(es), 746 fail(s), and 420 exception(s). View
#115 Screenshot_2_11_13_9_21_PM.png75.97 KBjstoller
#115 Screenshot_2_11_13_9_22_PM.png24.93 KBjstoller
#111 Screenshot_2_10_13_9_22_AM.png18.41 KBjstoller
#105 Screenshot_2_7_13_12_07_AM.png28.06 KBjstoller
#105 Screenshot_2_8_13_5_08_PM.png13.15 KBjstoller
#105 Screenshot_2_8_13_5_10_PM.png31.28 KBjstoller
#105 Screenshot_2_8_13_5_11_PM.png31.67 KBjstoller
#105 Screenshot_2_8_13_5_13_PM.png28.35 KBjstoller
#105 Screenshot_2_8_13_5_14_PM.png16.14 KBjstoller
#105 Screenshot_2_8_13_5_17_PM.png65.2 KBjstoller
#105 Screenshot_2_8_13_5_19_PM.png41.06 KBjstoller
#105 save-as-draft-1776796-105.patch73.31 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch save-as-draft-1776796-105.patch. Unable to apply patch. See the log in the details link for more information. View
#95 Revisions-Table-1.jpg40.07 KBeigentor
#94 Screenshot_2_5_13_9_57_PM.png29.89 KBjstoller
#94 Screenshot_2_5_13_10_02_PM.png33.76 KBjstoller
#94 Screenshot_2_5_13_10_03_PM.png33.85 KBjstoller
#94 Screenshot_2_5_13_10_10_PM.png36.84 KBjstoller
#94 Screenshot_2_5_13_10_15_PM.png35.92 KBjstoller
#94 Screenshot_2_5_13_10_28_PM.png39.92 KBjstoller
#94 Screenshot_2_5_13_10_33_PM.png45.51 KBjstoller
#94 Screenshot_2_5_13_10_33_PM.png45.51 KBjstoller
#94 Screenshot_2_5_13_11_02_PM.png33.82 KBjstoller
#94 Screenshot_2_5_13_11_07_PM.png44.69 KBjstoller
#94 Screenshot_2_5_13_11_09_PM.png54.44 KBjstoller
#92 annotations-current-state.png74.44 KBeigentor
#92 tree-structure-1.png31.73 KBeigentor
#86 Screenshot_2_3_13_10_51_PM.png55.45 KBjstoller
#86 save-as-draft-1776796-86.patch73.81 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 48,445 pass(es), 472 fail(s), and 211 exception(s). View
#84 Screenshot_2_3_13_3_56_PM.png95.3 KBjstoller
#84 Screenshot_2_3_13_3_58_PM.png57.72 KBjstoller
#84 save-as-draft-1776796-84.patch67.36 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 48,475 pass(es), 472 fail(s), and 211 exception(s). View
#78 save-as-draft-1776796-78.patch55.94 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 48,084 pass(es), 462 fail(s), and 211 exception(s). View
#74 save-as-draft-1776796-74.patch54.24 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 48,820 pass(es), 458 fail(s), and 210 exception(s). View
#62 Screenshot_1_22_13_9_09_PM.png30.5 KBjstoller
#62 Screenshot_1_22_13_9_09_PM.png30.62 KBjstoller
#62 Screenshot_1_22_13_9_10_PM.png34.3 KBjstoller
#62 save-as-draft-1776796-62.patch43.69 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 49,994 pass(es), 578 fail(s), and 282 exception(s). View
#57 save-as-draft-1776796-57.patch33.8 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 46,606 pass(es), 3,319 fail(s), and 2,202 exception(s). View
#55 save-as-draft-1776796-55.patch30.56 KBjstoller
FAILED: [[SimpleTest]]: [MySQL] 49,389 pass(es), 684 fail(s), and 291 exception(s). View
#53 save-as-draft-1776796-53.patch16.74 KBstevector
PASSED: [[SimpleTest]]: [MySQL] 50,579 pass(es). View
#49 save-as-draft-1776796-49.patch16.8 KBstevector
FAILED: [[SimpleTest]]: [MySQL] 50,545 pass(es), 36 fail(s), and 0 exception(s). View
#37 37-1776796-revision-edit.png62.51 KBmitchell
#37 37-1776796-revision-edit.bmml_.txt7.13 KBmitchell
#36 wp-doc-revisions.png104.86 KBmitchell
#34 34-1776796-revisions_ux.png43.93 KBmitchell
#34 34-1776796-revisions_ux.bmml_.txt2.88 KBmitchell
#32 drupal-revisions-page.png67.04 KBAlan D.
#27 1776796-revision-history-v2.png77.2 KBjstoller
#24 1776796-revision-history-2.png72.96 KBMustangGB
#22 1776796-no-revisions.png60.1 KBjstoller
#22 1776796-view-tabs.png149.58 KBjstoller
#22 1776796-revision-history.png73.52 KBjstoller
#22 1776796-edit-draft.png35.67 KBjstoller
#14 Workbench1.jpg98.75 KBGábor Hojtsy
#14 Workbench2.jpg78.12 KBGábor Hojtsy
#14 Workbench3.jpg110.18 KBGábor Hojtsy
#10 RevisionsComparison.jpg65.13 KBGábor Hojtsy
#1 RevisionsUI.jpg42 KBGábor Hojtsy

Comments

Gábor Hojtsy’s picture

FileSize
42 KB

Currently there is no editing at all for revisions :) You have the "revert" control, which makes a copy of the revision as the latest revision and then you can edit that. We should define goals as to what we want to edit in a revision, what happens when you edit a revision (do we overwrite it?, do we save a new revision that then might be ahead of all other revisions, etc.)

RevisionsUI.jpg

klonos’s picture

Title: Provide a better UX for editing draft revisions » Provide a better UX for creating & editing draft revisions.

Since this is about the UI and in order to back up @webchick's suggestion from #218755-219: Support revisions in different states I think that if there is a checkbox in the edit/create page, it should be labeled "Save as draft" instead of the "Set as default revision" we had and it's functionality should be inverted and default to false/unchecked.

moshe weitzman’s picture

1. Yeah, upon save you go to the revision you just worked on.
2. When you look at a revision that is not the published one, you should get a message that includes one or two links, depending on which revision it is - "You are viewing at a non-default revision. You may edit the [default revision] or the [most recent revision]."

Kiphaas7’s picture

Quoting Webchick in #219 and #220 of #218755: Support revisions in different states for bookkeeping:

Webchick
Here's a ~4-minute walk-through of the functionality, since that was easier than taking an ass-load of screenshots:

https://dl.dropbox.com/u/10160/default-revision-review.mov

The two biggest WTFs (and arguably commit blockers) are:

- After editing a draft revision, you are redirected back to node/X which still has the published text. Clicking "Edit" similarly loads the text from the published revision, not the text you just edited. This is totally discombobulating.
- Worse, there is actually no physical way to further refine said draft revision. Your only choices are to either delete it or publish it immediately, which essentially defeats the entire purpose.

I believe that there is probably value in getting the API change in alone, but without a sensical UX around it (which likely makes sense to start discussing in an issue that's not already at 200+ comments), this functionality currently seems to do more harm than good. Should we remove the UI entirely from this patch and make it an API-only thing for now? I dunno. Let's discuss.

If we were to commit this as-is, here would be the follow-ups I can spot:

- (critical task :\) Provide a better UX for editing draft revisions. You should see the text you just edited after you click the save button, and it should be given some sort of visual affordance for "draft mode." When you click "Edit," you should edit the text of your most recent revision, along with a visual affordance to let you know this is happening, and give you the option of editing the currently published revision instead. (Note: If we don't commit any UX with this patch, I think this would become a major feature instead and not count against thresholds.)
- (normal feature) Allow editing of revisions from node/X/revisions. Any revision should be able to be the base of a draft, really.
- (normal task) Change "Set as default revision" to "Set as published revision." Even better, actually, might be inverting the checkbox label / value. Make the label "Save as draft revision" and default it to false. If a contrib workflow module wants to add fancier draft options, they can hook_form_alter() the label/default value to something else, or replace it entirely. Not core's problem.
- (normal task) Add "Set as published revision" / "Save as draft revision" checkbox to the content type edit form, so that you can choose to put some content types through a default draft workflow.
- (normal task) Re-label "revert" in node/1/revisions operations column to "publish" for "future" revisions. Also adjust the text there to include this new capability. (e.g. "Revisions allow you to track differences between multiple versions of your content, revert back to older versions, and publish draft revisions.")

What say you?

Webchick
For those in a hurry (and aren't we all), ~2:30 on has the salient points. The rest is context for people coming into this patch fresh.

Kiphaas7’s picture

#3 totally agree. However, here's some bikeshedding:

non-default revision sounds a bit too abstract to me: I'd prefer to call it non-published in the UI. In my mind the message could be:
"You are viewing a non-published revision. The published revision is <link, revision id/title/??> and the most recent revision is <link, revision id/title/??>

Maybe make it a bit smarter to show only a single link for the common case where published == most recent.

moshe weitzman’s picture

Yeah, that's better language, and yes to single link.

John Pitcairn’s picture

@Kiphaas7: Agreed++. Some of my clients have a hard enough time getting their heads around published/unpublished, let alone vague concepts like "default".

I wonder, what are the chances of there being an admin config page where the UI terms for revision states can be set? So I could do something like:

draft, never published -> "draft"
current/default -> "published"
older than default (whether published or not) -> "archived"

Gábor Hojtsy’s picture

The revision you refer to as published might not be actually *published*. Whether the content is published depends on the status flag of said revision. This was discussed to death on the previous issue and the terminology issue linked from there. As per the current terminology:

- revision: any version of the node
- default revision: one specific vision that is considered the front-facing version (there might be other revisions saved before and after this one)
- published: if the default revision has the status flag as TRUE (any other revision might have the flag TRUE too, indicating the revison was published earlier/to be published later)

Kiphaas7’s picture

Gabor, so if I understand correctly:

  • for a revision to be published it needs to have state === TRUE and default === TRUE?
  • if a revision is not default and but has state TRUE, it was a previously published (but isn't anymore)
  • if a revision is not default and and has state FALSE, it hasn't been published (yet)

If the above is true, can't it be renamed to published, unpublished and previously published? I seriously had to read your reply 3 times to grep what default means, which had nothing to do with how you wrote it, but all the more because of the content. Even it was discussed to death in the previous topic, I forsee a lot of WTF's regarding default/published otherwise.

Gábor Hojtsy’s picture

FileSize
65.13 KB

Here is a quick drawing of the D7/D8 difference:

RevisionsComparison.jpg

Some possible workflows include:

A. Users edit a node to be published later; there are numerous revisions but the node was never published; none of the revisions are published (yet), one of them is the default revision, not necessarily the latest one based on review workflow.
B. A node that is published gets new revisions that are not yet front facing (there are draft edits to the node that are to be published later theoretically). When publishing one of these will be picked to be published, not necessarily the latest one.
C. A node that was previously published gets removed from the site (the default revision is not published anymore).

From the system's point of view, a node in A and C looks exactly the same, the node has various revisions, the front facing (default) revision is not published. However, based on the history of the node for A, all revisions would be considered drafts (none of them was ever published) and all of the revisions of C would be considered archived (they were either previously published or previously draft). If we follow the terminology suggestion that all revisions before the published ones are to be called archived and all after the published one should be called draft that is. I think its important to realize that "is default / is not default" and timestamps not necessarily indicate which workflow state a revision is in, so we should not jump to assumptions right away...

A bit more complicated is B, where several drafts were written but not the latest version was picked for publication (btw, due to the confusions around this, workbench_moderation does not even allow for this in my understanding). In this case, some forward revisions will remain but those are "archived" due to not being picked for publication. The current core workflow of copying a revision to be up front all others when reverting to it works around this by making the default revision the latest one, so we can safely call (in this case) all previous revisions archived. However @jstoller was arguing this should not be the case, and you should be able to pick any revision as default without this copying around behavior (which introduced the problem I explained).

John Pitcairn’s picture

I don't much care what the API calls the states. I care about what the UI calls the status + state + revision-id combination.

I care what my client has to deal with, so I want to be able to control what they see, ie manipulate the displayed terminology if I don't like it. If we're talking about a fairly typical, simple workflow where content is not published immediately but there also isn't a formal review process, my clients typically think of each content revision as being in one of three basic states:

A - never published and later than the default revision, or there has not yet been a default revision - DRAFT
B - published and default - PUBLISHED (there is only one revision in this state)
C - older than the default revision, or is the default revision but has been unpublished - ARCHIVED

I'm a fan of the Revisioning module in D6, which I think gets this basic workflow UX pretty right, from the client's point of view. "Edit" creates a new revision, in draft, or edits a single draft revision (this behavior is configurable per-node-type). You're taken to the draft node-revision view after saving, and can "publish" that draft revision directly from the draft node-revision view, without having to go elsewhere to do so. My clients can figure this out with little training.

Gábor Hojtsy’s picture

John: right, so that gets across the point that historic information of how we got to the current state is important, not just the states of each revision. We don't currently have that historic information in core. It might be as easy as another flag on the entity as to whether it was ever published or the entity is archived altogether - for the 80% use case.

John Pitcairn’s picture

I agree a value that gets irrevocably set when an entity is published for the first time would be a very useful thing. In the past I've resorted to a custom module or a field on the node to track exactly that, using a timestamp, so a "published date" could accurately reflect the first publication date, not the creation date or last-publication date.

Gábor Hojtsy’s picture

FileSize
110.18 KB
78.12 KB
98.75 KB

Here is the workbench UX to turn the discussion to possibly a more holistic level. Some key points:

- Workbench modifies the node tabs as appropriate for each state, so you know if you view the published or draft
- You can only edit the latest draft
- If you revert the revision, a new copy of the revision is created just like with core only

Node has no published version yet:

Workbench1.jpg

Node has a published version finally:

Workbench2.jpg

Node has draft revisions submitted later than the published revision:

Workbench3.jpg

Note the adjustments on the UI especially the interaction guidance with the node tabs. Contrast this with the revisions tab above.

John Pitcairn’s picture

Feedback I have had from nearly all my clients of Workbench Moderation vs Revisioning is that they don't like having to go to the separate "moderate" tab to publish the draft revision they are looking at. They lose context. Revisioning provides a "publish this" link (menu secondary link) directly on the draft revision view, so they are seeing exactly what they will publish when they click "publish this".

The same applies to revert - a "revert to this" link on the revision that they are looking at is far less error-prone than trying to find the appropriate revision in the list.

Workbench Moderation provides no confirmation step for state-changes, which happen as soon as "apply" is clicked. Most of my clients have indicated they would prefer a separate confirmation step before any state-change occurs.

All my clients like the separate "view published", "view draft", and "edit draft" tabs that Workbench Moderation provides. These help clarify the current status of the entity.

Workbench Moderation does not add any tabs when viewing a revision which is neither published nor the latest draft. This has a tendency to confuse some of my clients, and I believe the usual tabs should be shown in this case, with an additional active tab that describes the state of the revision being viewed, something like: View published | View draft | Archived revision | Moderate

"Moderate" is a very obscure term to most of my clients, who typically come from small-business backgrounds and have little or no web forum experience. I generally override that tab title to "history", which they find more descriptive.

One final modification I make when using Revisioning is to add "next revision" and "previous revision" links to each revision view. Clients can use these to directly step through revisions when they are attempting to find and revert to a previous version, as opposed to viewing the revision list, clicking a revision, going back to the revision list, trying to find the next revision, etc. So primary and secondary tabs on a revision view might look something like these examples:

View published | View draft | Edit draft | Archived revision | History
<< Previous revision | Revert to this | Delete this | Next revision >>

View published | View draft | Edit draft | History
<< Previous revision | Unpublish this | Edit this | Delete this | Next revision >>

View published | View draft | Edit draft | History
<< Previous revision | Publish this | Delete this

Hope this helps.

John Pitcairn’s picture

Ack, double post.

My general goal with the above is that clients ideally don't have to deal with the big, scary revision list at all, unless they are looking for a revision with a specific revision-log message (though I find they usually don't add those anyway).

klonos’s picture

I really like the idea of this handy, hybrid pager/actions sorta thing.

jstoller’s picture

Issue tags: +Usability, +EditorUX

Remember we need to consider the button/checkbox mess on content edit pages. #1123696: Does 'Save' publish my content? had a lot of discussion along those lines.

Also, we should keep in mind that this needs to work with entities other than nodes. I'll want a similar UX when editing Beans, Field collections, Relations, etc.

Finally, whatever we do should be adaptable to multi-state workflows. I'm still holding out hope for #1777388: Support arbitrary workflow states, but even if that doesn't get in there will be contrib modules implementing multi-state workflows and we shouldn't force them to do crazy hackish feats of contortion. In fact, we should strive to make their lives easier.

John Pitcairn’s picture

Issue tags: -Usability, -EditorUX

For a revision-view tabbed state/action/navigation widget, if you want to tweak the terms used or remove/rearrange/add tabs based on some workflow-specific logic, you can do a lot with just hook_menu_alter() and associated callbacks.

John Pitcairn’s picture

Issue tags: +Usability, +EditorUX

Sorry, cross-post, lost the tags

John Pitcairn’s picture

#18: I think a "Save as" button with an associated select element listing the available save states would improve things considerably. The select element should contain only the states that the current user role can transition content to, and a configurable per-role default value for the select element might be useful.

If there is only a single state that the current user role can save as, the select element should be hidden and the button text changed accordingly, ie "save as draft".

On the keep-it-simple front, I do not think the "published" checkbox should exist in the UI if there are also workflow states. Entity->status (which I understand may be required for historical reasons) could perhaps be set/unset automatically when the entity transitions to a specified workflow state.

John Pitcairn’s picture

Issue summary: View changes

Fixed issue link

jstoller’s picture

Here are some mock-ups showing how this might be implemented.

Revisioning disabled

First lets look at the UI when revisioning is disabled. No big changes here. We would need an element on the edit form to "Enable revisions for this node".

1776796-no-revisions.png

Revisioning enabled

If you haven't seen #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text, I recommend reading the summary of that issue, as I will be using some of the terminology discussed there. However, terms like "default" and "active" don't make for very good UX, so in the absence of real workflow states (see #1777388: Support arbitrary workflow states), I'm defining the following pseudo states for revisions:

  • Published: applies to the default revision if status=1
  • Archived: any revision older than the default revision, with status=0
  • Archived published: any revision older than the default revision, with status=1
  • Draft: any revision newer than the default revision, but older than the active revision, with status=0
  • Active draft: applies to the active revision if status=0

Viewing the node

The image below shows the UI when viewing a node in various contexts. Read the node body text for descriptions. Some things to note:

  • The "View draft" tab is displayed when there is an "Active draft" revision.
  • The "View published" tab displays when there is a "Published" revision.
  • The "Revision history" tab appears anytime there is more than one revision. "Previous version" and "Next version" sub-tabs will also appear as appropriate.
  • When the default revision is also the active revision, the edit tab just reads "Edit". In all other cases, the Edit tab reads "Edit draft".
  • When viewing a Draft, or Active Draft revision, the "Publish this version" sub-tab is displayed. If the node only has one revision, then it will just read "Publish this". Clicking this button will save the revision with status=1 and isDefaultRevision=TRUE.
  • When viewing a Published revision, the "Unpublish this" sub-tab is displayed. Clicking this button will do one of two things: a) If viewing the active revision, then we set status=0 for this revision and in the base node table; or b) if there is a more recent Active Draft, then we leave status=1 on this revision, but save the active revision as the default revision, thus maintaining the history of this revision as previously published. In either case, the user should be returned to viewing the same revision they started on, even if it is no longer the default revision.
  • When viewing any revision that isn't Published, the "Delete this version" sub-tab is displayed. Clicking it deletes the viewed revision and sends the user to view the next most recent revision. Unless the active revision is deleted, in which case the user is sent to the next oldest revision. The published version must be unpublished before it can be deleted.
  • When viewing any revision other than the active or default revisions, a "Version #vid" tab is displayed. Indicating the VID in the tab gives users a point of reference, so they know where they are and connects the viewing tab with the revision history.
  • When viewing any revision other than the active or default revisions, a "Revert to this version" sub-tab is displayed. Clicking this will go to the edit tab, with the form pre-filled with the older revision's content. This lets users make modifications to the content prior to saving it. When saved, this will create a new active revision and will not modify the old revision.
  • You should only be able to disable revisioning on a node if all but one of its revisions has been deleted.

1776796-view-tabs.png

Revision history

No big surprises here. The actions listed for each revision work the same as the corresponding tabs/sub-tabs when viewing a revision. I've also included checkboxes and bulk operations, which in the base implementation would simply let you delete multiple revisions at once.

1776796-revision-history.png

Editing content

The image below shows a node with published and draft revisions, but it would be essentially the same in other contexts, plus or minus some tabs.

I've done away with the "Publish" checkbox. Instead, there are two save buttons: "Save as draft" and "Save and Publish". Clicking either of these buttons will save a new revision of the node. "Save as draft" will set isDefaultRevision=FALSE and "Save and Publish" will set isDefaultRevision=TRUE. This removes any ambiguity about what will happen when you save a node.

I also pulled the revision log field out of the vertical tabs and placed it right above the buttons at the bottom of the page, providing a direct association between the log and the act of saving. I think this will make the log more useful and more used.

1776796-edit-draft.png

Workflow

You'll notice I'm enforcing a fairly strict linear workflow here. I'm not providing any way, through the UI, to edit an existing revision. The expectation is that every time you click save on a node edit page, you're creating a new revision.

I went back and forth on this. I know there are use cases for editing existing revisions and I definitely think contrib should be able to override the default behavior, but at the end of the day, enforcing this linear revision progression is far easier to understand and will provide a much better user experience out of the box.

Now lets look at some use cases. Generally users might save some drafts when they first create a node. Then at some point they'll publish the Active Draft of the node. Then they might make some edits, saving more drafts, and so on. But what if you're working on big changes in a new draft, when you realize you need to make a quick change to the published version. This is where reverting comes in. You would go either to the "View Published" or "Revision history" tab and click the revert link. This will load your published revision into an edit form, so you can make your changes. Then click "Save and publish" to immediately publish your changes. Crisis averted. But what about the big changes you were working on before? Well, they're still there in the older revision. So click the "Previous version" button to go back to the archived revision, then click the "Revert to this version" button and continue editing your draft where you left off.

In addition to reducing the complexity of dealing with multiple revisions, this linear approach also takes full advantage of revisions' ability to track the editing history of content. My revision history will now provide a clean, readable history of everything that's been done to the node.

About reverting

I've been struggling a bit with this idea that what we're calling "reverting" isn't really reverting. If we were truly reverting to a previous revision, then we would just move the VID pointer in the node table to that revision. What we're doing is creating a new revision that happens to start with the same content contained in the old revision. Now, I'm not suggesting we change how reverting works, but we may wish to think about what we call it. I've seen users confused by this term before. Would "copy" or "duplicate" be more intuitive than "revert"? Or would they imply you're creating a new node, rather than a new revision? Does anyone have any other ideas here?

Revision vs. version

If you've seen #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text then you know I'm not a fan of the word "revision" in the UI. I have come across many non-technical users who just don't understand what the word means, while "version" tends to be more universally understood. In fact, my impression is that if you use the words version and revision interchangeably on the same page, then the former will improve comprehension of the latter. As such, I am using the rule currently set in the referenced issue. I use "revision" when talking about a collection of revisions (i.e. revision history), but use "version" when just talking about a single revision (i.e. this version).

MustangGB’s picture

I would suggest that Archived and Draft both should be called Draft. Additionally Archived published should just be called Archived and Active draft should be Draft (active), if this one is even needed at all.
i.e.

  • Archived -> Draft
  • Archived published -> Archived
  • Active draft -> Draft (active)

Along with this the use case you discussed sounded pretty horrific to understand. I think that, with the above naming changes, in the "Revision history" tab that Archived revisions (currently called Archived published) should have the Revert action, but clicking it will not take you to an edit page, instead it will Publish the revision immediately. Also on the "Revision history" tab Draft revisions (which means both Draft and Archived in the current namings) will not have a Revert action, but instead an Edit action, which will take you to the edit page with the usual options, "Save as draft, Save and publish, etc."

MustangGB’s picture

So it might look something more like this (sorry for my poor Photoshop skills)

Gábor Hojtsy’s picture

@jstoller: Sorry I don't have the time to comment on the overall vision now, so just one thing. For me personally, the extensive list of buttons at places, eg. Preview|Save as draft|Save and publish|View changes|Delete|Cancel is pretty intimidating. That looks like a scary big set of buttons there.

John Pitcairn’s picture

@jstoller: Liking this in general, thanks for putting in all that effort.

@Gábor Hojsty: I agree that the big set of buttons at the bottom of the editing form is a bit too much. Not sure what "view changes" does, and if the opportunity to "save as draft" is available, then does "preview" make much sense, and how is it different from "view changes"? I usually disable previews in a revisioning workflow.

@akamustang: I don't agree on terminology. I think everything older than the published revision is simply "archived" (regardless of whether it was ever published), and everything newer than the published revision is "draft". If users need better descriptions then that's what the log messages are for. Either that or make the difference more explicit - "Archived published" and "Archived draft".

Perhaps instead of "revert" or "edit", the link should be something like "new draft", which is a better explanation of what will happen.

I think either use "revision" or "version", not a mix of the two. My preference would be to retain "revision", since that's what any users coming from D6 or D7 will be more familiar with.

jstoller’s picture

@Gábor Hojsty: I agree that's a lot of buttons, but some of the intimidation factor could be dealt with in the layout.

@John Pitcairn: The "Preview" button still has its place, I think. Saving a draft could potentially trigger email notifications and such, so we may want the ability for people to just see what they have before they save. I could be convinced otherwise, however. Especially if #1777388: Support arbitrary workflow states gets done, so we can have a true draft state.

The "view changes" button was assuming Diff in Core, but it can be removed. In fact, I'm thinking this would work better as a checkbox option when viewing the preview anyway.

The "Cancel" button is also a matter of some debate. I think it works best as a button, but many feel it should be a link instead (see #116939: Add a Cancel link on forms).

@akamustang: If we were to allow immediate reversion to a previously published revision, without going through the edit form, then we should do so by updating the base node table so it points to the older revision as the default. However, I think this would be a mistake. It is much cleaner to create a new revision when reverting and we should always give users the option to make small adjustments prior to publishing their reverted content. I stand by my recommended workflow.

I also tend to agree with @John Pitcairn that all revisions older than the Published revision are "Archived". However, I've made some alterations to the naming conventions, shown below:

1776796-revision-history-v2.png

Now I'm distinguishing between "Archived (draft)" and "Archived (published)". I've also changed "Active Draft" to "Draft (active)". Finally, I've changed the "Revert" link to "Copy to draft", which is the most discriptive term I've come up with so far. I would prefer something shorter, if possible, but I'm not sure if "new draft" is intuitive enough. This probably deserves some user testing.

@John Pitcairn: Regarding revision/version, I do not think people who understand what a revision is will have any problem understanding that a version is the same thing, so I'm not at all worried about our D6 and D7 people. However, I am worried about non-technical content managers. The approach I'm proposing is similar to what's done on Wikipedia and I've found it to be a huge improvement over Drupal's more consistent use of "revision". Sometimes changing terminology is a good thing. However, we should probably save that debate for #1643354: [Policy, No patch] Overhaul the entity revision terminology in UI text.

John Pitcairn’s picture

Finally, I've changed the "Revert" link to "Copy to draft", which is the most discriptive term I've come up with so far. I would prefer something shorter, if possible, but I'm not sure if "new draft" is intuitive enough. This probably deserves some user testing.

User testing ++. Most of my suggestions arise from informal user testing on my clients.

Is (active) really necessary? In the 80% use-case wouldn't the active draft be the latest draft?

I'd possibly like to see a separate "revert" link if the user has appropriate permissions. My clients generally think "revert" means "make this the published version", and would be one-click followed by a confirmation screen, then it's published.

Brainstorming a few alternatives for "copy to draft":

Edit
Edit (draft)
Edit new draft
New draft
New active draft

jstoller’s picture

Is (active) really necessary? In the 80% use-case wouldn't the active draft be the latest draft?

In the workflow I've presented, the active draft will always be the latest draft, but I don't think it hurts to emphasize that fact.

I'd possibly like to see a separate "revert" link if the user has appropriate permissions.

Would you allow this on all previous revisions, or just on previously published versions? Would you still copy the old content to a new revision, or just make the old revision the default?

I thought perhaps you could include the option to publish the reverted revision or save it as a new draft on your confirmation screen. But then I thought, how is that any different than going to the edit screen, which already has buttons for each of those options? If you're going to have a confirmation screen anyway (and you need to have one IMHO), then why not just let the edit page be your confirmation screen? That means you have one less link to worry about, since "Revert" and "New draft" are effectively the same thing.

John Pitcairn’s picture

I think any user action should create a new revision. When reverting I think a new published revision should be created on the top of the stack, and an appropriate log message added automatically, something like "Reverted from revision #143 by [user]".

Revert means "copy this to a new active draft and publish it" . Edit means "copy this to a new active draft and edit that". Programmatically they may be very similar, but conceptually they can be quite different for users, so there should be an action link for each.

Revert should mean "revert the published state to EXACTLY this revision", and would be useful in the situation where something needs to be rolled back right now, no mistakes. So revert needs to be available on archived draft revisions too - it's possible that someone has screwed up and published the wrong draft, and needs to publish the correct draft as fast as possible. I think giving users the opportunity to change the content of the reverting revision before publishing it would be a mistake. Just revert it.

The revert link would also appear on the node revision view, and I think this is where it is most useful, while the the user is looking at exactly the content that the action will apply to.

I think the edit page is rather too complicated as a confirmation screen, and the relevant buttons are likely to be offscreen. There is a lot of potential distraction there. A confirmation screen needs to clearly explain what will happen and present as few options as possible, preferably just OK and cancel. I'm in the "link" camp for cancel, BTW.

John Pitcairn’s picture

The revert link would also appear on the node revision view, and I think this is where it is most useful, while the the user is looking at exactly the content that the action will apply to.

Experience with several of my clients suggests that the action links for each revision on the revision history list are in fact fairly dangerous. It's all too easy to do something to the wrong revision (especially revert), because there isn't enough context there to select the correct revision.

Alan D.’s picture

FileSize
67.04 KB

I am coming in cold to this discussion, but as such it also provides a bit of novice UI feedback.

The tabs, wouldn't just "View", "View draft", "Edit draft" do? We have up to 9 tabs hanging off some node types, so I'm thinking in terms of space here. Also the title on a single line looks better imho.

Regarding the table:

  1. The version column seems redundant to me. How and why would a "standard" admin need to know this?
  2. The title is also taking a lot of space, and would rarely change. Is this needed?
  3. The editing author would be useful here.
  4. Mixing the status and state is confusing even though I know what these mean (I think). Maybe simply add a common suffix to the state that signifies that this revision is unpublished.

So with all of these changes you would get:

Alan D.’s picture

Actually, don't use author as this suggests the author field! (Way too many naming conflicts with these issues) Maybe a Submitted column combining both date and author, or rename the author column to Editor / Edited by / ?.

mitchell’s picture

Component: node system » entity system
FileSize
2.88 KB
43.93 KB

long-diff_revision-view

This is an example Documentation page for 'Module X' shown in "Long Diff View", which has the essential info organized into as small a window as possible. Its uses are for viewing revision history, but not for doing bulk edits.

Characteristics:

  • Each field of the entity's revision is shown in the revision-row's columns.
  • Widgets / fields for revision selection, action links, and log messages aren't used.
  • Editor is used in place of author.
  • Date is compacted.

Potential additions:

  • Exposed filter for authors
  • Entity selector for adding documents
  • Editable fields

Potential changes:

  • Change default sort of dates to ascend
  • Add 'Revise' button below list of revisions

---

Last edit: submit + 48m

Bojhan’s picture

This issue seems to be going a little overboard. Can anyone summarize precisely what we want to achieve here?

mitchell’s picture

FileSize
104.86 KB

I apologize for not reading much of the history. I had come over from Diff and was trying to speak to revision logs versus revision history views, but that wasn't very helpful with defining a UX for editing revisions.

After watching the video in #4 and looking at #218755: Support revisions in different states and #1777388: Support arbitrary workflow states, I get the sense that this issue is about creating a default workflow in core, which to me, trends toward something like the UX of Wordpress Document Revisions:

Wordpress Document Revisions Screenshot

This screenshot seems to unify a lot of the comments above and answers the issue summary on a single, reusable page.

mitchell’s picture

I mocked up an edit screen for "the current revision of a document entity". It uses Bojhan and yoroy's recent Design iterations for the content creation page as well as the Wordpress Document Revisions UX. Is this closer to how you guys imagine a revisioned entity edit screen?

revision-edit

Since this example should be a somewhat generic entity, and I wanted to include familiar fields, I used a documentation page, Develop for Drupal. The fields / field groups are split between content body and side pane by revisionable vs non-revisionable.

The VCS-style branching on the right pane is meant to provide links to each "named revision", and going to one shows a notification message at the top of the screen. Editing it creates a child of that revision. There is no checkbox to 'create a new revision', so that is assumed at each edit.

I added a "Compare revisions" block, above the content body, to select two revisions by name. That may be better suited for a history 'view'. What do you think?

jstoller’s picture

I fear this discussion may be getting a little off track. This is not about creating better revision logs or implementing VCS-like branching workflows. We are just trying to update the UI for editing entities and provide a better UX, taking into account the new concept of a default revision, which is no longer necessarily the latest revision.

Unless something changes (please, please, please), the only publication workflow in Core will remain a simple two-state published/unpublished workflow and that's all we need to support. Though ideally what we do should be extensible to support multi-state workflows without too much trouble, since we know contrib will do so if nothing else.

jstoller’s picture

Issue summary: View changes

Expanded issue description using issue template and added more references.

jstoller’s picture

I added #563550: Change "create new revision" check box as a related issue in the project summary and marked it as duplicate of this.

hass’s picture

May be off-topic here, but has someone already thought about keeping attachments in secured private folders until it get's published? If it's published the files moved to public and there is no longer a security check on the file attachments.

jstoller’s picture

@hass, re:#40, that sounds like a fine idea, but is outside the scope of this issue. Please consider posting it as a new issue in the queue.

hass’s picture

hass’s picture

Issue summary: View changes

Added issue #563550 as related issue.

jstoller’s picture

Title: Provide a better UX for creating & editing draft revisions. » Provide a better UX for creating, editing & managing draft revisions.

I added #1510532: [META] Implement the new create content page design to the list of "related issues" in the issue summary. We should pay very close attention to what's happening there, so as not to duplicate the discussion here.

peterx’s picture

Something worth looking at is the option to retain the original author. Why do we need that level of control? Because some editors edit on behalf of authors and the node should remain attached to the author no matter what the editor does. Editor A edits something for author B. The content never belongs to Editor A. Editor A might be one of dozens of editors editing the content. My first book had three editors working through it for different reasons. None of them were contributing content. The content never belongs to Editor X or Y or Ms Zed.

Because editors sometimes replace the author when multiple authors work on content and the current author is unavailable. Editor A changes the author from B to C. The content never belongs to Editor A.

http://drupal.org/node/1240850, Publishing/Unpublishing a revision changes the author of that revision, tackles a problem with node_save by changing the user id first, then saving, then changing the user back. Allowing a user id in node_save would be a better solution. Changing the workflow to let us retain the original author would be better. Log editors in the history of changes but do not change the author.

There were several issues open for node_save and all are closed as duplicates of something else in a trail leading to this issue but this issue does not mention the original problem of node_save changing the author. Somewhere in the revisioning workflow, we need the option to keep the original author.

lpalgarvio’s picture

jstoller’s picture

@peterx, re: #44, it sounds like your problem could be solved if we tracked the "owner" of an entity in the base table (i.e. {node}), in addition to tracking the author of each individual revision. I support the idea, but what you're requesting is an API change and outside the scope of this issue. I suggest opening a new issue in the queue.

peterx’s picture

#44 opened as http://drupal.org/node/1837906 as per #46.

jstoller’s picture

It seems this issue has fallen by the wayside, while its importance has not diminished. Does anyone have the ability to work on a patch between now and feature completion in February?

It seems there's still some discussion to be had about the structure of the table on the Revision History tab, (should this be broken into a separate issue?). And implementing changes to content editing pages has been moved to #1510532: [META] Implement the new create content page design, so I think we can safely ignore that here. Other than that, I still stand by the tab/sub-tab configuration I show in #22. Can we implement that?

stevector’s picture

FileSize
16.8 KB
FAILED: [[SimpleTest]]: [MySQL] 50,545 pass(es), 36 fail(s), and 0 exception(s). View

Implementing the revision management here implies that "Save as Draft" is functional. That piece is currently not in the core UI. I recently verified that it's possible in in contrib and that it is easier to do than in D7.

Here's a patch that does a very quick and dirty implementation of saving forward revisions through the UI. This is essentially a very light version of Workbench Moderation. The most valuable thing here is probably these tests. The implementation itself is full of @todos and places that are node-specific that should be entity-type generic. The actual form field for selecting draft/[published in this patch is done as a Field API field. That's not how I'd want to implement it going forward but it was a good way to just get the concepts testable.

Anyway, if someone wants to get Save as Draft in core, this should help.

webchick’s picture

Awesome! Marking needs review, since there's a patch.

Stevector, did you run into anything else that we could still clean-up in D8 to make it easier for something like Workbench Moderation?

stevector’s picture

Status: Active » Needs review

Hi webchick,

A piece of cleanup could be to get rid of some entity-type specific hooks like hook_node_presave() vs. hook_entity_presave(). Perhaps the most important line of code in this patch is $entity->set('isDefaultRevision', FALSE);. That's the big picture here. A revision is getting saved and this module wants it to be a "forward revision" (not the "default" revision). Right now I've got that code in hook_entity_presave because ideally this module could handle entities of all types. A potential problem is that anything responding to hook_node_presave() will run first and see an entity object with isDefaultRevision as TRUE. If there were only hook_entity_presave() then a moderation module could make its implementation run first and all following hooks would get the right data. Has the question of hooks like node_presave come up in other issues?

Another option might be to have a super light controller class in core for "workflow" so that the the save method asks the workflow class if 'isDefaultRevision' should be TRUE or FALSE. The core implementation might always return TRUE or if "Save as Draft" ends up in core then the workflow controller could check that value. The big idea behind using a controller class is that a contrib module could swap out the controller class for more complex workflows. I'd need fago, Crell or someone else with a better understanding of the Entity API and class structure to sanity check that idea.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-49.patch, failed testing.

stevector’s picture

Status: Needs work » Needs review
FileSize
16.74 KB
PASSED: [[SimpleTest]]: [MySQL] 50,579 pass(es). View

Those fails are from a reference to devel module. Taking that out for testbot.

jstoller’s picture

I just started looking over this code, but it seems to me that tracking the latest/active/editing revision id in the entity's base table would be very beneficial to what we're doing here. @stevector, does that seem like a reasonable assessment?

jstoller’s picture

FileSize
30.56 KB
FAILED: [[SimpleTest]]: [MySQL] 49,389 pass(es), 684 fail(s), and 291 exception(s). View

I thought I'd just jump in and see what I could do. Hopefully this is useful.

I added an isEditRevision property and method on entities, and implemented them in the node module. I also added a {node}.edit_vid column. Generally speaking, this should track the latest vid of any node. When a node is loaded, if vid=edit_vid then isEditRevision is set to TRUE. After a node is saved, if isEditRevision() is TRUE, then {node}.edit_vid is updated. All of this (and a little more) let me start playing with the tabs on the node pages, moving toward the UI proposed above.

I left the module from #53 in there as is, for reference.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-55.patch, failed testing.

jstoller’s picture

Status: Needs work » Needs review
FileSize
33.8 KB
FAILED: [[SimpleTest]]: [MySQL] 46,606 pass(es), 3,319 fail(s), and 2,202 exception(s). View

Hopefully this is somewhat less broken. :-)

For now I replaced the published checkbox with a select list, controlling both the node's status and whether it isDefaultRevision. This would ultimately need to be integrated with the dropbutton from #1751606: Move published status checkbox next to "Save".

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-57.patch, failed testing.

eigentor’s picture

Are there any screenshots of the current UI as implemented in the latest patch? As the title of the issue implements there is visual stuff involved...

jstoller’s picture

I can certainly pull some screenshots together later, but honestly there isn't too much to look at right now. Here's what the patch does:

If you are viewing an unpublished node, the view tab reads "View draft." If you are viewing a published node, it reads "View published." If the published node has a forward revision, then a second tab appears that reads "View draft." Also, the edit tab will change to "Edit draft" and, if you click it, the edit revision of the node will be loaded into the form, no matter which view tab you were on.

I haven't yet implemented any of the sub-tabs shown in #22, but I think we should.

On the node edit form itself, I've replaced the "publish" checkbox with a select list, titled "Moderation state," with three options: draft, published and archived. Again, this is just a temporary solution until the dropbutton from #1751606: Move published status checkbox next to "Save" is implemented, but that widget will need to offer equivalents of these three options.

"Draft" will save an unpublished revision. If the node is currently published, then it will save this as a forward revision. And if you save a forward revision, then you will be redirected to the "View draft" tab, which is one of the original motivations for this issue. [status=0; isDefaultRevision(FALSE) if the node is published, else TRUE; isEditRevision(TRUE)]

"Published" will save a published revision and set it as the default. [status=1; isDefaultRevision(TRUE); isEditRevision(TRUE)]

"Archived" only displays if the node is published. It unpublishes the node and saves a draft revision. [status=0; isDefaultRevision(TRUE); isEditRevision(TRUE)]

All of this assumes that we are saving new revisions. In the event that a content type does not support revisions, these options would need to be changed to mimic simple publish/unpublish behavior. I also strongly recommend we get rid of the "New revision" checkbox. Its coordination with the moderation state makes the UX very confusing.

jstoller’s picture

Also, any node you load now has the following properties:

$node->default_status = {node}.status (no matter what revision you're looking at)
$node->default_revision = {node}.vid (no matter what revision you're looking at)
$node->edit_vid = {node}.edit_vid

jstoller’s picture

Status: Needs work » Needs review
FileSize
43.69 KB
FAILED: [[SimpleTest]]: [MySQL] 49,994 pass(es), 578 fail(s), and 282 exception(s). View
34.3 KB
30.62 KB
30.5 KB

I managed to do a little more work on this issue and thought I'd upload what I have. It mostly works, but I'm sure the code isn't very pretty, so I'd love to get some help on this one. If anyone's interested in jumping in, please do so.

Here's a bit of the UI, as it stands. When you first create a node, this is what you get:
Screenshot_1_22_13_9_09_PM.png

If you publish that node, then it turns to this:
Screenshot_1_22_13_9_09_PM.png

If you then create a forward draft revision, you'll see this:
Screenshot_1_22_13_9_10_PM.png

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-62.patch, failed testing.

jstoller’s picture

I thought I'd offer a little more explanation for the local actions shown in #62. The first one is fairly self explanatory. The "Publish this" action shows when the default revision is also the edit revision and is unpublished. Pressing it will publish the node. 

When the default revision is published, the "Unpublish this" action is displayed. This shows even if there is a forward revision. If the default revision is also the edit revision then pressing this button will just unpublish the node normally. If there is a forward revision then "Unpublish this" will also set the edit revision to be the default revision, making things look like the first screenshot again. 

When viewing a forward edit revision you are given the option to publish or delete that revision. Pressing "Publish this version" will make the edit revision the default revision and publish it. Pressing "Delete this version" will delete the edit revision. The latest remaining revision will then be set as the Edit revision. 

Note that all of these actions will trigger a confirmation form and none of them create new revisions. 

John Pitcairn’s picture

Looking good!

I think the default revision should not change if it is unpublished. If somebody unpublishes using this button, then it should be possible to republish (and not necessarily from the UI), keeping the same previously-published revision. If the default revision is changed, then republishing will result in a different revision being published, eek. The default revision should only be changed if a foward revision is explicitly published.

jstoller’s picture

@John Pitcairn: I'm inclined to disagree. I think it would confuse the UX.

First, let me back up and say that it will always be possible to publish an older revision. That could be enabled through the UI, but it is also certainly possible programmaticaly through the API. And {nod_revision}.status will tell you which revisions have been published previously.

Now lets say I have a published default revision and a forward draft revision. If I unpublish the default revision, but allow it to retain its default status, then I would be left with two "View draft" tabs. Also, even if we renamed one of the tabs, it means that when users go to view that node, they will be shown the older version by default. All of this sounds very confusing for most use cases.

I think the sane default is to assume that content always moves forward. If I've already started editing published content, then I unpublish it, the next time I go back to publish it I'm likely to want my edits.

John Pitcairn’s picture

I'm thinking of how Workbench Moderation works - unpublishing does not change the "current" revision - I think there will be a large number of users expecting the same behaviour with revision-management in core, and it might be a fairly difficult re-education task by the time Drupal 8 is ready for real-world sites. Drafts will get published accidentally instead of re-publishing the unpublished default.

I think the extra "draft" tab is just a terminology problem. An unpublished default revision with forward drafts is not "the" draft in this context. So for an unpublished default revision, currently being viewed, with forward revisions, the tabs/tasks might look like:

View unpublished | View draft | Edit draft | Revisions

[Re-publish this]

John Pitcairn’s picture

Or maybe it's "View archived"...

jstoller’s picture

I think anyone using Workbench is going to continue using Workbench. We need to remember that this is not a full-featured workflow system we're building. We are just doing the best we can with the tiny bit of workflow support we have in core and trying to make it a little bit easier for contrib to implement bigger and better things. Realistically, most people don't use Workbench anyway, so this will be their first exposure to workflow in Drupal.

As I see it, most people will do one of two things. 1) They have a node which always remains published, but is edited periodically to update information; or 2) They have a node that is published for a time and then archived. That probably covers more than 95% of all content. A node that is unpublished in the middle of being edited and then republished is probably a tiny minority of content. And I presume that in most of those cases, if you were editing the content before, you're going to want to finish editing it before you publish it again.

It seems odd to me that my default view of a node would be an archived version, if I have a more recent draft. But perhaps I'm in the minority on this. I'd like to hear more opinions, if anyone has them.

John Pitcairn’s picture

Consider this scenario:

An organisation has a page advertising an upcoming contest, which is published.
There is also a draft version with more detail, which will be published in a few days time, and this is still being worked on.
The published page says something which may expose the organisation to an unforeseen liability, and this needs to be checked.
The lawyer is away until tomorrow.
The web admin unpublishes the node, from a VBO admin screen. Just publish/unpublish, nothing about revisions.
The lawyer returns, verifies that the liability is OK, and the page is republished, from the same admin screen.
Meanwhile, the draft version continues to be worked on by the marketing team who don't know about any of this.

This is not an imaginary situation ;-)

I think the danger here lies in "publish" silently becoming "publish latest draft". If you're going to have a "default" revision, then that is a separate thing from the "draft" revision, and should stay so until somebody explicitly wants to make the draft revision published.

peterx’s picture

Sometimes the current version is so damaged that people revert to an earlier version then move on from there, sort of like code version control. If the content is long, the changes extensive, and the exact changes forgotten, we might go back to a known version then apply changes in a more controlled manner.

If the UI is replaceable, add-on modules could offer alternatives. For some content, I like the system of revision display used for drupal.org documentation. This would give editors an easy way to find the last clean version, an approach often needed when there are multiple editors.

For legal documents, the list would be by date showing what is published when. Again it could be an add-on tab from an add-on module. We know people made some messy edits last Tuesday and we can revert content to the prior day.

The most complicated add-on would be a branch display straight out of version control systems. The examples I am thinking of here are product offers and T&C. You make a revision covering some special offer, say interest free credit, then at the end of the month you revert to the old terms. A few months later the special is offered again, content is altered in a new version then the change is reverted back at the end of the month. We want to unpublish the current revision then publish an older revision.

Gábor Hojtsy’s picture

Some random comments:

- I think it would be important for the functionality to be optional. For node types like forums where you most likely not going to do drafts and forward revisions, the extra UI elements like the "View published" tab and the buttons could become pretty confusing / distracting.
- You don't need to take this into account (just yet) but #1882482: [meta] Unify editing approaches in Drupal essentially proposes removing front end page level tabs in favour of contextual actions (so actions on blocks on the page for example would be on the same level in terms of exposed UI like actions on the main node). If this patch lands first, we'll need to consider how to expose this better there :)
- Your upgrade path code looked a bit puzzling. The latest vid value is already in the node table, so the upgrade path could be one query to take that over to the edit vid, no? Are you trying to take the contributed modules hacking in forward revisions into account here?!
- It feels like the moderation module included is just a test implementation and not actually proposed for core? Is that true? The separation of concerns between the node level implementation and the moderation module looks somewhat arbitrary.
- If the feature is targeted for all entity types, then the boilerplate to add to node module is a bit worrying; however there are no other entity types in core that support revisions yet AFAIK so it is not going to end up with code repetition in core

jstoller’s picture

Re #72:
The extra UI elements will only be visible if revisions are enabled on the content type, so in that sense, they are optional. If you're using revisions, then these are the tools to manage them.

I have some major concerns about the impact #1882482: [meta] Unify editing approaches in Drupal will have on workflow implementations in general. Right now I'm just crossing my fingers that those concerns are addressed before such sweeping UX changes are made. But that's for a different issue.

Like it or not, there are many sites that have been hacking in forward revisions and there's no reason we shouldn't take them into account if we can, so my upgrade path attempts to do so. I see no reason not to.

I'm keeping the moderation module, provided by stevector in the first patch above, in this patch for reference. There's some good code in there that I don't want to loose just yet. As this issue develops, that module will become unnecessary and eventually will be removed.

I am working on a new version of this patch that moves more of the functionality out of the node module and down to the entity level. I'm basically fumbling around and figuring this out as I go along, so it's taking me some time to get things in the right spot. Ultimately I think it should be easy for any entity to implement these behaviors.

jstoller’s picture

FileSize
54.24 KB
FAILED: [[SimpleTest]]: [MySQL] 48,820 pass(es), 458 fail(s), and 210 exception(s). View

This patch now uses the dropbutton introduced in #1751606: Move published status checkbox next to "Save" to handle state changes. I also moved most functionality to the entity, rather than it being node specific.

Entities can now specify $entity_keys['edit_revision'] if they support edit revisions.

The Entity interface now includes the following methods:

isRevisionable($edit_revision_check = FALSE)
getEditRevisionId()
isEditRevision($new_value = NULL)
setEditRevision($new_value = NULL)

And loaded entities have the following properties:

default_revision
isEditRevision

I've also defined the following functions in entity.inc:

entity_get_edit_revision_id($entity_type, $id)
entity_load_edit_revision($entity)
entity_load_edit_revision_by_id($entity_type, $id)

For now I am assuming that edit revision support is optional, even when implementing revision support in an entity.

jstoller’s picture

Status: Needs work » Needs review

I know this patch is going to fail, but let's see what happens.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-74.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

jstoller’s picture

Can someone with a deeper understanding of the API take a look at my code and see if this is making any sense? I'll keep fumbling around in Drupal's guts, but it would be nice to know I'm heading in the right direction.

jstoller’s picture

Status: Needs work » Needs review
FileSize
55.94 KB
FAILED: [[SimpleTest]]: [MySQL] 48,084 pass(es), 462 fail(s), and 211 exception(s). View

Updated patch to work with new commit from #1751606: Move published status checkbox next to "Save".

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-78.patch, failed testing.

tkoleary’s picture

@jstoller

I have some major concerns about the impact #1882482: [meta] Unify editing approaches in Drupal will have on workflow implementations in general. Right now I'm just crossing my fingers that those concerns are addressed before such sweeping UX changes are made. But that's for a different issue.

So in this issue http://drupal.org/node/1898946 I try to bring together a bunch of these related problems with some pretty in depth task flows. It touches on workflow at the end.

jstoller’s picture

Status: Needs work » Needs review

Changing the status to get some eyes on the work to date.

peterx’s picture

I am thinking about the user interface after we select something in the revision list. Everything up to #14 is good. #36 is a bit confusing. #62 looks nice and has just one problem I can see. When you press "delete this version", the only positive indicator of other versions is the published tab. I can see the need for a positive indication when you are deleting the only version of the content. Something like "There are no other versions of this content!".

jstoller’s picture

@peterx, re #62: The "Delete this version" action only displays when viewing a forward revision, so there is no way to delete the last remaining version of a node. At best you could delete all draft revisions saved since the last published version.

jstoller’s picture

FileSize
67.36 KB
FAILED: [[SimpleTest]]: [MySQL] 48,475 pass(es), 472 fail(s), and 211 exception(s). View
57.72 KB
95.3 KB

I started making some changes to the revision history page. It now lists the vid, along with the revision timestamp, for easy reference, and I added a "State" column.
revision page screenshot

Clicking to view any revision will take you to a new version tab. This image shows the tab for vid 21, at the path /node/1/version/21. If you try to go to the version tab for the default or edit revision, you'll be redirected to the "View draft" or "View published" tab, as appropriate.
version tab screenshot

If you revert to an older revision from either the revision history page, or using the action button on a version page, a new forward draft revision will be created with the same content as the reverted revision and you'll be redirected to the "View draft" page.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-84.patch, failed testing.

jstoller’s picture

Status: Needs work » Needs review
FileSize
73.81 KB
FAILED: [[SimpleTest]]: [MySQL] 48,445 pass(es), 472 fail(s), and 211 exception(s). View
55.45 KB

I got the "Previous revision" and "Next revision" actions working, though I suspect there's a better way to do it.

Screenshot_2_3_13_10_51_PM.png

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-86.patch, failed testing.

jstoller’s picture

Status: Needs work » Needs review

Still hoping for a review of the code changes to date.

eigentor’s picture

It may be that it just cannot be done simpler. But the screenshot from #86 looks really frightening.
If this does not have to land before feature freeze a healthy dose of prototyping and rethinking might be necessary. This is admittedly non-trivial.

Wim Leers’s picture

It may be that it just cannot be done simpler. But the screenshot from #86 looks really frightening.

Exactly my thoughts.

jstoller’s picture

Can you be more specific? What makes it frightening? Is it a question of better styling the elements, or is it something deeper than that.

Given the API changes required to get this working, I'm guessing we either get this in before code freeze, or we'll be waiting until D9. Along those lines, any help would be appreciated.

eigentor’s picture

The problem is that there are no less than for (even five, if you see the Revision history as something of its own) things, that are shown here and sound and look pretty much the same:
The current published version, Revisions, a forward Revision (the Draft) and in the lower button row revisions even get named versions so this is not consistent.

Current state

The problem itself is not trivial, as this is kind of like a git Repository with multiple branches. So we need to visualize something that is not easy to grok.
Basically it is only two things: Revisions (of which the published version is also one, the newest) and Drafts / forward revisions.
As the concept is not completely easy I see no way to do this with forward and back buttons and multiple tabs.

What can keep things together is a timeline. The revisions are chronological. Only the forward draft is somehow in its own timeline, as it can be published or not.
So what I would propose is keep all the revision handling on one page, in the revision history and name it "Revisions". If it shows people do not find it we might want to add more hints to it. But given they click that tab, a tree-like structure might help, as follows:

tree structure

The buttons are buttons and move to the revision that you clicked. I guess there is a confirmation form so people do not revert their content by chance and go all panic. The small "details" link opens a preview - I guess this needs to be a modal - and shows info in the top about when and by whom it was created.
Numbering the revisions and only showing date and author might be debatable, but if people find this info in the preview, is hopefully more grokable than the current state in drupal, where you see all those date info and it does not really help you.

The Drafts have their own column and a different button color, so drafts and revisions are clearly seperated.
Also seperated is previewing of a revision or draft or actually applying it. I would hope "Apply Revision 7" or "Publish Revision 7" or whatever it is named should make clear this actually is an action, while the small link rather implies to more info and no action.

tkoleary’s picture

@eigentor

if this does not have to land before feature freeze a healthy dose of prototyping and rethinking might be necessary. This is admittedly non-trivial.

I would be happy to revise and prototype this in invision for dharmesh to test along with the one below that I have put together for in place editing (since they are related):

http://invis.io/EXBVWNQS

jstoller’s picture

Re #92: I suspect you may be misunderstanding what's happening here.

To start with, there is no branching going on here at all. This is a completely linear workflow. Along these lines, it is very important to understand that the published version is not necessarily the latest revision. What we've been calling a "forward revision" is an unpublished draft revision which is newer than the published default revision, but it exists in the same branch.

Let me walk through the life of a node again, to explain what shows when.

First, I create a new article and save it as a draft.

Screenshot_2_5_13_9_57_PM.png

Now I edit that article and save it as a draft again, creating a new revision. This causes the "Previous revision" action and the "Revision history" tab to appear.

Screenshot_2_5_13_10_02_PM.png

Now I click the "Publish this" button and publish the node. This changes the tab from "View draft" to "View published" and exposes the "Unpublish this" action, replacing "Publish this." At this point vid #2 is both the default revision and the edit revision.

Screenshot_2_5_13_10_03_PM.png

Now I edit the node again, saving a new draft revision. This creates a forward revision. As such, both the "View published" and "View draft" tabs are visible. At this point vid #2 is still the default revision, but vid #3 is the edit revision, so the "Edit" tab changes to "Edit draft," making it clear that the draft version is what will be edited if you go there.

Screenshot_2_5_13_10_10_PM.png

When I first save a draft, I am taken to the "View draft" tab (i.e. /node/1/draft), but I can easily click back to the "View published" tab. When I do, the "View draft" tab is still visible, letting me know that there is a newer draft version of this node. Also note that the edit tab still reads "Edit draft." It doesn't matter which tab you're on, clicking edit will always load the draft revision.

Screenshot_2_5_13_10_15_PM.png

Lets say I save a couple more drafts and then look at the revision history. I would see something like the table below, showing the linear progression of this node. This clearly shows that version #2 is still published. It also notes that my most recent draft, version #5, is the edit revision.

Screenshot_2_5_13_10_28_PM.png

At this point, if I click on version #3 to see what it looked like, I'll get something like this:

Screenshot_2_5_13_10_33_PM.png

This is the most complex state of this page there is. It clearly tells me that I am now viewing version #3. I can also clearly see that there is a newer draft revision and an older published version, either of which I can easily click over to view. It may seem like a lot of tabs, but I'd argue that all of them are necessary. I think it's important to show a revision in the context like this, rather than rendering older revisions without the local tasks, as we do now. The tabs are also all conditional, so they only appear when needed.

If I click "Revert to this version," version 3 will be copied to a new draft revision and I'll be taken to the "View draft" tab. Note that the "Version" tab doesn't appear any more, since we are now viewing the edit revision.

Screenshot_2_5_13_10_49_PM.png

If I publish this version the "View draft" tab will also go away, since there is no longer a forward revision, leaving just the "View published" tab.

Screenshot_2_5_13_11_02_PM.png

Clicking "Previous revision" will take me backwards one step, to my previous draft, displayed in a "Version #5" tab.

Screenshot_2_5_13_11_07_PM.png

Looking at the revision history again, I'll see the table below. You can see that version #6 is published and is the edit revision. All the other revisions are now archived, but version #2 was once published.

Screenshot_2_5_13_11_09_PM.png

Hopefully this little tour helps. One more thing I'll note is that the local actions don't really work well as big buttons in this context. Especially with that "+" symbol that gets added on. However, this is largely a styling issue. They'd work better as a sub-tab bar, like shown in #22, or something closer to that direction.

eigentor’s picture

FileSize
40.07 KB

Thanks for your effort to give a process of the workflow.
I know that there is no actual branching and one could see the entire revisions.
What I am after is trying to visualize things in a way the editor can find the correct draft or revision and publish it. The way the actual patch does it is use the "the revision before this one" the "revision after this one" Scheme to find where I am.

The problem is: if there are more than say two, and even more so if the person wanting to publish things is another person than the one that created all these drafts, things get messy. How is he to know, what the "previous revision" is?
Having a delete button inside these actions is adding even one more thing in there. Deleting should be done in a different place (kept at the place normal node save and other actions are, bottom of form or wherever they are).

Let's not use multiple tabs and local action buttons, as I do not believe they help to navigate and understand what is going on.
The multiple-column tree that I proposed may actually be more confusing than it helps.
Keeping it all in a chronological order by using the revision table you show might be good.

So how about keeping it all in the revision table and working on it to make it more clear and usable?
One thing that would be first is to highlight more which version is the actual published one.

revision table

There could be arguments if the latest published version should always be moved to the top. Not doing it (as displayed now) has the advantage people know that the drafts they have created are newer than the published version, so this helps. One could also argue that the current version row should be rather blue instead of green (or any other color), but that's details.

If you sympathize with this I could apply the current path locally and work on it it more detail. E.g. the font sizes could also be used a bit more to highlight more importand information.

tkoleary’s picture

@eigentor, @jstoller

Per my comment above I am still planning to add this to the invision prototype* so that we can get more of a real feel of it in practice. However, as I have suggested before in http://drupal.org/node/1898946, I strongly believe that if we allow more than one draft we invite content forking and the need for users to manually merge content (without providing a programmatic way to do that). If the act of saving a draft automatically pushes the old draft into revisions then you nudge users towards a more scalable content workflow. There is essentially no downside to this since if there is a disagreement between two users the revision can always be surfaced and merged if needed.

If we feel that there is a need to distinguish between revisions that have been published and old drafts that have never been published we can simply Label those revisions "old draft".

*Try out the prototype here: http://invis.io/EXBVWNQS

Bojhan’s picture

The prototype requires contant info, can you please remove that? I do not want to be contacted or tracked, when looking at an prototype for an opensource project.

tkoleary’s picture

Hi Bojhan,

Because I am a project owner I did not realize that invision was requesting contact info to see projects. Unfortunately I'm not able to turn that off. I have a request in to Clark Valberg (invision CEO) to offer that as an option. They are a small startup—5 or 6 people—and they are eager to please so I'm hoping we will be able to get that done.

BTW, just realized that it started on the wrong page so use this one: http://invis.io/FVB89STC

Bojhan’s picture

@tkoleary Yhea, strange - I did not see it requesting before. Its asking "Jeff Noyes" wants to know who....

Gábor Hojtsy’s picture

@Bojhan: I think they recently updated their software. There are other visual changes. Since we cannot really help it, the workaround for now is you provide a fake name, like Will Smith and will@example.com. That should work :)

hass’s picture

Can we use the green color we are using in Workbench, please? I opened a case at #1893766: Highlight published revision in diff compare table with this color to use in diff.

table.diff-revisions tr.revision-published td {
  background-color: #aaffaa;
}
sun’s picture

@jstoller:

  1. Revision history: Can we split out the "Revision type" and "Publishing status" into two separate table columns? I think that would make the revisions table a lot more clear.
  2. Version view page: I guess that most people are getting confused by the revision navigation links that are styled as action links/buttons. Was there any particular reason for using that styling? I'd imagine a "pager" navigation would work best for the navigational previous/next links? Perhaps even a real/true pager? :)
  3. It would probably also help to rename the active tab into "View version #x" (i.e., consistency in "view" label prefix).
  4. Can we turn the "Revert" and "Delete" actions into second-level local tasks? (they are direct children of the active "View version #x" tab)
  5. The "Edit draft" tab does not appear to fit into the UI at all. I think it would be OK to just drop it and require users to go to "View draft" first? On the "View draft" page, you should probably see a second-level set of local tasks for "view", "edit", "delete", which all relate to the draft version you're looking at.
jstoller’s picture

@hass: Yes. :-)

@Sun:

  1. I do think this deserves some attention, but I'm not sold on the idea of separate columns. Let me think on that.
  2. I implemented the "next" and "previous" buttons due to popular demand. However, now that I see them in action, I'm not sure they really should be there. Right now I'm inclined to remove them all together and have users visit the revision history page if they wish to navigate to another revision.
  3. I'm not opposed to this. It was mostly a question of brevity, but if you think the "View" prefix helps, I'll add it.
  4. I think I originally tried to implement them as second level local tasks, but couldn't get it to work. I can try again and see how it looks. Are there any examples of second level local tasks in D8? I don't believe I've seen any.
  5. I think you should be able to get to the edit tab from any other tab. Forcing users to go to the draft tab first feels excessive. You could put an edit link as a sub-task on all view pages, but it would always need to load the draft revision, which could be confusing. I'll play with it, but right now I think a top-level tab seems the most straightforward.
klonos’s picture

...just a couple of thoughts:

Regarding the previous/next revision navigation (if we decide to keep this feature after all), another option would be to display these as links. Perhaps replace their static labels with the actual name/number of the revision too?

Also, the tabs with verbs in their labels strike a bit odd when they are the current tab. What I mean is that for example the "View published" tab should become "Viewing published" (or plain "Published") once on it.

jstoller’s picture

FileSize
73.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch save-as-draft-1776796-105.patch. Unable to apply patch. See the log in the details link for more information. View
41.06 KB
65.2 KB
16.14 KB
28.35 KB
31.67 KB
31.28 KB
13.15 KB
28.06 KB

I've gotten rid of the "next" and "previous" revision buttons all together. I think ultimately they were more confusing than useful. All of the other local action buttons have now been converted to second-tier local tasks. Per @Sun's suggestion, I tried moving the Edit task to the second level as well...sort of. Here's an updated tour:

I create a new article and save it as a draft. There's only one valid top level tab at this point, so none of them display.

Screenshot_2_7_13_12_07_AM.png

Note that "Edit" has moved to a second level local action. However, clicking edit still loads the form in a top level tab. This allows the edit form to open in the overlay and prevents any of the other second level links from showing, but the tab only displays when you are actually on that page.

Screenshot_2_8_13_5_08_PM.png

When I save another draft, the "Revision history" tab appears. Now I can see that I am on the "Draft" tab.

Screenshot_2_8_13_5_10_PM.png

If I publish the node, the tab changes to "Published".

Screenshot_2_8_13_5_11_PM.png

Saving a forward revision takes me to the "Draft" tab, but "Published" is still visible.

Screenshot_2_8_13_5_13_PM.png

If I view the published revision, I see that the edit link now reads "Edit draft," since we always edit the latest revision.

Only local images are allowed.

The edit tab label also now reads "Edit draft" to make this clear.

Screenshot_2_8_13_5_14_PM.png

I've made a few changes to the revision history tab. Notably, the published revision is now highlighted in green.

Screenshot_2_8_13_5_17_PM.png

Viewing an older revision opens in a new tab, as before.

Screenshot_2_8_13_5_19_PM.png

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-105.patch, failed testing.

jstoller’s picture

Status: Needs work » Needs review

Still needs review. We should of course keep cranking on the UI, but the back-end API stuff could really use a trained eye.

sun’s picture

#105 looks like a big step into the right direction to me.

However:

1) The local tasks "Publish this" and "Unpublish this" won't fly at all :) They directly conflict with #1751606: Move published status checkbox next to "Save" as well as the general architectural/UI design pattern of local tasks. "Publish" has to be a form button/action.

2) In the grand scheme of things, the second-level local tasks make sense to me - but they are pretty confusing when looking at this at first sight. I don't know how to improve this. Food for thought.

3) The local task "Edit draft" is still completely out of line with all of the others, since all of them refer to individual versions, which on their own, can be viewed, edited, deleted, and whatnot. I do not think it should exist.

4) The Revision history table looks a bit better now, but I still think that we should separate the publishing status from the revision type into two separate columns.

5) Speaking of 4), it looks exceptionally weird that you managed to have a Draft version that is archived there. If that is technically possible, then I think that's a major bug and we're allowing for too much, as I'm not able to make sense of that constellation in any way.

John Pitcairn’s picture

"Publish" has to be a form button/action.

So you have to edit a draft if you just want to publish it? That's not at all intuitive IMO.

From a UI perspective, I rather like what is going on in #105.

Alan D.’s picture

"Publish" has to be a form button/action.

This is a security related issue. A user knowing the link can embed that link in a hidden field and when the admin visits that page, the link will be called and that item published.

jstoller’s picture

Re #110:
All of the publish/unpublish/delete/revert actions shown in #105 lead to confirmation forms, which should negate any security concerns.

Re #108:
1) I disagree. #1751606: Move published status checkbox next to "Save" dealt with how to handle the submit button on a node edit form, which is very important, but I have long found it absurd that one must edit a node in order to publish/unpublish that node. In most cases those activities are better done when viewing the node directly. The edit page should be saved for when one wishes to make changes to a node's content. And because we are not viewing the node in a form, different UI patterns can apply. Besides, as noted above, these local tasks don't actually enact the action itself. They load a confirmation page, which has an actual button on it. For instance, here's what happens when you go to the "Publish this" page:

Screenshot_2_10_13_9_22_AM.png

2) I think much of this is a styling problem. I haven't started diving into the CSS at all yet. I'm just using the local tasks as is right now and they leave much to be desired.

3) Well, we need some way to get to the edit page when viewing a node. And the edit page always needs to load the edit revision, no matter what version the user is viewing when they go to it. I was happy just having a top-level Edit/Edit draft tab, but you didn't seem to like that either. I'm open to suggestions.

4) I still don't like the idea of two separate columns. A whole column is a lot of real estate when there are only two revisions that need to be called out—the default revision and the edit revision—and sometimes they're the same revision. I see no reason why all of this couldn't be accomplished in a single state column.

5) Until we add support for workflow states in core, none of these states actually exist. They are just pseudo-states, based on other attributes, that we use to label revisions. Right now we are calling any revision older than the default revision "Archived." We are also noting what state that revision was in prior to being archived. However, this is somewhat arbitrary and can be changed.

What if we changed our state naming convention to this. If the default revision has status=1, we call it "Published." If the edit revision is unpublished, we call it "Draft." If a revision is older then the default revision and was once published, then we call it "Archived." All other revisions we leave stateless.

John Pitcairn’s picture

I agree this is a styling issue, and that a form button need not look like a form button - my clients do not make any conceptual or functional distinction between a link and a button if there are no obvious form fields visible, and I frequently re-style any "toolbar" buttons as links for aesthetic or space reasons. I have encountered no usability issues doing so in testing.

The use of an admin theme blurs the distinction anyway since buttons may look very different from one theme to the next. The published/save/draft combi-button is very much an admin theme element, but the revision-management toolbar in this case is displayed in the site theme. I would only expect a button here if the same theme is used for both, and possibly not even then because the user is engaged in a different task (publish/unpublish vs edit/save).

What if we changed our state naming convention to this. If the default revision has status=1, we call it "Published." If the edit revision is unpublished, we call it "Draft." If a revision is older then the default revision and was once published, then we call it "Archived." All other revisions we leave stateless.

In the event that they wish to revert from a draft that was accidentally published, my clients would find it tremendously useful to quickly and unambiguously differentiate previously published revisions from revisions that were never published. My preference would be to retain the two archived states and indicate the transition that occurred, as something like:

Archived from published
Archived from draft

David_Rothstein’s picture

This is a security related issue. A user knowing the link can embed that link in a hidden field and when the admin visits that page, the link will be called and that item published.

...

Re #110:
All of the publish/unpublish/delete/revert actions shown in #105 lead to confirmation forms, which should negate any security concerns.

Just to clarify (since this is a common misconception), you can easily add a token to the URL to provide security if you don't want to have a form; Drupal already does this in many places (e.g. publishing comments). Confirmation forms don't provide any security on their own; the fact that the form API adds a token to the form behind the scenes is where the security comes from. But that is a process you can repeat in the URL if you're not using the form API.

It doesn't look like it will matter much here since both sides of the discussion seem to want a form somewhere, but I thought it was worth mentioning anyway...

tkoleary’s picture

@ John Pitcairn

In the event that they wish to revert from a draft that was accidentally published, my clients would find it tremendously useful to quickly and unambiguously differentiate previously published revisions from revisions that were never published.

Absolutely. This IMHO is the critical thing we want to illustrate to the user. Dcmistry, Prestonso and I worked out a way this can be done which I am prototyping now. Should be reviewable some time in the AM tomorrow (EST).

jstoller’s picture

Issue tags: +workflow
FileSize
24.93 KB
75.97 KB
78.63 KB
FAILED: [[SimpleTest]]: [MySQL] 49,113 pass(es), 746 fail(s), and 420 exception(s). View

I've changed the revision state labels. If the default revision has status=1, I call it "Published." If the edit revision is unpublished, I call it "Draft." If a revision is neither the default revision, or the edit revision, and was once published, then I call it "Archived from published." All other revisions are stateless. The revision history table now looks something like this:

Screenshot_2_11_13_9_21_PM.png

I also removed the "Create a new revision" checkbox. Now, if a content type is not set to save revisions, then revision information never shows when editing nodes of that type. If a content type is set to save revisions and the user has permission to administer nodes, then the old checkbox is replaced with a new "Overwrite current version" checkbox. I feel this approach works better. I want to make it clear that checking the box will bypass the default behavior and destroy data.

Screenshot_2_11_13_9_22_PM.png

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-115.patch, failed testing.

John Pitcairn’s picture

I'm fairly ambivalent about the stateless drafts but if others are OK with that then so be it.

About that "Publish the revision from ..." automatic message:
Why is "publish" italicized?
Would "Published on [date/time]" be simpler and clearer?
Why is revision #2 not also "Archived from published", if it has that publishing message?

Presumably revision #7 was reverted, then published. I am confused:
Where is the revision it was copied from? The only revision with that timestamp appears to be a later revision.
A reverted revision should presumably always appear somewhere above the revision it was copied from?
Could it say "reverted from revision #X" instead of just supplying the reverted-revision date?
If reverting does not automatically publish, should it also have a "published on" message?

Finally, should the "overwrite" checkbox label be "Overwrite current revision", for consistency?

jstoller’s picture

Re #117: I think most of the inconsistencies you're seeing are because of experimentation on my end during development, including some direct manipulation of the database, so I'd take that image with a grain of salt. I mostly just wanted to show what the state designation labels would look like in practice. I'll try to take a look at the log messages next time I get in there and see if I can clean them up a bit.

I could change all occurrences of "version" to "revision," but personally I've found the word "revision" to be a UX problem. I've just encountered too many users who don't understand what the word means. "It means different versions," I say. "Then why don't you just call them versions," they reply. So my general rule of thumb is to use "revision" when talking about it in the context of multiple revisions, but to use "version" when referencing one specific revision. I think using "version" and "revision" somewhat interchangeably like this actually increases comprehension of the latter. Wikipedia does something along these lines. So I say "Overwrite current version," because it is a specific version, but I follow that up with "Leave unchecked to create a new revision," because it implies we're saving multiple revisions. You'll also note I say things like "Publish this version" and "Version #3" on one side, but "Revision history" on the other.

I'm doing it this way, for now, because I think it is MUCH better for non-technical audiences and I hope others will agree when they see it in context. However, I absolutely do not want this to devolve into a lengthy debate over revision vs. version. That can happen elsewhere. So if there's big push-back, I'll relent (for now). It just means I'll need to form alter my own sites to fix it. Which is sad, but getting the rest of this patch in is more important than a terminology bike shed.

The bigger issue at this point, which needs some attention, is how I flipped the meaning of that checkbox from positive to negative. As I said, I think this is absolutely the way to go, but I know this too is a potential bike shed. For history, this was previously discussed in #563550: Change "create new revision" check box.

jstoller’s picture

Just FYI... the patch committed in #1874664: Introduce toolbar level "Edit" mode that shows all contextual links has done some serious damage to what we've been working on here. There's been significant backlash on that issue, which I think is good, but in the mean time I'm not yet sure what to do about it.

Wim Leers’s picture

There is indeed backlash on #1874664: Introduce toolbar level "Edit" mode that shows all contextual links, but there is also strong concensus about this being a step in the right direction.

In short:

  • #1874664: Introduce toolbar level "Edit" mode that shows all contextual links gets rid of local tasks tabs on full node views, but moves them into contextual links. Core rationale: edit/revisions/… are administrative tasks *and* they make it so that you always have the same way to start editing a node, whether it's rendered as a teaser or in full: always use contextual links. This means we're trading one set of assumptions (local task tabs are essential) for another (contextual links are essential). The difference being: worse visual discoverability, but more consistency+not visually invasive.
  • The backlash is about the fact that there are very valid use cases for using local task tabs, particularly on community-style sites where many users can edit a node, and thus it's vital to explicitly see the way you can edit a node.

The UI aspect of this issue is unfortunately moving more administrative tasks into the front-end. (As per screenshots in #105 & #111.) I think that's fundamentally wrong. It is in line with what we've been doing for so long though, so it totally makes sense :)


I'm very sorry that we've complicated progress on this patch. This clearly is a very important patch. But as I said before, putting all of this administrative stuff amongst your actual content doesn't seem a good idea to me. That being said, it's also not immediately obvious to me how you would do it otherwise.

I want to help this patch move forward.

So I think the key question is: If you didn't have local task tabs to work with at all, how would you make this UI?

Frando’s picture

So I think the key question is: If you didn't have local task tabs to work with at all, how would you make this UI?

I disagree. Was it decided somewhere that Local Tasks are a bad UI pattern per se? It should certainly be possible within Drupal core to have the revisions tab be a local task tab and not a contextual link. Think Wikipedia or other websites where browsing the existing revisions of a piece of content is not necesessarily an administrative task, but a part of the regular front-end content consumption.

Gábor Hojtsy’s picture

We are trying our best to accommodate actions on things regardless of what they are. If you want to edit a view, a panel, a block, a menu, a node, etc. you don't need to know what is what type and how that specific things is edited somewhere (does it have its own front end page, is it under a block library or in the views list?), you have contextual operations on them accessible via hover or the edit toggle. I think that concept is pretty powerful. Clearly there are things to figure out how to do best.

I don't think we removed the possibility of tabs at all, and we could very well end up figuring out that nodes need tabs because although almost nothing else on your site have them, nodes are really special and should not be dealt with in the same way. Even in that case, the question of what people will be able to do to the node when its displayed in a View, Panel, Block, etc. when the tabs are obviously not there stands I think. So sounds like your answer is no support for any of the editorial workflow features in that case?

John Pitcairn’s picture

My editors are typically small company PAs and admin staff, often quite transient and untrained. They are never admin-level users. They never get to edit blocks, views, menus, etc.

I tried D7 contextual links on them as a way of editing nodes. Most of them hated it (too hard to find/remember), and I had to turn the local task tabs back on. That's not to say I won't try again if the approach is consistent across everything, but I'm sure I will meet with considerable resistance.

My vote comes down on the side of discoverability for non-admins, and I agree wholeheartedly with David Rothstein's comment #110 in #1874664: Introduce toolbar level "Edit" mode that shows all contextual links.

peterx’s picture

+1 for #123

Wim Leers’s picture

#123: so your editors like that — when they are browsing the site, e.g. a View with teasers and spot a problem they need to fix — they always have to navigate to the full node page to check which revision is published because it doesn't look like the one they thought was there, or simply because they want to start editing it?

I.e. instead of clicking the contextual links gear/pencil + clicking the "Edit" link and having the node edit form show up in an overlay (i.e. in context), they have to do at least one full page load (much slower, and now out of context) and click the "Edit" tab (another page load, and even further out of context).

(Analogous example for revisions.)


In #120, I said:

If you didn't have local task tabs to work with at all, how would you make this UI?

#121 found this an almost offensive proposition. But what I was targeting here is: how would you make make all this work for e.g. teasers, where it is impossible to have local task tabs?

jstoller’s picture

@Wim Leers, re #125: You are making the assumption that we must choose between contextual links and tabs, but this is a false choice. There is no reason why we couldn't have both. Contextual links are great for following content around in different contexts, like the Views example you give, but they suck at identifying location. The tabs may be useless when dealing with a teaser list in a View, but they are invaluable as a wayfinding tool when I am on a node page. So what's wrong with implementing both?

I've already mentioned this in #1874664: Introduce toolbar level "Edit" mode that shows all contextual links, but I suppose it bears repeating here. There seems to be some irrational drive to remove all redundancy in Drupal's UX, but redundancy isn't always bad. Some times its appropriate to provide multiple paths for accomplishing the same thing. If I want to open a file on my computer, I can select the file, go to the "File" menu and select "Open." Or I can right-click on the file and select "Open." Or I can select the file and type command-O. Or I can double-click on the file. Or I can open it through a terminal application. Yet somehow, I have never been confused about how to open a file. Some people choose one method and some people chose another, but ultimately they end up in the same place. Having an edit tab on nodes that users can go to, while simultaneously having an edit link in a contextual menu, is not necessarily a bad thing.

I know Drupal calls them "local tasks," but to me those tabs have always been more location than task. "View," "Edit," and "Revisions" are places first and actions second. The edit tab is where I go when I want to edit a node. The revisions tab is where I go when I want to see my revision history. Removing those tabs removes any visual indication of where I am and what other places I could go to, which is a huge blow to UX.

In some contexts contextual links are very appropriate, but they should be seen as enhancing, rather than replacing, the local tabs. I have no problem with there being an edit link in a contextual menu, but when I click on it and it takes me to an edit page, I need a visual indication that I am on the edit tab of this content. I also need to know how to click back to the view tab. In the case of this issue, I need to know at a glance that there is a published revision and a draft revision. I need to know which one I am currently on and how to click back and forth between the two. Pulling "Draft" into a contextual menu is ridiculous in this context.

I'm interested in the idea of separating local tasks (displayed as contextual links) and local navigation (displayed as tabs). In the end, I think we need to have some things that are just tabs, some things that are just contextual links and some things that are both tabs and contextual links.

I know I'm not setting a good example here, but perhaps we should move this conversation over to #1874664: Introduce toolbar level "Edit" mode that shows all contextual links, so it isn't happening in two places.

John Pitcairn’s picture

Having an edit tab on nodes that users can go to, while simultaneously having an edit link in a contextual menu, is not necessarily a bad thing.

Agreed. There is no One True Way, and we do not need one forced upon us as a new paradigm.

so your editors like that — when they are browsing the site, e.g. a View with teasers and spot a problem they need to fix — they always have to navigate to the full node page to check which revision is published because it doesn't look like the one they thought was there, or simply because they want to start editing it?

Some of them, yes. They go to the admin menu content view, filter/find the node they are looking for, and edit from there. Or they find the full node ("page" to them) in the navigation and edit from there. Contextual edit links on teasers or other truncated views of content seem to confuse them, because they are confronted with far more than they were expecting on the edit screen. Assuming they even notice the contextual links.

If the contextual links just offered a "view full item" option, they'd probably cope with that as a way to find the item they need to edit, and be less surprised when there is a lot more to it. Possibly. But I wouldn't want to lose the option of just giving them the local task tabs.

(later) I know it is weird, illogical and more work. But it's what they're comfortable with.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
77.47 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/node/node.install. View

Just a no-op reroll cause #115 no longer applies to HEAD.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-128.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
462 bytes
77.47 KB
FAILED: [[SimpleTest]]: [MySQL] 49,726 pass(es), 788 fail(s), and 478 exception(s). View

Just an update function rename.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-130.patch, failed testing.

jthorson’s picture

Status: Needs work » Needs review

Bot is re-testing.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-130.patch, failed testing.

jstoller’s picture

Status: Needs work » Needs review
FileSize
79.94 KB
FAILED: [[SimpleTest]]: [MySQL] 50,381 pass(es), 729 fail(s), and 437 exception(s). View

Fixed merge conflicts, plus a few minor tweaks. New content types will now have revisions enabled by default. This should only effect articles in the standard install. We may or may not want to change this later, but it makes testing the patch easier for now.

Status: Needs review » Needs work

The last submitted patch, save-as-draft-1776796-134.patch, failed testing.

webchick’s picture

Issue tags: +Spark

Tagging as Spark as something for Kevin to look into.

klonos’s picture

...I know that the concerns about security etc. with regards to the "Publish" tab being a direct action vs. a first step to a confirmation page were answered, but I'd like to say that now that we have modal dialogs in D8 it will most likely not make sense to have confirmation forms exist in tabs. So the screenshot in #111 doesn't make sense to me and I consider it a waste of space to only place a confirm/cancel set of buttons in a whole tab. Just sayin'.

#126: AMEN!!!

jstoller’s picture

@klonos, I'm not familiar with the modal dialogs in D8. Is there any reason the "Publish this" local task couldn't trigger a modal dialog, instead of loading a page? Any examples of this sort of thing I can look at?

klonos’s picture

Main issue where the feature was added: #1667742: Add abstracted dialog to core (resolves accessibility bug)

Change notices:
Dialog API for JavaScript
'dialog' key added to #ajax settings

Related/follow-up issues where you might find how others are using it:
#1842040: [meta] Decide on where to use modal dialogs
#1842036: [META] Convert all confirm forms already converted to new routing system to use modal dialog
#617730: UX: Use modal dialogs for confirmation forms.
#1857398: Move the request for current password on email/password change to a modal dialog.
#1851414: Convert Views to use the abstracted dialog modal

Is there any reason the "Publish this" local task couldn't trigger a modal dialog, instead of loading a page?

Nope, that's precisely what I'm suggesting in #137. Having all most-used actions in one place is a good UX practice IMO. I realize though that what I am suggesting would break the behavior consistency of the local tasks (some behaving as tabs opening new pages while some others behave as buttons causing modals to pop up).

yoroy’s picture

Are #105 and #111 still the latest in how the UI looks? I don't think using a tab label to communicate status is a good idea. But discussion about tabs and/or contextual links seems a bit too much about the details, where I'm not sure yet the overall approach is settled on. Or is it?

I'm hesitant to re-read the whole thread, but it looks like the tour in #94 captures what we want to achieve with this issue. Right?

If so, that is a good write-up to use for prototyping a bit more.

jstoller’s picture

I think #105 is still pretty accurate, except for the changes shown in #115. #111 is accurate right now, but given @klonos' comments, those confirmation pages should be replaced with modal dialogs. There have been quite a few changes since #94, so I'm not sure I'd use that as the starting point.

From a UX perspective, here are my priorities:

  1. The node edit form submit button must be changed to allow the creation of forward revisions.
  2. The user must know at a glance whether he is viewing the published version of a node, the draft version, or just some random revision.
  3. If the user is viewing the published revision and there is also a newer draft revision, then that must be readily apparent. And vise versa.
  4. No matter where a user is when they select the edit action, the edit form should always load the edit revision (in the default implementation this will always be the latest revision). The UI should be such that the user expects this behavior.
  5. Actions like publish, unpublish, revert, and delete, should be available when viewing a node in situ, rather than requiring the user to navigate to an edit form. Edit forms are for editing content. Not for making workflow decisions.
  6. When a user edits a node and saves it, they should be taken to the view page for the new version they created, whether that be published or draft.
  7. The "Create new revision" checkbox needs to change (see #115), or go away completely.

Tabs are certainly not the only way this can be accomplished and they may not be the best way. However, I'm also conscious of the need for a generic solution that is largely independent of which theme is being used and which core modules are enabled. Given this and give the current state of Drupal's UI, tabs seem like an appropriate solution.

jstoller’s picture

Just a reminder that much of this UI work is leveraging additions to the API behind the scenes. So while continuing to hammer out the UI is great, it would be really nice if someone took a look at the rest of the code to see if there are any issues. In some ways getting that back-end stuff in is even more important than the front-end, since it will help support contrib.

webchick’s picture

Issue tags: -Spark

Curating the Spark issue queue, and I don't think this is something strictly in our wheelhouse.

kscheirer’s picture

I think this needs a backport to D7, these issues were closed in favor of this one:

Does that make this a bug instead of a feature request?

kscheirer’s picture

Issue summary: View changes

Updated issue summary.

jhedstrom’s picture

I'm guessing this has to be postponed until at least 8.1? If not, this would greatly simplify work that needs to happen for contrib workflow modules if this was in place when 8.0 was released.

webchick’s picture

Generally, yes, this would be punted to 8.1.x. However, if it was a UI change only and didn't change APIs, and those changes were pretty isolated, I think we could potentially still get it in in 8.0.x as a usability improvement (the UI is still unfrozen). AFAIK we should have the necessary API underneath; building a UI would help verify that.

jhedstrom’s picture

Issue tags: +Needs reroll

Hopefully the API changes from #134 will be small enough so as to be outweighed by the UX benefits.

Patch absolutely needs a reroll, keeping in mind that the interface changes should probably go in ContentEntityInterface given #2004244: Move entity revision, content translation, validation and field methods to ContentEntityInterface.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom
Issue tags: -Needs reroll

I'm working at porting the approach from #53 to work with the latest HEAD. Since it doesn't touch existing APIs, it will be a good starting point to see what, if anything, actually needs to change in the core APIs.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
34.73 KB
17.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,102 pass(es), 1 fail(s), and 0 exception(s). View

This approach builds on the module approach from #53. I wanted to see how far one could get without touching existing APIs. (Also, my intent is not to ignore the subsequent work that happened since that patch, but that needs substantial rerolling, and also may be too late for 8.0 since it heavily alters the current API.)

I removed the moderation field approach, and instead adopted a simpler concept of draft: If there are any revisions newer than the current revision (eg, the node available at node/X), those are considered drafts. A new button is added 'Save as draft', and the draft node is loaded into the edit form if one exists. I'm told this is how Wordpress works in regards to its draft system.

I don't think this is complete by any means, but hopefully it will pick up the conversation again.

One big omission is the revision overview page. I wanted to change the text 'Revert' to 'Publish draft' where applicable, but I didn't see any immediate way to alter a route without completely taking over the controller.

Status: Needs review » Needs work

The last submitted patch, 150: save-as-draft-1776796-150.patch, failed testing.

webchick’s picture

We used to have hook_page_alter() for this type of thing in D7 but it was removed for cache-ability reasons: https://www.drupal.org/node/2357755

I posed this question to effulgentsia and here's what he said:

[2/17/15, 3:48:48 PM] Alex Bronstein: 1) You can add a view subscriber that runs at a higher priority than main_content_view_subscriber. That would be how I'd do it until one of the following is available.
[2/17/15, 3:49:43 PM] Alex Bronstein: Note, the above isn't heavyweight, because you wouldn't be stopping main_content_view_subscriber from running, you would just be altering the render array before it runs.
[2/17/15, 3:51:38 PM] Alex Bronstein: 2) If we don't like the DX of adding subscribers to do something this common, then we can add one in core that calls out to a hook. This would be like hook_page_alter(), but the key difference is you wouldn't be altering the PAGE, you would only be altering the controller result (main content). That's the key difference with respect to cache ability / ESI.
[2/17/15, 3:52:50 PM] Alex Bronstein: 3) For these kinds of tables specifically, I think it would be good for us to be using theme suggestions. So instead of
[2/17/15, 3:52:52 PM] Alex Bronstein: $build['node_revisions_table'] = array(
'#theme' => 'table',
[2/17/15, 3:53:00 PM] Alex Bronstein: I suggest (pun!):
[2/17/15, 3:53:12 PM] Alex Bronstein: $build['node_revisions_table'] = array(
'#theme' => 'table__node_revisions',
[2/17/15, 3:53:47 PM] Alex Bronstein: Which would then allow changes to be done via a preprocess (or rather, would allow that if we ever manage to also fix https://www.drupal.org/node/939462)

jhedstrom’s picture

Status: Needs work » Needs review
Related issues: +#2429153: On node revision overview, use 'Set current' if revisions are newer than current version
FileSize
6.94 KB
20.73 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,313 pass(es). View

For now, rather than pursue the approach in #152 (which will be necessary for contrib), I spun off this issue to accomplish what is really needed here: #2429153: On node revision overview, use 'Set current' if revisions are newer than current version.

This patch adds the following:

  • Redirects user to the draft view page after saving
  • Removes ability for non-privileged users to 'save as published' for content types that default to unpublished (instead they 'save as draft')
  • New tests for the above
jhedstrom’s picture

FileSize
2.01 KB
21.53 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch save-as-draft-1776796-154.patch. Unable to apply patch. See the log in the details link for more information. View

This patch finishes the access logic for the draft page/tab.

This patch will most likely not go into 8.0 as-is, given that it adds an entire module. One possibility would be to adopt some of the logic from the new module to a minimal set of changes directly to the node module.

This would require that folks agree to an MVP workflow as outlined by the test included in this patch (or something similarly trimmed down), and also that it could go in as a usability enhancement, rather than a feature request.

miro_dietiker’s picture

I stumbled upon this issue because we discussed the fixing of the revision tab in context of multingual workflows.
#2453153: Node revisions cannot be reverted per translation

I see this issue is trying to make the revision system more advanced, but it is completely lacking the consideration of multilingual aspects. I fear that the proposal above is even adding more complexity to revisions with the result that the multilingual challenges are even harder or impossible to resolve at the same time.

We really need a resolution for the multilingual workflows in the revisions tab before adding new core features such as revision states.

jhedstrom’s picture

Fabianx’s picture

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

Feature Request + 8.0.x => 8.1.x

What is needed in Core to allow moderation to work properly as a contrib or 8.1 core add-on?

jhedstrom’s picture

re: #158 @Fabianx I think #2429153: On node revision overview, use 'Set current' if revisions are newer than current version would be nice to get into 8.0.0 to allow contrib moderation--overriding the revisions tab is quite difficult as it is now.

jhedstrom’s picture

I just created #2511584: Move NodeFormButtonsTest::assertButtons() to a trait, which will handle the only parts of the patch above that aren't in a new module. This fix will let contrib more readily test the node form without having to duplicate that method.

Status: Needs review » Needs work

The last submitted patch, 154: save-as-draft-1776796-154.patch, failed testing.

brantwynn’s picture

Issue tags: +Needs reroll
DamienMcKenna’s picture

Just wanted to say - a huge +1 for this effort!

googletorp’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.77 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Rerolled patch

Status: Needs review » Needs work

The last submitted patch, 165: provide_a_better_ux_for-1776796-165.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 165: provide_a_better_ux_for-1776796-165.patch, failed testing.

Jaesin’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
16.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

Anyone know why Drupal\node\Tests\NodeTestBase resolves but not Drupal\node\Tests\AssertButtonsTrait. It obviously did in #153. Compile-time vs run-time?

Status: Needs review » Needs work

The last submitted patch, 169: 1776796-169.patch, failed testing.

samuel.mortenson’s picture

AssertButtonsTrait doesn't exist in 8.1.x, #153 was submitted before the version changed. We'll need to bring the 8.1.x branch up to date with 8.0.x before tests start passing.

Jaesin’s picture

@samuel.mortenson thanks for pointing that out.

Jody Lynn’s picture

  1. +++ b/core/modules/moderation/moderation.module
    @@ -0,0 +1,52 @@
    + * The core module that allows moderation of revision-able entities.
    

    This description is worse than the one in previous patch - 'the core module' is redundant information and 'revision-able' should be 'revisionable'

  2. +++ b/core/modules/moderation/moderation.module
    @@ -0,0 +1,52 @@
    +function moderation_node_has_draft(NodeInterface $node) {
    

    This function smells. It's named like it just returns a boolean but it's used actually to get IDs.

    Replace it with a function that actually just gets the revision id. Something like:

    /**
     * Get the latest revision of a node.
     *
     * @param \Drupal\core\NodeInterface $node
     *   The node object.
     *
     * @return int
     *   The revision ID of the latest revision..
     */
    function moderation_latest_revision_vid(NodeInterface $node) {
      $node_storage = \Drupal::service('entity.manager')->getStorage('node');
      $vids = $node_storage->revisionIds($node);
      asort($vids);
      return array_pop($vids);
    }
    
  3. +++ b/core/modules/moderation/src/Access/DraftAccess.php
    @@ -0,0 +1,56 @@
    +  public function access(Route $route, AccountInterface $account, NodeInterface $node = NULL) {
    

    The ability to view a revision should be consistent with other access control for viewing revisions, not based on ability to update a node.

    This is closer:

    public function access(Route $route, AccountInterface $account, NodeInterface $node = NULL) {
        $latest_revision = moderation_latest_revision_vid($node);
        $access_control_handler = \Drupal::service('access_check.node.revision');
        $node = $this->nodeStorage->loadRevision($latest_revision);
    
        return AccessResult::allowedIf($node && $access_control_handler->checkAccess($node, $account, 'view'))->cachePerPermissions();
      }
    
ramarajuk78’s picture

will this make it into the core. I am planing to build a new site in drupal 8. content moderation is one of features that is essential for us. Are there any plans for workbench or the workflow modules to be ported to d8.

Thanks,

grndlvl’s picture

This addresses the concerns in #173.

Except:
The ability to view a revision should be consistent with other access control for viewing revisions, not based on ability to update a node.

At first I was thinking that maybe this isn't what we want, but then again... if it's published why not. Although, do you want users to be able to view the non-front facing published revision as it would be possible to have a published revision that is not the latest (someone correct me if I am wrong just remembering the current status based on this ticket.)

Also, I added the ModerationInterface and NodeModeration services and switched out all instances of moderation_node_has_draft() to our new ModerationInterface::hasDraft() or ModerationInterface::getDraftRevisionId().

I'll be looking into the following issue next:
After you have a draft revision to publish that is not the very first revision it is not saved as the published node...
To replicate:
1) Create a moderated node as Unpublished
2) Re-edit and "save as draft"
3) Edit new draft and "save as published"
I suspect that this has something to do with the removal of NodeForm::publish() in #2498919: Node::isPublished() and Node::getOwnerId() are expensive. Not sure what's with with NodeForm::draft() method. I didn't notice it being called elsewhere nor does it do what it describes. Is it necessary? If so where does it get called from?

grndlvl’s picture

FileSize
16.58 KB
22.59 KB

OK. I fixed the issue of the above scenario. I also fixed the cache issue with the Draft tab not showing up all the time.

The tests are now back up to snuff and passing with the node creation at least.

Some remaining issues:
* On the revisions page Revert shows up on the current published revision, however, when you attempt to "revert" you get an access denied, but only for the current published revision one.
* Write tests for revisions page.
* Revisions page needs work and I think we just need to figure out what we need to do for that.
* Others?

Note: The patch is against 8.0.x and not 8.1.x as 8.1.x is super old and is missing a good bit of what's needed for this to work as expected.

grndlvl’s picture

I have made a repo on github with these most recent changes over at https://github.com/grndlvl/moderation

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.