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

zoisite/Valerie&Tigist #12

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

zoisite/Valerie&Tigist #12

wants to merge 30 commits into from

Conversation

tigistlina
Copy link

No description provided.

app/routes.py Outdated
Comment on lines 44 to 46
results =[]
for planet in planets:
results.append(planet.create_dict())

Choose a reason for hiding this comment

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

This would be a good candidate for list comprehension, something like:

results = [planet.create_dict() for planet in planets]

app/routes.py Outdated
@planets_bp.route("/<planet_id>", methods=["GET"])
def get_planet(planet_id):
planet = validate_planet(planet_id)
return planet.create_dict()

Choose a reason for hiding this comment

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

I think lines 51 and 52 are indented two levels, but only needs to be indented once.

@yangashley
Copy link

@tigistlina @valerie-valentine Great work on waves 1 and 2 for Solar System, team! It looks like you've got a handle on creating a blueprint, registering it with your Flask app and writing routes. Nice job refactoring your code so that the routes can stay short and sweet!

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

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

Nice job on Part 2 of Solar System! Left some comments about how to beef up the tests and adding error handling to your PUT request. Let me know if you have questions!


def create_app(test_config=None):
app = Flask(__name__)
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,35 @@
from app import db

Choose a reason for hiding this comment

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

Nicely done 🪐

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

except KeyError as e:

Choose a reason for hiding this comment

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

Nice job using try/except to add in error handling for a request that relies on (possibly incorrect) client data.

Comment on lines +40 to +44
if name_query:
planets = Planet.query.filter_by(name = name_query)

if nickname_query:
planets = Planet.query.filter_by(nickname = nickname_query)

Choose a reason for hiding this comment

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

👍

Comment on lines +61 to +72

planet_to_update.name = planet_data["name"]
planet_to_update.description = planet_data["description"]
planet_to_update.size= planet_data["size"]
planet_to_update.moon_of_planet = planet_data["moon_of_planet"]
planet_to_update.habitable = planet_data["habitable"]
planet_to_update.gravity = planet_data["gravity"]
planet_to_update.nickname = planet_data["nickname"]

db.session.commit()

return make_response(jsonify(f"Planet { planet_to_update.name} updated"), 200)

Choose a reason for hiding this comment

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

Just like your POST request had error handling, your PUT request also needs it too. If a client sends a PUT req, but the request body had "planet_size" instead of "size", line 64 will throw an error (500 status code), but what we want our route to do is respond with an error message that gives the client feedback about what went wrong.

Comment on lines +19 to +28
assert response_body == {
"description": "A dusty, cold, desert world with a very thin atmosphere",
"gravity": "3.721 m/s²",
"habitable": False,
"id": 1,
"moon_of_planet": "Phobos & Deimos",
"name": "Mars",
"nickname": "Red Planet",
"size": "6,792 km"
}

Choose a reason for hiding this comment

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

Per my comment above in the multiple_planets fixture, if that fixture returned a list of your 3 saved planets then you could check the response_body against your fixture (where you saved the planets when this test first runs)!

saved_planet_1 = multiple_planets[0]
assert response_body["id"] == saved_planet_1.id
assert response_body["description"] == saved_planet_1.description
# continue writing the rest of the assertions for all the attributes in saved_planet_1

Comment on lines +55 to +82
assert response_body == [{
"description": "A dusty, cold, desert world with a very thin atmosphere",
"gravity": "3.721 m/s²",
"habitable": False,
"id": 1,
"moon_of_planet": "Phobos & Deimos",
"name": "Mars",
"nickname": "Red Planet",
"size": "6,792 km"
}, {
"name": "Saturn",
"description": "The second-largest planet in solar system.",
"size": "36,183.7 miles",
"moon_of_planet": "Iapetus, Rhea, Dione, and Tethys",
"habitable": False,
"id": 2,
"gravity": "10.44 m/s²",
"nickname": "Ringed Planet"
}, {
"name": "Neptune",
"description": "Dark, cold, and whipped by supersonic winds, ice giant.",
"size": "24,622 km",
"moon_of_planet": "Triton",
"habitable": False,
"id": 3,
"gravity": "11.15 m/s²",
"nickname": "Ice Giant"
}]

Choose a reason for hiding this comment

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

Similar to my comment above for test_get_one_planet_by_id, if the fixture multiple_planets returned a list of saved planets, you can check the response_body against the saved planets.

# incomplete assertions, but some examples for what you can write so that the test is more robust
    assert response_body[0]["id"] == multiple_planets[0].id
    assert response_body[0]["name"] == multiple_planets[0].name
    assert response_body[0]["description"] == multiple_planets[0].description
    assert response_body[0]["habitable"] == multiple_planets[0].habitable
    assert response_body[1]["id"] == multiple_planets[1].id
    assert response_body[1]["name"] == multiple_planets[1].name
    assert response_body[1]["description"] == multiple_planets[1].description
    assert response_body[1]["habitable"] == multiple_planets[1].habitable

# finish the rest of the assertions for the 3rd planet


def test_creates_one_planet(client):
#Arrange
Expected_planet = {

Choose a reason for hiding this comment

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

constant variable should be named in all caps like EXPECTED_PLANET

Comment on lines +101 to +102
assert response.status_code == 201
assert response_body == "Planet Venus successfully created"

Choose a reason for hiding this comment

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

A way to make this test more robust would to check the response_body against the request body that you create for the post request.

actual_planet = Planet.query.get(1) 
assert actual_planet.name == EXPECTED_PLANET["name"]
assert actual_planet.description == EXPECTED_PLANET["description"]
assert actual_planet.size == EXPECTED_PLANET["size"]

# write assertions for all the attributes on a planet

response_body = response.get_json()
assert response.status_code == 200
assert response_body == "Planet Earth updated"
assert actual_planet.name == replaced_planet["name"]

Choose a reason for hiding this comment

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

To make this test more robust, be sure to write assertions for all the attributes on a planet.

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