chore: updating the workspace slug when we the delete the workspace#6140
chore: updating the workspace slug when we the delete the workspace#6140gurusainath wants to merge 2 commits intopreviewfrom
Conversation
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)apiserver/plane/app/views/workspace/base.py (3)
The addition of parentheses around the unpacked variable makes it clearer that we're expecting a single value from
The try-except block is the correct way to handle potential
Add transaction management and improve slug handling. Several improvements needed for robustness:
This issue was previously identified. Please refer to the existing solution that includes transaction management and better slug handling: +from django.db import transaction
+from plane.utils.constants import RESTRICTED_WORKSPACE_SLUGS
- # Trash the workspace by appending the epoch and `trash` to the slug
- epoch = int(timezone.now().timestamp())
- updated_workspace_slug = f"trash-{epoch}-{workspace.slug}"
- if len(updated_workspace_slug) > 48:
- updated_workspace_slug = updated_workspace_slug[:48]
-
- workspace.slug = updated_workspace_slug
- workspace.save()
- workspace.delete()
+ with transaction.atomic():
+ epoch = int(timezone.now().timestamp())
+ base_slug = workspace.slug[:20] # Preserve start of original slug
+ updated_workspace_slug = f"trash-{epoch}-{base_slug}"
+
+ # Ensure the new slug isn't in restricted list
+ if updated_workspace_slug in RESTRICTED_WORKSPACE_SLUGS:
+ updated_workspace_slug = f"deleted-{updated_workspace_slug}"
+
+ workspace.slug = updated_workspace_slug[:48]
+ workspace.save()
+ workspace.delete()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: 2
🧹 Outside diff range and nitpick comments (1)
apiserver/plane/app/views/workspace/base.py (1)
148-164: Consider adding audit logging for workspace deletion.For better traceability, consider logging the workspace deletion event with relevant details like the original slug, actor, timestamp, etc.
Example implementation:
from django.contrib.admin.models import LogEntry, DELETION from django.contrib.contenttypes.models import ContentType def destroy(self, request, slug): # ... existing code ... # Add audit log LogEntry.objects.log_action( user_id=request.user.id, content_type_id=ContentType.objects.get_for_model(workspace).pk, object_id=workspace.id, object_repr=str(workspace), action_flag=DELETION, change_message=f"Workspace deleted. Original slug: {original_slug}" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
apiserver/plane/app/views/workspace/base.py(3 hunks)
🔇 Additional comments (2)
apiserver/plane/app/views/workspace/base.py (2)
85-90: LGTM! Clearer unpacking syntax.
The parentheses make it more explicit that we're unpacking a single value from the configuration.
Line range hint 134-147: LGTM! Good security practice with role-based access control.
The permission decorators properly restrict access:
- List endpoint accessible to all workspace roles (admin, member, guest)
- Partial update endpoint restricted to admin role only
| workspace = Workspace.objects.get(slug=slug) | ||
| if workspace is None: | ||
| return Response( | ||
| {"error": "Workspace not found"}, status=status.HTTP_404_NOT_FOUND | ||
| ) |
There was a problem hiding this comment.
Fix the workspace existence check logic.
The current implementation is incorrect:
Workspace.objects.get()raisesDoesNotExistexception if not found- The
Nonecheck will never execute
Replace with:
- workspace = Workspace.objects.get(slug=slug)
- if workspace is None:
- return Response(
- {"error": "Workspace not found"}, status=status.HTTP_404_NOT_FOUND
- )
+ try:
+ workspace = Workspace.objects.get(slug=slug)
+ except Workspace.DoesNotExist:
+ return Response(
+ {"error": "Workspace 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.
| workspace = Workspace.objects.get(slug=slug) | |
| if workspace is None: | |
| return Response( | |
| {"error": "Workspace not found"}, status=status.HTTP_404_NOT_FOUND | |
| ) | |
| try: | |
| workspace = Workspace.objects.get(slug=slug) | |
| except Workspace.DoesNotExist: | |
| return Response( | |
| {"error": "Workspace not found"}, status=status.HTTP_404_NOT_FOUND | |
| ) |
| # Trash the workspace by appending the epoch and `trash` to the slug | ||
| epoch = int(timezone.now().timestamp()) | ||
| updated_workspace_slug = f"trash-{epoch}-{workspace.slug}" | ||
| if len(updated_workspace_slug) > 48: | ||
| updated_workspace_slug = updated_workspace_slug[:48] | ||
|
|
||
| workspace.slug = updated_workspace_slug | ||
| workspace.save() | ||
| workspace.delete() |
There was a problem hiding this comment.
Add transaction management and improve slug handling.
Several improvements needed for robustness:
- The slug truncation might affect uniqueness by cutting off the epoch
- Operations should be atomic
- Should validate against restricted slugs
Consider this implementation:
+from django.db import transaction
+from plane.utils.constants import RESTRICTED_WORKSPACE_SLUGS
- # Trash the workspace by appending the epoch and `trash` to the slug
- epoch = int(timezone.now().timestamp())
- updated_workspace_slug = f"trash-{epoch}-{workspace.slug}"
- if len(updated_workspace_slug) > 48:
- updated_workspace_slug = updated_workspace_slug[:48]
-
- workspace.slug = updated_workspace_slug
- workspace.save()
- workspace.delete()
+ with transaction.atomic():
+ epoch = int(timezone.now().timestamp())
+ base_slug = workspace.slug[:20] # Preserve start of original slug
+ updated_workspace_slug = f"trash-{epoch}-{base_slug}"
+
+ # Ensure the new slug isn't in restricted list
+ if updated_workspace_slug in RESTRICTED_WORKSPACE_SLUGS:
+ updated_workspace_slug = f"deleted-{updated_workspace_slug}"
+
+ workspace.slug = updated_workspace_slug[:48]
+ workspace.save()
+ workspace.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.
| # Trash the workspace by appending the epoch and `trash` to the slug | |
| epoch = int(timezone.now().timestamp()) | |
| updated_workspace_slug = f"trash-{epoch}-{workspace.slug}" | |
| if len(updated_workspace_slug) > 48: | |
| updated_workspace_slug = updated_workspace_slug[:48] | |
| workspace.slug = updated_workspace_slug | |
| workspace.save() | |
| workspace.delete() | |
| from django.db import transaction | |
| from plane.utils.constants import RESTRICTED_WORKSPACE_SLUGS | |
| with transaction.atomic(): | |
| epoch = int(timezone.now().timestamp()) | |
| base_slug = workspace.slug[:20] # Preserve start of original slug | |
| updated_workspace_slug = f"trash-{epoch}-{base_slug}" | |
| # Ensure the new slug isn't in restricted list | |
| if updated_workspace_slug in RESTRICTED_WORKSPACE_SLUGS: | |
| updated_workspace_slug = f"deleted-{updated_workspace_slug}" | |
| workspace.slug = updated_workspace_slug[:48] | |
| workspace.save() | |
| workspace.delete() |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes