-
Notifications
You must be signed in to change notification settings - Fork 36
Possum - Mami Nishiwaki #23
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
f153de0
8369df0
a26d293
83c203c
1c0ee12
f2ca1f9
7abd0ed
f1b1fa9
1dd8362
07187ea
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_dict = { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| } | ||
| return goal_dict | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data): | ||
|
|
||
| new_goal = cls(title=data["title"]) | ||
| return new_goal | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,38 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from sqlalchemy import ForeignKey | ||
| from ..db import db | ||
| from typing import Optional, TYPE_CHECKING | ||
| from datetime import datetime | ||
|
|
||
| if TYPE_CHECKING: | ||
| from .goal import Goal | ||
|
Comment on lines
+7
to
+8
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] | ||
|
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 consistent use of |
||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] | ||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||
|
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. Nit: Leave a space between the end of the attributes and the following functions. This helps provide visual separation. |
||
| def to_dict(self): | ||
| task_dict = { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at is not None | ||
|
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 |
||
| } | ||
| if self.goal: | ||
| task_dict["goal_id"] = self.goal.id | ||
|
Comment on lines
+24
to
+25
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. In an actual application, I would probably favor including the |
||
| return task_dict | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data): | ||
| goal_id = data.get("goal_id") | ||
|
|
||
| new_task = cls( | ||
| title=data["title"], | ||
| description=data["description"], | ||
|
Comment on lines
+33
to
+34
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 |
||
| goal_id=goal_id) | ||
|
|
||
| return new_task | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,82 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from app.models.goal import Goal | ||
| from app.models.task import Task | ||
| from ..db import db | ||
| from .route_utilities import validate_model, create_model, get_models_with_filters | ||
|
|
||
| bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
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. |
||
|
|
||
| @bp.post("") | ||
| def create_goal(): | ||
| request_body = request.get_json() | ||
|
|
||
| return create_model(Goal, request_body) | ||
|
|
||
| @bp.post("/<goal_id>/tasks") | ||
| def create_task_with_goal(goal_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. While a typical reading of this endpoint (POST /goals/id/tasks) would be to create a Task associated with the specified goal, that's not how it was defined. A name such as |
||
|
|
||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| request_body = request.get_json() | ||
| task_ids = request_body["task_ids"] | ||
|
|
||
| task_list = [] | ||
| for id in task_ids: | ||
| task = validate_model(Task, id) | ||
| task_list.append(task) | ||
|
Comment on lines
+23
to
+26
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 validate all of the tasks in a list comprehension: task_list = [validate_model(Task, id) for id in task_ids] |
||
|
|
||
| goal.tasks = task_list | ||
|
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 the task collection to a new list of Task models takes care of unsetting any previous tasks associated with the goal, and updates the new Tasks, all in one little assignment. |
||
| db.session.commit() | ||
|
|
||
| return { | ||
| "id": goal.id, | ||
| "task_ids": task_ids | ||
| }, 200 | ||
|
Comment on lines
+31
to
+34
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. Rather than hard-coding this dict result here, we could create an additional instance method on |
||
|
|
||
|
|
||
| @bp.get("/<goal_id>/tasks") | ||
| def get_tasks_by_goal(goal_id): | ||
|
|
||
| goal = validate_model(Goal, goal_id) | ||
| tasks = [task.to_dict() for task in goal.tasks] | ||
|
|
||
| return { | ||
| "id": goal.id, | ||
| "title": goal.title, | ||
| "tasks": tasks | ||
| }, 200 | ||
|
Comment on lines
+41
to
+47
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 that the id and title keys here are the same as for a regular Goal GET, for which we wrote the Or we could make a different helper that itself uses the |
||
|
|
||
|
|
||
| @bp.get("") | ||
| def get_all_goals(): | ||
|
|
||
| return get_models_with_filters(Goal, request.args) | ||
|
|
||
| @bp.get("<goal_id>") | ||
| def get_one_goal(goal_id): | ||
|
|
||
| goal = validate_model(Goal, goal_id) | ||
| return goal.to_dict() | ||
|
|
||
|
|
||
| @bp.put("<goal_id>") | ||
| def update_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| request_body = request.get_json() | ||
|
|
||
| goal.title = request_body["title"] | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.delete("<goal_id>") | ||
| def delete_model(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/jason") | ||
|
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 typo cropped up again. Reducing the raw strings in our code minimizes this risk. |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| from flask import abort, make_response, request | ||
| 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} is 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} is 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 as error: | ||
| response = {"details": f"Invalid data"} | ||
| abort(make_response(response, 400)) | ||
|
Comment on lines
+23
to
+28
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 expecting the |
||
|
|
||
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return new_model.to_dict(), 201 | ||
|
|
||
|
|
||
| def get_models_with_filters(cls, filters=None): | ||
| query = db.select(cls) | ||
|
|
||
| if filters: | ||
| for attribute, value in filters.items(): | ||
| if hasattr(cls, attribute): | ||
| query = query.where(getattr(cls, attribute).ilike(f"%{value}%")) | ||
|
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 gradual build-up of the |
||
|
|
||
| if attribute == "sort": | ||
| if value == "desc": | ||
| query = query.order_by(cls.title.desc()) | ||
| elif value == "asc": | ||
| query = query.order_by(cls.title.asc()) | ||
|
Comment on lines
+44
to
+48
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 pick the sort direction based on the query parameter value. This works great for the required single sort field, but we might think about how to generalize this (similar to the filtering logic) if we wanted to be able to sort on multiple columns! |
||
|
|
||
| models = db.session.scalars(query.order_by(cls.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. 👍 Nice addition of id sorting even when one of the title sorting approaches is specified. This will sort first by the title order, then fall back to the task id, ensuring there is always an ordering applied. |
||
| models_response = [model.to_dict() for model in models] | ||
|
|
||
| return models_response | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,84 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from app.models.task import Task | ||
| from ..db import db | ||
| from datetime import datetime | ||
| import requests | ||
| import os | ||
| from .route_utilities import validate_model, create_model, get_models_with_filters | ||
|
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. |
||
|
|
||
| bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks") | ||
|
|
||
| @bp.post("") | ||
| def create_task(): | ||
|
|
||
| request_body = request.get_json() | ||
| return create_model(Task, request_body) | ||
|
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! |
||
|
|
||
|
|
||
| @bp.get("") | ||
| def get_all_tasks(): | ||
|
|
||
| return get_models_with_filters(Task, request.args) | ||
|
|
||
|
|
||
| @bp.get("/<task_id>") | ||
| def get_one_task(task_id): | ||
|
|
||
| task = validate_model(Task, task_id) | ||
| return task.to_dict() | ||
|
|
||
|
|
||
| @bp.put("/<task_id>") | ||
| def update_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
| request_body = request.get_json() | ||
|
|
||
| task.title = request_body["title"] | ||
| task.description = request_body["description"] | ||
|
|
||
| db.session.commit() | ||
|
Comment on lines
+33
to
+39
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 the data requirements for updating are to creating. After validating the record to update, we require certain keys to be present, 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? |
||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
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. If you find it tiresome to write this out all over the place, consider making a helper like |
||
|
|
||
|
|
||
| @bp.patch("/<task_id>/mark_complete") | ||
| def mark_complete_task(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. 👍 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, task_id) | ||
|
|
||
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
|
|
||
| # --- send message to slack channel --- # | ||
| url = "https://slack.com/api/chat.postMessage" | ||
| data = { | ||
| "text": f"Someone just completed the task {task.title}😺", | ||
| "channel": "test-slack-api" | ||
| } | ||
| headers = { | ||
| 'Content-Type': 'application/json', | ||
|
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 because you used the |
||
| 'Authorization': f"Bearer {os.environ.get('SLACK_BOT_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. Nice use of |
||
| } | ||
| requests.post(url, headers=headers, json=data) | ||
|
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. 👍 Here, we send the request and then move on with our logic without checking the result from Slack. This is fine here, since sending the message is just a side effect of completing the task. For example, it's unlikely we'd want to fail to complete a task in the event that Slack wasn't reachable. In a fuller application, we might write out the result of this call to a log file so that if a problem with our calls to Slack does occur, we'd have a record to investigate. |
||
| # ----------------------------------- # | ||
|
Comment on lines
+51
to
+62
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. As we add more logic to a route, it becomes less clear what its main focus is. We could help improve the understandability by moving the logic related to sending a notification to a helper function, maybe |
||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.patch("/<task_id>/mark_incomplete") | ||
| def mark_incomplete_task(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. 👍 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, task_id) | ||
|
|
||
| task.completed_at = None | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
|
|
||
| @bp.delete("/<task_id>") | ||
| def delete_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| db.session.delete(task) | ||
| db.session.commit() | ||
|
Comment on lines
+79
to
+82
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 the structure of deleting is to our other routes. 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? |
||
|
|
||
| return Response(status=204, mimetype="application/jason") | ||
|
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 the typo in the |
||
| 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.
👍 Your Goal implementation is consistent with your Task model. Looks good!