Crow_lexy Le task_list_api #89

xixilele1990 · 2025-11-07T06:24:04Z

all tests passed with the picture of wave4

kelsey-steven-ada · 2025-11-12T18:39:40Z

ada-project-docs/wave_04.md

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


Thanks for including the image ^_^

kelsey-steven-ada · 2025-11-12T18:56:00Z

app/__init__.py

+from dotenv import load_dotenv
+load_dotenv()


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.

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

kelsey-steven-ada · 2025-11-12T19:02:48Z

app/models/goal.py

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


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.

updated.thanks

kelsey-steven-ada · 2025-11-12T19:03:43Z

app/models/goal.py

+from sqlalchemy.orm import Mapped, mapped_column,relationship
 from ..db import db
+from sqlalchemy import String
+#from sqlalchemy import Date


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

kelsey-steven-ada · 2025-11-12T21:43:15Z

app/models/task.py

+
+
+


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.

kelsey-steven-ada · 2025-11-13T19:52:22Z

app/routes/task_routes.py

+
+

Best practices are that we should only have a single blank line at most separating lines inside of a function. More lines makes the reader think the function has ended and can be very confusing when trying to quickly read through code.

thanks for the headsup

kelsey-steven-ada · 2025-11-13T19:55:41Z

app/routes/task_routes.py

+    if not current_app.config.get("TESTING"):
+        response = requests.post(
+            "https://slack.com/api/chat.postMessage",
+            headers={"Authorization": f"Bearer {slack_token}"},
+
+            json={"channel": slack_channel, "text": message}
+        )
+    if current_app.config.get("TESTING"):
+        return Response(status=204, mimetype="application/json")


We could make this a little more concise and unindent the main flow of the application by first checking if we are in testing mode:

if current_app.config.get("TESTING"): return Response(status=204, mimetype="application/json") response = requests.post( "https://slack.com/api/chat.postMessage", headers={"Authorization": f"Bearer {slack_token}"}, json={"channel": slack_channel, "text": message} )

If we handle the edge case return first, then if we move past that without returning we must be on the production path and do not need to indent it with an if-statement to check.

if not current_app.config.get("TESTING"):
response = requests.post(
"https://slack.com/api/chat.postMessage",
headers={"Authorization": f"Bearer {slack_token}"},

json={"channel": slack_channel, "text": message} ) return Response(status=204, mimetype="application/json") , not sure how to avoid using if ..

kelsey-steven-ada · 2025-11-13T19:58:56Z

app/routes/task_routes.py

+        return Response(status=204, mimetype="application/json")
+
+
+    return jsonify({"task": task.to_dict()}), 200


This function is not meeting the specification in the project READMEs. The function returns a different result in production vs when in testing. This would not be acceptable in a development team since the tests are not actually testing what the end user would see.

This function needs to be revisited to return the same response no matter what mode we are in. What would need to change to gracefully handle ignoring the slack message if the token isn't set or we are in testing mode?

kelsey-steven-ada · 2025-11-13T20:04:41Z

app/routes/route_utilities.py

+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))
+        abort(make_response({"details": "Invalid data"}, 400))
+
+    db.session.add(new_model)
+    db.session.commit()
+
+    return new_model.to_dict(), 201


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.

kelsey-steven-ada · 2025-11-13T20:05:00Z

app/routes/route_utilities.py

+        #response = {"message": f"Invalid request: missing {e.args[0]}"}
+        #abort(make_response(response, 400))


Please remove commented code before opening PRs.

…per function and remove all jsonify

kelsey-steven-ada · 2025-11-17T20:29:44Z

app/routes/task_routes.py

-
    db.session.commit()
-
    return Response(status = 204,mimetype ="application/json")


Small spacing issues.

We don't include spaces around the = when we are filling out a keyword argument in a function invocation.

This differs from when it is used for variable assignment or as part of a comparison operator where we always surround = in spaces.

Suggested change

return Response(status = 204,mimetype ="application/json")

return Response(status=204, mimetype="application/json")

kelsey-steven-ada · 2025-11-17T20:30:54Z

app/routes/task_routes.py

-    if current_app.config.get("TESTING"):
-        return Response(status=204, mimetype="application/json")
+    #if current_app.config.get("TESTING"):
+    return Response(status=204, mimetype="application/json")


Nice restructuring so the message is only sent in production mode, but we always return the same response.

kelsey-steven-ada · 2025-11-17T20:32:38Z

app/routes/goal_routes.py


-    return jsonify(new_goal.to_dict()), 201
+    #return jsonify(new_goal.to_dict()), 201
+    return create_model(Goal,request_body)


Nice update to use create_model for the Goal class! Is there a reason we cannot use it for Tasks as well?

just updated

kelsey-steven-ada · 2025-11-17T20:37:50Z

app/routes/goal_routes.py

+    # try:
+    #     new_goal = Goal.from_dict(request_body)
+    # except KeyError:
+    #     return jsonify({"details": "Invalid data"}), 400

-    db.session.add(new_goal)
-    db.session.commit()
+    # db.session.add(new_goal)
+    # db.session.commit()

-    return jsonify(new_goal.to_dict()), 201
+    #return jsonify(new_goal.to_dict()), 201


As part of these updates, please follow best practices to remove the old code once the refactor is working as expected.

…model

kelsey-steven-ada · 2025-11-18T21:45:08Z

app/models/task.py

    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)


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.

xixilele1990 added 4 commits November 4, 2025 21:51

wave1-3

bd34383

wave5-7

61c3168

wave4 change

98afc1c

datetime library change

eb9c199

kelsey-steven-ada reviewed Nov 13, 2025

View reviewed changes

xixilele1990 added 2 commits November 14, 2025 16:01

mark_test_complete returns same in prod and dev environment,using hel…

c6e7a13

…per function and remove all jsonify

update formatting and duplicated import

769ad1a

kelsey-steven-ada reviewed Nov 17, 2025

View reviewed changes

remove all jsonify and reuse create_model helper functions for tasks …

e7c70f0

…model

xixilele1990 force-pushed the main branch 3 times, most recently from bc464ac to 6e7836f Compare November 18, 2025 00:21

remove all comment out lines and space

d340937

xixilele1990 force-pushed the main branch from 6e7836f to d340937 Compare November 18, 2025 00:23

kelsey-steven-ada reviewed Nov 18, 2025

View reviewed changes

change the datatype and keep it consistent

f49cafa

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

		return Response(status=204, mimetype="application/json")


		return jsonify({"task": task.to_dict()}), 200

		#response = {"message": f"Invalid request: missing {e.args[0]}"}
		#abort(make_response(response, 400))


		db.session.commit()

		return Response(status = 204,mimetype ="application/json")

Crow_lexy Le task_list_api #89

Are you sure you want to change the base?

Crow_lexy Le task_list_api #89

Uh oh!

Conversation

xixilele1990 commented Nov 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xixilele1990 Nov 15, 2025 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

xixilele1990 Nov 15, 2025 •

edited

Loading