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

Add file action to explicitly lock a file #37460

Merged
merged 1 commit into from
Jun 18, 2020
Merged

Add file action to explicitly lock a file #37460

merged 1 commit into from
Jun 18, 2020

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented May 28, 2020 •
edited
Loading

Description

A new file action to lock a file/folder will be added with this PR.

Related Issue

https://github.com/owncloud/enterprise/issues/3131

How Has This Been Tested?

  • ✋

Screenshots (if appropriate):

Screenshot from 2020-05-28 16-03-15

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:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented May 28, 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.

@DeepDiver1975 DeepDiver1975 self-assigned this May 28, 2020
@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review May 28, 2020 14:04
@DeepDiver1975 DeepDiver1975 force-pushed the feature/lock-file-action branch 3 times, most recently from 5f5b193 to a589c78 Compare May 29, 2020 08:28
@codecov
Copy link

codecov bot commented May 29, 2020 •
edited
Loading

Codecov Report

Merging #37460 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #37460      +/-   ##
============================================
- Coverage     64.67%   64.66%   -0.02%     
- Complexity    19339    19343       +4     
============================================
  Files          1279     1279              
  Lines         75569    75600      +31     
  Branches       1331     1333       +2     
============================================
+ Hits          48876    48885       +9     
- Misses        26301    26323      +22     
  Partials        392      392
Flag Coverage Δ Complexity Δ
#javascript 54.03% <0.00%> (-0.11%) 0.00 <0.00> (ø)
#phpunit 65.83% <0.00%> (-0.01%) 19343.00 <1.00> (+4.00) ⬇️
Impacted Files Coverage Δ Complexity Δ
...s/dav/lib/Connector/Sabre/PublicDavLocksPlugin.php 59.25% <0.00%> (-13.47%) 10.00 <1.00> (+1.00) ⬇️
apps/files/lib/App.php 20.00% <0.00%> (-5.00%) 3.00 <0.00> (ø)
core/js/files/client.js 79.94% <0.00%> (-3.49%) 0.00 <0.00> (ø)
...ederatedfilesharing/lib/FederatedShareProvider.php 63.36% <0.00%> (+0.52%) 94.00% <0.00%> (+3.00%)
...b/private/Authentication/TwoFactorAuth/Manager.php 93.75% <0.00%> (+0.56%) 19.00% <0.00%> (ø%)

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 1dad3e4...a75abb1. Read the comment docs.

@mmattel
Copy link
Contributor

mmattel commented May 29, 2020

Questions:

  1. I strongly believe that pressing „Lock File“ on a locked file does unlocking - or?
  2. In the millions of files accessed by oC, how do I identify the files that have a lock set?
    We have on the left panel a prefilter for lists like „Shared with/by“ ect. Would’nt it be sensful to have a predefined list with files being locked? Else I do not find the files with locks anymore.

@DeepDiver1975
Copy link
Member Author 

1. I strongly believe that pressing „Lock File“ on a locked file does unlocking - or?

in case a file is locked there will be no 'Lock file' action. Unlock is on the lock side bar

2\. In the millions of files accessed by oC, how do I identify the files that have a lock set?
    We have on the left panel a prefilter for lists like „Shared with/by“ ect. Would’nt it be sensful to have a predefined list with files being locked? Else I do not find the files with locks anymore.

something for product mgmt @pmaier1

@DeepDiver1975
Copy link
Member Author

missing code coverage can be ignored.... ready for review and merge from my pov
@micbar

@mmattel
Copy link
Contributor

mmattel commented May 29, 2020 •
edited
Loading

Unlock is on the lock side bar

maybe a stupid question, but what/where is this side bar - reason see below.

Just seen that this is user-doc relevant as it is a new item in the action list
(user/the webui/the overflow menu)
We need to document how to unlock a file

@mmattel mmattel mentioned this pull request May 29, 2020
Closed
12 tasks
jvillafanez
jvillafanez reviewed May 29, 2020
View reviewed changes
apps/files/js/filelockplugin.js Outdated
fileList.fileActions.registerAction({
name: 'lock',
mime: 'all',
displayName: t('extract', 'Lock file'),
Copy link
Member

Choose a reason for hiding this comment

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

Is extract intended?

apps/files/js/locktabview.js
this.render();
if (fileInfo) {
const self = this;
this.model.on('change', function(data) {
Copy link
Member

Choose a reason for hiding this comment

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

I have concerns about this...
What if the original this.model had more events? Shouldn't we configure the same on the new model? This could also apply if the original "change" event is modified or simply does another thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

While testing I never received an event from the old model, which seems reasonable since it is no longer being used.

Nevertheless what will happen in the worstcase: the tab will render again ... 🤷

@DeepDiver1975
Copy link
Member Author

@micbar ready to be finally reviewed and merged .... for 10.5 maybe? no idea ...

@micbar
Copy link
Contributor

micbar commented Jun 15, 2020

@DeepDiver1975 please add a changelog

@DeepDiver1975
Copy link
Member Author

@DeepDiver1975 please add a changelog

already in there ...

@micbar
Copy link
Contributor

micbar commented Jun 15, 2020

Sorry, I missed it.

@DeepDiver1975
FE - Add file action to explicitly lock a file (but no folder)
a75abb1 
BE - Make sure href to the lockroot includes the base uri.
@micbar micbar merged commit 187bd0d into master Jun 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the feature/lock-file-action branch June 18, 2020 08:21
@mmattel mmattel mentioned this pull request Jun 18, 2020
Closed
@davitol davitol mentioned this pull request Jun 25, 2020
Closed
65 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

Assignees

@DeepDiver1975 DeepDiver1975

Labels
3 - To Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DeepDiver1975 @mmattel @micbar @jvillafanez