feat: add api/v1 pages endpoint#8649
Closed
Vierkandt wants to merge 3 commits intomakeplane:previewfrom
Closed
Conversation
|
|
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds page management: a new DRF PageSerializer with label/project handling, four page API endpoints (list/create, detail/patch/delete, archive/unarchive, lock/unlock), URL routing for page routes, export re-exports, and a .gitignore entry for Changes
Sequence DiagramsequenceDiagram
actor Client
participant Endpoint as "PageListCreateAPIEndpoint"
participant Serializer as "PageSerializer"
participant Project as "Project (DB)"
participant PageModel as "Page (DB)"
participant ProjectPageRel as "ProjectPage (DB)"
participant PageLabelRel as "PageLabel (DB)"
Client->>Endpoint: POST /workspaces/{slug}/projects/{id}/pages/ (data + labels)
Endpoint->>Serializer: validate(data)
Serializer->>Project: fetch(project_id from context)
Serializer->>PageModel: create(page with owned_by, workspace)
Serializer->>ProjectPageRel: create(project-page link)
alt labels provided
Serializer->>PageLabelRel: bulk create page-label links
end
Serializer-->>Endpoint: return serialized page
Endpoint-->>Client: 201 Created
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
apps/api/plane/api/views/page.py (3)
32-58: Duplicateget_querysetimplementations inPageListCreateAPIEndpointandPageDetailAPIEndpoint.These two methods (lines 32–58 and 166–192) are identical. Consider extracting a shared mixin or base class to avoid the duplication and simplify future maintenance.
♻️ Suggested approach
class PageQuerySetMixin: def get_queryset(self): return ( Page.objects.filter( workspace__slug=self.kwargs.get("slug"), projects__id=self.kwargs.get("project_id"), project_pages__deleted_at__isnull=True, ) .annotate( label_ids=Coalesce( ArrayAgg( "page_labels__label_id", distinct=True, filter=~Q(page_labels__label_id__isnull=True), ), Value([], output_field=ArrayField(UUIDField())), ), project_ids=Coalesce( ArrayAgg( "projects__id", distinct=True, filter=~Q(projects__id__isnull=True), ), Value([], output_field=ArrayField(UUIDField())), ), ) .distinct() ) # Then: class PageListCreateAPIEndpoint(PageQuerySetMixin, BaseAPIView): ... class PageDetailAPIEndpoint(PageQuerySetMixin, BaseAPIView): ...Also applies to: 166-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/page.py` around lines 32 - 58, Extract the duplicated get_queryset implementation into a shared mixin (e.g., PageQuerySetMixin) that defines the existing filtering and annotate logic, then have PageListCreateAPIEndpoint and PageDetailAPIEndpoint inherit from that mixin and remove the duplicate get_queryset methods from both classes; ensure the mixin is placed before the endpoint base class in the inheritance list so its get_queryset is used, and keep all original query logic (filters, Coalesce/ArrayAgg annotations and .distinct()) unchanged when moving it.
394-404: Lock/unlockpage.save()saves all fields unnecessarily.Use
update_fields=["is_locked"]to limit the write to a single column, avoiding potential race conditions with concurrent updates to other page fields and reducing I/O.♻️ Proposed fix
page.is_locked = True - page.save() + page.save(update_fields=["is_locked"])page.is_locked = False - page.save() + page.save(update_fields=["is_locked"])Also applies to: 411-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/page.py` around lines 394 - 404, The current code sets page.is_locked = True (and elsewhere sets False) then calls page.save() which writes all fields; change these to page.save(update_fields=["is_locked"]) to only persist the is_locked column and avoid stomping concurrent updates—update the save call in the block where page.is_locked is set to True and in the similar unlock block (the other page.is_locked assignment further down) to use update_fields=["is_locked"] instead of a plain save().
274-300: Cleanup of favorites and recent visits happens afterpage.delete()— these records may already be cascade-deleted or the identifiers may no longer resolve.Lines 286–299 delete
UserFavoriteandUserRecentVisitrecords after the page has been deleted on line 283. If these models have a FK to Page withCASCADE, the records are already gone. If they reference by UUID (entity_identifier), this is fine but the ordering is misleading. Consider moving cleanup beforepage.delete()for clarity and safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/page.py` around lines 274 - 300, The cleanup for UserFavorite and UserRecentVisit should run before calling page.delete() to avoid relying on records that may have been cascade-removed or to make the intent explicit; move the UserFavorite.objects.filter(...entity_identifier=pk..., entity_type="page").delete() and UserRecentVisit.objects.filter(...entity_identifier=pk..., entity_name="page").delete(soft=False) so they execute after the parent-null update (Page.objects.filter(...).update(parent=None)) but before page.delete(); keep the same filters and delete(soft=False) call for recent visits to preserve behavior.apps/api/plane/api/serializers/page.py (1)
6-6: Unused import:Q.
Qis imported but never used in this serializer file.🧹 Proposed fix
-from django.db.models import Q🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/serializers/page.py` at line 6, Remove the unused import symbol Q from the serializer module (the line "from django.db.models import Q")—locate the import in the serializers page file and delete it to avoid unused-import lint errors; if any future logic needs complex queries, re-add Q where it's actually used (no other changes required to functions or classes in this file).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/plane/api/serializers/page.py`:
- Around line 14-18: The labels ListField currently uses
PrimaryKeyRelatedField(queryset=Label.objects.all()), allowing labels from any
workspace to be attached; update the serializer to scope label lookups to the
current workspace by filtering the queryset at validation time using the
serializer context (use context['project_id'] to derive the workspace) — either
replace the child field with a SlugRelatedField/PrimaryKeyRelatedField whose
queryset is filtered in __init__ or override validate()/create()/update() in the
serializer to verify each Label belongs to the derived workspace (raise
ValidationError for cross-workspace labels) so both create() and update()
enforce the same scoped validation.
- Around line 49-85: The create() method does multiple DB writes
(Page.objects.create, ProjectPage.objects.create, PageLabel.objects.bulk_create)
that must be executed atomically to avoid orphaned rows on partial failure; wrap
the sequence of operations in a single transaction.atomic() block (import
transaction from django.db) so that Page creation, ProjectPage creation, and
PageLabel bulk_create are rolled back together on error, keeping the rest of the
logic (labels handling, context usage for project_id and owned_by_id) the same.
- Around line 87-103: Wrap the label replacement in the PageSerializer.update
method inside a transaction: when labels is not None, perform
PageLabel.objects.filter(page=instance).delete() and
PageLabel.objects.bulk_create(...) inside a transaction.atomic() block so the
delete and bulk_create are atomic; reference the update method and PageLabel
model to locate the code and import Django's transaction if not already
imported.
In `@apps/api/plane/api/views/page.py`:
- Around line 322-334: The archive/unarchive permission check currently returns
HTTP 400 for unauthorized users; change the Response status to 403 Forbidden to
match the delete method behavior. Locate the two permission branches using
ProjectMember.objects.filter(..., role__lte=15) and the condition
request.user.id != page.owned_by_id in the archive and unarchive handlers and
update their Response(...) status from status.HTTP_400_BAD_REQUEST to
status.HTTP_403_FORBIDDEN so unauthorized attempts return 403.
- Around line 92-102: The code passes request.GET.get("access") directly into
queryset.filter(access=access) which can raise on invalid strings; parse and
validate the parameter before filtering: retrieve access via
request.GET.get("access"), attempt to cast to int, ensure the value is one of
the allowed choices (0 or 1), and only then call queryset.filter(access=access);
if casting/validation fails, either skip applying the access filter (safe
default) or return a 400 Bad Request—update the logic around the existing
queryset.filter(access=access) usage to use the validated integer and reference
the same queryset variable and request.GET("access") call site.
- Around line 343-345: Replace the two naive datetime.now() calls with a single
timezone-aware timestamp captured once using django.utils.timezone.now(), pass
its .date() to unarchive_archive_page_and_descendants(page_id, ...) to match the
archived_at DateField, and return that same date (e.g., date.isoformat() or
str(date)) in the Response so the DB value and API response are identical; key
symbols: unarchive_archive_page_and_descendants, archived_at, and the Response
return.
---
Nitpick comments:
In `@apps/api/plane/api/serializers/page.py`:
- Line 6: Remove the unused import symbol Q from the serializer module (the line
"from django.db.models import Q")—locate the import in the serializers page file
and delete it to avoid unused-import lint errors; if any future logic needs
complex queries, re-add Q where it's actually used (no other changes required to
functions or classes in this file).
In `@apps/api/plane/api/views/page.py`:
- Around line 32-58: Extract the duplicated get_queryset implementation into a
shared mixin (e.g., PageQuerySetMixin) that defines the existing filtering and
annotate logic, then have PageListCreateAPIEndpoint and PageDetailAPIEndpoint
inherit from that mixin and remove the duplicate get_queryset methods from both
classes; ensure the mixin is placed before the endpoint base class in the
inheritance list so its get_queryset is used, and keep all original query logic
(filters, Coalesce/ArrayAgg annotations and .distinct()) unchanged when moving
it.
- Around line 394-404: The current code sets page.is_locked = True (and
elsewhere sets False) then calls page.save() which writes all fields; change
these to page.save(update_fields=["is_locked"]) to only persist the is_locked
column and avoid stomping concurrent updates—update the save call in the block
where page.is_locked is set to True and in the similar unlock block (the other
page.is_locked assignment further down) to use update_fields=["is_locked"]
instead of a plain save().
- Around line 274-300: The cleanup for UserFavorite and UserRecentVisit should
run before calling page.delete() to avoid relying on records that may have been
cascade-removed or to make the intent explicit; move the
UserFavorite.objects.filter(...entity_identifier=pk...,
entity_type="page").delete() and
UserRecentVisit.objects.filter(...entity_identifier=pk...,
entity_name="page").delete(soft=False) so they execute after the parent-null
update (Page.objects.filter(...).update(parent=None)) but before page.delete();
keep the same filters and delete(soft=False) call for recent visits to preserve
behavior.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Implement four REST endpoint classes for pages under the api/v1 API-key-authenticated layer, mirroring the business logic of the existing app-layer PageViewSet: - PageListCreateAPIEndpoint — list (with archive/access/owned filters, external_id lookup, pagination) and create pages - PageDetailAPIEndpoint — retrieve, partial update (lock/access guards), and delete (archive-before-delete and owner/admin guard) - PageArchiveUnarchiveAPIEndpoint — archive/unarchive with recursive descendant update via unarchive_archive_page_and_descendants - PageLockUnlockAPIEndpoint — lock/unlock toggle Also adds PageSerializer (with label_ids/project_ids annotations and PageLabel bulk-create/update), URL patterns, and wires everything into the serializer, view, and URL __init__ exports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Vierkandt
force-pushed
the
feature/api-v1-pages
branch
from
e66e1de to
064bb40
Compare
- Remove unused Q import from serializer - Scope label queryset to workspace in serializer __init__ to prevent cross-workspace label assignment - Wrap create() and update() label operations in transaction.atomic() - Extract PageQuerySetMixin to eliminate duplicate get_queryset - Validate access query param before filtering (must be 0 or 1) - Fix archive/unarchive permission errors to return 403 instead of 400 - Replace datetime.now() with timezone.now().date() for archived_at, captured once to avoid double-call inconsistency - Move UserFavorite/UserRecentVisit cleanup before page.delete() - Use update_fields=["is_locked"] in lock/unlock to avoid full saves - Pass project_id context to patch serializer for label scoping Co-Authored-By: Edgar <e.zitha@hotmail.com>
Author
|
Closing to reopen with corrected commit authorship. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/api/plane/api/views/page.py (3)
401-402:page.save()withoutupdate_fieldsin lock/unlock — risks overwriting concurrent field changes.Calling
page.save()withoutupdate_fields=["is_locked"]issues a full-rowUPDATE. If another request modifies a different field between the fetch and this save, that change is silently overwritten.♻️ Proposed fix
page.is_locked = True - page.save() + page.save(update_fields=["is_locked"]) ... page.is_locked = False - page.save() + page.save(update_fields=["is_locked"])Also applies to: 418-419
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/page.py` around lines 401 - 402, The current lock/unlock logic sets page.is_locked and calls page.save(), which issues a full-row UPDATE and can overwrite concurrent changes; change both occurrences that set page.is_locked to persist only that field by using a targeted update (e.g., call save with update_fields=["is_locked"] or use Page.objects.filter(pk=page.pk).update(is_locked=...) instead) so only the is_locked column is modified and other concurrent field changes are preserved.
32-58:get_queryset()is duplicated verbatim acrossPageListCreateAPIEndpointandPageDetailAPIEndpoint.Both classes contain identical annotation logic. Extract it into a shared mixin or module-level helper to avoid divergence.
♻️ Suggested refactor
+def _page_queryset(slug, project_id): + return ( + Page.objects.filter( + workspace__slug=slug, + projects__id=project_id, + project_pages__deleted_at__isnull=True, + ) + .annotate( + label_ids=Coalesce( + ArrayAgg( + "page_labels__label_id", + distinct=True, + filter=~Q(page_labels__label_id__isnull=True), + ), + Value([], output_field=ArrayField(UUIDField())), + ), + project_ids=Coalesce( + ArrayAgg( + "projects__id", + distinct=True, + filter=~Q(projects__id__isnull=True), + ), + Value([], output_field=ArrayField(UUIDField())), + ), + ) + .distinct() + ) class PageListCreateAPIEndpoint(BaseAPIView): def get_queryset(self): - return ( - Page.objects.filter(...) - .annotate(...) - .distinct() - ) + return _page_queryset(self.kwargs["slug"], self.kwargs["project_id"]) class PageDetailAPIEndpoint(BaseAPIView): def get_queryset(self): - return ( - Page.objects.filter(...) - .annotate(...) - .distinct() - ) + return _page_queryset(self.kwargs["slug"], self.kwargs["project_id"])Also applies to: 166-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/page.py` around lines 32 - 58, get_queryset contains identical annotation/filter logic duplicated in PageListCreateAPIEndpoint and PageDetailAPIEndpoint; extract that logic into a shared helper or mixin and have both classes use it. Create either a module-level function (e.g., build_annotated_page_queryset(kwargs) that returns the annotated QuerySet using the same filters/Coalesce/ArrayAgg logic) or a PageQuerysetMixin that implements get_queryset with that logic, then replace the duplicated get_queryset implementations in PageListCreateAPIEndpoint and PageDetailAPIEndpoint to call the new helper or inherit the mixin so annotation/filtering is defined in one place.
248-301:page.delete()and its cleanup operations are not wrapped intransaction.atomic().If
page.delete()succeeds but the subsequentUserFavoriteorUserRecentVisitdeletion fails, those records become orphaned — users would continue to see the deleted page in their favorites or recent visits.Consider wrapping lines 276–299 in a
transaction.atomic()block, or at minimum ensuring the cleanup runs before hard-deleting the page row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/page.py` around lines 248 - 301, The delete flow for Page (Page.objects.get(...), page.delete()) currently performs the page.delete() before cleaning up UserFavorite and UserRecentVisit, and these operations are not wrapped in a DB transaction; wrap the deletion and its related cleanup in a single transaction.atomic() block (or move cleanup before the hard delete) so that Page.delete(), UserFavorite.objects.filter(...).delete(), and UserRecentVisit.objects.filter(...).delete(soft=False) succeed or roll back together; update the handler surrounding the Page.delete() call and the subsequent cleanup using transaction.atomic() and keep the permission checks (ProjectMember query / owned_by_id) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/api/plane/api/serializers/page.py`:
- Around line 14-18: The labels field uses
PrimaryKeyRelatedField(queryset=Label.objects.all()) which allows labels from
any workspace; update the serializer to scope that queryset to the current
project/workspace by filtering
Label.objects.filter(project_id=context["project_id"]) in the serializer's
__init__ (e.g., access self.context["project_id"] and set
self.fields["labels"].child.queryset accordingly), and as a belt-and-suspenders
measure also validate in create() and update() that each provided label belongs
to the same workspace (raise a ValidationError if any label.project_id !=
context["project_id"]).
- Around line 49-85: Wrap the multi-step DB writes in create() (the Page
creation, ProjectPage.objects.create, and PageLabel.objects.bulk_create) in a
single transaction.atomic() block so any failure rolls back all changes; update
the create method in the serializer to import and use
django.db.transaction.atomic and move Page.objects.create,
ProjectPage.objects.create, and the PageLabel bulk_create calls inside the
atomic context (including labels processing) so no orphaned records remain on
partial failure.
- Around line 87-103: The current update method deletes existing labels with
PageLabel.objects.filter(page=instance).delete() and then calls
PageLabel.objects.bulk_create(...), which can leave the page without labels if
bulk_create fails; wrap the delete + bulk_create sequence in a database
transaction so both operations are atomic: import transaction from django.db and
run the delete and bulk_create inside a with transaction.atomic(): block in the
update method (the block containing
PageLabel.objects.filter(page=instance).delete() and
PageLabel.objects.bulk_create(...)) so any bulk_create failure rolls back the
prior delete.
In `@apps/api/plane/api/views/page.py`:
- Around line 322-334: Change the authorization failure responses in the page
archive/unarchive handlers to use HTTP_403_FORBIDDEN instead of
HTTP_400_BAD_REQUEST and fix the unarchive message typo: locate the permission
checks inside the archive/unarchive methods (the block using
ProjectMember.objects.filter(..., role__lte=15) and comparing request.user.id to
page.owned_by_id) and replace the Response status from
status.HTTP_400_BAD_REQUEST to status.HTTP_403_FORBIDDEN and change any "un
archive" text to "unarchive" so the messages and status codes match the delete
handler's authorization behavior.
- Around line 343-345: Compute a single timezone-aware timestamp with
django.utils.timezone.now(), convert it to a date with .date(), and reuse that
value for both the call to unarchive_archive_page_and_descendants(page_id, ...)
and the API Response so DB and response match and the type fits the DateField;
i.e., replace the two datetime.now() calls with one tz-aware variable
(timezone.now()), derive date = tz_now.date(), pass date to
unarchive_archive_page_and_descendants and return str(date) (or
date.isoformat()) in Response.
- Around line 93-95: The raw request.GET.get("access") string is passed to
queryset.filter(access=access) which raises on non-integer input; change the
code to validate/convert the access value before filtering: retrieve access via
request.GET.get("access"), if not None attempt to cast to int in a try/except,
and on success call queryset.filter(access=converted_int); on ValueError raise a
DRF validation error (e.g. rest_framework.exceptions.ValidationError or
ParseError) with a clear message instead of letting the ValueError bubble up.
Refer to the access variable, request.GET.get("access"), and the
queryset.filter(access=...) call when making the change.
---
Nitpick comments:
In `@apps/api/plane/api/views/page.py`:
- Around line 401-402: The current lock/unlock logic sets page.is_locked and
calls page.save(), which issues a full-row UPDATE and can overwrite concurrent
changes; change both occurrences that set page.is_locked to persist only that
field by using a targeted update (e.g., call save with
update_fields=["is_locked"] or use
Page.objects.filter(pk=page.pk).update(is_locked=...) instead) so only the
is_locked column is modified and other concurrent field changes are preserved.
- Around line 32-58: get_queryset contains identical annotation/filter logic
duplicated in PageListCreateAPIEndpoint and PageDetailAPIEndpoint; extract that
logic into a shared helper or mixin and have both classes use it. Create either
a module-level function (e.g., build_annotated_page_queryset(kwargs) that
returns the annotated QuerySet using the same filters/Coalesce/ArrayAgg logic)
or a PageQuerysetMixin that implements get_queryset with that logic, then
replace the duplicated get_queryset implementations in PageListCreateAPIEndpoint
and PageDetailAPIEndpoint to call the new helper or inherit the mixin so
annotation/filtering is defined in one place.
- Around line 248-301: The delete flow for Page (Page.objects.get(...),
page.delete()) currently performs the page.delete() before cleaning up
UserFavorite and UserRecentVisit, and these operations are not wrapped in a DB
transaction; wrap the deletion and its related cleanup in a single
transaction.atomic() block (or move cleanup before the hard delete) so that
Page.delete(), UserFavorite.objects.filter(...).delete(), and
UserRecentVisit.objects.filter(...).delete(soft=False) succeed or roll back
together; update the handler surrounding the Page.delete() call and the
subsequent cleanup using transaction.atomic() and keep the permission checks
(ProjectMember query / owned_by_id) unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
api/v1(API-key-authenticated) layer, which previously had no page supportapi/v1conventions (naming, permissions, pagination,external_id/external_sourceupsert pattern)ProjectPagePermission,page_transactionbackground task, and theunarchive_archive_page_and_descendantshelper — no new business logic introducedEndpoints added
Files changed
apps/api/plane/api/serializers/page.pyPageSerializerwith label/project M2M handlingapps/api/plane/api/views/page.pyapps/api/plane/api/urls/page.pyapps/api/plane/api/serializers/__init__.pyPageSerializerapps/api/plane/api/views/__init__.pyapps/api/plane/api/urls/__init__.pyTest plan
GET /api/v1/workspaces/{slug}/projects/{pid}/pages/returns paginated list of public + owned pagesPOSTcreates a page andProjectPagejoin record; returns 409 on duplicateexternal_id/external_sourcePATCHis rejected on locked pages; access change rejected for non-ownersDELETEis rejected if page is not archived first🤖 Generated with Claude Code
Summary by CodeRabbit