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

Kayla Ecker - Accelerate #8

Open
wants to merge 7 commits into
base: master
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
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()'
5 changes: 5 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@

def create_app(test_config=None):
app = Flask(__name__)

from .routes import tasks_bp, goals_bp
app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)
Comment on lines +16 to +18

Choose a reason for hiding this comment

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

👍

One thing we started to touch on in the video store live code was that we can split routes into multiple files. We can make a routes folder, and put routes for each endpoint into separate files, named for their model. Then we can use the name bp for the blueprint in each file since it would be the only blueprint in the file. Then these imports might look like:

    from .routes import task, goal
    app.register_blueprint(task.bp)
    app.register_blueprint(goal.bp)


app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

if test_config is None:
Expand Down
4 changes: 3 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,6 @@


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

Choose a reason for hiding this comment

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

Should we be able to create a goal with a NULL title? Consider adding nullable=False here.

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

Choose a reason for hiding this comment

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

👍

There are lots of interesting values that lazy can be set to. True is a synonym for "select", and works great here. But check out some of the other options. 😄

7 changes: 6 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,9 @@


class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True, autoincrement=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)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True)
Comment on lines +7 to +9

Choose a reason for hiding this comment

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

Should title or description be allowed to be NULL? (Does that make sense from a data standpoint?) Consider adding nullable=False here.

The way the project emphasized that completed_at needs to accept NULL values may make it seem like we needed to explicitly call out that nullable should be True, but it turns out this is the default for nullable. Instead, we should think about the other data in our model and consider whether it makes sense for any of it to be NULL. If not, we can have the database help up protect against that happening!


goal_id = db.Column(db.Integer, db.ForeignKey('goal.id'), nullable=True)
260 changes: 259 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,260 @@
from flask import Blueprint
from flask import request, Blueprint, make_response, jsonify
from app.models.task import Task
from app import db
from datetime import datetime
import os
import requests

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

@tasks_bp.route("", strict_slashes=False, methods=["GET", "POST"])
def handle_tasks():

Choose a reason for hiding this comment

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

The logic for GET and POST doesn't share any code, so we could consider putting the logic for each in separate functions, maybe get_tasks and create_task.

if request.method == "GET":
sort_method = request.args.get("sort")
if sort_method == "asc":
tasks = Task.query.order_by(Task.title.asc()).all()
elif sort_method == "desc":
tasks = Task.query.order_by(Task.title.desc()).all()
else:
tasks = Task.query.all()
Comment on lines +13 to +19

Choose a reason for hiding this comment

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

👍

Good job handling the sort param (pushing the sorting off for the DB to handle) and having a reasonable fallback behavior.


tasks_response = []
for task in tasks:
tasks_response.append(
{
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at or False
Comment on lines +25 to +28

Choose a reason for hiding this comment

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

There are many places in your routes where you build a dictionary like this (or very similar). Consider making a helper method, either here in the routes file (e.g. def task_to_dict(task)) or as an instance method of the Task model class (e.g. def to_dict(self)).

})
return jsonify(tasks_response)
elif request.method == "POST":
request_body = request.get_json()
if 'title' not in request_body or 'description' not in request_body or 'completed_at' not in request_body:
return make_response(jsonify({ "details": "Invalid data" }), 400)
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.

👍

We should be doing similar checks when PUTting a task as well. So we could also think about moving checks like this into validation helpers so that they are easier to reuse elsewhere.

We could even think about adding a class method to Task to create a new Task using the dictionary from a request body

    @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 = Task(title=request_body["title"],
description=request_body["description"],
completed_at=request_body["completed_at"],
)

if "completed_at" in request_body:
new_task.completed_at = request_body["completed_at"]
# set_task = True - Todo: Why couldn't I just set it here? Shouldn't need to set task below
Comment on lines +41 to +43

Choose a reason for hiding this comment

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

You already know that completed_at is in request_body from the checks above, so this section seems unnecessary. The following line that calculates set_task_status should be sufficient.

What we'd really like is that to_dict method so that this calculation is baked into any dictionary we generate without needing to replicate the logic everywhere.


set_task_status = True if new_task.completed_at else False

db.session.add(new_task)
db.session.commit()
return make_response(jsonify({"task": { # todo - not really using jsonify?

Choose a reason for hiding this comment

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

The jsonify call isn't required, since flask will jsonify any 'dict' result by default. But calling jsonify may still help communicate intent to others reading the code. If we do it regularly, it also helps us not need to remember the cases where we do need to explicitly call jsonify (where we return something that _isn't) a dict but that we want interpreted as json, such as a bare string, or a collection of records).

"id": new_task.id,
"title": new_task.title,
"description": new_task.description,
"is_complete": set_task_status
}
}), 201)

@tasks_bp.route("/<task_id>", methods=["GET", "PUT", "DELETE"])
def handle_tast(task_id):
task = Task.query.get(task_id)
if not task:
return make_response(f"Invalid data", 404)
Comment on lines +59 to +61

Choose a reason for hiding this comment

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

👍

We do need to do this check for GET, PUT, and DELETE requests, but we could still think about splitting these into separate functions (e.g. get_task, update_task, and delete_task).


if request.method == "GET":

get_one_task_response = {
"task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at or False
}
}
if not task.goal_id:
return get_one_task_response
else:
get_one_task_response["task"]["goal_id"] = task.goal_id
Comment on lines +73 to +76

Choose a reason for hiding this comment

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

👍

This works great, but we could simplify a little bit by checking for the need to add the goal_id and adding it, then returning both from the same place (at the end) rather than having the early return for the case where goal id isn't needed.

        if task.goal_id:
            get_one_task_response["task"]["goal_id"] = task.goal_id

This complication could also get moved into a possible to_dict method so that this gets handled anywhere we need it.

return get_one_task_response

elif request.method == "PUT":
form_data = request.get_json()
task.title = form_data["title"],
task.description = form_data["description"]
task.completed_at = form_data["completed_at"]
Comment on lines +81 to +83

Choose a reason for hiding this comment

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

I mentioned this already above, but we should be sure that the same fields required for POSTing are included here for PUT. PUT replaces the value for the supplied task id, so we should ensure that all of the values required to represent a Task are provided with the same restrictions as we had when creating it in the first place.


db.session.commit()

set_task_status = True if task.completed_at else False

return make_response(jsonify({"task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": set_task_status
}}))

elif request.method == "DELETE":
db.session.delete(task)
db.session.commit()
return jsonify({'details': f'Task {task_id} "{task.title}" successfully deleted'})

@tasks_bp.route("/<task_id>/mark_complete", strict_slashes=False, methods=["PATCH"])
def mark_complete(task_id):
task = Task.query.get(task_id)
if not task:
return make_response(f"Task {task_id} not found", 404)

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

posts_message_to_slack(task)

return make_response(jsonify({"task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": True
}}))

@tasks_bp.route("/<task_id>/mark_incomplete", strict_slashes=False, methods=["PATCH"])
def mark_incomplete(task_id):
task = Task.query.get(task_id)
if not task:
return make_response(f"Task {task_id} not found", 404)

task.completed_at = None
db.session.commit()

return make_response(jsonify({
"task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": False
}
}))


def posts_message_to_slack(task):
slack_path = "https://slack.com/api/chat.postMessage"
slack_token = os.environ.get("SLACK_POST_MESSAGE_API_KEY")
params = {
"channel": "task-notifications",
"text": f"Task completed: {task.title}",
}
headers = {"Authorization": f"Bearer {slack_token}"}
requests.post(slack_path, params = params, headers = headers)
Comment on lines +138 to +146

Choose a reason for hiding this comment

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

👍

Nice helper method!

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=params, headers=headers)








# todo - separate out the route files
from app.models.goal import Goal

Choose a reason for hiding this comment

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

I do see the comment that you would like to split this into a separate file, but since this is all together it would still be more expected to find all the imports at the top of the file.

We had to import in strange places in app/__init__.py, but this was mostly to avoid circular imports. We could have taken another approach to move up some of the dependenncies to allow us to do all our imports at stickler for imports at the top of the file and in a particular order (all system imports before project imports).


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

@goals_bp.route("", strict_slashes=False, methods=["GET", "POST"])
def handle_goals():

Choose a reason for hiding this comment

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

Similar feedback about splitting these functions, and moving validation and dictionary-handling logic around that I made for Task will also apply to Goal. These are common themes for any model with CRUD operations.

if request.method == "GET":
goals = Goal.query.all()
goals_response = []
for goal in goals:
goals_response.append({
"id": goal.id,
"title": goal.title
})
return jsonify(goals_response)
elif request.method == "POST":
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":
{
"id": new_goal.id,
"title": new_goal.title
}
}, 201)

@goals_bp.route("/<goal_id>", strict_slashes=False, methods=["GET", "PUT", "DELETE"])
def handle_goal(goal_id):
goal = Goal.query.get(goal_id)
if not goal:
return make_response(f"Goal {goal_id} not found", 404)

if request.method == "GET":
return {
"goal": {"id": goal.id,
"title": goal.title}
}
elif request.method == "PUT":
form_data = request.get_json()
goal.title = form_data["title"],

db.session.commit()

return make_response({
"goal":{
"id": goal.id,
"title": goal.title
}
}
)
elif request.method == "DELETE":
db.session.delete(goal)
db.session.commit()
return make_response(
{"details":
f"Goal {goal.id} \"{goal.title}\" successfully deleted"
}
)

@goals_bp.route("/<goal_id>/tasks", strict_slashes=False, methods=["GET"])
def get_goal_tasks(goal_id):

Choose a reason for hiding this comment

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

👍

Nice job splitting these two routes into separate functions.

goal = Goal.query.get(goal_id)
if not goal:
return make_response(f"Goal {goal_id} not found", 404)
tasks = goal.tasks

Choose a reason for hiding this comment

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

👍

Nice use of the helper relationship!

tasks_details = []
for task in tasks:
task_dict = {
"id": task.id,
"goal_id": goal.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at or False
}
tasks_details.append(task_dict)
return make_response(
{
"id": goal.id,
"title": goal.title,
"tasks": tasks_details
})

@goals_bp.route("/<goal_id>/tasks", strict_slashes=False, methods=["POST"])
def add_goal_tasks(goal_id):
goal = Goal.query.get(goal_id)
if not goal:
return make_response(f"Goal {goal_id} not found", 404)

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

db.session.add(goal)
db.session.commit()
Comment on lines +250 to +254

Choose a reason for hiding this comment

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

👍

We do need to retrieve each Task to add it to the goal using the SqlAlchemy-created tasks field. But since the goal and each task are already retrieved from the DB, we don't need to add them to the session.

We can also wait to do the commit until after adding all the tasks. This will have the effect of committing this change all together in a single transaction, rather than running the risk of some of the tasks being added, and then possibly running into an error part of the way through (e.g. what if one of the task ids is invalid?).

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?


return make_response(
{
"id": goal.id,
"task_ids": request_body["task_ids"]
})
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.

Choose a reason for hiding this comment

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

Nice to see that your migrations appear to have survived intact over the course of the project. It seems like lots of folx needed to regenerate as an all-in-one migration at some point!

One reminder would be to add a message when generating a migration (-m "message") since this can help remind us which migration does what, and keep track of their relationships.

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