Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Your implementation looks good overall! Please review my comments, and let me know if you have any questions.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": f"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.
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": f"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={ | ||
| "id": 1, | ||
| "title": "My New Goal" | ||
| }) | ||
|
|
||
| # Assert | ||
| # ---- Complete Assertions Here ---- | ||
| # assertion 1 goes here | ||
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| # ---- Complete Assertions Here ---- | ||
| assert response.status_code == 204 | ||
|
|
||
| query = db.select(Goal).where(Goal.id == 1) | ||
| goal = db.session.scalar(query) | ||
|
|
||
| @pytest.mark.skip(reason="test to be completed by student") | ||
| assert goal.id == 1 | ||
| assert goal.title == "My New Goal" |
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.
| query = db.select(Goal).where(Goal.id == 1) | ||
| assert db.session.scalar(query) == None |
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.
So this logic does check that Goal 1 no longer remains in the database, but what we intended was to check the error message from the GET request that the goal could not be found.
| @@ -0,0 +1,38 @@ | |||
| """empty message | |||
There was a problem hiding this comment.
Remember to use the -m option when creating a migration to give it a descriptive message. Doing so would write the message into the migration file itself (where we currently see empty message). It would also become part of the filename. Since the migration files are otherwise just named with a hash, this can help us understand what each migration does.
There was a problem hiding this comment.
It looks like your migration generation went pretty smoothly. We had a fairly short roadmap of things to do. In a more extensive app, we might not have known the models and attributes so clearly. We might have needed to make additional changes, or experiment with a few approaches, which could lead to a more complicated set of migrations. In that case, I like to clear and recreate a single clean migration that captures all of the initial setup. Migrations really become more useful once an application has been deployed and contains production-critical data that we don't want to lose as we continue to make DB changes over time. But there's no need to start our project with superfluous migrations.
app/routes/goal_routes.py
Outdated
|
|
||
| @goals_bp.get("") | ||
| def get_all_goals(): | ||
| query = db.Select(Goal) |
There was a problem hiding this comment.
Here's that capital Select again. Prefer db.select()
| goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
|
||
| @goals_bp.post("") | ||
| def create_tasks(): |
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.
app/routes/goal_routes.py
Outdated
| for task_id in request_body["task_ids"]: | ||
| new_task = validate_model(Task, task_id) | ||
| new_task.goal_id = goal_id | ||
| db.session.commit() |
There was a problem hiding this comment.
👀We should wait to run the commit until all of the tasks have been processed. If we commit as we are looping, then we could end up finalizing a change in the database before the entire operation was validated. Consider a request with a mixture of valid and invalid task ids. We should prefer to have such a request fail completely. But if we commit after each Task, then we could clear the Goal of its associated tasks, and write a valid Task back to the Goal, only to fail to validate the next task id. This would leave the database in an inconsistent state (the old tasks were cleared, but not all new tasks were associated).
| response = goal.to_dict() | ||
| response["tasks"] = [task.to_dict() for task in goal.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.
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": True if self.completed_at else False |
There was a problem hiding this comment.
👍 Yes, we can compute this from completed_at. I'm not sure off the top of my head what might be considered a falsy datetime read from the DB might be, but to be a bit more focused, we could do something like
"is_complete": self.completed_at is not NoneWe could also consider moving the calculation to a helper function—even though it's only a single expression—to make it clear how we're expecting to interpret this result by giving it a clear function name
| if self.completed_at: | ||
| task_as_dict["completed_at"] = self.completed_at |
There was a problem hiding this comment.
This wasn't asked for in the output, but ends up not getting in the way of any tests.
We didn't have any tests sending datatime information since there's no standard JSON datetime representation. Working with is_complete and toggling completion through the dedicated endpoints avoided the need to do so!
app/routes/task_routes.py
Outdated
| if task.completed_at: | ||
| task.is_complete = True |
There was a problem hiding this comment.
👍 Since we calculate is_complete on demand, no need to set this any longer.
app/routes/goal_routes.py
Outdated
| for task_id in request_body["task_ids"]: | ||
| new_task = validate_model(Task, task_id) | ||
| new_task.goal_id = goal_id | ||
| db.session.commit() |
There was a problem hiding this comment.
👍 We only need to commit once, after all the work is done. Commiting once allows a clean rollback to occur in the case on an error part of the way through processing the tasks. While it may feel strange to have that happen, we always want database operations to appear as though they entirey succeeded, or entirely failed. We shouldn't leave data in an inconsistent state where only part of a modication was applied.
2 participants
No description provided.