Lions - Jessica Ambriz-Madrigal#127
Lions - Jessica Ambriz-Madrigal#127Jessicaenvisions wants to merge 18 commits intoAda-C18:masterfrom
Conversation
CheezItMan
left a comment
CheezItMan
left a comment
There was a problem hiding this comment.
Nice work Jessica, you hit the learning goals here. I left some minor comments in the code. If you have questions on them please let me know via Slack.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
| if self.completed_at is None: | ||
| if self.goal_id is None: | ||
| task_dict = { | ||
| "id":self.id, | ||
| "title":self.title, | ||
| "description":self.description, | ||
| "is_complete": False | ||
| } | ||
| else: | ||
| task_dict = { | ||
| "id":self.id, | ||
| "goal_id": self.goal_id, | ||
| "title":self.title, | ||
| "description":self.description, | ||
| "is_complete": False | ||
| } | ||
| else: | ||
| task_dict = { | ||
| "id":self.id, | ||
| "title":self.title, | ||
| "description":self.description, | ||
| "is_complete": True | ||
| } | ||
| return task_dict No newline at end of file |
There was a problem hiding this comment.
This seems a little over-complicated. Could you do the basic dictionary in one block and then add key-value pairs based on an if-else?
|
|
||
| task_bp = Blueprint("task_bp",__name__, url_prefix="/tasks") | ||
|
|
||
| def get_one_task_or_abort(task_id): |
| 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.
Good validation here. However I think you should also indicate what data is invalid.
| return { | ||
| "task":{ | ||
| "id" : new_task.id, | ||
| "title" : new_task.title, | ||
| "description" : new_task.description, | ||
| "is_complete" : new_task.is_complete | ||
| } | ||
| }, 201 |
There was a problem hiding this comment.
This return value might be useful as a helper function for reuse.
| if not title_param: | ||
| tasks = Task.query.all() | ||
|
|
||
| if sort_param == "desc": | ||
| tasks = Task.query.order_by(Task.title.desc()).all() | ||
|
|
||
| if sort_param == "asc": | ||
| tasks = Task.query.order_by(Task.title.asc()).all() |
There was a problem hiding this comment.
This might cause Task.query to execute twice making this potentially twice as slow as it needs to be.
| SLACK_PATH = "https://slack.com/api/chat.postMessage" | ||
| SLACK_BOT_TOKEN = os.environ.get("SLACK_BOT_TOKEN") | ||
|
|
||
| headers = { | ||
| "Authorization" : f"Bearer {SLACK_BOT_TOKEN}" | ||
| } | ||
| params = { | ||
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the task {update_is_valid_id.title}" | ||
| } | ||
|
|
||
| requests.get(url=SLACK_PATH, headers=headers, params=params) |
There was a problem hiding this comment.
The Slack portion would make a good candidate for a helper function.
| if "title" not in request_body: | ||
| return jsonify({"details": "Invalid data"}), 400 |
There was a problem hiding this comment.
Again indicating what data is invalid would be useful.
2 participants
No description provided.