Conversation
…ed Slack post message API
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 querying, I'll leave more details on that in Learn.
| title: Mapped[str] = mapped_column(nullable=False) | ||
| tasks: Mapped[List["Task"]] = relationship("Task", back_populates="goal") | ||
|
|
||
| def goal_dict(self): |
There was a problem hiding this comment.
Since this function is a method of the Goal class, including goal in the name doesn't give the reader new information. Names like create_dict, as_dict, or to_dict would convey a little more clearly to the user the intent of the function.
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(nullable=False) |
There was a problem hiding this comment.
In the current version of SQLAlchemy nullable=False is the default, so we could leave off the mapped_column statement without changing our tables:
title: Mapped[str]| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(nullable=False) | ||
| tasks: Mapped[List["Task"]] = relationship("Task", back_populates="goal") |
There was a problem hiding this comment.
This works, but we can also use the built-in Python list type here. I typically recommend using the Python built in unless there is a need to use a more specific library version of a data structure.
Since we define that the attribute is a list of Tasks on the left side of the assignment, we do not need to pass "Task" as the first parameter in the relationship. This feedback applies to the relationship attribute in the Task class as well.
tasks: Mapped[list["Task"]] = relationship(back_populates="goal")| title: Mapped[str] = mapped_column(nullable=False) | ||
| description: Mapped[str] = mapped_column(nullable=False) | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True, default=None) |
There was a problem hiding this comment.
Since we aren't changing the default options for the declarations on the left side of the assignment, we can leave off the mapped_column calls for these 3 attributes.
# Nullable is False by default unless an attribute is marked as Optional
# These 2 attributes are not nullable
title: Mapped[str]
description: Mapped[str]
# Since it is declared as Optional, nullable = True by default
# If an attribute is nullable, it will be defaulted to `None` if it is not set when a record is
# created unless a different default value is explicitly provided in the definition here
completed_at: Mapped[Optional[datetime]]| title: Mapped[str] = mapped_column(nullable=False) | ||
| description: Mapped[str] = mapped_column(nullable=False) | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True, default=None) | ||
| 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'))| "Content-Type": "application/json" | ||
| } | ||
|
|
||
| notification = { |
There was a problem hiding this comment.
The notification variable contains more that just the notification message, I suggest changing the variable name to reflect that.
| @@ -0,0 +1,28 @@ | |||
| import os | |||
There was a problem hiding this comment.
Really nice organization moving this out to its own file.
|
|
||
| goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
|
||
| 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?
| # @pytest.mark.skip(reason="test to be completed by student") | ||
| def test_delete_goal_not_found(client): | ||
| raise Exception("Complete test") | ||
|
|
||
| # Act | ||
| # ---- Complete Act Here ---- | ||
| response = client.delete("/goals/999") # Assumes no goal with id=999 exists | ||
| 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 999 not found"} |
There was a problem hiding this comment.
Updates look great across the tests!
| query = db.select(Task) | ||
| sort_order = request.args.get("sort", "asc") | ||
| if sort_order == "desc": | ||
| query = query.order_by(Task.title.desc()) | ||
| else: | ||
| query = query.order_by(Task.title.asc()) | ||
|
|
||
| # Execute the query | ||
| tasks = db.session.scalars(query) |
There was a problem hiding this comment.
This is the syntax we should be using for querying any time we are trying to query for a result that could be more than a single record and may be applying filters and sorting to the results. When we know we only want a single record and have the primary key, the shorthand syntax used in mark_task_complete is valid:
db.session.get(Task, task_id)2 participants
fyi: I tried to refactor my code while also doing wave 6 and majority of tests were failing so i went back to git and re-used the code i pushed up from up until wave 5 when all tests were passing and haven't refactored the updated code yet.