-
Notifications
You must be signed in to change notification settings - Fork 15
Accelerate - Dena Burd #5
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 all commits
69eaf7c
c03b03d
8786c2d
5a621df
9f8c889
ea97689
c4f320d
f832d84
f7a3c87
411033f
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 |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| from flask import Flask | ||
| from flask import Flask, render_template | ||
| from flask_sqlalchemy import SQLAlchemy | ||
| from flask_migrate import Migrate | ||
| import os | ||
|
|
@@ -12,6 +12,7 @@ | |
|
|
||
| def create_app(test_config=None): | ||
| app = Flask(__name__) | ||
| app.url_map.strict_slashes = False | ||
| app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False | ||
|
|
||
| if test_config is None: | ||
|
|
@@ -30,5 +31,12 @@ def create_app(test_config=None): | |
| migrate.init_app(app, db) | ||
|
|
||
| # Register Blueprints here | ||
| from .routes import tasks_bp, goals_bp | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) | ||
|
Comment on lines
+34
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. 👍 One thing we started to touch on in the video store live code was that we can split routes into multiple files. We can make a routes folder, and put routes for each endpoint into separate files, named for their model. Then we can use the name from .routes import task, goal
app.register_blueprint(task.bp)
app.register_blueprint(goal.bp) |
||
|
|
||
| @app.route("/") | ||
| def index(): | ||
| return render_template("index.html") | ||
|
Comment on lines
+38
to
+40
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 one-off route for your documentation page! |
||
|
|
||
| return app | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,5 @@ | |
|
|
||
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) | ||
| tasks = db.relationship("Task", backref="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. 👍 There are lots of interesting values that |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,3 +4,8 @@ | |
|
|
||
| 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) | ||
| goal_id = db.Column(db.Integer, db.ForeignKey( | ||
|
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. 👍 |
||
| 'goal.goal_id'), nullable=True) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,274 @@ | ||
| from flask import Blueprint | ||
| import requests | ||
| from dotenv import load_dotenv | ||
| import os | ||
| import datetime | ||
| from sqlalchemy import desc | ||
| from app import db | ||
| from app.models.task import Task | ||
| from app.models.goal import Goal | ||
| from flask import request, Blueprint, make_response, jsonify | ||
| tasks_bp = Blueprint("tasks", __name__, url_prefix="/tasks") | ||
| goals_bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
Comment on lines
+10
to
+11
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 could consider putting all the routes related to the tasks endpoint in their own file, and doing the same for the goals routes. |
||
|
|
||
| load_dotenv() | ||
|
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 curious why you decided to put the dotenv stuff here rather than in the |
||
|
|
||
|
|
||
| @tasks_bp.route("/", methods=["GET", "POST"]) | ||
|
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 logic for GET and POST doesn't share any code, so we could consider putting the logic for each in separate functions, maybe |
||
| def handle_tasks(): | ||
| if request.method == "GET": | ||
| 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(desc(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. Can also be written this way, which avoids the need to import desc tasks = Task.query.order_by(Task.title.desc()) |
||
| else: | ||
| tasks = Task.query.all() | ||
|
Comment on lines
+19
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. 👍 |
||
|
|
||
| tasks_response = [] | ||
| for task in tasks: | ||
| if not task.completed_at: | ||
| is_complete = False | ||
| else: | ||
| is_complete = True | ||
| tasks_response.append({ | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": is_complete | ||
| }) | ||
|
Comment on lines
+33
to
+38
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 are many places in your routes where you build a dictionary like this (or very similar). Consider making a helper method, either here in the routes file (e.g. |
||
| return make_response(jsonify(tasks_response), 200) | ||
|
|
||
| elif request.method == "POST": | ||
| request_body = request.get_json() | ||
| keys = ['title', 'description', 'completed_at'] | ||
| for key in keys: | ||
| if key not in request_body: | ||
| task_response = {"details": "Invalid data"} | ||
| return make_response(task_response, 400) | ||
|
Comment on lines
+43
to
+47
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 extensible way to check for fields in the request. Note that this would still let us create a task with an empty title (either empty string or explicitly set to null in the JSON body). We don't have a test that checks this, but it would be good to keep in mind. Additionally, we should be doing similar checks when PUTting a task as well (even though there aren't tests for it). So we could also think about moving checks like this into validation helpers so that they are easier to reuse elsewhere. We could even think about adding a class method to @classmethod
def from_dict(values):
# create a new task, and set the model values from the values passed in
# be sure to validate that all required values are present, we could return `None` or raise an error if needed
return new_task |
||
|
|
||
| new_task = Task(title=request_body["title"], | ||
| description=request_body["description"], | ||
| completed_at=request_body["completed_at"]) | ||
|
|
||
| db.session.add(new_task) | ||
| db.session.commit() | ||
| if not request_body["completed_at"]: | ||
| is_complete = False | ||
| else: | ||
| is_complete = True | ||
| task_response = {"task": | ||
| {"id": new_task.task_id, | ||
| "title": new_task.title, | ||
| "description": new_task.description, | ||
| "is_complete": is_complete} | ||
| } | ||
| return make_response(task_response, 201) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>", methods=["GET", "PUT", "DELETE"]) | ||
| def handle_task(task_id): | ||
| task = Task.query.get(task_id) | ||
| if not task: | ||
| return make_response(f"Task #{task_id} Not Found", 404) | ||
|
Comment on lines
+70
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. 👍 We could also consider using We do need to do this check for GET, PUT, and DELETE requests, but we could still think about splitting these into separate functions (e.g. |
||
|
|
||
| if request.method == "GET": | ||
| if not task.completed_at: | ||
| is_complete = False | ||
| else: | ||
| is_complete = True | ||
| task_response = {"task": { | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": is_complete, | ||
| }} | ||
| if task.goal_id: | ||
| task_response["task"]["goal_id"] = task.goal_id | ||
|
Comment on lines
+85
to
+86
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 way to conditionally add the |
||
| return make_response(task_response, 200) | ||
| elif request.method == "PUT": | ||
| form_data = request.get_json() | ||
| if ("completed_at" not in form_data or | ||
| form_data["completed_at"] == None): | ||
| is_complete = False | ||
| else: | ||
| is_complete = True | ||
| task.title = form_data["title"] | ||
| task.description = form_data["description"] | ||
| task.completed_at = form_data["completed_at"] | ||
|
Comment on lines
+95
to
+97
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 mentioned this already above, but we should be sure that the same fields required for POSTing are included here for PUT. PUT replaces the value for the supplied task id, so we should ensure that all of the values required to represent a |
||
| db.session.commit() | ||
| task_response = {"task": { | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": is_complete | ||
| }} | ||
| return make_response(task_response, 200) | ||
|
|
||
| elif request.method == "DELETE": | ||
| db.session.delete(task) | ||
| db.session.commit() | ||
| task_response = { | ||
| "details": f"Task {task.task_id} \"{task.title}\" successfully deleted" | ||
| } | ||
| return make_response(task_response, 200) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"]) | ||
| def handle_task_complete(task_id): | ||
| task = Task.query.get(task_id) | ||
| if not task: | ||
| return make_response(f"Task #{task_id} Not Found", 404) | ||
|
|
||
| task.completed_at = datetime.datetime.utcnow() | ||
| db.session.commit() | ||
| channel = "channel=task-notifications" | ||
| text = f"text=Someone just completed the task {task.title}" | ||
|
|
||
| path = f"https://slack.com/api/chat.postMessage?{channel}&{text}" | ||
|
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. Rather than us being responsible for adding query params into the url directly, we can make use of the Even better, since this is a post request, we can send the parameters in the post body using the |
||
|
|
||
| SLACKBOT_AUTH = os.environ.get( | ||
| "SLACKBOT_AUTH") | ||
|
|
||
| requests.post(path, headers={'Authorization': f'Bearer {SLACKBOT_AUTH}'}) | ||
|
|
||
| task_response = {"task": { | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": True | ||
| }} | ||
| return make_response(task_response, 200) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"]) | ||
| def handle_task_incomplete(task_id): | ||
| # get current data, set the thing | ||
| task = Task.query.get(task_id) | ||
| if not task: | ||
| return make_response(f"Task #{task_id} Not Found", 404) | ||
|
|
||
| task.completed_at = None | ||
| db.session.commit() | ||
|
|
||
| task_response = {"task": { | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": False | ||
| }} | ||
| return make_response(task_response, 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/", methods=["GET", "POST"]) | ||
| def handle_goals(): | ||
|
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. Similar feedback about splitting these functions, and moving validation and dictionary-handling logic around that I made for |
||
| if request.method == "GET": | ||
| goals = Goal.query.all() | ||
|
|
||
| goals_response = [] | ||
|
|
||
| for goal in goals: | ||
| goals_response.append({ | ||
| "id": goal.goal_id, | ||
| "title": goal.title | ||
| }) | ||
| return make_response(jsonify(goals_response), 200) | ||
|
|
||
| elif request.method == "POST": | ||
| request_body = request.get_json() | ||
| if 'title' not in request_body: | ||
| goal_response = {"details": "Invalid data"} | ||
| return make_response(goal_response, 400) | ||
|
|
||
| new_goal = Goal(title=request_body["title"]) | ||
|
|
||
| db.session.add(new_goal) | ||
| db.session.commit() | ||
|
|
||
| goal_response = {"goal": | ||
| {"id": new_goal.goal_id, | ||
| "title": new_goal.title, | ||
| }} | ||
| return make_response(goal_response, 201) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>", methods=["GET", "PUT", "DELETE"]) | ||
| def handle_goal(goal_id): | ||
| goal = Goal.query.get(goal_id) | ||
| if not goal: | ||
| return make_response(f"Goal #{goal_id} Not Found", 404) | ||
|
|
||
| if request.method == "GET": | ||
| goal_response = {"goal": { | ||
| "id": goal.goal_id, | ||
| "title": goal.title | ||
| }} | ||
| return make_response(goal_response, 200) | ||
|
|
||
| elif request.method == "PUT": | ||
| form_data = request.get_json() | ||
| if not 'title' in form_data: | ||
| goal_response = {"details": "Invalid data"} | ||
| return make_response(goal_response, 400) | ||
|
|
||
| goal.title = form_data["title"] | ||
| db.session.commit() | ||
|
|
||
| goal_response = {"goal": { | ||
| "id": goal.goal_id, | ||
| "title": goal.title | ||
| }} | ||
| return make_response(goal_response, 200) | ||
|
|
||
| elif request.method == "DELETE": | ||
| db.session.delete(goal) | ||
| db.session.commit() | ||
| goal_response = { | ||
| "details": f"Goal {goal.goal_id} \"{goal.title}\" successfully deleted" | ||
| } | ||
| return make_response(goal_response, 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>/tasks", methods=["POST", "GET"]) | ||
| def handle_goal_tasks(goal_id): | ||
| goal = Goal.query.get(goal_id) | ||
| if not goal: | ||
| return make_response(f"Goal #{goal_id} Not Found", 404) | ||
|
|
||
| if request.method == "POST": | ||
| request_body = request.get_json() | ||
| task_ids = request_body["task_ids"] | ||
| for task_id in task_ids: | ||
| task = Task.query.get(task_id) | ||
| # if not task: | ||
| # return make_response(f"Task #{task_id} Not Found", 404) | ||
|
Comment on lines
+242
to
+243
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. Curious why you commented this out. We should check to make sure that each task id we lookup is valid. |
||
| 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. I personally sort of like this style of setting the relationship, since this is really what is happening even if we use the sqlalchemy helpers. However, what would happen if the goal previously had some tasks set. Do we want to add the new tasks to the existing tasks? Do we want to replace them and sever any prior task → goal relationships? What behavior is implemented here? |
||
|
|
||
| db.session.commit() | ||
| goal_response = { | ||
| "id": goal.goal_id, | ||
| "task_ids": task_ids | ||
| } | ||
| return make_response(jsonify(goal_response), 200) | ||
|
|
||
| if request.method == "GET": | ||
| tasks = Task.query.filter_by(goal_id=goal.goal_id) | ||
| tasks_response = [] | ||
|
|
||
| for task in tasks: | ||
| if not task.completed_at: | ||
| is_complete = False | ||
| else: | ||
| is_complete = True | ||
| tasks_response.append({ | ||
| "id": task.task_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": is_complete, | ||
| "goal_id": task.goal_id, | ||
| }) | ||
| goal_response = { | ||
| "id": goal.goal_id, | ||
| "title": goal.title, | ||
| "tasks": tasks_response | ||
| } | ||
| return make_response(jsonify(goal_response), 200) | ||
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.
Nice way to set this across all your blueprints!