Possum - Wenxin Li by Likhoram · Pull Request #22 · Ada-C24/task-list-api

Likhoram · 2025-11-07T04:08:52Z

No description provided.

anselrognlie

Your implementation looks good overall! Please review my comments, and let me know if you have any questions.

anselrognlie · 2025-11-18T00:13:42Z

+    response = client.get("/goals/1")
+    response_body = response.get_json()
+    assert response_body["title"] == "Updated Goal Title"


Our update test for Task used a direct database lookup to confirm that the expected update had actually been applied to the database. Doing so crosses a level of abstraction (to a degree). There's a school of thought that we should only use the tools at a similar level of abstraction to the test in order to perform validations (i.e., a test checking HTTP route behavior oughtn't go poking around in the DB). If we take that approach, then issuing a follow up GET request gives us the same ability to ensure that the actual data was updated. If it hadn't been, then subsequent independent requests would not see the updated value to be returned. The fact that the subsequent response does have the updated title implies that the data update in the PUT persisted beyond that single request.

anselrognlie · 2025-11-18T00:14:41Z

    # Test that the correct status code and response message are returned
    response = e.value.get_response()
    assert response.status_code == 400
+    assert response.get_json() == {"details": "Invalid id"}


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

anselrognlie · 2025-11-18T00:15:17Z

@@ -0,0 +1,75 @@
+"""recreate initial


👍 Nice use of messages to describe the migrations. We can see that the message is written into the migration file itself, as well as becoming a part of the file name. Since the migration files are otherwise just named with a hash, this can help us understand what each migration does.

👍 It looks like you regenerated your migrations into a single clean migration during your development. This is something I actually like to do intentionally. During the initial phases of working on a project, it's more likely that we'll need to make major changes to the schema as we learn more about the problem we're solving. Doing this frequently can lead to a messy collection of updates. I like to clear and recreate a single clean migration that captures all of the initial setup. Migrations really become more useful once an application has been deployed and contains production-critical data that we don't want to lose as we continue to make DB changes over time. But there's no need to start our project with superfluous migrations.

anselrognlie · 2025-11-18T00:45:32Z

+    bind = op.get_bind()
+    insp = sa.inspect(bind)
+
+    if not insp.has_table("goal"):


👀 I'm curious what the reasoning is for this custom code in the migration. I haven't seen Alembic generate code like this on its own.

Migration code can be altered and tuned for certain cases, but I don't see a particular reason for the changes that are here. Is this something that was suggested by an AI tool?

The way that migrations are applied, general tables are in a well-known configuration after each migration, and a single migration is run only once (Alembic records the latest migration and won't rerun earlier ones). Since this is the only migration (it has no down_revision) This would be expected to be run against an empty database, so it should create the goal table (there's no need to check whether it exists).

Likewise, the task table shouldn't exist at the time this is run, so it should just create the fresh task table. Since the task table is created with with completed_at being a datetime, I'm not sure whether the else would run successfully even if it could be run more than once (Alembic should prevent that), as the instructions appear to migrate the completed_at column from being a string to being a datetime. But it wouldn have already been a datetime.

Incorporating AI suggestions into code isn't a problem, but I'm concerned that the instructions that I see here don't make sense in the context of setting up the task list database. Yes, it will work to create a fresh set of tables, but there's additional logic that seems unnecessary or possibly incorrect.

Please either add additional comments to this code to explain your understanding of what the important parts do and what makes them important. Or please regenerate a fresh migration that sticks closer to the expected migration logic we worked with in class.

anselrognlie · 2025-11-18T00:47:42Z

👍 Nice use of a seed file. This uses some of the patterns we showed in class, but takes a different approach to how to associate related records.

anselrognlie · 2025-11-18T06:27:40Z

+    if task.goal_id is not None:
+        task_dict["goal_id"] = task.goal_id


👀 The intent of having the goal_id field in the task output was so that any task that belongs to a goal would include that goal id in the output. This should occur even for the get all tasks route. So rather than making the route responsible for deciding whether to include goal information for a task, we can move that logic to be within the task to_dict itself!

That being said, in an actual application, I would probably favor including the goal_id key even when the value was empty just to keep a consistent output shape. But in this project, that would have required going back and updating a bunch of previous tests, or adding extra forward-looking logic to the tests that would have made them harder to work with in the earlier waves.

anselrognlie · 2025-11-18T06:31:25Z

+        t["goal_id"] = goal.id
+        tasks.append(t)
+
+    return {"id": goal.id, "title": goal.title, "tasks": tasks}


Notice that the id and title keys here are the same as for a regular Goal GET, for which we wrote the to_dict helper. Here, we could use our to_dict to get the regular result dictionary, and then add in the tasks key with the loaded task data.

Or we could make a different helper that itself uses the Goal to_dict for its basic structure, but adds in the Task content itself. This would tidy up the response generation for this route.

anselrognlie · 2025-11-18T06:32:21Z

+    for task in goal.tasks:
+        t = task.to_dict()
+        # Tests expect tasks returned for a goal to include the goal_id
+        t["goal_id"] = goal.id


👀 As mentioned in the get one task route, if the goal_id logic was conditionally included in the Task to_dict, then we wouldn't need to add it in specially within the routes themselves.

anselrognlie · 2025-11-18T06:35:50Z

+def post_task_ids_to_goal(id):
+    goal = validate_model(Goal, id)
+    data = request.get_json() or {}
+    return assign_related_by_ids(goal, "tasks", Task, data.get("task_ids"))


While moving the business logic of assigning relationships out of this route to a helper is a great idea, I'd probably make it a less general purpose route, and instead focus on this particualr case. This generalization feels a bit premature to me.

anselrognlie · 2025-11-18T06:42:02Z

+    db.session.commit()
+    return make_response("", 204)
+
+def assign_related_by_ids(parent, relation_name, child_cls, ids, response_key="task_ids"):


👍 This method shows a really strong understanding of passing class objects around and of using the setattr/getattr meta methods. However, this does make things a bit hard to read since nearly everything is a generic operation. So while this is an interesting approach, I'd probably keep my method a little bit more concrete until we had a better idea of whether there were any other situations where a method like this could be useful.

anselrognlie

👍 Thanks for the updates. Looks good.

anselrognlie · 2025-12-02T16:57:33Z

+        if self.goal_id is not None:
+            result["goal_id"] = self.goal_id


👍 This will conditionally include the goal id field in the task dictionary if the task belongs to a goal. This maintain the expected output for the early tests and meets the later outputs as well. Any route that uses the to_dict will gain this behavior automatically.

Note that we don't automatically include the tasks with a goal's dictionary since a goal might have many tasks, so we have the caller opt into getting that information through the tasks for goal route. But since a task belongs to only a single goal, this is a low-cost data item to include.

anselrognlie · 2025-12-02T16:59:44Z

    if sort == "asc":
-        query = query.order_by(cls.title.asc())
+        # title, then id
+        query = query.order_by(cls.title.asc(), cls.id)


Rather than adding id as an aditional sort column for each call, we can take on an additional order_by that sorts by the id after this conditional logic. That is, we conditionally can add the clause to sort by the title, but then always add a clause thsorts by the id. This would acheive the same result, with more compartmentalized logic.

anselrognlie · 2025-12-02T17:29:17Z

+    return empty_response()
+
+def empty_response():
+    return make_response(json.dumps({}), 204, {"Content-Type": "application/json"})


This will return an empty json response, but why not use the approach covered in Learn, and that you had on your mark complete endpoint?

return Response(status=204, mimetype="application/json")

This avoid needing to add a "placeholder" response body, which Flask will ignore anyway.

anselrognlie · 2025-12-02T17:40:38Z

-                existing_nullable=True,
-                postgresql_using="completed_at::timestamp without time zone",
-            )
+    op.create_table(


👍 These are more typical of what I'd expect to see for an "all in one" migration.

Likhoram added 12 commits October 30, 2025 16:02

setup

e6e9146

models and routes

2219f26

wave 1

7836853

wave 2

e3cf4a4

wave 3

ed89e6c

wave 4

36ab1af

wave 4 modified

6023976

wave 5 and 6 before migration

6712e2d

pass all tests

6fc8cac

test in postman

1522550

add seed.py

6000804

final

b2c4b1a

anselrognlie reviewed Nov 19, 2025

View reviewed changes

Likhoram added 4 commits November 30, 2025 23:34

goal id added

26c846f

created empty response

0dfee47

task route remove goal id

298073d

changed migration

d8f1908

anselrognlie reviewed Dec 2, 2025

View reviewed changes

		if task.goal_id is not None:
		task_dict["goal_id"] = task.goal_id

		if self.goal_id is not None:
		result["goal_id"] = self.goal_id

Conversation

Likhoram 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!

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