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

catch ImproperlyConfigured exc in user_login view #85

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

pacahon
Copy link
Contributor

@pacahon pacahon commented Aug 6, 2019

No description provided.

@pacahon
Copy link
Contributor Author

pacahon commented Aug 6, 2019

Not sure it's really necessary but we could save some LOC if use one try-catch block both for checking permissions and login action.

@pacahon
Copy link
Contributor Author

pacahon commented Aug 6, 2019

Btw I've fixed this line accidentally

raise ImproperlyConfigured("Could not find an appropriate authentication backend")
Let me know to revert it if I'm wrong. I'm not good at English :)

"""
An authentication backend is a class that implements two required methods: get_user(user_id) and
authenticate(**credentials). Unfortunately, some libraries don't comply with this interface (e.g.
`django-rules` with ObjectPermissionBackend) and omit required `get_user` method.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be "omit the required" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@skorokithakis
Copy link
Owner

This looks good to me, apart from the minor grammar error. If you've tested this in the admin and it works properly, I'm good to merge!

@skorokithakis skorokithakis merged commit 497540c into skorokithakis:master Aug 8, 2019
@skorokithakis
Copy link
Owner

Merged, thanks!

@pacahon
Copy link
Contributor Author

pacahon commented Aug 8, 2019

I've tested it on my own project with custom auth system, works fine. Also I can test it manually on bare django project with default configuration if the tests are not enough.

@skorokithakis
Copy link
Owner

If you get a chance, please do, as it would be good to know it doesn't break anything in the default configuration, but it's not urgent. Thanks for the help!

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.

None yet

2 participants