Olga_Karaivanska_task_list_api #41
Conversation
There was a problem hiding this comment.
Nice work over all, Ogla! The comments I left are mainly about refactors. There are a few 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.
app/__init__.py
Outdated
| from .routes.task_routes import tasks_bp | ||
| from .routes.goal_routes import goal_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
app/models/goal.py
Outdated
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(nullable=False) |
There was a problem hiding this comment.
If we want a column to be nullable then we would want to use Optional like you did like how you did in the Task model. Without Optional we don't need nullable=False.
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(DateTime, nullable=True ) | ||
| 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.
Nice work on getting these columns made!
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at is not None |
| return {"details": "Invalid data"}, 400 | ||
|
|
||
| goal_data = {"title": request_body.get("title")} |
There was a problem hiding this comment.
This logic could go inside of our create_model function, we would just have modify the logic here and in model methods to check if check if request was a bad one.
| "description": request_body.get("description"), | ||
| "completed_at": request_body.get("completed_at")} |
There was a problem hiding this comment.
We don't necessarily need to do this part, we could pass in the request body since it has the keys and the accompanying values we need for our model construction.
app/routes/task_routes.py
Outdated
|
|
||
| query = db.select(Task) | ||
| if sort_order == "asc": | ||
| query = query.order_by(Task.title.asc()) | ||
| elif sort_order == "desc": | ||
| query = query.order_by(Task.title.desc()) |
There was a problem hiding this comment.
Could this logic maybe be add to our get_model_with_filters function?
|
|
||
| if task.goal_id is not None: | ||
| response["task"]["goal_id"] = task.goal_id |
There was a problem hiding this comment.
I would put this as None that way there isn't confusion where people might think you are adding a goal_id even though there isn't one.
| url = "https://slack.com/api/chat.postMessage" | ||
| headers = {"Authorization": f"Bearer {slack_token}"} | ||
| request_body = { | ||
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the task '{task.title}'" | ||
| } | ||
| requests.post(url, json=request_body, headers=headers) | ||
|
|
||
| return{"task": task.to_dict()}, 200 |
There was a problem hiding this comment.
I would move this logic into a helper function and then that helper function into our utilities file to help with separation of concerns and readability.
| response = client.put("/goals/1", json={ | ||
| "title": "Build a habit of going outside daily" | ||
| }) | ||
| response_body = response.get_json() | ||
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Build a habit of going outside daily", | ||
|
|
||
| } | ||
| } |
No description provided.