[WIKI-811] fix: ensure only non-deleted project pages are retrieved in page queries #8182
Conversation
…geViewSet and PageDuplicateEndpoint
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant PageView as PageView (views/page/base.py)
participant DB as Database
rect rgb(235,245,255)
Note over PageView,DB: Page queries now include\nproject_pages__deleted_at__isnull = TRUE
end
Client->>PageView: Request (retrieve/update/lock/duplicate/...)
PageView->>DB: SELECT ... WHERE id = X\nAND project_pages__deleted_at__isnull = TRUE
alt non-deleted page found
DB-->>PageView: Page record
PageView->>DB: (cascade checks / related lookups)\nalso filtered by non-deleted
DB-->>PageView: related records
PageView-->>Client: 200 / action performed
else not found or deleted
DB-->>PageView: empty
PageView-->>Client: 404 / error (permissions/existence)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
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 |
There was a problem hiding this comment.
Pull request overview
This PR addresses a bug where deleted project pages could still be retrieved through various database queries. The fix adds a project_pages__deleted_at__isnull=True filter across page retrieval operations to ensure only non-deleted project-page associations are returned.
Key changes:
- Added soft-delete filtering to 12 database queries accessing pages through their project relationship
- Ensures deleted pages cannot be locked, unlocked, archived, unarchived, or duplicated
- Prevents updates to deleted page descriptions and access permissions
Comments suppressed due to low confidence (5)
apps/api/plane/app/views/page/base.py:599
- This query retrieves project IDs without filtering for deleted ProjectPage associations. It should include
deleted_at__isnull=Trueto avoid duplicating the page into projects where it has been soft-deleted.
project_ids = ProjectPage.objects.filter(page_id=page_id).values_list("project_id", flat=True)
apps/api/plane/app/views/page/base.py:432
- The
summarymethod's queryset is missing theproject_pages__deleted_at__isnull=Truefilter on both the main query (line 419-426) and the ProjectPage subquery annotation (line 429). This could include soft-deleted page-project associations in the statistics.
Page.objects.filter(workspace__slug=slug)
.filter(
projects__project_projectmember__member=self.request.user,
projects__project_projectmember__is_active=True,
projects__archived_at__isnull=True,
)
.filter(parent__isnull=True)
.filter(Q(owned_by=request.user) | Q(access=0))
.annotate(
project=Exists(
ProjectPage.objects.filter(page_id=OuterRef("id"), project_id=self.kwargs.get("project_id"))
)
)
.filter(project=True)
apps/api/plane/app/views/page/base.py:106
- The
get_querysetmethod's ProjectPage annotation should includedeleted_at__isnull=Trueto ensure theprojectannotation only returns True for non-deleted project-page associations. Without this filter, deleted associations will still be counted.
.annotate(
project=Exists(
ProjectPage.objects.filter(page_id=OuterRef("id"), project_id=self.kwargs.get("project_id"))
)
)
apps/api/plane/app/views/page/base.py:92
- The
get_querysetmethod is missing theproject_pages__deleted_at__isnull=Truefilter. This filter should be added to ensure pages with soft-deleted project associations are excluded from the main queryset.
.filter(workspace__slug=self.kwargs.get("slug"))
.filter(
projects__project_projectmember__member=self.request.user,
projects__project_projectmember__is_active=True,
projects__archived_at__isnull=True,
)
apps/api/plane/app/views/page/base.py:599
- The duplicate endpoint lacks test coverage for the scenario where a page is duplicated while it has soft-deleted ProjectPage associations. Consider adding tests to verify that only non-deleted project associations are duplicated.
project_ids = ProjectPage.objects.filter(page_id=page_id).values_list("project_id", flat=True)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/api/plane/app/views/page/base.py (3)
243-285: Now let me examine the actual content of the file around the methods in question:
<function_calls>
wc -l apps/api/plane/app/views/page/base.py
</function_calls><function_calls>
sed -n '230,300p' apps/api/plane/app/views/page/base.py
</function_calls><function_calls>
sed -n '235,295p' apps/api/plane/app/views/page/base.py
</function_calls>Now let me check for None guards in similar methods:
<function_calls>
rg "if page is None" apps/api/plane/app/views/page/base.py -A 2 -B 2
</function_calls>Let me check the full context of lock/unlock/access methods:
<function_calls>
rg "def lock(|def unlock(|def access(" apps/api/plane/app/views/page/base.py -A 15
</function_calls><function_calls>
cat -n apps/api/plane/app/views/page/base.py | sed -n '235,290p'
</function_calls>Let me check what HTTP status codes are used elsewhere in this file for "not found" scenarios:
<function_calls>
rg "404_NOT_FOUND|not found" apps/api/plane/app/views/page/base.py -B 3 -A 1
</function_calls>Let me check other view files for error handling patterns:
<function_calls>
find apps/api/plane/app/views -name "*.py" -type f | head -5
</function_calls><function_calls>
rg "if.*is None.*404|404.*if.*is None" apps/api/plane/app/views --type py -A 2 -B 2 | head -40
</function_calls>Let me search for general patterns of None checks in the views:
<function_calls>
rg ".first()" apps/api/plane/app/views/page/base.py -B 5 -A 3 | head -60
</function_calls><function_calls>
grep -n "def lock|def unlock|def access" apps/api/plane/app/views/page/base.py
</function_calls><function_calls>
sed -n '243,290p' apps/api/plane/app/views/page/base.py | cat -n
</function_calls>Let me get a broader view of the file structure and line numbers:
<function_calls>
grep -n "^\s*def " apps/api/plane/app/views/page/base.py | grep -E "lock|unlock|access"
</function_calls><function_calls>
python3 << 'EOF'
with open("apps/api/plane/app/views/page/base.py", "r") as f:
lines = f.readlines()
for i, line in enumerate(lines, 1):
if "def lock" in line or "def unlock" in line or "def access" in line:
# Print 20 lines starting from the method definition
for j in range(i-1, min(i+19, len(lines))):
print(f"{j+1:4d}: {lines[j]}", end="")
print("\n" + "="*80 + "\n")
EOF
</function_calls>Now let me check for error handling patterns in similar methods:
<function_calls>
python3 << 'EOF'
import re
with open("apps/api/plane/app/views/page/base.py", "r") as f:
content = f.read()Find all instances of .first() followed by None checks
pattern = r'.first()\s*\n.*?if.*is None'
matches = re.findall(pattern, content, re.DOTALL)
print(f"Found {len(matches)} instances of .first() with None checks")Find .first() without immediate None checks
pattern = r'.first()\s*\n\s*(\w+)\s*='
matches = re.findall(pattern, content)
print(f"\nAssignments after .first():")
for m in set(matches):
print(f" - {m}")Check what comes after .first()
lines = content.split('\n')
for i, line in enumerate(lines):
if '.first()' in line:
print(f"\nLine {i+1}: {line.strip()}")
if i+1 < len(lines):
next_line = lines[i+1].strip()
print(f" Next: {next_line}")
if i+2 < len(lines) and next_line == '':
print(f" Then: {lines[i+2].strip()}")
EOF
</function_calls><function_calls>
python3 << 'EOF'
with open("apps/api/plane/app/views/page/base.py", "r") as f:
lines = f.readlines()Look for lock, unlock, access methods
for i, line in enumerate(lines):
if re.search(r'def (lock|unlock|access)(', line):
print(f"Found at line {i+1}: {line.rstrip()}")
# Print next 20 lines
for j in range(i, min(i+20, len(lines))):
print(f"{j+1:4d}: {lines[j]}", end="")
print("\n" + "-"*80)import re
EOF
</function_calls><function_calls>
python3 -c "
import re
with open('apps/api/plane/app/views/page/base.py', 'r') as f:
lines = f.readlines()
for i, line in enumerate(lines):
if re.search(r'def (lock|unlock|access)(', line):
print(f'Found at line {i+1}: {line.rstrip()}')
for j in range(i, min(i+20, len(lines))):
print(f'{j+1:4d}: {lines[j]}', end='')
print('\n' + '-'*80)
"
</function_calls>Now let me check for Response patterns with 404:
<function_calls>
grep -n "404|not found|not_found" apps/api/plane/app/views/page/base.py
</function_calls>Let me look at error handling patterns across the project:
<function_calls>
rg "HTTP_404|404_NOT_FOUND" apps/api/plane/app/views --type py | head -20
</function_calls><function_calls>
rg "if.*is None.*Response|Response.*if.*is None" apps/api/plane/app/views --type py -B 1 -A 2 | head -50
</function_calls>Let me examine another page view file to understand patterns:
<function_calls>
ls -la apps/api/plane/app/views/page/
</function_calls><function_calls>
head -100 apps/api/plane/app/views/page/base.py | tail -50
</function_calls>Let me look at the exact lines specified in the review:
<function_calls>
sed -n '243,285p' apps/api/plane/app/views/page/base.py | nl -v 243
</function_calls>Perfect! Now let me check if these methods have None guards added:
<function_calls>
sed -n '240,300p' apps/api/plane/app/views/page/base.py | nl -v 240
</function_calls>Now let me check the project's error handling convention by looking at similar patterns:
<function_calls>
rg ".first()" apps/api/plane/app/views --type py -A 2 -B 1 | grep -A 2 "first()" | head -40
</function_calls><function_calls>
rg "if .* is None:" apps/api/plane/app/views/page/base.py -A 2 -B 1
</function_calls>Let me search for how this codebase handles missing resources in other methods in the same file:
<function_calls>
cat apps/api/plane/app/views/page/base.py | grep -A 3 ".first()" | head -60
</function_calls><function_calls>
python3 << 'EOF'
with open("apps/api/plane/app/views/page/base.py", "r") as f:
content = f.read()Find all .first() calls and what comes after
import re
pattern = r'.first()\s*\n\s*(.+?)(?=\n\s*(?:if|page.|return|def|\Z))'
matches = re.finditer(pattern, content, re.DOTALL)seen = set()
for match in matches:
next_code = match.group(1).strip()[:100]
if next_code not in seen:
print(f"After .first(): {next_code}")
seen.add(next_code)
EOF
</function_calls>Let me check what imports and patterns are used for error responses:
<function_calls>
head -50 apps/api/plane/app/views/page/base.py
</function_calls><function_calls>
rg "status." apps/api/plane/app/views/page/base.py | head -10
</function_calls>Let me look at a broader range around the methods:
<function_calls>
sed -n '200,320p' apps/api/plane/app/views/page/base.py | nl -v 200
</function_calls>Now let me check error code conventions in the codebase:
<function_calls>
rg "HTTP_404|404_NOT_FOUND" apps/api/plane/app/views --type py -B 2 -A 1 | head -30
</function_calls><function_calls>
rg ".first()" apps/api/plane/app/views -A 2 --type py | grep -B 1 "if.*is None" | head -20
</function_calls>Let me check a specific other method in the same file that might show the pattern:
<function_calls>
sed -n '1,100p' apps/api/plane/app/views/page/base.py
</function_calls>Let me search for common error responses in page-related views:
<function_calls>
rg "Response.*error.*404|404.*Response.*error" apps/api/plane/app/views --type py | head -10
</function_calls><function_calls>
rg ""error"" apps/api/plane/app/views/page/base.py | head -5
</function_calls>Let me check the full access method to confirm the issue:
<function_calls>
sed -n '265,285p' apps/api/plane/app/views/page/base.py | nl -v 265
</function_calls>Let me verify response patterns used in the codebase:
<function_calls>
rg "Response({.*error" apps/api/plane/app/views --type py | head -10
</function_calls>Now let me check if there's a get_object_or_404 pattern used:
<function_calls>
rg "get_object_or_404" apps/api/plane/app/views --type py | head -5
</function_calls>Let me look at the imports and understand the Response mechanism:
<function_calls>
sed -n '1,30p' apps/api/plane/app/views/page/base.py
</function_calls>Now let me check what happens elsewhere in the codebase when
.first()returns None:
<function_calls>
python3 << 'EOF'
import re
with open("apps/api/plane/app/views/page/base.py", "r") as f:
lines = f.readlines()Find patterns where .first() is used
for i, line in enumerate(lines):
if '.first()' in line:
# Check next 3 lines for None checks
has_guard = False
for j in range(i+1, min(i+4, len(lines))):
if 'is None' in lines[j]:
has_guard = True
breakmarker = "✓ HAS GUARD" if has_guard else "✗ NO GUARD" print(f"Line {i+1} {marker}: {line.strip()}") if not has_guard: for j in range(i, min(i+3, len(lines))): print(f" {j+1}: {lines[j].rstrip()}")EOF
</function_calls>Perfect! Let me verify the status code patterns used in similar contexts:
<function_calls>
rg "HTTP_404_NOT_FOUND|status.HTTP_404" apps/api/plane --type py | head -10
</function_calls><function_calls>
rg "status.HTTP.*404" apps/api/plane --type py | head -10
</function_calls>Now let me check what similar patterns exist for error handling in this view file:
<function_calls>
grep -n "return Response" apps/api/plane/app/views/page/base.py | head -15
</function_calls><function_calls>
sed -n '140,180p' apps/api/plane/app/views/page/base.py | nl -v 140
</function_calls>Let me check if there's a specific error handling method or pattern in this codebase for 404 errors:
<function_calls>
rg "page is None|not.*found|Page.*not found" apps/api/plane/app/views --type py | head -10
</function_calls>Now let me confirm the exact issue by checking what status codes the suggested fix uses:
<function_calls>
rg "404_NOT_FOUND" apps/api/plane --type py | wc -l
</function_calls><function_calls>
rg "HTTP_404|404_NOT_FOUND|404" apps/api/plane/app/views --type py -B 2 -A 1 | head -40
</function_calls>Let me check how the response library is imported and what constants are available:
<function_calls>
python3 << 'EOF'Check what status codes are available in Django REST framework
from rest_framework import status
Find 404 status code
status_code = getattr(status, 'HTTP_404_NOT_FOUND', None)
print(f"HTTP_404_NOT_FOUND = {status_code}")List all HTTP_* status codes
http_statuses = [attr for attr in dir(status) if attr.startswith('HTTP_')]
print(f"\nTotal HTTP status codes available: {len(http_statuses)}")
for s in http_statuses:
if '404' in s or '400' in s:
print(f" {s} = {getattr(status, s)}")
EOF
</function_calls>Let me do one final verification to see the exact error that would occur:
<function_calls>
python3 << 'EOF'Simulate what happens in the current code
class MockPage:
passpage = None # This is what .first() returns when no match
This is what the current code does:
try:
page.is_locked = True # This is line 251 in lock()
except AttributeError as e:
print(f"ERROR in lock(): {type(e).name}: {e}")Same for unlock()
try:
page.is_locked = False # This is line 261 in unlock()
except AttributeError as e:
print(f"ERROR in unlock(): {type(e).name}: {e}")Same for access()
try:
if page.access != 0: # This is line 277 in access()
pass
except AttributeError as e:
print(f"ERROR in access(): {type(e).name}: {e}")
EOF
</function_calls>Excellent. The verification is complete. Let me now generate the rewritten review comment:
Guard
lock/unlock/accessmethods against missing pagesThese three methods query the database with
.first(), which can returnNonewhen the page doesn't exist (e.g., invalidpage_id, wrongslug, or soft-deleted project-page link). The code currently accesses attributes on the potentially-Noneresult without checking, causingAttributeError→ 500 error instead of a controlled 4xx response.Add
Nonechecks before attribute access to return a proper 404 response:def lock(self, request, slug, project_id, page_id): page = Page.objects.filter( pk=page_id, workspace__slug=slug, projects__id=project_id, project_pages__deleted_at__isnull=True, ).first() + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) page.is_locked = True page.save() return Response(status=status.HTTP_204_NO_CONTENT) def unlock(self, request, slug, project_id, page_id): page = Page.objects.filter( pk=page_id, workspace__slug=slug, projects__id=project_id, project_pages__deleted_at__isnull=True, ).first() + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) page.is_locked = False page.save() return Response(status=status.HTTP_204_NO_CONTENT) def access(self, request, slug, project_id, page_id): access = request.data.get("access", 0) page = Page.objects.filter( pk=page_id, workspace__slug=slug, projects__id=project_id, project_pages__deleted_at__isnull=True, ).first() + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)
305-399: Based on my verification, I have confirmed all aspects of the review comment. Here is my rewritten review:
archive,unarchive, anddestroywill raise 500 errors on missing or soft-deleted pagesThese three methods use
Page.objects.get()without exception handling. When an invalidpage_id, mismatchedslug/project_id, or a soft-deleted project-page association is encountered,Page.DoesNotExistis raised and bubbles up as a 500 error rather than a proper 404 response.This is inconsistent with:
partial_update()which explicitly catchesPage.DoesNotExist(line 192)retrieve()which uses the safer.filter().first()pattern- REST API conventions expecting 404 for missing resources
Refactor to use
.filter().first()with None check, as suggested:def archive(self, request, slug, project_id, page_id): page = Page.objects.filter( pk=page_id, workspace__slug=slug, projects__id=project_id, project_pages__deleted_at__isnull=True, ).first() if page is None: return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) # ... rest of methodApply the same pattern to
unarchiveanddestroy.
587-617: Perfect! I have verified all three issues in the review comment. Let me confirm the findings:Verification Results:
Missing null check on
page(Line 595): ✅ CONFIRMED
.first()can returnNone, but the code accessespage.accesswithout checking- Will raise
AttributeError→ 500 error if page is not foundProjectPage query lacks
deleted_atfilter (Line 601): ✅ CONFIRMED
ProjectPagemodel inherits fromSoftDeleteModel(viaAuditModel)- The query at line 601 should include
deleted_at__isnull=Truefilter- Current code will recreate associations for soft-deleted project-page links, contradicting the intent
Variable shadowing of
project_id(Lines 606, 628): ✅ CONFIRMED
- Loop variable
for project_id in project_ids:shadows the URL parameter- Line 628 calls
.delay(..., project_id=project_id, ...)with the last value from the loop iteration- Should preserve the URL parameter (
project_id) as the source project contextAll three issues are critical and match the suggested fixes provided in the review comment.
Duplicate endpoint: handle missing pages, respect
deleted_aton ProjectPage, and avoidproject_idshadowingIn
PageDuplicateEndpoint.post:
The
page = .first()at line 595 returnsNoneif no match exists, but the code immediately accessespage.accesswithout a null check →AttributeError500 error.
ProjectPage.objects.filter(page_id=page_id)at line 601 ignoresdeleted_at, so duplication recreates associations for projects from which the original page had been soft-deleted, contradicting the "non-deleted project pages only" intent in the Page query.The
for project_id in project_ids:loop at line 606 shadows the URL parameterproject_id. Line 628's.delay(..., project_id=project_id, ...)then uses the last project id from the loop, not the route parameter, causing the background task to copy S3 objects in the wrong project context.Suggested fix:
def post(self, request, slug, project_id, page_id): page = Page.objects.filter( pk=page_id, workspace__slug=slug, projects__id=project_id, project_pages__deleted_at__isnull=True, ).first() + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) # check for permission if page.access == Page.PRIVATE_ACCESS and page.owned_by_id != request.user.id: return Response({"error": "Permission denied"}, status=status.HTTP_403_FORBIDDEN) # get all the project ids where page is present - project_ids = ProjectPage.objects.filter(page_id=page_id).values_list("project_id", flat=True) + project_ids = ProjectPage.objects.filter( + page_id=page_id, + deleted_at__isnull=True, + ).values_list("project_id", flat=True) page.pk = None page.name = f"{page.name} (Copy)" page.description_binary = None page.owned_by = request.user page.created_by = request.user page.updated_by = request.user page.save() + source_project_id = project_id # preserve the route project id - for project_id in project_ids: + for target_project_id in project_ids: ProjectPage.objects.create( workspace_id=page.workspace_id, - project_id=project_id, + project_id=target_project_id, page_id=page.id, created_by_id=page.created_by_id, updated_by_id=page.updated_by_id, ) page_transaction.delay( new_description_html=page.description_html, old_description_html=None, page_id=page.id, ) # Copy the s3 objects uploaded in the page copy_s3_objects_of_description_and_assets.delay( entity_name="PAGE", entity_identifier=page.id, - project_id=project_id, + project_id=source_project_id, slug=slug, user_id=request.user.id, )
🧹 Nitpick comments (2)
apps/api/plane/app/views/page/base.py (2)
150-177: Non-deleted filter inpartial_updatelooks correct; consider centralizing this lookupAdding
project_pages__deleted_at__isnull=Trueto both the main page lookup and the optional parent lookup keepspartial_updatefrom touching soft-deleted project-page associations and is aligned with the PR’s intent. To avoid repeating this same(pk, workspace__slug, projects__id, project_pages__deleted_at__isnull=True)pattern across many methods, consider extracting a helper/manager method (e.g.,Page.objects.active_in_project(slug, project_id)or a private_get_page_in_project(...)on the viewset) and reusing it here and elsewhere for consistency and easier future changes.
417-463: List/retrieve/summary still include pages with soft-deleted project-page linksOther endpoints now explicitly constrain queries with
project_pages__deleted_at__isnull=True, but:
PageViewSet.get_queryset(used bylistandretrieve), andPageViewSet.summarystill rely on joins through
projects/ProjectPagewithout that filter. That means pages whose association to the current project was soft-deleted can still show up in list/retrieve/summary, while lock/access/archive/destroy correctly treat them as non-existent, leading to inconsistent behavior.For consistency with the PR’s goal, consider tightening these querysets, e.g.:
def get_queryset(self): - return self.filter_queryset( - super() - .get_queryset() - .filter(workspace__slug=self.kwargs.get("slug")) + return self.filter_queryset( + super() + .get_queryset() + .filter(workspace__slug=self.kwargs.get("slug")) + .filter(project_pages__deleted_at__isnull=True) @@ - .annotate( - project=Exists( - ProjectPage.objects.filter(page_id=OuterRef("id"), project_id=self.kwargs.get("project_id")) - ) - ) + .annotate( + project=Exists( + ProjectPage.objects.filter( + page_id=OuterRef("id"), + project_id=self.kwargs.get("project_id"), + deleted_at__isnull=True, + ) + ) + )and similarly in
summary:- queryset = ( - Page.objects.filter(workspace__slug=slug) + queryset = ( + Page.objects.filter(workspace__slug=slug, project_pages__deleted_at__isnull=True) @@ - .annotate( - project=Exists( - ProjectPage.objects.filter(page_id=OuterRef("id"), project_id=self.kwargs.get("project_id")) - ) - ) + .annotate( + project=Exists( + ProjectPage.objects.filter( + page_id=OuterRef("id"), + project_id=self.kwargs.get("project_id"), + deleted_at__isnull=True, + ) + ) + )This would make visibility and operations agree on what “belongs to this project” means.
Also applies to: 77-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/page/base.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/app/views/page/base.py (1)
apps/api/plane/db/models/page.py (1)
Page(19-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Agent
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/app/views/page/base.py (1)
497-532: Description endpoints now correctly honour non-deleted project-page associationsThe added
project_pages__deleted_at__isnull=Truefilter in bothPagesDescriptionViewSet.retrieveand.partial_update, combined with the existingif page is Nonechecks, ensures that description reads/updates only operate on pages still actively associated with the project and return a clean 404 when not. This is consistent with the rest of the non-deleted-page logic.
… PageDuplicateEndpoint to enhance clarity
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/plane/app/views/page/base.py (2)
198-225:retrievecan still raise an AttributeError when the page is missingIn
retrieve, you do:page = self.get_queryset().filter(pk=page_id).first() project = Project.objects.get(pk=project_id) ... if ( ProjectMember.objects.filter(...).exists() and not project.guest_view_all_features and not page.owned_by == request.user ): ...and only afterwards:
if page is None: return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)If
pageisNone(e.g., invalidpage_id, soft-deleted project link onceget_querysetis updated, or permission-filtered out), the guest check will hitpage.owned_byand raise anAttributeError, returning a 500 instead of a 404.Reordering fixes this:
- page = self.get_queryset().filter(pk=page_id).first() - project = Project.objects.get(pk=project_id) + page = self.get_queryset().filter(pk=page_id).first() + if page is None: + return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND) + + project = Project.objects.get(pk=project_id) track_visit = request.query_params.get("track_visit", "true").lower() == "true" @@ - if ( + if ( ProjectMember.objects.filter( workspace__slug=slug, project_id=project_id, member=request.user, role=5, is_active=True, ).exists() and not project.guest_view_all_features and not page.owned_by == request.user ):
152-157: Soft-delete filter is not guaranteed to apply to the same project, and.get()will return 500s when the page is missingAcross these queries you combine:
projects__id=project_idproject_pages__deleted_at__isnull=Trueon
Page.objects.get(...)/Page.objects.filter(...). Becauseprojectsandproject_pagesare different relation paths, Django will generate separate joins; thedeleted_atcondition is not guaranteed to be on the sameProjectPagerow asproject_id. A page that is soft-removed from this project but still attached (non-deleted) to another project can still match these filters.At the same time, all the
.get(...)calls here (lock,unlock,access,archive,unarchive,destroy,duplicate, and the mainpartial_updatepage fetch) will now raisePage.DoesNotExistwhenever the page–project link is soft-deleted or invalid, which surfaces as a 500 instead of a clean 404/403.I’d recommend:
- Correlate
project_idanddeleted_aton the same through rowUse the through relation only, so the same
ProjectPagerow must match both conditions, e.g.:- page = Page.objects.get( - pk=page_id, - workspace__slug=slug, - projects__id=project_id, - project_pages__deleted_at__isnull=True, - ) + page = Page.objects.get( + pk=page_id, + workspace__slug=slug, + project_pages__project_id=project_id, + project_pages__deleted_at__isnull=True, + )and similarly for:
- the parent lookup in
partial_updatelock,unlock,access,archive,unarchive,destroy- the child update in
destroyPagesDescriptionViewSet.retrieve/.partial_updatePageDuplicateEndpoint.post.This ensures “non-deleted for this project” is actually enforced.
- Normalize missing-page behaviour to 404 instead of 500
Wrap these
Page.objects.get(...)calls in atry/except Page.DoesNotExistand return a consistent 404 (or 403/400 if that better matches your API contract), e.g.:try: page = Page.objects.get( pk=page_id, workspace__slug=slug, project_pages__project_id=project_id, project_pages__deleted_at__isnull=True, ) except Page.DoesNotExist: return Response({"error": "Page not found"}, status=status.HTTP_404_NOT_FOUND)and apply the same pattern to the other methods using
.get(...).This both aligns the soft-delete semantics with the per-project view of a page and prevents new 500s when a page–project association is logically deleted.
Also applies to: 164-169, 243-248, 255-260, 269-274, 305-310, 336-341, 365-370, 393-398, 499-504, 524-529, 587-592
🧹 Nitpick comments (1)
apps/api/plane/app/views/page/base.py (1)
77-123: Soft-delete handling is inconsistent in list/summary and duplication flowsA couple of related places still ignore the
deleted_atstate ofProjectPage:
PageViewSet.get_queryset/summary:
- Both use
Exists(ProjectPage.objects.filter(page_id=OuterRef("id"), project_id=self.kwargs.get("project_id")))without filtering ondeleted_at.- Once you enforce
project_pages__deleted_at__isnull=Truein the detail endpoints, list/summary may still show/aggregate pages whose association to this project has been soft-deleted.
PageDuplicateEndpoint.post:
project_ids = ProjectPage.objects.filter(page_id=page_id).values_list("project_id", flat=True)will include projects where the page–project link is soft-deleted, potentially re-creating associations that were intentionally removed.For consistency with the rest of this PR and the bug being fixed, consider:
- Adding
deleted_at__isnull=Trueto theProjectPagesubqueries inget_querysetandsummary.- Restricting the duplication project set to active links only, e.g.:
- project_ids = ProjectPage.objects.filter(page_id=page_id).values_list("project_id", flat=True) + project_ids = ProjectPage.objects.filter( + page_id=page_id, + deleted_at__isnull=True, + ).values_list("project_id", flat=True)so that all page operations consistently reflect the non-deleted project–page relationships.
Also applies to: 418-463, 599-600
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/api/plane/app/views/page/base.py(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/app/views/page/base.py (1)
apps/api/plane/db/models/page.py (1)
Page(19-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
…nViewSet to improve efficiency
…n page queries (makeplane#8182) * fix: ensure soft delete handling for pages in PageViewSet methods * refactor: streamline query for project IDs in PageDuplicateEndpoint * refactor: remove soft delete condition from ProjectPage queries in PageViewSet and PageDuplicateEndpoint * refactor: simplify ProjectPage query in PageViewSet for improved readability * refactor: replace filter with get for Page queries in PageViewSet and PageDuplicateEndpoint to enhance clarity * refactor: replace filter with get for Page queries in PagesDescriptionViewSet to improve efficiency
…n page queries (makeplane#8182) * fix: ensure soft delete handling for pages in PageViewSet methods * refactor: streamline query for project IDs in PageDuplicateEndpoint * refactor: remove soft delete condition from ProjectPage queries in PageViewSet and PageDuplicateEndpoint * refactor: simplify ProjectPage query in PageViewSet for improved readability * refactor: replace filter with get for Page queries in PageViewSet and PageDuplicateEndpoint to enhance clarity * refactor: replace filter with get for Page queries in PagesDescriptionViewSet to improve efficiency
…n page queries (makeplane#8182) * fix: ensure soft delete handling for pages in PageViewSet methods * refactor: streamline query for project IDs in PageDuplicateEndpoint * refactor: remove soft delete condition from ProjectPage queries in PageViewSet and PageDuplicateEndpoint * refactor: simplify ProjectPage query in PageViewSet for improved readability * refactor: replace filter with get for Page queries in PageViewSet and PageDuplicateEndpoint to enhance clarity * refactor: replace filter with get for Page queries in PagesDescriptionViewSet to improve efficiency
Description
adding
project_pages__deleted_atfilter across all relevant database queries,Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.