Open
Conversation
|
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a full Pages API: serializers, three view endpoints (list/create, detail, archive/unarchive), URL routes, OpenAPI tag and decorator, re-exports, extensive contract tests, and background task enqueuing with permission, validation, hierarchical, archive, and external_id-uniqueness behaviors. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as PageListCreateAPIEndpoint
participant DB as Database
participant Queue as Celery Queue
Client->>API: POST /workspaces/{slug}/projects/{project_id}/pages/ (payload)
API->>DB: Check existing page by external_id+external_source
DB-->>API: Conflict? / No conflict
alt conflict
API-->>Client: 409 Conflict (existing page id)
else no conflict
API->>DB: Create Page record and ProjectPage association (atomic)
DB-->>API: Created
API->>Queue: Enqueue page_transaction.delay(...) on commit
Queue-->>API: Task queued
API->>DB: Fetch annotated page (labels, projects)
DB-->>API: Annotated page data
API-->>Client: 201 Created (PageSerializer)
end
sequenceDiagram
actor Client
participant API as PageDetailAPIEndpoint
participant Perm as Permission Checker
participant DB as Database
Client->>API: DELETE /workspaces/{slug}/projects/{project_id}/pages/{page_id}/
API->>Perm: Verify owner or project admin
Perm-->>API: Allowed / Denied
alt Denied
API-->>Client: 403 Forbidden
else Allowed
API->>DB: Is page archived?
DB-->>API: Yes / No
alt Not archived
API-->>Client: 400 Bad Request (archive before deleting)
else Archived
API->>DB: Clear parent on child pages within workspace/project
DB-->>API: Parents cleared
API->>DB: Delete page and related favorites/recent visits
DB-->>API: Deleted
API-->>Client: 204 No Content
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
zimbatm
force-pushed
the
feat/v1-pages-crud
branch
from
c792e9d to
a43cc26
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Adds Pages CRUD plus archive/unarchive support to the v1 external API, along with contract tests to validate the new endpoints.
Changes:
- Introduce v1 API views for listing/creating pages, retrieving/updating/deleting a page, and archive/unarchive flows.
- Add v1 serializers for Page create/update/read payloads.
- Register new URLs and expand the contract test suite for Pages.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/plane/api/views/page.py | Implements v1 Pages list/create, detail (get/patch/delete), and archive/unarchive endpoints. |
| apps/api/plane/api/urls/page.py | Adds URL routes for the new Pages and archived-pages endpoints. |
| apps/api/plane/api/urls/init.py | Registers the new page URL patterns with the v1 API router. |
| apps/api/plane/api/serializers/page.py | Adds serializers for Page create/update and full read responses. |
| apps/api/plane/api/serializers/init.py | Exposes the new Page serializers via the serializers package. |
| apps/api/plane/api/views/init.py | Exposes the new Page API endpoints via the views package. |
| apps/api/plane/tests/contract/api/test_pages.py | Adds contract tests covering Pages CRUD and archive/unarchive behavior. |
![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: 4
🧹 Nitpick comments (1)
apps/api/plane/tests/contract/api/test_pages.py (1)
73-137: Add regression tests for three uncovered edge cases in the new endpoints.Current coverage is good, but it does not assert:
- create with
parentoutside current project is rejected,- update conflict when only
external_sourcechanges,- update with
description_html=""still triggers transaction semantics.These tests would lock in the intended contract and prevent silent regressions.
Also applies to: 292-306, 352-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/tests/contract/api/test_pages.py` around lines 73 - 137, Add three regression tests to the same test module to cover the missing edge cases: (1) test_create_page_parent_outside_project_rejected — attempt to create a Page with a parent that belongs to a different Project and assert the API rejects it (expect HTTP 400/403) and that no Page or ProjectPage was created (check Page.objects.count() and ProjectPage.filter(...).exists()); (2) test_update_external_source_conflict — create a Page with an external_id+external_source, then call the update endpoint changing only external_source and assert the API returns HTTP_409_CONFLICT and that the Page record was not partially updated (verify external_id and external_source remain consistent); (3) test_update_empty_description_triggers_transaction — perform an update that sets description_html = "" but triggers a validation error inside the update transaction and assert the response is an error (HTTP 400) and that no partial changes were persisted (check the Page record remains unchanged and related ProjectPage/owned_by are intact). Use the existing fixtures and helpers (api_key_client, get_page_url, Page, ProjectPage) and name the tests exactly as above so they are easy to locate.
🤖 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/views/page.py`:
- Around line 282-287: The current guard uses truthiness on
request.data.get("description_html") which skips updates when description_html
is set to an empty string; change the condition to check for key presence (e.g.,
if "description_html" in request.data) so page_transaction is invoked for
explicit clears too, passing
new_description_html=request.data.get("description_html", "<p></p>"),
old_description_html=page_description, page_id=pk to the page_transaction task.
- Around line 110-143: The create path allows attaching a parent from a
different project because it never verifies the parent page's scope; before
calling serializer.save in the PageCreateSerializer flow, if request.data
contains a "parent" PK, fetch that parent_id and verify
Page.objects.filter(pk=parent_id, workspace__slug=slug,
projects__id=project_id).exists(); if not, return a 400/409 response indicating
invalid parent scope. Ensure this check runs prior to serializer.save (and
before using Project/owned_by), referencing the PageCreateSerializer flow, the
existing Page.objects.filter(...) duplicate check, and the serializer.save call.
- Around line 261-277: The conflict check only runs when external_id changes,
missing cases where external_source alone changes; update the conditional around
the Page.objects.filter so it triggers when either external_id or
external_source in request.data differs from page (i.e., if
request.data.get("external_id") != page.external_id or
request.data.get("external_source") != page.external_source), then set local
variables external_id = request.data.get("external_id", page.external_id) and
external_source = request.data.get("external_source", page.external_source) and
run Page.objects.filter(workspace__slug=slug, projects__id=project_id,
external_source=external_source, external_id=external_id).exists() to detect
collisions and return the existing 409 response if true.
- Line 137: Replace unguarded model .get() calls with Django's get_object_or_404
to avoid raising DoesNotExist and returning 500s; specifically swap the
Project.objects.get(...) usage (e.g., the assignment project =
Project.objects.get(pk=project_id, workspace__slug=slug)) and the other .get()
usages at the noted locations (lines 227-232, 243-248, 303-308, 426-431,
469-474) to use get_object_or_404(Model, ...) instead and import
get_object_or_404 from django.shortcuts at the top of
apps/api/plane/api/views/page.py so missing records produce 404 responses.
---
Nitpick comments:
In `@apps/api/plane/tests/contract/api/test_pages.py`:
- Around line 73-137: Add three regression tests to the same test module to
cover the missing edge cases: (1)
test_create_page_parent_outside_project_rejected — attempt to create a Page with
a parent that belongs to a different Project and assert the API rejects it
(expect HTTP 400/403) and that no Page or ProjectPage was created (check
Page.objects.count() and ProjectPage.filter(...).exists()); (2)
test_update_external_source_conflict — create a Page with an
external_id+external_source, then call the update endpoint changing only
external_source and assert the API returns HTTP_409_CONFLICT and that the Page
record was not partially updated (verify external_id and external_source remain
consistent); (3) test_update_empty_description_triggers_transaction — perform an
update that sets description_html = "" but triggers a validation error inside
the update transaction and assert the response is an error (HTTP 400) and that
no partial changes were persisted (check the Page record remains unchanged and
related ProjectPage/owned_by are intact). Use the existing fixtures and helpers
(api_key_client, get_page_url, Page, ProjectPage) and name the tests exactly as
above so they are easy to locate.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/api/plane/api/serializers/__init__.pyapps/api/plane/api/serializers/page.pyapps/api/plane/api/urls/__init__.pyapps/api/plane/api/urls/page.pyapps/api/plane/api/views/__init__.pyapps/api/plane/api/views/page.pyapps/api/plane/settings/openapi.pyapps/api/plane/tests/contract/api/test_pages.py
- Add access filter (owned_by or public) to patch/delete/archive/unarchive to prevent operating on other users' private pages - Broaden external_id conflict check to also detect external_source changes - Return conflicting page ID (not current page) in conflict responses - Fix description_html truthiness check to use key presence so empty string updates still trigger page_transaction - Validate parent page belongs to the same project on create - Add page_docs OpenAPI decorator to all page endpoints - Add tests for external_source-only conflicts, cross-project parent rejection, and private page access enforcement
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/api/plane/api/views/page.py (1)
484-526: Archive should verify page is not already archived.Similar to unarchive, the
post()method for archiving doesn't verifypage.archived_atisNonebefore archiving. Re-archiving an already archived page would update thearchived_attimestamp, which could be confusing.Suggested fix
page = Page.objects.get( Q(owned_by=request.user) | Q(access=0), pk=page_id, workspace__slug=slug, projects__id=project_id, project_pages__deleted_at__isnull=True, ) + if page.archived_at is not None: + return Response( + {"error": "Page is already archived"}, + status=status.HTTP_400_BAD_REQUEST, + ) + # Only the owner or admin can archive🤖 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 484 - 526, After fetching the Page into the local variable page in the post() method, add an early guard that checks page.archived_at is None and if not returns a client error (e.g., Response({"error":"Page already archived"}, status=status.HTTP_400_BAD_REQUEST)) before removing UserFavorite or calling unarchive_archive_page_and_descendants; keep the existing permission check and only proceed to delete favorites and call unarchive_archive_page_and_descendants when page.archived_at is None so you don't overwrite the archived timestamp for an already-archived page.
🤖 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/views/page.py`:
- Around line 562-565: The check assumes page.parent exists; if the FK points to
a deleted row accessing page.parent.archived_at can raise an exception. Change
the logic in the block using page.parent_id/page.parent to first safely resolve
the parent (e.g., parent = getattr(page, "parent", None) or wrap access in a
try/except for the related-object DoesNotExist), then only check
parent.archived_at if parent is not None, and if so set page.parent = None and
call page.save(update_fields=["parent"]) as before.
- Around line 539-545: The delete() method currently uses Page.objects.get(...)
to fetch the page but does not verify it is archived; update the fetch or add a
guard so only archived pages can be unarchived: either add
archived_at__isnull=False to the Page.objects.get(...) call or immediately after
Page.objects.get(...) assert page.archived_at is not None and raise Http404 (or
the same error used by get_queryset()) if it is None; reference the delete()
method, Page.objects.get, get_queryset(), and the page.archived_at property when
making this change.
- Around line 266-278: The patch() method retrieves the Page via
Page.objects.get and only checks page.is_locked before returning a 400; you need
to also prevent updates to archived pages by checking the archived_at field (or
equivalent) on the Page instance. Update the patch handler in
apps/api/plane/api/views/page.py (the patch method that calls Page.objects.get
and checks page.is_locked) to add a conditional like "if page.archived_at is not
None" (or the model's archive flag) and return the same Response error/status
used for locked pages (or a clear "Page is archived" 400) so archived pages
cannot be patched.
- Around line 370-382: The permission check uses a hardcoded magic number
role=20; replace that with the ROLE enum constant by updating the
ProjectMember.objects.filter call (the block checking page.owned_by_id !=
request.user.id ... ProjectMember.objects.filter(..., role=20, ...).exists()) to
use role=ROLE.ADMIN.value so the check reads role=ROLE.ADMIN.value, keeping the
other filters (workspace__slug=slug, member=request.user, project_id=project_id,
is_active=True) unchanged.
---
Nitpick comments:
In `@apps/api/plane/api/views/page.py`:
- Around line 484-526: After fetching the Page into the local variable page in
the post() method, add an early guard that checks page.archived_at is None and
if not returns a client error (e.g., Response({"error":"Page already archived"},
status=status.HTTP_400_BAD_REQUEST)) before removing UserFavorite or calling
unarchive_archive_page_and_descendants; keep the existing permission check and
only proceed to delete favorites and call unarchive_archive_page_and_descendants
when page.archived_at is None so you don't overwrite the archived timestamp for
an already-archived page.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb1b436f-f432-4be8-bd0e-b4f97c30d86c
📒 Files selected for processing (4)
apps/api/plane/api/views/page.pyapps/api/plane/tests/contract/api/test_pages.pyapps/api/plane/utils/openapi/__init__.pyapps/api/plane/utils/openapi/decorators.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/tests/contract/api/test_pages.py
- Block patch on archived pages (consistent with other endpoints) - Replace magic number role=20 with ROLE.ADMIN.value in delete - Add archived_at guard to unarchive endpoint - Safely resolve parent in unarchive to handle deleted parent refs - Add tests for archived page update rejection and unarchive of non-archived page
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/views/page.py`:
- Around line 362-368: The current Page.objects.get call restricts results to
pages owned by request.user or public (access=0), preventing admins from loading
private pages; change the lookup to fetch the Page by scope only (use
Page.objects.get with pk=pk, workspace__slug=slug, projects__id=project_id,
project_pages__deleted_at__isnull=True) without the Q(owned_by...|access=0)
filter, then run the explicit permission check already present (e.g.,
is_project_admin or page-level permission check) to allow admins to act on
others' private pages; apply the same change to the other similar calls around
the blocks referenced (the occurrences at ~496-502 and ~545-551) so all
endpoints load by project scope first and enforce permissions afterwards.
- Around line 127-184: Wrap the duplicate-check + save + ProjectPage creation in
a DB transaction and serialize the project scope by locking the Project row (use
Project.objects.select_for_update(...) inside transaction.atomic()) so
concurrent requests cannot commit the same (external_source, external_id); call
serializer.save(...) and create ProjectPage inside that atomic block. Do not
call page_transaction.delay() directly—use transaction.on_commit(lambda:
page_transaction.delay(...)) so the Celery task is only enqueued after the DB
commit succeeds. Apply the same changes to the update path that also calls
page_transaction.delay() (the block around lines 307-341) to ensure identical
atomicity and on_commit scheduling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee9cd89e-1345-4a30-b858-ec9b900befaf
📒 Files selected for processing (2)
apps/api/plane/api/views/page.pyapps/api/plane/tests/contract/api/test_pages.py
Replace .exists() + .first() with a single .first() call to avoid hitting the database twice with the same filter.
- Wrap create and update paths in transaction.atomic() with select_for_update() on Project to serialize concurrent requests - Use transaction.on_commit() for page_transaction Celery tasks - Validate parent from serializer.validated_data instead of raw request.data; add cycle detection in update (self-parent and ancestor chain walk) - Remove access filter from delete/archive/unarchive page lookups so admins can act on private pages (explicit permission checks still enforce owner/admin authorization after fetch)
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
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.
Description
Adds full CRUD and archive/unarchive support for Pages in the v1 external API.
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
Expanded the test suite to cover the new API.
References
Closes #4108, closes #7319, closes #8598
Summary by CodeRabbit
New Features
Permissions
Documentation
Tests
Exports