Skip to content

Conversation

@n2020h
Copy link

@n2020h n2020h commented Nov 4, 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 Neema & Maria, you hit the basics here pretty well. I left some minor comments/suggestions. Ping me on slack if you have questions.


return planet_as_dict

def from_json(cls, req_body):

Choose a reason for hiding this comment

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

This should probably be a class method

Comment on lines +9 to +20
def validate_model(cls, model_id):
try:
model_id = int(model_id)
except:
abort(make_response({"message":f"{cls.__name__} {model_id} invalid"}, 400))

model = cls.query.get(model_id)

if not model:
abort(make_response({"message":f"{cls.__name__} {model_id} not found"}, 404))

return model

Choose a reason for hiding this comment

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

Great helper function



@planets_bp.route("", methods=["POST"])
def handle_planets():

Choose a reason for hiding this comment

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

Just curious, why name the function handle_planets instead of create_planet?

Comment on lines +27 to +28
@pytest.fixture
def two_saved_planets(app):

Choose a reason for hiding this comment

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

Why not include a fixture for one planet as well?

Comment on lines +69 to +70
assert response.status_code == 201
assert response_body == "Planet New Planet successfully created"

Choose a reason for hiding this comment

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

You should also assert that the number of planets in the DB has increased.

@@ -0,0 +1,70 @@
def test_get_all_planets_with_no_records(client):

Choose a reason for hiding this comment

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

Just noting that there are no tests for the delete or update actions.

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