Allow permission classes to customize the failure message. by donewell · Pull Request #2539 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
change detail to message and update text
| @@ -280,6 +282,8 @@ def check_permissions(self, request): |
|---|
| """ |
| for permission in self.get_permissions(): |
| if not permission.has_permission(request, self): |
| if hasattr(permission, 'message'): |
| self.permission_denied(request, permission.message) |
| self.permission_denied(request) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simplify this to self.permission_denied(request, message=getattr(permission, 'message', None))?
donewell added 2 commits
Thanks. Both suggestions worked and helped to simplify the code. I also made a small change to the custom message used in the tests.
@tomchristie This seems like a good and simple addition, but perhaps it should go on 3.1. Thoughts?
@jpadilla : Still pending a decision, but it doesn't matter that the pull request is against master - we're not planning on any further 3.0 releases so we'd be fine to just merge both this and the 3.1 branch to master.
Given that we don't do this uniformly throughout other exception classes let's not just do this in a single place.
The validator API uses the same convention. At the moment it's very brittle to create permissions with custom messages, so it would be worth thinking about other ways we could address this. The most natural, elegant solutions involve some kind of metaprogramming.
Have you considered an approach where an exception takes a permission as a parameter? At the moment default_details are implemented on both the permission and exception.
Reopening for later review.
tomchristie added a commit that referenced this pull request
add message to custom permission
add message to custom permission Allow permission classes to customize the failure message.
This was referenced
Mar 9, 2017
This was referenced
Oct 6, 2017
This was referenced
Oct 16, 2017
This was referenced
Nov 6, 2017
This was referenced
Nov 14, 2017
This was referenced
Dec 10, 2017