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

[full-ci] Switch upload logic to Uppy #6202

Merged
merged 23 commits into from
Apr 27, 2022
Merged

[full-ci] Switch upload logic to Uppy #6202

merged 23 commits into from
Apr 27, 2022

Conversation

pascalwengerter
Copy link
Contributor

@pascalwengerter pascalwengerter commented Dec 30, 2021

Description

We've implemented Uppy as a library for handling uploads. This concludes the following features and changes:

  • Resumable uploads when the backend supports the Tus-protocol
  • A nice looking overview for all files that have been uploaded successfully or failed to upload
  • Navigation across Web while uploads are in progress
  • Improved rendering of uploadProgress-visualization
  • Removed vue2-dropzone and vue-drag-drop libraries

Related Issue

Screenshots

image

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • add changelog item(s)
  • decide where to initialize/instantiate uppy -> currently not possibly globally due to our architecture.
  • make uppy work for OC10 (private&public context, upload-buttons&dropzone)
  • make uppy work for oCIS (private&public context, upload-buttons&dropzone)
  • make uppy create new folders (if none existing on the backend)
  • take care of UploadProgress-visualization
  • take care of Resume-button
  • make uppy work on FilesDrop.vue
  • take care of l10n
  • careful code cleanup
  • Fix Acceptance tests
  • fix unit tests
  • File conflict on same name

Open tasks/enhancements: #6819

@pascalwengerter pascalwengerter marked this pull request as draft December 30, 2021 08:27
@pascalwengerter pascalwengerter changed the base branch from master to simplify-appbar March 28, 2022 22:29
@pascalwengerter
Copy link
Contributor Author

I've rebased this PR on #6662 since simplifying the AppBar made the switch to Uppy way easier (we can&should change PR targets back to master once #6662 is merged, obviously). There's a couple of pitfalls and decisions to take still, but confident that the codebase & project profits even from merging a "simpler" version (e.g. without "Resume" button")

@ownclouders
Copy link
Contributor

ownclouders commented Mar 28, 2022

Results for oC10SharingPublic2 https://drone.owncloud.com/owncloud/web/24948/38/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L227.png

webUISharingPublicDifferentRoles-shareByPublicLinkDifferentRoles_feature-L227.png

@pascalwengerter pascalwengerter force-pushed the simplify-appbar branch 3 times, most recently from 49688aa to 30def9c Compare March 29, 2022 16:06
@delete-merged-branch delete-merged-branch bot deleted the branch master March 29, 2022 16:38
@pascalwengerter pascalwengerter changed the base branch from simplify-appbar to master March 29, 2022 17:11
@pascalwengerter pascalwengerter force-pushed the uppy-poc branch 4 times, most recently from c8e71ce to a866a98 Compare April 4, 2022 09:11
@JammingBen JammingBen force-pushed the uppy-poc branch 2 times, most recently from e27ed15 to d4143c1 Compare April 11, 2022 13:57
@JammingBen JammingBen changed the title Switch upload logic to Uppy [full-ci] Switch upload logic to Uppy Apr 13, 2022
Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳👏🎉

This is very nice!

It has a bunch of rough edges, we might want to move things around a bit still (and I would like to hear opinions of the others), but I feel we're heading in the right direction. I'm only worried that composables and the service interaction/relation feel slightly chaotic still... not sure yet, how we can improve that

Most of the comments I left are basically reminders for myself, so we can talk about it again.

Bare with my typos and rather short sentences, I am typing on my phone 😅

await this.createDirectoryTree(files)
await this.updateStoreForCreatedFolders(files)
this.$uppyService.uploadFiles(files)
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not so happy with this, it kind of feels the service should handle this - but we have so many dependencies here that are weird to get in a service... totally unsure about that.

At least we can move this function to the composable.

publicLinkPassword
}),
currentPath,
uploadPath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see good reasons for having currentPath and uploadPath computed in the composable vs having them passed in.... not sure

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good reason for it being passed to the composable is that those props might vary. The FileDrop component e.g. has a completely other upload path.

const storageId = params.storageId
const webDavPath = storageId
? buildWebDavSpacesPath(storageId, `${currentFolder}${directory}`)
: buildWebDavFilesPath(unref(user)?.id, `${currentFolder}${directory}`)
Copy link
Member

@dschmidt dschmidt Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering... what should happen if the unref'ed user is null or undefined? We might error out early as well, no?

packages/web-runtime/src/composables/upload/useUpload.ts Outdated Show resolved Hide resolved
packages/web-runtime/src/composables/upload/useUpload.ts Outdated Show resolved Hide resolved
packages/web-runtime/src/services/uppyService.ts Outdated Show resolved Hide resolved
packages/web-runtime/src/services/uppyService.ts Outdated Show resolved Hide resolved
import { UppyResource } from '../composables/upload'
import Vue from 'vue'

export class UppyService extends Vue {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used this as a quick hack, to port away from events on the $root instance. Do we have a better provider for the $on, $off and $emit functionality. EventBus maybe?

Copy link
Member

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay for now, but please check my remaining comments in the follow up ticket/PRs

@@ -249,6 +250,15 @@ export const announceClientService = ({
vue.prototype.$clientService.owncloudSdk = sdk
}

/**
* announce uppyService and owncloud SDK and inject it into vue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no owncloud SDK here :trollface:

@sonarcloud
Copy link

sonarcloud bot commented Apr 27, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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

20.8% 20.8% Coverage
0.0% 0.0% Duplication

@pascalwengerter pascalwengerter merged commit 2b91812 into master Apr 27, 2022
@delete-merged-branch delete-merged-branch bot deleted the uppy-poc branch April 27, 2022 17:40
@dschmidt
Copy link
Member

🎉 🥳 👏🏻 🥳 🎉

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.

Make file upload reliable
6 participants