-
Notifications
You must be signed in to change notification settings - Fork 127
Whitney Shake - Zoisite - Task List #113
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: main
Are you sure you want to change the base?
Conversation
utes file still present but commented out
…correct in Postman
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 work on task-list-api! Let me know if you have questions about my review comments
@@ -1,5 +1,26 @@ | |||
from app import db |
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.
Remember to make an empty init.py file in any package folder/subfolder. app and routes have one, but we should have one here in the models folder as well.
@@ -1,5 +1,26 @@ | |||
from app import db | |||
|
|||
|
|||
|
|||
class Goal(db.Model): | |||
goal_id = db.Column(db.Integer, primary_key=True) |
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.
My personal preference is to name id attributes just id
and then when you have a goal object you can write goal.id
instead of goal.goal_id
.
if tasks: | ||
build_dict["tasks"] = [task.to_dict() for task in self.tasks] |
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.
Nicely done! You could even remove tasks=False from the parameter and have the check on line 17 be if self.tasks:
instead
if tasks: | ||
build_dict["tasks"] = [task.to_dict() for task in self.tasks] | ||
|
||
return build_dict |
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.
A more descriptive name for this dictionary might be goal_dict
or goal_response_dict
@@ -2,4 +2,41 @@ | |||
|
|||
|
|||
class Task(db.Model): | |||
task_id = db.Column(db.Integer, primary_key=True) | |||
task_id = db.Column(db.Integer, primary_key=True, autoincrement=True) |
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.
Same comment as in goal, prefer primary key just to be called id
return make_response({"details": "Invalid data"}, 400) | ||
|
||
|
||
def get_all_items(cls): |
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.
👍
try: | ||
new_item = cls.from_dict(request_body) | ||
db.session.add(new_item) | ||
db.session.commit() | ||
return make_response({cls.__name__.lower(): new_item.to_dict()}, 201) | ||
except: | ||
return make_response({"details": "Invalid data"}, 400) |
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 error handling in case the request_body has invalid data
load_dotenv() | ||
|
||
def send_slack_message(completed_task): | ||
TOKEN = os.environ['SLACK_API_TOKEN'] |
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'll work on APIs where a route will have to make more than one external call to an API so we should be more specific about the kind of token we're using. This variable could be called SLACK_API_TOKEN
try: | ||
message = f"Someone just completed the task {completed_task.title}" | ||
payload = { | ||
"channel": CHANNEL_ID, | ||
"text": message | ||
} | ||
|
||
requests.post(SLACK_URL, data = payload, headers = AUTH_HEADERS) | ||
|
||
except: | ||
print("There was an error making the call to Slack") |
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.
👍
from app import db | ||
import os | ||
|
||
load_dotenv() |
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.
We don't need to invoke load_dotenv() since it was already executed in the create_app() method on startup. When that happens all the values in the .env file are loaded up and we can just import os in this file and grab the values without needing to call load_dotenv() again
No description provided.