-
Notifications
You must be signed in to change notification settings - Fork 36
Possum - Mumina A. #16
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
7b4e672
0119e1f
d673698
dc1c7fc
2d212f8
c0f8efb
a6218fe
4c299f2
9f50686
2845075
dd637c6
cc89207
0d76578
761c681
48d3ed2
b6c7cab
60ea7cf
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,26 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from ..db import db | ||
| from typing import List | ||
|
|
||
| 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") | ||
|
|
||
| from typing import TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
| from .task import Task | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data): | ||
|
|
||
| return cls( | ||
| title = data['title'], | ||
| ) | ||
|
|
||
| def to_dict(self): | ||
|
|
||
| return { | ||
| 'id': self.id, | ||
| 'title': self.title, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,73 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from ..db import db | ||
| from datetime import datetime, timezone | ||
| from sqlalchemy import ForeignKey | ||
| from typing import Optional | ||
|
|
||
| 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. |
||
| # completed_at: Mapped[Optional[datetime]] = mapped_column() | ||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||
|
|
||
| from typing import TYPE_CHECKING | ||
| if TYPE_CHECKING: | ||
| from .goal import Goal | ||
|
Comment on lines
+16
to
+18
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. |
||
|
|
||
| # Create a new Task object from data we received in a JSON request | ||
| @classmethod | ||
| def from_dict(cls, data): | ||
| title = data["title"] | ||
| description = data["description"] | ||
|
Comment on lines
+23
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. 👍 By reading title and description directly as keys we can trigger a However, note that the actual constructor call later on re-reads the values from the |
||
|
|
||
| # is_complete may or may not be in data | ||
| is_complete = data.get("is_complete", False) | ||
|
|
||
| completed_at = ( | ||
| datetime.now(timezone.utc) if is_complete else None | ||
| ) | ||
|
Comment on lines
+27
to
+31
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. The request body for the Task create route is documented as accepting a The actual constructor call below ignores the results of this calculation, so really this could all be removed.
Comment on lines
+30
to
+31
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: Watch the indentation. If wrapping a line, indent the line, and make sure the closing character (the |
||
|
|
||
| return cls( | ||
| title = data['title'], | ||
| description = data['description'], | ||
| completed_at = data.get('completed_at'), | ||
| goal_id=data.get("goal_id", 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. There wasn't a requirement to check for the Note that the default value for |
||
| ) | ||
|
|
||
| # Turn this Task object into a dictionary so we can send it as JSON in a response | ||
| def to_dict(self, include_goal: Optional[bool] = 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. 👍 If you still wanted to have a way for a caller to override the goal behavior, this is a reasonable way to do it. As I already wrote, I think it's a little odd that goal_id is conditionally included at all (I would have always included it, and just let it return null for tasks not part of a goal for shape consistency), but it does make sense for the way the waves are structured. |
||
| """Return a dict representation of the Task. | ||
|
|
||
| By default (include_goal is None) this will include the task's | ||
| `goal_id` when the task belongs to a goal. Callers can override | ||
| this by passing True/False explicitly for include_goal. | ||
| """ | ||
|
|
||
| # If caller didn't specify, include goal_id when it's present on the task | ||
| if include_goal is None: | ||
| include_goal = self.goal_id is not None | ||
|
|
||
| base = { | ||
| 'id': self.id, | ||
| 'title': self.title, | ||
| 'description': self.description, | ||
| 'is_complete': bool(self.completed_at), | ||
|
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 include_goal: | ||
| base['goal_id'] = self.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 do want to only conditionally include the goal_id information, but rather than having the caller be responsible for deciding this, instead, we'd like the Task to include this information if it has it, and exclude it if it doesn't. This will allow any route that returns task information to be able to indicate whether that task is a part of a goal without every route having logic to perform the check per task. That being said, in an actual application, I would probably favor including the |
||
|
|
||
| return base | ||
|
|
||
|
|
||
| # tasks = [ | ||
| # Task(id=1, title='assignment', description='complete assignment for wave 1'), | ||
| # Task(id=2, title='gym', description='go to the gym'), | ||
| # Task(id=3, title='errands', description='run errands'), | ||
| # Task(id=4, title='groceries', description='buy groceries'), | ||
| # ] | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,86 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from .route_utilities import create_model, validate_model | ||
| from app.models.goal import Goal | ||
| from app.db import db | ||
| from app.models.task import Task | ||
|
|
||
| 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 largely consistent with your Task CRUD routes, so keep an eye out for comments in the task routes that could also apply here. Note that while there wasn't an explicit filter/sort requirement for Goals, part of the benefit of having the fitler and sort logic in a helper is that it would allow us to apply that behavior here as well. |
||
|
|
||
| @bp.post('') | ||
| def create_goal(): | ||
| request_body = request.get_json() | ||
| return create_model(Goal, request_body) | ||
|
|
||
| @bp.get('') | ||
| def get_all_goals(): | ||
| query = db.select(Goal) | ||
|
|
||
| query = query.order_by(Goal.id) | ||
|
|
||
| goals = db.session.scalars(query) | ||
|
|
||
| goal_response = [] | ||
|
|
||
| for goal in goals: | ||
| goal_response.append(goal.to_dict()) | ||
|
Comment on lines
+22
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. List comprehension opportunity. |
||
|
|
||
| return goal_response | ||
|
|
||
| @bp.get("/<goal_id>") | ||
| def get_goal_by_id(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| return goal.to_dict() | ||
|
|
||
| @bp.put('/<goal_id>') | ||
| def update_one_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_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype='application/json') | ||
|
|
||
| @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() | ||
| # Get the list of task IDs from the request body | ||
| task_ids = request_body.get("task_ids") | ||
|
|
||
| if not task_ids or not isinstance(task_ids, list): | ||
| return {"details": "Invalid data"}, 400 | ||
|
|
||
| # remove all current tasks from this goal | ||
| for task in goal.tasks: | ||
| task.goal_id = None | ||
|
|
||
| # For each task ID, find the task and associate it with the goal | ||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| task.goal_id = goal.id | ||
|
Comment on lines
+64
to
+71
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. Since we need to validate each Task id anyway, if we stored the resulting tasks in a list, the replacement of the tasks for the goal could be accomplished using the goal.tasks = taskswhich avoids the need to manually clear the goal association from the existing tasks. |
||
|
|
||
| db.session.commit() | ||
|
|
||
| return { | ||
| "id": goal.id, | ||
| "task_ids": task_ids | ||
| }, 200 | ||
|
|
||
| @bp.get('/<goal_id>/tasks') | ||
| def get_task_for_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| tasks = [task.to_dict() for task in goal.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. 👍 Modifying the default behavior in |
||
| response = goal.to_dict() | ||
| response["tasks"] = tasks | ||
|
Comment on lines
+84
to
+85
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 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| from flask import abort, make_response | ||
| from ..db import db | ||
|
|
||
| # Get a Task by ID, validate the ID, and return 404 if not found | ||
| def validate_model(cls, model_id): | ||
| try: | ||
| model_id = int(model_id) | ||
| except: | ||
| 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 | ||
|
|
||
| # Create a new Task from a dictionary (like JSON data) | ||
| 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)) | ||
|
|
||
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return new_model.to_dict(), 201 | ||
|
|
||
| def get_model_with_filters(model, filters=None, sort_by=None, sort_order=None): | ||
| query = db.select(model) | ||
|
|
||
| # Apply filters (partial match using ilike) | ||
| if filters: | ||
| for attr, value in filters.items(): | ||
| if value is not None: | ||
| column = getattr(model, attr) | ||
| query = query.where(column.ilike(f"%{value}%")) | ||
|
|
||
| # Apply sorting | ||
| sort_by = sort_by or "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. Rather than picking to sort either by the specified column OR the id, it's always worth tacking on the id sorting at the end. If there were multiple records with the same sort column value, having the id as the fallback sort would still ensure a consistent order in the results. |
||
| column = getattr(model, sort_by) | ||
|
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 general column fetching logic for sorting. Note that we could even do this to get the direction! |
||
| if sort_order == "desc": | ||
| query = query.order_by(column.desc()) | ||
| else: | ||
| query = query.order_by(column.asc()) | ||
|
|
||
| return db.session.scalars(query) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,93 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from app.models.task import Task | ||
| from app.routes.route_utilities import validate_model, create_model, get_model_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. |
||
| from app.db import db | ||
| from datetime import datetime, timezone | ||
| import os | ||
| import requests | ||
|
|
||
| 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(): | ||
| title_param = request.args.get('title') | ||
| description_param = request.args.get('description') | ||
| sort = request.args.get('sort') | ||
|
|
||
| filters = { | ||
| "title": title_param, | ||
| "description": description_param | ||
| } | ||
|
|
||
| sort_by = "title" if sort in ["asc", "desc"] else "id" | ||
| sort_order = sort if sort in ["asc", "desc"] else "asc" | ||
|
Comment on lines
+18
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. Consider moving this logic into the filter route helper itself. While having logic like we see here to extract only specific params relevant to the model type is a great idea, we might consider having the model-specific data stored with the Task model itself, rather than having this route "know" which fields are filterable and sortable. It is a bit up for debate whether that information should live directly with the model, but at the very least, if that information were moved out of the route in a consistent fashion, we could follow a similar pattern for other model routes, and generalize even more code. |
||
|
|
||
| tasks = get_model_with_filters(Task, filters=filters, sort_by=sort_by, sort_order=sort_order) | ||
|
|
||
| return [task.to_dict() for task in tasks] | ||
|
|
||
| @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_one_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
| request_body = request.get_json() | ||
|
|
||
| task.title = request_body['title'] | ||
| task.description = request_body['description'] | ||
| task.completed_at = request_body.get('completed_at') | ||
|
Comment on lines
+41
to
+46
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? |
||
| db.session.commit() | ||
|
|
||
| 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.delete('/<task_id>') | ||
| def delete_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| db.session.delete(task) | ||
| db.session.commit() | ||
|
Comment on lines
+53
to
+56
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/json') | ||
|
|
||
| @bp.patch('/<task_id>/mark_complete') | ||
| def mark_complete_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
| url = 'https://slack.com/api/chat.postMessage' # Slack API endpoint | ||
|
|
||
| task.completed_at = datetime.now(timezone.utc) # Mark task as completed | ||
|
|
||
| db.session.commit() | ||
|
Comment on lines
+62
to
+67
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. |
||
|
|
||
| # Set headers for the Slack API request | ||
| headers = { | ||
| "Authorization": f"Bearer {os.environ.get('SLACK_API')}", # Slack 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 |
||
| "Content-Type": "application/json" # Sending 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. Note that because you used the |
||
| } | ||
|
|
||
| # Prepare the data payload for Slack | ||
| data = { | ||
| "channel": "task-notifications", # Slack channel to post message | ||
| "text": f"Someone just completed the task {task.title}" # Message content | ||
| } | ||
|
|
||
| requests.post(url, headers=headers, json=data) # Send notification to Slack | ||
|
Comment on lines
+69
to
+81
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 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. |
||
|
|
||
| return Response(status=204, mimetype='application/json') | ||
|
|
||
| @bp.patch('/<task_id>/mark_incomplete') | ||
| def mark_incomplete_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| task.completed_at = None | ||
|
|
||
| db.session.commit() | ||
|
Comment on lines
+87
to
+91
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. |
||
|
|
||
| return Response(status=204, mimetype='application/json') | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| Single-database configuration for Flask. |
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.