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

Add Metadata to lock #4133

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Add Metadata to lock #4133

merged 2 commits into from
Aug 28, 2023

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Aug 23, 2023

Adds ownername and locktime to the lock information

@kobergj kobergj requested review from labkode, ishank011 and a team as code owners August 23, 2023 14:39
@update-docs
Copy link

update-docs bot commented Aug 23, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@glpatcern
Copy link
Member

@kobergj FWIW the Lock object already features the owner as a user object, and the expiration as opposed to the lock time because at the time we agreed it's better to directly encode the expiration time. Are you sure you need to encode those extra properties as opaque parameters?

@kobergj
Copy link
Contributor Author

kobergj commented Aug 24, 2023

@glpatcern it only contains the ownerID. Web would need to make another call to get the user name. Also we need the lock time to show since when the file is locked.

Both values are only needed for the client. That's why I encoded them in the opaque. The server doesn't really need them.

This was the simplest solution how to get this values to the client that I could come up with. I'm happy to implement another solution if you have an idea.

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

change the xml namespace, pls

@butonic
Copy link
Contributor

butonic commented Aug 24, 2023

In general I would also prefer if web would cache more information like user displaynames ... they rarely change. With the graph api we can fine tune these requests ... but not part of this PR...

@glpatcern
Copy link
Member

I see, thanks. For the username I guess it's fine as it's essentially an optimization. For the lock time, I'd challenge the usefulness of that information, also considering that wopi apps often recreate locks (UnlockAndRelock in the WOPI API), therefore the lock time keeps being pushed forward. Same reasoning applies to the expiration of course, but IMHO it's better to know that "the lock may expire in x minutes with no activity" rather than "the lock was created x minutes ago".

@kobergj
Copy link
Contributor Author

kobergj commented Aug 24, 2023

Yeah, that is actually a good point. But not for me to decide unfortunately. @tbsbdr is that something we want to consider? Showing lock expiration time instead of lock creation time?

@kobergj kobergj requested a review from butonic August 24, 2023 11:33
@kobergj kobergj mentioned this pull request Aug 24, 2023
@tbsbdr
Copy link

tbsbdr commented Aug 24, 2023

Showing lock expiration time is totally fine for me 👍

Co-authored-by: Jörn Friedrich Dreyer <[email protected]>
Signed-off-by: jkoberg <[email protected]>
@kobergj kobergj merged commit 594d4e1 into cs3org:edge Aug 28, 2023
9 checks passed
@kobergj kobergj deleted the AddDetailsToLock branch August 28, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants