TBD: access mixins: support for custom exceptions #145

Closed
wants to merge 6 commits into
from

Projects

None yet

3 participants

@blueyed
Contributor
blueyed commented Aug 9, 2014

I wanted to use a custom exception with SuperuserRequiredMixin (Http404) and added support for it.

raise_exception can now be True, False, a custom exception or a callable (that should return a response).

I have then factored the no-permission handling into a separate function, and moved redirect_unauthenticated_users to the base mixin.

This does not include any documentation changes yet, because I wanted to gather feedback on it first.

E.g., I am not sure if handle_no_permission should fall back to raising an exception for any non-HTTP-response (not isinstance(ret, (HttpResponse, StreamingHttpResponse))).

@blueyed
Contributor
blueyed commented Sep 30, 2014

I would appreciate some feedback on this.

E.g. is redirect_unauthenticated_users still useful / correctly named? Because now it's used to fallback to / call no_permissions_fail.

@kennethlove
Member

This definitely needs docs. I'm not sure why you deleted the AnonymousUser test, though? That should still pass based on the changes to AccessMixin, shouldn't it?

We also definitely want to make sure the response is a valid HTTPResponse or StreamingHTTPResponse since Django requires a valid responses from views.

As for the naming of redirect_unauthenticated_users, that has been around for awhile, so if we did decide to rename/remove it, we'd need to go through a deprecation process. I'd say it should probably be renamed to redirect_invalid_users if anything.

@chrisjones-brack3t
Member

Yep, that removed test does pass. All tests pass for me after merging in master. I'd like to get a new release out tomorrow or at the latest early next week. If you can get this together with docs it'll make it into the 1.5 release. Otherwise I'll push it back to 1.6.

@blueyed
Contributor
blueyed commented Dec 5, 2014

@chrisjones-brack3t
Great. I'll probably not get to it until tomorrow, but will do so in the next days.

@blueyed
Contributor
blueyed commented Dec 9, 2014

I'm not sure why you deleted the AnonymousUser test, though?

If you mean test_anonymous_redirects, this has been moved (e294f01).

I'll leave redirect_unauthenticated_users as-is for now.

blueyed added some commits Aug 9, 2014
@blueyed blueyed Support for custom `raise_exception` (Exception or callable) 2d2e0b0
@blueyed blueyed Refactor no-perm handling into `handle_no_permission`
 - Add `handle_no_permission`.
 - Move `no_permissions_fail` and `redirect_unauthenticated_users` to
   base AccessMixin.
e7dc1fc
@blueyed blueyed tests: move/rename `test_anonymous_redirects`
The `redirect_unauthenticated_users` attribute has moved to the base
class.

Also add explicit tests for `redirect_unauthenticated_users=False`.
4cae126
@blueyed blueyed Check for HttpResponse/StreamingHttpResponse in `handle_no_permission` c9c06b4
@blueyed blueyed Update docs, pass `request` to callable `raise_exception` 5c272ea
@blueyed
Contributor
blueyed commented Dec 9, 2014

I've rebased the branch and updated the documentation: only the .rst for now, I am not sure about the class documentation - should this get added to the base class, or to all mixins, like it's currently done, or just both?

I've also changed it to pass the request argument to the raise_exception callback. Good?

@blueyed
Contributor
blueyed commented Apr 17, 2015

@kennethlove
Can you please provide some feedback to my last comment?
I can (try to) rebase it again, of course.

@kennethlove kennethlove referenced this pull request Apr 17, 2015
Merged

Pr/145 #173

@blueyed blueyed deleted the blueyed:custom-exceptions branch Apr 19, 2015
@blueyed
Contributor
blueyed commented Apr 19, 2015

Thanks for merging!

However, it is missing docs probably and some questions are unanswered.
Should be followed up in #173 though anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment