set default expiration date if expiration date is enforced and it is new share #35564

karakayasemi · 2019-06-17T18:41:41Z

Description

Automatic expiration date setting behavior changed with #34971. It caused regression on client sides. This PR fixes this regression by keeping #34971's functionality.

If max expiration date is enforced set default expiration date for new share. @ckamm please check the behavior is same as expected or not.

@micbar @pmaier1 we should consider this PR for v10.2.1.

Related Issue

Fixes Public link share creation no longer automatically sets expiration date #35550

Motivation and Context

Automatic expiration date setting behavior changed with #34971. It caused regression on client sides. This PR fixes this regression by keeping #34971's functionality.

How Has This Been Tested?.

To test issue #35550

Activate, "Set expiration date" and enforce max expiration date.
Try to create new public link from a desktop client
It should create link and fill expiration date with default.

To test issue #34960

Activate, "Set expiration date" but not enforce.
Click share a file via public link and remove default expiration date from its input field. The share should be created without an expiration date.

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)

micbar · 2019-06-17T19:18:29Z

@karakayasemi Great!
Please force push, random CI failure

codecov · 2019-06-17T20:56:06Z

Codecov Report

Merging #35564 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #35564      +/-   ##
============================================
+ Coverage     65.66%   65.67%   +<.01%     
- Complexity    18757    18761       +4     
============================================
  Files          1222     1222              
  Lines         70873    70880       +7     
  Branches       1289     1289              
============================================
+ Hits          46541    46548       +7     
  Misses        23954    23954              
  Partials        378      378

Flag	Coverage Δ	Complexity Δ
#javascript	`53.7% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`67.04% <100%> (ø)`	`18761 <0> (+4)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	`97.46% <100%> (+0.02%)`	`234 <0> (+4)`	⬆️

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 038c731...f050db6. Read the comment docs.

codecov · 2019-06-17T20:56:09Z

Codecov Report

Merging #35564 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #35564      +/-   ##
============================================
+ Coverage     65.66%   65.67%   +<.01%     
- Complexity    18757    18762       +5     
============================================
  Files          1222     1222              
  Lines         70873    70882       +9     
  Branches       1289     1289              
============================================
+ Hits          46541    46550       +9     
  Misses        23954    23954              
  Partials        378      378

Flag	Coverage Δ	Complexity Δ
#javascript	`53.7% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`67.04% <100%> (ø)`	`18762 <2> (+5)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Share20/Manager.php	`97.47% <100%> (+0.03%)`	`235 <2> (+5)`	⬆️

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 7a933d9...eb0446f. Read the comment docs.

jvillafanez · 2019-06-18T06:52:40Z

It feels better if this is added in the createShare method unless this is also somehow applied when a user updates the share.

karakayasemi · 2019-06-18T07:03:17Z

It feels better if this is added in the createShare method unless this is also somehow applied when a user updates the share.

Both createShare and updateShare method are using validateExpirationDate method.
If expirationDate null, for a new share validateExpirationDate method sets default date with this PR (expected behavior for clients), but for an update case, it throws InvalidArgumentException.

ckamm · 2019-06-18T07:41:39Z

@karakayasemi Excellent, I'll test it now.

ckamm · 2019-06-18T07:57:08Z

Tests:

Default + enforced expiration date

Web: Expiration date is pre-filled. Removing it leads to error. (good)
Desktop 2.6: Expiration date is pre-filled. Removing is impossible. (good)
Desktop 2.5: Creating share works, has expiration date. Removing is impossible. (good)

Default, but not enforced expiration date

Web: Expiration date is pre-filled. Removing it before creation leads to share with expiration. (bug)
Desktop 2.6: Expiration date is not pre-checked (client side bug). Removing it before creation leads to share with expiration. (bug)
Desktop 2.5: Creating share works, has expiration date. Can be removed. (good)

What's the API for "create share without expiration" vs "create share with default expiration"?

ckamm

Test issues, see comment

karakayasemi · 2019-06-18T08:29:29Z

@ckamm thanks for detailed explanation. I moved the code-block which set expiration date inside if($this->shareApiLinkDefaultExpireDateEnforced()) condition.
I tested scenarios with webUI and client 2.5.4, everything seems okay this time. Please you also test one more time. Thanks.

What's the API for "create share without expiration" vs "create share with default expiration"?

They both uses same OCS API and following same code flow, no difference between them.

jvillafanez · 2019-06-18T08:30:16Z

lib/private/Share20/Manager.php

@@ -331,6 +331,20 @@ protected function validateExpirationDate(\OCP\Share\IShare $share) {

 		// If we enforce the expiration date check that is does not exceed
 		if ($this->shareApiLinkDefaultExpireDateEnforced()) {
+			// If expiredate is empty set a default one if there is a default
+			$fullId = null;
+			try {


It might be better if this check is moved to a private function isNewShare or similar.

@jvillafanez moved to a new function. Thanks.

…new share

ckamm · 2019-06-18T08:58:13Z

Retested this case:
2) Default, but not enforced expiration date

Web: Expiration date is pre-filled. Removing it before creation leads to share without expiration. (good)
Desktop 2.6: Expiration date is not pre-checked (client side bug). Removing it before creation leads to share without expiration. (good)
Desktop 2.5: Creating share works, but has no expiration date. Can be added later. (bug)

There's still a regression with older clients that expect the server to add the default expiration date even if it's not required. But that's much less severe than not being able to create shares, so I'm fine with including this already.

I'm pretty sure one can only get everything right by keeping the old "null means default" behavior intact and making the new "request share without expiration" have different API.

jvillafanez · 2019-06-18T09:02:07Z

code looks fine, so if it behaves fine I think it's good to merge

patrickjahns · 2019-06-18T09:17:41Z

I'm pretty sure one can only get everything right by keeping the old "null means default" behavior intact and making the new "request share without expiration" have different API.

Thanks for highlighting this - was this change introduced in 10.2.0 or before?

phil-davis · 2019-06-19T08:58:00Z

Backports
stable10 #35592
release-10.2.1 #35593

karakayasemi added this to the development milestone Jun 17, 2019

karakayasemi requested review from ckamm and jvillafanez June 17, 2019 18:41

karakayasemi self-assigned this Jun 17, 2019

karakayasemi mentioned this pull request Jun 17, 2019

Public link share creation no longer automatically sets expiration date #35550

Closed

micbar mentioned this pull request Jun 17, 2019

Setting share expire date and password on creation impossible if fields are required by server owncloud/client#7246

Closed

phil-davis added the 3 - To Review label Jun 18, 2019

ckamm suggested changes Jun 18, 2019

View reviewed changes

karakayasemi force-pushed the set-default-expire branch from f050db6 to 04c9d97 Compare June 18, 2019 08:10

karakayasemi requested a review from ckamm June 18, 2019 08:30

jvillafanez reviewed Jun 18, 2019

View reviewed changes

set default expiration date if expiration date is enforced and it is …

eb0446f

…new share

karakayasemi force-pushed the set-default-expire branch from 04c9d97 to eb0446f Compare June 18, 2019 08:56

patrickjahns requested review from micbar and patrickjahns June 18, 2019 09:18

ckamm approved these changes Jun 18, 2019

View reviewed changes

ckamm mentioned this pull request Jun 18, 2019

Sharing: Fix fast share creation with enforced expiry and 10.2.0 owncloud/client#7249

Merged

patrickjahns added the backport-request label Jun 18, 2019

patrickjahns approved these changes Jun 18, 2019

View reviewed changes

micbar merged commit 11b6636 into master Jun 19, 2019

delete-merged-branch bot deleted the set-default-expire branch June 19, 2019 08:20

This was referenced Jun 19, 2019

[stable10] set default expiration date if expiration date is enforced and it is new share #35592

Merged

[10.2.1] set default expiration date if expiration date is enforced and it is new share #35593

Merged

phil-davis removed the backport-request label Jun 19, 2019

skshetry mentioned this pull request Jun 19, 2019

Test regression with public link not created when default expiration date set and enforced #35594

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set default expiration date if expiration date is enforced and it is new share #35564

set default expiration date if expiration date is enforced and it is new share #35564

karakayasemi commented Jun 17, 2019

micbar commented Jun 17, 2019

codecov bot commented Jun 17, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

jvillafanez commented Jun 18, 2019

karakayasemi commented Jun 18, 2019

ckamm commented Jun 18, 2019

ckamm commented Jun 18, 2019 •

edited

Loading

ckamm left a comment

karakayasemi commented Jun 18, 2019

jvillafanez Jun 18, 2019

karakayasemi Jun 18, 2019

ckamm commented Jun 18, 2019

jvillafanez commented Jun 18, 2019

patrickjahns commented Jun 18, 2019

phil-davis commented Jun 19, 2019

set default expiration date if expiration date is enforced and it is new share #35564

set default expiration date if expiration date is enforced and it is new share #35564

Conversation

karakayasemi commented Jun 17, 2019

Description

Related Issue

Motivation and Context

How Has This Been Tested?.

Types of changes

Checklist:

Open tasks:

micbar commented Jun 17, 2019

codecov bot commented Jun 17, 2019

Codecov Report

codecov bot commented Jun 17, 2019 • edited Loading

Codecov Report

jvillafanez commented Jun 18, 2019

karakayasemi commented Jun 18, 2019

ckamm commented Jun 18, 2019

ckamm commented Jun 18, 2019 • edited Loading

ckamm left a comment

Choose a reason for hiding this comment

karakayasemi commented Jun 18, 2019

jvillafanez Jun 18, 2019

Choose a reason for hiding this comment

karakayasemi Jun 18, 2019

Choose a reason for hiding this comment

ckamm commented Jun 18, 2019

jvillafanez commented Jun 18, 2019

patrickjahns commented Jun 18, 2019

phil-davis commented Jun 19, 2019

codecov bot commented Jun 17, 2019 •

edited

Loading

ckamm commented Jun 18, 2019 •

edited

Loading