diff --git a/greg/api/filters.py b/greg/api/filters.py index e3edf77388883316f09550b04885a9c5be1ff2c9..9a5d938d0ea6c4fbd1715962d518af302d5c82ed 100644 --- a/greg/api/filters.py +++ b/greg/api/filters.py @@ -23,15 +23,20 @@ class RoleFilter(FilterSet): class PersonFilter(FilterSet): - verified = BooleanFilter( - field_name="identities__verified_by_id", lookup_expr="isnull", exclude=True - ) + verified = BooleanFilter(method="verified_filter") active = BooleanFilter(method="active_role_filter") class Meta: model = Person fields = ["first_name", "last_name", "verified", "active"] + def verified_filter(self, queryset, name, value): + if value is True: + return queryset.with_verified_identities() + elif value is False: + return queryset.without_verified_identities() + return queryset + def active_role_filter(self, queryset, name, value): if value: datetime_now = datetime.now() diff --git a/greg/api/serializers/person.py b/greg/api/serializers/person.py index 0bfa97c4b762bd3db385082d5257d93bd7837d52..93958a212eb20b22d59cab74bd0db91b384bc5e0 100644 --- a/greg/api/serializers/person.py +++ b/greg/api/serializers/person.py @@ -75,11 +75,6 @@ class PersonSerializer(serializers.ModelSerializer): "first_name", "last_name", "date_of_birth", - "email", - "mobile_phone", - "email_verified_date", - "mobile_phone", - "mobile_phone_verified_date", "registration_completed_date", "identities", "roles", diff --git a/greg/managers.py b/greg/managers.py new file mode 100644 index 0000000000000000000000000000000000000000..c62a855f61f885303cabd7c318503c8b0143ed5f --- /dev/null +++ b/greg/managers.py @@ -0,0 +1,37 @@ +from django.db.models import Count, Manager, Q, QuerySet +from django.utils import timezone + + +class PersonQuerySet(QuerySet): + def count_verified_identities(self): + # the requirement is minimum one verified passport or national ID number + # TODO: should we make this configurable or simply outsource the logic + # to the systems consuming this data? + return Count( + "identities__id", + filter=self.model.get_verified_identities_query(), + distinct=True, + ) + + def with_verified_identities(self): + """Persons with at least one verified identity.""" + return self.annotate( + verified_identities_count=self.count_verified_identities() + ).filter(verified_identities_count__gte=1) + + def without_verified_identities(self): + """Persons without a verified identity.""" + return self.annotate( + verified_identities_count=self.count_verified_identities() + ).filter(verified_identities_count=0) + + +class PersonManager(Manager): + def get_queryset(self): + return PersonQuerySet(self.model, using=self._db) + + def with_verified_identities(self): + return self.get_queryset().with_verified_identities() + + def without_verified_identities(self): + return self.get_queryset().without_verified_identities() diff --git a/greg/migrations/0009_email_mobile_to_identity.py b/greg/migrations/0009_email_mobile_to_identity.py new file mode 100644 index 0000000000000000000000000000000000000000..95ce20c89aaa78ec2057db7ca0854723fb2d0d3e --- /dev/null +++ b/greg/migrations/0009_email_mobile_to_identity.py @@ -0,0 +1,65 @@ +# Generated by Django 3.2.7 on 2021-10-08 09:49 + +from django.db import migrations, models + + +def move_email_and_mobile_fields(apps, schema_editor): + # we simply move the value, not the date, as we're not in production yet + Person = apps.get_model("greg", "Person") + Identity = apps.get_model("greg", "Identity") + for person in Person.objects.all(): + field_map = ( + ("email", "private_email"), + ("mobile_phone", "private_mobile"), + ) + for field, identity_type in field_map: + value = getattr(person, field, None) + if value: + Identity.objects.create( + person=person, + type=identity_type, + source="greg", + value=value, + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ("greg", "0008_add_invitations"), + ] + + operations = [ + migrations.AlterField( + model_name="identity", + name="type", + field=models.CharField( + choices=[ + ("feide_id", "Feide Id"), + ("passport_number", "Passport Number"), + ("norwegian_national_id_number", "Norwegian National Id Number"), + ("private_email", "Private Email"), + ("private_mobile", "Private Mobile Number"), + ("other", "Other"), + ], + max_length=64, + ), + ), + migrations.RunPython(move_email_and_mobile_fields), + migrations.RemoveField( + model_name="person", + name="email", + ), + migrations.RemoveField( + model_name="person", + name="email_verified_date", + ), + migrations.RemoveField( + model_name="person", + name="mobile_phone", + ), + migrations.RemoveField( + model_name="person", + name="mobile_phone_verified_date", + ), + ] diff --git a/greg/models.py b/greg/models.py index ec14ef9756d9bd98d12464b3d6224279c6bdc9be..1ef4d22f7aba66348deb29086d65caced200f536 100644 --- a/greg/models.py +++ b/greg/models.py @@ -5,10 +5,12 @@ from datetime import date from dirtyfields import DirtyFieldsMixin from django.conf import settings from django.db import models -from django.db.models import Lookup +from django.db.models import Lookup, Q from django.db.models.fields import Field from django.utils import timezone +from greg.managers import PersonManager + @Field.register_lookup class Like(Lookup): @@ -39,10 +41,6 @@ class Person(BaseModel): first_name = models.CharField(max_length=256) last_name = models.CharField(max_length=256) date_of_birth = models.DateField(null=True) - email = models.EmailField(blank=True) - email_verified_date = models.DateField(null=True) - mobile_phone = models.CharField(max_length=15, blank=True) - mobile_phone_verified_date = models.DateField(null=True) registration_completed_date = models.DateField(null=True) user = models.ForeignKey( settings.AUTH_USER_MODEL, @@ -52,6 +50,8 @@ class Person(BaseModel): blank=True, ) + objects = PersonManager() + def __str__(self): return "{} {} ({})".format(self.first_name, self.last_name, self.pk) @@ -87,6 +87,17 @@ class Person(BaseModel): and self.registration_completed_date <= date.today() ) + @staticmethod + def get_verified_identities_query(): + return Q( + identities__type__in=[ + Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, + Identity.IdentityType.PASSPORT_NUMBER, + ], + identities__verified_at__isnull=False, + identities__verified_at__lte=timezone.now(), + ) + @property def is_verified(self) -> bool: """ @@ -113,9 +124,12 @@ class Person(BaseModel): Note that we do not distinguish between the quality, authenticity, or trust level of the guest's associated identities. """ - # the requirement is minimum one personal identity return ( self.identities.filter( + type__in=[ + Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, + Identity.IdentityType.PASSPORT_NUMBER, + ], verified_at__isnull=False, verified_at__lte=timezone.now(), ).count() @@ -222,6 +236,8 @@ class Identity(BaseModel): PASSPORT_NUMBER = "passport_number" # Norwegian national ID - "fødselsnummer" NORWEGIAN_NATIONAL_ID_NUMBER = "norwegian_national_id_number" + PRIVATE_EMAIL = "private_email" + PRIVATE_MOBILE_NUMBER = "private_mobile" # Sponsor writes what is used in the value column OTHER = "other" diff --git a/greg/tests/api/test_person.py b/greg/tests/api/test_person.py index a1268c1e04d318f869d85cd000b5393601bfe6c4..9a09193c4f98c5ac9135afd24bf4bf641273ae10 100644 --- a/greg/tests/api/test_person.py +++ b/greg/tests/api/test_person.py @@ -18,6 +18,11 @@ from greg.models import ( ) +def find(items, key, value): + """Return the first item in `items` where `key` equals `value`.""" + return next(x for x in items if x.get(key) == value) + + @pytest.fixture def role_type_visiting_professor() -> RoleType: return RoleType.objects.create( @@ -114,9 +119,9 @@ def test_persons_verified_filter_exclude( @pytest.mark.django_db def test_add_role( - client, person_foo, role_type_visiting_professor, sponsor_guy, unit_human_resources + client, person, role_type_visiting_professor, sponsor_guy, unit_human_resources ): - url = reverse("v1:person_role-list", kwargs={"person_id": person_foo.id}) + url = reverse("v1:person_role-list", kwargs={"person_id": person.id}) roles_for_person = client.get(url).json()["results"] # Check that there are no roles for the person, and then add one @@ -195,23 +200,23 @@ def test_identity_list( reverse("v1:person_identity-list", kwargs={"person_id": person_id}) ) data = response.json()["results"] - assert len(data) == 2 - - first = data[0] - assert first["person"] == 1 - assert first["type"] == "passport_number" - assert first["source"] == "Test" - assert first["value"] == "12345" - assert first["verified"] == "manual" - assert first["verified_by"] == 1 + assert len(data) == 4 + + passport = find(data, "type", "passport_number") + assert passport["person"] == 1 + assert passport["type"] == "passport_number" + assert passport["source"] == "Test" + assert passport["value"] == "12345" + assert passport["verified"] == "manual" + assert passport["verified_by"] == 1 # assert first["verified_at"] is a datetime string # assert first["created"] is a datetime string @pytest.mark.django_db -def test_identity_add(client, person_foo): +def test_identity_add(client, person): response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 0 @@ -222,12 +227,12 @@ def test_identity_add(client, person_foo): "value": "12345", } client.post( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}), + reverse("v1:person_identity-list", kwargs={"person_id": person.id}), data=data, ) response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 1 @@ -272,9 +277,9 @@ def test_identity_add_valid_duplicate(client, person_foo, person_bar): @pytest.mark.django_db -def test_identity_delete(client, person_foo): +def test_identity_delete(client, person): response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 0 @@ -285,7 +290,7 @@ def test_identity_delete(client, person_foo): "value": "12345", } post_response = client.post( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}), + reverse("v1:person_identity-list", kwargs={"person_id": person.id}), data=data, ) identity_id = post_response.json()["id"] @@ -297,13 +302,13 @@ def test_identity_delete(client, person_foo): "value": "1234413241235", } post_response2 = client.post( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}), + reverse("v1:person_identity-list", kwargs={"person_id": person.id}), data=data, ) identity_id2 = post_response2.json()["id"] response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 2 @@ -312,13 +317,13 @@ def test_identity_delete(client, person_foo): client.delete( reverse( "v1:person_identity-detail", - kwargs={"person_id": person_foo.id, "id": identity_id}, + kwargs={"person_id": person.id, "id": identity_id}, ) ) # Check that the other identity is still there response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] @@ -327,9 +332,9 @@ def test_identity_delete(client, person_foo): @pytest.mark.django_db -def test_identity_update(client, person_foo): +def test_identity_update(client, person): response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 0 @@ -340,12 +345,12 @@ def test_identity_update(client, person_foo): "value": "12345", } client.post( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}), + reverse("v1:person_identity-list", kwargs={"person_id": person.id}), data=data, ) response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 1 @@ -364,14 +369,14 @@ def test_identity_update(client, person_foo): patch_response = client.patch( reverse( "v1:person_identity-detail", - kwargs={"person_id": person_foo.id, "id": identity_id}, + kwargs={"person_id": person.id, "id": identity_id}, ), data=data_updated, ) assert patch_response.status_code == status.HTTP_200_OK response = client.get( - reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) + reverse("v1:person_identity-list", kwargs={"person_id": person.id}) ) results = response.json()["results"] assert len(results) == 1 @@ -394,7 +399,7 @@ def test_remove_person( response = client.get( reverse("v1:person_identity-list", kwargs={"person_id": person_foo.id}) ) - assert len(response.json()["results"]) == 2 + assert len(response.json()["results"]) == 4 # Delete the person and check that the data has been removed client.delete(reverse("v1:person-detail", kwargs={"id": person_foo.id})) diff --git a/greg/tests/conftest.py b/greg/tests/conftest.py index 71a08c36919fc2281a99ec0f4050c5124950469a..1998517c8c42c728e25053a4c150606aef750146 100644 --- a/greg/tests/conftest.py +++ b/greg/tests/conftest.py @@ -3,6 +3,7 @@ import logging from rest_framework.authtoken.models import Token from rest_framework.test import APIClient from django.contrib.auth import get_user_model +from django.db import transaction import pytest @@ -43,27 +44,55 @@ def pcm_mock(): @pytest.fixture -def person_foo() -> Person: +def person() -> Person: pe = Person.objects.create( - first_name="Foo", - last_name="Foo", - date_of_birth="2001-01-27", - email="test@example.org", - mobile_phone="123456788", + first_name="Test", + last_name="Tester", + date_of_birth="2000-01-27", ) return Person.objects.get(id=pe.id) +@pytest.fixture +def person_foo() -> Person: + with transaction.atomic(): + person = Person.objects.create( + first_name="Foo", + last_name="Foo", + date_of_birth="2001-01-27", + ) + Identity.objects.create( + person=person, + type=Identity.IdentityType.PRIVATE_EMAIL, + value="foo@example.org", + ) + Identity.objects.create( + person=person, + type=Identity.IdentityType.PRIVATE_MOBILE_NUMBER, + value="+4712345678", + ) + return Person.objects.get(id=person.id) + + @pytest.fixture def person_bar() -> Person: - pe = Person.objects.create( - first_name="Bar", - last_name="Bar", - date_of_birth="2000-07-01", - email="test2@example.org", - mobile_phone="123456789", - ) - return Person.objects.get(id=pe.id) + with transaction.atomic(): + person = Person.objects.create( + first_name="Bar", + last_name="Bar", + date_of_birth="2000-07-01", + ) + Identity.objects.create( + person=person, + type=Identity.IdentityType.PRIVATE_EMAIL, + value="bar@example.org", + ) + Identity.objects.create( + person=person, + type=Identity.IdentityType.PRIVATE_MOBILE_NUMBER, + value="+4712345678", + ) + return Person.objects.get(id=person.id) @pytest.fixture @@ -129,18 +158,6 @@ def role_person_foo( return Role.objects.get(id=role.id) -@pytest.fixture -def person() -> Person: - pe = Person.objects.create( - first_name="Test", - last_name="Tester", - date_of_birth="2000-01-27", - email="test@example.org", - mobile_phone="123456789", - ) - return Person.objects.get(id=pe.id) - - @pytest.fixture def consent_type() -> ConsentType: ct = ConsentType.objects.create( diff --git a/greg/tests/models/test_identity.py b/greg/tests/models/test_identity.py index 2b1b4d123affb21eec96c15aa222775977c79c7d..f004d70af61cd1c92be8a47bfa98880c0aae56d1 100644 --- a/greg/tests/models/test_identity.py +++ b/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=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>))" + == "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>))" ) diff --git a/greg/tests/models/test_person.py b/greg/tests/models/test_person.py index 44d451b42e1b1e0efb6e15b35335be2a9459b69b..396b0f0e5e3f1fe994ca70732e470cf7cda364f8 100644 --- a/greg/tests/models/test_person.py +++ b/greg/tests/models/test_person.py @@ -31,8 +31,6 @@ def person_registered_past() -> Person: first_name="Test", last_name="Tester", date_of_birth="2000-01-27", - email="test@example.org", - mobile_phone="123456789", registration_completed_date=_a_year_ago, ) return Person.objects.get(id=1) @@ -44,8 +42,6 @@ def person_registered_future() -> Person: first_name="Test", last_name="Tester", date_of_birth="2000-01-27", - email="test@example.org", - mobile_phone="123456789", registration_completed_date=_a_year_into_future, ) return Person.objects.get(id=1) @@ -59,6 +55,14 @@ def feide_id() -> Identity: ) +@pytest.fixture +def verified_national_id_number() -> Identity: + return Identity( + type=Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, + verified_at=_a_year_ago, + ) + + @pytest.fixture def passport() -> Identity: return Identity( @@ -131,11 +135,21 @@ def test_is_registered_completed_in_future(person_registered_future): @pytest.mark.django_db -@pytest.mark.parametrize("identity_type", Identity.IdentityType) -def test_is_verified(identity_type, person): +@pytest.mark.parametrize( + ("identity_type", "is_verified"), + ( + (Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER, True), + (Identity.IdentityType.PASSPORT_NUMBER, True), + (Identity.IdentityType.PRIVATE_EMAIL, False), + (Identity.IdentityType.PRIVATE_MOBILE_NUMBER, False), + (Identity.IdentityType.FEIDE_ID, False), + (Identity.IdentityType.OTHER, False), + ), +) +def test_is_verified(identity_type, is_verified, person): identity = Identity(type=identity_type, verified_at=_a_year_ago) person.identities.add(identity, bulk=False) - assert person.is_verified + assert person.is_verified == is_verified @pytest.mark.django_db @@ -153,11 +167,13 @@ def test_is_verified_at_identity_is_unverified(person, unverified_passport): @pytest.mark.django_db def test_is_verified_mixed_verified_and_unverified_identities( person, - feide_id, + verified_national_id_number, unverified_passport, future_identity, ): - person.identities.add(feide_id, unverified_passport, future_identity, bulk=False) + person.identities.add( + verified_national_id_number, unverified_passport, future_identity, bulk=False + ) assert person.is_verified diff --git a/greg/tests/test_signals.py b/greg/tests/test_signals.py index f54eafb24d11f82bd2186bb3b29e1aa85f0cc305..1570fd26c785cc228fe2e339c576841a3c16fb39 100644 --- a/greg/tests/test_signals.py +++ b/greg/tests/test_signals.py @@ -4,10 +4,10 @@ from greg.models import Notification @pytest.mark.django_db -def test_delete_signal_person(person_foo): +def test_delete_signal_person(person): """Check that a Notification is created when a Person object is deleted.""" assert Notification.objects.count() == 1 - person_foo.delete() + person.delete() assert Notification.objects.count() == 2 diff --git a/gregui/api/serializers/guest.py b/gregui/api/serializers/guest.py index 481bbfdf48e19989884f738c8be454f7ab733945..203aa441999692ad571fb96c288eb43f8f3b39d0 100644 --- a/gregui/api/serializers/guest.py +++ b/gregui/api/serializers/guest.py @@ -1,12 +1,23 @@ +from django.db import transaction from rest_framework import serializers -from greg.models import Person +from greg.models import Identity, Person class GuestRegisterSerializer(serializers.ModelSerializer): + email = serializers.CharField(required=True) + def create(self, validated_data): - obj = super().create(validated_data) - return obj + # TODO: this serializer is untested + email = validated_data.pop("email") + with transaction.atomic(): + person = super().create(**validated_data) + Identity.objects.create( + person=person, + type=Identity.IdentityType.PRIVATE_EMAIL, + value=email, + ) + return person class Meta: model = Person @@ -15,5 +26,4 @@ class GuestRegisterSerializer(serializers.ModelSerializer): extra_kwargs = { "first_name": {"required": True}, "last_name": {"required": True}, - "email": {"required": True}, } diff --git a/gregui/api/serializers/invitation.py b/gregui/api/serializers/invitation.py index bbb5a5cae80bef0a58e08689a840b8b78a35d016..5a57e99edd15c7f3ab803973c8e896f6ba979a50 100644 --- a/gregui/api/serializers/invitation.py +++ b/gregui/api/serializers/invitation.py @@ -4,16 +4,18 @@ from django.db import transaction from django.utils import timezone from rest_framework import serializers -from greg.models import Invitation, InvitationLink, Person, Role +from greg.models import Invitation, InvitationLink, Person, Role, Identity from gregui.api.serializers.role import RoleSerializerUi from gregui.models import GregUserProfile class InviteGuestSerializer(serializers.ModelSerializer): - role = RoleSerializerUi() + email = serializers.EmailField(required=True) + role = RoleSerializerUi(required=True) uuid = serializers.UUIDField(read_only=True) def create(self, validated_data): + email = validated_data.pop("email") role_data = validated_data.pop("role") user = GregUserProfile.objects.get(user=self.context["request"].user) @@ -21,6 +23,12 @@ class InviteGuestSerializer(serializers.ModelSerializer): # Create objects with transaction.atomic(): person = Person.objects.create(**validated_data) + Identity.objects.create( + person=person, + type=Identity.IdentityType.PRIVATE_EMAIL, + value=email, + source="greg", + ) role_data["person"] = person role_data["sponsor_id"] = user.sponsor role = Role.objects.create(**role_data) @@ -33,7 +41,15 @@ class InviteGuestSerializer(serializers.ModelSerializer): class Meta: model = Person - fields = ("id", "first_name", "last_name", "date_of_birth", "role", "uuid") + fields = ( + "id", + "first_name", + "last_name", + "email", + "date_of_birth", + "role", + "uuid", + ) read_only_field = ("uuid",) diff --git a/gregui/tests/api/test_invite_guest.py b/gregui/tests/api/test_invite_guest.py index 8b6c2e79deb4ec25b41e40c653f8c35eef8e76fb..55a962e330cf5bd92fbbf94f511da9f41ecfec73 100644 --- a/gregui/tests/api/test_invite_guest.py +++ b/gregui/tests/api/test_invite_guest.py @@ -3,7 +3,7 @@ from rest_framework import status from rest_framework.reverse import reverse from rest_framework.test import APIRequestFactory, force_authenticate -from greg.models import Person +from greg.models import Identity, Person from gregui.api.views.invitation import CreateInvitationView @@ -34,8 +34,13 @@ def test_invite_guest(client, user_sponsor, unit_foo, role_type_foo): assert response.status_code == status.HTTP_201_CREATED - all_persons = Person.objects.all() + assert Person.objects.count() == 1 + person = Person.objects.first() + assert person.first_name == "Foo" + assert person.last_name == "Bar" - assert len(all_persons) == 1 - assert all_persons[0].first_name == "Foo" - assert all_persons[0].last_name == "Bar" + assert Identity.objects.filter( + person=person, + type=Identity.IdentityType.PRIVATE_EMAIL, + value="test@example.com", + ).exists()