Skip to content

Tigers - Mica C. #124

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 31 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
da8a278
ran flask db init
mc-dev99 Nov 3, 2022
6c8bc83
updated Task model
mc-dev99 Nov 3, 2022
da8f6bf
ran flask migrate and upgrade for Task model
mc-dev99 Nov 3, 2022
518c2cf
added autoincrement=True to Task task_id attr
mc-dev99 Nov 6, 2022
36b073b
added from_dict and to_dict to Task model
mc-dev99 Nov 6, 2022
a6b91e0
created & imported tasks_bp to __init__
mc-dev99 Nov 6, 2022
7089d57
implemented create_task route
mc-dev99 Nov 6, 2022
2df39c9
restored, refactoring with from_dict raises error
mc-dev99 Nov 7, 2022
42a46a6
refactored Task model from_dict method
mc-dev99 Nov 7, 2022
74892e8
refactored Task model to_dict & is_complete helper
mc-dev99 Nov 7, 2022
7794a65
refactored removed old code
mc-dev99 Nov 7, 2022
904ccba
added validate_model, implemented get_all_routes
mc-dev99 Nov 9, 2022
e99987f
implemented get task
mc-dev99 Nov 9, 2022
b6753b9
wave 01 complete,
mc-dev99 Nov 9, 2022
48a1dbd
implemented get_tasks_sorted_asc
mc-dev99 Nov 9, 2022
d99a0d8
implemented get_task_sorted_desc
mc-dev99 Nov 9, 2022
e56518a
completed wave 03
mc-dev99 Nov 9, 2022
8a548fc
reinitialized database, implemented Slackbot post
mc-dev99 Nov 9, 2022
f0e0223
refactored task_routes.py and goal_routes.py
mc-dev99 Nov 9, 2022
db3d4cd
refactored goal_routes and task_routes
mc-dev99 Nov 9, 2022
1037704
implemented create_goal
mc-dev99 Nov 9, 2022
878a374
implemented get_all_goals
mc-dev99 Nov 10, 2022
a47daf0
completed wave 05
mc-dev99 Nov 10, 2022
d820188
added relationship to task and goal models
mc-dev99 Nov 10, 2022
b203c8d
added relationships to task and goal models 2/2
mc-dev99 Nov 10, 2022
a77190d
reinitialized db
mc-dev99 Nov 10, 2022
71b47a3
fixed Task model goal back_populates="tasks"
mc-dev99 Nov 10, 2022
f700f7d
implemented post_task_ids_to_goals
mc-dev99 Nov 10, 2022
206513c
completed wave 06
mc-dev99 Nov 10, 2022
026c963
created Procfile
mc-dev99 Nov 10, 2022
e45260e
refactor task model to_dict
mc-dev99 Nov 10, 2022
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
1 change: 1 addition & 0 deletions Procfile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
web: gunicorn 'app:create_app()'
12 changes: 8 additions & 4 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,17 @@ def create_app(test_config=None):
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get(
"SQLALCHEMY_TEST_DATABASE_URI")

db.init_app(app)
migrate.init_app(app, db)

# Import models here for Alembic setup
from app.models.task import Task
from app.models.goal import Goal

db.init_app(app)
migrate.init_app(app, db)

# Register Blueprints here

from .task_routes import tasks_bp
from .goal_routes import goals_bp
app.register_blueprint(tasks_bp)
app.register_blueprint(goals_bp)

return app
102 changes: 102 additions & 0 deletions app/goal_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
from flask import Blueprint, jsonify, abort, make_response, request
from app import db
from app.models.goal import Goal
from app.models.task import Task
from datetime import datetime
from .task_routes import validate_model
import requests
import os


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

@goals_bp.route("", methods=["POST"])
def create_goal():
request_body = request.get_json()

if (("title") not in request_body):

Choose a reason for hiding this comment

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

You shouldn't need the parentheses around "title" here, you can use a string directly with in against a dictionary.

return make_response({"details": "Invalid data"}, 400)

new_goal = Goal.from_dict(request_body)
db.session.add(new_goal)
db.session.commit()

goal_response = Goal.query.get(1)
return make_response({"goal": goal_response.to_dict()}, 201)

@goals_bp.route("", methods=["GET"])
def get_all_goals():
sort_query = request.args.get("sort")
if sort_query == "asc":
goals = Goal.query.order_by(Goal.title)
elif sort_query == "desc":
goals = Goal.query.order_by(Goal.title.desc()) #ColumnElement.desc() method produces a descending ORDER BY clause element

Choose a reason for hiding this comment

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

I'm waffling on this comment. This is right on the border of "why" versus "what" but I feel it's more on the "what" size as it describes something about the underlying SQL implementation. That very well can be a relevant topic for a comment, but in this case I think the exact details aren't really adding any more information than the code already has.

I think the biggest reason why is because it's not a comment so much about your code but about SQLAlchemy. And while we can't assume a future maintainer of your code knows all the in and outs of SQLAlchemy as you now do, it's the job of their documentation to teach them that, not you. :3

else:
goals = Goal.query.all()

goals_response = []
for goal in goals:
goals_response.append(goal.to_dict())
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({"goal": goal.to_dict()}, 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()

return make_response({"goal": goal.to_dict()}, 200)
Comment on lines +49 to +57

Choose a reason for hiding this comment

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

Minor Style Nitpick: These blank lines feel a little extraneous, actually making the code harder to read, in my opinion. I would group the first three lines together as a "block" since they are all dealing with updating the title:

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

I know in other cases the code here would be complicated enough in each block that you separated them, but sometimes if each block of code is one line or less it ends up feeling just too spacious as it does in this case.


@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()

return make_response(
{"details": f"Goal {goal.goal_id} \"{goal.title}\" successfully deleted"}, 200
)

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

task_list = []
for id in task_ids:
task = Task.query.get(id)
task_list.append(task)
goal.tasks = task_list
Comment on lines +76 to +80

Choose a reason for hiding this comment

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

Note that this would blow away any existing Tasks associated with the Goal, which may not be desirable.

We actually don't specify it in the requirements, so this is fine, but I think in a lot of cases, the preference would be that this would be adding the passed in tasks to that goal, in addition to the tasks that already are part of that goal. So you would have to append them instead.


db.session.commit()

return make_response(
{"id": goal.goal_id,
"task_ids": task_ids},
200
)

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

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

return make_response(
{"id": goal.goal_id,
"title": goal.title,
"tasks": tasks_response}
)
15 changes: 14 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,17 @@


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
goal_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.Text)
tasks = db.relationship("Task", back_populates="goal", lazy=True)

@classmethod
def from_dict(cls, goal_data):
new_task = Goal(title=goal_data["title"])
return new_task
Comment on lines +9 to +12

Choose a reason for hiding this comment

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

Note that a @classmethod like this takes in a cls variable that represents the class type, which is Goal in this case. And that means you can actually use cls as the constructor here:

    @classmethod
    def from_dict(cls, goal_data):
        new_task = cls(title=goal_data["title"])
        return new_task

It's very minor, but the benefit is that if you ever rename the Goal class, you won't have to change it here also. Not life changing, but nice to have.

Choose a reason for hiding this comment

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

Same with the from_dict in Task.


def to_dict(self):
return {
"id": self.goal_id,
"title": self.title
}
31 changes: 30 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,33 @@


class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True, autoincrement=True) #add autoincrement=True?

Choose a reason for hiding this comment

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

This seems like a note comment, which is completely fine to use, but I wager the question you had was answered considering autoincrement=True is in your call to db.Column.

title = db.Column(db.Text)
description = db.Column(db.Text)
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")

@classmethod
def from_dict(cls, task_data):
new_task = Task(title=task_data["title"],
description=task_data["description"],
completed_at=None)
return new_task

def is_complete(task_data):
if task_data.completed_at == None:
return False
else:
return True
Comment on lines +20 to +23

Choose a reason for hiding this comment

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

Note that operators like == return a boolean value. So you can just replace this entire if-else block with return task_data.completed_at != None. (Note that I changed == to !=.)


def to_dict(self):
task = {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": self.is_complete()
}
if self.goal_id:
task["goal_id"] = self.goal_id
return task
1 change: 0 additions & 1 deletion app/routes.py

This file was deleted.

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


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

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
Comment on lines +12 to +21

Choose a reason for hiding this comment

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

There's nothing wrong with this implementation of validate_model but considering this is used in both goal_routes and task_routes, I would consider pulling it out into its own file (perhaps under the models folder, something like models/model_utils).

The main benefit would be that if I was in, say goal_routes and I wanted to know what the validate_model does, I wouldn't expect to look in task_routes, where it is. Kind of just part of making your code maintainable.


@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):
Comment on lines +27 to +28

Choose a reason for hiding this comment

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

Same as in goal_routes, you can just pass the strings directly to not in.

return make_response({"details": "Invalid data"}, 400)

new_task = Task.from_dict(request_body)
db.session.add(new_task)
db.session.commit()

task_response = Task.query.get(1)

Choose a reason for hiding this comment

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

This seems odd, as it causes a bug. In this case you will always return the Task with ID 1. So even when you're adding the seventh Task, you'll get back that same one. Which seems odd, given that new_task already should have the right Task information.

There has been instances where that new_task was missing like an id and then I had to use db.session.refresh I believe to get it, but using a hardcoded value like 1 is especially fragile. It works with our tests because we never create more than one task, but but that's on our tests being rather flimsy.

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

@tasks_bp.route("", methods=["GET"])
def get_all_tasks():
sort_query = request.args.get("sort")
if sort_query == "asc":
tasks = Task.query.order_by(Task.title)
elif sort_query == "desc":
tasks = Task.query.order_by(Task.title.desc()) #ColumnElement.desc() method produces a descending ORDER BY clause element
else:
tasks = Task.query.all()

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

@tasks_bp.route("/<task_id>", methods=["GET"])
def read_one_task(task_id):
task = validate_model(Task, task_id)
return make_response({"task": task.to_dict()}, 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()

return make_response({"task": task.to_dict()}, 200)
Comment on lines +58 to +69

Choose a reason for hiding this comment

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

Yes, for example this spacing is fine here. I'm guessing this is likely the code you copy-pasted for the same function in Goal, and here just that one extra line on line 65 makes this feel less arbitrarily spaced.


@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()

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

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

task.completed_at = datetime.utcnow()
db.session.commit()

#send notif to Slack API
params = {"channel": "task-notifications", "text": f"Someone just completed the task {task.title}"}
header = {"Authorization": f"Bearer {os.environ.get('SLACKBOT_AUTH_TOKEN')}"}
requests.post("https://slack.com/api/chat.postMessage", params=params, headers=header)
Comment on lines +87 to +90

Choose a reason for hiding this comment

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

Surprisingly I'm fine with the comment here even though it's a "what" comment. This code is pretty hairy compared to everything else, so it needs a little describing.

My preference would be to handle that hairiness in a helper function, the name of which would serve much of the same purpose as your comment here. So something like send_slack_notification.

But given that this is the only place it gets used, I am fine with just a comment.


return make_response({"task": task.to_dict()}, 200)

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

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

return make_response({"task": task.to_dict()}, 200)
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