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
9 changes: 8 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
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
from dotenv import load_dotenv
load_dotenv() # Reads .env file and Loads all key-value pairs into environment
Comment on lines +7 to +8

Choose a reason for hiding this comment

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

It should not be necessary to add this explicit call to load_dotenv here. In all the places where we expect Flask to locate these settings in the .env file, Flask will do so properly (for instance, we don't need this call in deployment, since we load the values directly in the deployed environment).


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

15 changes: 15 additions & 0 deletions app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,20 @@
from sqlalchemy.orm import Mapped, mapped_column
from ..db import db
from sqlalchemy.orm import relationship

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.

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"]
)
31 changes: 30 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,34 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from ..db import db
from typing import Optional
from datetime import datetime
from sqlalchemy import ForeignKey

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)
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"), nullable=True)
Comment on lines +11 to +12

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.

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

def to_dict(self):
task_dict = {

Choose a reason for hiding this comment

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

👍 Not including the outer task "envelope" wrapper in our to_dict keeps it more general-purpose. We can use it in endpoints that require the envelope by embedding this result in an outer dictionary, or use in other cases where the wrapper is not called for in the specifications.

"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": self.completed_at is not None,

Choose a reason for hiding this comment

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

Even though we only use the calculated is_complete value when converting to a result dict, and even though it's a single line, it can be useful to move this logic to a well-named helper. This would make the dict building here more self-documenting, and provides a clear name to the operation being performed.

}

if self.goal_id is not None:
task_dict["goal_id"] = self.goal_id
Comment on lines +23 to +24

Choose a reason for hiding this comment

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

👍 Nice logic to add the goal data to the task dictionary only when it's present.


return task_dict

@classmethod
def from_dict(cls, task_data):
return cls(
title=task_data["title"], # this will raise KeyError if missing
description=task_data["description"],
Comment on lines +31 to +32

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") # this will not raise KeyError if missing
)
115 changes: 114 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,114 @@
from flask import Blueprint
from flask import Blueprint,make_response, abort, request, Response
from ..models.goal import Goal
from ..models.task import Task
from app.routes.route_utilities import validate_model
from app import db

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

@goal_bp.post("")
def create_goal():

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.


request_body = request.get_json()

try:
new_goal = Goal.from_dict(request_body)
except KeyError as error:
message = {
# "message": f"Missing '{error.args[0]}' attribute"
"details": "Invalid data"
}
abort(make_response(message, 400))
db.session.add(new_goal)
db.session.commit()

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

@goal_bp.get("")
def get_all_goals():
query = db.select(Goal)
goals = db.session.scalars(query) #This executes the SQL query

goals_response = []
for goal in goals:
goals_response.append(goal.to_dict())
return goals_response

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

@goal_bp.put("/<goal_id>")
def update_goal(goal_id):

goal = validate_model(Goal,goal_id)

request_body = request.get_json()

try:
goal.title = request_body["title"]
except KeyError as error:
message = {
"message": f"Missing '{error.args[0]}' attribute"
}
abort(make_response(message, 400))

db.session.commit()

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

@goal_bp.delete("/<goal_id>")
def remove_task(goal_id):

goal = validate_model(Goal,goal_id)

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

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

@goal_bp.post("/<goal_id>/tasks")
def assign_tasks_to_goal(goal_id):
goal = validate_model(Goal, goal_id)

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

# Clear existing task associations first
for task in goal.tasks:
task.goal_id = None

tasks = []
for task_id in task_ids:
task = validate_model(Task, task_id)
task.goal_id = goal.id
tasks.append(task)
Comment on lines +79 to +87

Choose a reason for hiding this comment

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

Once we have a list of validated tasks, the replacement of the tasks for the goal can be accomplished using the tasks property as

    goal.tasks = tasks

which avoids the need to manually clear the goal association from the existing tasks.


db.session.commit()

return {
"id": goal.id,
"task_ids": [task.id for task in tasks]
}, 200

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

task_list = [task.to_dict() for task in goal.tasks]
tasks_response = []
for task in goal.tasks:
task_dict = task.to_dict()
task_dict["goal_id"] = goal.id # add goal_id if not in to_dict
tasks_response.append(task_dict)
Comment on lines +101 to +105

Choose a reason for hiding this comment

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

Your Task to_dict method already includes the "goal_id" key in the dict of any Task belonging to a Goal, so we don't need to manually add the Goal id here.


return {
"id": goal.id,
"title": goal.title,
Comment on lines +108 to +109

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_response

Choose a reason for hiding this comment

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

The task_list collection (or the comprehension that created it) could be used here directly.

}, 200



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

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

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

if instance is None:
abort(make_response({"message": f"{cls.__name__} ID ({model_id}) not found."}, 404))
return instance

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

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

@task_bp.post("")
def create_task():

request_body = request.get_json()

try:
new_task = Task.from_dict(request_body)
except KeyError as error:
message = {
# "message": f"Missing '{error.args[0]}' attribute"
"details": "Invalid data"
}
abort(make_response(message, 400))
db.session.add(new_task)
db.session.commit()

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

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.


@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(asc(Task.title))
elif sort_param == "desc":
query = query.order_by(desc(Task.title))
Comment on lines +35 to +39

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 would allow for easier inclusion of other features like filtering or paging later on.

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 = db.session.scalars(query) #This executes the SQL query

tasks_response = []
for task in tasks:
tasks_response.append(task.to_dict())
Comment on lines +35 to +45

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!

Comment on lines +43 to +45

Choose a reason for hiding this comment

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

This pattern (empty result, iterate, process each record, append) can be expressed cleanly in a list comprehension.

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

return tasks_response

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


# def validate_task(task_id):
# try:
# task_id = int(task_id)
# except ValueError:
# response = {"message" : f"task {task_id} invalid"}
# abort(make_response(response, 400))

# # task = Task.query.get(task_id)
# query = db.select(Task).where(Task.id == task_id)
# task = db.session.scalar(query)
# # db.session.get(Task, task_id)

# if task is None:
# message = {
# "message": f"task ID ({task_id}) not found."
# }
# abort(make_response(message, 404))
# return task

@task_bp.put("/<task_id>")
def update_task(task_id):

task = validate_model(Task,task_id)

request_body = request.get_json()

try:
task.title = request_body["title"]
task.description = request_body["description"]
except KeyError as error:
message = {
"message": f"Missing '{error.args[0]}' attribute"
}
abort(make_response(message, 400))

db.session.commit()
Comment on lines +81 to +90

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.delete("/<task_id>")
def remove_task(task_id):

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?


task = validate_model(Task,task_id)

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

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

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.

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

Choose a reason for hiding this comment

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

👍 Using get results in a None value being assigned for the values missing from the .env (or using the explicit default value), which lets the code run without crashing during tests even if the expected keys aren't set. However, if we wanted to force that all the expected .env values are set, we could use direct key access.

channel = os.environ.get("SLACK_CHANNEL", "task-notifications")

url = "https://slack.com/api/chat.postMessage"
headers = {
"Authorization": f"Bearer {slack_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"
}
payload = {
"channel": f"#{channel}",
"text": f"Someone just completed the task {task_title}"
}

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

Choose a reason for hiding this comment

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

👍 Here, we send the request and then move on with our logic without checking the result from Slack. This is fine here, since sending the message is just a side effect of completing the task. For example, it's unlikely we'd want to fail to complete a task in the event that Slack wasn't reachable.

In a fuller application, we might write out the result of this call to a log file so that if a problem with our calls to Slack does occur, we'd have a record to investigate.


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

task.completed_at = datetime.now(timezone.utc)
db.session.commit()
Comment on lines +122 to +125

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 complete. We can use our validation helper to get the same behavior as the other id-based routes, leaving our route responsible only for updating the record with the current datetime, 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 complete a Task.


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

task.completed_at = None
db.session.commit()
Comment on lines +133 to +136

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.


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.
Loading