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

Ihovanna + Maria Fernanda #41

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

Ihovanna + Maria Fernanda #41

wants to merge 28 commits into from

Conversation

Ihovanna
Copy link

No description provided.

Mfpr603 and others added 28 commits October 24, 2022 11:51
…pdate your own database with upgrade commands
…ed. HTTP GET request returns all entries in database.
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.

Overall nice work. I left some suggestions for future APIs. Also try to remember to include venv in your .gitignore file so it doesn't get included in the Pull Request.

id = db.Column(db.Integer, primary_key=True, autoincrement=True)
name = db.Column(db.String)
description = db.Column(db.String)

Choose a reason for hiding this comment

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

some helper methods like

    def to_dict(self):

        return {
            "name": self.name,
            "color": self.color,
            "description": self.description,
            "id": self.id
        }
    @classmethod   
    def from_json(cls, input_dict):
        return cls(
            name= input_dict["name"],
            color= input_dict["color"],
            description= input_dict["description"]
        )

Might be helpful

planets_bp = Blueprint('planets_bp', __name__, url_prefix='/planets')

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

Choose a reason for hiding this comment

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

Some input validation on the request body would be appropriate here.

Comment on lines +27 to +31
planets_response.append({
"id": planet.id,
"name": planet.name,
"description": planet.description
})

Choose a reason for hiding this comment

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

This would be a great place to use that helper method I listed in the Planet Class.



@planets_bp.route("/<planet_id>", methods=["GET"])
def handle_planet(planet_id):

Choose a reason for hiding this comment

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

Why name this handle_planet instead of get_planet?

assert len(response_body) == 1


def test_create_planet(client):

Choose a reason for hiding this comment

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

You should also test the create action with an invalid input.

assert response_body == "Planet Earth has been added to the Planets database."


def test_update_planet_entry(client, one_planet_in_database):

Choose a reason for hiding this comment

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

You should also have a test for the update action with invalid input

assert response_body == "Planet Earth has been updated in the Planets database."


def test_delete_planet_entry(client, one_planet_in_database):

Choose a reason for hiding this comment

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

You should also try to test delete actions with invalid planet ids. Basically testing the 404 response.

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