Skip to content

Accelerate - Janice Lichtman #7

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
1 change: 1 addition & 0 deletions Procfile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
web: gunicorn 'app:create_app()'
8 changes: 6 additions & 2 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
migrate = Migrate()
load_dotenv()


def create_app(test_config=None):
app = Flask(__name__)
app.url_map.strict_slashes = False

Choose a reason for hiding this comment

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

👍

Nice way to set this across all your routes!

app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

if test_config is None:
Expand All @@ -30,5 +30,9 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here

import app.routes.task_routes as task_routes
import app.routes.goal_routes as goal_routes
Comment on lines +33 to +34

Choose a reason for hiding this comment

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

The as should only be required if we need to rename the module to something else (like to create an alias). We should be able to do either

    import app.routes.task_routes
    import app.routes.goal_routes

or

    from app.routes import task_routes
    from app.routes import goal_routes

app.register_blueprint(task_routes.bp)
app.register_blueprint(goal_routes.bp)

return app
10 changes: 9 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,12 @@


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True)

Choose a reason for hiding this comment

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

👍

I fully support this renaming!

title = db.Column(db.String)
tasks = db.relationship('Task', backref='goal', lazy=True)

def to_json(self):

Choose a reason for hiding this comment

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

Great decision to bring this helper into the model!

return {
"id": self.id,
"title": self.title
}
20 changes: 19 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,22 @@


class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True)

Choose a reason for hiding this comment

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

Technically, SqlAlchemy creates columns as nullable by default, so explicitly listing nullable=True isn't necessary. I think the way the project calls this out was a bit of a red herring.

As a corollary, we might want to set nullable=False for columns like the title, and maybe description. Does it make sense to have a task with a NULL title?

goal_id = db.Column(db.Integer, db.ForeignKey('goal.id'), nullable=True)

def is_complete(self):

Choose a reason for hiding this comment

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

Great little helper!

return True if self.completed_at else False

def to_json(self):
response = {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": self.is_complete()
}
if self.goal_id:
response["goal_id"] = self.goal_id
Comment on lines +22 to +23

Choose a reason for hiding this comment

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

Nice selective inclusion of the goal id!

The one warning I would make about this approach is that is there are a lot of necessary variations for various endpoints, it can make the "rendering" method a real bear. In those cases, I tend to prefer including data transfer objects (DTOs), classes whose sole responsibility is knowing how to serialize another type, so that those sorts of methods don't end up cluttering the model type, which really shouldn't need to know about all the situations in which it might need to be rendered.

return response
2 changes: 0 additions & 2 deletions app/routes.py

This file was deleted.

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

Choose a reason for hiding this comment

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

👍

This all looks good, but consider how the feedback I provided on Task could apply here as well.

from app.models.goal import Goal
from app.models.task import Task
from .route_helpers import validate_item
from flask import Blueprint, request, make_response, jsonify

bp = Blueprint("goals", __name__, url_prefix="/goals")

@bp.route("", methods=["GET"])
def get_goals():
goals = Goal.query.all()
return jsonify([goal.to_json() for goal in goals])

@bp.route("", methods=["POST"])
def create_goal():
request_body = request.get_json()
if "title" not in request_body:
return make_response({"details": "Invalid data"}, 400)
new_goal = Goal(title=request_body["title"])

db.session.add(new_goal)
db.session.commit()

return make_response({"goal": new_goal.to_json()}, 201)

@bp.route("/<goal_id>", methods=["GET"])
def get_goal(goal_id):
goal_response = validate_item("goal", goal_id)
if type(goal_response) != Goal:
return goal_response
return {"goal": goal_response.to_json()}

@bp.route("/<goal_id>", methods=["PUT"])
def update_goal(goal_id):
goal_response = validate_item("goal", goal_id)
if type(goal_response) != Goal:
return goal_response

form_data = request.get_json()
goal_response.title = form_data["title"],
db.session.commit()
return {"goal": goal_response.to_json()}


@bp.route("/<goal_id>", methods=["DELETE"])
def delete_goal(goal_id):
goal_response = validate_item("goal", goal_id)
if type(goal_response) != Goal:
return goal_response

db.session.delete(goal_response)
db.session.commit()
return make_response(
{"details":
f"Goal {goal_response.id} \"{goal_response.title}\" successfully deleted"
}
)

@bp.route("/<goal_id>/tasks", methods=["GET"])
def get_goal_tasks(goal_id):
goal_response = validate_item("goal", goal_id)
if type(goal_response) != Goal:
return goal_response

tasks = goal_response.tasks
tasks_details = [task.to_json() for task in tasks]
return make_response(
{
"id": goal_response.id,
"title": goal_response.title,
"tasks": tasks_details
})

@bp.route("/<goal_id>/tasks", methods=["POST"])
def add_goal_tasks(goal_id):
goal_response = validate_item("goal", goal_id)
if type(goal_response) != Goal:
return goal_response

request_body = request.get_json()
for id in request_body["task_ids"]:
task = Task.query.get(id)
goal_response.tasks.append(task)

db.session.add(goal_response)
Comment on lines +82 to +85

Choose a reason for hiding this comment

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

Since goal_response came from the DB already, we don't need to add it into the session. We should also be sure to validate the lookup of the individual tasks. What if one of them couldn't be found?

Also, what would happen if the goal previously had some tasks set. Do we want to add the new tasks to the existing tasks? Do we want to replace them and sever any prior task → goal relationships? What behavior is implemented here?

db.session.commit()

return make_response(
{"id": goal_response.id,
"task_ids": request_body["task_ids"]
})
36 changes: 36 additions & 0 deletions app/routes/route_helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from app.models.task import Task
from app.models.goal import Goal
from flask import make_response
from dateutil.parser import parse, ParserError
import os
import requests

def validate_item(type, id):
if not id.isdigit():
return make_response(f"{id} is not a valid {type}_id. {type.title()} ID must be an integer.", 400)

Choose a reason for hiding this comment

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

It's great to handle these checks in a helper like this, but you might have found it annoying to have to check for the type of the result in the endpoint functions. We'd really like this function work similarly to the get_or_404 helper, where it either returns the desired model, or terminates immediately. To do this, we can make use of flask's (or in this case, werkzeug, which underpins flask) exceptions.

from werkzeug.exceptions import BadRequest, NotFound

Then we can use these like

    type_name = type.__name__
    raise BadRequest(f"{id} is not a valid {type_name.lower()}_id. {type_name} ID must be an integer.")

As you noticed, the default way that flask handles a 404 is to generate an HTML error, but we can override this to send back JSON for a custom error. We can put the following in create_app (we need the app reference, but this could be moved without too much work):

    @app.errorhandler(NotFound)
    @app.errorhandler(BadRequest)
    def handle_invalid_usage(error):
        return jsonify(error.description), error.code

This will jsonify (setting the expected content type) whatever we passed to the exception, and set the status code appropriately.

Choose a reason for hiding this comment

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

Another way we can approach error handling is to write our own custom decorators that try to resolve the input params to a record before hitting the endpoint. In that case, we can use the regular make_response for returning errors before even getting into the main endpoint logic. The decorator syntax can get a little weird, but see what you can find.


if type == "task":
# item = Task.query.get_or_404(id) # This will give the standard 404 page instead of custom message
item = Task.query.get(id)
elif type == "goal":
item = Goal.query.get(id)
Comment on lines +12 to +16

Choose a reason for hiding this comment

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

This kind of check is exactly the kind of thing that OOP (or even small helper functions) can help us avoid. For instance, instead of passing in a string, what if we passed the class itself?

validate_item(Task, some_id)
#or
validate_item(Goal, some_other_id)

If type then contains the class, we can get its display name for messages like

name = type.__name__

Then instead of having the if condition, we can just call

type.query.get(id)


if not item:
return make_response(f"{type.title()} {id} not found", 404)
return item

def validate_datetime(date_text):
try:
return parse(date_text)
except ParserError:
return make_response(f"Invalid date format in \"completed_at\". Please resubmit with a valid date_time.", 400)

def post_to_slack(text):
slack_token = os.environ.get("SLACK_POST_MESSAGE_API_TOKEN")
slack_path = "https://slack.com/api/chat.postMessage"
query_params ={
"channel": "task-notifications",
"text": text,
}
headers = {"Authorization": f"Bearer {slack_token}"}
requests.post(slack_path, params = query_params, headers = headers)

Choose a reason for hiding this comment

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

Since we're sending a post request, we would typically send the parameters as form data, rather than as query params.

Query params do have a maximum length (as part of the HTTP standard), so when we have potentially large data (like a text message), we often send that data in the form-encoded body of a POST request (this stems from older web standards. Now, we might use JSON in the request body).

With the requests library, we can set the form-encoded body by using the data named parameter rather than params (which sets the query params).

    requests.post(slack_path, data=query_params, headers=headers)

116 changes: 116 additions & 0 deletions app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
from app import db
from app.models.task import Task
from .route_helpers import *

Choose a reason for hiding this comment

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

We should try to avoid import * since this could end up bringing in an unexpected function/variable that collides with something in our own code without us knowing it. With import *, any time we make a change to route_helpers, there's a chance we could unintentionally break code elsewhere in our project.

from flask import Blueprint, request, make_response, jsonify
import datetime

bp = Blueprint("tasks", __name__, url_prefix="/tasks")

@bp.route("", methods=["GET"])
def get_tasks():
sort_query = request.args.get("sort")
search_title_query = request.args.get("search_title")
if sort_query:
if sort_query == "asc":
tasks = Task.query.order_by(Task.title).all()

Choose a reason for hiding this comment

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

Rather than calling SqlAlchemy stuff directly from the routes, one thing we can consider is to make class helper methods to represent the operations we expect to perform. This forms a more self-documenting interface about what operations we expect to perform. This also follows the Law of Demeter.

We could make helper equivalents of things like get, order_by, filter_by, and all. Really, any of the query methods.

elif sort_query == "desc":
tasks = Task.query.order_by(Task.title.desc()).all()
else:
return make_response(f"{sort_query} is not a valid sort parameter. Please use sort=asc or sort=desc.", 400)

Choose a reason for hiding this comment

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

Wow, harsh! 😆

elif search_title_query:
tasks = Task.query.filter_by(title=search_title_query).all()
else:
tasks = Task.query.all()

return jsonify([task.to_json() for task in tasks])

@bp.route("", methods=["POST"])
def create_task():
request_body = request.get_json()
for attribute in {"title", "description", "completed_at"}:
if attribute not in request_body:
return make_response({"details": "Invalid data"}, 400)
Comment on lines +30 to +32

Choose a reason for hiding this comment

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

Nice way to check for a set of required fields. This could be pulled out into a validation method as you did for the the completed_at date string, and a similar approach to my comment about error handling in the route_helpers could turn this into a fire and forget method you could use here and in your PUT handler.


if request_body["completed_at"]:
date_time_response = validate_datetime(request_body["completed_at"])
if type(date_time_response) != datetime.datetime:
return date_time_response

new_task = Task(title=request_body["title"],
description=request_body["description"])
Comment on lines +39 to +40

Choose a reason for hiding this comment

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

We could even think about adding a class method to Task to create a new Task using the dictionary from a request body. A bunch of the validation code code move there as well!

    @classmethod
    def from_dict(values):
        # create a new task, and set the model values from the values passed in
        # be sure to validate that all required values are present, we could return `None` or raise an error if needed
        return new_task


new_task.completed_at = date_time_response.strftime("%m/%d/%Y, %H:%M:%S") if request_body["completed_at"] else None

Choose a reason for hiding this comment

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

Why not assign the parsed datetime value directly?


db.session.add(new_task)
db.session.commit()

return make_response({"task": new_task.to_json()}, 201)

@bp.route("/<task_id>", methods=["GET"])
def get_task(task_id):
task_response = validate_item("task", task_id)
if type(task_response) != Task:
return task_response

return {"task": task_response.to_json()}
Comment on lines +51 to +55

Choose a reason for hiding this comment

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

With some of the suggestions to validate_item, this code become

    task = validate_item(Task, task_id)
    return {"task": task.to_json()}


@bp.route("/<task_id>", methods=["PUT"])
def update_task(task_id):
task_response = validate_item("task", task_id)
if type(task_response) != Task:
return task_response

form_data = request.get_json()
for attribute in {"title", "description"}:
if attribute not in form_data:
return make_response({"details": "Invalid data"}, 400)

if form_data["completed_at"]:
date_time_response = validate_datetime(form_data["completed_at"])
if type(date_time_response) != datetime.datetime:
return date_time_response

task_response.title = form_data["title"],
task_response.description = form_data["description"]
task_response.completed_at = date_time_response.strftime("%m/%d/%Y, %H:%M:%S") if form_data["completed_at"] else None

db.session.commit()

return {"task": task_response.to_json()}

@bp.route("/<task_id>", methods=["DELETE"])
def delete_task(task_id):
task_response = validate_item("task", task_id)
if type(task_response) != Task:
return task_response

db.session.delete(task_response)
db.session.commit()
return make_response(
{"details":
f"Task {task_response.id} \"{task_response.title}\" successfully deleted"
}
)

@bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def mark_complete(task_id):
task_response = validate_item("task", task_id)
if type(task_response) != Task:
return task_response

task_response.completed_at = datetime.datetime.utcnow()
db.session.commit()

slack_text = f"Someone just completed the task {task_response.title}"
post_to_slack(slack_text)
Comment on lines +104 to +105

Choose a reason for hiding this comment

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

Nice way to build the status message without making post_to_slack need to know anything about the task!

return {"task": task_response.to_json()}

@bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def mark_incomplete(task_id):
task_response = validate_item("task", task_id)
if type(task_response) != Task:
return task_response

task_response.completed_at = None
db.session.commit()
return {"task": task_response.to_json()}
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
Loading