PR - Bukunmi G SL #135
PR - Bukunmi G SL #135
Conversation
There was a problem hiding this comment.
Nice job!
Your tests are all passing, and your code looks good. Try to keep any eye out for duplicate code, and places where we could otherwise move code into helpers. Ideally, the routes should have as little code as possible, and mostly just get the request data, pass it in to a helper, and send the result back to the caller. We really only scratched the surface with the refactorings in class, but we can keep looking for more!
| @@ -3,3 +3,20 @@ | |||
|
|
|||
There was a problem hiding this comment.
Remember to make a __init__.py file in any package folder/subfolder. app and routes each have one, but we should have one here in the models folder as well.
| def test_get_task_not_found(client): | ||
| # Act | ||
| response = client.get("/tasks/1") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {'details': 'Task 1 not found'} |
There was a problem hiding this comment.
👍 Nice test for the status message.
| # assertion 1 goes here | ||
| # assertion 2 goes here | ||
| # ---- Complete Test ---- | ||
| assert response.status_code == 404 |
| "title": "Updated Task Title", | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) |
There was a problem hiding this comment.
👍 We can check the database directly, like this. Alternatively, we could issue a get request for the goal using requests and check that the title has been updated that way. But one way or another, it's a good idea to confirm, as you did, that the data is actually updated, and not just in the response body.
| @@ -123,28 +131,23 @@ def test_delete_goal(client, one_goal): | |||
| # Check that the goal was deleted | |||
| response = client.get("/goals/1") | |||
| assert response.status_code == 404 | |||
| assert Goal.query.get(1) == None | |||
There was a problem hiding this comment.
The provided test logic used a follow-on get request, which tells us the equivalent of looking up the id in the database. The test comment asked us to confirm the response_body contents, though I find that a little redundant, since we already have a test that confirms the response body when we ask for a missing goal.
| def update_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| request_body = request.get_json() | ||
| goal.title = request_body["title"] |
There was a problem hiding this comment.
Be sure to handle the potential KeyError here as well.
| goals_response = [goal.to_dict() for goal in goals] | ||
| return jsonify(goals_response), 200 | ||
|
|
||
| @goals_bp.route("<goal_id>", methods=["GET"]) |
There was a problem hiding this comment.
The route endpoint here should be "/<goal_id>" (note the leading /). I tried digging a bit to see whether safe to leave off the leading / (can we depend on this behavior?) but I'm not seeing anything. So I'd recommend sticking with the leading /.
| goal = validate_model(Goal, goal_id) | ||
| request_body = request.get_json() | ||
|
|
||
| goal.tasks += Task.query.filter(Task.task_id.in_(request_body["task_ids"])).all() |
There was a problem hiding this comment.
This query is roughly the equivalent of something like
select * from task where task_id in (1, 2, ...);One effect of this is that it would this won't report an error if we supply a task id that isn't present in the database, since it would simply not be part of the list of tasks returned from the query. This might be OK, but we also might want our endpoint to raise an error if we try to add a task to the goal that doesn't exist in the database.
If we want it to report invalid task ids, we could iterate over the supplied ids and look them up with validate_model.
|
|
||
| return make_response (jsonify({ | ||
| "id": goal.goal_id, | ||
| "task_ids": request_body["task_ids"] |
There was a problem hiding this comment.
Your logic for associating the tasks with the goal appends the list of ids. So there could be tasks previously associated with the goal that we'd like to include in the output. The test cases don't have enough detail to determine which behavior is desired, so this works fine, but we should be internally consitent. Since you're appending the tasks, I'd expect to see something like this to return the task ids associated with the goal.
"task_ids": [task.task_id for task in goal.tasks]| @@ -0,0 +1,78 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
A lot of this code is very similar to routes in Task (as we'd expect), so for those parts, please refer to the task routes file to see which feedback there also applies here.
No description provided.