-
Notifications
You must be signed in to change notification settings - Fork 22
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 option to lock explorative annotations #7801
Conversation
…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)
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) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was unused
object AnnotationCompactInfo { | ||
implicit val jsonFormat: Format[AnnotationCompactInfo] = Json.format[AnnotationCompactInfo] | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was not needed
event: React.SyntheticEvent, | ||
event?: React.SyntheticEvent, | ||
): void => { | ||
event.stopPropagation(); // prevent the onClick event | ||
event?.stopPropagation(); // prevent the onClick event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event is now optional as this function is used by setLockedState
to add / remove a locked
tag.
@@ -527,23 +535,17 @@ export class UserBoundingBoxInput extends React.PureComponent<UserBoundingBoxInp | |||
}} | |||
onPressEnter={this.handleNameChanged} | |||
onBlur={this.handleNameChanged} | |||
disabled={!allowUpdate} | |||
disabled={disabled} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, didn't test yet, but the code looks good 👍
conf/messages
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not even an admin can do so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine by me too. Restricting it to the owner certainly is easier for the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if a user leaves the team, an admin would need to copy the annotation to make it editable again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After we agreed upon this, this pr should (and its potential changes) this pr should be ready for the 2nd review round
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
}, | ||
this.state.shouldShowArchivedTracings, | ||
); | ||
this.updateTracingInLocalState(tracing, (t) => update(t, { name: { $set: name } })); | ||
editAnnotation(tracing.id, tracing.typ, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/view/action-bar/tracing_actions_view.tsx
Outdated
Show resolved
Hide resolved
Testing went smooth with only two exceptions:
|
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend looking good :) I just added two comments
val teamOrganizationIds = parseArrayLiteral(<<[String]).map(ObjectId(_)) | ||
val modified = <<[Instant] | ||
val tags = parseArrayLiteral(<<[String]).toSet | ||
val state = AnnotationState.fromString(<<[String]).getOrElse(AnnotationState.Active) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 GetResult
s somewhere. But I honestly don't know how to do that 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <philippotto@users.noreply.github.com>
- include waiting time before reloading annotation view to properly show the success toast - updated changelog entry
@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:
@philippotto Could you please give this a quick recheck 🙏? |
URL of deployed dev instance (used for testing):
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:
allowUpdateAndIsNotLocked
as a locked annotation will always be set toallowUpdate: false
Issues:
(Please delete unneeded items, merge only when none are left open)