Skip to content

C22-Phoenix -Tatyana Venanzi #30

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 9 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
6 changes: 6 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from flask import Flask
from .db import db, migrate
from .routes.task_routes import tasks_bp
from .routes.goal_routes import goals_bp
from .models import task, goal
import os

Expand All @@ -18,5 +20,9 @@ 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


12 changes: 11 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,15 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from typing import List
from ..db import db

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

Choose a reason for hiding this comment

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

In the current version of SQLAlchemy nullable=False is the default, so we could leave off the mapped_column statement without changing our tables:

    title: Mapped[str]

tasks: Mapped[List["Task"]] = relationship("Task", back_populates="goal")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works, but we can also use the built-in Python list type here. I typically recommend using the Python built in unless there is a need to use a more specific library version of a data structure.

Since we define that the attribute is a list of Tasks on the left side of the assignment, we do not need to pass "Task" as the first parameter in the relationship. This feedback applies to the relationship attribute in the Task class as well.

    tasks: Mapped[list["Task"]] = relationship(back_populates="goal")


def goal_dict(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this function is a method of the Goal class, including goal in the name doesn't give the reader new information. Names like create_dict, as_dict, or to_dict would convey a little more clearly to the user the intent of the function.

return dict(
id=self.id,
title=self.title
)

23 changes: 22 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,26 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column, relationship
from sqlalchemy import ForeignKey
from typing import Optional
from ..db import db
from datetime import datetime

class Task(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str] = mapped_column(nullable=False)
description: Mapped[str] = mapped_column(nullable=False)
completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True, default=None)
Comment on lines +9 to +11
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we aren't changing the default options for the declarations on the left side of the assignment, we can leave off the mapped_column calls for these 3 attributes.

# Nullable is False by default unless an attribute is marked as Optional
# These 2 attributes are not nullable
title: Mapped[str] 
description: Mapped[str]

# Since it is declared as Optional, nullable = True by default
# If an attribute is nullable, it will be defaulted to `None` if it is not set when a record is
# created unless a different default value is explicitly provided in the definition here
completed_at: Mapped[Optional[datetime]]

goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey('goal.id'), nullable=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since goal_id is Optional, we can leave off the nullable argument in mapped_column:

goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey('goal.id'))

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

def task_dict(self):
task_data = {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": self.completed_at is not None
}

if self.goal_id is not None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

In Python it's preferred to use a Truthy/Falsy check over an explicit None check in most circumstances:

        if self.goal_id:

I see other places across the project where this applies, I suggest searching for is None or is not None to find them quicker.

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

104 changes: 103 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,103 @@
from flask import Blueprint
from flask import Blueprint, request, make_response, abort, Response
from app.models.goal import Goal
from app.models.task import Task
from ..db import db

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

def validate_goal(goal_id):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very similar to our validate_task function, how could we refactor these functions to reduce repetition?

try:
goal_id = int(goal_id)
except ValueError:
abort(make_response({"message": f"Goal {goal_id} is invalid"}, 400))

goal = Goal.query.get(goal_id)
if not goal:
abort(make_response({"message": f"Goal {goal_id} not found"}, 404))
return goal

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

if "title" not in request_body:
abort(make_response({"details": "Invalid data"}, 400))

new_goal = Goal(title=request_body["title"])

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

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

@goals_bp.post("/<goal_id>/tasks")
def associated_tasks_with_goal(goal_id):
goal = validate_goal(goal_id)
request_body = request.get_json()

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

task_ids = request_body["task_ids"]
for task_id in task_ids:
task = Task.query.get(task_id)
if task:
task.goal_id = goal.id

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

@goals_bp.get("")
def get_all_goals():
goals = Goal.query.all()
goals_response = [goal.goal_dict() for goal in goals]

return make_response(goals_response, 200)

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

return {"goal": goal.goal_dict()}, 200

@goals_bp.get("/<goal_id>/tasks")
def get_tasks_for_goal(goal_id):
goal = validate_goal(goal_id)
tasks_response = [{
"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]

return {
"id": goal.id,
"title": goal.title,
"tasks": tasks_response
}, 200

@goals_bp.put("/<goal_id>")
def update_goal(goal_id):
goal = validate_goal(goal_id)
request_body = request.get_json()

if "title" not in request_body:
abort(make_response({"details": "Invalid data"}, 400))

goal.title = request_body["title"]
db.session.commit()

return {"goal": goal.goal_dict()}, 200

@goals_bp.delete("/<goal_id>")
def delete_goal(goal_id):
goal = validate_goal(goal_id)

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

return make_response({
"details": f"Goal {goal_id} \"{goal.title}\" successfully deleted"
}, 200)
164 changes: 163 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,163 @@
from flask import Blueprint
from flask import Blueprint, request, jsonify, make_response, abort
from app.models.task import Task
from ..db import db
from datetime import datetime
from app.slack_service import send_slack_message


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

def validate_task(task_id):
try:
task_id = int(task_id)
except:
abort(make_response({"message": f"Task {task_id} not found"}, 400))

task = Task.query.get(task_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The <model>.query syntax has been deprecated since SQLAlchemy 2.0 and should not be used for new development. I would like you to revisit this project and update the code across the files to use the current SQLAlchemy syntax covered in Learn.

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

# POST: Create a new task
@tasks_bp.post("")
def create_task():
request_body = request.get_json()

if "title" not in request_body or "description" not in request_body:
abort(make_response({"details": "Invalid data"}, 400))

new_task = Task(
title=request_body["title"],
description=request_body["description"],
completed_at=None
)
db.session.add(new_task)
db.session.commit()

return make_response({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we have a to_dict function for Tasks, we should use it across the file for our Task responses to reduce repeated code. We can also remove the use of make_response and directly return a tuple of our response dictionary and our status code.

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

"task": {
"id": new_task.id,
"title": new_task.title,
"description": new_task.description,
"is_complete": False
}
}, 201)

# GET all tasks
@tasks_bp.get("")
def get_all_tasks():
# Start the base query without resetting it
query = db.select(Task)
sort_order = request.args.get("sort", "asc")
if sort_order == "desc":
query = query.order_by(Task.title.desc())
else:
query = query.order_by(Task.title.asc())

# Execute the query
tasks = db.session.scalars(query)
Comment on lines +50 to +58
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the syntax we should be using for querying any time we are trying to query for a result that could be more than a single record and may be applying filters and sorting to the results. When we know we only want a single record and have the primary key, the shorthand syntax used in mark_task_complete is valid:

db.session.get(Task, task_id)


# Build the response
tasks_response = [{
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": task.completed_at is not None
} for task in tasks]
Comment on lines +61 to +66
Copy link
Collaborator

@kelsey-steven-ada kelsey-steven-ada Nov 13, 2024

Choose a reason for hiding this comment

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

This comprehension is a bit hard to read because of being split across lines. If you are creating the Task dictionary here, I would consider writing out the loop since it would be easier to read quickly. If we want a more concise solution, using a list comprehension with the Task's to_dict function would work great:

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


return jsonify(tasks_response), 200
Copy link
Collaborator

Choose a reason for hiding this comment

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

jsonify is not required to convert lists and dictionaries to JSON in more recent versions of Flask. Unless it is necessary for the response, we should not use it since it's one more statement we need to consider and maintain over time which isn't changing the flow of our code.


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

return {
"task": task.task_dict()
}, 200


# PUT: Update an existing task
@tasks_bp.put("/<task_id>")
def update_task(task_id):
task = Task.query.get(task_id)

if task is None:
abort(make_response({"message": f"Task {task_id} not found"}, 404))
Comment on lines +83 to +86
Copy link
Collaborator

Choose a reason for hiding this comment

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

How could we reuse validate_model here and in the following functions to reduce some similar code?


request_body = request.get_json()

if "title" not in request_body or "description" not in request_body:
abort(make_response({"details": "Invalid data"}, 400))


task.title = request_body.get("title", task.title)
task.description = request_body.get("description", task.description)

db.session.commit()

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

# PATCH: Partial update tasks
@tasks_bp.patch("/<task_id>/mark_complete")
def mark_task_complete(task_id):
task = db.session.get(Task, task_id)

if task is None:
abort(make_response({"message": f"Task {task_id} not found"}, 404))

# task.title = request.json.get("title", task.title)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented code to clean up.

task.completed_at = datetime.now().date()
db.session.commit()

send_slack_message(task.title)

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

@tasks_bp.patch("/<task_id>/mark_incomplete")
def mark_task_incomplete(task_id):
task = db.session.get(Task, task_id)

if task is None:
abort(make_response({"message": f"Task {task_id} not found"}, 404))

task.completed_at = None
db.session.commit()

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

# DELETE: Delete a task
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see comments like this above some of the other routes, these comments echo information we have in the decorator and the function name, so I would suggest removing them since they aren't giving the reader new info.

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

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

return make_response({
"details": f"Task {task_id} \"{task.title}\" successfully deleted"
}, 200)



28 changes: 28 additions & 0 deletions app/slack_service.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import os
Copy link
Collaborator

Choose a reason for hiding this comment

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

Really nice organization moving this out to its own file.

import requests

SLACK_CHANNEL = "task-notifications"
SLACK_BASE_URL = "https://slack.com/api/chat.postMessage"

def send_slack_message(task_title):
slack_token = os.environ.get("SLACK_API_TOKEN")

if not slack_token:
return "Slack API token not found"

message = f"Someone just completed the task: {task_title}"

headers = {
"Authorization": f"Bearer {slack_token}",
"Content-Type": "application/json"
}

notification = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The notification variable contains more that just the notification message, I suggest changing the variable name to reflect that.

"channel": SLACK_CHANNEL,
"text": message
}

response = requests.post(SLACK_BASE_URL, json=notification, headers=headers)

if response.status_code != 200:
return (f"Failed to send Slack Message: {response.status_code}, {response.text}")
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