-
Notifications
You must be signed in to change notification settings - Fork 146
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
Lions - Task List - Nina-Tuyen Tran #132
base: master
Are you sure you want to change the base?
Conversation
… creating an .env file, and testing that flask runs properly.
… model to return is_complete to false when is_complete is empty/None.
…ing alphabetical order of title
…ete or incomplete on tasks
…tion to complete test_wave_04
… environmental variable to prevent open access
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 work Nina, you hit the learning goals here. I left some minor comments in the code. Let me know if you have questions via Slack.
if "title" not in request_body: | ||
return jsonify({"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 that you're doing input validation here, but it would be good to let the user know what data was invalid.
chosen_goal_dict = { | ||
"id":chosen_goal.goal_id, | ||
"title":chosen_goal.title, | ||
"tasks": list_of_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.
Turning a goal into a dict would be a great helper method in the model like the to_dict function you already have.
@@ -0,0 +1,16 @@ | |||
from flask import jsonify, abort, make_response | |||
|
|||
def get_one_obj_or_abort(cls, obj_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.
Great helper function
goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"), nullable=True) | ||
goal = db.relationship("Goal", back_populates = "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.
Another helpful method might be a class method which takes a dictionary as an argument and creates an instance of the model.
|
||
#---------------------------------------GET------------------------------------------------ | ||
|
||
def get_one_task_or_abort(task_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.
Great helper function
task_dict = { | ||
"id": task.task_id, | ||
"title": task.title, | ||
"description": task.description, | ||
"is_complete": False | ||
} | ||
response.append(task_dict) | ||
else: | ||
task_dict = { | ||
"id": task.task_id, | ||
"title": task.title, | ||
"description": task.description, | ||
"completed at": task.completed_at, | ||
"is_complete": 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.
This could be compressed a bit you could just make a local variable set to be equal to task.completed_at == None
|
||
# if chosen_task.completed_at is None: | ||
# task = { | ||
# "id": chosen_task.task_id, | ||
# "title": chosen_task.title, | ||
# "description": chosen_task.description, | ||
# "is_complete": False | ||
# } | ||
|
||
# else: | ||
# task = { | ||
# "id": chosen_task.task_id, | ||
# "title": chosen_task.title, | ||
# "description": chosen_task.description, | ||
# "completed at": chosen_task.completed_at, | ||
# "is_complete": 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.
# if chosen_task.completed_at is None: | |
# task = { | |
# "id": chosen_task.task_id, | |
# "title": chosen_task.title, | |
# "description": chosen_task.description, | |
# "is_complete": False | |
# } | |
# else: | |
# task = { | |
# "id": chosen_task.task_id, | |
# "title": chosen_task.title, | |
# "description": chosen_task.description, | |
# "completed at": chosen_task.completed_at, | |
# "is_complete": True} |
if "title" not in request_body or \ | ||
"description" not in request_body: | ||
return jsonify({"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.
Good that you're doing data validation here, but I suggest telling the end user what is invalid about the data.
header = os.environ.get("api_slack") | ||
url = "http://slack.com/api/chat.postMessage" | ||
response_str = f"Someone just completed the task {chosen_task.title}" | ||
data = {"channel":"task-notifications", "text": response_str} | ||
r = requests.post(url, params=data, headers={"Authorization":header}) |
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.
Interacting with Slack would make a good candidate for a helper function.
completed_at = db.Column(db.DateTime, nullable=True) | ||
is_complete = db.Column(db.Boolean, nullable=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.
I don't think you need two fields for this, one would do and it makes things more complicated to keep them in sync. That said it doesn't hurt much.
No description provided.