Pull Request from Yasiel by YasielLopez · Pull Request #25 · Ada-C23/task-list-api

YasielLopez · 2025-05-09T13:06:44Z

No description provided.

kelsey-steven-ada

Nice work overall! Please review the feedback and questions in the comments when you have time. Let me know here or on Slack if you have questions or there's anything I can clarify. =] There are some areas I would like you to look at, I'll leave specific info on those in Learn.

kelsey-steven-ada · 2025-05-13T17:13:04Z

app/__init__.py

+    from .routes.task_routes import tasks_bp
+    from .routes.goal_routes import goals_bp


Did you run into an issue with putting the imports at the top of the file? Unless there is a specific need due to the project structure or requirements, our imports should always go at the top of the file.

from flask import Flask from .db import db, migrate from .models import task, goal from .routes.task_routes import tasks_bp from .routes.goal_routes import goals_bp import os

kelsey-steven-ada · 2025-05-13T17:14:53Z

app/__init__.py

+    from .routes.task_routes import tasks_bp
+    from .routes.goal_routes import goals_bp


To be more consistent with Flask and API patterns, we should name our blueprints bp, and in other files like __init__.py here we should import them with more specific names if necessary.

kelsey-steven-ada · 2025-05-13T17:28:00Z

app/models/goal.py

@@ -1,5 +1,35 @@
-from sqlalchemy.orm import Mapped, mapped_column
+from sqlalchemy.orm import Mapped, mapped_column, relationship
+from sqlalchemy import String


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 Task model as well.

If it was removed, the change to this line was not pushed up to the repo.

kelsey-steven-ada · 2025-05-13T17:31:22Z

app/models/goal.py

    id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
+    title: Mapped[str] = mapped_column(String(100), nullable=False)
+
+    tasks = relationship("Task", back_populates="goal", cascade="all, delete-orphan")


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.

Ah, yeah. you could do something like

tasks = relationship("Tasl", back_populates="goal",
cascade="save-update, merge, refresh-expire")

kelsey-steven-ada · 2025-05-13T17:32:20Z

app/models/goal.py


 class Goal(db.Model):
    id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
+    title: Mapped[str] = mapped_column(String(100), nullable=False)


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. This feedback applies to attributes on the Task model as well.

kelsey-steven-ada · 2025-05-13T22:38:05Z

app/routes/goal_routes.py

+    for task_id in task_ids:
+        task = db.session.scalar(db.select(Task).where(Task.id == task_id))
+        if task:
+            goal.tasks.append(task)


It is functionally about the same, but if we had a generic validate_model function, another approach could be to create our list of validated tasks, and then assign it to goal.tasks:

task_ids = request_body["task_ids"] valid_tasks = [validate_model(Task, task_id) for task_id in task_ids] goal.tasks = valid_tasks

kelsey-steven-ada · 2025-05-14T18:45:42Z

app/routes/task_routes.py

+def get_task(task_id):
+    task = get_task_by_id(task_id)
+
+    if not task:
+        return make_response(jsonify({"error": "Task not found"}), 404)
+
+    return jsonify({"task": task.to_dict(include_goal_id=True)})


How does a user know if they typed the id in wrong or if the task actually doesn't exist? I recommend reflecting on the patterns we used in Learn and in-class projects for error handling and sharing information with users to create helpful and descriptive errors.

kelsey-steven-ada · 2025-05-14T18:49:32Z

app/routes/task_routes.py

+    task = get_task_by_id(task_id)
+
+    if not task:
+        return make_response(jsonify({"error": "Task not found"}), 404)


This code is repeated exactly across several task routes, what could it look like share this code between the routes, allowing the file to better follow D.R.Y. principles?

kelsey-steven-ada · 2025-05-14T18:51:27Z

app/routes/task_routes.py

+        # Optional: Log the Slack API response for debugging
+        print(f"Slack API Response: {slack_response.status_code}, {slack_response.text}")


Reviewing our project for commented code & debugging code left behind should be one of our final steps in completing a coding task. Debugging code should be removed prior to opening PRs so it does not get added to the production code base.

kelsey-steven-ada · 2025-05-14T19:10:56Z

app/routes/task_routes.py

+    # Send a notification to Slack
+    slack_token = os.environ.get('SLACK_BOT_TOKEN')
+    if slack_token:
+        slack_data = {
+            'channel': 'task-notifications',
+            'text': f"Someone just completed the task {task.title}"
+        }
+        headers = {
+            'Authorization': f'Bearer {slack_token}',
+            'Content-Type': 'application/json'
+        }
+
+        # Make the POST request to Slack API
+        slack_response = requests.post(
+            'https://slack.com/api/chat.postMessage',
+            json=slack_data,
+            headers=headers
+        )


Thinking about separation of concerns, what responsibilities does this function have? I see it doing 2 things, both updating the database record, and sending a Slack message.

If we look at this as a growing project like we would be working on in the industry, what happens if we wanted to send a slack message when we call mark_task_incomplete? Right now, our only code to send a Slack message is deeply tied to marking a task complete, we cannot do one without the other.

How could we make our code more flexible so that our ability to send a message could be shared with other routes?

I'll refactor my code to create a module that handles notifications

kelsey-steven-ada

It looks like some of the items I listed out in Learn as necessary to address have not been completed or could use further work. Please take a look at the original list and ensure all feedback from learn has been applied to both Goals and Tasks models and routes. I've added some new feedback around these items that are still in progress.

kelsey-steven-ada · 2025-05-19T18:24:04Z

app/__init__.py

@@ -1,22 +1,22 @@
 from flask import Flask
 from .db import db, migrate
-from .models import task, goal


Even though we don't use them in the function, we still want to import the models into __init__.py to 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.

kelsey-steven-ada · 2025-05-19T18:25:35Z

app/__init__.py

+    from .routes.task_routes import bp as tasks_bp
+    from .routes.goal_routes import bp as goals_bp


These imports should be at the top of the file for consistent layout and understanding of what dependencies a file is bringing in. We should not import things inside functions unless there is a reason doing so is absolutely necessary.

kelsey-steven-ada · 2025-05-19T18:26:08Z

app/models/goal.py

@@ -1,5 +1,35 @@
-from sqlalchemy.orm import Mapped, mapped_column
+from sqlalchemy.orm import Mapped, mapped_column, relationship
+from sqlalchemy import String


If it was removed, the change to this line was not pushed up to the repo.

kelsey-steven-ada · 2025-05-19T18:29:10Z

app/models/task.py

+    completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True)
+
+    goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"), nullable=True)


Since these two attributes, completed_at and goal_id are already using Optional to declare that they could be null, we should not also use the mapped_column(nullable=True) syntax because this duplicates effort. We should choose only one style of syntax for declaring something is optional and use it consistently across the project.

kelsey-steven-ada · 2025-05-19T18:39:20Z

app/routes/goal_routes.py

+    return "", 204
+
+@bp.route("/<goal_id>", methods=["DELETE"])
+def delete_goal(goal_id):
+    goal = get_goal_by_id(goal_id)
+
+    if not goal:
+        return {"error": "Goal not found"}, 404
+
+    db.session.delete(goal)
+    db.session.commit()
+
+    return "", 204


I would like you to revisit the UPDATE and DELETE routes for Goals and Tasks. This passes the test suite since those tests check the status code is 204 and whether the update/delete took place in the database. However, it is not following the requirements for the UPDATE and DELETE routes. These should all:

return 204 No Content, which means they should have no body at all. Currently an empty string is being sent.

have a mimetype of "application/json" to keep our response type consistent across the whole API

Please review the mechanisms we used in hello-books in Learn and in Flasky in-class for returning an empty body with "application/json" response type and update the UPDATE and DELETE routes for Goals and Tasks accordingly. If you have questions or would like to chat about the syntax, please reach out!

kelsey-steven-ada · 2025-05-19T18:45:16Z

app/routes/goal_routes.py

+    goal = get_goal_by_id(goal_id)
+
+    if not goal:
+        return {"error": "Goal not found"}, 404


From the prior feedback, I would like you to address error handling across all routes to use Flask's abort rather than returning directly. This is an old answer, but this stack overflow talks at a high level how these are different and might give you a place to start if you want to dig further into why we want to use abort: https://stackoverflow.com/questions/47288166/difference-between-flask-abort-or-returning-a-status

kelsey-steven-ada · 2025-05-19T18:59:52Z

app/routes/goal_routes.py

+
+def get_task_by_id(id):
+    query = db.select(Task).where(Task.id == id)
+    return db.session.scalar(query)


Nice updates to return the JSON and let the route handle adding the default 200 status code.

kelsey-steven-ada

Updates look good, nice work addressing the Model attributes and route return values!

kelsey-steven-ada · 2025-05-28T16:24:15Z

app/__init__.py

+    app.register_blueprint(tasks_bp)
+    app.register_blueprint(goals_bp)
+
+    @app.errorhandler(404)


Neat research into error handling to better control the user error messages!

YasielLopez added 5 commits May 9, 2025 02:50

completed the task model to include all required fields.

1c6dd23

all tests passed

ac78cf9

added an app variable for render

dae65a7

Add server.py for Render deployment

07bdbe3

added missing test for wave_01

c9ee41f

kelsey-steven-ada reviewed May 14, 2025

View reviewed changes

YasielLopez added 5 commits May 19, 2025 03:39

added all suggested changes

588a331

final changes

a19a178

changed render issue

60b54be

more render fixes

7782e71

ugh

8465843

kelsey-steven-ada reviewed May 19, 2025

View reviewed changes

all suggested changes implemented

660a666

kelsey-steven-ada reviewed May 28, 2025

View reviewed changes

		from .routes.task_routes import tasks_bp
		from .routes.goal_routes import goals_bp

		# Optional: Log the Slack API response for debugging
		print(f"Slack API Response: {slack_response.status_code}, {slack_response.text}")

		from .routes.task_routes import bp as tasks_bp
		from .routes.goal_routes import bp as goals_bp

		completed_at: Mapped[Optional[datetime]] = mapped_column(nullable=True)

		goal_id: Mapped[Optional[int]] = mapped_column(ForeignKey("goal.id"), nullable=True)

Conversation

YasielLopez commented May 9, 2025

Uh oh!

kelsey-steven-ada left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelsey-steven-ada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kelsey-steven-ada left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kelsey-steven-ada left a comment •

edited

Loading