-
Notifications
You must be signed in to change notification settings - Fork 148
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
Configurable response messages #65
base: master
Are you sure you want to change the base?
Configurable response messages #65
Conversation
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.
Thanks for your PR.
Your change is api-breaking, e.g.:
Instead of
return Response({'status': 'OK'})
you use
response_dict = dict({"status_code": None, "message": None})
message = get_response_message("PASSWORD_CHANGED")
response_dict.update({"status_code": 200, "message": message})
return Response(response_dict)
which results in the response {"status_code": 200, "message": "..."}
instead of {"status": "OK"}
.
Please re-introduce the "status"
field.
Furthermore, please rename the PR accordingly, and add the "documentation" that is currently in |
re-introduces the |
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 have done all requested change
…figurable-response Django rest passwordreset configurable response
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.
Sorry the answer to your PR took so long! There are a few further change-requests, mainly about language. If these are adressed and all new conflicts are resolved, we can finally merge your PR.
configurable message text changes, get_response_message function intr…
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.
Perfect! Thank you for the changes! There's one last merge-conflict that needs to be resolved - could you fix that too so we can finalize this?
…passwordreset into django-rest-passwordreset-configurable-response
…ps://github.com/surajraktate/django-rest-passwordreset into django-rest-passwordreset-configurable-response
sorry for commit without testing, now I can see all checks passed, Thanks |
I make changes in view.py to make responses configurable please review.