Annotation Locking Mechanism

[fm3]

new route POST /api/annotations/<annotationId>/acquireMutex (no body) will, for othersMayEdit annotations, return json body with two fields: canEdit: bool, blockedByUser: user (optional, only set if blocked. Uses compact version of user json, just like in user/owner in annotation json)

URL of deployed dev instance (used for testing):

• https://___.webknossos.xyz

Steps to test:

• create an additional user account / promote sample user2 to also be an admin.
• create an annotation with sample user 1, make it editable by others keep it open.
• Open the annotation in a private tab with sample user2. There it should switch to read only and show a toast that sample user is currently editing the annotation
• When you close the annotation with sample user and wait for max 3 minutes, the annotation should be available to sample user2 and automatically reload the annotation once the mutex is acquired.
• Now reopen the annotation with sample user and try to turn of othersMayEdit. This should not be possible, as sample user2 currently has the mutex.
• close the annotation with sample user2
• wait 3 mins and sample user should now be able to edit the annotation and the othersMayEdit setting again.#
• and of cause: Play around with it the way you want to discover uncovered edge cases :D

Issues:

• fixes Automatic Annotation Locking Mechanism (for collaboration) #6800

---

• Updated (unreleased) changelog
• Ready for review

[fm3]

@MichaelBuessemeyer I think the route is already usable, should be possible to start developing the front-end part :)

TODO backend:

• clean up expired mutexes
• extract mutex timeout to config, agree on good default value
• consider moving the mutexes from main memory to postgres to avoid losing them on wk restart
• include time logging to figure out in production if we need to tackle concurrent performance

[MichaelBuessemeyer]

@fm3 As this is a setting that might be changed, I think it would be a good idea to expose it to the frontend. Thus the frontend can adjust to the timeout duration so it requests the mutex in time.

If that setting is not designed to be changed, ignore my post 📰

[MichaelBuessemeyer]

@fm3 I don't know whether that's needed :)

[ ] Needs datastore update after deployment

[philippotto]

Great stuff 👍 The code looks very good. I only left some nitpicks/edge cases.

Kudos for writing the tests 💯 Didn't review those yet, though. I'll test after the feedback is addressed :)

[philippotto]

Suggested change

	if (isInitialRequest && canEdit) {
	// Only set allow update to true in case the first try to get the mutex succeeded.
	yield* put(setAnnotationAllowUpdateAction(true));
	} else {
	yield* put(setAnnotationAllowUpdateAction(false));
	}
	// Only set allow update to true in case the first try to get the mutex succeeded.
	yield* put(setAnnotationAllowUpdateAction(isInitialRequest && canEdit));

[philippotto]

while my suggestion should be equal to the current code, I just noticed during testing that the logic itself is faulty. at the moment, on each iteration that is not the first, the boolean will be false and thus disabling the "is allowed" property. thus, I cannot edit my own annotation even when I got the mutex.

[MichaelBuessemeyer]

at the moment, on each iteration that is not the first, the boolean will be false and thus disabling the "is allowed" property. thus, I cannot edit my own annotation even when I got the mutex.

Ah, I see what the problem is. Sorry, that I missed that. I did not test manually for long enough for this to happen 😅 .

The code now remembers whether the mutex could be acquired from the beginning. Now the code only sets the "is allowed" to false, if the mutex could wasn't acquired from the beginning or canEdit sent by the server is false. This should be the correct logic right?

The new logic should now be:
=> Keep allowUpdate true in case the mutex was acquired initially and the server sends back canEdit = true
=> once the server sends canEdit = false allowUpdate will never be set to true again until the user reloads the page.
=> If the mutex wasn't acquired initially, the allowUpdate will always be false until the user reloads the page.

This should cover all cases now, right? And blocking all updates from the annotation once the mutex was lost only a single time should be correct, right? Because there could be another potential user that had the mutex in the meantime and made some small changes to the annotation.

[philippotto]

another thing I just noticed: If I'm the owner of an annotation, but don't have the mutex currently, I cannot disable the "may others edit" property (because the annotation is read-only for me). one could argue that this is fine, because the owner shouldn't disturb another user that is currently working on it, but I feel like this can be annoying if a user decides that they want to regain full control.

@fm3 @MichaelBuessemeyer do you have thoughts on that (regardless of implementation)?
implementation-wise it might only be a front-end thing (not sure, though).

[philippotto]

see my one comment. I think, the logic is a bit broken now so that the mutex cannot be kept. I wonder why the tests didn't catch that?

[MichaelBuessemeyer]

see my one comment. I think, the logic is a bit broken now so the mutex cannot be kept. I wonder why the tests didn't catch that?

That was due to the tests not testing the case where the mutex was acquired correctly multiple times in a row. They only tested for one successful request and then another failing one (with canEdit = false). I added a test catching this case now.

[philippotto]

Works excellent now 👍 Only left one renaming suggestion..

UX-wise one thing caught my attention: If I'm viewing my own shared annotation, but don't have the mutex, the navbar switches to the read-only mode which is fine per se. However, (apart from the toast which can be closed), the UI doesn't tell me anything about this state. The "Copy to my account" even suggests that this annotation isn't mine (even though it is). Do you think the navbar could be tweaked a bit in that state? I think, you could check for annotation.owner == activeUser && isReadOnly. In that case, the copy-button could simply say "Duplicate" and an antd tag could be next to the avatar saying "Locked by ..." (with a tooltip which explains a bit more).

[philippotto]

I'd rather give this variable a semantic meaning (e.g., acquireMutexInterval).

[MichaelBuessemeyer]

@philippotto Thanks for the suggestions. Using a tag as a component doesn't seem very visually appealing to me, as it does not fit into the style of the navbar. But I cannot think of another component that could be used here instead :/

I additionally added a tooltip to the tag that uses the same message as the toast.

Visually it looks like this now:

I did not know where exactly to put it. Here is the other variation.

I personally prefer the second one.
I could also put the tag as the right-most item, but I think that the avatar should always be the right-most item. To I left out this possibility.

Another thing I noticed is that these messages frequently pop up:

Should I add the possibility to disable them or something like that? I think they are quite annoying. And I'd guess if another person is actively editing the annotation, this notification will nearly always pop up and annoy the user that is just waiting or viewing the annotation.

And this is an example image of the renaming of the "copy to my account" button:

I'll fix the tests now 😅

[philippotto]

Awesome!

@philippotto Thanks for the suggestions. Using a tag as a component doesn't seem very visually appealing to me, as it does not fit into the style of the navbar. But I cannot think of another component that could be used here instead :/

Visually it looks like this now: I did not know where exactly to put it. Here is the other variation. I personally prefer the second one.

I prefer the second version, too, and think it looks alright 👍

Another thing I noticed is that these messages frequently pop up: Should I add the possibility to disable them or something like that? I think they are quite annoying. And I'd guess if another person is actively editing the annotation, this notification will nearly always pop up and annoy the user that is just waiting or viewing the annotation.

Hm, yeah, you're probably right. Maybe adapt the saga to not display the warnings if the "others may edit" setting is true (in that case one has either locked the annotation --> no problem. OR: someone else has locked it --> navbar already shows what's up).

And this is an example image of the renaming of the "copy to my account" button:

👍

[MichaelBuessemeyer]

And this is how it looks, when the user acquired the mutex and only needs to reload:

[philippotto]

Awesome, lets ship it :)

[MichaelBuessemeyer]

Looks like I forgot to add a unique key attribute here 🙈