Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Marjana! Let me know if you have questions on any of my feedback =]
| from .models import task, goal | ||
| from .routes.task_routes import tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp | ||
| from .models import goal |
There was a problem hiding this comment.
We're importing goal on line 4 and line 6; this import should be removed to keep our file tidy.
| ) | ||
| @classmethod |
There was a problem hiding this comment.
We should leave a blank line between these functions to create visual space for readers - it makes it easier to quickly scan through a file when the functions are very clear from each other.
| goal : Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||
|
|
||
| def to_dict(self): | ||
| is_complete_task= False |
There was a problem hiding this comment.
| is_complete_task= False | |
| is_complete_task = False |
| task_dict = dict( | ||
| id=self.id, | ||
| title=self.title, | ||
| description=self.description, | ||
| is_complete=is_complete_task, | ||
|
|
||
| ) | ||
| if self.goal: | ||
| task_dict["goal_id"] = self.goal.id |
There was a problem hiding this comment.
The spacing is a little weird here, I recommend removing the blank line inside the dict declaration and adding a blank line after to separate the creation from the goal check:
task_dict = dict(
id=self.id,
title=self.title,
description=self.description,
is_complete=is_complete_task,
)
if self.goal:
task_dict["goal_id"] = self.goal.id| from sqlalchemy.orm import Mapped, mapped_column , relationship | ||
| from sqlalchemy import ForeignKey | ||
| from ..db import db | ||
| from datetime import datetime | ||
| from typing import Optional | ||
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title : Mapped[str] | ||
| description : Mapped [str] | ||
| completed_at: Mapped[Optional[datetime]] | ||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal : Mapped[Optional["Goal"]] = relationship(back_populates="tasks") |
There was a problem hiding this comment.
Really nice set up work for both your model classes! 🙌🏻
| def partial_update_complete_one_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| task.completed_at = date.today() |
There was a problem hiding this comment.
This works, but the datetime module also has a method .now() which can optionally accept a timezone parameter. In the Python documentation now is noted as preferred over today (documentation)
| } | ||
|
|
||
| @bp.post("/<goal_id>/tasks") | ||
| def create_task_with_goal_id(goal_id): |
There was a problem hiding this comment.
This function will look up existing tasks and associate them with an existing goal. Since we don't create any new tasks, how could we rename this route function to better reflect the actions it is taking?
| all_tasks= [] | ||
| for task in goal.tasks: | ||
| all_tasks.append(task.to_dict()) |
There was a problem hiding this comment.
This would be a great place for a list comprehension:
all_tasks = [task.to_dict() for task in goal.tasks]| new_task = Task(title="Go on my daily walk 🏞", | ||
| description="Notice something new every day", | ||
| completed_at=datetime.utcnow()) | ||
| completed_at=datetime.now()) |
There was a problem hiding this comment.
Nice catch that this should be updated to now since utcnow is deprecated!
| #@pytest.mark.skip(reason="No way to test this feature yet") | ||
| def test_get_tasks_for_specific_goal_no_goal(client): | ||
| # Act | ||
| response = client.get("/goals/1/tasks") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message" : "Goal 1 not found"} | ||
|
|
||
| raise Exception("Complete test with assertion about response body") | ||
| #raise Exception("Complete test with assertion about response body") |
There was a problem hiding this comment.
Updates look great across the tests!
2 participants
No description provided.