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

Added basic Django password validations #136

Merged
merged 1 commit into from May 11, 2016

Conversation

@JRodDynamite
Copy link
Contributor

JRodDynamite commented May 8, 2016

Partially fixes #113. Do let me know if there are any issues with the code.

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 8, 2016

Thanks a lot! I will leave some comments in the code.

@@ -3,6 +3,9 @@
from django.utils.translation import ugettext_lazy as _

from pretix.base.models import User
from django.contrib.auth.password_validation import (
validate_password, password_validators_help_texts
)

This comment has been minimized.

Copy link
@raphaelm

raphaelm May 8, 2016

Member

We use isort to keep imports deterministically ordered. Just run isort -rc pretix on the codebase and those will be ordered alphabetically.

@@ -68,6 +71,7 @@ class RegistrationForm(forms.Form):
error_messages = {
'duplicate_email': _("You already registered with that e-mail address, please use the login form."),
'pw_mismatch': _("Please enter the same password twice"),
'pw_invalid': _("Please enter a valid password")

This comment has been minimized.

Copy link
@raphaelm

raphaelm May 8, 2016

Member

I think this line is unnecessary as we never access it because password_validators_help_texts() already gives a human-readable message.

@@ -107,7 +117,8 @@ def clean_email(self):

class PasswordRecoverForm(forms.Form):
error_messages = {
'pw_mismatch': _("Please enter the same password twice")
'pw_mismatch': _("Please enter the same password twice"),
'pw_invalid': _("Please enter a valid password")

This comment has been minimized.

Copy link
@raphaelm

raphaelm May 8, 2016

Member

Same as above

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 8, 2016

I left a few small comments, otherwise this is totally perfect. Thanks especially for the extensive tests. Less tests would probably have sufficed since a lot of this is tested in Django itself, but more tests are always good.

The flake8 style checker yields ./pretix/control/views/auth.py:164:102: W291 trailing whitespace, the line seems to contain a space character at the end.

The test suite timed out on travis (this sadly happens sometimes), but locally it fails only due to something that is not your fault and that I just fixed in 749e44f on the master branch. So its only the stylistic issues mentioned above and in the comments that are left to be fixed :)

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 8, 2016

There is something else I missed on first sight: You changed RegistrationForm and PasswordRecoverForm but there is also a UserSettingsForm. Could you change that as well?

@JRodDynamite

This comment has been minimized.

Copy link
Contributor Author

JRodDynamite commented May 8, 2016

Thanks for the quick and detailed response. 👍 Will fix them and and push it up. :)

@JRodDynamite

This comment has been minimized.

Copy link
Contributor Author

JRodDynamite commented May 8, 2016

Hey @raphaelm, are there no test cases written for UserSettingsForm? Can't find them.

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 8, 2016

There are a few in tests/control/test_user.py. There surely is room for improvement in the organization of the test files.

@JRodDynamite

This comment has been minimized.

Copy link
Contributor Author

JRodDynamite commented May 9, 2016

Hey @raphaelm, I've updated my code. Made the corrections as requested. Let me know if I've still missed out on anything. :)

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 9, 2016

Looks near perfect. I have only one very minor thing to say: You put the new validation logic in UserSettingsForm.clean_new_pw_repeat. This means, if the input is wrong, the "password repeat" field will be red and has the error attached. From an UI point of view, think it would be better to attach the error to the password field itself, e.g. clean_new_pw. Dou you agree?

@JRodDynamite

This comment has been minimized.

Copy link
Contributor Author

JRodDynamite commented May 9, 2016

Okay. Understood. Will make the necessary changes.

@JRodDynamite JRodDynamite force-pushed the JRodDynamite:pwd-validation branch from 76f4303 to a9ce20a May 10, 2016
@JRodDynamite

This comment has been minimized.

Copy link
Contributor Author

JRodDynamite commented May 11, 2016

Hey @raphaelm I'm not sure why that one test case failed in the build. Could you let me know why that happened?

Also, I added 2 entries in the .gitignore file since, I use Eclipse. I hope its alright.

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 11, 2016

It's probably a false negative, as it is totally unrelated. It is one of the Selenium interface tests and sadly, Selenium is very unstable and flaky which gets even worse on Travis.

I'll try locally later today and then merge this PR.

@raphaelm

This comment has been minimized.

Copy link
Member

raphaelm commented May 11, 2016

Everything looks fine to me now. Thank you very very much for this contribution, I really appreciate it!

@raphaelm raphaelm merged commit e685f8e into pretix:master May 11, 2016
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@JRodDynamite JRodDynamite deleted the JRodDynamite:pwd-validation branch May 12, 2016
@JRodDynamite JRodDynamite restored the JRodDynamite:pwd-validation branch May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.