-
Notifications
You must be signed in to change notification settings - Fork 79
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Tigers - Renee Semhar #20
base: main
Are you sure you want to change the base?
Conversation
reneesu99
commented
Oct 25, 2022
added get method to planets
refactored and added query param moons
…ate app method, created tests folder and checked GET /planets returns200 and test run and passes
added testing for complete code coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work Renee & Semhar, you hit the learning goals here. Well done!
def to_dict(self): | ||
return { | ||
"name": self.name, | ||
"description": self.description, | ||
"moons": self.moons, | ||
"id": self.id, | ||
} | ||
|
||
@classmethod | ||
def from_dict(cls, planet_data): | ||
planet_1 = Planet( | ||
name=planet_data["name"], | ||
description=planet_data["description"], | ||
moons=planet_data["moons"] | ||
) | ||
return planet_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 nice helper functions
@@ -0,0 +1,50 @@ | |||
# from flask import jsonify, abort, make_response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can probably delete this file
@planet_bp.route("", methods=["POST", "GET"]) | ||
def handle_planets(): | ||
if request.method == "POST": | ||
request_body = request.get_json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing some validation here on the request body would be appropriate (make sure it has the required fields).
planets = Planet.query.filter_by(moons = moons_param) | ||
else: | ||
planets= Planet.query.all() | ||
response_body = [planet.to_dict() for planet in planets] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the list comprehension here and use of the to_dict
method.
def validate_id(id, cls): | ||
try: | ||
id = int(id) | ||
except: | ||
abort(make_response ({"Message": f"{cls.__name__} {id} invalid."}, 400)) | ||
|
||
# to access a planet with planet_id in our db, we use | ||
obj = cls.query.get(id) | ||
if not obj: | ||
abort(make_response({"Message": f"{cls.__name__} {id} not found."}, 404)) | ||
return obj |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good helper
return planet.to_dict() | ||
|
||
elif request.method == "PUT": | ||
request_body = request.get_json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validating the request body would be good here.
"description": "i luv storms" | ||
} | ||
|
||
def test_get_query_param_planets(client, two_saved_planets): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test on the query params!
|
||
|
||
|
||
def test_post_planet(client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also testing a post request with an invalid request body would be great.
# Assert | ||
assert response.status_code == 201 | ||
|
||
def test_update_planet(client, two_saved_planets): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also testing a put request with an invalid request body would be great.
You should also test a put request with an invalid/missing planet id.
def test_delete_planet(client, two_saved_planets): | ||
# Act | ||
response = client.delete("/planets/1") | ||
assert response.status_code == 200 | ||
|
||
def test_delete_planet_invalid_id(client, two_saved_planets): | ||
# Act | ||
response = client.delete("/planets/random") | ||
assert response.status_code == 400 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great