Tigers - Mica C. #124
Tigers - Mica C. #124
Conversation
and finished get_task_not_found test
implemented update_task and delete_task
imported goals_bp into __init__ fixed validate_model import into goal_routes
included goal_id, lazy=True, nullable=True
| def create_goal(): | ||
| request_body = request.get_json() | ||
|
|
||
| if (("title") not in request_body): |
There was a problem hiding this comment.
You shouldn't need the parentheses around "title" here, you can use a string directly with in against a dictionary.
| if sort_query == "asc": | ||
| goals = Goal.query.order_by(Goal.title) | ||
| elif sort_query == "desc": | ||
| goals = Goal.query.order_by(Goal.title.desc()) #ColumnElement.desc() method produces a descending ORDER BY clause element |
There was a problem hiding this comment.
I'm waffling on this comment. This is right on the border of "why" versus "what" but I feel it's more on the "what" size as it describes something about the underlying SQL implementation. That very well can be a relevant topic for a comment, but in this case I think the exact details aren't really adding any more information than the code already has.
I think the biggest reason why is because it's not a comment so much about your code but about SQLAlchemy. And while we can't assume a future maintainer of your code knows all the in and outs of SQLAlchemy as you now do, it's the job of their documentation to teach them that, not you. :3
| goal = validate_model(Goal, goal_id) | ||
|
|
||
| request_body = request.get_json() | ||
|
|
||
| goal.title = request_body["title"] | ||
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response({"goal": goal.to_dict()}, 200) |
There was a problem hiding this comment.
Minor Style Nitpick: These blank lines feel a little extraneous, actually making the code harder to read, in my opinion. I would group the first three lines together as a "block" since they are all dealing with updating the title:
goal = validate_model(Goal, goal_id)
request_body = request.get_json()
goal.title = request_body["title"]
db.session.commit()
return make_response({"goal": goal.to_dict()}, 200)I know in other cases the code here would be complicated enough in each block that you separated them, but sometimes if each block of code is one line or less it ends up feeling just too spacious as it does in this case.
| task_list = [] | ||
| for id in task_ids: | ||
| task = Task.query.get(id) | ||
| task_list.append(task) | ||
| goal.tasks = task_list |
There was a problem hiding this comment.
Note that this would blow away any existing Tasks associated with the Goal, which may not be desirable.
We actually don't specify it in the requirements, so this is fine, but I think in a lot of cases, the preference would be that this would be adding the passed in tasks to that goal, in addition to the tasks that already are part of that goal. So you would have to append them instead.
| @classmethod | ||
| def from_dict(cls, goal_data): | ||
| new_task = Goal(title=goal_data["title"]) | ||
| return new_task |
There was a problem hiding this comment.
Note that a @classmethod like this takes in a cls variable that represents the class type, which is Goal in this case. And that means you can actually use cls as the constructor here:
@classmethod
def from_dict(cls, goal_data):
new_task = cls(title=goal_data["title"])
return new_taskIt's very minor, but the benefit is that if you ever rename the Goal class, you won't have to change it here also. Not life changing, but nice to have.
| @tasks_bp.route("/<task_id>", methods=["PUT"]) | ||
| def update_task(task_id): | ||
| task = validate_model(Task, task_id) | ||
|
|
||
| request_body = request.get_json() | ||
|
|
||
| task.title = request_body["title"] | ||
| task.description = request_body["description"] | ||
|
|
||
| db.session.commit() | ||
|
|
||
| return make_response({"task": task.to_dict()}, 200) |
There was a problem hiding this comment.
Yes, for example this spacing is fine here. I'm guessing this is likely the code you copy-pasted for the same function in Goal, and here just that one extra line on line 65 makes this feel less arbitrarily spaced.
| #send notif to Slack API | ||
| params = {"channel": "task-notifications", "text": f"Someone just completed the task {task.title}"} | ||
| header = {"Authorization": f"Bearer {os.environ.get('SLACKBOT_AUTH_TOKEN')}"} | ||
| requests.post("https://slack.com/api/chat.postMessage", params=params, headers=header) |
There was a problem hiding this comment.
Surprisingly I'm fine with the comment here even though it's a "what" comment. This code is pretty hairy compared to everything else, so it needs a little describing.
My preference would be to handle that hairiness in a helper function, the name of which would serve much of the same purpose as your comment here. So something like send_slack_notification.
But given that this is the only place it gets used, I am fine with just a comment.
| @@ -2,7 +2,7 @@ | |||
| import pytest | |||
|
|
|||
|
|
|||
| @pytest.mark.skip(reason="No way to test this feature yet") | |||
| #@pytest.mark.skip(reason="No way to test this feature yet") | |||
There was a problem hiding this comment.
I would recommend getting rid of these skip decorators rather than commenting them out. There's no real reason to keep them around now that you've implemented everything so they can be removed to keep the code clean.
| def test_get_task_not_found(client): | ||
| # Act | ||
| response = client.get("/tasks/1") | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| assert response.status_code == 404 | ||
| assert response_body == {"message": f"Task 1 not found"} |
There was a problem hiding this comment.
All your asserts in this file are fine, though!
| def test_update_goal(client, one_goal): | ||
| raise Exception("Complete test") | ||
| # Act | ||
| # ---- Complete Act Here ---- | ||
| response = client.put("/goals/1", json={ | ||
| "title": "My Updated Goal" | ||
| }) | ||
| response_body = response.get_json() | ||
|
|
||
| # Assert | ||
| # ---- Complete Assertions Here ---- | ||
| # assertion 1 goes here | ||
| # assertion 2 goes here | ||
| # assertion 3 goes here | ||
| # ---- Complete Assertions Here ---- | ||
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "My Updated Goal" | ||
| } | ||
| } |
No description provided.