-
Notifications
You must be signed in to change notification settings - Fork 146
Snow Leopards_Galina R #122
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
cadd644
e5bf69c
c80c97b
ef23af0
e70d174
bcdb113
5110faf
8e3d8ec
d98c89b
169028b
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 |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| from flask import Blueprint, request, make_response, jsonify, abort | ||
| from app.models.goal import Goal | ||
| from app.models.task import Task | ||
| from app import db | ||
|
|
||
|
|
||
| goals_bp = Blueprint("goals", __name__, url_prefix="/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. Since this file is called goal_routes.py and the file is only concerned with the Goal class, we can rename this variable to just be |
||
|
|
||
| def get_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 this method so it is generic enough to validate any kind of model. However, you have this method repeated in both route files and they do the same thing. It would be better to put this method in a helper file and import the method into your 2 route files |
||
| 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 | ||
|
|
||
|
|
||
| @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
+27
to
+30
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 error handling. Whenever our methods rely on data sent by the client we should have error handling because we should anticipate the possibility of bad data being sent to our server. Another way to handle invalid request body from the client is to use try/except. try:
# create your goal here
except:
# return error message if the request body is invalid |
||
|
|
||
| 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. As we demonstrated during our roundtable, you can also create a class method in the Goal class to handle turning data from the request body into an instance of Goal. |
||
|
|
||
| db.session.add(new_goal) # track this object | ||
| db.session.commit() # any changes that are pending commit those changes as data written in SQL | ||
|
|
||
| new_goal_response = {"goal": new_goal.to_dict()} | ||
| return make_response(jsonify(new_goal_response), 201) | ||
|
|
||
|
|
||
| @goals_bp.route("", methods=["GET"]) | ||
| def read_all_goals(): | ||
| goals = Goal.query.all() | ||
|
|
||
| goals_list = [goal.to_dict() for goal in goals] | ||
| return make_response(jsonify(goals_list), 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>", methods=["GET"]) | ||
| def read_one_goal(goal_id): | ||
| goal = get_validate_model(Goal, goal_id) | ||
| current_goal_response = {"goal": goal.to_dict()} | ||
|
|
||
| return make_response(jsonify(current_goal_response), 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>", methods=["PUT"]) | ||
| def update_goal(goal_id): | ||
| goal = get_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. We need error handling when we handle UPDATE method (CRUD) just like you have error handline for CREATE method for POST request because you are relying on data passed to the server from a client. If they pass you a request body like {"title_name": "updated title name"} then your method will throw an unhandled KeyError on line 62 |
||
|
|
||
| db.session.commit() | ||
|
|
||
| current_goal_response = {"goal": goal.to_dict()} | ||
| return make_response(jsonify(current_goal_response), 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>", methods=["DELETE"]) | ||
| def delete_goal(goal_id): | ||
| goal = get_validate_model(Goal, goal_id) | ||
|
|
||
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| return make_response({"details": f'Goal {goal_id} "{goal.title}" successfully deleted'}, 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>/tasks", methods=["POST"]) | ||
| def sending_list_of_task_ids_to_goal(goal_id): | ||
| goal = get_validate_model(Goal, goal_id) | ||
|
|
||
| request_body = request.get_json() | ||
| task_ids = request_body["task_ids"] | ||
|
|
||
| goal.tasks = [Task.query.get(task_id) for task_id in 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. query.get(task_id) does not handle invalid values for task_id. We can use the get_validate_model() method here to have error handling goal.tasks = [task_id.get_validate_model() for task_id in task_ids] |
||
|
|
||
| db.session.commit() # any changes that are pending commit those changes as data written in SQL | ||
|
|
||
| return make_response(jsonify({ | ||
| "id": goal.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. Instead of using task_ids variable which comes from You could create a helper method in goal.py to grab the task_ids from an instance of goal: def task_ids(self):
return [task.id for task in self.tasks] |
||
| }), 200) | ||
|
|
||
|
|
||
| @goals_bp.route("/<goal_id>/tasks", methods=["GET"]) | ||
| def getting_list_of_tasks_by_goal(goal_id): | ||
| goal = get_validate_model(Goal, goal_id) | ||
|
|
||
| tasks_list = [task.to_dict() for task in goal.tasks] | ||
|
|
||
| return make_response(jsonify({ | ||
| "id": goal.goal_id, | ||
| "title": goal.title, | ||
| "tasks": tasks_list | ||
| }), 200) | ||
|
Comment on lines
+104
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. Could you use your to_dict() method from the Goal class here? You can add some logic to to_dict so that it can conditionally set "tasks" key when needed. |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,3 +3,12 @@ | |
|
|
||
| class Goal(db.Model): | ||
| goal_id = db.Column(db.Integer, primary_key=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. Since a goal without a title doesn't make much sense, we should add nullable=false to this attribute. |
||
| tasks = db.relationship("Task", back_populates="goal", lazy=True) | ||
|
|
||
| def to_dict(self): | ||
| result_dict = {} | ||
| result_dict["id"] = self.goal_id | ||
| result_dict["title"] = self.title | ||
|
|
||
| return result_dict | ||
| 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) | ||
| 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. Add nullable=false so that you don't get task with "title": null |
||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable=True) | ||
|
|
||
| # Many tasks to one goal | ||
| goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'), nullable=True) | ||
| goal = db.relationship("Goal", back_populates="tasks") | ||
|
|
||
|
|
||
| def to_dict(self): | ||
| result_dict = {} | ||
| result_dict["id"] = self.task_id | ||
| result_dict["title"] = self.title | ||
| result_dict["description"] = self.description | ||
| result_dict["is_complete"] = bool(self.completed_at) # False if completed_at is null | ||
|
|
||
| if self.goal_id: | ||
| result_dict["goal_id"] = self.goal_id | ||
|
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. 👍 |
||
|
|
||
| return result_dict | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| from flask import Blueprint, request, make_response, jsonify, abort | ||
| from sqlalchemy import asc, desc | ||
| from app.models.task import Task | ||
| from app import db | ||
| from datetime import datetime | ||
| import os | ||
| import requests | ||
|
|
||
|
|
||
| tasks_bp = Blueprint("tasks", __name__, url_prefix="/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. Can rename this to |
||
|
|
||
| def get_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 | ||
|
|
||
|
|
||
| @tasks_bp.route("", methods=["GET"]) | ||
| def read_all_tasks(): | ||
| order_by = request.args.get("sort") | ||
|
|
||
| if order_by == 'asc': | ||
| tasks = Task.query.order_by(asc('title')).all() | ||
| elif order_by == 'desc': | ||
| tasks = Task.query.order_by(desc('title')).all() | ||
| else: | ||
| tasks = Task.query.all() | ||
|
|
||
| tasks_list = [task.to_dict() for task in tasks] | ||
| return make_response(jsonify(tasks_list), 200) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>", methods=["GET"]) | ||
| def read_one_task(task_id): | ||
| task = get_validate_model(Task, task_id) | ||
| current_task_response = {"task": task.to_dict()} | ||
|
|
||
| return make_response(jsonify(current_task_response), 200) | ||
|
Comment on lines
+44
to
+46
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. Another way to write this would look like: return make_response(jsonify(dict(task=task.to_dict()))) |
||
|
|
||
|
|
||
| @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) | ||
|
|
||
| new_task = Task(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. Line 58 is concerned with creating an instance of the Task class. You could argue that the logic in this method create_task() should only be concerned with creating a valid response to return to the client. Therefore, the logic on line 58 should be moved into the Task class so that the Task class is responsible for creating an instance of itself and we can just invoke the method here. If you create a class method from_dict() in the Task class, then line 58 would just be: new_task = Task.from_dict(request_body) That way we don't need to expose the logic that belongs in Task in a route |
||
| description=request_body["description"]) | ||
|
|
||
| db.session.add(new_task) # track this object | ||
| db.session.commit() # any changes that are pending commit those changes as data written in SQL | ||
| current_task_response = {"task": new_task.to_dict()} | ||
| return make_response(jsonify(current_task_response), 201) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>", methods=["PUT"]) | ||
| def update_task(task_id): | ||
| task = get_validate_model(Task, task_id) | ||
|
|
||
| request_body = request.get_json() | ||
| 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. Need some error handling here in case the request_body is invalide |
||
|
|
||
| db.session.commit() | ||
|
|
||
| current_task_response = {"task": task.to_dict()} | ||
| return make_response(jsonify(current_task_response), 200) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>", methods=["DELETE"]) | ||
| def delete_task(task_id): | ||
| task = get_validate_model(Task, task_id) | ||
|
|
||
| db.session.delete(task) | ||
| db.session.commit() | ||
|
|
||
| return make_response({"details": f'Task {task_id} "{task.title}" successfully deleted'}, 200) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>/mark_complete", methods=["PATCH"]) | ||
| def update_incompleted_task_to_complete(task_id): | ||
| task = get_validate_model(Task, task_id) | ||
|
|
||
| task.completed_at = datetime.now() | ||
|
|
||
| db.session.commit() | ||
|
|
||
| slack_bot(task) | ||
|
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 using a helper function to contain the logic for making a request to Slack's API. This makes your method more concise and readable |
||
| current_task_response = {"task": task.to_dict()} | ||
| return make_response(jsonify(current_task_response), 200) | ||
|
|
||
|
|
||
| @tasks_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"]) | ||
| def update_completed_task_to_incomplete(task_id): | ||
| task = get_validate_model(Task, task_id) | ||
| task.completed_at = None | ||
|
|
||
| db.session.commit() | ||
|
|
||
| current_task_response = {"task": task.to_dict()} | ||
| return make_response(jsonify(current_task_response), 200) | ||
|
|
||
|
|
||
| def slack_bot(task): | ||
|
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 renaming this method to incorporate a verb to clarify what this method is doing. Something like |
||
| PATH = "https://slack.com/api/chat.postMessage" | ||
| SLACK_API_KEY = os.environ.get("SLACK_API") | ||
|
|
||
| bot_params = { | ||
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
|
|
||
| requests.post(PATH, data=bot_params, headers={"Authorization": SLACK_API_KEY}) | ||
|
|
||
| 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.
Nice work putting goal routes logic in a separate file from task routes logic. You can create a directory called routes (like we have with models) and put your two route files in it to further organize your project