From 6243df0f353ca1466484b02f509ed86598587592 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 11:34:15 +0000 Subject: [PATCH 01/22] Class join requests pt 1 --- backend/api/models/__init__.py | 2 +- backend/api/serializers/klass.py | 9 + backend/api/serializers/student.py | 55 +++++- backend/api/tests/serializers/test_klass.py | 30 +--- backend/api/tests/serializers/test_student.py | 39 +++- backend/api/tests/views/test_klass.py | 4 +- backend/api/tests/views/test_school.py | 4 +- backend/api/tests/views/test_user.py | 168 ++++++++++++++++-- backend/api/views/klass.py | 2 +- backend/api/views/school.py | 2 +- backend/api/views/user.py | 97 +++++++++- 11 files changed, 352 insertions(+), 60 deletions(-) diff --git a/backend/api/models/__init__.py b/backend/api/models/__init__.py index c45ded7b..6f468062 100644 --- a/backend/api/models/__init__.py +++ b/backend/api/models/__init__.py @@ -2,5 +2,5 @@ © Ocado Group Created on 06/02/2024 at 15:13:00(+00:00). """ - +# TODO: Move from common to here and update to match new models pattern from common.models import SchoolTeacherInvitation diff --git a/backend/api/serializers/klass.py b/backend/api/serializers/klass.py index 8b758535..4daf43c0 100644 --- a/backend/api/serializers/klass.py +++ b/backend/api/serializers/klass.py @@ -100,3 +100,12 @@ def create(self, validated_data): ), } ) + + def update(self, instance, validated_data): + accept_requests_until = validated_data.get("accept_requests_until") + + if accept_requests_until is not None: + instance.accept_requests_until = accept_requests_until + + instance.save() + return instance diff --git a/backend/api/serializers/student.py b/backend/api/serializers/student.py index 4aaaeaaa..e24eea9e 100644 --- a/backend/api/serializers/student.py +++ b/backend/api/serializers/student.py @@ -7,12 +7,18 @@ from codeforlife.user.models import Class, User from codeforlife.user.serializers import StudentSerializer as _StudentSerializer +from django.utils import timezone from rest_framework import serializers # pylint: disable-next=missing-class-docstring,too-many-ancestors class StudentSerializer(_StudentSerializer): klass = serializers.CharField(source="class_field.access_code") + pending_class_request = serializers.CharField( + source="pending_class_request.access_code", + required=False, + allow_blank=True, + ) class Meta(_StudentSerializer.Meta): pass @@ -26,8 +32,7 @@ def validate_klass(self, value: str): klass = Class.objects.get(access_code=value) except Class.DoesNotExist as ex: raise serializers.ValidationError( - "Class does not exist.", - code="does_not_exist", + "Class does not exist.", code="does_not_exist" ) from ex if klass.teacher.school_id != teacher.school_id: @@ -43,12 +48,54 @@ def validate_klass(self, value: str): return value + def validate_pending_class_request(self, value: str): + # NOTE: Error message is purposefully ambiguous to prevent class + # enumeration. + error_message = "Class does not exist or does not accept join requests." + error_code = "does_not_exist_or_accept_join_requests" + + if value != "": + try: + klass = Class.objects.get(access_code=value) + except Class.DoesNotExist as ex: + raise serializers.ValidationError( + error_message, code=error_code + ) from ex + + if klass.accept_requests_until is None: + raise serializers.ValidationError( + error_message, code=error_code + ) + if klass.accept_requests_until < timezone.now(): + raise serializers.ValidationError( + error_message, code=error_code + ) + + return value + def validate(self, attrs): instance = t.cast(t.Optional[User], self.parent.instance) if instance and not instance.student: raise serializers.ValidationError( - "Target user is not a student.", - code="not_student", + "Target user is not a student.", code="not_student" ) return attrs + + def update(self, instance, validated_data): + pending_class_request = validated_data.get("pending_class_request") + + if pending_class_request is not None: + if pending_class_request == "": + instance.pending_class_request = None + else: + instance.pending_class_request = Class.objects.get( + access_code=pending_class_request + ) + + # TODO: Send email to indy user confirming successful join request. + # TODO: Send email to teacher of selected class to notify them of + # join request. + + instance.save() + return instance diff --git a/backend/api/tests/serializers/test_klass.py b/backend/api/tests/serializers/test_klass.py index 1171913f..58e4c1d0 100644 --- a/backend/api/tests/serializers/test_klass.py +++ b/backend/api/tests/serializers/test_klass.py @@ -21,10 +21,7 @@ def setUp(self): self.class_1 = Class.objects.get(name="Class 1 @ School 1") def test_validate_teacher__does_not_exist(self): - """ - Teacher must exist. - """ - + """Teacher must exist.""" self.assert_validate_field( name="teacher", value=-1, @@ -32,10 +29,7 @@ def test_validate_teacher__does_not_exist(self): ) def test_validate_teacher__not_in_school(self): - """ - Teacher must be in school. - """ - + """Teacher must be in school.""" teacher = Teacher.objects.exclude( school=self.school_teacher_user.teacher.school ).first() @@ -53,10 +47,7 @@ def test_validate_teacher__not_in_school(self): ) def test_validate_teacher__not_admin(self): - """ - Teacher cannot assign another teacher if they're not an admin. - """ - + """Teacher cannot assign another teacher if they're not an admin.""" assert not self.school_teacher_user.teacher.is_admin teacher = ( @@ -80,10 +71,7 @@ def test_validate_teacher__not_admin(self): ) def test_validate_name__name_not_unique(self): - """ - Class names must be unique per school. - """ - + """Class names must be unique per school.""" self.assert_validate_field( name="name", value=self.class_1.name, @@ -96,10 +84,7 @@ def test_validate_name__name_not_unique(self): ) def test_create__teacher(self): - """ - Can successfully create with setting the teacher field. - """ - + """Can successfully create with setting the teacher field.""" self.assert_create( { "name": "ExampleClass", @@ -111,10 +96,7 @@ def test_create__teacher(self): ) def test_create__no_teacher(self): - """ - Can successfully create without setting the teacher field. - """ - + """Can successfully create without setting the teacher field.""" self.assert_create( { "name": "ExampleClass", diff --git a/backend/api/tests/serializers/test_student.py b/backend/api/tests/serializers/test_student.py index 4493d5a6..86683f43 100644 --- a/backend/api/tests/serializers/test_student.py +++ b/backend/api/tests/serializers/test_student.py @@ -6,6 +6,7 @@ from codeforlife.tests import ModelSerializerTestCase from codeforlife.user.models import ( Class, + IndependentUser, NonSchoolTeacherUser, SchoolTeacherUser, Student, @@ -19,17 +20,22 @@ class TestStudentSerializer(ModelSerializerTestCase[Student]): model_serializer_class = StudentSerializer fixtures = [ + "independent", "non_school_teacher", "school_1", "school_2", ] + def setUp(self): + self.independent = IndependentUser.objects.get(email="indy@man.com") + self.class_1 = Class.objects.get(name="Class 1 @ School 1") + self.class_3 = Class.objects.get(name="Class 3 @ School 1") + def test_validate_klass__does_not_exist(self): """ Requesting teacher cannot assign a student to a class that doesn't exist. """ - user = SchoolTeacherUser.objects.get(email="teacher@school1.com") self.assert_validate_field( @@ -46,7 +52,6 @@ def test_validate_klass__teacher_not_in_same_school(self): Requesting teacher cannot assign a student to a class if they're not in the same school. """ - user = SchoolTeacherUser.objects.get(email="teacher@school1.com") klass = Class.objects.exclude( @@ -68,7 +73,6 @@ def test_validate_klass__teacher_not_admin_or_class_owner(self): Requesting teacher cannot assign a student to a class if they're not an admin or they don't own the class. """ - user = SchoolTeacherUser.objects.get(email="teacher@school1.com") assert not user.teacher.is_admin @@ -88,11 +92,36 @@ def test_validate_klass__teacher_not_admin_or_class_owner(self): ), ) - def test_validate__not_student(self): + def test_validate_pending_class_request__does_not_exist(self): + """Join class request cannot be for a class that doesn't exist""" + self.assert_validate_field( + name="pending_class_request", + value="AAAAA", + error_code="does_not_exist_or_accept_join_requests", + ) + + def test_validate_pending_class_request__does_not_accept_requests(self): """ - Target user must be a student. + Join class request cannot be for a class that doesn't accept requests """ + self.assert_validate_field( + name="pending_class_request", + value=self.class_1.access_code, + error_code="does_not_exist_or_accept_join_requests", + ) + + def test_validate_pending_class_request__no_longer_accept_requests(self): + """ + Join class request cannot be for a class that no longer accepts requests + """ + self.assert_validate_field( + name="pending_class_request", + value=self.class_3.access_code, + error_code="does_not_exist_or_accept_join_requests", + ) + def test_validate__not_student(self): + """Target user must be a student.""" teacher_user = TeacherUser.objects.first() assert teacher_user diff --git a/backend/api/tests/views/test_klass.py b/backend/api/tests/views/test_klass.py index 5d30d1a3..17180b83 100644 --- a/backend/api/tests/views/test_klass.py +++ b/backend/api/tests/views/test_klass.py @@ -36,13 +36,13 @@ def test_get_permissions__create(self): action="create", ) - def test_get_permissions__update(self): + def test_get_permissions__partial_update(self): """Only admin-teachers or class-teachers can update a class.""" self.assert_get_permissions( permissions=[ OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) ], - action="update", + action="partial_update", ) def test_get_permissions__destroy(self): diff --git a/backend/api/tests/views/test_school.py b/backend/api/tests/views/test_school.py index 03e644cd..34538fa0 100644 --- a/backend/api/tests/views/test_school.py +++ b/backend/api/tests/views/test_school.py @@ -41,12 +41,12 @@ def test_get_permissions__create(self): action="create", ) - def test_get_permissions__update(self): + def test_get_permissions__partial_update(self): """Only admin-teachers in a school can update a school.""" self.assert_get_permissions( permissions=[IsTeacher(is_admin=True)], - action="update", + action="partial_update", ) def test_get_permissions__retrieve(self): diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 41560f1c..19f0b7c1 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -10,10 +10,11 @@ from codeforlife.user.models import ( AdminSchoolTeacherUser, Class, + IndependentUser, SchoolTeacherUser, User, ) -from codeforlife.user.permissions import IsTeacher +from codeforlife.user.permissions import IsIndependent, IsTeacher from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, default_token_generator, @@ -30,22 +31,27 @@ class TestUserViewSet(ModelViewSetTestCase[User]): basename = "user" model_view_set_class = UserViewSet - fixtures = ["independent", "non_school_teacher", "school_1"] + fixtures = ["independent", "non_school_teacher", "school_1", "school_2"] non_school_teacher_email = "teacher@noschool.com" school_teacher_email = "teacher@school1.com" indy_email = "indy@man.com" + indy_with_join_request_email = "indy@requester.com" def setUp(self): + self.non_admin_school_teacher_user = SchoolTeacherUser.objects.get( + email=self.school_teacher_email + ) self.admin_school_teacher_user = AdminSchoolTeacherUser.objects.get( email="admin.teacher@school1.com" ) - - def _login_non_admin_school_teacher(self): - return self.client.login_non_admin_school_teacher( - email=self.school_teacher_email, - password="password", + self.admin_school2_teacher_user = AdminSchoolTeacherUser.objects.get( + email="admin.teacher@school2.com" + ) + self.indy_with_join_request = IndependentUser.objects.get( + email=self.indy_with_join_request_email ) + self.class_2 = Class.objects.get(name="Class 2 @ School 1") def _get_pk_and_token_for_user(self, email: str): user = User.objects.get(email__iexact=email) @@ -82,6 +88,40 @@ def test_get_permissions__partial_update__student(self): request=self.client.request_factory.patch(data={"student": {}}), ) + def test_get_permissions__partial_update__student__pending_class_request( + self, + ): + """ + Only school-teachers and independents can update an independent's class + join request. + """ + self.assert_get_permissions( + permissions=[ + OR( + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), + IsIndependent(), + ) + ], + action="partial_update", + request=self.client.request_factory.patch( + data={"student": {"pending_class_request": ""}} + ), + ) + + def test_get_permissions__student__reject_join_request(self): + """ + Only school-teachers can reject an independent's class join request. + """ + self.assert_get_permissions( + permissions=[ + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) + ], + action="student__reject_join_request", + request=self.client.request_factory.patch( + data={"student": {"pending_class_request": None}} + ), + ) + # test: get queryset def _test_get_queryset__bulk(self, request_method: str): @@ -116,7 +156,10 @@ def test_get_queryset__bulk__delete(self): def test_bulk_create__students(self): """Teacher can bulk create students.""" - user = self._login_non_admin_school_teacher() + user = self.client.login_non_admin_school_teacher( + email=self.school_teacher_email, + password="password", + ) klass: t.Optional[Class] = user.teacher.class_teacher.first() assert klass is not None @@ -127,10 +170,7 @@ def test_bulk_create__students(self): "first_name": "Peter", "student": {"klass": klass.access_code}, }, - { - "first_name": "Mary", - "student": {"klass": klass.access_code}, - }, + {"first_name": "Mary", "student": {"klass": klass.access_code}}, ], make_assertions=False, ) @@ -144,7 +184,7 @@ def test_bulk_destroy(self): student_user_queryset = User.objects.filter( new_student__class_field__teacher__school=( self.admin_school_teacher_user.teacher.school - ), + ) ) student_user_ids = list( student_user_queryset.values_list("id", flat=True) @@ -160,6 +200,71 @@ def test_bulk_destroy(self): ).count() ) + # test: class join request actions + + def test_student__reject_join_request__invalid_school(self): + """Teacher cannot reject a join request outside their school""" + self.client.login_as(self.admin_school2_teacher_user) + + viewname = self.reverse_action( + "student--reject-join-request", + kwargs={"pk": self.indy_with_join_request.new_student.pk}, + ) + + response = self.client.patch( + viewname, status_code_assertion=status.HTTP_400_BAD_REQUEST + ) + + assert response.data["non_field_errors"] == [ + "This class join request is not in your school." + ] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_student__reject_join_request__invalid_class(self): + """Non-admin teacher cannot reject a join request outside their class""" + self.client.login_as(self.non_admin_school_teacher_user) + + viewname = self.reverse_action( + "student--reject-join-request", + kwargs={"pk": self.indy_with_join_request.new_student.pk}, + ) + + response = self.client.patch( + viewname, status_code_assertion=status.HTTP_400_BAD_REQUEST + ) + + assert response.data["non_field_errors"] == [ + "This class join request is not for one for your classes." + ] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_student__reject_join_request(self): + """Teacher can reject an independent's request to join a class.""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "student--reject-join-request", + kwargs={"pk": self.indy_with_join_request.new_student.pk}, + ) + + self.client.patch(viewname, status_code_assertion=status.HTTP_200_OK) + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + is None + ) + # test: reset password actions def test_request_password_reset__invalid_email(self): @@ -259,8 +364,7 @@ def test_reset_password__patch__teacher(self): self.client.patch(viewname, data={"password": "N3wPassword!"}) self.client.login( - email=self.non_school_teacher_email, - password="N3wPassword!", + email=self.non_school_teacher_email, password="N3wPassword!" ) def test_reset_password__patch__indy(self): @@ -280,8 +384,7 @@ def test_reset_password__patch__indy(self): def test_partial_update__teacher(self): """Admin-school-teacher can update another teacher's profile.""" admin_school_teacher_user = self.client.login_admin_school_teacher( - email="admin.teacher@school1.com", - password="password", + email="admin.teacher@school1.com", password="password" ) other_school_teacher_user = ( @@ -298,7 +401,36 @@ def test_partial_update__teacher(self): { "last_name": other_school_teacher_user.first_name, "teacher": { - "is_admin": not other_school_teacher_user.teacher.is_admin, + "is_admin": not other_school_teacher_user.teacher.is_admin }, }, ) + + def test_partial_update__indy__send_join_request(self): + """Independent user can request to join a class.""" + indy_user = self.client.login_indy( + email=self.indy_email, password="password" + ) + + self.client.partial_update( + indy_user, + {"student": {"pending_class_request": self.class_2.access_code}}, + ) + + def test_partial_update__indy__revoke_join_request(self): + """Independent user can revoke their request to join a class.""" + indy_user_with_join_request = self.client.login_indy( + email=self.indy_with_join_request_email, password="password" + ) + + self.client.partial_update( + indy_user_with_join_request, + {"student": {"pending_class_request": ""}}, + ) + + indy_user_with_join_request.refresh_from_db() + indy_user_with_join_request.new_student.refresh_from_db() + assert ( + indy_user_with_join_request.new_student.pending_class_request + is None + ) diff --git a/backend/api/views/klass.py b/backend/api/views/klass.py index 58c0cac4..8455cfd1 100644 --- a/backend/api/views/klass.py +++ b/backend/api/views/klass.py @@ -21,7 +21,7 @@ def get_permissions(self): return [AllowNone()] if self.action == "create": return [IsTeacher(in_school=True)] - if self.action in ["update", "destroy"]: + if self.action in ["partial_update", "destroy"]: return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() diff --git a/backend/api/views/school.py b/backend/api/views/school.py index f4c2400a..37a30648 100644 --- a/backend/api/views/school.py +++ b/backend/api/views/school.py @@ -23,7 +23,7 @@ def get_permissions(self): if self.action == "create": return [IsTeacher(in_school=False)] # Only admin-teachers in a school can update a school. - if self.action == "update": + if self.action == "partial_update": return [IsTeacher(is_admin=True)] return super().get_permissions() diff --git a/backend/api/views/user.py b/backend/api/views/user.py index cd0b80a4..6b525ce2 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -7,8 +7,8 @@ from codeforlife.permissions import OR from codeforlife.request import Request -from codeforlife.user.models import User -from codeforlife.user.permissions import IsTeacher +from codeforlife.user.models import Class, Student, User +from codeforlife.user.permissions import IsIndependent, IsTeacher from codeforlife.user.views import UserViewSet as _UserViewSet from django.contrib.auth.tokens import ( PasswordResetTokenGenerator, @@ -36,8 +36,20 @@ def get_permissions(self): if self.action == "partial_update": if "teacher" in self.request.data: return [IsTeacher(is_admin=True)] + if ( + "student" in self.request.data + and "pending_class_request" in self.request.data["student"] + ): + return [ + OR( + OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), + IsIndependent(), + ) + ] if "student" in self.request.data: return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] + if self.action == "student__reject_join_request": + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() @@ -142,3 +154,84 @@ def request_password_reset(self, request: Request): "token": token, } ) + + @action( + detail=True, methods=["patch"], url_path="student/reject-join-request" + ) + def student__reject_join_request( + self, request: Request, pk: t.Optional[str] = None + ): + """Reject an independent user's request to join a class.""" + indy = self._get_student_requesting_join_if_authorised(request, pk) + + indy.pending_class_request = None + indy.save() + + return Response() + + @action( + detail=True, methods=["patch"], url_path="student/accept-join-request" + ) + def student__accept_join_request( + self, request: Request, pk: t.Optional[str] = None + ): + """Accept an independent user's request to join a class.""" + self._get_student_requesting_join_if_authorised(request, pk) + + # indy.pending_class_request = None + # indy.save() + + # TODO: Redirect to next step which is renaming student + return Response() + + @action( + detail=True, methods=["patch"], url_path="student/add-to-class" + ) + def student__add_to_class( + self, request: Request, pk: t.Optional[str] = None + ): + """Add an independent user to a class""" + return Response() + + def _get_student_requesting_join_if_authorised( + self, + request: Request, + pk: str, + ) -> Student: + try: + indy = Student.objects.get(pk=int(pk)) + except (ValueError, Student.DoesNotExist): + return Response( + {"non_field_errors": ["No student found for given ID."]}, + status=status.HTTP_400_BAD_REQUEST, + ) + + klass = Class.objects.get( + access_code=indy.pending_class_request.access_code + ) + + if klass.teacher.school != request.user.new_teacher.school: + return Response( + { + "non_field_errors": [ + "This class join request is not in your school." + ] + }, + status=status.HTTP_400_BAD_REQUEST, + ) + + if ( + not request.user.new_teacher.is_admin + and klass.teacher != request.user.new_teacher + ): + return Response( + { + "non_field_errors": [ + "This class join request is not for " + "one for your classes." + ] + }, + status=status.HTTP_400_BAD_REQUEST, + ) + + return indy From d0fd406ffffdf89c5ac39d2a09eccd5a09edf656 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 11:42:06 +0000 Subject: [PATCH 02/22] Update lockfile --- backend/Pipfile | 4 +- backend/Pipfile.lock | 114 +++++++++++++++++++++---------------------- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 73e310ce..a3584955 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.7", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "join_requests", 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.7", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "join_requests", 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 d8f3d366..ca697935 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1058,7 +1058,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==2.8.2" }, "pytz": { @@ -1209,7 +1209,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==1.16.0" }, "sortedcontainers": { @@ -1526,61 +1526,61 @@ "toml" ], "hashes": [ - "sha256:0193657651f5399d433c92f8ae264aff31fc1d066deee4b831549526433f3f61", - "sha256:02f2edb575d62172aa28fe00efe821ae31f25dc3d589055b3fb64d51e52e4ab1", - "sha256:0491275c3b9971cdbd28a4595c2cb5838f08036bca31765bad5e17edf900b2c7", - "sha256:077d366e724f24fc02dbfe9d946534357fda71af9764ff99d73c3c596001bbd7", - "sha256:10e88e7f41e6197ea0429ae18f21ff521d4f4490aa33048f6c6f94c6045a6a75", - "sha256:18e961aa13b6d47f758cc5879383d27b5b3f3dcd9ce8cdbfdc2571fe86feb4dd", - "sha256:1a78b656a4d12b0490ca72651fe4d9f5e07e3c6461063a9b6265ee45eb2bdd35", - "sha256:1ed4b95480952b1a26d863e546fa5094564aa0065e1e5f0d4d0041f293251d04", - "sha256:23b27b8a698e749b61809fb637eb98ebf0e505710ec46a8aa6f1be7dc0dc43a6", - "sha256:23f5881362dcb0e1a92b84b3c2809bdc90db892332daab81ad8f642d8ed55042", - "sha256:32a8d985462e37cfdab611a6f95b09d7c091d07668fdc26e47a725ee575fe166", - "sha256:3468cc8720402af37b6c6e7e2a9cdb9f6c16c728638a2ebc768ba1ef6f26c3a1", - "sha256:379d4c7abad5afbe9d88cc31ea8ca262296480a86af945b08214eb1a556a3e4d", - "sha256:3cacfaefe6089d477264001f90f55b7881ba615953414999c46cc9713ff93c8c", - "sha256:3e3424c554391dc9ef4a92ad28665756566a28fecf47308f91841f6c49288e66", - "sha256:46342fed0fff72efcda77040b14728049200cbba1279e0bf1188f1f2078c1d70", - "sha256:536d609c6963c50055bab766d9951b6c394759190d03311f3e9fcf194ca909e1", - "sha256:5d6850e6e36e332d5511a48a251790ddc545e16e8beaf046c03985c69ccb2676", - "sha256:6008adeca04a445ea6ef31b2cbaf1d01d02986047606f7da266629afee982630", - "sha256:64e723ca82a84053dd7bfcc986bdb34af8d9da83c521c19d6b472bc6880e191a", - "sha256:6b00e21f86598b6330f0019b40fb397e705135040dbedc2ca9a93c7441178e74", - "sha256:6d224f0c4c9c98290a6990259073f496fcec1b5cc613eecbd22786d398ded3ad", - "sha256:6dceb61d40cbfcf45f51e59933c784a50846dc03211054bd76b421a713dcdf19", - "sha256:7ac8f8eb153724f84885a1374999b7e45734bf93a87d8df1e7ce2146860edef6", - "sha256:85ccc5fa54c2ed64bd91ed3b4a627b9cce04646a659512a051fa82a92c04a448", - "sha256:869b5046d41abfea3e381dd143407b0d29b8282a904a19cb908fa24d090cc018", - "sha256:8bdb0285a0202888d19ec6b6d23d5990410decb932b709f2b0dfe216d031d218", - "sha256:8dfc5e195bbef80aabd81596ef52a1277ee7143fe419efc3c4d8ba2754671756", - "sha256:8e738a492b6221f8dcf281b67129510835461132b03024830ac0e554311a5c54", - "sha256:918440dea04521f499721c039863ef95433314b1db00ff826a02580c1f503e45", - "sha256:9641e21670c68c7e57d2053ddf6c443e4f0a6e18e547e86af3fad0795414a628", - "sha256:9d2f9d4cc2a53b38cabc2d6d80f7f9b7e3da26b2f53d48f05876fef7956b6968", - "sha256:a07f61fc452c43cd5328b392e52555f7d1952400a1ad09086c4a8addccbd138d", - "sha256:a3277f5fa7483c927fe3a7b017b39351610265308f5267ac6d4c2b64cc1d8d25", - "sha256:a4a3907011d39dbc3e37bdc5df0a8c93853c369039b59efa33a7b6669de04c60", - "sha256:aeb2c2688ed93b027eb0d26aa188ada34acb22dceea256d76390eea135083950", - "sha256:b094116f0b6155e36a304ff912f89bbb5067157aff5f94060ff20bbabdc8da06", - "sha256:b8ffb498a83d7e0305968289441914154fb0ef5d8b3157df02a90c6695978295", - "sha256:b9bb62fac84d5f2ff523304e59e5c439955fb3b7f44e3d7b2085184db74d733b", - "sha256:c61f66d93d712f6e03369b6a7769233bfda880b12f417eefdd4f16d1deb2fc4c", - "sha256:ca6e61dc52f601d1d224526360cdeab0d0712ec104a2ce6cc5ccef6ed9a233bc", - "sha256:ca7b26a5e456a843b9b6683eada193fc1f65c761b3a473941efe5a291f604c74", - "sha256:d12c923757de24e4e2110cf8832d83a886a4cf215c6e61ed506006872b43a6d1", - "sha256:d17bbc946f52ca67adf72a5ee783cd7cd3477f8f8796f59b4974a9b59cacc9ee", - "sha256:dfd1e1b9f0898817babf840b77ce9fe655ecbe8b1b327983df485b30df8cc011", - "sha256:e0860a348bf7004c812c8368d1fc7f77fe8e4c095d661a579196a9533778e156", - "sha256:f2f5968608b1fe2a1d00d01ad1017ee27efd99b3437e08b83ded9b7af3f6f766", - "sha256:f3771b23bb3675a06f5d885c3630b1d01ea6cac9e84a01aaf5508706dba546c5", - "sha256:f68ef3660677e6624c8cace943e4765545f8191313a07288a53d3da188bd8581", - "sha256:f86f368e1c7ce897bf2457b9eb61169a44e2ef797099fb5728482b8d69f3f016", - "sha256:f90515974b39f4dea2f27c0959688621b46d96d5a626cf9c53dbc653a895c05c", - "sha256:fe558371c1bdf3b8fa03e097c523fb9645b8730399c14fe7721ee9c9e2a545d3" + "sha256:006d220ba2e1a45f1de083d5022d4955abb0aedd78904cd5a779b955b019ec73", + "sha256:06fe398145a2e91edaf1ab4eee66149c6776c6b25b136f4a86fcbbb09512fd10", + "sha256:175f56572f25e1e1201d2b3e07b71ca4d201bf0b9cb8fad3f1dfae6a4188de86", + "sha256:18cac867950943fe93d6cd56a67eb7dcd2d4a781a40f4c1e25d6f1ed98721a55", + "sha256:1a5ee18e3a8d766075ce9314ed1cb695414bae67df6a4b0805f5137d93d6f1cb", + "sha256:20a875bfd8c282985c4720c32aa05056f77a68e6d8bbc5fe8632c5860ee0b49b", + "sha256:2412e98e70f16243be41d20836abd5f3f32edef07cbf8f407f1b6e1ceae783ac", + "sha256:2599972b21911111114100d362aea9e70a88b258400672626efa2b9e2179609c", + "sha256:2ed37e16cf35c8d6e0b430254574b8edd242a367a1b1531bd1adc99c6a5e00fe", + "sha256:32b4ab7e6c924f945cbae5392832e93e4ceb81483fd6dc4aa8fb1a97b9d3e0e1", + "sha256:34423abbaad70fea9d0164add189eabaea679068ebdf693baa5c02d03e7db244", + "sha256:3507427d83fa961cbd73f11140f4a5ce84208d31756f7238d6257b2d3d868405", + "sha256:3733545eb294e5ad274abe131d1e7e7de4ba17a144505c12feca48803fea5f64", + "sha256:3ff5bdb08d8938d336ce4088ca1a1e4b6c8cd3bef8bb3a4c0eb2f37406e49643", + "sha256:3ff7f92ae5a456101ca8f48387fd3c56eb96353588e686286f50633a611afc95", + "sha256:42a9e754aa250fe61f0f99986399cec086d7e7a01dd82fd863a20af34cbce962", + "sha256:51593a1f05c39332f623d64d910445fdec3d2ac2d96b37ce7f331882d5678ddf", + "sha256:5b11f9c6587668e495cc7365f85c93bed34c3a81f9f08b0920b87a89acc13469", + "sha256:69f1665165ba2fe7614e2f0c1aed71e14d83510bf67e2ee13df467d1c08bf1e8", + "sha256:78cdcbf7b9cb83fe047ee09298e25b1cd1636824067166dc97ad0543b079d22f", + "sha256:7df95fdd1432a5d2675ce630fef5f239939e2b3610fe2f2b5bf21fa505256fa3", + "sha256:81a5fb41b0d24447a47543b749adc34d45a2cf77b48ca74e5bf3de60a7bd9edc", + "sha256:840456cb1067dc350af9080298c7c2cfdddcedc1cb1e0b30dceecdaf7be1a2d3", + "sha256:8562ca91e8c40864942615b1d0b12289d3e745e6b2da901d133f52f2d510a1e3", + "sha256:861d75402269ffda0b33af94694b8e0703563116b04c681b1832903fac8fd647", + "sha256:8b98c89db1b150d851a7840142d60d01d07677a18f0f46836e691c38134ed18b", + "sha256:a178b7b1ac0f1530bb28d2e51f88c0bab3e5949835851a60dda80bff6052510c", + "sha256:a8ddbd158e069dded57738ea69b9744525181e99974c899b39f75b2b29a624e2", + "sha256:ac4bab32f396b03ebecfcf2971668da9275b3bb5f81b3b6ba96622f4ef3f6e17", + "sha256:ac9e95cefcf044c98d4e2c829cd0669918585755dd9a92e28a1a7012322d0a95", + "sha256:adbdfcda2469d188d79771d5696dc54fab98a16d2ef7e0875013b5f56a251047", + "sha256:b3c8bbb95a699c80a167478478efe5e09ad31680931ec280bf2087905e3b95ec", + "sha256:b3f2b1eb229f23c82898eedfc3296137cf1f16bb145ceab3edfd17cbde273fb7", + "sha256:b4ae777bebaed89e3a7e80c4a03fac434a98a8abb5251b2a957d38fe3fd30088", + "sha256:b953275d4edfab6cc0ed7139fa773dfb89e81fee1569a932f6020ce7c6da0e8f", + "sha256:bf54c3e089179d9d23900e3efc86d46e4431188d9a657f345410eecdd0151f50", + "sha256:bf711d517e21fb5bc429f5c4308fbc430a8585ff2a43e88540264ae87871e36a", + "sha256:c00e54f0bd258ab25e7f731ca1d5144b0bf7bec0051abccd2bdcff65fa3262c9", + "sha256:c11ca2df2206a4e3e4c4567f52594637392ed05d7c7fb73b4ea1c658ba560265", + "sha256:c5f9683be6a5b19cd776ee4e2f2ffb411424819c69afab6b2db3a0a364ec6642", + "sha256:cf89ab85027427d351f1de918aff4b43f4eb5f33aff6835ed30322a86ac29c9e", + "sha256:d1b750a8409bec61caa7824bfd64a8074b6d2d420433f64c161a8335796c7c6b", + "sha256:d779a48fac416387dd5673fc5b2d6bd903ed903faaa3247dc1865c65eaa5a93e", + "sha256:d9a1ef0f173e1a19738f154fb3644f90d0ada56fe6c9b422f992b04266c55d5a", + "sha256:ddb79414c15c6f03f56cc68fa06994f047cf20207c31b5dad3f6bab54a0f66ef", + "sha256:ef00d31b7569ed3cb2036f26565f1984b9fc08541731ce01012b02a4c238bf03", + "sha256:f40ac873045db4fd98a6f40387d242bde2708a3f8167bd967ccd43ad46394ba2", + "sha256:f593a4a90118d99014517c2679e04a4ef5aee2d81aa05c26c734d271065efcb6", + "sha256:f5df76c58977bc35a49515b2fbba84a1d952ff0ec784a4070334dfbec28a2def", + "sha256:f72cdd2586f9a769570d4b5714a3837b3a59a53b096bb954f1811f6a0afad305", + "sha256:f8e845d894e39fb53834da826078f6dc1a933b32b1478cf437007367efaf6f6a", + "sha256:fe6e43c8b510719b48af7db9631b5fbac910ade4bd90e6378c85ac5ac706382c" ], "markers": "python_version >= '3.8'", - "version": "==7.4.1" + "version": "==7.4.2" }, "defusedxml": { "hashes": [ @@ -2551,7 +2551,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==2.8.2" }, "pytz": { @@ -2727,7 +2727,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", "version": "==1.16.0" }, "snapshottest": { From 8dcb5a403e08cef5908a4a12eb39a24bdecb042d Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 13:21:30 +0000 Subject: [PATCH 03/22] Map request class field --- backend/Pipfile.lock | 12 ++++-------- backend/api/serializers/user.py | 7 +++++++ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 6f7b7056..efb6abe8 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "133387961394ddba0202782e2bb4265b005cd74c06be1456cf8cae8c3df2a832" + "sha256": "4e8b7da0587f1d6895c184565939556a828a99b3c5f2b3d4c827008dd0662ba6" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "cd70f4fc980eee04a029402029a1dfb52a0f1e3e" + "ref": "2282ef2fa8910659b15820a79f809e682bb52423" }, "codeforlife-portal": { "hashes": [ @@ -806,7 +806,6 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], - "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "pandas": { @@ -1305,7 +1304,6 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { @@ -1514,7 +1512,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "cd70f4fc980eee04a029402029a1dfb52a0f1e3e" + "ref": "2282ef2fa8910659b15820a79f809e682bb52423" }, "codeforlife-portal": { "hashes": [ @@ -2186,7 +2184,6 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], - "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "packaging": { @@ -2495,7 +2492,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version >= '3.7'", + "markers": "python_version < '3.10'", "version": "==7.2.1" }, "pytest-cov": { @@ -2898,7 +2895,6 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index ef726f6f..a27ccca5 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -90,6 +90,7 @@ def get_first_name(user_fields: t.Dict[str, t.Any]): class UserSerializer(_UserSerializer): student = StudentSerializer(source="new_student", required=False) teacher = TeacherSerializer(source="new_teacher", required=False) + requesting_to_join_class = serializers.CharField(required=False) current_password = serializers.CharField( write_only=True, required=False, @@ -175,6 +176,12 @@ def create(self, validated_data): return user def update(self, instance, validated_data): + if "requesting_to_join_class" in validated_data: + instance.student.pending_class_request = validated_data[ + "requesting_to_join_class" + ] + instance.student.save() + password = validated_data.get("password") if password is not None: From e4957ce0875426e5e0cd02a775b2f155d6bea3f4 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 16:32:34 +0000 Subject: [PATCH 04/22] Move field to user and update tests --- backend/Pipfile | 4 +- backend/Pipfile.lock | 18 +- backend/api/serializers/student.py | 49 ---- backend/api/serializers/user.py | 44 +++- backend/api/tests/serializers/test_student.py | 42 +--- backend/api/tests/serializers/test_user.py | 37 ++- backend/api/tests/views/test_user.py | 230 +++++++++++++++--- backend/api/views/user.py | 131 ++++++---- 8 files changed, 372 insertions(+), 183 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index a3584955..261640be 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 = "join_requests", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.13.9", 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 = "join_requests", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.13.9", 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 efb6abe8..04b2c0e8 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "4e8b7da0587f1d6895c184565939556a828a99b3c5f2b3d4c827008dd0662ba6" + "sha256": "133387961394ddba0202782e2bb4265b005cd74c06be1456cf8cae8c3df2a832" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "2282ef2fa8910659b15820a79f809e682bb52423" + "ref": "cd70f4fc980eee04a029402029a1dfb52a0f1e3e" }, "codeforlife-portal": { "hashes": [ @@ -806,6 +806,7 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], + "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "pandas": { @@ -1058,7 +1059,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.8.2" }, "pytz": { @@ -1209,7 +1210,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==1.16.0" }, "sortedcontainers": { @@ -1304,6 +1305,7 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { @@ -1512,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "2282ef2fa8910659b15820a79f809e682bb52423" + "ref": "cd70f4fc980eee04a029402029a1dfb52a0f1e3e" }, "codeforlife-portal": { "hashes": [ @@ -2184,6 +2186,7 @@ "sha256:a6f5977418eff3b2d5500d54d9db50c8277a368436f4e4f8ddb1be3422870184", "sha256:f91456ead12ab3c6c2e9491cf33ba6d08357d802192379bb482f1033ade496f5" ], + "markers": "python_version >= '3.6'", "version": "==3.1.2" }, "packaging": { @@ -2551,7 +2554,7 @@ "sha256:0123cacc1627ae19ddf3c27a5de5bd67ee4586fbdd6440d9748f8abb483d3e86", "sha256:961d03dc3453ebbc59dbdea9e4e11c5651520a876d0f4db161e8674aae935da9" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==2.8.2" }, "pytz": { @@ -2727,7 +2730,7 @@ "sha256:1e61c37477a1626458e36f7b1d82aa5c9b094fa4802892072e49de9c60c4c926", "sha256:8abb2f1d86890a2dfb989f9a77cfcfd3e47c2a354b01111771326f8aa26e0254" ], - "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2'", + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3'", "version": "==1.16.0" }, "snapshottest": { @@ -2895,6 +2898,7 @@ "sha256:6a33ee89877bd9abc1158129f6e94be74e2679636b8a205b43b85206c3f0bbdd", "sha256:f72f148f54442c6b056bf931dbc34f986fd0c3b0b6b5a58d013c9aef274d0c88" ], + "markers": "python_version >= '2.7' and python_version not in '3.0, 3.1, 3.2, 3.3, 3.4, 3.5'", "version": "==2.0.1" }, "xlwt": { diff --git a/backend/api/serializers/student.py b/backend/api/serializers/student.py index e24eea9e..f07a0d39 100644 --- a/backend/api/serializers/student.py +++ b/backend/api/serializers/student.py @@ -7,18 +7,12 @@ from codeforlife.user.models import Class, User from codeforlife.user.serializers import StudentSerializer as _StudentSerializer -from django.utils import timezone from rest_framework import serializers # pylint: disable-next=missing-class-docstring,too-many-ancestors class StudentSerializer(_StudentSerializer): klass = serializers.CharField(source="class_field.access_code") - pending_class_request = serializers.CharField( - source="pending_class_request.access_code", - required=False, - allow_blank=True, - ) class Meta(_StudentSerializer.Meta): pass @@ -48,31 +42,6 @@ def validate_klass(self, value: str): return value - def validate_pending_class_request(self, value: str): - # NOTE: Error message is purposefully ambiguous to prevent class - # enumeration. - error_message = "Class does not exist or does not accept join requests." - error_code = "does_not_exist_or_accept_join_requests" - - if value != "": - try: - klass = Class.objects.get(access_code=value) - except Class.DoesNotExist as ex: - raise serializers.ValidationError( - error_message, code=error_code - ) from ex - - if klass.accept_requests_until is None: - raise serializers.ValidationError( - error_message, code=error_code - ) - if klass.accept_requests_until < timezone.now(): - raise serializers.ValidationError( - error_message, code=error_code - ) - - return value - def validate(self, attrs): instance = t.cast(t.Optional[User], self.parent.instance) if instance and not instance.student: @@ -81,21 +50,3 @@ def validate(self, attrs): ) return attrs - - def update(self, instance, validated_data): - pending_class_request = validated_data.get("pending_class_request") - - if pending_class_request is not None: - if pending_class_request == "": - instance.pending_class_request = None - else: - instance.pending_class_request = Class.objects.get( - access_code=pending_class_request - ) - - # TODO: Send email to indy user confirming successful join request. - # TODO: Send email to teacher of selected class to notify them of - # join request. - - instance.save() - return instance diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index a27ccca5..8e3a7f2a 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -19,6 +19,7 @@ from django.contrib.auth.password_validation import ( validate_password as _validate_password, ) +from django.utils import timezone from rest_framework import serializers from .student import StudentSerializer @@ -90,7 +91,9 @@ def get_first_name(user_fields: t.Dict[str, t.Any]): class UserSerializer(_UserSerializer): student = StudentSerializer(source="new_student", required=False) teacher = TeacherSerializer(source="new_teacher", required=False) - requesting_to_join_class = serializers.CharField(required=False) + requesting_to_join_class = serializers.CharField( + required=False, allow_blank=True + ) current_password = serializers.CharField( write_only=True, required=False, @@ -102,6 +105,7 @@ class Meta(_UserSerializer.Meta): "student", "teacher", "password", + "requesting_to_join_class", "current_password", ] extra_kwargs = { @@ -147,6 +151,31 @@ def validate_password(self, value: str): return value + def validate_requesting_to_join_class(self, value: str): + # NOTE: Error message is purposefully ambiguous to prevent class + # enumeration. + error_message = "Class does not exist or does not accept join requests." + error_code = "does_not_exist_or_accept_join_requests" + + if value != "": + try: + klass = Class.objects.get(access_code=value) + except Class.DoesNotExist as ex: + raise serializers.ValidationError( + error_message, code=error_code + ) from ex + + if klass.accept_requests_until is None: + raise serializers.ValidationError( + error_message, code=error_code + ) + if klass.accept_requests_until < timezone.now(): + raise serializers.ValidationError( + error_message, code=error_code + ) + + return value + def create(self, validated_data): user = User.objects.create_user( username=validated_data["email"], @@ -177,9 +206,20 @@ def create(self, validated_data): def update(self, instance, validated_data): if "requesting_to_join_class" in validated_data: - instance.student.pending_class_request = validated_data[ + requesting_to_join_class = validated_data[ "requesting_to_join_class" ] + if requesting_to_join_class == "": + instance.student.pending_class_request = None + else: + instance.student.pending_class_request = Class.objects.get( + access_code=requesting_to_join_class + ) + + # TODO: Send email to indy user confirming successful join request. + # TODO: Send email to teacher of selected class to notify them of + # join request. + instance.student.save() password = validated_data.get("password") diff --git a/backend/api/tests/serializers/test_student.py b/backend/api/tests/serializers/test_student.py index 86683f43..ea05d291 100644 --- a/backend/api/tests/serializers/test_student.py +++ b/backend/api/tests/serializers/test_student.py @@ -6,8 +6,6 @@ from codeforlife.tests import ModelSerializerTestCase from codeforlife.user.models import ( Class, - IndependentUser, - NonSchoolTeacherUser, SchoolTeacherUser, Student, TeacherUser, @@ -19,17 +17,7 @@ # pylint: disable-next=missing-class-docstring class TestStudentSerializer(ModelSerializerTestCase[Student]): model_serializer_class = StudentSerializer - fixtures = [ - "independent", - "non_school_teacher", - "school_1", - "school_2", - ] - - def setUp(self): - self.independent = IndependentUser.objects.get(email="indy@man.com") - self.class_1 = Class.objects.get(name="Class 1 @ School 1") - self.class_3 = Class.objects.get(name="Class 3 @ School 1") + fixtures = ["school_1"] def test_validate_klass__does_not_exist(self): """ @@ -92,34 +80,6 @@ def test_validate_klass__teacher_not_admin_or_class_owner(self): ), ) - def test_validate_pending_class_request__does_not_exist(self): - """Join class request cannot be for a class that doesn't exist""" - self.assert_validate_field( - name="pending_class_request", - value="AAAAA", - error_code="does_not_exist_or_accept_join_requests", - ) - - def test_validate_pending_class_request__does_not_accept_requests(self): - """ - Join class request cannot be for a class that doesn't accept requests - """ - self.assert_validate_field( - name="pending_class_request", - value=self.class_1.access_code, - error_code="does_not_exist_or_accept_join_requests", - ) - - def test_validate_pending_class_request__no_longer_accept_requests(self): - """ - Join class request cannot be for a class that no longer accepts requests - """ - self.assert_validate_field( - name="pending_class_request", - value=self.class_3.access_code, - error_code="does_not_exist_or_accept_join_requests", - ) - def test_validate__not_student(self): """Target user must be a student.""" teacher_user = TeacherUser.objects.first() diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index 08768f96..493c9855 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -4,7 +4,7 @@ """ from codeforlife.tests import ModelSerializerTestCase -from codeforlife.user.models import Class, Student, User +from codeforlife.user.models import Class, IndependentUser, Student, User from ...serializers import UserSerializer @@ -12,7 +12,12 @@ # pylint: disable-next=missing-class-docstring class TestUserSerializer(ModelSerializerTestCase[User]): model_serializer_class = UserSerializer - fixtures = ["school_1"] + fixtures = ["independent", "school_1"] + + def setUp(self): + self.independent = IndependentUser.objects.get(email="indy@man.com") + self.class_1 = Class.objects.get(name="Class 1 @ School 1") + self.class_3 = Class.objects.get(name="Class 3 @ School 1") def test_validate__first_name_not_unique_per_class_in_data(self): """First name must be unique per class in data.""" @@ -61,3 +66,31 @@ def test_validate__first_name_not_unique_per_class_in_db(self): error_code="first_name_not_unique_per_class_in_db", many=True, ) + + def test_validate_requesting_to_join_class__does_not_exist(self): + """Join class request cannot be for a class that doesn't exist""" + self.assert_validate_field( + name="requesting_to_join_class", + value="AAAAA", + error_code="does_not_exist_or_accept_join_requests", + ) + + 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 + """ + self.assert_validate_field( + name="requesting_to_join_class", + value=self.class_1.access_code, + error_code="does_not_exist_or_accept_join_requests", + ) + + 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 + """ + self.assert_validate_field( + name="requesting_to_join_class", + value=self.class_3.access_code, + error_code="does_not_exist_or_accept_join_requests", + ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index b30a77e2..7554133f 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -2,7 +2,6 @@ © Ocado Group Created on 20/01/2024 at 10:58:52(+00:00). """ - import typing as t from codeforlife.permissions import OR @@ -101,7 +100,7 @@ def test_get_permissions__partial_update__student(self): request=self.client.request_factory.patch(data={"student": {}}), ) - def test_get_permissions__partial_update__student__pending_class_request( + def test_get_permissions__partial_update__requesting_to_join_class( self, ): """ @@ -117,22 +116,20 @@ def test_get_permissions__partial_update__student__pending_class_request( ], action="partial_update", request=self.client.request_factory.patch( - data={"student": {"pending_class_request": ""}} + data={"requesting_to_join_class": ""} ), ) - def test_get_permissions__student__reject_join_request(self): + def test_get_permissions__handle_join_class_request(self): """ - Only school-teachers can reject an independent's class join request. + Only school-teachers can handle an independent's class join request. """ self.assert_get_permissions( permissions=[ OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)) ], - action="student__reject_join_request", - request=self.client.request_factory.patch( - data={"student": {"pending_class_request": None}} - ), + action="handle_join_class_request", + request=self.client.request_factory.patch(data={"accept": False}), ) # test: get queryset @@ -217,17 +214,20 @@ def test_bulk_destroy(self): # test: class join request actions - def test_student__reject_join_request__invalid_school(self): - """Teacher cannot reject a join request outside their school""" + def test_handle_join_class_request__invalid_school(self): + """Teacher cannot handle a join request outside their school""" self.client.login_as(self.admin_school2_teacher_user) viewname = self.reverse_action( - "student--reject-join-request", - kwargs={"pk": self.indy_with_join_request.new_student.pk}, + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, ) response = self.client.patch( - viewname, status_code_assertion=status.HTTP_400_BAD_REQUEST + viewname, + data={"accept": False}, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", ) assert response.data["non_field_errors"] == [ @@ -240,17 +240,20 @@ def test_student__reject_join_request__invalid_school(self): == self.class_2 ) - def test_student__reject_join_request__invalid_class(self): + def test_handle_join_class_request__invalid_class(self): """Non-admin teacher cannot reject a join request outside their class""" self.client.login_as(self.non_admin_school_teacher_user) viewname = self.reverse_action( - "student--reject-join-request", - kwargs={"pk": self.indy_with_join_request.new_student.pk}, + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, ) response = self.client.patch( - viewname, status_code_assertion=status.HTTP_400_BAD_REQUEST + viewname, + data={"accept": False}, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", ) assert response.data["non_field_errors"] == [ @@ -263,16 +266,73 @@ def test_student__reject_join_request__invalid_class(self): == self.class_2 ) - def test_student__reject_join_request(self): - """Teacher can reject an independent's request to join a class.""" + def test_handle_join_class_request__invalid_accept(self): + """Teacher cannot handle join class request with wrong accept type""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "student--reject-join-request", - kwargs={"pk": self.indy_with_join_request.new_student.pk}, + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + response = self.client.patch( + viewname, + data={"accept": "lol"}, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", ) - self.client.patch(viewname, status_code_assertion=status.HTTP_200_OK) + assert response.data["non_field_errors"] == [ + "Invalid type for accept - must be True or False." + ] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_handle_join_class_request__missing_accept(self): + """Teacher cannot handle join class request with missing accept""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + response = self.client.patch( + viewname, + data={}, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", + ) + + assert response.data["non_field_errors"] == [ + "Accept field is required." + ] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_handle_join_class_request__reject(self): + """Teacher can successfully reject a join class request.""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + self.client.patch( + viewname, + data={"accept": False}, + status_code_assertion=status.HTTP_200_OK, + format="json", + ) self.indy_with_join_request.refresh_from_db() assert ( @@ -280,6 +340,119 @@ def test_student__reject_join_request(self): is None ) + def test_handle_join_class_request__accept__invalid_first_name(self): + """Teacher cannot accept a join class request with invalid name""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + response = self.client.patch( + viewname, + data={"accept": True, "first_name": 1}, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", + ) + + assert response.data["first_name"] == ["First name must be a string."] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_handle_join_class_request__accept__missing_first_name(self): + """Teacher cannot accept a join class request with missing name""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + response = self.client.patch( + viewname, + data={"accept": True}, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", + ) + + assert response.data["first_name"] == ["This field is required."] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_handle_join_class_request__accept__duplicate_first_name(self): + """Teacher cannot accept a join class request with duplicate name""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + response = self.client.patch( + viewname, + data={ + "accept": True, + "first_name": self.class_2.students.first().new_user.first_name, + }, + status_code_assertion=status.HTTP_400_BAD_REQUEST, + format="json", + ) + + assert response.data["first_name"] == [ + "This name already exists in the class. " + "Please choose a different name." + ] + + self.indy_with_join_request.refresh_from_db() + assert ( + self.indy_with_join_request.new_student.pending_class_request + == self.class_2 + ) + + def test_handle_join_class_request__accept(self): + """Teacher can successfully accept a join class request.""" + self.client.login_as(self.admin_school_teacher_user) + + viewname = self.reverse_action( + "handle-join-class-request", + kwargs={"pk": self.indy_with_join_request.pk}, + ) + + self.client.patch( + viewname, + data={ + "accept": True, + "first_name": self.indy_with_join_request.first_name, + }, + status_code_assertion=status.HTTP_200_OK, + format="json", + ) + + self.indy_with_join_request.refresh_from_db() + + assert ( + self.indy_with_join_request.new_student.class_field == self.class_2 + ) + assert ( + self.indy_with_join_request.new_student.pending_class_request + is None + ) + assert self.indy_with_join_request.last_name == "" + assert self.indy_with_join_request.email == "" + assert ( + self.indy_with_join_request.username + != self.indy_with_join_request_email + ) + # test: reset password actions def test_request_password_reset__invalid_email(self): @@ -473,7 +646,7 @@ def test_partial_update__indy__send_join_request(self): self.client.partial_update( indy_user, - {"student": {"pending_class_request": self.class_2.access_code}}, + {"requesting_to_join_class": self.class_2.access_code}, ) def test_partial_update__indy__revoke_join_request(self): @@ -484,12 +657,5 @@ def test_partial_update__indy__revoke_join_request(self): self.client.partial_update( indy_user_with_join_request, - {"student": {"pending_class_request": ""}}, - ) - - indy_user_with_join_request.refresh_from_db() - indy_user_with_join_request.new_student.refresh_from_db() - assert ( - indy_user_with_join_request.new_student.pending_class_request - is None + {"requesting_to_join_class": ""}, ) diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 0fe2b245..059ef4a8 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -15,6 +15,7 @@ PasswordResetTokenGenerator, default_token_generator, ) +from django.utils.crypto import get_random_string from rest_framework import status from rest_framework.decorators import action from rest_framework.permissions import AllowAny @@ -37,19 +38,16 @@ def get_permissions(self): if self.action == "partial_update": if "teacher" in self.request.data: return [IsTeacher(is_admin=True)] - if ( - "student" in self.request.data - and "pending_class_request" in self.request.data["student"] - ): + if "student" in self.request.data: + return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] + if "requesting_to_join_class" in self.request.data: return [ OR( OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), IsIndependent(), ) ] - if "student" in self.request.data: - return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] - if self.action == "student__reject_join_request": + if self.action == "handle_join_class_request": return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() @@ -181,56 +179,22 @@ def students__reset_password(self, request: Request): return Response(fields) @action( - detail=True, methods=["patch"], url_path="student/reject-join-request" - ) - def student__reject_join_request( - self, request: Request, pk: t.Optional[str] = None - ): - """Reject an independent user's request to join a class.""" - indy = self._get_student_requesting_join_if_authorised(request, pk) - - indy.pending_class_request = None - indy.save() - - return Response() - - @action( - detail=True, methods=["patch"], url_path="student/accept-join-request" + detail=True, methods=["patch"], url_path="handle-join-class-request" ) - def student__accept_join_request( - self, request: Request, pk: t.Optional[str] = None - ): - """Accept an independent user's request to join a class.""" - self._get_student_requesting_join_if_authorised(request, pk) - - # indy.pending_class_request = None - # indy.save() - - # TODO: Redirect to next step which is renaming student - return Response() - - @action(detail=True, methods=["patch"], url_path="student/add-to-class") - def student__add_to_class( + def handle_join_class_request( self, request: Request, pk: t.Optional[str] = None ): - """Add an independent user to a class""" - return Response() - - def _get_student_requesting_join_if_authorised( - self, - request: Request, - pk: str, - ) -> Student: + """Handle an independent user's request to join a class.""" try: - indy = Student.objects.get(pk=int(pk)) + indy = User.objects.get(pk=int(pk)) except (ValueError, Student.DoesNotExist): return Response( - {"non_field_errors": ["No student found for given ID."]}, + {"non_field_errors": ["No user found for given ID."]}, status=status.HTTP_400_BAD_REQUEST, ) klass = Class.objects.get( - access_code=indy.pending_class_request.access_code + access_code=indy.student.pending_class_request.access_code ) if klass.teacher.school != request.user.new_teacher.school: @@ -257,4 +221,75 @@ def _get_student_requesting_join_if_authorised( status=status.HTTP_400_BAD_REQUEST, ) - return indy + try: + if not isinstance(request.data["accept"], bool): + return Response( + { + "non_field_errors": [ + "Invalid type for accept - must be True or False." + ] + }, + status=status.HTTP_400_BAD_REQUEST, + ) + except KeyError: + return Response( + {"non_field_errors": ["Accept field is required."]}, + status=status.HTTP_400_BAD_REQUEST, + ) + + request_accepted = request.data["accept"] + + if request_accepted: + try: + if not isinstance(request.data["first_name"], str): + return Response( + {"first_name": ["First name must be a string."]}, + status=status.HTTP_400_BAD_REQUEST, + ) + except KeyError: + return Response( + {"first_name": ["This field is required."]}, + status=status.HTTP_400_BAD_REQUEST, + ) + + name = request.data["first_name"] + if self._is_name_unique_in_class(name, klass): + indy.student.class_field = indy.student.pending_class_request + indy.student.pending_class_request = None + + username = None + + while ( + username is None + or User.objects.filter(username=username).exists() + ): + username = get_random_string(length=30) + + indy.username = username + indy.first_name = name + indy.last_name = "" + indy.email = "" + + indy.student.save() + indy.save() + else: + return Response( + { + "first_name": [ + "This name already exists in the class. " + "Please choose a different name." + ] + }, + status=status.HTTP_400_BAD_REQUEST, + ) + else: + indy.student.pending_class_request = None + indy.student.save() + + return Response() + + def _is_name_unique_in_class(self, name: str, klass: Class) -> bool: + """Check if a user's name is unique in a class""" + return not Student.objects.filter( + class_field=klass, new_user__first_name__iexact=name + ).exists() From 46d10f160bbdbd723a65faad093755233efa718a Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 18:49:43 +0000 Subject: [PATCH 05/22] Strengthen validation and add tests --- backend/api/serializers/user.py | 76 ++++++++++++++++++++-- backend/api/tests/serializers/test_user.py | 32 ++++++++- backend/api/tests/views/test_user.py | 1 + 3 files changed, 100 insertions(+), 9 deletions(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index 8e3a7f2a..712ade09 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -118,14 +118,71 @@ class Meta(_UserSerializer.Meta): list_serializer_class = UserListSerializer def validate(self, attrs): - if self.instance is not None and self.view.action != "reset-password": - # TODO: make current password required when changing self-profile. - pass + if self.instance: # Updating + if self.view.action != "reset_password": + # TODO: make current password required when changing + # self-profile. + pass + + if self.instance.teacher: + if "last_name" in attrs and not attrs["last_name"]: + raise serializers.ValidationError( + "Last name is required.", code="last_name_required" + ) - if "new_teacher" in attrs and "last_name" not in attrs: - raise serializers.ValidationError( - "Last name is required.", code="last_name_required" - ) + if "requesting_to_join_class" in attrs: + raise serializers.ValidationError( + "Teacher can't request to join a class.", + code="teacher_cannot_request_to_join_class", + ) + + if self.instance.student: + user = self.request_student_user + if ( + self.view.action != "reset_password" + and user.pk != self.instance.pk + ): + raise serializers.ValidationError( + "Cannot update another student.", code="is_not_self" + ) + + if ( + self.instance.student.class_field is not None + and "requesting_to_join_class" in attrs + ) or ( + self.instance.student.pending_class_request is not None + and "new_student" in attrs + and "class_field" in attrs["new_student"] + ): + raise serializers.ValidationError( + "Student can't be in a class and requesting to join a " + "class at the same time.", + code="class_and_join_class_mutually_exclusive", + ) + + else: # Creating + if "new_teacher" in attrs: + if "last_name" not in attrs or attrs["last_name"] is None: + raise serializers.ValidationError( + "Last name is required.", code="last_name_required" + ) + + if "requesting_to_join_class" in attrs: + raise serializers.ValidationError( + "Teacher can't request to join a class.", + code="teacher_cannot_request_to_join_class", + ) + + if "new_student" in attrs: + if ( + "class_field" in attrs["new_student"] + and "requesting_to_join_class" in attrs + ): + raise serializers.ValidationError( + "Student can't be in a class and requesting to join a " + "class at the same time.", + code="class_and_join_class_mutually_exclusive", + ) return attrs @@ -243,4 +300,9 @@ def to_representation(self, instance: User): if password is not None: representation["password"] = password + if representation["student"] is not None: + representation[ + "requesting_to_join_class" + ] = instance.student.pending_class_request + return representation diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index 493c9855..3979699a 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -2,7 +2,6 @@ © Ocado Group Created on 31/01/2024 at 16:07:32(+00:00). """ - from codeforlife.tests import ModelSerializerTestCase from codeforlife.user.models import Class, IndependentUser, Student, User @@ -67,13 +66,42 @@ def test_validate__first_name_not_unique_per_class_in_db(self): many=True, ) + def test_validate__teacher__first_name_required(self): + """Teacher must have a first name""" + self.assert_validate( + attrs={"new_teacher": {}}, error_code="last_name_required" + ) + + def test_validate__teacher__requesting_to_join_class_forbidden(self): + """Teacher cannot request to join a class""" + self.assert_validate( + attrs={ + "last_name": "Name", + "new_teacher": {}, + "requesting_to_join_class": "AAAAA", + }, + error_code="teacher_cannot_request_to_join_class", + ) + + def test_validate__student__requesting_to_join_class_and_class_field_forbidden( + self, + ): + """Student cannot have both requesting_to_join_class and class_field""" + self.assert_validate( + attrs={ + "new_student": {"class_field": "AAAAA"}, + "requesting_to_join_class": "BBBBB", + }, + error_code="class_and_join_class_mutually_exclusive", + ) + def test_validate_requesting_to_join_class__does_not_exist(self): """Join class request cannot be for a class that doesn't exist""" self.assert_validate_field( name="requesting_to_join_class", value="AAAAA", error_code="does_not_exist_or_accept_join_requests", - ) + ) def test_validate_requesting_to_join_class__does_not_accept_requests(self): """ diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 7554133f..18368609 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -49,6 +49,7 @@ def setUp(self): self.admin_school2_teacher_user = AdminSchoolTeacherUser.objects.get( email="admin.teacher@school2.com" ) + self.indy = IndependentUser.objects.get(email=self.indy_email) self.indy_with_join_request = IndependentUser.objects.get( email=self.indy_with_join_request_email ) From d25b386732906eb1880d6c8f2486af6d0dfa7143 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 18:50:40 +0000 Subject: [PATCH 06/22] Remove unnecessary code --- backend/api/serializers/user.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index 712ade09..6b3da82e 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -300,9 +300,4 @@ def to_representation(self, instance: User): if password is not None: representation["password"] = password - if representation["student"] is not None: - representation[ - "requesting_to_join_class" - ] = instance.student.pending_class_request - return representation From 8d88145b96a22514d5f59664d2b1a12a511b7386 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 21 Feb 2024 18:53:19 +0000 Subject: [PATCH 07/22] Update lockfile --- backend/Pipfile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 261640be..372cfc58 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.9", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.13.10", 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.9", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.13.10", 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" From 996d263c2c6424c02768a340a44a8fda59c941a9 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 22 Feb 2024 00:40:06 +0000 Subject: [PATCH 08/22] Ola, housekeeping --- backend/Pipfile.lock | 8 ++-- backend/api/serializers/auth_factor.py | 6 +-- backend/api/serializers/klass.py | 11 ++++-- .../serializers/school_teacher_invitation.py | 2 +- backend/api/serializers/student.py | 2 +- backend/api/serializers/teacher.py | 2 +- backend/api/serializers/user.py | 39 ++++++++----------- backend/api/tests/views/test_user.py | 12 +----- backend/api/views/auth_factor.py | 2 +- .../api/views/school_teacher_invitation.py | 2 +- backend/api/views/user.py | 21 ++++++---- 11 files changed, 53 insertions(+), 54 deletions(-) diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 04b2c0e8..a05f6b97 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "133387961394ddba0202782e2bb4265b005cd74c06be1456cf8cae8c3df2a832" + "sha256": "c3b11b9d8699621ec2071fed98072adb143c11ae3dd29b642c565fb491add82d" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "cd70f4fc980eee04a029402029a1dfb52a0f1e3e" + "ref": "9f3c8292be7400cef9255cdf6283f9bb7edea955" }, "codeforlife-portal": { "hashes": [ @@ -1514,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "cd70f4fc980eee04a029402029a1dfb52a0f1e3e" + "ref": "9f3c8292be7400cef9255cdf6283f9bb7edea955" }, "codeforlife-portal": { "hashes": [ @@ -2495,7 +2495,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version < '3.10'", + "markers": "python_version >= '3.7'", "version": "==7.2.1" }, "pytest-cov": { diff --git a/backend/api/serializers/auth_factor.py b/backend/api/serializers/auth_factor.py index 09b5ad20..01b0bf00 100644 --- a/backend/api/serializers/auth_factor.py +++ b/backend/api/serializers/auth_factor.py @@ -24,7 +24,7 @@ class Meta: # pylint: disable-next=missing-function-docstring def validate_type(self, value: str): if AuthFactor.objects.filter( - user=self.request_user, type=value + user=self.request.user, type=value ).exists(): raise serializers.ValidationError( "You already have this authentication factor enabled.", @@ -34,7 +34,7 @@ def validate_type(self, value: str): return value def create(self, validated_data): - user = self.request_user + user = self.request.user if not user.userprofile.otp_secret: user.userprofile.otp_secret = pyotp.random_base32() user.userprofile.save() @@ -51,6 +51,6 @@ def to_representation(self, instance): ): representation[ "totp_provisioning_uri" - ] = self.request_user.totp_provisioning_uri + ] = self.request.user.totp_provisioning_uri return representation diff --git a/backend/api/serializers/klass.py b/backend/api/serializers/klass.py index 4daf43c0..24ebb075 100644 --- a/backend/api/serializers/klass.py +++ b/backend/api/serializers/klass.py @@ -42,7 +42,7 @@ def validate_teacher(self, value: int): code="does_not_exist", ) - user = self.request_school_teacher_user + user = self.request.school_teacher_user if not queryset.filter(school=user.teacher.school_id).exists(): raise serializers.ValidationError( "This teacher is not in your school.", @@ -60,7 +60,7 @@ def validate_teacher(self, value: int): # pylint: disable-next=missing-function-docstring def validate_name(self, value: str): if Class.objects.filter( - teacher__school=self.request_school_teacher_user.teacher.school, + teacher__school=self.request.school_teacher_user.teacher.school, name=value, ).exists(): raise serializers.ValidationError( @@ -90,7 +90,7 @@ def create(self, validated_data): "teacher_id": ( validated_data["teacher"]["id"] if "teacher" in validated_data - else self.request_school_teacher_user.teacher.id + else self.request.school_teacher_user.teacher.id ), "classmates_data_viewable": validated_data[ "classmates_data_viewable" @@ -102,6 +102,11 @@ def create(self, validated_data): ) def update(self, instance, validated_data): + # TODO: Decide how to handle this field. Currently, teachers select a + # setting which equates to a number of "hours", then we calculate + # how long the class can accept requests for based on that number of + # hours. It's not the most elegant solution, so for now I've written + # this assuming that the user will input a date-time value. accept_requests_until = validated_data.get("accept_requests_until") if accept_requests_until is not None: diff --git a/backend/api/serializers/school_teacher_invitation.py b/backend/api/serializers/school_teacher_invitation.py index 3f94a961..c2b20394 100644 --- a/backend/api/serializers/school_teacher_invitation.py +++ b/backend/api/serializers/school_teacher_invitation.py @@ -39,7 +39,7 @@ class Meta: } def create(self, validated_data): - user = self.request_admin_school_teacher_user + user = self.request.admin_school_teacher_user token = get_random_string(length=32) validated_data["token"] = make_password(token) diff --git a/backend/api/serializers/student.py b/backend/api/serializers/student.py index f07a0d39..83a65c3c 100644 --- a/backend/api/serializers/student.py +++ b/backend/api/serializers/student.py @@ -20,7 +20,7 @@ class Meta(_StudentSerializer.Meta): # pylint: disable-next=missing-function-docstring def validate_klass(self, value: str): # Only teachers can manage students. - teacher = self.request_school_teacher_user.teacher + teacher = self.request.school_teacher_user.teacher try: klass = Class.objects.get(access_code=value) diff --git a/backend/api/serializers/teacher.py b/backend/api/serializers/teacher.py index ef868e69..c3d5e719 100644 --- a/backend/api/serializers/teacher.py +++ b/backend/api/serializers/teacher.py @@ -22,7 +22,7 @@ class Meta(_TeacherSerializer.Meta): def validate_is_admin(self, value: bool): instance = t.cast(t.Optional[User], self.parent.instance) if instance: - user = self.request_school_teacher_user + user = self.request.school_teacher_user if user.pk == instance.pk: raise serializers.ValidationError( "Cannot update own permissions.", diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index 6b3da82e..6a3ea1b8 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -137,14 +137,11 @@ def validate(self, attrs): ) if self.instance.student: - user = self.request_student_user - if ( - self.view.action != "reset_password" - and user.pk != self.instance.pk - ): - raise serializers.ValidationError( - "Cannot update another student.", code="is_not_self" - ) + if self.view.action != "reset_password": + if self.request.student_user.pk != self.instance.pk: + raise serializers.ValidationError( + "Cannot update another student.", code="is_not_self" + ) if ( self.instance.student.class_field is not None @@ -187,10 +184,7 @@ def validate(self, attrs): return attrs def validate_password(self, value: str): - """ - Validate the new password depending on user type. - """ - + """Validate the new password depending on user type.""" # If we're creating a new user, we do not yet have the user object. # Therefore, we need to create a dummy user and pass it to the password # validators so they know what type of user we have. @@ -222,11 +216,10 @@ def validate_requesting_to_join_class(self, value: str): error_message, code=error_code ) from ex - if klass.accept_requests_until is None: - raise serializers.ValidationError( - error_message, code=error_code - ) - if klass.accept_requests_until < timezone.now(): + if ( + klass.accept_requests_until is None + or klass.accept_requests_until < timezone.now() + ): raise serializers.ValidationError( error_message, code=error_code ) @@ -262,10 +255,12 @@ def create(self, validated_data): return user def update(self, instance, validated_data): - if "requesting_to_join_class" in validated_data: - requesting_to_join_class = validated_data[ - "requesting_to_join_class" - ] + requesting_to_join_class = validated_data.get( + "requesting_to_join_class" + ) + if requesting_to_join_class is not None: + # If value is empty, this means independent wants to revoke their + # join request if requesting_to_join_class == "": instance.student.pending_class_request = None else: @@ -293,7 +288,7 @@ def to_representation(self, instance: User): # Return student's auto-generated password. if ( representation["student"] is not None - and self.request_user.teacher is not None + and self.request.auth_user.teacher is not None ): # pylint: disable-next=protected-access password = instance._password diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 18368609..f4761c48 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -104,17 +104,9 @@ def test_get_permissions__partial_update__student(self): def test_get_permissions__partial_update__requesting_to_join_class( self, ): - """ - Only school-teachers and independents can update an independent's class - join request. - """ + """Only independents can update their class join request.""" self.assert_get_permissions( - permissions=[ - OR( - OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), - IsIndependent(), - ) - ], + permissions=[IsIndependent()], action="partial_update", request=self.client.request_factory.patch( data={"requesting_to_join_class": ""} diff --git a/backend/api/views/auth_factor.py b/backend/api/views/auth_factor.py index f417efb4..12883eb3 100644 --- a/backend/api/views/auth_factor.py +++ b/backend/api/views/auth_factor.py @@ -17,7 +17,7 @@ class AuthFactorViewSet(ModelViewSet[AuthFactor]): serializer_class = AuthFactorSerializer def get_queryset(self): - return AuthFactor.objects.filter(user=self.request_user) + return AuthFactor.objects.filter(user=self.request.user) def get_permissions(self): if self.action in ["retrieve", "bulk"]: diff --git a/backend/api/views/school_teacher_invitation.py b/backend/api/views/school_teacher_invitation.py index bff91cb3..1d9c2ed2 100644 --- a/backend/api/views/school_teacher_invitation.py +++ b/backend/api/views/school_teacher_invitation.py @@ -43,7 +43,7 @@ def get_queryset(self): return queryset return queryset.filter( - school=self.request_admin_school_teacher_user.teacher.school + school=self.request.admin_school_teacher_user.teacher.school ) @action( diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 059ef4a8..160b1eb8 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -41,12 +41,7 @@ def get_permissions(self): if "student" in self.request.data: return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] if "requesting_to_join_class" in self.request.data: - return [ - OR( - OR(IsTeacher(is_admin=True), IsTeacher(in_class=True)), - IsIndependent(), - ) - ] + return [IsIndependent()] if self.action == "handle_join_class_request": return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] @@ -184,7 +179,16 @@ def students__reset_password(self, request: Request): def handle_join_class_request( self, request: Request, pk: t.Optional[str] = None ): - """Handle an independent user's request to join a class.""" + """ + 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 + class' school, or the teacher of that specific school. The request + must then specify whether the teacher accepts or rejects the join + request by setting the boolean "accept". If "accept" is True, + then the request must contain the new student's first_name, ensuring + that it is unique in the class. + """ try: indy = User.objects.get(pk=int(pk)) except (ValueError, Student.DoesNotExist): @@ -286,6 +290,9 @@ def handle_join_class_request( indy.student.pending_class_request = None indy.student.save() + # TODO: Send independent user an email notifying them that their + # request has been rejected. + return Response() def _is_name_unique_in_class(self, name: str, klass: Class) -> bool: From d1db279fbdc603ce224827600f547c9ff92a0161 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 22 Feb 2024 11:39:00 +0000 Subject: [PATCH 09/22] Fix tests --- backend/api/tests/views/test_user.py | 30 +++++++++++----------------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 25addcd5..e4239d94 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -52,7 +52,7 @@ def setUp(self): self.admin_school2_teacher_user = AdminSchoolTeacherUser.objects.get( email="admin.teacher@school2.com" ) - self.indy = IndependentUser.objects.get(email="indy@man.com") + self.indy_user = IndependentUser.objects.get(email="indy@man.com") self.indy_with_join_request = IndependentUser.objects.get( email="indy@requester.com" ) @@ -432,6 +432,8 @@ def test_handle_join_class_request__accept(self): """Teacher can successfully accept a join class request.""" self.client.login_as(self.admin_school_teacher_user) + indy_email = self.indy_with_join_request.email + viewname = self.reverse_action( "handle-join-class-request", kwargs={"pk": self.indy_with_join_request.pk}, @@ -458,10 +460,7 @@ def test_handle_join_class_request__accept(self): ) assert self.indy_with_join_request.last_name == "" assert self.indy_with_join_request.email == "" - assert ( - self.indy_with_join_request.username - != self.indy_with_join_request_email - ) + assert self.indy_with_join_request.username != indy_email # test: reset password actions @@ -627,15 +626,14 @@ def test_students__reset_password(self): def test_partial_update__teacher(self): """Admin-school-teacher can update another teacher's profile.""" - admin_school_teacher_user = self.client.login_admin_school_teacher( - email="admin.teacher@school1.com", password="password" - ) + self.client.login_as(self.admin_school_teacher_user) other_school_teacher_user = ( SchoolTeacherUser.objects.filter( - new_teacher__school=admin_school_teacher_user.teacher.school + new_teacher__school=self.admin_school_teacher_user.teacher + .school ) - .exclude(pk=admin_school_teacher_user.pk) + .exclude(pk=self.admin_school_teacher_user.pk) .first() ) assert other_school_teacher_user @@ -652,23 +650,19 @@ def test_partial_update__teacher(self): def test_partial_update__indy__send_join_request(self): """Independent user can request to join a class.""" - indy_user = self.client.login_indy( - email=self.indy_email, password="password" - ) + self.client.login_as(self.indy_user) self.client.partial_update( - indy_user, + self.indy_user, {"requesting_to_join_class": self.class_2.access_code}, ) def test_partial_update__indy__revoke_join_request(self): """Independent user can revoke their request to join a class.""" - indy_user_with_join_request = self.client.login_indy( - email=self.indy_with_join_request_email, password="password" - ) + self.client.login_as(self.indy_with_join_request) self.client.partial_update( - indy_user_with_join_request, + self.indy_with_join_request, {"requesting_to_join_class": ""}, ) From 2517d8b4b6830c1d5ff315b3649d643d664e1cb4 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 22 Feb 2024 11:54:55 +0000 Subject: [PATCH 10/22] Simplify tests --- ...pendent_school_1_class_1_join_request.json | 11 ++ backend/api/tests/views/test_klass.py | 7 +- backend/api/tests/views/test_user.py | 136 +++++++----------- 3 files changed, 61 insertions(+), 93 deletions(-) create mode 100644 backend/api/fixtures/independent_school_1_class_1_join_request.json diff --git a/backend/api/fixtures/independent_school_1_class_1_join_request.json b/backend/api/fixtures/independent_school_1_class_1_join_request.json new file mode 100644 index 00000000..230d0baf --- /dev/null +++ b/backend/api/fixtures/independent_school_1_class_1_join_request.json @@ -0,0 +1,11 @@ +[ + { + "model": "common.student", + "pk": 18, + "fields": { + "user": 28, + "new_user": 28, + "pending_class_request": 7 + } + } +] diff --git a/backend/api/tests/views/test_klass.py b/backend/api/tests/views/test_klass.py index 17180b83..a3c1596d 100644 --- a/backend/api/tests/views/test_klass.py +++ b/backend/api/tests/views/test_klass.py @@ -2,7 +2,6 @@ © Ocado Group Created on 05/02/2024 at 16:13:46(+00:00). """ - from datetime import timedelta from codeforlife.permissions import OR, AllowNone @@ -78,10 +77,7 @@ def test_get_permissions__retrieve(self): ) def test_create__self(self): - """ - Teacher can create a class with themself as the class owner. - """ - + """Teacher can create a class with themselves as the class owner.""" user = self.client.login_school_teacher( email="teacher@school1.com", password="password", @@ -100,7 +96,6 @@ def test_create__other(self): """ Teacher can create a class with another teacher as the class owner. """ - user = self.client.login_admin_school_teacher( email="admin.teacher@school1.com", password="password", diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index e4239d94..ab3c4240 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -37,7 +37,13 @@ class TestUserViewSet(ModelViewSetTestCase[User]): basename = "user" model_view_set_class = UserViewSet - fixtures = ["independent", "non_school_teacher", "school_1", "school_2"] + fixtures = [ + "independent", + "independent_school_1_class_1_join_request", + "non_school_teacher", + "school_1", + "school_2", + ] def setUp(self): self.non_school_teacher_user = NonSchoolTeacherUser.objects.get( @@ -53,9 +59,6 @@ def setUp(self): email="admin.teacher@school2.com" ) self.indy_user = IndependentUser.objects.get(email="indy@man.com") - self.indy_with_join_request = IndependentUser.objects.get( - email="indy@requester.com" - ) self.class_2 = Class.objects.get(name="Class 2 @ School 1") def _get_pk_and_token_for_user(self, email: str): @@ -229,8 +232,7 @@ def test_handle_join_class_request__invalid_school(self): self.client.login_as(self.admin_school2_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -244,19 +246,15 @@ def test_handle_join_class_request__invalid_school(self): "This class join request is not in your school." ] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__invalid_class(self): """Non-admin teacher cannot reject a join request outside their class""" self.client.login_as(self.non_admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -270,19 +268,15 @@ def test_handle_join_class_request__invalid_class(self): "This class join request is not for one for your classes." ] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__invalid_accept(self): """Teacher cannot handle join class request with wrong accept type""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -296,19 +290,15 @@ def test_handle_join_class_request__invalid_accept(self): "Invalid type for accept - must be True or False." ] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__missing_accept(self): """Teacher cannot handle join class request with missing accept""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -322,19 +312,15 @@ def test_handle_join_class_request__missing_accept(self): "Accept field is required." ] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__reject(self): """Teacher can successfully reject a join class request.""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) self.client.patch( @@ -344,19 +330,15 @@ def test_handle_join_class_request__reject(self): format="json", ) - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - is None - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request is None def test_handle_join_class_request__accept__invalid_first_name(self): """Teacher cannot accept a join class request with invalid name""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -368,19 +350,15 @@ def test_handle_join_class_request__accept__invalid_first_name(self): assert response.data["first_name"] == ["First name must be a string."] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__accept__missing_first_name(self): """Teacher cannot accept a join class request with missing name""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -392,19 +370,15 @@ def test_handle_join_class_request__accept__missing_first_name(self): assert response.data["first_name"] == ["This field is required."] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__accept__duplicate_first_name(self): """Teacher cannot accept a join class request with duplicate name""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) response = self.client.patch( @@ -422,45 +396,36 @@ def test_handle_join_class_request__accept__duplicate_first_name(self): "Please choose a different name." ] - self.indy_with_join_request.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.pending_class_request - == self.class_2 - ) + self.indy_user.refresh_from_db() + assert self.indy_user.new_student.pending_class_request == self.class_2 def test_handle_join_class_request__accept(self): """Teacher can successfully accept a join class request.""" self.client.login_as(self.admin_school_teacher_user) - indy_email = self.indy_with_join_request.email + indy_email = self.indy_user.email viewname = self.reverse_action( - "handle-join-class-request", - kwargs={"pk": self.indy_with_join_request.pk}, + "handle-join-class-request", kwargs={"pk": self.indy_user.pk} ) self.client.patch( viewname, data={ "accept": True, - "first_name": self.indy_with_join_request.first_name, + "first_name": self.indy_user.first_name, }, status_code_assertion=status.HTTP_200_OK, format="json", ) - self.indy_with_join_request.refresh_from_db() + self.indy_user.refresh_from_db() - assert ( - self.indy_with_join_request.new_student.class_field == self.class_2 - ) - assert ( - self.indy_with_join_request.new_student.pending_class_request - is None - ) - assert self.indy_with_join_request.last_name == "" - assert self.indy_with_join_request.email == "" - assert self.indy_with_join_request.username != indy_email + assert self.indy_user.new_student.class_field == self.class_2 + assert self.indy_user.new_student.pending_class_request is None + assert self.indy_user.last_name == "" + assert self.indy_user.email == "" + assert self.indy_user.username != indy_email # test: reset password actions @@ -571,8 +536,7 @@ def test_reset_password__patch__indy(self): pk, token = self._get_pk_and_token_for_user(self.indy_user.email) viewname = self.reverse_action( - "reset-password", - kwargs={"pk": pk, "token": token}, + "reset-password", kwargs={"pk": pk, "token": token} ) self.client.patch(viewname, data={"password": "N3wPassword"}) @@ -630,8 +594,7 @@ def test_partial_update__teacher(self): other_school_teacher_user = ( SchoolTeacherUser.objects.filter( - new_teacher__school=self.admin_school_teacher_user.teacher - .school + new_teacher__school=self.admin_school_teacher_user.teacher.school ) .exclude(pk=self.admin_school_teacher_user.pk) .first() @@ -659,11 +622,10 @@ def test_partial_update__indy__send_join_request(self): def test_partial_update__indy__revoke_join_request(self): """Independent user can revoke their request to join a class.""" - self.client.login_as(self.indy_with_join_request) + self.client.login_as(self.indy_user) self.client.partial_update( - self.indy_with_join_request, - {"requesting_to_join_class": ""}, + self.indy_user, {"requesting_to_join_class": ""} ) def assert_user_is_anonymized(self, user: User): @@ -717,7 +679,7 @@ def _test_destroy( ) def test_destroy__class_teacher(self): - """Class-teacher-users can anonymize themself and their classes.""" + """Class-teacher-users can anonymize themselves and their classes.""" user = self.non_admin_school_teacher_user assert user.teacher.class_teacher.exists() class_names = list( @@ -731,8 +693,8 @@ def test_destroy__class_teacher(self): def test_destroy__school_teacher__last_teacher(self): """ - School-teacher-users can anonymize themself and their school if they are - the last teacher. + School-teacher-users can anonymize themselves and their school if they + are the last teacher. """ user = self.admin_school_teacher_user assert user.teacher.class_teacher.exists() @@ -754,7 +716,7 @@ def test_destroy__school_teacher__last_teacher(self): def test_destroy__school_teacher__last_admin_teacher(self): """ - School-teacher-users cannot anonymize themself if they are the last + School-teacher-users cannot anonymize themselves if they are the last admin teachers. """ self._test_destroy( @@ -763,7 +725,7 @@ def test_destroy__school_teacher__last_admin_teacher(self): ) def test_destroy__independent(self): - """Independent-users can anonymize themself.""" + """Independent-users can anonymize themselves.""" user = self.indy_user self._test_destroy(user) user.refresh_from_db() From c4918c1b8ea18824ac3f424bd3c34d9ae5ce588e Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 22 Feb 2024 12:02:41 +0000 Subject: [PATCH 11/22] Update pipfile --- backend/Pipfile | 4 ++-- backend/Pipfile.lock | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 372cfc58..93e5dea2 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.13.13", 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.13.13", 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..97783084 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "c3b11b9d8699621ec2071fed98072adb143c11ae3dd29b642c565fb491add82d" + "sha256": "1f4e7c014b59f08000fa321dccbb719a20e4361118ea42814e53a9c158b08822" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9f3c8292be7400cef9255cdf6283f9bb7edea955" + "ref": "cff3afe88e686fed386bfaf42b6eb5b0eb9567ec" }, "codeforlife-portal": { "hashes": [ @@ -1514,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "9f3c8292be7400cef9255cdf6283f9bb7edea955" + "ref": "cff3afe88e686fed386bfaf42b6eb5b0eb9567ec" }, "codeforlife-portal": { "hashes": [ @@ -2495,7 +2495,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version < '3.10'", + "markers": "python_version >= '3.7'", "version": "==7.2.1" }, "pytest-cov": { From fe6e0cf22a7a18175c9e969cfd3f3b30024807d4 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Fri, 23 Feb 2024 15:34:40 +0000 Subject: [PATCH 12/22] Feedback pt 1 --- ....json => independent_school_1_class_2_join_request.json} | 0 backend/api/serializers/klass.py | 5 ----- backend/api/tests/views/test_user.py | 2 +- backend/api/views/user.py | 6 ++---- 4 files changed, 3 insertions(+), 10 deletions(-) rename backend/api/fixtures/{independent_school_1_class_1_join_request.json => independent_school_1_class_2_join_request.json} (100%) diff --git a/backend/api/fixtures/independent_school_1_class_1_join_request.json b/backend/api/fixtures/independent_school_1_class_2_join_request.json similarity index 100% rename from backend/api/fixtures/independent_school_1_class_1_join_request.json rename to backend/api/fixtures/independent_school_1_class_2_join_request.json diff --git a/backend/api/serializers/klass.py b/backend/api/serializers/klass.py index 24ebb075..25f644a7 100644 --- a/backend/api/serializers/klass.py +++ b/backend/api/serializers/klass.py @@ -102,11 +102,6 @@ def create(self, validated_data): ) def update(self, instance, validated_data): - # TODO: Decide how to handle this field. Currently, teachers select a - # setting which equates to a number of "hours", then we calculate - # how long the class can accept requests for based on that number of - # hours. It's not the most elegant solution, so for now I've written - # this assuming that the user will input a date-time value. accept_requests_until = validated_data.get("accept_requests_until") if accept_requests_until is not None: diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index ab3c4240..1074f040 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -39,7 +39,7 @@ class TestUserViewSet(ModelViewSetTestCase[User]): model_view_set_class = UserViewSet fixtures = [ "independent", - "independent_school_1_class_1_join_request", + "independent_school_1_class_2_join_request", "non_school_teacher", "school_1", "school_2", diff --git a/backend/api/views/user.py b/backend/api/views/user.py index dba04dd1..da9c77e4 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -41,7 +41,7 @@ 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", "handle_join_class_request"]: return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] if self.action == "partial_update": if "teacher" in self.request.data: @@ -50,8 +50,6 @@ def get_permissions(self): return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] if "requesting_to_join_class" in self.request.data: return [IsIndependent()] - if self.action == "handle_join_class_request": - return [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))] return super().get_permissions() @@ -229,7 +227,7 @@ def handle_join_class_request( """ 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 + join. The teacher handling the request must either be an admin in the class' school, or the teacher of that specific school. The request must then specify whether the teacher accepts or rejects the join request by setting the boolean "accept". If "accept" is True, From f82617f7cb6ede2688893c637f58c5174d12a745 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 28 Feb 2024 17:03:13 +0000 Subject: [PATCH 13/22] Create specific serializer and move tests --- backend/Pipfile | 4 +- backend/Pipfile.lock | 112 ++++---- ...pendent_school_1_class_2_join_request.json | 11 - backend/api/serializers/__init__.py | 6 +- backend/api/serializers/klass.py | 9 - backend/api/serializers/user.py | 63 +++++ backend/api/tests/serializers/test_klass.py | 8 + backend/api/tests/serializers/test_user.py | 126 ++++++--- backend/api/tests/views/test_user.py | 265 +++++------------- backend/api/views/user.py | 152 ++-------- 10 files changed, 331 insertions(+), 425 deletions(-) delete mode 100644 backend/api/fixtures/independent_school_1_class_2_join_request.json diff --git a/backend/Pipfile b/backend/Pipfile index 93e5dea2..bac675c7 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.13", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "fixtures_fix", 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.13", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "fixtures_fix", 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 97783084..ccd3b99c 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "1f4e7c014b59f08000fa321dccbb719a20e4361118ea42814e53a9c158b08822" + "sha256": "5cbe6e4e30f1388dd38a69239fcab78acb1c60bae7645d578cfd147e65fb49ea" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "cff3afe88e686fed386bfaf42b6eb5b0eb9567ec" + "ref": "913c82547a97ec89d0b44f4e5e314dde19294d6b" }, "codeforlife-portal": { "hashes": [ @@ -1514,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "cff3afe88e686fed386bfaf42b6eb5b0eb9567ec" + "ref": "913c82547a97ec89d0b44f4e5e314dde19294d6b" }, "codeforlife-portal": { "hashes": [ @@ -1528,61 +1528,61 @@ "toml" ], "hashes": [ - "sha256:006d220ba2e1a45f1de083d5022d4955abb0aedd78904cd5a779b955b019ec73", - "sha256:06fe398145a2e91edaf1ab4eee66149c6776c6b25b136f4a86fcbbb09512fd10", - "sha256:175f56572f25e1e1201d2b3e07b71ca4d201bf0b9cb8fad3f1dfae6a4188de86", - "sha256:18cac867950943fe93d6cd56a67eb7dcd2d4a781a40f4c1e25d6f1ed98721a55", - "sha256:1a5ee18e3a8d766075ce9314ed1cb695414bae67df6a4b0805f5137d93d6f1cb", - "sha256:20a875bfd8c282985c4720c32aa05056f77a68e6d8bbc5fe8632c5860ee0b49b", - "sha256:2412e98e70f16243be41d20836abd5f3f32edef07cbf8f407f1b6e1ceae783ac", - "sha256:2599972b21911111114100d362aea9e70a88b258400672626efa2b9e2179609c", - "sha256:2ed37e16cf35c8d6e0b430254574b8edd242a367a1b1531bd1adc99c6a5e00fe", - "sha256:32b4ab7e6c924f945cbae5392832e93e4ceb81483fd6dc4aa8fb1a97b9d3e0e1", - "sha256:34423abbaad70fea9d0164add189eabaea679068ebdf693baa5c02d03e7db244", - "sha256:3507427d83fa961cbd73f11140f4a5ce84208d31756f7238d6257b2d3d868405", - "sha256:3733545eb294e5ad274abe131d1e7e7de4ba17a144505c12feca48803fea5f64", - "sha256:3ff5bdb08d8938d336ce4088ca1a1e4b6c8cd3bef8bb3a4c0eb2f37406e49643", - "sha256:3ff7f92ae5a456101ca8f48387fd3c56eb96353588e686286f50633a611afc95", - "sha256:42a9e754aa250fe61f0f99986399cec086d7e7a01dd82fd863a20af34cbce962", - "sha256:51593a1f05c39332f623d64d910445fdec3d2ac2d96b37ce7f331882d5678ddf", - "sha256:5b11f9c6587668e495cc7365f85c93bed34c3a81f9f08b0920b87a89acc13469", - "sha256:69f1665165ba2fe7614e2f0c1aed71e14d83510bf67e2ee13df467d1c08bf1e8", - "sha256:78cdcbf7b9cb83fe047ee09298e25b1cd1636824067166dc97ad0543b079d22f", - "sha256:7df95fdd1432a5d2675ce630fef5f239939e2b3610fe2f2b5bf21fa505256fa3", - "sha256:81a5fb41b0d24447a47543b749adc34d45a2cf77b48ca74e5bf3de60a7bd9edc", - "sha256:840456cb1067dc350af9080298c7c2cfdddcedc1cb1e0b30dceecdaf7be1a2d3", - "sha256:8562ca91e8c40864942615b1d0b12289d3e745e6b2da901d133f52f2d510a1e3", - "sha256:861d75402269ffda0b33af94694b8e0703563116b04c681b1832903fac8fd647", - "sha256:8b98c89db1b150d851a7840142d60d01d07677a18f0f46836e691c38134ed18b", - "sha256:a178b7b1ac0f1530bb28d2e51f88c0bab3e5949835851a60dda80bff6052510c", - "sha256:a8ddbd158e069dded57738ea69b9744525181e99974c899b39f75b2b29a624e2", - "sha256:ac4bab32f396b03ebecfcf2971668da9275b3bb5f81b3b6ba96622f4ef3f6e17", - "sha256:ac9e95cefcf044c98d4e2c829cd0669918585755dd9a92e28a1a7012322d0a95", - "sha256:adbdfcda2469d188d79771d5696dc54fab98a16d2ef7e0875013b5f56a251047", - "sha256:b3c8bbb95a699c80a167478478efe5e09ad31680931ec280bf2087905e3b95ec", - "sha256:b3f2b1eb229f23c82898eedfc3296137cf1f16bb145ceab3edfd17cbde273fb7", - "sha256:b4ae777bebaed89e3a7e80c4a03fac434a98a8abb5251b2a957d38fe3fd30088", - "sha256:b953275d4edfab6cc0ed7139fa773dfb89e81fee1569a932f6020ce7c6da0e8f", - "sha256:bf54c3e089179d9d23900e3efc86d46e4431188d9a657f345410eecdd0151f50", - "sha256:bf711d517e21fb5bc429f5c4308fbc430a8585ff2a43e88540264ae87871e36a", - "sha256:c00e54f0bd258ab25e7f731ca1d5144b0bf7bec0051abccd2bdcff65fa3262c9", - "sha256:c11ca2df2206a4e3e4c4567f52594637392ed05d7c7fb73b4ea1c658ba560265", - "sha256:c5f9683be6a5b19cd776ee4e2f2ffb411424819c69afab6b2db3a0a364ec6642", - "sha256:cf89ab85027427d351f1de918aff4b43f4eb5f33aff6835ed30322a86ac29c9e", - "sha256:d1b750a8409bec61caa7824bfd64a8074b6d2d420433f64c161a8335796c7c6b", - "sha256:d779a48fac416387dd5673fc5b2d6bd903ed903faaa3247dc1865c65eaa5a93e", - "sha256:d9a1ef0f173e1a19738f154fb3644f90d0ada56fe6c9b422f992b04266c55d5a", - "sha256:ddb79414c15c6f03f56cc68fa06994f047cf20207c31b5dad3f6bab54a0f66ef", - "sha256:ef00d31b7569ed3cb2036f26565f1984b9fc08541731ce01012b02a4c238bf03", - "sha256:f40ac873045db4fd98a6f40387d242bde2708a3f8167bd967ccd43ad46394ba2", - "sha256:f593a4a90118d99014517c2679e04a4ef5aee2d81aa05c26c734d271065efcb6", - "sha256:f5df76c58977bc35a49515b2fbba84a1d952ff0ec784a4070334dfbec28a2def", - "sha256:f72cdd2586f9a769570d4b5714a3837b3a59a53b096bb954f1811f6a0afad305", - "sha256:f8e845d894e39fb53834da826078f6dc1a933b32b1478cf437007367efaf6f6a", - "sha256:fe6e43c8b510719b48af7db9631b5fbac910ade4bd90e6378c85ac5ac706382c" + "sha256:0209a6369ccce576b43bb227dc8322d8ef9e323d089c6f3f26a597b09cb4d2aa", + "sha256:062b0a75d9261e2f9c6d071753f7eef0fc9caf3a2c82d36d76667ba7b6470003", + "sha256:0842571634f39016a6c03e9d4aba502be652a6e4455fadb73cd3a3a49173e38f", + "sha256:16bae383a9cc5abab9bb05c10a3e5a52e0a788325dc9ba8499e821885928968c", + "sha256:18c7320695c949de11a351742ee001849912fd57e62a706d83dfc1581897fa2e", + "sha256:18d90523ce7553dd0b7e23cbb28865db23cddfd683a38fb224115f7826de78d0", + "sha256:1bf25fbca0c8d121a3e92a2a0555c7e5bc981aee5c3fdaf4bb7809f410f696b9", + "sha256:276f6077a5c61447a48d133ed13e759c09e62aff0dc84274a68dc18660104d52", + "sha256:280459f0a03cecbe8800786cdc23067a8fc64c0bd51dc614008d9c36e1659d7e", + "sha256:28ca2098939eabab044ad68850aac8f8db6bf0b29bc7f2887d05889b17346454", + "sha256:2c854ce44e1ee31bda4e318af1dbcfc929026d12c5ed030095ad98197eeeaed0", + "sha256:35eb581efdacf7b7422af677b92170da4ef34500467381e805944a3201df2079", + "sha256:37389611ba54fd6d278fde86eb2c013c8e50232e38f5c68235d09d0a3f8aa352", + "sha256:3b253094dbe1b431d3a4ac2f053b6d7ede2664ac559705a704f621742e034f1f", + "sha256:3b2eccb883368f9e972e216c7b4c7c06cabda925b5f06dde0650281cb7666a30", + "sha256:451f433ad901b3bb00184d83fd83d135fb682d780b38af7944c9faeecb1e0bfe", + "sha256:489763b2d037b164846ebac0cbd368b8a4ca56385c4090807ff9fad817de4113", + "sha256:4af154d617c875b52651dd8dd17a31270c495082f3d55f6128e7629658d63765", + "sha256:506edb1dd49e13a2d4cac6a5173317b82a23c9d6e8df63efb4f0380de0fbccbc", + "sha256:6679060424faa9c11808598504c3ab472de4531c571ab2befa32f4971835788e", + "sha256:69b9f6f66c0af29642e73a520b6fed25ff9fd69a25975ebe6acb297234eda501", + "sha256:6c00cdc8fa4e50e1cc1f941a7f2e3e0f26cb2a1233c9696f26963ff58445bac7", + "sha256:6c0cdedd3500e0511eac1517bf560149764b7d8e65cb800d8bf1c63ebf39edd2", + "sha256:708a3369dcf055c00ddeeaa2b20f0dd1ce664eeabde6623e516c5228b753654f", + "sha256:718187eeb9849fc6cc23e0d9b092bc2348821c5e1a901c9f8975df0bc785bfd4", + "sha256:767b35c3a246bcb55b8044fd3a43b8cd553dd1f9f2c1eeb87a302b1f8daa0524", + "sha256:77fbfc5720cceac9c200054b9fab50cb2a7d79660609200ab83f5db96162d20c", + "sha256:7cbde573904625509a3f37b6fecea974e363460b556a627c60dc2f47e2fffa51", + "sha256:8249b1c7334be8f8c3abcaaa996e1e4927b0e5a23b65f5bf6cfe3180d8ca7840", + "sha256:8580b827d4746d47294c0e0b92854c85a92c2227927433998f0d3320ae8a71b6", + "sha256:8640f1fde5e1b8e3439fe482cdc2b0bb6c329f4bb161927c28d2e8879c6029ee", + "sha256:9a9babb9466fe1da12417a4aed923e90124a534736de6201794a3aea9d98484e", + "sha256:a78ed23b08e8ab524551f52953a8a05d61c3a760781762aac49f8de6eede8c45", + "sha256:abbbd8093c5229c72d4c2926afaee0e6e3140de69d5dcd918b2921f2f0c8baba", + "sha256:ae7f19afe0cce50039e2c782bff379c7e347cba335429678450b8fe81c4ef96d", + "sha256:b3ec74cfef2d985e145baae90d9b1b32f85e1741b04cd967aaf9cfa84c1334f3", + "sha256:b51bfc348925e92a9bd9b2e48dad13431b57011fd1038f08316e6bf1df107d10", + "sha256:b9a4a8dd3dcf4cbd3165737358e4d7dfbd9d59902ad11e3b15eebb6393b0446e", + "sha256:ba3a8aaed13770e970b3df46980cb068d1c24af1a1968b7818b69af8c4347efb", + "sha256:c0524de3ff096e15fcbfe8f056fdb4ea0bf497d584454f344d59fce069d3e6e9", + "sha256:c0a120238dd71c68484f02562f6d446d736adcc6ca0993712289b102705a9a3a", + "sha256:cbbe5e739d45a52f3200a771c6d2c7acf89eb2524890a4a3aa1a7fa0695d2a47", + "sha256:ce8c50520f57ec57aa21a63ea4f325c7b657386b3f02ccaedeccf9ebe27686e1", + "sha256:cf30900aa1ba595312ae41978b95e256e419d8a823af79ce670835409fc02ad3", + "sha256:d25b937a5d9ffa857d41be042b4238dd61db888533b53bc76dc082cb5a15e914", + "sha256:d6cdecaedea1ea9e033d8adf6a0ab11107b49571bbb9737175444cea6eb72328", + "sha256:dec9de46a33cf2dd87a5254af095a409ea3bf952d85ad339751e7de6d962cde6", + "sha256:ebe7c9e67a2d15fa97b77ea6571ce5e1e1f6b0db71d1d5e96f8d2bf134303c1d", + "sha256:ee866acc0861caebb4f2ab79f0b94dbfbdbfadc19f82e6e9c93930f74e11d7a0", + "sha256:f6a09b360d67e589236a44f0c39218a8efba2593b6abdccc300a8862cffc2f94", + "sha256:fcc66e222cf4c719fe7722a403888b1f5e1682d1679bd780e2b26c18bb648cdc", + "sha256:fd6545d97c98a192c5ac995d21c894b581f1fd14cf389be90724d21808b657e2" ], "markers": "python_version >= '3.8'", - "version": "==7.4.2" + "version": "==7.4.3" }, "defusedxml": { "hashes": [ diff --git a/backend/api/fixtures/independent_school_1_class_2_join_request.json b/backend/api/fixtures/independent_school_1_class_2_join_request.json deleted file mode 100644 index 230d0baf..00000000 --- a/backend/api/fixtures/independent_school_1_class_2_join_request.json +++ /dev/null @@ -1,11 +0,0 @@ -[ - { - "model": "common.student", - "pk": 18, - "fields": { - "user": 28, - "new_user": 28, - "pending_class_request": 7 - } - } -] diff --git a/backend/api/serializers/__init__.py b/backend/api/serializers/__init__.py index 554d2434..3ccdad76 100644 --- a/backend/api/serializers/__init__.py +++ b/backend/api/serializers/__init__.py @@ -9,4 +9,8 @@ from .school_teacher_invitation import SchoolTeacherInvitationSerializer from .student import StudentSerializer from .teacher import TeacherSerializer -from .user import ReleaseStudentUserSerializer, UserSerializer +from .user import ( + HandleIndependentUserJoinClassRequestSerializer, + ReleaseStudentUserSerializer, + UserSerializer, +) diff --git a/backend/api/serializers/klass.py b/backend/api/serializers/klass.py index 25f644a7..da97b09c 100644 --- a/backend/api/serializers/klass.py +++ b/backend/api/serializers/klass.py @@ -100,12 +100,3 @@ def create(self, validated_data): ), } ) - - def update(self, instance, validated_data): - accept_requests_until = validated_data.get("accept_requests_until") - - if accept_requests_until is not None: - instance.accept_requests_until = accept_requests_until - - instance.save() - return instance diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index ccabf966..d82b3e4e 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -9,6 +9,7 @@ from codeforlife.serializers import ModelListSerializer from codeforlife.user.models import ( Class, + IndependentUser, Student, StudentUser, Teacher, @@ -336,3 +337,65 @@ def validate_email(self, value: str): ) return value + + +class HandleIndependentUserJoinClassRequestSerializer( + _UserSerializer[IndependentUser] +): + accept = serializers.BooleanField(write_only=True) + + class Meta(_UserSerializer.Meta): + fields = [ + *_UserSerializer.Meta.fields, + "accept", + ] + + extra_kwargs = { + "first_name": { + "min_length": 1, + "read_only": False, + "required": False, + }, + } + + def validate_first_name(self, value: str): + if Student.objects.filter( + class_field=self.instance.student.pending_class_request, + new_user__first_name__iexact=value, # TODO: Check case sensitivity + ).exists(): + raise serializers.ValidationError( + "This name already exists in the class. " + "Please choose a different name.", + code="first_name_in_class", + ) + + return value + + def update(self, instance, validated_data): + if validated_data["accept"]: + instance.student.class_field = ( + instance.student.pending_class_request + ) + instance.student.pending_class_request = None + + instance.username = StudentUser.get_random_username() + instance.first_name = validated_data.get( + "first_name", instance.first_name + ) + instance.last_name = "" + instance.email = "" + + instance.student.save( + update_fields=["class_field", "pending_class_request"] + ) + instance.save( + update_fields=["username", "first_name", "last_name", "email"] + ) + else: + instance.student.pending_class_request = None + instance.student.save(update_fields=["pending_class_request"]) + + # TODO: Send independent user an email notifying them that their + # request has been rejected. + + return instance diff --git a/backend/api/tests/serializers/test_klass.py b/backend/api/tests/serializers/test_klass.py index 58e4c1d0..6c78cde1 100644 --- a/backend/api/tests/serializers/test_klass.py +++ b/backend/api/tests/serializers/test_klass.py @@ -19,6 +19,7 @@ def setUp(self): email="teacher@school1.com" ) self.class_1 = Class.objects.get(name="Class 1 @ School 1") + self.class_2 = Class.objects.get(name="Class 2 @ School 1") def test_validate_teacher__does_not_exist(self): """Teacher must exist.""" @@ -83,6 +84,13 @@ def test_validate_name__name_not_unique(self): }, ) + def test_update__set_accept_requests_until(self): + """Can successfully set the class' accept requests until field.""" + self.assert_update( + self.class_2, + {"accept_requests_until": "9999-02-09 20:26:08.298402+00:00"}, + ) + def test_create__teacher(self): """Can successfully create with setting the teacher field.""" self.assert_create( diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index c666d721..9bf19b90 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -3,9 +3,19 @@ Created on 31/01/2024 at 16:07:32(+00:00). """ from codeforlife.tests import ModelSerializerTestCase -from codeforlife.user.models import Class, IndependentUser, Student, StudentUser, User - -from ...serializers import ReleaseStudentUserSerializer, UserSerializer +from codeforlife.user.models import ( + Class, + IndependentUser, + Student, + StudentUser, + User, +) + +from ...serializers import ( + HandleIndependentUserJoinClassRequestSerializer, + ReleaseStudentUserSerializer, + UserSerializer, +) # pylint: disable=missing-class-docstring @@ -15,8 +25,10 @@ class TestUserSerializer(ModelSerializerTestCase[User]): fixtures = ["independent", "school_1"] def setUp(self): - self.independent = IndependentUser.objects.get(email="indy@man.com") - self.class_1 = Class.objects.get(name="Class 1 @ School 1") + self.independent = IndependentUser.objects.get( + email="indy.requester@email.com" + ) + self.class_2 = Class.objects.get(name="Class 2 @ School 1") self.class_3 = Class.objects.get(name="Class 3 @ School 1") def test_validate__first_name_not_unique_per_class_in_data(self): @@ -67,33 +79,6 @@ def test_validate__first_name_not_unique_per_class_in_db(self): 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}}], - ) - def test_validate__teacher__first_name_required(self): """Teacher must have a first name""" self.assert_validate( @@ -112,7 +97,7 @@ def test_validate__teacher__requesting_to_join_class_forbidden(self): ) def test_validate__student__requesting_to_join_class_and_class_field_forbidden( - self, + self, ): """Student cannot have both requesting_to_join_class and class_field""" self.assert_validate( @@ -137,7 +122,7 @@ def test_validate_requesting_to_join_class__does_not_accept_requests(self): """ self.assert_validate_field( name="requesting_to_join_class", - value=self.class_1.access_code, + value=self.class_2.access_code, error_code="does_not_exist_or_accept_join_requests", ) @@ -151,3 +136,76 @@ def test_validate_requesting_to_join_class__no_longer_accept_requests(self): error_code="does_not_exist_or_accept_join_requests", ) + +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}}], + ) + + +class TestHandleIndependentUserJoinClassRequestSerializer( + ModelSerializerTestCase[IndependentUser] +): + model_serializer_class = HandleIndependentUserJoinClassRequestSerializer + fixtures = ["independent", "school_1", "school_2"] + + def setUp(self): + indy_user = IndependentUser.objects.get( + email="indy.requester@email.com" + ) + assert indy_user + self.indy_user = indy_user + self.class_1 = Class.objects.get(name="Class 1 @ School 1") + + def test_validate_first_name(self): + """Cannot accept a new student into a class with a duplicate name.""" + user_fields = ( + StudentUser.objects.filter(new_student__class_field=self.class_1) + .values("first_name") + .first() + ) + assert user_fields + + self.assert_validate_field( + "first_name", + user_fields["first_name"], + error_code="first_name_in_class", + instance=self.indy_user, + ) + + def test_update(self): + """The indy-user becomes a student-user.""" + self.assert_update( + self.indy_user, + {"accept": True}, + new_data={ + "last_name": "", + "email": "", + "student": { + "pending_class_request": None, + "class_field": self.class_1.pk, + }, + }, + non_model_fields=["accept"], + ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 212bf162..69938354 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -38,13 +38,7 @@ class TestUserViewSet(ModelViewSetTestCase[User]): basename = "user" model_view_set_class = UserViewSet - fixtures = [ - "independent", - "independent_school_1_class_2_join_request", - "non_school_teacher", - "school_1", - "school_2", - ] + fixtures = ["independent", "non_school_teacher", "school_1", "school_2"] def setUp(self): self.non_school_teacher_user = NonSchoolTeacherUser.objects.get( @@ -59,8 +53,13 @@ def setUp(self): self.admin_school2_teacher_user = AdminSchoolTeacherUser.objects.get( email="admin.teacher@school2.com" ) - self.indy_user = IndependentUser.objects.get(email="indy@man.com") - self.class_2 = Class.objects.get(name="Class 2 @ School 1") + self.indy_user = IndependentUser.objects.get( + email="indy.requester@email.com" + ) + self.no_join_request_indy_user = IndependentUser.objects.get( + email="indy@email.com" + ) + self.class_1 = Class.objects.get(name="Class 1 @ School 1") def _get_pk_and_token_for_user(self, email: str): user = User.objects.get(email__iexact=email) @@ -129,7 +128,7 @@ def test_get_permissions__handle_join_class_request(self): """ self.assert_get_permissions( [OR(IsTeacher(is_admin=True), IsTeacher(in_class=True))], - action="handle_join_class_request", + action="independents__handle_join_class_request", request=self.client.request_factory.patch(data={"accept": False}), ) @@ -168,6 +167,33 @@ def _test_get_queryset__student_users( self.assert_get_queryset(student_users, action=action, request=request) + def _test_get_queryset__independents__handle_join_class_request( + self, is_admin: bool + ): + request = self.client.request_factory.generic( + "patch", + user=self.admin_school_teacher_user + if is_admin + else self.non_admin_school_teacher_user, + ) + + indy_users = list( + IndependentUser.objects.filter( + new_student__pending_class_request__in=Class.objects.filter( + teacher__school=request.school_teacher_user.teacher.school + ) + if request.school_teacher_user.teacher.is_admin + else request.school_teacher_user.teacher.class_teacher.all() + ) + ) + assert indy_users + + self.assert_get_queryset( + indy_users, + action="independents__handle_join_class_request", + request=request, + ) + def test_get_queryset__bulk__patch(self): """Bulk partial-update can only target student-users.""" self._test_get_queryset__student_users( @@ -192,6 +218,22 @@ def test_get_queryset__students__release(self): action="students__release", request_method="patch" ) + def test_get_queryset__independents__handle_join_class_request__admin(self): + """Handling a join class request can only target the independent + 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 + ) + + def test_get_queryset__independents__handle_join_class_request__non_admin( + self, + ): + """Handling a join class request can only target the independent + students who made a request to one of the teacher's classes.""" + self._test_get_queryset__independents__handle_join_class_request( + is_admin=False + ) + def test_get_queryset__destroy(self): """Destroying a user can only target the user making the request.""" return self.assert_get_queryset( @@ -251,205 +293,46 @@ def test_bulk_destroy(self): # test: class join request actions - def test_handle_join_class_request__invalid_school(self): - """Teacher cannot handle a join request outside their school""" - self.client.login_as(self.admin_school2_teacher_user) - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - response = self.client.patch( - viewname, - data={"accept": False}, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", - ) - - assert response.data["non_field_errors"] == [ - "This class join request is not in your school." - ] - - self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__invalid_class(self): - """Non-admin teacher cannot reject a join request outside their class""" - self.client.login_as(self.non_admin_school_teacher_user) - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - response = self.client.patch( - viewname, - data={"accept": False}, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", - ) - - assert response.data["non_field_errors"] == [ - "This class join request is not for one for your classes." - ] - - self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__invalid_accept(self): - """Teacher cannot handle join class request with wrong accept type""" + def test_independents__handle_join_class_request__accept(self): + """Teacher can successfully accept a class join request.""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - response = self.client.patch( - viewname, - data={"accept": "lol"}, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", - ) - - assert response.data["non_field_errors"] == [ - "Invalid type for accept - must be True or False." - ] - - self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__missing_accept(self): - """Teacher cannot handle join class request with missing accept""" - self.client.login_as(self.admin_school_teacher_user) - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} + "independents--handle-join-class-request", + kwargs={ + "pk": self.indy_user.pk, + }, ) - response = self.client.patch( - viewname, - data={}, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", - ) + self.client.patch(viewname, data={"accept": True}) - assert response.data["non_field_errors"] == [ - "Accept field is required." - ] + username = self.indy_user.username self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__reject(self): - """Teacher can successfully reject a join class request.""" - self.client.login_as(self.admin_school_teacher_user) - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - self.client.patch( - viewname, - data={"accept": False}, - status_code_assertion=status.HTTP_200_OK, - format="json", - ) - - self.indy_user.refresh_from_db() assert self.indy_user.new_student.pending_class_request is None + assert self.indy_user.new_student.class_field == self.class_1 + assert self.indy_user.last_name == "" + assert self.indy_user.email == "" + assert self.indy_user.username != username - def test_handle_join_class_request__accept__invalid_first_name(self): - """Teacher cannot accept a join class request with invalid name""" - self.client.login_as(self.admin_school_teacher_user) - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - response = self.client.patch( - viewname, - data={"accept": True, "first_name": 1}, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", - ) - - assert response.data["first_name"] == ["First name must be a string."] - - self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__accept__missing_first_name(self): - """Teacher cannot accept a join class request with missing name""" - self.client.login_as(self.admin_school_teacher_user) - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - response = self.client.patch( - viewname, - data={"accept": True}, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", - ) - - assert response.data["first_name"] == ["This field is required."] - - self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__accept__duplicate_first_name(self): - """Teacher cannot accept a join class request with duplicate name""" + def test_independents__handle_join_class_request__reject(self): + """Teacher can successfully reject a class join request.""" self.client.login_as(self.admin_school_teacher_user) viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - response = self.client.patch( - viewname, - data={ - "accept": True, - "first_name": self.class_2.students.first().new_user.first_name, + "independents--handle-join-class-request", + kwargs={ + "pk": self.indy_user.pk, }, - status_code_assertion=status.HTTP_400_BAD_REQUEST, - format="json", ) - assert response.data["first_name"] == [ - "This name already exists in the class. " - "Please choose a different name." - ] + self.client.patch(viewname, data={"accept": False}) self.indy_user.refresh_from_db() - assert self.indy_user.new_student.pending_class_request == self.class_2 - - def test_handle_join_class_request__accept(self): - """Teacher can successfully accept a join class request.""" - self.client.login_as(self.admin_school_teacher_user) - indy_email = self.indy_user.email - - viewname = self.reverse_action( - "handle-join-class-request", kwargs={"pk": self.indy_user.pk} - ) - - self.client.patch( - viewname, - data={ - "accept": True, - "first_name": self.indy_user.first_name, - }, - status_code_assertion=status.HTTP_200_OK, - format="json", - ) - - self.indy_user.refresh_from_db() - - assert self.indy_user.new_student.class_field == self.class_2 assert self.indy_user.new_student.pending_class_request is None - assert self.indy_user.last_name == "" - assert self.indy_user.email == "" - assert self.indy_user.username != indy_email + assert self.indy_user.new_student.class_field is None # test: reset password actions @@ -663,11 +546,11 @@ def test_partial_update__teacher(self): def test_partial_update__indy__send_join_request(self): """Independent user can request to join a class.""" - self.client.login_as(self.indy_user) + self.client.login_as(self.no_join_request_indy_user) self.client.partial_update( - self.indy_user, - {"requesting_to_join_class": self.class_2.access_code}, + self.no_join_request_indy_user, + {"requesting_to_join_class": self.class_1.access_code}, ) def test_partial_update__indy__revoke_join_request(self): diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 8a066998..2970c568 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -10,8 +10,8 @@ from codeforlife.types import DataDict from codeforlife.user.models import ( Class, + IndependentUser, SchoolTeacher, - Student, StudentUser, User, ) @@ -21,13 +21,16 @@ PasswordResetTokenGenerator, default_token_generator, ) -from django.utils.crypto import get_random_string from rest_framework import status from rest_framework.decorators import action from rest_framework.permissions import AllowAny from rest_framework.response import Response -from ..serializers import ReleaseStudentUserSerializer, UserSerializer +from ..serializers import ( + HandleIndependentUserJoinClassRequestSerializer, + ReleaseStudentUserSerializer, + UserSerializer, +) # NOTE: type hint to help Intellisense. default_token_generator: PasswordResetTokenGenerator = default_token_generator @@ -43,7 +46,7 @@ def get_permissions(self): return [OR(IsTeacher(), IsIndependent())] if self.action in [ "bulk", - "handle_join_class_request", + "independents__handle_join_class_request", "students__reset_password", "students__release", ]: @@ -61,10 +64,21 @@ def get_permissions(self): def get_serializer_class(self): if self.action == "students__release": return ReleaseStudentUserSerializer + if self.action == "independents__handle_join_class_request": + return HandleIndependentUserJoinClassRequestSerializer return super().get_serializer_class() def get_queryset(self, user_class=User): + if self.action == "independents__handle_join_class_request": + return IndependentUser.objects.filter( + new_student__pending_class_request__in=Class.objects.filter( + teacher__school=self.request.school_teacher_user.teacher.school + ) + if self.request.school_teacher_user.teacher.is_admin + else self.request.school_teacher_user.teacher.class_teacher.all() + ) + queryset = super().get_queryset(user_class) if self.action == "destroy": queryset = queryset.filter(pk=self.request.auth_user.pk) @@ -126,12 +140,7 @@ def anonymize_user(user: User): url_path="reset-password/(?P.+)", permission_classes=[AllowAny], ) - def reset_password( - self, - request: Request, - pk: t.Optional[str] = None, - token: t.Optional[str] = None, - ): + def reset_password(self, request: Request, pk: str, token: str): """ Handles password reset for a user. On GET, checks validity of user PK and token. On PATCH, rechecks these params and performs new password @@ -246,10 +255,12 @@ def students__release(self, request: Request): return Response(serializer.data) @action( - detail=True, methods=["patch"], url_path="handle-join-class-request" + detail=True, + methods=["patch"], + url_path="independents/handle-join-class-request", ) - def handle_join_class_request( - self, request: Request, pk: t.Optional[str] = None + def independents__handle_join_class_request( + self, request: Request, pk: str ): """ Handles an independent user's request to join a class. First tries to @@ -261,114 +272,13 @@ def handle_join_class_request( then the request must contain the new student's first_name, ensuring that it is unique in the class. """ - try: - indy = User.objects.get(pk=int(pk)) - except (ValueError, Student.DoesNotExist): - return Response( - {"non_field_errors": ["No user found for given ID."]}, - status=status.HTTP_400_BAD_REQUEST, - ) - - klass = Class.objects.get( - access_code=indy.student.pending_class_request.access_code + serializer = self.get_serializer( + self.get_object(), + data=request.data, + context=self.get_serializer_context(), ) - if klass.teacher.school != request.user.new_teacher.school: - return Response( - { - "non_field_errors": [ - "This class join request is not in your school." - ] - }, - status=status.HTTP_400_BAD_REQUEST, - ) - - if ( - not request.user.new_teacher.is_admin - and klass.teacher != request.user.new_teacher - ): - return Response( - { - "non_field_errors": [ - "This class join request is not for " - "one for your classes." - ] - }, - status=status.HTTP_400_BAD_REQUEST, - ) - - try: - if not isinstance(request.data["accept"], bool): - return Response( - { - "non_field_errors": [ - "Invalid type for accept - must be True or False." - ] - }, - status=status.HTTP_400_BAD_REQUEST, - ) - except KeyError: - return Response( - {"non_field_errors": ["Accept field is required."]}, - status=status.HTTP_400_BAD_REQUEST, - ) - - request_accepted = request.data["accept"] - - if request_accepted: - try: - if not isinstance(request.data["first_name"], str): - return Response( - {"first_name": ["First name must be a string."]}, - status=status.HTTP_400_BAD_REQUEST, - ) - except KeyError: - return Response( - {"first_name": ["This field is required."]}, - status=status.HTTP_400_BAD_REQUEST, - ) - - name = request.data["first_name"] - if self._is_name_unique_in_class(name, klass): - indy.student.class_field = indy.student.pending_class_request - indy.student.pending_class_request = None - - username = None - - while ( - username is None - or User.objects.filter(username=username).exists() - ): - username = get_random_string(length=30) - - indy.username = username - indy.first_name = name - indy.last_name = "" - indy.email = "" - - indy.student.save() - indy.save() - else: - return Response( - { - "first_name": [ - "This name already exists in the class. " - "Please choose a different name." - ] - }, - status=status.HTTP_400_BAD_REQUEST, - ) - else: - indy.student.pending_class_request = None - indy.student.save() - - # TODO: Send independent user an email notifying them that their - # request has been rejected. - - return Response() + serializer.is_valid(raise_exception=True) + serializer.save() - def _is_name_unique_in_class(self, name: str, klass: Class) -> bool: - """Check if a user's name is unique in a class""" - return not Student.objects.filter( - class_field=klass, new_user__first_name__iexact=name - ).exists() + return Response(serializer.data) From 03405e46763dfebdd040c27927027ffcca6bdea7 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 28 Feb 2024 17:04:30 +0000 Subject: [PATCH 14/22] Remove unnecessary test --- backend/api/tests/serializers/test_klass.py | 8 -------- 1 file changed, 8 deletions(-) diff --git a/backend/api/tests/serializers/test_klass.py b/backend/api/tests/serializers/test_klass.py index 6c78cde1..58e4c1d0 100644 --- a/backend/api/tests/serializers/test_klass.py +++ b/backend/api/tests/serializers/test_klass.py @@ -19,7 +19,6 @@ def setUp(self): email="teacher@school1.com" ) self.class_1 = Class.objects.get(name="Class 1 @ School 1") - self.class_2 = Class.objects.get(name="Class 2 @ School 1") def test_validate_teacher__does_not_exist(self): """Teacher must exist.""" @@ -84,13 +83,6 @@ def test_validate_name__name_not_unique(self): }, ) - def test_update__set_accept_requests_until(self): - """Can successfully set the class' accept requests until field.""" - self.assert_update( - self.class_2, - {"accept_requests_until": "9999-02-09 20:26:08.298402+00:00"}, - ) - def test_create__teacher(self): """Can successfully create with setting the teacher field.""" self.assert_create( From 51d90bc1070b60368bc85d56dc0179d8a3520181 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Wed, 28 Feb 2024 17:08:22 +0000 Subject: [PATCH 15/22] Remove TOOD, names are case insensitive --- backend/api/serializers/user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index d82b3e4e..6d6c4426 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -361,7 +361,7 @@ class Meta(_UserSerializer.Meta): def validate_first_name(self, value: str): if Student.objects.filter( class_field=self.instance.student.pending_class_request, - new_user__first_name__iexact=value, # TODO: Check case sensitivity + new_user__first_name__iexact=value, ).exists(): raise serializers.ValidationError( "This name already exists in the class. " From bf878fdbb3c3e617c702561b95171910c7d17b87 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 29 Feb 2024 14:47:55 +0000 Subject: [PATCH 16/22] Feedback --- backend/Pipfile | 4 +- backend/api/serializers/user.py | 117 ++++++++++++--------- backend/api/tests/serializers/test_user.py | 2 +- backend/api/tests/views/test_user.py | 2 +- backend/api/views/user.py | 11 +- 5 files changed, 70 insertions(+), 66 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index bac675c7..86a2759b 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 = "fixtures_fix", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "0.14.4", 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 = "fixtures_fix", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "0.14.4", 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/api/serializers/user.py b/backend/api/serializers/user.py index 6d6c4426..1db8071b 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -26,6 +26,7 @@ from .student import StudentSerializer from .teacher import TeacherSerializer + # pylint: disable=missing-class-docstring,too-many-ancestors @@ -93,7 +94,7 @@ class UserSerializer(_UserSerializer[User]): student = StudentSerializer(source="new_student", required=False) teacher = TeacherSerializer(source="new_teacher", required=False) requesting_to_join_class = serializers.CharField( - required=False, allow_blank=True + required=False, allow_null=True ) current_password = serializers.CharField( write_only=True, @@ -123,6 +124,21 @@ class Meta(_UserSerializer.Meta): list_serializer_class = UserListSerializer def validate(self, attrs): + last_name_required_error = serializers.ValidationError( + "Last name is required.", code="last_name_required" + ) + + teacher_cannot_request_join_class_error = serializers.ValidationError( + "Teacher can't request to join a class.", + code="teacher_cannot_request_to_join_class", + ) + + student_cannot_request_join_class_error = serializers.ValidationError( + "Student can't be in a class and requesting to join a " + "class at the same time.", + code="class_and_join_class_mutually_exclusive", + ) + if self.instance: # Updating if self.view.action != "reset_password": # TODO: make current password required when changing @@ -131,15 +147,10 @@ def validate(self, attrs): if self.instance.teacher: if "last_name" in attrs and not attrs["last_name"]: - raise serializers.ValidationError( - "Last name is required.", code="last_name_required" - ) + raise last_name_required_error if "requesting_to_join_class" in attrs: - raise serializers.ValidationError( - "Teacher can't request to join a class.", - code="teacher_cannot_request_to_join_class", - ) + raise teacher_cannot_request_join_class_error if self.instance.student: if self.view.action != "reset_password": @@ -156,35 +167,28 @@ def validate(self, attrs): and "new_student" in attrs and "class_field" in attrs["new_student"] ): - raise serializers.ValidationError( - "Student can't be in a class and requesting to join a " - "class at the same time.", - code="class_and_join_class_mutually_exclusive", - ) + raise student_cannot_request_join_class_error else: # Creating + if "new_teacher" in attrs and "new_student" in attrs: + raise serializers.ValidationError( + "Cannot create a user with both teacher and student " + "attributes.", + code="teacher_and_student", + ) if "new_teacher" in attrs: - if "last_name" not in attrs or attrs["last_name"] is None: - raise serializers.ValidationError( - "Last name is required.", code="last_name_required" - ) + if not attrs.get("last_name"): + raise last_name_required_error if "requesting_to_join_class" in attrs: - raise serializers.ValidationError( - "Teacher can't request to join a class.", - code="teacher_cannot_request_to_join_class", - ) + raise teacher_cannot_request_join_class_error - if "new_student" in attrs: + elif "new_student" in attrs: if ( "class_field" in attrs["new_student"] and "requesting_to_join_class" in attrs ): - raise serializers.ValidationError( - "Student can't be in a class and requesting to join a " - "class at the same time.", - code="class_and_join_class_mutually_exclusive", - ) + raise student_cannot_request_join_class_error return attrs @@ -213,7 +217,7 @@ def validate_requesting_to_join_class(self, value: str): error_message = "Class does not exist or does not accept join requests." error_code = "does_not_exist_or_accept_join_requests" - if value != "": + if value is not None: try: klass = Class.objects.get(access_code=value) except Class.DoesNotExist as ex: @@ -260,24 +264,24 @@ def create(self, validated_data): return user def update(self, instance, validated_data): - requesting_to_join_class = validated_data.get( - "requesting_to_join_class" - ) - if requesting_to_join_class is not None: - # If value is empty, this means independent wants to revoke their - # join request - if requesting_to_join_class == "": + if "requesting_to_join_class" in validated_data: + requesting_to_join_class = validated_data[ + "requesting_to_join_class" + ] + + if requesting_to_join_class is None: instance.student.pending_class_request = None + instance.student.save(update_fields=["pending_class_request"]) else: instance.student.pending_class_request = Class.objects.get( access_code=requesting_to_join_class ) + instance.student.save(update_fields=["pending_class_request"]) - # TODO: Send email to indy user confirming successful join request. - # TODO: Send email to teacher of selected class to notify them of - # join request. - - instance.student.save() + # TODO: Send email to indy user confirming successful join + # request. + # TODO: Send email to teacher of selected class to notify + # them of join request. password = validated_data.get("password") @@ -342,15 +346,19 @@ def validate_email(self, value: str): class HandleIndependentUserJoinClassRequestSerializer( _UserSerializer[IndependentUser] ): + """ + Handles an independent user's request to join a class. If "accept" is + True, convert the independent user to a student user and add them to the + class in question. First name validation is also done to avoid duplicate + first names within the class (case-insensitive). + """ + accept = serializers.BooleanField(write_only=True) class Meta(_UserSerializer.Meta): - fields = [ - *_UserSerializer.Meta.fields, - "accept", - ] - + fields = [*_UserSerializer.Meta.fields, "accept"] extra_kwargs = { + **_UserSerializer.Meta.extra_kwargs, "first_name": { "min_length": 1, "read_only": False, @@ -359,14 +367,14 @@ class Meta(_UserSerializer.Meta): } def validate_first_name(self, value: str): - if Student.objects.filter( - class_field=self.instance.student.pending_class_request, - new_user__first_name__iexact=value, + if StudentUser.objects.filter( + new_student__class_field=self.instance.student.pending_class_request, + first_name__iexact=value, ).exists(): raise serializers.ValidationError( "This name already exists in the class. " "Please choose a different name.", - code="first_name_in_class", + code="already_in_class", ) return value @@ -378,6 +386,10 @@ def update(self, instance, validated_data): ) instance.student.pending_class_request = None + instance.student.save( + update_fields=["class_field", "pending_class_request"] + ) + instance.username = StudentUser.get_random_username() instance.first_name = validated_data.get( "first_name", instance.first_name @@ -385,12 +397,13 @@ def update(self, instance, validated_data): instance.last_name = "" instance.email = "" - instance.student.save( - update_fields=["class_field", "pending_class_request"] - ) instance.save( update_fields=["username", "first_name", "last_name", "email"] ) + + # TODO: Send new student user an email notifying them that their + # request has been accepted. + else: instance.student.pending_class_request = None instance.student.save(update_fields=["pending_class_request"]) diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index 9bf19b90..914f7f70 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -190,7 +190,7 @@ def test_validate_first_name(self): self.assert_validate_field( "first_name", user_fields["first_name"], - error_code="first_name_in_class", + error_code="already_in_class", instance=self.indy_user, ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index 69938354..ea73ac36 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -558,7 +558,7 @@ def test_partial_update__indy__revoke_join_request(self): self.client.login_as(self.indy_user) self.client.partial_update( - self.indy_user, {"requesting_to_join_class": ""} + self.indy_user, {"requesting_to_join_class": None} ) def assert_user_is_anonymized(self, user: User): diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 2970c568..3dfab292 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -262,16 +262,7 @@ def students__release(self, request: Request): def independents__handle_join_class_request( self, request: Request, pk: str ): - """ - 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 in the - class' school, or the teacher of that specific school. The request - must then specify whether the teacher accepts or rejects the join - request by setting the boolean "accept". If "accept" is True, - then the request must contain the new student's first_name, ensuring - that it is unique in the class. - """ + """Handle an independent user's request to join a class.""" serializer = self.get_serializer( self.get_object(), data=request.data, From 1f37409fa81a7b8850e0e8e271ae28be55c18757 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 29 Feb 2024 15:34:25 +0000 Subject: [PATCH 17/22] Queryset --- backend/api/serializers/user.py | 6 ------ backend/api/tests/views/test_user.py | 9 +++++++++ backend/api/views/user.py | 4 +++- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index 1db8071b..e4e41068 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -153,12 +153,6 @@ def validate(self, attrs): raise teacher_cannot_request_join_class_error if self.instance.student: - if self.view.action != "reset_password": - if self.request.student_user.pk != self.instance.pk: - raise serializers.ValidationError( - "Cannot update another student.", code="is_not_self" - ) - if ( self.instance.student.class_field is not None and "requesting_to_join_class" in attrs diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index ea73ac36..edbfd3b3 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -244,6 +244,15 @@ def test_get_queryset__destroy(self): ), ) + def test_get_queryset__partial_update__student(self): + """Updating a student can only target the user making the request if + the user is a student.""" + return self.assert_get_queryset( + [self.indy_user], + action="partial__update", + request=self.client.request_factory.patch(user=self.indy_user), + ) + # test: bulk actions def test_bulk_create__students(self): diff --git a/backend/api/views/user.py b/backend/api/views/user.py index 3dfab292..d54b1ab0 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -80,7 +80,9 @@ def get_queryset(self, user_class=User): ) queryset = super().get_queryset(user_class) - if self.action == "destroy": + if self.action == "destroy" or ( + self.action == "partial_update" and self.request.auth_user.student + ): queryset = queryset.filter(pk=self.request.auth_user.pk) elif self.action in [ "students__reset_password", From 864f21039815479b808aef783bce7aaca808e170 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 29 Feb 2024 15:43:44 +0000 Subject: [PATCH 18/22] Lockfile --- backend/Pipfile | 4 ++-- backend/Pipfile.lock | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index 86a2759b..be78a118 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 = "0.14.4", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.14.5", 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 = "0.14.4", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.14.5", 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 ccd3b99c..6c30471f 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "5cbe6e4e30f1388dd38a69239fcab78acb1c60bae7645d578cfd147e65fb49ea" + "sha256": "c9775dbeb9de6e7aaba5136896d5633e921dd947f37711a83930d7949605e2d8" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "913c82547a97ec89d0b44f4e5e314dde19294d6b" + "ref": "3944581dd6cf015b10c3ef410db386fcd8df39ed" }, "codeforlife-portal": { "hashes": [ @@ -1514,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "913c82547a97ec89d0b44f4e5e314dde19294d6b" + "ref": "3944581dd6cf015b10c3ef410db386fcd8df39ed" }, "codeforlife-portal": { "hashes": [ @@ -2495,7 +2495,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version >= '3.7'", + "markers": "python_version < '3.10'", "version": "==7.2.1" }, "pytest-cov": { From 2163f47d458bd5ede4698e4bb032bcfda54fec80 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 29 Feb 2024 15:47:22 +0000 Subject: [PATCH 19/22] Spacing --- backend/api/serializers/user.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index e4e41068..b4db8970 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -2,7 +2,6 @@ © Ocado Group Created on 18/01/2024 at 15:14:32(+00:00). """ - import typing as t from itertools import groupby @@ -26,7 +25,6 @@ from .student import StudentSerializer from .teacher import TeacherSerializer - # pylint: disable=missing-class-docstring,too-many-ancestors From 7a4877d5fbe26c959ff2456bf336e14427882564 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Tue, 5 Mar 2024 17:22:41 +0000 Subject: [PATCH 20/22] Feedback part 1 bajillion --- backend/api/serializers/user.py | 37 ++++++++-------- backend/api/tests/serializers/test_user.py | 51 ++++++++++++++++------ 2 files changed, 55 insertions(+), 33 deletions(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index b4db8970..da5b62a3 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -122,13 +122,9 @@ class Meta(_UserSerializer.Meta): list_serializer_class = UserListSerializer def validate(self, attrs): - last_name_required_error = serializers.ValidationError( - "Last name is required.", code="last_name_required" - ) - - teacher_cannot_request_join_class_error = serializers.ValidationError( + requesting_to_join_class__teacher_error = serializers.ValidationError( "Teacher can't request to join a class.", - code="teacher_cannot_request_to_join_class", + code="requesting_to_join_class__teacher", ) student_cannot_request_join_class_error = serializers.ValidationError( @@ -144,13 +140,10 @@ def validate(self, attrs): pass if self.instance.teacher: - if "last_name" in attrs and not attrs["last_name"]: - raise last_name_required_error - if "requesting_to_join_class" in attrs: - raise teacher_cannot_request_join_class_error + raise requesting_to_join_class__teacher_error - if self.instance.student: + elif self.instance.student: if ( self.instance.student.class_field is not None and "requesting_to_join_class" in attrs @@ -170,10 +163,12 @@ def validate(self, attrs): ) if "new_teacher" in attrs: if not attrs.get("last_name"): - raise last_name_required_error + raise serializers.ValidationError( + "Last name is required.", code="last_name__required" + ) if "requesting_to_join_class" in attrs: - raise teacher_cannot_request_join_class_error + raise requesting_to_join_class__teacher_error elif "new_student" in attrs: if ( @@ -207,22 +202,23 @@ def validate_requesting_to_join_class(self, value: str): # NOTE: Error message is purposefully ambiguous to prevent class # enumeration. error_message = "Class does not exist or does not accept join requests." - error_code = "does_not_exist_or_accept_join_requests" if value is not None: try: klass = Class.objects.get(access_code=value) except Class.DoesNotExist as ex: raise serializers.ValidationError( - error_message, code=error_code + error_message, code="does_not_exist" ) from ex - if ( - klass.accept_requests_until is None - or klass.accept_requests_until < timezone.now() - ): + if klass.accept_requests_until is None: raise serializers.ValidationError( - error_message, code=error_code + error_message, code="does_not_accept_requests" + ) + + if klass.accept_requests_until < timezone.now(): + raise serializers.ValidationError( + error_message, code="no_longer_accepts_requests" ) return value @@ -316,6 +312,7 @@ class ReleaseStudentUserSerializer(_UserSerializer[StudentUser]): class Meta(_UserSerializer.Meta): extra_kwargs = { + **_UserSerializer.Meta.extra_kwargs, "first_name": { "min_length": 1, "read_only": False, diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index 914f7f70..3d7b10fc 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -4,6 +4,7 @@ """ from codeforlife.tests import ModelSerializerTestCase from codeforlife.user.models import ( + AdminSchoolTeacherUser, Class, IndependentUser, Student, @@ -28,6 +29,9 @@ def setUp(self): self.independent = IndependentUser.objects.get( email="indy.requester@email.com" ) + self.admin_school_teacher_user = AdminSchoolTeacherUser.objects.get( + email="admin.teacher@school1.com", + ) self.class_2 = Class.objects.get(name="Class 2 @ School 1") self.class_3 = Class.objects.get(name="Class 3 @ School 1") @@ -79,21 +83,40 @@ def test_validate__first_name_not_unique_per_class_in_db(self): many=True, ) - def test_validate__teacher__first_name_required(self): - """Teacher must have a first name""" + def test_validate__create__teacher__last_name_required(self): + """Teacher's last name is required during creation.""" + self.assert_validate( + attrs={"new_teacher": {}}, error_code="last_name__required" + ) + + def test_validate__create__teacher__requesting_to_join_class(self): + """Teacher cannot request to join a class on creation.""" self.assert_validate( - attrs={"new_teacher": {}}, error_code="last_name_required" + attrs={ + "last_name": "Name", + "new_teacher": {}, + "requesting_to_join_class": "AAAAA", + }, + error_code="requesting_to_join_class__teacher", ) - def test_validate__teacher__requesting_to_join_class_forbidden(self): - """Teacher cannot request to join a class""" + def test_validate__update__teacher__requesting_to_join_class(self): + """Teacher cannot request to join a class on update.""" self.assert_validate( attrs={ "last_name": "Name", "new_teacher": {}, "requesting_to_join_class": "AAAAA", }, - error_code="teacher_cannot_request_to_join_class", + error_code="requesting_to_join_class__teacher", + parent=UserSerializer( + instance=self.admin_school_teacher_user, + context={ + "request": self.request_factory.patch( + user=self.admin_school_teacher_user + ), + }, + ), ) def test_validate__student__requesting_to_join_class_and_class_field_forbidden( @@ -109,31 +132,33 @@ def test_validate__student__requesting_to_join_class_and_class_field_forbidden( ) def test_validate_requesting_to_join_class__does_not_exist(self): - """Join class request cannot be for a class that doesn't exist""" + """Student cannot request to join a class which doesn't exist.""" self.assert_validate_field( name="requesting_to_join_class", value="AAAAA", - error_code="does_not_exist_or_accept_join_requests", + error_code="does_not_exist", ) 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 + Student cannot request to join a class which doesn't accept requests. """ self.assert_validate_field( name="requesting_to_join_class", value=self.class_2.access_code, - error_code="does_not_exist_or_accept_join_requests", + error_code="does_not_accept_requests", ) - def test_validate_requesting_to_join_class__no_longer_accept_requests(self): + def test_validate_requesting_to_join_class__no_longer_accepts_requests( + self, + ): """ - Join class request cannot be for a class that no longer accepts requests + Student cannot request to join a class which no longer accepts requests. """ self.assert_validate_field( name="requesting_to_join_class", value=self.class_3.access_code, - error_code="does_not_exist_or_accept_join_requests", + error_code="no_longer_accepts_requests", ) From 07cb64563385baa5c4a11f63a821c0911f4f4ca9 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 7 Mar 2024 17:44:39 +0000 Subject: [PATCH 21/22] Feedback lol --- backend/Pipfile | 4 +- backend/Pipfile.lock | 8 +- backend/api/serializers/user.py | 43 +++++++---- backend/api/tests/serializers/test_user.py | 85 +++++++++++++++++----- backend/api/tests/views/test_user.py | 31 +++----- backend/api/views/user.py | 8 +- 6 files changed, 112 insertions(+), 67 deletions(-) diff --git a/backend/Pipfile b/backend/Pipfile index e7fd47c1..014f2e4c 100644 --- a/backend/Pipfile +++ b/backend/Pipfile @@ -23,7 +23,7 @@ name = "pypi" # 5. Run `pipenv install --dev` in your terminal. [packages] -codeforlife = {ref = "v0.14.5", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} +codeforlife = {ref = "v0.14.6", git = "https://github.com/ocadotechnology/codeforlife-package-python.git"} # 🚫 Don't add [packages] below that are inhertited from the CFL package. # TODO: check if we need the below packages whitenoise = "==6.5.0" @@ -48,7 +48,7 @@ google-cloud-container = "==2.3.0" # "django-anymail[amazon_ses]" = "==7.0.*" [dev-packages] -codeforlife = {ref = "v0.14.5", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} +codeforlife = {ref = "v0.14.6", git = "https://github.com/ocadotechnology/codeforlife-package-python.git", extras = ["dev"]} # codeforlife = {file = "../../codeforlife-package-python", editable = true, extras = ["dev"]} # 🚫 Don't add [dev-packages] below that are inhertited from the CFL package. # TODO: check if we need the below packages diff --git a/backend/Pipfile.lock b/backend/Pipfile.lock index 6c30471f..ca62e429 100644 --- a/backend/Pipfile.lock +++ b/backend/Pipfile.lock @@ -1,7 +1,7 @@ { "_meta": { "hash": { - "sha256": "c9775dbeb9de6e7aaba5136896d5633e921dd947f37711a83930d7949605e2d8" + "sha256": "7601f83b4e5fce9dcdcac76d889ae9f0a5c38f089f6a1b577df72bee8b3aa4a4" }, "pipfile-spec": 6, "requires": { @@ -168,7 +168,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "3944581dd6cf015b10c3ef410db386fcd8df39ed" + "ref": "da9effd31e6bbb0f2ff280ca0669e9ab2ceb165d" }, "codeforlife-portal": { "hashes": [ @@ -1514,7 +1514,7 @@ }, "codeforlife": { "git": "https://github.com/ocadotechnology/codeforlife-package-python.git", - "ref": "3944581dd6cf015b10c3ef410db386fcd8df39ed" + "ref": "da9effd31e6bbb0f2ff280ca0669e9ab2ceb165d" }, "codeforlife-portal": { "hashes": [ @@ -2495,7 +2495,7 @@ "sha256:c7c6ca206e93355074ae32f7403e8ea12163b1163c976fee7d4d84027c162be5", "sha256:d45e0952f3727241918b8fd0f376f5ff6b301cc0777c6f9a556935c92d8a7d42" ], - "markers": "python_version < '3.10'", + "markers": "python_version >= '3.7'", "version": "==7.2.1" }, "pytest-cov": { diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index da5b62a3..e7ffbef9 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -25,6 +25,7 @@ from .student import StudentSerializer from .teacher import TeacherSerializer + # pylint: disable=missing-class-docstring,too-many-ancestors @@ -122,17 +123,6 @@ class Meta(_UserSerializer.Meta): list_serializer_class = UserListSerializer def validate(self, attrs): - requesting_to_join_class__teacher_error = serializers.ValidationError( - "Teacher can't request to join a class.", - code="requesting_to_join_class__teacher", - ) - - student_cannot_request_join_class_error = serializers.ValidationError( - "Student can't be in a class and requesting to join a " - "class at the same time.", - code="class_and_join_class_mutually_exclusive", - ) - if self.instance: # Updating if self.view.action != "reset_password": # TODO: make current password required when changing @@ -141,18 +131,31 @@ def validate(self, attrs): if self.instance.teacher: if "requesting_to_join_class" in attrs: - raise requesting_to_join_class__teacher_error + raise serializers.ValidationError( + "Teacher can't request to join a class.", + code="requesting_to_join_class__teacher__update", + ) elif self.instance.student: if ( self.instance.student.class_field is not None and "requesting_to_join_class" in attrs - ) or ( + ): + raise serializers.ValidationError( + "Student cannot request to join a class.", + code="requesting_to_join_class__student__update__in_class", + ) + + if ( self.instance.student.pending_class_request is not None and "new_student" in attrs and "class_field" in attrs["new_student"] ): - raise student_cannot_request_join_class_error + raise serializers.ValidationError( + "Independent user cannot be added to class as they " + "are already requesting to join a class.", + code="class_field__indy__update__requesting_to_join_class", + ) else: # Creating if "new_teacher" in attrs and "new_student" in attrs: @@ -161,6 +164,7 @@ def validate(self, attrs): "attributes.", code="teacher_and_student", ) + if "new_teacher" in attrs: if not attrs.get("last_name"): raise serializers.ValidationError( @@ -168,14 +172,21 @@ def validate(self, attrs): ) if "requesting_to_join_class" in attrs: - raise requesting_to_join_class__teacher_error + raise serializers.ValidationError( + "Teacher can't request to join a class.", + code="requesting_to_join_class__teacher__create", + ) elif "new_student" in attrs: if ( "class_field" in attrs["new_student"] and "requesting_to_join_class" in attrs ): - raise student_cannot_request_join_class_error + raise serializers.ValidationError( + "Cannot create a student in class who is also " + "requesting to join a class.", + code="requesting_to_join_class__student__create__in_class", + ) return attrs diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index 3d7b10fc..dbe92e37 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -17,6 +17,8 @@ ReleaseStudentUserSerializer, UserSerializer, ) +from ...views import UserViewSet + # pylint: disable=missing-class-docstring @@ -32,6 +34,7 @@ def setUp(self): self.admin_school_teacher_user = AdminSchoolTeacherUser.objects.get( email="admin.teacher@school1.com", ) + self.student_user = StudentUser.objects.get(first_name="Student1") self.class_2 = Class.objects.get(name="Class 2 @ School 1") self.class_3 = Class.objects.get(name="Class 3 @ School 1") @@ -83,13 +86,20 @@ def test_validate__first_name_not_unique_per_class_in_db(self): many=True, ) - def test_validate__create__teacher__last_name_required(self): + def test_validate__create__teacher_and_student(self): + """Cannot create a user with both teacher and student attributes.""" + self.assert_validate( + attrs={"new_teacher": {}, "new_student": {}}, + error_code="teacher_and_student", + ) + + def test_validate__create__teacher__last_name__required(self): """Teacher's last name is required during creation.""" self.assert_validate( attrs={"new_teacher": {}}, error_code="last_name__required" ) - def test_validate__create__teacher__requesting_to_join_class(self): + def test_validate__requesting_to_join_class__teacher__create(self): """Teacher cannot request to join a class on creation.""" self.assert_validate( attrs={ @@ -97,10 +107,10 @@ def test_validate__create__teacher__requesting_to_join_class(self): "new_teacher": {}, "requesting_to_join_class": "AAAAA", }, - error_code="requesting_to_join_class__teacher", + error_code="requesting_to_join_class__teacher__create", ) - def test_validate__update__teacher__requesting_to_join_class(self): + def test_validate__requesting_to_join_class__teacher__update(self): """Teacher cannot request to join a class on update.""" self.assert_validate( attrs={ @@ -108,27 +118,66 @@ def test_validate__update__teacher__requesting_to_join_class(self): "new_teacher": {}, "requesting_to_join_class": "AAAAA", }, - error_code="requesting_to_join_class__teacher", - parent=UserSerializer( - instance=self.admin_school_teacher_user, - context={ - "request": self.request_factory.patch( - user=self.admin_school_teacher_user - ), - }, - ), + error_code="requesting_to_join_class__teacher__update", + instance=self.admin_school_teacher_user, + context={ + "view": UserViewSet(action="partial_update"), + "request": self.request_factory.patch( + user=self.admin_school_teacher_user + ), + }, ) - def test_validate__student__requesting_to_join_class_and_class_field_forbidden( + def test_validate__requesting_to_join_class__student__create__in_class( self, ): - """Student cannot have both requesting_to_join_class and class_field""" + """ + Student cannot have both requesting_to_join_class and class_field on + creation. + """ self.assert_validate( attrs={ "new_student": {"class_field": "AAAAA"}, "requesting_to_join_class": "BBBBB", }, - error_code="class_and_join_class_mutually_exclusive", + error_code="requesting_to_join_class__student__create__in_class", + ) + + def test_validate__requesting_to_join_class__student__update__in_class( + self, + ): + """Student cannot be updated to request to join a class.""" + self.assert_validate( + attrs={ + "requesting_to_join_class": "BBBBB", + }, + error_code="requesting_to_join_class__student__update__in_class", + instance=self.student_user, + context={ + "view": UserViewSet(action="partial_update"), + "request": self.request_factory.patch( + user=self.admin_school_teacher_user + ), + }, + ) + + def test_validate__class_field__indy__update__requesting_to_join_class( + self, + ): + """ + Independent user with a pending join class request cannot also be + updated to have a class. + """ + self.assert_validate( + attrs={ + "new_student": {"class_field": "AAAAA"}, + }, + error_code="class_field__indy__update__requesting_to_join_class", + instance=self.independent, + context={ + "view": UserViewSet(action="partial_update"), + "request": self.request_factory.patch(user=self.independent), + }, ) def test_validate_requesting_to_join_class__does_not_exist(self): @@ -206,7 +255,9 @@ def setUp(self): def test_validate_first_name(self): """Cannot accept a new student into a class with a duplicate name.""" user_fields = ( - StudentUser.objects.filter(new_student__class_field=self.class_1) + StudentUser.objects.filter( + new_student__class_field=self.indy_user.student.pending_class_request + ) .values("first_name") .first() ) diff --git a/backend/api/tests/views/test_user.py b/backend/api/tests/views/test_user.py index edbfd3b3..53322846 100644 --- a/backend/api/tests/views/test_user.py +++ b/backend/api/tests/views/test_user.py @@ -122,7 +122,7 @@ def test_get_permissions__partial_update__requesting_to_join_class( ), ) - def test_get_permissions__handle_join_class_request(self): + def test_get_permissions__independents__handle_join_class_request(self): """ Only school-teachers can handle an independent's class join request. """ @@ -168,24 +168,11 @@ def _test_get_queryset__student_users( self.assert_get_queryset(student_users, action=action, request=request) def _test_get_queryset__independents__handle_join_class_request( - self, is_admin: bool + self, user: SchoolTeacherUser ): - request = self.client.request_factory.generic( - "patch", - user=self.admin_school_teacher_user - if is_admin - else self.non_admin_school_teacher_user, - ) + request = self.client.request_factory.patch(user=user) - indy_users = list( - IndependentUser.objects.filter( - new_student__pending_class_request__in=Class.objects.filter( - teacher__school=request.school_teacher_user.teacher.school - ) - if request.school_teacher_user.teacher.is_admin - else request.school_teacher_user.teacher.class_teacher.all() - ) - ) + indy_users = list(user.teacher.indy_users) assert indy_users self.assert_get_queryset( @@ -222,7 +209,7 @@ def test_get_queryset__independents__handle_join_class_request__admin(self): """Handling a join class request can only target the independent 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_user ) def test_get_queryset__independents__handle_join_class_request__non_admin( @@ -231,7 +218,7 @@ def test_get_queryset__independents__handle_join_class_request__non_admin( """Handling a join class request can only target the independent 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_user ) def test_get_queryset__destroy(self): @@ -249,7 +236,7 @@ def test_get_queryset__partial_update__student(self): the user is a student.""" return self.assert_get_queryset( [self.indy_user], - action="partial__update", + action="partial_update", request=self.client.request_factory.patch(user=self.indy_user), ) @@ -313,6 +300,8 @@ def test_independents__handle_join_class_request__accept(self): }, ) + pending_class_request = self.indy_user.student.pending_class_request + self.client.patch(viewname, data={"accept": True}) username = self.indy_user.username @@ -320,7 +309,7 @@ def test_independents__handle_join_class_request__accept(self): self.indy_user.refresh_from_db() assert self.indy_user.new_student.pending_class_request is None - assert self.indy_user.new_student.class_field == self.class_1 + assert self.indy_user.new_student.class_field == pending_class_request assert self.indy_user.last_name == "" assert self.indy_user.email == "" assert self.indy_user.username != username diff --git a/backend/api/views/user.py b/backend/api/views/user.py index d54b1ab0..d737f72f 100644 --- a/backend/api/views/user.py +++ b/backend/api/views/user.py @@ -71,13 +71,7 @@ def get_serializer_class(self): def get_queryset(self, user_class=User): if self.action == "independents__handle_join_class_request": - return IndependentUser.objects.filter( - new_student__pending_class_request__in=Class.objects.filter( - teacher__school=self.request.school_teacher_user.teacher.school - ) - if self.request.school_teacher_user.teacher.is_admin - else self.request.school_teacher_user.teacher.class_teacher.all() - ) + return self.request.school_teacher_user.teacher.indy_users queryset = super().get_queryset(user_class) if self.action == "destroy" or ( From 216ba60cfc823fdae5c3a5172d9d977bfac890b6 Mon Sep 17 00:00:00 2001 From: faucomte97 Date: Thu, 7 Mar 2024 19:44:59 +0000 Subject: [PATCH 22/22] Fix spacing --- backend/api/serializers/user.py | 1 - backend/api/tests/serializers/test_user.py | 1 - 2 files changed, 2 deletions(-) diff --git a/backend/api/serializers/user.py b/backend/api/serializers/user.py index e7ffbef9..f0950731 100644 --- a/backend/api/serializers/user.py +++ b/backend/api/serializers/user.py @@ -25,7 +25,6 @@ from .student import StudentSerializer from .teacher import TeacherSerializer - # pylint: disable=missing-class-docstring,too-many-ancestors diff --git a/backend/api/tests/serializers/test_user.py b/backend/api/tests/serializers/test_user.py index dbe92e37..3db2a4f0 100644 --- a/backend/api/tests/serializers/test_user.py +++ b/backend/api/tests/serializers/test_user.py @@ -19,7 +19,6 @@ ) from ...views import UserViewSet - # pylint: disable=missing-class-docstring