ns-task-list-api #39
ns-task-list-api #39
Conversation
…tests in test_wave_02
…hed tasks and passed all tests in test_wave_03
…r goal_bp, fixed route_utilities, wrote and passed test_wave_05.py
There was a problem hiding this comment.
YELLOW - No need to make changes unless you would like to! It is receiving a Yellow because two of the tests are not passing and could benefit from refactoring.
Overall, this is a good API that mostly works, but could use some refining. A few areas to focus on:
-
When it comes to error handling, you have a function that does most of it for you (
validate_model). After using validate_model however, you often included the same error handling that had already been done. Make sure you aren't overdoing it in the realm of error handling! -
You've got some really great helper functions and class functions that you use in some cases and not in others. This includes functions like the
to_dictor thefrom_dictor others. If you are writing these functions, make sure you are using them! There were several places where you could have condensed some code with them. -
In general, make sure you are clear about what is happening in each route. There were times where certain routes felt overfilled and certain processes were being repeated. Don't be afraid to go through and find places where you can remove redundant code!
Overall, well done, Natalie!
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp |
|
|
||
| return app | ||
|
|
There was a problem hiding this comment.
The rest of your configuration and blueprint registration looks good!
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| tasks: Mapped[list["Task"]] = relationship("Task", back_populates="goal") |
There was a problem hiding this comment.
This model looks good! Nice one-to-many mapping!
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| tasks: Mapped[list["Task"]] = relationship("Task", back_populates="goal") | ||
| def to_dict(self): |
There was a problem hiding this comment.
Small nitpick here, but make sure you add some space before your functions!
| "id": self.id, | ||
| "title": self.title | ||
| } | ||
| @classmethod |
There was a problem hiding this comment.
Same thing here, some whitespace can drastically increase readability!
|
|
||
| tasks_response = [task.to_dict() for task in tasks] | ||
| return tasks_response, 200 |
| } | ||
|
|
||
| return task_response, 200 |
| abort(make_response({"message": f"task_id {task_id} not found"}, 404)) |
There was a problem hiding this comment.
This should be caught by validate_model
| db.session.commit() | ||
|
|
||
| url = "https://slack.com/api/chat.postMessage" | ||
| slack_token = os.environ.get("SLACK_BOT_TOKEN") | ||
| headers = { | ||
| "Authorization": f"Bearer {slack_token}", | ||
| "Content-Type": "application/json" | ||
| } | ||
| request_body = { | ||
| "channel": "C07US894WRL", | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
| response = requests.post(url, json=request_body, headers=headers) |
There was a problem hiding this comment.
It couldn't hurt to move this to a helper function!
| abort(make_response({"message": f"task_id {task_id} not found"}, 404)) |
There was a problem hiding this comment.
Validate model will already take care of this!
No description provided.