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.

Sign up for GitHub

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

Jump to bottom

Feature/tus upload #3345

Merged
merged 1 commit into from
Apr 24, 2020
Merged

Feature/tus upload #3345

merged 1 commit into from
Apr 24, 2020

Conversation

LukasHirt
Copy link
Collaborator

@LukasHirt LukasHirt commented Apr 16, 2020 •
edited
Loading

Description

Use tus-js-client to upload files

@LukasHirt LukasHirt self-assigned this Apr 16, 2020
@update-docs
Copy link

update-docs bot commented Apr 16, 2020

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.

PVince81
PVince81 reviewed Apr 16, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Good start!

src/plugins/upload.js Outdated
methods: {
uploadChunkedFile(file) {
const upload = new tus.Upload(file, {
endpoint: this.$client.helpers._webdavUrl,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll need to extract the path from the file name and append it to the endpoint.

as far as I understand the metadata should only contain the basename of the file name, not the full path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I'll adjust it 😉

src/plugins/upload.js Outdated
},

onError: error => {
console.error(error)
Copy link
Contributor

Choose a reason for hiding this comment

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

reject the promise here ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was also thinking about this but I want to test how does it behave when retrying the upload. AFAICT this will be triggered every time when the upload fails and rejecting it before retry feels a bit odd to me.

@PVince81
Copy link
Contributor

Do all code paths lead to this FileUpload location ? I guess we might need to detect public page mode and fall back to regular upload there

@PVince81
Copy link
Contributor

For #67

@PVince81 PVince81 mentioned this pull request Apr 16, 2020
Closed
@PVince81
Copy link
Contributor

we should be able to test this with cs3org/reva#661 once ready

@PVince81 PVince81 mentioned this pull request Apr 16, 2020
Closed
6 tasks
@LukasHirt
Copy link
Collaborator Author

Do all code paths lead to this FileUpload location ? I guess we might need to detect public page mode and fall back to regular upload there

This is only for file upload. Folder and d&d are not part of this. I put it only here for now for test purpose. When I'll be able to test it and will see it will work I'll move it to the others and disable it for public links.

@PVince81
Copy link
Contributor

@LukasHirt please rebase at the next opportunity so we get yarn watch-all-ocis for future testing :-)

@LukasHirt
Copy link
Collaborator Author

please rebase at the next opportunity so we get yarn watch-all-ocis for future testing :-)

Done

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9426/

20200417-094118-999.png
20200417-094209-088.png
20200417-094259-858.png
20200417-094348-487.png
20200417-094440-296.png
20200417-094531-682.png
20200417-094559-309.png
20200417-094624-823.png
20200417-094652-262.png
20200417-094721-674.png
20200417-094815-854.png
20200417-094927-589.png
20200417-095038-625.png
20200417-095127-891.png
20200417-095241-061.png
20200417-095352-886.png
20200417-095446-810.png
20200417-095537-341.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9427/

20200417-101959-387.png
20200417-102048-031.png
20200417-102139-338.png
20200417-102230-256.png
20200417-102323-216.png
20200417-102412-462.png
20200417-102438-491.png
20200417-102504-683.png
20200417-102532-481.png
20200417-102600-491.png
20200417-102709-883.png
20200417-102759-880.png
20200417-102849-661.png
20200417-102958-503.png
20200417-103050-269.png
20200417-103200-756.png
20200417-103230-090.png
20200417-103321-894.png

@PVince81
Copy link
Contributor

PVince81 commented Apr 17, 2020 •
edited
Loading

  • clean up, move the code to some proper place
  • handle success case
  • handle progress
  • detect if backend is TUS-capable else fallback
  • improve error handling

@PVince81
Copy link
Contributor

PVince81 commented Apr 17, 2020 •
edited
Loading

  • file overwrite dialog not popping up
  • what happens when uploading into a finished file when said file not visible in the list ? (bypassing the dialog, expecting TUS lib to fail but graceful fallback to upload again) => check conflict handling with 409 => let's address this with Dubious overwrite logic with etag / server side conflict handling #3262 and fixing conflict handling

@PVince81
Copy link
Contributor

PVince81 commented Apr 17, 2020 •
edited
Loading

  • remove double slash in endpoint when a folder is specified

@PVince81
Copy link
Contributor

PVince81 commented Apr 17, 2020 •
edited
Loading

@PVince81
Copy link
Contributor

PVince81 commented Apr 17, 2020 •
edited
Loading

  • check why tus.isSupported is undefined

@PVince81
Copy link
Contributor

PVince81 commented Apr 17, 2020 •
edited
Loading

  • optional to try: set uploadDataDuringCreation=true, hoping that the backend POST accepts it ;-)
    => the library marked it as experimental, let's not use it for now

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9430/

20200417-125906-233.png
20200417-125952-707.png
20200417-130040-027.png
20200417-130127-136.png
20200417-130214-510.png
20200417-130301-537.png
20200417-130325-901.png
20200417-130350-198.png
20200417-130415-569.png
20200417-130441-039.png
20200417-130528-042.png
20200417-130634-703.png
20200417-130741-414.png
20200417-130828-429.png
20200417-130938-896.png
20200417-131047-071.png
20200417-131133-449.png
20200417-131159-855.png

@PVince81
Copy link
Contributor

PVince81 commented Apr 20, 2020 •
edited
Loading

  • configurable chunk size ? how to expose it from server ? or client only ? => added to config.json

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9453/

20200420-102904-850.png
20200420-105015-772.png
20200420-105103-321.png
20200420-105151-010.png
20200420-105238-814.png
20200420-105327-571.png
20200420-105416-064.png
20200420-105425-965.png
20200420-105435-689.png
20200420-105446-006.png
20200420-105456-399.png
20200420-105603-960.png
20200420-105711-870.png
20200420-105800-526.png
20200420-105910-524.png
20200420-110001-143.png
20200420-110051-681.png
20200420-110145-673.png
20200420-110238-915.png

PVince81
PVince81 reviewed Apr 20, 2020
View reviewed changes
src/plugins/upload.js Outdated Show resolved Hide resolved
@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9456/

20200420-115755-440.png
20200420-115841-520.png
20200420-115927-946.png
20200420-120014-524.png
20200420-120101-475.png
20200420-120148-139.png
20200420-120156-690.png
20200420-120205-170.png
20200420-120214-012.png
20200420-120222-914.png
20200420-120329-175.png
20200420-120435-420.png
20200420-120541-797.png
20200420-120648-006.png
20200420-120736-190.png
20200420-120843-940.png
20200420-120936-883.png
20200420-121009-627.png

@PVince81
Copy link
Contributor

PVince81 commented Apr 20, 2020 •
edited
Loading

  • also pass in mtime as metadata

@PVince81
Copy link
Contributor

  • find out how long TUS client caches resumable entries in localStorage. might need to provide a way to cancel such uploads

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9457/

20200420-125615-305.png
20200420-125644-034.png
20200420-125712-795.png
20200420-125741-966.png
20200420-125812-500.png
20200420-125843-505.png
20200420-125856-855.png
20200420-125909-972.png
20200420-125924-643.png
20200420-125939-562.png
20200420-130030-216.png
20200420-130104-040.png
20200420-130135-228.png
20200420-130225-132.png
20200420-130317-741.png
20200420-130415-003.png
20200420-130506-355.png
20200420-130557-612.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9458/

20200420-130006-412.png
20200420-130033-635.png
20200420-130101-932.png
20200420-130130-637.png
20200420-130159-878.png
20200420-130229-084.png
20200420-130241-107.png
20200420-130253-581.png
20200420-130306-514.png
20200420-130319-007.png
20200420-130407-602.png
20200420-130456-773.png
20200420-130545-402.png
20200420-130634-459.png
20200420-130723-981.png
20200420-130811-066.png
20200420-130841-919.png
20200420-130913-134.png

@ownclouders
Copy link
Contributor

💥 Acceptance tests webUIOCIS failed. Please find the screenshots inside ...

https://drone.owncloud.com/owncloud/phoenix/9459/

20200420-132937-887.png
20200420-133002-580.png
20200420-133027-567.png
20200420-133052-863.png
20200420-133118-699.png
20200420-133144-813.png
20200420-133154-226.png
20200420-133203-216.png
20200420-133212-904.png
20200420-133222-288.png
20200420-133308-184.png
20200420-133354-154.png
20200420-133420-907.png
20200420-133506-770.png
20200420-133534-360.png
20200420-133621-022.png
20200420-133710-581.png
20200420-133740-788.png

@PVince81 PVince81 mentioned this pull request Apr 21, 2020
Closed
@PVince81
Copy link
Contributor

PVince81 commented Apr 21, 2020 •
edited
Loading

For implementing the TUS detection capability, the following tasks remain:

@PVince81
Copy link
Contributor

TUS capability is now working.

I had to fix chunked upload as we mistakenly included the file name in the endpoint URL.

Now requires the other PRs to be merged.

@butonic
Copy link
Member

butonic commented Apr 22, 2020

cool, I did not spot your butonic/reva#4 ... I created cs3org/reva#674 yesterday. Can you check if that includes butonic/reva#4

@PVince81
Copy link
Contributor

@butonic I tested now with cs3org/reva#674 and we're getting a CORS error on the PATCH request, missing the "Authorization" header. As I remember this should have been fixed by owncloud/ocis-reva#136 but doesn't work even though I am using the latest master from ocis-reva.

@PVince81
Copy link
Contributor

PVince81 commented Apr 22, 2020 •
edited
Loading

PVince81
PVince81 reviewed Apr 23, 2020
View reviewed changes
src/plugins/upload.js Outdated Show resolved Hide resolved
@PVince81
Copy link
Contributor

PVince81 commented Apr 23, 2020 •
edited
Loading

  • retest folder upload with TUS

@PVince81 PVince81 mentioned this pull request Apr 23, 2020
Merged
@PVince81
Copy link
Contributor

@LukasHirt I've added the following:

  • mtime is now passed as metadata, following the logic that we used in OC 10 (might have been for IE11 compat)
  • chunk size is configurable in config.json

I've squashed everything into a single commit.

I was able to test many of the checkboxes using the tus-dataprovider branch (and bypass the capability check) because tus-ocdav is currently broken, see cs3org/reva#674 (comment)

@PVince81 PVince81 marked this pull request as ready for review April 23, 2020 15:07
@LukasHirt
Add chunked upload with tus-js-client
b2db7ba 
Added chunked upload with tus-js-client if the server supports it.

Co-authored-by: Vincent Petry <pvince81@owncloud.com>
@PVince81
Copy link
Contributor

Tiny addition: added removeFingerprintOnSuccess option as recommended.
This means that now the library removes the entries (fingerprints) in local storage after an upload succeeded.
So it only keeps them for aborted uploads for later resuming.

@PVince81 PVince81 added Status:Needs-Review Needs review from a maintainer and removed Status:In-Progress labels Apr 24, 2020
PVince81
PVince81 approved these changes Apr 24, 2020
View reviewed changes
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 7184b9d into master Apr 24, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/tus-upload branch April 24, 2020 08:53
@PVince81
Copy link
Contributor

This has now introduced a little console annoyance when running against OC 10: owncloud/owncloud-sdk#462

@micbar micbar mentioned this pull request Apr 28, 2020
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

Assignees

@LukasHirt LukasHirt

Labels
Category:Enhancement Add new functionality Status:Needs-Review Needs review from a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LukasHirt @PVince81 @ownclouders @butonic