[WEB-2043] chore: updated permissions for delete operation #5231
Conversation
WalkthroughThe recent changes bolster security across various API views by implementing stricter permission checks for deleting cycles, issues, modules, and pages. Users must now be either admins or creators to perform deletions, ensuring that only authorized individuals can modify project components. This enhances the overall integrity of the system while streamlining the access control logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant ProjectMember
participant WorkspaceMember
User->>API: Request delete action
API->>ProjectMember: Check user role and status
ProjectMember-->>API: Return role status
alt Authorized
API->>Database: Delete item
Database-->>API: Confirm deletion
API-->>User: Return success response
else Not Authorized
API-->>User: Return 403 Forbidden
end
User->>API: Request delete action on workspace
API->>WorkspaceMember: Check membership and role
WorkspaceMember-->>API: Return membership status
alt Authorized
API->>Database: Delete workspace
Database-->>API: Confirm deletion
API-->>User: Return success response
else Not Authorized
API-->>User: Return 403 Forbidden
end
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 as PR comments)
Additionally, you can add 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: 10
Outside diff range, codebase verification and nitpick comments (3)
apiserver/plane/api/views/cycle.py (1)
380-383: Improve error message clarity.The error message should specify that only admins or the creator can delete the cycle, not the issue.
- {"error": "Only admin or creator can delete the issue"}, + {"error": "Only admin or creator can delete the cycle"},apiserver/plane/app/views/cycle/base.py (2)
1056-1059: Improve error message clarity.The error message should specify that only admins or the owner can delete the cycle, not the view.
- {"error": "Only admin or owner can delete the view"}, + {"error": "Only admin or owner can delete the cycle"},
1043-1045: Ensure the cycle object retrieval handles non-existent cycles.The
destroymethod inapiserver/plane/app/views/cycle/base.pydoes not handle cases where the cycle does not exist, which can lead to unhandled exceptions. Consider adding exception handling forDoesNotExistexceptions to improve robustness.
apiserver/plane/app/views/cycle/base.py:1043-1045Analysis chain
Ensure the cycle object is retrieved correctly.
The retrieval of the
cycleobject appears to be correct, but ensure that it handles cases where the cycle does not exist.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the cycle object retrieval handles non-existent cycles. # Test: Search for the usage of `Cycle.objects.get` in the `destroy` method. rg --type python -A 5 $'Cycle.objects.get'Length of output: 72
Script:
#!/bin/bash # Description: Verify that the cycle object retrieval handles non-existent cycles. # Test: Search for the usage of `Cycle.objects.get` in the `destroy` method. rg --type py -A 5 'Cycle.objects.get'Length of output: 4031
Script:
#!/bin/bash # Description: Verify that the cycle object retrieval handles non-existent cycles by checking for exception handling. # Test: Search for exception handling in the `destroy` method in the target file. rg --type py -A 10 'def destroy' apiserver/plane/app/views/cycle/base.pyLength of output: 861
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- apiserver/plane/api/views/cycle.py (2 hunks)
- apiserver/plane/api/views/inbox.py (1 hunks)
- apiserver/plane/api/views/issue.py (2 hunks)
- apiserver/plane/api/views/module.py (2 hunks)
- apiserver/plane/app/views/cycle/base.py (2 hunks)
- apiserver/plane/app/views/inbox/base.py (1 hunks)
- apiserver/plane/app/views/issue/base.py (3 hunks)
- apiserver/plane/app/views/issue/draft.py (2 hunks)
- apiserver/plane/app/views/module/base.py (2 hunks)
- apiserver/plane/app/views/page/base.py (1 hunks)
- apiserver/plane/app/views/view/base.py (2 hunks)
Additional comments not posted (6)
apiserver/plane/app/views/issue/base.py (1)
621-632: Ensure consistent permission checks.The permission checks are duplicated. The first check verifies if the user is an admin, while the second check verifies if the user is an admin again but with different roles. Consolidate these checks into a single block to avoid redundancy and potential inconsistencies.
- if ProjectMember.objects.filter( - workspace__slug=slug, - member=request.user, - role=20, - project_id=project_id, - is_active=True, - ).exists(): - return Response( - {"error": "Only admin can perform this action"}, - status=status.HTTP_403_FORBIDDEN, - ) + if not ProjectMember.objects.filter( + workspace__slug=slug, + member=request.user, + role=20, + project_id=project_id, + is_active=True, + ).exists(): + return Response( + {"error": "Only admin can perform this action"}, + status=status.HTTP_403_FORBIDDEN, + )Likely invalid or redundant comment.
apiserver/plane/api/views/module.py (1)
269-282: Ensure correct permission check logic.The permission check logic ensures that only admins or the creator can delete the module. This enhances security by restricting deletion permissions to authorized users.
apiserver/plane/app/views/module/base.py (1)
741-755: Ensure correct permission check logic.The permission check logic ensures that only admins or the creator can delete the module. This enhances security by restricting deletion permissions to authorized users.
apiserver/plane/api/views/issue.py (1)
392-405: Ensure correct permission check logic.The permission check logic ensures that only admins or the creator can delete the issue. This enhances security by restricting deletion permissions to authorized users.
apiserver/plane/api/views/cycle.py (1)
367-369: Ensure the cycle object is retrieved correctly.The retrieval of the
cycleobject appears to be correct, but ensure that it handles cases where the cycle does not exist.apiserver/plane/app/views/cycle/base.py (1)
1046-1059: Correctness of permission check logic.The permission check logic correctly ensures that only admins or the owner can delete a cycle. However, the condition
and cycle.owned_by != request.usershould beor cycle.owned_by != request.userto ensure proper validation.- ).exists() - and cycle.owned_by != request.user + ).exists() + or cycle.owned_by != request.userLikely invalid or redundant comment.
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and page.owned_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or owner can delete the page"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
Ensure consistent permission checks.
The permission checks are duplicated. The first check verifies if the user is an admin or owner, while the second check verifies if the user is an admin or owner again but with different roles. Consolidate these checks into a single block to avoid redundancy and potential inconsistencies.
- if (
- ProjectMember.objects.filter(
- workspace__slug=slug,
- member=request.user,
- role__in=[15, 10, 5],
- project_id=project_id,
- is_active=True,
- ).exists()
- and page.owned_by != request.user
- ):
- return Response(
- {"error": "Only admin or owner can delete the page"},
- status=status.HTTP_403_FORBIDDEN,
- )
+ if not (
+ ProjectMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ role__in=[15, 10, 5],
+ project_id=project_id,
+ is_active=True,
+ ).exists()
+ or request.user.id == page.owned_by_id
+ ):
+ return Response(
+ {"error": "Only admin or owner can delete the page"},
+ status=status.HTTP_403_FORBIDDEN,
+ )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| and page.owned_by != request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or owner can delete the page"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if not ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| or request.user.id == page.owned_by_id | |
| ): | |
| return Response( | |
| {"error": "Only admin or owner can delete the page"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) |
apiserver/plane/api/views/cycle.py
Outdated
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and cycle.owned_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or creator can delete the issue"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
Correctness of permission check logic.
The permission check logic correctly ensures that only admins or the creator can delete a cycle. However, the condition and cycle.owned_by != request.user should be or cycle.owned_by != request.user to ensure proper validation.
- ).exists()
- and cycle.owned_by != request.user
+ ).exists()
+ or cycle.owned_by != request.userCommittable suggestion was skipped due to low confidence.
| issue = Issue.objects.filter( | ||
| workspace__slug=slug, project_id=project_id, pk=issue_id | ||
| ).delete() | ||
| ).first() | ||
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and issue.created_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or creator can delete the issue"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| issue.delete() |
There was a problem hiding this comment.
Ensure consistent permission checks.
The permission checks are duplicated. The first check verifies if the user is an admin or creator, while the second check verifies if the user is an admin or creator again but with different roles. Consolidate these checks into a single block to avoid redundancy and potential inconsistencies.
- issue = Issue.objects.filter(
- workspace__slug=slug, project_id=project_id, pk=issue_id
- ).first()
- if (
- ProjectMember.objects.filter(
- workspace__slug=slug,
- member=request.user,
- role__in=[15, 10, 5],
- project_id=project_id,
- is_active=True,
- ).exists()
- and issue.created_by != request.user
- ):
- return Response(
- {"error": "Only admin or creator can delete the issue"},
- status=status.HTTP_403_FORBIDDEN,
- )
- issue.delete()
+ issue = Issue.objects.filter(
+ workspace__slug=slug, project_id=project_id, pk=issue_id
+ ).first()
+ if not (
+ ProjectMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ role__in=[15, 10, 5],
+ project_id=project_id,
+ is_active=True,
+ ).exists()
+ or issue.created_by == request.user
+ ):
+ return Response(
+ {"error": "Only admin or creator can delete the issue"},
+ status=status.HTTP_403_FORBIDDEN,
+ )
+ issue.delete()Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| issue = Issue.objects.filter( | |
| workspace__slug=slug, project_id=project_id, pk=issue_id | |
| ).delete() | |
| ).first() | |
| if ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| and issue.created_by != request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| issue.delete() | |
| issue = Issue.objects.filter( | |
| workspace__slug=slug, project_id=project_id, pk=issue_id | |
| ).first() | |
| if not ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| or issue.created_by == request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| issue.delete() |
apiserver/plane/api/views/inbox.py
Outdated
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and issue.created_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or creator can delete the issue"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
Refactor permission checks for clarity and efficiency.
Refactor the permission checks to improve readability and efficiency.
- if (
- ProjectMember.objects.filter(
- workspace__slug=slug,
- member=request.user,
- role__in=[15, 10, 5],
- project_id=project_id,
- is_active=True,
- ).exists()
- and issue.created_by != request.user
- ):
+ if not (
+ ProjectMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ role__in=[15, 10, 5],
+ project_id=project_id,
+ is_active=True,
+ ).exists()
+ or issue.created_by == request.user
+ ):
return Response(
{"error": "Only admin or creator can delete the issue"},
status=status.HTTP_403_FORBIDDEN,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| and issue.created_by != request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if not ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| or issue.created_by == request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) |
| issue = Issue.objects.filter( | ||
| workspace__slug=slug, project_id=project_id, pk=issue_id |
There was a problem hiding this comment.
Ensure the issue exists before proceeding with deletion.
Add a check to ensure the issue exists before proceeding with the deletion logic to avoid potential errors.
+ issue = Issue.objects.filter(
+ workspace__slug=slug, project_id=project_id, pk=issue_id
+ ).first()
+ if not issue:
+ return Response(
+ {"error": "Issue not found"},
+ status=status.HTTP_404_NOT_FOUND,
+ )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| issue = Issue.objects.filter( | |
| workspace__slug=slug, project_id=project_id, pk=issue_id | |
| issue = Issue.objects.filter( | |
| workspace__slug=slug, project_id=project_id, pk=issue_id | |
| ).first() | |
| if not issue: | |
| return Response( | |
| {"error": "Issue not found"}, | |
| status=status.HTTP_404_NOT_FOUND, | |
| ) |
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and issue.created_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or creator can delete the issue"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
Refactor permission checks for clarity and efficiency.
Refactor the permission checks to improve readability and efficiency.
- if (
- ProjectMember.objects.filter(
- workspace__slug=slug,
- member=request.user,
- role__in=[15, 10, 5],
- project_id=project_id,
- is_active=True,
- ).exists()
- and issue.created_by != request.user
- ):
+ if not (
+ ProjectMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ role__in=[15, 10, 5],
+ project_id=project_id,
+ is_active=True,
+ ).exists()
+ or issue.created_by == request.user
+ ):
return Response(
{"error": "Only admin or creator can delete the issue"},
status=status.HTTP_403_FORBIDDEN,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| and issue.created_by != request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if not ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| or issue.created_by == request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) |
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and issue.created_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or creator can delete the issue"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Ensure consistent permission checks.
The permission checks are duplicated. The first check verifies if the user is an admin or creator, while the second check verifies if the user is an admin or creator again but with different roles. Consolidate these checks into a single block to avoid redundancy and potential inconsistencies.
- if (
- ProjectMember.objects.filter(
- workspace__slug=slug,
- member=request.user,
- role__in=[15, 10, 5],
- project_id=project_id,
- is_active=True,
- ).exists()
- and issue.created_by != request.user
- ):
- return Response(
- {"error": "Only admin or creator can delete the issue"},
- status=status.HTTP_403_FORBIDDEN,
- )
+ if not (
+ ProjectMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ role__in=[15, 10, 5],
+ project_id=project_id,
+ is_active=True,
+ ).exists()
+ or issue.created_by == request.user
+ ):
+ return Response(
+ {"error": "Only admin or creator can delete the issue"},
+ status=status.HTTP_403_FORBIDDEN,
+ )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| and issue.created_by != request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if not ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| project_id=project_id, | |
| is_active=True, | |
| ).exists() | |
| or issue.created_by == request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or creator can delete the issue"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) |
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| project_id=project_id, | ||
| is_active=True, | ||
| ).exists() | ||
| and issue.created_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "Only admin or creator can delete the issue"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
Ensure the issue exists before proceeding with deletion.
Add a check to ensure the issue exists before proceeding with the deletion logic to avoid potential errors.
+ issue = Issue.objects.filter(
+ workspace__slug=slug, project_id=project_id, pk=pk
+ ).first()
+ if not issue:
+ return Response(
+ {"error": "Issue not found"},
+ status=status.HTTP_404_NOT_FOUND,
+ )Committable suggestion was skipped due to low confidence.
| if ( | ||
| ProjectMember.objects.filter( | ||
| workspace__slug=slug, | ||
| project_id=project_id, | ||
| member=request.user, | ||
| role=20, | ||
| is_active=True, | ||
| ).exists() | ||
| or project_view.owned_by == request.user | ||
| ): |
There was a problem hiding this comment.
Refactor permission checks for clarity and efficiency.
Refactor the permission checks to improve readability and efficiency.
- if (
- ProjectMember.objects.filter(
- workspace__slug=slug,
- project_id=project_id,
- member=request.user,
- role=20,
- is_active=True,
- ).exists()
- or project_view.owned_by == request.user
- ):
+ if not (
+ ProjectMember.objects.filter(
+ workspace__slug=slug,
+ project_id=project_id,
+ member=request.user,
+ role=20,
+ is_active=True,
+ ).exists()
+ or project_view.owned_by == request.user
+ ):
return Response(
{"error": "Only admin or owner can delete the view"},
status=status.HTTP_400_BAD_REQUEST,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| project_id=project_id, | |
| member=request.user, | |
| role=20, | |
| is_active=True, | |
| ).exists() | |
| or project_view.owned_by == request.user | |
| ): | |
| if not ( | |
| ProjectMember.objects.filter( | |
| workspace__slug=slug, | |
| project_id=project_id, | |
| member=request.user, | |
| role=20, | |
| is_active=True, | |
| ).exists() | |
| or project_view.owned_by == request.user | |
| ): | |
| return Response( | |
| {"error": "Only admin or owner can delete the view"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) |
| if ( | ||
| WorkspaceMember.objects.filter( | ||
| workspace__slug=slug, | ||
| member=request.user, | ||
| role__in=[15, 10, 5], | ||
| is_active=True, | ||
| ).exists() | ||
| and workspace_view.owned_by != request.user | ||
| ): | ||
| return Response( | ||
| {"error": "You do not have permission to delete this view"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) |
There was a problem hiding this comment.
Refactor permission checks for clarity and efficiency.
Refactor the permission checks to improve readability and efficiency.
- if (
- WorkspaceMember.objects.filter(
- workspace__slug=slug,
- member=request.user,
- role__in=[15, 10, 5],
- is_active=True,
- ).exists()
- and workspace_view.owned_by != request.user
- ):
+ if not (
+ WorkspaceMember.objects.filter(
+ workspace__slug=slug,
+ member=request.user,
+ role__in=[15, 10, 5],
+ is_active=True,
+ ).exists()
+ or workspace_view.owned_by == request.user
+ ):
return Response(
{"error": "You do not have permission to delete this view"},
status=status.HTTP_403_FORBIDDEN,
)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| WorkspaceMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| is_active=True, | |
| ).exists() | |
| and workspace_view.owned_by != request.user | |
| ): | |
| return Response( | |
| {"error": "You do not have permission to delete this view"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| if not ( | |
| WorkspaceMember.objects.filter( | |
| workspace__slug=slug, | |
| member=request.user, | |
| role__in=[15, 10, 5], | |
| is_active=True, | |
| ).exists() | |
| or workspace_view.owned_by == request.user | |
| ): | |
| return Response( | |
| {"error": "You do not have permission to delete this view"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- apiserver/plane/api/views/cycle.py (2 hunks)
- apiserver/plane/api/views/inbox.py (1 hunks)
- apiserver/plane/api/views/issue.py (2 hunks)
- apiserver/plane/api/views/module.py (2 hunks)
- apiserver/plane/app/views/cycle/base.py (2 hunks)
- apiserver/plane/app/views/inbox/base.py (1 hunks)
- apiserver/plane/app/views/issue/base.py (3 hunks)
- apiserver/plane/app/views/issue/draft.py (2 hunks)
- apiserver/plane/app/views/module/base.py (2 hunks)
- apiserver/plane/app/views/page/base.py (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- apiserver/plane/api/views/cycle.py
- apiserver/plane/api/views/inbox.py
- apiserver/plane/api/views/issue.py
- apiserver/plane/api/views/module.py
- apiserver/plane/app/views/inbox/base.py
- apiserver/plane/app/views/issue/base.py
- apiserver/plane/app/views/issue/draft.py
- apiserver/plane/app/views/module/base.py
- apiserver/plane/app/views/page/base.py
Additional comments not posted (4)
apiserver/plane/app/views/cycle/base.py (4)
50-50: Import statement update looks good.The import statement for
ProjectMemberis necessary for the new permission logic.
1046-1058: Permission check logic looks good.The permission check ensures that only the owner or an admin can delete the cycle. This enhances security.
1059-1065: Asynchronous task for issue activity looks good.The
issue_activity.delaycall ensures that the activity is logged asynchronously, which is good for performance.
1066-1067: Cycle deletion logic looks good.The cycle deletion is straightforward and follows the permission check.
| cycle = Cycle.objects.get( | ||
| workspace__slug=slug, project_id=project_id, pk=pk | ||
| ) |
There was a problem hiding this comment.
Ensure proper exception handling for Cycle.objects.get.
Using get without exception handling can raise Cycle.DoesNotExist if the cycle is not found. It's better to handle this exception.
- cycle = Cycle.objects.get(
+ try:
+ cycle = Cycle.objects.get(
+ workspace__slug=slug, project_id=project_id, pk=pk
+ )
+ except Cycle.DoesNotExist:
+ return Response(
+ {"error": "Cycle not found"},
+ status=status.HTTP_404_NOT_FOUND,
+ )Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cycle = Cycle.objects.get( | |
| workspace__slug=slug, project_id=project_id, pk=pk | |
| ) | |
| try: | |
| cycle = Cycle.objects.get( | |
| workspace__slug=slug, project_id=project_id, pk=pk | |
| ) | |
| except Cycle.DoesNotExist: | |
| return Response( | |
| {"error": "Cycle not found"}, | |
| status=status.HTTP_404_NOT_FOUND, | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- apiserver/plane/app/views/view/base.py (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- apiserver/plane/app/views/view/base.py
pushya22
changed the title
chore:
issue,cycle,module,view,pagecan perform the delete operation.Issue Link: WEB-2043
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
ProjectMemberandWorkspaceMemberfor permission checks across various views.Refactor