Skip to content

Accelerate C15 - Trish G. #6

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 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
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()'
4 changes: 4 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ def create_app(test_config=None):
db.init_app(app)
migrate.init_app(app, db)

from .routes import tasks_bp
from .routes import goals_bp
# Register Blueprints here
app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)
Comment on lines +32 to +36

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)


return app
2 changes: 2 additions & 0 deletions app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@

class Goal(db.Model):
goal_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. 😄

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


class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
task_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
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.goal_id"), nullable=True)
291 changes: 290 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,291 @@
from flask import Blueprint
from flask.json import jsonify
from app import db
from app.models.task import Task
from app.models.goal import Goal
from flask import request, Blueprint, make_response, jsonify
from datetime import date, datetime

Choose a reason for hiding this comment

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

It doesn't look like date is used anywhere, so we can remove that.

Suggested change
from datetime import date, datetime
from datetime import datetime

from dotenv import load_dotenv
import requests
import os

load_dotenv()
tasks_bp = Blueprint("tasks", __name__, url_prefix="/tasks")
goals_bp = Blueprint('goals', __name__, url_prefix="/goals")
Comment on lines +12 to +13

Choose a reason for hiding this comment

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

We could consider putting all the routes related to the tasks endpoint in their own file, and doing the same for the goals routes.


@tasks_bp.route("", 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 == "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 {
"details": f"Invalid data"
}, 400
Comment on lines +19 to +22

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"]
)
db.session.add(new_task)
db.session.commit()
if new_task.completed_at == None:
completed_at = False
else:
completed_at = True
return make_response({
"task": {
"id": new_task.task_id,
"title": new_task.title,
"description": new_task.description,
"is_complete": completed_at
Comment on lines +36 to +39

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)).

}
}, 201)

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

Choose a reason for hiding this comment

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

👍

Nice handling of the sort param (pushing that off to the database to do for us!) and falling back to a reasonable default if it's not a recognize value.


tasks_response = []
for task in tasks:
if task.completed_at == None:
completed_at = False
else:
completed_at = True
tasks_response.append({
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": completed_at
})
return make_response(jsonify(tasks_response), 200)

@tasks_bp.route("/<task_id>", methods=["GET","PUT","DELETE"])
def handle_task(task_id):
task = Task.query.get(task_id)
if task is None:
return make_response("", 404)
Comment on lines +68 to +70

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":
if task.completed_at == None:
completed_at = False
else:
completed_at = True

if task.goal_id == None:
return make_response({
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": completed_at
}
})
else:
# goal_id = task.goal_id
return make_response({
"task": {
"id": task.task_id,
"title": task.title,
"goal_id": task.goal_id,
"description": task.description,
"is_complete": completed_at
}
})
Comment on lines +78 to +97

Choose a reason for hiding this comment

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

Rather than duplicating the entire dict creation, we could create a dict without goal_id, and only add goal_id' to the dict` if it's defined for the task. This could get handled in a helper function as well, since this logic can apply to other task output code.


elif request.method == "PUT":
form_data = request.get_json()
if ("completed_at" not in form_data or form_data["completed_at"] == None):
is_complete = False
else:
is_complete = True
task.title = form_data["title"]
task.description = form_data["description"]
task.completed_at = form_data["completed_at"]
Comment on lines +105 to +107

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()
response_task = {
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": is_complete
}
}
return make_response(response_task, 200)

elif request.method == "DELETE":
db.session.delete(task)
db.session.commit()
if task.completed_at == None:
completed_at = False
else:
completed_at = True
Comment on lines +122 to +125

Choose a reason for hiding this comment

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

We don't need to calculate this for DELETE

return make_response(
{
"details":
f"Task {task.task_id} \"{task.title}\" successfully deleted"
}
)

@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def mark_complete(task_id):
task = Task.query.get(task_id)

if task == None:
return make_response(f"Task {task_id} not found", 404)

task.completed_at = datetime.utcnow()
db.session.commit()
slack_token = os.environ.get("SLACK_BOT_TOKEN")
slack_path = "https://slack.com/api/chat.postMessage"
query_params = {
"channel": "task-notifications",
"text": f"Someone just completed the {task.title}",
}

headers = {"Authorization": f"Bearer {slack_token}"}
requests.post(slack_path, params = query_params, headers = headers)
Comment on lines +142 to +150

Choose a reason for hiding this comment

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

We could move this into a helper function so that the main purpose of this function (marking a task complete) is clearer.

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)


return {
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": True
}
}, 200

@tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def mark_incomplete(task_id):
task = Task.query.get(task_id)

if task == None:
return make_response("Task {taske_id} not found", 404)
task.completed_at = None

db.session.commit()

return {
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": False
}
}, 200

@goals_bp.route("", methods=["POST", "GET"])
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 == "POST":
request_body = request.get_json()
if "title" not in request_body:
return {
"details": f"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.goal_id,
"title": new_goal.title

}
}, 201)
if request.method == "GET":
goals = Goal.query.all()
goals_response = []
for goal in goals:
goals_response.append({
"id": goal.goal_id,
"title": goal.title
})
return make_response(jsonify(goals_response))

@goals_bp.route("/<goal_id>", methods=["GET", "PUT", "DELETE"])
def handle_goal(goal_id):
goal = Goal.query.get(goal_id)
if goal is None:
return make_response(f"", 404)

if request.method == "GET":
return make_response({
"goal": {
"id": goal.goal_id,
"title": goal.title
}
})
elif request.method == "PUT":
form_data = request.get_json()
goal.title = form_data["title"]
db.session.commit()
goal_response = {
"goal": {
"id": goal.goal_id,
"title": goal.title
}
}
return make_response(goal_response, 200)

elif request.method == "DELETE":
db.session.delete(goal)
db.session.commit()

return make_response(
{
"details": f"Goal {goal.goal_id} \"{goal.title}\" successfully deleted"
}
)

@goals_bp.route("/<goal_id>/tasks", methods=["GET", "POST"])
def handle_goals_and_tasks(goal_id):
if request.method == "POST":
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 +254 to +257

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 should also be sure to validate that the Task retrieval succeeded.

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 (again, 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.goal_id,
"task_ids": request_body["task_ids"]
})

elif request.method == "GET":
goal = Goal.query.get(goal_id)

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

list_of_tasks = []

for task in tasks:
if task.completed_at == None:
completed_at = False
else:
completed_at = True

individual_task = {
"id": task.task_id,
"goal_id": goal.goal_id,
"title": task.title,
"description": task.description,
"is_complete": completed_at
}
list_of_tasks.append(individual_task)
return make_response({
"id": goal.goal_id,
"title": goal.title,
"tasks": list_of_tasks
})
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