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

feat: Create base password validator per user type #64

Merged
merged 11 commits into from
Feb 1, 2024
Merged

Conversation

faucomte97
Copy link
Contributor

@faucomte97 faucomte97 commented Jan 29, 2024

This change is Reviewable

@faucomte97 faucomte97 self-assigned this Jan 29, 2024
Copy link
Contributor

@SKairinos SKairinos left a 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):
  1. create a base password validator in base.py
  2. defined method signature for validate() and add type hints for arguments. raise a NotImplementedError() in the base.validate
  3. inherit base validator here.
  4. ...
  5. 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:

  1. test_teacher.py
  2. test_student.py
  3. 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.

Copy link
Contributor

@SKairinos SKairinos left a 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

Copy link
Contributor

@SKairinos SKairinos left a 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)

Copy link
Contributor Author

@faucomte97 faucomte97 left a 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:

  1. test_teacher.py
  2. test_student.py
  3. 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

Copy link
Contributor

@SKairinos SKairinos left a 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

Copy link
Contributor Author

@faucomte97 faucomte97 left a 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…
  1. create a base password validator in base.py
  2. defined method signature for validate() and add type hints for arguments. raise a NotImplementedError() in the base.validate
  3. inherit base validator here.
  4. ...
  5. 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.

Copy link
Contributor

@SKairinos SKairinos left a 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()

Copy link
Contributor

@SKairinos SKairinos left a 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

Copy link
Contributor Author

@faucomte97 faucomte97 left a 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.

Copy link
Contributor

@SKairinos SKairinos left a 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

@faucomte97 faucomte97 merged commit fb47ef5 into main Feb 1, 2024
4 checks passed
@faucomte97 faucomte97 deleted the reset_password branch February 1, 2024 10:04
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.

2 participants