feat: add api/v1 pages endpoint#8650
Open
Vierkandt wants to merge 6 commits intomakeplane:previewfrom
Open
Conversation
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>
- 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>
Contributor
📝 WalkthroughWalkthroughAdds a Pages API: new PageSerializer (workspace-scoped labels, transactional create/update), four page endpoints (list/create, detail, archive/unarchive, lock/unlock) with permissions and lifecycle handling, URL routing and re-exports, and Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as PageListCreateAPIEndpoint
participant Serializer as PageSerializer
participant DB as Database/Models
participant BG as BackgroundTaskWorker
Client->>API: POST /workspaces/{slug}/projects/{id}/pages/ (payload)
API->>Serializer: validate + create(validated_data, context)
Serializer->>DB: create Page, create ProjectPage, bulk create PageLabel (atomic)
DB-->>Serializer: created Page instance
Serializer-->>API: return created Page data
API->>BG: trigger page_transaction task (description_html)
API-->>Client: 201 Created (page payload)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 2
🧹 Nitpick comments (2)
.gitignore (1)
113-113: LGTM —.worktrees/ignore entry is appropriate.Correctly prevents local
git worktreedirectories from being tracked.As an optional housekeeping note: the file has accumulated several duplicate entries (e.g.,
build/on lines 21/106/109,.react-router/on 22/107/110,package-lock.jsonon 74/81,out/on 16/20). Worth a one-time dedup pass, though it's unrelated to this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 113, The .gitignore contains duplicated entries (e.g., duplicate patterns "build/", ".react-router/", "package-lock.json", "out/") which should be cleaned up; open the .gitignore and remove repeated lines so each ignore pattern appears only once while preserving entries like ".worktrees/" from the diff, keeping a single canonical occurrence of each pattern to avoid clutter and potential merge noise.apps/api/plane/api/views/page.py (1)
326-360: Consider usingselect_related('parent')to avoid an extra query on line 354.
page.parent.archived_attriggers a lazy-load query for the parent. Since the page is already fetched on line 331, addingselect_related('parent')there would eliminate this extra query.Proposed optimization
- page = Page.objects.get( + page = Page.objects.select_related("parent").get( pk=page_id, 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` around lines 326 - 360, The Page lookup in the delete method is causing an extra DB query when accessing page.parent.archived_at; update the Page.objects.get call to use select_related('parent') so the parent is fetched in the same query (i.e., modify the query that calls Page.objects.get(...) inside delete(...) to include select_related('parent')); keep the subsequent logic (checking page.parent and archived_at, clearing parent, saving with update_fields, and calling unarchive_archive_page_and_descendants) unchanged.
🤖 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 22-49: The serializer's Meta currently exposes "is_locked" and
"archived_at" as writable fields allowing bypass of the
PageLockUnlockAPIEndpoint and PageArchiveUnarchiveAPIEndpoint; update the
Meta.read_only_fields for the Page serializer to include "is_locked" and
"archived_at" (in addition to the existing "workspace", "owned_by",
"project_ids", "label_ids") so those two fields cannot be set via the
serializer/PATCH and remain controlled only by the dedicated endpoints.
In `@apps/api/plane/api/views/page.py`:
- Around line 196-201: The access-change check compares page.access (int) to
request.data.get("access") which may be a string, causing false positives;
update the check to coerce the incoming value to int safely before comparing:
retrieve new_access_raw = request.data.get("access", page.access), attempt
new_access = int(new_access_raw) with a fallback to page.access on
TypeError/ValueError/None, then use if new_access != page.access and
page.owned_by_id != request.user.id: ... so the comparison uses integers and
won’t block valid requests; reference page.access, request.data.get("access"),
page.owned_by_id, and request.user.id when making the change.
---
Nitpick comments:
In @.gitignore:
- Line 113: The .gitignore contains duplicated entries (e.g., duplicate patterns
"build/", ".react-router/", "package-lock.json", "out/") which should be cleaned
up; open the .gitignore and remove repeated lines so each ignore pattern appears
only once while preserving entries like ".worktrees/" from the diff, keeping a
single canonical occurrence of each pattern to avoid clutter and potential merge
noise.
In `@apps/api/plane/api/views/page.py`:
- Around line 326-360: The Page lookup in the delete method is causing an extra
DB query when accessing page.parent.archived_at; update the Page.objects.get
call to use select_related('parent') so the parent is fetched in the same query
(i.e., modify the query that calls Page.objects.get(...) inside delete(...) to
include select_related('parent')); keep the subsequent logic (checking
page.parent and archived_at, clearing parent, saving with update_fields, and
calling unarchive_archive_page_and_descendants) unchanged.
- Add is_locked and archived_at to serializer read_only_fields to prevent
bypassing the dedicated lock/archive endpoints via PATCH
- Coerce access param to int before comparison in patch() to handle
string values from form-encoded requests
- Use select_related("parent") in unarchive lookup to avoid an extra
query when checking page.parent.archived_at
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/api/plane/api/views/page.py (2)
268-273: Nit: preferproject_id=project_idoverproject=project_idfor theUserFavoritefilter.The archive block (Line 322) uses
project_id=project_id(FK id shorthand), but hereproject=project_idis used. Both work in Django ORM filtering, butproject_id=avoids an implicit PK-lookup step and is consistent with the rest of the file.✏️ Proposed fix
UserFavorite.objects.filter( - project=project_id, + project_id=project_id, workspace__slug=slug, entity_identifier=pk, entity_type="page", ).delete()🤖 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 268 - 273, Change the Django ORM filter on UserFavorite to use the FK id shorthand: replace the filter argument project=project_id with project_id=project_id in the UserFavorite.objects.filter(...) call so it avoids an implicit PK lookup and matches the style used elsewhere (e.g., the archive block).
111-111: Nit: use thePage.PUBLIC_ACCESSconstant instead of the magic integer0.✏️ Proposed fix
- queryset = queryset.filter(Q(owned_by=request.user) | Q(access=0)) + queryset = queryset.filter(Q(owned_by=request.user) | Q(access=Page.PUBLIC_ACCESS))🤖 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 111, Replace the magic integer 0 with the Page.PUBLIC_ACCESS constant in the queryset filter (i.e., change Q(access=0) to Q(access=Page.PUBLIC_ACCESS)) to make intent explicit; ensure the Page model is imported or referenced in this module so Page.PUBLIC_ACCESS is available, and run tests to confirm no import/name errors in the view where queryset is defined.
🤖 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 155-159: The page creation path currently calls page_transaction
with old_description_html=None which causes
extract_all_components(old_description_html) to raise a TypeError and silently
skip creating PageLog entries; change the call in the POST/create flow to pass
an empty string (old_description_html="") instead of None (e.g., use
request.data.get("description_html", "<p></p>") for new_description_html and
old_description_html="") so extract_all_components receives a string and PageLog
entries are created correctly.
- Line 175: The BaseAPIView.dispatch implementation currently returns the
exception object instead of the HTTP response from handle_exception; update the
dispatch method in BaseAPIView so that after calling
self.handle_exception(request, exc) it returns the generated response (i.e., use
"return response") rather than "return exc", ensuring exception-to-response
conversion (e.g., ObjectDoesNotExist→404) actually reaches callers like page
retrieval in PageView where get_queryset().get(pk=pk) may raise exceptions.
---
Nitpick comments:
In `@apps/api/plane/api/views/page.py`:
- Around line 268-273: Change the Django ORM filter on UserFavorite to use the
FK id shorthand: replace the filter argument project=project_id with
project_id=project_id in the UserFavorite.objects.filter(...) call so it avoids
an implicit PK lookup and matches the style used elsewhere (e.g., the archive
block).
- Line 111: Replace the magic integer 0 with the Page.PUBLIC_ACCESS constant in
the queryset filter (i.e., change Q(access=0) to Q(access=Page.PUBLIC_ACCESS))
to make intent explicit; ensure the Page model is imported or referenced in this
module so Page.PUBLIC_ACCESS is available, and run tests to confirm no
import/name errors in the view where queryset is defined.
Comment on lines
+155
to
+159
| page_transaction.delay( | ||
| new_description_html=request.data.get("description_html", "<p></p>"), | ||
| old_description_html=None, | ||
| page_id=serializer.data["id"], | ||
| ) |
Contributor
There was a problem hiding this comment.
Pass an empty string for old_description_html on page creation, not None.
The page_transaction background task calls extract_all_components(old_description_html). An HTML-parsing function receiving None instead of a string will raise a TypeError; the broad except Exception in the task body swallows it silently, meaning no PageLog entries are created for pages with initial description content. The PATCH path correctly reads the current description_html; on POST the equivalent base state is an empty document.
🐛 Proposed fix
page_transaction.delay(
new_description_html=request.data.get("description_html", "<p></p>"),
- old_description_html=None,
+ old_description_html="",
page_id=serializer.data["id"],
)🤖 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 155 - 159, The page creation
path currently calls page_transaction with old_description_html=None which
causes extract_all_components(old_description_html) to raise a TypeError and
silently skip creating PageLog entries; change the call in the POST/create flow
to pass an empty string (old_description_html="") instead of None (e.g., use
request.data.get("description_html", "<p></p>") for new_description_html and
old_description_html="") so extract_all_components receives a string and PageLog
entries are created correctly.
|
|
||
| Retrieve details of a specific page. | ||
| """ | ||
| page = self.get_queryset().get(pk=pk) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find BaseAPIView definition and exception handling
find . -name "base.py" -path "*/api/views/*" | head -5Repository: makeplane/plane
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Search for BaseAPIView class definition
rg -n "class BaseAPIView" --type py -A 20 | head -100Repository: makeplane/plane
Length of output: 6021
🏁 Script executed:
#!/bin/bash
# Look for exception_handler registration or custom exception handling
rg -n "exception_handler|ExceptionHandler|EXCEPTION_HANDLER" --type py | head -50Repository: makeplane/plane
Length of output: 486
🏁 Script executed:
#!/bin/bash
# Examine the page.py file at the critical lines
if [ -f "apps/api/plane/api/views/page.py" ]; then
echo "=== Lines 170-180 ==="
sed -n '170,180p' apps/api/plane/api/views/page.py | cat -n
echo ""
echo "=== Lines 180-195 ==="
sed -n '180,195p' apps/api/plane/api/views/page.py | cat -n
echo ""
echo "=== Lines 230-240 ==="
sed -n '230,240p' apps/api/plane/api/views/page.py | cat -n
fiRepository: makeplane/plane
Length of output: 1481
🏁 Script executed:
#!/bin/bash
# Search for ObjectDoesNotExist handling in page.py and related views
rg -n "DoesNotExist|except.*DoesNotExist|try:" --type py apps/api/plane/api/views/page.py | head -50Repository: makeplane/plane
Length of output: 95
🏁 Script executed:
#!/bin/bash
# Read the custom exception handler
cat -n apps/api/plane/authentication/adapter/exception.pyRepository: makeplane/plane
Length of output: 1495
🏁 Script executed:
#!/bin/bash
# Read BaseAPIView.handle_exception method from the api/views/base.py
sed -n '50,150p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 4318
🏁 Script executed:
#!/bin/bash
# Check context around the try blocks at 95 and 198 in page.py
echo "=== Lines 90-105 (try at 95) ==="
sed -n '90,105p' apps/api/plane/api/views/page.py | cat -n
echo ""
echo "=== Lines 193-210 (try at 198) ==="
sed -n '193,210p' apps/api/plane/api/views/page.py | cat -nRepository: makeplane/plane
Length of output: 1808
🏁 Script executed:
#!/bin/bash
# Check if DRF's exception_handler handles ObjectDoesNotExist by default
# First, let's see what imports are in the exception handler file
head -30 apps/api/plane/authentication/adapter/exception.py | cat -nRepository: makeplane/plane
Length of output: 1333
🏁 Script executed:
#!/bin/bash
# Check what PageDetailAPIEndpoint inherits from in page.py
grep -n "class PageDetailAPIEndpoint\|class.*APIView\|import.*BaseAPIView" apps/api/plane/api/views/page.py | head -20Repository: makeplane/plane
Length of output: 330
🏁 Script executed:
#!/bin/bash
# Verify the dispatch bug - does it really return exc instead of response?
sed -n '67,78p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 511
🏁 Script executed:
#!/bin/bash
# Check what ObjectDoesNotExist is imported from in base.py
head -50 apps/api/plane/api/views/base.py | grep -n "import\|from" | cat -nRepository: makeplane/plane
Length of output: 1235
🏁 Script executed:
#!/bin/bash
# Let me see the full dispatch method clearly
sed -n '67,77p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 492
🏁 Script executed:
#!/bin/bash
# Get the exact dispatch method from base.py
cat -n apps/api/plane/api/views/base.py | sed -n '67,77p'Repository: makeplane/plane
Length of output: 492
🏁 Script executed:
#!/bin/bash
# Find the dispatch method in base.py
rg -n "def dispatch" apps/api/plane/api/views/base.py -A 12Repository: makeplane/plane
Length of output: 1148
🏁 Script executed:
#!/bin/bash
# Get the full context of lines 116-126 to see the bug clearly
sed -n '116,126p' apps/api/plane/api/views/base.py | cat -nRepository: makeplane/plane
Length of output: 588
BaseAPIView.dispatch returns the exception instead of the response — all exception handling is broken.
Line 126 in apps/api/plane/api/views/base.py returns exc instead of response after calling handle_exception(). This means that even though handle_exception() correctly converts ObjectDoesNotExist to a 404 response, the response is never returned to the client. Instead, the raw exception object is returned, causing an error.
This affects all views using BaseAPIView and should be fixed to 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` at line 175, The BaseAPIView.dispatch
implementation currently returns the exception object instead of the HTTP
response from handle_exception; update the dispatch method in BaseAPIView so
that after calling self.handle_exception(request, exc) it returns the generated
response (i.e., use "return response") rather than "return exc", ensuring
exception-to-response conversion (e.g., ObjectDoesNotExist→404) actually reaches
callers like page retrieval in PageView where get_queryset().get(pk=pk) may
raise exceptions.
- Use Page.PUBLIC_ACCESS constant instead of magic integer 0 - Use project_id= shorthand instead of project= in UserFavorite filter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 131-136: The current conflict check uses Page.objects.filter(...)
and inadvertently matches soft-deleted pages; update the queryset used in the
external ID conflict check (the Page.objects.filter call) to exclude pages whose
project_pages relation is soft-deleted by adding
project_pages__deleted_at__isnull=True to the filter so only active
(non-deleted) pages are considered when detecting duplicates for
external_id/external_source.
- Around line 275-280: The call to
UserRecentVisit.objects.filter(...).delete(soft=False) uses a non-existent
keyword argument and will raise TypeError; change the delete invocation on the
queryset returned by UserRecentVisit.objects.filter(project_id=project_id,
workspace__slug=slug, entity_identifier=pk, entity_name="page") to call
.delete() with no arguments (or implement/route to a hard-delete helper if hard
deletion semantics are required), ensuring the removal logic for UserRecentVisit
uses the standard QuerySet.delete() signature.
---
Duplicate comments:
In `@apps/api/plane/api/views/page.py`:
- Around line 175-176: BaseAPIView.dispatch currently returns the exception
object instead of the handled Response, so exceptions raised by unprotected
.get() calls (e.g., Page queryset .get() in views using PageSerializer) bubble
out; update BaseAPIView.dispatch to call self.handle_exception(exc) and if it
returns a Response, return that Response (not the original exc) so Django REST
Framework 404/handled responses are returned for exceptions like
Model.DoesNotExist; locate BaseAPIView.dispatch and replace the code path that
returns exc with one that checks for and returns the Response produced by
handle_exception.
- Around line 155-159: When scheduling the background task, don't pass
old_description_html=None because page_transaction forwards it to
extract_all_components which raises a TypeError on None; instead pass an
empty-document baseline (e.g. "<p></p>") as old_description_html so
extract_all_components receives a string; update the page_transaction.delay call
in the view to use old_description_html="<p></p>" (or the same default you use
for new_description_html) so PageLog entries are created on page creation.
Comment on lines
+275
to
+280
| UserRecentVisit.objects.filter( | ||
| project_id=project_id, | ||
| workspace__slug=slug, | ||
| entity_identifier=pk, | ||
| entity_name="page", | ||
| ).delete(soft=False) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. Find the UserRecentVisit model definition
echo "=== Searching for UserRecentVisit model class ==="
rg -n "class UserRecentVisit" --type py -A 10
echo -e "\n=== Searching for delete methods with soft parameter ==="
rg -n "def delete.*soft" --type py -B 2 -A 5
echo -e "\n=== Searching for UserRecentVisit in models or related files ==="
fd -e py | xargs grep -l "UserRecentVisit" | head -20
echo -e "\n=== Looking for custom QuerySet/Manager definitions ==="
rg -n "class.*QuerySet|class.*Manager" --type py | grep -i "recent\|soft" | head -20Repository: makeplane/plane
Length of output: 4691
🏁 Script executed:
#!/bin/bash
# Check WorkspaceBaseModel definition and its queryset/manager
echo "=== Searching for WorkspaceBaseModel definition ==="
rg -n "class WorkspaceBaseModel" --type py -A 15
echo -e "\n=== Check UserRecentVisit model full definition ==="
sed -n '22,40p' apps/api/plane/db/models/recent_visit.py
echo -e "\n=== Check if UserRecentVisit or parent has custom manager ==="
rg -n "objects.*=" apps/api/plane/db/models/recent_visit.py
echo -e "\n=== Check SoftDeletionManager get_queryset method ==="
rg -n "class SoftDeletionManager" --type py -A 10Repository: makeplane/plane
Length of output: 2910
🏁 Script executed:
#!/bin/bash
# Check BaseModel definition
echo "=== Searching for BaseModel definition ==="
rg -n "^class BaseModel" --type py -A 20
echo -e "\n=== Check if BaseModel or its parents have SoftDeletionManager ==="
rg -n "objects.*=.*SoftDeletionManager" --type py
echo -e "\n=== Check if UserRecentVisit has deleted_at field ==="
rg -n "deleted_at" apps/api/plane/db/models/recent_visit.py
echo -e "\n=== Full UserRecentVisit model file ==="
cat apps/api/plane/db/models/recent_visit.pyRepository: makeplane/plane
Length of output: 3056
🏁 Script executed:
#!/bin/bash
# Check the actual code context in page.py
echo "=== Code around lines 275-280 in page.py ==="
sed -n '270,285p' apps/api/plane/api/views/page.py
echo -e "\n=== Check if there's any manager override or monkey-patching ==="
rg -n "UserRecentVisit" apps/api/plane/api/views/page.py -B 2 -A 2
echo -e "\n=== Verify UserRecentVisit has no custom manager ==="
rg -n "objects\s*=" apps/api/plane/db/models/recent_visit.pyRepository: makeplane/plane
Length of output: 1050
UserRecentVisit.objects.filter(...).delete(soft=False) will raise TypeError at runtime.
Django's standard QuerySet.delete() method signature is def delete(self): and does not accept keyword arguments. UserRecentVisit does not inherit from SoftDeleteModel and has no custom queryset that supports the soft parameter. Calling .delete(soft=False) will raise TypeError: delete() got an unexpected keyword argument 'soft'.
Use .delete() without arguments, or refactor to use hard delete if that is the intention.
🤖 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 275 - 280, The call to
UserRecentVisit.objects.filter(...).delete(soft=False) uses a non-existent
keyword argument and will raise TypeError; change the delete invocation on the
queryset returned by UserRecentVisit.objects.filter(project_id=project_id,
workspace__slug=slug, entity_identifier=pk, entity_name="page") to call
.delete() with no arguments (or implement/route to a hard-delete helper if hard
deletion semantics are required), ensuring the removal logic for UserRecentVisit
uses the standard QuerySet.delete() signature.
A page removed from a project sets ProjectPage.deleted_at but leaves Page.deleted_at null. Without this filter the conflict check would find it and return a spurious 409, permanently blocking re-creation under that external_id/external_source pair. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 218-223: The current truthiness check skips calling
page_transaction when description_html is intentionally set to an empty string;
change the condition to check key presence instead (use "if 'description_html'
in request.data") and pass the submitted value directly (use
request.data.get("description_html") without a non-empty default) so an explicit
"" is forwarded to page_transaction (still passing
old_description_html=page_description and page_id=pk) ensuring PageLog updates
occur even when the description is cleared.
---
Duplicate comments:
In `@apps/api/plane/api/views/page.py`:
- Around line 276-281: The call
UserRecentVisit.objects.filter(...).delete(soft=False) is invalid because
QuerySet.delete() does not accept a soft kwarg; remove the keyword argument and
call delete() on the queryset instead, or if you intended a model-level soft
delete use one of two fixes: either iterate the queryset and call
instance.delete(soft=False) on each UserRecentVisit instance, or implement a
custom QuerySet/manager that supports a soft parameter and use that; update the
code around UserRecentVisit.objects.filter(...).delete(...) accordingly so it no
longer passes soft=False to QuerySet.delete().
- Around line 156-160: The create path is passing old_description_html=None into
page_transaction which causes extract_all_components(old_description_html) to
TypeError and silently fail; change the call in the view to pass an empty
document string (e.g. "<p></p>") as old_description_html instead of None so
page_transaction and extract_all_components receive a valid string and PageLog
entries are created for initial content; locate the call to page_transaction in
the view and set old_description_html=request.data.get("old_description_html",
"<p></p>") or simply old_description_html="<p></p>" on create.
Comment on lines
+218
to
+223
| 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, | ||
| ) |
Contributor
There was a problem hiding this comment.
page_transaction skipped when description_html is cleared to an empty string.
request.data.get("description_html") evaluates to False for "", so a PATCH that intentionally clears the page content bypasses the transaction. The serializer still persists the empty value, but PageLog is never updated: any mentions/embeds from the old description remain as stale PageLog rows.
Use a key-presence check instead of a truthiness check:
🐛 Proposed 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,
)🤖 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 218 - 223, The current
truthiness check skips calling page_transaction when description_html is
intentionally set to an empty string; change the condition to check key presence
instead (use "if 'description_html' in request.data") and pass the submitted
value directly (use request.data.get("description_html") without a non-empty
default) so an explicit "" is forwarded to page_transaction (still passing
old_description_html=page_description and page_id=pk) ensuring PageLog updates
occur even when the description is cleared.
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
1 participant
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 workspace-scoped label validation, atomic create/updateapps/api/plane/api/views/page.pyPageQuerySetMixinapps/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
New Features
Chores