Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Your implementation looks good overall! Please review my comments, and let me know if you have any questions. Be sure to revisit the approaches we show in the curriculum, as they can differ from other courses or previous experience. The curriculum way isn't necessarily the "best", but be sure that you have a good understanding of the needs our approaches were meant to address so that you can be sure that you're addressing similar needs with your own approaches.
Procfile
Outdated
There was a problem hiding this comment.
To my understanding, Procfiles are used in Heroku, but not in Render. Did you also try deploying to Heroku?
| from app.routes.task_routes import bp as task_bp | ||
| from app.routes.goal_routes import bp as goal_bp |
There was a problem hiding this comment.
Nesting imports under code rather than having it at the top of a file can occasionally be necessary to avoid import cycles. However, due to how we've set up our imports (specifically how we access db), these imports can safely be listed at the top of the file.
| from .models.base import Base | ||
|
|
||
| db = SQLAlchemy(model_class=Base) |
There was a problem hiding this comment.
👀 This approach is from the recommended project setup from Flask documentation. We shouldn't remove these without a clear reason.
app/models/__init__.py
Outdated
| from .task import Task | ||
| from .goal import Goal No newline at end of file |
There was a problem hiding this comment.
It's sufficient for this file to be empty. Code amy be added to be run when a package is first imported, or to control the visibility of exported modules, but we don't need that in our project.
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
|
|
||
| assert response_body == {"message": "No task with ID 1 found"} |
There was a problem hiding this comment.
Checking for the "not found" message is the main behavior we want to verify. If we were particularly paranoid, we might also check that no record was surreptitiously added to the database. This is less of a concern for the other "not found" tests, since they wouldn't involve any logic that could conceivably add a new record, but since PUT replaces data, it could be the case that an implementer might have added logic to create the record if it were missing. That is, they could treat a PUT to a nonexistent record as a request to create that particular record.
Again, that's not a requirement, but it is something to keep in mind.
app/routes/goal_routes.py
Outdated
| # GET all goals | ||
| @bp.get("") | ||
| def get_goals(): | ||
| goals = Goal.query.all() |
There was a problem hiding this comment.
👀 The query property of models has been deprecated and should no longer be used. Prefer db.session.scalars(db.select(Goal)).
|
|
||
| # CREATE a goal | ||
| @bp.post("") | ||
| def create_goal(): |
There was a problem hiding this comment.
👍 Your Goal CRUD routes are consistent with your Task CRUD routes. Check the Task feedback for anything that could apply here.
| if self.goal_id: | ||
| task_dict["goal_id"] = self.goal_id |
There was a problem hiding this comment.
👍 Nice logic to add the goal data to the task dictionary only when it's present.
app/routes/goal_routes.py
Outdated
| for task_id in task_ids: | ||
| task = db.session.get(Task, task_id) | ||
| if task: | ||
| task.goal_id = goal.id |
There was a problem hiding this comment.
👀 From the original way the test_post_task_ids_to_goal_already_with_goals test was written, the expected behavior when posting the list of task ids to the goal is for the tasks associated with the goal to be replaced. The logic here will append.
Please update the logic here to replace the associated tasks (the .tasks relationship can significantly simplify this!), and restore the test to its original behavior.
| goal_dict = goal.to_dict() | ||
| goal_dict["tasks"] = [task.to_dict() for task in goal.tasks] |
There was a problem hiding this comment.
👍 Nice reuse of your to_dict methods to build up your response. Rather than leaving this logic here in the route, we could move it to an additional Goal method. Perhaps to_dict_with_tasks, or we could add a parameter to the main Goal to_dict that determines whether or not to include the Task details.
2 participants
No description provided.