review #5
review #5
Conversation
app/auth/views.py
Outdated
| def post(self): | ||
| # Handle POST request for this view. Url ---> /auth/register""" | ||
| # Query to see if the user already exists | ||
| post_data = request.get_json(force=True) |
There was a problem hiding this comment.
Look into how to use reqeuest.json instead of request.get_json to help with data validation.
There was a problem hiding this comment.
It looks like request.json in deprecated in flask versions greater than 0.11
pallets/flask#1421
wtforms/flask-wtf#245
There was a problem hiding this comment.
@dennisja thank you for that. Had no idea and it looks like the get functionality has been added to.get_json().
app/auth/views.py
Outdated
| # Register new user | ||
| username = post_data['username'] | ||
| fullname = post_data['fullname'] | ||
| password = post_data['password'] |
There was a problem hiding this comment.
The assignment in lines 45-47 can be handled immediately after defining post. then use the variables in all your queries.
Then it would be better to use username = request.json.get('username') and password = request.json.get('password') to avoid errors if one of the keys is missing in the request
There was a problem hiding this comment.
@code-sleuth as per the concern raised by @dennisja it would be better to use username = post_data.get('username') and password = post_data.get('password') instead of extracting the data using keys.
app/auth/views.py
Outdated
| response = { | ||
| 'message': str(ex) | ||
| } | ||
| return make_response(jsonify(response)), 401 |
app/auth/views.py
Outdated
| 'message': 'User exists. Login' | ||
| } | ||
|
|
||
| return make_response(jsonify(response)), 202 |
There was a problem hiding this comment.
Why 202?....Status code 202 means the request has been accepted.
http://www.restapitutorial.com/httpstatuscodes.html
| else: | ||
| # User does not exist. Therefore, we return an error message | ||
| response = { | ||
| 'message': 'Invalid username or password' |
There was a problem hiding this comment.
This message is confusing. It would be better to inform the user they should register first before trying to login.
app/recipe_blue_print/views.py
Outdated
| recipe_blue_print.add_url_rule('/recipes/<int:id>', view_func=recipes_view_edit, methods=['DELETE', 'PUT', 'GET']) | ||
| # Define the rule for recipes url ---> /recipes/search?q=<sting:q>&limit=<int:limit>&page=<int:page>' | ||
| recipe_blue_print.add_url_rule('/recipes/search', view_func=search_by_name, methods=['GET']) | ||
|
|
There was a problem hiding this comment.
Make use of class based views i.e. MethodView makes code much cleaner.
Look into how to cover the method for /recipes/search url under the method for /recipes url
app/users_blue_print/views.py
Outdated
| })), 200 | ||
|
|
||
| users_blue_print.add_url_rule('/users', view_func=users, methods=['GET']) | ||
| users_blue_print.add_url_rule('/users/<int:id>', view_func=edit_by_id, methods=['DELETE', 'PUT', 'GET']) |
There was a problem hiding this comment.
Is this file, app/users_blue_print/views.py, necessary?
|
|
||
| if __name__ == '__main__': | ||
| app.run() | ||
| app.run(debug=True, port=5005) |
There was a problem hiding this comment.
The file manage.py makes this file(main_app.py) redundant.
|
|
||
|
|
||
| def downgrade(): | ||
| ${downgrades if downgrades else "pass"} |
There was a problem hiding this comment.
Delete and gitignore the migrations folder and its contents.
| else: | ||
| return None | ||
|
|
||
|
|
There was a problem hiding this comment.
Place line 8-27 in a different file. You can look into how to make a decorator. It'll help you avoid repetition every time you access endpoints.
|
Changes Unknown when pulling 73144b3 on dev into ** on master**. |
|
Changes Unknown when pulling 827b55e on dev into ** on master**. |
|
Changes Unknown when pulling 36e851f on dev into ** on master**. |
No description provided.