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
11 changes: 9 additions & 2 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,19 @@
from flask import Flask
from .db import db, migrate
from .models import task, goal
from .routes.task_routes import task_bp
from .routes.goal_routes import goal_bp
import os


def create_app(config=None):
app = Flask(__name__)

app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI')
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get(
'SQLALCHEMY_DATABASE_URI')
# app.config['SLACK_BOT_TOKEN'] = os.environ.get("SLACK_BOT_TOKEN")
# app.config['SLACK_CHANNEL'] = os.environ.get("SLACK_CHANNEL")

if config:
# Merge `config` into the app's configuration
Expand All @@ -18,5 +24,6 @@ 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
18 changes: 17 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,21 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
# from sqlalchemy import ForeignKe
from typing import List
from ..db import db


class Goal(db.Model):

Choose a reason for hiding this comment

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

👍 Your Goal implementation is consistent with your Task model. Check the Task feedback for anything that could apply here.

👀 Note the comment below about the to_dict as well.

id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str] = mapped_column(nullable=False)
tasks: Mapped[List["Task"]] = relationship("Task", back_populates="goal")

def to_dict(self):
return {
"id": self.id,
"title": self.title,
# "tasks": [task.to_dict() for task in self.tasks]

Choose a reason for hiding this comment

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

It would be preferable to remove the unnecessary line rather than commenting it out.

In professional code leaving commented code invites questions about why the code is commented, and whether it should be reinstated every time someone comes across it. Keep your code clean.

}

@classmethod
def from_dict(cls, goal_data):
return cls(title=goal_data["title"])
43 changes: 42 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,46 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from datetime import datetime
from typing import Optional
from sqlalchemy import ForeignKey
# from sqlalchemy import Datetime
from ..db import db


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

Choose a reason for hiding this comment

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

Using the Mapped approach, columns aren't nullable by default. We can leave off the nullable=False on columns for which we want to require a value. If nullable was the only option being passed to mapped_column, the entire mapped_column can be omitted!

goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
goal: Mapped[Optional["Goal"]] = relationship(
"Goal", back_populates="tasks")
description: Mapped[str] = mapped_column(nullable=False)
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 the Mapped approach, we can obtain nullable columns by marking them Optional. So these columns that are Optional don't need to have the nullable option. In fact, we would prefer to omit the nullable, since that leads to the potential for them being out of sync. For nullable columns, just use Optional, and non-nullable columns should not use Optional.


def to_dict(self, include_completed_at=False, include_goal_id=False):

Choose a reason for hiding this comment

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

👍 Nice idea to include an optional parameter allowing the inclusion of completed_at if required. We didn't show customizing to_dict in the curriculum materials, but something like this can work very well, as can including separate variations of to_dict to be called in certain contexts.

In larger applications where there may need to be many "rendered" forms of the record types, the problem of converting from model instances to a rendered form may be delegated to an entirely separate group of classes, whose sole responsibility is generating output formats from that model type.

task_dict = {
"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 else None, # Default to None

Choose a reason for hiding this comment

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

Same here, just remove it.

# "goal_title": self.goal.title if self.goal else None # Default to None

}
if include_goal_id and self.goal_id:

Choose a reason for hiding this comment

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

This works as written (with you explicitly passing in True for include_goal_id in a few places). But the intended behavior was to conditionally include the goal id in any Task output if it belongs to a goal, with no need to "turn it on" in certain routes.

The reason the author might feel like this is OK for goal info, but not for task info (on goals) is that the goal info is a single value, while the task info is a whole additional collection of unknown size.

Personally, I disagree with the goal_id coming and going (I would have always included the goal_id with a None value if it didn't belong to a Goal rather than modifying the "shape" of a Task response), but hey, a spec is a spec. 🤷

task_dict["goal_id"] = self.goal_id
return task_dict

# if include_completed_at:
# task_dict["completed_at"] = self.completed_at

# if include_goal_id and self.goal:
# task_dict["goal_id"] = self.goal.id
# task_dict["goal_title"] = self.goal.title
# return task_dict

@classmethod
def from_dict(cls, task_data):
return cls(
title=task_data["title"],
description=task_data["description"],
Comment on lines +43 to +44

Choose a reason for hiding this comment

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

👍 By reading title and description directly as keys we can trigger a KeyError if they are absent, giving us a way to indicate they are required.

completed_at=task_data.get("completed_at")
)
93 changes: 92 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,92 @@
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 ..db import db
from .route_utilities import validate_model, create_model

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


@goal_bp.get("")
def get_all_goals():

Choose a reason for hiding this comment

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

👍 Your Goal CRUD routes are consistent with your Task CRUD routes. Check the Task feedback for anything that could apply here.

query = db.select(Goal)
goals = db.session.scalars(query)
goal_response = [goal.to_dict() for goal in goals]

return goal_response


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


@goal_bp.post("")
def create_goal():
request_body = request.get_json()

if not request_body.get("title"):
return {"details": "Invalid data"}, 400

new_goal = Goal.from_dict(request_body)

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

return {"goal": new_goal.to_dict()}, 201


@goal_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")


@goal_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")


@goal_bp.get("/<id>/tasks")
def get_all_goal_tasks(id):
goal = validate_model(Goal, id)
tasks = [task.to_dict(include_goal_id=True) for task in goal.tasks]

return {
"id": goal.id,
"title": goal.title,
Comment on lines +64 to +65

Choose a reason for hiding this comment

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

Notice that the id and title keys here are the same as for a regular Goal GET, for which we wrote the to_dict helper. Here, we could use our to_dict to get the regular result dictionary, and then add in the tasks key with the loaded task data.

"tasks": tasks
}, 200


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

task_ids = request_body.get("task_ids")
if not task_ids or not isinstance(task_ids, list):
return {"message": "'task_ids' field is required and must be a list"}, 400

# Query all tasks by the given IDs
tasks = Task.query.filter(Task.id.in_(task_ids)).all()

Choose a reason for hiding this comment

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

👀 Task.query is deprecated and should no longer be used. Instead, we could validate all of the tasks in a list comprehension:

    tasks = [get_model_by_id(Task, task_id) for task_id in task_ids]


if len(tasks) != len(task_ids):
return {"message": "One or more task_ids are invalid"}, 404

# Replace the tasks associated with the goal
goal.tasks = tasks

Choose a reason for hiding this comment

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

👍 Setting the task collection to a new list of Task models taks care of unsetting any previous tasks associated with the goal, and updates the new Tasks, all in one little assignment.

db.session.commit()

return {
"id": goal.id,
"task_ids": [task.id for task in tasks]
}, 200
56 changes: 56 additions & 0 deletions app/routes/route_utilities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
from flask import abort, make_response, jsonify
import requests
from ..db import db
import os


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


def validate_model(cls, modelid):
try:
model_id = int(modelid)
except ValueError:
invalid_response = {
"message": f"{cls.__name__} id ({model_id}) is invalid."}
abort(make_response(invalid_response, 400))

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

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


def create_model(cls, model_data):
try:
new_model = cls.from_dict(model_data)
except KeyError as e:
response = {"message": f"Invalid request: missing {e.args[0]}"}
abort(make_response(response, 400))

db.session.add(new_model)
db.session.commit()
return new_model.to_dict(), 201


def send_slack_message(task_title):

Choose a reason for hiding this comment

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

This function encapsulates the responsibility of sending a completion notification about the provided task title. Notice that we could make a further helper function that wraps the responsibility of sending a general message to a specified channel. This function would then be responsible for the logic of building the messaging, and knowing what channel to use.

Even the logic to build the notification message based on the task could be in its own helper. Think about whether such a potential function would be a model method, or some other method to which we pass a Task.

url = "https://slack.com/api/chat.postMessage"

Choose a reason for hiding this comment

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

We could store the Slack API URL in the .env too. If for some reason, there was a change to the API endpoint, we could then update where the endpoint looked for it without needing to update the code itself.


headers = {
"Authorization": f"Bearer {SLACK_BOT_TOKEN}",

Choose a reason for hiding this comment

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

👍 The Slack API documentation prefers passing the API key in the header rather than in the request body.

Since we're passing the body as JSON (by using the json= named parameter) Slack wouldn't even accept the token if passed in the body, so including it in the header allows our preferred style of API call, as well as following Slack's preferred approach.

"Content-Type": "application/json; charset=UTF-8"
}
payload = {
"channel": SLACK_CHANNEL,
"text": f"Someone just completed the task {task_title}"

}
response = requests.post(url, headers=headers, json=payload)

if not response.ok or not response.json().get("ok"):
print("Failed to send slack message:", response.text)
Comment on lines +55 to +56

Choose a reason for hiding this comment

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

Nice check for whether the API call succeeded. Since the notification isn't core to the act of updating our task status, if an error occurs we wouldn't want the entire route to fail, so logging a message is something we can do.

In a full implementation, we probably wouldn't have the notification happen directly as part of the route. Instead, we would schedule a task to try to send the notification, potentially with retry logic, and logging. Note that Flask has its own logging subsystem that we would prefer to use over a regular print, but this is fine for now. The main thing is to still allow the update to complete.

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

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


@task_bp.delete("/<id>")
def delete_task(id):
task = validate_model(Task, id)
db.session.delete(task)
db.session.commit()
Comment on lines +12 to +14

Choose a reason for hiding this comment

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

Notice how similar deleting is to getting. After validating the record to delete, we delete and commit it, then return a common response. How could we add model or route helpers to simplify our DELETE route?

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


@task_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"]
db.session.commit()
Comment on lines +22 to +24

Choose a reason for hiding this comment

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

Notice how similar updating is to creating. After validating the record to update, we require certain keys to be present, others can be optional, then after updating and committing the model, we return a common response. How could we add model or route helpers to simplify our PUT route?

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


@task_bp.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

new_task = Task.from_dict(request_body)

db.session.add(new_task)
db.session.commit()
Comment on lines +30 to +38

Choose a reason for hiding this comment

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

🔎 We could have adapted create_model from the curriculum to be used here and for the Goal create routes. This would allow us to avoid duplicating most of the logic and error handling.

return {"task": new_task.to_dict()}, 201


@task_bp.get("")
def get_all_tasks():
sort_param = request.args.get("sort")
query = db.select(Task)

if sort_param == "asc":
query = 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.

👍 Nice gradual build-up of the query instance to allow us to add in sorting if present. This approach allows for easier inclusion of features like filtering, sorting, or paging.

elif sort_param == "desc":
query = query.order_by(Task.title.desc())
title_param = request.args.get("title")

if title_param:
query = query.where(Task.name == title_param)

tasks = db.session.scalars(query)
Comment on lines +45 to +56

Choose a reason for hiding this comment

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

Rather than coding this sort logic directly into the route, we could take an approach similar to get_models_with_filters from the curriculum which would allow us to move the sort logic into a shared function. Since both Goals and Tasks have a title column, this would give us the ability to sort both Tasks and Goals by their titles!

Choose a reason for hiding this comment

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

If no sort is specified, it's still a good idea to sort on the record ids, as this will give us a consistent ordering if records are modified. Otherwise, records will be returned in the internal ordering used by a table, which could change from request to request.

tasks_response = [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 use of a list comprehension to build the result response and convert the records to dicts all in a concise, readable structure.

return tasks_response


@task_bp.get("/<id>")
def get_one_task(id):
task = validate_model(Task, id)
return {"task": task.to_dict(include_goal_id=True)}


@task_bp.patch("/<id>/mark_complete")
def mark_task_complete(id):
task = validate_model(Task, id)

task.completed_at = datetime.now(timezone.utc)
db.session.commit()

send_slack_message(task.title)

Choose a reason for hiding this comment

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

👍 Nice use of a helper function to hold the logic for performing the notification.

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


@task_bp.patch("/<id>/mark_incomplete")
def mark_task_incomplete(id):
task = validate_model(Task, id)
task.completed_at = None
db.session.commit()
return Response(status=204, mimetype="application/json")
Comment on lines +80 to +83

Choose a reason for hiding this comment

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

👍 Nice route to mark the task incomplete. We can use our validation helper to get the same behavior as the other id-based routes, leaving our route responsible only for clearing the completion date, saving it, and generating the response.

One thing we might still consider is moving the actual update logic into a model helper so that the Task model class itself is responsible for "knowing" how to mark a Task incomplete.

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