Madina_Dzhetegenova_Ada_C22_Phoenix #40
Conversation
| } | ||
|
|
||
| def validate_task(task_id): |
There was a problem hiding this comment.
👀 Note that we could refactor this validate_task and the validate_goal functions to be able to handle both.
| } | ||
|
|
||
| def validate_goal(goal_id): |
There was a problem hiding this comment.
👀 Note that we could refactor this validate_goal and the validate_task functions to be able to handle both.
| def test_get_task_not_found(client): | ||
| # Act | ||
| response = client.get("/tasks/1") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
|
|
||
| raise Exception("Complete test with assertion about response body") | ||
| assert response_body == {"message": f"task 1 not found"} |
There was a problem hiding this comment.
👀 This message value doesn't need to be an f-string. There's no interpolation happening there.
| @@ -130,14 +130,14 @@ def test_update_task_not_found(client): | |||
|
|
|||
| # Assert | |||
| assert response.status_code == 404 | |||
|
|
|||
| raise Exception("Complete test with assertion about response body") | |||
| assert response_body == {"message": f"task 1 not found"} | |||
There was a problem hiding this comment.
Here again and throughout, the f-string is unnecessary.
| # assertion 3 goes here | ||
| goal = Goal.query.get(1) | ||
| assert response.status_code == 200 | ||
| assert goal.title == "Updated Title" |
There was a problem hiding this comment.
Prefer to keep this line closer to line that retrieves the updated goal. It's a little confusing to have it mixed in with the response checks. But great idea to check that the attempted change is actually visible in the database (not just in the response).
| for goal in goals: | ||
| goals_response.append( | ||
| { | ||
| "id": goal.id, | ||
| "title": goal.title | ||
| } | ||
| ) |
There was a problem hiding this comment.
With a to_dict method, a list comprehension would become a readable alternative to writing out the loop here longhand.
goals_response = [goal.to_dict() for goal in goals]| request_body = request.get_json() | ||
|
|
||
| goal.title = request_body["title"] |
There was a problem hiding this comment.
Similar to the Task PUT route, this route could benefit from some error handling for any missing keys.
| response = {"message": "No task_ids provided"} | ||
| abort(make_response(response, 400)) |
There was a problem hiding this comment.
We don't include a test for passing an empty list here, so treating it as an error is fine. Consider adding additional test cases if you add additional logic not covered by existing tests so that you can try this out.
An alternative way to handle this might be to interpret sending an empty list as meaning to disassociate the goal from any current tasks, though this might work better if we make a different behavioral choice below.
| task = validate_task(task_id) | ||
| task.goal = goal |
There was a problem hiding this comment.
By setting the goal for each task to be the current goal, this route is effectively appending the supplied tasks to the goal. There might have been other tasks already associated with the goal, though such a scenario doesn't appear in any of the tests we supplied. As a result, we might consider retrieving all the tasks now associated with the goal (such as with something like all_task_ids = [task.id for task in goal.tasks]), rather than returning a list only of the ids we just added.
If we interpretted this route as replacing the tasks associated with this goal (such as by generating a list of the tasks, and then replacing the whole goal.tasks list with the updated list), then the current behavior of only returning the incoming tasks would more closely match expectations. This might match the behavior described above, where we might consider an empty list of task ids to mean there should be no tasks associated with the goal.
| "id": task.id, | ||
| "goal_id": goal.id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": task.completed_at is not None | ||
| } for task in goal.tasks] |
There was a problem hiding this comment.
Notice that without a to_dict method, the conciseness of the list comprehension is lost. Compare to the case where we do create a to_dict method.
tasks = [task.to_dict() for task in goal.tasks]We would need to be sure that the to_dict method correctly added the goal_id information only when available so that it could be used in all the places where we convert tasks to dictionary responses.
No description provided.