-
Notifications
You must be signed in to change notification settings - Fork 146
C18 Snow Leopards Jessica Hebert #138
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
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,5 +1,21 @@ | ||
| from app import db | ||
|
|
||
| from flask import abort, make_response | ||
|
|
||
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) | ||
| tasks = db.relationship("Task", back_populates="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. The relationship between goal and task is one-to-many. I wrote a comment in Task model about renaming the field tasks = db.relationship("Task", back_populates="goal") |
||
|
|
||
| def to_json(self): | ||
|
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. During the live code for Flasky, I did initially name this method to to_json, but I ended up refactoring it to to_dict to better reflect the fact that we're taking a model and turning it into a dictionary. Just wanted to call that out. |
||
| return { | ||
| "id" : self.id, | ||
| "title" : self.title | ||
| } | ||
|
|
||
| @classmethod | ||
| def create_task(cls, request_body): | ||
| try: | ||
| new_task = cls(title=request_body['title']) | ||
| return new_task | ||
| except: | ||
| return abort(make_response({"details": "Invalid data"}, 400)) | ||
|
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. Ooooooh this is nice, I like that you've got some error handling here! When we use the request body, which comes from the client making a request, we should operate under the assumption that the client may send us bad data so we need to have some error handling. However, I think you misnamed this method. Since it's part of the Goal class, it should be called create_goal and the variable name should be new_goal |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,29 @@ | ||
| from app import db | ||
|
|
||
|
|
||
| class Task(db.Model): | ||
| task_id = db.Column(db.Integer, primary_key=True) | ||
| task_id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
|
Comment on lines
4
to
+5
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 you've got the same field two times on Task. Pay close attention to these attributes that you create for a class because these attributes with SqlAlchemy translate into fields in the table. Delete line 4 |
||
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
|
Comment on lines
+6
to
+7
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 at least the title field. It doesn't make much sense to have tasks that have null for title. You might be able to get away with letting description accept null values if the title has a value -- that's up to you |
||
| completed_at = db.Column(db.DateTime, nullable=True) | ||
| goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True) | ||
| goals = db.relationship("Goal", back_populates="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. The relationship between goal and task is one-to-many. One goal can have many tasks, but each task only has one goal. We also represent this relationship by having a foreign key "goal_id" which can only be a single goal id on the Task model. Therefore, we should rename the attribute |
||
|
|
||
| def to_dict(self): | ||
| if self.completed_at: | ||
| complete = True | ||
| else: | ||
| complete = False | ||
|
Comment on lines
+13
to
+16
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, this method being called to_dict feels a little more accurate than to_json. Another way to write lines 13-16 would be to use a ternary: complete = True if self.completed_at else False |
||
|
|
||
| task_as_dict = {} | ||
| task_as_dict["id"] = self.task_id | ||
| task_as_dict["title"] = self.title | ||
| task_as_dict["description"] = self.description | ||
| task_as_dict["is_complete"] = complete | ||
|
|
||
| if self.goal_id: | ||
| task_as_dict["goal_id"] = self.goal_id | ||
|
Comment on lines
+24
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. 👍 |
||
|
|
||
| return task_as_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. Consider adding a create_task() class method with error handling, just like you had in goal.py on lines 15-21 |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,215 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint | ||
|
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. For future projects, a way to further organize your files is to create a routes directory and create separate files for each entity's routes. So you'd have a task_routes.py and a goal_routes.py and they'd both live in the routes directory (just like how you have your goal.py and task.py living in the models directory) |
||
| from app import db | ||
| from app.models.task import Task | ||
| from app.models.goal import Goal | ||
| from flask import Blueprint, jsonify, request, make_response, abort | ||
| from datetime import datetime | ||
| import requests | ||
| import os | ||
| from dotenv import load_dotenv | ||
|
|
||
| load_dotenv() | ||
| task_bp = Blueprint("task_bp", __name__, url_prefix="/tasks") | ||
| goal_bp = Blueprint("goal_bp", __name__, url_prefix="/goals") | ||
|
|
||
| def validate_model(cls, model_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. Nice work refactoring your validation method to be generic enough to validate any kind of model |
||
| 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 | ||
|
|
||
|
|
||
| @task_bp.route("", methods=["POST"]) | ||
| def handle_tasks(): | ||
| request_body = request.get_json() | ||
| try: | ||
| new_task = Task(title=request_body["title"], | ||
| description=request_body["description"]) | ||
| except: | ||
| abort(make_response({"details": "Invalid data"}, 400)) | ||
|
Comment on lines
+31
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. Earlier I made a comment about creating a class method like create_task in task.py If you use that method, then line 32 would look like new_task = Task.create_task(request_body)You can either have error handling (which is necessary because we're using data provided by the client to create an entity that we're going to save to the db) here in the route method or in the class method like you did for create_goal(). Since you've done it both ways, just want to call out that both approaches work, but stick with one so your code is consistent and predictable to others reading your code |
||
|
|
||
| db.session.add(new_task) | ||
| db.session.commit() | ||
|
|
||
| return make_response({"task":{ | ||
| "id": new_task.task_id, | ||
| "title": new_task.title, | ||
| "description": new_task.description, | ||
| "is_complete": False | ||
| }}, 201) | ||
|
Comment on lines
+40
to
+45
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 wrote a to_dict() method in task.py that does exactly what you have written on lines 40-44. Let's leverage the method you already wrote to keep the logic in this route concise. Also prefer to explicitly wrap a dictionary with jsonify for consistency and to explicitly convey that you are returning json. Refactoring these lines of code would look like: return make_response(jsonify(dict(task = new_task.to_dict())), 201) |
||
|
|
||
| @task_bp.route("", methods=["GET"]) | ||
| def read_all_tasks(): | ||
|
|
||
| sort_query = request.args.get("sort") | ||
|
|
||
| if sort_query == "asc": | ||
| task = Task.query.order_by(Task.title.asc()) | ||
| elif sort_query == "desc": | ||
| task = Task.query.order_by(Task.title.desc()) | ||
| else: | ||
| task = Task.query.all() | ||
|
|
||
| task_response = [] | ||
| for task in task: | ||
| task_response.append(task.to_dict()) | ||
|
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. This would be a good place to refactor with list comprehension |
||
|
|
||
| return jsonify(task_response) | ||
|
|
||
| @task_bp.route("/<task_id>", methods=["GET"]) | ||
| def read_one_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
| return {"task": task.to_dict()} | ||
|
|
||
| @task_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"] | ||
|
Comment on lines
+76
to
+77
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 how you have error handling in your POST request, we should also have error handling for the PUT request too since we're using client-provided data to update an entity in the db and we want to be sure this method can handle it. For example, line 76 could throw a KeyError if the client sent a request body like {"title_name": "Prep for dinner", "description": "chop veggies for dinner"} |
||
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response({"task": task.to_dict()}) | ||
|
|
||
| def slack_bot_message(message): | ||
|
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 work pulling this logic into a helper function which keeps your mark_complete method concise and readable. |
||
| PATH = "https://slack.com/api/chat.postMessage" | ||
| SLACK_API_KEY = os.environ.get("SLACK_API_KEY") | ||
|
|
||
| query_params = { | ||
| "channel" : "#task-notification", | ||
| "text" : message | ||
| } | ||
|
|
||
| requests.post(PATH, params=query_params,headers={"Authorization": SLACK_API_KEY}) | ||
|
|
||
| @task_bp.route("/<task_id>/mark_complete", methods=["PATCH"]) | ||
| def complete_incomplete_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| task.completed_at = datetime.utcnow() | ||
|
|
||
| db.session.commit() | ||
|
|
||
| slack_bot_message(f"Someone just completed the task {task.title}") | ||
|
|
||
| return make_response({"task": task.to_dict()}) | ||
|
|
||
| @task_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"]) | ||
| def mark_complete_task_incomplete(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| task.completed_at = None | ||
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response({"task": task.to_dict()}) | ||
|
|
||
|
|
||
| @task_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'}) | ||
|
|
||
| #GOAL ROUTES | ||
|
|
||
| @goal_bp.route("", methods=["POST"]) | ||
| def handle_goals(): | ||
| request_body = request.get_json() | ||
| try: | ||
| new_goal = 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. This is where you'd use your class method in the goal.py file. Your method also has try/except block for error handling so you wouldn't need the try/except here. |
||
| except: | ||
| abort(make_response({"details": "Invalid data"}, 400)) | ||
|
|
||
| db.session.add(new_goal) | ||
| db.session.commit() | ||
|
|
||
| return make_response({"goal":{ | ||
| "id": new_goal.goal_id, | ||
| "title": new_goal.title | ||
| }}, 201) | ||
|
Comment on lines
+139
to
+142
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've also got your to_json() method in the Goal class that you should leverage here so you don't repeat yourself. Also as previously stated, I prefer wrapping a dict with jsonify() for consistency |
||
|
|
||
| @goal_bp.route("", methods=["GET"]) | ||
| def read_all_goals(): | ||
|
|
||
| goal = Goal.query.all() | ||
|
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 will return more than one goal, it should be named |
||
| goal_response = [] | ||
| for goal in goal: | ||
| goal_response.append({ | ||
| "id": goal.goal_id, | ||
| "title": goal.title | ||
| }) | ||
|
Comment on lines
+149
to
+153
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 place for list comprehension and using your to_json method so you're not repeating yourself. goal_response = [goal.to_json() for goal in goals] |
||
|
|
||
| return jsonify(goal_response) | ||
|
|
||
| @goal_bp.route("/<goal_id>", methods=["GET"]) | ||
| def read_one_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| return {"goal":{ | ||
| "id": goal.goal_id, | ||
| "title": goal.title, | ||
| }} | ||
|
Comment on lines
+160
to
+163
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 these 3 lines of code exist in several routes: {
"id": goal.goal_id,
"title": goal.title,
}When you start noticing this kind of repetition, review your code to see if you need to create a helper method (which you did do in goal.py with to_json(). Let's use it! |
||
|
|
||
| @goal_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"] | ||
|
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. Need error handling when using data from the client, just like your POST request |
||
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response({"goal":{ | ||
| "id": goal.goal_id, | ||
| "title": goal.title | ||
| }}) | ||
|
|
||
| @goal_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_id} "{goal.title}" successfully deleted'}) | ||
|
|
||
| @goal_bp.route("/<goal_id>/tasks", methods=["POST"]) | ||
| def add_task_ids_to_goal(goal_id): | ||
|
|
||
| goal = validate_model(Goal, goal_id) | ||
| request_body = request.get_json() | ||
| goal.tasks = [Task.query.get(task_id) 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. Nice list comprehension! Instead of using goal.tasks = [validate_model(task_id) for task_id in request_body["task_ids"]] |
||
|
|
||
| db.session.commit() | ||
|
|
||
| return {"id":goal.goal_id, | ||
| "task_ids":[task.task_id for task in goal.tasks]}, 200 | ||
|
|
||
| @goal_bp.route("/<goal_id>/tasks", methods=["GET"]) | ||
|
|
||
|
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. Delete white space so that decorator is directly above the method |
||
| def read_tasks(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 | ||
|
Comment on lines
+212
to
+214
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. How could you refactor the to_json() method in goal.py so that it can conditionally set the value for "tasks" if necessary? You do something similar in to_dict() in the Task class with the goal_id. |
||
| }) | ||
| 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.
A goal without a title doesn't make much sense so we might want to make it a required field. We can do so by adding nullable=False to the title field