C22 Phoenix - Liubov D. #46
Conversation
There was a problem hiding this comment.
Looks good overall. Some tests were modified to account for the response approach taken, but we should leave them as is and make changes to the response approach. Please restore the tests back to their original version, and get them to pass by modifying your route logic. Review my comments, and let me know if you have any questions.
tests/test_wave_01.py
Outdated
| @@ -46,12 +46,13 @@ def test_get_task(client, one_task): | |||
| "id": 1, | |||
| "title": "Go on my daily walk 🏞", | |||
| "description": "Notice something new every day", | |||
| "is_complete": False | |||
| "is_complete": False, | |||
| "goal_id": None | |||
There was a problem hiding this comment.
We should not change the response structure expected by this test. The way the tests are written, we do not expect any goal information to appear in the task response if the task is not associated with a goal. Changing the test changes the specification.
How can we modify the logic that builds the task dictionary so that we don't need to include the goal_id in the response here?
| return response, 200 | ||
|
|
||
| def validate_task(task_id): |
There was a problem hiding this comment.
👀 Note that we could refactor this validate_task and the validate_goal functions to be able to handle both.
| return response, 200 | ||
|
|
||
| def validate_goal(goal_id): |
There was a problem hiding this comment.
👀 Note that we could refactor this validate_goal and the validate_task functions to be able to handle both.
| db.session.commit() | ||
|
|
||
| if os.environ.get('SEND_SLACK_NOTIFICATIONS') == "True": |
There was a problem hiding this comment.
Was this added to silence notification sending during tests? There was an accidental change to the wave 3 tests that would have resulted in notifications being sent during tests.
tests/test_wave_03.py
Outdated
| # Act | ||
| response = client.patch("/tasks/1/mark_complete") | ||
| # Act | ||
| response = client.patch("/tasks/1/mark_complete") |
There was a problem hiding this comment.
👀 This got accidentally unindented. It needs to remain under the with block so that the post behavior (used by by the Slack integration) is deisabled while the endpoint is called. With this unindented, each time this test runs, it will attempt to send a Slack message.
| for task_id in task_ids: | ||
| task = validate_task(task_id) | ||
| task.goal_id = goal_id |
There was a problem hiding this comment.
Notice that by updating the goal id associated with the tasks, this effectively has "append" behavior (any existing tasks already associated with this Goal will remain unchanged). The tests don't provide enough specificity to determine whether append or replace behavior is desired, but we should be consistent between the operation we carry out here, and the result that we return (see below).
app/routes/goal_routes.py
Outdated
| task.goal_id = goal_id | ||
| db.session.add(task) | ||
| db.session.commit() |
There was a problem hiding this comment.
👀 We should wait to commit this change until all the tasks have been processed. Otherwise, we could start updating tasks to be related to this goal, but then find an invalid task later in the input list. This would have the effect that only part of the request would have been applied, leaving the system in an inconsistent configuration.
| response = { | ||
| "id": int(goal_id), | ||
| "task_ids": task_ids |
There was a problem hiding this comment.
Echoing back the task_ids that were supplied here effectively acts like a "replace" behavior. We assume that the only tasks belonging to the Goal after this endpoint runs are the tasks we were given in this call. But the logic above is taking an "append" approach, meaning there could still be other tasks associated with this goal, which we should list here (by fetching the ids of all tasks now associated with the goal rather than using the task ids only supplied to this call).
| response["title"] = goal.title |
There was a problem hiding this comment.
A dictionary with these two keys could be produced by the goal to_dict method (if it weren't wrapping the goal dict in an additional layer). Then we could graft in the additional required task data.
app/routes/goal_routes.py
Outdated
|
|
||
| task_dict = dict( | ||
| id=task.id, | ||
| title=task.title, | ||
| description=task.description, | ||
| is_complete=task.completed_at!=None, | ||
| goal_id=task.goal_id | ||
| ) | ||
|
|
||
| response["tasks"].append(task_dict) |
There was a problem hiding this comment.
Since the Task to_dict is wrapping the task data in an additional wrapper dict, that makes it harder to reuse elsewhere. If we didn't have to_dict add the additional wrapper, we could reuse it here as
response["tasks"] = [task.to_dict() for task in tasks]| if self.goal_id is None: | ||
| return dict( | ||
| id=self.id, | ||
| title=self.title, | ||
| description=self.description, | ||
| is_complete=self.completed_at!=None | ||
| ) | ||
| else: | ||
| return dict( | ||
| id=self.id, | ||
| title=self.title, | ||
| description=self.description, | ||
| is_complete=self.completed_at!=None, | ||
| goal_id=self.goal_id | ||
| ) |
There was a problem hiding this comment.
Nice. This addresses the underlying test issue. Notice that there is a significant amount of repetition between the two halves of the conditional. Instead of duplicating so much of the dictionary creation, we could take an approach more like this. (Also remember that we prefer is/is not to compare with None)
result = dict(
id=self.id,
title=self.title,
description=self.description,
is_complete=self.completed_at is not None
)
if self.goal_id:
result["goal_id"] = self.goal_id
return result
Changed the implementation that is used to send messages in Slack from slack_sdk to requests.