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

Sea Turtles - Georgia A & Lili P #9

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,29 @@
from flask import Flask
from flask_sqlalchemy import SQLAlchemy
from flask_migrate import Migrate
from dotenv import load_dotenv
import os

db = SQLAlchemy()
migrate = Migrate()
load_dotenv()

def create_app(test_config=None):

Choose a reason for hiding this comment

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

👍

app = Flask(__name__)

app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False

if not test_config:
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_DATABASE_URI")
else:
app.config["TESTING"] = True
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_TEST_DATABASE_URI")

db.init_app(app)
migrate.init_app(app, db)

from app.routes.routes import planets_bp
app.register_blueprint(planets_bp)
Comment on lines +25 to +26

Choose a reason for hiding this comment

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

If you renamed things according to my later suggestions, these lines could be re-written as:

    from app.routes import planet_routes
    app.register_blueprint(planet_routes.bp)


return app

Empty file added app/models/__init__.py
Empty file.
15 changes: 15 additions & 0 deletions app/models/planet.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
from app import db

class Planet(db.Model):

Choose a reason for hiding this comment

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

👍

id = db.Column(db.Integer, primary_key=True, autoincrement=True)
name = db.Column(db.String, nullable=False)
description = db.Column(db.String)
has_life = db.Column(db.Boolean, nullable=False)

def to_dict(self):
return dict(
id=self.id,
name=self.name,
description=self.description,
has_life=self.has_life
)
2 changes: 0 additions & 2 deletions app/routes.py

This file was deleted.

68 changes: 68 additions & 0 deletions app/routes/routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
from unicodedata import name

Choose a reason for hiding this comment

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

Looks like a stray import from VS Code got in here. Thanks, VS Code!

Choose a reason for hiding this comment

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

Also, since you moved this file into a routes folder, consider naming this file something like planet_routes.py. This is helpful when we have routes for multiple model types, and also makes the import in the app init a little more legible.

Choose a reason for hiding this comment

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

Don't forget to add a __init__.py file in this directory. Without it, python is actually doing a slightly older style of import that can lead to trouble down the road.

from app import db
from app.models.planet import Planet
from flask import Blueprint, jsonify, make_response, request
from .routes_helper import validate_planet, error_message


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

Choose a reason for hiding this comment

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

When a route file has only a single blueprint in it (pretty typical), consider naming the variable simply bp


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

planet = Planet(
name=request_body["name"],
description=request_body["description"],
has_life=request_body["has_life"])
Comment on lines +14 to +17

Choose a reason for hiding this comment

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

This would be great to move into a @classmethod in the Planet model class.


db.session.add(planet)
db.session.commit()

return jsonify(planet.to_dict()), 201


@planets_bp.route("", methods=["GET"])
def get_planets():

Choose a reason for hiding this comment

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

👍


name_param = request.args.get("name")

if name_param:
planets = Planet.query.filter_by(name=name_param)
else:
planets = Planet.query.all()

planets_response = [planet.to_dict() for planet in planets]

return jsonify(planets_response), 200


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

Choose a reason for hiding this comment

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

👍


planet = validate_planet(planet_id)

return jsonify(planet.to_dict()), 200

@planets_bp.route("/<planet_id>", methods=["PUT"])
def replace_planet(planet_id):
planet = validate_planet(planet_id)
request_body = request.get_json()

try:
planet.name = request_body["name"]
planet.description = request_body["description"]
planet.has_life = request_body["has_life"]
Comment on lines +53 to +55

Choose a reason for hiding this comment

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

Consider moving this to an instance method in Planet.

except KeyError as err:
error_message(f"Missing key {err}", 400)

db.session.commit()
return make_response(f"Planet {planet_id} successfully updated!", 200)

Choose a reason for hiding this comment

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

Make sure to jsonify this string, otherwise Flask won't return it as a JSON string. It will be returned as HTML. And here, if we jsonify we don't need the make_response. Also, keep in mind that 200 is the default response code, so we could leave that off.

    return jsonify(f"Planet {planet_id} successfully updated!"), 200


@planets_bp.route("/<planet_id>", methods=["DELETE"])
def delete_planet(planet_id):
planet = validate_planet(planet_id)

db.session.delete(planet)
db.session.commit()
return make_response(f"Planet {planet_id} successfully deleted.")

Choose a reason for hiding this comment

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

Here, too, make sure to jsonify this.

19 changes: 19 additions & 0 deletions app/routes/routes_helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
from flask import jsonify, make_response, abort
from app.models.planet import Planet

def error_message(message, status_code):
abort(make_response(message, status_code))
Comment on lines +4 to +5

Choose a reason for hiding this comment

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

It looks like the places where you're using this to return a message, there are some places where you're passing in a full dictionary, and others where you're passing in a stand-alone string. From the naming of the parameter message, I would assume it should receive a string. Python doesn't have aa problem with us passing in different things at different times, but we should be careful to note the effect of the different types.

If we pass in a dictionary, Flask will assume we are returning JSON data, and automatically set the proper headers. But if we pass in a string, 1) the structure of the response body won't match the cases where a dictionary is being used, which can be confusing for the client, and 2) Flask assumes a string response is HTML, not JSON, so the response headers will be set to tell the client the result is HTML, which can cause some HTTP client libraries to assume there is no response data (like python's requests library).

We could address point 2 by always calling jsonify here.

def error_message(message, status_code):
    abort(make_response(jsonify(message), status_code))

And we could address point 1 by having the function accept a string, and building the dictionary wrapper internally.

def error_message(message, status_code):
    abort(make_response(jsonify({"message": message}), status_code))


def validate_planet(id):

Choose a reason for hiding this comment

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

👍
I'm of two minds as to where this function should live. On the one hand, the only routes you have right now are the planet routes, so having this here makes sense. On the other hand, I sort of expect a file called routes_helper.py to have general purpose helpers that aren't related to any particular model. So from that perspective, I'd kind of like to see a planet_route_helpers.py that has stuff that's for routes and planet models. On the other (third?) hand, we might need to look up a planet from other routes if we were establishing relationships between multiple model types, so it could be fine here!

Deciding where to put stuff is an extension of deciding how to name things, which is really hard!

try:
id = int(id)
except:
error_message({"message":f"planet {id} invalid"}, 400)

planet = Planet.query.get(id)

if not planet:
error_message({"message":f"planet {id} not found"}, 404)
else:
return planet

1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Generic single-database configuration.
45 changes: 45 additions & 0 deletions migrations/alembic.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# A generic, single database configuration.

[alembic]
# template used to generate migration files
# file_template = %%(rev)s_%%(slug)s

# set to 'true' to run the environment during
# the 'revision' command, regardless of autogenerate
# revision_environment = false


# Logging configuration
[loggers]
keys = root,sqlalchemy,alembic

[handlers]
keys = console

[formatters]
keys = generic

[logger_root]
level = WARN
handlers = console
qualname =

[logger_sqlalchemy]
level = WARN
handlers =
qualname = sqlalchemy.engine

[logger_alembic]
level = INFO
handlers =
qualname = alembic

[handler_console]
class = StreamHandler
args = (sys.stderr,)
level = NOTSET
formatter = generic

[formatter_generic]
format = %(levelname)-5.5s [%(name)s] %(message)s
datefmt = %H:%M:%S
96 changes: 96 additions & 0 deletions migrations/env.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
from __future__ import with_statement

import logging
from logging.config import fileConfig

from sqlalchemy import engine_from_config
from sqlalchemy import pool
from flask import current_app

from alembic import context

# this is the Alembic Config object, which provides
# access to the values within the .ini file in use.
config = context.config

# Interpret the config file for Python logging.
# This line sets up loggers basically.
fileConfig(config.config_file_name)
logger = logging.getLogger('alembic.env')

# add your model's MetaData object here
# for 'autogenerate' support
# from myapp import mymodel
# target_metadata = mymodel.Base.metadata
config.set_main_option(
'sqlalchemy.url',
str(current_app.extensions['migrate'].db.engine.url).replace('%', '%%'))
target_metadata = current_app.extensions['migrate'].db.metadata

# other values from the config, defined by the needs of env.py,
# can be acquired:
# my_important_option = config.get_main_option("my_important_option")
# ... etc.


def run_migrations_offline():
"""Run migrations in 'offline' mode.

This configures the context with just a URL
and not an Engine, though an Engine is acceptable
here as well. By skipping the Engine creation
we don't even need a DBAPI to be available.

Calls to context.execute() here emit the given string to the
script output.

"""
url = config.get_main_option("sqlalchemy.url")
context.configure(
url=url, target_metadata=target_metadata, literal_binds=True
)

with context.begin_transaction():
context.run_migrations()


def run_migrations_online():
"""Run migrations in 'online' mode.

In this scenario we need to create an Engine
and associate a connection with the context.

"""

# this callback is used to prevent an auto-migration from being generated
# when there are no changes to the schema
# reference: http://alembic.zzzcomputing.com/en/latest/cookbook.html
def process_revision_directives(context, revision, directives):
if getattr(config.cmd_opts, 'autogenerate', False):
script = directives[0]
if script.upgrade_ops.is_empty():
directives[:] = []
logger.info('No changes in schema detected.')

connectable = engine_from_config(
config.get_section(config.config_ini_section),
prefix='sqlalchemy.',
poolclass=pool.NullPool,
)

with connectable.connect() as connection:
context.configure(
connection=connection,
target_metadata=target_metadata,
process_revision_directives=process_revision_directives,
**current_app.extensions['migrate'].configure_args
)

with context.begin_transaction():
context.run_migrations()


if context.is_offline_mode():
run_migrations_offline()
else:
run_migrations_online()
24 changes: 24 additions & 0 deletions migrations/script.py.mako
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
"""${message}

Revision ID: ${up_revision}
Revises: ${down_revision | comma,n}
Create Date: ${create_date}

"""
from alembic import op
import sqlalchemy as sa
${imports if imports else ""}

# revision identifiers, used by Alembic.
revision = ${repr(up_revision)}
down_revision = ${repr(down_revision)}
branch_labels = ${repr(branch_labels)}
depends_on = ${repr(depends_on)}


def upgrade():
${upgrades if upgrades else "pass"}


def downgrade():
${downgrades if downgrades else "pass"}
34 changes: 34 additions & 0 deletions migrations/versions/e29e7cf3fb9b_adds_planet_model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
"""adds Planet model

Choose a reason for hiding this comment

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

👍 Nice migration message


Revision ID: e29e7cf3fb9b
Revises:
Create Date: 2022-04-29 14:14:00.240448

"""
from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision = 'e29e7cf3fb9b'
down_revision = None
branch_labels = None
depends_on = None


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.create_table('planet',
sa.Column('id', sa.Integer(), autoincrement=True, nullable=False),
sa.Column('name', sa.String(), nullable=False),
sa.Column('description', sa.String(), nullable=True),
sa.Column('has_life', sa.Boolean(), nullable=False),
sa.PrimaryKeyConstraint('id')
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_table('planet')
# ### end Alembic commands ###
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Mako==1.1.4
MarkupSafe==1.1.1
psycopg2-binary==2.8.6
pycodestyle==2.6.0
pytest==6.2.3
pytest==6.2.5
pytest-cov==2.12.1
python-dateutil==2.8.1
python-dotenv==0.15.0
Expand Down
Empty file added tests/__init__.py
Empty file.
34 changes: 34 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
from flask.signals import request_finished
from app import db
import pytest
from app.models.planet import Planet

from app import create_app

@pytest.fixture
def app():
app = create_app({"TESTING": True})

@request_finished.connect_via(app)
def expire_session(sender,response, **extra):
db.session.remove()

with app.app_context():
db.create_all()
yield app

with app.app_context():
db.drop_all()

@pytest.fixture
def client(app):
return app.test_client()

@pytest.fixture
def two_planets(app):
earth = Planet(name="earth", description="green and blue", has_life=True)
mercury = Planet(name="mercury", description="extremely hot", has_life=False)

db.session.add(earth)
db.session.add(mercury)
db.session.commit()
Comment on lines +27 to +34

Choose a reason for hiding this comment

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

👍
Because our SQL table is set to assign ids, it's fine to leave off the ids for the records as you did here.

The migration logic sets up the id behavior a little differently from how we did it when we worked with SQL manually, and would also allow us to set the ids explicitly. One nice thing about setting the id explicitly in a fixture is that we know what id to use in subsequent tests. If we don't, then we should still be able to assume that the ids will start from 1 and increment, but we probably shouldn't depend on it. So if we leave off the ids, then we'd probably want to return the two records in a list so that the test using these records could get their ids safely, regardless of what they were set to. This would look like:

@pytest.fixture
def two_planets(app):
    earth = Planet(name="earth", description="green and blue", has_life=True)
    mercury = Planet(name="mercury", description="extremely hot", has_life=False)

    db.session.add(earth)
    db.session.add(mercury)
    db.session.commit()

    return [earth, mercury]

And then in a test using the two_planets fixture, we could get the id of the first planet as two_planets[0].id

Loading