-
Notifications
You must be signed in to change notification settings - Fork 111
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
Sharks Kassidy Buslach task-list-api #112
base: master
Are you sure you want to change the base?
Changes from all commits
f1cb776
48637a5
c34485c
11ec58c
b79425b
08597b1
52a6fc0
6e1e810
fea2bf0
396ec29
e542494
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,37 @@ | |
|
||
|
||
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.String) | ||
tasks = db.relationship("Task", back_populates="goal", lazy = True) | ||
|
||
def goal_to_json(self): | ||
json_response = { | ||
"id": self.goal_id, | ||
"title": self.title | ||
} | ||
if self.tasks: | ||
json_response["tasks"]= self.title | ||
Comment on lines
+14
to
+15
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. It looks like you do a check here to see if tasks is a non empty value. Then you assign the value for json_response["tasks"] to self.title. Should line 15 be something like this instead? json_response["tasks"]= self.tasks |
||
|
||
return json_response | ||
|
||
def update_goal(self, request_body): | ||
self.title = request_body["title"] | ||
|
||
|
||
@classmethod | ||
def create_goal(cls, request_body): | ||
new_goal = cls( | ||
title = request_body["title"]) | ||
|
||
return new_goal | ||
|
||
|
||
def add_tasks(self, response_body): | ||
if response_body["task_ids"]: | ||
self.tasks = response_body["task_ids"] | ||
|
||
|
||
|
||
|
||
# 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. Remember to remove commented out code to keep your files clean and to prevent a situation where code is accidentally uncommented and executes when it shouldn't. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
from .task import Task | ||
from .goal import Goal | ||
from flask import abort, make_response | ||
|
||
def validate_id(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. You could rename validate_id() to be more specific since you have a Task and a Goal model, something like |
||
try: | ||
task_id = int(task_id) | ||
except: | ||
return abort(make_response({"message": f"Task {task_id} is not valid"}, 400)) | ||
task = Task.query.get(task_id) | ||
if not task: | ||
return abort(make_response({"message": f"Task {task_id} does not exist"}, 404)) | ||
|
||
return task | ||
|
||
|
||
def validate_goal_id(goal_id): | ||
try: | ||
goal_id = int(goal_id) | ||
except: | ||
return abort(make_response({"message": f"Goal {goal_id} is not valid"}, 400)) | ||
goal = Goal.query.get(goal_id) | ||
if not goal: | ||
return abort(make_response({"message": f"Goal {goal_id} does not exist"}, 404)) | ||
|
||
return goal | ||
|
||
def validate_data(request_body): | ||
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 helper function here to help handle errors if a request doesn't have all the necessary data. We can't assume that clients will always send us correct requests so this a great way to handle the error and send back some feedback to the client. Consider renaming the method to be a little more descriptive like validate_task_request() |
||
# return abort(make_response(f"Invalid data"),400) | ||
if 'title' not in request_body: | ||
return abort(make_response({"details":f"Invalid data"},400)) | ||
elif 'description' not in request_body: | ||
return abort(make_response({"details":f"Invalid data"},400)) | ||
return request_body | ||
|
||
def validate_goal(request_body): | ||
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 could rename this to |
||
if 'title' not in request_body: | ||
return abort(make_response({"details":f"Invalid data"},400)) | ||
return request_body |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,49 @@ | ||
from app import db | ||
from flask import make_response | ||
from sqlalchemy import asc | ||
from datetime import datetime | ||
from app.models.goal import Goal | ||
|
||
|
||
class Task(db.Model): | ||
task_id = db.Column(db.Integer, primary_key=True) | ||
task_id = db.Column(db.Integer, primary_key=True, autoincrement = True) | ||
title = db.Column(db.String) | ||
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. Consider adding nullable=False to ensure every task requires a title. |
||
description = db.Column(db.String) | ||
completed_at = db.Column(db.DateTime, nullable = True, default = None) | ||
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id')) | ||
goal = db.relationship("Goal", back_populates="tasks") | ||
|
||
def to_json(self): | ||
json_response = { | ||
"id": self.task_id, | ||
"title": self.title, | ||
"description":self.description | ||
} | ||
if self.completed_at: | ||
json_response["is_complete"] = True | ||
else: | ||
json_response["is_complete"] = False | ||
Comment on lines
+22
to
+25
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 could also be written with a ternary assignment like: json_response["is_complete"] = True if self.completed_at else False You can read more about the ternary operator here |
||
|
||
if self.goal_id: | ||
json_response["goal_id"] =self.goal_id | ||
|
||
return json_response | ||
|
||
def update_task(self, request_body): | ||
self.title = request_body["title"] | ||
self.description = request_body["description"] | ||
Comment on lines
+33
to
+34
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. request_body["title"] and request_body["description"] can throw KeyErrors if the request_body doesn't have these keys because the client sent a request without all the data. Consider calling your helper function |
||
if self.completed_at: | ||
if self.completed_at == request_body['completed_at']: | ||
Comment on lines
+35
to
+36
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 think lines 35 and 36 can be combined into if request_body['completed_at']: since you want to check that completed_at exists in the request_body so you don't run into a KeyError |
||
self.completed_at = datetime.utcnow() | ||
|
||
|
||
|
||
@classmethod | ||
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 class method for creating a task. I like that you used the get() method on the request_body dictionary which doesn't throw an exception and allows you to pass in a default value! |
||
def create_task(cls, request_body): | ||
|
||
new_task = cls(title = request_body["title"], | ||
description = request_body["description"], | ||
completed_at = request_body.get("completed_at", None) | ||
) | ||
|
||
return new_task |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,175 @@ | ||
from flask import Blueprint | ||
from flask import Blueprint, jsonify, make_response, request, abort | ||
from requests import session | ||
from app.models.task import Task | ||
from app.models.goal import Goal | ||
from app.models.helper import validate_id, validate_data, validate_goal_id, validate_goal | ||
from app import db | ||
from sqlalchemy import asc, desc | ||
from datetime import datetime | ||
import requests | ||
import os | ||
|
||
task_db = Blueprint("tasks",__name__, url_prefix = "/tasks") | ||
goal_db = Blueprint("goals", __name__, url_prefix = "/goals") | ||
Comment on lines
+12
to
+13
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. Consider creating a directory called routes under your app directory and splitting your routes for Task and Goal into separate files (goal_routes.py and task_routes.py) so that you have 2 shorter files separated by class type. |
||
|
||
# guard clause for invalid sort request | ||
@task_db.route("", methods = ["GET"]) | ||
def get_all_tasks(): | ||
task_response = [] | ||
sort_query = request.args.get("sort") | ||
if not sort_query: | ||
ordered_tasks = Task.query.all() | ||
Comment on lines
+20
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. You could also putting this logic in an else block so your conditional statements would be like
|
||
elif sort_query == "asc": | ||
ordered_tasks = Task.query.order_by(asc(Task.title)).all() | ||
elif sort_query == "desc": | ||
ordered_tasks = Task.query.order_by(desc(Task.title)).all() | ||
Comment on lines
+22
to
+25
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. with order_by() you don't need to use chain .all() to the end of these statements |
||
|
||
for task in ordered_tasks: | ||
task_response.append(task.to_json()) | ||
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. This works well and is very readable, but if you'd like to incorporate list comprehensions into your code, you could write it like this: task_response = [task.to_json() for task in ordered_tasks] |
||
return jsonify(task_response), 200 | ||
|
||
@task_db.route("/<task_id>", methods = ["GET"]) | ||
def get_one_task(task_id): | ||
task_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. Looks like task_response isn't being used and can be deleted |
||
task = validate_id(task_id) | ||
return jsonify({"task": task.to_json()}), 200 | ||
|
||
@task_db.route("", methods = ["POST"]) | ||
def create_one_response(): | ||
request_body = request.get_json() | ||
valid_data = validate_data(request_body) | ||
new_task = Task.create_task(valid_data) | ||
db.session.add(new_task) | ||
db.session.commit() | ||
return jsonify({"task":new_task.to_json()}), 201 | ||
|
||
|
||
@task_db.route("/<task_id>", methods = ["PUT"]) | ||
def update_task(task_id): | ||
task = validate_id(task_id) | ||
request_body = request.get_json() | ||
task.update_task(request_body) | ||
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 have error handling for the request that gets sent to the POST endpoint for /tasks. You also need to have error handling for a PUT request in case the client doesn't send a valid request. I left a comment in your Task class for your update_task() method about adding error handling too. You could call validate_data() directly in the update_task() function. Or you could call validate_data() before line 50 here liked you do above on line 40. |
||
|
||
db.session.commit() | ||
return jsonify({"task":task.to_json()}), 200 | ||
|
||
@task_db.route("/<task_id>", methods = ["DELETE"]) | ||
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. 👍 |
||
def delete_task(task_id): | ||
task = validate_id(task_id) | ||
task_title = Task.query.get(task_id) | ||
db.session.delete(task) | ||
db.session.commit() | ||
return { | ||
"details": f'Task {task_id} \"{task_title.title}\" successfully deleted'}, 200 | ||
|
||
# @task_db.route("/<task_id>/mark_complete", methods = ["PATCH"]) | ||
# def mark_task_complete(task_id): | ||
# task = validate_id(task_id) | ||
|
||
# task.completed_at = datetime.now() | ||
|
||
# db.session.commit() | ||
# return jsonify({"task":task.to_json()}), 200 | ||
Comment on lines
+65
to
+72
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. Remove unused code to keep your files clean |
||
|
||
@task_db.route("/<task_id>/mark_incomplete", methods = ["PATCH"]) | ||
def mark_task_incomplete(task_id): | ||
task = validate_id(task_id) | ||
|
||
task.completed_at = None | ||
|
||
db.session.commit() | ||
return jsonify({"task":task.to_json()}), 200 | ||
|
||
TOKEN = os.environ.get("SLACK_TOKEN") | ||
SLACK_URL = os.environ.get("SLACK_URL") | ||
Comment on lines
+83
to
+84
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 can move these two constants into your mark_task_complete_by_slack_bot() method so they're not hanging around in the global scope (since no other function needs to use the constants) |
||
@task_db.route("/<task_id>/mark_complete", methods = ["PATCH"]) | ||
def mark_task_complete_by_slack_bot(task_id): | ||
task = validate_id(task_id) | ||
validated_task = task.query.get(task_id) | ||
task.completed_at = datetime.now() | ||
|
||
headers = {"Authorization":f"Bearer {TOKEN}"} | ||
data = { | ||
"channel":"task-notifications", | ||
"text": f"Someone just completed the task {task.title}." | ||
} | ||
res = requests.post(SLACK_URL, headers=headers, data=data) | ||
Comment on lines
+91
to
+96
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 can put this logic into a helper function and then call the function here. Doing so would help make your route a bit more concise |
||
|
||
db.session.commit() | ||
return jsonify({"task":task.to_json()}), 200 | ||
|
||
|
||
@goal_db.route("/<goal_id>", methods = ["GET"]) | ||
def get_one_goal(goal_id): | ||
goal_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. You can delete goal_response since it's not being used in this method |
||
goal = validate_goal_id(goal_id) | ||
return jsonify({"goal": goal.goal_to_json()}), 200 | ||
|
||
@goal_db.route("", methods = ["GET"]) | ||
def get_all_goals(): | ||
goal_response = [] | ||
goals = Goal.query.all() | ||
for goal in goals: | ||
goal_response.append(goal.goal_to_json()) | ||
|
||
return jsonify(goal_response), 200 | ||
|
||
@goal_db.route("", methods = ["POST"]) | ||
def create_goals(): | ||
request_body = request.get_json() | ||
is_valid = validate_goal(request_body) | ||
new_goal = Goal.create_goal(is_valid) | ||
|
||
db.session.add(new_goal) | ||
db.session.commit() | ||
|
||
return jsonify({"goal":new_goal.goal_to_json()}), 201 | ||
|
||
@goal_db.route("/<goal_id>", methods = ["DELETE"]) | ||
def delete_task(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
goal_title = Goal.query.get(goal_id) | ||
db.session.delete(goal_title) | ||
db.session.commit() | ||
return { | ||
"details": f'Goal {goal_id} \"{goal_title.title}\" successfully deleted'}, 200 | ||
|
||
@goal_db.route("/<goal_id>", methods = ["PUT"]) | ||
def update_task(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
request_body = request.get_json() | ||
goal.update_goal(request_body) | ||
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. Consider using your validate_goal() instance method before line 141 where you do the updating. Calling the validate_goal() from your helper file will raise an exception if the request_body doesn't have the key "title". Without adding in your validate_goal() helper function, update_goal() will raise a KeyError if "title" isn't in the request |
||
|
||
db.session.commit() | ||
return jsonify({"Goal":goal.goal_to_json()}), 200 | ||
|
||
|
||
|
||
@goal_db.route("/<goal_id>/tasks", methods = ["GET"]) | ||
def get_all_goals_and_tasks(goal_id): | ||
valid_goal = validate_goal_id(goal_id) | ||
|
||
task_response = [] | ||
for task in valid_goal.tasks: | ||
task_response.append(task.to_json()) | ||
|
||
|
||
result = {"id": valid_goal.goal_id, | ||
"title":valid_goal.title, | ||
"tasks": task_response} | ||
Comment on lines
+157
to
+159
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 can call your instance method from the Goal class |
||
|
||
return result, 200 | ||
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. Even though we know that this method returns a dict, and that automatically triggers jsonify to happen, I still like to call it explicitly to make it clear that this method will return a JSON response |
||
|
||
@goal_db.route("/<goal_id>/tasks", methods = ["POST"]) | ||
def create_one_goal(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. In this POST request, we are sending an ID of a goal that already exists and also list of task IDs. Consider renaming this method to something like assign_tasks_to_goal() to be more descriptive. |
||
valid_goal = validate_goal_id(goal_id) | ||
request_body = request.get_json() | ||
|
||
|
||
for task_id in 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. You could add error handling here to make sure that the request_body has key "task_ids". |
||
validate_id(task_id) | ||
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. Your validate_id() helper function returns a task object. See lines 10 and 14 in your helper.py file. Since that's the case, you can combine line 170 and 171 into: task = validate_id(task_id) |
||
valid_goal.tasks.append(task) | ||
db.session.commit() | ||
|
||
return {"id":valid_goal.goal_id,"task_ids":request_body["task_ids"]}, 200 | ||
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 comment as above about wrapping this dictionary in the jsonify() method to make it clear that this method will return a JSON response |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Generic single-database configuration. |
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.
Consider adding nullable=False to ensure every goal requires a title.