Drag and Drop Parent Folder into Subfolder

Thank you for reporting this issue. Really seems critical to me too.

I'll provide a fix in the next days.

jsTree does not allow that anymore, showing a red cross. (running 3.3.9) now 3.3.10 is out.
Anway the event was still fired, as of 3.3.8, although already a red cross was shown ..

The whole folder moving seems to be "buggy" older versions of jsTree - strongly recommend to update to the latest version.

For sure we should double check .. the AJAX request still can be made.

Here a patch - needs to be tested with jsTree 3.3.8, otherwise the new code won't be touched.

Testing this issue as well as it's patch at #5:

-----

BEFORE patch testing:

1/ I created a taxonomy called "directories" to configure my Media Directories settings.

2/ I created a parent folder, and a subfolder as shown here:

and the corresponding hierarchy at /admin/structure/taxonomy/manage/directories/overview

3/ I started moving the subfolder1 directory to its child directory: a red cross appears suggesting this move in

4/ If yet release the move, the below structure is completely lost. Because the subparent1 and sub-subfolder1 does no more exists, the structure is also destroyed at /admin/structure/taxonomy/manage/directories/overview
Also, because Media are assigned to a term that is no more existing, the Media inside sub-subfolder1 do not appear anymore, I suppose because it is still assigned to a term that does not exists and can't be filtered.

NOTE: contrarily to annonced in #4, the issue still exists when using the 3.3.10 library as shown in above test. This means there is currently no way to actually work-around this issue. Hence the "critical" on this issue.

NOTE2: I also tested using the "master" version itself and the issue still exists.

NOTE3: I tested on the v4 in dev version, but it is too different to be compatible and usable currently

--------

AFTER patch testing:

Using the same protocol from 1 to 3 as described previously: everything is working the same.

4/ When releasing the drag and drop: a drupal message appears to warn the user why the operation was aborted.

The patch works properly and was functionnaly manually tested with all above procedure with all versions from 3.3.6 to 3.3.10.

However, when doing the move (4) multiple times, the drupal messages cumulates:

Technical opinion:
Have a look at this demo code: https://www.jstree.com/demo_filebrowser/index.php
The "red cross" and "green tick" marks are enough to me to let the user understand his move is forbidden. Also you notice there is no ajax call on forbidden moves.

Maybe I am wrong, but by my tests and observing the demo source code :
- dnd_stop.vakata event is fired after every drag&drop action, no matter if valid or wrong
- move_node.jstree is fired ONLY when the drag&drop operation is actually possible and done

Moreover : using the dnd_stop.vakata, the server is responsible to determine the eligeibility to drag&drop, that required to load all parents terms as shown in the patch. This seems to require DB call, ajax call, PHP process for something that is either:
- already working on jstree.js using the proper event
OR
- can be checked JS side, before a drupal ajax call by simply observing the DOM structure.

Therefore, another full JS approach make more sense to me and may seems simpler and more performant.
I will provide a patch soon to POC my words, just not had time to finish at the moment.

Thank you for your exhaustive review, Dom.!

I really thought this does is no longer happening with 3.3.10, but must admit, I didn't really tested that, nor debugged the JS code.

Also I totally agree that we should already prevent this at JS level. I just made sure that the AJAX callbacks are save with patch #5.
Right now we're only handling the dnd_stop event which seems to be wrong, this really could be more reliable using node_move/node_copy(not really sure when this will be triggered) event.

I also thought of exposing setting to disable (initially set to enabled) dnd completely... but this will be done in a new issue: #3180139: Expose setting to enable drag and drop functionality.

After deeper look:

- external nodes are not supported (at least older versions) to throw a move_node.jstree so we must keep dnd_stop.vakata for thoses. External nodes are here the media which can be dragged from outside the jstree (right panel) to the jstree to be changed of folder.
- events can be queued to the original jstree() call as per documentation. This removes a loop .each over all configured jstree in code. The multiple ".on('eventname') have been moved outside that loop
- drag&drop of media is handled externally and configured from media_directories_ui.media.js. I added a IsMedia property on the fake node created by this code to discriminate from folder d&d (the vakata events fires no matter the nature of the element being dropped). Therefore that code can be much simplified to only handle media
- move_node.jstree event have been added to handle folder moving on the jsTree structure. Even back to 3.3.6, this works fine to prevent moving a folder in itself and in a subfolder of itself
- move_node.jstree event is only fired if the move_node operation is allowed. Therefore I had a little change to do in treeCheckCallback() method to allow that operation too

The patch attached does all this.
It does not come with an interdiff since it is the interdiff itself with previous patch : the approach is so different, not a single line (or even file) is shared between the two.

The patch has been tested with all jsTree versions from 3.3.6 to 3.3.10 using the procedure described at #7.

Here the backend and JS approach combined.

I added an additional check in mediaMove, not updating to the current folder.

Your JS goes in almost one 2 one, I made sure to handle the red cross consistantly, and made sure to keep in the not-drop inbetween check.

Tested with jsTree 3.3.8 and 3.3.10.

Thanks again for your efforts, I think we have now a robust solution. Still I would be happy if you could test it too with your procedure :D

I am very sorry but patch #10 is broken from my tests.

1/ The main issue with patch #10 is changes in MediaDirectoriesUiHelper() are missing in the patch, leading to every AJAX request from drag&drop failing with
Error : Call to undefined method Drupal\media_directories_ui\MediaDirectoriesUiHelper::termIsAnAnchestorOf()

2/ I don't understand why adding those (costly?) checks on PHP side since this situation can never happen anymore from the UI. Is this to prevent the case to happen triggering programmatically the service methods ?

3/ I don't understand this below section compared to my patch at #9: what does it functionnaly prevents that is going wrong without it compared to #9 ? drag&drop in sub-child, or drag&drop in itself is already prevented by jsTree itself without having to double check in callback.

if (operation === 'move_node') {
  // While dragging, don't allow dropping between nodes.
  if(more.dnd === true && more.pos === 'i') {
    if (node.parent !== node_parent.id) {
        return true;
    }
  }
  // Handle the last event aka "drop".
  else if (more.core === true) {
     if (node.parent !== node_parent.id) {
        return true;
    }
  }
}

I add a patch #11 identical to #10 adding the missing methods in MediaDirectoriesUiHelper().
Could you eventually explain me the choices over points 2 and 3 please ?

---
NOTE as a possible follow-up: when moving media, if the ajax process is slow (tested on local website with every cache disabled and debug activated, also a poor man's cron simulating a long process on that request) then nothing happens visually on screen until a few seconds when the ajax process is done. Could/should we add somewhere a loading indicator to indicate the process is actually running but slow ?

Sorry, I excluded the methods in the patch by fault. Thanks for bringing them back.

2. Yes, you got me right here - this needs to be in, the AJAX calls still can be crafted, and can maybe shot by JS if jsTree somehow changes the event handling. Beside of that, they need to be save, we never know by what they will be invoked, or the code might be copied.

3. This change makes sure a node cannot be dropped in-between, if you move slowly around the tree you'll see the difference, there is a white arrow just before (b) or after a (a) "row", the more.pos i makes this feature disappearing, and nodes can only be dropped over a row. The last call to this check-method comes suddenly with more.core, and needs special treatment, otherwise the move_node event is not triggered at all. The check for the parent makes sure a red cross is shown, when trying to move a directory to it's current parent (and also make sure the move_node event is not triggered).
#9 was not wrong, but removing functionality, and #10/11 have now enhanced functionality.

I see it the same with moving media, it's slow :( And we should somehow indicate WIP to the user.
I tried yesterday to find an easy way to check for not allowing moving media to the current directory too, so the AJAX call will not be made.
These two things should be done, here, or in another issue. (I also want to make a new release soon ;)

Features missing

From comment #12, I determine the following points:
1) The various points in #11 have been explained, thus patch #11 is technically correct
2) Should-have: add a way to not run the ajax request when moving a media from its folder to itself
3) Nice-to-have: an indicator has per when moving folder (green tick / red cross) when moving a media
4) Should-have: an indicator to identify the moving of media is running since this operation can be slow

Technical explanations

2) This point is adressed by having 'currentDirectory' given along to 'handleDndStopMedia' in media_directories_ui.media.js to be used as a check in 'dnd_stop.vakata' event.

3) For this point, I wanted to reuse at most the jstree dnd plugin indicator. Therefore I added a 'dnd_move.vakata' event as per what's done in jsTree.
Technical notes:
- I added a tag with class jstree-icon as per what's done for folder by jstree itself
- I added 'dnd_move.vakata' event to set this indicator
- Because we reuse the jstree indicator, this one is ALWAYS set to false unless moving a folder to an appropriate destination. We add to make sure our event runs AFTER jstree one therefore. To do so, I add to extract the global event binding from tree() method and bind it outside Drupal behavior, to make sure it binds after jstree.js code and thus runs after. Without it, the green tick is always destroyed by jstree.js since media is not a folder during d&d event.

Incorrect move

Correct move

4) I did so by reusing startLoader() and stopLoader() methods

Lock during operation

----
Attached is a patch that adresses thoses points.

Merci beaucoup otre fois - Dom. :D

You really leaned in here.

I reviewed and tested and am really happy with the solution we have now.

I changed to minor things;
the freaking new line at the end of the JS files, my IDE also makes this by default,
but I once applied to core coding standards, which does not have them ..
and a fix, that the "Clear selection" link is not shown when the searchTerm is null.

I have been working with this patch on prod for a few days without further notice.
Do you need any more work on this issue I could help with ? Otherwise RTBC, if you can commit it, that would allow me to work on
#3178219: Support file field paths module (filefield_paths) as per your feedbacks.

Also note, I could help probably as a co-maintainer if you need one.

We would be definitely be happy to have you as a co-maintainer. Let's work in the current manner for another 2 months, and then have a chat with Rang501 (who actually added me as co-maintainer ;)), to level you up. Is this fine for you?