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

[For 10.4] Add dir/path sharing infos #36254

Closed
wants to merge 37 commits into from
Closed

Conversation

felixheidecke
Copy link
Contributor

@felixheidecke felixheidecke commented Oct 8, 2019

Description

Filelist shows share indicators on files and folders, that reside inside a shared folder / shared folders.
The sidebar share-tab reveals a detailed view of the share-tree including share-recipients and the respective folders the life in.

2019-11-12-21-23-13

Related Issue

https://github.com/owncloud/enterprise/issues/3453 (Discussion)
https://github.com/owncloud/enterprise/issues/3556 (Vis. Concept)

How Has This Been Tested?

Manual testing in

  • Google Chome
  • Mozilla Firefox
  • MSIE 11

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

@felixheidecke felixheidecke self-assigned this Oct 8, 2019
@felixheidecke felixheidecke requested a review from PVince81 October 8, 2019 12:16
@felixheidecke felixheidecke changed the title Add dir/path sharing info [WIP] Add dir/path sharing info Oct 8, 2019
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 so far, except for the missing docs.

Now to see how this will be used

apps/files/js/filelist.js Show resolved Hide resolved
apps/files/js/filelist.js Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
@owncloud owncloud deleted a comment from codecov bot Oct 9, 2019
@owncloud owncloud deleted a comment from codecov bot Oct 9, 2019
@codecov
Copy link

codecov bot commented Oct 11, 2019

Codecov Report

Merging #36254 into master will increase coverage by 0.5%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #36254     +/-   ##
===========================================
+ Coverage     64.68%   65.18%   +0.5%     
- Complexity    19023    19794    +771     
===========================================
  Files          1268     1271      +3     
  Lines         74362    74736    +374     
  Branches       1309     1310      +1     
===========================================
+ Hits          48100    48719    +619     
+ Misses        25876    25630    -246     
- Partials        386      387      +1
Flag Coverage Δ Complexity Δ
#javascript 54.05% <71.42%> (+0.05%) 0 <0> (ø) ⬇️
#phpunit 66.41% <ø> (+0.54%) 19794 <ø> (+771) ⬆️
Impacted Files Coverage Δ Complexity Δ
core/js/share.js 36.97% <ø> (ø) 0 <0> (ø) ⬇️
apps/files_sharing/js/share.js 72.44% <100%> (+0.44%) 0 <0> (ø) ⬇️
core/js/sharedialogview.js 78.71% <100%> (+0.1%) 0 <0> (ø) ⬇️
core/js/apps.js 50% <33.33%> (-1.36%) 0 <0> (ø)
lib/private/DB/MySqlTools.php 0% <0%> (-88.47%) 4% <0%> (-7%)
core/Command/App/CheckCode.php 34.31% <0%> (-3.19%) 43% <0%> (+23%)
lib/private/App/CodeChecker/CodeChecker.php 41.66% <0%> (-2.46%) 12% <0%> (+4%)
apps/dav/lib/CalDAV/Publishing/PublishPlugin.php 63.63% <0%> (-2.41%) 20% <0%> (+3%)
apps/dav/lib/DAV/Sharing/Plugin.php 68.57% <0%> (-2.02%) 7% <0%> (+1%)
apps/dav/lib/Files/PublicFiles/RootCollection.php 33.33% <0%> (-1.97%) 7% <0%> (+1%)
... and 209 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 817b3f1...f413df3. Read the comment docs.

@felixheidecke felixheidecke changed the title [WIP] Add dir/path sharing info Add dir/path sharing infos Oct 23, 2019
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.

See comments.

apps/files/css/files.css Show resolved Hide resolved
apps/files/css/files.css Outdated Show resolved Hide resolved
apps/files/css/files.css Outdated Show resolved Hide resolved
apps/files/css/files.css Outdated Show resolved Hide resolved
apps/files/css/files.css Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Show resolved Hide resolved
@felixheidecke felixheidecke force-pushed the path-share-info branch 2 times, most recently from 92b667b to ea6253f Compare October 28, 2019 15:16
@felixheidecke
Copy link
Contributor Author

@PVince81 there is no need to merge it yet, but if the code is fine, we can supply a patch to the customer.

apps/files/css/files.css Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
apps/files/js/filelist.js Outdated Show resolved Hide resolved
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 so far except for the concern about scrolling to the next page where items are rendered dynamically. Once this is solved this could be tested as patch.

Also, before we merge this we must add JS unit tests.

@felixheidecke felixheidecke force-pushed the path-share-info branch 2 times, most recently from 3ee5b4f to d3078ab Compare October 30, 2019 15:17
@phil-davis phil-davis requested review from kiranparajuli589 and removed request for kiranparajuli589 December 2, 2019 04:43
@phil-davis
Copy link
Contributor

See #36534 for a rebased/squashed version of this.

@individual-it
Copy link
Member

closing in favour of #36534

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

Successfully merging this pull request may close these issues.

7 participants