Tigers- Lindsey B. #126
Tigers- Lindsey B. #126
Conversation
There was a problem hiding this comment.
Good Work!
While I point out several issues and there's definitely a lot of cleanup and polish that can be applied to this code, I am confident that you were at least on the right path and applying your knowledge well. As such I am marking this Green!
I'm guessing the deadline caught up with you and you submitted this a bit early, but that's fine, that's exactly what I asked you to do, so this is fine. If you think any of my comments would be particularly important to go back over, feel free, but I think what's here is good enough to not be Yellow.
| @@ -1,3 +1,4 @@ | |||
| from dotenv import load_dotenv | |||
There was a problem hiding this comment.
Not sure how this got added, it's a duplicate of line 6's import statement. I figure it was just an accidental extension add.
| return cls( | ||
| title=req_body['title'], | ||
| description=req_body['description'], | ||
| #TODO completed_at=req_body['completed_at'] |
| def __str__(self): | ||
| return "Task" | ||
|
|
||
| def __repr__(self): | ||
| return "Task" | ||
|
|
There was a problem hiding this comment.
This is also good. These type of functions are often overlooked but setting a good value here can be very helpful when debugging in a REPL as well as other cases.
Though, these particular implementation of these aren't the best in that they just print "Task" rather than using the attributes of self to print more information. But even thinking about them at this point is worth praising.
(Also, if you're confused about the difference between __str__ and __repr__, like I was, this GeeksForGeeks page explained it pretty well.)
| def to_new_dict(self): | ||
| return { | ||
| "title": self.title, | ||
| "description": self.description, | ||
| "is_complete": self.completed_at != None, | ||
| "id": self.task_id, | ||
| "goal_id": self.goal_id | ||
| } |
There was a problem hiding this comment.
It's kind of odd to make a whole separate to_new_dict function here rather than updating to_dict to handle adding the "goal_id" key. I cloned your code to check if there was another reason, but that was the main use of it in your GET /tasks/<task_id> endpoint.
By forcing the call to to_new_dict I did see that a test failed because of the "goal_id" key. But then that's something that can be handled by saving the dictionary to a variable and then adding the"goal_id" key in an if block:
def to_dict(self):
new_dict = {
"title": self.title,
"description": self.description,
"is_complete": self.completed_at != None,
"id": self.task_id,
}
if self.goal_id:
new_dict["goal_id"] = self.goal_id
return new_dictPlus, this extra function gets copied over to Goal, presumably when you copy-pasted Task to create that class. There's nothing wrong with doing that, I do it constantly, but do make sure to check because the Goal.to_new_dict function does not ever get used, so there is no need for it.
| @@ -1,5 +1,36 @@ | |||
| from app import db | |||
| from flask import Blueprint, make_response, request, jsonify, abort, request | |||
|
|
|||
|
|
|||
|
|
|||
| class Goal(db.Model): | |||
There was a problem hiding this comment.
Though, I will mention that your base implementation for both your model classes, Task and Goal is very good overall. I like your pattern for writing the update methods, it's a great use of except.
There was a problem hiding this comment.
Aw, I just noticed you didn't get a chance to use the update methods in the routes. Well, just consider this a comment on how you're on the right track!
| "details": f'Task {task_id} "{task_dict["title"]}" successfully deleted'} | ||
|
|
||
| #MARK COMPLETE | ||
| #MODIFY MARK COMPLETE TO CALL SLACK API |
There was a problem hiding this comment.
You did!
(Probably just an overlooked TODO comment.)
|
|
||
| return jsonify(response) | ||
|
|
||
| ### GOAL ROUTES |
There was a problem hiding this comment.
I like the use of multiple #s in these headings. Gets the point across without being too fancy.
| response.append(task.to_new_dict()) | ||
|
|
||
| return {"id": goal.goal_id, "title": goal.title, "tasks": response} | ||
|
|
| def test_create_task(client): | ||
| # TODO revert task id 3 to 1 | ||
| task_id = 1 |
There was a problem hiding this comment.
This is a reasonable thing to do in tests by the way!
For example, you could do it with all the test data like this, and putting it in constants:
def test_create_task(client):
# Arrange
EXPECTED_TASK_ID = 1
EXPECTED_TITLE = "A Brand New Task"
EXPECTED_DESCRIPTION = "Test Description
# Act
response = client.post("/tasks", json={
"title": EXPECTED_TITLE,
"description": EXPECTED_DESCRIPTION,
})
response_body = response.get_json()
# Assert
assert response.status_code == 201
assert "task" in response_body
assert response_body == {
"task": {
"id": EXPECTED_TASK_ID,
"title": EXPECTED_TITLE,
"description": EXPECTED_DESCRIPTION,
"is_complete": False
}
}Well, you could if you wrote the test. I know we wrote this test, but I just wanted to point this pattern out.
| assert response.status_code == 200 | ||
| assert "goal" in response_body | ||
| assert response_body == { | ||
| "goal": { | ||
| "id": 1, | ||
| "title": "Updated Goal Title" | ||
| } | ||
| } | ||
| goal = Goal.query.get(1) | ||
| assert goal.title == "Updated Goal Title" |
There was a problem hiding this comment.
Good test design by checking in the DB also!
No description provided.