U4-10023 - Update all tree/tree pickers, etc... to support multiple start nodes

Created by Shannon Deminick 14 Jun 2017, 20:35:41 Updated by Shannon Deminick 01 Aug 2017, 07:51:57

Relates to: U4-10147

Subtask of: U4-8632

Update all tree/tree pickers, etc... to support multiple start nodes

Comments

Dan White 19 Jun 2017, 17:12:09

Please consider an optional setting that allows users to pick nodes above their start nodes. In our case, users should only be able to edit specific sections, but can pick/link to any page or media on the site.

We did it via a config setting (LimitPickersToStartNodes) in the Multiple Start Nodes package (https://our.umbraco.org/projects/backoffice-extensions/multiple-start-nodes/)


Stephan 12 Jul 2017, 10:41:53

Status:

  • the main tree seems to support multiple start nodes
  • including duplicates and nested start nodes
  • the basic Umbraco.ContentPicker2 picker seems to be ok too
  • the Umbraco.MultiNodeTreePicker2 seems to be ok too


Stephan 12 Jul 2017, 10:48:12

@dawhite I have moved your request to U4-10147 as it's a different thing - here we are "just" making sure that multiple start nodes don't break the existing stuff - hope that's ok


Stephan 12 Jul 2017, 11:05:37

Status:

  • media pickers incl. picking images in the grid, all seem to be ok


Stephan 13 Jul 2017, 11:43:44

Cannot really find something that does not work? => review


Claus Jensen 13 Jul 2017, 13:10:07

Media picker (both the MediaPicker2 and the obsolete one) does not work with multiple start nodes assigned.

If I assign multiple start nodes directly on the user:

  • I only see the first start node I have assigned - and no way to navigate into any other start nodes.

If I assign multiple start nodes via two different User Groups (both groups assigned to the user):

  • I only see the first start node, but the picker seems to "know" the path of this folder. So I actually see a link to the parent folders although I'm not allowed there. When I navigate to the immediate parent folder (which I dont actually have access to .. I have access to its two children), I can see and access any of the child folders I have access to, but if I then navigate up to the root of media where I am not allowed, I'm stuck and can't get anywhere or get back.

(and since we're storing the "last browsed folder" for media pickers in a cookie .. there's really no way back from here as it will always open up at this folder...)

Content pickers (new and legacy) seems to be working fine though; tested with start nodes directly assigned to the user, and also via start nodes assign by having multiple groups.


Claus Jensen 13 Jul 2017, 13:27:14

Have gone through whatever else I could find .. everything else ''seems'' to be working :) RelatedLinks, mntp configured both for content and for media .. and also the obsoleted ones.


Stephan 13 Jul 2017, 14:17:19

Confirmed - looking into it


Stephan 14 Jul 2017, 11:15:40

It's not only the pickers, the main media tree is totally confused with multiple start nodes ;(


Stephan 14 Jul 2017, 15:25:32

So, I can fix the media picker so that it only show the proper nodes at top.

But then, few issues:

  • the search box needs to be adapted to filter out nodes that are not under start nodes
  • the breadcrumb show the full path from root, thus exposing nodes that should not be visible, and needs to be adapted to hide them


Stephan 14 Jul 2017, 16:06:13

Fixing (to be reviewed):

  • the main tree search should return items below start nodes ''and'' start nodes not exclude these start nodes
  • the media picker breadcrumb should skip nodes above start nodes, not show them - so if structure is Media/foo/bar/nil and bar is your start node, the breadcrumb should show Media/bar/nil"
  • this should also fix the breadcrumb at the bottom of content/media editing


Stephan 14 Jul 2017, 18:09:03

Fixing (to be reviewed):

  • searching in the media picker should only return accessible node
  • to review: create NodeA > NodeB > NodeC and set NodeB as start node, searching should return NodeB and NodeC but not NodeA

works but to achieve this I had to modify EntityController.GetPagedDescendants so that when getting descendants of the root node, we instead start at start nodes. wondering if it may break other parts of the back-office?


Stephan 14 Jul 2017, 18:09:40

So... running out of ideas re. how to break these pickers, so moving back to review.


Shannon Deminick 17 Jul 2017, 08:51:58

There's plenty more that will not work. In fact none of the media pickers or link pickers 'work'. None of them will display multiple start nodes, they will only display the first start node found in the user's list of start nodes. This is the code that will launch the media picker dialog:

$scope.mediaPickerOverlay = {
   	 currentTarget: currentTarget,
		onlyImages: true,
		showDetails: true,
		disableFolderSelect: true,
		startNodeId: userData.startMediaIds.length === 0 ? -1 : userData.startMediaIds[0],
		view: "mediapicker",
		show: true,
		submit: function(model) {
			tinyMceService.insertMediaInEditor(editor, model.selectedImages[0]);
			$scope.mediaPickerOverlay.show = false;
			$scope.mediaPickerOverlay = null;
		}
	};

You can see the problem, the startNode that will be passed up is just a single start node and it will just pick the user's 'first one' so multiple start nodes will not work. This same code is used like this throughout the js!

This is what we need to be careful about fixing because people use this JS code a lot in their own code. So we have some options:

#Allow an Array to be passed in for the startNodeId parameter OR #Have another property called startNodeIds and we check for that one, if it's not there we'll fallback to checking the singular one OR #We don't take into account the user start nodes in this code at all! The media pickers can have a pre-value defining a single start node, we will need to of course use this, however, if one is not defined than we could probably just pass up the root node id and have the server side deal with the user's start nodes and return the data accordingly ... This is probably the 'best' approach

--Other things that need to be fixed:--

  • --A User Group will need to explicitly select the root media or content node to give it access to the root, otherwise it will not have any start node selected - I've fixed some logic to allow this: e13529198032985c80f561a19b566e0d6a02ae21--
  • --This means that the tree must be able to select the root node, currently it cannot do this (i'll look into this)--


Stephan 18 Jul 2017, 14:08:29

Ah... we have fixes here and there so that if you as for children of the root media, we "cook" the list according to start nodes - so it works as long as the user does not have a root media specified and we start at -1 - but as soon as the user has a root media specified, we get children for eg node 1146 and the mechanism does not work anymore - wondering if... soon as the user has more than 1 start node we should revert to root/-1 and let the existing mechanism deal with it (however dirty it may be)?


Shannon Deminick 19 Jul 2017, 00:46:58

@zpqrtbnk yes exactly and is what I mentioned above (Option #3). In all places in the JS where we do something like:

startNodeId: userData.startMediaIds.length === 0 ? -1 : userData.startMediaIds[0],

this shouldn't be done at all. For any picker that has a pre-value pre-set as part of it's pre-values, we'd pass in the startNodeId to it but otherwise we should just pass the root (-1) as the Id and let the server deal with the user's start nodes.


Stephan 20 Jul 2017, 15:55:21

Done:

Fixed User deep-cloning that had threading issues.

Replaced line above with startMediaIds.length !== 1 ? -1 : startMediaIds[0] -- so if it's only 1 start node we use it else we use -1 and our server-side logic will understand it needs to deal with start nodes.

Refactor so that no start node means no start node (and not, "everything").

Review:

Create a group that has a content start node, but no media start node. Create a user in that group. So that the user has no media start node at all. Now create a content type with all sorts of media pickers, create and edit a corresponding content item, and make sure that the media pickers are all empty (nothing to be seen, searched, uploaded).

Then, do the opposite: a media start node but no content start node. A content type with all sorts of content pickers. A corresponding media item. All content pickers should empty.

Todo:

Create migration that runs if the 'administrator' group exists and has no start nodes (otherwise... bah) - and sets start nodes on our built-in groups (-1/-1) - and not sure about existing groups?

Fix the media pickers, still show the upload / create folder in places where they should not. Fix many things that probably don't work yet.


Stephan 26 Jul 2017, 08:49:09

Pushed [15c5900a|https://github.com/umbraco/Umbraco-CMS/commit/15c5900a249338625b79581154badb4bf582cc38] which makes sure that the media picker disables uploading and creating folder under the ''Media'' root in case it is "virtual" ie not the actual start node, but just the container for several start nodes.

Test: create a user that has several media start nodes. Open the media picker for this user. Should show the ''Media'' root, with the start nodes underneath. Within the start nodes and under, it should be possible to click ''+'' and add a folder, click ''Upload'', and drag-and-drop files. But on the ''Media'' root, these 3 things should be impossible.


Stephan 26 Jul 2017, 09:42:52

Pushed [0762bd7e|https://github.com/umbraco/Umbraco-CMS/commit/0762bd7e766308c1dfa105b70d18168c49aec807] which ensures it also work when picking an image from the Grid.

Test: pick an image from the grid, for a user having several start nodes.


Stephan 26 Jul 2017, 09:45:30

So... cannot find another picker/tree/whatever that multiple start nodes would break

Moving this back to review!


Shannon Deminick 28 Jul 2017, 06:13:26

Test #1 - lock down media pickers is successful only for the actual property editors that are media pickers but this does not work for the TinyMCE media picker dialogs (including link picker). Another issue I've found is that the logic for start nodes hasn't been updated for the dashboard security which is still checking like: currentUser.startMediaIds.length === 0 || currentUser.startMediaIds.indexOf(-1) >= 0

I'll fix this up.


Shannon Deminick 31 Jul 2017, 05:54:24

I've fixed up the media uploading for the RTE media picker and link picker: 9db91124075b46ced18c662f66b0f1b2ba61c214, 6de2cea9c5a9c87332679a1e793684e69cd29b66 will look at the dashboard security now and then continue testing.


Shannon Deminick 31 Jul 2017, 06:43:17

I've tested the opposite so created a media type with all of the content pickers and have a user that has no start node access to content and the pickers work correctly (don't list anything, no search results)


Priority: Normal

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.7.0

Sprint: Sprint 64

Story Points: 3

Cycle: