Skip to content
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 - Mica + Reyna Solar System API Parts 1 + 2 #31

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

reynamdiaz
Copy link

@reynamdiaz reynamdiaz commented Oct 26, 2022

Part 1 - Waves 01 + 02
Part 2 - Waves 03 - 06

@reynamdiaz reynamdiaz changed the title Tigers - Mica + Reyna Solar System API Part 1 Tigers - Mica + Reyna Solar System API Parts 1 + 2 Nov 3, 2022
Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nic ework Reyna & Mica, you did really well here. I particularly like your validation work on the routes. I left a few minor comments let me know if you have questions via Slack.

Comment on lines +10 to +23
@classmethod
def from_dict(cls, planet_data):
new_planet = Planet(name=planet_data["name"],
description=planet_data["description"],
moons=planet_data["moons"])
return new_planet

def to_dict(self):
return {
"id": self.id,
"name": self.name,
"description": self.description,
"moons": self.moons
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great helper methods!

Comment on lines +25 to +27
if ("name" not in request_body or "description" not in request_body
or "moons" not in request_body):
return make_response(f"Invalid Request", 400)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great validation, could this be abstracted in to a helper method and used in the update action as well?

planets_response.append(planet.to_dict())
return jsonify(planets_response)

def validate_model(cls, model_id):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Great helper function

Comment on lines +82 to +85
planet.name = request_body["name"]
planet.description = request_body["description"]
planet.moons = request_body["moons"]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some validation here would be good

assert response.status_code == 404
assert response_body == {"message": "Planet 6 not found"}

def test_create_one_planet(client):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also test an invalid create action.

Comment on lines +77 to +97
def update_planet(planet_id):
planet = validate_model(Planet, planet_id)

request_body = request.get_json()

planet.name = request_body["name"]
planet.description = request_body["description"]
planet.moons = request_body["moons"]

db.session.commit()

return make_response(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(f"Planet #{planet.id} successfully deleted")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting the delete and update actions are untested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants