Skip to content

C22 Phoenix - Luqi Xie #29

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: main
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
5 changes: 5 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
from flask import Flask
from .db import db, migrate
from .models import task, goal

Choose a reason for hiding this comment

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

Once we have other import paths to our models (here, through the blueprints, which import the models), we technically don't need the imports here any more. It's fine to leave them for clarity (and a reminder that any other models we add would need to be included until other routes are setup), but if the VS Code warning is bothersome, feel free to remove these.

from .routes.task_routes import tasks_bp
from .routes.goal_routes import goals_bp
Comment on lines +4 to +5

Choose a reason for hiding this comment

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

We would prefer to name the task_bp and goal_bp just bp in their respective route files, and then either do an import as here to differentiate them, or import each entire route module and access its Blueprint using dor notation.

import os

def create_app(config=None):
Expand All @@ -18,5 +20,8 @@ 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

24 changes: 23 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,27 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from ..db import db
from typing import List, 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):
goal_to_dict = {
"id": self.id,
"title": self.title,
}

if self.tasks:
goal_to_dict["tasks"] = [task.to_dict() for task in self.tasks]
Comment on lines +18 to +19

Choose a reason for hiding this comment

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

Note that the cases that return task data are more related to the route than to whether the goal has tasks. There's a nested endpoint that the client is supposed to access if they want to retrieve the list of tasks for a goal. So rather than adding this conditional logic to the main to_dict method, we could make an additional dictionary conversion routine that uses the main to_dict to generate the scalar data, but then also adds the task data.

The distinction between adding goal details to tasks, but not task details to goal may seem arbitrary, and to a certain degree it is. When designing our own APIs, it's really up to us, and can be influenced largely by how many child records we expect there to typically be. If the main record (here, goal) had many properties, and often had many many tasks, we'd likely want to give the caller a way to retrieve the goal properties without getting all the task details at the same time, which could significantly increase the size of the response. On the other side, since a Task only ever has 1 goal, and even then we're only returning the goal id, conditionally adding that value doesn't add significantly to the overall size of the response.

So that's one justification for always including goal id data in the task response (if it has any), but not including task data in the goal response, unless specifically requested through the nested route.


return goal_to_dict

@classmethod
def from_dict(cls, goal_data):
new_goal = cls(title=goal_data["title"])
return new_goal

34 changes: 33 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,37 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from sqlalchemy import DateTime, ForeignKey
from datetime import datetime
from ..db import db
from typing import Optional
from typing import TYPE_CHECKING
if TYPE_CHECKING:
from .goal import Goal
Comment on lines +6 to +8

Choose a reason for hiding this comment

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

Nice inclusion of this extra check to clear up the type warnings in VS Code.


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(DateTime, nullable=True, default=None)

Choose a reason for hiding this comment

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

Because of the Optional[datetime] marker, we don't need to also set the column type, or that it's nullable. Further, for nullable columns, their default value is automatically NULL (None). So we don't need the mapped_column at all.

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


def to_dict(self):
task_to_dict = {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": self.completed_at is not None
}
if self.goal_id:
task_to_dict["goal_id"] = self.goal_id
Comment on lines +26 to +27

Choose a reason for hiding this comment

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

Nice detection of goal membership to detemine whether to include the goal information.


return task_to_dict

@classmethod
def from_dict(cls, task_data):
return cls(
title=task_data["title"],
description=task_data["description"]

)
Empty file added app/routes/__init__.py

Choose a reason for hiding this comment

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

Nice job remembering to create the __init__.py. Even if things appear to work without it, there are corner cases where it can become a problem, so we should always make it.

Empty file.
85 changes: 84 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,84 @@
from flask import Blueprint
from flask import Blueprint, abort, make_response, request, Response
from app.models.goal import Goal
from app.models.task import Task
from .route_utilities import validate_model, create_model
from ..db import db

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

@goals_bp.post("")
def create_goal():
request_body = request.get_json()
return {"goal": create_model(Goal, request_body)}, 201


@goals_bp.get("")
def get_all_tasks():
query = db.select(Goal)
goals = db.session.scalars(query)

goals_response = []
for goal in goals:
goals_response.append(
{
"id": goal.id,
"title": goal.title
}
Comment on lines +23 to +26

Choose a reason for hiding this comment

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

Prefer to use the to_dict method here.

)
return goals_response

@goals_bp.get("/<goal_id>")
def get_single_goal(goal_id):
goal = validate_model(Goal, goal_id)

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

@goals_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"]

Choose a reason for hiding this comment

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

Like for Task, we might want similar error handling here as for the POST route.


db.session.commit()

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

@goals_bp.delete("/<goal_id>")
def delete_goal(goal_id):
goal = validate_model(Goal, goal_id)

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

response = {"details": f'Goal {goal.id} "{goal.title}" successfully deleted'}
return response, 200


@goals_bp.post("/<goal_id>/tasks")
def create_task_with_goal_id(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()

task_ids = request_body["task_ids"]
for task_id in task_ids:
task = validate_model(Task, task_id)
goal.tasks.append(task)

Choose a reason for hiding this comment

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

This logic appends each task to the current goal, so any pre-existing tasks would still be associated with the goal. In this case, we'd want the result to show all of the tasks now associated with the goal. See below.


db.session.commit()

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

Choose a reason for hiding this comment

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

Rather than echoing back the input task ids, since we appended the new tasks to the existing goal, we should fetch the task ids directly from the goal, in case there were any other tasks already associated with this goal. Something like this

        "task_ids": [task.id for task in goal.tasks]}

return response, 200

@goals_bp.get("/<goal_id>/tasks")
def get_tasks_by_goal(goal_id):
goal = validate_model(Goal, goal_id)

response = {
"id": goal.id,
"title": goal.title,
"tasks": [task.to_dict() for task in goal.tasks]

Choose a reason for hiding this comment

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

Since this is the only route that includes the child tasks, and it's being explicitly added here, that would allow us to remove the logic to do some from to_dict, leaving that to build a dictionary of only the scalar values of Goal. We could still use to_dict here, and then add in the tasks collection that we generate.

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


def validate_model(cls, model_id):
try:
model_id = int(model_id)
except:
response = {"message": f"{cls.__name__} {model_id} invalid"}
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 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()


108 changes: 107 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,107 @@
from flask import Blueprint
import os
from flask import Blueprint, abort, make_response,request, Response
from app.models.task import Task
from ..db import db
from datetime import datetime
from .route_utilities import validate_model, create_model
import requests

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

@tasks_bp.post("")
def create_task():
request_body = request.get_json()
return {"task": create_model(Task, request_body)}, 201

# try:
# new_task = Task.from_dict(request_body)

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

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

# response = new_task.to_dict()
# return {"task": response}, 201
Comment on lines +16 to +27

Choose a reason for hiding this comment

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

👀 Remove code that's no longer being used.


@tasks_bp.patch("/<task_id>/mark_incomplete")
def mark_incomplete_task(task_id):
task = validate_model(Task, task_id)
task.completed_at = None

db.session.commit()

response = task.to_dict()

return {"task": response}, 200

@tasks_bp.patch("/<task_id>/mark_complete")
def mark_task_complete(task_id):
task = validate_model(Task, task_id)
task.completed_at = datetime.now()

Choose a reason for hiding this comment

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

datetime values are surpisingly difficult to work with correctly. This is why we only ask for a vlaue to be stored to mark completion, with the lack of a value indiciation incomplete. Especially for apps that might be storing information from remote users from around the world, a common approach for storing datetimes is to store timezone-aware data in the UTC timezone, then leave it to the local client to transform the value to the local timezone for display. This requires both using timezone-aware storage in the database, as well as timezone-aware Python calls. It's definitely worth looking into for future work!


db.session.commit()

response = task.to_dict()
url = "https://slack.com/api/chat.postMessage"
headers = {"Authorization": f"Bearer {os.environ.get('AUTHORIZATION_TOKEN')}"}
data = {
"channel": "task-notification",
"text": f"Someone just completed the task {task.title}"
}

slack_response = requests.post(url, json=data, headers=headers)
Comment on lines +48 to +55

Choose a reason for hiding this comment

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

Consider moving the logic that's specific to sending a Slack message to a helper function.

return {"task": response}, 200


@tasks_bp.get("")
def get_all_tasks():
sort_title_param = request.args.get("sort", "asc")

if sort_title_param == "desc":
tasks = Task.query.order_by(Task.title.desc())
else:
tasks = Task.query.order_by(Task.title.asc())
Comment on lines +61 to +66

Choose a reason for hiding this comment

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

Notice that even if no sorting is requested, this will always sort by title. When no particular sorting option is supplied, consider falling back to the id itself.


task_response = [task.to_dict() for task in tasks]

return task_response, 200

@tasks_bp.get("/<task_id>")
def get_single_task(task_id):
task = validate_model(Task, task_id)

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


@tasks_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"]
Comment on lines +84 to +85

Choose a reason for hiding this comment

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

👀 Notice that we could have similar KeyError behavior here as when creating a new Task. Consider including similar error handling as exists in create_model to catch the KeyError and report the error more nicely. Currently, a missing key would result in a 500 internal error (a crash).


if request_body.get("is_complete"):
task.completed_at = datetime.now()
else:
task.completed_at = None
Comment on lines +87 to +90

Choose a reason for hiding this comment

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

Typically, the input expected for a PUT will be identical to what was required for a POST, in this case title and description. We have a custom routes exposed for working with the completion status.


db.session.commit()

response = {"task": task.to_dict()}
return response, 200
# return Response(status=200, mimetype="application/json")

@tasks_bp.delete("/<task_id>")
def delete_task(task_id):
task = validate_model(Task, task_id)

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

response = {"details": f'Task {task.id} "{task.title}" successfully deleted'}
return response, 200

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