Sharks Kassidy Buslach task-list-api#112
Conversation
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Great work on Task List, Kassidy! You earned a 🟢 on this project. Really nice to see you making frequent commits too.
From your code, I can see that you've got a handle on Flask and SqlAlchemy.
I added a few comments about using list comprehension, where you could re-use helper functions and instance methods you already wrote in your routes to make them more concise, and using nullable=False.
Let me know if you have any questions!
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| goal_id = db.Column(db.Integer, primary_key=True, autoincrement = True) | ||
| title = db.Column(db.String) |
There was a problem hiding this comment.
Consider adding nullable=False to ensure every goal requires a title.
| if self.tasks: | ||
| json_response["tasks"]= self.title |
There was a problem hiding this comment.
It looks like you do a check here to see if tasks is a non empty value. Then you assign the value for json_response["tasks"] to self.title.
Should line 15 be something like this instead?
json_response["tasks"]= self.tasks|
|
||
|
|
||
|
|
||
| # goal_id = goal_id, No newline at end of file |
There was a problem hiding this comment.
Remember to remove commented out code to keep your files clean and to prevent a situation where code is accidentally uncommented and executes when it shouldn't.
|
|
||
| return goal | ||
|
|
||
| def validate_data(request_body): |
There was a problem hiding this comment.
Nice helper function here to help handle errors if a request doesn't have all the necessary data. We can't assume that clients will always send us correct requests so this a great way to handle the error and send back some feedback to the client.
Consider renaming the method to be a little more descriptive like validate_task_request()
| class Task(db.Model): | ||
| task_id = db.Column(db.Integer, primary_key=True) | ||
| task_id = db.Column(db.Integer, primary_key=True, autoincrement = True) | ||
| title = db.Column(db.String) |
There was a problem hiding this comment.
Consider adding nullable=False to ensure every task requires a title.
| return result, 200 | ||
|
|
||
| @goal_db.route("/<goal_id>/tasks", methods = ["POST"]) | ||
| def create_one_goal(goal_id): |
There was a problem hiding this comment.
In this POST request, we are sending an ID of a goal that already exists and also list of task IDs. Consider renaming this method to something like assign_tasks_to_goal() to be more descriptive.
| request_body = request.get_json() | ||
|
|
||
|
|
||
| for task_id in request_body["task_ids"]: |
There was a problem hiding this comment.
You could add error handling here to make sure that the request_body has key "task_ids".
|
|
||
| for task_id in request_body["task_ids"]: | ||
| validate_id(task_id) | ||
| task = Task.query.get(task_id) |
There was a problem hiding this comment.
Your validate_id() helper function returns a task object. See lines 10 and 14 in your helper.py file.
Since that's the case, you can combine line 170 and 171 into:
task = validate_id(task_id)| valid_goal.tasks.append(task) | ||
| db.session.commit() | ||
|
|
||
| return {"id":valid_goal.goal_id,"task_ids":request_body["task_ids"]}, 200 No newline at end of file |
There was a problem hiding this comment.
Same comment as above about wrapping this dictionary in the jsonify() method to make it clear that this method will return a JSON response
| assert response.status_code == 200 | ||
| assert response_body=={"Goal": | ||
| {'id': 1,"title": "Updated goal Title"}} | ||
| assert "Goal" in response_body |
2 participants
No description provided.