From 781b9f7b810b9003e745b96ce9f608937bf43aa8 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 29 Apr 2024 18:39:00 -0400 Subject: [PATCH 1/6] if phone number is already filled in on user, do not overwrite --- src/djangooidc/backends.py | 30 +++++++++++++-- src/djangooidc/tests/test_backends.py | 53 +++++++++++++++++++++++++-- 2 files changed, 76 insertions(+), 7 deletions(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index b0e6417db..69b34d775 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -65,13 +65,35 @@ class OpenIdConnectBackend(ModelBackend): return user def update_existing_user(self, user, kwargs): - """Update other fields without overwriting first_name and last_name. - Overwrite first_name and last_name if not empty string""" + """ + Update user fields without overwriting certain fields. + Args: + user: User object to be updated. + kwargs: Dictionary containing fields to update and their new values. + + Note: + This method updates user fields while preserving the values of 'first_name', + 'last_name', and 'phone' fields, unless specific conditions are met. + + - 'phone' field will be updated if it's None or an empty string. + - 'first_name' and 'last_name' will be updated if the provided value is not empty. + """ + + # Iterate over fields to update for key, value in kwargs.items(): - # Check if the key is not first_name or last_name or value is not empty string - if key not in ["first_name", "last_name"] or value: + # Check if the field is not 'first_name', 'last_name', or 'phone', + # or if it's the 'phone' field and 'user.phone' is None or empty, + # or if it's 'first_name' or 'last_name' and the provided value is not empty + if ( + key not in ["first_name", "last_name", "phone"] + or (key == "phone" and (user.phone is None or user.phone == "")) + or (key in ["first_name", "last_name"] and value) + ): + # Update the corresponding attribute of the user object setattr(user, key, value) + + # Save the user object with the updated fields user.save() def clean_username(self, username): diff --git a/src/djangooidc/tests/test_backends.py b/src/djangooidc/tests/test_backends.py index ac7f74903..ffe26e2cb 100644 --- a/src/djangooidc/tests/test_backends.py +++ b/src/djangooidc/tests/test_backends.py @@ -50,11 +50,16 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "123456789") - def test_authenticate_with_existing_user_no_name(self): + def test_authenticate_with_existing_user_with_existing_first_last_phone(self): """Test that authenticate updates an existing user if it finds one. - For this test, given_name and family_name are not supplied""" + For this test, given_name and family_name are not supplied. + + The existing user's first and last name are not overwritten. + The existing user's phone number is not overwritten""" # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user", first_name="John", last_name="Doe") + existing_user = User.objects.create_user( + username="test_user", first_name="WillNotBe", last_name="Replaced", phone="9999999999" + ) # Remove given_name and family_name from the input, self.kwargs self.kwargs.pop("given_name", None) @@ -67,6 +72,48 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertIsInstance(user, User) self.assertEqual(user, existing_user) # The same user instance should be returned + # Verify that user fields are correctly updated + self.assertEqual(user.first_name, "WillNotBe") + self.assertEqual(user.last_name, "Replaced") + self.assertEqual(user.email, "john.doe@example.com") + self.assertEqual(user.phone, "9999999999") + + def test_authenticate_with_existing_user_with_no_phone_will_update_phone(self): + """Test that authenticate updates an existing user if it finds one. + For this test, the existing user has an empty string for phone + + The existing user's phone number is overwritten""" + # Create an existing user with the same username and with first and last names + existing_user = User.objects.create_user(username="test_user", phone="") + + # Ensure that the authenticate method updates the existing user + # and preserves existing first and last names + user = self.backend.authenticate(request=None, **self.kwargs) + self.assertIsNotNone(user) + self.assertIsInstance(user, User) + self.assertEqual(user, existing_user) # The same user instance should be returned + + # Verify that user fields are correctly updated + self.assertEqual(user.first_name, "John") + self.assertEqual(user.last_name, "Doe") + self.assertEqual(user.email, "john.doe@example.com") + self.assertEqual(user.phone, "123456789") + + def test_authenticate_with_existing_user_with_none_phone_will_update_phone(self): + """Test that authenticate updates an existing user if it finds one. + For this test, the existing user has None for phone + + The existing user's phone number is overwritten""" + # Create an existing user with the same username and with first and last names + existing_user = User.objects.create_user(username="test_user") + + # Ensure that the authenticate method updates the existing user + # and preserves existing first and last names + user = self.backend.authenticate(request=None, **self.kwargs) + self.assertIsNotNone(user) + self.assertIsInstance(user, User) + self.assertEqual(user, existing_user) # The same user instance should be returned + # Verify that user fields are correctly updated self.assertEqual(user.first_name, "John") self.assertEqual(user.last_name, "Doe") From 8b36fef5c1dad003991d37f7302e33e570e7e71f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 29 Apr 2024 19:41:51 -0400 Subject: [PATCH 2/6] If phone number is already filled in for the associated contact, do not overwrite --- src/djangooidc/backends.py | 12 +++++++++++- src/registrar/models/contact.py | 3 ++- src/registrar/tests/common.py | 2 +- src/registrar/tests/test_models.py | 24 ++++++++++++++++++++---- 4 files changed, 34 insertions(+), 7 deletions(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 69b34d775..6556d8d7a 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -8,6 +8,8 @@ from django.contrib.auth import get_user_model from django.contrib.auth.backends import ModelBackend from django.utils import timezone +from registrar.models.contact import Contact + logger = logging.getLogger(__name__) @@ -80,6 +82,14 @@ class OpenIdConnectBackend(ModelBackend): - 'first_name' and 'last_name' will be updated if the provided value is not empty. """ + contacts = Contact.objects.filter(user=user) + + if len(contacts) == 0: # no matching contact + logger.warning("Could not find a contact when one should've existed.") + + if len(contacts) > 1: # multiple matches + logger.warning("There are multiple Contacts with the same email address.") + # Iterate over fields to update for key, value in kwargs.items(): # Check if the field is not 'first_name', 'last_name', or 'phone', @@ -87,7 +97,7 @@ class OpenIdConnectBackend(ModelBackend): # or if it's 'first_name' or 'last_name' and the provided value is not empty if ( key not in ["first_name", "last_name", "phone"] - or (key == "phone" and (user.phone is None or user.phone == "")) + or (key == "phone" and not contacts[0].phone) or (key in ["first_name", "last_name"] and value) ): # Update the corresponding attribute of the user object diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index 9deb22641..185b84ded 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -102,9 +102,10 @@ class Contact(TimeStampedModel): super().save(*args, **kwargs) # Update the related User object's first_name and last_name - if self.user and (not self.user.first_name or not self.user.last_name): + if self.user and (not self.user.first_name or not self.user.last_name or not self.user.phone): self.user.first_name = self.first_name self.user.last_name = self.last_name + self.user.phone = self.phone self.user.save() def __str__(self): diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 6dd88c1c1..b895e2916 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -834,7 +834,7 @@ def completed_domain_request( ) if not investigator: investigator, _ = User.objects.get_or_create( - username="incrediblyfakeinvestigator", + username="incrediblyfakeinvestigator" + str(uuid.uuid4())[:8], first_name="Joe", last_name="Bob", is_staff=True, diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ca77d1ddf..1558ab310 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1152,12 +1152,18 @@ class TestContact(TestCase): def setUp(self): self.email_for_invalid = "intern@igorville.gov" self.invalid_user, _ = User.objects.get_or_create( - username=self.email_for_invalid, email=self.email_for_invalid, first_name="", last_name="" + username=self.email_for_invalid, + email=self.email_for_invalid, + first_name="", + last_name="", + phone="", ) self.invalid_contact, _ = Contact.objects.get_or_create(user=self.invalid_user) self.email = "mayor@igorville.gov" - self.user, _ = User.objects.get_or_create(email=self.email, first_name="Jeff", last_name="Lebowski") + self.user, _ = User.objects.get_or_create( + email=self.email, first_name="Jeff", last_name="Lebowski", phone="123456789" + ) self.contact, _ = Contact.objects.get_or_create(user=self.user) self.contact_as_ao, _ = Contact.objects.get_or_create(email="newguy@igorville.gov") @@ -1169,19 +1175,22 @@ class TestContact(TestCase): Contact.objects.all().delete() User.objects.all().delete() - def test_saving_contact_updates_user_first_last_names(self): + def test_saving_contact_updates_user_first_last_names_and_phone(self): """When a contact is updated, we propagate the changes to the linked user if it exists.""" # User and Contact are created and linked as expected. # An empty User object should create an empty contact. self.assertEqual(self.invalid_contact.first_name, "") self.assertEqual(self.invalid_contact.last_name, "") + self.assertEqual(self.invalid_contact.phone, "") self.assertEqual(self.invalid_user.first_name, "") self.assertEqual(self.invalid_user.last_name, "") + self.assertEqual(self.invalid_user.phone, "") # Manually update the contact - mimicking production (pre-existing data) self.invalid_contact.first_name = "Joey" self.invalid_contact.last_name = "Baloney" + self.invalid_contact.phone = "123456789" self.invalid_contact.save() # Refresh the user object to reflect the changes made in the database @@ -1190,20 +1199,25 @@ class TestContact(TestCase): # Updating the contact's first and last names propagate to the user self.assertEqual(self.invalid_contact.first_name, "Joey") self.assertEqual(self.invalid_contact.last_name, "Baloney") + self.assertEqual(self.invalid_contact.phone, "123456789") self.assertEqual(self.invalid_user.first_name, "Joey") self.assertEqual(self.invalid_user.last_name, "Baloney") + self.assertEqual(self.invalid_user.phone, "123456789") - def test_saving_contact_does_not_update_user_first_last_names(self): + def test_saving_contact_does_not_update_user_first_last_names_and_phone(self): """When a contact is updated, we avoid propagating the changes to the linked user if it already has a value""" # User and Contact are created and linked as expected self.assertEqual(self.contact.first_name, "Jeff") self.assertEqual(self.contact.last_name, "Lebowski") + self.assertEqual(self.contact.phone, "123456789") self.assertEqual(self.user.first_name, "Jeff") self.assertEqual(self.user.last_name, "Lebowski") + self.assertEqual(self.user.phone, "123456789") self.contact.first_name = "Joey" self.contact.last_name = "Baloney" + self.contact.phone = "987654321" self.contact.save() # Refresh the user object to reflect the changes made in the database @@ -1212,8 +1226,10 @@ class TestContact(TestCase): # Updating the contact's first and last names propagate to the user self.assertEqual(self.contact.first_name, "Joey") self.assertEqual(self.contact.last_name, "Baloney") + self.assertEqual(self.contact.phone, "987654321") self.assertEqual(self.user.first_name, "Jeff") self.assertEqual(self.user.last_name, "Lebowski") + self.assertEqual(self.user.phone, "123456789") def test_saving_contact_does_not_update_user_email(self): """When a contact's email is updated, the change is not propagated to the user.""" From c6b6c17540ff91a8b37d15e92454790806a85038 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 13:28:03 -0400 Subject: [PATCH 3/6] revise logic a bit to ensure user and contact do not overwrite each other --- src/djangooidc/backends.py | 21 +++--------- src/djangooidc/tests/test_backends.py | 49 +++------------------------ src/registrar/models/contact.py | 23 +++++++++---- 3 files changed, 26 insertions(+), 67 deletions(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 6556d8d7a..9b500cd3f 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -8,8 +8,6 @@ from django.contrib.auth import get_user_model from django.contrib.auth.backends import ModelBackend from django.utils import timezone -from registrar.models.contact import Contact - logger = logging.getLogger(__name__) @@ -79,26 +77,15 @@ class OpenIdConnectBackend(ModelBackend): 'last_name', and 'phone' fields, unless specific conditions are met. - 'phone' field will be updated if it's None or an empty string. - - 'first_name' and 'last_name' will be updated if the provided value is not empty. + - 'first_name', 'last_name' or 'phone' will be updated if the provided value is not empty. """ - contacts = Contact.objects.filter(user=user) - - if len(contacts) == 0: # no matching contact - logger.warning("Could not find a contact when one should've existed.") - - if len(contacts) > 1: # multiple matches - logger.warning("There are multiple Contacts with the same email address.") - # Iterate over fields to update for key, value in kwargs.items(): # Check if the field is not 'first_name', 'last_name', or 'phone', - # or if it's the 'phone' field and 'user.phone' is None or empty, - # or if it's 'first_name' or 'last_name' and the provided value is not empty - if ( - key not in ["first_name", "last_name", "phone"] - or (key == "phone" and not contacts[0].phone) - or (key in ["first_name", "last_name"] and value) + # or if it's 'first_name' or 'last_name' or 'phone' and the provided value is not empty + if key not in ["first_name", "last_name", "phone"] or ( + key in ["first_name", "last_name", "phone"] and value ): # Update the corresponding attribute of the user object setattr(user, key, value) diff --git a/src/djangooidc/tests/test_backends.py b/src/djangooidc/tests/test_backends.py index ffe26e2cb..c15106fa9 100644 --- a/src/djangooidc/tests/test_backends.py +++ b/src/djangooidc/tests/test_backends.py @@ -64,6 +64,7 @@ class OpenIdConnectBackendTestCase(TestCase): # Remove given_name and family_name from the input, self.kwargs self.kwargs.pop("given_name", None) self.kwargs.pop("family_name", None) + self.kwargs.pop("phone", None) # Ensure that the authenticate method updates the existing user # and preserves existing first and last names @@ -78,53 +79,13 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "9999999999") - def test_authenticate_with_existing_user_with_no_phone_will_update_phone(self): - """Test that authenticate updates an existing user if it finds one. - For this test, the existing user has an empty string for phone - - The existing user's phone number is overwritten""" - # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user", phone="") - - # Ensure that the authenticate method updates the existing user - # and preserves existing first and last names - user = self.backend.authenticate(request=None, **self.kwargs) - self.assertIsNotNone(user) - self.assertIsInstance(user, User) - self.assertEqual(user, existing_user) # The same user instance should be returned - - # Verify that user fields are correctly updated - self.assertEqual(user.first_name, "John") - self.assertEqual(user.last_name, "Doe") - self.assertEqual(user.email, "john.doe@example.com") - self.assertEqual(user.phone, "123456789") - - def test_authenticate_with_existing_user_with_none_phone_will_update_phone(self): - """Test that authenticate updates an existing user if it finds one. - For this test, the existing user has None for phone - - The existing user's phone number is overwritten""" - # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user") - - # Ensure that the authenticate method updates the existing user - # and preserves existing first and last names - user = self.backend.authenticate(request=None, **self.kwargs) - self.assertIsNotNone(user) - self.assertIsInstance(user, User) - self.assertEqual(user, existing_user) # The same user instance should be returned - - # Verify that user fields are correctly updated - self.assertEqual(user.first_name, "John") - self.assertEqual(user.last_name, "Doe") - self.assertEqual(user.email, "john.doe@example.com") - self.assertEqual(user.phone, "123456789") - - def test_authenticate_with_existing_user_different_name(self): + def test_authenticate_with_existing_user_different_name_phone(self): """Test that authenticate updates an existing user if it finds one. For this test, given_name and family_name are supplied and overwrite""" # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user", first_name="WillBe", last_name="Replaced") + existing_user = User.objects.create_user( + username="test_user", first_name="WillBe", last_name="Replaced", phone="987654321" + ) # Ensure that the authenticate method updates the existing user # and preserves existing first and last names diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index 185b84ded..3ebd8bc3e 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -101,12 +101,23 @@ class Contact(TimeStampedModel): # Call the parent class's save method to perform the actual save super().save(*args, **kwargs) - # Update the related User object's first_name and last_name - if self.user and (not self.user.first_name or not self.user.last_name or not self.user.phone): - self.user.first_name = self.first_name - self.user.last_name = self.last_name - self.user.phone = self.phone - self.user.save() + if self.user: + updated = False + + # Update first name and last name if necessary + if not self.user.first_name or not self.user.last_name: + self.user.first_name = self.first_name + self.user.last_name = self.last_name + updated = True + + # Update phone if necessary + if not self.user.phone: + self.user.phone = self.phone + updated = True + + # Save user if any updates were made + if updated: + self.user.save() def __str__(self): if self.first_name or self.last_name: From 7160efa51536f3ecd0f0a08c9fb597f66774d1c2 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 13:30:26 -0400 Subject: [PATCH 4/6] comment --- src/djangooidc/backends.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 9b500cd3f..81e782c7b 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -76,7 +76,6 @@ class OpenIdConnectBackend(ModelBackend): This method updates user fields while preserving the values of 'first_name', 'last_name', and 'phone' fields, unless specific conditions are met. - - 'phone' field will be updated if it's None or an empty string. - 'first_name', 'last_name' or 'phone' will be updated if the provided value is not empty. """ From c0a967c4ea4a72f42746c2f8bb4a4aeabb0660e4 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 1 May 2024 14:30:08 -0400 Subject: [PATCH 5/6] cleanup --- src/djangooidc/backends.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 81e782c7b..df086059e 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -79,12 +79,14 @@ class OpenIdConnectBackend(ModelBackend): - 'first_name', 'last_name' or 'phone' will be updated if the provided value is not empty. """ + fields_to_check = ["first_name", "last_name", "phone"] + # Iterate over fields to update for key, value in kwargs.items(): # Check if the field is not 'first_name', 'last_name', or 'phone', # or if it's 'first_name' or 'last_name' or 'phone' and the provided value is not empty - if key not in ["first_name", "last_name", "phone"] or ( - key in ["first_name", "last_name", "phone"] and value + if key not in fields_to_check or ( + key in fields_to_check and value ): # Update the corresponding attribute of the user object setattr(user, key, value) From 550e6df8b129833d1053e45f8ff103baf99c0dad Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 2 May 2024 14:49:40 -0400 Subject: [PATCH 6/6] lint --- src/djangooidc/backends.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index df086059e..41e442f2d 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -85,9 +85,7 @@ class OpenIdConnectBackend(ModelBackend): for key, value in kwargs.items(): # Check if the field is not 'first_name', 'last_name', or 'phone', # or if it's 'first_name' or 'last_name' or 'phone' and the provided value is not empty - if key not in fields_to_check or ( - key in fields_to_check and value - ): + if key not in fields_to_check or (key in fields_to_check and value): # Update the corresponding attribute of the user object setattr(user, key, value)