Skip to content

Accelerate - Denisse #9

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
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()'
6 changes: 4 additions & 2 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
from flask import Flask
from flask_sqlalchemy import SQLAlchemy
from flask_migrate import Migrate
import os
from dotenv import load_dotenv
import os


db = SQLAlchemy()
Expand Down Expand Up @@ -30,5 +30,7 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here

from .routes import tasks_bp, goal_bp
app.register_blueprint(tasks_bp)
app.register_blueprint(goal_bp)
Comment on lines +33 to +35

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)

Choose a reason for hiding this comment

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

Since you changed Task.task_id to Task.id, consider changing this to Goal.id too.

title = db.Column(db.String)
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)
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 totally agree with changing task_id to id! 🎉

title = db.Column(db.String)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True, default=None)
goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"), nullable=True)

Choose a reason for hiding this comment

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

If you chose to rename Goal.goal_id to Goal.id, then the foreign key here would change to goal.id.

224 changes: 223 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,224 @@
from flask import Blueprint
from app import db
from app.models.task import Task
from app.models.goal import Goal
from flask import json, request, Blueprint, make_response, jsonify

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 you're using the json package here.

Suggested change
from flask import json, request, Blueprint, make_response, jsonify
from flask import request, Blueprint, make_response, jsonify

from datetime import datetime
import os
import requests

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

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

title_from_url = request.args.get("title")
if title_from_url:
tasks = Task.query.filter_by(title=title_from_url)
else:
sort = request.args.get("sort")
Comment on lines +17 to +18

Choose a reason for hiding this comment

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

It does make sense for you to have the sort in an else, since the filter_by will only return matching titles (possibly multiple, if there is more than one task with sharing the same title) and since the titles would be the same, sorting doesn't make sense. But in general, we might want to allow a combination of sorting and filtering.

Interestingly, until we call all() on a query expression, we can continue to chain additional filter expressions at the end of the expression built so far. So we could have a query that filters on a substring (for instance), and then chain an order_by onto the end before finally calling all() to get the list of results.

Give it a try!

if not sort:
tasks = Task.query.all()
Comment on lines +19 to +20

Choose a reason for hiding this comment

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

Wouldn't this get handled by the else?

elif sort == "asc":
tasks = Task.query.order_by(Task.title.asc()).all()
elif sort == "desc":
tasks = Task.query.order_by(Task.title.desc()).all()
else:
tasks = Task.query.all()

tasks_response = []
for task in tasks:
tasks_response.append({
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": bool(task.completed_at)
})
Comment on lines +30 to +35

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()
title = request_body.get("title")
description = request_body.get("description")

if not title or not description or "completed_at" not in request_body:
return jsonify({"details": "Invalid data"}), 400
Comment on lines +43 to +44

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=title,
description=description,
completed_at=request_body["completed_at"])
db.session.add(new_task)
db.session.commit()
commited_task = {"task":
{"id": new_task.id,
"title": new_task.title,
"description": new_task.description,
"is_complete": bool(new_task.completed_at)
}}
return jsonify(commited_task), 201

@tasks_bp.route("/<tasks_id>", methods=["GET", "PUT", "DELETE"])
def handle_task(tasks_id):
task = Task.query.get_or_404(tasks_id)

Choose a reason for hiding this comment

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

👍

Nice use of 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":
if task.goal_id != None:
selected_task = {"task":
{"id": task.id,
"goal_id": task.goal_id,

Choose a reason for hiding this comment

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

👍

It's a little strange that the tests expected goal_id to appear only when it's set. I would typically expect a public API that had relationships like this to simply return null for a task that wasn't associated with a Goal, but this does the job.

However, we can also notice that including this key is the only difference between the two cases, so rather than duplicating the dictionary code, we could generate the base dictionary, then only add goal_id if the value is defined in the Task.

Selectively including the goal_id

"title": task.title,
"description": task.description,
"is_complete": bool(task.completed_at)
}}
else:
selected_task = {"task":
{"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": bool(task.completed_at)
}}
return jsonify(selected_task),200

elif request.method == "PUT":
request_body = request.get_json()
task.title = request_body["title"]
task.description = request_body["description"]
task.completed_at = request_body["completed_at"]
Comment on lines +82 to +84

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.

updated_task = {'task':{
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": bool(task.completed_at)
}}
db.session.commit()
return jsonify(updated_task),200

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

@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def handle_task_complete(task_id):
task = Task.query.get_or_404(task_id)
task.completed_at = datetime.now()

db.session.commit()

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

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

db.session.commit()

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

# Slack Portion
def post_to_slack(text):

Choose a reason for hiding this comment

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

👍

This looks good. But it looks like you're not calling this from anywhere? Did you intentionally remove its use from your code?

slack_token = os.environ.get("SLACK_TOKEN_POST")
slack_path = "https://slack.com/api/chat.postMessage"
query_params = {
"channel": "task-notification",
"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=params, headers=headers)


# Goals Route Portion
goal_bp = Blueprint("goal_bp", __name__, url_prefix="/goals")

@goal_bp.route("", methods=["GET", "POST"])
def handle_goals():
if request.method == "GET":

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.

goals = Goal.query.all()
goals_response = []
for goal in goals:
goals_response.append({
"id": goal.goal_id,
"title": goal.title,
})
return jsonify(goals_response), 200
elif request.method == "POST":
request_body = request.get_json()
title = request_body.get("title")
if not title:
return jsonify({"details": "Invalid data"}), 400
new_goal = Goal(title=request_body["title"])

db.session.add(new_goal)
db.session.commit()
goal_response_body = {"goal": {"id": new_goal.goal_id, "title": new_goal.title}}

return jsonify(goal_response_body), 201

@goal_bp.route("/<goal_id>", methods=["GET", "PUT", "DELETE"])
def handle_goal(goal_id):
goal = Goal.query.get_or_404(goal_id)
if request.method == "GET":
selected_goal = {"goal":
{"title": goal.title,
"id": goal.goal_id
}}
return jsonify(selected_goal), 200
elif request.method == "PUT":
request_body = request.get_json()
goal.title = request_body["title"]
updated_goal = {'goal':{
"id": goal.goal_id,
"title": goal.title
}}
db.session.commit()
return jsonify(updated_goal),200

elif request.method == "DELETE":
db.session.delete(goal)
db.session.commit()
goal_response_body = {"details": f'Goal {goal.goal_id} "{goal.title}" successfully deleted'}
return jsonify(goal_response_body),200

@goal_bp.route("/<goal_id>/tasks", methods=["GET", "POST"])
def handle_goals_and_tasks(goal_id):
if request.method == "POST":
goal = Goal.query.get_or_404(goal_id)
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 +200 to +204

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?


goal_task_response_body = {"id": goal.goal_id, "task_ids": request_body["task_ids"]}
return jsonify(goal_task_response_body), 200

elif request.method == "GET":
goal = Goal.query.get_or_404(goal_id)
tasks = goal.tasks
list_of_tasks = []

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