-
Notifications
You must be signed in to change notification settings - Fork 97
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
base: master
Are you sure you want to change the base?
Changes from all commits
cc9a62a
ea55ed5
e61777d
287aea8
3e84b1b
360dd1c
773e638
6db250b
5d790e5
c385230
004f704
7c55119
582c253
0dc4ce6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
web: gunicorn 'app:create_app()' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
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 | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the SqlAlchemy docs about autoincrement:
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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). |
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It turns out that 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Notice that, as written, getting multiple tasks would omit any goal information since the route wasn't updated to be aware of goals. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,179 @@ | ||
from flask import Blueprint | ||
from io import DEFAULT_BUFFER_SIZE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to import the dotenv stuff anywhere but in the main |
||
|
||
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(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, nice use of |
||
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")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Notice also that using the static column field |
||
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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice POST request to Slack! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Generic single-database configuration. |
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 |
There was a problem hiding this comment.
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?