Arvados

After some research, it looks like the package we use for uploading files in this form, react-dropzone, does not support folder uploading. The package https://www.npmjs.com/package/@rpldy/uploady looks like it will suit our needs.

19378-folder-upload @ a0bcc6910900b8f80659923bd105cce6e833c562

developer-run-tests-services-workbench2: #1542

• ✅ All agreed upon points are implemented / addressed.
• ✅ Anything not implemented (discovered or discussed during work) has a follow-up story.
• ✅ Code is tested and passing, both automated and manual, what manual testing was done is described
• ✅ Documentation has been updated.
• ✅ Behaves appropriately at the intended scale (describe intended scale).
  • creating multiple nested dirs is a bit slow, but the slowness is all in the network requests, so without a new endpoint to create multiple dirs at once, the slowness is what it is
• ✅ Considered backwards and forwards compatibility issues between client and server.
• ✅ Follows our coding standards and GUI style guidelines.

• It turns out that while Uploady would meet our needs, using it would require us to rewrite a substantial part of the upload interface to get it to work with Redux Form. Writing the components from scratch was the simplest solution, so that's what I did, which also removes our dependency on react-dropzone.
• There are now two clickable fields within the drag-and-drop field, because browsers do not support a single element that can open a combined folder/file selector.
• The drag-and drop field itself now supports both file and folder drops, except on certain firefox versions that do not support it (such as Mozilla Firefox Flatpak v140.0.2 (64-bit)).
• Folder drops retain nested file structure
• Cypress does not support uploading folders, so the best test I could make is to simulate creating nested dirs without any content

So I did some more investigation and discovered that:

• Like i suspected, you can't replace_files a parent folder with the empty collection uuid to create a directory followed by a subdirectory in the same request, but since you can directly replace_files a nonexistant nested path with the empty collection uuid, this doesn't really matter - this allows creating all the directories in a single request as long as you only give it terminal paths
• You can use preserve_version=false to avoid versioning the directory changes, so that the following file upload results in a single version change with the new directories

So I have these recommendations:

• I noticed that getMinNecessaryPaths doesn't actually filter out parent folder paths, since uniquePaths.some((existing) => path === existing will always be true because path is an anumeration of uniquePaths so path === existing will always be true for at least 1 path.
  • Instead I had success with:

    const minimalPaths = uniquePaths.filter((path) =>
        // false to filter out parent dirs
        // true for unique dirs
        uniquePaths.every((existing) => // If this is parent to any other dir (hence every instead of some), return false
            path === existing || !existing.startsWith(path + '/') //if equal, pass check
            // if startswith, eject with false
        )
    );

        const pathsToCreate = minNecessaryPaths.filter((path) => !existingDirPaths.has(path));
        if (pathsToCreate.length) {
            try {
                await this.createDirectory(collectionUuid, pathsToCreate, showErrors, preserveVersion);
            } catch (error) {
                console.error("Error creating directory", collectionUuid, error);
            }
        }

• I would add a preserveVersion = true to replaceFiles and createDirectory (in case we use that elsewhere and want it to save versions by default) and pass false from createMinNecessaryDirs or add another param there and pass false from the caller.

With this it should only tick up the version once, and only make a single call to create dirs

• I think it would be good to add a few unit tests to getMinNecessaryPaths

19378-folder-upload @ b73816a4dc649c7c79ba07636c530e9b6a17d1e3

developer-run-tests-services-workbench2: #1543

These suggestions were all implemented.

Also addressed is the bug where dropping a file + a folder would only upload the file, it now works as expected, uploading everything.

There is now a line in the form to indicate that empty folders will not be uploaded, which is a limitation imposed by the browser. If we want to support creating empty folders in the future, it would be trivial, but it's not likely that we will.

This looks great! lgtm!

19378-test-fix @ 8c77cf55162641f6583c030f1e7b38a92f121223

developer-run-tests-services-workbench2: #1544

One of the tests that was updated in #22793 got overwritten in the merge with main, now fixed.

Lisa Knox wrote in #note-48:

developer-run-tests-services-workbench2: #1544

Please run all of developer-run-tests. This job does not run the Workbench integration tests, which was a contributing factor to being in this situation in the first place.

One of the tests that was updated in #22793 got overwritten in the merge with main, now fixed.

I'm not sure I follow the history here. If I run git log -p origin/19378-test-fix -- services/workbench2/cypress/e2e/collection.cy.js, the three most recent commits are:

• 96569b43db932face7e73eb1b05ff04d3159d504 adds the completely new test, with the code that fails
• ef636fbbdeb49eecfaee61b659261ece7985d689 merges the branch to main, apparently without changes to this specific file
• 8c77cf55162641f6583c030f1e7b38a92f121223 changes the new test's code

When I hear "overwritten in the merge to main," what I imagine is that there was a merge conflict at some point and it got resolved incorrectly. In a case like that, I would expect to see a diff in a merge commit that shows the bad resolution. There's no diff like that here, so it seems like I'm misunderstanding what you mean. At what point in the development process did the code in your fix commit get overwritten?

Discussed at standup:

• I misunderstood the job, as shown in the console output it does run integration tests as well.
• This is basically a classic "conflict between parallel branches" development problem. One factor that makes this one feel avoidable is the fact that the conflicting branch #22793 got merged 14 days prior. We will add a review checklist item about this.

This is good to merge, thanks.