[SILO-671] feat: add sticky external apis #8139
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
WalkthroughAdds a Sticky feature: a DRF serializer with HTML/binary validation, a Workspace-scoped viewset with full CRUD, URL registration under workspaces//stickies/, and OpenAPI examples plus a sticky_docs decorator. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant StickyViewSet
participant StickySerializer
participant StickyModel
Client->>Router: POST /workspaces/{slug}/stickies/
Router->>StickyViewSet: create(request, slug)
StickyViewSet->>StickySerializer: validate(data)
StickySerializer->>StickySerializer: validate_html_content / validate_binary_data
alt validation success
StickySerializer-->>StickyViewSet: sanitized data
StickyViewSet->>StickyModel: create(instance with workspace_id, owner_id)
StickyModel-->>StickyViewSet: instance
StickyViewSet->>StickySerializer: serialize(instance)
StickySerializer-->>Client: 201 + JSON
else validation error
StickySerializer-->>Client: 400 + errors
end
Client->>Router: GET /workspaces/{slug}/stickies/?description=...
Router->>StickyViewSet: list(request, slug)
StickyViewSet->>StickyModel: queryset filtered by workspace slug & owner (select_related)
StickyModel-->>StickyViewSet: list
StickyViewSet->>StickySerializer: serialize(page)
StickySerializer-->>Client: 200 + paginated JSON
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (3)
apps/api/plane/api/serializers/sticky.py (1)
1-30: StickySerializer validation is functionally sound; consider a few refinementsThe overall structure looks good: you’re sanitizing HTML, validating binary data, and preserving read-only
workspace/owner. A few small improvements to consider:
- Chain
super().validateto preserve base behaviorEven though
ModelSerializer.validatecurrently just returns attrs, it’s safer to keep the chain intact (and future‑proof against changes inBaseSerializer):- def validate(self, data): + def validate(self, data): + data = super().validate(data)
- Use field-specific error keys for HTML to match DRF error shapes
Right now HTML errors are returned under a generic
"error"key, while binary errors are properly scoped to"description_binary". For consistency with other serializers and easier client‑side mapping, consider:- if not is_valid: - raise serializers.ValidationError({"error": "html content is not valid"}) + if not is_valid: + raise serializers.ValidationError( + {"description_html": "Invalid HTML content"} + )
- (Optional) Surface or log the validator’s
error_msgYou’re currently discarding
error_msgfromvalidate_html_content/validate_binary_data. If you don’t want to expose detailed reasons to clients, it may still be useful to log them for debugging, or at least attach them to the message when safe.Overall, no blocking issues; these are consistency/robustness tweaks.
apps/api/plane/utils/openapi/decorators.py (1)
265-279: sticky_docs decorator matches existing OpenAPI decorator patternsThe defaults (tags, workspace slug parameter, and 401/403/404 responses) are consistent with the other *_docs helpers and work correctly with
_merge_schema_options, so per-endpoint overrides will behave as expected. The extra defaultsummaryis fine, though it will usually be overridden by call-site summaries.apps/api/plane/api/views/sticky.py (1)
45-51: Preferget_serializer(...)over directStickySerializer(...)for consistency and contextIn
create,list(the pagination lambda), andpartial_update, you instantiateStickySerializerdirectly. This works, but it bypassesBaseViewSet/DRF’sget_serializer, which typically injects useful context likerequest,view, andformat, and centralizes serializer selection.You can simplify and future‑proof these methods by leaning on
get_serializer:@@ def create(self, request, slug): - serializer = StickySerializer(data=request.data) + serializer = self.get_serializer(data=request.data) @@ def list(self, request, slug): - return self.paginate( - request=request, - queryset=(stickies), - on_results=lambda stickies: StickySerializer(stickies, many=True).data, - default_per_page=20, - ) + return self.paginate( + request=request, + queryset=stickies, + on_results=lambda stickies: self.get_serializer(stickies, many=True).data, + default_per_page=20, + ) @@ def partial_update(self, request, slug, pk): - serializer = StickySerializer(sticky, data=request.data, partial=True) + serializer = self.get_serializer(sticky, data=request.data, partial=True)This keeps behavior aligned with other viewsets and makes later serializer changes (e.g., swapping
StickySerializeror using mixins) less error‑prone.Also applies to: 63-74, 93-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/api/plane/api/serializers/__init__.py(1 hunks)apps/api/plane/api/serializers/sticky.py(1 hunks)apps/api/plane/api/urls/__init__.py(2 hunks)apps/api/plane/api/urls/sticky.py(1 hunks)apps/api/plane/api/views/__init__.py(1 hunks)apps/api/plane/api/views/sticky.py(1 hunks)apps/api/plane/utils/openapi/__init__.py(2 hunks)apps/api/plane/utils/openapi/decorators.py(1 hunks)apps/api/plane/utils/openapi/examples.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/api/plane/api/serializers/sticky.py (2)
apps/api/plane/api/serializers/base.py (1)
BaseSerializer(5-114)apps/api/plane/utils/content_validator.py (2)
validate_html_content(212-244)validate_binary_data(25-66)
apps/api/plane/api/urls/sticky.py (1)
apps/api/plane/api/views/sticky.py (1)
StickyViewSet(20-110)
apps/api/plane/api/views/__init__.py (1)
apps/api/plane/api/views/sticky.py (1)
StickyViewSet(20-110)
apps/api/plane/api/serializers/__init__.py (2)
apps/api/plane/api/serializers/member.py (1)
ProjectMemberSerializer(11-39)apps/api/plane/api/serializers/sticky.py (1)
StickySerializer(8-30)
apps/api/plane/api/views/sticky.py (5)
apps/api/plane/utils/permissions/workspace.py (1)
WorkspaceUserPermission(99-106)apps/api/plane/db/models/workspace.py (1)
Workspace(115-178)apps/api/plane/api/serializers/sticky.py (1)
StickySerializer(8-30)apps/api/plane/utils/openapi/decorators.py (1)
sticky_docs(266-279)apps/api/plane/utils/openapi/responses.py (1)
create_paginated_response(354-401)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
apps/api/plane/api/views/__init__.py (1)
57-59: ExposingWorkspaceInvitationsViewsetandStickyViewSetvia package init looks consistentThe new imports match the existing pattern of surfacing viewsets from this package; no issues from a wiring perspective. Please just ensure router/url modules are importing from here as intended after this change.
apps/api/plane/api/serializers/__init__.py (1)
57-58: Serializer exports forProjectMemberSerializerandStickySerializerare aligned with existing patternRe-exporting these serializers from the package init is consistent with how other serializers are surfaced and will support dynamic expansion/imports in
BaseSerializer.apps/api/plane/api/urls/__init__.py (1)
12-26: Sticky URL patterns are wired consistently with existing modulesImporting
sticky_patternsand splatting them intourlpatternsfollows the same pattern as other feature modules. This should correctly expose sticky endpoints without altering existing routes’ behavior.apps/api/plane/utils/openapi/__init__.py (1)
93-144: OpenAPI example wiring for Sticky is consistent with existing examplesAdding
STICKY_EXAMPLEto the examples import list and__all__aligns with how other example objects are exposed from this module. That should make it straightforward to reference insticky_docsand responses.Also applies to: 172-297
apps/api/plane/api/urls/sticky.py (1)
1-12: Workspace-scoped Sticky routing looks correctRegistering
StickyViewSeton theDefaultRouterand mounting it underworkspaces/<str:slug>/cleanly yields routes likeworkspaces/<slug>/stickies/andworkspaces/<slug>/stickies/<pk>/, matching the viewset’s use ofsluginget_querysetand method signatures.apps/api/plane/utils/openapi/examples.py (1)
675-683: Sticky OpenAPI examples and schema mapping are consistent
STICKY_EXAMPLE,SAMPLE_STICKY, and the"Sticky": SAMPLE_STICKYentry inSCHEMA_EXAMPLESline up with each other and with the use ofcreate_paginated_response(..., "Sticky", ...), so Sticky endpoints will have coherent single-item and paginated examples.Also applies to: 793-799, 814-815
apps/api/plane/api/views/sticky.py (1)
20-35: Queryset scoping for StickyViewSet is appropriateFiltering by
workspace__slugfromkwargs["slug"]andowner_id=self.request.user.id, plusselect_related("workspace", "owner"), gives you correctly scoped, efficient queries and pairs well withWorkspaceUserPermissionto avoid leaking stickies across workspaces or users.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/api/plane/api/views/sticky.py (1)
64-64: Consider using None as the default for the query parameter.Using
Falseas the default for the query parameter works but is unconventional. An empty string""orNonewould be more idiomatic for optional string parameters.- query = request.query_params.get("query", False) + query = request.query_params.get("query")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/api/views/sticky.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/api/views/sticky.py (6)
apps/api/plane/utils/permissions/workspace.py (1)
WorkspaceUserPermission(99-106)apps/api/plane/db/models/workspace.py (1)
Workspace(115-178)apps/api/plane/api/serializers/sticky.py (1)
StickySerializer(8-30)apps/api/plane/utils/openapi/decorators.py (1)
sticky_docs(266-279)apps/api/plane/utils/openapi/responses.py (1)
create_paginated_response(354-401)apps/api/plane/app/views/workspace/sticky.py (1)
WorkspaceStickyViewSet(12-60)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
apps/api/plane/api/views/sticky.py (7)
1-17: LGTM: Imports are well-organized.All necessary imports are present and properly structured for the viewset functionality and OpenAPI documentation.
20-24: LGTM: ViewSet configuration is appropriate.The class attributes are correctly configured with proper permission checking and read replica optimization.
26-34: LGTM: Query optimization and filtering are correct.The queryset properly filters by workspace and owner while using
select_relatedfor query optimization. The owner filtering ensures users can only access their own stickies.
82-84: LGTM: Retrieve method is correctly implemented.The method properly uses
self.get_object()which handles 404 responses and applies the queryset filtering for security.
93-99: LGTM: Partial update is correctly implemented.The method properly validates and applies partial updates, with security ensured through the queryset filtering by owner.
107-110: LGTM: Delete method is correctly implemented.The method properly deletes the sticky with security ensured through queryset filtering, and returns the appropriate 204 status code.
63-74: Verify the ordering field choice and document the design intent.The Sticky model has a
sort_orderfield and default ordering by"-created_at", yetWorkspaceStickyViewSetorders by"-sort_order"while the external API endpoint (line 65) orders by"-created_at". This inconsistency is confirmed and could be intentional (different UX for internal vs. external APIs) or an oversight. Please clarify the design intent and add a code comment explaining the choice if this difference is deliberate.
| def create(self, request, slug): | ||
| workspace = Workspace.objects.get(slug=slug) | ||
| serializer = StickySerializer(data=request.data) | ||
| if serializer.is_valid(): | ||
| serializer.save(workspace_id=workspace.id, owner_id=request.user.id) | ||
| return Response(serializer.data, status=status.HTTP_201_CREATED) | ||
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) |
There was a problem hiding this comment.
Handle missing workspace gracefully.
Line 46 uses Workspace.objects.get(slug=slug) which will raise an unhandled Workspace.DoesNotExist exception if the workspace is not found, resulting in a 500 error instead of a proper 404 response.
Apply this diff to handle the missing workspace gracefully:
+from django.shortcuts import get_object_or_404
+
def create(self, request, slug):
- workspace = Workspace.objects.get(slug=slug)
+ workspace = get_object_or_404(Workspace, slug=slug)
serializer = StickySerializer(data=request.data)📝 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.
| def create(self, request, slug): | |
| workspace = Workspace.objects.get(slug=slug) | |
| serializer = StickySerializer(data=request.data) | |
| if serializer.is_valid(): | |
| serializer.save(workspace_id=workspace.id, owner_id=request.user.id) | |
| return Response(serializer.data, status=status.HTTP_201_CREATED) | |
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | |
| def create(self, request, slug): | |
| workspace = get_object_or_404(Workspace, slug=slug) | |
| serializer = StickySerializer(data=request.data) | |
| if serializer.is_valid(): | |
| serializer.save(workspace_id=workspace.id, owner_id=request.user.id) | |
| return Response(serializer.data, status=status.HTTP_201_CREATED) | |
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) |
🤖 Prompt for AI Agents
In apps/api/plane/api/views/sticky.py around lines 45 to 51, the call
Workspace.objects.get(slug=slug) can raise Workspace.DoesNotExist and cause a
500; change this to handle missing workspace by either using
django.shortcuts.get_object_or_404(Workspace, slug=slug) or wrapping the get in
a try/except Workspace.DoesNotExist and returning Response({"detail": "Workspace
not found."}, status=status.HTTP_404_NOT_FOUND) before proceeding; ensure
imports are added (get_object_or_404) or the Workspace.DoesNotExist is caught
and the function exits after returning the 404 response.
Description
Type of Change
Summary by CodeRabbit