-
Notifications
You must be signed in to change notification settings - Fork 71
YuliyaP_Rock_Task_List_Api #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job Yuliya! You hit all the learning goals for this project. I saw a lot of great helper methods/functions throughout the project and I found some additional areas where you could use them in this project. I made some comments about refactoring and other ways you could write code that I'd love to discuss more during a 1:1!
Overall, great job! This project was tough! Keep up the hard work 💪 ✨
def to_json(self): | ||
return{ | ||
"id": self.goal_id, | ||
"title": self.title, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice helper method!
def is_complete(self): | ||
if self.completed_at is None: | ||
is_complete = False | ||
else: | ||
is_complete = True | ||
return is_complete | ||
|
||
def to_json(self): | ||
if self.goal_id: | ||
return { | ||
"id": self.task_id, | ||
"goal_id": self.goal_id, | ||
"title": self.title, | ||
"description": self.description, | ||
"is_complete": self.is_complete() | ||
} | ||
else: | ||
return { | ||
"id": self.task_id, | ||
"title": self.title, | ||
"description": self.description, | ||
"is_complete": self.is_complete() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great helper method/functions here to return a dictionary. There's a way to refactor this part of the code that I'd love to show you during our 1:1.
tasks_response.append({ | ||
"id": task.task_id, | ||
"title": task.title, | ||
"description": task.description, | ||
"is_complete": task.is_complete() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would have been a good spot to use the helper method in your Task model
"task": { | ||
"id": new_task.task_id, | ||
"title": new_task.title, | ||
"description": new_task.description, | ||
"is_complete": new_task.is_complete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another good spot to use the helper method as well
headers = {"Authorization": os.environ.get("SLACK_KEY")} | ||
data = { | ||
"channel": "C021BV8A5V0", | ||
"text": f"Someone just completed the task {task.title}" | ||
} | ||
|
||
requests.patch('https://slack.com/api/chat.postMessage', | ||
headers=headers, data=data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job setting up the slack bot! A future refactor can include putting this code into a helper function.
task = Task.query.get(task_id) | ||
if task is None: | ||
return make_response("", 404) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flask has a method called .get_or_404()
that can refine these 3 lines into one!
task = Task.query.get(task_id) | |
if task is None: | |
return make_response("", 404) | |
task = Task.query.get_or_404(task_id) | |
|
||
for task_id in request_body["task_ids"]: | ||
task = Task.query.get(task_id) | ||
task.goal_id = goal.goal_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job finding all the tasks for a specific goal id!
goal = Goal.query.get(goal_id) | ||
if goal is None: | ||
return make_response("", 404) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about get_or_404()
above
return { | ||
"task": { | ||
"id": task.task_id, | ||
"title": task.title, | ||
"description": task.description, | ||
"is_complete": task.is_complete() | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can refactor this section to use your code block like so
return { | |
"task": { | |
"id": task.task_id, | |
"title": task.title, | |
"description": task.description, | |
"is_complete": task.is_complete() | |
}} | |
return { | |
"task": task.to_json() | |
} |
No description provided.