Bumblebee Class - Jeslyn Lai #22
Conversation
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert response_body == {"message": "Task 1 not found"} |
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.
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert response_body == {"message": "Task 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.
| response = client.put("/goals/1", json={ | ||
| "title": "Updated Goal Title" | ||
| }) | ||
| # Assert | ||
| # ---- Complete Assertions Here ---- | ||
| # assertion 1 goes here | ||
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| assert response.status_code == 204 | ||
|
|
||
| query = db.select(Goal).where(Goal.id == 1) | ||
| goal = db.session.scalar(query) | ||
|
|
||
| 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.
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert response_body == {"message": "Goal 1 not found"} |
There was a problem hiding this comment.
👍 Our supplied structure for this test differs from the DELETE Task route. The Task version checked the DB directly, while this one uses a follow-up GET request. The Goal version keeps the test at a similar level of abstraction throughout (what a client might be able to determine without direct DB access). This potentially offers additional freedom in implementation, where we could decide to handle deletion by only marking a record as deleted with an additional model attribute rather than actually deleting it. In business systems, marking records deleted tends to be preferred, since data is valuable. Deleting one record could necessitate deleting many related records, resulting in a loss of a lot of information that could have been used for other purposes.
| batch_op.add_column(sa.Column('title', sa.String(), nullable=False)) | ||
| batch_op.add_column(sa.Column('description', sa.String(), nullable=False)) | ||
| batch_op.add_column(sa.Column('completed_at', sa.DateTime(), nullable=True)) | ||
| batch_op.add_column(sa.Column('is_complete', sa.Boolean(), nullable=False)) |
There was a problem hiding this comment.
👀 This line is triggered by the addition of the is_complete Task attribute. We don't actually want this column, as it duplicates information that could already be determined from the other Task attributes (i.e., completed_at).
|
|
||
| #Create a Goal: Valid Goal | ||
| @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_id: | ||
| 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.
| for task in goal.tasks: #Add this to pass the test test_post_task_ids_to_goal_already_with_goals | ||
| task.goal_id = None | ||
|
|
||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| if task.goal_id is None: | ||
| 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.
On the other hand, updating each Task individually does prevent us from needing to keep all the tasks in memory at once.
| request_body = request.get_json() | ||
| task_ids = request_body.get("task_ids", []) | ||
|
|
||
| for task in goal.tasks: #Add this to pass the test test_post_task_ids_to_goal_already_with_goals |
There was a problem hiding this comment.
Notice that this isn't just about passing the test. The test implies certain behaviors, in this case, replacement behavior. So our route needs to implement replacement (rather than append) behavior, of which one way to accoplish this is by disassociating any previous tasks from the goal.
| tasks = [task.to_dict() for task in goal.tasks] | ||
| response = goal.to_dict() | ||
| response["tasks"] = 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.
2 participants
No description provided.