Conversation
|
|
📝 WalkthroughWalkthroughThis change introduces a complete CRUD API feature for Pages. It adds three serializers (PageCreateSerializer, PageUpdateSerializer, PageSerializer) for model operations, three API endpoint classes handling list/create, detail retrieval/update/deletion, and archive/unarchive workflows, five new URL routes, permission controls, background task integration, and comprehensive contract tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as PageListCreateAPIEndpoint
participant DB as Database
participant Celery as Background Task Queue
User->>API: POST /pages/ with name, description_html, color
API->>DB: Check for duplicate external_id/external_source
DB-->>API: No conflicts found
API->>DB: Create Page instance
DB-->>API: Page object returned
API->>DB: Create ProjectPage association
DB-->>API: Association created
API->>Celery: Enqueue page_transaction task
Celery-->>API: Task queued
API->>DB: Fetch page with annotations (labels, projects)
DB-->>API: Annotated page data
API-->>User: 201 Created with PageSerializer
sequenceDiagram
actor User
participant API as PageDetailAPIEndpoint
participant PermCheck as Permission Validator
participant DB as Database
User->>API: DELETE /pages/{page_id}/ (non-archived page)
API->>PermCheck: Verify user is owner or admin
PermCheck-->>API: Permission denied / granted
alt Not archived
API-->>User: 400 Bad Request (archived-before-delete)
else Insufficient permissions
API-->>User: 403 Forbidden
else Authorized & archived
API->>DB: Find all descendants
DB-->>API: List of child pages
API->>DB: Clear parent reference from children
DB-->>API: Updated
API->>DB: Delete page record
DB-->>API: Deleted
API->>DB: Remove UserFavorite entries
DB-->>API: Removed
API->>DB: Remove UserRecentVisit entries
DB-->>API: Removed
API-->>User: 204 No Content
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
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. |
| if ( | ||
| request.data.get("external_id") | ||
| and (page.external_id != request.data.get("external_id")) | ||
| and Page.objects.filter( | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| external_source=request.data.get("external_source", page.external_source), | ||
| external_id=request.data.get("external_id"), | ||
| ).exists() |
There was a problem hiding this comment.
The external-id conflict check in patch() only runs when external_id changes. Changing external_source while keeping the same external_id can still create a duplicate (external_source, external_id) pair but bypasses this check.
Trigger the conflict check when either external_id OR external_source changes (using the effective pair after defaults).
| if ( | |
| request.data.get("external_id") | |
| and (page.external_id != request.data.get("external_id")) | |
| and Page.objects.filter( | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| external_source=request.data.get("external_source", page.external_source), | |
| external_id=request.data.get("external_id"), | |
| ).exists() | |
| effective_external_id = request.data.get("external_id", page.external_id) | |
| effective_external_source = request.data.get("external_source", page.external_source) | |
| if ( | |
| effective_external_id | |
| and ( | |
| page.external_id != effective_external_id | |
| or page.external_source != effective_external_source | |
| ) | |
| and Page.objects.filter( | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| external_source=effective_external_source, | |
| external_id=effective_external_id, | |
| ) | |
| .exclude(pk=page.pk) | |
| .exists() |
| if ( | ||
| request.data.get("external_id") | ||
| and (page.external_id != request.data.get("external_id")) | ||
| and Page.objects.filter( | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| external_source=request.data.get("external_source", page.external_source), | ||
| external_id=request.data.get("external_id"), | ||
| ).exists() | ||
| ): | ||
| return Response( | ||
| { | ||
| "error": "Page with the same external id and external source already exists", | ||
| "id": str(page.id), | ||
| }, | ||
| status=status.HTTP_409_CONFLICT, | ||
| ) |
There was a problem hiding this comment.
On external-id conflict during patch(), the response returns "id": str(page.id) where page is the current page being updated. That doesn’t help callers find the existing conflicting page (and differs from the create endpoint behavior).
Return the id of the conflicting Page record found by the duplicate query instead.
| if ( | |
| request.data.get("external_id") | |
| and (page.external_id != request.data.get("external_id")) | |
| and Page.objects.filter( | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| external_source=request.data.get("external_source", page.external_source), | |
| external_id=request.data.get("external_id"), | |
| ).exists() | |
| ): | |
| return Response( | |
| { | |
| "error": "Page with the same external id and external source already exists", | |
| "id": str(page.id), | |
| }, | |
| status=status.HTTP_409_CONFLICT, | |
| ) | |
| if request.data.get("external_id") and (page.external_id != request.data.get("external_id")): | |
| conflicting_page = Page.objects.filter( | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| external_source=request.data.get("external_source", page.external_source), | |
| external_id=request.data.get("external_id"), | |
| ).first() | |
| if conflicting_page: | |
| return Response( | |
| { | |
| "error": "Page with the same external id and external source already exists", | |
| "id": str(conflicting_page.id), | |
| }, | |
| status=status.HTTP_409_CONFLICT, | |
| ) |
| serializer.save() | ||
|
|
||
| # Fire page transaction on description change | ||
| if request.data.get("description_html"): |
There was a problem hiding this comment.
page_transaction is only triggered when request.data.get("description_html") is truthy. If a client updates the description to an empty string, the description changes but the transaction task won’t run.
Check for field presence (e.g., 'description_html' in request.data) instead of truthiness so transactions fire on all description updates.
| if request.data.get("description_html"): | |
| if "description_html" in request.data: |
| page = Page.objects.get( | ||
| pk=page_id, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, | ||
| ) |
There was a problem hiding this comment.
PageArchiveUnarchiveAPIEndpoint.delete() retrieves the page without applying the access filter used by list/retrieve. This allows unarchiving another user’s private page by id.
Apply the same access filter (or explicitly block non-owners for private pages) before the owner/admin authorization logic.
| page = Page.objects.get( | |
| pk=page_id, | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| project_pages__deleted_at__isnull=True, | |
| ) | |
| try: | |
| page = self.get_queryset().get(pk=page_id) | |
| except Page.DoesNotExist: | |
| return Response(status=status.HTTP_404_NOT_FOUND) |
| def test_list_pages_excludes_private_pages_of_other_users(self, api_key_client, workspace, project, create_user): | ||
| """Test that private pages owned by other users are excluded""" | ||
| url = self.get_page_url(workspace.slug, project.id) | ||
|
|
||
| from plane.db.models import User |
There was a problem hiding this comment.
The suite verifies that private pages owned by other users are excluded from listing, but it doesn’t assert that non-owners also cannot PATCH/archive/unarchive/delete those private pages via v1.
Add negative tests ensuring these mutations are rejected and do not modify the page when access=1 and owned_by!=request.user.
|
|
||
| page = serializer.save( | ||
| workspace_id=project.workspace_id, | ||
| owned_by=request.user, | ||
| ) |
There was a problem hiding this comment.
PageListCreateAPIEndpoint.post() allows setting parent but doesn’t validate that the parent page belongs to the same workspace/project (and isn’t soft-deleted). This can create cross-project/cross-workspace hierarchies.
Validate parent against the same project/workspace constraints before saving (similar to the PATCH endpoint).
| class PageListCreateAPIEndpoint(BaseAPIView): | ||
| """Page List and Create Endpoint""" | ||
|
|
||
| serializer_class = PageSerializer | ||
| model = Page |
There was a problem hiding this comment.
All other v1 API views use @*_docs(...) decorators for OpenAPI (cycles/modules/issues/etc.), but this Pages view has none. This risks these endpoints being undocumented or inconsistent in the generated schema.
Add a page_docs decorator and annotate these methods (list/create/detail/archive/unarchive).
| page = Page.objects.get( | ||
| pk=pk, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, |
There was a problem hiding this comment.
PageDetailAPIEndpoint.delete() retrieves the page without applying the access filter used elsewhere (owned_by=request.user or access=0). This lets a project admin delete another user’s private page by id, even though private pages are hidden from list/retrieve.
Apply the same access filter (or explicitly forbid deletes on private pages by non-owners) before deletion.
| page = Page.objects.get( | ||
| pk=page_id, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, |
There was a problem hiding this comment.
PageArchiveUnarchiveAPIEndpoint.post() retrieves the page without applying the access filter used by list/retrieve. This allows archiving another user’s private page by id.
Apply the same access filter (or explicitly block non-owners for private pages) before the owner/admin authorization logic.
| page = Page.objects.get( | ||
| pk=pk, | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| project_pages__deleted_at__isnull=True, | ||
| ) |
There was a problem hiding this comment.
PageDetailAPIEndpoint.patch() fetches the page with Page.objects.get(...) without the access filter used by get_queryset() (owned_by=request.user or access=0). A project member can PATCH a private page they don’t own (update succeeds, then response re-fetch can 404), which is an authorization bypass.
Fetch via an access-filtered queryset (or explicitly block non-owners when access=PRIVATE) before saving.
| page = Page.objects.get( | |
| pk=pk, | |
| workspace__slug=slug, | |
| projects__id=project_id, | |
| project_pages__deleted_at__isnull=True, | |
| ) | |
| page = self.get_queryset().get(pk=pk) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| serializer = PageCreateSerializer(data=request.data) | ||
| if serializer.is_valid(): | ||
| # Check for duplicate external_id + external_source | ||
| if ( | ||
| request.data.get("external_id") | ||
| and request.data.get("external_source") | ||
| and Page.objects.filter( | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| external_source=request.data.get("external_source"), | ||
| external_id=request.data.get("external_id"), | ||
| ).exists() | ||
| ): | ||
| page = Page.objects.filter( | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| external_source=request.data.get("external_source"), | ||
| external_id=request.data.get("external_id"), | ||
| ).first() | ||
| return Response( | ||
| { | ||
| "error": "Page with the same external id and external source already exists", | ||
| "id": str(page.id), | ||
| }, | ||
| status=status.HTTP_409_CONFLICT, | ||
| ) | ||
|
|
||
| project = Project.objects.get(pk=project_id, workspace__slug=slug) | ||
|
|
||
| page = serializer.save( | ||
| workspace_id=project.workspace_id, | ||
| owned_by=request.user, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Validate parent scope during create to prevent cross-project hierarchy links.
PageCreateSerializer validates that the parent PK exists, but Line 110-143 does not validate that the parent belongs to the same slug/project_id. That allows linking a newly created page to a parent outside the current project context.
Suggested fix
serializer = PageCreateSerializer(data=request.data)
if serializer.is_valid():
+ parent = serializer.validated_data.get("parent")
+ if parent and not Page.objects.filter(
+ pk=parent.id,
+ workspace__slug=slug,
+ projects__id=project_id,
+ project_pages__deleted_at__isnull=True,
+ ).exists():
+ return Response(
+ {"error": "Parent page must belong to the same project"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
# Check for duplicate external_id + external_source
if (🤖 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 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.
| status=status.HTTP_409_CONFLICT, | ||
| ) | ||
|
|
||
| project = Project.objects.get(pk=project_id, workspace__slug=slug) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/api/plane/api/views/page.pyRepository: makeplane/plane
Length of output: 95
🏁 Script executed:
sed -n '130,145p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 744
🏁 Script executed:
sed -n '220,255p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 1556
🏁 Script executed:
sed -n '295,315p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 889
🏁 Script executed:
sed -n '420,440p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 886
🏁 Script executed:
sed -n '465,480p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 686
🏁 Script executed:
sed -n '1,50p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 1728
🏁 Script executed:
rg -n 'class.*APIView|def (post|patch|get|delete|put)' apps/api/plane/api/views/page.py | head -20Repository: makeplane/plane
Length of output: 743
🏁 Script executed:
cat -n apps/api/plane/api/views/base.py | head -100Repository: makeplane/plane
Length of output: 4037
🏁 Script executed:
sed -n '77,120p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 1957
🏁 Script executed:
sed -n '40,65p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 1046
🏁 Script executed:
rg -n 'def dispatch' apps/api/plane/api/views/base.py -A 10Repository: makeplane/plane
Length of output: 984
🏁 Script executed:
sed -n '116,130p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 816
Replace direct .get() calls with get_object_or_404() to prevent 500 errors.
At lines 137, 227-232, 243-248, 303-308, 426-431, and 469-474, unguarded .get() calls will raise DoesNotExist exceptions that surface as 500 errors due to a bug in the dispatch() method (line 126 of base.py returns the exception instead of the handled response).
Use get_object_or_404() for all .get() calls:
Suggested fix pattern
+from django.shortcuts import get_object_or_404
...
- project = Project.objects.get(pk=project_id, workspace__slug=slug)
+ project = get_object_or_404(Project, pk=project_id, workspace__slug=slug)
...
- page = Page.objects.get(
+ page = get_object_or_404(
+ Page,
pk=pk,
workspace__slug=slug,
projects__id=project_id,
project_pages__deleted_at__isnull=True,
)🤖 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` at 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.
| if ( | ||
| request.data.get("external_id") | ||
| and (page.external_id != request.data.get("external_id")) | ||
| and Page.objects.filter( | ||
| workspace__slug=slug, | ||
| projects__id=project_id, | ||
| external_source=request.data.get("external_source", page.external_source), | ||
| external_id=request.data.get("external_id"), | ||
| ).exists() | ||
| ): | ||
| return Response( | ||
| { | ||
| "error": "Page with the same external id and external source already exists", | ||
| "id": str(page.id), | ||
| }, | ||
| status=status.HTTP_409_CONFLICT, | ||
| ) |
There was a problem hiding this comment.
Conflict detection misses updates where only external_source changes.
Line 261-270 only performs duplicate checking when external_id is in the payload and changed. If external_source changes alone to a conflicting pair, the collision is not blocked.
Suggested fix
- if (
- request.data.get("external_id")
- and (page.external_id != request.data.get("external_id"))
- and Page.objects.filter(
- workspace__slug=slug,
- projects__id=project_id,
- external_source=request.data.get("external_source", page.external_source),
- external_id=request.data.get("external_id"),
- ).exists()
- ):
+ incoming_external_id = request.data.get("external_id", page.external_id)
+ incoming_external_source = request.data.get("external_source", page.external_source)
+ if (
+ incoming_external_id
+ and incoming_external_source
+ and (
+ incoming_external_id != page.external_id
+ or incoming_external_source != page.external_source
+ )
+ and Page.objects.filter(
+ workspace__slug=slug,
+ projects__id=project_id,
+ external_source=incoming_external_source,
+ external_id=incoming_external_id,
+ )
+ .exclude(pk=page.pk)
+ .exists()
+ ):
return Response(🤖 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 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.
| if request.data.get("description_html"): | ||
| page_transaction.delay( | ||
| new_description_html=request.data.get("description_html", "<p></p>"), | ||
| old_description_html=page_description, | ||
| page_id=pk, | ||
| ) |
There was a problem hiding this comment.
Description transaction hook is skipped when clearing content to empty string.
At Line 282, the task is gated by truthiness (if request.data.get("description_html")). An explicit empty string update is a valid change but won’t trigger page_transaction, leaving derived transaction state stale.
Suggested fix
- if request.data.get("description_html"):
+ if "description_html" in request.data:
page_transaction.delay(
new_description_html=request.data.get("description_html", "<p></p>"),
old_description_html=page_description,
page_id=pk,
)📝 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 request.data.get("description_html"): | |
| page_transaction.delay( | |
| new_description_html=request.data.get("description_html", "<p></p>"), | |
| old_description_html=page_description, | |
| page_id=pk, | |
| ) | |
| if "description_html" in request.data: | |
| page_transaction.delay( | |
| new_description_html=request.data.get("description_html", "<p></p>"), | |
| old_description_html=page_description, | |
| page_id=pk, | |
| ) |
🤖 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 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.
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
Documentation
Tests