[SILO-1028] feat: Project Summary external API#8661
Open
Saurabhkmr98 wants to merge 2 commits intopreviewfrom
Open
[SILO-1028] feat: Project Summary external API#8661Saurabhkmr98 wants to merge 2 commits intopreviewfrom
Saurabhkmr98 wants to merge 2 commits intopreviewfrom
Conversation
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
Saurabhkmr98
changed the title
Contributor
📝 WalkthroughWalkthroughAdds project summary and full estimate management (estimates and estimate points) endpoints, updates serializers and model choice for Estimate.type, adds migration, and expands OpenAPI parameters/examples and URL routing; permissions and validation are applied across new endpoints. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as ProjectSummaryAPIEndpoint
participant Auth as Permissions
participant DB as Database
Client->>API: GET /workspaces/{slug}/projects/{id}/summary?fields=members,issues,...
API->>Auth: check WorkSpaceAdminPermission
Auth-->>API: allowed / denied
API->>DB: aggregated ORM query (per-field subqueries + Coalesce)
DB-->>API: annotated counts
API-->>Client: 200 OK with JSON summary
sequenceDiagram
participant Client as Client
participant API as EstimatesAPI
participant Auth as Permissions
participant DB as Database
participant Serializer as Serializer/Validation
Client->>API: POST/GET/PATCH/DELETE /workspaces/{slug}/projects/{id}/estimate...
API->>Auth: check ProjectEntityPermission
Auth-->>API: allowed / denied
API->>Serializer: validate request data (Estimate / EstimatePoint)
Serializer-->>API: valid / errors
API->>DB: create/read/update/delete estimate or estimate points
DB-->>API: persisted/read objects
API-->>Client: 2XX or 4XX/409 responses (with OpenAPI docs)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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: 1
🧹 Nitpick comments (1)
apps/api/plane/api/views/project.py (1)
577-595: Two DB round-trips, not one as the comment claims.Line 582 executes a
Project.objects.filter(...).first()to check existence and fetchproject.id/project.name. Line 666 inside_get_all_summary_countsruns a second query for the annotations. You can fold the name/id retrieval into the annotated query by adding"id"and"name"tovalues_list, removing the first lookup entirely.♻️ Sketch — single-query approach
def get(self, request, slug, project_id): - project = Project.objects.filter(pk=project_id, workspace__slug=slug).first() - if not project: - return Response({"error": "Project not found"}, status=status.HTTP_404_NOT_FOUND) fields = request.GET.get("fields", "").split(",") requested_fields = set(filter(None, (f.strip() for f in fields))) & set(ALLOWED_PROJECT_SUMMARY_FIELDS) if not requested_fields: requested_fields = set(ALLOWED_PROJECT_SUMMARY_FIELDS) - counts = self._get_all_summary_counts(project_id, requested_fields) - summary = {field: counts[field] for field in requested_fields} - summary["project_id"] = project.id - summary["project_name"] = project.name + result = self._get_all_summary_counts(project_id, slug, requested_fields) + if result is None: + return Response({"error": "Project not found"}, status=status.HTTP_404_NOT_FOUND) + summary = {field: result[field] for field in requested_fields} + summary["project_id"] = result["id"] + summary["project_name"] = result["name"] return Response(summary, status=status.HTTP_200_OK)Then in
_get_all_summary_counts, filter by bothpkandworkspace__slug, and include"id"and"name"invalues_list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/project.py` around lines 577 - 595, The current get() does an extra Project.objects.filter(...).first() DB hit; remove that initial lookup and rely on _get_all_summary_counts to both compute counts and return the project's id and name in the same query: change _get_all_summary_counts to filter by pk and workspace__slug (use both slug and project_id), include "id" and "name" in its values_list/annotations and return them along with the counts, then in get() call that method and extract project_id/project_name from its result instead of doing Project.objects.filter(...).first(); this collapses the two DB round-trips into a single query.
🤖 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/project.py`:
- Around line 573-575: ProjectSummaryAPIEndpoint currently uses
WorkSpaceAdminPermission (allowing Admin and Member) while other project
endpoints use ProjectBasePermission (which permits any workspace member on GET);
update ProjectSummaryAPIEndpoint.permission_classes to ProjectBasePermission to
match the pattern unless the intent is to restrict summary counts to admins
only—if admin-only behavior is intended, add an explicit comment explaining that
and keep WorkSpaceAdminPermission; reference the class name
ProjectSummaryAPIEndpoint and the permission symbols WorkSpaceAdminPermission
and ProjectBasePermission when making the change.
---
Nitpick comments:
In `@apps/api/plane/api/views/project.py`:
- Around line 577-595: The current get() does an extra
Project.objects.filter(...).first() DB hit; remove that initial lookup and rely
on _get_all_summary_counts to both compute counts and return the project's id
and name in the same query: change _get_all_summary_counts to filter by pk and
workspace__slug (use both slug and project_id), include "id" and "name" in its
values_list/annotations and return them along with the counts, then in get()
call that method and extract project_id/project_name from its result instead of
doing Project.objects.filter(...).first(); this collapses the two DB round-trips
into a single query.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/plane/api/urls/project.pyapps/api/plane/api/views/__init__.pyapps/api/plane/api/views/project.py
Comment on lines
+573
to
+575
| class ProjectSummaryAPIEndpoint(BaseAPIView): | ||
| permission_classes = [WorkSpaceAdminPermission] | ||
| use_read_replica = True |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check WorkSpaceAdminPermission implementation
echo "=== WorkSpaceAdminPermission ==="
rg -n "class WorkSpaceAdminPermission" --type py -A 15
echo ""
echo "=== ProjectBasePermission ==="
rg -n "class ProjectBasePermission" --type py -A 15
echo ""
echo "=== ProjectSummaryAPIEndpoint context ==="
rg -n "class ProjectSummaryAPIEndpoint" --type py -B 2 -A 20Repository: makeplane/plane
Length of output: 7442
🏁 Script executed:
#!/bin/bash
# Check what role types exist
echo "=== Role definitions (Admin, Member constants) ==="
rg -n "^(Admin|Member)\s*=" --type py | head -20
echo ""
echo "=== Other ProjectAPIView endpoints and their permissions ==="
rg -n "class Project.*APIView.*|permission_classes" apps/api/plane/api/views/project.py | head -40
echo ""
echo "=== Check if ProjectBasePermission is used elsewhere ==="
rg -l "ProjectBasePermission" --type pyRepository: makeplane/plane
Length of output: 1141
🏁 Script executed:
#!/bin/bash
# Get full ProjectBasePermission implementation
echo "=== Full ProjectBasePermission implementation ==="
rg -n "class ProjectBasePermission" --type py -A 40 | head -60
echo ""
echo "=== Check if there are project-level roles/permissions ==="
rg -n "ProjectMember|project.*role" --type py -i | grep -E "(class|def)" | head -20
echo ""
echo "=== Check context around ProjectSummaryAPIEndpoint for comments/rationale ==="
rg -n "class ProjectSummaryAPIEndpoint" --type py -B 10 -A 30Repository: makeplane/plane
Length of output: 10560
Reconsider permission consistency — WorkSpaceAdminPermission allows both admin and member roles, but other project endpoints use ProjectBasePermission
WorkSpaceAdminPermission allows workspace members with role Admin or Member to access this endpoint. However, all other project API endpoints (ProjectListCreateAPIEndpoint, ProjectDetailAPIEndpoint, ProjectArchiveUnarchiveAPIEndpoint) use ProjectBasePermission, which for GET requests permits any workspace member. This endpoint is inconsistent with the pattern. If project summary counts are sensitive and should be restricted to admins/members only, confirm this is intentional; otherwise, consider using ProjectBasePermission for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/api/views/project.py` around lines 573 - 575,
ProjectSummaryAPIEndpoint currently uses WorkSpaceAdminPermission (allowing
Admin and Member) while other project endpoints use ProjectBasePermission (which
permits any workspace member on GET); update
ProjectSummaryAPIEndpoint.permission_classes to ProjectBasePermission to match
the pattern unless the intent is to restrict summary counts to admins only—if
admin-only behavior is intended, add an explicit comment explaining that and
keep WorkSpaceAdminPermission; reference the class name
ProjectSummaryAPIEndpoint and the permission symbols WorkSpaceAdminPermission
and ProjectBasePermission when making the change.
dheeru0198
previously approved these changes
Feb 26, 2026
Member
|
Ideally if we are creating a structure like this, it will be scalable. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/plane/api/views/project.py (2)
591-591: Redundant dict comprehension.
_get_all_summary_countsalready returns a dict containing exactly therequested_fields, so this comprehension just rebuilds the same dict.♻️ Suggested simplification
- counts_dict = {field: counts[field] for field in requested_fields} + counts_dict = counts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/project.py` at line 591, The dict comprehension rebuilding counts_dict is redundant because _get_all_summary_counts already returns a dict keyed by requested_fields; replace the reconstruction by using the returned dict directly (e.g., use the counts variable returned from _get_all_summary_counts where counts_dict is currently used) and remove the comprehension that reassigns counts_dict = {field: counts[field] for field in requested_fields}; ensure any references to counts_dict now point to counts (or rename counts to counts_dict if you prefer that identifier).
576-598: Missing OpenAPI documentation for this endpoint.All other endpoints in this file use
@project_docsdecorator for API documentation. Consider adding documentation for consistency with the rest of the file and to ensure the external API is properly documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/api/views/project.py` around lines 576 - 598, Add the missing OpenAPI documentation by decorating the get method with the same `@project_docs` decorator pattern used elsewhere in this file; update the get method (function name: get) to include a `@project_docs` call that documents the endpoint summary/description, query parameter "fields" (allowed values from ALLOWED_PROJECT_SUMMARY_FIELDS), response schema (including id, name, identifier, counts) and 404/200 responses so the endpoint is included in the generated API docs and consistent with other endpoints in this module.
🤖 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/project.py`:
- Around line 609-614: The new "members" subquery on ProjectMember (the lambda
returning ProjectMember.objects.filter(project_id=OuterRef("pk"),
is_active=True).values("project_id").annotate(count=Count("*")).values("count"))
is missing the same bot-exclusion used elsewhere; update that filter to include
member__is_bot=False so it matches the existing total_members logic (i.e., add
member__is_bot=False to the filter on ProjectMember in the members lambda) to
ensure consistent member counts across endpoints.
---
Nitpick comments:
In `@apps/api/plane/api/views/project.py`:
- Line 591: The dict comprehension rebuilding counts_dict is redundant because
_get_all_summary_counts already returns a dict keyed by requested_fields;
replace the reconstruction by using the returned dict directly (e.g., use the
counts variable returned from _get_all_summary_counts where counts_dict is
currently used) and remove the comprehension that reassigns counts_dict =
{field: counts[field] for field in requested_fields}; ensure any references to
counts_dict now point to counts (or rename counts to counts_dict if you prefer
that identifier).
- Around line 576-598: Add the missing OpenAPI documentation by decorating the
get method with the same `@project_docs` decorator pattern used elsewhere in this
file; update the get method (function name: get) to include a `@project_docs` call
that documents the endpoint summary/description, query parameter "fields"
(allowed values from ALLOWED_PROJECT_SUMMARY_FIELDS), response schema (including
id, name, identifier, counts) and 404/200 responses so the endpoint is included
in the generated API docs and consistent with other endpoints in this module.
Comment on lines
+609
to
+614
| "members": lambda: ( | ||
| ProjectMember.objects.filter(project_id=OuterRef("pk"), is_active=True) | ||
| .values("project_id") | ||
| .annotate(count=Count("*")) | ||
| .values("count") | ||
| ), |
Contributor
There was a problem hiding this comment.
Missing member__is_bot=False filter — inconsistent with existing member count logic.
The existing total_members annotation at lines 105-106 and 320-321 filters out bot members with member__is_bot=False, but this new members subquery does not include that filter. This inconsistency will cause different member counts between the project list/detail endpoints and this summary endpoint.
🐛 Proposed fix
"members": lambda: (
- ProjectMember.objects.filter(project_id=OuterRef("pk"), is_active=True)
+ ProjectMember.objects.filter(project_id=OuterRef("pk"), is_active=True, member__is_bot=False)
.values("project_id")
.annotate(count=Count("*"))
.values("count")
),📝 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.
Suggested change
| "members": lambda: ( | |
| ProjectMember.objects.filter(project_id=OuterRef("pk"), is_active=True) | |
| .values("project_id") | |
| .annotate(count=Count("*")) | |
| .values("count") | |
| ), | |
| "members": lambda: ( | |
| ProjectMember.objects.filter(project_id=OuterRef("pk"), is_active=True, member__is_bot=False) | |
| .values("project_id") | |
| .annotate(count=Count("*")) | |
| .values("count") | |
| ), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/api/views/project.py` around lines 609 - 614, The new
"members" subquery on ProjectMember (the lambda returning
ProjectMember.objects.filter(project_id=OuterRef("pk"),
is_active=True).values("project_id").annotate(count=Count("*")).values("count"))
is missing the same bot-exclusion used elsewhere; update that filter to include
member__is_bot=False so it matches the existing total_members logic (i.e., add
member__is_bot=False to the filter on ProjectMember in the members lambda) to
ensure consistent member counts across endpoints.
52a255f to
dda0739
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/api/plane/utils/openapi/examples.py (2)
690-698: Minor inconsistency:ESTIMATE_EXAMPLEmissing fields present inSAMPLE_ESTIMATE.The
ESTIMATE_EXAMPLEresponse example lackstype,last_used, andcreated_atfields that are present inSAMPLE_ESTIMATE(lines 867-874). Consider aligning these for consistency in the API documentation.🔧 Suggested enhancement
ESTIMATE_EXAMPLE = OpenApiExample( name="Estimate", value={ "id": "550e8400-e29b-41d4-a716-446655440000", "name": "Estimate 1", "description": "Estimate 1 description", + "type": "categories", + "last_used": False, + "created_at": "2024-01-01T10:30:00Z", }, description="Example response for an estimate", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/utils/openapi/examples.py` around lines 690 - 698, Update the OpenApiExample ESTIMATE_EXAMPLE so its value includes the same fields as SAMPLE_ESTIMATE: add "type", "last_used", and "created_at" (use the same example values/formats as SAMPLE_ESTIMATE) to keep the API documentation consistent; edit the ESTIMATE_EXAMPLE object (the OpenApiExample instance named ESTIMATE_EXAMPLE) to include these keys in its value dictionary.
710-725: Create and update examples are identical.
ESTIMATE_CREATE_EXAMPLEandESTIMATE_UPDATE_EXAMPLEhave identical values. This is acceptable if the API truly uses the same schema for both operations, but consider differentiating them slightly (e.g., "Updated Estimate 1" for the update example) to make the documentation clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/utils/openapi/examples.py` around lines 710 - 725, The create and update OpenApiExample instances currently use identical example payloads; update ESTIMATE_UPDATE_EXAMPLE (the OpenApiExample named "EstimateUpdateSerializer") to show a distinct updated payload (e.g., change "name" to "Estimate 1 Updated" and/or adjust "description" to "Updated Estimate 1 description") so the examples differ and clearly communicate the update operation.apps/api/plane/utils/openapi/parameters.py (1)
499-505: Consider adding an example for consistency with other parameters.Other ID parameters in this file (e.g.,
CYCLE_ID_PARAMETER,MODULE_ID_PARAMETER,STATE_ID_PARAMETER) includeOpenApiExampleinstances to enhance API documentation. Adding a similar example here would maintain consistency.🔧 Suggested enhancement
ESTIMATE_ID_PARAMETER = OpenApiParameter( name="estimate_id", description="Estimate ID", required=True, type=OpenApiTypes.UUID, location=OpenApiParameter.PATH, + examples=[ + OpenApiExample( + name="Example estimate ID", + value="550e8400-e29b-41d4-a716-446655440000", + description="A typical estimate UUID", + ) + ], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/plane/utils/openapi/parameters.py` around lines 499 - 505, ESTIMATE_ID_PARAMETER lacks an OpenApiExample like the other ID params; update the ESTIMATE_ID_PARAMETER definition to include an examples=[OpenApiExample(...)] entry (mirroring style used by CYCLE_ID_PARAMETER / MODULE_ID_PARAMETER / STATE_ID_PARAMETER) so the OpenAPI docs show a sample UUID and descriptive name for the estimate_id path parameter.
🤖 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/estimate.py`:
- Around line 26-32: The validate method in the serializer (validate)
incorrectly rejects partial updates because of the "if not data" check and uses
a confusing error message and a stricter length cap than the model; remove or
relax the empty-data guard so partial PATCH requests are allowed (only raise if
no fields provided AND this serializer is used for full-create flows), change
the message "Estimate points are required" to something specific like "Estimate
value is required" when you do intend to require the field, and either align the
value length check with the model's max_length=255 or add a clear
comment/docstring explaining the intentional 20-character business rule; update
the validate method (function name: validate) accordingly to implement these
changes.
In `@apps/api/plane/api/views/estimate.py`:
- Around line 47-69: The code in post() calls Project.objects.get(...) and
Workspace.objects.get(...), which will raise exceptions instead of returning
None, so replace those calls with Project.objects.filter(id=project_id,
workspace__slug=slug).first() and Workspace.objects.filter(slug=slug).first()
(or equivalent) to match the existing pattern used for project_estimate; then
keep the existing if not project / if not workspace 404 responses intact so
missing records return 404 instead of raising. Ensure you update any references
to the retrieved objects (project, workspace) and maintain serializer
instantiation via self.serializer_class(..., context={"workspace": workspace,
"project": project}).
---
Nitpick comments:
In `@apps/api/plane/utils/openapi/examples.py`:
- Around line 690-698: Update the OpenApiExample ESTIMATE_EXAMPLE so its value
includes the same fields as SAMPLE_ESTIMATE: add "type", "last_used", and
"created_at" (use the same example values/formats as SAMPLE_ESTIMATE) to keep
the API documentation consistent; edit the ESTIMATE_EXAMPLE object (the
OpenApiExample instance named ESTIMATE_EXAMPLE) to include these keys in its
value dictionary.
- Around line 710-725: The create and update OpenApiExample instances currently
use identical example payloads; update ESTIMATE_UPDATE_EXAMPLE (the
OpenApiExample named "EstimateUpdateSerializer") to show a distinct updated
payload (e.g., change "name" to "Estimate 1 Updated" and/or adjust "description"
to "Updated Estimate 1 description") so the examples differ and clearly
communicate the update operation.
In `@apps/api/plane/utils/openapi/parameters.py`:
- Around line 499-505: ESTIMATE_ID_PARAMETER lacks an OpenApiExample like the
other ID params; update the ESTIMATE_ID_PARAMETER definition to include an
examples=[OpenApiExample(...)] entry (mirroring style used by CYCLE_ID_PARAMETER
/ MODULE_ID_PARAMETER / STATE_ID_PARAMETER) so the OpenAPI docs show a sample
UUID and descriptive name for the estimate_id path parameter.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/api/plane/api/serializers/__init__.pyapps/api/plane/api/serializers/estimate.pyapps/api/plane/api/urls/estimate.pyapps/api/plane/api/views/estimate.pyapps/api/plane/db/migrations/0121_alter_estimate_type.pyapps/api/plane/db/models/estimate.pyapps/api/plane/utils/openapi/__init__.pyapps/api/plane/utils/openapi/decorators.pyapps/api/plane/utils/openapi/examples.pyapps/api/plane/utils/openapi/parameters.py
Comment on lines
26
to
32
| def validate(self, data): | ||
| if not data: | ||
| raise serializers.ValidationError("Estimate points are required") | ||
| value = data.get("value") | ||
| if value and len(value) > 20: | ||
| raise serializers.ValidationError("Value can't be more than 20 characters") | ||
| return data |
Contributor
There was a problem hiding this comment.
Validation logic may be overly restrictive for partial updates.
-
Line 27:
if not datawill reject valid partial update requests where only some fields are provided. Consider whether this check is necessary. -
Error message: "Estimate points are required" is confusing when this serializer handles a single point, not multiple points.
-
Line 30: The 20-character limit on
valueis more restrictive than the model'smax_length=255. This appears intentional based on the OpenAPI examples, but consider documenting this business rule.
🐛 Suggested fix for the empty data check
def validate(self, data):
- if not data:
- raise serializers.ValidationError("Estimate points are required")
value = data.get("value")
if value and len(value) > 20:
- raise serializers.ValidationError("Value can't be more than 20 characters")
+ raise serializers.ValidationError("Value cannot exceed 20 characters")
return data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/api/serializers/estimate.py` around lines 26 - 32, The
validate method in the serializer (validate) incorrectly rejects partial updates
because of the "if not data" check and uses a confusing error message and a
stricter length cap than the model; remove or relax the empty-data guard so
partial PATCH requests are allowed (only raise if no fields provided AND this
serializer is used for full-create flows), change the message "Estimate points
are required" to something specific like "Estimate value is required" when you
do intend to require the field, and either align the value length check with the
model's max_length=255 or add a clear comment/docstring explaining the
intentional 20-character business rule; update the validate method (function
name: validate) accordingly to implement these changes.
apps/api/plane/api/views/estimate.py
Outdated
Comment on lines
47
to
69
| def post(self, request, slug, project_id): | ||
| project = Project.objects.get(id=project_id, workspace__slug=slug) | ||
| if not project: | ||
| return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Project not found"}) | ||
|
|
||
| workspace = Workspace.objects.get(slug=slug) | ||
| if not workspace: | ||
| return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Workspace not found"}) | ||
|
|
||
| project_estimate = self.get_queryset().first() | ||
| if project_estimate: | ||
| # return 409 if the project estimate already exists | ||
| return Response( | ||
| status=status.HTTP_409_CONFLICT, | ||
| data={"error": "An estimate already exists for this project", "id": str(project_estimate.id)}, | ||
| ) | ||
| # create the project estimate | ||
| serializer = self.serializer_class(data=request.data, context={"workspace": workspace, "project": project}) | ||
| if not serializer.is_valid(): | ||
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| serializer.save() | ||
| return Response(serializer.data, status=status.HTTP_201_CREATED) |
Contributor
There was a problem hiding this comment.
Model.objects.get() raises exception; subsequent checks are dead code.
Lines 48-54 use .get() which raises ObjectDoesNotExist when no matching record exists. The if not project: and if not workspace: checks are never reached because the exception is thrown first—likely resulting in a 500 error instead of the intended 404.
Use .filter().first() consistently with other methods in this file.
🐛 Proposed fix
def post(self, request, slug, project_id):
- project = Project.objects.get(id=project_id, workspace__slug=slug)
+ project = Project.objects.filter(id=project_id, workspace__slug=slug).first()
if not project:
return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Project not found"})
- workspace = Workspace.objects.get(slug=slug)
+ workspace = Workspace.objects.filter(slug=slug).first()
if not workspace:
return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Workspace not found"})📝 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.
Suggested change
| def post(self, request, slug, project_id): | |
| project = Project.objects.get(id=project_id, workspace__slug=slug) | |
| if not project: | |
| return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Project not found"}) | |
| workspace = Workspace.objects.get(slug=slug) | |
| if not workspace: | |
| return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Workspace not found"}) | |
| project_estimate = self.get_queryset().first() | |
| if project_estimate: | |
| # return 409 if the project estimate already exists | |
| return Response( | |
| status=status.HTTP_409_CONFLICT, | |
| data={"error": "An estimate already exists for this project", "id": str(project_estimate.id)}, | |
| ) | |
| # create the project estimate | |
| serializer = self.serializer_class(data=request.data, context={"workspace": workspace, "project": project}) | |
| if not serializer.is_valid(): | |
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | |
| serializer.save() | |
| return Response(serializer.data, status=status.HTTP_201_CREATED) | |
| def post(self, request, slug, project_id): | |
| project = Project.objects.filter(id=project_id, workspace__slug=slug).first() | |
| if not project: | |
| return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Project not found"}) | |
| workspace = Workspace.objects.filter(slug=slug).first() | |
| if not workspace: | |
| return Response(status=status.HTTP_404_NOT_FOUND, data={"error": "Workspace not found"}) | |
| project_estimate = self.get_queryset().first() | |
| if project_estimate: | |
| # return 409 if the project estimate already exists | |
| return Response( | |
| status=status.HTTP_409_CONFLICT, | |
| data={"error": "An estimate already exists for this project", "id": str(project_estimate.id)}, | |
| ) | |
| # create the project estimate | |
| serializer = self.serializer_class(data=request.data, context={"workspace": workspace, "project": project}) | |
| if not serializer.is_valid(): | |
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | |
| serializer.save() | |
| return Response(serializer.data, status=status.HTTP_201_CREATED) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/plane/api/views/estimate.py` around lines 47 - 69, The code in
post() calls Project.objects.get(...) and Workspace.objects.get(...), which will
raise exceptions instead of returning None, so replace those calls with
Project.objects.filter(id=project_id, workspace__slug=slug).first() and
Workspace.objects.filter(slug=slug).first() (or equivalent) to match the
existing pattern used for project_estimate; then keep the existing if not
project / if not workspace 404 responses intact so missing records return 404
instead of raising. Ensure you update any references to the retrieved objects
(project, workspace) and maintain serializer instantiation via
self.serializer_class(..., context={"workspace": workspace, "project":
project}).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Screenshots and Media (if applicable)
Sample response
API Query Profile
Test Scenarios
References
Summary by CodeRabbit
New Features
Documentation
Database