Fix bug introduced on drag and drop external files

[pjft]

Drag and drop of external (OS filesystem) to subdirectories in the browser would fail on specific cases, mainly when the subdirectory was no longer off the root folder.
This seemed to have been an issue introduced with the subdirectory free space calculation here and it seems to fail for any subdirectory that doesn't belong to the root folder.

Bug reports:

• https://help.nextcloud.com/t/drag-drop-into-subfolders/120731
• Dragging and dropping files into the shared directory does not work unless we go into it. #24720

I couldn't find any reference on scenarios or quota management that would suggest when a subdirectory's free space would be different to the parent's free space, other than when on the root folder, where subdirectories can be external mounts.

As such, if my understanding is correct (please let me know if it isn't), this calculation can - and should - be made by getting the free space from the first subdirectory in the total path, which caters for all subdirectory scenarios.

Please advise, happy to help improve this.

[pjft]

Apologies, had made a mistake in my assessment. Actually the issue was that the subdirectory had "/" at the beginning, and we were unable to create a model for it. Getting the last subdirectory in the path without the slash works.
I updated the file.

[pjft]

@skjnldsv Thanks for the help - it works great , but from testing a new case now I find that the upload-to-subfolder code path doesn't quite work as intended. I'm happy to try and fix it, but would appreciate guidance.

Specifically, the current issue is that the upload code will check for conflicts against files in the currently open directory, so when uploading to a subfolder it risks not detecting the right conflicts if there are any - and it will also trigger a wrong conflict dialog if there are files with the same name in the current folder.

The current code does self.checkExistingFiles(selection, callbacks);. Ideally I'd be able to either call this with a comparable object from the subfolder, or I'd have to change the code in checkExistingFiles to compare against the correct fileList, if it's a subfolder.

EDIT 1: I am a bit at a loss as to how to go about it, though, but happy to look into it. The code has been around for 6-8 years depending on the areas, so I am no longer sure whether this check ever worked as intended in previous versions, though I recall being able to drag and drop to a subfolder in older versions, so perhaps it was happening the same, comparing to the wrong filelist.

Actually, disregard it all. Apparently the issue is just comparing with the current folder. The files do get properly compared against destination at some point, when I try to replace existing files there, just through another code path.

[pjft]

Updated checkExistingFiles with the same conditions to not execute conflict check at that stage, as it'd compare with the incorrect file set. Should be good now.

[szaimen]

Unfortunately jsunit is failing now, do you mind fixing that? :)

[pjft]

Definitely. It seems that my attempt at correcting the conflict check with the right folder is making a test fail. I thought that my commit earlier today had fixed it, but alas it didn't. I'm looking into it.

Apologies for the hassle.

[pjft]

So, I'd like some validation here as the way I fixed jsunit was to update the test - which is always awkward.
What I did was:

• The test was creating an upload with a blank targetDir. Technically, this is a different folder than root ("/") - which is effectively where the test is uploading things to - and that's why the test was now failing: we are no longer checking for conflicts with different folders than the one we are uploading to.
• I changed the test to set the targetDir to "/". My assumption for this change is that there is no user-driven interaction that should result in targetDir being blank. Please let me know if that is incorrect. Also, if there is, uploading to (blank path) should technically not be the same as uploading to ("/"). If it is, I suppose I can change the code to treat those the same, but it becomes less... elegant.
• I ran the test without my current changes, and it ran successfully (meaning that the original code passes the new test). I then made my change to avoid the wrong existingFilesCheck comparison, and the runs successfully.

Since I am not fully familiar with this part of the code, I'd appreciate it if someone more knowledgeable than I am to confirm whether this is an adequate change for the test, or if I unknowingly now changed the test in a way that it no longer suits its intended purpose.

Thank you.

[pjft]

Hi all,

Apologies for bothering once again.

Is there anything pending on my end on this PR? I'm happy to hear that there hasn't been time to look into it, but I'd like some guidance as it's not sure whether I'm the one who's expecting to do something right now on it - and if so, what.

Hope you're doing well, and keep up the great work!

[artonge]

Sorry for the delay here, can you address my comments? :)

[pjft]

@artonge definitely - thank you for taking the time. Let me know further thoughts - and definitely a preference on that code statement rewrite.

Have a great day! :)

[pjft]

Hi @artonge - apologies for bothering. Just received a notification from #29202 around this and came to check. It seems I'm still pending a change requested by you, and apparently another approver.

I'm not sure you can approve, but if you or anyone else could help me navigate this it'd be very much appreciated. Thank you all, and keep up the great work :)

[artonge]

Hi @artonge - apologies for bothering. Just received a notification from #29202 around this and came to check. It seems I'm still pending a change requested by you, and apparently another approver.

I'm not sure you can approve, but if you or anyone else could help me navigate this it'd be very much appreciated. Thank you all, and keep up the great work :)

Thanks for repinging and for the change @pjft ! And sorry for the delay in our review :)

[pjft]

Not at al - thank you for taking the time. :)

Regards.

[szaimen]

Backports?

[pjft]

Happy to - I run this exact change on 21. What's the right process for it?

[szaimen]

/backport to stable22

[szaimen]

/backport to stable21

[pjft]

Well, that was easy :D Thank you!