Skip to content

Conversation

@jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Jun 17, 2020

Description

Add support to expire federated shares the same way it was added for user and group shares.
New settings to add a default date and enforce it as maximum also included (the same way as for user and group shares)

Related Issue

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

Motivation and Context

How Has This Been Tested?

Manually tested:

  • expiration date for federated shares is stored and retrieved
  • default expiration used when the share is created from the web UI
  • maximum expiration enforced in the web UI if the setting is active

Screenshots (if appropriate):

(orange bars are scrollbars, nothing to do with the page style)

Screenshot from 2020-06-17 17-59-11

Screenshot from 2020-06-17 18-00-19

Screenshot from 2020-06-17 18-01-10

We might need to review how we can deal with long sharee's names. Text is moved below, which looks bad compared with how shorter names are handled (local user shares)

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

@jvillafanez jvillafanez self-assigned this Jun 17, 2020
@jvillafanez jvillafanez requested review from VicDeo and micbar June 18, 2020 11:12
@jvillafanez jvillafanez marked this pull request as ready for review June 18, 2020 11:13
@jvillafanez jvillafanez force-pushed the federated_expiration branch from e96f6f5 to 157dd9f Compare June 19, 2020 12:31
@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #37548 into master will increase coverage by 0.00%.
The diff coverage is 69.35%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37548   +/-   ##
=========================================
  Coverage     64.66%   64.66%           
- Complexity    19343    19350    +7     
=========================================
  Files          1279     1279           
  Lines         75600    75654   +54     
  Branches       1333     1336    +3     
=========================================
+ Hits          48885    48924   +39     
- Misses        26323    26336   +13     
- Partials        392      394    +2     
Flag Coverage Δ Complexity Δ
#javascript 54.06% <78.94%> (+0.03%) 0.00 <0.00> (ø)
#phpunit 65.84% <65.11%> (+<0.01%) 19350.00 <5.00> (+7.00)
Impacted Files Coverage Δ Complexity Δ
core/js/config.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
core/js/shareitemmodel.js 80.10% <0.00%> (-0.42%) 0.00 <0.00> (ø)
settings/templates/panels/admin/filesharing.php 19.29% <20.00%> (+0.06%) 0.00 <0.00> (ø)
...ederatedfilesharing/lib/FederatedShareProvider.php 63.76% <72.72%> (+0.39%) 95.00 <1.00> (+1.00)
core/js/shareconfigmodel.js 83.05% <86.66%> (-0.63%) 0.00 <0.00> (ø)
apps/files_sharing/lib/Capabilities.php 98.71% <100.00%> (+5.56%) 13.00 <0.00> (+1.00)
core/js/sharedialogshareelistview.js 72.00% <100.00%> (+0.32%) 0.00 <0.00> (ø)
lib/private/Share20/Manager.php 95.97% <100.00%> (-0.22%) 278.00 <4.00> (+5.00) ⬇️
settings/Panels/Admin/FileSharing.php 98.71% <100.00%> (+0.05%) 7.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 2bc948d...157dd9f. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 19, 2020

Codecov Report

Merging #37548 into master will increase coverage by 0.00%.
The diff coverage is 69.35%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37548   +/-   ##
=========================================
  Coverage     64.75%   64.75%           
- Complexity    19396    19403    +7     
=========================================
  Files          1285     1285           
  Lines         75762    75816   +54     
  Branches       1333     1336    +3     
=========================================
+ Hits          49057    49096   +39     
- Misses        26313    26326   +13     
- Partials        392      394    +2     
Flag Coverage Δ Complexity Δ
#javascript 54.06% <78.94%> (+0.03%) 0.00 <0.00> (ø)
#phpunit 65.93% <65.11%> (+<0.01%) 19403.00 <5.00> (+7.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
core/js/config.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
core/js/shareitemmodel.js 80.10% <0.00%> (-0.42%) 0.00 <0.00> (ø)
settings/templates/panels/admin/filesharing.php 19.29% <20.00%> (+0.06%) 0.00 <0.00> (ø)
...ederatedfilesharing/lib/FederatedShareProvider.php 63.76% <72.72%> (+0.39%) 95.00 <1.00> (+1.00)
core/js/shareconfigmodel.js 83.05% <86.66%> (-0.63%) 0.00 <0.00> (ø)
apps/files_sharing/lib/Capabilities.php 98.71% <100.00%> (+5.56%) 13.00 <0.00> (+1.00)
core/js/sharedialogshareelistview.js 72.00% <100.00%> (+0.32%) 0.00 <0.00> (ø)
lib/private/Share20/Manager.php 95.97% <100.00%> (-0.22%) 278.00 <4.00> (+5.00) ⬇️
settings/Panels/Admin/FileSharing.php 98.71% <100.00%> (+0.05%) 7.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 a992db8...9a6dc50. Read the comment docs.

Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

LGTM

@micbar
Copy link
Contributor

micbar commented Jun 22, 2020

@VicDeo can you do a code review?

@phil-davis
Copy link
Contributor

We should make some acceptance tests like what is done for expiration dates on user, group and public link shares. Should those be added here? Or merge this and make a separate isssue for the tests?

micbar
micbar previously requested changes Jun 22, 2020
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

Please include the acceptance tests here in that PR.

@phil-davis phil-davis self-assigned this Jun 22, 2020
@phil-davis
Copy link
Contributor

Assigned myself to sort out acceptance tests.

@individual-it
Copy link
Member

assigned @swoichha for API tests and @haribhandari07 for UI tests

@phil-davis phil-davis removed their assignment Jun 24, 2020
@haribhandari07
Copy link
Contributor

@jvillafanez Here's an issue #37595 related to this PR

@mmattel
Copy link
Contributor

mmattel commented Jun 25, 2020

Pls run: make test-php-style-fix to fix coding style error(s)

@swoichha swoichha force-pushed the federated_expiration branch from 81c4f45 to 4bb8b4c Compare June 30, 2020 10:24
@jvillafanez
Copy link
Member Author

Anything left to do here? I see CI failing but I'm not sure if there is a problem with the tests or with the code...

@phil-davis
Copy link
Contributor

I rebased just now. There were various conflicts and I think I sorted out the correct changes.

Now I will review what is here and get it going.

@phil-davis phil-davis self-requested a review August 21, 2020 08:52
@phil-davis phil-davis force-pushed the federated_expiration branch from 60f9155 to 743cf33 Compare August 21, 2020 10:53
@owncloud owncloud deleted a comment from update-docs bot Aug 21, 2020
@micbar
Copy link
Contributor

micbar commented Aug 24, 2020

@phil-davis Thank you.

How can we move that forward?

@phil-davis
Copy link
Contributor

I am working on it. But currently need to sort out #37838 also so that #37835 can be merged.

@phil-davis phil-davis force-pushed the federated_expiration branch from d780480 to d594159 Compare August 24, 2020 14:00
@phil-davis
Copy link
Contributor

Squashed into just 2 commits and rebased. IMO there is just 1 test scenario to sort out...

@phil-davis phil-davis force-pushed the federated_expiration branch from d594159 to 8b5d8b5 Compare August 25, 2020 10:51
@phil-davis phil-davis force-pushed the federated_expiration branch from 9a6dc50 to d2a0d53 Compare August 25, 2020 14:15
@phil-davis phil-davis dismissed micbar’s stale review August 25, 2020 14:16

tests are done

@phil-davis phil-davis merged commit bf857ae into master Aug 25, 2020
@phil-davis phil-davis deleted the federated_expiration branch August 25, 2020 14:17
@mmattel
Copy link
Contributor

mmattel commented Aug 25, 2020

@phil-davis @jvillafanez @micbar
This is doc relevant and I do not see an issue created in docs and linked to here!
Please file one else this gets forgotten!

@jvillafanez
Copy link
Member Author

Doc ticket opened in owncloud/docs#2759

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.

9 participants