Conversation
…ted. Planets endpoint created.
…r non-integer planet id and non-existing planet addressed.
…cord, and refactored endpoint to get all records from database table.
… query params to get a planet by name.
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice job with Solar System!
I left some comments that I'm hoping you can apply to your work on Task List!
| planets_response = [] | ||
| for planet in planets: | ||
| planets_response.append({ | ||
| "id": planet.id, | ||
| "name": planet.name, | ||
| "description": planet.description | ||
| }) |
There was a problem hiding this comment.
This would be a good spot to try out list comprehension. You could create an instance method in the Planet class to_dict that turns an instance of planet into a dictionary representing a planet. We need to send back objects represented by dictionaries since our client doesn't understand python.
| class Planet(db.Model): | ||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
| name = db.Column(db.String) | ||
| description = db.Column(db.String) No newline at end of file |
There was a problem hiding this comment.
Would be good to add the instance method to_dict and class method from_dict to this class, which will shorten up the code in your routes.
def to_dict(self):
return dict(
id = self.id,
name = self.name,
description = self.description
)
@classmethod
def from_dict(cls, planet_data):
return cls(
name = planet_data["name"],
description = planet_data["description"]
)No problem if you don't have time to refactor here, but try implementing this for Task List.
|
|
||
| planets_bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
|
|
||
| @planets_bp.route("", methods=["GET", "POST"]) |
There was a problem hiding this comment.
Prefer that GET and POST are separate routes
| }) | ||
| return jsonify(planets_response) | ||
|
|
||
| elif request.method == "POST": |
There was a problem hiding this comment.
Older patterns for designing routes in Flask have shown a route that handles different REST methods and then uses if/elif to conditionally return the correct response. By separating the two request types out into their own methods, the code becomes more readable and each function has a single responsibility.
| request_body = request.get_json() | ||
| new_planet = Planet(name=request_body["name"], | ||
| description=request_body["description"]) |
There was a problem hiding this comment.
Would be good to add in some error handling here since request_body is the data that a client sends via the request body. We should assume that requests could have bad data, either missing keys or values or the keys could be misspelled.
If the request_body didn't have "name" but had "planet_name" then line 28 would throw an unhandled exception.
| return planet | ||
|
|
||
|
|
||
| @planets_bp.route("/<planet_id>", methods=["GET", "PUT", "DELETE"]) |
There was a problem hiding this comment.
Prefer each request type to have their own function.
|
|
||
| db.session.add_all([planet_mars, planet_jupiter]) | ||
|
|
||
| db.session.commit() No newline at end of file |
There was a problem hiding this comment.
I know that the Learn reading has a two_saved_books that looks like this. It creates two objects, adds them and commits to the DB. It would be nice if this fixture (while not shown in Learn) actually returns the objects it creates so that you can use them for comparison in your unit tests, like this and this
| assert response_body == [{ | ||
| "id": 1, | ||
| "name": "Mars", | ||
| "description": "Smallest planet" | ||
| }, | ||
| { | ||
| "id": 2, | ||
| "name": "Jupiter", | ||
| "description": "Largest planet" | ||
| }] |
There was a problem hiding this comment.
If the fixture two_saved_planets returned two planet objects, you'd be able to compare what's in the response body against what was used to create records in the database (as we do in Flasky)
| response = client.post("/planets", json={ | ||
| "name": "Earth", | ||
| "description": "Water planet" | ||
| }) | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 201 | ||
| assert response_body == "Planet Earth successfully created" No newline at end of file |
There was a problem hiding this comment.
In addition to using the response to write assertions, your test would be more robust if after the post request, you query the database for the object you just created. Since we run fixtures before each test, we know that before your test runs we will have a database with empty tables, after your post request in the test we will have one planet record. We can query for that one record by assuming it's id will be 1 and compare its attributes to the request body you sent with the post request. Like this
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response(f"Planet #{planet.id} successfully updated") |
There was a problem hiding this comment.
When we pass a string to make_response(), the datatype that gets sent back as a header in the response is HTML. However, your GET /planets and POST /planets routes return jsonify'd objects which have a datatype of JSON. Therefore, one API is sending a mix of HTML and JSON responses, which isn't ideal.
I know this is what was shown in Learn 😖. In the last reading from Part 6 Testing, there's mention of using jsonify like this:
return make_response(jsonify(f"Planet #{planet.id} successfully updated"))
Doing so changes the header data type from HTML to JSON. Making the changes in the DELETE requests would make your API only return JSON responses, which is good.
An even better way to go about this, I think, would be for your PUT and your POST requests to just return a dictionary representing the planet that was inserted into the DB or the planet that was updated in the DB. So for your PUT and your POST request you could return a response like:
return jsonify(planet.to_dict()), 201 # or whatever correct status code you want to pass in the response.2 participants
No description provided.