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

Accelerate - Karolina #11

Open
wants to merge 9 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()'
4 changes: 4 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,9 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here
from .routes import tasks_bp
from .routes import goals_bp
app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)
Comment on lines +33 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. 😄

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

class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)
description = db.Column(db.String)
Comment on lines +7 to +8

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.

completed_at = db.Column(db.DateTime)
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'),
nullable=True)
243 changes: 243 additions & 0 deletions app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,245 @@
from app import db
from datetime import datetime
from flask import Blueprint
from flask import request
from flask import jsonify
from .models.task import Task
from .models.goal import Goal
from flask import make_response
import requests
import os
from dotenv import load_dotenv

load_dotenv()
tasks_bp = Blueprint("tasks", __name__, url_prefix="/tasks")
goals_bp = Blueprint("goals", __name__, url_prefix="/goals")
Comment on lines +14 to +15

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=["POST", "GET"])
def handle_tasks():
if request.method == "GET":

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.

tasks = Task.query.all()
tasks_response = []
for task in tasks:
tasks_response.append({
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": True if task.completed_at else False
})
Comment on lines +23 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 our routes where we 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)).

sort_by_title = request.args.get("sort")
if sort_by_title:
if sort_by_title == "asc":
tasks_response = sorted(tasks_response, key = lambda i: i['title'])
if sort_by_title == "desc":
tasks_response = sorted(tasks_response, key = lambda i: i['title'],reverse=True)
Comment on lines +31 to +34

Choose a reason for hiding this comment

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

We can do the sort ourselves, like you've written here, but another possibility is to make the database do the sorting for us! Consider

tasks = Task.query.order_by(Task.title).all()  # orders by title ascending
tasks = Task.query.order_by(Task.title.desc()).all()  # orders by title descending

# or

from sqlalchemy import desc
tasks = Task.query.order_by(desc(Task.title)).all()  # also orders by title descending

Doing this offloads the sorting from our code to the database, which could be configured to make this very fast!

return jsonify(tasks_response), 200

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 {"details": "Invalid data"}, 400
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 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()

return {
"task": {
"id": new_task.task_id,
"title": new_task.title,
"description": new_task.description,
"is_complete": True if new_task.completed_at else False
}
}, 201
Comment on lines +49 to +56

Choose a reason for hiding this comment

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

This code is really only related to the POST case, so consider indenting this under the POST branch. It turns out not to be a problem here since the GET branch ends in a return, so doesn't reach this code.


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

Choose a reason for hiding this comment

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

👍

We could consider using the Flask helper method get_or_404

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":
result = {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": True if task.completed_at else False
}
if task.goal_id:
result["goal_id"] = task.goal_id
Comment on lines +71 to +72

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. Note that this requirement from the project was a little bit strange, but it's good to see that we can alter the output we send back to a client based on the endpoint, or even characteristics of a particular model!

return {
"task": result
}
elif request.method == "DELETE":
message = {"details": f"Task {task.task_id} \"{task.title}\" successfully deleted"}
db.session.delete(task)
db.session.commit()
return make_response(message)
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 +84 to +86

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

return make_response({
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": True if task.completed_at else False
}
})

@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def handle_task_complete(task_id):
task = Task.query.get(task_id)
if task is None:
return make_response("", 404)

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

response = requests.post('https://slack.com/api/chat.postMessage', params={'channel':'task-notifications', 'text': f'Someone just completed the task {task.title}'}, headers={'Authorization': os.environ.get("SLACKBOT_API_KEY")})

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 (requests.post), we would typically send the parameters as form data, rather than as query params (even though it was shown with query params in the example).

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)

Choose a reason for hiding this comment

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

Also, I would expect to see something in your Authorization value about the token being a "Bearer" token. Did you happen to add that to your key value in your .env file? Otherwise, I think Slack would reject the way the Authorization header is set here.


json_response = response.json()
print(json_response)

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

@tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def handle_task_incomplete(task_id):
task = Task.query.get(task_id)
if task is None:
return make_response("", 404)

task.completed_at = None

db.session.commit()

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

@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 Goals. 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.goal_id,
"title": goal.title
})
sort_by_title = request.args.get("sort")
if sort_by_title:
if sort_by_title == "asc":
goals_response = sorted(goals_response, key = lambda i: i['title'])
if sort_by_title == "desc":
goals_response = sorted(goals_response, key = lambda i: i['title'],reverse=True)
return jsonify(goals_response), 200

elif request.method == "POST":
request_body = request.get_json()
if 'title' not in request_body:
return {"details": "Invalid data"}, 400
Comment on lines +161 to +162

Choose a reason for hiding this comment

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

👍

This should get checked for a PUT as well

new_goal = Goal(title=request_body["title"])

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

return {
"goal": {
"id": new_goal.goal_id,
"title": new_goal.title
}
}, 201
Comment on lines +168 to +173

Choose a reason for hiding this comment

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

Like for Task, this should probably be indented under the POST logic


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

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

goal.title = form_data["title"]

db.session.commit()

return make_response({
"goal": {
"id": goal.goal_id,
"title": goal.title
}
})

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

if request.method == "GET":
tasks = Task.query.filter(Task.goal_id == goal_id)

Choose a reason for hiding this comment

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

We can retrieve the tasks this way, but also, our Goal model has a tasks member, so we could also do

tasks = goal.tasks

results = []
for task in tasks:
results.append({
"id": task.task_id,
"goal_id": task.goal_id,
"title": task.title,
"description": task.description,
"is_complete": True if task.completed_at else False
})

return make_response(
{
"id": goal.goal_id,
"title": goal.title,
"tasks": results
}, 200)
elif request.method == "POST":
form_data = request.get_json()
task_ids = form_data['task_ids']

for id in task_ids:
task = Task.query.get(id)
task.goal_id = goal_id
Comment on lines +235 to +237

Choose a reason for hiding this comment

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

👍

Yep. We need to retrieve each task to be able to associate it to the goal.

But 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.goal_id,
"task_ids": task_ids
}, 200)
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