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

Upload-only link shares #27548

Merged
merged 18 commits into from May 12, 2017

Conversation

Projects
None yet
9 participants
@PVince81
Member

PVince81 commented Mar 31, 2017

Fixes #7747.

Checklist here: #7747 (comment)

There's more to come, we can't merge this as is because backend support is required.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 31, 2017

Member
  • BUG: how to solve conflicts ? => deduplicate name on server side

  • TODO: JS unit tests for the new view

  • TODO: beautify the view (cc @felixheidecke)

Member

PVince81 commented Mar 31, 2017

  • BUG: how to solve conflicts ? => deduplicate name on server side

  • TODO: JS unit tests for the new view

  • TODO: beautify the view (cc @felixheidecke)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 31, 2017

Member

This is using the file-upload.js code so when this #26306 (comment) is fixed to work with chunking, the button from anonymous upload should also be able to work with chunking.

@DeepDiver1975 @tomneedham @IljaN @butonic check this out

Member

PVince81 commented Mar 31, 2017

This is using the file-upload.js code so when this #26306 (comment) is fixed to work with chunking, the button from anonymous upload should also be able to work with chunking.

@DeepDiver1975 @tomneedham @IljaN @butonic check this out

@PVince81 PVince81 changed the title from [WIP] Add checkbox to remove listing permission in public links to [WIP] Upload-only link shares Mar 31, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 31, 2017

Member
  • TODO: error handling, properly display error message
    • conflict
    • folder gone / whatever
    • insufficient space
Member

PVince81 commented Mar 31, 2017

  • TODO: error handling, properly display error message
    • conflict
    • folder gone / whatever
    • insufficient space
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 3, 2017

Member

@pmaier1 I think having it called "Allow file listing" and set by default is better.

This is how it looks on this PR:

files_drop_core

If you disable "Allow uploads" the option "Allow file listing" becomes greyed out and checked.

Member

PVince81 commented May 3, 2017

@pmaier1 I think having it called "Allow file listing" and set by default is better.

This is how it looks on this PR:

files_drop_core

If you disable "Allow uploads" the option "Allow file listing" becomes greyed out and checked.

@pmaier1

This comment has been minimized.

Show comment
Hide comment
@pmaier1

pmaier1 May 3, 2017

Contributor

Ok nice so far. I don't like the wording "Allow viewing file listing" :) "Hide file listing" would be perfect IMO.
As you would need to change your logic probably call it "Show file listing".

Contributor

pmaier1 commented May 3, 2017

Ok nice so far. I don't like the wording "Allow viewing file listing" :) "Hide file listing" would be perfect IMO.
As you would need to change your logic probably call it "Show file listing".

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 3, 2017

Member

The part I dislike about "Hide file listing" is mostly because you need to think positive for "Allow uploads" and then negative "Hide" instead of "Show".

We could also rename "Allow uploads" to "Disallow uploads".

@felixheidecke any feedback on the wordings ?

Member

PVince81 commented May 3, 2017

The part I dislike about "Hide file listing" is mostly because you need to think positive for "Allow uploads" and then negative "Hide" instead of "Show".

We could also rename "Allow uploads" to "Disallow uploads".

@felixheidecke any feedback on the wordings ?

@pmaier1

This comment has been minimized.

Show comment
Hide comment
@pmaier1

pmaier1 May 3, 2017

Contributor

I leave this up to you. 'Allow' and 'Show' or 'Disallow' and 'Hide' is both fine for me while I like the first pair a bit more :)

Contributor

pmaier1 commented May 3, 2017

I leave this up to you. 'Allow' and 'Show' or 'Disallow' and 'Hide' is both fine for me while I like the first pair a bit more :)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 3, 2017

Member
  • TODO: add capability entry
Member

PVince81 commented May 3, 2017

  • TODO: add capability entry
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 5, 2017

Member
  • Change "Allow upload" to "Allow editing"
Member

PVince81 commented May 5, 2017

  • Change "Allow upload" to "Allow editing"
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 5, 2017

Member
  • "Show file listing"
Member

PVince81 commented May 5, 2017

  • "Show file listing"
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

Added integration tests.

One covers the file name deduplication and will fail currently as I still need to implement this.

Member

PVince81 commented May 10, 2017

Added integration tests.

One covers the file name deduplication and will fail currently as I still need to implement this.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member
  • IMPORTANT: prevent downloading when manually appending "/download" in the URL
Member

PVince81 commented May 10, 2017

  • IMPORTANT: prevent downloading when manually appending "/download" in the URL
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

Capability added "supports_upload_only"

Member

PVince81 commented May 10, 2017

Capability added "supports_upload_only"

@PVince81 PVince81 changed the title from [WIP] Upload-only link shares to Upload-only link shares May 10, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 10, 2017

Member

Found a relatively clean way to deduplicate names by overriding the PUT method, but only when a header "X-OC-Autorename" is set.

@DeepDiver1975 can you have a look and tell me whether the approach is acceptable ? It's all in c47b09b

Member

PVince81 commented May 10, 2017

Found a relatively clean way to deduplicate names by overriding the PUT method, but only when a header "X-OC-Autorename" is set.

@DeepDiver1975 can you have a look and tell me whether the approach is acceptable ? It's all in c47b09b

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member

As discussed with @DeepDiver1975 in a chat, the approach is acceptable.
One change:

  • rename "X-OC-Autorename" to "OC-Autorename"
Member

PVince81 commented May 11, 2017

As discussed with @DeepDiver1975 in a chat, the approach is acceptable.
One change:

  • rename "X-OC-Autorename" to "OC-Autorename"
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member

Added JS unit tests.

Next up: beautify and proper error messages

Member

PVince81 commented May 11, 2017

Added JS unit tests.

Next up: beautify and proper error messages

@tomneedham

This comment has been minimized.

Show comment
Hide comment
@tomneedham
Member

tomneedham commented May 11, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member

Uh oh, seems the push of JS unit tests from the other computer didn't work. Note to self: no rebase!

Member

PVince81 commented May 11, 2017

Uh oh, seems the push of JS unit tests from the other computer didn't work. Note to self: no rebase!

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member
  • cherry pick commits from my other computer/branch which weren't pushed properly (OC-Autorename + JS unit tests)
Member

PVince81 commented May 11, 2017

  • cherry pick commits from my other computer/branch which weren't pushed properly (OC-Autorename + JS unit tests)
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member
  • TEST: drag and drop upload
Member

PVince81 commented May 11, 2017

  • TEST: drag and drop upload
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 11, 2017

Member
  • design adjustments:
    • visible dropzone with text "Select files for upload"
      • clicking dropzone opens file picker
      • dropping on dropzone uploads files directly
Member

PVince81 commented May 11, 2017

  • design adjustments:
    • visible dropzone with text "Select files for upload"
      • clicking dropzone opens file picker
      • dropping on dropzone uploads files directly
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81
Member

PVince81 commented May 11, 2017

files-drop

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| path | FOLDER |
| shareType | 3 |
| permissions | 4 |
And User "user0" deletes file "/FOLDER"

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

This should be 'When'.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

This should be 'When'.

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| shareType | 3 |
| permissions | 4 |
And User "user0" deletes file "/FOLDER"
Then publicly uploading a file does not work

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Too much indentation here.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Too much indentation here.

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| shareType | 3 |
| password | publicpw |
| permissions | 4 |
And publicly uploading file "test.txt" with password "publicpw" and content "test"

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Indentation from here to the end of the scenario is not aligned.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Indentation from here to the end of the scenario is not aligned.

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| path | FOLDER |
| shareType | 3 |
| permissions | 4 |
And publicly uploading file "test.txt" with content "test"

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Indentation from here to the end of the scenario is not aligned.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Indentation from here to the end of the scenario is not aligned.

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| path | FOLDER |
| shareType | 3 |
| permissions | 4 |
And publicly uploading file "test.txt" with content "test"

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Indentation from here to the end of the scenario is not aligned.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Indentation from here to the end of the scenario is not aligned.

PVince81 added some commits Mar 31, 2017

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| file_parent | A_NUMBER |
| share_with_displayname | user1 |
| displayname_owner | user0 |
| mimetype | text/plain |

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Too separated.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Too separated.

This comment has been minimized.

@PVince81

PVince81 May 12, 2017

Member

I did a search and replace of two spaces " " with tabs. Apparently for some reason there were two spaces there. I'll adjust it.

@PVince81

PVince81 May 12, 2017

Member

I did a search and replace of two spaces " " with tabs. Apparently for some reason there were two spaces there. I'll adjust it.

Show outdated Hide outdated tests/integration/features/sharing-v1.feature
| storage_id | home::user0 |
| file_parent | A_NUMBER |
| displayname_owner | user0 |
| mimetype | text/plain |

This comment has been minimized.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Too separated.

@SergioBertolinSG

SergioBertolinSG May 12, 2017

Member

Too separated.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

Rebased and fixes review comments

Member

PVince81 commented May 12, 2017

Rebased and fixes review comments

PVince81 added some commits May 10, 2017

Add Sabre Autorename plugin for public upload
Introduces a new header X-OC-Autorename that tells the autorename plugin
to automatically rename a file if the target already exists.

Currently only used and applied for upload-only shares.
Throw NotFound exception when link share source not found
Plus some bonus integration tests
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

Going to pre-review the diff and fix missing stuff.

Member

PVince81 commented May 12, 2017

Going to pre-review the diff and fix missing stuff.

PVince81 added some commits May 11, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

Ok, fixed everything I found when looking at the diff.

Please review! See whitespace-less diff: https://github.com/owncloud/core/pull/27548/files?w=1

@pmaier1 @DeepDiver1975 @tomneedham @felixheidecke

Member

PVince81 commented May 12, 2017

Ok, fixed everything I found when looking at the diff.

Please review! See whitespace-less diff: https://github.com/owncloud/core/pull/27548/files?w=1

@pmaier1 @DeepDiver1975 @tomneedham @felixheidecke

Show outdated Hide outdated apps/files_sharing/js/PublicUploadView.js
render: function () {
this.$el.html(this.template({
title: t('files_sharing', 'Anonymous upload'),
uploadButtonLabel: t('files_sharing', 'Select files for upload'),

This comment has been minimized.

@felixheidecke

felixheidecke May 12, 2017

Contributor

I suggest "Drop files here for upload" to hint to the drag & drop ability.

@felixheidecke

felixheidecke May 12, 2017

Contributor

I suggest "Drop files here for upload" to hint to the drag & drop ability.

This comment has been minimized.

@pmaier1

pmaier1 May 12, 2017

Contributor

"Click here to select files or use drag & drop to upload" as you can also use the file picker I assume.

@pmaier1

pmaier1 May 12, 2017

Contributor

"Click here to select files or use drag & drop to upload" as you can also use the file picker I assume.

This comment has been minimized.

@PVince81

PVince81 May 12, 2017

Member

I'll wait for you guys to settle on a solution, let me know 😄

@PVince81

PVince81 May 12, 2017

Member

I'll wait for you guys to settle on a solution, let me know 😄

This comment has been minimized.

@pmaier1

pmaier1 May 12, 2017

Contributor

My final call: "Click to select files or use drag & drop to upload"

@pmaier1

pmaier1 May 12, 2017

Contributor

My final call: "Click to select files or use drag & drop to upload"

@DeepDiver1975 DeepDiver1975 self-requested a review May 12, 2017

@DeepDiver1975

NICE

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

I'll go with @pmaier1's text for now and we can merge.

If it's not optimal we can change it in a subsequent PR. This will make it possible to already get this tested...

Member

PVince81 commented May 12, 2017

I'll go with @pmaier1's text for now and we can merge.

If it's not optimal we can change it in a subsequent PR. This will make it possible to already get this tested...

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 12, 2017

Member

Tests passed on previous commit and the text is not covered by unit tests => merge

Member

PVince81 commented May 12, 2017

Tests passed on previous commit and the text is not covered by unit tests => merge

@PVince81 PVince81 merged commit 4522d2a into master May 12, 2017

1 of 3 checks passed

Scrutinizer Installing Code
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the anon-upload branch May 12, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 15, 2017

Member

Tests passed on previous commit and the text is not covered by unit tests => merge

Never trust the green ticks, Jenkins wasn't present, there's a failure on master now 💥

Member

PVince81 commented May 15, 2017

Tests passed on previous commit and the text is not covered by unit tests => merge

Never trust the green ticks, Jenkins wasn't present, there's a failure on master now 💥

@butonic

This comment has been minimized.

Show comment
Hide comment
@butonic

butonic May 22, 2017

Member

@PVince81 Is there a migration path when upgrading from an oc 9.x with files_drop app?

Member

butonic commented May 22, 2017

@PVince81 Is there a migration path when upgrading from an oc 9.x with files_drop app?

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 May 22, 2017

Member

@butonic no migration path possible for technical reasons (route name is different).

It was decided to say in the release notes that the shares must be recreated.

cc @pmaier1

Member

PVince81 commented May 22, 2017

@butonic no migration path possible for technical reasons (route name is different).

It was decided to say in the release notes that the shares must be recreated.

cc @pmaier1

@ckamm

This comment has been minimized.

Show comment
Hide comment
@ckamm

ckamm Jun 15, 2017

Member

@PVince81 I'm adding the relevant functionality to the client. To clarify: Previously the client checked the "Allow editing" box if create and update permission were present. This seems to have changed? To get matching behavior I must only check for create permission.

Does the update permission still toggle anything ui wise?

EDIT: Nevermind, I read the code and see that there are three states now: READ or CREATE or READ+CREATE+UPDATE+DELETE.

Member

ckamm commented Jun 15, 2017

@PVince81 I'm adding the relevant functionality to the client. To clarify: Previously the client checked the "Allow editing" box if create and update permission were present. This seems to have changed? To get matching behavior I must only check for create permission.

Does the update permission still toggle anything ui wise?

EDIT: Nevermind, I read the code and see that there are three states now: READ or CREATE or READ+CREATE+UPDATE+DELETE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment