Skip to content

Implement collaborative locking #2248

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
Merged
juliusknorr merged 8 commits into from
Apr 21, 2022
Merged

Implement collaborative locking #2248

juliusknorr merged 8 commits into from
Apr 21, 2022

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Mar 23, 2022 •
edited
Loading

  • Lock/unlock a file when opening/closing
  • Open files in read only mode when locked
  • Indicate other lock in the UI
  • Safeguard unlocking through the cleanup cronjob if no close request was sent
  • Changing ETag on lock/unlock causes a conflict, so we either need a way to still propagate the change to the root without changing the actual file tag for shares as mentioned in Collaborative locking files_lock#54 or adjust the change detection in text to maybe also compare the mtime in addition

To be tested together with nextcloud/files_lock#54

@juliusknorr juliusknorr mentioned this pull request Mar 24, 2022
Merged
2 tasks
@juliusknorr juliusknorr added enhancement New feature or request 2. developing labels Mar 24, 2022
@juliusknorr juliusknorr added this to the Nextcloud 24 milestone Mar 24, 2022
@juliusknorr juliusknorr mentioned this pull request Mar 24, 2022
Closed
8 tasks
@skjnldsv skjnldsv mentioned this pull request Mar 24, 2022
Merged
@blizzz blizzz mentioned this pull request Mar 31, 2022
Merged
@blizzz blizzz mentioned this pull request Apr 7, 2022
Merged
@juliusknorr juliusknorr self-assigned this Apr 9, 2022
@blizzz blizzz mentioned this pull request Apr 13, 2022
Merged
@blizzz blizzz modified the milestones: Nextcloud 24, Nextcloud 25 Apr 21, 2022
@juliusknorr
Implement collaborative locking
8c00c8e 
Signed-off-by: Julius Härtl <[email protected]>

TMP: Switch to OCP branch for tests

Signed-off-by: Julius Härtl <[email protected]>

Let locked files open in read only

Signed-off-by: Julius Härtl <[email protected]>

Adapt LockScope to LockContext rename

Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Open files read only if locked
53d1012 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Expose lock in the text UI
5c9225e 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Also take mtime into account for change detection during editing
fa139cf 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Unlock during session cleanup
ede33b5 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr
Make psalm happy
ff819af 
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr juliusknorr marked this pull request as ready for review April 21, 2022 14:04
@juliusknorr
Copy link
Member Author

juliusknorr commented Apr 21, 2022

/compile amend /

@juliusknorr @nextcloud-command
Bump bundles
42a7882 
Signed-off-by: Julius Härtl <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
julien-nc
julien-nc approved these changes Apr 21, 2022
View reviewed changes
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

LGTM. A few minor remarks.

lib/Service/DocumentService.php
$this->lockManager->lock(new LockContext(
$file,
ILock::TYPE_APP,
Application::APP_NAME
Copy link
Member

@julien-nc julien-nc Apr 21, 2022

Choose a reason for hiding this comment

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

Out of topic but this could become Application::APP_ID at some point.

Copy link
Member Author

@juliusknorr juliusknorr Apr 21, 2022

Choose a reason for hiding this comment

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

Yes, something for a separate PR.

lib/Service/DocumentService.php
Application::APP_NAME
));
} catch (NoLockProviderException $e) {
} catch (PreConditionNotMetException $e) {
Copy link
Member

@julien-nc julien-nc Apr 21, 2022

Choose a reason for hiding this comment

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

No warning logs here?

Copy link
Member Author

@juliusknorr juliusknorr Apr 21, 2022

Choose a reason for hiding this comment

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

I'd let the actual lock provider handle that, from the text app side there is no special interest if we unlock a file that has already been unlocked.

@juliusknorr juliusknorr merged commit d61932e into master Apr 21, 2022
@delete-merged-branch delete-merged-branch bot deleted the enh/locking branch April 21, 2022 16:08
@juliusknorr
Copy link
Member Author

juliusknorr commented Apr 21, 2022

/backport to stable24

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Apr 21, 2022
Merged
@danxuliu danxuliu mentioned this pull request Jun 17, 2022
Closed
@max-nextcloud max-nextcloud added the backported successfully backported label Jul 4, 2022
@juliusknorr juliusknorr mentioned this pull request Jul 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@julien-nc julien-nc julien-nc approved these changes

@mejo- mejo- Awaiting requested review from mejo-

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud

Assignees

@juliusknorr juliusknorr

Labels

3. to review backported successfully backported enhancement New feature or request

Projects

📝 Office team
Archived in project

Milestone

Nextcloud 25

Development

Successfully merging this pull request may close these issues.

5 participants

@juliusknorr @julien-nc @blizzz @max-nextcloud