Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Nice work overall! There are some items I would like to see you revisit, particularly around establishing the one-to-many relationship and tests passing, I'll leave more details on that in Learn.
app/__init__.py
Outdated
| from flask_sqlalchemy import SQLAlchemy | ||
| from flask_migrate import Migrate |
There was a problem hiding this comment.
We don't use these modules directly in __init__.py so should remove these imports so the file doesn't load up dependencies that it will not use.
app/models/goal.py
Outdated
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column() | ||
|
|
||
| #tasks: Mapped[list["Task"]] = relationship("Task", backref="goal", lazy="dynamic") |
There was a problem hiding this comment.
In hello-books we created a relationship attribute between an Author and their list of Books with syntax like:
books: Mapped[list["Book"]] = relationship(back_populates="author")How could we use the same structure of syntax here to create a relationship attribute between a Goal and a list of Tasks?
app/models/goal.py
Outdated
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column() |
There was a problem hiding this comment.
Since we aren't doing any customization inside the mapped_column call, we could leave it off:
title: Mapped[str]
app/models/task.py
Outdated
| title: Mapped[str] = mapped_column() | ||
| description: Mapped[str] = mapped_column() | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
Similar to the feedback in Goal, we could remove the mapped_column calls here:
title: Mapped[str]
description: Mapped[str]
completed_at: Mapped[Optional[datetime]]
app/models/task.py
Outdated
| description: Mapped[str] = mapped_column() | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) | ||
|
|
||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"), nullable=True) |
There was a problem hiding this comment.
Since goal_id is Optional, we can leave off the nullable argument in mapped_column:
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey('goal.id'))
app/routes/task_routes.py
Outdated
| def patch_complete(task_id): | ||
| task = validate_task(task_id) | ||
| task.completed_at = datetime.now().isoformat() | ||
| db.session.commit() | ||
|
|
||
| url = "https://slack.com/api/chat.postMessage" | ||
| headers = { | ||
| "Authorization": f"Bearer {os.environ.get('SLACK_API_KEY')}", | ||
| "Content-Type": "application/json" | ||
| } | ||
| data = { | ||
| "channel" : "#api-test-channel", | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
|
|
||
| requests.post(url, headers=headers, json=data) | ||
|
|
||
| response = {"task": task.to_dict()} | ||
|
|
||
| return response, 200 |
There was a problem hiding this comment.
I see this function doesn't have a route annotation, what pieces were you still working on?
app/routes/goal_routes.py
Outdated
| goals_response =[] | ||
| for goal in goals: | ||
| goals_response.append(goal.to_dict()) |
There was a problem hiding this comment.
This would be another great place for a list comprehension:
goals_response = [goal.to_dict() for goal in goals]
app/routes/goal_routes.py
Outdated
| query = db.select(Goal) | ||
| goals = db.session.scalars(query) | ||
|
|
||
| goals_response =[] |
There was a problem hiding this comment.
| goals_response =[] | |
| goals_response = [] |
| "goal":goal.to_dict() | ||
| }, 200 | ||
|
|
||
| def validate_goal(goal_id): |
There was a problem hiding this comment.
This is very similar to our validate_task function, how could we refactor these functions to reduce repetition?
There was a problem hiding this comment.
I know I can create validate_models in bass.py then impor it in goal_routes and task_routes!
tests/test_wave_05.py
Outdated
| "title": "Updated Goal Title", | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) |
There was a problem hiding this comment.
I understand you may have seen this syntax in the tests and used the same. These tests need to be updated, the <model>.query syntax has been deprecated since SQLAlchemy 2.0 and should not be used in any new development.
There is a similar structure we can use through the db.session object to look up a single record by its id that we could use here instead (though we would need to import db into the test file).
db.session.get(Goal, 1)2 participants
No description provided.