Add option to lock explorative annotations #7801

MichaelBuessemeyer · 2024-05-14T14:05:56Z

URL of deployed dev instance (used for testing):

https://___.webknossos.xyz

Steps to test:

Create an explorative annotation of your favourite dataset
Go to the explorative annotations view. There should be a button in the actions column to lock the annotation. Click it. It should add the "locked" tag to the annotation.
View the annotation by clicking the respective action. In the annotation view, you should not be able to annotate the annotation. A "banner" in the top right corner should inform you about the fact that the annotation is locked by you. A tooltip should give information by whom this annotation is locked.
Moreover, there should be a menu item to unlock the annotation in the menu header bar. Once clicked, the annotation is unlocked and reloaded.
In the explorative annotations view, it should not be possible to archive a locked annotation.
An annotation cannot be locked and unlocked by a user with whom the annotation is shared.

TODOs:

Lock all edit possibilities in the Tracing view
Write migration
Fix the
bug, that after unlocking an annotation in the annotation view, the locked tag was still present in the explorative annotations list.
Do not allow to archive locked mappings
In the frontend: Get rid of allowUpdateAndIsNotLocked as a locked annotation will always be set to allowUpdate: false

Issues:

fixes #5087

(Please delete unneeded items, merge only when none are left open)

Updated changelog
Updated migration guide if applicable
Adapted wk-libs python client if relevant API parts change
- likely yes

…e [ci skip] - To continue check usages of "allowUpdate"

- Block Mapping ID Setting when annotation update is not allowed - minor code refactoring - add option to navbar menu to unlock annotation for owner

…ere no capability checking was done before

…-locked-state-to-annotation

- and disallow archiving locked annotations

… pr but turned out to be not needed)

MichaelBuessemeyer · 2024-05-23T15:24:32Z

app/controllers/AnnotationController.scala

-  def addSegmentIndex(id: String, tracingId: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
-    for {
-      annotation <- provider.provideAnnotation(id, request.identity)
-      _ <- bool2Fox(AnnotationType.Explorational == annotation.typ) ?~> "annotation.addSegmentIndex.explorationalsOnly"
-      annotationLayer <- annotation.annotationLayers
-        .find(_.tracingId == tracingId)
-        .toFox ?~> "annotation.addSegmentIndex.layerNotFound"
-      _ <- annotationService.addSegmentIndex(annotation, annotationLayer) ?~> "annotation.addSegmentIndex.failed"
-      updated <- provider.provideAnnotation(id, request.identity)
-      json <- annotationService.publicWrites(updated, Some(request.identity)) ?~> "annotation.write.failed"
-    } yield JsonOk(json)
-  }
-


This was unused

MichaelBuessemeyer · 2024-05-23T15:24:44Z

app/models/annotation/Annotation.scala

-object AnnotationCompactInfo {
-  implicit val jsonFormat: Format[AnnotationCompactInfo] = Json.format[AnnotationCompactInfo]
-}
-


This was not needed

MichaelBuessemeyer · 2024-05-24T08:12:44Z

frontend/javascripts/dashboard/explorative_annotations_view.tsx

-    event: React.SyntheticEvent,
+    event?: React.SyntheticEvent,
  ): void => {
-    event.stopPropagation(); // prevent the onClick event
+    event?.stopPropagation(); // prevent the onClick event


The event is now optional as this function is used by setLockedState to add / remove a locked tag.

frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx

MichaelBuessemeyer · 2024-05-24T09:31:19Z

frontend/javascripts/oxalis/view/components/setting_input_views.tsx

@@ -527,23 +535,17 @@ export class UserBoundingBoxInput extends React.PureComponent<UserBoundingBoxInp
                  }}
                  onPressEnter={this.handleNameChanged}
                  onBlur={this.handleNameChanged}
-                  disabled={!allowUpdate}
+                  disabled={disabled}


disabled sounded better to me as this is designed like a normal "form/info" input and doesn't need the semantics of the "allowUpdate" restriction setting. IMO the parent component should care about the semantic whether the input should be enabled / disabled :)

But feel free to argue

philippotto

nice, didn't test yet, but the code looks good 👍

MIGRATIONS.unreleased.md

philippotto · 2024-05-24T12:20:49Z

conf/messages

@@ -231,6 +231,11 @@ annotation.reopen.tooLate=The annotation cannot be reopened anymore, since it ha
 annotation.reopen.notAllowed=You are not allowed to reopen this annotation.
 annotation.reopen.notFinished=The requested annotation is not finished.
 annotation.reopen.failed=Failed to reopen the annotation.
+annotation.reopen.locked=This annotation is locked by the owner and therefore cannot be reopened.
+annotation.isLockedByUser.notAllowed=Only the owner of this annotation is allowed to change the locked state of an annotation.


not even an admin can do so?

I think this is what the current implementation does, yes. We should discuss it. Would you prefer if admins were allowed to lock/unlock other people’s annotations? I would vote for it. cc @normanrz

Would you prefer if admins were allowed to lock/unlock other people’s annotations?

I think, I'm against that. For the same reason that only the owner can mutate an annotation.. If we should decide to allow admins this, then, the interface should make it very clear that one is locking/unlocking somebody else's annotation.

Fine by me too. Restricting it to the owner certainly is easier for the moment

So, if a user leaves the team, an admin would need to copy the annotation to make it editable again?

So, if a user leaves the team, an admin would need to copy the annotation to make it editable again?

In your described scenario -> yes

After we agreed upon this, this pr should (and its potential changes) this pr should be ready for the 2nd review round

I just talked with @fm3 about this pending decision: -> Should we decided that admins can also unlock/lock an annotation, this can be done as a follow-up pr :)

frontend/javascripts/dashboard/explorative_annotations_view.tsx

philippotto · 2024-05-24T12:29:37Z

frontend/javascripts/dashboard/explorative_annotations_view.tsx

-      },
-      this.state.shouldShowArchivedTracings,
-    );
+    this.updateTracingInLocalState(tracing, (t) => update(t, { name: { $set: name } }));
    editAnnotation(tracing.id, tracing.typ, {


could you also catch the error case here in case the update didn't work? i.e., show an error toast and restore the name again. maybe it makes sense to switch the order, too (first update on server, then update locally; however I'm not sure if this causes a flicker or something similar).

I decided to first update locally so the user does not get confused why the name not changed successful when having something of a connection bottleneck. In case of an error I shown an error toast and revert back to the previous name (so the UI should by in sync with the data saved by the backend)

Nvm this contradicts how I implemented setLockedState. Thus, I first update the backend and if that worked out, update the view in the frontend.

And Philipp also: See #7801 (review)

frontend/javascripts/oxalis/view/left-border-tabs/mapping_settings_view.tsx

frontend/javascripts/navbar.tsx

frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx

philippotto · 2024-05-24T13:01:59Z

Testing went smooth with only two exceptions:

I'm not sure whether we really want to re-use the tags in the dashboard for "locked". for skeleton, volume and hybrids, we also have these special tags, but I'd rather get rid of that instead of extending these. the reason is mostly that this blocks these names for user-provided tags and it can be hard to understand that these are "automatic". what do you think, @MichaelBuessemeyer @fm3 ? apart from that: when clicking on an "locked" tag, the tag appears in the search bar but not in the correct color.
even though "archive" is disabled in the dashboard for a locked annotation, I can still click it and get an error toast. ideally, nothing should happen when clicking on an disabled button/link.

fm3 · 2024-05-27T08:21:38Z

Regarding the tags, I think we will want to clean those up soon (enable key-value pairs, etc). I agree that the existing “automatic” tags are confusing and we should get rid of them. However, this is at least not getting much worse by this PR, because we’ll have to deal with that anyway. But still, if we could find another way to display this locked state, I agree that it would be preferrable. Related: #7814

fm3

Backend looking good :) I just added two comments

app/controllers/AnnotationController.scala

fm3 · 2024-05-27T08:28:50Z

app/models/annotation/Annotation.scala

+    val teamOrganizationIds = parseArrayLiteral(<<[String]).map(ObjectId(_))
+    val modified = <<[Instant]
+    val tags = parseArrayLiteral(<<[String]).toSet
+    val state = AnnotationState.fromString(<<[String]).getOrElse(AnnotationState.Active)


While we’re here, could you try out if this and the other enums also work without the String detour, with <<[AnnotationState] (also changing the type in the case class)? In theory, the enums have automatic adapters both to SQL and to JSON.

i tried bug failed 🙈:

I changed to

val state = <<[AnnotationState] val isLockedByOwner = <<[Boolean] val dataSetName = <<[String] val typ = <<[AnnotationType] val visibility = <<[AnnotationVisibility]

But scala could not find the matching GetResult "converters". Thus, I added them as implicit params:

implicit def GetResultAnnotationCompactInfo(implicit e0: GetResult[AnnotationState], e1: GetResult[AnnotationType], e2: GetResult[AnnotationVisibility] ): GetResult[AnnotationCompactInfo] = GetResult { prs =>

But then the call run(query.as[AnnotationCompactInfo]) claims it needs to define the implicit params GetResult[AnnotationState]

=> I'd say no to the question / suggestion or we need to define the GetResults somewhere. But I honestly don't know how to do that 🙈

OK, thanks for experimenting! Then let’s do this another time. Feel free to go via the string route as is :)

- rename isLockedByUser to isLockedByOwner - show toast upon unlock from tracing view before reloading - catch error on failed annotation update in explorative annotations view

…defining columns as children

MichaelBuessemeyer · 2024-05-28T11:32:31Z

Ok I removed the "locked" tag and instead added a hint to the id column in case the annotation is locked:

This is similar to read-only tracings which are marked / highlighted in the same way.

even though "archive" is disabled in the dashboard for a locked annotation, I can still click it and get an error toast. ideally, nothing should happen when clicking on a disabled button/link.

Good find: I adjusted the AsyncLink to do nothing in case it is disabled.

…d annotation

frontend/javascripts/oxalis/model/reducers/reducer_helpers.ts

philippotto

front-end looks good to me 👍 it would be fine with me to merge this with the current "only the owner may change the locking" behavior. we will see whether we need to change this in the future.

Co-authored-by: Philipp Otto <[email protected]>

- include waiting time before reloading annotation view to properly show the success toast - updated changelog entry

MichaelBuessemeyer · 2024-05-30T07:56:47Z

@philippotto Yesterday I got feedback from @valentin-pinkau that locking from within the annotation view would also be very useful / essential as vx produced links directly open an annotation and the one can just lock the annotation from there instead of first having to navigate to the annotations table, locate the annotation and lock it there. Thus, I added the following changes:

The option to unlock an annotation from within the navbar dropdown menu was extended to also lock the active annotation in case the current user in the owner of the annotation.
The option was moved down to be the last item in the dropdown list to make it less "present" as this is likely not the most important feature of the dropdown menu -> Thus, let's not put it into prime (first) position in the list.
I added a short waiting time between the successful update to the locked annotation state and the reloading of the annotation view to give the UI some time to render the toast properly and give the user the potential time to read the toast to understand why wk is reloading.
The toast is now a success toast, as the update to the "lockedState" was successful.
I edited the changelog entry to hint at the new navbar dropdown menu item.

@philippotto Could you please give this a quick recheck 🙏?

philippotto · 2024-05-30T10:01:16Z

Great, looks and feels good 👍 Just one thing:

Can you add something like this to this tooltip: "You can unlock the annotation in the annotation menu." (only for the owner obviously). Otherwise, it could be confusing and users might think they cannot unlock it.

…-locked-state-to-annotation

MichaelBuessemeyer added 2 commits May 14, 2024 16:03

add backend route to update isBlockedByUser state of an annotation

75ddb36

WIP: restrict frontend edits on locked annotations [ci skip]

aec1293

MichaelBuessemeyer self-assigned this May 14, 2024

MichaelBuessemeyer added backend frontend new feature labels May 14, 2024

MichaelBuessemeyer added 11 commits May 14, 2024 17:40

WIP: blocking edit action in tracing view whn isLockedByUser is activ…

d25b004

…e [ci skip] - To continue check usages of "allowUpdate"

add evolution file

47b5b0c

improve tooltip messages in locked annotation

0a48511

- Block Mapping ID Setting when annotation update is not allowed - minor code refactoring - add option to navbar menu to unlock annotation for owner

fix backend formatting

be6a02e

Add some allowUpdate checks to other routes updating an annotation wh…

0a65e05

…ere no capability checking was done before

fix annotations.csv for e2e tests [ci skip]

85d31ab

fix/update e2e tests

2007f5e

Merge branch 'master' of github.com:scalableminds/webknossos into add…

81a01c3

…-locked-state-to-annotation

add migration entry

70935a4

remove locked tag from annotation when unlocked in annotation view

51993d6

- and disallow archiving locked annotations

add changelog entry

61fbf22

MichaelBuessemeyer marked this pull request as ready for review May 23, 2024 15:18

MichaelBuessemeyer requested a review from fm3 May 23, 2024 15:18

clean up and remove allowUpdateAndIsNotLocked function (added in this…

1980d35

… pr but turned out to be not needed)

MichaelBuessemeyer commented May 24, 2024

View reviewed changes

MichaelBuessemeyer requested a review from philippotto May 24, 2024 09:38

remove outdated comment

730c6d3

philippotto reviewed May 24, 2024

View reviewed changes

fm3 reviewed May 27, 2024

View reviewed changes

MichaelBuessemeyer added 3 commits May 28, 2024 11:31

remove migration reason

d77d856

apply pr feedback

4b91dd3

- rename isLockedByUser to isLockedByOwner - show toast upon unlock from tracing view before reloading - catch error on failed annotation update in explorative annotations view

refactor explorative annotations table to use column prop instead of …

5950da0

…defining columns as children

replace locked tag with disabled text in id column hinting at a locke…

6e593ba

…d annotation

philippotto reviewed May 28, 2024

View reviewed changes

frontend/javascripts/oxalis/model/reducers/reducer_helpers.ts Outdated Show resolved Hide resolved

philippotto approved these changes May 28, 2024

View reviewed changes

MichaelBuessemeyer and others added 4 commits May 28, 2024 15:54

Update frontend/javascripts/oxalis/model/reducers/reducer_helpers.ts

f3c4893

Co-authored-by: Philipp Otto <[email protected]>

update snapshots

79cd978

Merge branch 'master' into add-locked-state-to-annotation

95535b8

make it possible to lock an annotation from annotation view

c20baf6

- include waiting time before reloading annotation view to properly show the success toast - updated changelog entry

add revision

581099d

MichaelBuessemeyer added 2 commits May 30, 2024 13:15

add hint for unlocking for owners to navbar locked tag

073f941

Merge branch 'master' of github.com:scalableminds/webknossos into add…

c05b825

…-locked-state-to-annotation

MichaelBuessemeyer merged commit d3b0638 into master May 30, 2024
2 checks passed

MichaelBuessemeyer deleted the add-locked-state-to-annotation branch May 30, 2024 12:18

MichaelBuessemeyer mentioned this pull request May 31, 2024

Fix lock explorative annotations #7847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to lock explorative annotations #7801

Add option to lock explorative annotations #7801

MichaelBuessemeyer commented May 14, 2024 •

edited

Loading

MichaelBuessemeyer May 23, 2024

MichaelBuessemeyer May 23, 2024

MichaelBuessemeyer May 24, 2024

MichaelBuessemeyer May 24, 2024

philippotto left a comment

philippotto May 24, 2024

fm3 May 27, 2024

philippotto May 27, 2024

fm3 May 27, 2024

normanrz May 28, 2024

MichaelBuessemeyer May 28, 2024

MichaelBuessemeyer May 28, 2024

MichaelBuessemeyer May 28, 2024

philippotto May 24, 2024

MichaelBuessemeyer May 28, 2024

MichaelBuessemeyer May 28, 2024 •

edited

Loading

philippotto commented May 24, 2024

fm3 commented May 27, 2024

fm3 left a comment

fm3 May 27, 2024

MichaelBuessemeyer May 28, 2024

fm3 May 28, 2024

MichaelBuessemeyer commented May 28, 2024

philippotto left a comment

MichaelBuessemeyer commented May 30, 2024

philippotto commented May 30, 2024

Add option to lock explorative annotations #7801

Add option to lock explorative annotations #7801

Conversation

MichaelBuessemeyer commented May 14, 2024 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

TODOs:

Issues:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philippotto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer May 28, 2024 • edited Loading

Choose a reason for hiding this comment

philippotto commented May 24, 2024

fm3 commented May 27, 2024

fm3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented May 28, 2024

philippotto left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented May 30, 2024

philippotto commented May 30, 2024

MichaelBuessemeyer commented May 14, 2024 •

edited

Loading

MichaelBuessemeyer May 28, 2024 •

edited

Loading