-
Notifications
You must be signed in to change notification settings - Fork 128
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
base: main
Are you sure you want to change the base?
Zoisite - Jackie A. #126
Changes from all commits
ac009bc
6eee42c
7ab4209
42b6c54
957e007
eca3eb0
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 |
---|---|---|
|
@@ -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
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. The default value for |
||
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 | ||
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. Instead of hardcoding 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 |
This file was deleted.
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. 👍 yep, every directory needs an |
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
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. 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
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. 👍 |
||
|
||
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
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. 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
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. 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"] | ||
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. 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"] | ||
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. 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) | ||
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. Instead of explicitly using |
||
task.goal_id = goal.id | ||
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. 👍 |
||
|
||
db.session.commit() | ||
|
||
response_message = {"id": int(goal_id), "task_ids": task_ids} | ||
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. 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) | ||
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. We don't need to use Task.query.filter_by actually. Since a goal has associated tasks, we can access the goal's So this can be refactored to |
||
tasks_response = [] | ||
|
||
if tasks: | ||
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. No need for line 103. if |
||
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
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. 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)) | ||
|
||
|
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
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. 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
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. 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") | ||
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. 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 | ||
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 noticed that you only invoke Since you're not doing anything with the return value, do we need to return anything? |
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
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. 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
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. 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
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. 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 = 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
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. No need to check if |
||
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
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 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
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'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
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. 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 | ||
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. Can this logic be moved into the to_dict method in the Task class? |
||
|
||
db.session.commit() | ||
|
||
post_slack_message(task.title) | ||
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. 👍 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 | ||
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. Prefer this logic to be moved into to_dict() |
||
|
||
return make_response(jsonify({"task" : task_dict})) |
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.
👍 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.