Sapphire - Monica L. #15
Conversation
There was a problem hiding this comment.
Great work on this project Monica! I'm really really really sorry this review is coming so late. I hope it's still helpful somehow.
Overall, your project implementation is great! All of the features I was expecting and looking for look good here. Your tests look good, too. Most of my feedback is around small nitpicks around code style. In addition, I encourage you to use more descriptive git commit messages-- naming which bug is being fixed is crucial!
Besides that, this is a solid API implementation and a solid project submission! You did great work here, I'm so sorry this review is so late, and please let me know any comments and questions you have.
| def validate_object(cls, object_id): | ||
| # handle invalid object id, return 400 | ||
| try: | ||
| object_id = int(object_id) | ||
| except: | ||
| abort(make_response({"msg": f"{cls.__name__} {object_id} is invalid."}, 400)) | ||
|
|
||
| obj = cls.query.get(object_id) | ||
| if obj is None: | ||
| abort(make_response({"msg": f"{cls.__name__} not found."}, 404)) | ||
|
|
||
| return obj |
| def tasks_to_dict(self): | ||
| return { | ||
| "id": int(self.goal_id), | ||
| "title": self.title, | ||
| "tasks": [task.to_dict() for task in self.tasks] | ||
| } |
There was a problem hiding this comment.
Wonderful :D ! I think this is a really smart helper method to make, and you did it in a very neat and concise way.
| new_task = Task(title=task_data["title"], | ||
| description=task_data["description"], | ||
| completed_at=task_data["completed at"]) | ||
| return task_data |
There was a problem hiding this comment.
In this case, it seems like new_task isn't being used, and you're returning the input task_data... It seems like it hasn't caused any bugs so far, but if there are any bugs in the future I might check into this method.
| def validate_model_by_id(model, id): | ||
| #handle invalid task id, return 400 | ||
| try: | ||
| id = int(id) | ||
| except: | ||
| abort(make_response(jsonify({"msg": f"{model.__name__}{id} is invalid."}), 400)) | ||
|
|
||
| model_object = model.query.get(id) | ||
|
|
||
| if model_object is None: | ||
| abort(make_response(jsonify({"msg": "task not found"}), 404)) | ||
|
|
||
| return model_object |
There was a problem hiding this comment.
It seems like you have a more generic implementation of the same logic in helper_routes.py named validate_object, which your goal_routes file uses. I think it'd work out if this validate_model_by_id method was deleted, and instead you usedyour generic validate_object, to help keep your code DRY.
| if sorting_query == "asc": | ||
| tasks=Task.query.order_by(Task.title.asc()) | ||
| elif sorting_query=="desc": | ||
| tasks=Task.query.order_by(Task.title.desc()) | ||
| else: | ||
| tasks = Task.query.all() |
There was a problem hiding this comment.
Nitpick: Consistency with spacing!
if sorting_query == "asc":
tasks = Task.query.order_by(Task.title.asc())
elif sorting_query == "desc":
tasks = Task.query.order_by(Task.title.desc())
else:
tasks = Task.query.all()| get_task = Task.query.get(task_id) | ||
| task = validate_model_by_id(Task, task_id) |
There was a problem hiding this comment.
In this case, you're getting the correct instance of Task that you need with task = validate_model_by_id ..., so you probably don't need get_task = ...
| print("1111111111111111111111111") | ||
| task = validate_model_by_id(Task, task_id) | ||
| print("222222222222222222222") | ||
| if task.completed_at is not None: | ||
| # The task is already complete, no need to update it | ||
| return {"task": task.to_dict()}, 200 | ||
|
|
||
| task.completed_at = datetime.utcnow() | ||
| print("3**************************") | ||
| db.session.commit() | ||
|
|
||
| print("4**************************") | ||
| #integrate slack | ||
| slack_token= os.environ["SLACK_TOKEN"] | ||
| client = WebClient(token=slack_token) | ||
| result = client.chat_postMessage(channel="task-notifications", | ||
| text=f"Someone just completed the task {task.title}") | ||
| print("5**************************") | ||
|
|
||
| return {"task": task.to_dict()}, 200 | ||
| ############ |
There was a problem hiding this comment.
Nitpick: Feel free to delete your print statements before submission!
| nullable=True) | ||
|
|
||
| def to_dict(self): | ||
| dictionary = { "id": self.task_id, |
There was a problem hiding this comment.
Nitpick: instead of dictionary as a variable name, I think that something like task would be better.
| print("4**************************") | ||
| #integrate slack | ||
| slack_token= os.environ["SLACK_TOKEN"] | ||
| client = WebClient(token=slack_token) |
| # Assert | ||
| assert response.status_code == 200 | ||
| assert "details" in response_body | ||
| assert response_body == { | ||
| "details": 'Goal 1 "Build a habit of going outside daily" successfully deleted' | ||
| "details": 'goal 1 "Build a habit of going outside daily" successfully deleted' |
There was a problem hiding this comment.
I'm surprised that you changed the test to lowercase instead of changing your implementation 🤔
No description provided.