-
Notifications
You must be signed in to change notification settings - Fork 43
Pull Request from Yasiel #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
1c6dd23
ac78cf9
dae65a7
07bdbe3
c9ee41f
588a331
a19a178
60b54be
7782e71
8465843
660a666
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,22 +1,35 @@ | ||
| from flask import Flask | ||
| from flask import Flask, request | ||
| from .db import db, migrate | ||
| from .routes.task_routes import bp as tasks_bp | ||
| from .routes.goal_routes import bp as goals_bp | ||
| from .models import task, goal | ||
| import os | ||
|
|
||
| def create_app(config=None): | ||
| app = Flask(__name__) | ||
|
|
||
| app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
| app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI') | ||
|
|
||
| if config: | ||
| # Merge `config` into the app's configuration | ||
| # to override the app's default settings for testing | ||
| app.config.update(config) | ||
|
|
||
| db.init_app(app) | ||
| migrate.init_app(app, db) | ||
|
|
||
| # Register Blueprints here | ||
|
|
||
| return app | ||
|
|
||
| app.register_blueprint(tasks_bp) | ||
| app.register_blueprint(goals_bp) | ||
|
|
||
| @app.errorhandler(404) | ||
|
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. Neat research into error handling to better control the user error messages! |
||
| def handle_not_found(e): | ||
| if request.path.startswith('/goals'): | ||
| return {"error": "Goal not found"}, 404 | ||
| elif request.path.startswith('/tasks'): | ||
| return {"error": "Task not found"}, 404 | ||
| else: | ||
| return {"error": "Not found"}, 404 | ||
|
|
||
| @app.errorhandler(400) | ||
| def handle_bad_request(e): | ||
| return {"details": "Invalid data"}, 400 | ||
|
|
||
| return app | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,31 @@ | ||
| from sqlalchemy.orm import Mapped, mapped_column | ||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
| from sqlalchemy import String | ||
|
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'm not seeing where this import is used. If an import is not required, we should remove it so that our files are not loading up extra code into our execution environment that will never be used. This makes our programs larger and run slower without any benefit. This feedback applies to the
Author
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. Removed. 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 it was removed, the change to this line was not pushed up to the repo. |
||
| from ..db import db | ||
|
|
||
| class Goal(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(String(100)) | ||
|
|
||
| tasks = relationship("Task", back_populates="goal", cascade="all, delete-orphan") | ||
|
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 cascade for deleting child objects. From a design standpoint, is there ever a situation where you might want to delete a goal but keep the tasks? We could choose to disconnect the goal from the tasks and then delete the goal if we didn't necessarily want to delete all related tasks.
Author
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. Ah, yeah. you could do something like tasks = relationship("Tasl", back_populates="goal",
Author
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. *Task |
||
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| if not goal_data.get("title"): | ||
|
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 is not called from anywhere, the If we wanted to use them together, there seems to be a little disconnect around what the Taking a look at the create goal route: @goals_bp.route("", methods=["POST"])
def create_goal():
request_body = request.get_json()
if not request_body.get("title"):
return make_response(jsonify({"details": "Invalid data"}), 400)
new_goal = Goal(title=request_body.get("title"))
db.session.add(new_goal)
db.session.commit()
|
||
| return None | ||
|
Comment on lines
+13
to
+14
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 can handle bad information a number of ways, but this is a place where it would be totally valid and normal to let the exception raise if someone provides bad data to the request, and use a try/except in the route to catch the issue and respond accordingly. This feedback applies to |
||
|
|
||
| return cls( | ||
| title=goal_data.get("title") | ||
| ) | ||
|
|
||
| def to_dict(self): | ||
| return { | ||
| "id": self.id, | ||
| "title": self.title | ||
| } | ||
|
|
||
| def to_dict_with_tasks(self): | ||
| goal_dict = self.to_dict() | ||
|
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 |
||
| goal_dict["tasks"] = [task.to_dict(include_goal_id=True) for task in self.tasks] | ||
| return goal_dict | ||
|
|
||
| #finished, i think | ||
| 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 String, Text, ForeignKey | ||
| from datetime import datetime | ||
| from typing import Optional | ||
| from ..db import db | ||
|
|
||
| class Task(db.Model): | ||
| id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
| title: Mapped[str] = mapped_column(String(100)) | ||
| description: Mapped[str] = mapped_column(Text) | ||
| completed_at: Mapped[Optional[datetime]] | ||
|
|
||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
| goal = relationship("Goal", back_populates="tasks") | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, task_data): | ||
| if not task_data.get("title") or not task_data.get("description"): | ||
| return None | ||
|
|
||
| return cls( | ||
| title=task_data.get("title"), | ||
| description=task_data.get("description"), | ||
| completed_at=task_data.get("completed_at") | ||
| ) | ||
|
|
||
| def to_dict(self, include_goal_id=False): | ||
| task_dict = { | ||
| "id": self.id, | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at is not None | ||
| } | ||
|
|
||
| if include_goal_id and self.goal_id is not None: | ||
| task_dict["goal_id"] = self.goal_id | ||
|
|
||
| return task_dict |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,105 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, make_response, abort | ||
| from ..db import db | ||
| from ..models.goal import Goal | ||
| from ..models.task import Task | ||
|
|
||
| bp = Blueprint("goals", __name__, url_prefix="/goals") | ||
|
|
||
| def get_goal_by_id(id): | ||
| query = db.select(Goal).where(Goal.id == id) | ||
| return db.session.scalar(query) | ||
|
|
||
| def get_task_by_id(id): | ||
| query = db.select(Task).where(Task.id == id) | ||
| return db.session.scalar(query) | ||
|
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 updates to return the JSON and let the route handle adding the default 200 status code. |
||
|
|
||
| @bp.route("", methods=["POST"]) | ||
| def create_goal(): | ||
| request_body = request.get_json() | ||
|
|
||
| new_goal = Goal.from_dict(request_body) | ||
|
|
||
| if not new_goal: | ||
| abort(400) | ||
|
|
||
| db.session.add(new_goal) | ||
| db.session.commit() | ||
|
|
||
| return {"goal": new_goal.to_dict()}, 201 | ||
|
|
||
| @bp.route("", methods=["GET"]) | ||
| def get_all_goals(): | ||
| goals = db.session.scalars(db.select(Goal)).all() | ||
|
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 consider breaking this into 2 lines since there is a lot going on here. |
||
| return [goal.to_dict() for goal in goals] | ||
|
|
||
| @bp.route("/<goal_id>", methods=["GET"]) | ||
| def get_goal(goal_id): | ||
| goal = get_goal_by_id(goal_id) | ||
|
|
||
| if not goal: | ||
| abort(404) | ||
|
|
||
| return {"goal": goal.to_dict()} | ||
|
|
||
| @bp.route("/<goal_id>", methods=["PUT"]) | ||
| def update_goal(goal_id): | ||
| goal = get_goal_by_id(goal_id) | ||
|
|
||
| if not goal: | ||
| abort(404) | ||
|
|
||
| request_body = request.get_json() | ||
|
|
||
| goal.title = request_body.get("title", goal.title) | ||
|
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. To keep stronger separation of concerns, just like we have a |
||
|
|
||
| db.session.commit() | ||
|
|
||
| response = make_response("", 204) | ||
| response.mimetype = "application/json" | ||
| return response | ||
|
|
||
| @bp.route("/<goal_id>", methods=["DELETE"]) | ||
| def delete_goal(goal_id): | ||
| goal = get_goal_by_id(goal_id) | ||
|
|
||
| if not goal: | ||
| abort(404) | ||
|
|
||
| db.session.delete(goal) | ||
| db.session.commit() | ||
|
|
||
| response = make_response("", 204) | ||
| response.mimetype = "application/json" | ||
| return response | ||
|
|
||
| @bp.route("/<goal_id>/tasks", methods=["GET"]) | ||
| def get_goal_tasks(goal_id): | ||
| goal = get_goal_by_id(goal_id) | ||
|
|
||
| if not goal: | ||
| abort(404) | ||
|
|
||
| return goal.to_dict_with_tasks() | ||
|
|
||
| @bp.route("/<goal_id>/tasks", methods=["POST"]) | ||
| def associate_tasks_with_goal(goal_id): | ||
| goal = get_goal_by_id(goal_id) | ||
|
|
||
| if not goal: | ||
| abort(404) | ||
|
|
||
| request_body = request.get_json() | ||
| task_ids = request_body.get("task_ids", []) | ||
|
|
||
| valid_tasks = [task for task in | ||
| (get_task_by_id(task_id) for task_id in task_ids) | ||
| if task is not None] | ||
|
|
||
| goal.tasks = valid_tasks | ||
|
|
||
| db.session.commit() | ||
|
|
||
| return { | ||
| "id": goal.id, | ||
| "task_ids": [task.id for task in goal.tasks] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,111 @@ | ||
| from flask import Blueprint | ||
| from flask import Blueprint, request, make_response, abort | ||
| from ..db import db | ||
| from ..models.task import Task | ||
| from datetime import datetime | ||
| from ..services.slack_service import send_slack_notification | ||
|
|
||
| bp = Blueprint("tasks", __name__, url_prefix="/tasks") | ||
|
|
||
| def get_task_by_id(id): | ||
| query = db.select(Task).where(Task.id == id) | ||
| return db.session.scalar(query) | ||
|
|
||
| @bp.route("", methods=["POST"]) | ||
| def create_task(): | ||
| request_body = request.get_json() | ||
|
|
||
| new_task = Task.from_dict(request_body) | ||
|
|
||
| if not new_task: | ||
| abort(400) | ||
|
|
||
| db.session.add(new_task) | ||
| db.session.commit() | ||
|
|
||
| return {"task": new_task.to_dict()}, 201 | ||
|
|
||
| @bp.route("", methods=["GET"]) | ||
| def get_all_tasks(): | ||
| sort_param = request.args.get("sort") | ||
|
|
||
| query = db.select(Task) | ||
|
|
||
| if sort_param == "asc": | ||
| query = query.order_by(Task.title.asc()) | ||
| elif sort_param == "desc": | ||
| query = query.order_by(Task.title.desc()) | ||
|
|
||
| tasks = db.session.scalars(query).all() | ||
|
|
||
| return [task.to_dict() for task in tasks] | ||
|
|
||
| @bp.route("/<task_id>", methods=["GET"]) | ||
| def get_task(task_id): | ||
| task = get_task_by_id(task_id) | ||
|
|
||
| if not task: | ||
| abort(404) | ||
|
|
||
| return {"task": task.to_dict(include_goal_id=True)} | ||
|
|
||
| @bp.route("/<task_id>", methods=["PUT"]) | ||
| def update_task(task_id): | ||
| task = get_task_by_id(task_id) | ||
|
|
||
| if not task: | ||
| abort(404) | ||
|
|
||
| request_body = request.get_json() | ||
|
|
||
| task.title = request_body.get("title", task.title) | ||
| task.description = request_body.get("description", task.description) | ||
|
|
||
| db.session.commit() | ||
|
|
||
| response = make_response("", 204) | ||
| response.mimetype = "application/json" | ||
| return response | ||
|
|
||
| @bp.route("/<task_id>", methods=["DELETE"]) | ||
| def delete_task(task_id): | ||
| task = get_task_by_id(task_id) | ||
|
|
||
| if not task: | ||
| abort(404) | ||
|
|
||
| db.session.delete(task) | ||
| db.session.commit() | ||
|
|
||
| response = make_response("", 204) | ||
| response.mimetype = "application/json" | ||
| return response | ||
|
|
||
| @bp.route("/<task_id>/mark_complete", methods=["PATCH"]) | ||
| def mark_task_complete(task_id): | ||
| task = get_task_by_id(task_id) | ||
|
|
||
| if not task: | ||
| abort(404) | ||
|
|
||
| task.completed_at = datetime.now() | ||
| db.session.commit() | ||
|
|
||
| send_slack_notification(f"Someone just completed the task {task.title}") | ||
|
|
||
| response = make_response("", 204) | ||
| response.mimetype = "application/json" | ||
| return response | ||
|
|
||
| @bp.route("/<task_id>/mark_incomplete", methods=["PATCH"]) | ||
| def mark_task_incomplete(task_id): | ||
| task = get_task_by_id(task_id) | ||
|
|
||
| if not task: | ||
| abort(404) | ||
|
|
||
| task.completed_at = None | ||
| db.session.commit() | ||
|
|
||
| response = make_response("", 204) | ||
| response.mimetype = "application/json" | ||
| return response |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| import os | ||
| import requests | ||
| from flask import current_app | ||
|
|
||
| def send_slack_notification(message): | ||
| """ | ||
| Send a notification message to Slack | ||
| Returns True if successful, False otherwise | ||
| """ | ||
| slack_token = os.environ.get('SLACK_BOT_TOKEN') | ||
|
|
||
| if not slack_token: | ||
| return False | ||
|
|
||
| slack_data = { | ||
| 'channel': 'task-notifications', | ||
| 'text': message | ||
| } | ||
|
|
||
| headers = { | ||
| 'Authorization': f'Bearer {slack_token}', | ||
| 'Content-Type': 'application/json' | ||
| } | ||
|
|
||
| try: | ||
| slack_response = requests.post( | ||
| 'https://slack.com/api/chat.postMessage', | ||
| json=slack_data, | ||
| headers=headers | ||
| ) | ||
|
|
||
| if current_app and current_app.debug: | ||
| current_app.logger.debug(f"Slack API Response: {slack_response.status_code}") | ||
|
|
||
| return slack_response.status_code == 200 | ||
|
|
||
| except Exception as e: | ||
| if current_app: | ||
| current_app.logger.error(f"Error sending Slack notification: {str(e)}") | ||
| return False |
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.
Even though we don't use them in the function, we still want to import the models into
__init__.pyto make the migration system aware of the model classes. If we were to make changes to the model columns now and then try to migrate, nothing would happen because the Flask application cannot see the models.