Bumblebee - Gitika K #34
Conversation
| assert response_body == { | ||
| "message": "Task ID (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.
|
|
||
| raise Exception("Complete test with assertion about response body") | ||
| # raise Exception("Complete test with assertion about response body") | ||
| assert response_body == {"message": "Task 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.
| assert response_body == { | ||
| "message": "Goal ID (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.
| @@ -0,0 +1,39 @@ | |||
| """Create Task model | |||
There was a problem hiding this comment.
👍 Nice use of messages to describe the migrations. We can see that the message is written into the migration file itself, as well as becoming a part of the file name. 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.
| from dotenv import load_dotenv | ||
| load_dotenv() # Reads .env file and Loads all key-value pairs into environment |
There was a problem hiding this comment.
It should not be necessary to add this explicit call to load_dotenv here. In all the places where we expect Flask to locate these settings in the .env file, Flask will do so properly (for instance, we don't need this call in deployment, since we load the values directly in the deployed environment).
| if self.goal_id is not None: | ||
| task_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.
| # Clear existing task associations first | ||
| for task in goal.tasks: | ||
| task.goal_id = None | ||
|
|
||
| tasks = [] | ||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| task.goal_id = goal.id | ||
| tasks.append(task) |
There was a problem hiding this comment.
Once we have a list of validated tasks, the replacement of the tasks for the goal can be accomplished using the tasks property as
goal.tasks = taskswhich avoids the need to manually clear the goal association from the existing tasks.
| tasks_response = [] | ||
| for task in goal.tasks: | ||
| task_dict = task.to_dict() | ||
| task_dict["goal_id"] = goal.id # add goal_id if not in to_dict | ||
| tasks_response.append(task_dict) |
There was a problem hiding this comment.
Your Task to_dict method already includes the "goal_id" key in the dict of any Task belonging to a Goal, so we don't need to manually add the Goal id 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 { | ||
| "id": goal.id, | ||
| "title": goal.title, | ||
| "tasks": tasks_response |
There was a problem hiding this comment.
The task_list collection (or the comprehension that created it) could be used here directly.
2 participants
No description provided.