Kayla Ecker - Accelerate #8
Conversation
There was a problem hiding this comment.
Great job!
All your tests are passing and your code is very readable. The main thing I'd focus on going forward would be in going after a lot of the repetitive code that shows up when working with CRUD operations on models. Mainly this would cover input handling, validation, and output generation. We could look into the use of python errors as a "native" way of doing error handling, or decorator functions to do some of the common tasks like looking up model records. We can also be really aggressive about pushing a lot of this code into model methods (and class methods). A model class is a class like any other, so we can put code that is more closely related to the model within the model class itself.
If we want to avoid a possible explosion of helper methods for rendering output, we can introduce classes that specialize in knowing how to render a model type. These are often referred to as data transfer objects (DTOs) which can present a curated view of a model.
Overall, nice work!
| @@ -0,0 +1 @@ | |||
| Generic single-database configuration. | |||
There was a problem hiding this comment.
Nice to see that your migrations appear to have survived intact over the course of the project. It seems like lots of folx needed to regenerate as an all-in-one migration at some point!
One reminder would be to add a message when generating a migration (-m "message") since this can help remind us which migration does what, and keep track of their relationships.
| from .routes import tasks_bp, goals_bp | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) |
There was a problem hiding this comment.
👍
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)| @@ -3,4 +3,6 @@ | |||
|
|
|||
|
|
|||
| class Goal(db.Model): | |||
| goal_id = db.Column(db.Integer, primary_key=True) | |||
| id = db.Column(db.Integer, primary_key=True) | |||
| title = db.Column(db.String) | |||
There was a problem hiding this comment.
Should we be able to create a goal with a NULL title? Consider adding nullable=False here.
| goal_id = db.Column(db.Integer, primary_key=True) | ||
| id = db.Column(db.Integer, primary_key=True) | ||
| title = db.Column(db.String) | ||
| tasks = db.relationship('Task', backref='goal', lazy=True) |
There was a problem hiding this comment.
👍
There are lots of interesting values that lazy can be set to. True is a synonym for "select", and works great here. But check out some of the other options. 😄
| @@ -3,4 +3,9 @@ | |||
|
|
|||
|
|
|||
| class Task(db.Model): | |||
| task_id = db.Column(db.Integer, primary_key=True) | |||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | |||
| def posts_message_to_slack(task): | ||
| slack_path = "https://slack.com/api/chat.postMessage" | ||
| slack_token = os.environ.get("SLACK_POST_MESSAGE_API_KEY") | ||
| params = { | ||
| "channel": "task-notifications", | ||
| "text": f"Task completed: {task.title}", | ||
| } | ||
| headers = {"Authorization": f"Bearer {slack_token}"} | ||
| requests.post(slack_path, params = params, headers = headers) |
There was a problem hiding this comment.
👍
Nice helper method!
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(slack_path, data=params, headers=headers)| ) | ||
|
|
||
| @goals_bp.route("/<goal_id>/tasks", strict_slashes=False, methods=["GET"]) | ||
| def get_goal_tasks(goal_id): |
There was a problem hiding this comment.
👍
Nice job splitting these two routes into separate functions.
| goals_bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
|
||
| @goals_bp.route("", strict_slashes=False, methods=["GET", "POST"]) | ||
| def handle_goals(): |
There was a problem hiding this comment.
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.
| goal = Goal.query.get(goal_id) | ||
| if not goal: | ||
| return make_response(f"Goal {goal_id} not found", 404) | ||
| tasks = goal.tasks |
There was a problem hiding this comment.
👍
Nice use of the helper relationship!
| task = Task.query.get(id) | ||
| goal.tasks.append(task) | ||
|
|
||
| db.session.add(goal) | ||
| db.session.commit() |
There was a problem hiding this comment.
👍
We do need to retrieve each Task to add it to the goal using the SqlAlchemy-created tasks field. But since the goal and each task are already retrieved from the DB, we don't need to add them to the session.
We can also wait to do the commit until after adding all the tasks. This will have the effect of committing this change all together in a single transaction, rather than running the risk of some of the tasks being added, and then possibly running into an error part of the way through (e.g. what if one of the task ids is invalid?).
Also, what would happen if the goal previously had some tasks set. Do we want to add the new tasks to the existing tasks? Do we want to replace them and sever any prior task → goal relationships? What behavior is implemented here?
No description provided.