Joanna and Shari - Sea Turtles C17 #6
Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work y'all! I've left a few comments, please reach out if you have any questions on the feedback 🙂
…t_all_planets function.
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work Joanna & Shari! I've left a few comments & questions, feel free to reply here or message me on Slack if you have questions on the feedback 🙂
| has_moons = data_dict["has_moons"] | ||
| ) | ||
| else: | ||
| remaining_keys= set(data_dict.keys())-set(cls.required_attributes.keys()) |
There was a problem hiding this comment.
Really nice use of set operations!
| required_attributes = { | ||
| "name":True, | ||
| "description":True, | ||
| "has_moons":True | ||
| } |
There was a problem hiding this comment.
Since the values for required_attributes aren't used, this could be a list of strings. create_from_dict uses the .keys lists currently for the comparisons, so performance shouldn't be impacted.
| if hasattr(self, key): | ||
| setattr(self, key, data_dict[key]) | ||
| else: | ||
| raise ValueError(key) |
There was a problem hiding this comment.
This will return the first bad key in the update request with the error, but what if there are several? The create_from_dict function below does a great job of showing the user all the keys that are wrong from the request. Its error handling code could be factored out into a helper function and used here as well, providing more consistency in how request data errors are handled.
| for key in data_dict.keys(): | ||
| if hasattr(self, key): | ||
| setattr(self, key, data_dict[key]) |
There was a problem hiding this comment.
Really nice solution for updating the planets!
|
|
||
| planets_bp = Blueprint("Planets", __name__, url_prefix="/planets") | ||
|
|
||
| # Helper Functions: |
| if moon_param: | ||
| planets = Planet.query.filter_by(has_moons=moon_param) | ||
| if description_param: | ||
| planets = Planet.query.filter(Planet.description.like('%'+description_param+'%')).all() |
There was a problem hiding this comment.
Love the wildcard use to match partial descriptions!
| if moon_param: | ||
| planets = Planet.query.filter_by(has_moons=moon_param) |
There was a problem hiding this comment.
Does this work as expected for moon_param? Since this if-block is independent of the if/else below, when the request contains a has_moons query parameter and no description query parameter, it looks like the flow of execution would be that we hit line 63, enter the if and assign planets, then line 65 would be evaluated. Since there's no description query param we would enter the else and the planets variable would be reassigned to Planet.query.all().
| assert response_body == {"details":"Planet Ice Planet successfully added to the Planets Database."} | ||
|
|
||
| def test_update_planet_patches_properly(client, sample_data_with_two_planets): | ||
| pre_patch_response = client.get("/planets/1") |
There was a problem hiding this comment.
We generally want to start with hardcoded data rather than the result of a call to our application. While ideally there wouldn't be, if there were a bug in the GET route it could affect this test's ability to accurately test what we want/hide issues with the route we're actively testing.
| assert response.status_code == 200 | ||
| assert response_body["description"] == "It's wet and full of fish." | ||
| assert response_body["name"] == pre_patch_response_body["name"] | ||
| assert response_body["has_moons"] == pre_patch_response_body["has_moons"] | ||
| assert response_body["id"] == pre_patch_response_body["id"] | ||
| assert response_body["description"] != pre_patch_response_body["description"] |
There was a problem hiding this comment.
Great checking all the relevant data!
3 participants
No description provided.