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
2 changes: 1 addition & 1 deletion ada-project-docs/testing.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ We will need to complete part – or all – of some of the tests for this proje

You may wish to review details about how to run tests [here](https://github.com/AdaGold/viewing-party#details-about-how-to-run-tests).

Recall that it is always a good idea to search the file for any `@pytest.mark.skip` decorators you may have missed before moving to the next wave.
Recall that it is always a good idea to search the file for any `# @pytest.mark.skip` decorators you may have missed before moving to the next wave.
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 global serach and replace you did for skips also affected some of the project docs. Make sure to undo unintended changes like this (we can revert the changes on files we didn't intend to modify).


### Code Coverage

Expand Down
2 changes: 1 addition & 1 deletion ada-project-docs/wave_05.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ Our plan for this wave is to be able to create, read, update, and delete differe

This wave requires more test writing. The tests you need to write are scaffolded in the `test_wave_05.py` file.
- As with incomplete tests in other waves, you should comment out the `Exception` when implementing a test.
- These tests are currently skipped with `@pytest.mark.skip(reason="test to be completed by student")` and the function body has `pass` in it.
- These tests are currently skipped with `# @pytest.mark.skip(reason="test to be completed by student")` and the function body has `pass` in it.
- Once you implement these tests you should remove the `skip` decorator and the `pass`.

For the tests you write, use the requirements in this document to guide your test writing.
Expand Down
10 changes: 5 additions & 5 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
from .routes.task_routes import bp as task_bp
from .routes.goal_routes import bp as goal_bp

import os

def create_app(config=None):
Expand All @@ -10,13 +12,11 @@ def create_app(config=None):
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI')

if config:
# Merge `config` into the app's configuration
# to override the app's default settings for testing
app.config.update(config)

db.init_app(app)
migrate.init_app(app, db)

# Register Blueprints here

app.register_blueprint(task_bp)
app.register_blueprint(goal_bp)
return app
16 changes: 15 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
from sqlalchemy.orm import Mapped, mapped_column
from ..db import db
from sqlalchemy.orm import Mapped, mapped_column, relationship

class Goal(db.Model):
Copy link
Copy Markdown

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.

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):
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 ..db import db
from datetime import datetime
from typing import Optional
from sqlalchemy import ForeignKey
from sqlalchemy.orm import Mapped, mapped_column, relationship

class Task(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]
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 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.

description: Mapped[str]
completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Setting nullable=True is equivalent to marking the Mapped type Optional (as you also did). While either works, the Optional approach is preferred since it also provides information to the type checking warnings in VS Code. And if we set Optional, we don't also need the mapped_column.

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

def to_dict(self):
result = {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at)
Copy link
Copy Markdown

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.

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 project intended the logic that includes the goal_id for a task in its dict to be part of the to_dict logic itself. This would ensure that any time we generate output for a task that belongs to a goal, the task would include that information without each route needing to have logic to add goal_id information into task dictionaries that need it.

}
if self.goal_id is not None:
result["goal_id"] = self.goal_id
Comment on lines +22 to +23
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 will conditionally include the goal id field in the task dictionary if the task belongs to a goal. This maintain the expected output for the early tests and meets the later outputs as well. Any route that uses the to_dict will gain this behavior automatically.

Note that we don't automatically include the tasks with a goal's dictionary since a goal might have many tasks, so we have the caller opt into getting that information through the tasks for goal route. But since a task belongs to only a single goal, this is a low-cost data item to include.

return result

@classmethod
def from_dict(cls, task_data):
# If `is_complete` is True, set completed_at to now; otherwise keep it None.
is_complete = task_data.get("is_complete", False)

completed_at = datetime.now() if is_complete else None
Comment on lines +29 to +31
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 request body for the Task create route is documented as accepting a completed_at value (which we never pass a value for in our tests), not is_complete, which is in the output.

The task creation CRUD route shows an example request body of

{
  "title": "A Brand New Task",
  "description": "Test Description",
  "completed_at": null
}

The intent of receiving completed_at is that the caller should be able to create a new Task that's already been completed at some specific time. We never actually exercise this behavior, since that would require working with string representations of datetimes (JSON has no standard way to pass them), and ensuring that it could be interpreted as an actual datetime by the database.

Your is_complete behavior is a potentially useful extension, but it's not exactly what the prompt was going for.


return cls(title=task_data["title"],
description=task_data["description"],
Comment on lines +33 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.

👍 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=completed_at,
goal_id= task_data.get("goal_id", None)
)
57 changes: 56 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,56 @@
from flask import Blueprint
from flask import Blueprint, request
from ..routes.routes_utilities import (
validate_model,
create_model,
get_models_with_filters,
update_model_fields,
delete_model,
assign_related_by_ids,
)
from ..models.goal import Goal
from ..models.task import Task
from ..db import db

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

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.


@bp.get("")
def get_all_goals():
return get_models_with_filters(Goal, request.args)

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

return goal.to_dict()

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

return {"id": goal.id, "title": goal.title, "tasks": 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.

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.

Or we could make a different helper that itself uses the Goal to_dict for its basic structure, but adds in the Task content itself. This would tidy up the response generation for this route.


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

@bp.post("/<id>/tasks")
def post_task_ids_to_goal(id):
goal = validate_model(Goal, id)
data = request.get_json() or {}
return assign_related_by_ids(goal, "tasks", Task, data.get("task_ids"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

While moving the business logic of assigning relationships out of this route to a helper is a great idea, I'd probably make it a less general purpose route, and instead focus on this particualr case. This generalization feels a bit premature to me.


@bp.put("/<id>")
def update_goal(id):
goal = validate_model(Goal, id)
request_data = request.get_json()
return update_model_fields(goal, request_data, ["title"])

@bp.delete("/<id>")
def delete_goal(id):
goal = validate_model(Goal, id)
return delete_model(goal)
77 changes: 77 additions & 0 deletions app/routes/routes_utilities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
from flask import abort, make_response
import json
from ..db import db

def validate_model(cls, id):
try:
id = int(id)
except (ValueError, TypeError):
abort(make_response({"details": "Invalid id"}, 400))

model = db.session.get(cls, id)

if not model:
abort(make_response({"details": "Not found"}, 404))

return model

def create_model(cls, model_data):
try:
new_model = cls.from_dict(model_data)
except Exception:
abort(make_response({"details": "Invalid data"}, 400))

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

return new_model.to_dict(), 201

def get_models_with_filters(cls, args=None):
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 method doesn't actually filter. It only sorts. Filtering was an optional extension, but it would still be preferred to have the function name match the behavior a bit more closely, such as get_models_sorted. Alternatively, you could also add in the filtering behavior that we saw in Learn!

query = db.select(cls)

# Handle sorting
sort = args.get("sort") if args else None
if sort == "asc":
# title, then id
query = query.order_by(cls.title.asc(), cls.id)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Rather than adding id as an aditional sort column for each call, we can take on an additional order_by that sorts by the id after this conditional logic. That is, we conditionally can add the clause to sort by the title, but then always add a clause thsorts by the id. This would acheive the same result, with more compartmentalized logic.

elif sort == "desc":
# title (descending), then id
query = query.order_by(cls.title.desc(), cls.id)
else:
query = query.order_by(cls.id)
Comment on lines +40 to +41
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In addition to sorting by the title, it would still be helpful to do a final sort by the id (even if a title sort is applied), since this would still provide a predictable ordering even if there were records with the same title.


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

def update_model_fields(model, data, allowed_fields):
if not isinstance(data, dict):
abort(make_response({"details": "Invalid data"}, 400))

for field in allowed_fields:
if field in data:
setattr(model, field, data[field])

db.session.commit()
return empty_response()

def delete_model(model):
db.session.delete(model)
db.session.commit()
return empty_response()

def empty_response():
return make_response(json.dumps({}), 204, {"Content-Type": "application/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.

This will return an empty json response, but why not use the approach covered in Learn, and that you had on your mark complete endpoint?

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

This avoid needing to add a "placeholder" response body, which Flask will ignore anyway.


def assign_related_by_ids(parent, relation_name, child_cls, ids, response_key="task_ids"):
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 method shows a really strong understanding of passing class objects around and of using the setattr/getattr meta methods. However, this does make things a bit hard to read since nearly everything is a generic operation. So while this is an interesting approach, I'd probably keep my method a little bit more concrete until we had a better idea of whether there were any other situations where a method like this could be useful.

if not isinstance(ids, list):
abort(make_response({"details": "Invalid data"}, 400))

related = [validate_model(child_cls, cid) for cid in ids]
setattr(parent, relation_name, related)
db.session.commit()

return {
"id": parent.id,
response_key: [getattr(obj, "id") for obj in related]
}, 200
91 changes: 90 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,90 @@
from flask import Blueprint
from flask import Blueprint, request, Response
from ..models.task import Task
from ..db import db
from ..routes.routes_utilities import (
validate_model,
create_model,
get_models_with_filters,
update_model_fields,
delete_model,
Comment on lines +5 to +9
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 use of the main 3 route helpers introduced in the curriculum, as well as some of your own for the remaining CRUD routes! This isn't the only way to address the common behaviors across the model routes. There are details we didn't explore. These could be organized in other ways. But as a general rule, we should keep an eye out for additional opportunities to DRY our code and separate responsibilities as we continue to work with larger codebases.

)
from ..routes.routes_utilities import empty_response
from datetime import datetime
from dotenv import load_dotenv
import os

load_dotenv()
Copy link
Copy Markdown

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

SLACK_TOKEN = os.getenv("SLACK_TOKEN")
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 use of get to ensure that a missing token doesn't prevent the route from running at all. In a full application, we'd probably want to log this situation somehow for diagnostic purposes.

SLACK_CHANNEL = os.getenv("SLACK_CHANNEL")

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

@bp.get("")
def get_all_tasks():
return get_models_with_filters(Task, request.args)

@bp.get("/<id>")
def get_single_tasks(id):
task = validate_model(Task, id)
return task.to_dict()

@bp.patch("/<id>/mark_complete")
def mark_task_complete(id):
task = validate_model(Task, id)
# No request body is expected for marking a task complete; simply set
# the completed timestamp.
task.completed_at = datetime.now()
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 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.


db.session.commit()

send_completed_task_to_slack(task)
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 use of a helper function to hold the logic for performing the notification.

return empty_response()

def send_completed_task_to_slack(task):
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 function encapsulates the responsibility of sending a completion notification about the provided task. Notice that we could make a further helper function that wraps the responsibility of sending a 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.

import requests

slack_message_url = "https://slack.com/api/chat.postMessage"
# channel is required by Slack API; allow configuration via SLACK_CHANNEL env var
channel = SLACK_CHANNEL or os.getenv("SLACK_CHANNEL")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There was code earlier to read the value from the SLACK_CHANNEL environment variable, so it's not needed again here. The value of an environment variable never changes as long as a a program stays running, so after reading the environment at startup, it wouldn't change on subsequent API calls.


message = {
"channel": channel,
"text": f"Someone just completed the task '{task.title}'!"
}
headers = {
"Content-Type": "application/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.

Note that because you used the json= named parameter in your call, requests will automatically set the content type header appropriately, so we could omit this.

"Authorization": f"Bearer {SLACK_TOKEN}"
}

response = requests.post(slack_message_url, json=message, headers=headers)
# print(response.status_code, response.text) # debug output
# print("SLACK_TOKEN:", SLACK_TOKEN) # debug output

response.raise_for_status()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If we check the documentation for raise_for_status, it only reports 4xx and 5xx codes. But Slack returns 2xx for everything, even API errors. As a result, there will be many possible cases that Slack could report an error about, such as invalid token, or invalid channel errors. If we really wanted those to be reported, we'd have to do something different.

But let's say that we do report those errors. Then the question becomes do we really want to end up causing the call to apparently fail, just because there might be a problem with Slack? Generally not.

Even if the Slack call fails, the task would still be marked as complete, since the database commit happens prior to the Slack integration call, but this would potentially be confusing for the caller. They would see their call to complete the task crash, but if they then get the details for the task, it would have been successfully completed.

In general, unless the external call we're making is integral to the functionality of what we're trying to accomplish (completing a task), I wouldn't treat an error so seriously. We could take inspiration from Slack and return a success code, but include a response body with a warning in it. Or we could just log that there was a problem so that support staff can see that there is an issue to follow up on later, but otherwise not bother the caller about it.


@bp.patch("/<id>/mark_incomplete")
def mark_task_incomplete(id):
task = validate_model(Task, id)

task.completed_at = None
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 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.


db.session.commit()

return empty_response()

@bp.post("")
def create_task():
model_dict, status_code = create_model(Task, request.get_json())
return model_dict, status_code
Comment on lines +77 to +78
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note that we could return the result of the helper directly without unpacking it. But this can be helpful if we need to make any changes to the result dict before sending it back.


@bp.put("/<id>")
def replace_task(id):
task = validate_model(Task, id)

request_body = request.get_json()
return update_model_fields(task, request_body, ["title", "description", "completed_at"])
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 custom route helper to coordinate updating a model. However, we might want to take an approach more like create_model and from_dict than filtering. Since this is a PUT endpoint, the expectation is that we're replacing the record. And much like when creating a record, there will be some fields that are required, and others that might be optional. For creating, this logic was originally housed in the post route itself, but then we moved the parts that were specific to the model to the from_dict method, and the flow that was shared among models to a route helper (create_model). We could do the same thing for updating, where the model specific part (actually updating fields) could be in a per-model method, such as Task.update_from_dict, and then the parts that are common across any PUT route would live in the route helper (here update_model_fields).


@bp.delete("/<id>")
def delete_task(id):
task = validate_model(Task, id)
return delete_model(task)
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 delete route helper is great to add! We do need to be a little careful, since whether a model can be the target of a foreign key can impact how the delete is carried out. So we might still want a per-model delete method. But from what we've looked at so far, this works well!

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