Conversation
There was a problem hiding this comment.
Before adding files to your commit history, review what files you have staged by running git status. Changes like the ones here that are not meaningful should not be a part of your pull request because that's essentially asking to merge in unnecessary changes to the codebase which can make the codebase messy.
| from ..db import db | ||
| from datetime import datetime | ||
|
|
||
| goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") |
There was a problem hiding this comment.
In Learn and in the livecode for Flasky, we name blueprints for each model bp. In goal_routes.py I can deduce that bp is the blueprint for Goal given the name of the file.
If we have multiple route files that each have a blueprint named bp, we can account for name collisions when importing them into app/__init__.py by using aliases:
from .routes.task_routes import bp as tasks_bp
from .routes.goal_routes import bp as goals_bpThere was a problem hiding this comment.
Unless these changes were required, I'd revert unnecessary changes so they don't get committed and reviewed when you open a PR.
|
|
||
| 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 odd to allow someone to create a goal that gets saved to the DB without a title.
| def to_dict(self): | ||
| return dict(id=self.id, title=self.title) | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| return cls(title=goal_data["title"]) |
| @goals_bp.get("/<goal_id>/tasks") | ||
| def get_tasks_by_goal(goal_id): | ||
| goal = validate_goal(goal_id) | ||
| goal_dict = goal.to_dict() |
There was a problem hiding this comment.
While we are creating a dictionary that represents a goal, it might be more descriptive to name this variable response since we're creating a dictionary that we want to send back as a response to the client.
| from datetime import datetime | ||
| from app.models.task import Task | ||
|
|
||
| tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") |
There was a problem hiding this comment.
Same comment as above in goal_routes.py, could be named just bp
| request_body = request.get_json() | ||
|
|
||
| try: | ||
| new_task = Task.from_dict(request_body) | ||
|
|
||
| except KeyError: | ||
| response = {"details": "Invalid data"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
|
|
||
| db.session.add(new_task) | ||
| db.session.commit() |
There was a problem hiding this comment.
Same comment as above in goal_routes.py, would prefer this logic to be in a helper function so you're not duplicating very similar code across different files.
| task = validate_task(task_id) | ||
|
|
||
|
|
||
| return {"task": task.to_dict()} |
| def mark_task_complete(task_id): | ||
| task = validate_task(task_id) | ||
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
|
|
||
| return {"task": task.to_dict()}, 200 |
There was a problem hiding this comment.
For the sake of time, I went to your repo and found your other branch to look at your Slack implementation. Note: I cannot leave comments on a codebase. That's why we have you open PRs for your projects. In the future, please make sure all of your solution is in your PR.
I'm copy and pasting your helper function here and leaving my review comments as comments in your code
def send_slack_message(task_title):
# constants should be named using all capital letters
token = os.environ.get("SLACK_BOT_TOKEN")
if not token:
raise ValueError("Slack Bot Token is not set in environment variables.")
headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"}
payload = {"channel": SLACK_CHANNEL, "text": f"Beautiful task {task_title}"}
response = requests.post(SLACK_API_URL, headers=headers, json=payload)
if response.status_code != 200:
print("Failed to send Slack message:", response.json())
else:
print("Slack message sent successfully.")
nelasunitha
changed the title
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
2 participants
Please review.