-
Notifications
You must be signed in to change notification settings - Fork 146
C18 Snow Leopards Jessica Hebert #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on task-list-api! Your structure, added tests, and functionality look good!
I noticed you have some methods you wrote in task.py and goal.py that you didn't end up using which lead to a fair amount of repetition. Have a look at the comments and see how you can DRY up your code.
Also, when I ran your tests 11/24 failed. Please review your tests to see why they failed. In industry, pull requests will not be reviewed until all tests are passing so it'll be good to get some additional practice with running tests and making them pass.
Let me know if you have any questions.
|
||
class Goal(db.Model): | ||
goal_id = db.Column(db.Integer, primary_key=True) | ||
title = db.Column(db.String) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A goal without a title doesn't make much sense so we might want to make it a required field. We can do so by adding nullable=False to the title field
description = db.Column(db.String) | ||
completed_at = db.Column(db.DateTime, nullable=True) | ||
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True) | ||
goals = db.relationship("Goal", back_populates="tasks") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relationship between goal and task is one-to-many. One goal can have many tasks, but each task only has one goal. We also represent this relationship by having a foreign key "goal_id" which can only be a single goal id on the Task model. Therefore, we should rename the attribute goals
to the singular goal
|
||
class Goal(db.Model): | ||
goal_id = db.Column(db.Integer, primary_key=True) | ||
title = db.Column(db.String) | ||
tasks = db.relationship("Task", back_populates="goals") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relationship between goal and task is one-to-many. I wrote a comment in Task model about renaming the field goals
to goal
to represent that a Task can only have one Goal. Therefore, you'd need to make an update here to:
tasks = db.relationship("Task", back_populates="goal")
title = db.Column(db.String) | ||
tasks = db.relationship("Task", back_populates="goals") | ||
|
||
def to_json(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During the live code for Flasky, I did initially name this method to to_json, but I ended up refactoring it to to_dict to better reflect the fact that we're taking a model and turning it into a dictionary. Just wanted to call that out.
except: | ||
return abort(make_response({"details": "Invalid data"}, 400)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooooooh this is nice, I like that you've got some error handling here!
When we use the request body, which comes from the client making a request, we should operate under the assumption that the client may send us bad data so we need to have some error handling.
However, I think you misnamed this method. Since it's part of the Goal class, it should be called create_goal and the variable name should be new_goal
# ***************************************************************** | ||
# **Complete test with assertion about response body*************** | ||
# ***************************************************************** | ||
assert response_body == {"message" : "Task 1 is not found"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails when I run it, but not due to your implementation of the GET method for /tasks/<task_id>
Here's the output:
E {'message': 'Task 1 not found'} != {'message': 'Task 1 is not found'}
How would you update your assertion so the test passes?
# ***************************************************************** | ||
# **Complete test with assertion about response body*************** | ||
# ***************************************************************** | ||
|
||
assert response_body == {"message" : "Task 1 is not found"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same message as above
# ***************************************************************** | ||
# **Complete test with assertion about response body*************** | ||
# ***************************************************************** | ||
assert response_body == {"message" : "Task 1 is not found"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the ones I left in test_wave_01.py -- double check your assertion message here. The test fails but only because response_body doesn't match the message by one word.
In industry, when you submit a PR for review, you'll need to ensure that all the tests pass. Usually teams don't review code that has failing tests
# ***************************************************************** | ||
# **Complete test with assertion about response body*************** | ||
# ***************************************************************** | ||
assert response_body == {"message" : "Task 1 is not found"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this test too
} | ||
} | ||
goal = Goal.query.get(1) | ||
assert goal.id == 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test fails with the message:
goal = Goal.query.get(1)
> assert goal.id == 1
E AttributeError: 'Goal' object has no attribute 'id'
What do you think is going on here? Have a look at what you named the attribute that refers to the goal's id in your Goal class: https://github.com/Ada-C18/task-list-api/pull/138/files#diff-b2c259b85990b30a9cbf1e8b4dcba2ce11bc2cfb7907527e60f78e1456f201f3R5
No description provided.