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
7 changes: 7 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,14 @@
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
from flask_cors import CORS

def create_app(config=None):
app = Flask(__name__)
CORS(app)
app.config['CORS_HEADERS'] = 'Content-Type'

app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI')
Expand All @@ -18,5 +23,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
29 changes: 28 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,32 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
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.

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_as_dict = {}
goal_as_dict["id"] = self.id
goal_as_dict["title"] = self.title

if self.tasks:
goal_as_dict["tasks"] = [task.to_dict() for task in self.tasks]
Comment on lines +14 to +15

Choose a reason for hiding this comment

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

👀 In the project, the task data for a goal isn't included by default, even if present. The client must explicitly ask for it through an endpoint. It's a common consideration in an API how much variable length data to include per record by default, and often the static length data and variable length data will be split as we see in this project.

We don't see this presenting a problem in wave 5 because the goals there have no tasks. But notice that in wave 6, we are going to a special endpoint to get the task data for a goal. The caller must opt-in to receiving it.


return goal_as_dict

def nested_category(self):
updated_goal_as_dict = {}
updated_goal_as_dict["goal"] = self.to_dict()
return updated_goal_as_dict


@classmethod
def from_dict(cls, goal_data):

new_goal = cls(
title=goal_data["title"]
)

return new_goal
48 changes: 47 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,51 @@
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]]
is_complete : Mapped[Optional[bool]]

Choose a reason for hiding this comment

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

👀 The wave instructions said that the model should contain completed_at, not is_complete. is_complete is a data abstraction used only in transforming a Task instance into a dictionary for being used in a JSON response. It can be calculated directly from the completed_at data, meaning there's no need to store the is_complete literally. In fact, we explicitly would not want to store it, since it duplicates data for a Task, and could result in data ending up in inconsistent configurations. For instance, we could end up with a Task that is marked is_complete, but which has no completed_at value. Or that has a completed_at value, but isn't marked is_complete.

goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks")
Comment on lines +9 to +14

Choose a reason for hiding this comment

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

👍 Nice consistent use of Mapped-style column definitions. All the type and nullable information can be inferred by Alembic from the Mapped entry itself, without needing to supply details in mapped_column unless we need additional data, such as key constraints.


def to_dict(self):
task_as_dict = {}
task_as_dict["id"] = self.id
task_as_dict["title"] = self.title
task_as_dict["description"] = self.description
Comment on lines +17 to +20

Choose a reason for hiding this comment

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

Purely from a code perspective, there's a lot of repetition in building up a dictionary like this. Consider setting more of these keys in an initial literal

task_as_dict = {
    "id": self.id,
    ...
}

or by using the dict constructor

task_as_dict = dict(
    id=self.id,
    ...
}

if self.completed_at:
task_as_dict["completed_at"] = self.completed_at
Comment on lines +21 to +22

Choose a reason for hiding this comment

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

Note that we didn't specify that completed_at should be included in the Task response format. This was primarily because there's no standard format for datetime content in JSON and we wanted to avoid any complications that would introduce. Conditionally including it only in records that have been marked complete avoids causing problems in the supplied tests (which only retrieve incomplete Task responses).

if self.is_complete:
task_as_dict["is_complete"] = self.is_complete
else:
task_as_dict["is_complete"] = False
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.

👀 Rather than reading the value for is_complete from a column, how could we calculate this from the value of completed_at?


if self.goal_id:
task_as_dict["goal_id"] = self.goal_id
Comment on lines +28 to +29

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_as_dict

def nested_category(self):

Choose a reason for hiding this comment

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

👍 Nice inclusion of a helper that does wrap the main response data in an outer envelope. There's nothing special about our using to_dict elsewhere, so including what is effectively a custom version of to_dict works well if we have different output formats used in different locations.

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.

updated_task_as_dict = {}
updated_task_as_dict["task"] = self.to_dict()
return updated_task_as_dict


@classmethod
def from_dict(cls, task_data):
goal_id = task_data.get("goal_id")

new_task = cls(
title=task_data["title"],
description=task_data["description"],
Comment on lines +44 to +45

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", None),
is_complete=task_data.get("is_complete", False),
goal_id=goal_id
)

return new_task
76 changes: 75 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,75 @@
from flask import Blueprint
from flask import Blueprint, request, Response
from app.models.goal import Goal
from app.models.task import Task
from .route_utilities import validate_model, create_model, get_models_with_sort
from ..db import db

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

#Create a Goal: Valid 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()
return create_model(Goal, request_body)

#Get Goals: Getting Saved Goals
@bp.get("")
def get_all_goals():
return get_models_with_sort(Goal, request.args)

#Get One Goal: One Saved Goal
@bp.get("/<goal_id>")
def get_one_goal(goal_id):
goal = validate_model(Goal, goal_id)
return goal.nested_category()

#Update Goal
@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")

#Delete Goal: Deleting a Goal
@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")

#Sending a List of Task IDs to a Goal
@bp.post("/<goal_id>/tasks")
def create_tasks_with_goal(goal_id):
goal = validate_model(Goal, goal_id)

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

for task in goal.tasks: #Add this to pass the test test_post_task_ids_to_goal_already_with_goals

Choose a reason for hiding this comment

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

Notice that this isn't just about passing the test. The test implies certain behaviors, in this case, replacement behavior. So our route needs to implement replacement (rather than append) behavior, of which one way to accoplish this is by disassociating any previous tasks from the goal.

task.goal_id = None

for task_id in task_ids:
task = validate_model(Task, task_id)
if task.goal_id is None:
task.goal_id = goal.id
Comment on lines +54 to +60

Choose a reason for hiding this comment

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

Since we need to validate each Task id anyway, if we stored the resulting tasks in a list, the replacement of the tasks for the goal could be accomplished using the tasks property as

    goal.tasks = tasks

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

On the other hand, updating each Task individually does prevent us from needing to keep all the tasks in memory at once.


db.session.commit()
return {
"id": goal.id,
"task_ids": task_ids
}, 200

#Getting Tasks of One Goal
@bp.get("/<goal_id>/tasks")
def get_tasks_by_goal(goal_id):
goal = validate_model(Goal, goal_id)
tasks = [task.to_dict() for task in goal.tasks]
response = goal.to_dict()
response["tasks"] = tasks
Comment on lines +72 to +74

Choose a reason for hiding this comment

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

👍 Nice reuse of your to_dict methods to build up your response. Rather than leaving this logic here in the route, we could move it to an additional Goal method. Perhaps to_dict_with_tasks, or we could add a parameter to the main Goal to_dict that determines whether or not to include the Task details.

return response
47 changes: 47 additions & 0 deletions app/routes/route_utilities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
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": f"Invalid data"}
abort(make_response(response, 400))

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

return new_model.nested_category(), 201

Choose a reason for hiding this comment

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

👍 Nice customization of create_model to account for the outer wrapper "envelope" in a way that works for either model type.


def get_models_with_sort(cls, args):

Choose a reason for hiding this comment

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

👍 Nice custom helper similar to get_models_with_filters to provide sorting functionality. How could we generalize this even further to sort on other attributes?

query = db.select(cls)
sort = args.get("sort")

if sort == "asc":
query = query.order_by(cls.title)
elif sort == "desc":
query = query.order_by(cls.title.desc())
else:
query = query.order_by(cls.id)

models = db.session.scalars(query)
models_response = [model.to_dict() for model in models]

return models_response
80 changes: 79 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,79 @@
from flask import Blueprint
from flask import Blueprint, request, Response
from app.models.task import Task
from .route_utilities import validate_model, create_model, get_models_with_sort
from ..db import db
from datetime import date
import os
import requests

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

#Create a Task: Valid Task With null completed_at
@bp.post("")
def create_task():
request_body = request.get_json()
return create_model(Task, request_body)

Choose a reason for hiding this comment

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

👍 Nice use of route helpers throughout your route logic. There are lots of ways that we could write such methods, but starting from the features provided by the curriculum functions is a good starting place, and it's great to see the customizations that you added. There are even other routes that could benefit from similar helpers (such as PUT and DELETE). We should keep an eye out for additional opportunities to DRY our code and separate responsibilities as we continue to work with larger codebases.


#Get Tasks: Getting Saved Tasks
@bp.get("")
def get_all_tasks():
return get_models_with_sort(Task, request.args)

#Get One Task: One Saved Task
@bp.get("/<task_id>")
def get_one_task(task_id):
task = validate_model(Task, task_id)
return task.nested_category()

#Update Task
@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()
Comment on lines +34 to +36

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

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

Comment on lines +43 to +46

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

#Mark Complete on an Incomplete Task
@bp.patch("/<task_id>/mark_complete")
def complete_an_incomplete_task(task_id):
task = validate_model(Task, task_id)

task.is_complete = True

Choose a reason for hiding this comment

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

👀 With the literal is_complete column, we need to update two items when a task is completed. However, we don't require there being an actual attribute solely to include it in the dictionary format of Task. How can we drop this column, but include the key in the dictionary format, based on the value of completed_at?

task.completed_at = date.today()
db.session.commit()

slack_token = os.environ.get("SLACK_API_TOKEN")
slack_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.

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

slack_data = {
"channel": "task-notifications",
"text": f"Someone just completed the task {task.title}"
}

requests.post(slack_url, headers=slack_headers, json=slack_data)
Comment on lines +58 to +66

Choose a reason for hiding this comment

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

As we add more logic to a route, it becomes less clear what its main focus is. We could help improve the understandability by moving the logic related to sending a notification to a helper function, maybe notify_task_complete.

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.


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

#Mark Incomplete on a Completed Task
@bp.patch("/<task_id>/mark_incomplete")
def incomplete_an_complete_task(task_id):
task = validate_model(Task, task_id)

task.is_complete = False

Choose a reason for hiding this comment

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

👀 With the literal is_complete column, we need to update two items when a task is marked incomplete. However, we don't require there being an actual attribute solely to include it in the dictionary format of Task. How can we drop this column, but include the key in the dictionary format, based on the value of completed_at?

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