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

Fix listing other users’ annotations #7563

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jan 17, 2024

Fixed a bug where listing the annotations of other users would result in empty lists even if there are annotations and you should be allowed to see them.

URL of deployed dev instance (used for testing):

Steps to test:

  • Log in as sample@scm.io, change permissions of a dataset to e.g. include the default team
  • Log in as sample2@scm.io, create an annotation on the dataset
  • Each user should still only see their own annotations or those with team sharing in their own dashboard
  • But when navigating to the user list and clicking »show annotations« there, an admin/teammanager should see all annotations of the other user. (This is what was broken before)

Context


@fm3 fm3 self-assigned this Jan 17, 2024
@fm3 fm3 marked this pull request as ready for review January 17, 2024 15:48
@@ -553,15 +553,6 @@ export function deletePrivateLink(linkId: string): Promise<{
}

// ### Annotations
export function getAnnotationInfos(
Copy link
Member

Choose a reason for hiding this comment

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

Am I assuming correctly, that this route does not exist anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

The backend still provides this route for old clients, but the frontend doesn’t use it anymore (was replaced by getReadableAnnotations)

Copy link
Member

@hotzenklotz hotzenklotz left a comment

Choose a reason for hiding this comment

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

Tested and works.

@fm3 fm3 merged commit c000c08 into master Jan 17, 2024
2 checks passed
@fm3 fm3 deleted the fix-user-list-annotations branch January 17, 2024 16:14
Copy link
Member

@frcroth frcroth left a comment

Choose a reason for hiding this comment

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

Had this review pending, forgot to submit

Comment on lines +310 to +311
// In dashboard, list only own + explicitly shared annotations. When listing those of another user, list all of their annotations the viewer is allowed to see
isForOwnDashboard: Boolean,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// In dashboard, list only own + explicitly shared annotations. When listing those of another user, list all of their annotations the viewer is allowed to see
isForOwnDashboard: Boolean,
// In dashboard, list only own + explicitly shared annotations. When listing those of another user, list all of their annotations the viewer is allowed to see
isForOwnDashboard: Boolean,

I find the naming here to be too closely linked with the intent and not with what actually happens. Also surprising behavior when the function is called findAllListable and the parameter gives no hint on that the result may change. Maybe more like filterOwnedAndExplicitlyShared

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, wrote #7565 to track that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants