Skip to content
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

Zoisite - Jackie A. #126

Open
wants to merge 6 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
8 changes: 7 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def create_app(test_config=None):

if test_config is None:
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_DATABASE_URI")
"RENDER_DATABASE_URI")
else:
app.config["TESTING"] = True
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
Expand All @@ -30,5 +30,11 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here
from .routes.task_routes import tasks_bp
app.register_blueprint(tasks_bp)

from .routes.goal_routes import goals_bp
app.register_blueprint(goals_bp)


return app
17 changes: 16 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,19 @@


class Goal(db.Model):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 LGTM

Remember to make an empty init.py file in any package folder/subfolder. app and routes both have one, but we should have one here in the models folder as well.

goal_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)
tasks = db.relationship("Task", back_populates="goal", lazy=True)

def to_dict(self):
goal_as_dict = {}
goal_as_dict["id"] = self.id
goal_as_dict["title"] = self.title

return goal_as_dict

@classmethod
def from_dict(cls, goal_data):
new_goal = Goal(title=goal_data["title"])

return new_goal
23 changes: 22 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,25 @@


class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True)
goal_id = db.Column(db.Integer, db.ForeignKey("goal.id"), nullable=True)
Comment on lines +8 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default value for nullable is True so we don't need to explicitly add it

goal = db.relationship("Goal", back_populates="tasks")

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
task_as_dict["is_complete"] = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of hardcoding is_complete to False, how could you use the value of self.completed_at to derive the boolean value?

We could use a ternary operator like:

task_as_dict["is_complete"] = True if self.completed_at else False


return task_as_dict

@classmethod
def from_dict(cls, task_data):
new_task = Task(title=task_data["title"],
description=task_data["description"],
completed_at=None)
return new_task
1 change: 0 additions & 1 deletion app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 yep, every directory needs an __init__.py file

Empty file.
113 changes: 113 additions & 0 deletions app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
from app import db
from flask import Blueprint, jsonify, request, make_response, abort
from app.models.task import Task
from app.models.goal import Goal
from app.routes.helper_functions import validate_model
from sqlalchemy import text
import datetime
import requests
import os
Comment on lines +7 to +9
Copy link
Collaborator

Choose a reason for hiding this comment

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

These 3 imports and abort on line 2 are not accessed and should be removed


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


@goals_bp.route("", methods=["POST"])
def create_goal():
request_body = request.get_json()
if "title" not in request_body:
return make_response({ "details": "Invalid data"}, 400)
Comment on lines +17 to +18
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


new_goal = Goal.from_dict(request_body)

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

response = {"goal" : new_goal.to_dict()}

return make_response(jsonify(response), 201)

@goals_bp.route("", methods = ["GET"])
def read_all_goals():
goals = Goal.query.all()

goals_response = []
if not goals:
return jsonify(goals_response)
Comment on lines +34 to +35
Copy link
Collaborator

Choose a reason for hiding this comment

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

If goals is an empty list, then the for loop has nothing to iterate over and line 43 will return the same response as what you have on line 35 so you can remove line 34-35 altogether.

for goal in goals:
goals_response.append(
{
"id": goal.id,
"title": goal.title,
}
Comment on lines +38 to +41
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you use your to_dict() method in the Goal class here?

This for loop would also be a good candidate for list comprehension!

)
return jsonify(goals_response)


@goals_bp.route("/<goal_id>", methods =["GET"])
def read_one_goal(goal_id):
goal = validate_model(Goal, goal_id)

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


@goals_bp.route("/<goal_id>", methods=["PUT"])
def update_goal(goal_id):
request_body = request.get_json()
goal = validate_model(Goal, goal_id)

goal.title = request_body["title"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should have some error handling here in case the request body that the client sent has invalid data


db.session.commit()

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


@goals_bp.route("/<goal_id>", methods=["DELETE"])
def delete_goal(goal_id):
goal = validate_model(Goal, goal_id)

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

response= (f"Goal {goal_id} \"{goal.title}\" successfully deleted")

return make_response(jsonify({"details": response}))


@goals_bp.route("/<goal_id>/tasks", methods=["POST"])
def add_tasks_to_goal(goal_id):
request_body = request.get_json()
task_ids = request_body["task_ids"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add error handling here since we're trying to access a data from the request body that the client sent.


goal = validate_model(Goal, goal_id)

for task_id in task_ids:
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.

Instead of explicitly using Task.query.get() we could rely on the validate_model() helper method you have which also provides error handling too.

task.goal_id = 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.

👍


db.session.commit()

response_message = {"id": int(goal_id), "task_ids": task_ids}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The value referenced by task_ids comes from the request body that the client sent to your API. Instead of returning the values that the client sent in the response message, I'd prefer to get the task ids from the goal object that you validated and then associated tasks with instead.


return make_response(jsonify(response_message))


@goals_bp.route("/<goal_id>/tasks", methods=["GET"])
def get_all_tasks_one_goal(goal_id):
goal = validate_model(Goal, goal_id)

response_message = goal.to_dict()
tasks = Task.query.filter_by(goal=goal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need to use Task.query.filter_by actually. Since a goal has associated tasks, we can access the goal's tasks attribute (see goal.py line 7).

So this can be refactored to tasks = goal.tasks

tasks_response = []

if tasks:
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for line 103. if tasks is an empty list, then the for loop won't execute.

for task in tasks:
task_dict = task.to_dict()
task_dict["goal_id"] = int(goal_id)
tasks_response.append(task_dict)
Comment on lines +105 to +107
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you move the logic on line 106 into the to_dict() method then this for loop could be refactored to use list comprehension.


response_message["tasks"] = tasks_response

return make_response(jsonify(response_message))


33 changes: 33 additions & 0 deletions app/routes/helper_functions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from flask import Blueprint, jsonify, request, make_response, abort
from app.models.task import Task
from app.models.goal import Goal
Comment on lines +1 to +3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Notice that Blueprint, jsonify, request, Task, and Goal are imported but not accessed. Any imports not being accessed should be removed to keep your file clutter free.

import requests
import os

def validate_model(cls, model_id):
try:
model_id = int(model_id)
except:
abort(make_response({"message":f"{cls.__name__} {model_id} invalid"}, 400))

model = cls.query.get(model_id)

if not model:
abort(make_response({"message":f"{cls.__name__} {model_id} not found"}, 404))

return model


def post_slack_message(task_title):
slack_url = "https://slack.com/api/chat.postMessage"
channel_id = "task-notifications"
Comment on lines +22 to +23
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice job putting this logic into a helper function!

Since slack_url and channel_id are constant variables, they should be spelled with all caps.

slack_message = f"Someone just completed the task {task_title}"
headers = dict(
Authorization = os.environ.get("SLACK_AUTH")
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll work on projects where you'll have multiple API calls that need to happen so you'll have multiple keys. Consider renaming this variable to be more specific like SLACK_API_KEY.

Also, I'm assuming that your .env file has "Bearer" in front of your api key since i don't see the word "Bearer" here. Technically, "Bearer" isn't part of the API key. I would have the key in your .env and then prepend the API key in this helper function.

)
data = dict(
channel = channel_id,
text = slack_message
)
response = requests.post(slack_url, headers=headers, data=data)
return response
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed that you only invoke post_slack_message on line 102 in task_routes.py

Since you're not doing anything with the return value, do we need to return anything?

119 changes: 119 additions & 0 deletions app/routes/task_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
from app import db
from flask import Blueprint, jsonify, request, make_response, abort
from app.models.task import Task
from app.models.goal import Goal
from app.routes.helper_functions import validate_model, post_slack_message
from sqlalchemy import text
import datetime
import requests
Comment on lines +6 to +8
Copy link
Collaborator

Choose a reason for hiding this comment

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

the imports on line 5, 6, 8 are not being accessed and should be removed.

import os


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


@tasks_bp.route("", methods = ["POST"])
def create_task():
request_body = request.get_json()
if "title" not in request_body or "description" not in request_body:
return make_response({ "details": "Invalid data"}, 400)
Comment on lines +18 to +19
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice job handling a possible exception if a client sent a request body that didn't have "title" or "description" in it. Another way to handle exceptions related to invalid data from the client would be to use the try/except statement. Using try/except is nice because if the attempt to create a Task object fails, then you can catch the exception and send back an error response to the client.

try:
    new_task = Task.from_dict(request_body)

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

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

    return make_response(jsonify(response), 201)
except KeyError:
        abort(make_response({"details" : "Invalid Data"}))


new_task = Task.from_dict(request_body)

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

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

return make_response(jsonify(response), 201)


@tasks_bp.route("", methods = ["GET"])
def read_all_tasks():
tasks = Task.query.all()

sort_query = request.args.get("sort")
if sort_query == "asc":
tasks = Task.query.order_by(Task.title.asc()).all()
elif sort_query == "desc":
tasks = Task.query.order_by(Task.title.desc()).all()
Comment on lines +33 to +39
Copy link
Collaborator

Choose a reason for hiding this comment

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

On line 33, we should just create the query without calling .all() just yet because you may need to sort (like you have on lines 36-39. Instead, you should add an else block to what you have so if the conditions on line 36 or 38 aren't met then your route will finally call .all() and get all the results without sorting.

Also, if you don't call .all() on line 33, then you can use tasks on lines 37 and 39. That would look like:

    tasks = Task.query

    sort_query = request.args.get("sort")
    if sort_query == "asc":
        tasks = tasks.order_by(Task.title.asc()).all()
    elif sort_query == "desc":
        tasks = tasks.order_by(Task.title.desc()).all()
    else:
        tasks = tasks.all()


tasks_response = []
if not tasks:
return jsonify(tasks_response)
Comment on lines +42 to +43
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to check if tasks is empty and then returning tasks_response within this conditional block. If tasks is empty then the for loop will have nothing to iterate over and then line 53 will return your empty list.

for task in tasks:
tasks_response.append(
{
"id": task.id,
"title": task.title,
"description": task.description,
"is_complete": False
}
)
return jsonify(tasks_response)
Comment on lines +45 to +53
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be a great opportunity to use list comprehension!

Also lines 46-51 look almost identical to your to_dict() method in the Task class. Could you invoke .to_dict on each task instead of repeating code?



@tasks_bp.route("/<task_id>", methods =["GET"])
def read_one_task(task_id):
task = validate_model(Task, task_id)
task_dict = task.to_dict()
if task.goal_id:
task_dict["goal_id"] = int(task.goal_id)
Comment on lines +60 to +61
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd prefer this logic be put in the to_dict method in the Task class. While this works, a task object should be able to turn itself into a dictionary and it feels like half of it is in the Task class and some of it is in the route.


return make_response(jsonify({"task" : task_dict}))


@tasks_bp.route("/<task_id>", methods=["PUT"])
def update_task(task_id):
request_body = request.get_json()

task = validate_model(Task, task_id)

task.title = request_body["title"]
task.description = request_body["description"]
Comment on lines +72 to +73
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just like you have error handling in your POST route, you'll need error handling here (either using if or try/except) because there's a possibility that the client could send you a request body that doesn't have the key "title" or it's misspelled. If that were to happen then line 72 would throw an exception and without handling it, then the client would get a 500 internal server error in response.


db.session.commit()

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


@tasks_bp.route("/<task_id>", methods=["DELETE"])
def delete_task(task_id):
task = validate_model(Task, task_id)

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

response= (f"Task {task_id} \"{task.title}\" successfully deleted")

return make_response(jsonify({"details": response}))


@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def mark_complete(task_id):
task = validate_model(Task, task_id)

task.completed_at = datetime.datetime.today()
task_dict = task.to_dict()
task_dict["is_complete"] = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this logic be moved into the to_dict method in the Task class?


db.session.commit()

post_slack_message(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.

👍 Nice job keeping this route concise by putting this logic in a helper function


return make_response(jsonify({"task" : task_dict}))


@tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def mark_incomplete_incomplete(task_id):
request_body = request.get_json()
task = validate_model(Task, task_id)

task.completed_at = None

db.session.commit()

task_dict = task.to_dict()
task_dict["is_complete"] = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer this logic to be moved into to_dict()


return make_response(jsonify({"task" : task_dict}))
1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Generic single-database configuration.
45 changes: 45 additions & 0 deletions migrations/alembic.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# 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

[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

[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