Skip to content

Madina_Dzhetegenova_Ada_C22_Phoenix #40

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 8 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
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,7 @@ This project is designed to fulfill the features described in detail in each wav
1. [Wave 6: Establishing a one-to-many relationship between two models](ada-project-docs/wave_06.md)
1. [Wave 7: Deployment](ada-project-docs/wave_07.md)
1. [Optional Enhancements](ada-project-docs/optional-enhancements.md)




6 changes: 6 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
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):
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('SLACK_API_TOKEN')

if config:
# Merge `config` into the app's configuration
Expand All @@ -19,4 +22,7 @@ def create_app(config=None):

# Register Blueprints here

app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)

return app
6 changes: 5 additions & 1 deletion app/models/goal.py

Choose a reason for hiding this comment

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

Like with the Task model, we can add helper methods to do common actions like converting to or loading from dictionaries.

Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from ..db import db

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


13 changes: 11 additions & 2 deletions app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from ..db import db

from datetime import datetime
from typing import Optional
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]]
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks")


Choose a reason for hiding this comment

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

Remember that we can add helper methods to do common actions like converting to dictionaries, or load from dictionaries.

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.
146 changes: 145 additions & 1 deletion app/routes/goal_routes.py

Choose a reason for hiding this comment

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

Check the equivalent routes in the task_routes file for points to watch out for here.

Original file line number Diff line number Diff line change
@@ -1 +1,145 @@
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 datetime import datetime
from ..db import db
from .task_routes import validate_task

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

Choose a reason for hiding this comment

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

Like for tasks, we would prefer to call this variable bp (no model in the name).


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

if "title" not in request_body:
return {"details": "Invalid data"}, 400

title = request_body["title"]


new_goal = Goal(title=title)
db.session.add(new_goal)
db.session.commit()


response = { "goal" : {
"id": new_goal.id,
"title": new_goal.title
}
}
Comment on lines +25 to +29

Choose a reason for hiding this comment

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

Like for tasks, a to_dict helper would reduce repetition in all the places where we need to generate a goal dictionary response.

return response, 201

@goals_bp.get("")
def get_all_goals():
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 +37 to +44

Choose a reason for hiding this comment

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

With a to_dict method, a list comprehension would become a readable alternative to writing out the loop here longhand.

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

return goals_response, 200

@goals_bp.get("/<goal_id>")
def get_one_goal(goal_id):
goal = validate_goal(goal_id)

return { "goal" : {
"id": goal.id,
"title": goal.title
}
}

def validate_goal(goal_id):

Choose a reason for hiding this comment

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

👀 Note that we could refactor this validate_goal and the validate_task functions to be able to handle both.

try:
goal_id = int(goal_id)
except:
response = {"message": f"goal {goal_id} invalid"}
abort(make_response(response , 400))

query = db.select(Goal).where(Goal.id == goal_id)
goal = db.session.scalar(query)

if not goal:
response = {"message": f"goal {goal_id} not found"}
abort(make_response(response, 404))
return goal


@goals_bp.put("/<goal_id>")
def update_goal(goal_id):
goal = validate_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.

Similar to the Task PUT route, this route could benefit from some error handling for any missing keys.

db.session.commit()


response = { "goal" : {
"id": goal.id,
"title": goal.title
}
}
return response, 200


@goals_bp.delete("/<goal_id>")
def delete_goal(goal_id):
goal = validate_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(goal_id):
goal = validate_goal(goal_id)

request_data = request.get_json()
task_ids = request_data.get("task_ids")

if not task_ids:
response = {"message": "No task_ids provided"}
abort(make_response(response, 400))
Comment on lines +112 to +114

Choose a reason for hiding this comment

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

We don't include a test for passing an empty list here, so treating it as an error is fine. Consider adding additional test cases if you add additional logic not covered by existing tests so that you can try this out.

An alternative way to handle this might be to interpret sending an empty list as meaning to disassociate the goal from any current tasks, though this might work better if we make a different behavioral choice below.


for task_id in task_ids:
task = validate_task(task_id)
task.goal = goal
Comment on lines +116 to +118

Choose a reason for hiding this comment

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

By setting the goal for each task to be the current goal, this route is effectively appending the supplied tasks to the goal. There might have been other tasks already associated with the goal, though such a scenario doesn't appear in any of the tests we supplied. As a result, we might consider retrieving all the tasks now associated with the goal (such as with something like all_task_ids = [task.id for task in goal.tasks]), rather than returning a list only of the ids we just added.

If we interpretted this route as replacing the tasks associated with this goal (such as by generating a list of the tasks, and then replacing the whole goal.tasks list with the updated list), then the current behavior of only returning the incoming tasks would more closely match expectations. This might match the behavior described above, where we might consider an empty list of task ids to mean there should be no tasks associated with the goal.


db.session.commit()

response_body = {
"id": goal.id,
"task_ids": task_ids
}
return response_body, 200

@goals_bp.get("/<goal_id>/tasks")
def get_tasks_of_goal(goal_id):
goal = validate_goal(goal_id)

tasks = [{
"id": task.id,
"goal_id": goal.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
} for task in goal.tasks]
Comment on lines +132 to +138

Choose a reason for hiding this comment

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

Notice that without a to_dict method, the conciseness of the list comprehension is lost. Compare to the case where we do create a to_dict method.

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

We would need to be sure that the to_dict method correctly added the goal_id information only when available so that it could be used in all the places where we convert tasks to dictionary responses.


response_body = {"id": goal.id,
"title": goal.title,
"tasks": tasks}

return response_body, 200

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


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

Choose a reason for hiding this comment

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

Moving forward, prefer to name the Blueprint in a route file as just bp rather than having the variable include the model name. The string name would still be written as is for differentiation from other blueprints.


@tasks_bp.post("")
def create_task():
request_body = request.get_json()
if "title" not in request_body:
return {"details": "Invalid data"}, 400
if "description" not in request_body:
return {"details": "Invalid data"}, 400
Comment on lines +15 to +18

Choose a reason for hiding this comment

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

Alternatively, we could watch for a KeyError when trying to retrieve these below. If we had a from_dict method, the key accesses could be within that method, and we have a create_model wrapper to watch for KeyErrors and turn those into the appropriate error behavior.


title = request_body["title"]
description = request_body["description"]
completed_at= request_body.get("completed_at")

Choose a reason for hiding this comment

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

Nit: watch the spacing around assignment =

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 make the "completed_at" key be treated as optional.


new_task = Task(title=title, description=description, completed_at= None if completed_at is None else datetime.datetime(completed_at)

Choose a reason for hiding this comment

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

What kind of data is this expecting completed_at to have when it's non-None? The most common way to send datetime information through JSON is with a specially formatted string (there's no specific datetime type in JSON), but the python datetime constructor doesn't accept strings. It expects the information to be passed in as several numbers. Instead, we might want to use the datetime.fromisoformat() to read the data from a string. Alternatively, depending on the string format, the database might be able to convert it from a string on its own.

There were no provided tests that passed in a completed_at value, so if you wanted to support being able to make a completed Task, be sure to write your own tests to check how the logic behaves.

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


response = { "task" : {
"id": new_task.id,
"title": new_task.title,
"description": new_task.description,
"is_complete": new_task.completed_at is not None
}
Comment on lines +30 to +35

Choose a reason for hiding this comment

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

Notice how the dictionary structure of a Task is needed in several of the endpoints (both here, and in the goals routes). Including a to_dict method in Task would allow this repetitive logic to be kept in a single place, with the guarantee that they are all generating the same output format.

}
return response, 201

@tasks_bp.get("")
def get_all_tasks():
sort_order = request.args.get("sort")

if sort_order == "asc":
query = db.select(Task).order_by(Task.title.asc())
elif sort_order == "desc":
query = db.select(Task).order_by(Task.title.desc())
else:
query = db.select(Task).order_by(Task.id)
Comment on lines +47 to +48

Choose a reason for hiding this comment

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

Nice fallback to order by id when no sort is requested.

Comment on lines +43 to +48

Choose a reason for hiding this comment

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

Note that the db.select is repeated in each of these. Consider starting to build a query with the shared portion, then update it as needed according to any passed options.

    query = db.select(Task)

    if sort_order == "asc":
        query = query.order_by(Task.title.asc())
    elif sort_order == "desc":
        query = query.order_by(Task.title.desc())
    else:
        query = query.order_by(Task.id)

Even though it doesn't shorten things much here, it's a good practice to get into.


tasks = db.session.scalars(query)

tasks_response = []
for task in tasks:
tasks_response.append(
{
"id": task.id,
# "goal_id": task.goal_id is not None,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
}
)
return tasks_response, 200

@tasks_bp.get("/<task_id>")
def get_one_task(task_id):
task = validate_task(task_id)

if task.goal_id is not None:
return {
"task": {
"id": task.id,
"goal_id": task.goal_id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
}
}
else:
return {
"task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
}
}
Comment on lines +69 to +87

Choose a reason for hiding this comment

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

Notice that most of the keys are returned in either case. Only the goal id is variable. We could generate a dictionary holding the common values first, then add in the goal id if needed. Something like this.

    result = {
            "id": task.id,
            "title": task.title,
            "description": task.description,
            "is_complete": task.completed_at is not None
        }

    if task.goal_id is not None:
        result["goal_id"] = task.goal_id

    return {"task": result}


def validate_task(task_id):

Choose a reason for hiding this comment

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

👀 Note that we could refactor this validate_task and the validate_goal functions to be able to handle both.

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

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

if not task:
response = {"message": f"task {task_id} not found"}
abort(make_response(response, 404))
return task

@tasks_bp.put("/<task_id>")
def update_task(task_id):
task = validate_task(task_id)
request_body = request.get_json()

task.title = request_body["title"]
task.description = request_body["description"]
Comment on lines +109 to +110

Choose a reason for hiding this comment

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

The same kind of key errors that were possible in the POST route could also happen here in the PUT, so this route could also benefit from similar error handling.

task.completed_at= request_body.get("completed_at")
# task.goal_id= request_body.get("goal_id")
db.session.commit()


response = { "task" : {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
}
}
return response, 200

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

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

response = {
"details": f"Task {task.id} \"{task.title}\" successfully deleted"

Choose a reason for hiding this comment

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

Since this string needs to store double quotes, we could use a single quote f-string to avoid having to escpae the quotes.

}

return response, 200

@tasks_bp.patch("/<task_id>/mark_complete")
def update_task_to_complete(task_id):
task = validate_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()
token = os.environ.get('SLACK_API_TOKEN')
headers = {
"Authorization": f"Bearer {token}"
}
url = "https://slack.com/api/chat.postMessage"

message = {"channel": "D07V10LPBM4",
"text": f"Someone just completed the task {task.title}"}

response = requests.post(url, headers=headers, data=message)

print(response.text)
Comment on lines +143 to +154

Choose a reason for hiding this comment

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

Consider moving this to a helper function so that the completion route can stay focused on marking the task complete, with the logic related to posting a message handled elsewhere.

response = { "task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
}
}
return response, 200

@tasks_bp.patch("/<task_id>/mark_incomplete")
def update_task_to_incomplete(task_id):
task = validate_task(task_id)
task.completed_at = None
db.session.commit()

response_body = { "task": {
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
}
}
return response_body, 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.
Loading