search

mozilla / django-browserid

Django application for adding BrowserID support.
Mozilla Public License 2.0
180 stars 80 forks source link

Allow usage of named urls for redirection URLs in the settings. #299

Closed abompard closed 8 years ago

abompard commented 8 years ago

Since Django 1.7, the *_REDIRECT_URL settings can be named URLs, to reduce configuration duplication. This change teaches Django BrowserID about this possibility.

Documentation: https://docs.djangoproject.com/en/1.7/ref/settings/#login-url

Osmose commented 8 years ago

Thanks for the PR! I think this is a good thing to merge in (for however long it'll be useful, given Persona impending doom).

The only thing missing beyond the other comments I already made are updates to the docs explaining what types of values these settings can take. Django's line "This setting also accepts view function names and named URL patterns which can be used to reduce configuration duplication since you don’t have to define the URL in two places (settings and URLconf)." is a fine thing to just tack onto the docs.

Otherwise, this looks pretty good. Nice work!

Osmose commented 8 years ago

Oh and the test failure isn't your fault, I think I might just remove Python 3.2 compat before doing the point release to add this. No one should be using 3.2 anyway.

abompard commented 8 years ago

Thanks for your review @Osmose , I've updated the pull request accordingly.

Osmose commented 8 years ago

r+, merged. Nice work! I'll be cutting a release shortly.

Osmose commented 8 years ago

2.0.2 is out and should include this fix. Thanks again!