Skip to content

Remove share permission check from the file upload#35060

Merged
PVince81 merged 1 commit into
masterfrom
remove-sharepermission-check
Apr 23, 2019
Merged

Remove share permission check from the file upload#35060
PVince81 merged 1 commit into
masterfrom
remove-sharepermission-check

Conversation

@sharidas

Copy link
Copy Markdown
Contributor

Remove share permission check from the file upload
js file.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Remove sharePermission checks from the file-upload.js file. We are no where using it and there is a problem in the code due to this. When we know its a file, we just have to check the fileinfo ( in the if condition ). The problem this PR tries to address is, when files are uploaded, the file information should be shown. Instead of this, the directory information was shown.

This also fixes the cancel button problem in the conflict dialog. The cancel button clicked once cancels the conflict dialog.

Related Issue

Motivation and Context

Remove sharePermission checks from the file-upload.js file. This solves 2 problems:

  1. The file which has conflict will show the file information in the conflict dialog popup and not the directory info.
  2. Cancel button of the conflict dialog pop up will work with one click, instead of multiple click issue mentioned at conflict dialogue for "Download / View / Upload" public link multiplies the confict files on "cancel" #35057.

How Has This Been Tested?

  • Kindly see the screen share
    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 Apr 18, 2019
@sharidas sharidas self-assigned this Apr 18, 2019
@individual-it

Copy link
Copy Markdown
Member

solves the problem for me, should be covered in acceptance tests

@PVince81 PVince81 modified the milestones: development, QA Apr 18, 2019
@PVince81

PVince81 commented Apr 18, 2019

Copy link
Copy Markdown
Contributor

@sharidas please research with git blame to see why these checks were added in the first place.

I remember a special case with share folders and conflict dialog that needed to be covered, we need to make sure that your change does not break that.

@codecov

codecov Bot commented Apr 18, 2019

Copy link
Copy Markdown

Codecov Report

Merging #35060 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #35060   +/-   ##
=========================================
  Coverage     65.37%   65.37%           
  Complexity    18622    18622           
=========================================
  Files          1215     1215           
  Lines         70510    70510           
  Branches       1295     1295           
=========================================
  Hits          46095    46095           
  Misses        24041    24041           
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 52.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.81% <ø> (ø) 18622 <ø> (ø) ⬇️

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 ec033b2...5ad29dc. Read the comment docs.

@codecov

codecov Bot commented Apr 18, 2019

Copy link
Copy Markdown

Codecov Report

Merging #35060 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #35060   +/-   ##
=========================================
  Coverage     65.37%   65.37%           
  Complexity    18622    18622           
=========================================
  Files          1215     1215           
  Lines         70510    70510           
  Branches       1295     1295           
=========================================
  Hits          46095    46095           
  Misses        24041    24041           
  Partials        374      374
Flag Coverage Δ Complexity Δ
#javascript 52.85% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 66.81% <ø> (ø) 18622 <ø> (ø) ⬇️

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 ec033b2...d19644c. Read the comment docs.

@sharidas

Copy link
Copy Markdown
Contributor Author

I remember a special case with share folders and conflict dialog that needed to be covered, we need to make sure that your change does not break that.

I did a quick test by sharing a folder from one oC user to another ( local share ), and my change did not break this. Here is the screen share for the quick test I did:
sharefolder
I hope this is what you meant.
Also the git blame was pointing towards my commit and I didn't found any trace where the sharePermissions was used in the file.

@sharidas
sharidas force-pushed the remove-sharepermission-check branch from 5ad29dc to e870fc0 Compare April 18, 2019 09:19
@PVince81

Copy link
Copy Markdown
Contributor

@sharidas you need to git blame further in the past. I believe Github has some buttons for this.
please find the original PR that instroduced the permissions check as it might contain more information about the test case, to confirm that what you tested matches. from what I remember it was a rather twisted setup...

@sharidas

Copy link
Copy Markdown
Contributor Author

I guess there is a confusion with respect to the PR description or heading. Looking at the history of file-upload.js file, when I try to get the state of this file before my changes were applied, the variable sharePermission is not available. This variable is introduced from my commit. I had removed only the if part from 521722a#diff-9112ab761e9b96bef644fc2c1f6e35c1R853 , that is b9879c3. This is not to consider the autorename for the new granular permission task. I should have also removed sharePermission variable from the commit which was introduced in the commit 521722a

@sharidas
sharidas force-pushed the remove-sharepermission-check branch 2 times, most recently from a8bf9a3 to 689bc56 Compare April 18, 2019 12:24
Remove share permission check from the file upload
js file.

Signed-off-by: Sujith H <sharidasan@owncloud.com>

@jvillafanez jvillafanez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, not tested

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍 in regard of the clarification.

@PVince81
PVince81 merged commit 409d1f7 into master Apr 23, 2019
@PVince81
PVince81 deleted the remove-sharepermission-check branch April 23, 2019 09:54
@PVince81

Copy link
Copy Markdown
Contributor

@sharidas please backport to release-10.2.0 as it fixes a bug introduced by a recent change found by QA

@sharidas

Copy link
Copy Markdown
Contributor Author

Backport to release-10.2.0 branch #35082

@phil-davis

Copy link
Copy Markdown
Contributor

The backport was merged into release-10.2.0 and the release branch was merged into stable10 so the backporting is done here.

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

Projects

None yet

5 participants