-
Notifications
You must be signed in to change notification settings - Fork 44
ns-task-list-api #39
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?
ns-task-list-api #39
Changes from all commits
79ab3f0
d44d786
670fd84
3c963af
83f35f1
fb2c005
2618ef1
ef0d1c8
489a064
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,7 @@ | ||
from flask import Flask | ||
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 | ||
|
||
|
@@ -10,13 +12,13 @@ def create_app(config=None): | |
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 | ||
app.register_blueprint(tasks_bp) | ||
app.register_blueprint(goals_bp) | ||
|
||
return app | ||
|
||
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 rest of your configuration and blueprint registration looks good! |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,17 @@ | ||
from sqlalchemy.orm import Mapped, mapped_column | ||
from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
from ..db import db | ||
# from app.models.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("Task", back_populates="goal") | ||
Comment on lines
6
to
+8
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 model looks good! Nice one-to-many mapping! |
||
def to_dict(self): | ||
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. Small nitpick here, but make sure you add some space before your functions! |
||
return { | ||
"id": self.id, | ||
"title": self.title | ||
} | ||
@classmethod | ||
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 thing here, some whitespace can drastically increase readability! |
||
def from_dict(cls, goal_data): | ||
new_goal = cls(title=goal_data["title"]) | ||
return new_goal |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,39 @@ | ||
from sqlalchemy.orm import Mapped, mapped_column | ||
from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
from ..db import db | ||
from datetime import datetime | ||
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] = mapped_column(nullable=False) | ||
description: Mapped[Optional[str]] = mapped_column(nullable=True) | ||
completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True) | ||
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
goal: Mapped[Optional["Goal"]] = relationship("Goal", back_populates="tasks") | ||
Comment on lines
8
to
+13
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 goal model! |
||
|
||
|
||
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. Another small nitpick here, but try and keep whitespace between different sections of your code to just one line! |
||
def to_dict(self, include_goal=False, include_goal_id=False): | ||
task_dict = dict( | ||
id=self.id, | ||
title=self.title, | ||
description=self.description, | ||
is_complete=self.completed_at is not 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. Love this notation here! |
||
) | ||
Comment on lines
+17
to
+22
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 include_goal_id: | ||
task_dict["goal_id"] = self.goal_id | ||
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. I know you want to check and see if the goal id exists, but we already have a way to do that which doesn't include an extra parameter! We can always just check to see if |
||
|
||
if include_goal and self.goal: | ||
task_dict['goal'] = self.goal.title | ||
Comment on lines
+26
to
+27
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 understand the urge to include the goal in some cases, but it's important to note that this to_dict is most likely going to be used for situations where we are converting a model into a dictionary. There won't necessarily be a clean way to include the goal in that calculation, so it's a good idea to just omit it completely here! |
||
|
||
|
||
return task_dict | ||
|
||
@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) | ||
) | ||
Comment on lines
+32
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. This from_dict looks good! Small tweak: the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,160 @@ | ||
from flask import Blueprint | ||
from flask import Blueprint, request, abort, make_response | ||
from app.models.goal import Goal | ||
from ..db import db | ||
from .route_utilities import validate_model, get_model_with_filters, create_model | ||
|
||
|
||
|
||
bp = Blueprint("goals_bp", __name__, url_prefix="/goals") | ||
|
||
@bp.post("") | ||
def create_goal(): | ||
request_body = request.get_json() | ||
if "title" not in request_body: | ||
abort(make_response({"details": "Invalid data"}, 400)) | ||
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. This works well here, especially because "title" is the only attribute in the goal model besides id which will be generated automatically. That being said, we created the from_dict within our models! Don't be afraid to use that within a try/except block! You could try to create a new Goal using the from_dict method. If it works, run your add and commit as usual. If not, throw an error response like the one you created on line 14! |
||
|
||
new_goal = Goal.from_dict(request_body) | ||
db.session.add(new_goal) | ||
db.session.commit() | ||
|
||
response = { | ||
"goal": new_goal.to_dict() | ||
} | ||
return response, 201 | ||
|
||
@bp.post("/<goal_id>/tasks") | ||
def create_task_with_goal_id(goal_id): | ||
from app.models.goal import Goal | ||
from app.models.task import Task | ||
Comment on lines
+27
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. If these imports are at the top of the file, we don't need to add them to each function! |
||
|
||
goal = validate_model(Goal, goal_id) | ||
if not goal: | ||
response = {"message": f"Goal with ID {goal_id} not found"} | ||
abort(make_response(response, 404)) | ||
Comment on lines
+31
to
+33
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 your validate model hasn't returned a goal, it will likely return some form of an error, so this error checking here is unnecessary! |
||
|
||
request_body = request.get_json() | ||
task_ids = request_body.get("task_ids") | ||
|
||
if not task_ids: | ||
response = {"message": "Invalid request: missing 'task_ids'"} | ||
abort(make_response(response, 400)) | ||
|
||
tasks = Task.query.filter(Task.id.in_(task_ids)).all() | ||
|
||
if len(tasks) != len(task_ids): | ||
response = {"message": "One or more tasks not found"} | ||
abort(make_response(response, 404)) | ||
Comment on lines
+42
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. I really love all the error checking you've done here! One small thing that you could do to just streamline this just a bit comes in the form of the |
||
|
||
for task in tasks: | ||
if task not in goal.tasks: | ||
goal.tasks.append(task) | ||
|
||
db.session.commit() | ||
|
||
goal_data = { | ||
"id": goal.id, | ||
"task_ids": [task.id 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. Great list comprehension! |
||
} | ||
|
||
response = goal_data | ||
return response, 200 | ||
|
||
|
||
@bp.get("/<goal_id>/tasks") | ||
def get_tasks_for_goal(goal_id): | ||
from app.models.goal import Goal | ||
from app.models.task import Task | ||
Comment on lines
+65
to
+66
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. Go ahead and just add these imports both to the top of the file! |
||
goal = validate_model(Goal, goal_id) | ||
|
||
|
||
if goal.tasks: | ||
task_data = [task.to_dict() for task in goal.tasks] | ||
else: | ||
tasks = Task.query.filter_by(goal_id=goal.id).all() | ||
task_data = [ | ||
task.to_dict() for task in tasks | ||
] | ||
Comment on lines
+70
to
+76
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. Two things here!
As a result, this whole section could be distilled down to just:
I believe this is also what is causing your last two tests to fail! |
||
|
||
task_data = task_data or [] | ||
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 make the suggested changes above, this line will no longer be necessary. |
||
|
||
for task in task_data: | ||
task["is_complete"] = bool(task.get("is_complete", False)) # Explicitly cast to bool | ||
Comment on lines
+80
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. This should already be a boolean so we don't need to explicitly cast it! |
||
|
||
response = { | ||
"id": goal.id, | ||
"title": goal.title, | ||
"tasks": task_data | ||
} | ||
|
||
return response, 200 | ||
|
||
@bp.get("/tasks/<task_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. This seems like it should be in the task_routes.py! |
||
def get_task_by_id(task_id): | ||
from app.models.task import Task | ||
from app.models.goal import Goal | ||
Comment on lines
+93
to
+94
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. Just import at the top! |
||
|
||
# Validate that the task exists | ||
task = Task.query.get(task_id) | ||
if not task: | ||
response = {"message": f"task id {task_id} not found"} | ||
abort(make_response(response, 404)) | ||
Comment on lines
+97
to
+100
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 can all be replaced with the validate_model function! |
||
|
||
task_data = { | ||
"id": task.id, | ||
"goal_id": task.goal_id, | ||
"title": task.title, | ||
"description": task.description, | ||
"is_complete": task.completed_at is not None | ||
} | ||
Comment on lines
+102
to
+108
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 looks strikingly similar to the to_dict function from our Task model. I wonder if we could use that instead? |
||
|
||
return {"task": task_data}, 200 | ||
|
||
|
||
|
||
@bp.get("") | ||
def get_all_goal(): | ||
return get_model_with_filters(Goal, request.args) | ||
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 job using the function from your route utilities! |
||
|
||
@bp.get("/<goal_id>") | ||
def get_one_goal(goal_id): | ||
goal = validate_model(Goal, goal_id) | ||
if not goal: | ||
abort(make_response({"message": f"goal_id {goal_id} not found"}, 404)) | ||
Comment on lines
+121
to
+122
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 will have been caught by the validate_model! |
||
goal_response = { | ||
"goal": goal.to_dict() | ||
} | ||
|
||
return goal_response, 200 | ||
|
||
|
||
@bp.put("/<goal_id>") | ||
def update_goal(goal_id): | ||
goal = validate_model(Goal, goal_id) | ||
if not goal: | ||
abort(make_response({"message": f"goal_id {goal_id} not found"}, 404)) | ||
Comment on lines
+133
to
+134
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. Validate model will handle this! |
||
request_body = request.get_json() | ||
|
||
goal.title = request_body["title"] | ||
|
||
db.session.commit() | ||
|
||
response = { | ||
"goal": goal.to_dict() | ||
} | ||
|
||
return response, 200 | ||
|
||
@bp.delete("/<goal_id>") | ||
def delete_goal(goal_id): | ||
goal = validate_model(Goal, goal_id) | ||
if not goal: | ||
abort(make_response({"message": f"goal_id {goal_id} not found"}, 404)) | ||
Comment on lines
+150
to
+151
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 should be caught by the validate_model method! |
||
|
||
db.session.delete(goal) | ||
db.session.commit() | ||
|
||
response = { | ||
"details": f'Goal {goal.id} "{goal.title}" successfully deleted' | ||
} | ||
|
||
return response, 200 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
from flask import abort, make_response | ||
from ..db import db | ||
|
||
def validate_model(cls, model_id): | ||
model_prefix = 'goal' if cls.__name__.lower() == 'goal' else '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 idea with functions such as this one is to make it so it can work with any class. This line will only make it work with goal or task and nothing else! Feel free to use |
||
|
||
try: | ||
model_id = int(model_id) | ||
except ValueError: | ||
abort(make_response({"message": f"{model_prefix}_id {model_id} invalid"}, 400)) | ||
|
||
# query = db.select(cls).where(cls.id == model_id) | ||
# model = db.session.scalar(query) | ||
model = cls.query.get(model_id) | ||
if not model: | ||
abort(make_response({"message": f"{model_prefix}_id {model_id} not found"}, 404)) | ||
|
||
return model | ||
|
||
|
||
def create_model(cls, model_data): | ||
try: | ||
new_model = cls.from_dict(model_data) | ||
except KeyError as error: | ||
response = {"message": f"Invalid request: missing {error.args[0]}"} | ||
abort(make_response(response, 400)) | ||
|
||
def get_model_with_filters(cls, filters=None): | ||
query = db.select(cls) | ||
|
||
if filters: | ||
for attribute, value in filters.items(): | ||
if isinstance(value, list): | ||
query = query.where(getattr(cls, attribute).in_(value)) | ||
else: | ||
query = query.where(getattr(cls, attribute).ilike(f"%{value}%") if isinstance(value, str) else getattr(cls, attribute) == value) | ||
models = db.session.scalars(query.order_by(cls.id)) | ||
models_response = [model.to_dict() for model in models] | ||
|
||
return models_response |
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.
Great use of aliasing here!