Somy Cho and Elzara Turakulova

[ielzara]

Wave 1 and Wave 2

[apradoada]

Just to make sure that you have all the feedback in one place, I went ahead and took a look at your refactoring branch and will put the feedback here because I can’t add comments on the repo itself!

init.py

This file looks great! It follows all the naming conventions we are looking for. I especially appreciate that y’all took the time to go ahead an alias the blueprints for ease. Your configuration looks good too!

Models

The Moon and the Planet model look really good as well. These two show me that y’all have a solid grasp of how models work. I appreciate that y’all took a stab at creating the Moon model as well. The One to Many relationship y’all built between the Moon and Planet models is done well. The nerd in me likes the fact that a moon doesn’t have to have a planet as moons can exist around asteroids or other non-star objects as well!
Routes

Route Utilities

Great job with the validate_model function. This is exactly what we were looking for. Beyond that, The outside research that y’all did to put together the create_model and get_models_with_filters functions is very impressive! Using the hasattr builtin is great when combined with these models. The way you’ve structured the query as well is done really well! This is a great example of what a route utilities file should look like!

Planet Routes

Overall these routes look great! You’ve used the route utilities well and I love that in some cases, you are able to replace an entire route with just a call to one of the utility functions. This is helpful when you repeat the same or similar steps for multiple models!

One thing I noticed is in the update model endpoint! This was not a requirement, so you do not need to go back and make any changes, but the way this function currently works is that it requires a request body to have all of the model’s attributes. I wonder if there is a way to make it so that we could make an update to a planet with only a few of the attributes?

One last thing is in your get_moons_by_planet route. Y’all copied over some of the code from HelloBooks, which is totally fine, just make sure you remove all references to the original! On line 59, I see a couple references to “Book”.

Moon Routes

These were not required so my comments will be brief although they are mostly the same as the Planet Routes. I did notice you had some error handling in the update_moon here that you didn’t for update_planet. In a more robust project, just make sure there’s some consistency there! Same comment applies as well when it comes to allowing an update without all the attributes! Other than that, it looks really good!

Testing

My first thought is kudos to your test organization! Y’all have done a great job of separating out the tests for each part of the project. I especially love that y’all have tested the individual models and the route utilities as well. This is a very smart move in terms of integration testing where we need to make sure each individual function works before testing the whole!

Conftest

You’ve followed the conventions we’ve taught really well. Great job adding in an additional fixture for the moons!

Test Planet Model

Great job here. Y’all have been very thorough when it comes to testing each of the different attributes individually. The one downside to this is that you do repeat a lot of repeated code. I wonder if it would be possible to test everything at once to make sure it all comes back as None if the information is missing. I would say that testing the from_dict is different though and testing each key individually is ok!

Test Planet Routes

One thing to think about is an extension question not addressed in the Learn lesson! For the create Planet tests, we check for a response, but there is no check to see if the planet makes its way to the database. What might that look like? Hint: The tests will require a second request.

I see what you were doing with the last two delete planet tests. However, the only difference between them is the name. Other than that they are the exact same test. I would go ahead and remove one to avoid that redundancy! If you do want to keep it, just make sure the id for one of them isn’t an integer!

Test Moon Model

Same feedback as testing the Planet model! I would maybe replace one of the to_dict tests with a from_dict test!

Test Moon Routes

Similar feedback as testing the Planet routes! It wasn’t required, but the one test I do see missing is creating a moon via the planet!

Test Route Utilities
These look good!

Overall, this is a great Solar System Project! It is one of the cleanest and most robust that I have seen! Well done, y’all!