Sela Crow HoneyBees Cohort 23 #32
Conversation
created task model
…ime with default noone and true to nullable
…ction in new file in routes folder
…at else...painful
…complete with time stamp
…er the place to see what worked. Created a slack send message function in the router utilities had to poke around for a bit to see what stuck in getting the token read...there was a typo on SLACK_BOT_TOKEN and SlACK_BOT_TOKEN
… 5, next need to connect relationship between goals and tasks
… in dict from and created new routes for showing list of tasks in goals and creating a task in a goal
tests/test_wave_01.py
Outdated
| "description": "Notice something new every day", | ||
| "is_complete": False | ||
| "is_complete": False, | ||
| "goal_id": None, # Expecting None since there's no goal |
There was a problem hiding this comment.
👀 Please don't change the test structure. The dictionary logic should be modified to be able to pass these tests as written, in addition to the later wave 6 tests as written.
tests/test_wave_01.py
Outdated
| "description": "Notice something new every day", | ||
| "is_complete": False | ||
| "is_complete": False, | ||
| "goal_id": None, # Expecting None since there's no goal |
There was a problem hiding this comment.
👀 Please don't change the test structure. The dictionary logic should be modified to be able to pass these tests as written, in addition to the later wave 6 tests as written.
tests/test_wave_01.py
Outdated
| "is_complete": False | ||
| "is_complete": False, | ||
|
|
||
| "goal_id": None, # Expecting goal_id as None |
There was a problem hiding this comment.
👀 Please don't change the test structure. The dictionary logic should be modified to be able to pass these tests as written, in addition to the later wave 6 tests as written.
tests/test_wave_02.py
Outdated
| "is_complete": False}, | ||
| "is_complete": False, | ||
|
|
||
| "goal_id": None, |
There was a problem hiding this comment.
👀 Please don't change the test structure throughout. The dictionary logic should be modified to be able to pass these tests as written, in addition to the later wave 6 tests as written.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": "Task with id (1) not found."} |
There was a problem hiding this comment.
Checking for the "not found" message is again the main behavior we want to verify. And even though this technically changes data, I'd be less likely to check that a record had been added as a result. Unlike the PUT request, which should include all necessary data to make a Task, this endpoint really doesn't have enough information, so it would have been very difficult to assume the endpoint should create the requested Task.
| from ..db import db | ||
|
|
||
|
|
||
| class Goal(db.Model): |
There was a problem hiding this comment.
👍 Your Goal implementation is consistent with your Task model. Check the Task feedback for anything that could apply here.
👀 Note the comment below about the to_dict as well.
|
|
||
|
|
||
| @goal_bp.get("") | ||
| def get_all_goals(): |
There was a problem hiding this comment.
👍 Your Goal CRUD routes are consistent with your Task CRUD routes. Check the Task feedback for anything that could apply here.
| "id": goal.id, | ||
| "title": goal.title, |
There was a problem hiding this comment.
Notice that the id and title keys here are the same as for a regular Goal GET, for which we wrote the to_dict helper. Here, we could use our to_dict to get the regular result dictionary, and then add in the tasks key with the loaded task data.
| return {"message": "'task_ids' field is required and must be a list"}, 400 | ||
|
|
||
| # Query all tasks by the given IDs | ||
| tasks = Task.query.filter(Task.id.in_(task_ids)).all() |
There was a problem hiding this comment.
👀 Task.query is deprecated and should no longer be used. Instead, we could validate all of the tasks in a list comprehension:
tasks = [get_model_by_id(Task, task_id) for task_id in task_ids]| return {"message": "One or more task_ids are invalid"}, 404 | ||
|
|
||
| # Replace the tasks associated with the goal | ||
| goal.tasks = tasks |
There was a problem hiding this comment.
👍 Setting the task collection to a new list of Task models taks care of unsetting any previous tasks associated with the goal, and updates the new Tasks, all in one little assignment.
…so fix goal route for specific task as well as task routes to include attribute IF it has a goal
| "id": self.id, | ||
| "title": self.title, | ||
| "tasks": [task.to_dict() for task in self.tasks] | ||
| # "tasks": [task.to_dict() for task in self.tasks] |
There was a problem hiding this comment.
It would be preferable to remove the unnecessary line rather than commenting it out.
In professional code leaving commented code invites questions about why the code is commented, and whether it should be reinstated every time someone comes across it. Keep your code clean.
| "is_complete": bool(self.completed_at), | ||
| "goal_id": self.goal.id if self.goal else None, # Default to None | ||
| "is_complete": self.completed_at is not None, | ||
| # "goal_id": self.goal.id if self.goal else None, # Default to None |
| if include_goal_id and self.goal: | ||
| task_dict["goal_id"] = self.goal.id | ||
| task_dict["goal_title"] = self.goal.title | ||
| if include_goal_id and self.goal_id: |
There was a problem hiding this comment.
This works as written (with you explicitly passing in True for include_goal_id in a few places). But the intended behavior was to conditionally include the goal id in any Task output if it belongs to a goal, with no need to "turn it on" in certain routes.
The reason the author might feel like this is OK for goal info, but not for task info (on goals) is that the goal info is a single value, while the task info is a whole additional collection of unknown size.
Personally, I disagree with the goal_id coming and going (I would have always included the goal_id with a None value if it didn't belong to a Goal rather than modifying the "shape" of a Task response), but hey, a spec is a spec. 🤷
2 participants
No description provided.