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

Use Webdav PUT for uploads #1283

Merged
merged 10 commits into from
Oct 25, 2016
Merged

Use Webdav PUT for uploads #1283

merged 10 commits into from
Oct 25, 2016

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Sep 6, 2016

See owncloud/core#21237

Please test carefully:

  • Test upload via button
  • Test upload drag/drop
  • Test upload link share
  • Test files_drop

CC: @nickvergessen @MorrisJobke @LukasReschke @blizzz @icewind1991 @schiessle

@rullzer rullzer added this to the Nextcloud 11.0 milestone Sep 6, 2016
@nickvergessen
Copy link
Member

  • Test activities which broke last time we tried this

@icewind1991
Copy link
Member

Ok, the name of the PR is misleading since it's implementing webdav POST, not using webdav PUT

From what I understand from reading the spec this is not actually implementing RFC5995 as it says it is, but instead doing something that somewhat looks like the spec on the surface. according to the RFC every collection has a (possibly) separate endpoint where it allows POST requests to create new files in the collection, this endpoint url is static for the collection and does not contain any filename suggestion for the created file, if the client wants to suggest a filename it can use the Slug header.

As it's currently implemented it uses a "magic path" /path/to/folder/&suggested_file_name instead of having a per-collection endpoint for adding files to a collection

Imo it would be better if we properly follow the rfc

@rullzer
Copy link
Member Author

rullzer commented Sep 8, 2016

@icewind1991 so close this and you fix it according to the RFC ;)?

@LukasReschke
Copy link
Member

@icewind1991 I'd either like to have this fixed or merged. 🙈

@LukasReschke
Copy link
Member

Blocks #1511

@blizzz
Copy link
Member

blizzz commented Sep 29, 2016

Needs rebase

@rullzer rullzer assigned rullzer and icewind1991 and unassigned rullzer Oct 2, 2016
@MorrisJobke
Copy link
Member

@icewind1991 @rullzer How to proceed here? stalled PRs doesn't help neither.

@nickvergessen nickvergessen added 1. to develop Accepted and waiting to be taken care of and removed 3. to review Waiting for reviews labels Oct 6, 2016
@LukasReschke
Copy link
Member

Rebase and just merge and enhance latter? Keeps the diff smaller 🙈

@LukasReschke
Copy link
Member

Will rebase.

@icewind1991
Copy link
Member

I'm fine with this as long as it has some documentation about how the magic path works (and doesn't suggest it's RFC5995)

@MorrisJobke MorrisJobke force-pushed the us_files-ui-webdav-upload branch from 95e3183 to 34a61bd Compare October 20, 2016 14:55
@MorrisJobke
Copy link
Member

I rebased this and want to get this in, because it blocks soo many other downstream PRs.

@MorrisJobke
Copy link
Member

MorrisJobke commented Oct 20, 2016

  • fix anonymous upload feature (files/ajax/upload.php doesn't exist anymore)

@rullzer rullzer force-pushed the us_files-ui-webdav-upload branch from 34a61bd to 2724b60 Compare October 22, 2016 20:14
@rullzer
Copy link
Member Author

rullzer commented Oct 23, 2016

Files_drop is kind of working again.

@icewind1991 do you agree with 35f9d3c Else it is not possible to upload via webdav with only create permissions since rename is not allowed.

@nickvergessen
Copy link
Member

Some tests are failing

@rullzer rullzer force-pushed the us_files-ui-webdav-upload branch from 2724b60 to 76c133c Compare October 24, 2016 13:43
Vincent Petry added 3 commits October 24, 2016 21:45
- uses PUT method with jquery.fileupload for regular and public file
  lists
- for IE and browsers that don't support it, use POST with iframe
  transport
- implemented Sabre plugin to handle iframe transport and redirect the
  embedded PUT request to the proper handler
- added RFC5995 POST to file collection with "add-member" property to
  make it possible to auto-rename conflicting file names
- remove obsolete ajax/upload.php and obsolete ajax routes

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Not needed any more in IE >= 11

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Vincent Petry and others added 7 commits October 24, 2016 21:45
Hacked around Blueimp's jquery.fileupload to make it work with our new
chunking API.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
If we move a file from the temp part file to the original file we don't
need update permissions.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer rullzer force-pushed the us_files-ui-webdav-upload branch from 76c133c to c8a13f6 Compare October 24, 2016 19:45
@codecov-io
Copy link

Current coverage is 57.35% (diff: 36.36%)

Merging #1283 into master will increase coverage by 0.07%

@@             master      #1283   diff @@
==========================================
  Files          1075       1075          
  Lines         61319      61975   +656   
  Methods        6851       6960   +109   
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          35124      35546   +422   
- Misses        26195      26429   +234   
  Partials          0          0          

Sunburst

Diff Coverage File Path
0% apps/dav/appinfo/v1/publicwebdav.php
0% apps/files/templates/list.php
0% new apps/dav/lib/Files/Sharing/FilesDropPlugin.php
•••••••• 80% ...ib/private/Files/Storage/Wrapper/PermissionsMask.php
•••••••• 88% apps/dav/lib/Connector/Sabre/FilesPlugin.php
•••••••••• 100% apps/files/appinfo/routes.php

Powered by Codecov. Last update 8a231a4...c8a13f6

@rullzer rullzer added 3. to review Waiting for reviews and removed 1. to develop Accepted and waiting to be taken care of labels Oct 25, 2016
@rullzer
Copy link
Member Author

rullzer commented Oct 25, 2016

Ok files_drop works here.
Lets get this in and fiddle with the details later.

CC: @MorrisJobke @icewind1991 @nickvergessen @LukasReschke @blizzz

@MorrisJobke
Copy link
Member

Works 👍

Tested in IE, Safari and Chrome

@MorrisJobke
Copy link
Member

@LukasReschke @nickvergessen we should get this in as fast as possible - other downstreams are depending on this

@LukasReschke
Copy link
Member

LGTM

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

Successfully merging this pull request may close these issues.

7 participants