Skip to content

Accelerate - Risha A. #13

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 13 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
4 changes: 4 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,9 @@ def create_app(test_config=None):
migrate.init_app(app, db)

# Register Blueprints here
from .routes import task_bp
from .routes import goals_bp
app.register_blueprint(task_bp)
app.register_blueprint(goals_bp)
Comment on lines +33 to +36

Choose a reason for hiding this comment

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

👍

One thing we started to touch on in the video store live code was that we can split routes into multiple files. We can make a routes folder, and put routes for each endpoint into separate files, named for their model. Then we can use the name bp for the blueprint in each file since it would be the only blueprint in the file. Then these imports might look like:

    from .routes import task, goal
    app.register_blueprint(task.bp)
    app.register_blueprint(goal.bp)


return app
10 changes: 9 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
from flask import current_app
from app import db

from app.models.task import Task

Choose a reason for hiding this comment

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

We don't need to import the Task class name here. We only use the string "Task" in the relationship declaration. We only need to import Task if we actually use the Task constructor function here in this file.


class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)
tasks = db.relationship("Task", backref="task", lazy=True)

Choose a reason for hiding this comment

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

The backref argument is used to provide a name that SqlAlchemy will use to add a property to the target model (Task in this case) to get back to the current model Goal. goal would be an appropriate name.

Then, if we have a Task instance, we could write task.goal to get a reference to the Goal with which the Task is associated (or None if the Task isn't associated with a Goal).

The tasks association itself would let us write goal.tasks to get the collection of all the Tasks associated with a particular Goal.

Suggested change
tasks = db.relationship("Task", backref="task", lazy=True)
tasks = db.relationship("Task", backref="goal", lazy=True)


def goal_json(self):
return( {
"id": self.goal_id,
"title": self.title
}, 200)
Comment on lines +10 to +14

Choose a reason for hiding this comment

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

This method is the main reason wave 5 has some failures. Your routes logic is trying to call a method called goal_to_json but the method here is called goal_json. Also, we should only be returning the dictionary here, not the status code.

The following change would allow Wave 5 to pass!

Suggested change
def goal_json(self):
return( {
"id": self.goal_id,
"title": self.title
}, 200)
def goal_to_json(self):
return {
"id": self.goal_id,
"title": self.title
}

25 changes: 24 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,29 @@
from flask import current_app
from app import db

# from app.models.goal import Goal

Choose a reason for hiding this comment

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

This line is not necessary (it's commented, so you probably noticed that) 😄

We only need to import thee Goal identifier if we literally use the Goal constructor in this file. Even if we had a parameter that received a Goal, we wouldn't need to import the Goal constructor!

Additionally. since there was an import in goal.py for task.py, this import would cause a circular import dependency. Avoiding circular dependencies is why we have imports in strange places in app/__init__.py too!


class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
title = db.Column(db.String)
description = db.Column(db.String)
completed_at = db.Column(db.DateTime, nullable=True)

Choose a reason for hiding this comment

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

By default, columns are nullable. We really ony need to specify nullable is we want it to be False. The way the project description mentioned that this column should be nullable was a bit of a red herring.

But, we should think about whether the other columns should be nullable. Does it make sense for a task to have a NULL (None) description? How about a title?

We should think about what logically makes sense for our data, and as much as we can, use the database to help us protect the integrity of our data!

goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"), nullable=True)
__tablename__ = "tasks"

Choose a reason for hiding this comment

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

What prompted you to change the default table name?


def to_json(self):

Choose a reason for hiding this comment

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

This helper doesn't seem to be used in your code. By adding this method, we would be able to avoid building the json dictionary literals for tasks throughout the routes file.

By using this method (with the minor fixes mentioned below), most of wave 6 would pass.

if self.completed_at:
completed = True
else:
completed = False

task_dict={
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": completed,
"goal_id": self.goal_id # if the task belongs a goal add if not do not add
Comment on lines +20 to +24

Choose a reason for hiding this comment

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

There's a small typo in getting the task id (the field is called task_id). Also, we don't need to add the goal_id here, since it is selectively added just below.

Suggested change
"id": self.id,
"title": self.title,
"description": self.description,
"is_complete": completed,
"goal_id": self.goal_id # if the task belongs a goal add if not do not add
"id": self.task_id,
"title": self.title,
"description": self.description,
"is_complete": completed,

}

if self.goals_id:
task_dict["goal_id"]= self.goals_id
Comment on lines +27 to +28

Choose a reason for hiding this comment

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

Be careful of the field name. It should be goal_id.

Suggested change
if self.goals_id:
task_dict["goal_id"]= self.goals_id
if self.goal_id:
task_dict["goal_id"]= self.goal_id

return task_dict
273 changes: 272 additions & 1 deletion app/routes.py
Original file line number Diff line number Diff line change
@@ -1,2 +1,273 @@
from flask import Blueprint
from app import db
from flask import Blueprint, json
from sqlalchemy import asc, desc
from app.models import task
from app.models.task import Task
from app.models.goal import Goal
from datetime import datetime
from sqlalchemy.sql.functions import now
from flask import request, make_response, jsonify
from dotenv import load_dotenv
import os
import requests

load_dotenv()

task_bp = Blueprint("tasks", __name__, url_prefix="/tasks")
goals_bp = Blueprint("goals", __name__, url_prefix="/goals")
Comment on lines +16 to +17

Choose a reason for hiding this comment

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

We could consider putting all the routes related to the tasks endpoint in their own file, and doing the same for the goals routes.



# return all tasks
@task_bp.route("/", methods=["GET", "POST"], strict_slashes = False)
def handle_tasks():
if request.method == "GET":

Choose a reason for hiding this comment

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

The logic for GET and POST doesn't share any code, so we could consider putting the logic for each in separate functions, maybe get_tasks and create_task.


# tasks by asc and desc order
sort_by_url = request.args.get("sort") # query parameters and replace the previous query
if sort_by_url == "asc": # this is a list queried by title in asc order
tasks = Task.query.order_by(Task.title.asc()).all()
elif sort_by_url == "desc":
tasks = Task.query.order_by(Task.title.desc()).all()
else:
tasks = Task.query.all()
Comment on lines +26 to +32

Choose a reason for hiding this comment

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

👍

Nice handling of the sort parameter (great job pushing the sorting work to the database!), and a sensible fallback if the option is invalid (just return everything as usual).

# end of the new code

tasks_response = []
for task in tasks:
if task.completed_at == None:
completed_at = False
else:
completed_at = True

tasks_response.append({
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": completed_at,
Comment on lines +43 to +46

Choose a reason for hiding this comment

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

Places like this are where we'd like to use that to_json method in your Task class. That can really help reduce the repetition and chance for typos in our output code!

})
return make_response(jsonify(tasks_response), 200)


#create a task with null completed at
elif request.method == "POST":
request_body = request.get_json()
if "title" not in request_body or "description" not in request_body or "completed_at" not in request_body:
return {"details": "Invalid data"}, 400

if not "completed_at" in request_body or not request_body["completed_at"]:
completed_at = False
else:
completed_at = True

Comment on lines +54 to +61

Choose a reason for hiding this comment

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

👍

Good job on handling all these cases for creating a new Task. And you have just about the same thing in the PUT handler later on, which is exactly what you should have. But, we could think about moving this code to a helper, or even make it part of some logic in a custom "constructor" to avoid duplicating the code.

This can be created as a class method on Task, like:

    @classmethod
    def from_dict(values):
        # create a new task, and set the model values from the values passed in
        # be sure to validate that all required values are present, we could return `None` or raise an error if needed
        return new_task

new_task = Task(
title=request_body["title"],
description=request_body["description"],
completed_at=request_body["completed_at"]
)

db.session.add(new_task) # "adds model to the db"
db.session.commit() # commits the action above

return make_response({
"task":{
"id": new_task.task_id,
"title": new_task.title,
"description": new_task.description,
"is_complete": completed_at
}
}), 201

# return one task
@task_bp.route("/<task_id>", methods=["GET", "PUT", "DELETE"], strict_slashes = False)
def handle_task(task_id):
task = Task.query.get(task_id)

if request.method == "GET":
if task is None:
return make_response(f"Task #{task_id} not found"), 404
Comment on lines +86 to +87

Choose a reason for hiding this comment

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

All of these verbs need to check that the Task was valid, so this could be moved up and out of the per-verb logic.

We could think about using .get_or_404 as a helper that immediately terminates the request if the model instance isn't found.

And if the shared lookup code is reduced to using get_or_404, then we can seriously consider splitting each of these branches into its own function too, like get_task, update_task, and delete_task.

if not task.completed_at:
completed_at = False
else:
completed_at = task.completed_at["completed_at"]
return {

Choose a reason for hiding this comment

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

One reason wave 6 was failing was because here, goal_id needs to be included if the task is related to a goal, and otherwise excluded. Calling the updated to_json would accomplish this.

"task":{
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": completed_at
}
}, 200

# Update a task
elif request.method == "PUT":
if task is None:
return make_response(f"Task #{task_id} not found"), 404
form_data = request.get_json()

if "title" not in form_data or "description" not in form_data or "completed_at" not in form_data:
return {"details": "Invalid data"}, 400

if not "completed_at" in form_data or not form_data["completed_at"]:
is_complete = False
else:
is_complete = True
Comment on lines +107 to +113

Choose a reason for hiding this comment

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

I mentioned this above, but this and the equivalent POST code could benefit from being moved to a helper function.

task.title = form_data["title"]
task.description = form_data["description"]
task.completed_at = form_data["completed_at"]

db.session.commit()
return {
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": is_complete
}
}, 200

# Delete a task
elif request.method == "DELETE":
if not task:
return "", 404
db.session.delete(task)
db.session.commit()
return {
"details": f"Task {task_id} \"{task.title}\" successfully deleted"
}, 200


# modify complete task
@task_bp.route("/<task_id>/mark_complete", methods=["PATCH"], strict_slashes = False)
def mark_task_complete(task_id):
task = Task.query.get(task_id)

if not task: # this is the task check (wash my car)
return make_response(f"Task #{task_id} not found"), 404
task.completed_at = datetime.utcnow()
db.session.commit()

base_path = "https://slack.com/api/chat.postMessage"
slack_tocken = os.environ.get("SLACK_API_KEY")
query_params = {
"channel": "task-notifications",
"text": f"Someone just completed the {task.title}"
}
headers = {
"Authorization": f"Bearer {slack_tocken}"
}
requests.post(base_path, params=query_params, headers=headers)
Comment on lines +149 to +158

Choose a reason for hiding this comment

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

👍

Nice Slack integration. We could consider moving this to a helper function so that the main purpose of this function (marking the task complete) is easier to see.

Also, since we're sending a post request, we would typically send the parameters as form data, rather than as query params.

Query params do have a maximum length (as part of the HTTP standard), so when we have potentially large data (like a text message), we often send that data in the form-encoded body of a POST request (this stems from older web standards. Now, we might use JSON in the request body).

With the requests library, we can set the form-encoded body by using the data named parameter rather than params (which sets the query params).

    requests.post(base_path, data=query_params, headers=headers)


response_body = {
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": True # this is always true bc we are always setting completed at
}
}
return jsonify(response_body), 200

# modify incomplete task
@task_bp.route("/<task_id>/mark_incomplete", methods=["PATCH"])
def make_task_incomplete(task_id):
task = Task.query.get(task_id)

if task is None:
return make_response(f"Task #{task_id} not found"), 404
else:
task.completed_at = None
db.session.commit()
response_body = {
"task": {
"id": task.task_id,
"title": task.title,
"description": task.description,
"is_complete": False
}
}
return jsonify(response_body), 200


# goals route
@goals_bp.route("", methods = ["POST", "GET"], strict_slashes = False)
def handle_goals():

Choose a reason for hiding this comment

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

Similar feedback about splitting these functions, and moving validation and dictionary-handling logic around that I made for Task will also apply to Goal. These are common themes for any model with CRUD operations.

if request.method == "POST":
request_body = request.get_json()
if "title" not in request_body:
return {"details": "Invalid data"}, 400
new_goal = Goal(title = request_body["title"])
db.session.add(new_goal)
db.session.commit()
response_body = {
"goal": new_goal.goal_to_json()

Choose a reason for hiding this comment

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

The main issue with wave 5 was that your equivalent "to json" method was defined with the name goal_json instead. With the updated version of goal_to_json provided earlier, this should start working!

}
return jsonify(response_body), 201

#get all goals
if request.method == "GET":
goals = Goal.query.all()
response_body = []
for a_goal in goals:
response_body.append(a_goal.goal_to_json())
return jsonify(response_body), 200

# get one goal
@goals_bp.route("/<goal_id>", methods=["GET", "PUT", "DELETE"], strict_slashes = False)
def handle_goal(goal_id):
goal = Goal.query.get(goal_id)

if request.method == "GET":

if goal is None:
return make_response("", 404)

response_body = {
"goal": goal.goal_to_json()
}
return jsonify(response_body), 200

# Update a goal
elif request.method == "PUT":
if goal is None:
return make_response(f"Goal #{goal_id} not found"), 404
form_data = request.get_json()

if "title" not in form_data:
return {"details": "Invalid data"}, 400

goal.title = form_data["title"]
db.session.commit()

response_body = {
"goal": goal.goal_to_json()
}
return jsonify(response_body), 200

# Delete a goal
elif request.method == "DELETE":
if not goal:
return "", 404
db.session.delete(goal)
db.session.commit()

return {
"details": f"Goal {goal.goal_id} \"{goal.title}\" successfully deleted"
}
# one-to-many relationship
@goals_bp.route("/<goal_id>/tasks", methods = ["POST", "GET"], strict_slashes = False)
def handle_task_to_goals(goal_id):
goal = Goal.query.get(goal_id)
if not goal:
return "", 404

if request.method == "POST":
request_body = request.get_json()
task_ids = request_body["task_ids"]

for task_id in task_ids:
goal_id.task_id = goal_id

Choose a reason for hiding this comment

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

goal_id is a simple string (of read directly from query params) or number, so it doesn't have a field called task_id. Even a Goal instance (like goal) wouldn't. A Goal has the field tasks (created by our relationship code) to which we can append Task instances (not IDs). On the other side of the relationship each Task instance has a field called goal_id that can be used to associate a Task with a Goal. With the changes suggested in Goal, they also have a field called goal to which we can assign an actual Goal instance.

For any of these cases, we really need to lookup each passed task id to get an instance, then either set the goal/goal_id on each task, or update the tasks collection on the goal. Something like

            task = Task.query.get_or_404(task_id)
            task.goal = goal

This wouldn't remove any existing tasks from the goal (we should think about what to do for those tasks).

Additionally, to ensure that the change to the relationships occur transactionally (either they all apply, or none of them do), we can move the commit outside the loop, so that it gets called once at the end.


db.session.commit()

return {"id":(goal_id), "task_ids": task_ids},200

Choose a reason for hiding this comment

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

The test is expecting a numeric version of the goal id, so we can get the version stored in the goal itself (or force the conversion with int).

Suggested change
return {"id":(goal_id), "task_ids": task_ids},200
return {"id":(goal.goal_id), "task_ids": task_ids},200

Choose a reason for hiding this comment

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

The last bit that would be needed for wave 6 would be to handle GET requests for the tasks of a goal. Something like the following would be a good start.

    else if request.method == "GET":
        tasks = []
        for task in goal.tasks:
            tasks.append(task.to_json())

        return {
            "id": goal.goal_id, 
            "title": goal.title,
            "tasks": tasks
        }, 200


1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Generic single-database configuration.
45 changes: 45 additions & 0 deletions migrations/alembic.ini
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
Loading