Skip to content

Tigers - Paje B #128

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

# Register Blueprints here
from app.routes import tasks_bp
from app.goal_routes import goals_bp

# Register Blueprints here
app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)

return app
92 changes: 92 additions & 0 deletions app/goal_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
from flask import Blueprint, jsonify, request, make_response, abort
from app.models.task import Task
from app.models.goal import Goal
from app.routes import validate_model_id
from sqlalchemy import desc, asc
import datetime
import requests
import os
from app import db

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


@goals_bp.route("", methods=["GET"])
def retrieve_goals():
goals = Goal.query.all()
goals_response = []

for goal in goals:
goals_response.append(goal.format_goal())
return jsonify(goals_response)


@goals_bp.route("", methods=["POST"])
def create_goal():
request_body = request.get_json()

if not request_body.get("title"):
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}}, 201


@goals_bp.route("/<goal_id>", methods=["GET"])
def handle_goal(goal_id):
goal = validate_model_id(Goal, goal_id)

return {"goal": goal.format_goal()}


@goals_bp.route("/<goal_id>", methods=["PUT"])
def update_goal(goal_id):
goal = validate_model_id(Goal, goal_id)

request_body = request.get_json()

goal.title = request_body["title"]
db.session.commit()

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

Choose a reason for hiding this comment

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

Like my comment on missed format_task opportunities, format_goal could have been used here.

Choose a reason for hiding this comment

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

And other places, but I'll not repeat myself.



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

db.session.delete(goal)
db.session.commit()

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


@goals_bp.route("/<goal_id>/tasks", methods=["GET"])
def add_task_to_goal(goal_id):
goal = validate_model_id(Goal, goal_id)
tasks_response = []
for task in Goal.query.get(goal_id).tasks:
task.goal_id = goal.goal_id
tasks_response.append(task.format_task_goal())

return {"id": goal.goal_id, "title": goal.title, "tasks": tasks_response}


@goals_bp.route("/<goal_id>/tasks", methods=["POST"])
def retrieve_tasks_from_goal(goal_id):
goal = validate_model_id(Goal, goal_id)
request_body = request.get_json()
task_ids = []
for task_id in request_body["task_ids"]:
task = validate_model_id(Task, task_id)
task.goal_id = goal.goal_id
task_ids.append(task.task_id)

db.session.commit()

return {"id": goal.goal_id, "task_ids": task_ids}
10 changes: 9 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,12 @@


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

def format_goal(self):
return {
"id": self.goal_id,
"title": self.title
}
24 changes: 23 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,26 @@


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

def format_task(self):
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at),

Choose a reason for hiding this comment

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

I like this use of bool here. There's a few different ways to convert completed_at but this one is probably the most succinct. Not that succinctness is always a virtue, especially when it impacts readability (which it does not in your case). But here it's nice.

}

def format_task_goal(self):
return {
"id": self.task_id,
"goal_id": self.goal_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at),
}
Comment on lines +12 to +27

Choose a reason for hiding this comment

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

But this repetition between format_task_goal and format_task is not as nice. Rather than having two methods, keep in mind that keys (and their values) can be added to Python dictionaries after initialization. So you can do the adding of the "goal_id" key in an if block:

    def format_task(self):
        task_dict = {
            "id": self.task_id,
            "title": self.title,
            "description": self.description,
            "is_complete": bool(self.completed_at),
        }

        if self.goal_id:
            task_dict["goal_id"] = self.goal_id

        return task_dict

and avoid the repetition.

148 changes: 147 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,147 @@
from flask import Blueprint
from flask import Blueprint, jsonify, request, make_response, abort

Choose a reason for hiding this comment

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

Minor file naming: While keeping the Goal routes in their separate module is fine, it would have been a good chance to rename this ile to something like task_routes.py. When I see just routes.py compared to goal_routes.py, I would guess that routes.py are general routes, not specifically Task routes. Just kind of the idea that since there's no descriptor for these routes, it must be more general.

If there is some concern that validate_model_id is shared between the two files, then that's probably a sign that it would be good in its own separate file, perhaps along with the slackbot_call function.

This file would be kind of a grab bag of utility methods, which, while not ideal are very common. So I would call it utils.py or something like that.

from app.models.task import Task
from sqlalchemy import desc, asc
import datetime
import requests
import os
from app import db


def slackbot_call(message):
PATH = "https://slack.com/api/chat.postMessage"
parameters = {"channel": "task-notifications", "text": message}

requests.post(
PATH,
params=parameters,
headers={"Authorization": os.environ.get("SLACKBOT_API_KEY")},
)
Comment on lines +10 to +18

Choose a reason for hiding this comment

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

Nice helper function!



def validate_model_id(cls, model_id):
try:
model_id = int(model_id)
except ValueError:
abort(
make_response(
f"{model_id} is an invalid input. Please input an integer.", 400
)
)

model = cls.query.get(model_id)
if model:
return model
else:
abort(make_response({"error": f"Item #{model_id} cannot be found"}, 404))


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


@tasks_bp.route("", methods=["GET"])
def retrieve_tasks():
tasks = Task.query.all()
tasks_response = []
sort_query = request.args.get("sort")

if sort_query == "asc":
tasks = Task.query.order_by(asc("title"))
elif sort_query == "desc":
tasks = Task.query.order_by(desc("title"))

for task in tasks:
tasks_response.append(task.format_task())
return jsonify(tasks_response)


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

if not request_body.get("title") or not request_body.get("description"):
return {"details": "Invalid data"}, 400
Comment on lines +61 to +62

Choose a reason for hiding this comment

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

Good job using get here so you don't get the KeyError if you had used the indexing brackets (request_body["title"], for example).


new_task = Task(
title=request_body["title"],
description=request_body["description"],
completed_at=request_body.get("completed_at"),
)
db.session.add(new_task)
db.session.commit()
return {"task": new_task.format_task()}, 201


@tasks_bp.route("/<task_id>", methods=["GET"])
def handle_task(task_id):
task = validate_model_id(Task, task_id)
if task.goal_id:
return {"task": task.format_task_goal()}
else:
return {"task": task.format_task()}


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

request_body = request.get_json()

task.title = request_body["title"]
task.description = request_body["description"]
task.completed_at = request_body.get("completed_at")

db.session.commit()

return {"task": task.format_task()}


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

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

return make_response(
{"details": f'Task {task_id} "{task.title}" successfully deleted'}
)
Comment on lines +105 to +107

Choose a reason for hiding this comment

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

Not really a need for make_response here as we're not doing something like changing what the status code is. So we can just return this dictionary you've created as you do on line 95 above.

Choose a reason for hiding this comment

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

I see a few other places where this is true, but I'll just point it out this once and not repeat myself.



@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def update_task_as_complete(task_id):
task = validate_model_id(Task, task_id)
task.completed_at = datetime.datetime.now()
db.session.commit()

slackbot_call(f"Someone just completed the task {task.title}")

return make_response(
{
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": (bool(task.completed_at)),
}
Comment on lines +120 to +125

Choose a reason for hiding this comment

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

I had a comment pointing out that format_task could have been used here, and while that's true, this could have been a downside to my suggestion that you combine format_task_goal when the Task returned contains a "goal_id" key (and thus possibly fail a test not expecting that key).

This could justify a separate format_task_goal... However, in most cases, we would still expect the "goal_id" key to be returned when doing this update. It's not so much that we have a reason to not show that key as part of the task, it's more that we didn't have that key during the earlier waves, but now we do.

Either way, I still think the cleanest way I would implement is still handling in my suggested format_task, as well as using it as a helper method here. In that way, format_task is defining what format we always return a task in, and that stays the same across endpoints.

Choose a reason for hiding this comment

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

The use of format_task also applies to update_task_as_incomplete down below.

}
)


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

task.completed_at = None

db.session.commit()

return make_response(
{
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": (bool(task.completed_at)),
}
}
)
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