Skip to content

Heather M Task List API#92

Open
cathos wants to merge 10 commits into
ada-c17:masterfrom
cathos:master
Open

Heather M Task List API#92
cathos wants to merge 10 commits into
ada-c17:masterfrom
cathos:master

Conversation

@cathos

@cathos cathos commented May 13, 2022

Copy link
Copy Markdown

No description provided.

@tgoslee tgoslee left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great job Heather!! I left some comments on some possible ways you could refactor! Let me know if you have questions!

Comment thread app/__init__.py
migrate.init_app(app, db)

# Register Blueprints here
from .routes import tasks_bp, goals_bp

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you could refactor your routes files to be goal_routes.py and task_routes.py for a little more organization

Comment thread app/models/goal.py
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, nullable = False)
tasks = db.relationship("Task", back_populates="goal")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because the default value for lazy is True you could technically leave it off. More info here https://docs.sqlalchemy.org/en/14/orm/loading_relationships.html

Comment thread app/models/task.py
task_id = db.Column(db.Integer, primary_key=True, autoincrement=True)
title = db.Column(db.String, nullable = False)
description = db.Column(db.String, nullable = False)
completed_at = db.Column(db.DateTime)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you could add nullable=True here because completed_at doesn't have to exist

Comment thread app/models/task.py
Comment on lines +13 to +27
if self.goal_id != None:
return {
"id": self.task_id,
"goal_id": self.goal_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at)
}
else:
return {
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": bool(self.completed_at)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because this is repetitive you could create a response dictionary to check for goal_id something like

response_dict = dict(
    id=self.task_id,
    title=self.title,
    description=self.description,
    is_complete= bool(self.completed_at)
)
        
if self.goal_id:
    response_dict["goal_id"] = self.goal_id
            
return response_dict

Comment thread app/routes.py
tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks")
goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals")

def send_slack_message(task, message):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like that you pulled this out into a helper function

Comment thread app/routes.py
return make_response(jsonify(new_task.single_dict()), 201)

except KeyError:
return make_response(jsonify({"details":"Invalid data"}), 400)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you could make your message more specific to let the user know what's wrong. Invalid data: title or description is missing

Comment thread app/routes.py
return make_response(jsonify(task.single_dict()), 200)

@tasks_bp.route("/<task_id>/<mark_completion>", methods=["PATCH"])
def task_completion(task_id, mark_completion):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this seems similar to lines 119-124. Are they performing the same task?

Comment thread app/routes.py
Comment on lines +176 to +192
# sort_query = request.args.get("sort")
# title_query = request.args.get("title")

# if title_query:
# tasks = Task.query.filter(Task.title.ilike("%" + title_query + "%"))
# elif description_query:
# tasks = Task.query.filter(Task.description.ilike("%" + description_query + "%"))
# elif completed_query == "false":
# tasks = Task.query.filter(Task.completed_at == None)
# elif completed_query == "true":
# tasks = Task.query.filter(Task.completed_at != None)
# elif sort_query in ("asc","desc"):
# if sort_query == "asc":
# tasks = Task.query.order_by(Task.title).all()
# else:
# tasks = Task.query.order_by(Task.title.desc()).all()
# else:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

you can remove this if you aren't using it

Comment thread app/routes.py
if "task_ids" in request_body_keys:
for task_id in request_body["task_ids"]:
task_ids.append(validate_tasks(task_id))
print(task_ids)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

after debugging you can remove this

Comment thread app/routes.py
Comment on lines +278 to +279
# tasks = goal.tasks
# print(tasks)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove this commented code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants