Accelerate C15 - Trish G. #6
Conversation
Merging my branch `trish_again` with my master branch.
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 would be looking into ways to reduce the repetition in our code that tends to happen with CRUD operations on models. Handling input, validation, and generating output are areas where we can build helper methods to reduce the repetition and the chance for typos or code drift. Imagine what would happen if we decided that we needed to change the structure of our output consistently across out endpoints. Currently, we'd have to visit each endpoint and make that change, and each endpoint is an opportunity to introduce an error.
It's definitely fine to duplicate implementation as we're starting a section of work. We might not have a clear idea about which code similarities are truly related, and which are purely coincidental (in which case it may be dangerous to introduce a dependency between them), but as we progress, we can get a better idea where our code could benefit from helpers!
Overall, nice work!
| from .routes import tasks_bp | ||
| from .routes import goals_bp | ||
| # Register Blueprints here | ||
| 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)| @@ -4,3 +4,5 @@ | |||
|
|
|||
| class Goal(db.Model): | |||
| goal_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.
| @@ -4,3 +4,5 @@ | |||
|
|
|||
| class Goal(db.Model): | |||
| goal_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. 😄
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable=True) |
There was a problem hiding this comment.
Should title or description be allowed to be NULL? (Does that make sense from a data standpoint?) Consider adding nullable=False here.
The way the project emphasized that completed_at needs to accept NULL values may make it seem like we needed to explicitly call out that nullable should be True, but it turns out this is the default for nullable. Instead, we should think about the other data in our model and consider whether it makes sense for any of it to be NULL. If not, we can have the database help up protect against that happening!
| from app.models.task import Task | ||
| from app.models.goal import Goal | ||
| from flask import request, Blueprint, make_response, jsonify | ||
| from datetime import date, datetime |
There was a problem hiding this comment.
It doesn't look like date is used anywhere, so we can remove that.
| from datetime import date, datetime | |
| from datetime import datetime |
| if task.completed_at == None: | ||
| completed_at = False | ||
| else: | ||
| completed_at = True |
There was a problem hiding this comment.
We don't need to calculate this for DELETE
| slack_token = os.environ.get("SLACK_BOT_TOKEN") | ||
| slack_path = "https://slack.com/api/chat.postMessage" | ||
| query_params = { | ||
| "channel": "task-notifications", | ||
| "text": f"Someone just completed the {task.title}", | ||
| } | ||
|
|
||
| headers = {"Authorization": f"Bearer {slack_token}"} | ||
| requests.post(slack_path, params = query_params, headers = headers) |
There was a problem hiding this comment.
We could move this into a helper function so that the main purpose of this function (marking a task complete) is clearer.
| } | ||
|
|
||
| headers = {"Authorization": f"Bearer {slack_token}"} | ||
| requests.post(slack_path, params = query_params, headers = headers) |
There was a problem hiding this comment.
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=query_params, headers=headers)| }, 200 | ||
|
|
||
| @goals_bp.route("", methods=["POST", "GET"]) | ||
| 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.
| 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 should also be sure to validate that the Task retrieval succeeded.
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 (again, 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.