Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flatten folder structure from drag & drop for public file upload #40643

Merged
merged 5 commits into from
Feb 25, 2023

Conversation

jnweiger
Copy link
Contributor

@jnweiger jnweiger commented Feb 17, 2023

This allows drag & drop to work with folders too. The code change is minimal, as all the code top pick up files from subdirectories is already there. It only missed the step to actually remove subdirecory levels from the path before uploading.

Description

Add missing piece for folder drag & drop with public file drop.
Drag & drop uses javascript code to generate a list of files.
With one file or multiple files selected, this is a list of filenames and disregards the folders where they came from.
With one ore multiple folders selected, the list of filenames includes relative pathnames, the selected folder and potentially subdorectories within that folder.

In case of normal file view the uploader implements creating of the subdirectory levels so that the structure of the selected tree is faithfully reproduced on the server.
In case of a public file drop, subdirectories cannot be created. And as agreed in https://github.com/owncloud/enterprise/issues/5489#issuecomment-1430908616 it is also not wanted.

File Drop as implemented in PublicFileView.js already uses the same mechanics as normal drag drop fir files view, so 99% of the code for folder drop is already present. The missing piece is the subfolder problem.
It is actually sufficient, to remove the subfolders (as stored in file.relativePath) and the upload succeeds.

Related Issue

Motivation and Context

Folder upload never worked on Public file Drop.

How Has This Been Tested?

Manual testing with files, folders, multiple files, multiple folders selected in drag and drop;
both with public file drop view and normal file view.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

by implementing flattening of folder structure, as agreed.
@update-docs
Copy link

update-docs bot commented Feb 17, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@IljaN IljaN self-requested a review February 20, 2023 09:31
@mmattel mmattel requested a review from jvillafanez February 24, 2023 10:25
@mmattel
Copy link
Contributor

mmattel commented Feb 24, 2023

@phil-davis we possibly need a check if the test is sufficient and a changelog, could you or someone of your team assist? 😄

@phil-davis
Copy link
Contributor

@phil-davis we possibly need a check if the test is sufficient and a changelog, could you or someone of your team assist? smile

We can't do drag-and-drop in automated tests, because the browser-test functionality can't "get outside the browser" to do that sort of thing. So we need to manually test this sort of thing.

The change is going to work. My question is "is this behavior the required/acceptable behavior?" "public" users will drop a folder into a public upload-only link browser window, and the whole folder structure will be flattened into the public-link folder. If it is just a few files then I suppose that will be OK for the real user that made the public upload-only link. But when someone drops a folder that has a lot of files in it, the result will be difficult for the real user to realize what happened. And specially confusing if multiple "public users" do this.

It needs someone (Product Manager?) to confirm that this solution is acceptable.

@jnweiger
Copy link
Contributor Author

It is the agreed solution. Customer expectation is already set in this way.

Exploiting a sanity check in the uploader is not a fine way to stop an upload, but the API of the beforeadd callback is rather limited.
Tested with single empty folder, multiple empty folders, empty folder in subfolder and deep hierarchies of hundreds of folders and files.
Provide test dummy for setTargetFolder()
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@phil-davis
Copy link
Contributor

Works as designed. When I drag-and-drop a folder that has various files and sub-folders then all the files throughout the tree are uploaded "flat" into the the "file drop" folder on the server. If I do different uploads and file names are duplicated, then the 2nd files with the same name all get "(2)" on the end of their names.

@phil-davis phil-davis merged commit 2b2476c into master Feb 25, 2023
@delete-merged-branch delete-merged-branch bot deleted the jw-flatten-public-upload-e5489 branch February 25, 2023 02:08
@phil-davis
Copy link
Contributor

Note: when you "click to upload" and the file explorer window opens for you to find the file to upload, when you click folders to navigate, they open, just like they did before. You can't upload a whole folder using "click to upload", which is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants