Skip to content

Conversation

@ellenjin
Copy link

No description provided.

Copy link

@mikellewade mikellewade left a comment

Choose a reason for hiding this comment

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

Solhee, great job on this project! There is a comment I tagged you in that I would like you to take a moment really consider, the relationship one.

Comment on lines +7 to +8
from .routes.task_routes import bp as task_list_bp
from .routes.goal_routes import bp as goals_bp

Choose a reason for hiding this comment

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

Nice! You are following Flask convention to by naming your Blueprints bp and then using as to import them under an alias.

@@ -1,6 +1,11 @@
from dotenv import load_dotenv # loads .env variables into os.environ
load_dotenv()

Choose a reason for hiding this comment

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

I would move this function call below all of your other imports, there is no need for it to run before them.

Comment on lines +26 to +27
app.register_blueprint(task_list_bp)
app.register_blueprint(goals_bp)

Choose a reason for hiding this comment

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

⭐️


class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]

Choose a reason for hiding this comment

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

Great work using the declarative mapping here! Since we are doing any specific declarations like in id we can simply use Mapped in conjunction with a Python datatype to declare what this column will look like.

class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]
tasks: Mapped[list["Task"]] = relationship(back_populates="goal")

Choose a reason for hiding this comment

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

Perfect! You are making a relationship attribute on the Goal model. This attribute is going to be a list of Task models. You then use relationship with back_populates to tell SQLAlchemy to sync this attribute with relationship attribute called goal on the Task model.


# Assert
assert response.status_code == 404
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.

⭐️

Comment on lines +90 to +95
assert response.status_code == 204

query = db.select(Goal).where(Goal.id == 1)
goal = db.session.scalar(query)

assert goal.title == "Updated Goal Title"

Choose a reason for hiding this comment

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

Great job, checking the database to ensure the changes persisted!


assert goal.title == "Updated Goal Title"

# DO I NEED TO ADD ANOTHER ASSERTION -- it said 3

Choose a reason for hiding this comment

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

You could also check to ensure that nothing changed about the .tasks attribute.

Choose a reason for hiding this comment

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

But I think the two you have there are fine. @ellenjin

Comment on lines +103 to +110
response = client.put("/goals/1", json={
"title": "Updated Goal Title"
})
response_body = response.get_json()

# Assert
# ---- Complete Assertions Here ----
# assertion 1 goes here
# assertion 2 goes here
# ---- Complete Assertions Here ----
assert response.status_code == 404
assert response_body == {"message": "Goal 1 not found"}

Choose a reason for hiding this comment

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

Comment on lines +138 to +140
assert response.status_code == 404
assert response_body == {"message": "Goal 1 not found"}
assert db.session.scalars(db.select(Goal)).all() == []

Choose a reason for hiding this comment

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

🫡

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