Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Your implementation looks good overall! Please review my comments, and let me know if you have any questions.
| db.session.add(new_task) | ||
| db.session.commit() | ||
|
|
||
There was a problem hiding this comment.
🔎 When adding files, be sure to check whether any unintentional changes were added.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": "Task id 1 is not found."} |
There was a problem hiding this comment.
Checking for the "not found" message is the main behavior we want to verify. If we were particularly paranoid, we might also check that no record was surreptitiously added to the database. This is less of a concern for the other "not found" tests, since they wouldn't involve any logic that could conceivably add a new record, but since PUT replaces data, it could be the case that an implementer might have added logic to create the record if it were missing. That is, they could treat a PUT to a nonexistent record as a request to create that particular record.
Again, that's not a requirement, but it is something to keep in mind.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": "Task id 1 is not found."} |
There was a problem hiding this comment.
Checking for the "not found" message is again the main behavior we want to verify. And even though this technically changes data, I'd be less likely to check that a record had been added as a result. Unlike the PUT request, which should include all necessary data to make a Task, this endpoint really doesn't have enough information, so it would have been very difficult to assume the endpoint should create the requested Task.
| response = client.put("/goals/1", json={ | ||
| "title": "Updated Goal Title",}) | ||
|
|
||
| assert response.status_code == 204 | ||
|
|
||
| query = db.select(Goal).where(Goal.id == 1) | ||
| goal = db.session.scalar(query) | ||
|
|
||
| assert goal.title == "Updated Goal Title" |
There was a problem hiding this comment.
👍 Comparing this test with the Task update test gives us an idea of what we should verify here. After performing the PUT, we need to confirm both the response (204 No Content) and check that the change has actually occurred in the database.
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": "Goal id 1 is not found."} | ||
| assert db.session.scalars(db.select(Goal)).all() == [] |
There was a problem hiding this comment.
🔎 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.
|
|
||
| goals_bp = Blueprint("goals_bp", __name__, url_prefix = "/goals") | ||
|
|
||
| @goals_bp.get("") |
There was a problem hiding this comment.
👍 Your Goal CRUD routes are consistent with your Task CRUD routes. Check the Task feedback for anything that could apply here.
| if self.goal: | ||
| task_dict["goal_id"] = self.goal_id |
There was a problem hiding this comment.
👍 Nice logic to add the goal data to the task dictionary only when it's present.
| goal.tasks.clear() | ||
|
|
||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| task.goal_id = goal.id |
There was a problem hiding this comment.
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 = taskswhich avoids the need to manually clear the goal association from the existing tasks.
| @goals_bp.get("/<goal_id>/tasks") | ||
| def get_tasks_by_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| response = {"id": goal.id, "title": goal.title, "tasks": [task.to_dict() for task in goal.tasks]} |
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.
2 participants
No description provided.