Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work pulling together many concepts in this project Esther! I've left comments and a couple questions, please take a look and reach out if you have questions or need clarifications on any of the feedback 😊
| As a client, I want to be able to make a `PUT` request to `/tasks/1` when there is at least one saved task with this request body: | ||
|
|
||
| ```json | ||
| ```jso |
There was a problem hiding this comment.
As we build our skills, something to fold into our process should be reviewing our code when we open PRs to make sure we haven't made unintended changes to other files.
| from .task_routes import bp | ||
| app.register_blueprint(bp) | ||
|
|
||
| from .goal_routes import bp | ||
| app.register_blueprint(bp) |
There was a problem hiding this comment.
Great choice to divide the routes into their own files!
| return make_response(jsonify({"id": goal.goal_id, | ||
| "task_ids": tasks_list})) |
There was a problem hiding this comment.
Nice line wrapping! Though it takes more lines, another way we could format this to help the structure be really clear at a quick glance might be:
return make_response(
jsonify({
"id": goal.goal_id,
"task_ids": tasks_list
})
)| @bp.route("/<goal_id>", methods=("GET",)) | ||
| def read_goal(goal_id): | ||
| goal = validate_goal_id(goal_id) | ||
| return make_response(jsonify({"goal": goal.to_dict()})) |
There was a problem hiding this comment.
Notice the pattern of calling make_response(jsonify(...) used by many of the functions in goal_routes.py and task_routes.py. We could make another helper function in routes_helper.py that takes an optional response code and use it across all these functions, similar to the create_message helper function that handles abort(...) calls.
def get_success_response(response_data, response_code = 200):
return make_response(jsonify(response_data), response_code)| def read_specific_goal_tasks(goal_id): | ||
| goal = validate_goal_id(goal_id) | ||
|
|
||
| goal_tasks = [Task.to_dict(task) for task in goal.tasks] |
There was a problem hiding this comment.
Great use of list comprehensions across the code!
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert response_body == {"details": "Task not found"} |
There was a problem hiding this comment.
To ensure we're checking all the relevant data, it might be good to confirm here that the database has no records or that there is no record with an ID of 1.
| # ***************************************************************** | ||
|
|
||
| assert Task.query.all() == [] | ||
| assert response_body == {"details": "Task not found" } |
There was a problem hiding this comment.
The feedback on line 140 applies here and to the following tests in other files around missing tasks.
| assert response.status_code == 404 | ||
| # assertion 2 goes here | ||
| assert response_body == {"details": "Goal not found"} | ||
| assert Goal.query.all() == [] |
tests/test_wave_05.py
Outdated
| # assertion 1 goes here | ||
| assert response.status_code == 200 | ||
| # assertion 2 goes here | ||
| assert "goal" in response_body |
There was a problem hiding this comment.
Since we check the whole structure of the response body on the line below, we could omit this specific check for goal in the response body.
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert Goal.query.get(1) == None |
There was a problem hiding this comment.
Nice check to confirm the DB contents!
2 participants
No description provided.