-
Notifications
You must be signed in to change notification settings - Fork 146
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
base: master
Are you sure you want to change the base?
Tigers - Mica C. #124
Changes from all commits
da8a278
6c8bc83
da8f6bf
518c2cf
36b073b
a6b91e0
7089d57
2df39c9
42a46a6
74892e8
7794a65
904ccba
e99987f
b6753b9
48a1dbd
d99a0d8
e56518a
8a548fc
f0e0223
db3d4cd
1037704
878a374
a47daf0
d820188
b203c8d
a77190d
71b47a3
f700f7d
206513c
026c963
e45260e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
web: gunicorn 'app:create_app()' |
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): | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
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} | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that a @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same with the |
||
|
||
def to_dict(self): | ||
return { | ||
"id": self.goal_id, | ||
"title": self.title | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that operators like |
||
|
||
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 |
This file was deleted.
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's nothing wrong with this implementation of The main benefit would be that if I was in, say |
||
|
||
@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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There has been instances where that |
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
@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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Generic single-database configuration. |
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 |
There was a problem hiding this comment.
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 within
against a dictionary.