Maybellene Aung C22 task list #42
Conversation
| from flask import Blueprint, Response, request, abort, make_response |
There was a problem hiding this comment.
Response, abort, and make_response are imported but never accessed so should be removed.
| from flask import Blueprint, request, Response, abort, make_response |
There was a problem hiding this comment.
Response, is imported but never accessed so should be removed.
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] |
There was a problem hiding this comment.
Consider whether the this column for Goal should be nullable. It feels unconventional to allow someone to create a goal that gets saved to the DB without a title.
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] |
There was a problem hiding this comment.
As with Goal, consider which of the columns for Task should be nullable and which should not be.
| goal_as_dict = {} | ||
| goal_as_dict["id"] = self.id | ||
| goal_as_dict["title"] = self.title | ||
|
|
||
| return goal_as_dict |
There was a problem hiding this comment.
Can also directly return a dictionary literal
| goal_as_dict = {} | |
| goal_as_dict["id"] = self.id | |
| goal_as_dict["title"] = self.title | |
| return goal_as_dict | |
| return { | |
| "id": self.id, | |
| "title": self.title | |
| } |
| response = { | ||
| "id": goal.id, | ||
| "task_ids": task_ids |
There was a problem hiding this comment.
Notice that task_ids comes from the request body (line 23). Instead of taking the client-provided value, it would be nice to return a response that has task ids that we get from the goal that we fetched from the db on line 21. This way we know for sure that we're returning data from our source of truth (the DB) instead of just echoing back what the user sent to the API (what we get from request_body["task_ids"])
You'd have to grab tasks from goal like:
tasks_from_goal = goal.tasks
task_ids_from_goal = [task.id for task in tasks_from_goal]
return {
"id": int(goal_id),
"task_ids": task_ids_from_goal
}| request_body = request.get_json() | ||
| task_ids = request_body["task_ids"] | ||
| tasks = Task.query.filter(Task.id.in_(task_ids)).all() |
There was a problem hiding this comment.
The <Model>.query syntax has been deprecated since SQLAlchemy 2.0 and should not be used for new development.
Review the docs here
Here, I'd probably use your validate_model method since task_ids are values that are grabbed from the request body that the client sent with the request. Since we can't assume that they're valid, we can validate:
| tasks = Task.query.filter(Task.id.in_(task_ids)).all() | |
| tasks = [validate_model(Task, task_id) for task_id in task_ids] |
| try: | ||
| task_id = int(task_id) | ||
| except: | ||
| response = {"message": f"Task {task_id} is invalid"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
| query = db.select(Task).where(Task.id == task_id) | ||
| task = db.session.scalar(query) | ||
|
|
||
| if not task: | ||
| response = {"message": f"Task {task_id} was not found"} | ||
| abort(make_response(response, 404)) | ||
|
|
||
| return task |
There was a problem hiding this comment.
Looks like this duplicates the logic in validate_model in route_utilities.py. This should be removed so that you can use the more generic version of this helper function
| token = os.environ.get("SLACK_BOT_TOKEN") | ||
| channel_id = "C07V4J7ABF1" |
There was a problem hiding this comment.
Nice job using variables to reference these values so you don't have string literals embedded below. Since these are constant variables, they should be named with all capital letters.
| } | ||
|
|
||
| requests.post(path, headers=headers, json=data) |
There was a problem hiding this comment.
Prefer all the logic relating to making a call to the Slack API be encapsulated in a helper function (maybe something like call_slack_api). This would keep this route more concise and reduce the number of responsibilities the logic has to account for.
No description provided.