-
Couldn't load subscription status.
- Fork 44
Bumblebee - Esther K. #26
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
base: main
Are you sure you want to change the base?
Changes from all commits
7abc8f2
c4c5f7f
c4b2d73
68f60f6
44d2fbd
8ff8680
54cfdb7
c6a2082
be8f552
fd4561f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,25 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from ..db import db | ||
| from typing import TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
| from .task import Task | ||
|
|
||
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| tasks: Mapped[list["Task"]] = relationship(back_populates="goal") | ||
|
|
||
| def to_dict(self): | ||
| goal_as_dict = { | ||
| "id": self.id, | ||
| "title": self.title | ||
| } | ||
|
|
||
| return goal_as_dict | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, task_data): | ||
| new_goal = cls(title=task_data["title"]) | ||
|
|
||
| return new_goal | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,37 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from sqlalchemy import ForeignKey | ||
| from datetime import datetime | ||
| from ..db import db | ||
| from typing import Optional, TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
| from .goal import Goal | ||
|
Comment on lines
+6
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice use of this snippet to suppress the squiggles in VS Code. |
||
|
|
||
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[datetime] = mapped_column(nullable=True) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting Generally, if there are multiple ways to achieve the same outcome, we should be consistent in our approaches, as encountering different approaches requires the reader to stop and think through whether there is a particular reason why the different approaches are used. |
||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||
|
|
||
| def to_dict(self): | ||
| task_as_dict = { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": True if self.completed_at else False, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even though we only use the calculated And even though this line is concise, notice that it's still effectively of the form if some_condition: # (some condition is either truthy or falsy)
return True # (returns True if some_condition is truthy)
else:
return False # (returns False if some_condition is falsy)This is an antipattern, which we should try to avoid. Rather than checking the condition and using that to pick between bool(some_condition) # (None would become False, and a completed date would be True)or if we prefer to be more explicit some_condition is not None # (same behavior) |
||
| } | ||
|
Comment on lines
+19
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Not including the outer task "envelope" wrapper in our |
||
|
|
||
| if self.goal_id: | ||
| task_as_dict["goal_id"] = self.goal_id | ||
|
Comment on lines
+26
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice logic to add the goal data to the task dictionary only when it's present. |
||
|
|
||
| return task_as_dict | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, task_data): | ||
| new_task = cls( | ||
| title=task_data["title"], | ||
| description=task_data["description"]) | ||
|
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 By reading title and description directly as keys we can trigger a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Though none of our tests attempt to pass However, once we have settled on our own method of representing |
||
|
|
||
| return new_task | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Your Goal CRUD routes are consistent with your Task CRUD routes. Check the Task feedback for anything that could apply here. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,79 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from app.models.goal import Goal | ||
| from app.models.task import Task | ||
| from .route_utilities import validate_model, create_model | ||
| from .route_utilities import get_models_with_filters | ||
| from ..db import db | ||
|
|
||
|
|
||
| bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
|
||
|
|
||
| @bp.post("") | ||
| def create_goal(): | ||
| request_body = request.get_json() | ||
| return create_model(Goal, request_body) | ||
|
|
||
|
|
||
| @bp.get("") | ||
| def get_all_goals(): | ||
| return get_models_with_filters(Goal, request.args) | ||
|
|
||
|
|
||
| @bp.get("/<id>") | ||
| def get_one_goal(id): | ||
| goal = validate_model(Goal, id) | ||
|
|
||
| return {"goal": goal.to_dict()} | ||
|
|
||
|
|
||
| @bp.put("/<id>") | ||
| def update_goal(id): | ||
| goal = validate_model(Goal, id) | ||
| request_body = request.get_json() | ||
|
|
||
| goal.title = request_body["title"] | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.delete("/<id>") | ||
| def delete_goal(id): | ||
| goal = validate_model(Goal, id) | ||
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.post("/<goal_id>/tasks") | ||
| def add_tasks_to_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| request_body = request.get_json() | ||
|
|
||
| for task in goal.tasks: | ||
| task.goal_id = None | ||
|
|
||
| tasks = [validate_model(Task, id) for id in request_body["task_ids"]] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice way to validate each of the task ids, getting an actual Task model. |
||
|
|
||
| for task in tasks: | ||
| task.goal_id = goal_id | ||
|
Comment on lines
+60
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could make use of the goal.tasks = tasks |
||
|
|
||
| db.session.commit() | ||
|
|
||
| response = { | ||
| "id": goal.id, | ||
| "task_ids": request_body["task_ids"] | ||
| } | ||
|
|
||
| return response, 200 | ||
|
|
||
|
|
||
| @bp.get("/<goal_id>/tasks") | ||
| def get_all_tasks_of_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| response = goal.to_dict() | ||
| response["tasks"] = [task.to_dict() for task in goal.tasks] | ||
|
Comment on lines
+76
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice reuse of your |
||
|
|
||
| return response, 200 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice use of the main 3 route helpers introduced in the curriculum. This isn't the only way to address the common behaviors across the model routes. There are details we didn't explore. These could be organized in other ways. There are even other routes that could benefit from similar helpers (such as PUT and DELETE). We should keep an eye out for additional opportunities to DRY our code and separate responsibilities as we continue to work with larger codebases. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| from flask import abort, make_response | ||
| from ..db import db | ||
|
|
||
|
|
||
| def validate_model(cls, model_id): | ||
| try: | ||
| model_id = int(model_id) | ||
| except ValueError: | ||
| response = {"message": f"{cls.__name__} {model_id} invalid"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
| query = db.select(cls).where(cls.id == model_id) | ||
| model = db.session.scalar(query) | ||
|
|
||
| if not model: | ||
| response = {"message": f"{cls.__name__} {model_id} not found"} | ||
| abort(make_response(response, 404)) | ||
|
|
||
| return model | ||
|
|
||
|
|
||
| def create_model(cls, model_data): | ||
| try: | ||
| new_model = cls.from_dict(model_data) | ||
|
|
||
| except KeyError: | ||
| response = {"details": "Invalid data"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return {cls.__name__.lower(): new_model.to_dict()}, 201 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice customization of |
||
|
|
||
|
|
||
| def get_models_with_filters(cls, filters=None): | ||
| query = db.select(cls) | ||
| sort = "" | ||
|
|
||
| if filters: | ||
| for attribute, value in filters.items(): | ||
| if hasattr(cls, attribute): | ||
| query = query.where(getattr(cls, attribute).ilike(f"%{value}%")) | ||
| if attribute == "sort": | ||
| sort = value | ||
|
|
||
| if sort == "asc": | ||
| models = db.session.scalars(query.order_by(cls.title.asc())) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of |
||
| elif sort == "desc": | ||
| models = db.session.scalars(query.order_by(cls.title.desc())) | ||
| else: | ||
| models = db.session.scalars(query.order_by(cls.id)) | ||
|
Comment on lines
+44
to
+52
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice customization of |
||
|
|
||
| models_response = [model.to_dict() for model in models] | ||
| return models_response | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Great use of the route helpers to simplify our task routes! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,85 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from app.models.task import Task | ||
| from .route_utilities import validate_model, create_model | ||
| from .route_utilities import get_models_with_filters | ||
| from ..db import db | ||
| from datetime import datetime | ||
| import requests | ||
| import os | ||
|
|
||
| bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||
|
|
||
|
|
||
| @bp.post("") | ||
| def create_task(): | ||
| request_body = request.get_json() | ||
| return create_model(Task, request_body) | ||
|
|
||
|
|
||
| @bp.get("") | ||
| def get_all_tasks(): | ||
| return get_models_with_filters(Task, request.args) | ||
|
|
||
|
|
||
| @bp.get("/<id>") | ||
| def get_one_task(id): | ||
| task = validate_model(Task, id) | ||
|
|
||
| return {"task": task.to_dict()} | ||
|
|
||
|
|
||
| @bp.put("/<id>") | ||
| def update_task(id): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice how similar updating is to creating. After validating the record to update, we require certain keys to be present, others can be optional, then after updating and committing the model, we return a common response. How could we add model or route helpers to simplify our PUT route? |
||
| task = validate_model(Task, id) | ||
| request_body = request.get_json() | ||
|
|
||
| task.title = request_body["title"] | ||
| task.description = request_body["description"] | ||
| if "completed_at" in request_body: | ||
| task.completed_at = request_body["completed_at"] | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.delete("/<id>") | ||
| def delete_task(id): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Notice how similar deleting is to getting. After validating the record to delete, we delete and commit it, then return a common response. How could we add model or route helpers to simplify our DELETE route? |
||
| task = validate_model(Task, id) | ||
| db.session.delete(task) | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.patch("/<id>/mark_incomplete") | ||
| def marks_task_incomplete(id): | ||
|
Comment on lines
+54
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice route to mark the task incomplete. We can use our validation helper to get the same behavior as the other id-based routes, leaving our route responsible only for clearing the completion date, saving it, and generating the response. One thing we might still consider is moving the actual update logic into a model helper so that the Task model class itself is responsible for "knowing" how to mark a Task incomplete. |
||
| task = validate_model(Task, id) | ||
| task.completed_at = None | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.patch("/<id>/mark_complete") | ||
| def marks_task_complete(id): | ||
|
Comment on lines
+63
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice route to mark the task complete. We can use our validation helper to get the same behavior as the other id-based routes, leaving our route responsible only for updating the record with the current datetime, saving it, and generating the response. One thing we might still consider is moving the actual update logic into a model helper so that the Task model class itself is responsible for "knowing" how to complete a Task. |
||
| task = validate_model(Task, id) | ||
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
|
|
||
| send_slack_notif(task) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice use of a helper function to hold the logic for performing the notification. Prefer to spell out the words in the name fully rather than abbreviating them (notification rather than notif). |
||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| def send_slack_notif(task): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function encapsulates the responsibility of sending a completion notification about the provided task. Notice that we could make a further helper function that wraps the responsibility of sending a message to a specified channel. This function would then be responsible for the logic of building the messaging, and knowing what channel to use. Even the logic to build the notification message based on the task could be in its own helper. Think about whether such a potential function would be a model method, or some other method to which we pass a Task. |
||
| data = { | ||
| "token": os.environ.get('SLACKBOT_TOKEN'), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the Slack API documentation prefers passing the API key in the header rather than in the request body. Passing the token as part of the request body works here due to the use of the |
||
| "channel": os.environ.get('SLACK_CHANNEL'), | ||
|
Comment on lines
+76
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Using |
||
| "text": f"Someone just complete the task {task.title}" | ||
| } | ||
|
|
||
| response = requests.post( | ||
| "https://slack.com/api/chat.postMessage", | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could store the Slack API URL in the |
||
| data=data) | ||
|
|
||
| return response | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Single-database configuration for Flask. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| # 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,flask_migrate | ||
|
|
||
| [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 | ||
|
|
||
| [logger_flask_migrate] | ||
| level = INFO | ||
| handlers = | ||
| qualname = flask_migrate | ||
|
|
||
| [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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's always a risk of forgetting to rename things if we copy/paste from a similar file. If we know we're likely to copy from a file, a more neutral name can help avoid cases like this.