Say Zoisite #109
Open
Say Zoisite #109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Congrats on your first solo project API! There are a couple things I would like to see you revisit if you have time, I'll add more details on those items in Learn. I've left some comments and questions across the PR, please take a look when you can and reply here or on Slack if there's anything I can clarify =]
|
|
||
| # client = slack.WebClient(token=os.environ['SLACK_TOKEN']) |
There was a problem hiding this comment.
I see this commented line duplicated on line 13, we should remove commented code like this and rely on our repo's commit history if we need to look up past code.
Comment on lines
+38
to
+39
| from app.routes.task_routes import tasks_bp | ||
| from app.routes.goal_routes import goals_bp |
There was a problem hiding this comment.
Nice choice to split up the routes into files that are more specific to the resources they work with.
There was a problem hiding this comment.
I recommend placing this file in the routes directory to keep it close to the code that uses it.
Comment on lines
+7
to
+10
| sort_by = query_params.get("sort") | ||
|
|
||
| if sort_by: | ||
| return get_sorted_items_by_params(cls, query_params) |
There was a problem hiding this comment.
I would consider separating out the sort into its own function or renaming filter_by_params to reflect that it results in both sorting and filtering.
Comment on lines
+12
to
+16
| if query_params: | ||
| query_params = {k.lower(): v.title() for k, v in query_params.items()} | ||
| items = cls.query.filter_by(**query_params).all() | ||
| else: | ||
| items = cls.query.all() |
There was a problem hiding this comment.
There's some similar code repeated depending on if we want to sort and filter or only filter. If we want to D.R.Y. up the code, we could get a reference to the class's query object that we keep updating. One possibility might look like:
model_query = cls.query
if query_params:
query_params = {k.lower(): v.title() for k, v in query_params.items()}
model_query = model_query.filter_by(**query_params).all()
if sort_by and sort_by == "asc":
model_query = model_query.order_by(cls.title.asc())
elif sort_by and sort_by == "desc":
model_query = model_query.order_by(cls.title.desc())
items = model_query.all()
Comment on lines
+80
to
+86
| task_data.append({ | ||
| "id": task.task_id, | ||
| "goal_id": goal.goal_id, | ||
| "title": task.title, | ||
| "description": task.description, | ||
| "is_complete": task.is_complete | ||
| }) |
There was a problem hiding this comment.
Could we use the task's task_to_dict function?
| "description": task.description, | ||
| "is_complete": task.is_complete | ||
| }) | ||
| task_data |
There was a problem hiding this comment.
We should remove this line since we aren't performing an operation.
There was a problem hiding this comment.
Great looking code across the route files as far as line length, clear variable names, spacing, and overall readability 😊
Comment on lines
+1
to
+8
| # import os | ||
| # from pathlib import Path | ||
| # from dotenv import load_dotenv | ||
|
|
||
| # env_path = Path('.') / '.env' | ||
| # load_dotenv(dotenv_path=env_path) | ||
|
|
||
| # token = os.environ.get('SLACK_TOKEN') |
There was a problem hiding this comment.
Was this file intended to be committed?
Comment on lines
+98
to
+118
| # @pytest.mark.skip(reason="No way to test this feature yet") | ||
| def test_get_task_includes_goal_id(client, one_task_belongs_to_one_goal): | ||
| response = client.get("/tasks/1") | ||
| response = client.get("goals/1/tasks") | ||
| response_body = response.get_json() | ||
|
|
||
| assert response.status_code == 200 | ||
| assert "task" in response_body | ||
| assert "goal_id" in response_body["task"] | ||
| assert "tasks" in response_body | ||
| assert "goal_id" in response_body["tasks"][0] | ||
| assert response_body == { | ||
| "task": { | ||
| "id": 1, | ||
| "goal_id": 1, | ||
| "title": "Go on my daily walk 🏞", | ||
| "description": "Notice something new every day", | ||
| "is_complete": False | ||
| } | ||
| } | ||
| "id": 1, | ||
| "title": "Build a habit of going outside daily", | ||
| "tasks": [ | ||
| { | ||
| "id": 1, | ||
| "goal_id": 1, | ||
| "title": "Go on my daily walk 🏞", | ||
| "description": "Notice something new every day", | ||
| "is_complete": False | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
The original test does not pass. We should use the tests to guide our development - by changing the tests to meet our code, we change the specification of the project. What would need to change about our models or routes to make the original test case pass?
|
Hi Kelsey,
This is my link for render!
https://task-list-say-d19.onrender.com/tasks
Say
…On Thu, May 18, 2023 at 4:28 PM Kelsey Steven ***@***.***> wrote:
***@***.**** commented on this pull request.
Congrats on your first solo project API! There are a couple things I would
like to see you revisit if you have time, I'll add more details on those
items in Learn. I've left some comments and questions across the PR, please
take a look when you can and reply here or on Slack if there's anything I
can clarify =]
------------------------------
In app/__init__.py
<#109 (comment)>
:
>
+ # client = slack.WebClient(token=os.environ['SLACK_TOKEN'])
I see this commented line duplicated on line 13, we should remove
commented code like this and rely on our repo's commit history if we need
to look up past code.
------------------------------
In app/__init__.py
<#109 (comment)>
:
> + from app.routes.task_routes import tasks_bp
+ from app.routes.goal_routes import goals_bp
Nice choice to split up the routes into files that are more specific to
the resources they work with.
------------------------------
On app/helper.py
<#109 (comment)>
:
I recommend placing this file in the routes directory to keep it close to
the code that uses it.
------------------------------
In app/helper.py
<#109 (comment)>
:
> + sort_by = query_params.get("sort")
+
+ if sort_by:
+ return get_sorted_items_by_params(cls, query_params)
I would consider separating out the sort into its own function or renaming
filter_by_params to reflect that it results in both sorting and filtering.
------------------------------
In app/helper.py
<#109 (comment)>
:
> + if query_params:
+ query_params = {k.lower(): v.title() for k, v in query_params.items()}
+ items = cls.query.filter_by(**query_params).all()
+ else:
+ items = cls.query.all()
There's some similar code repeated depending on if we want to sort and
filter or only filter. If we want to D.R.Y. up the code, we could get a
reference to the class's query object that we keep updating. One
possibility might look like:
model_query = cls.query
if query_params:
query_params = {k.lower(): v.title() for k, v in query_params.items()}
model_query = model_query.filter_by(**query_params).all()
if sort_by and sort_by == "asc":
model_query = model_query.order_by(cls.title.asc())elif sort_by and sort_by == "desc":
model_query = model_query.order_by(cls.title.desc())
items = model_query.all()
------------------------------
In app/helper.py
<#109 (comment)>
:
> +
+
+def validate_model(cls, id):
+ try:
+ id = int(id)
+ except:
+ message = f"{cls.__name__} {id} is invalid"
+ abort(make_response({"message": message}, 400))
+
+ obj = cls.query.get(id)
+ if not obj:
+ abort(make_response(jsonify(message=f"{cls.__name__} not found"), 404))
+
+ return obj
+
+def slack_post_message(task):
Great use of a helper function for the slack message!
------------------------------
In app/models/goal.py
<#109 (comment)>
:
> @@ -3,3 +3,35 @@
class Goal(db.Model):
goal_id = db.Column(db.Integer, primary_key=True)
+ title = db.Column(db.String)
+ tasks = db.relationship("Task", back_populates="goal", lazy=True)
+
+ def goal_to_dict(self):
+ return {
+ "id": self.goal_id,
+ "title": self.title
+ }
+ @classmethod
Style best practice note: We should leave a blank line between functions
to make it visually clear where one ends and another begins.
------------------------------
In app/models/goal.py
<#109 (comment)>
:
> + def __str__(self):
+ return {
+ self.__class__.__name__.lower(): {
+ "id": self.goal_id,
+ "title": self.title
+ }
+ }
To reduce confusion for users of our code, I suggest following
expectations of a function when implementing python's built in methods for
a class. __str__ is meant to return a human-readable, or informal, string
representation of an object, and it would be unexpected by most users that
this function would return a dictionary. A little more info can be found in
the python docs:
https://docs.python.org/3/reference/datamodel.html?highlight=__str__#object.__str
__
------------------------------
In app/models/goal.py
<#109 (comment)>
:
> + for key, value in goal.items():
+ if key == "title":
+ self.title = value
The dictionary could contain many extraneous keys that we need to loop
through, another option could be to check if the dictionary contains the
title key (which can be an O(1) operation) then set the title attribute
if the key exists.
------------------------------
In app/models/goal.py
<#109 (comment)>
:
> + if "title" not in request_data:
+ raise KeyError
+
+ return cls(
+ title=request_data["title"].title()
+ )
+
+ def __str__(self):
+ return {
+ self.__class__.__name__.lower(): {
+ "id": self.goal_id,
+ "title": self.title
+ }
+ }
+
+ def update(self, goal):
If the parameter goal is not a Goal object, then for clarity I'd suggest
updating the name to better reflect the contents. If the parameter holds
data to update a goal, what's a descriptive name we could use?
------------------------------
In app/models/task.py
<#109 (comment)>
:
> + request_data["completed_at"] = None
+ is_complete = False if request_data["completed_at"] is None else True
In this implementation, we hardcode a value in the input dictionary to
None, then use that to set is_complete and completed_at. The result would
be the same if we hardcoded is_complete to False and completed_at to None
without adding a key to the request_data parameter.
------------------------------
In app/models/task.py
<#109 (comment)>
:
> + completed_at=db.Column(db.DateTime, nullable=True)
+ is_complete= db.Column(db.Boolean, default=False)
is_complete duplicates data we have available by looking at the truthy or
falsy value of the completed_at field, and creates a situation where we
need to manually keep the values of completed_at and is_complete in sync
with each other. I recommend removing is_complete and checking if
completed_at is truthy or falsy in those locations.
------------------------------
In app/routes/goal_routes.py
<#109 (comment)>
:
> + try:
+ new_goal = Goal.create_new_goal(request_body)
+ db.session.add(new_goal)
+ db.session.commit()
+
+ message = new_goal.__str__()
+
+ return make_response(jsonify(message), 201)
+ except KeyError as e:
+ message = "Invalid data"
+ return make_response({"details": message}, 400)
To make it easier to debug what line caused an exception, we typically
want to limit what we put in try blocks to just the code we think might
cause an exception. What part of this code could result in a KeyError? I
suggest keeping just that code in the try, and moving the rest to after the
try/except block.
------------------------------
In app/routes/goal_routes.py
<#109 (comment)>
:
> + "details": f'Goal {goal_id} "{goal.title}" successfully deleted'
+ }
+ return make_response(jsonify(message), 200)
+
***@***.***_bp.route("/<goal_id>/tasks", methods=["POST"])
+def assign_tasks_to_goal(goal_id):
+ goal = validate_model(Goal, goal_id)
+ request_body = request.get_json()
+ if not request_body:
+ return jsonify({'error': 'Invalid request body'}), 400
+
+ task_ids = request_body.get('task_ids', [])
+ for task_id in task_ids:
+ task = validate_model(Task, task_id)
+ task.goal_id = goal.goal_id
+ db.session.commit()
We could move this line outside of the for-loop to save the changes to all
the tasks at once.
------------------------------
In app/models/task.py
<#109 (comment)>
:
> @@ -3,3 +3,54 @@
class Task(db.Model):
task_id = db.Column(db.Integer, primary_key=True)
+ title = db.Column(db.String(length=255))
+ description=db.Column(db.String)
+ completed_at=db.Column(db.DateTime, nullable=True)
+ is_complete= db.Column(db.Boolean, default=False)
+ goal_id = db.Column(db.Integer, db.ForeignKey('goal.goal_id'))
+ goal = db.relationship("Goal", back_populates="tasks")
+
+ def task_to_dict(self):
+ return {
+ "id": self.task_id,
+ "title": self.title,
+ "description": self.description,
+ "is_complete": False if self.completed_at is None else True
+ if self.completed_at is not None else None
Is line 19 intended to be there?
------------------------------
In app/routes/goal_routes.py
<#109 (comment)>
:
> + task_data.append({
+ "id": task.task_id,
+ "goal_id": goal.goal_id,
+ "title": task.title,
+ "description": task.description,
+ "is_complete": task.is_complete
+ })
Could we use the task's task_to_dict function?
------------------------------
In app/routes/goal_routes.py
<#109 (comment)>
:
> + return jsonify({'id': goal.goal_id, 'task_ids': task_ids}), 200
+
***@***.***_bp.route("/<goal_id>/tasks", methods=["GET"])
+def get_task_of_goal(goal_id):
+ goal = validate_model(Goal, goal_id)
+ goal = Goal.query.get(goal_id)
+ task_data = []
+ for task in goal.tasks:
+ task_data.append({
+ "id": task.task_id,
+ "goal_id": goal.goal_id,
+ "title": task.title,
+ "description": task.description,
+ "is_complete": task.is_complete
+ })
+ task_data
We should remove this line since we aren't performing an operation.
------------------------------
On app/routes/task_routes.py
<#109 (comment)>
:
Great looking code across the route files as far as line length, clear
variable names, spacing, and overall readability 😊
------------------------------
In app/say_bot.py
<#109 (comment)>
:
> +# import os
+# from pathlib import Path
+# from dotenv import load_dotenv
+
+# env_path = Path('.') / '.env'
+# load_dotenv(dotenv_path=env_path)
+
+# token = os.environ.get('SLACK_TOKEN')
Was this file intended to be committed?
------------------------------
In tests/test_wave_06.py
<#109 (comment)>
:
> +# @pytest.mark.skip(reason="No way to test this feature yet")
def test_get_task_includes_goal_id(client, one_task_belongs_to_one_goal):
- response = client.get("/tasks/1")
+ response = client.get("goals/1/tasks")
response_body = response.get_json()
assert response.status_code == 200
- assert "task" in response_body
- assert "goal_id" in response_body["task"]
+ assert "tasks" in response_body
+ assert "goal_id" in response_body["tasks"][0]
assert response_body == {
- "task": {
- "id": 1,
- "goal_id": 1,
- "title": "Go on my daily walk 🏞",
- "description": "Notice something new every day",
- "is_complete": False
- }
- }
+ "id": 1,
+ "title": "Build a habit of going outside daily",
+ "tasks": [
+ {
+ "id": 1,
+ "goal_id": 1,
+ "title": "Go on my daily walk 🏞",
+ "description": "Notice something new every day",
+ "is_complete": False
+ }
+ ]
+ }
The original test does not pass. We should use the tests to guide our
development - by changing the tests to meet our code, we change the
specification of the project. What would need to change about our models or
routes to make the original test case pass?
—
Reply to this email directly, view it on GitHub
<#109 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AS7ZJXUAJX7HI23H2ROQOSLXG2A7PANCNFSM6AAAAAAX7YCFQE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Awesome, thanks for the link! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.