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: Misha + Leimomi #24

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

Tigers: Misha + Leimomi #24

wants to merge 35 commits into from

Conversation

leilow
Copy link

@leilow leilow commented Oct 25, 2022

No description provided.

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.

Nice work Misha & Leimomi. This is quite well done. I left some minor comments. Let me know if you have questions/comments via slack.

Comment on lines +11 to +28
def to_dict(self):
planet_as_dict = {}
planet_as_dict["id"] = self.id
planet_as_dict["name"] = self.name
planet_as_dict["color"] = self.color
planet_as_dict["livability"] = self.livability
planet_as_dict["moons"] = self.moons
planet_as_dict["is_dwarf"] = self.is_dwarf
return planet_as_dict

@classmethod
def from_dict(cls, planet_data):
new_planet = Planet(name=planet_data["name"],
color=planet_data["color"],
moons=planet_data["moons"],
livability=planet_data["livability"],
is_dwarf=planet_data["is_dwarf"])
return new_planet

Choose a reason for hiding this comment

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

Good helper methods

Comment on lines +7 to +18
def validate_model(cls, model_id):
try:
model_id = int(model_id)
except:
abort(make_response({"message":f"the planet {cls.__name__} {model_id} is invalid, please search by planet_id."}, 400))

planet = cls.query.get(model_id)

if not planet:
abort(make_response({"message":f"the planet {cls.__name__} {model_id} doesn't exist."}, 404))
return planet

Choose a reason for hiding this comment

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

Good validation function


@planet_bp.route("", methods=["POST"])
def create_new_planet():
request_body = request.get_json()

Choose a reason for hiding this comment

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

Doing some validation on the request body would be appropriate.

Comment on lines +53 to +57
planet.name = request_body["name"],
planet.color = request_body["color"],
planet.moons = request_body["moons"],
planet.livability = request_body["livability"],
planet.is_dwarf = request_body["is_dwarf"]

Choose a reason for hiding this comment

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

Doing some validation on the request body here would also be appropriate.

@@ -0,0 +1,80 @@
from app.models.planet import Planet

Choose a reason for hiding this comment

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

I love these tests ❤️

assert response.status_code == 200
assert response_body == {'id': 1, 'color': "pink", 'is_dwarf': True, 'livability': 3, 'moons': 99, 'name': "Pretend Planet X"}

def test_create_one_new_planet(client):

Choose a reason for hiding this comment

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

Also testing a create action with an invalid request body would be appropriate.

return make_response(f"Planet #{model_id} successfully updated.")

@planet_bp.route("/<model_id>", methods = ["DELETE"])
def delete_planet(model_id):

Choose a reason for hiding this comment

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

Just noting this is untested.

return make_response(f"Planet {new_planet.name} successfully created", 201)

@planet_bp.route("/<model_id>", methods = ["PUT"])
def update_planet(model_id):

Choose a reason for hiding this comment

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

Just noting this is 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