-
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
Sea Turtles - Christina W #100
base: master
Are you sure you want to change the base?
Changes from all commits
be2a08c
1d348fe
95466c8
218136d
5a667b7
982d969
c78104e
eeef1e0
390032c
e043105
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 app.routes.task_routes import task_bp | ||
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 splitting the routes into their own files (and remembering the |
||
app.register_blueprint(task_bp) | ||
|
||
from app.routes.goal_routes import goal_bp | ||
app.register_blueprint(goal_bp) | ||
|
||
return app |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,4 +2,46 @@ | |
|
||
|
||
class Goal(db.Model): | ||
__tablename__ = 'goals' | ||
goal_id = db.Column(db.Integer, primary_key=True) | ||
title = db.Column(db.String) | ||
tasks = db.relationship("Task", backref="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 value for the |
||
|
||
def to_dict(self): | ||
task_list = [] | ||
for task in self.tasks: | ||
task_list.append(task.id) | ||
if task_list: | ||
return dict( | ||
id=self.goal_id, | ||
task_ids=task_list | ||
) | ||
return dict( | ||
id=self.goal_id, | ||
title=self.title) | ||
Comment on lines
+14
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. Nice thought to have the to_dict method pick between the output styles, similar to how a task decides whether to include goal information. However, here I would be more inclined to have to different method that build different kinds of dictionaries. The Task |
||
|
||
def get_tasks(self): | ||
from .task import 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. This import isn't necessary since we're not using the |
||
task_list = [] | ||
for task in self.tasks: | ||
readable_task = task.task_to_dict() | ||
task_list.append(readable_task) | ||
return dict( | ||
id=self.goal_id, | ||
title=self.title, | ||
tasks=task_list) | ||
|
||
@classmethod | ||
def from_dict(cls, data_dict): | ||
return cls(title=data_dict["title"]) | ||
|
||
def replace_details(self, data_dict): | ||
if "task_ids" in data_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. Nice idea to bring both branches of this login into a helper, but like with the This also might be an opportunity to create a separate type to handle that relationship logic, say like a |
||
from .task import Task | ||
for id in data_dict["task_ids"]: | ||
task = Task.query.get(id) | ||
if task not in self.tasks: | ||
self.tasks.append(task) | ||
Comment on lines
+43
to
+44
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. Appending the task to the list of tasks for this goal will have the effect of adding the supplied tasks to the goal. If the intent of this operation was to set the tasks for the goal to be exactly what was passed in, we would need to take a slightly different approach. The tests for how this should behave are ambiguous, but think about how we might approach this if we wanted the list of tasks to replace the current tasks associated with this goal. |
||
else: | ||
self.title=data_dict["title"] | ||
return self.to_dict() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,64 @@ | ||
import datetime | ||
from app import db | ||
from app.models.goal import Goal | ||
|
||
|
||
class Task(db.Model): | ||
task_id = db.Column(db.Integer, primary_key=True) | ||
__tablename__ = 'tasks' | ||
id = db.Column(db.Integer, primary_key=True) | ||
title = db.Column(db.String) | ||
description = db.Column(db.String) | ||
Comment on lines
+9
to
+10
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 whether we might want to prevent these from being nullable. Should we be able to create a task without a title? without a description? |
||
is_complete = db.Column(db.Boolean, default=False) | ||
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 not making an actual |
||
completed_at = db.Column(db.DateTime) | ||
goal_id = db.Column(db.Integer, db.ForeignKey(Goal.goal_id)) | ||
|
||
def task_to_dict(self): | ||
if self.goal_id: | ||
return dict( | ||
id=self.id, | ||
title=self.title, | ||
description=self.description, | ||
is_complete=self.is_complete, | ||
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 the |
||
goal_id = self.goal_id | ||
) | ||
else: | ||
return dict( | ||
id=self.id, | ||
title=self.title, | ||
description=self.description, | ||
is_complete=self.is_complete) | ||
Comment on lines
+26
to
+29
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 only difference between this set of values and the one above is whether to include |
||
|
||
@classmethod | ||
def from_dict(cls, data_dict): | ||
if "completed_at" in data_dict: | ||
return cls( | ||
title=data_dict["title"], | ||
description=data_dict["description"], | ||
is_complete=True, | ||
completed_at=data_dict["completed_at"] | ||
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. Here's another spot where handling the completed data is the main difference between these two branches. Check out the |
||
) | ||
else: | ||
return cls( | ||
title=data_dict["title"], | ||
description=data_dict["description"] | ||
) | ||
|
||
def replace_details(self, data_dict): | ||
self.title=data_dict["title"] | ||
self.description=data_dict["description"] | ||
self.is_complete=False | ||
if "completed_at" in data_dict: | ||
self.completed_at=data_dict["completed_at"] | ||
self.is_complete=True | ||
Comment on lines
+50
to
+52
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 to update the contents of the model, as well as checking for the presence of |
||
return self | ||
|
||
def mark_done(self): | ||
self.is_complete = True | ||
current_time = datetime.datetime.now() | ||
self.completed_at = current_time | ||
return self | ||
|
||
def mark_not_done(self): | ||
self.is_complete = False | ||
self.completed_at = None | ||
return self | ||
Comment on lines
+55
to
+64
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 love little helpers like this that are strictly speaking necessary, but still provide an abstraction of the operation we want to perform: setting or clearing the |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
from flask import Blueprint, jsonify, make_response, request, abort | ||
from app import db | ||
from app.models.goal import Goal | ||
from .task_routes import validate_task_id, Task | ||
|
||
goal_bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
||
def error_message(message, status_code): | ||
abort(make_response(jsonify(dict(details=message)), status_code)) | ||
|
||
def make_goal_safely(data_dict): | ||
try: | ||
return Goal.from_dict(data_dict) | ||
except KeyError as err: | ||
error_message(f"Invalid data", 400) | ||
|
||
def update_goal_safely(goal, data_dict): | ||
try: | ||
return goal.replace_details(data_dict) | ||
except KeyError as err: | ||
error_message(f"Invalid data", 400) | ||
|
||
def validate_goal_id(id): | ||
try: | ||
id = int(id) | ||
except ValueError: | ||
error_message(f"Invalid id {id}", 400) | ||
goal = Goal.query.get(id) | ||
if goal: | ||
return goal | ||
error_message(f"No goal with ID {id}. SORRY.", 404) | ||
Comment on lines
+8
to
+31
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. These are really close to the similar functions in the task routes. Think about how we could pull them into their own helper file and what we would need to do to make them work for more than one class. |
||
|
||
@goal_bp.route("", methods=["POST"]) | ||
def add_goal(): | ||
request_body = request.get_json() | ||
new_goal=make_goal_safely(request_body) | ||
|
||
db.session.add(new_goal) | ||
db.session.commit() | ||
|
||
task_response = {"goal":new_goal.to_dict()} | ||
|
||
return jsonify(task_response), 201 | ||
|
||
@goal_bp.route("", methods=["GET"]) | ||
def get_goals(): | ||
goals = Goal.query.all() | ||
|
||
result_list = [goal.to_dict() for goal in goals] | ||
return jsonify(result_list) | ||
|
||
@goal_bp.route("/<id>", methods=["GET"]) | ||
def get_goal_by_id(id): | ||
goal = validate_goal_id(id) | ||
result = {"goal": goal.to_dict()} | ||
return jsonify(result) | ||
|
||
@goal_bp.route("<id>", methods=["PUT"]) | ||
def update_goal(id): | ||
goal = validate_goal_id(id) | ||
request_body = request.get_json() | ||
updated_goal = update_goal_safely(goal, request_body) | ||
|
||
db.session.commit() | ||
|
||
return jsonify({"goal":updated_goal}) | ||
|
||
@goal_bp.route("<id>", methods=["DELETE"]) | ||
def delete_goal(id): | ||
goal = validate_goal_id(id) | ||
db.session.delete(goal) | ||
db.session.commit() | ||
|
||
return jsonify({"details":f'Goal {id} "{goal.title}" successfully deleted'}) | ||
|
||
@goal_bp.route("/<id>/tasks", methods=["POST"]) | ||
def add_task_to_goal(id): | ||
goal = validate_goal_id(id) | ||
request_body = request.get_json() | ||
data_dict = {"task_ids": []} | ||
for task in request_body["task_ids"]: | ||
validate_task_id(task) | ||
data_dict["task_ids"].append(task) | ||
Comment on lines
+81
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. 👍 Nice validation that all of the supplied task ids were valid. Consider using the name If you are doing the resolving here, you could consider reworking your helper function to not use a dictionary. Simply pass in the list. Otherwise, consider moving all of the logic (including the task id validation) into a separate function, or even into Goal. But you would want to use a different approach to validating the tasks (since we wouldn't want to use |
||
# updated_goal = update_goal_safely(goal, request_body) | ||
update_tasks_in_goal = update_goal_safely(goal, data_dict) | ||
db.session.commit() | ||
# task_response = {"goal":update_tasks_in_goal} | ||
|
||
return jsonify(update_tasks_in_goal), 200 | ||
|
||
@goal_bp.route("/<id>/tasks", methods=["GET"]) | ||
def get_tasks_from_goal(id): | ||
goal = validate_goal_id(id) | ||
result = goal.get_tasks() | ||
return jsonify(result) |
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.
Notice that the documentation for the endpoint lists POST as the method. So the endpoint itself would be only the https://slack.com/api/chat.postMessage, and we should be thinking about how to send the request data not as query parameters, but instead in the body of the request.