Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

abecerrilsalas
Copy link

got to Wave 5

Copy link

@alope107 alope107 left a 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'

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.

Comment on lines +13 to +17
def to_dict(self):
return{
"id": self.goal_id,
"title": self.title
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and clean!

Comment on lines +19 to +27
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,
}

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,

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!

Comment on lines +14 to +29
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
}

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.

Comment on lines +135 to +136
if not goal:
return make_response("", 404)

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)

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.

Comment on lines +171 to +175
answer = {
"id": goal.goal_id,
"title": goal.title,
"tasks": [task.to_dict() for task in goal.tasks]
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

Comment on lines +83 to +91
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)

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"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants