Skip to content

PR - Bukunmi G SL #135

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 3 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
6 changes: 5 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ def create_app(test_config=None):
db.init_app(app)
migrate.init_app(app, db)

# Register Blueprints here
# Register Blueprints here -- Reminder to self alwaus update when you add a new route
from.routes.task import tasks_bp
app.register_blueprint(tasks_bp)
from .routes.goal import goals_bp
app.register_blueprint(goals_bp)

return app
17 changes: 17 additions & 0 deletions app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,20 @@

Choose a reason for hiding this comment

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

Remember to make a __init__.py file in any package folder/subfolder. app and routes each have one, but we should have one here in the models folder as well.

class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)
tasks = db.relationship("Task", back_populates="goal")

def to_dict(self, tasks=False):

Choose a reason for hiding this comment

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

👍 Nice use of a default parameter to allow you to include the task details selectively.

goal_as_dict = {
"id": self.goal_id,
"title": self.title,
}
if tasks:
goal_as_dict["tasks"] = [task.to_dict() for task in self.tasks]
return goal_as_dict


@classmethod
def from_dict(cls, task_data):

Choose a reason for hiding this comment

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

Consider renaming the dictionary parameter to goal_data or data_dict rather than task_data.

new_goal = cls(title=task_data["title"],)
return new_goal
25 changes: 24 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,27 @@


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)

Choose a reason for hiding this comment

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

Note that the default value for nullable is True, so we don't need to list this explicitly.

However, an option we might want to investigate for DateTime is whether it should be timezone aware. Python supports both timezone-aware and timezone-naïve datetimes. Generally, python docs recommend using timezone-aware datetimes, but in that case we should also store them in a timezone-aware db column like

db.Column(db.DateTime(timezone=True))

There's a lot of nuance for dealing with datetimes (for instance, even having a timezone-aware column doesn't help us correct for daylight saving time), so generally you'll want to follow whatever practices your team recommends. If this has piqued your curiosity at all, it's really interesting to dig into!

is_complete = db.Column(db.Boolean, default=False)

Choose a reason for hiding this comment

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

👀 The project README told us that a null value for completed_at implies that a task is incomplete, whereas one with a value marks a completed task. We can determine the value for is_complete from completed_at, which allows us to avoid needing to worry about keeping these two columns in sync if they're stored separately. Storing state one way, and reporting it another way which can be calculated from the oher value is described as having a derived attribute, and is generally preferred when possible.

goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"))
goal = db.relationship("Goal", back_populates="tasks")
Comment on lines +10 to +11

Choose a reason for hiding this comment

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

👍 Nice job setting up the relationship to goals.


def to_dict(self):
task = {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": self.is_complete
}
if self.goal_id:
task["goal_id"] = self.goal_id
Comment on lines +20 to +21

Choose a reason for hiding this comment

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

👍 Nice job selectively including the goal_id only for tasks that are associated with a goal.

We could take a similar approach for setting is_complete based on completed_at.

return task

@classmethod
def from_dict(cls,task_data):
new_task = cls(title=task_data["title"],
description=task_data["description"],)
return new_task
1 change: 0 additions & 1 deletion app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Empty file.
78 changes: 78 additions & 0 deletions app/routes/goal.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
from app import db

Choose a reason for hiding this comment

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

A lot of this code is very similar to routes in Task (as we'd expect), so for those parts, please refer to the task routes file to see which feedback there also applies here.

from app.models.goal import Goal
from app.models.task import Task
from flask import Flask, Blueprint, jsonify, make_response, request, abort, render_template, redirect, url_for
from datetime import datetime as dt
from app.routes.task import validate_model


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

#purpose: create goal.reminder to refactor
@goals_bp.route("", methods=["POST"])
def create_goal():
try:
request_body = request.get_json()
new_goal = Goal.from_dict(request_body)

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

goal_dict = new_goal.to_dict()

return make_response(jsonify({
"goal":goal_dict }), 201)
except:

Choose a reason for hiding this comment

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

Be sure to be explicit about what errors we're watching for, so here,

    except KeyError:

abort(make_response({"details": "Invalid data"}, 400))

#purpose: get saved goals
@goals_bp.route("", methods=["GET"])
def retrieve_goals():
goals = Goal.query.all()
goals_response = [goal.to_dict() for goal in goals]
return jsonify(goals_response), 200

@goals_bp.route("<goal_id>", methods=["GET"])

Choose a reason for hiding this comment

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

The route endpoint here should be "/<goal_id>" (note the leading /). I tried digging a bit to see whether safe to leave off the leading / (can we depend on this behavior?) but I'm not seeing anything. So I'd recommend sticking with the leading /.

def read_one_goal(goal_id):
goal = validate_model(Goal, goal_id)
return make_response(jsonify({
"goal": goal.to_dict()
}))

@goals_bp.route("<goal_id>", methods=["PUT"])
def update_goal(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()
goal.title = request_body["title"]

Choose a reason for hiding this comment

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

Be sure to handle the potential KeyError here as well.

db.session.commit()
return make_response(jsonify({
"goal":goal.to_dict()
})), 200

@goals_bp.route("<goal_id>", methods=["DELETE"])
def delete_goal(goal_id):
goal = validate_model(Goal, goal_id)

db.session.delete(goal)
db.session.commit()
return make_response(jsonify({
"details": f'Goal {goal_id} "{goal.title}" successfully deleted'
}))

@goals_bp.route("<goal_id>/tasks", methods=["POST"])
def add_task(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()

goal.tasks += Task.query.filter(Task.task_id.in_(request_body["task_ids"])).all()

Choose a reason for hiding this comment

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

This query is roughly the equivalent of something like

select * from task where task_id in (1, 2, ...);

One effect of this is that it would this won't report an error if we supply a task id that isn't present in the database, since it would simply not be part of the list of tasks returned from the query. This might be OK, but we also might want our endpoint to raise an error if we try to add a task to the goal that doesn't exist in the database.

If we want it to report invalid task ids, we could iterate over the supplied ids and look them up with validate_model.

db.session.commit()

return make_response (jsonify({
"id": goal.goal_id,
"task_ids": request_body["task_ids"]

Choose a reason for hiding this comment

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

Your logic for associating the tasks with the goal appends the list of ids. So there could be tasks previously associated with the goal that we'd like to include in the output. The test cases don't have enough detail to determine which behavior is desired, so this works fine, but we should be internally consitent. Since you're appending the tasks, I'd expect to see something like this to return the task ids associated with the goal.

        "task_ids": [task.task_id for task in goal.tasks]

}))

@goals_bp.route("<goal_id>/tasks", methods=["GET"])
def get_tasks(goal_id):
goal = validate_model(Goal, goal_id)
return make_response(jsonify(goal.to_dict(tasks=True)))
119 changes: 119 additions & 0 deletions app/routes/task.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
from app import db
from app.models.task import Task
from flask import Flask, Blueprint, jsonify, make_response, request, abort
from datetime import datetime as dt


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

def validate_model(cls, model_id):

Choose a reason for hiding this comment

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

Nice job using the refactored style of validate_model that can be applied to various models. We should also move it into a separate file so that one route doesn't need to import things from another route file. If validate_model lived in a different file (maybe route_helpers.py), the both routes would import it from the helper file.

try:
model_id = int(model_id)
except:
abort(make_response({"details":f"{cls.__name__} {model_id} invalid"}, 400))

model = cls.query.get(model_id)
if not model:
abort(make_response({"details": f"{cls.__name__} {model_id} not found"}, 404))
return model


@tasks_bp.route("", methods=["POST"])
def create_task():
try:
request_body = request.get_json()
new_task = Task.from_dict(request_body)


db.session.add(new_task)
db.session.commit()

task_dict = new_task.to_dict()
return make_response(jsonify({"task": task_dict}), 201)
except KeyError:
return (make_response(jsonify({"details": "Invalid data"})), 400)
Comment on lines +33 to +34

Choose a reason for hiding this comment

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

👍 Nice use of looking for a KeyError to determine whether all the required data was present!



# @tasks_bp.route("", methods=["GET"])
# def no_saved_tasks():
# tasks_response = []
# return jsonify(tasks_response)

@tasks_bp.route("", methods=["GET"])
def get_saved_tasks():
sort_query = request.args.get("sort")
task_query = Task.query

if sort_query =="asc":
task_query = Task.query.order_by(Task.title.asc())

Choose a reason for hiding this comment

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

It's a little safer to make use of the task_query variable you set aside earlier in case we added additional filtering logic at a later date.

        task_query = task_query.order_by(Task.title.asc())

if sort_query == "desc":

Choose a reason for hiding this comment

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

This could be an elif since it's related to the first if condition (we would only do one or the other).

task_query = Task.query.order_by(Task.title.desc())
tasks = task_query.all()

tasks_response = [task.to_dict() for task in tasks]
return jsonify(tasks_response), 200


@tasks_bp.route("/<task_id>", methods=["GET"])
def get_one_saved_tasks(task_id):
# task_response = []

Choose a reason for hiding this comment

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

Remove unused (commented) code from the final code. While it may feel useful to have some of the code you thought about using in your code for future reference, in industry, leaving commented code in the project is confusing to other team members who need to decide whether code they see that's commented out was supposed tobe commented or not.

To keep code, we can always copy it into an external file. But we can also make use of our version history to look at what previous versions of the code were like.

# id_query = request.args.get("id")
# if id_query:
# task = Task.query.filter_by(task_id=id)
# print(task,"test")
# task_response.append(task)
# return jsonify(task_response)

task = validate_model(Task, task_id)
return {"task": task.to_dict()}, 200
# return make_response(jsonify({
# "task": task.to_dict()
# }))

@tasks_bp.route("/<task_id>", methods=["PUT"])
def update_task(task_id):
task = validate_model(Task, task_id)

request_body = request.get_json()
task.title = request_body["title"]
task.description = request_body["description"]
Comment on lines +78 to +79

Choose a reason for hiding this comment

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

Notice that this can cause the same KeyErrors as in the post request. So even though there aren't tests for it, we should have similar invalid request handling to the POST route.


db.session.commit()

return make_response(jsonify({
"task": task.to_dict()
}))

@tasks_bp.route("/<task_id>", methods=["DELETE"])
def delete_task(task_id):
task = validate_model(Task, task_id)

db.session.delete(task)
db.session.commit()

return make_response(jsonify({"details": f'Task {task_id} "{task.title}" successfully deleted'}))

@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def update_task_to_complete(task_id):
task = validate_model(Task, task_id)

task.completed_at = dt.utcnow()

Choose a reason for hiding this comment

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

utcnow returns a naïve datetime (no timeszone information). This matches the way you made you database column (no timezone data) but the python docs recommend using datetimes that are timezone-aware. So we may want to consider using a database column that is timezone aware, and dt.now() rather than utcnow.

task.is_complete = True
Comment on lines +100 to +101

Choose a reason for hiding this comment

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

This shows why we like to take advantage of derived attributes when we can. It's not too bad here, and there are only a few places where the values are being updated, but this can get out of hand surprisingly quickly.


db.session.commit()
return make_response(jsonify({

Choose a reason for hiding this comment

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

This is also where we add our logic for posting to slack. If you have time, I'd still suggest looking into trying that out, as it will be good practice for making requests to other APIs using the requests module.

"task": task.to_dict()})), 200

@tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def change_task_incomplete(task_id):
task = validate_model(Task, task_id)

task.completed_at = None
task.is_complete = False

db.session.commit()
return make_response(jsonify({
"task": task.to_dict()
}))


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