U4-10995 - Selecting non-root content start shows page twice in content tree

Created by Matt Cheale 20 Feb 2018, 15:00:37 Updated by Sebastiaan Janssen 19 Jun 2018, 12:50:17

Tags: Gold partner

Relates to: U4-11341

Steps to reproduce:

  • Install Umbraco 7.8.1 as default
  • Create new user and add to "Writer" group
  • Add the "About Us" page to the user's "Content start nodes"

Expected: "About Us" is only at the root of the content tree, OR, the tree is shown greyed out as is and the root version not shown

Actual: Both are shown, see attached image.

May have some cross over with U4-10820

1 Attachments

Comments

Dan Bramall 20 Feb 2018, 15:14:44

To add to this, removing the start node from the User Group and adding it to the individual user still yields the same behaviour. The duplicate tree seems to show up any time the start node is set to anything except the top-most content node.


Matt Cheale 20 Feb 2018, 16:32:27

Looking at the code: Umbraco.Web/Trees/ContentTreeControllerBase.cs:148-164 seems to add all the start ids and then add all the root nodes for those start ids. But when this ends up in the umbTree it's not de-duping the nodes at the root and in the tree context. Were you intending to only return the top level nodes from GetApplicationTrees so the user needs to dig down?


Georgs Bormanis 15 Mar 2018, 13:28:10

I can reproduce this on 7.9.2 as well.


Anders Brohäll 23 Mar 2018, 09:08:49

In what version was this behaviour introduced? What issue?


Markus Johansson 23 Mar 2018, 11:35:15

It looks like this is by design. Removing the part below this comment will render only the nodes that the user has access to (https://github.com/umbraco/Umbraco-CMS/blob/7ee510ed386495120666a78c61497f58ff05de8f/src/Umbraco.Web/Trees/ContentTreeControllerBase.cs#L159).

I'm not sure if I agree with the decision to show the root nodes to provide "provide some context". @sebastiaan What would you say? I could do a PR and remove this part - but not sure if we should remove it?

Edit: Or maybe if could be some kind of setting somewhere if we really want to keep the whole tree?


Dan Bramall 23 Mar 2018, 12:04:06

Personally I feel it's confusing to have duplicate content trees (I guess everyone who's voted for this issue agrees). For my editors at least, it's a show-stopper, there'd be no end of confusion so everyone has root access for now. Clearly there was a reason for this being added but I'm curious as to what it is/was specifically and whether it's still relevant.


Markus Johansson 23 Mar 2018, 12:34:29

@Dan.Bramall I agree, if its a show-stopper, I actually tried to remove the lines in the code and recompile and after that it worked as before so if you really need a solution asap you could compile your own umbraco.dll until it's solved in the core.


Markus Johansson 26 Mar 2018, 11:50:10

@Shandem or @sebastiaan what would you guys say? If you want me to create a PR I'm on it =D


Shannon Deminick 16 Apr 2018, 06:23:59

This is probably a bug. Yes, we want to show each start node that a user has and it's full tree... there are reasons for this, it's intentional and this should not be just removed. For example, you can have a tree:

  • EN ** Home ** News
  • FR ** Home ** News
  • SP ** Home ** News

And you want a user to have access to edit "News" in all languages but nothing else. Well in previous versions, they would just see this when they login:

  • News
  • News
  • News

This is not helpful. Now they will get a context of where they are in the tree.

In this case it's not composing from the common root node, which it probably should so you don't end up with what looks like 'duplicate' start nodes that are the same.


Robert Copilau 17 Apr 2018, 13:32:34

PR:https://github.com/umbraco/Umbraco-CMS/pull/2591

How to test: *Issue description does a pretty good job of showing how to test.

Note: --*Custom user groups seem to be broken right now so don't test with those.--


Markus Johansson 18 Apr 2018, 11:07:28

@Shandem @robertcopilau Makes sense to show the parent nodes down to where the user have access, but I also think that nodes that the used don't have access to that's not used to build the path should not be shown.

Ie

EN

  • Home
  • News

FR

  • Home
  • News

SP

  • Home
  • News

If the user does not have access to "Home" (or any of its descendants) Home should not be rendered, the tree should look like this:

EN

  • News

FR

  • News

SP

  • News

Not sure if you agree but for me that should make more sense.


Sebastiaan Janssen 18 Apr 2018, 14:02:49

@enkelmedia That's not how it was designed. It's good for the editor to see the entire path anyway, so they know what exactly they're editing and in which area of the site.

You can only see the path, nodes that you don't have access to are not even clickable.


Markus Johansson 18 Apr 2018, 14:47:03

@sebastiaan I just tested around and if I was to put all these news-containers as Start Nodes for the users they would not see other nodes that they don't have access to - that was what I was looking for. Sorry for the confusion from my side.


Sebastiaan Janssen 18 Apr 2018, 14:49:03

No worries!


Shannon Deminick 19 Apr 2018, 05:28:53

@enkelmedia Exactly, what you mentioned above is how it works, it only shows any nodes that are in the path of your start node, not ALL nodes.


Markus Johansson 19 Apr 2018, 07:41:30

@Shandem =D Great stuff, sorry for my confusion =D


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.8.0, 7.8.1, 7.9.2, 7.10.3

Due in version: 7.10.4

Sprint: Sprint 83

Story Points: 1

Cycle: 9