From b65f1a63d34d10763914d0d096ae1741ff5a3aed Mon Sep 17 00:00:00 2001 From: Seamus Johnston Date: Tue, 7 Mar 2023 11:07:20 -0600 Subject: [PATCH] Update post_save to work with Contact --- src/djangooidc/backends.py | 12 +- .../0014_user_phone_alter_contact_user.py | 27 +++++ src/registrar/models/user.py | 9 ++ src/registrar/signals.py | 48 ++++++-- src/registrar/tests/test_signals.py | 103 ++++++++++++++++++ 5 files changed, 180 insertions(+), 19 deletions(-) create mode 100644 src/registrar/migrations/0014_user_phone_alter_contact_user.py create mode 100644 src/registrar/tests/test_signals.py diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 52cbc712b..ac65c89d4 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -28,18 +28,14 @@ class OpenIdConnectBackend(ModelBackend): UserModel = get_user_model() username = self.clean_username(kwargs["sub"]) - if "upn" in kwargs.keys(): - username = kwargs["upn"] # Some OP may actually choose to withhold some information, so we must # test if it is present openid_data = {"last_login": timezone.now()} - openid_data["first_name"] = kwargs.get("first_name", "") openid_data["first_name"] = kwargs.get("given_name", "") - openid_data["first_name"] = kwargs.get("christian_name", "") openid_data["last_name"] = kwargs.get("family_name", "") - openid_data["last_name"] = kwargs.get("last_name", "") openid_data["email"] = kwargs.get("email", "") + openid_data["phone"] = kwargs.get("phone", "") # Note that this could be accomplished in one try-except clause, but # instead we use get_or_create when creating unknown users since it has @@ -47,6 +43,7 @@ class OpenIdConnectBackend(ModelBackend): if getattr(settings, "OIDC_CREATE_UNKNOWN_USER", True): args = { UserModel.USERNAME_FIELD: username, + # defaults _will_ be updated, these are not fallbacks "defaults": openid_data, } user, created = UserModel.objects.update_or_create(**args) @@ -56,10 +53,7 @@ class OpenIdConnectBackend(ModelBackend): try: user = UserModel.objects.get_by_natural_key(username) except UserModel.DoesNotExist: - try: - user = UserModel.objects.get(email=kwargs["email"]) - except UserModel.DoesNotExist: - return None + return None return user def clean_username(self, username): diff --git a/src/registrar/migrations/0014_user_phone_alter_contact_user.py b/src/registrar/migrations/0014_user_phone_alter_contact_user.py new file mode 100644 index 000000000..452b2b9d5 --- /dev/null +++ b/src/registrar/migrations/0014_user_phone_alter_contact_user.py @@ -0,0 +1,27 @@ +# Generated by Django 4.1.6 on 2023-03-07 16:39 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import phonenumber_field.modelfields # type: ignore + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0013_publiccontact_contact_user"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="phone", + field=phonenumber_field.modelfields.PhoneNumberField( + blank=True, + db_index=True, + help_text="Phone", + max_length=128, + null=True, + region=None, + ), + ), + ] diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index acfd1769e..9fc726114 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -1,5 +1,7 @@ from django.contrib.auth.models import AbstractUser +from phonenumber_field.modelfields import PhoneNumberField # type: ignore + class User(AbstractUser): """ @@ -7,6 +9,13 @@ class User(AbstractUser): but can be customized later. """ + phone = PhoneNumberField( + null=True, + blank=True, + help_text="Phone", + db_index=True, + ) + def __str__(self): # this info is pulled from Login.gov if self.first_name or self.last_name: diff --git a/src/registrar/signals.py b/src/registrar/signals.py index 492f535d0..62c5873f0 100644 --- a/src/registrar/signals.py +++ b/src/registrar/signals.py @@ -5,7 +5,7 @@ from django.core.management import call_command from django.db.models.signals import post_save, post_migrate from django.dispatch import receiver -from .models import User, UserProfile +from .models import User, Contact logger = logging.getLogger(__name__) @@ -15,18 +15,46 @@ logger = logging.getLogger(__name__) def handle_profile(sender, instance, **kwargs): """Method for when a User is saved. - If the user is being created, then create a matching UserProfile. Otherwise - save an updated profile or create one if it doesn't exist. + A first time registrant may have been invited, so we'll search for a matching + Contact record, by email address, and associate them, if possible. + + A first time registrant may not have a matching Contact, so we'll create one, + copying the contact values we received from Login.gov in order to initialize it. + + During subsequent login, a User record may be updated with new data from Login.gov, + but in no case will we update contact values on an existing Contact record. """ - if kwargs.get("created", False): - UserProfile.objects.create(user=instance) + first_name = getattr(instance, "first_name", "") + last_name = getattr(instance, "last_name", "") + email = getattr(instance, "email", "") + phone = getattr(instance, "phone", "") + + is_new_user = kwargs.get("created", False) + + if is_new_user: + contacts = Contact.objects.filter(email=email) else: - # the user is not being created. - if hasattr(instance, "userprofile"): - instance.userprofile.save() - else: - UserProfile.objects.create(user=instance) + contacts = Contact.objects.filter(user=instance) + + if len(contacts) == 0: # no matching contact + Contact.objects.create( + user=instance, + first_name=first_name, + last_name=last_name, + email=email, + phone=phone, + ) + + if len(contacts) >= 1 and is_new_user: # a matching contact + contacts[0].user = instance + contacts[0].save() + + if len(contacts) > 1: # multiple matches + logger.warning( + "There are multiple Contacts with the same email address." + f" Picking #{contacts[0].id} for User #{instance.id}." + ) @receiver(post_migrate) diff --git a/src/registrar/tests/test_signals.py b/src/registrar/tests/test_signals.py new file mode 100644 index 000000000..ddc47e60d --- /dev/null +++ b/src/registrar/tests/test_signals.py @@ -0,0 +1,103 @@ +from django.test import TestCase +from django.contrib.auth import get_user_model + +from registrar.models import Contact + + +class TestUserPostSave(TestCase): + def setUp(self): + self.username = "test_user" + self.first_name = "First" + self.last_name = "Last" + self.email = "info@example.com" + self.phone = "202-555-0133" + + self.preferred_first_name = "One" + self.preferred_last_name = "Two" + self.preferred_email = "front_desk@example.com" + self.preferred_phone = "202-555-0134" + + def test_user_created_without_matching_contact(self): + """Expect 1 Contact containing data copied from User.""" + self.assertEquals(len(Contact.objects.all()), 0) + user = get_user_model().objects.create( + username=self.username, + first_name=self.first_name, + last_name=self.last_name, + email=self.email, + phone=self.phone, + ) + actual = Contact.objects.get(user=user) + self.assertEquals(actual.first_name, self.first_name) + self.assertEquals(actual.last_name, self.last_name) + self.assertEquals(actual.email, self.email) + self.assertEquals(actual.phone, self.phone) + + def test_user_created_with_matching_contact(self): + """Expect 1 Contact associated, but with no data copied from User.""" + self.assertEquals(len(Contact.objects.all()), 0) + Contact.objects.create( + first_name=self.preferred_first_name, + last_name=self.preferred_last_name, + email=self.email, # must be the same, to find the match! + phone=self.preferred_phone, + ) + user = get_user_model().objects.create( + username=self.username, + first_name=self.first_name, + last_name=self.last_name, + email=self.email, + ) + actual = Contact.objects.get(user=user) + self.assertEquals(actual.first_name, self.preferred_first_name) + self.assertEquals(actual.last_name, self.preferred_last_name) + self.assertEquals(actual.email, self.email) + self.assertEquals(actual.phone, self.preferred_phone) + + def test_user_updated_without_matching_contact(self): + """Expect 1 Contact containing data copied from User.""" + # create the user + self.assertEquals(len(Contact.objects.all()), 0) + user = get_user_model().objects.create( + username=self.username, first_name="", last_name="", email="", phone="" + ) + # delete the contact + Contact.objects.all().delete() + self.assertEquals(len(Contact.objects.all()), 0) + # modify the user + user.username = self.username + user.first_name = self.first_name + user.last_name = self.last_name + user.email = self.email + user.phone = self.phone + user.save() + # test + actual = Contact.objects.get(user=user) + self.assertEquals(actual.first_name, self.first_name) + self.assertEquals(actual.last_name, self.last_name) + self.assertEquals(actual.email, self.email) + self.assertEquals(actual.phone, self.phone) + + def test_user_updated_with_matching_contact(self): + """Expect 1 Contact associated, but with no data copied from User.""" + # create the user + self.assertEquals(len(Contact.objects.all()), 0) + user = get_user_model().objects.create( + username=self.username, + first_name=self.first_name, + last_name=self.last_name, + email=self.email, + phone=self.phone, + ) + # modify the user + user.first_name = self.preferred_first_name + user.last_name = self.preferred_last_name + user.email = self.preferred_email + user.phone = self.preferred_phone + user.save() + # test + actual = Contact.objects.get(user=user) + self.assertEquals(actual.first_name, self.first_name) + self.assertEquals(actual.last_name, self.last_name) + self.assertEquals(actual.email, self.email) + self.assertEquals(actual.phone, self.phone)