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

Renaming and adding new options for public link #34687

Merged
merged 2 commits into from
Apr 5, 2019

Conversation

sharidas
Copy link
Contributor

@sharidas sharidas commented Mar 5, 2019

  1. Renaming View/Download/Upload to View/Download/Edit.
  2. Adding new option View/Download/Upload.
    So if user wants to give provide all permissions,
    View/Download/Edit option could be opted. If the user
    wants to have an option where no edit and delete to be
    granted for link shared, the View/Download/Upload could
    be used.

Signed-off-by: Sujith H [email protected]

Description

This PR renames and adds an option in the public link share dialog. The View/Download/Upload is now renamed to View/Download/Edit. So if the user wants to create a link which would let others to create, edit, upload and delete in the public link then View/Download/Edit should be selected.
The new option View/Download/Upload has lesser permission. When a public link with this option is created, the user could create folder(s), no delete option is available, no edit possible. By no edit option, I mean no re-write of the existing files. Say for example if there is a file a.txt already available in the public link, and user tries to uplaod a file a.txt, the end result would be like this: the newly uploaded file would become a (2).txt. The original file a.txt would not be overwritten.

Related Issue

  • Fixes <issue_link>

Motivation and Context

  • Renaming View/Download/Upload to View/Download/Edit in the public link share creation dialog.
  • Created View/Download/Upload with minimal permission. Removed edit and delete option.

How Has This Been Tested?

  • Here is the screenshot
    granularPermission

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

  • Backport (if applicable set "backport-request" label and remove when the backport was done)

@sharidas sharidas added this to the development milestone Mar 5, 2019
@sharidas sharidas self-assigned this Mar 5, 2019
@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #34687 into master will decrease coverage by 27.1%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #34687       +/-   ##
=============================================
- Coverage     65.28%   38.17%   -27.11%     
=============================================
  Files          1210       48     -1162     
  Lines         69984     3282    -66702     
  Branches       1280        0     -1280     
=============================================
- Hits          45688     1253    -44435     
+ Misses        23924     2029    -21895     
+ Partials        372        0      -372
Flag Coverage Δ Complexity Δ
#javascript ? ?
#phpunit 38.17% <ø> (-28.52%) 0 <ø> (-18481)
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
apps/dav/lib/Upload/ChunkLocationProvider.php
apps/files/lib/AppInfo/Application.php
apps/systemtags/list.php
... and 1150 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecdaeb0...1127ef3. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #34687 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #34687      +/-   ##
============================================
- Coverage     65.38%   65.38%   -0.01%     
- Complexity    18592    18593       +1     
============================================
  Files          1213     1213              
  Lines         70408    70410       +2     
  Branches       1295     1295              
============================================
+ Hits          46036    46037       +1     
- Misses        23998    23999       +1     
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 52.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.83% <100%> (-0.01%) 18593 <0> (+1)
Impacted Files Coverage Δ Complexity Δ
apps/files_sharing/templates/public.php 0% <ø> (ø) 0 <0> (ø) ⬇️
core/js/sharedialoglinkshareview.js 83.46% <ø> (ø) 0 <0> (ø) ⬇️
...es_sharing/lib/Controller/Share20OcsController.php 85.84% <100%> (+0.02%) 203 <0> (+1) ⬆️
.../files_sharing/lib/Controllers/ShareController.php 54.58% <100%> (+0.2%) 54 <0> (ø) ⬇️
apps/files_trashbin/lib/Expiration.php 96.55% <0%> (-1.73%) 29% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e4b2f9...b9879c3. Read the comment docs.

@sharidas sharidas changed the title [WIP] Renaming and adding new options for public link Renaming and adding new options for public link Mar 8, 2019
@sharidas
Copy link
Contributor Author

Here is the snapshot of how this feature looks and behaves, kindly have a look at the screencast:
share

@sharidas sharidas force-pushed the public-link-upload-edit branch 2 times, most recently from c273808 to 18e4729 Compare March 12, 2019 11:32
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.

Looks fine in general.
Please check behavior with local shares. If conflict dialog has the same issue then we should rather move the conflict dialog fix to a separate PR / topic.

apps/files/js/file-upload.js Outdated Show resolved Hide resolved
apps/files/js/file-upload.js Outdated Show resolved Hide resolved
1) Renaming View/Download/Upload to View/Download/Edit.
2) Adding new option View/Download/Upload.
So if user wants to give provide all permissions,
View/Download/Edit option could be opted. If the user
wants to have an option where no edit and delete to be
granted for link shared, the View/Download/Upload could
be used.

Signed-off-by: Sujith H <[email protected]>
Adding acceptance test for new upload option added
for creating public links. This new option would
create public links which would help users to download,
upload files or folders.

Signed-off-by: Sujith H <[email protected]>
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.

if you retested this and still works then it's good to go 👍

@sharidas
Copy link
Contributor Author

sharidas commented Apr 5, 2019

@PVince81 Yes I have tested this and I have uploaded the screenshare in the PR template.

@PVince81 PVince81 merged commit 56fe77e into master Apr 5, 2019
@PVince81 PVince81 deleted the public-link-upload-edit branch April 5, 2019 15:11
@PVince81
Copy link
Contributor

PVince81 commented Apr 5, 2019

@sharidas please backport

@sharidas
Copy link
Contributor Author

sharidas commented Apr 5, 2019

Backport PR to stable10 #34983

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants