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
@@ -1,4 +1,6 @@
from flask import Flask
from .routes.task_routes import bp as task_bp
from .routes.goal_routes import bp as goal_bp
from .db import db, migrate
from .models import task, goal
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove unused imports

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

# Register Blueprints here
app.register_blueprint(task_bp)
app.register_blueprint(goal_bp)

return app
26 changes: 25 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,29 @@
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")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 A goal has many tasks


def to_dict(self, include_tasks=False, include_task_ids = False, include_goal_id_in_tasks = False):
goal_as_dict = {"id": self.id,
"title": self.title,
}

if include_tasks:
goal_as_dict["tasks"] = []
if self.tasks and include_tasks:
goal_as_dict["tasks"] = [task.to_dict(include_goal_id = include_goal_id_in_tasks) for task in self.tasks]
elif include_task_ids and self.tasks:
goal_as_dict["task_ids"] = [task.id for task in self.tasks]

return goal_as_dict
Comment on lines +12 to +24
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see that you include these default params so that you can conditionally include tasks and task ids in the dictionary that to_dict returns.

I do like that you handle these scenarios in the to_dict method so that this logic doesn't leak out into the route.

However, we can simplify this method by accessing these values from self, which is an instance of goal. Since goal knows what tasks are associated with it, we don't need to pass these boolean values into the function.

Suggested change
def to_dict(self, include_tasks=False, include_task_ids = False, include_goal_id_in_tasks = False):
goal_as_dict = {"id": self.id,
"title": self.title,
}
if include_tasks:
goal_as_dict["tasks"] = []
if self.tasks and include_tasks:
goal_as_dict["tasks"] = [task.to_dict(include_goal_id = include_goal_id_in_tasks) for task in self.tasks]
elif include_task_ids and self.tasks:
goal_as_dict["task_ids"] = [task.id for task in self.tasks]
return goal_as_dict
def to_dict(self):
goal_as_dict = {"id": self.id,
"title": self.title,
}
if self.tasks:
goal_as_dict["tasks"] = [task.to_dict(include_goal_id = include_goal_id_in_tasks) for task in self.tasks]
goal_as_dict["task_ids"] = [task.id for task in self.tasks]
else:
goal_as_dict["tasks"] = []
return goal_as_dict


@classmethod
def from_dict(cls, goal_data):
new_goal = cls(title = goal_data["title"])
return new_goal
Comment on lines +28 to +29
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You could directly return the object since you create the variable new_goal and then immediately return it without accessing it.

Suggested change
new_goal = cls(title = goal_data["title"])
return new_goal
return cls(title = goal_data["title"])

33 changes: 32 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,36 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from sqlalchemy import ForeignKey
from ..db import db
from datetime import datetime
from typing import Optional
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[datetime | None]
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
Comment on lines +14 to +15
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You use two different styles of syntax for declaring an attribute as optional that essentially get treated the same by SQLAlchemy under the hood: [type|None] and Mapped[Optional[type]]. Would prefer that you stick to one style for consistency.

goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks")

def to_dict(self, include_goal_id = False):
task_as_dict = {"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": True if self.completed_at != None else False}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice job computing this value. We can simplify it even more:

Suggested change
"is_complete": True if self.completed_at != None else False}
"is_complete": self.completed_at is not None

if include_goal_id and self.goal_id is not None:
task_as_dict["goal_id"] = self.goal_id
Comment on lines +18 to +24
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same as my comment above, we can get away with not passing any additional arguments into to_dict by relying on self to figure out if we need to conditionally include goal_id in task_as_dict.

Suggested change
def to_dict(self, include_goal_id = False):
task_as_dict = {"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": True if self.completed_at != None else False}
if include_goal_id and self.goal_id is not None:
task_as_dict["goal_id"] = self.goal_id
def to_dict(self):
task_as_dict = {"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": True if self.completed_at != None else False}
if self.goal_id:
task_as_dict["goal_id"] = self.goal_id


return task_as_dict

@classmethod
def from_dict(cls, task_data):
new_task = cls(title = task_data["title"],
description = task_data["description"],
completed_at = None if ("is_complete" not in task_data
or task_data["is_complete"] is False)
else cls.completed_at)
Comment on lines +32 to +34
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The readability is a little compromised when everything is jammed inline like this.

This logic could be simplified by also using a ternary like this:

Suggested change
completed_at = None if ("is_complete" not in task_data
or task_data["is_complete"] is False)
else cls.completed_at)
completed_at = datetime.now() if task_data.get("is_complete") else None

This checks if the task data has a field has valid k/v pairs for is_complete and if it does, then it sets the completed_at to be the current date time. If is_complete is not a truthy value, then completed_at is set to None.

return new_task

69 changes: 68 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,68 @@
from flask import Blueprint
from flask import Blueprint, abort, make_response, request, Response
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

abort, make_response are imported but not accessed so they should be removed

Suggested change
from flask import Blueprint, abort, make_response, request, Response
from flask import Blueprint, request, Response

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

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

@bp.post("")
def create_goal():
request_body = request.get_json()
return create_model(Goal, request_body)

@bp.get("")
def get_all_goals():
return get_models_by_order(Goal)

@bp.get("/<goal_id>")
def get_one_goal(goal_id):
goal = validate_model(Goal, goal_id)
goal_dict = {"id": goal.id, "title": goal.title}

return goal_dict
Comment on lines +24 to +26
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would prefer you use the to_dict instance method that you wrote in goal.py.

Suggested change
goal_dict = {"id": goal.id, "title": goal.title}
return goal_dict
return goal_dict.to_dict()


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

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

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

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

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

@bp.post("/<goal_id>/tasks")
def creat_tasks_for_goal(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()
task_id_list = request_body.get("task_ids", [])

for task_obj in list(goal.tasks):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We don't need to cast goal.tasks to type list in order to iterate over them

Suggested change
for task_obj in list(goal.tasks):
for task_obj in goal.tasks:

task_obj.goal = None

for task_id in task_id_list:
task = validate_model(Task, task_id)
task.goal = goal
Comment on lines +52 to +57
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This works! However, you might find it to be more concise to invert the logic here though. Instead of getting every goal's task and setting each task's goal id to be None, you could just re-assign the tasks that a goal has:

Suggested change
for task_obj in list(goal.tasks):
task_obj.goal = None
for task_id in task_id_list:
task = validate_model(Task, task_id)
task.goal = goal
goal.tasks = [validate_model(Task, id) for id in task_id_list]

db.session.commit()

response = {"id": goal.id, "task_ids": task_id_list}

return response, 200
Comment on lines +60 to +62
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Prefer that you use the to_dict method that you wrote in goal.py.

Also, Flask will send a 200 status code by default so we can leave it off here.

Suggested change
response = {"id": goal.id, "task_ids": task_id_list}
return response, 200
return goal.to_dict()


@bp.get("/<goal_id>/tasks")
def get_tasks_for_one_goal(goal_id):
goal = validate_model(Goal, goal_id)
response = goal.to_dict(include_tasks = True, include_goal_id_in_tasks = True)
return response
66 changes: 66 additions & 0 deletions app/routes/route_utilities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
from flask import abort, make_response, Response, request
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove unused imports

Suggested change
from flask import abort, make_response, Response, request
from flask import abort, make_response, request

from ..db import db
import os
import requests

def validate_model(cls, model_id):
try:
model_id = int(model_id)
except:
response = {"details": "Invalid data"}
abort(make_response(response, 400))

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

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

return model

def get_models_by_order(cls, filters=None):
query = db.select(cls)

sort_param = request.args.get('sort', None)

if sort_param == "desc":
query = query.order_by(cls.title.desc())
elif sort_param == "asc":
query = query.order_by(cls.title.asc())

models = db.session.scalars(query)

model_response = []
for model in models:
model_dict = model.to_dict()
model_response.append(model_dict)

return model_response
Comment on lines +34 to +39
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would be nice to use list comprehension, which is very Pythonic, to make this method more concise.

Suggested change
model_response = []
for model in models:
model_dict = model.to_dict()
model_response.append(model_dict)
return model_response
return [model.to_dict() for model in models]


def create_model(cls, model_data):
try:
new_model = cls.from_dict(model_data)

except KeyError as error:
response = {"details": "Invalid data"}
abort(make_response(response, 400))

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

return new_model.to_dict(), 201

def generate_slack_notification(model_attribute):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like that you create a helper function for this logic!

path = "https://slack.com/api/chat.postMessage"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

constant variables should be named with all caps

Suggested change
path = "https://slack.com/api/chat.postMessage"
PATH = "https://slack.com/api/chat.postMessage"

SLACK_TOKEN = os.environ.get("SLACK_BOT_TOKEN")

headers = {
"Authorization": f"Bearer {SLACK_TOKEN}"
}
json = {
"channel": "task-notifications",
Comment on lines +61 to +62
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The channel name appears here as a magic string should be avoided by using a constant variable instead.

Suggested change
json = {
"channel": "task-notifications",
CHANNEL_NAME = "task-notifications"
json = {
"channel": CHANNEL_NAME,

"text": f"Someone just completed the task {model_attribute}"
}

slack_response = requests.post(path, headers=headers, json=json)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You create the variable slack_response bud don't return it from the funciton.

Do you need this value and mean to return it or do we not need to return anything at all?

Suggested change
slack_response = requests.post(path, headers=headers, json=json)
requests.post(path, headers=headers, json=json)

65 changes: 64 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,64 @@
from flask import Blueprint
from flask import Blueprint, abort, make_response, request, Response
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remove unused imports

from ..db import db
from app.models.task import Task
from .route_utilities import validate_model, create_model, get_models_by_order, generate_slack_notification
from datetime import datetime


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

@bp.post("")
def create_task():
request_body = request.get_json()
return create_model(Task, request_body)

@bp.get("")
def get_all_tasks():
return get_models_by_order(Task)


@bp.get("<task_id>")
def get_one_task(task_id):
task = validate_model(Task, task_id)
return task.to_dict(include_goal_id = True)

@bp.put("/<task_id>")
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"]
db.session.commit()

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

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

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


@bp.patch("/<task_id>/mark_complete")
def patch_complete_task(task_id):
task = validate_model(Task, task_id)
task.completed_at = datetime.now()
db.session.commit()
generate_slack_notification(task.title)

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


@bp.patch("/<task_id>/mark_incomplete")
def patch_incomplete_task(task_id):
task = validate_model(Task, 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