-
Notifications
You must be signed in to change notification settings - Fork 44
C22 Phoenix - Liubov D. #46
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
f71d8f2
6ffaa32
5cf4d3c
d36600e
c184c18
d3b9a04
e1c34d9
4a93336
413cb82
4170310
357c86b
8d49583
72703ba
12b2671
e2a4241
7fcb7dc
c775d41
84ca5c4
d042708
87586e0
f6eff49
a61d5c7
19de496
c92f8fa
de57375
b815c8e
7ec9c0f
472b7a2
92bb214
33086d8
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,11 +1,17 @@ | ||
from flask import Flask | ||
from .db import db, migrate | ||
from .models import task, goal | ||
from .models import goal, task | ||
from .routes.task_routes import bp as tasks_bp | ||
from .routes.goal_routes import bp as goals_bp | ||
Comment on lines
+4
to
+5
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 renamed importing of the Alternatively, we could import just the module itself, then access the from .routes import task_routes
from .routes import goal_routes And then at the registration app.register_blueprint(task_routes.bp)
app.register_blueprint(goal_routes.bp) |
||
import os | ||
from flask_cors import CORS | ||
|
||
def create_app(config=None): | ||
app = Flask(__name__) | ||
|
||
CORS(app) | ||
app.config['CORS_HEADERS'] = 'Content-Type' | ||
|
||
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI') | ||
|
||
|
@@ -18,5 +24,8 @@ def create_app(config=None): | |
migrate.init_app(app, db) | ||
|
||
# Register Blueprints here | ||
app.register_blueprint(tasks_bp) | ||
app.register_blueprint(goals_bp) | ||
|
||
|
||
return app |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,23 @@ | ||
from sqlalchemy.orm import Mapped, mapped_column | ||
from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
from ..db import db | ||
|
||
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 dict( | ||
goal=dict( | ||
id=self.id, | ||
title=self.title | ||
) | ||
) | ||
|
||
# from JSON to model | ||
@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,47 @@ | ||
from sqlalchemy.orm import Mapped, mapped_column | ||
from sqlalchemy.orm import Mapped, mapped_column, relationship | ||
from sqlalchemy import ForeignKey | ||
from ..db import db | ||
from datetime import datetime | ||
from typing import Optional | ||
|
||
class Task(db.Model): | ||
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True) | ||
title: Mapped[str] | ||
description: Mapped[str] | ||
completed_at: Mapped[Optional[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. 👀 There wasn't anything in our functionality that would have required storing the |
||
goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id")) | ||
goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks") | ||
|
||
|
||
# from model to JSON | ||
def to_dict(self): | ||
if self.goal_id is None: | ||
return dict( | ||
id=self.id, | ||
title=self.title, | ||
description=self.description, | ||
is_complete=self.completed_at!=None | ||
) | ||
else: | ||
return dict( | ||
id=self.id, | ||
title=self.title, | ||
description=self.description, | ||
is_complete=self.completed_at!=None, | ||
goal_id=self.goal_id | ||
) | ||
Comment on lines
+18
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. Nice. This addresses the underlying test issue. Notice that there is a significant amount of repetition between the two halves of the conditional. Instead of duplicating so much of the dictionary creation, we could take an approach more like this. (Also remember that we prefer result = dict(
id=self.id,
title=self.title,
description=self.description,
is_complete=self.completed_at is not None
)
if self.goal_id:
result["goal_id"] = self.goal_id
return result |
||
|
||
# from JSON to model | ||
@classmethod | ||
def from_dict(cls, task_data): | ||
return cls( | ||
title=task_data["title"], | ||
description=task_data["description"] | ||
) | ||
|
||
|
||
|
||
|
||
|
||
|
||
|
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. Remember to always create a |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,146 @@ | ||
from flask import Blueprint | ||
from flask import Blueprint, request, Response, make_response, abort | ||
from app.models.goal import Goal | ||
from ..db import db | ||
from ..models.task import Task | ||
from ..routes.task_routes import validate_task | ||
|
||
bp = Blueprint("goals_bp", __name__, url_prefix="/goals" ) | ||
|
||
@bp.post("") | ||
def create_goal(): | ||
request_body = request.get_json() | ||
|
||
try: | ||
new_goal = Goal.from_dict(request_body) | ||
|
||
except KeyError as e: | ||
response = { | ||
"details": "Invalid data" | ||
} | ||
abort(make_response(response, 400)) | ||
Comment on lines
+13
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. Note how similar this error handling is to the POST route in Task. We could refactor this to have the shared logic in a helper. There's an example of this in Learn. |
||
|
||
db.session.add(new_goal) | ||
db.session.commit() | ||
|
||
return new_goal.to_dict(), 201 | ||
|
||
@bp.get("") | ||
def get_all_goals(): | ||
query = db.select(Goal) | ||
|
||
title_param = request.args.get("title") | ||
if title_param: | ||
query = query.where(Goal.title.ilike(f"%{title_param}%")) | ||
|
||
sort_param = request.args.get("sort") | ||
if sort_param: | ||
if sort_param == "asc": | ||
query = query.order_by(Goal.title.asc()) | ||
elif sort_param == "desc": | ||
query = query.order_by(Goal.title.desc()) | ||
|
||
query = query.order_by(Goal.id) | ||
goals = db.session.scalars(query) | ||
|
||
goals_response = [] | ||
for goal in goals: | ||
goals_response.append( | ||
{ | ||
"id": goal.id, | ||
"title": goal.title | ||
} | ||
Comment on lines
+48
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. Prefer to use the to_dict logic here and throughout to avoid duplicating the dictionary structure logic. |
||
) | ||
|
||
return goals_response | ||
|
||
@bp.get("/<goal_id>") | ||
def get_one_goal(goal_id): | ||
goal = validate_goal(goal_id) | ||
|
||
return goal.to_dict() | ||
|
||
|
||
@bp.put("/<goal_id>") | ||
def update_goal(goal_id): | ||
goal = validate_goal(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. As with the Task PUT route, this route could benefit from key error handling similar to the POST request. |
||
|
||
db.session.commit() | ||
|
||
response = { | ||
"goal": { | ||
"id": goal.id, | ||
"title": goal.title | ||
} | ||
} | ||
|
||
return response, 200 | ||
|
||
@bp.delete("/<goal_id>") | ||
def delete_goal(goal_id): | ||
goal = validate_goal(goal_id) | ||
db.session.delete(goal) | ||
db.session.commit() | ||
|
||
response = { | ||
"details": f"Goal {goal_id} \"{goal.title}\" successfully deleted" | ||
} | ||
|
||
return response, 200 | ||
|
||
def validate_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. 👀 Note that we could refactor this |
||
try: | ||
goal_id = int(goal_id) | ||
except: | ||
response = {"details": "Invalid data"} | ||
|
||
abort(make_response(response , 400)) | ||
|
||
query = db.select(Goal).where(Goal.id == goal_id) | ||
goal = db.session.scalar(query) | ||
|
||
if not goal: | ||
response = {"message": f"Goal {goal_id} not found"} | ||
abort(make_response(response, 404)) | ||
|
||
return goal | ||
|
||
@bp.post("/<goal_id>/tasks") | ||
def assign_tasks_to_goal(goal_id): | ||
goal = validate_goal(goal_id) | ||
|
||
request_body = request.get_json() | ||
task_ids = request_body["task_ids"] | ||
|
||
for task_id in task_ids: | ||
task = validate_task(task_id) | ||
task.goal_id = 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. Notice that by updating the goal id associated with the tasks, this effectively has "append" behavior (any existing tasks already associated with this Goal will remain unchanged). The tests don't provide enough specificity to determine whether append or replace behavior is desired, but we should be consistent between the operation we carry out here, and the result that we return (see below). |
||
db.session.add(task) | ||
|
||
db.session.commit() | ||
|
||
response = { | ||
"id": int(goal_id), | ||
"task_ids": 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. Echoing back the task_ids that were supplied here effectively acts like a "replace" behavior. We assume that the only tasks belonging to the Goal after this endpoint runs are the tasks we were given in this call. But the logic above is taking an "append" approach, meaning there could still be other tasks associated with this goal, which we should list here (by fetching the ids of all tasks now associated with the goal rather than using the task ids only supplied to this call). |
||
} | ||
|
||
return response, 200 | ||
|
||
@bp.get("/<goal_id>/tasks") | ||
def get_goal_with_assigned_tasks(goal_id): | ||
goal = validate_goal(goal_id) | ||
|
||
query = db.select(Task).where(Goal.id == goal_id) | ||
|
||
tasks = db.session.scalars(query) | ||
|
||
response = {} | ||
response["id"] = goal.id | ||
response["title"] = goal.title | ||
Comment on lines
+140
to
+141
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 dictionary with these two keys could be produced by the goal |
||
response["tasks"] = [] | ||
|
||
response["tasks"] = [task.to_dict() for task in tasks] | ||
|
||
return response, 200 |
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.
Once we have other import paths to our models (here, through the blueprints, which import the models), we technically don't need the imports here any more. It's fine to leave them for clarity (and a reminder that any other models we add would need to be included until other routes are setup), but if the VS Code warning is bothersome, feel free to remove these.