Merge branch 'GREG-71-person-email-mobile-to-identity-part-2' into 'master'

Fix handling of private email and mobile for the invitation endpoints

See merge request !107

.pylintrc

@@ -18,4 +18,5 @@ disable=
     redefined-outer-name,
     too-few-public-methods,
     too-many-ancestors,
+    too-many-arguments,
     unused-argument,

greg/models.py

 import uuid
 
 from datetime import date
+from typing import Optional
 
 from dirtyfields import DirtyFieldsMixin
 from django.conf import settings
@@ -63,6 +64,18 @@ class Person(BaseModel):
             self.last_name,
         )
 
+    @property
+    def private_email(self) -> Optional["Identity"]:
+        """The user provided private email address."""
+        return self.identities.filter(type=Identity.IdentityType.PRIVATE_EMAIL).first()
+
+    @property
+    def private_mobile(self) -> Optional["Identity"]:
+        """The user provided private mobile number."""
+        return self.identities.filter(
+            type=Identity.IdentityType.PRIVATE_MOBILE_NUMBER
+        ).first()
+
     @property
     def is_registered(self) -> bool:
         """
@@ -257,10 +270,19 @@ class Identity(BaseModel):
     )
     verified_at = models.DateTimeField(null=True)
 
+    def __str__(self):
+        return "{}(id={!r}, type={!r}, value={!r})".format(
+            self.__class__.__name__,
+            self.pk,
+            self.type,
+            self.value,
+        )
+
     def __repr__(self):
-        return "{}(id={!r}, type={!r}, source={!r}, value={!r}, verified_by={!r}, verified_at={!r})".format(
+        return "{}(id={!r}, person_id={!r}, type={!r}, source={!r}, value={!r}, verified_by={!r}, verified_at={!r})".format(
             self.__class__.__name__,
             self.pk,
+            self.person_id,
             self.type,
             self.source,
             self.value,

greg/tests/models/test_identity.py

@@ -5,5 +5,5 @@ import pytest
 def test_identity_repr(person_foo_verified):
     assert (
         repr(person_foo_verified)
-        == "Identity(id=3, type='passport_number', source='Test', value='12345', verified_by=Sponsor(id=1, feide_id='guy@example.org', first_name='Sponsor', last_name='Guy'), verified_at=datetime.datetime(2021, 6, 15, 12, 34, 56, tzinfo=<UTC>))"
+        == "Identity(id=3, person_id=1, type='passport_number', source='Test', value='12345', verified_by=Sponsor(id=1, feide_id='guy@example.org', first_name='Sponsor', last_name='Guy'), verified_at=datetime.datetime(2021, 6, 15, 12, 34, 56, tzinfo=<UTC>))"
     )

gregui/api/serializers/guest.py

-from django.db import transaction
 from rest_framework import serializers
 
 from greg.models import Identity, Person
 
 
 class GuestRegisterSerializer(serializers.ModelSerializer):
+    first_name = serializers.CharField(required=True)
+    last_name = serializers.CharField(required=True)
     email = serializers.CharField(required=True)
+    mobile_phone = serializers.CharField(required=True)
 
-    def create(self, validated_data):
-        # TODO: this serializer is untested
+    def update(self, instance, validated_data):
         email = validated_data.pop("email")
-        with transaction.atomic():
-            person = super().create(**validated_data)
+        mobile_phone = validated_data.pop("mobile_phone")
+
+        if not instance.private_email:
             Identity.objects.create(
-                person=person,
+                person=instance,
                 type=Identity.IdentityType.PRIVATE_EMAIL,
                 value=email,
             )
-        return person
+        else:
+            instance.private_email.value = email
+            instance.private_email.save()
+
+        if not instance.private_mobile:
+            Identity.objects.create(
+                person=instance,
+                type=Identity.IdentityType.PRIVATE_MOBILE_NUMBER,
+                value=mobile_phone,
+            )
+        else:
+            instance.private_mobile.value = mobile_phone
+            instance.private_mobile.save()
+
+        # TODO: we only want to allow changing the name if we don't have one
+        #       from a reliable source (Feide/KORR)
+        instance.first_name = validated_data["first_name"]
+        instance.last_name = validated_data["last_name"]
+
+        return instance
 
     class Meta:
         model = Person
-        fields = ("id", "first_name", "last_name", "email")
+        fields = ("id", "first_name", "last_name", "email", "mobile_phone")
         read_only_fields = ("id",)
-        extra_kwargs = {
-            "first_name": {"required": True},
-            "last_name": {"required": True},
-        }

gregui/api/serializers/invitation.py

@@ -51,6 +51,3 @@ class InviteGuestSerializer(serializers.ModelSerializer):
             "uuid",
         )
         read_only_field = ("uuid",)
-
-
-foo = InviteGuestSerializer()

gregui/api/views/invitation.py

@@ -8,7 +8,7 @@ from django.http.response import JsonResponse
 from django.utils import timezone
 from rest_framework import serializers, status
 from rest_framework.authentication import SessionAuthentication, BasicAuthentication
-from rest_framework.generics import CreateAPIView
+from rest_framework.generics import CreateAPIView, GenericAPIView
 from rest_framework.parsers import JSONParser
 from rest_framework.permissions import AllowAny, IsAuthenticated
 from rest_framework.response import Response
@@ -64,6 +64,8 @@ class CreateInvitationView(CreateAPIView):
             data=request.data, context={"request": request}
         )
         serializer.is_valid(raise_exception=True)
+
+        # TODO: check that sponsor has access to OU
         person = serializer.save()
 
         invitationlink = InvitationLink.objects.filter(
@@ -99,7 +101,7 @@ class CheckInvitationView(APIView):
         return Response(status=status.HTTP_200_OK)
 
 
-class InvitedGuestView(APIView):
+class InvitedGuestView(GenericAPIView):
     authentication_classes = [SessionAuthentication, BasicAuthentication]
     permission_classes = [AllowAny]
     parser_classes = [JSONParser]
@@ -140,8 +142,8 @@ class InvitedGuestView(APIView):
             "person": {
                 "first_name": person.first_name,
                 "last_name": person.last_name,
-                "email": person.email,
-                "mobile_phone": person.mobile_phone,
+                "email": person.private_email and person.private_email.value,
+                "mobile_phone": person.private_mobile and person.private_mobile.value,
                 "fnr": fnr,
                 "passport": passport,
             },
@@ -172,22 +174,20 @@ class InvitedGuestView(APIView):
         invite_id = request.session.get("invite_id")
         data = request.data
 
+        # Ensure the invitation link is valid and not expired
+        try:
+            invite_link = InvitationLink.objects.get(uuid=invite_id)
+        except (InvitationLink.DoesNotExist, exceptions.ValidationError):
+            return Response(status=status.HTTP_403_FORBIDDEN)
+        if invite_link.expire <= timezone.now():
+            return Response(status=status.HTTP_403_FORBIDDEN)
+
+        person = invite_link.invitation.role.person
+
         with transaction.atomic():
-            # Ensure the invitation link is valid and not expired
-            try:
-                invite_link = InvitationLink.objects.get(uuid=invite_id)
-            except (InvitationLink.DoesNotExist, exceptions.ValidationError):
-                return Response(status=status.HTTP_403_FORBIDDEN)
-            if invite_link.expire <= timezone.now():
-                return Response(status=status.HTTP_403_FORBIDDEN)
-
-            # Get objects to update
-            person = invite_link.invitation.role.person
-
-            # Update with input from the guest
-            mobile = data.get("mobile_phone")
-            if mobile:
-                person.mobile_phone = data["mobile_phone"]
+            serializer = self.get_serializer(instance=person, data=request.data)
+            serializer.is_valid(raise_exception=True)
+            person = serializer.save()
 
             # Mark guest interaction done
             person.registration_completed_date = timezone.now().date()
@@ -197,4 +197,4 @@ class InvitedGuestView(APIView):
             invite_link.expire = timezone.now()
             invite_link.save()
             # TODO: Send an email to the sponsor?
-        return Response(status=status.HTTP_201_CREATED)
+        return Response(status=status.HTTP_200_OK)

gregui/tests/api/test_invitation.py

@@ -43,7 +43,9 @@ def test_get_invited_info_no_session(client, invitation_link):
 
 
 @pytest.mark.django_db
-def test_get_invited_info_session_okay(client, invitation_link):
+def test_get_invited_info_session_okay(
+    client, invitation_link, person_foo_data, sponsor_guy_data, role_type_foo, unit_foo
+):
     # get a session
     client.get(
         reverse("gregui-v1:invite-verify", kwargs={"uuid": invitation_link.uuid})
@@ -52,20 +54,39 @@ def test_get_invited_info_session_okay(client, invitation_link):
     response = client.get(reverse("gregui-v1:invited-info"))
     assert response.status_code == status.HTTP_200_OK
     data = response.json()
-    assert data.get("person")
-    assert data.get("sponsor")
-    assert data.get("role")
+    assert data.get("person") == dict(
+        **person_foo_data,
+        email=None,
+        mobile_phone=None,
+        fnr=None,
+        passport=None,
+    )
+    assert data.get("sponsor") == dict(
+        first_name=sponsor_guy_data["first_name"],
+        last_name=sponsor_guy_data["last_name"],
+    )
+    assert data.get("role") == dict(
+        start=None,
+        end="2050-10-15",
+        comments="",
+        ou_name_en=unit_foo.name_en,
+        ou_name_nb=unit_foo.name_nb,
+        role_name_en=role_type_foo.name_en,
+        role_name_nb=role_type_foo.name_nb,
+    )
 
 
 @pytest.mark.django_db
-def test_get_invited_info_expired_link(client, invitation_link):
+def test_get_invited_info_expired_link(
+    client, invitation_link, invitation_expired_date
+):
     # Get a session while link is valid
     client.get(
         reverse("gregui-v1:invite-verify", kwargs={"uuid": invitation_link.uuid})
     )
     # Set expire link to expire long ago
     invlink = InvitationLink.objects.get(uuid=invitation_link.uuid)
-    invlink.expire = "1970-01-01"
+    invlink.expire = invitation_expired_date
     invlink.save()
     # Make a get request that should fail because invite expired after login, but
     # before get to userinfo
@@ -74,45 +95,46 @@ def test_get_invited_info_expired_link(client, invitation_link):
 
 
 @pytest.mark.django_db
-def test_post_invited_info_ok_mobile_update(client: APIClient, invitation_link):
+def test_invited_guest_can_post_information(
+    client: APIClient, invitation_link, person_foo_data
+):
     # get a session
     client.get(
         reverse("gregui-v1:invite-verify", kwargs={"uuid": invitation_link.uuid})
     )
+
+    person = invitation_link.invitation.role.person
+    assert person.private_mobile is None
+
     # post updated info to confirm from guest
-    new_phone = "12345678"
-    post_data = {"mobile_phone": new_phone}
+    new_email = "private@example.org"
+    new_phone = "+4712345678"
+    data = dict(email=new_email, mobile_phone=new_phone, **person_foo_data)
     response = client.post(
         reverse("gregui-v1:invited-info"),
-        post_data,
+        data,
         format="json",
     )
-    assert response.status_code == status.HTTP_201_CREATED
-    # Check that the object was updated in the database
-    person = Person.objects.get(id=invitation_link.invitation.role.person.id)
-    assert person.mobile_phone == new_phone
-
+    print(response.content)
+    assert response.status_code == status.HTTP_200_OK
 
-@pytest.mark.django_db
-def test_post_invited_info_ok(client, invitation_link):
-    # get a session
-    client.get(
-        reverse("gregui-v1:invite-verify", kwargs={"uuid": invitation_link.uuid})
-    )
-    # post updated info to confirm from guest
-    response = client.post(reverse("gregui-v1:invited-info"))
-    assert response.status_code == status.HTTP_201_CREATED
+    # Check that the object was updated in the database
+    assert Person.objects.count() == 1
+    assert person.private_email.value == new_email
+    assert person.private_mobile.value == new_phone
 
 
 @pytest.mark.django_db
-def test_post_invited_info_expired_session(client, invitation_link):
+def test_post_invited_info_expired_session(
+    client, invitation_link, invitation_expired_date
+):
     # get a session
     client.get(
         reverse("gregui-v1:invite-verify", kwargs={"uuid": invitation_link.uuid})
     )
     # Set expire link to expire long ago
     invlink = InvitationLink.objects.get(uuid=invitation_link.uuid)
-    invlink.expire = "1970-01-01"
+    invlink.expire = invitation_expired_date
     invlink.save()
     # post updated info to confirm from guest, should fail because of expired
     # invitation link

gregui/tests/conftest.py

+import datetime
 import logging
 
+import pytest
+
+
 from django.contrib.auth import get_user_model
+from django.utils.timezone import make_aware
 from rest_framework.authtoken.admin import User
 from rest_framework.test import APIClient
 
-import pytest
 
 from greg.models import (
     Invitation,
@@ -30,21 +34,28 @@ def client() -> APIClient:
 
 @pytest.fixture
 def unit_foo() -> OrganizationalUnit:
-    ou = OrganizationalUnit.objects.create(orgreg_id="12345", name_en="foo_unit")
+    ou = OrganizationalUnit.objects.create(
+        orgreg_id="12345", name_en="Foo EN", name_nb="Foo NB"
+    )
     return OrganizationalUnit.objects.get(id=ou.id)
 
 
 @pytest.fixture
 def role_type_foo() -> RoleType:
-    rt = RoleType.objects.create(identifier="role_foo", name_en="Role Foo")
+    rt = RoleType.objects.create(
+        identifier="role_foo", name_en="Role Foo EN", name_nb="Role Foo NB"
+    )
     return RoleType.objects.get(id=rt.id)
 
 
 @pytest.fixture
-def sponsor_guy(unit_foo: OrganizationalUnit) -> Sponsor:
-    sponsor = Sponsor.objects.create(
-        feide_id="guy@example.org", first_name="Sponsor", last_name="Guy"
-    )
+def sponsor_guy_data() -> dict:
+    return dict(feide_id="guy@example.org", first_name="Sponsor", last_name="Guy")
+
+
+@pytest.fixture
+def sponsor_guy(unit_foo: OrganizationalUnit, sponsor_guy_data) -> Sponsor:
+    sponsor = Sponsor.objects.create(**sponsor_guy_data)
     sponsor.units.add(unit_foo, through_defaults={"hierarchical_access": False})
     return Sponsor.objects.get(id=sponsor.id)
 
@@ -67,8 +78,16 @@ def user_sponsor(sponsor_guy: Sponsor) -> User:
 
 
 @pytest.fixture
-def person() -> Person:
-    pe = Person.objects.create()
+def person_foo_data() -> dict:
+    return dict(
+        first_name="Foo",
+        last_name="Bar",
+    )
+
+
+@pytest.fixture
+def person(person_foo_data) -> Person:
+    pe = Person.objects.create(**person_foo_data)
     return Person.objects.get(id=pe.id)
 
 
@@ -91,12 +110,26 @@ def invitation(role) -> Invitation:
 
 
 @pytest.fixture
-def invitation_link(invitation) -> InvitationLink:
-    il = InvitationLink.objects.create(invitation=invitation, expire="2060-10-15")
+def invitation_valid_date() -> datetime.datetime:
+    return make_aware(datetime.datetime(2060, 10, 15))
+
+
+@pytest.fixture
+def invitation_expired_date() -> datetime.datetime:
+    return make_aware(datetime.datetime(1970, 1, 1))
+
+
+@pytest.fixture
+def invitation_link(invitation, invitation_valid_date) -> InvitationLink:
+    il = InvitationLink.objects.create(
+        invitation=invitation, expire=invitation_valid_date
+    )
     return InvitationLink.objects.get(id=il.id)
 
 
 @pytest.fixture
-def invitation_link_expired(invitation) -> InvitationLink:
-    il = InvitationLink.objects.create(invitation=invitation, expire="1970-01-01")
+def invitation_link_expired(invitation, invitation_expired_date) -> InvitationLink:
+    il = InvitationLink.objects.create(
+        invitation=invitation, expire=invitation_expired_date
+    )
     return InvitationLink.objects.get(id=il.id)