Phoenix (C22)- Jenny M #24
Conversation
There was a problem hiding this comment.
GREEN
Well done, Jenny! Overall this looks good and works well and passes all the tests. The biggest thing that I would focus on is modifying your task's to_dict function to include a conditional that adds the goal id if it exists but doesn't include it if it doesn't. There are so many places where you could have used it, but didn't because it doesn't quite work correctly.
All in all, if you have time, make sure you are going back over your code to see where and how you can refactor effectively!
| from .routes.task_routes import tasks_bp | ||
| import os | ||
| from dotenv import load_dotenv | ||
| from .routes.goal_routes import goals_bp |
There was a problem hiding this comment.
Feel free to get in the habit of naming all our blueprints bp within their routes and aliasing them within the init.py
| load_dotenv() | ||
| SLACKBOT_TOKEN = os.environ.get("SLACKBOT_TOKEN") |
There was a problem hiding this comment.
Because we aren't using the Slackbot token in this file, we don't need to bring it in here!
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
|
|
There was a problem hiding this comment.
Just make sure to remove unnecessary empty lines like this in the PR!
|
|
||
| class Task(db.Model): | ||
|
|
||
| __tablename__ = 'tasks' |
There was a problem hiding this comment.
It doesn't seem as though you are using tablename anywhere else so we don't need to include this!
| def to_dict(self): | ||
| return dict( | ||
| id=self.id, | ||
| goal_id=self.goal_id, | ||
| title=self.title, | ||
| description=self.description, | ||
| is_complete=check_complete(self.completed_at) |
There was a problem hiding this comment.
While it feels very complete to return the dictionary in its entirety here, the goal_id is an optional attribute. When you have an optional attribute, you should only add it to the returned dictionary if it exists! This is a simple conditional that we can build in here!
Also, I see where you are coming from with the check_complete helper function. My one concern is that this is coming from the routes utilities in the routes directory. While it is not forbidden to use helper methods from the tasks in your models, it can get confusing. You could use a ternary instead to ascertain the truthiness of is_complete.
| { | ||
| "id": task.id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": check_complete(task.completed_at) | ||
|
|
||
| } |
There was a problem hiding this comment.
This is another place where the modified to_dict could definitely come in handy!
| return { | ||
| "task": task.to_dict() | ||
|
|
||
| } | ||
| else: | ||
| return { | ||
| "task": { | ||
| "id": task.id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": check_complete(task.completed_at) | ||
| } | ||
| } |
There was a problem hiding this comment.
A similar conditional in the task's to_dict would allow much more flexibility in its use!
| # def task_mark_complete(task_id): | ||
| # task = validate_task(task_id) | ||
|
|
||
| # task.completed_at = datetime.now() | ||
|
|
||
| # db.session.commit() | ||
|
|
||
| # return { | ||
| # "task": { | ||
| # "id": task.id, | ||
| # "title": task.title, | ||
| # "description": task.description, | ||
| # "is_complete": check_complete(task.completed_at) | ||
| # } | ||
| # } |
There was a problem hiding this comment.
Make sure to remove unused code from the PR!
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the task {task.title}" | ||
|
|
||
| } | ||
|
|
||
| # Send the Slack message | ||
| headers = {"Authorization": f"Bearer {SLACKBOT_TOKEN}"} | ||
| slack_response = requests.post( | ||
| "https://slack.com/api/chat.postMessage", | ||
| json=slack_message, | ||
| headers=headers | ||
| ) |
There was a problem hiding this comment.
This looks good! In order to elevate your code further, I would say this is a great candidate for a helper function!
| return jsonify({"message": "Failed to send Slack notification"}), 500 |
There was a problem hiding this comment.
You're the only person I've seen who makes this check and I think it's a great idea!
No description provided.