-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat: Create base password validator per user type #64
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r1, all commit messages.
Reviewable status: 1 of 7 files reviewed, 31 unresolved discussions (waiting on @faucomte97)
codeforlife/settings/django.py
line 107 at r1 (raw file):
# TODO: replace with custom validators: # codeforlife.user.auth.password_validators.CommonPasswordValidator
Do we still need a custom common-password validator if django includes one out the box?
codeforlife/settings/django.py
line 116 at r1 (raw file):
}, { "NAME": "django.contrib.auth.password_validation.NumericPasswordValidator",
Don't think we need the numeric validator.
From django docs: "checks whether the password isn’t entirely numeric."
We already do this in our custom validators.
codeforlife/user/auth/password_validators/__init__.py
line 1 at r1 (raw file):
from .dependent_student import DependentStudentPasswordValidator
add module docstring
codeforlife/user/auth/password_validators/__init__.py
line 2 at r1 (raw file):
from .dependent_student import DependentStudentPasswordValidator from .independent_student import IndependentStudentPasswordValidator
now that we've remodelled the data, let's use the following terminology to denote our 3 user types:
teacher - self explanatory
student - this will only ever refer to a school student
independent - this will only ever refer to an independent learner. We can use "indy" for short variable names (but not class/function names)
codeforlife/user/auth/password_validators/dependent_student.py
line 1 at r1 (raw file):
from django.core.exceptions import ValidationError
add module docstring and rename file to student.py
codeforlife/user/auth/password_validators/dependent_student.py
line 4 at r1 (raw file):
class DependentStudentPasswordValidator:
Rename to StudentPasswordValidator
codeforlife/user/auth/password_validators/dependent_student.py
line 7 at r1 (raw file):
def __init__(self): self.min_length = 6 self.help_text = (
use the gettext util
"from django.utils.translation import gettext as _"
See example: https://docs.djangoproject.com/en/3.2/topics/auth/passwords/#writing-your-own-validator
codeforlife/user/auth/password_validators/dependent_student.py
line 11 at r1 (raw file):
) def validate(self, password, user=None):
- create a base password validator in base.py
- defined method signature for validate() and add type hints for arguments. raise a NotImplementedError() in the base.validate
- inherit base validator here.
- ...
- profit
codeforlife/user/auth/password_validators/dependent_student.py
line 13 at r1 (raw file):
def validate(self, password, user=None): if len(password) < self.min_length: raise ValidationError(self.help_text, code="password_not_valid")
code="password_too_short"
codeforlife/user/auth/password_validators/independent_student.py
line 1 at r1 (raw file):
import re
add module docstring and rename file to independent.py
codeforlife/user/auth/password_validators/independent_student.py
line 6 at r1 (raw file):
class IndependentStudentPasswordValidator:
rename to IndependentPasswordValidator
codeforlife/user/auth/password_validators/independent_student.py
line 9 at r1 (raw file):
def __init__(self): self.min_length = 8 self.help_text = (
use gettext util
codeforlife/user/auth/password_validators/independent_student.py
line 11 at r1 (raw file):
self.help_text = ( f"Your password must contain at least {self.min_length} characters, 1 uppercase character, " f"1 lowercase character and 1 digit.",
you've made this a tuple of strings, not a string. does your IDE inform you of these type errors?
codeforlife/user/auth/password_validators/independent_student.py
line 14 at r1 (raw file):
) def validate(self, password, user=None):
see previous comment about adding a base validator
codeforlife/user/auth/password_validators/independent_student.py
line 21 at r1 (raw file):
and re.search(r"[0-9]", password) ): raise ValidationError(self.help_text, code="password_not_valid")
on second thought, let's split this into multiple if checks. for each, raise a specific error message as to why that specific if check failed and give each a unique error code. This will give pointed help text to the user but more importantly, allow us to unit test specific error codes.
codeforlife/user/auth/password_validators/teacher.py
line 1 at r1 (raw file):
import re
add module docstring
codeforlife/user/auth/password_validators/teacher.py
line 10 at r1 (raw file):
self.min_length = 10 self.help_text = ( f"Your password must contain at least {self.min_length} characters, 1 uppercase character, "
line too long. max chars = 80
codeforlife/user/auth/password_validators/teacher.py
line 11 at r1 (raw file):
self.help_text = ( f"Your password must contain at least {self.min_length} characters, 1 uppercase character, " f"1 lowercase character, 1 digit and 1 special character.",
tuple of strings, not a string
codeforlife/user/auth/password_validators/teacher.py
line 14 at r1 (raw file):
) def validate(self, password, user=None):
see previous comment about adding a base validator
codeforlife/user/auth/password_validators/teacher.py
line 22 at r1 (raw file):
and re.search(r"[!@#$%^&*()_+\-=\[\]{};':\"\\|,.<>\/?]", password) ): raise ValidationError(self.help_text, code="password_not_valid")
on second thought, let's split this into multiple if checks. for each, raise a specific error message as to why that specific if check failed and give each a unique error code. This will give pointed help text to the user but more importantly, allow us to unit test specific error codes.
codeforlife/user/tests/auth/password_validators/__init__.py
line 0 at r1 (raw file):
add module docstring
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 1 at r1 (raw file):
import pytest
add module docstring
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 1 at r1 (raw file):
import pytest
need to split this test case into 3 separate test cases, since we have 3 separate validators.
We should have 3 files called:
- test_teacher.py
- test_student.py
- test_independent.py
The file structure of the tests need to match exactly the file structure of the code.
This will simplify finding tests.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 1 at r1 (raw file):
import pytest
no need to import pytest. django's test case provides all the helper functions you require. just do self.helperNameHere()
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 13 at r1 (raw file):
class TestPasswordValidators(TestCase): password_no_special_char = "kR48SsAwrE"
all of these class-level attributes should be placed in setupClass()
@classmethod
def setUpClass(cls):
cls.some_variable = "hi"
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 26 at r1 (raw file):
] def test_teacher_password_validator(self):
create a unit_test per error code with naming convention:
test_validate__{error_code}
for example:
test_validate__password_too_short(self):
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 31 at r1 (raw file):
validator = TeacherPasswordValidator() with pytest.raises(ValidationError):
replace: with self.assertRaises
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 32 at r1 (raw file):
with pytest.raises(ValidationError): [
no need to create a list instance. replace with standard for loop
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 33 at r1 (raw file):
with pytest.raises(ValidationError): [ validator.validate(bad_password)
we need to assert the specific error code raised to make sure the validation failed for the right reason.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 47 at r1 (raw file):
validator = IndependentStudentPasswordValidator() with pytest.raises(ValidationError):
see previous feedback about not using pytest, using a for loop and asserting specific error codes.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 61 at r1 (raw file):
validator = DependentStudentPasswordValidator() with pytest.raises(ValidationError):
see previous feedback about not using pytest, and asserting specific error codes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 7 files reviewed, 34 unresolved discussions (waiting on @faucomte97)
codeforlife/user/auth/password_validators/dependent_student.py
line 12 at r1 (raw file):
def validate(self, password, user=None): if len(password) < self.min_length:
you need to check user type
codeforlife/user/auth/password_validators/independent_student.py
line 15 at r1 (raw file):
def validate(self, password, user=None): if not (
you need to check user type
codeforlife/user/auth/password_validators/teacher.py
line 15 at r1 (raw file):
def validate(self, password, user=None): if not (
you need to check user type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r2, all commit messages.
Reviewable status: 1 of 7 files reviewed, 25 unresolved discussions (waiting on @faucomte97)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 of 7 files reviewed, 25 unresolved discussions (waiting on @faucomte97)
codeforlife/settings/django.py
line 107 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Do we still need a custom common-password validator if django includes one out the box?
I was wondering that, I guess we could have a deeper look and see exactly how it works maybe, but I should think we can trust it.
If for some reason we want to keep our custom common password validator (with the logic to ping pwnd then we can, but needs more investigation I'd say).
codeforlife/settings/django.py
line 116 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Don't think we need the numeric validator.
From django docs: "checks whether the password isn’t entirely numeric."
We already do this in our custom validators.
Done.
codeforlife/user/auth/password_validators/__init__.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
add module docstring
Done.
codeforlife/user/auth/password_validators/__init__.py
line 2 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
now that we've remodelled the data, let's use the following terminology to denote our 3 user types:
teacher - self explanatory
student - this will only ever refer to a school student
independent - this will only ever refer to an independent learner. We can use "indy" for short variable names (but not class/function names)
Agreed, I hate the sound of "dependent" student anyway. Done.
codeforlife/user/auth/password_validators/teacher.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
add module docstring
Done.
codeforlife/user/auth/password_validators/teacher.py
line 10 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
line too long. max chars = 80
Black didn't automatically change this - am I supposed to manually keep track?
codeforlife/user/auth/password_validators/teacher.py
line 11 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
tuple of strings, not a string
Cf comment above
codeforlife/user/auth/password_validators/teacher.py
line 15 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
you need to check user type
Same question as before
codeforlife/user/tests/auth/password_validators/__init__.py
line at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
add module docstring
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
add module docstring
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
need to split this test case into 3 separate test cases, since we have 3 separate validators.
We should have 3 files called:
- test_teacher.py
- test_student.py
- test_independent.py
The file structure of the tests need to match exactly the file structure of the code.
This will simplify finding tests.
I felt like having a separate test case for each validator was going to lead to a lot of repeated code and annoyance having to repeat & copy everything, which is why I favoured a one-file approach. Does the file structure of the test absolutely always have to match the file structure, if it's just gonna lead to repeated code and more setup?...
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 13 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
all of these class-level attributes should be placed in setupClass()
@classmethod
def setUpClass(cls):
cls.some_variable = "hi"
I'm a bit fuzzy on this, what's the difference / benefit of this?
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 31 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
replace: with self.assertRaises
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 32 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
no need to create a list instance. replace with standard for loop
why not, aren't list comprehensions preferred when possible?
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 47 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
see previous feedback about not using pytest, using a for loop and asserting specific error codes.
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 61 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
see previous feedback about not using pytest, and asserting specific error codes.
Done.
codeforlife/user/auth/password_validators/dependent_student.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
add module docstring and rename file to student.py
Done.
codeforlife/user/auth/password_validators/dependent_student.py
line 4 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Rename to StudentPasswordValidator
Done.
codeforlife/user/auth/password_validators/dependent_student.py
line 7 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
use the gettext util
"from django.utils.translation import gettext as _"
See example: https://docs.djangoproject.com/en/3.2/topics/auth/passwords/#writing-your-own-validator
We've never really cared about translation strings, are we planning on doing this going forward then?
codeforlife/user/auth/password_validators/dependent_student.py
line 12 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
you need to check user type
Why?
codeforlife/user/auth/password_validators/dependent_student.py
line 13 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
code="password_too_short"
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
add module docstring and rename file to independent.py
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 6 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
rename to IndependentPasswordValidator
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 9 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
use gettext util
Cf question above
codeforlife/user/auth/password_validators/independent_student.py
line 11 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
you've made this a tuple of strings, not a string. does your IDE inform you of these type errors?
It's just a multiline f-string not a tuple of strings. Cf a random example of it from codeforlife-portal below.
(Notice the lack of comma between the different f-strings).
Code snippet:
def resetEmailPasswordMessage(request, domain, uid, token, protocol):
password_reset_uri = reverse_lazy("password_reset_check_and_confirm", kwargs={"uidb64": uid, "token": token})
url = f"{protocol}://{domain}{password_reset_uri}"
return {
"subject": f"Password reset request",
"message": (
f"You are receiving this email because you requested "
f"a password reset for your Code For Life user account.\n\n"
f"Please go to the following page and choose a new password: {url}"
),
}
codeforlife/user/auth/password_validators/independent_student.py
line 15 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
you need to check user type
Same question as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 7 files at r2.
Reviewable status: 2 of 7 files reviewed, 22 unresolved discussions (waiting on @faucomte97)
codeforlife/settings/django.py
line 107 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
I was wondering that, I guess we could have a deeper look and see exactly how it works maybe, but I should think we can trust it.
If for some reason we want to keep our custom common password validator (with the logic to ping pwnd then we can, but needs more investigation I'd say).
modify todo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 2 of 12 files reviewed, 22 unresolved discussions (waiting on @faucomte97 and @SKairinos)
codeforlife/settings/django.py
line 107 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
modify todo
Done.
codeforlife/user/auth/password_validators/teacher.py
line 10 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Black didn't automatically change this - am I supposed to manually keep track?
Done.
codeforlife/user/auth/password_validators/teacher.py
line 11 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Cf comment above
Done.
codeforlife/user/auth/password_validators/teacher.py
line 14 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
see previous comment about adding a base validator
Done.
codeforlife/user/auth/password_validators/teacher.py
line 15 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Same question as before
Done.
codeforlife/user/auth/password_validators/teacher.py
line 22 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
on second thought, let's split this into multiple if checks. for each, raise a specific error message as to why that specific if check failed and give each a unique error code. This will give pointed help text to the user but more importantly, allow us to unit test specific error codes.
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 1 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
no need to import pytest. django's test case provides all the helper functions you require. just do self.helperNameHere()
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 13 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
I'm a bit fuzzy on this, what's the difference / benefit of this?
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 26 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
create a unit_test per error code with naming convention:
test_validate__{error_code}
for example:
test_validate__password_too_short(self):
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 32 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
why not, aren't list comprehensions preferred when possible?
Done.
codeforlife/user/tests/auth/password_validators/test_password_validators.py
line 33 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
we need to assert the specific error code raised to make sure the validation failed for the right reason.
Done.
codeforlife/user/auth/password_validators/dependent_student.py
line 7 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
We've never really cared about translation strings, are we planning on doing this going forward then?
Done.
codeforlife/user/auth/password_validators/dependent_student.py
line 11 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
- create a base password validator in base.py
- defined method signature for validate() and add type hints for arguments. raise a NotImplementedError() in the base.validate
- inherit base validator here.
- ...
- profit
Done.
codeforlife/user/auth/password_validators/dependent_student.py
line 12 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Why?
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 9 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Cf question above
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 11 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
It's just a multiline f-string not a tuple of strings. Cf a random example of it from codeforlife-portal below.
(Notice the lack of comma between the different f-strings).
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 14 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
see previous comment about adding a base validator
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 15 at r1 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Same question as before
Done.
codeforlife/user/auth/password_validators/independent_student.py
line 21 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
on second thought, let's split this into multiple if checks. for each, raise a specific error message as to why that specific if check failed and give each a unique error code. This will give pointed help text to the user but more importantly, allow us to unit test specific error codes.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @faucomte97)
codeforlife/user/auth/password_validators/base.py
line 11 at r3 (raw file):
class BasePasswordValidator:
rename to just "PasswordValidator" and add a one-liner class-docstring explain it's a base class
codeforlife/user/auth/password_validators/teacher.py
line 16 at r3 (raw file):
class TeacherPasswordValidator(BasePasswordValidator): def __init__(self): self.min_length = 10
remove init and put variable directly in validate()
codeforlife/user/tests/auth/password_validators/password_validator.py
line 1 at r3 (raw file):
"""
rename file to base.py
codeforlife/user/tests/auth/password_validators/password_validator.py
line 38 at r3 (raw file):
def __exit__(self, *args, **kwargs): value = self.context.__exit__(*args, **kwargs) print(self.context)
remove print
codeforlife/user/tests/auth/password_validators/test_independent.py
line 14 at r3 (raw file):
@classmethod def setUpClass(cls): cls.user = User.objects.filter(
"assert cls.user is not None"
need to check there is a first user
codeforlife/user/tests/auth/password_validators/test_student.py
line 14 at r3 (raw file):
@classmethod def setUpClass(cls): cls.user = User.objects.filter(new_student__isnull=False).first()
"assert cls.user is not None"
need to check there is a first user
codeforlife/user/tests/auth/password_validators/test_teacher.py
line 14 at r3 (raw file):
@classmethod def setUpClass(cls): cls.user = User.objects.first()
"assert cls.user is not None"
need to check there is a first user
codeforlife/user/tests/auth/password_validators/test_teacher.py
line 14 at r3 (raw file):
@classmethod def setUpClass(cls): cls.user = User.objects.first()
cls.user = User.objects.filter(new_teacher__isnull=False).first()
codeforlife/user/auth/password_validators/independent.py
line 16 at r3 (raw file):
class IndependentPasswordValidator(BasePasswordValidator): def __init__(self): self.min_length = 8
remove init and put variable directly in validate()
codeforlife/user/auth/password_validators/student.py
line 14 at r3 (raw file):
class StudentPasswordValidator(BasePasswordValidator): def __init__(self): self.min_length = 6
remove init and put variable directly in validate()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 8 files at r4, all commit messages.
Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @faucomte97)
codeforlife/user/auth/password_validators/base.py
line 15 at r4 (raw file):
"""Base class for all password validators""" # pylint: disable-next=missing-class-docstring
I think you meant: "# pylint: disable-next=missing-function-docstring"
codeforlife/user/tests/auth/password_validators/test_independent.py
line 23 at r4 (raw file):
def test_validate__password_too_short(self): """Check password validator rejects too short password"""
Small thing but only pointing it out now since we'll be writing so many unit tests going forward.
There's no need to add pretext in each test description ("Checks password validator rejects") .
This is because it's clear what the test case is. I already know you're testing the "IndependentPasswordValidator" from the name of the test case class.
Therefore, just describe what the unit is testing. In this example, something like """Password cannot be too short."""
codeforlife/user/tests/auth/password_validators/test_student.py
line 21 at r4 (raw file):
def test_validate__password_too_short(self): """Check password validator rejects too short password"""
see comment about removing pretext
codeforlife/user/tests/auth/password_validators/test_teacher.py
line 21 at r4 (raw file):
def test_validate__password_too_short(self): """Check password validator rejects too short password"""
see comment about removing pretext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 8 of 12 files reviewed, 4 unresolved discussions (waiting on @SKairinos)
codeforlife/user/auth/password_validators/base.py
line 11 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
rename to just "PasswordValidator" and add a one-liner class-docstring explain it's a base class
Done.
codeforlife/user/auth/password_validators/base.py
line 15 at r4 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
I think you meant: "# pylint: disable-next=missing-function-docstring"
Done.
codeforlife/user/auth/password_validators/teacher.py
line 16 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
remove init and put variable directly in validate()
Done.
codeforlife/user/tests/auth/password_validators/test_independent.py
line 14 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
"assert cls.user is not None"
need to check there is a first user
Done.
codeforlife/user/tests/auth/password_validators/test_independent.py
line 23 at r4 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Small thing but only pointing it out now since we'll be writing so many unit tests going forward.
There's no need to add pretext in each test description ("Checks password validator rejects") .
This is because it's clear what the test case is. I already know you're testing the "IndependentPasswordValidator" from the name of the test case class.
Therefore, just describe what the unit is testing. In this example, something like """Password cannot be too short."""
Done.
codeforlife/user/tests/auth/password_validators/test_student.py
line 14 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
"assert cls.user is not None"
need to check there is a first user
Done.
codeforlife/user/tests/auth/password_validators/test_student.py
line 21 at r4 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
see comment about removing pretext
Done.
codeforlife/user/tests/auth/password_validators/test_teacher.py
line 14 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
"assert cls.user is not None"
need to check there is a first user
Done.
codeforlife/user/tests/auth/password_validators/test_teacher.py
line 14 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
cls.user = User.objects.filter(new_teacher__isnull=False).first()
Done.
codeforlife/user/tests/auth/password_validators/test_teacher.py
line 21 at r4 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
see comment about removing pretext
Done.
codeforlife/user/auth/password_validators/independent.py
line 16 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
remove init and put variable directly in validate()
Done.
codeforlife/user/auth/password_validators/student.py
line 14 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
remove init and put variable directly in validate()
Done.
codeforlife/user/tests/auth/password_validators/password_validator.py
line 1 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
rename file to base.py
Done.
codeforlife/user/tests/auth/password_validators/password_validator.py
line 38 at r3 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
remove print
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)
This change is