-
Notifications
You must be signed in to change notification settings - Fork 111
Sea Turtles - Esther Annorzie #96
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?
Changes from 28 commits
c359154
da8b4b9
522cdb1
f82647d
ff50e45
355fbb5
1c54fb7
807ad61
533d1bd
6631836
507e35f
f00d20d
c0a44b4
8321582
6d83c97
84e7a21
f14d1ad
99e989c
a511f84
c015c33
62c5100
77e105c
9250b1b
582d476
15a79a8
bf93f05
f3a7cf3
2758c5d
ce9e220
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 |
---|---|---|
|
@@ -30,5 +30,10 @@ def create_app(test_config=None): | |
migrate.init_app(app, db) | ||
|
||
# Register Blueprints here | ||
from .task_routes import bp | ||
app.register_blueprint(bp) | ||
|
||
from .goal_routes import bp | ||
app.register_blueprint(bp) | ||
Comment on lines
+33
to
+37
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. Great choice to divide the routes into their own files! |
||
|
||
return app |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
from flask import Blueprint, jsonify, make_response, request | ||
from app.models.task import Task | ||
from app.models.goal import Goal | ||
from .routes_helper import validate_goal_id, create_message, validate_task_id | ||
from app import db | ||
|
||
|
||
bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
||
|
||
@bp.route("", methods=("POST",)) | ||
def create_goal(): | ||
request_body = request.get_json() | ||
if "title" not in request_body: | ||
create_message("Invalid data", 400) | ||
|
||
goal = Goal.from_dict(request_body) | ||
db.session.add(goal) | ||
db.session.commit() | ||
|
||
return make_response(jsonify({"goal": goal.to_dict()}), 201) | ||
|
||
|
||
@bp.route("/<goal_id>/tasks", methods=("POST",)) | ||
def add_tasks_to_goal(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
request_body = request.get_json() | ||
|
||
tasks_list = [] | ||
for task_id in request_body["task_ids"]: | ||
task = validate_task_id(task_id) | ||
task.goal = goal | ||
tasks_list.append(task.task_id) | ||
|
||
db.session.commit() | ||
return make_response(jsonify({"id": goal.goal_id, | ||
"task_ids": tasks_list})) | ||
Comment on lines
+36
to
+37
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 line wrapping! Though it takes more lines, another way we could format this to help the structure be really clear at a quick glance might be: return make_response(
jsonify({
"id": goal.goal_id,
"task_ids": tasks_list
})
) |
||
|
||
|
||
@bp.route("/<goal_id>", methods=("GET",)) | ||
def read_goal(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
return make_response(jsonify({"goal": goal.to_dict()})) | ||
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 the pattern of calling def get_success_response(response_data, response_code = 200):
return make_response(jsonify(response_data), response_code) |
||
|
||
|
||
@bp.route("", methods=("GET",)) | ||
def read_all_goals(): | ||
goals = Goal.query.all() | ||
goals_response = [goal.to_dict() for goal in goals] | ||
return make_response(jsonify(goals_response)) | ||
|
||
|
||
@bp.route("/<goal_id>/tasks", methods=("GET",)) | ||
def read_specific_goal_tasks(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
|
||
goal_tasks = [Task.to_dict(task) for task in goal.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. Great use of list comprehensions across the code! |
||
|
||
return make_response(jsonify({"id": goal.goal_id, | ||
"title": goal.title, "tasks": goal_tasks})) | ||
|
||
|
||
|
||
@bp.route("/<goal_id>", methods=("PUT",)) | ||
def replace_goal(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
request_body = request.get_json() | ||
goal.override_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. Right now this function returns a 500 error without a specific error message if the user forgets or misspells the "title" key in the PUT request, because of the KeyError that will be generated by the This feedback applies to where the |
||
db.session.commit() | ||
|
||
return jsonify({"goal": goal.to_dict()}) | ||
|
||
|
||
@bp.route("/<goal_id>", methods=("DELETE",)) | ||
def delete_goal(goal_id): | ||
goal = validate_goal_id(goal_id) | ||
db.session.delete(goal) | ||
db.session.commit() | ||
|
||
create_message(f'Goal {goal_id} "{goal.title}" successfully deleted') |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,3 +3,23 @@ | |||||||||||||||||
|
||||||||||||||||||
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) | ||||||||||||||||||
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 we check out the SQLAlchemy docs for loading items: https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html the docs say:
Doing some more digging about the possible lazy parameter values we might find further info like in https://medium.com/@ns2586/sqlalchemys-relationship-and-lazy-parameter-4a553257d9ef where we see the line:
Putting these together we can see that the default value for tasks = db.relationship("Task", back_populates="goal") |
||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
@classmethod | ||||||||||||||||||
def from_dict(cls, data_dict): | ||||||||||||||||||
return cls( | ||||||||||||||||||
title=data_dict["title"] | ||||||||||||||||||
) | ||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
def to_dict(self): | ||||||||||||||||||
return dict( | ||||||||||||||||||
id=self.goal_id, | ||||||||||||||||||
title=self.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. The indentation inside the returns in the
Suggested change
|
||||||||||||||||||
|
||||||||||||||||||
|
||||||||||||||||||
def override_goal(self, data_dict): | ||||||||||||||||||
self.title = data_dict["title"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,43 @@ | ||
from requests import request | ||
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 import isn't used by the |
||
from app import db | ||
|
||
|
||
class Task(db.Model): | ||
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, default=None) | ||
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 choices to ensure |
||
goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id')) | ||
goal = db.relationship("Goal", back_populates="tasks") | ||
|
||
|
||
def to_dict(self): | ||
if self.goal_id is None: | ||
return dict( | ||
id=self.task_id, | ||
title=self.title, | ||
description=self.description, | ||
is_complete=self.completed_at is not None | ||
) | ||
else: | ||
return dict( | ||
id=self.task_id, | ||
title=self.title, | ||
description=self.description, | ||
goal_id=self.goal_id, | ||
is_complete=self.completed_at is not None | ||
) | ||
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 code is mostly duplicated between the if and else blocks here. We could remove the duplication by creating a response dictionary with the items that will always exist on a task, then checking if response_dict = dict(
id=self.task_id,
title=self.title,
description=self.description,
is_complete=self.completed_at is not None
)
if self.goal_id:
response_dict["goal_id"] = self.goal_id
return response_dict |
||
|
||
|
||
@classmethod | ||
def from_dict(cls, data_dict): | ||
return cls( | ||
title=data_dict["title"], | ||
description=data_dict["description"], | ||
completed_at = data_dict.get("completed_at", None) | ||
) | ||
|
||
|
||
def override_task(self, data_dict): | ||
self.title = data_dict["title"] | ||
self.description = data_dict["description"] |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
from flask import make_response, abort | ||
from app.models.task import Task | ||
from app.models.goal import Goal | ||
|
||
|
||
def validate_task_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. Since the primary purpose of this function is to return a task if it exists, rather than validate if a particular task ID is valid, I recommend choosing another function name that reflects the main purpose of the function. |
||
try: | ||
task_id = int(task_id) | ||
except: | ||
create_message("Invalid data", 400) | ||
|
||
task = Task.query.get(task_id) | ||
|
||
if not task: | ||
create_message("Task not found", 404) | ||
return task | ||
|
||
|
||
def validate_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. The feedback about |
||
try: | ||
goal_id = int(goal_id) | ||
except: | ||
create_message("Invalid data", 400) | ||
|
||
goal = Goal.query.get(goal_id) | ||
|
||
if not goal: | ||
create_message("Goal not found", 404) | ||
return goal | ||
|
||
|
||
def create_message(details_info, status_code=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. Since this function only handles abort messages, I'd strongly recommend making that clear in the function name. What are some names that would add clarity about the function's purpose? |
||
abort(make_response({"details": details_info}, status_code)) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
from flask import Blueprint, jsonify, make_response, request | ||
from app.models.task import Task | ||
from .routes_helper import validate_task_id, create_message | ||
from app import db | ||
from datetime import datetime | ||
import requests, os | ||
|
||
|
||
bp = Blueprint("tasks", __name__, url_prefix="/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: | ||
create_message("Invalid data", 400) | ||
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 use of an error message when data is missing! I'd recommend adding explicit info about what key or keys are missing in the response message to help guide the user to what was wrong in their request. The info could be as little as: create_message("Invalid data: title or description is missing", 400) |
||
task = Task.from_dict(request_body) | ||
db.session.add(task) | ||
db.session.commit() | ||
|
||
return make_response(jsonify({"task": task.to_dict()}), 201) | ||
|
||
|
||
@bp.route("/<task_id>", methods=("GET",)) | ||
def read_one_task(task_id): | ||
task = validate_task_id(task_id) | ||
return make_response(jsonify({"task": task.to_dict()})) | ||
|
||
|
||
@bp.route("", methods=("GET",)) | ||
def read_all_tasks(): | ||
title_query = request.args.get("sort") | ||
|
||
if title_query == "asc": | ||
tasks = Task.query.order_by(Task.title.asc()) | ||
elif title_query == "desc": | ||
tasks = Task.query.order_by(Task.title.desc()) | ||
else: | ||
tasks = Task.query.all() | ||
|
||
tasks_response = [task.to_dict() for task in tasks] | ||
|
||
return make_response(jsonify(tasks_response)) | ||
|
||
|
||
@bp.route("/<task_id>", methods=("PUT",)) | ||
def replace_task(task_id): | ||
task = validate_task_id(task_id) | ||
request_body = request.get_json() | ||
task.override_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. The feedback on line 68 of |
||
db.session.commit() | ||
|
||
return jsonify({"task": task.to_dict()}) | ||
|
||
|
||
@bp.route("/<task_id>", methods=("DELETE",)) | ||
def delete_task(task_id): | ||
task = validate_task_id(task_id) | ||
db.session.delete(task) | ||
db.session.commit() | ||
|
||
create_message(f'Task {task_id} "{task.title}" successfully deleted') | ||
|
||
|
||
@bp.route("/<task_id>/mark_complete", methods=("PATCH",)) | ||
def mark_task_complete(task_id): | ||
task = validate_task_id(task_id) | ||
task.completed_at = datetime.utcnow() | ||
db.session.commit() | ||
|
||
requests.post( | ||
url="https://slack.com/api/chat.postMessage", | ||
data={ | ||
"channel": "task-notifications", | ||
"text": f"Someone just completed the task {task.title}" | ||
}, | ||
headers={"Authorization": os.environ.get("token")}, | ||
) | ||
Comment on lines
+76
to
+83
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. Nicely organized post request! |
||
|
||
return jsonify({"task": task.to_dict()}) | ||
|
||
|
||
@bp.route("/<task_id>/mark_incomplete", methods=("PATCH",)) | ||
def mark_task_incomplete(task_id): | ||
task = validate_task_id(task_id) | ||
task.completed_at = None | ||
db.session.commit() | ||
return jsonify({"task": task.to_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.
As we build our skills, something to fold into our process should be reviewing our code when we open PRs to make sure we haven't made unintended changes to other files.