C22 Phoenix - Luqi Xie #29
Conversation
| # assertion 2 goes here | ||
| assert "goal" in response_body |
There was a problem hiding this comment.
We do provide tests with assertions like this, which look for the wrapper key before comparing the dictionary values, but it might be more useful to retrieve the goal from the database and confirm that the expected change was applied.
| assert response_body == { | ||
| "details": 'Goal 1 "Build a habit of going outside daily" successfully deleted' | ||
| } |
There was a problem hiding this comment.
👀 The response_body hasn't been updated to get the "missing" message from this second API call. That's why we're still seeing the first response. We would need to add something like
response_body = response.get_json()We should make sure that what we're actually asserting makes sense for the case, and not just pass the test. Here, if we try to retrieve the goal we just deleted, we shouldn't be getting the delete success message again.
| # ***************************************************************** | ||
| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| assert response_body == {"message":"Goal 1 not found"} |
| @@ -0,0 +1,32 @@ | |||
| """add goal model | |||
There was a problem hiding this comment.
Nice use of labels on the migrations to be able to tell what each is doing at a glance. Often, since the migrations are named first with a hash, they don't list in the order they were created, making it difficult to trace through a series of migrations.
There was a problem hiding this comment.
It looks like you went through a couple of steps to create your migrations. This is completely fine, and reflects the progress of the work.
On the other hand, while I'm still in "development phase", I often like to have just one clean migration to setup the initial database. So another approach would be to periodically clear out the database and migrations, and make a fresh one once you've got the models configured as desired. This isn't a good idea once a project has been deployed publicly (since we would lose any exisiting data when we drop the database), but can make the migrations a little easier to understand for setting up a fresh deployment.
| from typing import TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
| from .goal import Goal |
There was a problem hiding this comment.
Nice inclusion of this extra check to clear up the type warnings in VS Code.
| "id": goal.id, | ||
| "title": goal.title | ||
| } |
There was a problem hiding this comment.
Prefer to use the to_dict method here.
| request_body = request.get_json() | ||
|
|
||
| goal.title = request_body["title"] |
There was a problem hiding this comment.
Like for Task, we might want similar error handling here as for the POST route.
| "id": goal.id, | ||
| "title": goal.title, | ||
| "tasks": [task.to_dict() for task in goal.tasks] |
There was a problem hiding this comment.
Since this is the only route that includes the child tasks, and it's being explicitly added here, that would allow us to remove the logic to do some from to_dict, leaving that to build a dictionary of only the scalar values of Goal. We could still use to_dict here, and then add in the tasks collection that we generate.
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| goal.tasks.append(task) |
There was a problem hiding this comment.
This logic appends each task to the current goal, so any pre-existing tasks would still be associated with the goal. In this case, we'd want the result to show all of the tasks now associated with the goal. See below.
| response = { | ||
| "id": goal.id, | ||
| "task_ids": task_ids} |
There was a problem hiding this comment.
Rather than echoing back the input task ids, since we appended the new tasks to the existing goal, we should fetch the task ids directly from the goal, in case there were any other tasks already associated with this goal. Something like this
"task_ids": [task.id for task in goal.tasks]}
No description provided.