Accelerate - Denisse #9
Conversation
There was a problem hiding this comment.
Great job!
All your tests are passing and your code is very readable.
One main thing I'd focus on going forward is identifying repetitive parts of the code and moving them to helper methods or class instance methods. We can create our own additional classes to help us organize common code and represent reusable ideas. Especially for CRUD models, there is a lot of very similar code. Even if it's not exactly the same, thinking about how to move that similar code into shared routines can help up organize and test our code. Our testing in this project focused on testing through the flask client helper, but we can write smaller unit tests similar to the tests we used in Video Party and Swap Meet, which are easier to write when the logic is pulled out into small units of code that focus on single responsibilities.
Specifically with respect to writing web apis with Flask and SqlAlchemy, keep looking at other examples and looking through the documentation for other ideas about helper methods and implementation patterns. Using decorator methods can also be a useful strategy for common tasks if we split our routes into separate methods.
Overall, you did really well, and the rest will come with continued practice and experimentation. Keep it up!!! 🎉
| @@ -3,4 +3,8 @@ | |||
|
|
|||
|
|
|||
| class Task(db.Model): | |||
| task_id = db.Column(db.Integer, primary_key=True) | |||
| id = db.Column(db.Integer, primary_key=True, autoincrement=True) | |||
There was a problem hiding this comment.
I totally agree with changing task_id to id! 🎉
| @@ -4,3 +4,5 @@ | |||
|
|
|||
| class Goal(db.Model): | |||
| goal_id = db.Column(db.Integer, primary_key=True) | |||
There was a problem hiding this comment.
Since you changed Task.task_id to Task.id, consider changing this to Goal.id too.
| title = db.Column(db.String) | ||
| description = db.Column(db.String) | ||
| completed_at = db.Column(db.DateTime, nullable=True, default=None) | ||
| goal_id = db.Column(db.Integer, db.ForeignKey("goal.goal_id"), nullable=True) |
There was a problem hiding this comment.
If you chose to rename Goal.goal_id to Goal.id, then the foreign key here would change to goal.id.
| @@ -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. 😄
| from .routes import tasks_bp, goal_bp | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goal_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)| task.title = request_body["title"] | ||
| task.description = request_body["description"] | ||
| task.completed_at = request_body["completed_at"] |
There was a problem hiding this comment.
I mentioned this already above, but we should be sure that the same fields required for POSTing are included here for PUT. PUT replaces the value for the supplied task id, so we should ensure that all of the values required to represent a Task are provided with the same restrictions as we had when creating it in the first place.
| return jsonify(patched_task),200 | ||
|
|
||
| # Slack Portion | ||
| def post_to_slack(text): |
There was a problem hiding this comment.
👍
This looks good. But it looks like you're not calling this from anywhere? Did you intentionally remove its use from your code?
| 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=params, headers=headers)|
|
||
| @goal_bp.route("", methods=["GET", "POST"]) | ||
| def handle_goals(): | ||
| if request.method == "GET": |
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 Goals. These are common themes for any model with CRUD operations.
| for id in request_body["task_ids"]: | ||
| 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.