-
Notifications
You must be signed in to change notification settings - Fork 36
Possum - Geetha #34
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?
Possum - Geetha #34
Changes from all commits
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,24 @@ | ||
| 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) : | ||
| return { | ||
| "id": self.id, | ||
| "title" : self.title | ||
| #"tasks" : [task.to_dict() for task in self.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. It would be nice to conditionally return You could create a goal dictionary and then add the tasks if necessary. goal = {
"id": self.id,
"title" : self.title
}
if self.tasks:
goal["tasks"] = [task.to_dict() for task in self.tasks]
return goal |
||
| } | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| return cls( | ||
| title = goal_data["title"] | ||
| #tasks = [Task.from_dict(task_data) for task_data in goal_data["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. Same as my comment above. How could you conditionally pass |
||
| ) | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,43 @@ | ||||||||||
| from sqlalchemy.orm import Mapped, mapped_column | ||||||||||
| from datetime import datetime | ||||||||||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||||||||||
| from ..db import db | ||||||||||
| from sqlalchemy import ForeignKey | ||||||||||
| from typing import Optional | ||||||||||
| from typing import TYPE_CHECKING | ||||||||||
| if TYPE_CHECKING: | ||||||||||
| from .goal import Goal | ||||||||||
|
|
||||||||||
| class Task(db.Model): | ||||||||||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||||||||||
| title: Mapped[str] | ||||||||||
| description: Mapped[str] | ||||||||||
| completed_at: Mapped[datetime | None] = mapped_column(nullable=True) | ||||||||||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("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. 👍 The foreign key goes on the model that represents the 'many' side of the one-to-many relationship. Since a goal has many tasks, the goal's id should be the foreign key for a task. |
||||||||||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||||||||||
|
|
||||||||||
| def to_dict(self) : | ||||||||||
| result = { | ||||||||||
| "id": self.id, | ||||||||||
| "title": self.title, | ||||||||||
| "description": self.description, | ||||||||||
| "is_complete": False if not self.completed_at else 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. While this logic does work, it does take a moment to understand what you are trying to achieve. We can compute the logic instead which might make the code more readable:
Suggested change
|
||||||||||
| } | ||||||||||
| #"is_complete" : self.is_complete() | ||||||||||
| if self.goal_id: | ||||||||||
| result["goal_id"] = self.goal_id | ||||||||||
| return result | ||||||||||
|
|
||||||||||
| @classmethod | ||||||||||
| def from_dict(cls, task_data): | ||||||||||
| return cls( | ||||||||||
| title = task_data["title"], | ||||||||||
| description = task_data["description"], | ||||||||||
| completed_at = task_data.get("completed_at"), | ||||||||||
| goal_id = task_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.
Suggested change
Nitpick: remove unnecessary blank line. Be sure to check that your code adheres to the language style guide before creating a PR for review, especially when you're at internship. Typically a teammate will ask you to clean up your code before they will review your work. Small style issues like this can signal inattentiveness to the code. |
||||||||||
| ) | ||||||||||
|
|
||||||||||
| # def is_complete(self): | ||||||||||
| # if self.completed_at is not None: | ||||||||||
| # return True | ||||||||||
| # return False | ||||||||||
|
Comment on lines
+40
to
+43
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 are several places throughout the project where you have left commented out code. Please take care to clean this up before opening a pull request for review. Especially when you're in internship. It clutters the codebase, can be confusing to other devs, and if it was accidentally uncommented and the code executed unintentionally then we'd have a bug.
Suggested change
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 want to call my attention to a question or parts of your code, you can leave a comment in Github just like I leave code review comments! I prefer not to have a helper function that returns True or False when we can compute the value for |
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,63 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from ..db import db | ||
| from ..models.goal import Goal | ||
| from ..models.task import Task | ||
| from .routes_utilities import validate_model, create_model, get_models_with_filters | ||
| from datetime import datetime | ||
|
|
||
| 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("/<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_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 send_list_of_task_to_goal_id(goal_id): | ||
| goal = validate_model(Goal,goal_id) | ||
| query = db.select(Task).where(Task.goal_id == goal.id) | ||
| existing_tasks = db.session.scalars(query).all() | ||
| for each_task in existing_tasks: | ||
| each_task.goal_id = None | ||
|
Comment on lines
+42
to
+45
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 is a more straightforward way to think about overwriting any existing tasks that were associated with the goal. We don't need to call db.select to get a Select object and query the DB for the tasks that were associated with a goal. We can leverage Python and re-assign tasks to the new tasks that comes from the request body from the POST request. |
||
| request_body = request.get_json() | ||
| task_ids= request_body.get("task_ids", []) | ||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| task.goal_id =goal.id | ||
| db.session.commit() | ||
|
Comment on lines
+46
to
+51
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. Instead of giving each new incoming task a goal id like you do on line 50, we reassign goal.tasks to be whatever comes from the request body. @bp.post("/<goal_id>/tasks")
def send_list_of_task_to_goal_id(goal_id):
goal = validate_model(Goal,goal_id)
request_body = request.get_json()
ids = request_body.get("task_ids",[])
goal.tasks = [validate_model(Task, id) for id in ids]
db.session.commit() |
||
| response_body = { | ||
| "id" : goal.id, | ||
| "task_ids" : [task.id for task in goal.tasks] | ||
| } | ||
|
Comment on lines
+52
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. It would be nice if your |
||
| return response_body, 200 | ||
|
|
||
| @bp.get("/<id>/tasks") | ||
| def get_all_tasks_with_one_goal(id): | ||
| goal = validate_model(Goal, id) | ||
| goal_dict = goal.to_dict() | ||
| goal_dict["tasks"] = [task.to_dict() for task in goal.tasks] | ||
| return goal_dict | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| from flask import abort, make_response | ||
| from ..db import db | ||
|
|
||
| def validate_model(cls, id): | ||
| try: | ||
| id = int(id) | ||
| except ValueError: | ||
| invalid = {"message": f"{cls.__name__} id ({id}) is invalid."} | ||
| abort(make_response(invalid, 400)) | ||
|
|
||
| query = db.select(cls).where(cls.id == id) | ||
| model = db.session.scalar(query) | ||
| if not model: | ||
| not_found = {"message": f"{cls.__name__} with id ({id}) not found."} | ||
| abort(make_response(not_found, 404)) | ||
|
|
||
| return model | ||
|
|
||
| def create_model(cls, model_data): | ||
| try: | ||
| new_model = cls.from_dict(model_data) | ||
| except KeyError as e: | ||
| response = {"details": "Invalid data"} | ||
| abort(make_response(response, 400)) | ||
|
|
||
| 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 attribute == "sort" and value.lower() == "asc": | ||
| query = query.order_by(cls.title.asc()) | ||
| elif attribute == "sort" and value.lower() == "desc": | ||
| query = query.order_by(cls.title.desc()) | ||
| elif hasattr(cls, attribute): | ||
| query = query.where(getattr(cls, attribute).ilike(f"%{value}%")) | ||
|
|
||
| models = db.session.scalars(query) | ||
| models_response = [model.to_dict() for model in models] | ||
| return models_response |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1 +1,70 @@ | ||||||||||||
| from flask import Blueprint | ||||||||||||
| from flask import Blueprint, request, Response, abort, make_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.
Suggested change
abort and make_response are imported but not accessed so they should be removed. |
||||||||||||
| from ..db import db | ||||||||||||
| from ..models.task import Task | ||||||||||||
| from .routes_utilities import validate_model, create_model, get_models_with_filters | ||||||||||||
| from datetime import datetime | ||||||||||||
| 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(): | ||||||||||||
| 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"] | ||||||||||||
| if "completed_at" in request_body: | ||||||||||||
| task.completed_at = request_body["completed_at"] | ||||||||||||
| else: | ||||||||||||
| 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() | ||||||||||||
| return Response(status=204, mimetype="application/json") | ||||||||||||
|
|
||||||||||||
| @bp.patch("/<task_id>/mark_complete") | ||||||||||||
| def mark_task_complete(task_id): | ||||||||||||
| task = validate_model(Task, task_id) | ||||||||||||
| task.completed_at = datetime.now() | ||||||||||||
| task.title="My Beautiful 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. The task title shouldn't be hardcoded to "My Beautiful Task" like we showed in an example. The
Suggested change
|
||||||||||||
| db.session.commit() | ||||||||||||
| slack_url = "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. Constant variables should be named with all caps
Suggested change
|
||||||||||||
| slack_request_body = { | ||||||||||||
| "channel": "#task-notifications", | ||||||||||||
|
Comment on lines
+52
to
+53
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. Prefer the channel name to be referenced by a constant variable too.
Suggested change
|
||||||||||||
| "text": f"Someone just completed the task {task.title}" | ||||||||||||
| } | ||||||||||||
| slack_headers = { | ||||||||||||
| "Authorization": f"Bearer {os.environ.get('SLACK_BOT_TOKEN')}", | ||||||||||||
| "Content-Type": "application/json" | ||||||||||||
| } | ||||||||||||
| requests.post(slack_url, json = slack_request_body, headers = slack_headers) | ||||||||||||
|
Comment on lines
+51
to
+60
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. I'd prefer this logic related to making a request to the Slack API to be wrapped in a helper function to keep this logic more concise and single responsibility. Maybe a helper function called something like |
||||||||||||
|
|
||||||||||||
| return Response(status=204, mimetype="application/json") | ||||||||||||
|
|
||||||||||||
| @bp.patch("/<task_id>/mark_incomplete") | ||||||||||||
| def mark_task_incomplete(task_id): | ||||||||||||
| task = validate_model(Task, task_id) | ||||||||||||
| task.completed_at = None | ||||||||||||
| db.session.commit() | ||||||||||||
| 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. |
| 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.
👍 A goal has many tasks!