Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
| # ***************************************************************** | ||
|
|
||
| # raise Exception("Complete test with assertion about response body") | ||
| assert response_body["message"] == "Goal 1 is not found" |
There was a problem hiding this comment.
Checking this specific key works fine, but I would probably check against a literal dictionary, which would enforce the overall response structure in addition to validating the expected message.
| # ** Complete test with an assertion about the response body **************** | ||
| # ***************************************************************************** | ||
| # raise Exception("Complete test with an assertion about the response body") | ||
| assert response.get_json() == {"message": "Task One is invalid"} |
There was a problem hiding this comment.
👍 Nice job making use of the response we're able to extract from the error value.
There was a problem hiding this comment.
It looks like your migration generation went pretty smoothly. We had a fairly short roadmap of things to do. In a more extensive app, we might not have known the models and attributes so clearly. We might have needed to make additional changes, or experiment with a few approaches, which could lead to a more complicated set of migrations. In that case, 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.
| @@ -0,0 +1,34 @@ | |||
| """added relationship | |||
There was a problem hiding this comment.
👍 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.
| @@ -0,0 +1,39 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
Remember to use the -m option when creating a migration to give it a descriptive message.
| return create_model(Goal, request_body) | ||
|
|
||
| @bp.post("/<goal_id>/tasks") | ||
| def create_task_with_goal(goal_id): |
There was a problem hiding this comment.
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.
| task_list = [] | ||
| for id in task_ids: | ||
| task = validate_model(Task, id) | ||
| task_list.append(task) |
There was a problem hiding this comment.
We could validate all of the tasks in a list comprehension:
task_list = [validate_model(Task, id) for id in task_ids]| task = validate_model(Task, id) | ||
| task_list.append(task) | ||
|
|
||
| goal.tasks = task_list |
There was a problem hiding this comment.
👍 Setting the task collection to a new list of Task models takes care of unsetting any previous tasks associated with the goal, and updates the new Tasks, all in one little assignment.
| return { | ||
| "id": goal.id, | ||
| "task_ids": task_ids | ||
| }, 200 |
There was a problem hiding this comment.
Rather than hard-coding this dict result here, we could create an additional instance method on Goal to generate this output, or a standalone helper. That could help tidy up this function.
| tasks = [task.to_dict() for task in goal.tasks] | ||
|
|
||
| return { | ||
| "id": goal.id, | ||
| "title": goal.title, | ||
| "tasks": tasks | ||
| }, 200 |
There was a problem hiding this comment.
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.
2 participants
Notes
I used
wave-01until wave-06. After that, I merged tomain. So main has everything for this project.Thank you