Phinx C22 Task-list Somy Cho #44
Conversation
There was a problem hiding this comment.
Nice work, Somy! Though I would like for you to read through the comments to see the ones I left about refactoring. There are a lot of places where you could have DRY'd up your code. Refactoring is a big part of this project because it helps train your eye for seeing similarities in your codebase as well as practicing maintainability and scalability. Also it seems like you didn't complete the slack implementation. I would like you to go back and complete that part of the project.
| from .routes.task_routes import tasks_bp | ||
| from .routes.goal_routes import goals_bp |
There was a problem hiding this comment.
Don't forget that the convention for naming blueprints is to name them bp. With each blueprint being named the same thing we will need to import them under an alias like so:
from .routes.task_routes import bp as tasks_bp| goal_as_dict = { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "tasks": [task.to_dict() for task in self.tasks] # Include tasks in the goal's dict |
There was a problem hiding this comment.
We have an endpoint that will provide us with the same data that goal_as_dict["tasks"] will give us so we don't need to put it on the dictionary representation of the object. If I client has the information to retrieve a Goal (i.e. goal.id) then they have the information to get the same information from our endpoint.
| def add_tasks(self, task_ids: List[int]): | ||
| tasks = Task.query.filter(Task.id.in_(task_ids)).all() | ||
| for task in tasks: | ||
| task.goal_id = self.id # Assign each task's goal_id |
There was a problem hiding this comment.
Nice work here, Somy! This is good work! Since we are searching the database we don't have to worry about validating any of the tasks!
|
|
||
| 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]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
If we want a column to be nullable then we would want to use Optional. Without Optional we don't need nullable=False.
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": bool(self.completed_at) |
|
|
||
| if sort_param and sort_param.lower() == "asc": | ||
| query = db.select(Task).order_by(Task.title) | ||
|
|
||
| elif sort_param and sort_param.lower() == "desc": | ||
| query = db.select(Task).order_by(Task.title.desc()) | ||
| else: | ||
| query = db.select(Task).order_by(Task.title) | ||
|
|
||
| tasks = db.session.scalars(query) |
There was a problem hiding this comment.
Remember in class we wrote a function called get_model_with_filters though the implementation we had doesn't work in this project, the logic could be modified used here so we could DRY up our code when getting multiple records of a table. IT would also expand our functionality beyond what's required our the README.
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": bool(task.completed_at) |
| "task": { | ||
| "id": task.id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": bool(task.completed_at) # Convert completed_at to boolean | ||
| } | ||
| }, 200 |
There was a problem hiding this comment.
Is there a reason we don't use our to_dict method here?
| def mark_complete(task_id): | ||
| task = validate_task(task_id) | ||
|
|
||
| # Update completed_at to mark the task as completed | ||
| task.completed_at = datetime.now(timezone.utc) | ||
|
|
||
| # Commit the changes to the database | ||
| db.session.commit() | ||
|
|
||
| # Prepare the response | ||
| response = { | ||
| "task": { | ||
| "id": task.id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": bool(task.completed_at) | ||
| } | ||
| } | ||
| return response, 200 |
There was a problem hiding this comment.
Looks like you didn't do the slack implementation for this route.
| response = client.put("/goals/1", json={ | ||
| "title": "Updated Goal Title" | ||
| }) | ||
|
|
||
| # Assert | ||
| # ---- Complete Assertions Here ---- | ||
| # assertion 1 goes here | ||
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| # ---- Complete Assertions Here ---- | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updated Goal Title", | ||
| } | ||
| } |
There was a problem hiding this comment.
Nice work on your test, very robust!
No description provided.