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

Spruce - Sarah Olivas #82

Open
wants to merge 14 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()'
11 changes: 7 additions & 4 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,10 @@ def create_app(test_config=None):
app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

if test_config is None:
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_DATABASE_URI")
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_DATABASE_URI")

Choose a reason for hiding this comment

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

I'm curious what prompted unwrapping these lines. Doing so exceeds the typical 80 column limit. Are you running a formatting tool?

else:
app.config["TESTING"] = True
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_TEST_DATABASE_URI")
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_TEST_DATABASE_URI")

# Import models here for Alembic setup
from app.models.task import Task
Expand All @@ -30,5 +28,10 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here
from .routes import tasks_bp
app.register_blueprint(tasks_bp)

from .routes import goals_bp
app.register_blueprint(goals_bp)
Comment on lines +31 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 can do to help our routes file from getting too large is to consider making multiple files containing routes separated by the resource type. We might have a routes folder instead of a routes.py file, and inside that folder (along with a __init__.py) we could have a file per resource, so task.py and goal.py. Where each would have a blueprint and endpoints for that resource. When we have one blueprint per file, we often name the blueprint simply bp rather than including the resource name as part of it.

Then here, we could import and register the blueprints like

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


return app
20 changes: 18 additions & 2 deletions app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,22 @@
from flask import current_app
from app import db


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
goal_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.

According to the SqlAlchemy docs about autoincrement:

The default value is the string "auto" which indicates that a single-column primary key that is of an INTEGER type with no stated client-side or python-side defaults should receive auto increment semantics automatically; all other varieties of primary key columns will not.

This means that here, since the primary is not a compound key, the type is integer, and there's no specified default value, the desired autoincrement behavior will apply even without listing it. It doesn't hurt anything to list it, but it's not required.

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", back_populates="goal", lazy=True)


def goal_dict(self):
return {
"id": self.goal_id,
"title": self.title
}


def goal_and_task_dict(self):
return{
"id": self.goal_id,
"title": self.title,
"tasks": [task.task_and_goal_dict() for task in self.tasks]
}
Comment on lines +17 to +22

Choose a reason for hiding this comment

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

👍

Nice job having two different methods to provide the two different needed output styles. We could have used the goal_dict version to get the base values, then add in tasks, but given the size this seems ok.

Notice that as applications grow, we may end up with a large variety of endpoints that may need slightly different outputs. In that situation, rather than adding method after method to model class itself, we often will make helper classes whose sole responsibility is knowing how to render a model instance into a certain output configuration. Sometimes we call these helper classes data transfer objects (DTOs).

27 changes: 25 additions & 2 deletions app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,29 @@
from flask import current_app
from app import db


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)
goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"), nullable=True)
Comment on lines +6 to +9

Choose a reason for hiding this comment

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

It turns out that nullable=True is the default value for nullable. So all of the columns for Task here are currently marked as nullable. But should title or description be allowed to be NULL? (Does that make sense from a data standpoint?) Consider adding nullable=False to those columns.

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 us protect against that happening!

goal = db.relationship("Goal", back_populates="tasks")


def task_dict(self):
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": False if self.completed_at is None else True,
}


def task_and_goal_dict(self):
return {
"id":self.task_id,
"title": self.title,
"description": self.description,
"is_complete": False if self.completed_at is None else True,
"goal_id": self.goal_id

Choose a reason for hiding this comment

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

Again, having these two methods is fine and clearly indicates to a caller that they might produce different output. But another approach would be to have a single dictionary function, and only add the goal_id field to the output if the task is associated with a goal. This means that the output format for the task could change depending on its data relationships, which could be confusing, but it also means that the task itself is the one deciding whether to include that information, which the argument could be made that the task is the best one to make that decision, rather than logic in a route. This would also allow this behavior to occur anywhere we return a task, whether individually or in a list.

Notice that, as written, getting multiple tasks would omit any goal information since the route wasn't updated to be aware of goals.

}
179 changes: 178 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,179 @@
from flask import Blueprint
from io import DEFAULT_BUFFER_SIZE

Choose a reason for hiding this comment

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

Unnecessary input, likely from a spurious autocomplete

from app import db
from flask import Blueprint, json, request, jsonify
from app.models.task import Task
from app.models.goal import Goal
from sqlalchemy import asc, desc
from datetime import datetime
import os, requests
from dotenv import load_dotenv

load_dotenv()
Comment on lines +9 to +11

Choose a reason for hiding this comment

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

No need to import the dotenv stuff anywhere but in the main __init__.py file. Once the settings have been loaded at startup, they become available in the os.environ without any further action.


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

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

Choose a reason for hiding this comment

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

Notice that the logic for GET and POST doesn't share any code, so if we wanted to try refactoring this 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
):
Comment on lines +20 to +24

Choose a reason for hiding this comment

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

👍

Good job enforcing the POST data requirements. 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

Choose a reason for hiding this comment

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

Also, nice use of () in the if to enable implicit line wrapping. This is prefered over using explicit line wrapping (\ at the end of a line).

return jsonify({"details": "Invalid data"}), 400

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 jsonify({"task": new_task.task_dict()}), 201

elif request.method == "GET":
sort_query = request.args.get("sort")
if sort_query == "asc":
tasks = Task.query.order_by(asc("title"))

Choose a reason for hiding this comment

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

This can also be written as

        tasks = Task.query.order_by(Task.title.asc())

which lets us avoid importing the asc function. We can do the same thing for desc and then that extra import could be removed.

Notice also that using the static column field Task.title has a greater opportunity for the IDE to correctly autocomplete, whereas using "title" could introduce typos.

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

task_response = [task.task_dict() for task in tasks]

Choose a reason for hiding this comment

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

👍

Nice list comprehension!


return jsonify(task_response), 200


@tasks_bp.route("/<task_id>", methods={"GET", "PUT", "DELETE"})
def handle_one_task(task_id):

Choose a reason for hiding this comment

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

Each of these verbs does depend on retrieving an existing task from the database by id, so there's a stronger case for keeping these together. But we could still think about splitting these into separate functions (e.g. get_task, update_task, and delete_task). We would need to either duplicate the lookup code in each function, or create a helper function that can be shared among the separated functions.

task = Task.query.get(task_id)
if task is None:
return jsonify(task), 404

if request.method == "GET":
if task.goal_id:
return jsonify({"task": task.task_and_goal_dict()})
Comment on lines +59 to +60

Choose a reason for hiding this comment

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

As alluded to earlier, should the route be the one that decides which model rendering function to use by inspecting the goal relationship within the task itself? Or should the task simply be asked to convert itself to a dictionary, and then it decides how to build the dictionary given its own internal state? It's not clear either way, but it's definitely worth thinking about!

return ({"task": task.task_dict()}), 200

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

Choose a reason for hiding this comment

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

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.

At the least, we should ensure that the keys are present in the dictionary before trying to access them, otherwise this request would cause a KeyError.


db.session.commit()
return jsonify({"task": task.task_dict()}), 200

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


@tasks_bp.route("<task_id>/mark_complete", methods=["PATCH"])
def mark_task_complete(task_id):
task= Task.query.get(task_id)
if task is None:
return jsonify(task), 404
Comment on lines +80 to +82

Choose a reason for hiding this comment

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

Notice that this chunk of looking up an id, checking for None, and returning a 404 when not found recurs frequently. It's a little tricky to extract this into a helper function, mostly since a helper function can't force the calling function to exit. That is, unless it raises an error. So we could think about writing a helper function to perform this set of steps, but we would also need to customize the Flask error handling behavior to return JSON rather than HTML. There is the get_or_404 method that essentially does the lookup with errors already, but we would should still update the flask error handling (see here for more details.)

Another approach could be to use a decorator to intercept the task id from the route, and performt the lookup before reaching the body of our endpoint. There's an example of that here.


current_time = datetime.now()

Choose a reason for hiding this comment

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

Consider using utcnow() rather than now(), especially if there's no timezone information on the datetime in the database. When working with dates and time, it's usually a good idea to pass them around and store them in UTC, and only convert them to local time when needed for display (like in a UI). Working with dates and times is tricky!

task.completed_at = current_time

BOT_TOKEN = os.environ.get('BOT_TOKEN')
requests.post(
url="https://slack.com/api/chat.postMessage",
headers={"Authorization": f"Bearer {BOT_TOKEN}"},
data={"channel": "task_notifications", "text": f"Someone just completed the task {task.title}"}
)
Comment on lines +88 to +92

Choose a reason for hiding this comment

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

👍

Nice POST request to Slack!

Choose a reason for hiding this comment

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

Consider putting this part of the code into its own helper function. All the mark complete endpoint really needs to know how to do is call a method that can post to Slack with the message to post. The rest of the details could be hidden in the helper, and potentially called from multiple places!


db.session.commit()

return jsonify({"task": task.task_dict()}), 200


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

db.session.commit()

return jsonify({"task": task.task_dict()}), 200


@goals_bp.route("", 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 == "POST":
request_body = request.get_json()
if "title" not in request_body:
return jsonify({"details": "Invalid data"}), 400

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

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

return jsonify({"goal": new_goal.goal_dict()}), 201

elif request.method == "GET":
request_body = request.get_json()
goals = Goal.query.all()
goal_response = [goal.goal_dict() for goal in goals]
return jsonify(goal_response), 200


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

request_body = request.get_json()
if request.method == "GET":
return jsonify({"goal": goal.goal_dict()}), 200

elif request.method == "PUT":
if "title" not in request_body:
return jsonify({"details": "Invalid data"}), 400

goal.title = request_body["title"]
db.session.commit()
return jsonify({"goal": goal.goal_dict()}), 200

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


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

if request.method == "GET":
return jsonify(goal.goal_and_task_dict()), 200

elif request.method=="POST":
request_body = request.get_json()

for task_id in request_body["task_ids"]:
task = Task.query.get(task_id)
if task is None:
return jsonify(None), 404
goal.tasks.append(task)
db.session.commit()
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.

Good job checking whether the each requested task id lookup is successful before trying to append it to the task list. However, you may want to wait to perform the appending and commiting until after you have validated all the tasks, rather than task by task. With the task-by-task approach, we could end up partially modifying the tasks in a goal, but then fail partway through the list, leaving the goal partially modified. It's often desirable to ensure that either the entire change has been applied, or none of the change has.

Choose a reason for hiding this comment

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

Appending to the tasks realtionship is absolutely a valid way to associate a task with a goal. Notice that it's not entirely clear from the project description or tests whether POSTing to the task list is adding tasks to the goal (in addition to anything that was already there), or replacing the tasks in the goal. Which perspective does your implementation take? How could we implement the other approach?


task_ids = []
for task in goal.tasks:
task_ids.append(task.task_id)

return jsonify({"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