Skip to content

Sphinx - Salma Anany #27

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 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 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: 3 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,6 @@ dmypy.json
.pytype/

# Cython debug symbols
cython_debug/
cython_debug/

.idea
12 changes: 11 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,15 @@
import os

from dotenv import load_dotenv
from flask import Flask

from .db import db, migrate
from .models import task, goal
import os
from .routes.goal_routes import goal_bp
from .routes.task_routes import tasks_bp

load_dotenv()


def create_app(config=None):
app = Flask(__name__)
Expand All @@ -16,6 +24,8 @@ def create_app(config=None):

db.init_app(app)
migrate.init_app(app, db)
app.register_blueprint(tasks_bp)
app.register_blueprint(goal_bp)

# Register Blueprints here

Expand Down
5 changes: 3 additions & 2 deletions app/db.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from flask_sqlalchemy import SQLAlchemy
from flask_migrate import Migrate
from flask_sqlalchemy import SQLAlchemy

from .models.base import Base

db = SQLAlchemy(model_class=Base)
migrate = Migrate()
migrate = Migrate()
3 changes: 2 additions & 1 deletion app/models/base.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from sqlalchemy.orm import DeclarativeBase


class Base(DeclarativeBase):
pass
pass
20 changes: 19 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.

Consider adding the class method from_dict to this model so you don't need to create an instance of Goal in your routes (example from Flasky)

Original file line number Diff line number Diff line change
@@ -1,5 +1,23 @@
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]

Choose a reason for hiding this comment

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

Consider whether the this column for Goal should be nullable. It feels odd to allow someone to create a goal that gets saved to the DB without a title.

Copy link
Author

Choose a reason for hiding this comment

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

The default is not Nullable, right?

tasks = relationship('Task', back_populates='goal', cascade='all, delete-orphan')

def to_dict(self, noTasks=True):

Choose a reason for hiding this comment

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

we should use snake case for variables in python

Suggested change
def to_dict(self, noTasks=True):
def to_dict(self, no_tasks=True):

result = {"id": self.id, "title": self.title, }
if noTasks:
return result
tasks = []
for task in self.tasks:
tasks.append({"id": task.id,
"goal_id": task.goal_id,
"title": task.title,
"description": task.description,
"is_complete": task.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.

You could also put this logic in a helper function like is_complete

Suggested change
"is_complete": task.completed_at is not None})
"is_complete": task.is_complete()})

result["tasks"] = tasks
return result
23 changes: 22 additions & 1 deletion app/models/task.py

Choose a reason for hiding this comment

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

Same comment as in goal.py, consider adding the class method from_dict to this model.

Original file line number Diff line number Diff line change
@@ -1,5 +1,26 @@
from sqlalchemy.orm import Mapped, mapped_column
from datetime import datetime

from sqlalchemy import Integer, ForeignKey
from sqlalchemy.orm import Mapped, mapped_column, relationship

from ..db import db


class Task(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]

Choose a reason for hiding this comment

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

Same as what I mentioned in goal.py, consider which columns in Task can allow null values. I'd expect title to have nullable=False to ensure that we don't create a task and save it to the db without a value for title.

Copy link
Author

Choose a reason for hiding this comment

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

so you are saying the default for it is to be nullable 🤦‍♀️ , I'll do it, sure.

description: Mapped[str]
completed_at: Mapped[datetime] = mapped_column(nullable=True)

Choose a reason for hiding this comment

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

This white space should be removed

goal_id = db.Column(Integer, ForeignKey('goal.id'), nullable=True)
goal = relationship('Goal', back_populates='tasks')

def to_dict(self):
result = dict(
id=self.id,
title=self.title,
description=self.description,
is_complete=self.completed_at != None)
if self.goal is not None:
result["goal_id"] = self.goal.id
return result
Empty file added app/routes/__init__.py
Empty file.
137 changes: 136 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,136 @@
from flask import Blueprint
from flask import Blueprint, make_response, abort, request

from app import db
from app.models.goal import Goal
from app.models.task import Task

goal_bp = Blueprint("goal_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.

In Learn and in the livecode for Flasky, we name blueprints for each model bp. In goal_routes.py I can deduce that bp is the blueprint for Goal given the name of the file.

If we have multiple route files that each have a blueprint named bp, we can account for name collisions when importing them into app/__init__.py by using aliases:

from .routes.task_routes import bp as tasks_bp
from .routes.goal_routes import bp as goals_bp



@goal_bp.post("")
def create_goal():
request_body = request.get_json()
if "title" not in request_body.keys():
abort(make_response({"details": "Invalid data"}, 400))
title = request_body["title"]
new_goal = Goal(title=title)
db.session.add(new_goal)
db.session.commit()
response = {
"goal": new_goal.to_dict()
}
return response, 201

Choose a reason for hiding this comment

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

This logic here works and meets requirements. It would be great to see use of helper functions like create_model from Flasky (see example here).

By using helper functions, we can keep our route logic concise and concerned with fewer responsibilities because the route would only need to get the request body and call create_model. Then create_model would be responsible for error handling, converting the request body to a Python Goal object, adding/committing the new record to the DB, returning a Response object and status code to the client.

With a create_model helper function, this POST route could be written like:

@goal_bp.post("")
def create_goal():
    request_body = request.get_json()
    return create_model(Goal, request_body)

Copy link
Author

Choose a reason for hiding this comment

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

I did not know about that!, Thanks!



@goal_bp.post("/<goal_id>")
def create_goal_with_id(goal_id):
request_body = request.get_json()
if "title" not in request_body.keys():
abort(make_response({"details": "Invalid data"}, 400))
title = request_body["title"]
new_goal = Goal(id=goal_id, title=title)
db.session.add(new_goal)
db.session.commit()
response = {
"goal": new_goal.to_dict()
}
return response, 201

Choose a reason for hiding this comment

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

This route is nearly identical to the route above called create_goal - notice that only line 16 is different from line 31.

It's unconventional to have a client provide an ID for a record when making a POST request. What happens if goal_id that the client passes in the request is already an ID that was auto assigned to a goal that was previously created?

It seems odd to allow a user to assign an ID value when the attribute is a primary key that is auto generated and there's no way for a client to know what ID they should provide for the goal they want to create.

What did you see when you tested this route? I created a goal which had ID 1, then I sent a POST request to /goals/1 to test this route. This is the error I get:

sqlalchemy.exc.IntegrityError: (psycopg2.errors.UniqueViolation) duplicate key value violates unique constraint "goal_pkey"

Prefer to remove this route entirely.

Copy link
Author

Choose a reason for hiding this comment

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

I am using it for debugging porpuses, when I delete Ids, they get deleted and I cannot call Id1 again, so I wanted to force it, I can remove it.



@goal_bp.get("")
def get_all_tasks():

Choose a reason for hiding this comment

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

Would be nice if all the logic for a GET request to /goals was in a helper method that can be used across different routes. Using a helper here would make this route more concise. Here's how we did it in Flasky

title_param = request.args.get("title")

query = db.select(Goal)
if title_param:
query = query.where(Goal.title.like(f"%{title_param}"))

goals = db.session.scalars(query).all()
goals_response = [goal.to_dict() for goal in goals]
return goals_response


def goal_id_validation(id):
if not id.isnumeric():
abort(make_response({"details": f" {id} Invalid data"}, 400))


@goal_bp.get("/<goal_id>")
def get_one_goal(goal_id):
goal_id_validation(goal_id)
query = db.select(Goal).where(Goal.id == goal_id)
goal = db.session.scalar(query)

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

Choose a reason for hiding this comment

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

Would love to see all this logic encapsulated in a helper function like validate_model (here's how we did it in Flasky)

return {"goal": goal.to_dict()}


@goal_bp.post("/<goal_id>/tasks")
def set_one_goal_tasks(goal_id):
request_body = request.get_json()
goal_id_validation(goal_id)

goal = db.session.get(Goal, int(goal_id))
if not goal:
response = {"message": f"{goal_id} not found"}
abort(make_response(response, 404))

Choose a reason for hiding this comment

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

Same comment as above, it would be preferred for this logic to be encapsulated in a validate_model helper function.


task_ids = request_body.get("task_ids", [])
if not task_ids:
abort(make_response({"message": "No task IDs provided"}, 400))

tasks = db.session.query(Task).filter(Task.id.in_(task_ids)).all()
if len(tasks) != len(task_ids):
abort(make_response({"message": "One or more tasks not found"}, 404))

for task in tasks:
task.goal = goal

db.session.commit()

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


@goal_bp.put("/<goal_id>")
def update_goal(goal_id):
goal_id_validation(goal_id)
query = db.select(Goal).where(Goal.id == goal_id)
goal = db.session.scalar(query)

if goal is None:
abort(make_response({"details": f"Goal {goal_id} not found"}, 404))

request_body = request.get_json()
goal.title = request_body["title"]
db.session.commit()
result = {"goal": goal.to_dict()}
return make_response(result, 200)


@goal_bp.delete("/<goal_id>")
def delete_task(goal_id):
goal_id_validation(goal_id)
query = db.select(Goal).where(Goal.id == goal_id)
goal = db.session.scalar(query)
if not goal:
response = {"details": f"{goal_id} not found."}
abort(make_response(response, 404))

db.session.delete(goal)
db.session.commit()
response = {"details": f'Goal {goal_id} "{goal.title}" successfully deleted'}
return make_response(response, 200)


@goal_bp.get("/<goal_id>/tasks")
def get_one_goal_tasks(goal_id):
goal_id_validation(goal_id)
query = db.select(Goal).where(Goal.id == goal_id)
goal = db.session.scalar(query)

if not goal:
response = {"message": f"{goal_id} not found"}
abort(make_response(response, 404))
return goal.to_dict(False)
145 changes: 144 additions & 1 deletion app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,144 @@
from flask import Blueprint
from datetime import datetime

from flask import request, Blueprint, make_response, abort
from sqlalchemy import asc, desc

from app import db
from app.models.task import Task
from app.slack.slack_client import SlackClient
from app.slack.slackmessage import SlackMessage

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


@tasks_bp.post("")
def create_task():
request_body = request.get_json()
if "title" not in request_body.keys():
abort(make_response({"details": "Invalid data"}, 400))
title = request_body["title"]
if "description" not in request_body.keys():
abort(make_response({"details": "Invalid data"}, 400))
description = request_body["description"]
new_task = Task(title=title, description=description, completed_at=None)
db.session.add(new_task)
db.session.commit()
Comment on lines +17 to +26

Choose a reason for hiding this comment

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

Another great candidate for refactoring to use a create_model helper function so that this route is more concise.

When writing route logic, we might ask ourselves questions like, "Does this route need to know how to process a request, handle potential errors in the request body, create an instance of the object, and add/commit it to the DB?"

We can make our routes concise and readable by using helper methods

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


@tasks_bp.get("")
def get_all_tasks():
description_param = request.args.get("description")
title_param = request.args.get("title")
sort_param = request.args.get("sort")

query = db.select(Task)
if description_param:
query = query.where(Task.description.like(f"%{description_param}%"))

if title_param:
query = query.where(Task.title.like(f"%{title_param}"))
if sort_param == "asc":
query = query.order_by(asc(Task.title))
elif sort_param == "desc":
query = query.order_by(desc(Task.title))

tasks = db.session.scalars(query).all()
tasks_response = [task.to_dict() for task in tasks]
return tasks_response


def validate_id(id):
if not id.isnumeric():
abort(make_response({"message": f"Tasks id {id} invalid"}, 400))

Comment on lines +55 to +58

Choose a reason for hiding this comment

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

I'd expect the logic for a method like validate_id to check that an ID is valid in the database too, not just check that id is an integer. If you have both of these pieces of logic in this method and also put your error handling in this method, you would be able to DRY up many of your routes quite a bit.


@tasks_bp.get("/<task_id>")
def get_one_task(task_id):
validate_id(task_id)
query = db.select(Task).where(Task.id == task_id)
task = db.session.scalar(query)

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

return {"task": task.to_dict()}


@tasks_bp.put("/<task_id>")
def update_task(task_id):
validate_id(task_id)
query = db.select(Task).where(Task.id == task_id)
task = db.session.scalar(query)

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

request_body = request.get_json()
task.title = request_body["title"]
task.description = request_body["description"]
if "is_complete" in request_body.keys() and request_body["is_complete"]:
task.completed_at = datetime.now()
else:
task.completed_at = None
db.session.commit()
return {
"task": task.to_dict()
}


@tasks_bp.delete("/<task_id>")
def delete_task(task_id):
validate_id(task_id)
query = db.select(Task).where(Task.id == task_id)
task = db.session.scalar(query)
if not task:
response = {"message": f"{task_id} not found"}
abort(make_response(response, 404))

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

return {
"details": f'Task {task_id} "{task.title}" successfully deleted'
}


def change_task_status(task_id, is_completed):
task = Task.query.get(task_id)

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. This needs to removed to use the newer syntax:

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

See how we made a generic helper method for validating and fetching records from the DB in Flasky here on lines 10-11

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

if is_completed:
completed_date = datetime.now()
task_text = f"Someone just completed the task '{task.title}'"
else:
completed_date = None
task_text = f"Someone just marked the task '{task.title}' incomplete"

notification_messages = SlackMessage("task-notifications", task_text)
notification_was_sent = slack_client.post_message(notification_messages)
Copy link

@yangashley yangashley Nov 19, 2024

Choose a reason for hiding this comment

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

~~Recall that the project requirement was to use the Python package requests to make 3rd party API calls.

We want folks to get practice using requests and not use the SlackClient because not all 3rd party APIs wil provide you with a custom client.

@SalmaAnany Please let me know, as a repy to this comment, how you would re-write the code for calling Slack using requests.~~

Copy link
Author

Choose a reason for hiding this comment

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

I used requests, please check the client, that's my own class using requests.

Choose a reason for hiding this comment

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

Ah yes, I meant to remove this comment after I saw your SlackClient class! Thanks for pointing me there again.

if notification_was_sent:
Task.query.filter_by(id=task_id).update({Task.completed_at: completed_date})
db.session.commit()
return task


@tasks_bp.patch("/<task_id>/mark_complete")
def mark_completed_task(task_id):
validate_id(task_id)
task = change_task_status(task_id, True)
return make_response({"task": task.to_dict()}, 200)


@tasks_bp.patch("/<task_id>/mark_incomplete")
def mark_in_complete_task(task_id):
validate_id(task_id)
task = change_task_status(task_id, False)
return make_response({"task": task.to_dict()}, 200)
Empty file added app/slack/__init__.py
Empty file.
Loading