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
4 changes: 4 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
from .db import db, migrate
from .models import task, goal
import os
from .routes.task_routes import bp as tasks_bp
from .routes.goal_routes import bp as goals_bp
Comment on lines +5 to +6

Choose a reason for hiding this comment

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

👍


def create_app(config=None):
app = Flask(__name__)
Expand All @@ -18,5 +20,7 @@ def create_app(config=None):
migrate.init_app(app, db)

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

return app
21 changes: 20 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,24 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from ..db import db
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from .task import Task


class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]
tasks: Mapped[list["Task"]] = relationship(back_populates="goal")

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

@classmethod
def from_dict(cls, goal_data):
return cls(
title=goal_data["title"],
tasks=goal_data.get("tasks", [])
)
32 changes: 31 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,35 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from sqlalchemy import ForeignKey
from ..db import db
from typing import Optional
from datetime import datetime
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from .goal import Goal


class Task(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]
description: Mapped[str]
completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True)

Choose a reason for hiding this comment

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

Using Optional[] is all that we need to indicate that a field is nullable so we don't need to also include mapped_column with nullable=True.

Here's the SQLAlchemy documentation about nullability

image
Suggested change
completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True)
completed_at: Mapped[Optional[datetime]]

goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks")

def to_dict(self):
return {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": self.completed_at is not None,
**({"goal_id": self.goal_id} if self.goal_id is not None else {})
}

@classmethod
def from_dict(cls, task_data):
return cls(
title=task_data["title"],
description=task_data["description"],
completed_at=task_data.get("completed_at"),
goal_id=task_data.get("goal_id")
Comment on lines +31 to +34

Choose a reason for hiding this comment

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

Lines 31-32 you use square bracket notation for getting values and lines 33-34 you use get. Either way works, but get allows you to pass a default value if a key is not in the dictionary, which might be something you can leverage if needed.

Prefer that you pick one way and consistently use it throughout the project.

)
84 changes: 83 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,83 @@
from flask import Blueprint
from flask import Blueprint, request, Response, jsonify

Choose a reason for hiding this comment

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

Suggested change
from flask import Blueprint, request, Response, jsonify
from flask import Blueprint, request, Response

We do not need to use jsonify in our API because Flask will convert a list or dictionary into JSON that we send back as a response.

There are times when we need to create a Response object manually in order to set the mimetype to JSON and the status code to 204 (for our routes that don't return a response body, see line 44 in the PUT route in this file).

Anywhere you use jsonify should be removed.

from ..db import db
from app.models.goal import Goal
from app.models.task import Task
from .route_utilities import validate_model, create_model

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


@bp.post("")
def create_goal():
request_body = request.get_json()
goal_data, status_code = create_model(Goal, request_body)
return jsonify({"goal": goal_data}), status_code


@bp.get("")
def get_all_goals():
query = db.select(Goal)

title_param = request.args.get("title")
if title_param:
query = query.where(Goal.title.ilike(f"%{title_param}%"))

goals = db.session.scalars(query)

return [goal.to_dict() for goal in goals]


@bp.get("/<id>")
def get_one_goal(id):
goal = validate_model(Goal, id)
return jsonify({"goal": goal.to_dict()}), 200

Choose a reason for hiding this comment

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

Suggested change
return jsonify({"goal": goal.to_dict()}), 200
return {"goal": goal.to_dict()}

Per my comment above, we should remove jsonify since we don't need it.

Also, anywhere you return status code 200 can be removed since that's the default status code Flask will return.



@bp.put("<id>")
def update_goal(id):
goal = validate_model(Goal, id)
request_body = request.get_json()

goal.title = request_body["title"]

db.session.commit()
return Response(status=204, mimetype="application/json")


@bp.delete("<id>")
def delete_goal(id):
goal = validate_model(Goal, id)
db.session.delete(goal)
db.session.commit()

return Response(status=204, mimetype="application/json")


@bp.post("/<id>/tasks")
def create_task_for_goal(id):
goal = validate_model(Goal, id)
request_body = request.get_json()

task_ids = request_body.get("task_ids", [])

for task in goal.tasks:
task.goal_id = None

for task_id in task_ids:
task = validate_model(Task, task_id)
task.goal_id = goal.id

db.session.commit()

return {
"id": goal.id,
"task_ids": task_ids
}, 200


@bp.get("/<id>/tasks")
def get_tasks_for_goal(id):
goal = validate_model(Goal, id)
goal_dict = goal.to_dict()
goal_dict["tasks"] = [task.to_dict() for task in goal.tasks]
return goal_dict, 200

Choose a reason for hiding this comment

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

There are several routes where you return status code 200. In all those routes, we can safely remove this from all the routes in your route files because Flask will return a status code of 200 by default.

Suggested change
return goal_dict, 200
return goal_dict

31 changes: 31 additions & 0 deletions app/routes/route_utilities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from flask import abort, make_response
from ..db import db

def validate_model(cls, id):
try:
id = int(id)
except ValueError:
response = {"details": f"{cls.__name__} {id} invalid"}
abort(make_response(response, 400))

query = db.select(cls).where(cls.id == id)
model = db.session.scalar(query)

if not model:
response = {"details": f"{cls.__name__} {id} not found"}
abort(make_response(response, 404))

return model


def create_model(cls, model_data):
try:
new_model = cls.from_dict(model_data)
except KeyError as e:
response = {"details": "Invalid data"}

Choose a reason for hiding this comment

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

It might be nice to provide even more details in the response you send back so that the client knows how to fix up the request body they will re-send.

Suggested change
response = {"details": "Invalid data"}
response = {"details": f"Invalid data for field: {error.args[0]}"}

For example, this would evaluate to "title" for example if I sent a bad request body to create a Task.

abort(make_response(response, 400))

db.session.add(new_model)
db.session.commit()

return new_model.to_dict(), 201

Choose a reason for hiding this comment

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

Nice, no of jsonify here because we don't need it. A dictionary will be converted to JSON by Flask for us without invoking the method.

100 changes: 99 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,99 @@
from flask import Blueprint
from flask import Blueprint, request, Response, jsonify

Choose a reason for hiding this comment

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

Suggested change
from flask import Blueprint, request, Response, jsonify
from flask import Blueprint, request, Response

See my comments in goal_routes.py about not needing to use jsonify at all in our Flask application as it's designed.

Therefore, we can safely remove jsonify in all parts of this file.

from ..db import db
from app.models.task import Task
from .route_utilities import validate_model, create_model
from datetime import datetime
import requests, os


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


@bp.post("")
def create_task():
request_body = request.get_json()
task_data, status_code = create_model(Task, request_body)
return jsonify({"task": task_data}), status_code


@bp.get("")
def get_all_tasks():
query = db.select(Task)

title_param = request.args.get("title")
if title_param:
query = query.where(Task.title.ilike(f"%{title_param}%"))

description_param = request.args.get("description")
if description_param:
query = query.where(Task.description.ilike(f"%{description_param}%"))

sort_param = request.args.get("sort")
if sort_param == "desc":
query = query.order_by(Task.title.desc())
else:
query = query.order_by(Task.title.asc())

tasks = db.session.scalars(query)

return [task.to_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 job directly returning a list and not using jsonify. The newer version of Flask we use, under the hood, is able to convert a list or a dictionary into JSON without needing us to call jsonify on either the list or dictionary.



@bp.get("/<id>")
def get_one_task(id):
task = validate_model(Task, id)
return jsonify({"task": task.to_dict()}), 200

Choose a reason for hiding this comment

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

Recall that the default status code that Flask sends back is 200 so we don't need to explicitly return it here.

Suggested change
return jsonify({"task": task.to_dict()}), 200
return {"task": task.to_dict()}

Notice on line 39 above that you don't return any status code. Flask will return status code 200 by default there as well.



@bp.put("/<id>")
def update_task(id):
task = validate_model(Task, id)
request_body = request.get_json()

task.title = request_body["title"]
task.description = request_body["description"]
Comment on lines +53 to +54

Choose a reason for hiding this comment

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

Either of these lines could throw unhandled exceptions if the client's request body doesn't have title or description. How could you update the logic to add some error handling so if you have a bad request body, you could send back an appropriate response and status code?


db.session.commit()
return Response(status=204, mimetype="application/json")


@bp.delete("/<id>")
def delete_task(id):
task = validate_model(Task, id)
db.session.delete(task)
db.session.commit()

return Response(status=204, mimetype="application/json")


@bp.patch("/<id>/mark_complete")
def mark_complete(id):
task = validate_model(Task, id)
if task.completed_at is None:
task.completed_at = datetime.now()
db.session.commit()

slack_bot_token = os.environ.get("SLACK_BOT_TOKEN")
slack_channel = os.environ.get("SLACK_CHANNEL", "task-notifications")
Comment on lines +76 to +77

Choose a reason for hiding this comment

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

These two values are constant and we should indicate that by using constant variables with all capital letters.

Suggested change
slack_bot_token = os.environ.get("SLACK_BOT_TOKEN")
slack_channel = os.environ.get("SLACK_CHANNEL", "task-notifications")
SLACK_BOT_TOKEN = os.environ.get("SLACK_BOT_TOKEN")
SLACK_CHANNEL = os.environ.get("SLACK_CHANNEL", "task-notifications")


headers = {
"Authorization": f"Bearer {slack_bot_token}"
}

payload = {
"channel": slack_channel,
"text": f"Someone just completed the task {task.title}"
}

requests.post("https://slack.com/api/chat.postMessage", headers=headers, json=payload)
Comment on lines +76 to +88

Choose a reason for hiding this comment

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

Prefer this logic to live in a helper function, maybe something like call_slack_api, to keep this route more concise and single-responsibility.


return Response(status=204, mimetype="application/json")


@bp.patch("/<id>/mark_incomplete")
def mark_incomplete(id):
task = validate_model(Task, id)
task.completed_at = None
db.session.commit()

return Response(status=204, mimetype="application/json")
1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Single-database configuration for Flask.
50 changes: 50 additions & 0 deletions migrations/alembic.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# 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,flask_migrate

[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

[logger_flask_migrate]
level = INFO
handlers =
qualname = flask_migrate

[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