Fix/tasks#44
Merged
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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: 4
🧹 Nitpick comments (2)
todos/tests.py (2)
314-315: Remove leftover debug prints.These add noise to local test runs and CI logs without validating behavior.
Also applies to: 409-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@todos/tests.py` around lines 314 - 315, Remove the leftover debug print statements that produce noise in test output by deleting the print(response.json()) calls found in todos/tests.py (the occurrences near the response variable around the test bodies shown in the diff, including the second occurrence reported). Locate the tests where response is used (e.g., the test functions that call the API and assign to response) and remove the print(...) lines; if needed, replace them with meaningful assertions on response.status_code or response.json() to validate behavior instead of printing.
398-410: Assert the state change, not just the 200.Both tests currently pass even if the endpoint returns success without actually changing
parent. Please refresh the task from the DB and assertchild.parent_id == parent.idafter convert, thenchild.parent_id is Noneafter promote.Also applies to: 412-427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@todos/tests.py` around lines 398 - 410, Update the tests to assert the actual DB state change instead of only checking HTTP 200: after calling the convert endpoint in test_convert_to_subtask_view reload the child task from persistence (e.g., via TaskService.get_task(child.id) or Task.objects.get(pk=child.id)) and assert child.parent_id == parent.id; likewise in the corresponding promote test (the one around 412-427) reload the child after the promote endpoint and assert child.parent_id is None. Ensure you reload the task after each request before making the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@todos/services/tests.py`:
- Line 381: The failing precondition compares different types (object vs UUID):
change the assertion so it compares the task-list IDs instead of the TaskList
instance to the UUID; e.g., use created_task.task_list.id (or
created_task.task_list.pk) and compare that to school_task_list.id so the test
actually verifies the task was not already pointing at the destination list
(references: created_task, task_list, school_task_list, id).
In `@todos/views.py`:
- Around line 467-468: Multiple generic "except Exception as e" handlers in
todos/views.py (the catch-alls at the shown locations) are returning raw
exception strings as HTTP 400s and leaking internals; replace each catch-all to
log the full exception (use the module logger or logger.exception) and return a
generic Response with status=status.HTTP_500_INTERNAL_SERVER_ERROR and a
non-sensitive message (e.g., "Internal server error"); keep existing 400/404
responses only for known validation paths and do not expose str(e) in responses.
Ensure you update each handler that currently matches "except Exception as e:"
(the occurrences around lines referenced) to follow this pattern.
- Around line 448-459: Serializer is instantiated without owner context so
TaskSerializer.validate (which reads self.context["owner_id"] for parent_id
ownership checks) rejects valid requests; update the instantiation in the create
flow that uses self.serializer_class (where serializer =
self.serializer_class(data=request.data)) to pass context={"owner_id": user_id}
(or merge with existing context) so validation can see owner_id before you copy
serializer.validated_data and call TaskService.create_task with owner_id and
validated_data.
- Around line 521-533: Patch code currently copies raw request.data and forwards
it to TaskService.update_task, allowing clients to overwrite protected fields;
instead, instantiate TaskSerializer with the existing Task instance and
request.data using partial=True, call is_valid(raise_exception=True), use
serializer.validated_data (not raw request.data) as the update payload, and map
parent_id to parent via the serializer or by converting
validated_data.pop("parent_id", None) into a Task.objects.get(...) before
calling TaskService.update_task(owner_id=user_id, task_id=task_id,
**validated_data); also explicitly remove or ignore any protected fields (e.g.,
owner_id, deleted, sync_status) from validated_data before passing to
TaskService.update_task.
---
Nitpick comments:
In `@todos/tests.py`:
- Around line 314-315: Remove the leftover debug print statements that produce
noise in test output by deleting the print(response.json()) calls found in
todos/tests.py (the occurrences near the response variable around the test
bodies shown in the diff, including the second occurrence reported). Locate the
tests where response is used (e.g., the test functions that call the API and
assign to response) and remove the print(...) lines; if needed, replace them
with meaningful assertions on response.status_code or response.json() to
validate behavior instead of printing.
- Around line 398-410: Update the tests to assert the actual DB state change
instead of only checking HTTP 200: after calling the convert endpoint in
test_convert_to_subtask_view reload the child task from persistence (e.g., via
TaskService.get_task(child.id) or Task.objects.get(pk=child.id)) and assert
child.parent_id == parent.id; likewise in the corresponding promote test (the
one around 412-427) reload the child after the promote endpoint and assert
child.parent_id is None. Ensure you reload the task after each request before
making the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08815b2e-39f6-4380-a8b5-ac169bee72eb
📒 Files selected for processing (6)
todos/services/__init__.pytodos/services/task_service.pytodos/services/tests.pytodos/tests.pytodos/urls.pytodos/views.py
Comment on lines
+448
to
+459
| serializer = self.serializer_class(data=request.data) | ||
| if not serializer.is_valid(): | ||
| return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) | ||
|
|
||
| try: | ||
| validated_data = serializer.validated_data.copy() | ||
| if "parent_id" in validated_data: | ||
| validated_data["parent"] = validated_data.pop("parent_id") | ||
|
|
||
| created_task = TaskService.create_task( | ||
| owner_id=user_id, | ||
| **validated_data, |
Contributor
There was a problem hiding this comment.
Pass owner_id into TaskSerializer on create.
TaskSerializer.validate() uses self.context["owner_id"] for parent_id ownership checks. Building the serializer without that context means valid create requests with parent_id will be rejected before TaskService.create_task() runs. See todos/serializers.py:310-341.
Suggested fix
- serializer = self.serializer_class(data=request.data)
+ serializer = self.serializer_class(
+ data=request.data,
+ context={"owner_id": user_id},
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@todos/views.py` around lines 448 - 459, Serializer is instantiated without
owner context so TaskSerializer.validate (which reads self.context["owner_id"]
for parent_id ownership checks) rejects valid requests; update the instantiation
in the create flow that uses self.serializer_class (where serializer =
self.serializer_class(data=request.data)) to pass context={"owner_id": user_id}
(or merge with existing context) so validation can see owner_id before you copy
serializer.validated_data and call TaskService.create_task with owner_id and
validated_data.
Comment on lines
+467
to
+468
| except Exception as e: | ||
| return Response({"message": str(e)}, status=status.HTTP_400_BAD_REQUEST) |
Contributor
There was a problem hiding this comment.
Stop returning raw exception strings as 400s.
These catch-alls turn unexpected server faults into client errors and leak internal exception details in the response body. Keep 400/404 for known validation paths, and log unknown failures before returning a generic 500.
Also applies to: 497-500, 548-551, 575-578, 612-615, 645-648, 678-681, 729-732, 780-783, 813-816
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 467-467: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@todos/views.py` around lines 467 - 468, Multiple generic "except Exception as
e" handlers in todos/views.py (the catch-alls at the shown locations) are
returning raw exception strings as HTTP 400s and leaking internals; replace each
catch-all to log the full exception (use the module logger or logger.exception)
and return a generic Response with status=status.HTTP_500_INTERNAL_SERVER_ERROR
and a non-sensitive message (e.g., "Internal server error"); keep existing
400/404 responses only for known validation paths and do not expose str(e) in
responses. Ensure you update each handler that currently matches "except
Exception as e:" (the occurrences around lines referenced) to follow this
pattern.
Comment on lines
+521
to
+533
| validated_data = request.data.copy() | ||
|
|
||
| # Rename parent_id to parent for update_task | ||
| if "parent_id" in validated_data: | ||
| parent_id = validated_data.pop("parent_id") | ||
| if parent_id: | ||
| validated_data["parent"] = Task.objects.get(id=parent_id) | ||
| else: | ||
| validated_data["parent"] = None | ||
|
|
||
| updated_task = TaskService.update_task( | ||
| owner_id=user_id, task_id=task_id, **validated_data | ||
| ) |
Contributor
There was a problem hiding this comment.
Don't mass-assign raw PATCH data into TaskService.update_task.
This path bypasses TaskSerializer entirely and forwards arbitrary request keys to TaskService.update_task(), which then setattr()s every field onto the model. A client can patch protected fields like owner_id, deleted, or sync_status, which is an auth/data-integrity bug.
Suggested direction
- validated_data = request.data.copy()
-
- # Rename parent_id to parent for update_task
- if "parent_id" in validated_data:
- parent_id = validated_data.pop("parent_id")
- if parent_id:
- validated_data["parent"] = Task.objects.get(id=parent_id)
- else:
- validated_data["parent"] = None
+ task = TaskService.get_task(user_id, task_id)
+ serializer = self.serializer_class(
+ task,
+ data=request.data,
+ partial=True,
+ context={"owner_id": user_id},
+ )
+ serializer.is_valid(raise_exception=True)
+ validated_data = serializer.validated_data.copy()
+ if "parent_id" in validated_data:
+ validated_data["parent"] = validated_data.pop("parent_id")
updated_task = TaskService.update_task(
owner_id=user_id, task_id=task_id, **validated_data
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@todos/views.py` around lines 521 - 533, Patch code currently copies raw
request.data and forwards it to TaskService.update_task, allowing clients to
overwrite protected fields; instead, instantiate TaskSerializer with the
existing Task instance and request.data using partial=True, call
is_valid(raise_exception=True), use serializer.validated_data (not raw
request.data) as the update payload, and map parent_id to parent via the
serializer or by converting validated_data.pop("parent_id", None) into a
Task.objects.get(...) before calling TaskService.update_task(owner_id=user_id,
task_id=task_id, **validated_data); also explicitly remove or ignore any
protected fields (e.g., owner_id, deleted, sync_status) from validated_data
before passing to TaskService.update_task.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of UpdateTaskView.patch method
rg -n "validated_data|serializer_class|is_valid|partial=True" todos/views.py -A2 -B2 | grep -A30 "class UpdateTaskView"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get the UpdateTaskView class implementation
ast-grep --pattern 'class UpdateTaskView($$$) {
$$$
}'
# Also try with rg to get the patch method content
rg -n "UpdateTaskView|validated_data = request.data|serializer_class\(.*partial" todos/views.py -C3
Length of output: 668
@IamMuuo, after checking the current branch, the fix doesn't appear to be in place yet. Line 521 of todos/views.py still reads:
validated_data = request.data.copy()The raw request.data is still being forwarded directly to TaskService.update_task without serializer validation. Here's the fix that needs to be applied to UpdateTaskView.patch:
def patch(self, request, task_id, *args, **kwargs):
user_id, error_response = self.get_user_id(request)
if error_response:
return error_response
try:
task = TaskService.get_task(user_id, task_id)
serializer = self.serializer_class(
task,
data=request.data,
partial=True,
context={"owner_id": user_id},
)
if not serializer.is_valid():
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
validated_data = serializer.validated_data.copy()
if "parent_id" in validated_data:
parent_id = validated_data.pop("parent_id")
validated_data["parent"] = Task.objects.get(id=parent_id) if parent_id else None
updated_task = TaskService.update_task(
owner_id=user_id, task_id=task_id, **validated_data
)
return Response(
data=self.serializer_class(updated_task).data,
status=status.HTTP_200_OK,
)
except Task.DoesNotExist:
return Response(
{"message": "Task not found"},
status=status.HTTP_404_NOT_FOUND,
)
except ValueError as e:
return Response(
{"message": str(e)},
status=status.HTTP_400_BAD_REQUEST,
)
except Exception as e:
return Response(
{"message": str(e)},
status=status.HTTP_400_BAD_REQUEST,
)Could you push the fix to the branch? The key change is routing request.data through TaskSerializer with partial=True before passing to TaskService.
…tasks Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
TaskSerializer.validate() uses self.context[owner_id] for parent_id ownership checks. Building the serializer without that context means valid create requests with parent_id will be rejected before TaskService.create_task()
…into fix/tasks
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
2 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.
Enhanced Task management API
Summary by CodeRabbit
New Features
Bug Fixes
Chores