Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
.vscode
.DS_Store
.env

# Byte-compiled / optimized / DLL files
__pycache__/
Expand Down
Binary file added ada-project-docs/assets/lexy-slackbot.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions ada-project-docs/wave_04.md
Original file line number Diff line number Diff line change
Expand Up @@ -192,3 +192,11 @@ Send `PATCH` requests to `localhost:5000/tasks/<task_id>/mark_complete` (use the
![](assets/postman_patch.png)

![](assets/slack_notification_feature.png)



The result show as below
![](assets/lexy-slackbot.png)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for including the image ^_^




8 changes: 8 additions & 0 deletions app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
from flask import Flask
from .db import db, migrate
from .models import task, goal
from app.routes.task_routes import bp as tasks_bp
from app.routes.goal_routes import bp as goals_bp
import os

from dotenv import load_dotenv
load_dotenv()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you running into an issue that required calling load_dotenv() from here?

We need to call load_dotenv() in conftest.py to load up variables for the testing flow, but that shouldn't be necessary in production mode if we've configured things the same as we did for the hello-books and solar-system APIs.

If this code is not necessary, then we should remove it.

Copy link
Author

@xixilele1990 xixilele1990 Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I have duplicated , the one in init removed in my update commit, thanks


def create_app(config=None):
app = Flask(__name__)

Expand All @@ -18,5 +23,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
22 changes: 21 additions & 1 deletion app/models/goal.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,25 @@
from sqlalchemy.orm import Mapped, mapped_column
from sqlalchemy.orm import Mapped, mapped_column,relationship
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small spacing issue:

Suggested change
from sqlalchemy.orm import Mapped, mapped_column,relationship
from sqlalchemy.orm import Mapped, mapped_column, relationship

The same issue for the same import shows up in the Task model file.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated.thanks

from ..db import db
from sqlalchemy import String
#from sqlalchemy import Date
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an import was not required, the line should be removed completely before merging code from our branch into the main repo.



class Goal(db.Model):

id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str] = mapped_column(String(100), nullable=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For many applications, it may make sense to add a limit to the title, however nothing in the project requirements asks for there to be a limit on the title length for the Goal class.

Especially when working with others, we want to avoid changing or adding behavior that has already been decided for a product, unless those changes have been discussed and signed off on by stakeholders for the project.

We are working on this in isolation, but in a project with other folks, the choice to have no title limit may have come from user research on how folks are using the product. Introducing a limit that other developers were not expecting could lead to issues for other developers features or customer complaints when things do not work as expected or advertised.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nullable=False is the default value for an attribute in SQLAlchemy. It is more common to leave off that section if we are not changing the default behavior. If we are removing the added length limit as well, then we could remove the mapped_column content entirely.

tasks: Mapped[list["Task"]] = relationship(
back_populates="goal")


def to_dict(self):
return {
"id": self.id,
"title": self.title
}

@classmethod
def from_dict(cls, data_dict):
if "title" not in data_dict:
raise KeyError("title")
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern of checking for a key and raising a KeyError ourselves is an anti-pattern we should avoid.

The Python language will raise a KeyError for us if we try to access the title key, so we should simplify our code by relying on that.

  • The create goal route already has a try/except to catch the raised error, so here in this function, we can remove this if block and let the error raise on the next line if it does come up. We can rely on the route to handle the raised error and use abort to send a correctly formed error response.

return cls(title=data_dict["title"])
48 changes: 47 additions & 1 deletion app/models/task.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,51 @@
from sqlalchemy.orm import Mapped, mapped_column
from flask_sqlalchemy import SQLAlchemy
from sqlalchemy.orm import Mapped, mapped_column,relationship
from sqlalchemy import ForeignKey
from ..db import db
from sqlalchemy import String, Date
from datetime import datetime
from flask import Blueprint, abort, make_response, request, Response,jsonify
from typing import Optional
Comment on lines 1 to 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. What of these imports are used by this file?
  2. Which of these imports are not used and can be removed to ensure the file is not loading up libraries that the file will not access?

I would like to see a response to these questions.


class Task(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str] = mapped_column(String(100), nullable=False)
description: Mapped[str] = mapped_column(String(255))
completed_at:Mapped[datetime.date] = mapped_column(Date, nullable=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to the feedback around the title attribute on the Goal class. The feedback around adding limits that are not part of the problem description apply to title and description and the feedback around including nullable applies to all 3 attributes here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The completed_at attribute gives users info on the day when a task was completed, but depending on how we intend to use that date, it might not be the best fit for a project.

There is a difference between datetime.datetime and datetime.date objects in Python.

  • date only includes year, month, and day information, it has no way to include the time of day.
  • datetime includes a combination of a date and a time attributes, so it has year, month, and day, but also hour, minute, and second data.

Consider if we wanted to build a front end for this API so that users could look at their list of tasks and see exactly when each was checked off.

  • By using a date object we can only show a user what day they finished a task, we cannot show them if they finished that task at 9AM or at noon.
  • datetime give us more flexibility, we store more data on the object than date, but that means we can then present the exact time of day that someone checked off their task in our UI.

Just something to consider when deciding on data types for a project!

Docs covering the objects included in the datetime module can be found here for more info: https://docs.python.org/3/library/datetime.html#available-types

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is referencing two different data types datetime.date and sqlalchemy.Date:

completed_at:Mapped[datetime.date] = mapped_column(Date, nullable=True)

It seems like some kind of type coercion might be happening under the hood such that we are not running into errors, but this can lead to bugs or unexpected behavior if changes to either data type in the future means that the type coercion fails.

Depending on the requirements of a project we might choose one type or the other, but as with many programming best practices, it's most important to choose a single type and be consistent across the project with what you use. For this project either datetime.date or sqlalchemy.Date works here, but we should choose only one of them.


goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"))
goal: Mapped[Optional["Goal"]] = relationship(back_populates="tasks")

def to_dict(self): # json


task_dict = {"id": self.id,
"title":self.title,
"description":self.description,
#"is_complete":self.completed_at if self.completed_at else False
"is_complete": self.completed_at is not None}

if self.goal_id is not None:
task_dict["goal_id"] = self.goal_id


return task_dict



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small spacing issue, PEP8 best practices are to use a single line to separate methods within a class to visually show that these are closely related.

@classmethod
def from_dict(cls, data_dict):
if "title" not in data_dict:

raise KeyError("title")
if "description" not in data_dict:
#abort(make_response({"details": "Invalid data"}, 400))
raise KeyError("description")
return cls(
title=data_dict["title"],
description=data_dict["description"],
completed_at=None # default to None when creating
)
Comment on lines 35 to 46
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the feedback on the from_dict function in the goal class, how can we update this function to avoid the antipattern of checking if there would be a KeyError ourselves?




101 changes: 100 additions & 1 deletion app/routes/goal_routes.py
Original file line number Diff line number Diff line change
@@ -1 +1,100 @@
from flask import Blueprint
from flask import Blueprint, jsonify, request, abort, make_response
from ..db import db
from ..models.goal import Goal
from ..models.task import Task
from .route_utilities import validate_model


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:
return jsonify({"details": "Invalid data"}), 400

db.session.add(new_goal)
db.session.commit()

return jsonify(new_goal.to_dict()), 201
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wave 7 asks folks to implement the refactors we covered for hello-books and solar-system. I see that the functions exist in the route_utilities file, but the functions are not implemented in the routes.

I would like to see you revisit the project and implement the helper functions inside the routes to D.R.Y. up the create routes for Goals and Tasks to meet the project requirements.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you tell me more about where the syntax using jsonify came from?

In older versions of Flask, jsonify used to be required to convert python data structures to JSON. jsonify is no longer required and should not be used when we are sending back simple data types, dictionaries, or lists.

I would like to see you revisit this project to remove all usage of jsonify. Please use hello-books and/or solar-system as reference to follow best practices and make our return values more concise.



@bp.get("")
def get_all_goals():
goals = Goal.query.order_by(Goal.id).all()
return jsonify([goal.to_dict() for goal in goals]), 200


@bp.get("/<id>")
def get_one_goal(id):
goal = validate_model(Goal, id)
return jsonify(goal.to_dict()), 200




@bp.put("/<id>")
def update_goal(id):
goal = validate_model(Goal, id)
request_body = request.get_json()

goal.title = request_body["title"]

db.session.commit()
return jsonify(goal.to_dict()), 200



@bp.delete("/<id>")
def delete_goal(id):
goal = validate_model(Goal, id)

db.session.delete(goal)
db.session.commit()

return jsonify({"message": f'Goal {goal.id} successfully deleted'}), 204


#nested
@bp.post("/<goal_id>/tasks")
def add_tasks_to_goal(goal_id):
goal = validate_model(Goal, goal_id)
request_body = request.get_json()

task_ids = request_body.get("task_ids", [])

for task in goal.tasks:
task.goal_id = None



for task_id in task_ids:
task = validate_model(Task, task_id)
task.goal_id = goal.id

db.session.commit()

return jsonify({
"id": goal.id,
"task_ids": task_ids
}), 200



@bp.get("/<goal_id>/tasks")
def get_tasks_for_goal(goal_id):
goal = validate_model(Goal, goal_id)

tasks_response = [task.to_dict() for task in goal.tasks]

goal_dict = goal.to_dict()
goal_dict["tasks"] = tasks_response

return jsonify(goal_dict), 200



44 changes: 44 additions & 0 deletions app/routes/route_utilities.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
from flask import abort, make_response
from ..db import db

def validate_model(cls, id):
try:
id = int(id)
except ValueError:
invalid = {"message": f"{cls.__name__} id({id}) is invalid."}
abort(make_response(invalid, 400))

query = db.select(cls).where(cls.id == id)
model = db.session.scalar(query)

if not model:
not_found = {"message": f"{cls.__name__} with id({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 = {"message": f"Invalid request: missing {e.args[0]}"}
#abort(make_response(response, 400))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove commented code before opening PRs.

abort(make_response({"details": "Invalid data"}, 400))

db.session.add(new_model)
db.session.commit()

return new_model.to_dict(), 201
Comment on lines 20 to 29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this both creates a model and creates an http response, I'd consider renaming this to create_model_and_response. Or to better follow single responsibility principles, we could split this into 2 functions:

  • one function that only manages creating and returns the new model
  • one function that uses the function above to get the model, then creates and returns the response based on that model.


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
Comment on lines 20 to 41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two functions are not implemented in the relevant routes. How can we update the task and goal create routes to use create_model and how can we update one or both of the get_all routes to use the get_models_with_filters function?

  • If get_models_with_filters can't work for both goals and tasks as-is, why? Could get_models_with_filters be more useful if it returned a query object rather than executing the query and returning a list of models?

Loading