Submit Task List #121
Submit Task List #121
Conversation
There was a problem hiding this comment.
Hi Hoku! I can tell that you've already learned a fair amount about RESTful APIs and are off to a good start.
I've had a chance to review your task-list-api project and left comments about how you can refactor it so that your code can be more readable, concise, and have robust error handling.
There was a problem hiding this comment.
Thanks for revisiting this project and refactoring it based on the previous comments. I left a some more comments this time.
A couple were about writing test assertions and making sure that you compare one value to another for a test so that we can ensure the method is actually being tested.
I did see that you had an extra route that Flask won't be able to match (see line 116 in goal_routes.py).
Also, pay close attention to your indentation. There are languages that are white space agnostic, but Python is not one of them. Python very much depends on correct indentation to know when code should be executed (think about your for loops and if/else statements).
Lastly, I see you leveraged your vaidate_model method (nice work!) but then still have some additional checks to see if you have a valid object, which is unnecessary. Have a look at the comment I left in the method read_one_goal, I'd like to make sure that this concept is firmed up.
|
|
||
| request_body = request.get_json() | ||
|
|
||
| if "title" in request_body: |
There was a problem hiding this comment.
In your POST request for goals, instead of checking if "title" is in request_body, you check to see if "title" is absent from the request body. If it is absent, then you returned an error message to let the client know that the data was invalid.
That's the more appropriate logic for routes that use client data to create or update entities in the database. So on line 60, we should invert the logic to check if "title" is missing and if it is, you would return an error message.
If "title" is present, then you can update goal.title, commit the change, and return the updated goal
| goal.title = request_body["title"] | ||
| db.session.commit() | ||
| response_body = { | ||
| "goal": goal.to_dict_goal() | ||
| } |
There was a problem hiding this comment.
Watch your indentation here. Lines 62 and 63 will execute even if "title" is not in request_body since they not indented inside the body if the if statement.
| @bp.route("/<goal_id>", methods=["DELETE"]) | ||
| def delete_goal(goal_id): | ||
| goal = validate_model(Goal, goal_id) | ||
| if goal: |
There was a problem hiding this comment.
No need to check if goal, the validate_model() method will return a valid goal if it doesn't return an error
| @@ -0,0 +1,15 @@ | |||
| from flask import abort, make_response, jsonify | |||
|
|
|||
| def validate_model(cls, model_id): | |||
There was a problem hiding this comment.
Nice job pulling this into a separate file and importing it in two different places. A suggestion for the file name is to call it helper.py
| assert Task.query.get(1)== None | ||
| assert Task.query.all() == [] | ||
| assert "message" in response_body | ||
| assert response_body == {"message": "Task #1 was not found"} |
|
|
||
|
|
||
| # Added assertion | ||
| assert "'message': 'Task #1 was not found' in response_body" |
There was a problem hiding this comment.
This assertion has no comparison since there's no == in this line of code. You have the assert keyword and then a string. Since the string is not empty, it will evaluate to a Truthy value so the test will pass, but you're not actually testing anything.
This assertion could be written like
assert response_body == {"message": "Task #1 was not found' in response_body"}| # **Complete test with assertion about response body*************** | ||
| # ***************************************************************** | ||
| # Added assertions | ||
| assert "'message': 'Task #1 was not found' in response_body" |
There was a problem hiding this comment.
Same comment as above here -- this line is not comparing anything since it's the assert keyword and a string. For a test to test something there needs to be something compared to something else.
This assertion could be written as
assert response_body == {"message": "Task #1 was not found' in response_body"}| assert response.status_code == 404 | ||
| #Added assertions | ||
| assert "message" in response_body | ||
| assert response_body == {"message": "Goal #1 was not found"} |
No description provided.