-
Notifications
You must be signed in to change notification settings - Fork 0
Implement activities model #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
|
||
| response = client.get("/activities/%s" % invalid_id, follow_redirects=True) | ||
|
|
||
| assert 422 == response.status_code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of Status codes use constants if you don't mind :D
| class Activities(Resource): | ||
| @ns.doc('list_activities') | ||
| @ns.marshal_list_with(activity, code=200) | ||
| @ns.marshal_list_with(activity, code=HTTPStatus.OK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by default it is OK. So this is redundant.
| return activity_dao.get_all(), HTTPStatus.OK | ||
|
|
||
| @ns.doc('create_activity') | ||
| @ns.response(HTTPStatus.BAD_REQUEST, 'Bad request') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to put a message, lets better put one customized, like:
Invalid format or structure of the attributes of the activity
Also, it would be nice to specify that a CONFLICT may occur if there is already an activity with the same name
@ns.response(HTTPStatus.CONFLICT, 'This activity already exists')
| class Activity(Resource): | ||
| @ns.doc('get_activity') | ||
| @ns.marshal_with(activity) | ||
| @ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The id has an invalid format') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of here, or you will have to repeat it every time.
| def delete(self, id): | ||
| return None, 204 | ||
| """Get an activity""" | ||
| return activity_dao.get(id), HTTPStatus.OK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary to specify OK as a result. By default it returns 200.
| @ns.doc('put_activity') | ||
| @ns.response(400, 'Invalid format of the attributes of the activity.') | ||
| @ns.doc('update_activity') | ||
| @ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The data has an invalid format.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to remove UNPROCESSABLE_ENTITY (it would be in the class) and add:
@ns.response(HTTPStatus.BAD_REQUEST, 'Invalid format or structure of the activity')
@ns.response(HTTPStatus.CONFLICT, 'An activity already exists with this new data')
| return activity_dao.update(id, ns.payload) | ||
|
|
||
| @ns.doc('delete_activity') | ||
| @ns.response(HTTPStatus.UNPROCESSABLE_ENTITY, 'The id has an invalid format') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the UNPROCESSABLE_ENTITY. It will be in the class.
EliuX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KUDOS
This PR includes :
I reused most of the implementation of projects. Thanks @EliuX for your help on it.