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

Amethyst - Elaine W. #116

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Amethyst - Elaine W. #116

wants to merge 7 commits into from

Conversation

ewatki
Copy link

@ewatki ewatki commented May 13, 2023

No description provided.

@@ -1,5 +1,15 @@
from app import db


class Goal(db.Model):

Choose a reason for hiding this comment

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

Great job completing Task List Elaine! Your code looks clean and easily readable. I want to point out the good variable naming and your code is DRY. Great use of helper methods in your models! Excellent work using blue prints & creating RESTful CRUD routes for each model.

Comment on lines +7 to +10
completed_at = db.Column(db.DateTime, nullable=True)
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'))
goal = db.relationship('Goal', back_populates='tasks')

Choose a reason for hiding this comment

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

Nice approach creating this relationship to your Goal model. Why not use lazy here?

Comment on lines +11 to +29
@classmethod
def from_dict(cls, task_data):
new_task = Task(title=task_data["title"],
description=task_data["description"],
completed_at=None)
return new_task

def to_dict(self):
task_as_dict = {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at)
}

if self.goal_id:
task_as_dict["goal_id"] = self.goal_id

return task_as_dict

Choose a reason for hiding this comment

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

Great job creating this reusable helper function

Comment on lines +36 to +39
from .routes import task_bp
app.register_blueprint(task_bp)
from .routes import goal_bp
app.register_blueprint(goal_bp)

Choose a reason for hiding this comment

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

Great work registering these blueprints ✅

from flask import Blueprint, jsonify, request, abort, make_response

Choose a reason for hiding this comment

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

👍🏾 These imports help a great deal when it comes to our request & response cycle

Comment on lines +10 to +11
goal_bp = Blueprint("goals", __name__, url_prefix="/goals")

Choose a reason for hiding this comment

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

👍🏾

Comment on lines +13 to +26
try:
model_id = int(model_id)
except:
abort(make_response({"message":f"{model_id} invalid type ({type(model_id)})"}, 400))
# abort(make_response({"message":f"Invalid data"}, 400))

model = cls.query.get(model_id)

if not model:
abort(make_response({"message":f"{cls.__name__.lower()} {model_id} not found"}, 404))

return model

Choose a reason for hiding this comment

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

😁


# database collects new changes - adding new_task as a record
db.session.add(new_task)

Choose a reason for hiding this comment

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

Great job using db.session.<method-name>. Session gives us access to the follow:

  • db is our app’s instance of SQLAlchemy.
  • session represents our active database connection.
  • By referencing db.session we can use SQLAlchemy’s methods to perform tasks like:
    • committing a change to a model
    • storing a new record of a model
    • deleting a record.
  • By referencing db.session.add() you are able to use the SQLAlchemy method to store a new record of the task model

"title": new_task.title,
"description": new_task.description,
"is_complete": bool(new_task.completed_at)

Choose a reason for hiding this comment

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

😁

Comment on lines +61 to +67

if sort_query == "asc":
tasks = Task.query.order_by(Task.title).all()
elif sort_query == "desc":
tasks = Task.query.order_by(desc(Task.title)).all()
else:

Choose a reason for hiding this comment

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

⭐️ Great work handling this query param logic ⭐️

Comment on lines +62 to 63
assert response_body == {"message": "task 1 not found"}

Choose a reason for hiding this comment

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

Nice work with this test completion! This assertion is a great way to validate functionality is working as expected

"title": new_goal.title
}
}, 201

Choose a reason for hiding this comment

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

Usually a resource creation relates to a 201 response code 👍🏾

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