Conversation
| slack_url = os.environ.get("SLACK_URL") | ||
| channel_id = os.environ.get("CHANNEL_ID") |
There was a problem hiding this comment.
CHANNEL_ID and SLACK_URL do not need to be secret. Consider saving them as a constant rather than in the .env
| slack_key = os.environ.get("SLACK_KEY") | ||
| slack_url = os.environ.get("SLACK_URL") | ||
| channel_id = os.environ.get("CHANNEL_ID") | ||
| requests.post(slack_url, headers= {'Authorization': f"Bearer {slack_key}"}, \ | ||
| data= {'channel' : f"{channel_id}", 'text' : f"Someone just completed the task My Beautiful Task"}) |
There was a problem hiding this comment.
Great work implementing this functionality. Consider encapsulating in a helper function to enhance readability and flexibility (You may want to use this function when someone updates a task, for instance).
| task = Task.query.get(task_id) | ||
| if task is None: | ||
| return jsonify(None), 404 |
There was a problem hiding this comment.
Here is another place you can use the helper function get_task_from_id
beccaelenzil
left a comment
beccaelenzil
left a comment
There was a problem hiding this comment.
Great work on this complex Flask project. You've made good use of helper function and instance methods. I've left a few inline comments on small ways to consider refactoring. Please let me know if you have any questions. Nice work!
| "title": self.title, | ||
| } | ||
|
|
||
| def task_list(self): |
There was a problem hiding this comment.
Nice work encapsulating this functionality in an instance method.
| def get_task_from_id(task_id): | ||
| valid_int(task_id, "task_id") | ||
| return Task.query.get_or_404(task_id, description="{task not found}") |
There was a problem hiding this comment.
Consider implementing a similar method for goals
| if "title" not in request_body or "description" not in request_body\ | ||
| or "completed_at" not in request_body: | ||
| return jsonify({"details": "Invalid data"}), 400 | ||
|
|
||
| new_task = Task(title=request_body["title"], |
There was a problem hiding this comment.
Consider encapsulating this functionality in a helper method that either aborts for invalid data or returns the new_task if the request body is valid
| task = Task.query.get(task_id) | ||
|
|
||
| if task is None: | ||
| return jsonify(None), 404 |
There was a problem hiding this comment.
To DRY up your code, you could use the get_task_from_id helper function here.
| elif sort == "desc": | ||
| query = query.order_by(Task.title.desc()) | ||
|
|
||
| query = query.all() # Final query |
There was a problem hiding this comment.
Consider naming query tasks to enhance readability.
| query = query.all() # Final query | |
| tasks = query.all() # Final query |
| query = query.all() # Final query | |
| query = query.all() # Final query |
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable =True) | ||
| is_complete = db.Column(db.Boolean) |
There was a problem hiding this comment.
is_complete should not be a separate attribute. is_complete us False when completed_at is None and True when completed_at has a value as you calculated below on line 20 (nice work there!). The danger in adding is_complete as another attribute on the task model is that we could store data that is inconsistent
| return { | ||
| "id": goal.goal_id, | ||
| "title": goal.title, | ||
| "tasks": goal.task_list() |
| "title": "Updated Goal Title" | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) |
There was a problem hiding this comment.
Nice work including the test that queries the database an make sure the change has been persisted.
2 participants
No description provided.