-
Notifications
You must be signed in to change notification settings - Fork 43
Task List Api #28
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?
Task List Api #28
Changes from all commits
34946b9
f481c65
91ca025
5f881db
4375e2b
829f594
95722d8
4824099
f70fdb3
d2fdfaf
066cf5c
2fee59f
ff35734
c27491f
3446d80
e475429
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,22 @@ | ||
| 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 | ||
| } | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| return cls( | ||
| title=goal_data["title"] | ||
| ) |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,37 @@ | ||||||
| from sqlalchemy.orm import Mapped, mapped_column | ||||||
| from sqlalchemy.orm import Mapped, mapped_column, relationship | ||||||
| from ..db import db | ||||||
| from sqlalchemy import ForeignKey | ||||||
| from datetime import datetime | ||||||
| from typing import TYPE_CHECKING, Optional | ||||||
| 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[Optional[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. Using
Suggested change
|
||||||
| goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||||||
| goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||||||
|
|
||||||
| def to_dict(self): | ||||||
| task = { | ||||||
| "id":self.id, | ||||||
| "title":self.title, | ||||||
| "description":self.description, | ||||||
| "is_complete": self.is_complete() | ||||||
|
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
Instead of using a helper function, we could just use a ternary operator here. |
||||||
| } | ||||||
| if self.goal_id: | ||||||
| task["goal_id"] = self.goal_id | ||||||
| return task | ||||||
|
|
||||||
| def is_complete(self): | ||||||
| return self.completed_at is not None | ||||||
|
Comment on lines
+28
to
+29
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 logic is simple enough that you could get away with not using a helper function and using a ternary instead on line 22 |
||||||
|
|
||||||
| @classmethod | ||||||
| def from_dict(cls, task_data): | ||||||
| return cls( | ||||||
| title=task_data["title"], | ||||||
| description=task_data["description"], | ||||||
| completed_at=task_data.get("completed_at") | ||||||
| ) | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1 +1,75 @@ | ||||||||||||
| from flask import Blueprint | ||||||||||||
| from flask import Blueprint, request, Response, abort, make_response | ||||||||||||
| import requests | ||||||||||||
| import os | ||||||||||||
| from .route_utilities import validate_model, create_model | ||||||||||||
| from ..models.goal import Goal | ||||||||||||
| from ..models.task import Task | ||||||||||||
| from ..db import db | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| bp=Blueprint("goal_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. Nitpick: spacing is off
Suggested change
|
||||||||||||
|
|
||||||||||||
| @bp.get("") | ||||||||||||
| def get_goals(): | ||||||||||||
| query=db.select(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. Nitpick: spacing
Suggested change
|
||||||||||||
| goals = db.session.scalars(query.order_by(Goal.id)) | ||||||||||||
| goals_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. Nitpick: spacing
Suggested change
|
||||||||||||
| for goal in goals: | ||||||||||||
| goals_response.append(goal.to_dict()) | ||||||||||||
| return goals_response | ||||||||||||
|
Comment on lines
+16
to
+19
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 would be a good place for list comprehension to make the route more concise.
Suggested change
|
||||||||||||
|
|
||||||||||||
| @bp.get("/<id>") | ||||||||||||
| def get_one_goals(id): | ||||||||||||
| goal = validate_model(Goal, id) | ||||||||||||
| return {"goal": goal.to_dict()},200 | ||||||||||||
|
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. Nitpick: missing space after comma The default status code Flask returns to the client is 200. Therefore we do not need to return it ourselves explicitly. Note that you don't explicitly return 200 for the I'd prefer not to explicitly return a status code if I don't need to because it adds more code and feels redundant because the framework does it for us. If you choose to return 200 or choose not to, just be consistent in the choice and apply the decision throughout the entire codebase, instead of sometimes returning it and sometimes not.
Suggested change
|
||||||||||||
|
|
||||||||||||
| @bp.get("/<goal_id>/tasks") | ||||||||||||
| def get_one_goal_tasks(goal_id): | ||||||||||||
| goal = validate_model(Goal,goal_id) | ||||||||||||
| response_body = goal.to_dict() | ||||||||||||
|
|
||||||||||||
| if goal.tasks: | ||||||||||||
| tasks = [task.to_dict() for task in goal.tasks] | ||||||||||||
| response_body["tasks"] = tasks | ||||||||||||
| else: | ||||||||||||
| response_body["tasks"] = [] | ||||||||||||
|
Comment on lines
+34
to
+35
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 a goal doesn't have tasks then the attribute
Suggested change
|
||||||||||||
|
|
||||||||||||
| return response_body | ||||||||||||
|
|
||||||||||||
| @bp.post("") | ||||||||||||
| def create_goal(): | ||||||||||||
| request_body = request.get_json() | ||||||||||||
| response = create_model(Goal, request_body) | ||||||||||||
| return {"goal": response[0]}, response[1] | ||||||||||||
|
|
||||||||||||
| @bp.post("/<goal_id>/tasks") | ||||||||||||
| def create_tasks_with_goal_id(goal_id): | ||||||||||||
|
|
||||||||||||
| goal = validate_model(Goal,goal_id) | ||||||||||||
| request_body = request.get_json() | ||||||||||||
| tasks = request_body.get("task_ids",[]) | ||||||||||||
|
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. Naming is a little unclear. We're actually getting a list of task ids, not a list of task instances.
Suggested change
|
||||||||||||
|
|
||||||||||||
| if goal.tasks: | ||||||||||||
| goal.tasks.clear() | ||||||||||||
|
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. Do we need to clear
Suggested change
|
||||||||||||
|
|
||||||||||||
| goal.tasks = [validate_model(Task, task) for task in 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. Naming is a little unclear.
Suggested change
|
||||||||||||
|
|
||||||||||||
| db.session.commit() | ||||||||||||
|
|
||||||||||||
| return make_response({"id":goal.id, "task_ids":tasks},200) | ||||||||||||
|
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. Nitpick: missing white spaces 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 shouldn't need to use the 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
|
||||||||||||
|
|
||||||||||||
| @bp.put("/<id>") | ||||||||||||
| def update_goal(id): | ||||||||||||
| goal = validate_model(Goal, id) | ||||||||||||
| request_body = request.get_json() | ||||||||||||
| goal.title=request_body["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. Nitpick: missing whitespace
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. This line of code will throw an unhandled exception if a client sends a bad request body that doesn't have a key "title". How could you update this route so that you could send back an error response with some details and an appropriate status code so that the client knows what they need to fix when they resend the request? |
||||||||||||
| db.session.commit() | ||||||||||||
| return Response(status=204, mimetype="application/json") | ||||||||||||
|
|
||||||||||||
| @bp.delete("/<id>") | ||||||||||||
| def delete_goal(id): | ||||||||||||
| goal = validate_model(Goal, id) | ||||||||||||
| db.session.delete(goal) | ||||||||||||
| db.session.commit() | ||||||||||||
| return Response(status=204, mimetype="application/json") | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| from flask import abort, make_response | ||
| from ..db import db | ||
|
|
||
| def validate_model(cls, model_id): | ||
| try: | ||
| model_id = int(model_id) | ||
| except ValueError: | ||
| invalid_response = {"message": f"{cls.__name__} id ({model_id}) is invalid."} | ||
| abort(make_response(invalid_response, 400)) | ||
|
|
||
| query = db.select(cls).where(cls.id == model_id) | ||
| model = db.session.scalar(query) | ||
|
|
||
| if not model: | ||
| not_found = {"message": f"{cls.__name__} with id ({model_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 hasattr(cls, attribute): | ||
| query = query.where(getattr(cls, attribute).ilike(f"%{value}%")) | ||
|
|
||
| models = db.session.scalars(query.order_by(cls.id)) | ||
| models_response = [model.to_dict() for model in models] | ||
|
|
||
| return models_response, 200 |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1 +1,98 @@ | ||||||
| from flask import Blueprint | ||||||
| from flask import Blueprint, request, Response, abort, make_response | ||||||
| import requests | ||||||
| import os | ||||||
| from .route_utilities import validate_model, create_model | ||||||
| from datetime import datetime | ||||||
| from ..models.task import Task | ||||||
| from ..db import db | ||||||
|
|
||||||
| bp=Blueprint("tasks_bp", __name__, url_prefix="/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. Nitpick: spacing is off. The assignment operator should have spaces on either side. Here's the guidance the PEP8 style guide provides: https://peps.python.org/pep-0008/#whitespace-in-expressions-and-statements
Suggested change
|
||||||
|
|
||||||
| @bp.post("") | ||||||
| def create_task(): | ||||||
| request_body = request.get_json() | ||||||
| response = create_model(Task, request_body) | ||||||
| return {"task": response[0]}, response[1] | ||||||
|
|
||||||
| @bp.get("") | ||||||
| def get_tasks(): | ||||||
| query = db.select(Task) | ||||||
|
|
||||||
| sort_param = request.args.get("sort") | ||||||
|
|
||||||
| if sort_param == "asc": | ||||||
| tasks = db.session.scalars(query.order_by(Task.title.asc())) | ||||||
| elif sort_param == "desc": | ||||||
| tasks = db.session.scalars(query.order_by(Task.title.desc())) | ||||||
| else: | ||||||
| tasks = db.session.scalars(query.order_by(Task.id)) | ||||||
|
|
||||||
| tasks_response = [] | ||||||
| for task in tasks: | ||||||
| valid_task = task.to_dict() | ||||||
| if not has_special_chars(valid_task["title"]): | ||||||
| tasks_response.append(valid_task) | ||||||
|
Comment on lines
+30
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. Would be nice to use list comprehension here. |
||||||
| return tasks_response | ||||||
|
|
||||||
| def has_special_chars(title): | ||||||
| special_chars = "#$%()*+,-./:;<=>?@[\]^_`{|}~" | ||||||
| for char in title: | ||||||
| if char in special_chars: | ||||||
| raise ValueError | ||||||
| return False | ||||||
|
Comment on lines
+37
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. Why do we need this logic? How would a request with a request body that has key "title" save a record to the DB in the first place? Your post request uses return cls(
title=task_data["title"],
description=task_data["description"],
completed_at=task_data.get("completed_at")
)If we can't ever create and save a record with special characters, I think we could skip this logic. |
||||||
|
|
||||||
| @bp.get("/<id>") | ||||||
| def get_one_task(id): | ||||||
| task = validate_model(Task, id) | ||||||
| return {"task": task.to_dict()},200 | ||||||
|
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
|
||||||
|
|
||||||
| @bp.put("/<id>") | ||||||
| def update_task(id): | ||||||
| task = validate_model(Task, id) | ||||||
| request_body = request.get_json() | ||||||
|
|
||||||
| task.title=request_body["title"] | ||||||
| task.description=request_body["description"] | ||||||
|
Comment on lines
+54
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. These two lines will throw unhandled exceptions if the request body does not have keys exactly as "title" or "description". How could you update the logic in this route so that, instead of throwing a server error, you could return a nice error response with an appropriate status code? |
||||||
|
|
||||||
| db.session.commit() | ||||||
| return Response(status=204, mimetype="application/json") | ||||||
|
|
||||||
| @bp.patch("/<id>/mark_complete") | ||||||
| def mark_task_complete(id): | ||||||
| task = validate_model(Task, id) | ||||||
|
|
||||||
| data = { | ||||||
| "token": f"{os.environ.get('SLACK_API_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. We need to pass in the authorization token as a header, not as a key value pair in the json we send with the request. You do this on line 71 so we should remove the unnecessary code here.
Suggested change
|
||||||
| "channel":"test-slack-api", | ||||||
| "text":"Someone just completed the task My Beautiful Task" | ||||||
| } | ||||||
| response = requests.post("https://slack.com/api/chat.postMessage", data=data, | ||||||
|
Comment on lines
+66
to
+69
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 not to have string literal hanging out in a function. We should use a constant variable to reference the URL and channel name. We call a string literal that appears in code magic strings: "Magic Strings are literal string values that are used directly in code without a clear explanation...This makes it difficult to maintain and extend the code in the future." Read more about why we should avoid magic strings here SLACK_CHANNEL = "test-slack-api"
SLACK_URL = "https://slack.com/api/chat.postMessage" |
||||||
| headers={ | ||||||
| "Authorization": f"Bearer {os.environ.get('SLACK_API_TOKEN')}" | ||||||
| }) | ||||||
|
Comment on lines
+64
to
+72
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 all this logic to live in a helper function, maybe like |
||||||
|
|
||||||
| if not task.completed_at: | ||||||
| task.completed_at = datetime.now() | ||||||
|
|
||||||
| db.session.commit() | ||||||
| return Response(status=204, mimetype="application/json") | ||||||
|
|
||||||
| @bp.patch("/<id>/mark_incomplete") | ||||||
| def mark_task_incomplete(id): | ||||||
| task = validate_model(Task, id) | ||||||
|
|
||||||
| if task.completed_at: | ||||||
| task.completed_at = None | ||||||
|
|
||||||
| db.session.commit() | ||||||
| return Response(status=204, mimetype="application/json") | ||||||
|
|
||||||
| @bp.delete("/<id>") | ||||||
| def delete_task(id): | ||||||
| task = validate_model(Task, id) | ||||||
| db.session.delete(task) | ||||||
| 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.
👍