Skip to content

Shark - Ying #98

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Procfile
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
web: gunicorn 'app:create_app()'
8 changes: 8 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@

def create_app(test_config=None):
app = Flask(__name__)

# app.config["SLACK_TOKEN"] = os.environ.get("SLACK_API_KEY")

app.config["SQLALCHEMY_TRACK_MODIFICATIONS"] = False

if test_config is None:
Expand All @@ -30,5 +33,10 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here
from app.routes.routes import tasks_bp
app.register_blueprint(tasks_bp)
from app.routes.goal_routes import goals_bp
app.register_blueprint(goals_bp)
Comment on lines +36 to +39

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work in organizing your routes into separate files. My one minor suggestion is to change the routes file name to task_routes.


return app

19 changes: 18 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,21 @@


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
goal_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String)
tasks = db.relationship("Task", back_populates="goal", lazy = True)
Comment on lines +5 to +7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work making the Goal class! We may want to consider whether we should storing empty/null titles for a goal is useful (should goals have no titles?). How might we change line 6, title = db.Column(db.String) to prevent null values?



Comment on lines +8 to +9

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can get rid of extra whitespace


def to_json(self):
return {
"id": self.goal_id,
"title": self.title
}

@classmethod
def create_goal(cls, req_body):
new_goal = cls(
title = req_body["title"],
)
return new_goal
Comment on lines +11 to +22

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

57 changes: 55 additions & 2 deletions app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,58 @@
from app import db

from datetime import datetime

class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
id = db.Column(db.Integer, primary_key=True, )

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to add autoincrement=True just to communicate to other developers that the id is being autoincremented by postgres/SQLAlchemy

title = db.Column(db.String)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, default = None)
# (- Nullable=True is default)
goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"))
goal = db.relationship("Goal", back_populates="tasks")
Comment on lines +6 to +11

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work setting up the relationship between Task and Goal. The comment about null column values from Goal can also apply here to the task. Are there specific columns that would or would not benefit from storing null ?




def to_json(self):

to_json_dict = {
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": True if self.completed_at else False
}
if self.goal_id:
to_json_dict["goal_id"] =self.goal_id

return to_json_dict
Comment on lines +15 to +26

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work in handling the goal_id. As well as using a ternary!


@classmethod
def create(cls, req_body):
if not "completed_at" in req_body:
completed_at_status = None
else:
completed_at_status = req_body["completed_at"]

new_task = cls(
title = req_body["title"],
description = req_body["description"],
completed_at = completed_at_status

)
return new_task
Comment on lines +29 to +41

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍



def update(self, req_body):

if not "completed_at" in req_body:
completed_at_status = None
else:
completed_at_status = req_body["completed_at"]


self.title = req_body["title"],
self.description = req_body["description"],
self.completed_at = completed_at_status
Comment on lines +44 to +54

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍





1 change: 0 additions & 1 deletion app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Empty file.
136 changes: 136 additions & 0 deletions app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
from pydoc import describe
from urllib import response
Comment on lines +1 to +2

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like these imports snuck their way in! If we aren't using these packages/modules then we can remove them from our codebase.

from flask import Blueprint, jsonify, request, make_response,abort
from app import db
from app.models.goal import Goal
from app.models.task import Task
from .helper import goal_validate_client_requests, validate_goal, validate_task

goals_bp = Blueprint("goals", __name__,url_prefix="/goals" )

#GET all
@goals_bp.route("", methods = ["GET"])
def get_all_goals():
goals = Goal.query.all()

goals_response = []
for goal in goals:
goals_response.append(goal.to_json())

return jsonify(goals_response),200
Comment on lines +12 to +20

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work! We can also use list comprehension to build goals_response.

@goals_bp.route("", methods = ["GET"])
def get_all_goals():
    goals = Goal.query.all()

    goals_response = [goal.to_json() for goal in goals]
    
    return jsonify(goals_response),200



#GET one
@goals_bp.route("/<goal_id>", methods = ["GET"])
def get_one_goal(goal_id):
goal = validate_goal(goal_id)

return {"goal": goal.to_json()},200
Comment on lines +24 to +28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice use of a helper!



#CREATE one
@goals_bp.route("", methods = ["POST"])
def create_one_goal():

request_body = request.get_json()
if "title" in request_body:
# new_goal = goal_validate_client_requests(request_body)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure but when I updated your import to from .helpers import * I was able to get goal_validate_client_requests to work.


#?????? why below does not work
# new_goal = Goal(title = request_body["title"])
Comment on lines +39 to +40

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I uncommented this line and was able to make this line work? Perhaps we can debug this together in a 1:1?

new_goal = Goal.create_goal(request_body)
db.session.add(new_goal)
db.session.commit()

return {"goal": new_goal.to_json()}, 201
else:
return {"details": "Invalid data"}, 400
Comment on lines +41 to +47

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work Ying! We can practice the single responsibility principle by moving the logic to validate request body data into a helper function.






#UPDATE one goal
@goals_bp.route("/<goal_id>", methods = ["PUT"])
def update_one_goal(goal_id):
goal = validate_goal(goal_id)

request_body = request.get_json()
goal.title = request_body["title"]

db.session.commit()

return make_response(jsonify({"goal": goal.to_json()}), 200)
Comment on lines +54 to +63

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍



#DELETE
@goals_bp.route("/<goal_id>", methods = ["DELETE"])
def delete_goal(goal_id):
goal = validate_goal(goal_id)

db.session.delete(goal)
db.session.commit()

return {"details": f'Goal {goal.goal_id} "{goal.title}" successfully deleted'}, 200
Comment on lines +67 to +74

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍




# Sending a List of Task IDs to a Goal
@goals_bp.route("/<goal_id>/tasks", methods = ["POST"])
def create_tasks(goal_id):
#1 get goal for adding tasks:
goal = validate_goal(goal_id)

#2 get tasks that need to add to goal
# 2.a get task ids from client
request_body = request.get_json()
# 2.b get tasks by ids from DB and connect with FK
for id in request_body["task_ids"]:
task = validate_task(id)
task.goal_id = goal_id
# or - add a list of tasks to goal
# goal.tasks.append(task)
# or - assign goal to task.goal
# task.goal = goal

db.session.commit()

#3 create respons body and return it
tasks_id_list =[]
for task in goal.tasks:
tasks_id_list.append(task.id)
Comment on lines +99 to +101

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use list comprehension here.

response_body = {
"id": goal.goal_id,
"task_ids": tasks_id_list
}
return make_response(jsonify(response_body), 200)


#GET task from specific goal
@goals_bp.route("/<goal_id>/tasks", methods = ["GET"])
def get_task_from_specific_goal(goal_id):
#get target goal
goal = validate_goal(goal_id)


tasks_list = []
for task in goal.tasks:
tasks_list.append( task.to_json())

response_boday = {
"id": goal.goal_id,
"title": goal.title,
"tasks": tasks_list
}
Comment on lines +110 to +124

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice work in displaying all tasks associated with a goal. A few suggestions:

  • make the spacing consistent within the function body
  • use list comprehension
  • Change boday to body



return response_boday, 200

#Get a specific Task under given Goal
@goals_bp.route("/tasks/<id>", methods = ["GET"])
def get_task_under_specific_goal(id):
task = validate_task(id)
goal_id = task.goal_id

response_body = task.to_json()
return response_body, 200
Comment on lines +130 to +136

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there are multiple tasks associated with a goal? Are you planning on retrieving only one?

Let's review this together in a 1:1!

48 changes: 48 additions & 0 deletions app/routes/helper.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
from app.models.task import Task
from app.models.goal import Goal
from flask import abort, make_response,request


def validate_task(id):
try:
id = int(id)
except:
abort(make_response({"message": f"Task{id} is invalid"}, 400))

task = Task.query.get(id)
if not task:
abort(make_response({"message": f"Task{id} not found"}, 404))

return task
Comment on lines +6 to +16

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍



def validate_client_requests(request_body):
try:
new_task = Task.create(request_body)
except KeyError:
abort(make_response({"details": "Invalid data"},400))

return new_task
Comment on lines +19 to +25

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍



def validate_goal(id):
try:
id = int(id)
except:
abort(make_response({"message": f"Goal{id} is invalid"}, 400))

goal = Goal.query.get(id)
if not goal:
abort(make_response({"message": f"Goal{id} not found"}, 404))

return goal

def goal_validate_client_requests(request_body):
try:
new_goal = Goal.create(request_body)
except KeyError:
abort(make_response({"details": "Invalid data"},400))

return new_goal
Comment on lines +28 to +46

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice how similar the logic is for both validate_goal and validate_task ( as well as validating client requests for both). We can further refactor by combining these functions and requiring a class reference (represented as the variable obj) to be passed as an argument like so:

def validate_goal(obj, id):
    try:
        id = int(id)
    except:
        abort(make_response({"message": f"{obj.__name__} {id} is invalid"}, 400))

    obj_item = obj.query.get(id)
    if not goal:
        abort(make_response({"message": f"{obj.__name__} {id} not found"}, 404))

    return obj_item



Loading