Handle join class requests

[faucomte97]

This change is 

[SKairinos]

Reviewed 13 of 16 files at r1, all commit messages.
Reviewable status: 13 of 16 files reviewed, 12 unresolved discussions (waiting on @faucomte97)

---

backend/api/fixtures/independent_school_1_class_1_join_request.json line 8 at r1 (raw file):

      "user": 28,
      "new_user": 28,
      "pending_class_request": 7

this is actually class 2 @ school 1. Please rename file accordingly.

---

backend/api/serializers/klass.py line 104 at r1 (raw file):

        )

    def update(self, instance, validated_data):

the problem with this update function is we are unable to update any other fields. let's discuss on call

---

backend/api/serializers/klass.py line 105 at r1 (raw file):

    def update(self, instance, validated_data):
        # TODO: Decide how to handle this field. Currently, teachers select a

comment not needed. the FE will be responsible for sending the datetime. if the user selects a number of hours, the FE will compute the final datetime (now + 3 hours) and send it the backend.

---

backend/api/views/user.py line 53 at r1 (raw file):

            if "requesting_to_join_class" in self.request.data:
                return [IsIndependent()]
        if self.action == "handle_join_class_request":

delete this if. add the action name to the list above (in ["bulk", ...])

---

backend/api/views/user.py line 227 at r1 (raw file):

    )
    def handle_join_class_request(
        self, request: Request, pk: t.Optional[str] = None

pk is not optional. The url path won't match if pk is not provided. for example, it has to be

/api/users/1/handle-join-class-request

---

backend/api/views/user.py line 232 at r1 (raw file):

        Handles an independent user's request to join a class. First tries to
        retrieve the independent user, then the class they're requesting to
        join. The teacher handling the request must either be an admin the

an admin in the

---

backend/api/views/user.py line 240 at r1 (raw file):

        """
        try:
            indy = User.objects.get(pk=int(pk))

replace this entire try block with

user = self.get_object()

DRF handles all of this for you in that function.

---

backend/api/views/user.py line 247 at r1 (raw file):

            )

        klass = Class.objects.get(

add this as validation in a custom serializer. let's do some pair programming.

---

backend/api/views/user.py line 261 at r1 (raw file):

            )

        if (

should be a serializer validation. let's do some pair programming.

---

backend/api/views/user.py line 295 at r1 (raw file):

        if request_accepted:
            try:
                if not isinstance(request.data["first_name"], str):

should be a serializer validation. let's do some pair programming.

---

backend/api/views/user.py line 307 at r1 (raw file):

            name = request.data["first_name"]
            if self._is_name_unique_in_class(name, klass):

should be a serializer validation. let's do some pair programming.

---

backend/api/views/user.py line 313 at r1 (raw file):

                username = None

                while (

let's make this a helper function on the StudentUser object so we're not repeating logic.

```StudentUser.generate_username()``

[faucomte97]

Reviewable status: 13 of 16 files reviewed, 12 unresolved discussions (waiting on @SKairinos)

---

backend/api/serializers/klass.py line 104 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

the problem with this update function is we are unable to update any other fields. let's discuss on call

Don't we have the same problem with the other update functions then?

---

backend/api/views/user.py line 227 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

pk is not optional. The url path won't match if pk is not provided. for example, it has to be

/api/users/1/handle-join-class-request

Why did we make pk optional in the other custom actions then?

[faucomte97]

Reviewable status: 13 of 16 files reviewed, 12 unresolved discussions (waiting on @SKairinos)

---

backend/api/fixtures/independent_school_1_class_1_join_request.json line 8 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

this is actually class 2 @ school 1. Please rename file accordingly.

Done.

---

backend/api/serializers/klass.py line 105 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

comment not needed. the FE will be responsible for sending the datetime. if the user selects a number of hours, the FE will compute the final datetime (now + 3 hours) and send it the backend.

Done.

---

backend/api/views/user.py line 53 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

delete this if. add the action name to the list above (in ["bulk", ...])

Done.

---

backend/api/views/user.py line 232 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

an admin in the

Done.

---

backend/api/views/user.py line 240 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

replace this entire try block with

user = self.get_object()

DRF handles all of this for you in that function.

Why don't we do that in the reset_password action then?

[faucomte97]

Reviewable status: 8 of 17 files reviewed, 12 unresolved discussions (waiting on @faucomte97 and @SKairinos)

---

backend/api/views/user.py line 247 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

add this as validation in a custom serializer. let's do some pair programming.

Done.

---

backend/api/views/user.py line 261 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

should be a serializer validation. let's do some pair programming.

Done.

---

backend/api/views/user.py line 295 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

should be a serializer validation. let's do some pair programming.

Done.

---

backend/api/views/user.py line 307 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

should be a serializer validation. let's do some pair programming.

Done.

---

backend/api/views/user.py line 313 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

let's make this a helper function on the StudentUser object so we're not repeating logic.

```StudentUser.generate_username()``

Done.

[SKairinos]

Reviewed 4 of 9 files at r2, all commit messages.
Reviewable status: 12 of 17 files reviewed, 35 unresolved discussions (waiting on @faucomte97)

---

backend/Pipfile line 10 at r2 (raw file):

#   https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile
# Replace "{ref}" in the above URL with the ref set below.
codeforlife = {ref = "fixtures_fix", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"}

replace with latest version

---

backend/Pipfile line 37 at r2 (raw file):

#   https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile
# Replace "{ref}" in the above URL with the ref set below.
codeforlife = {ref = "fixtures_fix", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]}

replace with latest version

---

backend/api/serializers/user.py line 96 at r2 (raw file):

    teacher = TeacherSerializer(source="new_teacher", required=False)
    requesting_to_join_class = serializers.CharField(
        required=False, allow_blank=True

instead of allow_blank=True, rather don't allow blank but make it nullable. If you're not requesting to join a class, it will be null. Let's generally agree to use null to mean "no value" for all fields (so not "" for char fields).

---

backend/api/serializers/user.py line 134 at r2 (raw file):

            if self.instance.teacher:
                if "last_name" in attrs and not attrs["last_name"]:
                    raise serializers.ValidationError(

any errors that you reuse should be saved in a variable

last_name_required_error = serializers.ValidationError(
    "Last name is required.", code="last_name_required"
 )
...
raise last_name_required_error

---

backend/api/serializers/user.py line 139 at r2 (raw file):

                if "requesting_to_join_class" in attrs:
                    raise serializers.ValidationError(

any errors that you reuse should be saved in a variable

---

backend/api/serializers/user.py line 146 at r2 (raw file):

            if self.instance.student:
                if self.view.action != "reset_password":
                    if self.request.student_user.pk != self.instance.pk:

should not check this here. instead, in get_queryset() you should filter/exclude the instances. The instance passed to the serializer should come from the valid-queryset. Therefore, there's no need to do this validation in the serializer. Let me know if I missed something.

---

backend/api/serializers/user.py line 159 at r2 (raw file):

                    and "class_field" in attrs["new_student"]
                ):
                    raise serializers.ValidationError(

any errors that you reuse should be saved in a variable

---

backend/api/serializers/user.py line 165 at r2 (raw file):

                    )

        else:  # Creating

please add this validation here

if "new_teacher" in attrs and "new_student" in attrs:
    raise serializers.ValidationError("...", code="teacher_and_student")

feel free to set whatever message and code you feel appropriate

---

backend/api/serializers/user.py line 167 at r2 (raw file):

        else:  # Creating
            if "new_teacher" in attrs:
                if "last_name" not in attrs or attrs["last_name"] is None:

can just simplify to not attrs.get("last_name"). keep in mind last_name != "" also. you are only checking None in your code.

---

backend/api/serializers/user.py line 169 at r2 (raw file):

                if "last_name" not in attrs or attrs["last_name"] is None:
                    raise serializers.ValidationError(
                        "Last name is required.", code="last_name_required"

side note (nothing to do) here we do need to prefix the code with "last_name" because we are raising from inside validate().

---

backend/api/serializers/user.py line 178 at r2 (raw file):

                    )

            if "new_student" in attrs:

should be in elif since you can only be a teacher or an independent

---

backend/api/serializers/user.py line 211 at r2 (raw file):

    def validate_requesting_to_join_class(self, value: str):
        # NOTE: Error message is purposefully ambiguous to prevent class

Why does it matter if they can enumerate classes? It's not sensitive data (unlike email addresses)

---

backend/api/serializers/user.py line 213 at r2 (raw file):

        # NOTE: Error message is purposefully ambiguous to prevent class
        # enumeration.
        error_message = "Class does not exist or does not accept join requests."

(don't make these changes until we discuss the above question)

replace these 2 lines with

error = serializers.ValidationError(
"Class does not exist or does not accept join requests.",
code="does_not_exist_or_accept_join_requests"
)

then below raise error or raise error from ex

---

backend/api/serializers/user.py line 269 at r2 (raw file):

            # If value is empty, this means independent wants to revoke their
            # join request
            if requesting_to_join_class == "":

make None instead, avoid "" for char fields to denote "no value"

---

backend/api/serializers/user.py line 269 at r2 (raw file):

            # If value is empty, this means independent wants to revoke their
            # join request
            if requesting_to_join_class == "":

simplify with an inline if-else check

instance.student.pending_class_request = None if requesting_to_join_class is None else Class.objects.get(
                    access_code=requesting_to_join_class
                )

---

backend/api/serializers/user.py line 280 at r2 (raw file):

            #  join request.

            instance.student.save()

instance.student.save(update_fields=["pending_class_request"])

also save should come straight after instance.student.pending_class_request = ...

---

backend/api/serializers/user.py line 352 at r2 (raw file):

            "accept",
        ]

remove space

---

backend/api/serializers/user.py line 353 at r2 (raw file):

        ]

        extra_kwargs = {

you need to include the inherited extra_kwargs

**_UserSerializer.Meta.extra_kwargs

---

backend/api/serializers/user.py line 362 at r2 (raw file):

    def validate_first_name(self, value: str):
        if Student.objects.filter(

Use StudentUser instead:

if StudentUser.objects.filter(
            new_student__class_field=self.instance.student.pending_class_request,
            first_name__iexact=value,
        ).exists():

After this, can the Student import removed?

---

backend/api/serializers/user.py line 369 at r2 (raw file):

                "This name already exists in the class. "
                "Please choose a different name.",
                code="first_name_in_class",

can change to something like "already_in_class".

No need to prefix the code with "first_name". We know the code is tied to first_name because we are raising this inside validate_first_name()

---

backend/api/serializers/user.py line 388 at r2 (raw file):

            instance.email = ""

            instance.student.save(

relocate save to straight after these lines

instance.student.class_field = (
                instance.student.pending_class_request
            )
            instance.student.pending_class_request = None

---

backend/api/serializers/user.py line 391 at r2 (raw file):

                update_fields=["class_field", "pending_class_request"]
            )
            instance.save(

relocate save to straight after these lines

instance.username = StudentUser.get_random_username()
            instance.first_name = validated_data.get(
                "first_name", instance.first_name
            )
            instance.last_name = ""
            instance.email = ""

---

backend/api/serializers/user.py line 393 at r2 (raw file):

            instance.save(
                update_fields=["username", "first_name", "last_name", "email"]
            )

let's add a TODO to send them an acceptance email. even if we don't do this in the current system, let's get around to doing it at a later point.

[faucomte97]

Reviewable status: 10 of 17 files reviewed, 35 unresolved discussions (waiting on @faucomte97 and @SKairinos)

---

backend/Pipfile line 10 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

replace with latest version

Done.

---

backend/Pipfile line 37 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

replace with latest version

Done.

---

backend/api/serializers/user.py line 96 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

instead of allow_blank=True, rather don't allow blank but make it nullable. If you're not requesting to join a class, it will be null. Let's generally agree to use null to mean "no value" for all fields (so not "" for char fields).

Done.

---

backend/api/serializers/user.py line 134 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

any errors that you reuse should be saved in a variable

last_name_required_error = serializers.ValidationError(
    "Last name is required.", code="last_name_required"
 )
...
raise last_name_required_error

Done.

---

backend/api/serializers/user.py line 139 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

any errors that you reuse should be saved in a variable

Done.

---

backend/api/serializers/user.py line 146 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

should not check this here. instead, in get_queryset() you should filter/exclude the instances. The instance passed to the serializer should come from the valid-queryset. Therefore, there's no need to do this validation in the serializer. Let me know if I missed something.

Done.

---

backend/api/serializers/user.py line 159 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

any errors that you reuse should be saved in a variable

Done.

---

backend/api/serializers/user.py line 165 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

please add this validation here

if "new_teacher" in attrs and "new_student" in attrs:
    raise serializers.ValidationError("...", code="teacher_and_student")

feel free to set whatever message and code you feel appropriate

Done.

---

backend/api/serializers/user.py line 167 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

can just simplify to not attrs.get("last_name"). keep in mind last_name != "" also. you are only checking None in your code.

Done.

---

backend/api/serializers/user.py line 169 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

side note (nothing to do) here we do need to prefix the code with "last_name" because we are raising from inside validate().

Done.

---

backend/api/serializers/user.py line 178 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

should be in elif since you can only be a teacher or an independent

Done.

---

backend/api/serializers/user.py line 211 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Why does it matter if they can enumerate classes? It's not sensitive data (unlike email addresses)

You're right but class codes are used to login and enumerating existing class codes means attackers can reduce the range of login details used to brute force login.

---

backend/api/serializers/user.py line 213 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

(don't make these changes until we discuss the above question)

replace these 2 lines with

error = serializers.ValidationError(
"Class does not exist or does not accept join requests.",
code="does_not_exist_or_accept_join_requests"
)

then below raise error or raise error from ex

See reply above

---

backend/api/serializers/user.py line 269 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

make None instead, avoid "" for char fields to denote "no value"

Done.

---

backend/api/serializers/user.py line 269 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

simplify with an inline if-else check

instance.student.pending_class_request = None if requesting_to_join_class is None else Class.objects.get(
                    access_code=requesting_to_join_class
                )

I would but we need the else block for the emails we'll be sending out. I could do the inline check now but we'll have to revert it down the line anyway

---

backend/api/serializers/user.py line 280 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

instance.student.save(update_fields=["pending_class_request"])

also save should come straight after instance.student.pending_class_request = ...

Done.

---

backend/api/serializers/user.py line 352 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

remove space

Done.

---

backend/api/serializers/user.py line 353 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

you need to include the inherited extra_kwargs

**_UserSerializer.Meta.extra_kwargs

Done, but why? Your serializer above didn't need it.

---

backend/api/serializers/user.py line 362 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Use StudentUser instead:

if StudentUser.objects.filter(
            new_student__class_field=self.instance.student.pending_class_request,
            first_name__iexact=value,
        ).exists():

After this, can the Student import removed?

Done, but no 😞

---

backend/api/serializers/user.py line 369 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

can change to something like "already_in_class".

No need to prefix the code with "first_name". We know the code is tied to first_name because we are raising this inside validate_first_name()

Done.

---

backend/api/serializers/user.py line 388 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

relocate save to straight after these lines

instance.student.class_field = (
                instance.student.pending_class_request
            )
            instance.student.pending_class_request = None

Done.

---

backend/api/serializers/user.py line 391 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

relocate save to straight after these lines

instance.username = StudentUser.get_random_username()
            instance.first_name = validated_data.get(
                "first_name", instance.first_name
            )
            instance.last_name = ""
            instance.email = ""

Done.

---

backend/api/serializers/user.py line 393 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

let's add a TODO to send them an acceptance email. even if we don't do this in the current system, let's get around to doing it at a later point.

Done.

[SKairinos]

Reviewed 3 of 10 files at r3, all commit messages.
Reviewable status: 9 of 17 files reviewed, 15 unresolved discussions (waiting on @faucomte97)

---

backend/api/serializers/user.py line 211 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

You're right but class codes are used to login and enumerating existing class codes means attackers can reduce the range of login details used to brute force login.

fair point

---

backend/api/serializers/user.py line 213 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

See reply above

still need to make error a reusable variable

---

backend/api/serializers/user.py line 269 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I would but we need the else block for the emails we'll be sending out. I could do the inline check now but we'll have to revert it down the line anyway

fair point

---

backend/api/serializers/user.py line 353 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done, but why? Your serializer above didn't need it.

this is mistake on my part. it should be in the one above also.

---

backend/api/serializers/user.py line 129 at r3 (raw file):

        )

        teacher_cannot_request_join_class_error = serializers.ValidationError(

make var name match code

---

backend/api/serializers/user.py line 137 at r3 (raw file):

            "Student can't be in a class and requesting to join a "
            "class at the same time.",
            code="class_and_join_class_mutually_exclusive",

make var name match code

[SKairinos]

Reviewable status: 9 of 17 files reviewed, 25 unresolved discussions (waiting on @faucomte97)

---

backend/api/serializers/user.py line 213 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

still need to make error a reusable variable

actually, although the error message is reusable, the code is not. The code should be unique for each error scenario. then, please assert each code in the tests. error codes are for internal use only so it's not a security risk.

---

backend/api/serializers/user.py line 126 at r3 (raw file):

    def validate(self, attrs):
        last_name_required_error = serializers.ValidationError(
            "Last name is required.", code="last_name_required"

error_code="last_name__required"

since we're in the general validate(), we should make the error code follow the naming convention:

"{field_name}__{error_code}"

in theory, if we were raising this error inside validate_last_name(), the error code could just be "required"

---

backend/api/serializers/user.py line 129 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

make var name match code

also error code should following naming convention = "requesting_to_join_class__teacher". In other words, the error is that a teacher is requesting to join a class.

var name should be "requesting_to_join_class__teacher_error"

---

backend/api/serializers/user.py line 137 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

make var name match code

also error code should following naming convention = "requesting_to_join_class__student". In other words, the error is that a student is requesting to join a class.

var name should be "requesting_to_join_class__student_error"

---

backend/api/serializers/user.py line 147 at r3 (raw file):

            if self.instance.teacher:
                if "last_name" in attrs and not attrs["last_name"]:

on second thought, there's no need to do this check. we've already specified last_name must have a min_length=1 and last_name is not a nullable field. the error probably isn't reused anymore so we can just put it inline below.

---

backend/api/serializers/user.py line 153 at r3 (raw file):

                    raise teacher_cannot_request_join_class_error

            if self.instance.student:

make elif. can only be teacher or student - no need to check both

---

backend/api/tests/serializers/test_user.py line 82 at r3 (raw file):

        )

    def test_validate__teacher__first_name_required(self):

rename to "test_validate__create__teacher__last_name_required"
you are testing:

1. the validate func
2. when a user is created
3. when a teacher-user is created
4. a teacher-user's last name is required during creation (this must be the exact error code)

please update all the validate test names to have the same naming convention.

"test_validate__{create/update}{user_type}{error_code}"

---

backend/api/tests/serializers/test_user.py line 83 at r3 (raw file):

    def test_validate__teacher__first_name_required(self):
        """Teacher must have a first name"""

Not fully describing the test case. Something like:

"""Teacher-user's last_name is required during creation."""

Also, you said first name. You meant last name.

Please update all test docstring to fully describe the test case.

---

backend/api/tests/serializers/test_user.py line 89 at r3 (raw file):

    def test_validate__teacher__requesting_to_join_class_forbidden(self):
        """Teacher cannot request to join a class"""

see above comment about test naming convention and docstring missing details

---

backend/api/tests/serializers/test_user.py line 102 at r3 (raw file):

        self,
    ):
        """Student cannot have both requesting_to_join_class and class_field"""

see above comment about test naming convention and docstring missing details. Also, describe the test case/scenario rather than how it's implemented. For example:

"Student-user cannot request to join a class."

---

backend/api/tests/serializers/test_user.py line 112 at r3 (raw file):

    def test_validate_requesting_to_join_class__does_not_exist(self):
        """Join class request cannot be for a class that doesn't exist"""

see above comment about test naming convention and docstring missing details

---

backend/api/tests/serializers/test_user.py line 121 at r3 (raw file):

    def test_validate_requesting_to_join_class__does_not_accept_requests(self):
        """
        Join class request cannot be for a class that doesn't accept requests

see above comment about test naming convention and docstring missing details

---

backend/api/tests/serializers/test_user.py line 131 at r3 (raw file):

    def test_validate_requesting_to_join_class__no_longer_accept_requests(self):
        """
        Join class request cannot be for a class that no longer accepts requests

see above comment about test naming convention and docstring missing details

[faucomte97]

Reviewable status: 9 of 17 files reviewed, 25 unresolved discussions (waiting on @faucomte97 and @SKairinos)

---

backend/api/serializers/user.py line 213 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

actually, although the error message is reusable, the code is not. The code should be unique for each error scenario. then, please assert each code in the tests. error codes are for internal use only so it's not a security risk.

Done.

---

backend/api/serializers/user.py line 126 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

error_code="last_name__required"

since we're in the general validate(), we should make the error code follow the naming convention:

"{field_name}__{error_code}"

in theory, if we were raising this error inside validate_last_name(), the error code could just be "required"

Done.

---

backend/api/serializers/user.py line 129 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

also error code should following naming convention = "requesting_to_join_class__teacher". In other words, the error is that a teacher is requesting to join a class.

var name should be "requesting_to_join_class__teacher_error"

Done.

---

backend/api/serializers/user.py line 137 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

also error code should following naming convention = "requesting_to_join_class__student". In other words, the error is that a student is requesting to join a class.

var name should be "requesting_to_join_class__student_error"

The error isn't strictly that a student is requesting to join a class, the error is that a student who already is in a class is requesting to join a class or that a student who is requesting to join a class is being edited to now be in a class. Not sure what name to give then

---

backend/api/serializers/user.py line 147 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

on second thought, there's no need to do this check. we've already specified last_name must have a min_length=1 and last_name is not a nullable field. the error probably isn't reused anymore so we can just put it inline below.

Done.

---

backend/api/serializers/user.py line 153 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

make elif. can only be teacher or student - no need to check both

Done.

---

backend/api/tests/serializers/test_user.py line 82 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

rename to "test_validate__create__teacher__last_name_required"
you are testing:

1. the validate func
2. when a user is created
3. when a teacher-user is created
4. a teacher-user's last name is required during creation (this must be the exact error code)

please update all the validate test names to have the same naming convention.

"test_validate__{create/update}{user_type}{error_code}"

Done.

---

backend/api/tests/serializers/test_user.py line 83 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Not fully describing the test case. Something like:

"""Teacher-user's last_name is required during creation."""

Also, you said first name. You meant last name.

Please update all test docstring to fully describe the test case.

Done.

---

backend/api/tests/serializers/test_user.py line 89 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about test naming convention and docstring missing details

Done.

---

backend/api/tests/serializers/test_user.py line 102 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about test naming convention and docstring missing details. Also, describe the test case/scenario rather than how it's implemented. For example:

"Student-user cannot request to join a class."

See my reply above about specifics of error

---

backend/api/tests/serializers/test_user.py line 112 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about test naming convention and docstring missing details

Done.

---

backend/api/tests/serializers/test_user.py line 121 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about test naming convention and docstring missing details

Done.

---

backend/api/tests/serializers/test_user.py line 131 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about test naming convention and docstring missing details

Done.

[SKairinos]

Reviewed 1 of 9 files at r2, 6 of 10 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 19 unresolved discussions (waiting on @faucomte97)

---

backend/Pipfile line 26 at r4 (raw file):

[packages]
codeforlife = {ref = "v0.14.5", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"}

use the l

---

backend/api/serializers/user.py line 137 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

The error isn't strictly that a student is requesting to join a class, the error is that a student who already is in a class is requesting to join a class or that a student who is requesting to join a class is being edited to now be in a class. Not sure what name to give then

remember that we agreed "student" would mean a user that's a school/class student. For a student that's not in a class/school, we refer to them as "independent". Therefore, the error code "requesting_to_join_class__student" is the correct one because it means a [school/class] student is requesting to join a class.

also, as per our convo, never reuse error codes. I suggest codes:
"requesting_to_join_class__student__create"
"requesting_to_join_class__student__update"

---

backend/api/serializers/user.py line 127 at r4 (raw file):

        requesting_to_join_class__teacher_error = serializers.ValidationError(
            "Teacher can't request to join a class.",
            code="requesting_to_join_class__teacher",

as per our convo, never reuse error codes. I suggest codes:
"requesting_to_join_class__teacher__create"
"requesting_to_join_class__teacher__update"

---

backend/api/serializers/user.py line 147 at r4 (raw file):

            elif self.instance.student:
                if (

as per our convo, never reuse error codes. since these are two different scenarios, I suggest codes:
"requesting_to_join_class__student__update__in_class"
"requesting_to_join_class__student__update__requesting_to_join_class"

That way we can assert the 2 different scenarios with distinct codes.

---

backend/api/tests/serializers/test_user.py line 82 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done.

small fix, since the error code is "last_name__required", the test name should be:

"test_validate__create__teacher__last_name__required"

Naming convention = "test_validate__{create/update}__{user_type}__{error_code}"

fix all test names below

---

backend/api/tests/serializers/test_user.py line 89 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done.

small fix, since the error code is "requesting_to_join_class__teacher", the test name should be:

"test_validate__create__teacher__requesting_to_join_class__teacher"

Naming convention = "test_validate__{create/update}__{user_type}__{error_code}"

fix all test names below

---

backend/api/tests/serializers/test_user.py line 102 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

See my reply above about specifics of error

see my response and above comment about naming convention

---

backend/api/tests/serializers/test_user.py line 103 at r4 (raw file):

        )

    def test_validate__update__teacher__requesting_to_join_class(self):

see above comment about naming convention

---

backend/api/tests/serializers/test_user.py line 209 at r4 (raw file):

        """Cannot accept a new student into a class with a duplicate name."""
        user_fields = (
            StudentUser.objects.filter(new_student__class_field=self.class_1)

should be: new_student__class_field=self.indy_user.student.pending_class_request

you specifically want whatever class the indy user is requesting to join.

I understand that class_1 implicitly is the same class. But should be explicit reference, not implicit.

This is important for 2 reasons:

1. readability - i don't know class_1 is the class the indy user is requesting to join unless I go and read the fixtures (I shouldn't need to do this)
2. maintainability - if we ever decide to change the fixtures in the future, this implicit reference will break. An explicit one won't.

---

backend/api/tests/views/test_user.py line 125 at r4 (raw file):

        )

    def test_get_permissions__handle_join_class_request(self):

name should be "test_get_permissions__independents__handle_join_class_request"

Naming convention: "test_get_permissions__{action}"

---

backend/api/tests/views/test_user.py line 171 at r4 (raw file):

    def _test_get_queryset__independents__handle_join_class_request(
        self, is_admin: bool

instead, pass in the user.

user: SchoolTeacherUser

---

backend/api/tests/views/test_user.py line 173 at r4 (raw file):

        self, is_admin: bool
    ):
        request = self.client.request_factory.generic(

self.client.request_factory.patch(user=user)

---

backend/api/tests/views/test_user.py line 180 at r4 (raw file):

        )

        indy_users = list(

after installing my new py package, just do

indy_users = list(user.teacher.indy_users)

---

backend/api/tests/views/test_user.py line 225 at r4 (raw file):

        students who made a request to any class in the teacher's school."""
        self._test_get_queryset__independents__handle_join_class_request(
            is_admin=True

user=self.admin_school_teacher_use

---

backend/api/tests/views/test_user.py line 234 at r4 (raw file):

        students who made a request to one of the teacher's classes."""
        self._test_get_queryset__independents__handle_join_class_request(
            is_admin=False

user=self.non_admin_school_teacher_use

---

backend/api/tests/views/test_user.py line 252 at r4 (raw file):

        return self.assert_get_queryset(
            [self.indy_user],
            action="partial__update",

you mean partial_update?

---

backend/api/tests/views/test_user.py line 323 at r4 (raw file):

        assert self.indy_user.new_student.pending_class_request is None
        assert self.indy_user.new_student.class_field == self.class_1

see earlier comment about explicit references. instead of this, before you refresh from db, you should have

pending_class_request = self.indy_user.student.pending_class_request

after refresh

assert self.indy_user.student.class_field == pending_class_request

---

backend/api/views/user.py line 74 at r4 (raw file):

    def get_queryset(self, user_class=User):
        if self.action == "independents__handle_join_class_request":
            return IndependentUser.objects.filter(

using new py package, just do:

return self.request.school_teacher_user.teacher.indy_users

it's just one line now! 👯

[faucomte97]

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @SKairinos)

---

backend/Pipfile line 26 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

use the l

Done.

---

backend/api/serializers/user.py line 137 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

remember that we agreed "student" would mean a user that's a school/class student. For a student that's not in a class/school, we refer to them as "independent". Therefore, the error code "requesting_to_join_class__student" is the correct one because it means a [school/class] student is requesting to join a class.

also, as per our convo, never reuse error codes. I suggest codes:
"requesting_to_join_class__student__create"
"requesting_to_join_class__student__update"

Done.

---

backend/api/serializers/user.py line 127 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

as per our convo, never reuse error codes. I suggest codes:
"requesting_to_join_class__teacher__create"
"requesting_to_join_class__teacher__update"

Done.

---

backend/api/serializers/user.py line 147 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

as per our convo, never reuse error codes. since these are two different scenarios, I suggest codes:
"requesting_to_join_class__student__update__in_class"
"requesting_to_join_class__student__update__requesting_to_join_class"

That way we can assert the 2 different scenarios with distinct codes.

Done.

---

backend/api/tests/serializers/test_user.py line 82 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

small fix, since the error code is "last_name__required", the test name should be:

"test_validate__create__teacher__last_name__required"

Naming convention = "test_validate__{create/update}__{user_type}__{error_code}"

fix all test names below

Done.

---

backend/api/tests/serializers/test_user.py line 89 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

small fix, since the error code is "requesting_to_join_class__teacher", the test name should be:

"test_validate__create__teacher__requesting_to_join_class__teacher"

Naming convention = "test_validate__{create/update}__{user_type}__{error_code}"

fix all test names below

Done.

---

backend/api/tests/serializers/test_user.py line 102 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see my response and above comment about naming convention

Done.

---

backend/api/tests/serializers/test_user.py line 103 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about naming convention

Done.

---

backend/api/tests/serializers/test_user.py line 209 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

should be: new_student__class_field=self.indy_user.student.pending_class_request

you specifically want whatever class the indy user is requesting to join.

I understand that class_1 implicitly is the same class. But should be explicit reference, not implicit.

This is important for 2 reasons:

1. readability - i don't know class_1 is the class the indy user is requesting to join unless I go and read the fixtures (I shouldn't need to do this)
2. maintainability - if we ever decide to change the fixtures in the future, this implicit reference will break. An explicit one won't.

Done.

---

backend/api/tests/views/test_user.py line 125 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

name should be "test_get_permissions__independents__handle_join_class_request"

Naming convention: "test_get_permissions__{action}"

Done.

---

backend/api/tests/views/test_user.py line 171 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

instead, pass in the user.

user: SchoolTeacherUser

Done.

---

backend/api/tests/views/test_user.py line 173 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

self.client.request_factory.patch(user=user)

Done.

---

backend/api/tests/views/test_user.py line 180 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

after installing my new py package, just do

indy_users = list(user.teacher.indy_users)

Done.

---

backend/api/tests/views/test_user.py line 225 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

user=self.admin_school_teacher_use

Done.

---

backend/api/tests/views/test_user.py line 234 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

user=self.non_admin_school_teacher_use

Done.

---

backend/api/tests/views/test_user.py line 252 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

you mean partial_update?

Yes. How exactly did this test still pass?

---

backend/api/tests/views/test_user.py line 323 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see earlier comment about explicit references. instead of this, before you refresh from db, you should have

pending_class_request = self.indy_user.student.pending_class_request

after refresh

assert self.indy_user.student.class_field == pending_class_request

Done.

---

backend/api/views/user.py line 74 at r4 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

using new py package, just do:

return self.request.school_teacher_user.teacher.indy_users

it's just one line now! 👯

Done.

[SKairinos]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)