Otters - Jodi Denney #104
Otters - Jodi Denney #104
Conversation
There was a problem hiding this comment.
Fantastic job, Jodi! 🎉
Great job making models, tests, and clear CRUD endpoints in your task routes and goal routes files. This project is a strong implementation of major Flask concepts from Unit 2 and earns a Green. 🟢
I've left a few comments around consistency and where we could DRY up code in the future. Overall, keep up the great work! 🚀
| def to_json(self): | ||
| return {"id": self.goal_id, | ||
| "title": self.title} |
| 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 "message" in response_body | ||
| assert response_body == {"message": f"Task 1 not found"} |
There was a problem hiding this comment.
👍🏻
Since you're not interpolating a string in L63, you don't need the f in front of `"Task 1 not found"
| assert response.status_code == 404 | ||
| assert "message" in response_body | ||
| assert response_body == {"message": f"Goal 1 not found"} |
| @@ -51,13 +51,8 @@ def test_get_tasks_for_specific_goal_no_goal(client): | |||
| # Assert | |||
| assert response.status_code == 404 | |||
|
|
|||
There was a problem hiding this comment.
Forgot an assertion about the response body here -- remember to do a scan over for future projects!
|
|
||
| tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||
|
|
||
| SLACK_TOKEN = os.environ.get("SLACK_TOKEN") |
There was a problem hiding this comment.
Great job using a constant to call the token from your env file! ✨
| @@ -1 +0,0 @@ | |||
| from flask import Blueprint | |||
There was a problem hiding this comment.
I like that you split up our routes in two separate files! As our project and codebase grows, splitting up big files (and routes.py is often a notoriously big file) in a reasonable way is a good call.
| def validate_goal_id(goal_id): | ||
| try: | ||
| id = int(goal_id) | ||
| except: | ||
| abort(make_response( | ||
| {"message": f"goal {goal_id} invalid. Must be numerical"}, 400)) | ||
|
|
||
| goal = Goal.query.get(goal_id) | ||
|
|
||
| if not goal: | ||
| abort(make_response({"message": f"Goal {goal_id} not found"}, 404)) | ||
|
|
||
| return goal |
There was a problem hiding this comment.
Nice helper method here! This code is also very similar to validate_task_id in task_routes file -- could we modify this code to be able to work for both Models? 🤔
| goals = Goal.query.filter_by(title=title_query) | ||
| else: | ||
| goals = Goal.query.all() | ||
| print(goals) |
There was a problem hiding this comment.
Remember to remove your print statements when you submit 😉
| @goals_bp.route("/<goal_id>", methods=["GET"]) | ||
| def get_single_goal(goal_id): | ||
| goal = validate_goal_id(goal_id) | ||
| return{"goal": goal.to_json()} |
There was a problem hiding this comment.
Beautiful use of the to_json method! I see that in some endpoints, you use jsonify but it's not consistently used. I recommend using it here as well! Flask will handle this dictionary correctly, but using a consistent pattern helps someone who's reading the code understand that the same thing is happening here.
Additionally, remember to give it an explicit status code!
| # tasks = [] | ||
| # for id in task_ids: | ||
| # tasks.append(validate_task_id(id)) | ||
| # for task in tasks: | ||
| # task.goal_id = goal.goal_id | ||
|
|
There was a problem hiding this comment.
Also remember to remove commented out code!
Otters - Jodi Denney