From 6361369953c1a4cf2b0d92ed3eb18876a943fae1 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 2 Jul 2024 15:10:49 -0400 Subject: [PATCH 1/8] removed user-contact connection --- docs/developer/README.md | 43 -------- src/registrar/apps.py | 9 -- src/registrar/forms/__init__.py | 2 +- src/registrar/forms/domain.py | 59 ++++++++++- src/registrar/forms/user_profile.py | 4 +- .../management/commands/import_tables.py | 7 -- src/registrar/models/contact.py | 7 -- src/registrar/models/user.py | 23 ++-- src/registrar/signals.py | 59 ----------- .../admin/includes/contact_detail_list.html | 27 ++--- src/registrar/templates/domain_detail.html | 2 +- .../includes/finish_profile_form.html | 4 +- .../includes/profile_information.html | 6 +- src/registrar/tests/common.py | 4 + src/registrar/tests/test_admin.py | 47 +++----- src/registrar/tests/test_models.py | 98 +---------------- src/registrar/tests/test_signals.py | 100 ------------------ src/registrar/tests/test_views.py | 12 +-- src/registrar/tests/test_views_domain.py | 4 +- src/registrar/views/domain.py | 6 +- src/registrar/views/user_profile.py | 16 ++- .../views/utility/permission_views.py | 6 +- 22 files changed, 128 insertions(+), 417 deletions(-) delete mode 100644 src/registrar/signals.py delete mode 100644 src/registrar/tests/test_signals.py diff --git a/docs/developer/README.md b/docs/developer/README.md index 72f6b9f208..f63f019388 100644 --- a/docs/developer/README.md +++ b/docs/developer/README.md @@ -353,49 +353,6 @@ cf env getgov-{app name} Then, copy the variables under the section labled `s3`. -## Signals -The application uses [Django signals](https://docs.djangoproject.com/en/5.0/topics/signals/). In particular, it uses a subset of prebuilt signals called [model signals](https://docs.djangoproject.com/en/5.0/ref/signals/#module-django.db.models.signals). - -Per Django, signals "[...allow certain senders to notify a set of receivers that some action has taken place.](https://docs.djangoproject.com/en/5.0/topics/signals/#module-django.dispatch)" - -In other words, signals are a mechanism that allows different parts of an application to communicate with each other by sending and receiving notifications when events occur. When an event occurs (such as creating, updating, or deleting a record), signals can automatically trigger specific actions in response. This allows different parts of an application to stay synchronized without tightly coupling the component. - -### Rules of use -When using signals, try to adhere to these guidelines: -1. Don't use signals when you can use another method, such as an override of `save()` or `__init__`. -2. Document its usage in this readme (or another centralized location), as well as briefly on the underlying class it is associated with. For instance, since the `handle_profile` directly affects the class `Contact`, the class description notes this and links to [signals.py](../../src/registrar/signals.py). -3. Where possible, avoid chaining signals together (i.e. a signal that calls a signal). If this has to be done, clearly document the flow. -4. Minimize logic complexity within the signal as much as possible. - -### When should you use signals? -Generally, you would use signals when you want an event to be synchronized across multiple areas of code at once (such as with two models or more models at once) in a way that would otherwise be difficult to achieve by overriding functions. - -However, in most scenarios, if you can get away with avoiding signals - you should. The reasoning for this is that [signals give the appearance of loose coupling, but they can quickly lead to code that is hard to understand, adjust and debug](https://docs.djangoproject.com/en/5.0/topics/signals/#module-django.dispatch). - -Consider using signals when: -1. Synchronizing events across multiple models or areas of code. -2. Performing logic before or after saving a model to the database (when otherwise difficult through `save()`). -3. Encountering an import loop when overriding functions such as `save()`. -4. You are otherwise unable to achieve the intended behavior by overrides or other means. -5. (Rare) Offloading tasks when multi-threading. - -For the vast majority of use cases, the [pre_save](https://docs.djangoproject.com/en/5.0/ref/signals/#pre-save) and [post_save](https://docs.djangoproject.com/en/5.0/ref/signals/#post-save) signals are sufficient in terms of model-to-model management. - -### Where should you use them? -This project compiles signals in a unified location to maintain readability. If you are adding a signal or otherwise utilizing one, you should always define them in [signals.py](../../src/registrar/signals.py). Except under rare circumstances, this should be adhered to for the reasons mentioned above. - -### How are we currently using signals? -At the time of writing, we currently only use signals for the Contact and User objects when synchronizing data returned from Login.gov. This is because the `Contact` object holds information that the user specified in our system, whereas the `User` object holds information that was specified in Login.gov. - -To keep our signal usage coherent and well-documented, add to this document when a new function is added for ease of reference and use. - -#### handle_profile -This function is triggered by the post_save event on the User model, designed to manage the synchronization between User and Contact entities. It operates under the following conditions: - -1. For New Users: Upon the creation of a new user, it checks for an existing `Contact` by email. If no matching contact is found, it creates a new Contact using the user's details from Login.gov. If a matching contact is found, it associates this contact with the user. In cases where multiple contacts with the same email exist, it logs a warning and associates the first contact found. - -2. For Existing Users: For users logging in subsequent times, the function ensures that any updates from Login.gov are applied to the associated User record. However, it does not alter any existing Contact records. - ## Disable email sending (toggling the disable_email_sending flag) 1. On the app, navigate to `\admin`. 2. Under models, click `Waffle flags`. diff --git a/src/registrar/apps.py b/src/registrar/apps.py index fcb5c17fd9..b5952208bc 100644 --- a/src/registrar/apps.py +++ b/src/registrar/apps.py @@ -5,12 +5,3 @@ class RegistrarConfig(AppConfig): """Configure signal handling for our registrar Django application.""" name = "registrar" - - def ready(self): - """Runs when all Django applications have been loaded. - - We use it here to load signals that connect related models. - """ - # noqa here because we are importing something to make the signals - # get registered, but not using what we import - from . import signals # noqa diff --git a/src/registrar/forms/__init__.py b/src/registrar/forms/__init__.py index be3b634f6f..8f4f8ea143 100644 --- a/src/registrar/forms/__init__.py +++ b/src/registrar/forms/__init__.py @@ -4,7 +4,7 @@ NameserverFormset, DomainSecurityEmailForm, DomainOrgNameAddressForm, - ContactForm, + UserForm, AuthorizingOfficialContactForm, DomainDnssecForm, DomainDsdataFormset, diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 0e9fbb693a..6f6c9077e0 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -16,7 +16,7 @@ SecurityEmailErrorCodes, ) -from ..models import Contact, DomainInformation, Domain +from ..models import Contact, DomainInformation, Domain, User from .common import ( ALGORITHM_CHOICES, DIGEST_TYPE_CHOICES, @@ -203,6 +203,63 @@ def clean(self): ) +class UserForm(forms.ModelForm): + """Form for updating contacts.""" + + email = forms.EmailField(max_length=None) + + class Meta: + model = User + fields = ["first_name", "middle_name", "last_name", "title", "email", "phone"] + widgets = { + "first_name": forms.TextInput, + "middle_name": forms.TextInput, + "last_name": forms.TextInput, + "title": forms.TextInput, + "email": forms.EmailInput, + "phone": RegionalPhoneNumberWidget, + } + + # the database fields have blank=True so ModelForm doesn't create + # required fields by default. Use this list in __init__ to mark each + # of these fields as required + required = ["first_name", "last_name", "title", "email", "phone"] + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + # take off maxlength attribute for the phone number field + # which interferes with out input_with_errors template tag + self.fields["phone"].widget.attrs.pop("maxlength", None) + + # Define a custom validator for the email field with a custom error message + email_max_length_validator = MaxLengthValidator(320, message="Response must be less than 320 characters.") + self.fields["email"].validators.append(email_max_length_validator) + + for field_name in self.required: + self.fields[field_name].required = True + + # Set custom form label + self.fields["middle_name"].label = "Middle name (optional)" + + # Set custom error messages + self.fields["first_name"].error_messages = {"required": "Enter your first name / given name."} + self.fields["last_name"].error_messages = {"required": "Enter your last name / family name."} + self.fields["title"].error_messages = { + "required": "Enter your title or role in your organization (e.g., Chief Information Officer)" + } + self.fields["email"].error_messages = { + "required": "Enter your email address in the required format, like name@example.com." + } + self.fields["phone"].error_messages["required"] = "Enter your phone number." + self.domainInfo = None + + def set_domain_info(self, domainInfo): + """Set the domain information for the form. + The form instance is associated with the contact itself. In order to access the associated + domain information object, this needs to be set in the form by the view.""" + self.domainInfo = domainInfo + + class ContactForm(forms.ModelForm): """Form for updating contacts.""" diff --git a/src/registrar/forms/user_profile.py b/src/registrar/forms/user_profile.py index 682e1a5df5..bfdcd0da81 100644 --- a/src/registrar/forms/user_profile.py +++ b/src/registrar/forms/user_profile.py @@ -1,6 +1,6 @@ from django import forms -from registrar.models.contact import Contact +from registrar.models.user import User from django.core.validators import MaxLengthValidator from phonenumber_field.widgets import RegionalPhoneNumberWidget @@ -13,7 +13,7 @@ class UserProfileForm(forms.ModelForm): redirect = forms.CharField(widget=forms.HiddenInput(), required=False) class Meta: - model = Contact + model = User fields = ["first_name", "middle_name", "last_name", "title", "email", "phone"] widgets = { "first_name": forms.TextInput, diff --git a/src/registrar/management/commands/import_tables.py b/src/registrar/management/commands/import_tables.py index cb78e13bda..62562e7f7d 100644 --- a/src/registrar/management/commands/import_tables.py +++ b/src/registrar/management/commands/import_tables.py @@ -65,13 +65,6 @@ def import_table(self, table_name): resourcename = f"{table_name}Resource" - # if table_name is Contact, clean the table first - # User table is loaded before Contact, and signals create - # rows in Contact table which break the import, so need - # to be cleaned again before running import on Contact table - if table_name == "Contact": - self.clean_table(table_name) - # Define the directory and the pattern for csv filenames tmp_dir = "tmp" pattern = f"{table_name}_" diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index f94938dd1e..f7bae3491a 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -8,13 +8,6 @@ class Contact(TimeStampedModel): """ Contact information follows a similar pattern for each contact. - - This model uses signals [as defined in [signals.py](../../src/registrar/signals.py)]. - When a new user is created through Login.gov, a contact object will be created and - associated on the `user` field. - - If the `user` object already exists, the underlying user object - will be updated if any updates are made to it through Login.gov. """ class Meta: diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index bb02766076..87b7799d39 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -23,10 +23,6 @@ class User(AbstractUser): A custom user model that performs identically to the default user model but can be customized later. - This model uses signals [as defined in [signals.py](../../src/registrar/signals.py)]. - When a new user is created through Login.gov, a contact object will be created and - associated on the contacts `user` field. - If the `user` object already exists, said user object will be updated if any updates are made to it through Login.gov. """ @@ -113,15 +109,11 @@ def finished_setup(self): Tracks if the user finished their profile setup or not. This is so we can globally enforce that new users provide additional account information before proceeding. """ - - # Change this to self once the user and contact objects are merged. - # For now, since they are linked, lets test on the underlying contact object. - user_info = self.contact # noqa user_values = [ - user_info.first_name, - user_info.last_name, - user_info.title, - user_info.phone, + self.first_name, + self.last_name, + self.title, + self.phone, ] return None not in user_values @@ -169,8 +161,13 @@ def get_ineligible_requests_count(self): """Return count of ineligible requests""" return self.domain_requests_created.filter(status=DomainRequest.DomainRequestStatus.INELIGIBLE).count() + def get_formatted_name(self): + """Returns the contact's name in Western order.""" + names = [n for n in [self.first_name, self.middle_name, self.last_name] if n] + return " ".join(names) if names else "Unknown" + def has_contact_info(self): - return bool(self.contact.title or self.contact.email or self.contact.phone) + return bool(self.title or self.email or self.phone) @classmethod def needs_identity_verification(cls, email, uuid): diff --git a/src/registrar/signals.py b/src/registrar/signals.py deleted file mode 100644 index bc0480b2a6..0000000000 --- a/src/registrar/signals.py +++ /dev/null @@ -1,59 +0,0 @@ -import logging - -from django.db.models.signals import post_save -from django.dispatch import receiver - -from .models import User, Contact - - -logger = logging.getLogger(__name__) - - -@receiver(post_save, sender=User) -def handle_profile(sender, instance, **kwargs): - """Method for when a User is saved. - - 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. - """ - - first_name = getattr(instance, "first_name", "") - middle_name = getattr(instance, "middle_name", "") - last_name = getattr(instance, "last_name", "") - email = getattr(instance, "email", "") - phone = getattr(instance, "phone", "") - title = getattr(instance, "title", "") - - is_new_user = kwargs.get("created", False) - - if is_new_user: - contacts = Contact.objects.filter(email=email) - else: - contacts = Contact.objects.filter(user=instance) - - if len(contacts) == 0: # no matching contact - Contact.objects.create( - user=instance, - first_name=first_name, - middle_name=middle_name, - last_name=last_name, - email=email, - phone=phone, - title=title, - ) - - 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}." - ) diff --git a/src/registrar/templates/django/admin/includes/contact_detail_list.html b/src/registrar/templates/django/admin/includes/contact_detail_list.html index 3b49e62a4a..2ee490d760 100644 --- a/src/registrar/templates/django/admin/includes/contact_detail_list.html +++ b/src/registrar/templates/django/admin/includes/contact_detail_list.html @@ -12,37 +12,24 @@ {% if user.has_contact_info %} {# Title #} - {% if user.title or user.contact.title %} - {% if user.contact.title %} - {{ user.contact.title }} - {% else %} - {{ user.title }} - {% endif %} + {% if user.title %} + {{ user.title }}
{% else %} None
{% endif %} {# Email #} - {% if user.email or user.contact.email %} - {% if user.contact.email %} - {{ user.contact.email }} - {% include "admin/input_with_clipboard.html" with field=user invisible_input_field=True %} - {% else %} - {{ user.email }} - {% include "admin/input_with_clipboard.html" with field=user invisible_input_field=True %} - {% endif %} + {% if user.email %} + {{ user.email }} + {% include "admin/input_with_clipboard.html" with field=user invisible_input_field=True %}
{% else %} None
{% endif %} {# Phone #} - {% if user.phone or user.contact.phone %} - {% if user.contact.phone %} - {{ user.contact.phone }} - {% else %} - {{ user.phone }} - {% endif %} + {% if user.phone %} + {{ user.phone }}
{% else %} None
diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 815df49429..d201831cc7 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -62,7 +62,7 @@

DNS name servers

{# Conditionally display profile #} {% if not has_profile_feature_flag %} {% url 'domain-your-contact-information' pk=domain.id as url %} - {% include "includes/summary_item.html" with title='Your contact information' value=request.user.contact contact='true' edit_link=url editable=domain.is_editable %} + {% include "includes/summary_item.html" with title='Your contact information' value=request.user contact='true' edit_link=url editable=domain.is_editable %} {% endif %} {% url 'domain-security-email' pk=domain.id as url %} diff --git a/src/registrar/templates/includes/finish_profile_form.html b/src/registrar/templates/includes/finish_profile_form.html index d43ff812c7..88f7a73af2 100644 --- a/src/registrar/templates/includes/finish_profile_form.html +++ b/src/registrar/templates/includes/finish_profile_form.html @@ -77,11 +77,11 @@

What contact information should we use to reach you?

- {% if user_finished_setup and going_to_specific_page %} - {% endif %} diff --git a/src/registrar/templates/includes/profile_information.html b/src/registrar/templates/includes/profile_information.html index 2922fd3f73..3e7c827f1b 100644 --- a/src/registrar/templates/includes/profile_information.html +++ b/src/registrar/templates/includes/profile_information.html @@ -13,10 +13,10 @@

    -
  • Full name: {{ user.contact.get_formatted_name }}
    • +
  • Full name: {{ user.get_formatted_name }}
  • Organization email: {{ user.email }}
    • -
  • Title or role in your organization: {{ user.contact.title }}
    • -
  • Phone: {{ user.contact.phone.as_national }}
    • +
  • Title or role in your organization: {{ user.title }}
    • +
  • Phone: {{ user.phone.as_national }}
diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 017964299a..a8bdfec297 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -810,6 +810,8 @@ def create_superuser(): user = User.objects.create_user( username="superuser", email="admin@example.com", + first_name="first", + last_name="last", is_staff=True, password=p, ) @@ -826,6 +828,8 @@ def create_user(): user = User.objects.create_user( username="staffuser", email="staff@example.com", + first_name="first", + last_name="last", is_staff=True, password=p, ) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 07e97d1599..ff68e5a7c4 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -242,15 +242,11 @@ def test_contact_fields_on_domain_change_form_have_detail_table(self): username="MrMeoward", first_name="Meoward", last_name="Jones", + email="meoward.jones@igorville.gov", + phone="(555) 123 12345", + title="Treat inspector", ) - # Due to the relation between User <==> Contact, - # the underlying contact has to be modified this way. - _creator.contact.email = "meoward.jones@igorville.gov" - _creator.contact.phone = "(555) 123 12345" - _creator.contact.title = "Treat inspector" - _creator.contact.save() - # Create a fake domain request domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) domain_request.approve() @@ -2067,15 +2063,11 @@ def test_contact_fields_have_detail_table(self): username="MrMeoward", first_name="Meoward", last_name="Jones", + email="meoward.jones@igorville.gov", + phone="(555) 123 12345", + title="Treat inspector", ) - # Due to the relation between User <==> Contact, - # the underlying contact has to be modified this way. - _creator.contact.email = "meoward.jones@igorville.gov" - _creator.contact.phone = "(555) 123 12345" - _creator.contact.title = "Treat inspector" - _creator.contact.save() - # Create a fake domain request domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) @@ -2092,11 +2084,11 @@ def test_contact_fields_have_detail_table(self): # == Check for the creator == # - # Check for the right title, email, and phone number in the response. + # Check for the right title and phone number in the response. + # Email will appear more than once expected_creator_fields = [ # Field, expected value ("title", "Treat inspector"), - ("email", "meoward.jones@igorville.gov"), ("phone", "(555) 123 12345"), ] self.test_helper.assert_response_contains_distinct_values(response, expected_creator_fields) @@ -3103,15 +3095,11 @@ def test_contact_fields_have_detail_table(self): username="MrMeoward", first_name="Meoward", last_name="Jones", + email="meoward.jones@igorville.gov", + phone="(555) 123 12345", + title="Treat inspector", ) - # Due to the relation between User <==> Contact, - # the underlying contact has to be modified this way. - _creator.contact.email = "meoward.jones@igorville.gov" - _creator.contact.phone = "(555) 123 12345" - _creator.contact.title = "Treat inspector" - _creator.contact.save() - # Create a fake domain request domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW, user=_creator) domain_request.approve() @@ -3133,13 +3121,12 @@ def test_contact_fields_have_detail_table(self): # == Check for the creator == # - # Check for the right title, email, and phone number in the response. + # Check for the right title and phone number in the response. # We only need to check for the end tag # (Otherwise this test will fail if we change classes, etc) expected_creator_fields = [ # Field, expected value ("title", "Treat inspector"), - ("email", "meoward.jones@igorville.gov"), ("phone", "(555) 123 12345"), ] self.test_helper.assert_response_contains_distinct_values(response, expected_creator_fields) @@ -4067,8 +4054,8 @@ def test_readonly_when_restricted_superuser(self): self.assertEqual(readonly_fields, expected_fields) def test_change_view_for_joined_contact_five_or_less(self): - """Create a contact, join it to 4 domain requests. The 5th join will be a user. - Assert that the warning on the contact form lists 5 joins.""" + """Create a contact, join it to 4 domain requests. 5th join is user. + Assert that the warning on the contact form lists 4 joins.""" with less_console_noise(): self.client.force_login(self.superuser) @@ -4099,17 +4086,17 @@ def test_change_view_for_joined_contact_five_or_less(self): "
  • Joined to DomainRequest: city4.gov
    • " "
  • Joined to User: staff@example.com
    • " + f"user/{self.staffuser.pk}/change/'>first last staff@example.com" "", ) def test_change_view_for_joined_contact_five_or_more(self): - """Create a contact, join it to 5 domain requests. The 6th join will be a user. + """Create a contact, join it to 5 domain requests. 6th join is user. Assert that the warning on the contact form lists 5 joins and a '1 more' ellispsis.""" with less_console_noise(): self.client.force_login(self.superuser) # Create an instance of the model - # join it to 5 domain requests. The 6th join will be a user. + # join it to 6 domain requests. contact, _ = Contact.objects.get_or_create(user=self.staffuser) domain_request1 = completed_domain_request(submitter=contact, name="city1.gov") domain_request2 = completed_domain_request(submitter=contact, name="city2.gov") diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 37fdc53546..d7a8dc6f7c 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1209,24 +1209,14 @@ def test_has_contact_info(self): # test with a user with contact info defined self.assertTrue(self.user.has_contact_info()) # test with a user without contact info defined - self.user.contact.title = None - self.user.contact.email = None - self.user.contact.phone = None + self.user.title = None + self.user.email = None + self.user.phone = None self.assertFalse(self.user.has_contact_info()) 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="", - 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", phone="123456789" @@ -1242,87 +1232,6 @@ def tearDown(self): Contact.objects.all().delete() User.objects.all().delete() - 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 - self.invalid_user.refresh_from_db() - - # 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_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 - self.user.refresh_from_db() - - # 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.""" - self.contact.email = "joey.baloney@diaperville.com" - self.contact.save() - - # Refresh the user object to reflect the changes made in the database - self.user.refresh_from_db() - - # Updating the contact's email does not propagate - self.assertEqual(self.contact.email, "joey.baloney@diaperville.com") - self.assertEqual(self.user.email, "mayor@igorville.gov") - - def test_saving_contact_does_not_update_user_email_when_none(self): - """When a contact's email is updated, and the first/last name is none, - the change is not propagated to the user.""" - self.invalid_contact.email = "joey.baloney@diaperville.com" - self.invalid_contact.save() - - # Refresh the user object to reflect the changes made in the database - self.invalid_user.refresh_from_db() - - # Updating the contact's email does not propagate - self.assertEqual(self.invalid_contact.email, "joey.baloney@diaperville.com") - self.assertEqual(self.invalid_user.email, "intern@igorville.gov") - def test_has_more_than_one_join(self): """Test the Contact model method, has_more_than_one_join""" # test for a contact which has one user defined @@ -1334,6 +1243,7 @@ def test_has_more_than_one_join(self): def test_has_contact_info(self): """Test that has_contact_info properly returns""" + self.contact.title = "Title" # test with a contact with contact info defined self.assertTrue(self.contact.has_contact_info()) # test with a contact without contact info defined diff --git a/src/registrar/tests/test_signals.py b/src/registrar/tests/test_signals.py deleted file mode 100644 index e796bd12ab..0000000000 --- a/src/registrar/tests/test_signals.py +++ /dev/null @@ -1,100 +0,0 @@ -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.assertEqual(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.assertEqual(actual.first_name, self.first_name) - self.assertEqual(actual.last_name, self.last_name) - self.assertEqual(actual.email, self.email) - self.assertEqual(actual.phone, self.phone) - - def test_user_created_with_matching_contact(self): - """Expect 1 Contact associated, but with no data copied from User.""" - self.assertEqual(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.assertEqual(actual.first_name, self.preferred_first_name) - self.assertEqual(actual.last_name, self.preferred_last_name) - self.assertEqual(actual.email, self.email) - self.assertEqual(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.assertEqual(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.assertEqual(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.assertEqual(actual.first_name, self.first_name) - self.assertEqual(actual.last_name, self.last_name) - self.assertEqual(actual.email, self.email) - self.assertEqual(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.assertEqual(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.assertEqual(actual.first_name, self.first_name) - self.assertEqual(actual.last_name, self.last_name) - self.assertEqual(actual.email, self.email) - self.assertEqual(actual.phone, self.phone) diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 2ac0caf2c9..54581ad656 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -57,12 +57,10 @@ def setUp(self): last_name = "Last" email = "info@example.com" phone = "8003111234" + title = "test title" self.user = get_user_model().objects.create( - username=username, first_name=first_name, last_name=last_name, email=email, phone=phone + username=username, first_name=first_name, last_name=last_name, title=title, email=email, phone=phone ) - title = "test title" - self.user.contact.title = title - self.user.contact.save() username_regular_incomplete = "test_regular_user_incomplete" username_other_incomplete = "test_other_user_incomplete" @@ -560,7 +558,7 @@ def test_new_user_with_profile_feature_on(self): self.assertContains(finish_setup_page, "Enter your phone number.") # Check for the name of the save button - self.assertContains(finish_setup_page, "contact_setup_save_button") + self.assertContains(finish_setup_page, "user_setup_save_button") # Add a phone number finish_setup_form = finish_setup_page.form @@ -598,7 +596,7 @@ def test_new_user_goes_to_domain_request_with_profile_feature_on(self): self.assertContains(finish_setup_page, "Enter your phone number.") # Check for the name of the save button - self.assertContains(finish_setup_page, "contact_setup_save_button") + self.assertContains(finish_setup_page, "user_setup_save_button") # Add a phone number finish_setup_form = finish_setup_page.form @@ -613,7 +611,7 @@ def test_new_user_goes_to_domain_request_with_profile_feature_on(self): # Submit the form using the specific submit button to execute the redirect completed_setup_page = self._submit_form_webtest( - finish_setup_form, follow=True, name="contact_setup_submit_button" + finish_setup_form, follow=True, name="user_setup_submit_button" ) self.assertEqual(completed_setup_page.status_code, 200) diff --git a/src/registrar/tests/test_views_domain.py b/src/registrar/tests/test_views_domain.py index c3bcb02dbd..f62605d696 100644 --- a/src/registrar/tests/test_views_domain.py +++ b/src/registrar/tests/test_views_domain.py @@ -1478,8 +1478,8 @@ def test_domain_your_contact_information(self): def test_domain_your_contact_information_content(self): """Logged-in user's contact information appears on the page.""" - self.user.contact.first_name = "Testy" - self.user.contact.save() + self.user.first_name = "Testy" + self.user.save() page = self.app.get(reverse("domain-your-contact-information", kwargs={"pk": self.domain.id})) self.assertContains(page, "Testy") diff --git a/src/registrar/views/domain.py b/src/registrar/views/domain.py index b7ce06cce2..d6436b058d 100644 --- a/src/registrar/views/domain.py +++ b/src/registrar/views/domain.py @@ -40,7 +40,7 @@ from registrar.views.utility.permission_views import UserDomainRolePermissionDeleteView from ..forms import ( - ContactForm, + UserForm, AuthorizingOfficialContactForm, DomainOrgNameAddressForm, DomainAddUserForm, @@ -573,7 +573,7 @@ class DomainYourContactInformationView(DomainFormBaseView): """Domain your contact information editing view.""" template_name = "domain_your_contact_information.html" - form_class = ContactForm + form_class = UserForm @waffle_flag("!profile_feature") # type: ignore def dispatch(self, request, *args, **kwargs): # type: ignore @@ -582,7 +582,7 @@ def dispatch(self, request, *args, **kwargs): # type: ignore def get_form_kwargs(self, *args, **kwargs): """Add domain_info.submitter instance to make a bound form.""" form_kwargs = super().get_form_kwargs(*args, **kwargs) - form_kwargs["instance"] = self.request.user.contact + form_kwargs["instance"] = self.request.user return form_kwargs def get_success_url(self): diff --git a/src/registrar/views/user_profile.py b/src/registrar/views/user_profile.py index 3f9aeb79f9..e378b91808 100644 --- a/src/registrar/views/user_profile.py +++ b/src/registrar/views/user_profile.py @@ -9,9 +9,6 @@ from django.views.generic.edit import FormMixin from registrar.forms.user_profile import UserProfileForm, FinishSetupProfileForm from django.urls import NoReverseMatch, reverse -from registrar.models import ( - Contact, -) from registrar.models.user import User from registrar.models.utility.generic_helper import replace_url_queryparams from registrar.views.utility.permission_views import UserProfilePermissionView @@ -25,7 +22,7 @@ class UserProfileView(UserProfilePermissionView, FormMixin): Base View for the User Profile. Handles getting and setting the User Profile """ - model = Contact + model = User template_name = "profile.html" form_class = UserProfileForm base_view_name = "user-profile" @@ -57,6 +54,7 @@ def dispatch(self, request, *args, **kwargs): # type: ignore def get_context_data(self, **kwargs): """Extend get_context_data to include has_profile_feature_flag""" context = super().get_context_data(**kwargs) + logger.info("UserProfileView::get_context_data") # This is a django waffle flag which toggles features based off of the "flag" table context["has_profile_feature_flag"] = flag_is_active(self.request, "profile_feature") @@ -123,9 +121,7 @@ def form_valid(self, form): def get_object(self, queryset=None): """Override get_object to return the logged-in user's contact""" self.user = self.request.user # get the logged in user - if hasattr(self.user, "contact"): # Check if the user has a contact instance - return self.user.contact - return None + return self.user class FinishProfileSetupView(UserProfileView): @@ -134,7 +130,7 @@ class FinishProfileSetupView(UserProfileView): template_name = "finish_profile_setup.html" form_class = FinishSetupProfileForm - model = Contact + model = User base_view_name = "finish-user-profile-setup" @@ -160,11 +156,11 @@ def post(self, request, *args, **kwargs): # Get the current form and validate it if form.is_valid(): self.redirect_page = False - if "contact_setup_save_button" in request.POST: + if "user_setup_save_button" in request.POST: # Logic for when the 'Save' button is clicked, which indicates # user should stay on this page self.redirect_page = False - elif "contact_setup_submit_button" in request.POST: + elif "user_setup_submit_button" in request.POST: # Logic for when the other button is clicked, which indicates # the user should be taken to the redirect page self.redirect_page = True diff --git a/src/registrar/views/utility/permission_views.py b/src/registrar/views/utility/permission_views.py index d35647af2f..db727c26ea 100644 --- a/src/registrar/views/utility/permission_views.py +++ b/src/registrar/views/utility/permission_views.py @@ -4,7 +4,7 @@ from django.views.generic import DetailView, DeleteView, TemplateView from registrar.models import Domain, DomainRequest, DomainInvitation -from registrar.models.contact import Contact +from registrar.models.user import User from registrar.models.user_domain_role import UserDomainRole from .mixins import ( @@ -154,9 +154,9 @@ class UserProfilePermissionView(UserProfilePermission, DetailView, abc.ABC): """ # DetailView property for what model this is viewing - model = Contact + model = User # variable name in template context for the model object - context_object_name = "contact" + context_object_name = "user" # Abstract property enforces NotImplementedError on an attribute. @property From 7c1b3ee698dc8fec69d15357759b59113f3a3212 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 2 Jul 2024 15:20:29 -0400 Subject: [PATCH 2/8] fixed test_management_scripts for import_tables script --- src/registrar/tests/test_management_scripts.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index cebee99940..cfe19b091a 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -1077,11 +1077,6 @@ def test_handle( for table_name in table_names: mock_path_exists.assert_any_call(f"{table_name}_1.csv") - # Check that clean_tables is called for Contact - mock_get_model.assert_any_call("registrar", "Contact") - model_mock = mock_get_model.return_value - model_mock.objects.all().delete.assert_called() - # Check that logger.info was called for each successful import for table_name in table_names: mock_logger.info.assert_any_call(f"Successfully imported {table_name}_1.csv into {table_name}") From 45c7f1aaa614b78838b8b9eb38ff041bd9a94e96 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 2 Jul 2024 16:59:18 -0400 Subject: [PATCH 3/8] removed user from contact model and all associated logic --- src/registrar/admin.py | 26 +- .../copy_names_from_contacts_to_users.py | 242 ------------------ ...registrar_c_user_id_4059c4_idx_and_more.py | 21 ++ src/registrar/models/contact.py | 40 --- src/registrar/tests/test_admin.py | 23 +- .../test_copy_names_from_contacts_to_users.py | 124 --------- src/registrar/tests/test_models.py | 8 +- src/registrar/tests/test_views.py | 31 ++- src/registrar/tests/test_views_request.py | 5 +- src/registrar/views/domain_request.py | 6 +- 10 files changed, 67 insertions(+), 459 deletions(-) delete mode 100644 src/registrar/management/commands/copy_names_from_contacts_to_users.py create mode 100644 src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py delete mode 100644 src/registrar/tests/test_copy_names_from_contacts_to_users.py diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a92e4c6959..6f455fc895 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -593,12 +593,6 @@ def get_filters(self, request): return filters -class UserContactInline(admin.StackedInline): - """Edit a user's profile on the user page.""" - - model = models.Contact - - class MyUserAdmin(BaseUserAdmin, ImportExportModelAdmin): """Custom user admin class to use our inlines.""" @@ -615,8 +609,6 @@ class Meta: _meta = Meta() - inlines = [UserContactInline] - list_display = ( "username", "overridden_email_field", @@ -894,30 +886,20 @@ class ContactAdmin(ListHeaderAdmin, ImportExportModelAdmin): list_display = [ "name", "email", - "user_exists", ] # this ordering effects the ordering of results - # in autocomplete_fields for user + # in autocomplete_fields ordering = ["first_name", "last_name", "email"] fieldsets = [ ( None, - {"fields": ["user", "first_name", "middle_name", "last_name", "title", "email", "phone"]}, + {"fields": ["first_name", "middle_name", "last_name", "title", "email", "phone"]}, ) ] - autocomplete_fields = ["user"] - change_form_template = "django/admin/email_clipboard_change_form.html" - def user_exists(self, obj): - """Check if the Contact has a related User""" - return "Yes" if obj.user is not None else "No" - - user_exists.short_description = "Is user" # type: ignore - user_exists.admin_order_field = "user" # type: ignore - # We name the custom prop 'contact' because linter # is not allowing a short_description attr on it # This gets around the linter limitation, for now. @@ -935,9 +917,7 @@ def name(self, obj: models.Contact): name.admin_order_field = "first_name" # type: ignore # Read only that we'll leverage for CISA Analysts - analyst_readonly_fields = [ - "user", - ] + analyst_readonly_fields = [] def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. diff --git a/src/registrar/management/commands/copy_names_from_contacts_to_users.py b/src/registrar/management/commands/copy_names_from_contacts_to_users.py deleted file mode 100644 index 3840294007..0000000000 --- a/src/registrar/management/commands/copy_names_from_contacts_to_users.py +++ /dev/null @@ -1,242 +0,0 @@ -import logging -import argparse -import sys - -from django.core.management import BaseCommand - -from registrar.management.commands.utility.terminal_helper import ( - TerminalColors, - TerminalHelper, -) -from registrar.models.contact import Contact -from registrar.models.user import User -from registrar.models.utility.domain_helper import DomainHelper - -logger = logging.getLogger(__name__) - - -class Command(BaseCommand): - help = """Copy first and last names from a contact to - a related user if it exists and if its first and last name - properties are null or blank strings.""" - - # ====================================================== - # ===================== ARGUMENTS ===================== - # ====================================================== - def add_arguments(self, parser): - parser.add_argument("--debug", action=argparse.BooleanOptionalAction) - - # ====================================================== - # ===================== PRINTING ====================== - # ====================================================== - def print_debug_mode_statements(self, debug_on: bool): - """Prints additional terminal statements to indicate if --debug - or --limitParse are in use""" - TerminalHelper.print_conditional( - debug_on, - f"""{TerminalColors.OKCYAN} - ----------DEBUG MODE ON---------- - Detailed print statements activated. - {TerminalColors.ENDC} - """, - ) - - def print_summary_of_findings( - self, - skipped_contacts, - eligible_users, - processed_users, - debug_on, - ): - """Prints to terminal a summary of findings from - copying first and last names from contacts to users""" - - total_eligible_users = len(eligible_users) - total_skipped_contacts = len(skipped_contacts) - total_processed_users = len(processed_users) - - logger.info( - f"""{TerminalColors.OKGREEN} - ============= FINISHED =============== - Skipped {total_skipped_contacts} contacts - Found {total_eligible_users} users linked to contacts - Processed {total_processed_users} users - {TerminalColors.ENDC} - """ # noqa - ) - - # DEBUG: - TerminalHelper.print_conditional( - debug_on, - f"""{TerminalColors.YELLOW} - ======= DEBUG OUTPUT ======= - Users who have a linked contact: - {eligible_users} - - Processed users (users who have a linked contact and a missing first or last name): - {processed_users} - - ===== SKIPPED CONTACTS ===== - {skipped_contacts} - - {TerminalColors.ENDC} - """, - ) - - # ====================================================== - # =================== USER ===================== - # ====================================================== - def update_user(self, contact: Contact, debug_on: bool): - """Given a contact with a first_name and last_name, find & update an existing - corresponding user if her first_name and last_name are null. - - Returns tuple of eligible (is linked to the contact) and processed - (first and last are blank) users. - """ - - user_exists = User.objects.filter(contact=contact).exists() - if user_exists: - try: - # ----------------------- UPDATE USER ----------------------- - # ---- GET THE USER - eligible_user = User.objects.get(contact=contact) - processed_user = None - # DEBUG: - TerminalHelper.print_conditional( - debug_on, - f"""{TerminalColors.YELLOW} - > Found linked user for contact: - {contact} {contact.email} {contact.first_name} {contact.last_name} - > The linked user is {eligible_user} {eligible_user.username} - {TerminalColors.ENDC}""", # noqa - ) - - # Get the fields that exist on both User and Contact. Excludes id. - common_fields = DomainHelper.get_common_fields(User, Contact) - if "email" in common_fields: - # Don't change the email field. - common_fields.remove("email") - - for field in common_fields: - # Grab the value that contact has stored for this field - new_value = getattr(contact, field) - - # Set it on the user field - setattr(eligible_user, field, new_value) - - eligible_user.save() - processed_user = eligible_user - - return ( - eligible_user, - processed_user, - ) - - except Exception as error: - logger.warning( - f""" - {TerminalColors.FAIL} - !!! ERROR: An exception occured in the - User table for the following user: - {contact.email} {contact.first_name} {contact.last_name} - - Exception is: {error} - ----------TERMINATING----------""" - ) - sys.exit() - else: - return None, None - - # ====================================================== - # ================= PROCESS CONTACTS ================== - # ====================================================== - - def process_contacts( - self, - debug_on, - skipped_contacts=[], - eligible_users=[], - processed_users=[], - ): - for contact in Contact.objects.all(): - TerminalHelper.print_conditional( - debug_on, - f"{TerminalColors.OKCYAN}" - "Processing Contact: " - f"{contact.email}," - f" {contact.first_name}," - f" {contact.last_name}" - f"{TerminalColors.ENDC}", - ) - - # ====================================================== - # ====================== USER ======================= - (eligible_user, processed_user) = self.update_user(contact, debug_on) - - debug_string = "" - if eligible_user: - # ---------------- UPDATED ---------------- - eligible_users.append(contact.email) - debug_string = f"eligible user: {eligible_user}" - if processed_user: - processed_users.append(contact.email) - debug_string = f"processed user: {processed_user}" - else: - skipped_contacts.append(contact.email) - debug_string = f"skipped user: {contact.email}" - - # DEBUG: - TerminalHelper.print_conditional( - debug_on, - (f"{TerminalColors.OKCYAN} {debug_string} {TerminalColors.ENDC}"), - ) - - return ( - skipped_contacts, - eligible_users, - processed_users, - ) - - # ====================================================== - # ===================== HANDLE ======================== - # ====================================================== - def handle( - self, - **options, - ): - """Parse entries in Contact table - and update valid corresponding entries in the - User table.""" - - # grab command line arguments and store locally... - debug_on = options.get("debug") - - self.print_debug_mode_statements(debug_on) - - logger.info( - f"""{TerminalColors.OKCYAN} - ========================== - Beginning Data Transfer - ========================== - {TerminalColors.ENDC}""" - ) - - logger.info( - f"""{TerminalColors.OKCYAN} - ========= Adding Domains and Domain Invitations ========= - {TerminalColors.ENDC}""" - ) - ( - skipped_contacts, - eligible_users, - processed_users, - ) = self.process_contacts( - debug_on, - ) - - self.print_summary_of_findings( - skipped_contacts, - eligible_users, - processed_users, - debug_on, - ) diff --git a/src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py b/src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py new file mode 100644 index 0000000000..858210be7e --- /dev/null +++ b/src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py @@ -0,0 +1,21 @@ +# Generated by Django 4.2.10 on 2024-07-02 19:52 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0109_domaininformation_sub_organization_and_more"), + ] + + operations = [ + migrations.RemoveIndex( + model_name="contact", + name="registrar_c_user_id_4059c4_idx", + ), + migrations.RemoveField( + model_name="contact", + name="user", + ), + ] diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index f7bae3491a..9036337499 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -14,17 +14,9 @@ class Meta: """Contains meta information about this class""" indexes = [ - models.Index(fields=["user"]), models.Index(fields=["email"]), ] - user = models.OneToOneField( - "registrar.User", - null=True, - blank=True, - on_delete=models.SET_NULL, - ) - first_name = models.CharField( null=True, blank=True, @@ -103,38 +95,6 @@ def get_formatted_name(self): def has_contact_info(self): return bool(self.title or self.email or self.phone) - def save(self, *args, **kwargs): - # Call the parent class's save method to perform the actual save - super().save(*args, **kwargs) - - 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 middle_name if necessary - if not self.user.middle_name: - self.user.middle_name = self.middle_name - updated = True - - # Update phone if necessary - if not self.user.phone: - self.user.phone = self.phone - updated = True - - # Update title if necessary - if not self.user.title: - self.user.title = self.title - 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: return self.get_formatted_name() diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 65427bb652..a1532cd395 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -4036,9 +4036,7 @@ def test_readonly_when_restricted_staffuser(self): readonly_fields = self.admin.get_readonly_fields(request) - expected_fields = [ - "user", - ] + expected_fields = [] self.assertEqual(readonly_fields, expected_fields) @@ -4054,15 +4052,18 @@ def test_readonly_when_restricted_superuser(self): self.assertEqual(readonly_fields, expected_fields) def test_change_view_for_joined_contact_five_or_less(self): - """Create a contact, join it to 4 domain requests. 5th join is user. + """Create a contact, join it to 4 domain requests. Assert that the warning on the contact form lists 4 joins.""" with less_console_noise(): self.client.force_login(self.superuser) # Create an instance of the model - contact, _ = Contact.objects.get_or_create(user=self.staffuser) + contact, _ = Contact.objects.get_or_create( + first_name="Henry", + last_name="McFakerson", + ) - # join it to 4 domain requests. The 5th join will be a user. + # join it to 4 domain requests. domain_request1 = completed_domain_request(submitter=contact, name="city1.gov") domain_request2 = completed_domain_request(submitter=contact, name="city2.gov") domain_request3 = completed_domain_request(submitter=contact, name="city3.gov") @@ -4085,24 +4086,26 @@ def test_change_view_for_joined_contact_five_or_less(self): f"domainrequest/{domain_request3.pk}/change/'>city3.gov" "
  • Joined to DomainRequest: city4.gov
    • " - "
  • Joined to User: first last staff@example.com
    • " "", ) def test_change_view_for_joined_contact_five_or_more(self): - """Create a contact, join it to 5 domain requests. 6th join is user. + """Create a contact, join it to 6 domain requests. Assert that the warning on the contact form lists 5 joins and a '1 more' ellispsis.""" with less_console_noise(): self.client.force_login(self.superuser) # Create an instance of the model # join it to 6 domain requests. - contact, _ = Contact.objects.get_or_create(user=self.staffuser) + contact, _ = Contact.objects.get_or_create( + first_name="Henry", + last_name="McFakerson", + ) domain_request1 = completed_domain_request(submitter=contact, name="city1.gov") domain_request2 = completed_domain_request(submitter=contact, name="city2.gov") domain_request3 = completed_domain_request(submitter=contact, name="city3.gov") domain_request4 = completed_domain_request(submitter=contact, name="city4.gov") domain_request5 = completed_domain_request(submitter=contact, name="city5.gov") + domain_request6 = completed_domain_request(submitter=contact, name="city6.gov") with patch("django.contrib.messages.warning") as mock_warning: # Use the test client to simulate the request response = self.client.get(reverse("admin:registrar_contact_change", args=[contact.pk])) diff --git a/src/registrar/tests/test_copy_names_from_contacts_to_users.py b/src/registrar/tests/test_copy_names_from_contacts_to_users.py deleted file mode 100644 index 7fcbede1e1..0000000000 --- a/src/registrar/tests/test_copy_names_from_contacts_to_users.py +++ /dev/null @@ -1,124 +0,0 @@ -from django.test import TestCase - -from registrar.models import ( - User, - Contact, -) - -from registrar.management.commands.copy_names_from_contacts_to_users import Command - - -class TestDataUpdates(TestCase): - def setUp(self): - """We cannot setup the user details because contacts will override the first and last names in its save method - so we will initiate the users, setup the contacts and link them, and leave the rest of the setup to the test(s). - """ - - self.user1 = User.objects.create(username="user1") - self.user2 = User.objects.create(username="user2") - self.user3 = User.objects.create(username="user3") - self.user4 = User.objects.create(username="user4") - # The last user created triggers the creation of a contact and attaches itself to it. See signals. - # This bs_user defuses that situation. - self.bs_user = User.objects.create() - - self.contact1 = Contact.objects.create( - user=self.user1, - email="email1@igorville.gov", - first_name="first1", - last_name="last1", - middle_name="middle1", - title="title1", - ) - self.contact2 = Contact.objects.create( - user=self.user2, - email="email2@igorville.gov", - first_name="first2", - last_name="last2", - middle_name="middle2", - title="title2", - ) - self.contact3 = Contact.objects.create( - user=self.user3, - email="email3@igorville.gov", - first_name="first3", - last_name="last3", - middle_name="middle3", - title="title3", - ) - self.contact4 = Contact.objects.create( - email="email4@igorville.gov", first_name="first4", last_name="last4", middle_name="middle4", title="title4" - ) - - self.command = Command() - - def tearDown(self): - """Clean up""" - # Delete users and contacts - User.objects.all().delete() - Contact.objects.all().delete() - - def test_script_updates_linked_users(self): - """Test the script that copies contact information to the user object""" - - # Set up the users' first and last names here so - # they that they don't get overwritten by Contact's save() - # User with no first or last names - self.user1.first_name = "" - self.user1.last_name = "" - self.user1.title = "dummytitle" - self.user1.middle_name = "dummymiddle" - self.user1.save() - - # User with a first name but no last name - self.user2.first_name = "First name but no last name" - self.user2.last_name = "" - self.user2.save() - - # User with a first and last name - self.user3.first_name = "An existing first name" - self.user3.last_name = "An existing last name" - self.user3.save() - - # Call the parent method the same way we do it in the script - skipped_contacts = [] - eligible_users = [] - processed_users = [] - ( - skipped_contacts, - eligible_users, - processed_users, - ) = self.command.process_contacts( - # Set debugging to False - False, - skipped_contacts, - eligible_users, - processed_users, - ) - - # Trigger DB refresh - self.user1.refresh_from_db() - self.user2.refresh_from_db() - self.user3.refresh_from_db() - - # Asserts - # The user that has no first and last names will get them from the contact - self.assertEqual(self.user1.first_name, "first1") - self.assertEqual(self.user1.last_name, "last1") - self.assertEqual(self.user1.middle_name, "middle1") - self.assertEqual(self.user1.title, "title1") - # The user that has a first but no last will be updated - self.assertEqual(self.user2.first_name, "first2") - self.assertEqual(self.user2.last_name, "last2") - self.assertEqual(self.user2.middle_name, "middle2") - self.assertEqual(self.user2.title, "title2") - # The user that has a first and a last will be updated - self.assertEqual(self.user3.first_name, "first3") - self.assertEqual(self.user3.last_name, "last3") - self.assertEqual(self.user3.middle_name, "middle3") - self.assertEqual(self.user3.title, "title3") - # The unlinked user will be left alone - self.assertEqual(self.user4.first_name, "") - self.assertEqual(self.user4.last_name, "") - self.assertEqual(self.user4.middle_name, None) - self.assertEqual(self.user4.title, None) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 9a29f54284..0c5ca9193e 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1221,7 +1221,10 @@ def setUp(self): 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, _ = Contact.objects.get_or_create( + first_name="Jeff", + last_name="Lebowski", + ) self.contact_as_so, _ = Contact.objects.get_or_create(email="newguy@igorville.gov") self.domain_request = DomainRequest.objects.create(creator=self.user, senior_official=self.contact_as_so) @@ -1234,9 +1237,6 @@ def tearDown(self): def test_has_more_than_one_join(self): """Test the Contact model method, has_more_than_one_join""" - # test for a contact which has one user defined - self.assertFalse(self.contact.has_more_than_one_join("user")) - self.assertTrue(self.contact.has_more_than_one_join("senior_official")) # test for a contact which is assigned as a senior official on a domain request self.assertFalse(self.contact_as_so.has_more_than_one_join("senior_official")) self.assertTrue(self.contact_as_so.has_more_than_one_join("submitted_domain_requests")) diff --git a/src/registrar/tests/test_views.py b/src/registrar/tests/test_views.py index 42c7b2d385..b98a30e79a 100644 --- a/src/registrar/tests/test_views.py +++ b/src/registrar/tests/test_views.py @@ -372,7 +372,10 @@ def test_home_deletes_domain_request_and_orphans(self): ) # Attach a user object to a contact (should not be deleted) - contact_user, _ = Contact.objects.get_or_create(user=self.user) + contact_user, _ = Contact.objects.get_or_create( + first_name="Hank", + last_name="McFakey", + ) site = DraftDomain.objects.create(name="igorville.gov") domain_request = DomainRequest.objects.create( @@ -405,17 +408,12 @@ def test_home_deletes_domain_request_and_orphans(self): igorville = DomainRequest.objects.filter(requested_domain__name="igorville.gov") self.assertFalse(igorville.exists()) - # Check if the orphaned contact was deleted + # Check if the orphaned contacts were deleted orphan = Contact.objects.filter(id=contact.id) self.assertFalse(orphan.exists()) + orphan = Contact.objects.filter(id=contact_user.id) + self.assertFalse(orphan.exists()) - # All non-orphan contacts should still exist and are unaltered - try: - current_user = Contact.objects.filter(id=contact_user.id).get() - except Contact.DoesNotExist: - self.fail("contact_user (a non-orphaned contact) was deleted") - - self.assertEqual(current_user, contact_user) try: edge_case = Contact.objects.filter(id=contact_2.id).get() except Contact.DoesNotExist: @@ -444,7 +442,10 @@ def test_home_deletes_domain_request_and_shared_orphans(self): ) # Attach a user object to a contact (should not be deleted) - contact_user, _ = Contact.objects.get_or_create(user=self.user) + contact_user, _ = Contact.objects.get_or_create( + first_name="Hank", + last_name="McFakey", + ) site = DraftDomain.objects.create(name="igorville.gov") domain_request = DomainRequest.objects.create( @@ -863,7 +864,10 @@ def test_domain_your_contact_information_when_profile_feature_on(self): def test_request_when_profile_feature_on(self): """test that Your profile is in request page when profile feature is on""" - contact_user, _ = Contact.objects.get_or_create(user=self.user) + contact_user, _ = Contact.objects.get_or_create( + first_name="Hank", + last_name="McFakerson", + ) site = DraftDomain.objects.create(name="igorville.gov") domain_request = DomainRequest.objects.create( creator=self.user, @@ -882,7 +886,10 @@ def test_request_when_profile_feature_on(self): def test_request_when_profile_feature_off(self): """test that Your profile is not in request page when profile feature is off""" - contact_user, _ = Contact.objects.get_or_create(user=self.user) + contact_user, _ = Contact.objects.get_or_create( + first_name="Hank", + last_name="McFakerson", + ) site = DraftDomain.objects.create(name="igorville.gov") domain_request = DomainRequest.objects.create( creator=self.user, diff --git a/src/registrar/tests/test_views_request.py b/src/registrar/tests/test_views_request.py index e78dfe8601..de924576b5 100644 --- a/src/registrar/tests/test_views_request.py +++ b/src/registrar/tests/test_views_request.py @@ -2974,7 +2974,10 @@ def test_unlocked_steps_partial_domain_request(self): ) # Attach a user object to a contact (should not be deleted) - contact_user, _ = Contact.objects.get_or_create(user=self.user) + contact_user, _ = Contact.objects.get_or_create( + first_name="Hank", + last_name="McFakey", + ) site = DraftDomain.objects.create(name="igorville.gov") domain_request = DomainRequest.objects.create( diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index e8e82500e4..a7d6aa6ae8 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -812,15 +812,15 @@ def post(self, request, *args, **kwargs): self.object = self.get_object() self.object.delete() - # Delete orphaned contacts - but only for if they are not associated with a user - Contact.objects.filter(id__in=contacts_to_delete, user=None).delete() + # Delete orphaned contacts + Contact.objects.filter(id__in=contacts_to_delete).delete() # After a delete occurs, do a second sweep on any returned duplicates. # This determines if any of these three fields share a contact, which is used for # the edge case where the same user may be an SO, and a submitter, for example. if len(duplicates) > 0: duplicates_to_delete, _ = self._get_orphaned_contacts(domain_request, check_db=True) - Contact.objects.filter(id__in=duplicates_to_delete, user=None).delete() + Contact.objects.filter(id__in=duplicates_to_delete).delete() # Return a 200 response with an empty body return HttpResponse(status=200) From 1dc3bf883e299cd89df521b8ccad2163bcfc6976 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 2 Jul 2024 17:10:52 -0400 Subject: [PATCH 4/8] updated for linter --- src/registrar/admin.py | 2 +- src/registrar/tests/test_admin.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6f455fc895..967c36494e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -917,7 +917,7 @@ def name(self, obj: models.Contact): name.admin_order_field = "first_name" # type: ignore # Read only that we'll leverage for CISA Analysts - analyst_readonly_fields = [] + analyst_readonly_fields: list[str] = [] def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index a1532cd395..889a2c0e0c 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -4105,7 +4105,7 @@ def test_change_view_for_joined_contact_five_or_more(self): domain_request3 = completed_domain_request(submitter=contact, name="city3.gov") domain_request4 = completed_domain_request(submitter=contact, name="city4.gov") domain_request5 = completed_domain_request(submitter=contact, name="city5.gov") - domain_request6 = completed_domain_request(submitter=contact, name="city6.gov") + completed_domain_request(submitter=contact, name="city6.gov") with patch("django.contrib.messages.warning") as mock_warning: # Use the test client to simulate the request response = self.client.get(reverse("admin:registrar_contact_change", args=[contact.pk])) From a7abfa9cdb16ac85fad271a01c665e0a7f0e276b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 9 Jul 2024 16:48:03 -0400 Subject: [PATCH 5/8] made email readonly and fixed some unit tests --- src/registrar/admin.py | 2 +- src/registrar/forms/domain.py | 2 +- src/registrar/tests/test_admin.py | 4 +++- src/registrar/views/user_profile.py | 1 - 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7ddff76c0f..325fc53dd4 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -917,7 +917,7 @@ def name(self, obj: models.Contact): name.admin_order_field = "first_name" # type: ignore # Read only that we'll leverage for CISA Analysts - analyst_readonly_fields: list[str] = [] + analyst_readonly_fields: list[str] = ["email"] def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 06163dbce1..9b8f1b7fce 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -204,7 +204,7 @@ def clean(self): class UserForm(forms.ModelForm): - """Form for updating contacts.""" + """Form for updating users.""" email = forms.EmailField(max_length=None) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 87fd9b5b86..c8fe64c423 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2105,6 +2105,7 @@ def test_contact_fields_have_detail_table(self): ] self.test_helper.assert_response_contains_distinct_values(response, expected_submitter_fields) self.assertContains(response, "Testy2 Tester2") + self.assertContains(response, "meoward.jones@igorville.gov") # == Check for the senior_official == # self.assertContains(response, "testy@town.com", count=2) @@ -3130,6 +3131,7 @@ def test_contact_fields_have_detail_table(self): ("phone", "(555) 123 12345"), ] self.test_helper.assert_response_contains_distinct_values(response, expected_creator_fields) + self.assertContains(response, "meoward.jones@igorville.gov") # Check for the field itself self.assertContains(response, "Meoward Jones") @@ -4036,7 +4038,7 @@ def test_readonly_when_restricted_staffuser(self): readonly_fields = self.admin.get_readonly_fields(request) - expected_fields = [] + expected_fields = ["email"] self.assertEqual(readonly_fields, expected_fields) diff --git a/src/registrar/views/user_profile.py b/src/registrar/views/user_profile.py index e378b91808..dec1d1af1e 100644 --- a/src/registrar/views/user_profile.py +++ b/src/registrar/views/user_profile.py @@ -54,7 +54,6 @@ def dispatch(self, request, *args, **kwargs): # type: ignore def get_context_data(self, **kwargs): """Extend get_context_data to include has_profile_feature_flag""" context = super().get_context_data(**kwargs) - logger.info("UserProfileView::get_context_data") # This is a django waffle flag which toggles features based off of the "flag" table context["has_profile_feature_flag"] = flag_is_active(self.request, "profile_feature") From d393350a7f132eb079acf5762404656a20af9f5c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 9 Jul 2024 17:05:37 -0400 Subject: [PATCH 6/8] fixed migrations --- ...2_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename src/registrar/migrations/{0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py => 0112_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py} (84%) diff --git a/src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py b/src/registrar/migrations/0112_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py similarity index 84% rename from src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py rename to src/registrar/migrations/0112_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py index 858210be7e..db9b7970a9 100644 --- a/src/registrar/migrations/0110_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py +++ b/src/registrar/migrations/0112_remove_contact_registrar_c_user_id_4059c4_idx_and_more.py @@ -6,7 +6,7 @@ class Migration(migrations.Migration): dependencies = [ - ("registrar", "0109_domaininformation_sub_organization_and_more"), + ("registrar", "0111_create_groups_v15"), ] operations = [ From f0dfe96e974e4313b1316e6d6717b088b9e86639 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 9 Jul 2024 17:24:35 -0400 Subject: [PATCH 7/8] fixed tests --- src/registrar/tests/test_admin.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 4fa4431054..06fb272375 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -939,7 +939,10 @@ def test_domain_request_senior_official_is_alphabetically_sorted(self): SeniorOfficial.objects.get_or_create(first_name="alex", last_name="smoe", title="some guy") SeniorOfficial.objects.get_or_create(first_name="Zoup", last_name="Soup", title="title") - contact, _ = Contact.objects.get_or_create(user=self.staffuser) + contact, _ = Contact.objects.get_or_create( + first_name="Henry", + last_name="McFakerson" + ) domain_request = completed_domain_request(submitter=contact, name="city1.gov") request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) model_admin = AuditedAdmin(DomainRequest, self.site) @@ -2943,7 +2946,10 @@ def test_domain_information_senior_official_is_alphabetically_sorted(self): SeniorOfficial.objects.get_or_create(first_name="alex", last_name="smoe", title="some guy") SeniorOfficial.objects.get_or_create(first_name="Zoup", last_name="Soup", title="title") - contact, _ = Contact.objects.get_or_create(user=self.staffuser) + contact, _ = Contact.objects.get_or_create( + first_name="Henry", + last_name="McFakerson" + ) domain_request = completed_domain_request( submitter=contact, name="city1244.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW ) From d981c410e827854e0321681451c7288a1b4e960d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 9 Jul 2024 17:30:45 -0400 Subject: [PATCH 8/8] linter --- src/registrar/tests/test_admin.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 06fb272375..b77a1faf45 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -939,10 +939,7 @@ def test_domain_request_senior_official_is_alphabetically_sorted(self): SeniorOfficial.objects.get_or_create(first_name="alex", last_name="smoe", title="some guy") SeniorOfficial.objects.get_or_create(first_name="Zoup", last_name="Soup", title="title") - contact, _ = Contact.objects.get_or_create( - first_name="Henry", - last_name="McFakerson" - ) + contact, _ = Contact.objects.get_or_create(first_name="Henry", last_name="McFakerson") domain_request = completed_domain_request(submitter=contact, name="city1.gov") request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(domain_request.pk)) model_admin = AuditedAdmin(DomainRequest, self.site) @@ -2946,10 +2943,7 @@ def test_domain_information_senior_official_is_alphabetically_sorted(self): SeniorOfficial.objects.get_or_create(first_name="alex", last_name="smoe", title="some guy") SeniorOfficial.objects.get_or_create(first_name="Zoup", last_name="Soup", title="title") - contact, _ = Contact.objects.get_or_create( - first_name="Henry", - last_name="McFakerson" - ) + contact, _ = Contact.objects.get_or_create(first_name="Henry", last_name="McFakerson") domain_request = completed_domain_request( submitter=contact, name="city1244.gov", status=DomainRequest.DomainRequestStatus.IN_REVIEW )