Tigers - Neida #130
Tigers - Neida #130
Conversation
| def to_json(self): | ||
| if self.completed_at: | ||
| is_complete = True | ||
| else: | ||
| is_complete = False | ||
|
|
||
| if self.goal_id: | ||
| return { | ||
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": is_complete, | ||
| "goal_id": self.goal_id | ||
| } | ||
| else: | ||
| return { | ||
| "id": self.task_id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": is_complete | ||
| } |
There was a problem hiding this comment.
This works fine, but you can cut down on a bit of repetition using bool and a local variable for your return value:
def to_json(self):
json_dict = {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at)
}
if self.goal_id:
json_dict["goal_id"] = self.goal_id
return json_dict| # ================================ | ||
| # Create one goal | ||
| # ================================ |
There was a problem hiding this comment.
I like how the borders here make these comments pop.
| @@ -0,0 +1,96 @@ | |||
| from app import db | |||
There was a problem hiding this comment.
Good job splitting this into goal routes and task routes!
| db.session.add(new_goal) | ||
| db.session.commit() | ||
|
|
||
| return jsonify({"goal": new_goal.to_json()}), 201 |
There was a problem hiding this comment.
Since you are returning a dictionary, you shouldn't need to use jsonify here, so you could replace this return statement with return {"goal": new_goal.to_json()}, 201.
I wager there's other places where you used jsonify like this, but I won't point them all out to avoid repeating myself.
| results_list = [] | ||
| for goal in all_goals: | ||
| results_list.append(goal.to_json()) | ||
| return jsonify(results_list), 200 |
There was a problem hiding this comment.
Here, though, you do have to use jsonify since results_list is a list, not a dictionary.
That being said, if your HTTP status code is 200, you don't need to include it in your return value, so you could just do return jsonify(results_list).
That being said, keeping it can be easily justified as a way to be more explicit, which is often good style IMO.
| def validate_id(class_name,id): | ||
| try: | ||
| id = int(id) | ||
| except: | ||
| abort(make_response({"message":f"Id {id} is an invalid id"}, 400)) | ||
|
|
||
| query_result = class_name.query.get(id) | ||
| if not query_result: | ||
| abort(make_response({"message":f"Id {id} not found"}, 404)) | ||
|
|
||
| return query_result |
There was a problem hiding this comment.
Two minor things:
- Since this function is used by both the
goal_routesandtask_routesmodules, I would recommend pulling it out to a sharedutilsmodule or something like that. The idea being to signal to a reader where they could find thevalidate_idmodule that is shared between them both. - While your messages here are pretty good, I think they could be even more informative if you used the
class_nameparameter in them. So for example,f"{class_name} id {id} has an invalid id"would produce"Task id foo has an invalid id". This would be helpful for thePOST /goals/<goal_id>/tasksendpoint where you have bothGoals andTasks you're working with.
| @task_bp.route("/<task_id>/<mark>", methods=["PATCH"]) | ||
| def mark_tasks_complete_or_incomplete(task_id, mark): | ||
| task = validate_id(Task, task_id) | ||
| task.completed_at = datetime.now() if mark == "mark_complete" else None |
There was a problem hiding this comment.
Nice, I like how succinct this is, and saves having to define two different endpoints.
There's a downside in that I can provide other mark values than are acceptable, so PATCH /tasks/13/mark_foo would be the same as PATCH /tasks/13/mark_incomplete (or as a scarier example, PATCH /tasks/13/mark_complet, where I misspelled "complete").
But this is still a good idea, and there's a pretty easy fix.
| # original implementation using slack_sdk to make calls to slack API | ||
|
|
There was a problem hiding this comment.
Good job at explaining why this commented code was here! In general, I recommend getting rid of commented code, especially if it's been committed already and thus is available from the source control history, but if that doesn't work for some reason, this is exactly what you should do!
| def send_to_slack(title, channel_name, mark): | ||
| header_data = {'Authorization': f"Bearer {os.environ.get('SLACK_BOT_TOKEN')}"} | ||
| message_data = {'channel': channel_name, 'text': f"Someone just completed the task {title}"} | ||
| if mark == 'mark_complete': | ||
| requests.post('https://slack.com/api/chat.postMessage', data=message_data, headers=header_data) |
There was a problem hiding this comment.
While I made a comment about the validate_id function, this send_to_slack function is only used by the task_routes so it's fine to remain here.
If you began expanding the Slack integration to goal_routes, then it may make sense to move this to its own module, perhaps something like slack_utils...
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updated Goal Title", | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" |
There was a problem hiding this comment.
Good test design! Especially with going to the DB to verify it's been updated there too!
No description provided.