Spruce: Nourhan and Kaitlyn #21
Conversation
Add PUT and DELETE requests
There was a problem hiding this comment.
Overall, this looks good! The main thing to consider going forward is looking for ways to move shared code into helper methods, and to reduce the complexity of the individual route functions by splitting them into their own functions, following the single responsibility principle.
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| from app.models.planet import Planet |
There was a problem hiding this comment.
Once we import blueprints (which in turn import the models), we don't need the model import here any more.
| name = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| type = db.Column(db.String) |
There was a problem hiding this comment.
Consider which columns we might want to make "required" (non-nullable). As it is, with this definition, we could make a Planet with a NULL name, description, and type. Would we want to allow this to happen?
| description = db.Column(db.String) | ||
| type = db.Column(db.String) | ||
|
|
||
| def create_dict(self): |
There was a problem hiding this comment.
Nice helper to create a dictionary structure from a model instance.
| @planet_bp.route("", methods=["GET", "POST"]) | ||
| def handle_planets(): |
There was a problem hiding this comment.
Consider making separate functions for the get and post (the GET method is often called index, while the POST method is often called create). This allows each function to focus on a single piece of functionality, and avoids the need for the if within the route method.
| if name_from_url: | ||
| planets = Planet.query.filter_by(name=name_from_url).all() | ||
| if not planets: | ||
| planets = Planet.query.filter(Planet.name.contains(name_from_url)) |
There was a problem hiding this comment.
Nice fallback query behavior here!
| db.session.add(planet_1) | ||
| db.session.add(planet_2) | ||
|
|
||
| db.session.commit() |
There was a problem hiding this comment.
Consider returning [planet_1, planet_2] after this line, so that tests can make use of the ids of the records, and anything else that's part of the record that might be useful in the test.
| planet1 = dict(id=1, name="Planet 1", description="I'm planet 1", type="gas giant") | ||
| planet2 = dict(id=2, name="Planet 2", description="I'm planet 2", type="gas giant") |
There was a problem hiding this comment.
If two_saved_planets actually returned the records it created in the fixture, that list would be available here and we wouldn't need to build the dictionaries ourselves. That being said, sometimes it's nice to explicitly build the results we are looking for so that we know what's being tested simply by looking at the test itself. It can be a bit of a balancing game.
| assert planet1 in response_body | ||
| assert planet2 in response_body |
There was a problem hiding this comment.
Great job using in to check for the dictionaries in the results. Unless we explicitly ordered the results in our route, we should try to avoid assuming that they are coming back in any particular order, since they will tend to come back in the internal database order, which might not match our intuitive expectations!
| assert response.status_code == 200 | ||
| assert response_body == [] | ||
|
|
||
| def test_get_planet_1_when_none_exist(client): |
There was a problem hiding this comment.
Consider adding a test along the lines of test_get_planet_1_when_two_exist
| assert planet1 in response_body | ||
| assert planet2 in response_body | ||
|
|
||
| def test_create_a_planet_when_none_exist(client): |
There was a problem hiding this comment.
In a test like this, it would be a good idea to ensure that the database has actually been updated with the new record as expected. We can perform a Planet.query.get() call to load the record. As the POST route is currently written, we would have to assume the id that was assigned to use to try to look it up (or try to parse it out of the status message). If the POST route returned more detailed information about the created Planet record, we could use that to retrieve the record from the database instead.
No description provided.