From 698e750cf5bb14c5a06591a2b0e963c581034a58 Mon Sep 17 00:00:00 2001 From: Stefan Kairinos Date: Fri, 23 Feb 2024 16:38:12 +0000 Subject: [PATCH] Release students (#309) * reset students' password * move fixtures to py package * add school teacher invitation fixtures * use school teacher invitation fixtures * reset login id also * new py package * feedback * use new py package * remove comment * new python package * anonymize independent- or teacher-user * Merge branch 'development' into anonymize_user * remove duplicate test * new py package * release students * merge from dev * TestReleaseStudentUserSerializer * new py package * first name --- backend/Pipfile | 4 +- backend/Pipfile.lock | 6 +-- backend/api/serializers/__init__.py | 2 +- backend/api/serializers/teacher.py | 4 +- backend/api/serializers/user.py | 48 +++++++++++++++++++-- backend/api/tests/serializers/test_user.py | 34 +++++++++++++-- backend/api/tests/views/test_user.py | 50 ++++++++++++++++++++++ backend/api/views/user.py | 46 +++++++++++++++----- 8 files changed, 169 insertions(+), 25 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 372cfc58..8cfa0241 100644 --- a/backend/Pipfile +++ b/backend/Pipfile @@ -7,7 +7,7 @@ name = "pypi" # Before adding a new package, check it's not listed under [packages] at # https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile # Replace "{ref}" in the above URL with the ref set below. -codeforlife = {ref = "v0.13.10", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.14.0", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} # TODO: check if we need the below packages whitenoise = "==6.5.0" django-pipeline = "==2.0.8" @@ -34,7 +34,7 @@ google-cloud-container = "==2.3.0" # Before adding a new package, check it's not listed under [dev-packages] at # https://github.com/ocadotechnology/codeforlife-package-python/blob/{ref}/Pipfile # Replace "{ref}" in the above URL with the ref set below. -codeforlife = {ref = "v0.13.10", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.14.0", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} # TODO: check if we need the below packages django-selenium-clean = "==0.3.3" django-test-migrations = "==1.2.0" diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 5bba802a..7e45438c 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "c3b11b9d8699621ec2071fed98072adb143c11ae3dd29b642c565fb491add82d" + "sha256": "1dcbd4e9dfef2a67abf37fd986485a1e56e678acb83ec5007de02e8c8c8e1545" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9f3c8292be7400cef9255cdf6283f9bb7edea955" + "ref": "f346dac3ad566173da83124c4a36ca27d0dd365e" }, "codeforlife-portal": { "hashes": [ @@ -1514,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9f3c8292be7400cef9255cdf6283f9bb7edea955" + "ref": "f346dac3ad566173da83124c4a36ca27d0dd365e" }, "codeforlife-portal": { "hashes": [ diff --git a/backend/api/serializers/__init__.py b/backend/api/serializers/__init__.py index 5f37dfae..554d2434 100644 --- a/backend/api/serializers/__init__.py +++ b/backend/api/serializers/__init__.py @@ -9,4 +9,4 @@ from .school_teacher_invitation import SchoolTeacherInvitationSerializer from .student import StudentSerializer from .teacher import TeacherSerializer -from .user import UserSerializer +from .user import ReleaseStudentUserSerializer, UserSerializer diff --git a/backend/api/serializers/teacher.py b/backend/api/serializers/teacher.py index c3d5e719..d9f61613 100644 --- a/backend/api/serializers/teacher.py +++ b/backend/api/serializers/teacher.py @@ -5,13 +5,13 @@ import typing as t -from codeforlife.user.models import User +from codeforlife.user.models import Teacher, User from codeforlife.user.serializers import TeacherSerializer as _TeacherSerializer from rest_framework import serializers # pylint: disable-next=missing-class-docstring,too-many-ancestors -class TeacherSerializer(_TeacherSerializer): +class TeacherSerializer(_TeacherSerializer[Teacher]): class Meta(_TeacherSerializer.Meta): extra_kwargs = { **_TeacherSerializer.Meta.extra_kwargs, diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index 5903d202..cb4c0c61 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -24,8 +24,9 @@ from .student import StudentSerializer from .teacher import TeacherSerializer +# pylint: disable=missing-class-docstring,too-many-ancestors + -# pylint: disable-next=missing-class-docstring class UserListSerializer(ModelListSerializer[User]): def create(self, validated_data): classes = { @@ -86,8 +87,7 @@ def get_first_name(user_fields: t.Dict[str, t.Any]): return attrs -# pylint: disable-next=missing-class-docstring,too-many-ancestors -class UserSerializer(_UserSerializer): +class UserSerializer(_UserSerializer[User]): student = StudentSerializer(source="new_student", required=False) teacher = TeacherSerializer(source="new_teacher", required=False) current_password = serializers.CharField( @@ -106,7 +106,11 @@ class Meta(_UserSerializer.Meta): extra_kwargs = { **_UserSerializer.Meta.extra_kwargs, "first_name": {"read_only": False}, - "last_name": {"read_only": False, "required": False}, + "last_name": { + "read_only": False, + "required": False, + "min_length": 1, + }, "email": {"read_only": False}, "password": {"write_only": True, "required": False}, } @@ -197,3 +201,39 @@ def to_representation(self, instance: User): representation["password"] = password return representation + + +class ReleaseStudentUserListSerializer(ModelListSerializer[StudentUser]): + def update(self, instance, validated_data): + for student_user, data in zip(instance, validated_data): + student_user.student.class_field = None + student_user.student.save(update_fields=["class_field"]) + + student_user.email = data["email"] + student_user.save(update_fields=["email"]) + + return instance + + +class ReleaseStudentUserSerializer(_UserSerializer[StudentUser]): + """Convert a student to an independent learner.""" + + class Meta(_UserSerializer.Meta): + extra_kwargs = { + "first_name": { + "min_length": 1, + "read_only": False, + "required": False, + }, + "email": {"read_only": False}, + } + list_serializer_class = ReleaseStudentUserListSerializer + + # pylint: disable-next=missing-function-docstring + def validate_email(self, value: str): + if User.objects.filter(email__iexact=value).exists(): + raise serializers.ValidationError( + "Already exists.", code="already_exists" + ) + + return value diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index 08768f96..8e4c25ff 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -4,12 +4,13 @@ """ from codeforlife.tests import ModelSerializerTestCase -from codeforlife.user.models import Class, Student, User +from codeforlife.user.models import Class, Student, StudentUser, User -from ...serializers import UserSerializer +from ...serializers import ReleaseStudentUserSerializer, UserSerializer + +# pylint: disable=missing-class-docstring -# pylint: disable-next=missing-class-docstring class TestUserSerializer(ModelSerializerTestCase[User]): model_serializer_class = UserSerializer fixtures = ["school_1"] @@ -61,3 +62,30 @@ def test_validate__first_name_not_unique_per_class_in_db(self): error_code="first_name_not_unique_per_class_in_db", many=True, ) + + +class TestReleaseStudentUserSerializer(ModelSerializerTestCase[StudentUser]): + model_serializer_class = ReleaseStudentUserSerializer + fixtures = ["school_1"] + + def setUp(self): + student_user = StudentUser.objects.first() + assert student_user + self.student_user = student_user + + def test_validate_email__already_exists(self): + """Cannot release a student with an email that already exists.""" + user_fields = User.objects.values("email").first() + assert user_fields + + self.assert_validate_field( + "email", user_fields["email"], error_code="already_exists" + ) + + def test_update(self): + """The student-user is converted in an independent-user.""" + self.assert_update_many( + instance=[self.student_user], + validated_data=[{"email": f"{self.student_user.pk}@email.com"}], + new_data=[{"student": {"class_field": None}}], + ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index a09c88c8..fbc327d0 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -28,6 +28,7 @@ from django.db.models.query import QuerySet from rest_framework import status +from ...serializers import ReleaseStudentUserSerializer from ...views import UserViewSet # NOTE: type hint to help Intellisense. @@ -76,6 +77,15 @@ def test_get_permissions__students__reset_password(self): action="students__reset_password", ) + def test_get_permissions__students__release(self): + """ + Only admin-teachers or class-teachers can release students. + """ + self.assert_get_permissions( + [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))], + action="students__release", + ) + def test_get_permissions__partial_update__teacher(self): """Only admin-teachers can update a teacher.""" self.assert_get_permissions( @@ -99,6 +109,14 @@ def test_get_permissions__destroy(self): action="destroy", ) + # test: get serializer class + + def test_get_serializer_class(self): + """The action for releasing students has a dedicated serializer.""" + self.assert_get_serializer_class( + ReleaseStudentUserSerializer, action="students__release" + ) + # test: get queryset def _test_get_queryset__student_users( @@ -137,6 +155,12 @@ def test_get_queryset__students__reset_password(self): action="students__reset_password", request_method="patch" ) + def test_get_queryset__students__release(self): + """Releasing students can only target student-users.""" + self._test_get_queryset__student_users( + action="students__release", request_method="patch" + ) + def test_get_queryset__destroy(self): """Destroying a user can only target the user making the request.""" return self.assert_get_queryset( @@ -358,6 +382,32 @@ def test_students__reset_password(self): self.client.login_as(student_user, password) assert student_user.student.login_id == student_login_id + def test_students__release(self): + """ + Admin-teacher or class_teacher can convert their students to independent + learners. + """ + user = self.admin_school_teacher_user + student_users = list(user.teacher.student_users) + + self.client.login_as(user) + response = self.client.patch( + self.reverse_action("students--release"), + data={ + student_user.id: {"email": f"{student_user.id}@email.com"} + for student_user in student_users + }, + ) + + for student_user, json_model in zip(student_users, response.json()): + student_user.refresh_from_db() + self.assert_serialized_model_equals_json_model( + model=student_user, + json_model=json_model, + action="students__release", + request_method="patch", + ) + # test: generic actions def test_partial_update__teacher(self): diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 6b4f9883..d65679ef 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -20,7 +20,7 @@ from rest_framework.permissions import AllowAny from rest_framework.response import Response -from ..serializers import UserSerializer +from ..serializers import ReleaseStudentUserSerializer, UserSerializer # NOTE: type hint to help Intellisense. default_token_generator: PasswordResetTokenGenerator = default_token_generator @@ -34,7 +34,11 @@ class UserViewSet(_UserViewSet): def get_permissions(self): if self.action == "destroy": return [OR(IsTeacher(), IsIndependent())] - if self.action in ["bulk", "students__reset_password"]: + if self.action in [ + "bulk", + "students__reset_password", + "students__release", + ]: return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] if self.action == "partial_update": if "teacher" in self.request.data: @@ -44,13 +48,22 @@ def get_permissions(self): return super().get_permissions() - def get_queryset(self): - queryset = super().get_queryset() + def get_serializer_class(self): + if self.action == "students__release": + return ReleaseStudentUserSerializer + + return super().get_serializer_class() + + def get_queryset(self, user_class=User): + queryset = super().get_queryset(user_class) if self.action == "destroy": queryset = queryset.filter(pk=self.request.auth_user.pk) - elif ( + elif self.action in [ + "students__reset_password", + "students__release", + ] or ( self.action == "bulk" and self.request.method in ["PATCH", "DELETE"] - ) or self.action == "students__reset_password": + ): queryset = queryset.filter( new_student__isnull=False, new_student__class_field__isnull=False, @@ -190,14 +203,13 @@ def request_password_reset(self, request: Request): @action(detail=False, methods=["patch"], url_path="students/reset-password") def students__reset_password(self, request: Request): """Bulk reset students' password.""" - queryset = self._get_bulk_queryset(request.data) + queryset = self.get_bulk_queryset(request.data, StudentUser) fields: t.Dict[int, DataDict] = {} - for pk in queryset.values_list("pk", flat=True): - student_user = StudentUser(pk=pk) + for student_user in queryset: student_user.set_password() - fields[pk] = { + fields[student_user.pk] = { # pylint: disable-next=protected-access "password": student_user._password, "student": {"login_id": student_user.student.login_id}, @@ -208,3 +220,17 @@ def students__reset_password(self, request: Request): student_user.student.save(update_fields=["login_id"]) return Response(fields) + + @action(detail=False, methods=["patch"], url_path="students/release") + def students__release(self, request: Request): + """Convert a list of students into independent learners.""" + queryset = self.get_bulk_queryset(request.json_dict.keys(), StudentUser) + serializer = self.get_serializer( + queryset, + data=request.data, + many=True, + context=self.get_serializer_context(), + ) + serializer.is_valid(raise_exception=True) + serializer.save() + return Response(serializer.data)