Bumblebees, Kate Marantidi #33
Conversation
| @@ -1,4 +1,4 @@ | |||
| # Wave 6: Establishing a One-to-Many Relationship | |||
| # Wave 6: Establishing a One-to-Many Relationship | |||
There was a problem hiding this comment.
🔎 When adding files to a commit, be sure to check whether there are any unintentional changes that can be undone.
| # Act | ||
| response = client.get("/goals/1/tasks") | ||
| response_body = response.get_json() | ||
| print(response_body) |
There was a problem hiding this comment.
🔎 Be sure to remove debug code before checking changes in.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": "Task with 1 does not exist"} |
There was a problem hiding this comment.
Checking for the "not found" message is the main behavior we want to verify. If we were particularly paranoid, we might also check that no record was surreptitiously added to the database. This is less of a concern for the other "not found" tests, since they wouldn't involve any logic that could conceivably add a new record, but since PUT replaces data, it could be the case that an implementer might have added logic to create the record if it were missing. That is, they could treat a PUT to a nonexistent record as a request to create that particular record.
Again, that's not a requirement, but it is something to keep in mind.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {'message': 'Task with 1 does not exist'} |
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.
| response = client.put("/goals/1", json={ | ||
| "title": "Updated Goal Title" | ||
| }) | ||
| query = db.select(Goal).where(Goal.id == 1) | ||
| goal = db.session.scalar(query) | ||
|
|
||
| # Assert | ||
| assert response.status_code == 204 | ||
| assert response.headers.get("Content-Length") == None | ||
| assert goal.title == "Updated Goal Title" |
There was a problem hiding this comment.
👍 Comparing this test with the Task update test gives us an idea of what we should verify here. After performing the PUT, we need to confirm both the response (204 No Content) and check that the change has actually occurred in the database.
(I think the request for 3 asserts might be left over from an older version of this project, so you could leave out the check for the Content Length.)
|
|
||
|
|
||
| @bp.post("") | ||
| def create_goal(): |
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.
| if self.goal: | ||
| task_as_dict["goal_id"] = self.goal.id |
There was a problem hiding this comment.
👍 Nice logic to add the goal data to the task dictionary only when it's present.
| if "task_ids" in request_body: | ||
| for task in goal.tasks: | ||
| task.goal_id = None | ||
|
|
||
| task_list = request_body.get("task_ids") | ||
|
|
||
| for task_id in task_list: | ||
| task = validate_model(Task, task_id) | ||
| task.goal_id = goal.id |
There was a problem hiding this comment.
Since we need to validate each Task id anyway, if we stored the resulting tasks in a list, the replacement of the tasks for the goal could be accomplished using the tasks property as
goal.tasks = taskswhich avoids the need to manually clear the goal association from the existing tasks.
app/routes/goal_routes.py
Outdated
| goal_dict = goal.to_dict() | ||
| goal_dict["tasks"] = [task.to_dict() for task in tasks] |
There was a problem hiding this comment.
👍 Nice reuse of your to_dict methods to build up your response. Rather than leaving this logic here in the route, we could move it to an additional Goal method. Perhaps to_dict_with_tasks, or we could add a parameter to the main Goal to_dict that determines whether or not to include the Task details.
…tant (modified class Goal and Task to add more functionality, moved Slack API to utilities)
…y and Slack API bot
app/routes/task_routes.py
Outdated
| @bp.patch("/<task_id>/mark_complete") | ||
| def completed_task_notification_by_API(task_id): | ||
| task = validate_model(Task, task_id) | ||
| task.title = "My Beautiful Task" |
|
|
||
| return Response(status=204) | ||
| # ADDED mimetype to fix json | ||
| return Response(status=204, mimetype="application/json") |
| # } | ||
|
|
||
|
|
||
| # CORRECTED YELLOW ISSUE #2 (There are two route handlers registered for /tasks/id/mark_incomplete) |
| return task_to_dict | ||
| return task_to_dict | ||
|
|
||
| def send_message_task_complete_slack(task_title): |
There was a problem hiding this comment.
👍 Nice to see this logic extracted from the route.
| logger = logging.getLogger(__name__) | ||
| logger.error(f"Slack message failed: {response.json()}") |
There was a problem hiding this comment.
👍 Oh cool. You got the logger instance.
| logger.info("Database initialized successfully.") | ||
|
|
||
| with app.app_context(): | ||
| db.session.execute("SELECT 1") | ||
| logger.info("DB connection test passed") | ||
| except Exception as e: | ||
| logger.exception("Error during DB initialization or connection") |
There was a problem hiding this comment.
🔎 Notice the use of logger up here will result in crashing the startup. logger isn't available until line 43.
…l and task routes. Final commit
2 participants
No description provided.