-
Notifications
You must be signed in to change notification settings - Fork 36
Crow - Stephanie Lin Chen #25
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
779b5e8
2b9495a
4810805
f388a4b
109577d
ac93aeb
4c6edbb
98f9ea5
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,6 +1,8 @@ | ||
| from flask import Flask | ||
| from .db import db, migrate | ||
| from .models import task, goal | ||
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp | ||
| import os | ||
|
|
||
| def create_app(config=None): | ||
|
|
@@ -18,5 +20,8 @@ def create_app(config=None): | |
| migrate.init_app(app, db) | ||
|
|
||
| # Register Blueprints here | ||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) | ||
|
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. ✅ |
||
|
|
||
|
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. Make sure that you aren't padding your codebase with whitespace and to surround your different blocks of logic with a single line of whitespace above/below. |
||
|
|
||
| return app | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,20 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from typing import Optional | ||
| from ..db import db | ||
|
|
||
| class Goal(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. ✅ |
||
| tasks: Mapped[list["Task"]] = relationship(back_populates="goal") | ||
|
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. Perfect! You are making a relationship attribute on the |
||
|
|
||
|
|
||
| def to_dict(self): | ||
| response_dict = { | ||
| "id": self.id, | ||
| "title": self.title | ||
| } | ||
| return response_dict | ||
|
Comment on lines
+11
to
+16
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. A way to D.R.Y. up your codebase is a flag as a parameter, say def to_dict(self, with_tasks=False):
goal_dict = {
self.id,
self.title
}
if with_tasks:
goal_dict["tasks"] = [task.to_dict() for task in self.tasks]
return goal_dict |
||
|
|
||
| @classmethod | ||
| def from_dict(cls, task_data): | ||
| return cls(title=task_data["title"]) | ||
|
Comment on lines
+18
to
+20
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. ✅ |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,35 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from sqlalchemy import ForeignKey | ||
| from typing import Optional | ||
| from datetime import datetime | ||
| from ..db import db | ||
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] | ||
| description: Mapped[str] | ||
| completed_at: Mapped[Optional[datetime]] = mapped_column(default=None) | ||
| 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. ✅ |
||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||
|
|
||
| def to_dict(self): | ||
| response_dict = { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": False if not self.completed_at else True | ||
| } | ||
| if self.goal_id: | ||
| response_dict["goal_id"] = self.goal_id | ||
|
|
||
| return response_dict | ||
|
Comment on lines
+15
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. The code above could be rewritten more cleanly with the following: def to_dict(self):
return {
'id': self.id,
'title': self.title,
'description': self.description,
'is_complete': bool(self.completed_at),
**({'goal_id': self.goal_id} if self.goal_id else {})
}The final line is using |
||
|
|
||
| @classmethod | ||
| def from_dict(cls, task_data): | ||
| params = ["title","description"] | ||
| kwarg_dict = {param:task_data[param] for param in params} | ||
| if "goal_id" in task_data: | ||
| kwarg_dict["goal_id"] = task_data["goal_id"] | ||
|
|
||
| return cls(**kwarg_dict) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,83 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, Response | ||
| from ..db import db | ||
| from app.models.goal import Goal | ||
| from app.models.task import Task | ||
| from .route_utilities import validate_model, create_model, get_models_with_filters, delete_model | ||
|
|
||
| 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. 👍🏿 |
||
|
|
||
| @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. @stephaugi, I would suggest changing this name to something like |
||
| request_body = request.get_json() | ||
| task_ids = request_body["task_ids"] | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| if task_ids: | ||
| remove_all_task_ids(goal) | ||
| add_task_ids(goal, task_ids) | ||
|
Comment on lines
+22
to
+23
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 work, you encapsulated the logic into their own functions. However, I think that since they are tied to specific model for a specific case (the one described in this route) that might be better served living in this route function. However, I think if you were to generalize them to where they could be used on any models with a one to many relationship they would warrant their own functions. @stephaugi |
||
| db.session.commit() | ||
|
|
||
| response_body = {"id" : goal.id, | ||
| "task_ids" : task_ids} | ||
| return response_body, 200 | ||
| else: | ||
| return create_model(Task, request_body) | ||
|
Comment on lines
+29
to
+30
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 line I would remove because of the description of the route, it should only be associating existing tasks with a goal, not creating them. |
||
|
|
||
| @bp.get("") | ||
| def get_all_goals(): | ||
| params = request.args | ||
|
|
||
| return get_models_with_filters(Goal, params) | ||
|
|
||
| @bp.get("<goal_id>") | ||
| def get_one_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| return goal.to_dict() | ||
|
Comment on lines
+32
to
+42
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. 👍🏿 |
||
|
|
||
| @bp.get("<goal_id>/tasks") | ||
| def get_one_goal_with_tasks(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| response = goal.to_dict() | ||
| response["tasks"] = [] | ||
|
|
||
| if goal.tasks: | ||
| response["tasks"] = [task.to_dict() for task in goal.tasks] | ||
|
Comment on lines
+51
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. With the |
||
|
|
||
| return response, 200 | ||
|
|
||
| @bp.put("<goal_id>") | ||
| def update_goal(goal_id): | ||
| request_body = request.get_json() | ||
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| goal.title = request_body["title"] | ||
|
|
||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
|
||
| @bp.delete("<goal_id>") | ||
| def delete_goal(goal_id): | ||
|
|
||
| return delete_model(Goal, goal_id) | ||
|
|
||
| def remove_all_task_ids(goal): | ||
| for task in goal.tasks: | ||
| task.goal_id = None | ||
|
|
||
| db.session.flush() | ||
|
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 you aren't rolling back any of your changes/accessing the changes before committing them, I don't think you need this line since its not being leveraged. |
||
|
|
||
| def add_task_ids(goal, task_ids): | ||
| for task_id in task_ids: | ||
| task = validate_model(Task, task_id) | ||
| task.goal_id = goal.id | ||
|
|
||
| db.session.flush() | ||
|
Comment on lines
+72
to
+83
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 would move these functions into the |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| from flask import abort, make_response, Response | ||
| from ..db import db | ||
| import requests | ||
| import os | ||
|
|
||
|
|
||
| def validate_model(cls, id): | ||
| try: | ||
| int(id) | ||
| except: | ||
| response = {"message": f"{cls.__name__} {id} invalid"} | ||
| abort(make_response(response, 400)) | ||
| query = db.select(cls).where(cls.id == id) | ||
| model = db.session.scalar(query) | ||
|
|
||
| if not model: | ||
| response = {"message": f"{cls.__name__} {id} not found"} | ||
| abort(make_response(response, 404)) | ||
|
|
||
| return model | ||
|
Comment on lines
+7
to
+20
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. ✅ |
||
|
|
||
| def create_model(cls, model_data): | ||
| try: | ||
| new_model = cls.from_dict(model_data) | ||
| except KeyError: | ||
| response_body = {"details": "Invalid data"} | ||
| abort(make_response(response_body, 400)) | ||
|
|
||
| db.session.add(new_model) | ||
| db.session.commit() | ||
|
|
||
| return new_model.to_dict(), 201 | ||
|
Comment on lines
+22
to
+32
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. ✅ |
||
|
|
||
| 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}%")) | ||
|
|
||
| sort_by = filters.get("sort") | ||
| if sort_by == 'asc': | ||
| query = query.order_by(cls.title.asc()) | ||
| elif sort_by == 'desc': | ||
| query = query.order_by(cls.title.desc()) | ||
| else: | ||
| query = query.order_by(cls.id) | ||
|
|
||
| models = db.session.scalars(query.order_by(cls.id)) | ||
|
|
||
| return [model.to_dict() for model in models] | ||
|
Comment on lines
+34
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. ✅ |
||
|
|
||
| def delete_model(cls, id): | ||
| model = validate_model(cls, id) | ||
| db.session.delete(model) | ||
| db.session.commit() | ||
|
|
||
| return Response(status=204, mimetype="application/json") | ||
|
Comment on lines
+54
to
+59
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. ✅ |
||
|
|
||
| def send_slack_complete(cls, model): | ||
| json = { | ||
| "channel" : os.environ.get("SLACK_CHANNEL"), | ||
| "text" : f"Someone just completed the {cls.__name__.lower()} {model.title}" | ||
| } | ||
|
|
||
| headers = { | ||
| "Authorization":f"Bearer {os.environ.get("SLACK_BOT_TOKEN")}" | ||
| } | ||
|
|
||
| requests.post("https://slack.com/api/chat.postMessage", json=json, headers=headers) | ||
|
Comment on lines
+61
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. Nice work encapsulating this logic!!! |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1 +1,66 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| from flask import Blueprint | ||||||||||||||||||||||||||||||||||||||||||||
| from flask import Blueprint, request, Response | ||||||||||||||||||||||||||||||||||||||||||||
| from app.models.task import Task | ||||||||||||||||||||||||||||||||||||||||||||
| from .route_utilities import validate_model, create_model, get_models_with_filters, send_slack_complete, delete_model | ||||||||||||||||||||||||||||||||||||||||||||
| from ..db import db | ||||||||||||||||||||||||||||||||||||||||||||
| from datetime import datetime | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| 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(): | ||||||||||||||||||||||||||||||||||||||||||||
| params = request.args | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return get_models_with_filters(Task, params) | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+9
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. Notice how your spacing is inconsistent here. You want to be mindful of your formatting to make sure you aren't introducing any inconsistencies when making PRs.
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @bp.get("<task_id>") | ||||||||||||||||||||||||||||||||||||||||||||
| def get_one_task(task_id): | ||||||||||||||||||||||||||||||||||||||||||||
| task = validate_model(Task, task_id) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return task.to_dict() | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @bp.delete("<task_id>") | ||||||||||||||||||||||||||||||||||||||||||||
| def delete_task(task_id): | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return delete_model(Task, task_id) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @bp.put("<task_id>") | ||||||||||||||||||||||||||||||||||||||||||||
| def update_task(task_id): | ||||||||||||||||||||||||||||||||||||||||||||
| request_body = request.get_json() | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| task = validate_model(Task, task_id) | ||||||||||||||||||||||||||||||||||||||||||||
| valid_attrs = ["title", "description"] | ||||||||||||||||||||||||||||||||||||||||||||
| for attr in valid_attrs: | ||||||||||||||||||||||||||||||||||||||||||||
| if attr in request_body: | ||||||||||||||||||||||||||||||||||||||||||||
| setattr(task, attr, request_body[attr]) | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+37
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. Great work, @stephaugi! You could have written a helper function to update our def update_from_dict(obj, data):
for attr, value in data.items():
if hasattr(obj, attr):
setattr(obj, attr, value)
db.session.commit()
return Response(status=204, mimetype="application/json")Then you could move this in your utilities file and have your code more D.R.Y. |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| db.session.commit() | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return Response(status=204, mimetype="application/json") | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @bp.patch("<task_id>/mark_complete") | ||||||||||||||||||||||||||||||||||||||||||||
| def mark_complete(task_id): | ||||||||||||||||||||||||||||||||||||||||||||
| task = validate_model(Task, task_id) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| date = datetime.now() | ||||||||||||||||||||||||||||||||||||||||||||
| task.completed_at = date | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| db.session.commit() | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| send_slack_complete(Task, 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. 🫡 |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return Response(status=204, mimetype="application/json") | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @bp.patch("<task_id>/mark_incomplete") | ||||||||||||||||||||||||||||||||||||||||||||
| def mark_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.
Nice! You are following Flask convention to by naming your Blueprints
bpand then usingasto import them under an alias.