Mikaela 🧚 #21
Mikaela 🧚 #21
Conversation
…l model, added tests, passed tests
…d tasks, db migrate, passed all tests.
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp |
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) |
There was a problem hiding this comment.
Using Optional[] is all that we need to indicate that a field is nullable so we don't need to also include mapped_column with nullable=True.
Here's the SQLAlchemy documentation about nullability
| completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) | |
| completed_at: Mapped[Optional[datetime]] |
| title=task_data["title"], | ||
| description=task_data["description"], | ||
| completed_at=task_data.get("completed_at"), | ||
| goal_id=task_data.get("goal_id") |
There was a problem hiding this comment.
Lines 31-32 you use square bracket notation for getting values and lines 33-34 you use get. Either way works, but get allows you to pass a default value if a key is not in the dictionary, which might be something you can leverage if needed.
Prefer that you pick one way and consistently use it throughout the project.
| try: | ||
| new_model = cls.from_dict(model_data) | ||
| except KeyError as e: | ||
| response = {"details": "Invalid data"} |
There was a problem hiding this comment.
It might be nice to provide even more details in the response you send back so that the client knows how to fix up the request body they will re-send.
| response = {"details": "Invalid data"} | |
| response = {"details": f"Invalid data for field: {error.args[0]}"} |
For example, this would evaluate to "title" for example if I sent a bad request body to create a Task.
| slack_bot_token = os.environ.get("SLACK_BOT_TOKEN") | ||
| slack_channel = os.environ.get("SLACK_CHANNEL", "task-notifications") | ||
|
|
||
| headers = { | ||
| "Authorization": f"Bearer {slack_bot_token}" | ||
| } | ||
|
|
||
| payload = { | ||
| "channel": slack_channel, | ||
| "text": f"Someone just completed the task {task.title}" | ||
| } | ||
|
|
||
| requests.post("https://slack.com/api/chat.postMessage", headers=headers, json=payload) |
There was a problem hiding this comment.
Prefer this logic to live in a helper function, maybe something like call_slack_api, to keep this route more concise and single-responsibility.
| @bp.get("/<id>") | ||
| def get_one_task(id): | ||
| task = validate_model(Task, id) | ||
| return jsonify({"task": task.to_dict()}), 200 |
There was a problem hiding this comment.
Recall that the default status code that Flask sends back is 200 so we don't need to explicitly return it here.
| return jsonify({"task": task.to_dict()}), 200 | |
| return {"task": task.to_dict()} |
Notice on line 39 above that you don't return any status code. Flask will return status code 200 by default there as well.
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return new_model.to_dict(), 201 No newline at end of file |
There was a problem hiding this comment.
Nice, no of jsonify here because we don't need it. A dictionary will be converted to JSON by Flask for us without invoking the method.
| goal = validate_model(Goal, id) | ||
| goal_dict = goal.to_dict() | ||
| goal_dict["tasks"] = [task.to_dict() for task in goal.tasks] | ||
| return goal_dict, 200 |
There was a problem hiding this comment.
There are several routes where you return status code 200. In all those routes, we can safely remove this from all the routes in your route files because Flask will return a status code of 200 by default.
| return goal_dict, 200 | |
| return goal_dict |
| @@ -1 +1,83 @@ | |||
| from flask import Blueprint | |||
| from flask import Blueprint, request, Response, jsonify | |||
There was a problem hiding this comment.
| from flask import Blueprint, request, Response, jsonify | |
| from flask import Blueprint, request, Response |
We do not need to use jsonify in our API because Flask will convert a list or dictionary into JSON that we send back as a response.
There are times when we need to create a Response object manually in order to set the mimetype to JSON and the status code to 204 (for our routes that don't return a response body, see line 44 in the PUT route in this file).
Anywhere you use jsonify should be removed.
| @bp.get("/<id>") | ||
| def get_one_goal(id): | ||
| goal = validate_model(Goal, id) | ||
| return jsonify({"goal": goal.to_dict()}), 200 |
There was a problem hiding this comment.
| return jsonify({"goal": goal.to_dict()}), 200 | |
| return {"goal": goal.to_dict()} |
Per my comment above, we should remove jsonify since we don't need it.
Also, anywhere you return status code 200 can be removed since that's the default status code Flask will return.
2 participants
No description provided.