Problem/Motivation

We want to show users with the appropriate permissions what the current draft would look like were it to be published.

Proposed resolution

Add a "Latest revision" tab that appears only if there is one, accessible to just people with appropriate permissions. (I figure edit access?)

Remaining tasks

* Add a route
* Add a local task
* Use an access checker to block access to the tab if there is no forward revision
* Use/reuse the param checker to ensure the latest-revision tab always shows the forward-most revision
* Figure out why we're getting a double-title
* Test it all

User interface changes

A new tab appears!

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

Comments

larowlan created an issue. See original summary.

Crell’s picture

I'm actually not 100% certain if we should take this approach, or put it on a separate tab. We should get a UX person to weigh in on that. Either way, some way to preview the entity in "view" mode before publishing it is needed.

larowlan’s picture

Asking UX guru

pameeela’s picture

FWIW I agree you need to be able to see both, and I think by default node/x should be the published version. The idea that what version you see at node/x could depend on your permissions is problematic.

But in D7 it is easy to miss the View published / View draft tabs and I among others have been thrown by it, wondering why I can't see my changes and then realising I'm on the wrong tab (most common reason for this is edit from admin/content, save new draft, end up on admin/content, click your content and expect to see what you just did - but instead you are seeing the published draft).

Perhaps the best approach is to provide a more obvious prompt to the user about which version they are seeing?

Also maybe if there is a draft of content, go to node/x/draft over node/x if user has permission to view drafts?

jptaranto’s picture

What Pam says!

Changing the behaviour of the url based on permissions is disorientating. node/x should always have the same behaviour regardless of what perms you have.

The D7 workbench block was often pretty handy. Ideally something similar, a large, obvious status block that says something like "Viewing latest draft, view current published revision" and vice versa. Colour coding would be good too. Published revisions are green, unpublished drafts are orange.

You could then keep the standard core tabs of "view / edit". The view tab is always for viewing either the draft or published version (and the status block lets you flick between them). Which version that displays by default could be decided by a user setting, a permission, or a standard approach for all users. The edit tab allows you to edit the draft (or create a new one if one does not exist).

Crell’s picture

Title: Let node/x show the latest revision for those with sufficient perms » Add a View-latest tab to show forward revisions
Project: Moderation State » Workbench Moderation
Version: » 8.x-1.x-dev
Assigned: Unassigned » Crell
Issue summary: View changes

Retitling and moving to the WBM project. Also updated issue summary.

Crell’s picture

WIP patch.

The main bug at the moment is the ModerationInformation::hasForwardRevision() method. The @todo notes the assumption it makes, which breaks as soon as the upcaster is added so that the view-latest tab gets the right revision. :-) I will try and sort that out tomorrow. Ideally we can get this merged before I go on break Thursday. (After that I won't be back to this module until after New Years. Vacation!)

Code review welcome in the meantime. I'm in particular not at all sure that the extra flag I added for the ParamConverter is appropriate, but I couldn't think of anything more elegant off hand. Ideas?

dawehner’s picture

FileSize
3.37 KB
  1. +++ b/src/Access/LatestRevisionCheck.php
    @@ -0,0 +1,84 @@
    +    // This tab should not show up period unless there's a reason to show it.
    +    // @todo Do we need any extra cache tags here?
    +    return $this->moderationInfo->hasForwardRevision($this->loadEntity($route, $route_match))
    +      ? AccessResult::allowed()
    +      : AccessResult::forbidden();
    

    Well, ideally we should add the one from the entity itself

  2. +++ b/src/Access/LatestRevisionCheck.php
    @@ -0,0 +1,84 @@
    +   */
    +  protected function loadEntity(Route $route, RouteMatchInterface $route_match) {
    +    // Split the entity type and the operation.
    +    $requirement = $route->getRequirement('_entity_access');
    +    list($entity_type, $operation) = explode('.', $requirement);
    +    // If there is valid entity of the given entity type, check its access.
    +    $parameters = $route_match->getParameters();
    +    if ($parameters->has($entity_type)) {
    +      $entity = $parameters->get($entity_type);
    +      if ($entity instanceof EntityInterface) {
    +        return $entity;
    +      }
    +    }
    +    return NULL;
    +  }
    

    Yeah I'm really not sure whether this magic (probably copied from EntityAccessCheck) was a good pattern in the beginning. Maybe specifying that explicit on the route is actually better.

  3. +++ b/src/EntityTypeInfo.php
    --- a/src/ModerationInformation.php
    +++ b/src/ModerationInformation.php
    

    While reading through ModerationInformation I'm pretty sure that \Drupal\workbench_moderation\ModerationInformation::isLiveRevision
    should not check whether its the latest revision.

  4. +++ b/src/Routing/EntityModerationRouteProvider.php
    @@ -0,0 +1,114 @@
    +        ->setRequirement('_entity_access', "{$entity_type_id}.update")
    +        ->setRequirement('_workbench_moderation_latest_version', 'TRUE')
    

    We seriously need to come up with a solution for the underscore issue here, this is just madness.

  5. +++ b/src/Routing/EntityModerationRouteProvider.php
    @@ -0,0 +1,114 @@
    +          $entity_type_id => ['type' => 'entity:' . $entity_type_id, 'load_forward_revision' => 1],
    

    I'd have preferred entity_latest_revision:$entity_type_id to be honest. Its more explicit that we use a different parameter converter

Here are just some minor stuff changed while reading the code

Crell’s picture

Committed the patch from #8, thanks!

2. Yes, that's where I copied it from for lack of a better alternative. I am very open to suggestions. I don't know what the good pattern here is.

3. Hm. Maybe? We really need to put some tests around ModerationInformation...

4. I do not disagree. Suggestions welcome.

5. I don't know where all the entity type information is used. IIRC Panels uses it, too, or will, so I didn't want to mess with it. It's not clear what all uses are of the various options on the route. Again, very open to suggestions on a cleaner way of doing all of this since I'm not a big fan of what we've got right now.

Crell’s picture

New patch! Fixes the logic issue mentioned before, doesn't fix the other route tagging issues dawehner mentioned. I'm still not sure what to do with those, or maybe just punt since this does work.

The only remaining issue IMO is that the view-latest tab is now showing the full version of the entity... but with the title repeated as a link. That seems wrong, but I'm not sure yet what's causing that. Suggestions for where to look welcome. (Or, just someone fix it in an interdiff. :-) ).

Interdiff is post-dawehner's additions from #8.

Crell’s picture

Status: Needs review » Fixed

Actually, change of plans. There's enough other bugs being fixed as part of this issue that I'd rather just get it in. I've opened #2639990: View-latest tab rendering wrong as a follow-up, and we can also adjust the other issues dawehner mentioned in follow ups if needed. This gets us to a stable point for me to break for the holidays. :-) I'll spend the rest of today working on tests and core issues related to this module.

Forward (revisions)!

The last submitted patch, 7: view-latest.patch, failed testing.

Status: Fixed » Needs work

The last submitted patch, 10: view-latest.patch, failed testing.

The last submitted patch, 7: view-latest.patch, failed testing.

The last submitted patch, 10: view-latest.patch, failed testing.

The last submitted patch, 7: view-latest.patch, failed testing.

The last submitted patch, 10: view-latest.patch, failed testing.

jibran’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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