Skip to content

Tigers - Maria S. #134

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 3 commits into
base: master
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
5 changes: 5 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,10 @@ def create_app(test_config=None):
migrate.init_app(app, db)

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


return app
20 changes: 20 additions & 0 deletions app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,25 @@
from app import db
from sqlalchemy import Column, ForeignKey, Integer, Table
from sqlalchemy.orm import relationship


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=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.goal_id
goal_as_dict["title"] = self.title

if self.tasks:
goal_as_dict["tasks"] = self.tasks.title

Choose a reason for hiding this comment

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

This code causes a 500 error when calling GET /goals/<goal_id> if the goal has tasks associated with it. My guess is you meant something like [task.title for task in self.tasks], but your code here means you're trying to access the title attribute of the list (really an InstrumentedList) rather than its elements.

That being said, rather than returning the title of the tasks here, if you used this for , I would recommend returning the tasks themselves, using the code you wrote for GET /goals/<goal_id>/tasks, [task.to_dict() for task in self.tasks]. Handling this here would have greatly reduced the amount of code needed for your GET /goals/<goal_id>/tasks route as you could just return goal.to_dict.

I do recognize that you cannot return the "tasks" key here as it will cause some failures in the tests you wrote for GET /goals/<goal_id> that don't expect this key. However, I think that could be better fixed by updating the test or writing the test in a more future-proof way I'll describe in a comment on the test.


return goal_as_dict


def from_json(cls, req_body):
return cls(
title = req_body["title"]
)
30 changes: 30 additions & 0 deletions app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,35 @@
from app import db
from sqlalchemy import Column, ForeignKey, Integer, Table
from sqlalchemy.orm import relationship


class Task(db.Model):

Choose a reason for hiding this comment

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

Other than that issue I pointed out, this model class for Task otherwise looks good!

task_id = db.Column(db.Integer, primary_key=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.goal_id'), nullable=True)
goal = db.relationship("Goal", back_populates="tasks", lazy=True)

def to_dict(self):
task_as_dict = {}
task_as_dict["is_complete"] = False

Choose a reason for hiding this comment

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

This doesn't match the specification, as it means that the response will always have "is_complete" set to False, when it should be True if self.completed_at is not None.

While you fix this in your response to PATCH /tasks/<task_id>/mark_complete, this means that if I called PATCH /tasks/13/mark_complete and then GET /tasks/13/mark_complete, the second call will say that "is_complete" is false in contradiction to the first call which says it's true. So even though it has just been marked complete and has an completed_at column in the database, a user would be led to believe that it has not been completed though they marked it as such.

There are a few different ways you could have set "is_complete" here, a very succinct way is bool(self.completed_at).

task_as_dict["id"] = self.task_id
task_as_dict["title"] = self.title
task_as_dict["description"] = self.description

if self.goal_id:
task_as_dict["goal_id"] = self.goal_id

return task_as_dict


def from_json(cls, req_body):
return cls(
title = req_body["title"],
description = req_body["description"],
completed_at = req_body["completed_at"]
)



1 change: 0 additions & 1 deletion app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Empty file.
243 changes: 243 additions & 0 deletions app/routes/routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,243 @@
from os import abort
from app import db
from app.models.task import Task
from app.models.goal import Goal
from flask import Blueprint, jsonify, abort, make_response, request
import datetime
import requests
import os

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

@goals_bp.route("/<goal_id>/tasks", methods=["POST"])

Choose a reason for hiding this comment

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

I originally had a comment here that your file seemed to lack organization, but as I've read more, I now get that it's just these two /goals/<goal_id>/tasks functions that are out-of-place. I would recommend putting these after your /tasks/<task_id>/mark_(in)complete routes.

A common pattern is to arrange routes by increasing size of endpoint URL then by HTTP verbs, which you do seem to follow below. Moving this to after the task completion routes would maintain that organization, which makes your code more maintainable as there's an obvious pattern methods are organized in.

(Though it can be hard to get a good organization that works, I admit!)

def add_task_list_to_goal(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()
list_task_id = []

for task_id in request_body["task_ids"]:
list_task_id.append(task_id)
task = validate_model(Task, task_id)
goal.tasks.append(task)
db.session.commit()
new_goal = {
"id" : goal.goal_id,
"task_ids" : list_task_id
}

return jsonify (new_goal), 200

Choose a reason for hiding this comment

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

In this case where you are returning a dictionary, you don't need jsonify because Flask will convert dictionaries to JSON objects by default. You would need it if you were returning, say, a list, like you do for GET /tasks.

Likewise, if your status code is going to be 200, you don't need to supply it in your return statement, as that is the default. Now in that case, an argument could be made that being explicit here has benefits in... well, being explicit, but I think the benefit of that is marginal here.

Choose a reason for hiding this comment

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

Also, watch spacing! There's an extra space between jsonify and its argument list, which is very minor but can cause a bit of confusion, as the standard style is no space between a function call and its argument list.

Choose a reason for hiding this comment

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

There are a few other places where you use jsonify on a dictionary, but I'll not point them all out to avoid repeating myself.


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

Choose a reason for hiding this comment

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

Naming: Not only is this name misspelled (I'm guessing it should be read_all_tasks), it's not very descriptive, and that name would collide with your other read_all_tasks route for the GET /tasks/ endpoint.

I would recommend something like get_tasks_for_goal as being more descriptive and avoiding the name collision.

(Now this technically isn't a name collision because this function is distinguished from read_all_tasks below by taking in one parameter, but it's a logical name collision that makes your code harder to read.)

goal = validate_model(Goal, goal_id)
try:
response_body = {
"id":goal.goal_id,
"title": goal.title,
"tasks": [task.to_dict() for task in goal.tasks]
}
for task in response_body["tasks"]:
task["goal_id"] = goal.goal_id

# return make_response(jsonify(response_body), 200)
except:
response_body = {
"id":goal.goal_id,
"title": goal.title,
"tasks": []
}
Comment on lines +34 to +49

Choose a reason for hiding this comment

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

This block is a bit more complicated than you really need. Putting this code in a try-except block would make sense if goal.tasks could be None but that actually won't happen as Flask returns an empty list in the case that no tasks are associated with the goal. This means that [task.to_dict() for task in goal.tasks] (good use of a list comprehension, by the way!) will resolve to an empty list itself, so no error will be thrown.

Like I said, this would be different if goal.tasks would return None, which then would necessitate this code. It could be defended as defensive programming, though, though I feel that would be a bit over-defensive.

Likewise, your Task.to_dict() method already sets the "goal_id" key, so the for loop that follows is unnecessary.

So doing both of those changes brings this down to:

response_body = {
    "id": goal.goal_id,
    "title": goal.title,
    "tasks": [task.to_dict() for task in goal.tasks]

return response_body


return jsonify(response_body), 200


@goals_bp.route("", methods=["POST"])
def add_goal():
try:
request_body = request.get_json()
new_goal = Goal.from_json(Goal, request_body)
db.session.add(new_goal)
db.session.commit()
goals_response = {}
goal_in_dict = new_goal.to_dict()
goals_response["goal"] = goal_in_dict

return jsonify (goals_response), 201

except:
response_body = {}
response_body["details"] = "Invalid data"

return response_body, 400

@tasks_bp.route("", methods=["POST"])
def add_task():
try:
request_body = request.get_json()
if "completed_at" not in request_body:
request_body["completed_at"] = None

new_task = Task.from_json(Task, request_body)
db.session.add(new_task)
db.session.commit()
tasks_response = {}
task_in_dict = new_task.to_dict()
tasks_response["task"] = task_in_dict

return jsonify (tasks_response), 201

except:
response_body = {}
response_body["details"] = "Invalid data"

return response_body, 400

@goals_bp.route("", methods=["GET"])
def read_all_goals():
goals = Goal.query.all()
goals_response = []
for goal in goals:
goals_response.append(goal.to_dict())

return jsonify (goals_response), 200

@tasks_bp.route("", methods=["GET"])
def read_all_tasks():
title_query = request.args.get("title")
description_query = request.args.get("description")
completed_query = request.args.get("completed_at")
sort_query = request.args.get("sort")

if sort_query == "desc":
tasks = Task.query.order_by(Task.title.desc())
elif sort_query == "asc":
tasks = Task.query.order_by(Task.title.asc())
elif title_query:
tasks = Task.query.filter_by(title=title_query)
elif description_query:
tasks = Task.query.filter_by(description=description_query)
elif completed_query:
tasks = Task.query.filter_by(completed_at=completed_query)
else:
tasks = Task.query.all()
Comment on lines +111 to +122

Choose a reason for hiding this comment

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

Good extra work on getting these other columns!


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

return jsonify (tasks_response), 200

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

return jsonify (goals_response), 200

@tasks_bp.route("/<task_id>", methods=["GET"])
def read_one_task(task_id):
task = validate_model(Task, task_id)
tasks_response = {}
task_in_dict = task.to_dict()
tasks_response["task"]=task_in_dict

return jsonify (tasks_response), 200

@goals_bp.route("/<goal_id>", methods=["PUT"])
def update_goal(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()
goal.title = request_body["title"]
db.session.commit()
goals_response = {}
goal_in_dict = goal.to_dict()
goals_response["goal"]=goal_in_dict

return jsonify (goals_response), 200

@tasks_bp.route("/<task_id>", methods=["PUT"])
def update_task(task_id):
task = validate_model(Task, task_id)
request_body = request.get_json()
task.title = request_body["title"]
task.description = request_body["description"]
db.session.commit()
tasks_response = {}
task_in_dict = task.to_dict()
tasks_response["task"]=task_in_dict

return jsonify (tasks_response), 200

@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_body = {}
response_body["details"] = (f'Goal {goal_id} "{goal.title}" successfully deleted')

return response_body, 200

@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_body = {}
response_body["details"] = (f'Task {task_id} "{task.title}" successfully deleted')

return response_body, 200

@tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"])
def mark_complete_on_incomplete_task(task_id):
task = validate_model(Task, task_id)
task.completed_at = datetime.datetime.utcnow()
db.session.commit()
slack_bot(f"Someone just completed the task {task.title}")
task_in_dict = task.to_dict()
task_in_dict["is_complete"] = True
tasks_response = {"task" : task_in_dict}

return jsonify (tasks_response), 200

@tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def mark_incomplete_on_complete_task(task_id):
task = validate_model(Task, task_id)
task.completed_at = None
db.session.commit()
tasks_response = {}
task_in_dict = task.to_dict()
tasks_response["task"]=task_in_dict

return jsonify (tasks_response), 200

# slack bot helper func
def slack_bot(msg):
PATH = "https://slack.com/api/chat.postMessage"
SLACK_API_KEY = os.environ.get("SLACK_API_KEY")

query_params = {
"channel" : "#task-notifications",
"text" : msg
}
headers = {
"Authorization" : f"Bearer {SLACK_API_KEY}"
}

requests.post(PATH, params=query_params, headers=headers)

# verify id
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
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