Only show locked banner if current user is in organization of the annotation #8273
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files to enhance the handling of user permissions and organizational context within the application. Key changes include the addition of new state properties and functions to evaluate whether annotations are from different organizations. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
frontend/javascripts/oxalis/model/accessors/annotation_accessor.ts (1)
27-31: Consider improving edge case handling and type safetyThe function could be enhanced to handle edge cases more explicitly:
export function isAnnotationFromDifferentOrganization(state: OxalisState) { const activeUser = state.activeUser; - - return !!(activeUser && activeUser?.organization !== state.tracing.organization); + // If there's no active user, consider it as a different organization + if (!activeUser) return true; + + return activeUser.organization !== state.tracing.organization; }This refactor:
- Makes the edge case handling more explicit
- Removes redundant optional chaining since we've already checked for activeUser
- Eliminates the need for double bang (!!) operator
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/javascripts/navbar.tsx(5 hunks)frontend/javascripts/oxalis/default_state.ts(1 hunks)frontend/javascripts/oxalis/model/accessors/annotation_accessor.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/reducer_helpers.ts(2 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)
🔇 Additional comments (7)
frontend/javascripts/oxalis/model/reducers/reducer_helpers.ts (1)
98-98: LGTM: Organization field properly added to annotation conversion
The organization field is correctly extracted from the API annotation and included in the frontend annotation object.
Also applies to: 112-112
frontend/javascripts/oxalis/default_state.ts (1)
181-181: LGTM: Organization field properly initialized
The organization field is correctly initialized with an empty string, consistent with other similar fields in the default state.
Let's verify that organization handling is consistent across the codebase:
✅ Verification successful
Based on the search results, I can see that the organization field is consistently used across the codebase:
- It's initialized with an empty string in the default state (
frontend/javascripts/oxalis/default_state.ts) - It's used in permission checks:
frontend/javascripts/libs/utils.ts: Comparing user organization with dataset ownershipfrontend/javascripts/oxalis/view/right-border-tabs/dataset_info_tab_view.tsx: Organization-based access controlfrontend/javascripts/dashboard/folders/details_sidebar.tsx: Organization ownership checksfrontend/javascripts/oxalis/model/accessors/annotation_accessor.ts: Cross-organization annotation checks
LGTM: Organization field is consistently handled
The empty string default is appropriate and the organization field is used consistently throughout the codebase for permission checks and access control. The implementation aligns with the application's organization-based security model.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other organization-related fields and their initialization
rg "organization: \"\"" --type typescript
# Check for organization field usage in conditions
rg "\.organization\s*[!=]==?\s*" --type typescript
Length of output: 164
Script:
#!/bin/bash
# Check for other organization-related fields and their initialization
rg "organization:\s*[\"']\""
# Check for organization field usage in conditions
rg "\.organization\s*[!=]==?\s*"
# Look for organization-related type definitions
ast-grep --pattern 'interface $_ {
$$$
organization: $_
$$$
}'
# Look for organization field usage in components
ast-grep --pattern '$_.organization'
Length of output: 67170
frontend/javascripts/oxalis/store.ts (1)
200-200: LGTM: Well-structured type definition addition
The addition of the readonly organization property to the Annotation type is clean and follows immutability best practices for Redux state management.
frontend/javascripts/navbar.tsx (4)
67-70: LGTM: Clean import organization
Good separation of concerns by importing the organization check from a dedicated accessor.
91-91: LGTM: Well-typed state property addition
The new state property follows the existing type system and naming conventions.
885-890: LGTM: Clear conditional logic for locked banner
The modified condition correctly implements the PR objective by only showing the locked banner when:
- Others may edit (
othersMayEdit) - Updates are not allowed (
!allowUpdate) - Not locked by owner (
!isLockedByOwner) - User is in the same organization (
!isAnnotationFromDifferentOrganization)
1021-1021: LGTM: Clean Redux state mapping
The new state mapping follows the existing pattern and properly connects the organization check to the component.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
26-26: Enhance the changelog entry with more details.While the current entry is accurate, consider expanding it to provide more context and reference the fixed issue:
-A "Locked by anonymous user" banner is no longer shown when opening public editable annotations of other organizations. [#8273](https://github.com/scalableminds/webknossos/pull/8273) +A "Locked by anonymous user" banner is no longer incorrectly shown when viewing public editable annotations from organizations that the current user is not a member of. Fixes [#7934](https://github.com/scalableminds/webknossos/issues/7934). [#8273](https://github.com/scalableminds/webknossos/pull/8273)
| tracingStore, | ||
| owner, | ||
| contributors, | ||
| organization, |
There was a problem hiding this comment.
apparently the organization prop was already sent by the backend but the frontend discarded this.
-> I fixed this here.
Side note: But there might also be other fields the frontend drops in this function which might be useful to still have 🤔.
There was a problem hiding this comment.
But there might also be other fields the frontend drops in this function which might be useful to still have 🤔.
if the frontend doesn't use these properties, there's no need to add them, I think.
And do not show it when the annotation is from another organization
URL of deployed dev instance (used for testing):
Steps to test:
othersMayEditin the sharing settings.Issues:
(Please delete unneeded items, merge only when none are left open)