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

Feature/webdav locking frontend #32250

Merged
merged 3 commits into from
Dec 20, 2018
Merged

Conversation

DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Aug 6, 2018

Description

Frontend part of #31651

Related Issue

How Has This Been Tested?

  • manual testing
  • js unit tests are missing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

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

Open tasks:

  • unit tests
  • properly update status on unlock
  • decide on information to be displayed on the details tab

@DeepDiver1975 DeepDiver1975 added this to the development milestone Aug 6, 2018
@DeepDiver1975 DeepDiver1975 self-assigned this Aug 6, 2018
@DeepDiver1975 DeepDiver1975 requested a review from PVince81 August 6, 2018 14:33
@codecov
Copy link

codecov bot commented Aug 6, 2018

Codecov Report

Merging #32250 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #32250      +/-   ##
============================================
+ Coverage     64.76%   64.78%   +0.01%     
  Complexity    18330    18330              
============================================
  Files          1198     1198              
  Lines         69382    69386       +4     
  Branches       1276     1276              
============================================
+ Hits          44938    44949      +11     
+ Misses        24072    24068       -4     
+ Partials        372      369       -3
Flag Coverage Δ Complexity Δ
#javascript 53.09% <100%> (+0.11%) 0 <0> (ø) ⬇️
#phpunit 66.13% <100%> (ø) 18330 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
core/js/files/client.js 82.84% <100%> (+1.85%) 0 <0> (ø) ⬇️
apps/files/lib/Controller/ViewController.php 90.6% <100%> (+0.12%) 26 <0> (ø) ⬇️
core/js/files/fileinfo.js 90% <0%> (+5%) 0% <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 98d8d44...7cf5737. Read the comment docs.

@DeepDiver1975 DeepDiver1975 force-pushed the feature/webdav-locking-frontend branch from a075277 to f2edd02 Compare August 6, 2018 15:33
@PVince81
Copy link
Contributor

PVince81 commented Aug 8, 2018

The code looks good, we'll need to add unit tests as well.
Do you want me to take over for those ?

I suggest we wait for the backend PR to be merged, unless you can make this PR detect whether the server supports locking :-D

(also something to think about for Phoenix)

@PVince81 PVince81 force-pushed the feature/webdav-locking-frontend branch from abb501a to f823de9 Compare October 24, 2018 14:25
@PVince81
Copy link
Contributor

PVince81 commented Oct 24, 2018

I've added some JS tests, still WIP:

  • fix Webdav response tests that fail, strange parsing issue
  • add test that asserts that the "lockdiscovery" property appears in PROPFIND
  • add tests for lockingtabview

@PVince81
Copy link
Contributor

PVince81 commented Oct 25, 2018

The XML part fails on PhantomJS but not Chrome or Firefox.

I've decided to take this opportunity to switch to ChromeHeadless, needs review: #33296

Edit: found another solution, using more cross-browser API calls

@PVince81 PVince81 force-pushed the feature/webdav-locking-frontend branch from f823de9 to 44771d7 Compare October 25, 2018 11:16
@PVince81
Copy link
Contributor

@DeepDiver1975 I'm done with the JS tests.

Had to do some small fixes in the original code, like:

Mind retesting with real data ?
Also please answer my comments above.
Also review my modifications.

Thanks!

@PVince81
Copy link
Contributor

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@ownclouders ownclouders force-pushed the feature/webdav-locking-frontend branch from 7377907 to 771e3c0 Compare October 30, 2018 11:50
@ownclouders
Copy link
Contributor

Automated rebase with GitMate.io was successful! 🎉

@DeepDiver1975
Copy link
Member Author

@PVince81 one thing - which I failed to implement myself - if the user removes the last lock the icon in the file list is not removed. No idea how to do this ....

@PVince81
Copy link
Contributor

@DeepDiver1975 I thought this would already work: whenever you change the FileInfoModel by setting attributes, there's a listener in FileList that would rerender the current row and would remove the lock. Maybe it needs more tweaking.

@individual-it
Copy link
Member

individual-it commented Oct 31, 2018

  • bug: Sharing tab enables the lock symbol
    sharingtabbug

  • bug: shared files have too many /
    sharer:
    image
    receiver:
    image

  • bug: unsharing a locked folder as receiver does not work on the main files page The file "folder4" is locked and cannot be deleted => Persistent lock: cannot unshare from self #33847

  • bug: creating a folder structure with the same names as a shared and declined that is locked also locks the local structure => Don't apply persistent lock on received declined shares #33885

    1. as user "sharer" create a folder called "parent" and inside that a folder called "subfolder"
    2. from user "sharer" share a folder "parent" with the user "receiver"
    3. as user "sharer" set a lock on the folder "parent"
    4. as user "receiver" decline the share "parent" from the "Shared with you" page
    5. as user "receiver" create a folder called "parent" and inside that a folder called "subfolder"
    6. result:
      1. the folder "parent/subfolder" of the user "receiver" will be locked by sharer
      2. deleting that lock as "receiver" results in Unlock failed with status: 409
      3. deleting/renaming parent/subfolder" as "receiver" is not possible because its locked
  • bug: locking as a user with an email address set does not show the userid/displayname on the UI => Missing lock owner when creating lock after logging in by email address #33822

    1. create the user "user1" and set an email address for the user (ether during creation or afterwards)
    2. as "user1" lock a file of yourself
    3. result: no username is shown in the list of locks:
      image

=> backend issue, raised here #33822

@@ -205,6 +205,14 @@ img.icon-loading-small-dark, object.icon-loading-small-dark, video.icon-loading-
background-image: url('../img/actions/info-white.svg');
}

.icon-lock-closed {
Copy link
Member

Choose a reason for hiding this comment

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

that seems not to be used anywhere

@PVince81
Copy link
Contributor

raised #33848 which I found while investigating the overwrite issue

@PVince81
Copy link
Contributor

fixed file upload overwrite lock error to be better than "Locked".

I've addressed all issues. There's only one left to raise separately after merge related to "Shared with you".

@individual-it do you have any outstanding tasks ? (would be good to list as checkboxes)

Once done we can manually squash commits into logical blocks.

@individual-it
Copy link
Member

@ownclouders rebase

@ownclouders
Copy link
Contributor

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@ownclouders ownclouders force-pushed the feature/webdav-locking-frontend branch from ccd5ffb to 8824e5e Compare December 12, 2018 05:44
@ownclouders
Copy link
Contributor

Automated rebase with GitMate.io was successful! 🎉

@individual-it
Copy link
Member

@PVince81 only two tests are left failing creating a subfolder structure that is the same as the structure of a declined & locked share
the linked issue in #32250 (comment) has IMHO nothing to do with the problem

@PVince81
Copy link
Contributor

@PVince81 only two tests are left failing creating a subfolder structure that is the same as the structure of a declined & locked share
the linked issue in #32250 (comment) has IMHO nothing to do with the problem

you're right. I got confused when editing and added the link in the wrong location. then when putting it in the right location I somehow forgot to restore the other checkbox to its state.

I'll have a look. It's likely also a backend issue to be raised separately.

@PVince81
Copy link
Contributor

@individual-it I've moved "bug: creating a folder structure with the same names as a shared and declined that is locked also locks the local structure" to a separate backend bug: #33885

please disable the test accordingly for now

@PVince81
Copy link
Contributor

PVince81 commented Dec 13, 2018

  • double check whether oc_persistentlocks supports depth infinity as I see "int" used...

ok, the value -1 is used for infinite

Handle http 423 on file operations
Show lock status to file list in its own sidebar tab
@PVince81 PVince81 force-pushed the feature/webdav-locking-frontend branch from 12e0a0c to e997f7c Compare December 13, 2018 14:39
@PVince81
Copy link
Contributor

I've squashed all this into three commits, one for each of us.

@individual-it I've disabled the test for #33885

Expecting all green now.

Vincent Petry and others added 2 commits December 13, 2018 15:49
Improve lock tab view code.
Add tests for lock messages in file list
Added tests for the lock notification messages when performing move,
rename or delete.
Add JS tests for file locking plugin
Fixed XML parsing code to work with PhantomJS and potentially others by
using more standard functions.
Added filter to remove text nodes in case of paddings in the XML.
Add missing translation.
Don't show lock icon if lock array is empty
Don't display sidebar lock tab when no locks set
Properly trigger change event for activeLocks
Adjust locked error message for upload
@PVince81 PVince81 force-pushed the feature/webdav-locking-frontend branch from e997f7c to 7cf5737 Compare December 13, 2018 14:49
@DeepDiver1975
Copy link
Member Author

Fine by me 👍

@PVince81 PVince81 merged commit 5182ad0 into master Dec 20, 2018
@delete-merged-branch delete-merged-branch bot deleted the feature/webdav-locking-frontend branch December 20, 2018 11:53
@PVince81
Copy link
Contributor

stable10: #33951

@lock lock bot locked as resolved and limited conversation to collaborators Dec 20, 2019
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.

5 participants