Bulk transfer students

[SKairinos]

This change is 

[faucomte97]

Reviewed 7 of 16 files at r1, all commit messages.
Reviewable status: 7 of 16 files reviewed, 1 unresolved discussion (waiting on @SKairinos)

---

backend/Pipfile line 10 at r1 (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 = "bulk_transfer_students", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"}

Update

[SKairinos]

Reviewable status: 3 of 17 files reviewed, 1 unresolved discussion (waiting on @faucomte97)

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

Update

Done.

[faucomte97]

Reviewed 5 of 16 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @SKairinos)

---

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

    def update(self, instance, validated_data):
        for student_user, data in zip(instance, validated_data):
            student_user.student.class_field = Class.objects.get(

Does the queryset already check whether the Class that's retrieved here is in the same school as the requester?

---

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

        for student_user, data in zip(instance, validated_data):
            student_user.student.class_field = Class.objects.get(
                access_code=data["new_student"]["class_field"]["access_code"]

In theory the requester here could submit a request and assign different destination classes to different students in the same request, correct? If so, we don't currently support that so maybe we should enforce a check that the request sends all students to the same class?

---

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

                access_code=data["new_student"]["class_field"]["access_code"]
            )
            student_user.student.save(update_fields=["class_field"])

You're missing a step in the flow.

First, the teacher selects the students they want to transfer to a new class.
Second, the teacher has to double-check the names are all unique.

During this step the teacher can edit the names of his students to ensure they don't clash with the names in the destination class.

---

backend/api/tests/serializers/test_user.py line 106 at r2 (raw file):

        self.class_2 = Class.objects.get(name="Class 2 @ School 1")

        # TODO: make this a property of Class in new data schema.

Wouldn't this just be the same as calling the related_name of the class_field on the Student model, ie students?

---

backend/api/tests/views/test_user.py line 146 at r2 (raw file):

        )

    def test_get_queryset__bulk__delete(self):

Why delete this?

---

backend/api/views/user.py line 35 at r2 (raw file):

# pylint: disable-next=missing-class-docstring,too-many-ancestors
class UserViewSet(_UserViewSet):
    http_method_names = ["get", "post", "patch", "delete", "put"]

Why are we using put and not patch?

[SKairinos]

Reviewable status: 5 of 24 files reviewed, 7 unresolved discussions (waiting on @faucomte97)

---

backend/api/serializers/student.py line 55 at r3 (raw file):

        def get_first_name(student_fields: DataDict):
            new_user: DataDict = student_fields.get("new_user", {})
            return t.cast(str, new_user.get("first_name", ""))

make case insensitive with first_name.lower()

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

Does the queryset already check whether the Class that's retrieved here is in the same school as the requester?

yes. see StudentSerializer.validate_klass(). In a bulk/list update, each item in the list will be validated

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

In theory the requester here could submit a request and assign different destination classes to different students in the same request, correct? If so, we don't currently support that so maybe we should enforce a check that the request sends all students to the same class?

Yes, technically, a teacher can transfer 3 diff students to 3 diff classes (so long as all students and classes are in the requesting teachers' school). I know we don't support this on the FE but this is a limitation FE's forms not being flexible enough to support multi-class transfers.

I don't think we need to break this on the BE as we're not 'breaking' any business logic. As long as students are only getting transferred to classes in the same school, we haven't done anything 'illegal', if that makes sense.

[SKairinos]

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

Previously, faucomte97 (Florian Aucomte) wrote…

You're missing a step in the flow.

First, the teacher selects the students they want to transfer to a new class.
Second, the teacher has to double-check the names are all unique.

During this step the teacher can edit the names of his students to ensure they don't clash with the names in the destination class.

Done

[SKairinos]

Reviewable status: 5 of 24 files reviewed, 7 unresolved discussions (waiting on @faucomte97)

---

backend/api/tests/serializers/test_user.py line 106 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Wouldn't this just be the same as calling the related_name of the class_field on the Student model, ie students?

no longer relevant

---

backend/api/tests/views/test_user.py line 146 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why delete this?

no longer relevant. these test exist on the student view set

---

backend/api/views/user.py line 35 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why are we using put and not patch?

We are using put because all custom actions use put, not patch. Patch is for partially updating a model (forces all required serializer fields to be optional). Put is for providing all data necessary to complete an action (required fields stay required). For example, when transferring a student from one class to another, the klass field is required. This should not be PATCHed, it should be PUTed. 😂

[faucomte97]

Reviewed 19 of 19 files at r3, all commit messages.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @SKairinos)

---

backend/api/serializers/student.py line 55 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

make case insensitive with first_name.lower()

Yes 👍

---

backend/api/serializers/student.py line 204 at r3 (raw file):

            user_fields = t.cast(DataDict, data["new_user"])
            student.new_user.email = user_fields["email"]

Just realised that the username field needs to be set to the email value too.
Update tests too if needed

---

backend/api/serializers/student.py line 222 at r3 (raw file):

                **BaseUserSerializer.Meta.extra_kwargs,
                "first_name": {"required": False},
                "email": {"read_only": False},

Might need to add username in here as not read only

---

backend/api/serializers/student.py line 235 at r3 (raw file):

    def update(self, instance, validated_data):
        for student, data in zip(instance, validated_data):
            student.class_field = Class.objects.get(

If this is to allow different students to be transferred to different classes in one call then fair enough, but I just thought that we could get the class the students are being moved to before the loop so we don't perform x gets where x is the number of students the teacher is moving.

---

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

                        "Teacher can't request to join a class.",
                        code="requesting_to_join_class__teacher__update",
                    )

Didn't you say you had removed this check since a Teacher can't have the new_student attr anyway?
The check here should just be if "new_student" in attrs, right?

Code quote:

            if self.instance.teacher:
                if (
                    "new_student" in attrs
                    and "pending_class_request" in attrs["new_student"]
                ):
                    raise serializers.ValidationError(
                        "Teacher can't request to join a class.",
                        code="requesting_to_join_class__teacher__update",
                    )

---

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

                    )

            elif self.instance.student:

Side note: Should we add a check in here, similar to above, to ensure a student instance cannot be updated with a new_teacher attribute?

---

backend/api/tests/serializers/test_student.py line 43 at r3 (raw file):

    def setUp(self):
        student_user = StudentUser.objects.first()

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

---

backend/api/tests/serializers/test_student.py line 81 at r3 (raw file):

    def test_validate__first_name__db__exists_in_class(self):
        """
        Cannot provide transfer a student to another class if their first name

remove "provide"

---

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

        klass = Class.objects.exclude(
            teacher__school=user.teacher.school
        ).first()

This will get Albert Einstein, correct? Ie, not data from the fixtures.

---

backend/api/tests/serializers/test_student.py line 166 at r3 (raw file):

    def setUp(self):
        student = Student.objects.first()

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

---

backend/api/tests/serializers/test_student.py line 192 at r3 (raw file):

    def setUp(self):
        klass = Class.objects.first()

This just gets Class AB123 doesn't it? Ie, nothing to do with the school_1 fixture, right?

---

backend/api/tests/serializers/test_student.py line 217 at r3 (raw file):

    def setUp(self):
        student = Student.objects.first()

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

---

backend/api/tests/serializers/test_student.py line 233 at r3 (raw file):

                }
            ],
            new_data=[{"class_field_id": None}],

Username too (set to email)

---

backend/api/tests/serializers/test_student.py line 266 at r3 (raw file):

    def setUp(self):
        student = Student.objects.first()

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

---

backend/api/tests/serializers/test_student.py line 283 at r3 (raw file):

                instance=[self.student],
                validated_data=[{"new_user": {"password": password}}],
                new_data=[{"new_user": {"password": password_hash}}],

Do we need to check that the login_id was updated too?

---

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

    def test_validate_email__already_exists(self):
        """Cannot assign a user an email that already exists."""
        user_fields = User.objects.values("email").first()

This just gets Albert Einstein doesn't it? Ie, nothing to do with the school_1 fixture, right?

---

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

        Password is validated using django's installed password-validators.
        """
        raise NotImplementedError()  # TODO

Do we want this here? Isn't this already tested in the Python package?

Code quote:

    def test_validate_password(self):
        """
        Password is validated using django's installed password-validators.
        """
        raise NotImplementedError()  # TODO

---

backend/api/views/user.py line 35 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

We are using put because all custom actions use put, not patch. Patch is for partially updating a model (forces all required serializer fields to be optional). Put is for providing all data necessary to complete an action (required fields stay required). For example, when transferring a student from one class to another, the klass field is required. This should not be PATCHed, it should be PUTed. 😂

Add TODOs next to other custom actions that need to be updated to use PUT instead of PATCH

[SKairinos]

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

---

backend/api/serializers/student.py line 55 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Yes 👍

Done.

---

backend/api/serializers/student.py line 204 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Just realised that the username field needs to be set to the email value too.
Update tests too if needed

we have a signal that handles this

---

backend/api/serializers/student.py line 222 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Might need to add username in here as not read only

username is not a field a user-field in the new schema so it's intentionally omitted from the data-access layer

---

backend/api/serializers/student.py line 235 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

If this is to allow different students to be transferred to different classes in one call then fair enough, but I just thought that we could get the class the students are being moved to before the loop so we don't perform x gets where x is the number of students the teacher is moving.

Done.

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

Didn't you say you had removed this check since a Teacher can't have the new_student attr anyway?
The check here should just be if "new_student" in attrs, right?

Done.

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

Side note: Should we add a check in here, similar to above, to ensure a student instance cannot be updated with a new_teacher attribute?

Done.

---

backend/api/tests/serializers/test_student.py line 43 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

Done.

---

backend/api/tests/serializers/test_student.py line 81 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

remove "provide"

Done.

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

This will get Albert Einstein, correct? Ie, not data from the fixtures.

Done. Yes.

---

backend/api/tests/serializers/test_student.py line 166 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

Done.

---

backend/api/tests/serializers/test_student.py line 192 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This just gets Class AB123 doesn't it? Ie, nothing to do with the school_1 fixture, right?

Done.

---

backend/api/tests/serializers/test_student.py line 217 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

Done.

---

backend/api/tests/serializers/test_student.py line 233 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Username too (set to email)

Done.

---

backend/api/tests/serializers/test_student.py line 266 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

This just gets Leonardo doesn't it? Ie, nothing to do with the school_1 fixture, right?

Done.

---

backend/api/tests/serializers/test_student.py line 283 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Do we need to check that the login_id was updated too?

Done.

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

This just gets Albert Einstein doesn't it? Ie, nothing to do with the school_1 fixture, right?

Done.

---

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

Previously, faucomte97 (Florian Aucomte) wrote…

Do we want this here? Isn't this already tested in the Python package?

Yes, we want this here. This is not tested in PY package because the PY package has read-only views (only HTTP GET). There's no need to validate the password in PY package because create and update are write actions.

---

backend/api/views/user.py line 35 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Add TODOs next to other custom actions that need to be updated to use PUT instead of PATCH

Done.

[faucomte97]

Reviewed 7 of 7 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)

---

backend/api/tests/serializers/test_student.py line 108 at r3 (raw file):

        )

    def test_validate_klass__does_not_exist(self):

what?

---

backend/api/tests/serializers/test_student.py line 283 at r3 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Done.

can't see no change

[SKairinos]

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

---

backend/api/tests/serializers/test_student.py line 108 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

what?

my bad. the def should not have been deleted. The tests were still running as the test's func body was just being inside the setUp() instead (the above def). Since the test passes, it didn't cause an issue inside of setUp()

---

backend/api/tests/serializers/test_student.py line 283 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

can't see no change

Double Done.

[faucomte97]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)