Skip to content
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

Allow permission classes to customize the failure message. #2539

Merged
merged 3 commits into from Jun 24, 2015

Conversation

donewell
Copy link

@donewell donewell commented Feb 10, 2015

No description provided.

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)
Copy link
Member

@tomchristie tomchristie Feb 11, 2015

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
Copy link
Author

donewell commented Feb 11, 2015

Thanks. Both suggestions worked and helped to simplify the code. I also made a small change to the custom message used in the tests.

@jpadilla
Copy link
Member

jpadilla commented Feb 13, 2015

@tomchristie This seems like a good and simple addition, but perhaps it should go on 3.1. Thoughts?

@tomchristie
Copy link
Member

tomchristie commented Feb 13, 2015

@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.

@tomchristie
Copy link
Member

tomchristie commented Mar 3, 2015

Given that we don't do this uniformly throughout other exception classes let's not just do this in a single place.

@tomchristie tomchristie closed this Mar 3, 2015
@donewell
Copy link
Author

donewell commented Mar 3, 2015

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.

@donewell
Copy link
Author

donewell commented Mar 3, 2015

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.

@tomchristie tomchristie reopened this Mar 3, 2015
@tomchristie
Copy link
Member

tomchristie commented Mar 3, 2015

Reopening for later review.

@tomchristie tomchristie added this to the 3.1.4 Release milestone Jun 24, 2015
@tomchristie
Copy link
Member

tomchristie commented Jun 24, 2015

Sure.

tomchristie added a commit that referenced this issue Jun 24, 2015
@tomchristie tomchristie merged commit 8329411 into encode:master Jun 24, 2015
2 checks passed
@tomchristie tomchristie changed the title add message to custom permission Allow permission classes to customize the failure message. Jun 24, 2015
@tomchristie tomchristie modified the milestones: 3.1.4 Release, 3.2.0 Release Jul 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants