Possum - Mumina A. by MuminaA · Pull Request #16 · Ada-C24/task-list-api

MuminaA · 2025-11-07T00:43:12Z

Task list requirements completed
All tests passing
Deployed to render
one-to-many relationship created for goal and tasks
code refactored

Wave 1

wave 2 complete, all tests passing

incomplete and complete tasks created and all test passing

slack api connected and working

Wave 5

Wave 6 complete, connected goals to tasks, tests passing

completed wave 7 refactoring, completed all test

anselrognlie

Looks good overall, but please review my comments, and let me know if you have any questions.

anselrognlie · 2025-11-17T04:11:23Z

+    assert isinstance(response_body, dict)
+    assert "message" in response_body


These two checks aren't really essential. Being able to access the result like a dict (by key) and the presense of the key itself would be enforced by the final check for the value of response_body["message"]. And if we really want to make sure that it's actually a dictionary, with only the expected keys, we could compare the result with a dictionary literal containing only the data we expect to see.

assert response_body == {"message": "Task 1 not found"}

It looks like a number of following tests took a similar approach. Similar feedback applies.

anselrognlie · 2025-11-17T04:15:14Z

-    # assertion 2 goes here
+    assert isinstance(response_body, dict)
+    assert "message" in response_body
+    assert response_body["message"] == "Goal 1 not found"


In subsequent tests, you checked the status code, but we should do so here as well.

anselrognlie · 2025-11-17T04:20:29Z

+    query = db.select(Goal).where(Goal.id == 1)
+    assert db.session.scalar(query) == None


In other tests, you checked the message string in the response, but we should do so here as well.

We don't really need to perform a database lookup here. The follow-up request already received a 404, meaning the record was not found (so it was deleted). Querying the database, or sending a follow up request are two ways that can be used to check a result. Querying the database requires this test to make assumptions about how the deletion was handled (that it was actually removed), while performing a follow-up request allows the backend its own implementation (maybe it just marks a record as deleted without actually deleting it). So a case can be made for either approach, but we don't really need both in a single test.

anselrognlie · 2025-11-17T04:25:37Z

+    assert isinstance(response_body, dict)
+    assert "message" in response_body
+    assert response_body["message"] == "Goal 1 not found"
+    assert db.session.scalars(db.select(Goal)).all() == []


🔎 It would be particularly surprising for the attempt to delete an unknown Goal to result in a Goal record being added to the database. So there's no need to confirm that the database is still empty.

anselrognlie · 2025-11-17T04:30:03Z

    # Test that the correct status code and response message are returned
    response = e.value.get_response()
    assert response.status_code == 400
+    assert response.json == {"message": "Task One invalid"}


👍 Nice job making use of the response we're able to extract from the error value.

anselrognlie · 2025-11-17T05:45:56Z

+    goal_response = []
+
+    for goal in goals:
+        goal_response.append(goal.to_dict())


List comprehension opportunity.

anselrognlie · 2025-11-17T06:00:00Z

+    from typing import TYPE_CHECKING
+    if TYPE_CHECKING:
+        from .goal import Goal


👍 Nice use of this snippet to suppress the squiggles in VS Code.

anselrognlie · 2025-11-17T06:00:30Z

+    return Response(status=204, mimetype='application/json')
+
+@bp.post('/<goal_id>/tasks')
+def create_task_with_goal(goal_id):


While a typical reading of this endpoint (POST /goals/id/tasks) would be to create a Task associated with the specified goal, that's not how it was defined. A name such as associate_tasks_to_goal might be more descriptive.

anselrognlie · 2025-11-17T06:02:13Z

+    # remove all current tasks from this goal
+    for task in goal.tasks:
+        task.goal_id = None
+
+    # For each task ID, find the task and associate it with the goal
+    for task_id in task_ids:
+        task = validate_model(Task, task_id)
+        task.goal_id = goal.id


Since we need to validate each Task id anyway, if we stored the resulting tasks in a list, the replacement of the tasks for the goal could be accomplished using the tasks property as

goal.tasks = tasks

which avoids the need to manually clear the goal association from the existing tasks.

anselrognlie · 2025-11-17T06:03:08Z

+    response = goal.to_dict()
+    response["tasks"] = tasks


👍 Nice reuse of your to_dict methods to build up your response. Rather than leaving this logic here in the route, we could move it to an additional Goal method. Perhaps to_dict_with_tasks, or we could add a parameter to the main Goal to_dict that determines whether or not to include the Task details.

anselrognlie

👍 Thanks for the updates. Looks good!

anselrognlie · 2025-11-20T01:07:54Z


    # Turn this Task object into a dictionary so we can send it as JSON in a response
-    def to_dict(self, include_goal=False):
+    def to_dict(self, include_goal: Optional[bool] = None):


👍 If you still wanted to have a way for a caller to override the goal behavior, this is a reasonable way to do it.

As I already wrote, I think it's a little odd that goal_id is conditionally included at all (I would have always included it, and just let it return null for tasks not part of a goal for shape consistency), but it does make sense for the way the waves are structured.

anselrognlie · 2025-11-20T01:11:23Z

 def get_task_for_goal(goal_id):
    goal = validate_model(Goal, goal_id)
-    tasks = [task.to_dict(include_goal=True) for task in goal.tasks]
+    tasks = [task.to_dict() for task in goal.tasks]


👍 Modifying the default behavior in to_dict lets us omit the extra arguemnt where it was previous needed, and it makes this behavior uniform across any other endpoints that currently (or would in future) be part of a goal.

anselrognlie · 2025-11-20T01:17:14Z

+
+def test_to_dict_includes_goal_id_when_set(app, one_task_belongs_to_one_goal):
+    task = db.session.scalar(db.select(Task).where(Task.id == 1))
+    assert task is not None


We wouldn't really need to assert this result. If something did go wrong (or there were an issue with our query) so that a Task wasn't returned, the following line would crash and fail the test. Only the asserts below that are relatd to the to_dict result are needed.

But what we should really do is have the one_task_belongs_to_one_goal actually return the task it associated in the fixture code (or even a tuple of the related goal and task!). Then that task would be available through the one_task_belongs_to_one_goal parameter, without us needing to look up the task ourselves. This is why we can use client in other tests. the client fixture returns the nedded value, which becomes the value of the parameter within the test itself.

anselrognlie · 2025-11-20T01:17:49Z

+    task = db.session.scalar(db.select(Task).where(Task.id == 1))
+    assert task is not None


Similar to the comment above, if one_task returned the task it created, we wouldn't need to look up the task here. But even if we do look it up, we don't need to assert it.

MuminaA and others added 16 commits October 30, 2025 16:44

Set up task-list-api, added initial migration'

7b4e672

wave 1 implemented and all tests passing

0119e1f

Merge pull request #1 from MuminaA/Wave-1

d673698

Wave 1

wave 2 complete, all tests passing

dc1c7fc

Merge pull request #2 from MuminaA/Wave-2

2d212f8

wave 2 complete, all tests passing

incomplete and complete tasks created and all test passing

c0f8efb

Merge pull request #3 from MuminaA/Wave-3

a6218fe

incomplete and complete tasks created and all test passing

slck api connected and working

4c299f2

Merge pull request #4 from MuminaA/Wave-4

9f50686

slack api connected and working

Wave 5 complete, added goal model, more refactoring, updated migrations

2845075

completed tasks for wave 5

dd637c6

Merge pull request #5 from MuminaA/Wave-5

cc89207

Wave 5

Wave 6 complete, connected goals to tasks, tests passing

0d76578

Merge pull request #6 from MuminaA/Wave-6

761c681

Wave 6 complete, connected goals to tasks, tests passing

completed wave 7 refactoring, completed all test

48d3ed2

Merge pull request #7 from MuminaA/Wave-7

b6c7cab

completed wave 7 refactoring, completed all test

anselrognlie reviewed Nov 19, 2025

View reviewed changes

correcting issues

60ea7cf

anselrognlie reviewed Nov 20, 2025

View reviewed changes

		assert isinstance(response_body, dict)
		assert "message" in response_body

		query = db.select(Goal).where(Goal.id == 1)
		assert db.session.scalar(query) == None

		task = db.session.scalar(db.select(Task).where(Task.id == 1))
		assert task is not None

Conversation

MuminaA commented Nov 7, 2025

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants