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

Implement expiration date for federated shares #25320

Merged
merged 4 commits into from
Apr 15, 2021
Merged

Implement expiration date for federated shares #25320

merged 4 commits into from
Apr 15, 2021

Conversation

PVince81
Copy link
Member

@PVince81 PVince81 commented Jan 25, 2021 •
edited
Loading

Fixes issue that the UI checkbox for expiration date was not being saved for federated share.

Todos

  • Bring back expiration date field (revert Hide expiration date field for remote shares #25933)
  • Setting expiration date is saved in DB
  • Clearing expiration date is saved in DB
  • Refresh the page and open the share panel: expiration date is sameas DB
  • The share actually expires (tested by manually setting a past date in DB)
  • When expiring, should notify the remote servers of the disappearance => not needed, they find out during next access
  • Add to NextCloud properly saves expiration date from link (can't test due to another bug)
  • Default expiration date applied to remote share
  • Remote group thing ??
  • New capability for clients to recognize this support: had to add a new one, there was a bogus old one there...
  • Extend unit tests
  • Add separate setting for expiration date of federated shares instead of reusing the one from internal shares ?

Notes

I do wonder if that checkbox was added by mistake in the UI through recent refactorings.
The more I dig, the more it looks like this feature was never implemented.
We removed it here #25933 and will add it back in this PR.

@rullzer

@PVince81 PVince81 added this to the Nextcloud 21 milestone Jan 25, 2021
@PVince81 PVince81 self-assigned this Jan 25, 2021
@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch from 59c1599 to 72e2c0a Compare January 25, 2021 14:46
This was referenced Jan 27, 2021
Merged
Merged
@PVince81 PVince81 modified the milestones: Nextcloud 21, Nextcloud 22 Jan 29, 2021
@PVince81 PVince81 mentioned this pull request Mar 4, 2021
Merged
@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch 2 times, most recently from 4ffd5e5 to aa75e33 Compare March 16, 2021 10:09
@PVince81
Copy link
Member Author

been lucky so far, the missing bit was reading expiration date from the DB.
after that, expiration already worked automagically

@PVince81
Copy link
Member Author

confusing report...
image

I did run cs:fix locally on related folders but nothing got fixed. I'll try a full run then.

@PVince81 PVince81 mentioned this pull request Mar 16, 2021
Merged
@PVince81
Copy link
Member Author

attempt at fixing CS on master, as a result of running composer cs:fix: #26143

@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch from 16691c6 to cac7062 Compare March 16, 2021 10:47
@PVince81
Copy link
Member Author

I've also tested remote group share and made adjustment, it works and expires properly too

@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch from cac7062 to d872821 Compare March 16, 2021 13:37
@PVince81 PVince81 added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 16, 2021
@PVince81 PVince81 marked this pull request as ready for review March 16, 2021 13:39
@PVince81
Copy link
Member Author

Implementation is done, here are topics to discuss before I move forward with unit tests:

  1. Do we want a separate setting for the default/enforced expiration of shares ?

  2. Should we delete the bogus "federation.expire_data" capability ? It was there before, stating that expiration date works for federation when it doesn't.

@PVince81
Copy link
Member Author

PVince81 commented Mar 16, 2021 •
edited
Loading

@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch from 53ea4a8 to 553afe1 Compare March 26, 2021 12:07
@PVince81
Copy link
Member Author

rebased for CI

@skjnldsv
Copy link
Member

failure unrelated

@PVince81
Copy link
Member Author

rebased and recompiled

@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch from 553afe1 to eb5aefe Compare April 12, 2021 08:10
@PVince81 PVince81 mentioned this pull request Apr 12, 2021
Closed
@PVince81
Copy link
Member Author

the random failure was supposed to be fixed but is still happening: #26314

rullzer
rullzer approved these changes Apr 12, 2021
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Didn't fully test but looks good

@PVince81
Copy link
Member Author

/compile amend /

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 14, 2021
@PVince81
Implement expiration date for federated shares
8680baf 
Add expiration date field in UI.
Save expiration date when creating or updating federated share.
Read expiration date from DB in federated share provider.
Applies to both federated user and group shares.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Fix expire date capability for federated shares
b0aaafe 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Unit tests for federated share expiration date
2650da7 
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81
Separate settings for remote share expiration
af61486 
Added separate settings for default and enforced expiration date for
remote shares.

Signed-off-by: Vincent Petry <vincent@nextcloud.com>
@PVince81 PVince81 force-pushed the bugfix/noid/save-fed-share-expiration branch from eb5aefe to af61486 Compare April 15, 2021 08:06
@PVince81
Copy link
Member Author

manual rebase + recompile

@juliusknorr juliusknorr merged commit 1c35b38 into master Apr 15, 2021
@juliusknorr juliusknorr deleted the bugfix/noid/save-fed-share-expiration branch April 15, 2021 13:06
LukasReschke added a commit to nextcloud/documentation that referenced this pull request May 11, 2021
@LukasReschke
Document lack of expiration propagation
b95d2e6 
Ref nextcloud/server#25320 for Nextcloud 22
@LukasReschke LukasReschke mentioned this pull request May 11, 2021
Merged
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request May 11, 2021
@LukasReschke @backportbot
Document lack of expiration propagation
99bd4c9 
Ref nextcloud/server#25320 for Nextcloud 22
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request May 11, 2021
@LukasReschke @backportbot
Document lack of expiration propagation
d4c2dfc 
Ref nextcloud/server#25320 for Nextcloud 22
backportbot-nextcloud bot pushed a commit to nextcloud/documentation that referenced this pull request May 11, 2021
@LukasReschke @backportbot
Document lack of expiration propagation
8ec8bfe 
Ref nextcloud/server#25320 for Nextcloud 22
This was referenced Sep 30, 2021
Open
Open
Closed
@tobiasKaminsky tobiasKaminsky mentioned this pull request Oct 28, 2021
Open
3 tasks
@tflidd tflidd mentioned this pull request Jul 10, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rullzer rullzer rullzer approved these changes

@LukasReschke LukasReschke LukasReschke approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

Assignees

@PVince81 PVince81

Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement feature: federation
Projects
None yet
Milestone
Nextcloud 22
Development

Successfully merging this pull request may close these issues.
6 participants
@PVince81 @skjnldsv @rullzer @LukasReschke @icewind1991 @juliusknorr