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

Minor improvements proposal #4

Open
wants to merge 4 commits into
base: auth-cbv
Choose a base branch
from

Conversation

benoitbryon
Copy link

Hi,

I started to work on django#502, but then I discovered the auth-cbv branch... So, let's contribute to it!

Here is a proposal for some minor improvements in the auth-cbv branch:

  • moved some implementation logic into methods (such as LoginView.set_test_cookie) so that, if one wants to inherit from LoginView (that's my use case), it's easier to perform fine-grained overrides.
  • converted is_valid_redirect() to a validator.
  • used success_url in LoginView too.

Then, here are some notes about the branch status...

  • We currently pass (i.e. fail silently) when LoginView or LogoutView have empty success_url. Is that ok? I mean, shouldn't we raise ImproperlyConfigured instead?
  • We don't validate settings.LOGIN_REDIRECT_URL or views' success_url. Is that ok? I know it's safer than input from the request, but shouldn't we use at least some URLValidator?
  • contrib/auth/views.py changed since last "rebase" (actually a merge). Could be useful to update.

@benoitbryon
Copy link
Author

contrib/auth/views.py changed since last "rebase" (actually a merge). Could be useful to update.

I said it because I couldn't run the tests on my computer when switching to the auth-cbv branch, whereas tests run successfully with Django's master branch:

>>> git checkout 2b09e576210ab8761310e6daa494bf5016618d65
>>> ./runtests.py --settings=test_sqlite --failfast
Traceback (most recent call last):
  ...
  TypeError: 'class Meta' got invalid attribute(s): index_together

I'm sorry, I didn't run the tests :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant