-
Notifications
You must be signed in to change notification settings - Fork 79
Tigers - Kat and Sunny - Wave 03-06 #34
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
base: main
Are you sure you want to change the base?
Conversation
… refactored code for updating, reading one planet, read all planets, add planet and validate model routes file
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 Kat & Sunny, this is well done. I left some minor comments here. If you have questions ping me in slack.
def to_dict(self): | ||
return { | ||
"id": self.id, | ||
"name": self.name, | ||
"description": self.description, | ||
"diameter": self.diameter | ||
} | ||
|
||
@classmethod | ||
def from_dict(cls, planet_data): | ||
new_planet = Planet(name=planet_data["name"], | ||
description=planet_data["description"], | ||
diameter=planet_data["diameter"]) | ||
return new_planet | ||
|
||
def update(self, req_body): | ||
try: | ||
self.name = req_body["name"] | ||
self.description = req_body["description"] | ||
self.diameter = req_body["diameter"] | ||
except KeyError as error: | ||
abort(make_response({"message": f"Missing attribute: {error}"},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.
Great helper methods!
request_body = request.get_json() | ||
new_planet = Planet.from_dict(request_body) | ||
|
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.
Some some validation on request body would be nice here to ensure required fields are present.
planets_response = [planet.to_dict() for planet in planets] | ||
return jsonify(planets_response) | ||
|
||
def validate_model(cls, model_id): |
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 function
}] | ||
assert len(response_body) == 2 | ||
|
||
def test_create_one_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.
Having a test with an invalid request body would be nice.
def update_planet(planet_id): | ||
planet = validate_model(Planet,planet_id) | ||
|
||
request_body = request.get_json() | ||
|
||
planet.update(request_body) | ||
db.session.commit() | ||
|
||
return make_response(jsonify(f"Planet #{planet.id} successfully updated")) | ||
|
||
@planets_bp.route("/<planet_id>", methods=["DELETE"]) | ||
def delete_planet(planet_id): | ||
planet = validate_model(Planet,planet_id) | ||
|
||
db.session.delete(planet) | ||
db.session.commit() | ||
|
||
return make_response(jsonify(f"Planet #{planet.id} successfully deleted")) | ||
|
||
|
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.
Just noting these functions are untested.
No description provided.