-
Notifications
You must be signed in to change notification settings - Fork 97
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
C16-Pine Alma #79
base: master
Are you sure you want to change the base?
C16-Pine Alma #79
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.
Awesome job Alma! This was a huge project and it's all looking great! Very solid on both the models and routes. This project is definitely green! 🎉
db = SQLAlchemy() | ||
migrate = Migrate() | ||
DATABASE_CONNECTION_STRING='postgresql+psycopg2://postgres:postgres@localhost:5432/task_list_api_development' |
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.
Because you now get this string from environment variables you no longer need this line.
def to_dict(self): | ||
return{ | ||
"id": self.goal_id, | ||
"title": self.title | ||
} |
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 and clean!
def to_dict_with_tasks(self, goal_id): | ||
tasks = Task.query.filter_by(fk_goal_id=f"{goal_id}") | ||
task_list = [] | ||
for task in tasks: | ||
task_list.append(task.to_dict()) | ||
return{ | ||
"id":self.goal_id, | ||
"title":self.title, | ||
} |
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.
It looks like this isn't ever used and can be removed.
"id":self.task_id, | ||
"title": self.title, | ||
"description": self.description, | ||
"is_complete": True if self.completed_at else False, |
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 use of ternary operatory!
if self.goal_id: | ||
return { | ||
"id":self.task_id, | ||
"title": self.title, | ||
"description": self.description, | ||
"is_complete": True if self.completed_at else False, | ||
"goal_id": self.goal_id | ||
} | ||
|
||
else: | ||
return{ | ||
"id":self.task_id, | ||
"title": self.title, | ||
"description": self.description, | ||
"is_complete": True if self.completed_at else False | ||
} |
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 definitely works! Can you think of a way to restructure it so we set up most of the dictionary outside of the if/else and only add the "goal_id"
key when needed? This could help reduce repetition in your code.
if not goal: | ||
return make_response("", 404) |
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.
We don't need this part. Goal.query.all()
should only give us valid goals back.
|
||
@goals_bp.route("/<goal_id>", methods=["GET", "PUT", "DELETE"]) | ||
def handle_goal(goal_id): | ||
goal_id=int(goal_id) |
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.
Make sure to do input validation here - if the user passed in an invalid goal_id
this code would crash.
answer = { | ||
"id": goal.goal_id, | ||
"title": goal.title, | ||
"tasks": [task.to_dict() for task in goal.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.
Gorgeous!
goal.tasks.append(task) | ||
|
||
db.session.commit() | ||
return jsonify({"id":goal.goal_id, "task_ids": [task.task_id for task in goal.tasks]}), 200 |
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.
Very nice!
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) |
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 job asserting what should be in the response body! It would also be a good idea to assert things about the goal we get back from querying the DB. Perhaps something like assert goal.title == "Updated Goal Title"
got to Wave 5