Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on task-list-api! Let me know if you have questions about my review comments.
| @@ -2,4 +2,19 @@ | |||
|
|
|||
|
|
|||
| class Goal(db.Model): | |||
There was a problem hiding this comment.
👍 LGTM
Remember to make an empty init.py file in any package folder/subfolder. app and routes both have one, but we should have one here in the models folder as well.
| completed_at = db.Column(db.DateTime, nullable=True) | ||
| goal_id = db.Column(db.Integer, db.ForeignKey("goal.id"), nullable=True) |
There was a problem hiding this comment.
The default value for nullable is True so we don't need to explicitly add it
| task_as_dict["id"] = self.id | ||
| task_as_dict["title"] = self.title | ||
| task_as_dict["description"] = self.description | ||
| task_as_dict["is_complete"] = False |
There was a problem hiding this comment.
Instead of hardcoding is_complete to False, how could you use the value of self.completed_at to derive the boolean value?
We could use a ternary operator like:
task_as_dict["is_complete"] = True if self.completed_at else FalseThere was a problem hiding this comment.
👍 yep, every directory needs an __init__.py file
| slack_url = "https://slack.com/api/chat.postMessage" | ||
| channel_id = "task-notifications" |
There was a problem hiding this comment.
nice job putting this logic into a helper function!
Since slack_url and channel_id are constant variables, they should be spelled with all caps.
|
|
||
| for task_id in task_ids: | ||
| task = Task.query.get(task_id) | ||
| task.goal_id = goal.id |
|
|
||
| db.session.commit() | ||
|
|
||
| response_message = {"id": int(goal_id), "task_ids": task_ids} |
There was a problem hiding this comment.
The value referenced by task_ids comes from the request body that the client sent to your API. Instead of returning the values that the client sent in the response message, I'd prefer to get the task ids from the goal object that you validated and then associated tasks with instead.
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| response_message = goal.to_dict() | ||
| tasks = Task.query.filter_by(goal=goal) |
There was a problem hiding this comment.
We don't need to use Task.query.filter_by actually. Since a goal has associated tasks, we can access the goal's tasks attribute (see goal.py line 7).
So this can be refactored to tasks = goal.tasks
| tasks = Task.query.filter_by(goal=goal) | ||
| tasks_response = [] | ||
|
|
||
| if tasks: |
There was a problem hiding this comment.
No need for line 103. if tasks is an empty list, then the for loop won't execute.
| task_dict = task.to_dict() | ||
| task_dict["goal_id"] = int(goal_id) | ||
| tasks_response.append(task_dict) |
There was a problem hiding this comment.
If you move the logic on line 106 into the to_dict() method then this for loop could be refactored to use list comprehension.
2 participants
No description provided.