Conversation
audreyandoy
left a comment
audreyandoy
left a comment
There was a problem hiding this comment.
Ngozi! You hit all the learning goals and passed all the tests. You did a great job practicing the single responsibility principle for your goal routes and organizing your code neatly.
I added comments to your PR for ways to refactor, use helper methods, and list comprehension.
Great work! You definitely earned 🟢 on this project :)
| title = db.Column(db.String) | ||
| tasks = db.relationship('Task', backref='goal', lazy=True) No newline at end of file |
There was a problem hiding this comment.
Do we want to store goals with a NULL title? Consider setting nullable=False to ensure every goal requires a title.
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable=True) |
There was a problem hiding this comment.
Our tests don't confirm this, but should tasks be able to store NULL titles and descriptions?
All columns are nullable by default, so we can remove the nullable = True attribute from the completed_at column and it will work the same way.
| from flask import Blueprint | ||
| import re | ||
| from flask import Blueprint, jsonify, request | ||
| from flask.helpers import make_response |
There was a problem hiding this comment.
You're not using make_response anywhere else in this file so we can omit this import.
| "id": new_task.task_id, | ||
| "title": new_task.title, | ||
| "description": new_task.description, | ||
| "is_complete": new_task.completed_at is not None |
There was a problem hiding this comment.
Nice! Great use the not operator to determine the value of is_complete.
| request_body = request.get_json() | ||
|
|
||
| if "title" not in request_body or "description" not in request_body or "completed_at" not in request_body: | ||
| return jsonify({"details": "Invalid data"}), 400 |
There was a problem hiding this comment.
When a Flask app returns a dictionary, it automatically converts that dictionary into JSON. So there's no need to use jsonify() for dictionaries. We should use jsonify() for all other datatypes though!
| "is_complete": False if task.completed_at == None else True | ||
| }) | ||
|
|
||
| return jsonify({"id": goal.goal_id, "title": goal.title, "tasks": tasks_and_goals}), 200 No newline at end of file |
There was a problem hiding this comment.
Great work practicing the single responsibility principle for the goal routes!
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == None | ||
|
|
| # Act | ||
| response = client.put("/goals/1", json={ | ||
| "title": "Updated Goal Title" | ||
| }) | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updated Goal Title" | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" | ||
|
|
| # Act | ||
| response = client.put("/goals/1", json={ | ||
| "title": "Updated Goal Title" | ||
| }) | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == None |
| # Act | ||
| response = client.delete("/goals/1") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == None | ||
| assert Goal.query.all() == [] |
There was a problem hiding this comment.
This last assert statement doesn't scale well. We can't assume that deleting a goal would always produce an empty goal table. Rather, we can do a query to retrieve a goal at id 1 and confirm that None is returned.
| # Act | |
| response = client.delete("/goals/1") | |
| response_body = response.get_json() | |
| # Assert | |
| assert response.status_code == 404 | |
| assert response_body == None | |
| assert Goal.query.all() == [] | |
| assert Goal.query.get(1) == None |
2 participants
No description provided.