Skip to content
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
7 changes: 7 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,12 @@ 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 +33 to +37

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


19 changes: 18 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,23 @@
from flask import current_app
from flask import current_app, jsonify

Choose a reason for hiding this comment

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

Unless you are actually calling the jsonify method in the file (which I don't see) there's no need to import jsonify. Though come to think of it, I don't see current_app used here, so I don't know why the boilerplate code included that import at all! 😂

from app import db


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

def goal_dict(self):
return{
"id":self.goal_id,
"title":self.title
}
Comment on lines +10 to +14

Choose a reason for hiding this comment

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

👍

Nice helper to convert from a model instance to a dict for JSON results.

@classmethod
def goal_arguments(cls, title_from_url):
if title_from_url:
goals = Goal.query.filter_by(title=title_from_url).all()
if not goals:
goals = Goal.query.filter(Goal.title.contains(title_from_url))
else:
goals = Goal.query.all()
return goals
Comment on lines +15 to +23

Choose a reason for hiding this comment

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

Neat class helper to load goals with optional filter. One thing to be careful of is what we should always try to return consistent result type from methods, so if one path returns a list of results, we should generally try to return lists (even if they are empty do to no results) fo consistency. This can help the function caller write simpler code. Currently, if title_from_url is falsy, this function falls through and would return None. Returning None can be ok (though we should generally do it explicitly, otherwise it can look like we simply forgot to think about that case), but if we returned an empty list, then the caller could always simply iterate over any (possible no) results safely. But currently, they would first need to perform a falsy check first.

All that being said, I don't see this being used anywhere else in the code currently?

26 changes: 23 additions & 3 deletions app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,26 @@
from flask import current_app
from flask import current_app, request
from app import db
from dotenv import load_dotenv
from sqlalchemy import desc,asc
Comment on lines +1 to +4

Choose a reason for hiding this comment

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

Here again, be careful with the imports. None of the changes that were done here import symbols that are actually being used in this file, so they're unnecessary.



#task is a class
class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
task_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 wouldn't hurt anything to list it, but it's not required.

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'))
Comment on lines +9 to +12

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 impicitly 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_id = db.relationship("Goal", back_populates="tasks")

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

if self.goal_id:
task_dict["goal_id"]=self.goal_id
Comment on lines +16 to +24

Choose a reason for hiding this comment

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

👍

Nice job providing this helper, and having it selectively include the goal id in the output if present.


return task_dict
239 changes: 238 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,239 @@
from flask import Blueprint
from flask import Blueprint, jsonify, make_response,request

Choose a reason for hiding this comment

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

Here, we definitely need jsonify and request, but it doesn't look like you're using make_response anywhere (no need for it).

from app.models.task import Task
from app.models.goal import Goal
from app import db
from datetime import datetime
#what comes from the "from" is the package name
#what comes after the import is a variable that matches the name of thing we want to import
Comment on lines +6 to +7

Choose a reason for hiding this comment

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

👍

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

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

@goals_bp.route("",methods=["POST", "GET"])
def handle_goal():

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 below will also apply to Goal here. 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": "Invalid data"
},400

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
}
Comment on lines +29 to +32

Choose a reason for hiding this comment

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

Don't forget about you goal_dict function. You should be able to use that here.

}, 201

elif request.method == "GET":
sorting_goals= request.args.get('sort')
list = None

Choose a reason for hiding this comment

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

list will definitely have a value by the time the following conditions have been checked (each assigns to list), so the None here isn't all that dangerous, but it's a little confusing. If we're trying to fill in a list with deferred lookups, it's usually better to initialize the variable with an empty list rather than None, as it works as expected with iteration, while None would crash.

Also, consider using a name other than list, since that hides the class name list. Python lets us do this, but again, it can be confusing.

if sorting_goals== "desc":
list = Goal.query.order_by(Goal.title.desc()) # descending method
elif sorting_goals == "asc":
list = Goal.query.order_by(Goal.title.asc()) # ascending method
else:
list = Goal.query.all()
Comment on lines +38 to +43

Choose a reason for hiding this comment

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

👍

Nice sorting handling.

goals_response = []
for goal in list:
goals_response.append({
"id": goal.goal_id,
"title": goal.title,
})
Comment on lines +46 to +49

Choose a reason for hiding this comment

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

Another spot where goal_dict could be used.


return jsonify(goals_response)

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

if request.method == "GET":
return {
"goal": {
"id":goal.goal_id,
"title":goal.title,
}
}
if request.method == "PUT":
form_data = request.get_json()

goal.title = form_data["title"]

db.session.commit()

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

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

return jsonify({
"details": f'Goal {goal.goal_id} "{goal.title}" successfully deleted'
}),200


@goals_bp.route("/<goal_id>/tasks", methods=["POST","GET"])
def post_tasked_goal(goal_id):

goal = Goal.query.get(goal_id)

if goal == None:
return (""), 404

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

tasks_instances= []
for task_id in request_body["task_ids"]:
tasks_instances.append(Task.query.get(task_id))

Choose a reason for hiding this comment

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

Don't forget to ensure that the result of calling get returns a valid Task. what if an invalid task id had been included in the input? Then we would get back a None that would get added to the list.


goal.tasks = tasks_instances

Choose a reason for hiding this comment

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

👍

Assigning to the tasks relationship is absolutely a valid way to associate tasks with a goal. We should also think about whether to interpret POSTing to this endpoint is setting the exact list of tasks that should be a part of the goal, or whether we are adding these tasks to the goal (in addition to anything that was already there). Neither the project description nor the test test_post_task_ids_to_goal_already_with_goals are entirely clear on the desired behavior. Which perspective does your implementation take? How could we implement the other approach?


db.session.commit()

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

response_body = {
"id": goal.goal_id,
"task_ids": task_ids
}

return jsonify(response_body), 200

if request.method == "GET":
tasks_response =[]
for task in goal.tasks:
tasks_response.append({
"id": task.task_id,
"goal_id": task.goal_id,
"title": task.title,
"description": task.description,
"is_complete": bool(task.completed_at)
})
response_body = {
Comment on lines +122 to +129

Choose a reason for hiding this comment

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

Don't forget about task_dict. Helper methods get sad when they aren't used. 😀

"id": goal.goal_id,
"title": goal.title,
"tasks" : tasks_response
}
return jsonify(response_body), 200


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

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 == "GET":

sorting_task = request.args.get('sort')
list = None
if sorting_task == "desc":
list = Task.query.order_by(Task.title.desc()) # descending method
elif sorting_task == "asc":
list = Task.query.order_by(Task.title.asc()) # ascending method
else:
list = Task.query.all()
Comment on lines +143 to +148

Choose a reason for hiding this comment

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

👍

tasks_response = []
for task in list:
tasks_response.append({
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete" : False
if task.completed_at == None else True
})
Comment on lines +151 to +157

Choose a reason for hiding this comment

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

Why not use your task_dict method to generate this dictionary?


return jsonify(tasks_response)

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:

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

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

def update_completion(task_id, value):

Choose a reason for hiding this comment

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

👍

Nice helper to take care up updating the completion status of a task!

task = Task.query.get(task_id)

if not task:
return("Task not found",404)

task.completed_at = value

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

@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def mark_complete(task_id):
return update_completion(task_id, 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!


@tasks_bp.route("/<task_id>/mark_incomplete", methods =["PATCH"])
def mark_incomplete(task_id):
return update_completion(task_id,None)


@tasks_bp.route("/<task_id>", methods=["GET","PUT","DELETE"])
def handle_task_get(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 == None:
return ("", 404)
Comment on lines +204 to +206

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.


if request.method == "GET":
response_body = {}
response_body["task"] = task.task_dict()

return jsonify(response_body)

if request.method == "PUT":
form_data = request.get_json()

task.title = form_data["title"]
task.description = form_data["description"]
Comment on lines +217 to +218

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":{
"id":task.task_id,
"title":task.title,
"description":task.description,
"is_complete":False if task.completed_at
== None else True
}
Comment on lines +223 to +229

Choose a reason for hiding this comment

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

Another spot where task_dict could be used.

}),200

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

return jsonify({
"details": f'Task {task.task_id} "{task.title}" successfully deleted'
}),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