From 49c900297ed3f2a08c0cbaa774102e31e574c2cc Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 2 Jan 2025 20:09:32 -0500 Subject: [PATCH 1/7] accessible markup and unit test coverage --- .../assets/src/sass/_theme/_admin.scss | 34 ++++ .../admin/includes/detail_table_fieldset.html | 38 ++-- src/registrar/tests/common.py | 25 ++- src/registrar/tests/test_admin_request.py | 181 +++++++++++++++++- src/registrar/tests/test_emails.py | 12 +- 5 files changed, 259 insertions(+), 31 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_admin.scss b/src/registrar/assets/src/sass/_theme/_admin.scss index 58ce1e4df..f5f94c8f3 100644 --- a/src/registrar/assets/src/sass/_theme/_admin.scss +++ b/src/registrar/assets/src/sass/_theme/_admin.scss @@ -342,6 +342,40 @@ div#content > h2 { } } +.module { + .margin-left-0 { + margin-left: 0; + } + .margin-top-0 { + margin-top: 0; + } + .padding-left-0 { + padding-left: 0; + } +} + +.admin-list-inline { + li { + float: left; + padding-top: 0; + margin-right: 4px; + } + li:not(:last-child)::after { + content: ","; + } +} + +.form-row { + .margin-y-0 { + margin-top: 0; + margin-bottom: 0; + } + .padding-y-0 { + padding-top: 0; + padding-bottom: 0; + } +} + // Fixes a display issue where the list was entirely white, or had too much whitespace .select2-dropdown { display: inline-grid !important; diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index d5b1130ab..57c59f202 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -109,22 +109,38 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) This ONLY applies to analysts. For superusers, its business as usual. {% endcomment %}
- {% with total_websites=field.contents|split:", " %} - {% for website in total_websites %} - {{ website }}{% if not forloop.last %}, {% endif %} - {# Acts as a
#} - {% if total_websites|length < 5 %} -
+ {% with total_websites=field.contents|split:", " %} + {% if total_websites|length == 1 %} +

+ + {{ total_websites.0 }} + +

+ {% elif total_websites|length > 1 %} + {% endif %} - {% endfor %} - {% endwith %} + {% endwith %}
{% elif field.field.name == "alternative_domains" %}
{% with current_path=request.get_full_path %} - {% for alt_domain in original_object.alternative_domains.all %} - {{ alt_domain }}{% if not forloop.last %}, {% endif %} - {% endfor %} + {% if original_object.alternative_domains.all|length == 1 %} +

+ {{ original_object.alternative_domains.all.0 }} +

+ {% elif original_object.alternative_domains.all|length > 1 %} + + {% endif %} {% endwith %}
{% elif field.field.name == "domain_managers" or field.field.name == "invited_domain_managers" %} diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index af4345a14..a039af3d0 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -13,6 +13,7 @@ from django.contrib.auth import get_user_model, login from django.utils.timezone import make_aware from datetime import date, datetime, timedelta from django.utils import timezone +from django.utils.html import strip_spaces_between_tags from registrar.models import ( Contact, @@ -107,6 +108,11 @@ def get_time_aware_date(date=datetime(2023, 11, 1)): return timezone.make_aware(date) +def normalize_html(html): + """Normalize HTML by removing newlines and extra spaces.""" + return strip_spaces_between_tags(" ".join(html.split())) + + class GenericTestHelper(TestCase): """A helper class that contains various helper functions for TestCases""" @@ -1017,8 +1023,9 @@ def create_ready_domain(): # TODO in 1793: Remove the federal agency/updated federal agency fields def completed_domain_request( # noqa has_other_contacts=True, - has_current_website=True, - has_alternative_gov_domain=True, + # pass empty [] if you want current_websites or alternative_domains set to None + current_websites=["city.com"], + alternative_domains=["city1.gov"], has_about_your_organization=True, has_anything_else=True, has_cisa_representative=True, @@ -1046,8 +1053,6 @@ def completed_domain_request( # noqa phone="(555) 555 5555", ) domain, _ = DraftDomain.objects.get_or_create(name=name) - alt, _ = Website.objects.get_or_create(website="city1.gov") - current, _ = Website.objects.get_or_create(website="city.com") other, _ = Contact.objects.get_or_create( first_name="Testy", last_name="Tester", @@ -1102,10 +1107,14 @@ def completed_domain_request( # noqa if has_other_contacts: domain_request.other_contacts.add(other) - if has_current_website: - domain_request.current_websites.add(current) - if has_alternative_gov_domain: - domain_request.alternative_domains.add(alt) + if len(current_websites) > 0: + for website in current_websites: + current, _ = Website.objects.get_or_create(website=website) + domain_request.current_websites.add(current) + if len(alternative_domains) > 0: + for alternative_domain in alternative_domains: + alt, _ = Website.objects.get_or_create(website=alternative_domain) + domain_request.alternative_domains.add(alt) if has_cisa_representative: domain_request.cisa_representative_first_name = "CISA-first-name" domain_request.cisa_representative_last_name = "CISA-last-name" diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 439f4fab0..8d0afc1c4 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -35,6 +35,7 @@ from .common import ( multiple_unalphabetical_domain_objects, MockEppLib, GenericTestHelper, + normalize_html, ) from unittest.mock import patch @@ -1445,8 +1446,9 @@ class TestDomainRequestAdmin(MockEppLib): self.assertContains(response, expected_url) @less_console_noise_decorator - def test_other_websites_has_readonly_link(self): - """Tests if the readonly other_websites field has links""" + def test_other_websites_has_one_readonly_link(self): + """Tests if the readonly other_websites field has links. + Test markup for one website.""" # Create a fake domain request domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) @@ -1462,8 +1464,179 @@ class TestDomainRequestAdmin(MockEppLib): self.assertContains(response, domain_request.requested_domain.name) # Check that the page contains the link we expect. - expected_url = 'city.com' - self.assertContains(response, expected_url) + expected_markup = """ +

+ + city.com + +

+ """ + + normalized_expected = normalize_html(expected_markup) + normalized_response = normalize_html(response.content.decode("utf-8")) + + self.assertIn(normalized_expected, normalized_response) + + @less_console_noise_decorator + def test_other_websites_has_few_readonly_links(self): + """Tests if the readonly other_websites field has links. + Test markup for l5 or less websites.""" + + # Create a domain request with 4 current websites + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + current_websites=["city.gov", "city2.gov", "city3.gov", "city4.gov"], + ) + + self.client.force_login(self.staffuser) + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + + # Check that the page contains the link we expect. + expected_markup = """ + + """ + + normalized_expected = normalize_html(expected_markup) + normalized_response = normalize_html(response.content.decode("utf-8")) + + self.assertIn(normalized_expected, normalized_response) + + @less_console_noise_decorator + def test_other_websites_has_lots_readonly_links(self): + """Tests if the readonly other_websites field has links. + Test markup for 6 or more websites.""" + + # Create a domain requests with 6 current websites + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + current_websites=["city.gov", "city2.gov", "city3.gov", "city4.gov", "city5.gov", "city6.gov"], + ) + + self.client.force_login(self.staffuser) + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + + # Check that the page contains the link we expect. + expected_markup = """ + + """ + + normalized_expected = normalize_html(expected_markup) + normalized_response = normalize_html(response.content.decode("utf-8")) + + self.assertIn(normalized_expected, normalized_response) + + @less_console_noise_decorator + def test_alternative_domains_has_one_readonly_link(self): + """Tests if the readonly alternative_domains field has links. + Test markup for one website.""" + + # Create a fake domain request + domain_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.IN_REVIEW) + + self.client.force_login(self.staffuser) + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + + # Check that the page contains the link we expect. + website = Website.objects.filter(website="city1.gov").first() + base_url = "/admin/registrar/website" + return_path = f"/admin/registrar/domainrequest/{domain_request.pk}/change/" + expected_markup = f""" +

+ city1.gov +

+ """ + + normalized_expected = normalize_html(expected_markup) + normalized_response = normalize_html(response.content.decode("utf-8")) + + self.assertIn(normalized_expected, normalized_response) + + @less_console_noise_decorator + def test_alternative_domains_has_lots_readonly_link(self): + """Tests if the readonly other_websites field has links. + Test markup for 6 or more websites.""" + + # Create a domain request with 6 alternative domains + domain_request = completed_domain_request( + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + alternative_domains=[ + "altcity1.gov", + "altcity2.gov", + "altcity3.gov", + "altcity4.gov", + "altcity5.gov", + "altcity6.gov", + ], + ) + + self.client.force_login(self.staffuser) + response = self.client.get( + "/admin/registrar/domainrequest/{}/change/".format(domain_request.pk), + follow=True, + ) + + # Make sure the page loaded, and that we're on the right page + self.assertEqual(response.status_code, 200) + self.assertContains(response, domain_request.requested_domain.name) + + # Check that the page contains the link we expect. + website1 = Website.objects.filter(website="altcity1.gov").first() + website2 = Website.objects.filter(website="altcity2.gov").first() + website3 = Website.objects.filter(website="altcity3.gov").first() + website4 = Website.objects.filter(website="altcity4.gov").first() + website5 = Website.objects.filter(website="altcity5.gov").first() + website6 = Website.objects.filter(website="altcity6.gov").first() + base_url = "/admin/registrar/website" + return_path = f"/admin/registrar/domainrequest/{domain_request.pk}/change/" + attr = 'target="_blank"' + expected_markup = f""" + + """ + + normalized_expected = normalize_html(expected_markup) + normalized_response = normalize_html(response.content.decode("utf-8")) + + self.assertIn(normalized_expected, normalized_response) @less_console_noise_decorator def test_contact_fields_have_detail_table(self): diff --git a/src/registrar/tests/test_emails.py b/src/registrar/tests/test_emails.py index e76a6124f..f39f11517 100644 --- a/src/registrar/tests/test_emails.py +++ b/src/registrar/tests/test_emails.py @@ -150,7 +150,7 @@ class TestEmails(TestCase): def test_submission_confirmation_no_current_website_spacing(self): """Test line spacing without current_website.""" domain_request = completed_domain_request( - has_current_website=False, user=User.objects.create(username="test", email="testy@town.com") + current_websites=[], user=User.objects.create(username="test", email="testy@town.com") ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() @@ -164,9 +164,7 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_current_website_spacing(self): """Test line spacing with current_website.""" - domain_request = completed_domain_request( - has_current_website=True, user=User.objects.create(username="test", email="testy@town.com") - ) + domain_request = completed_domain_request(user=User.objects.create(username="test", email="testy@town.com")) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -218,9 +216,7 @@ class TestEmails(TestCase): @less_console_noise_decorator def test_submission_confirmation_alternative_govdomain_spacing(self): """Test line spacing with alternative .gov domain.""" - domain_request = completed_domain_request( - has_alternative_gov_domain=True, user=User.objects.create(username="test", email="testy@town.com") - ) + domain_request = completed_domain_request(user=User.objects.create(username="test", email="testy@town.com")) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() _, kwargs = self.mock_client.send_email.call_args @@ -234,7 +230,7 @@ class TestEmails(TestCase): def test_submission_confirmation_no_alternative_govdomain_spacing(self): """Test line spacing without alternative .gov domain.""" domain_request = completed_domain_request( - has_alternative_gov_domain=False, user=User.objects.create(username="test", email="testy@town.com") + alternative_domains=[], user=User.objects.create(username="test", email="testy@town.com") ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class): domain_request.submit() From 61bfec9982e9a4555f8a9fa94b011cd34c1204e4 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 3 Jan 2025 12:02:33 -0500 Subject: [PATCH 2/7] avoid flooding console --- src/registrar/tests/test_admin_request.py | 50 ++++++++++++++++++++--- 1 file changed, 45 insertions(+), 5 deletions(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 8d0afc1c4..260772c20 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1475,7 +1475,15 @@ class TestDomainRequestAdmin(MockEppLib): normalized_expected = normalize_html(expected_markup) normalized_response = normalize_html(response.content.decode("utf-8")) - self.assertIn(normalized_expected, normalized_response) + index = normalized_response.find(normalized_expected) + + # Assert that the expected markup is found in the response + assert index != -1, ( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_other_websites_has_few_readonly_links(self): @@ -1511,7 +1519,15 @@ class TestDomainRequestAdmin(MockEppLib): normalized_expected = normalize_html(expected_markup) normalized_response = normalize_html(response.content.decode("utf-8")) - self.assertIn(normalized_expected, normalized_response) + index = normalized_response.find(normalized_expected) + + # Assert that the expected markup is found in the response + assert index != -1, ( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_other_websites_has_lots_readonly_links(self): @@ -1549,7 +1565,15 @@ class TestDomainRequestAdmin(MockEppLib): normalized_expected = normalize_html(expected_markup) normalized_response = normalize_html(response.content.decode("utf-8")) - self.assertIn(normalized_expected, normalized_response) + index = normalized_response.find(normalized_expected) + + # Assert that the expected markup is found in the response + assert index != -1, ( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_alternative_domains_has_one_readonly_link(self): @@ -1582,7 +1606,15 @@ class TestDomainRequestAdmin(MockEppLib): normalized_expected = normalize_html(expected_markup) normalized_response = normalize_html(response.content.decode("utf-8")) - self.assertIn(normalized_expected, normalized_response) + index = normalized_response.find(normalized_expected) + + # Assert that the expected markup is found in the response + assert index != -1, ( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_alternative_domains_has_lots_readonly_link(self): @@ -1636,7 +1668,15 @@ class TestDomainRequestAdmin(MockEppLib): normalized_expected = normalize_html(expected_markup) normalized_response = normalize_html(response.content.decode("utf-8")) - self.assertIn(normalized_expected, normalized_response) + index = normalized_response.find(normalized_expected) + + # Assert that the expected markup is found in the response + assert index != -1, ( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_contact_fields_have_detail_table(self): From 60fcc888adbc79fc2569954962a32ae38244d69d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 6 Jan 2025 14:30:00 -0500 Subject: [PATCH 3/7] change assert to satisfy linting --- src/registrar/tests/test_admin_request.py | 65 ++++++++++++----------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 81ce5d1a3..eeb14380f 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1559,12 +1559,13 @@ class TestDomainRequestAdmin(MockEppLib): index = normalized_response.find(normalized_expected) # Assert that the expected markup is found in the response - assert index != -1, ( - f"Expected markup not found in the response.\n\n" - f"Expected:\n{normalized_expected}\n\n" - f"Start index of mismatch: {index}\n\n" - f"Consider checking the surrounding response for context." - ) + if index == -1: + self.fail( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_other_websites_has_few_readonly_links(self): @@ -1603,12 +1604,13 @@ class TestDomainRequestAdmin(MockEppLib): index = normalized_response.find(normalized_expected) # Assert that the expected markup is found in the response - assert index != -1, ( - f"Expected markup not found in the response.\n\n" - f"Expected:\n{normalized_expected}\n\n" - f"Start index of mismatch: {index}\n\n" - f"Consider checking the surrounding response for context." - ) + if index == -1: + self.fail( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_other_websites_has_lots_readonly_links(self): @@ -1649,12 +1651,13 @@ class TestDomainRequestAdmin(MockEppLib): index = normalized_response.find(normalized_expected) # Assert that the expected markup is found in the response - assert index != -1, ( - f"Expected markup not found in the response.\n\n" - f"Expected:\n{normalized_expected}\n\n" - f"Start index of mismatch: {index}\n\n" - f"Consider checking the surrounding response for context." - ) + if index == -1: + self.fail( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_alternative_domains_has_one_readonly_link(self): @@ -1690,12 +1693,13 @@ class TestDomainRequestAdmin(MockEppLib): index = normalized_response.find(normalized_expected) # Assert that the expected markup is found in the response - assert index != -1, ( - f"Expected markup not found in the response.\n\n" - f"Expected:\n{normalized_expected}\n\n" - f"Start index of mismatch: {index}\n\n" - f"Consider checking the surrounding response for context." - ) + if index == -1: + self.fail( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_alternative_domains_has_lots_readonly_link(self): @@ -1752,12 +1756,13 @@ class TestDomainRequestAdmin(MockEppLib): index = normalized_response.find(normalized_expected) # Assert that the expected markup is found in the response - assert index != -1, ( - f"Expected markup not found in the response.\n\n" - f"Expected:\n{normalized_expected}\n\n" - f"Start index of mismatch: {index}\n\n" - f"Consider checking the surrounding response for context." - ) + if index == -1: + self.fail( + f"Expected markup not found in the response.\n\n" + f"Expected:\n{normalized_expected}\n\n" + f"Start index of mismatch: {index}\n\n" + f"Consider checking the surrounding response for context." + ) @less_console_noise_decorator def test_contact_fields_have_detail_table(self): From 08efc6629ee14c30e313d8cfbf3b913d4e3f1b45 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 16:32:28 -0500 Subject: [PATCH 4/7] in django admin, new domain invitations are auto retrieved when user exists --- src/registrar/admin.py | 20 +++++++++ src/registrar/tests/test_admin.py | 67 ++++++++++++++++++++++++++----- 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index fba675bf7..8c16c8bbe 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1434,6 +1434,26 @@ class DomainInvitationAdmin(ListHeaderAdmin): # Get the filtered values return super().changelist_view(request, extra_context=extra_context) + def save_model(self, request, obj, form, change): + """ + Override the save_model method. + + On creation of a new domain invitation, attempt to retrieve the invitation, + which will be successful if a user exists for that email; otherwise, will + raise a RuntimeError, and in this case can continue to create the invitation. + """ + # NOTE: is a future ticket accounting for a 'not member of this org' scenario + # to mirror the logic in DomainAddUser view? + if not change: # Domain Invitation creation + try: + User.objects.get(email=obj.email) + obj.retrieve() + except User.DoesNotExist: + # Proceed with invitation as new as exception indicates user does not exist + pass + # Call the parent save method to save the object + super().save_model(request, obj, form, change) + class PortfolioInvitationAdmin(ListHeaderAdmin): """Custom portfolio invitation admin class.""" diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index dceb3a79e..c2a8b5e64 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -131,13 +131,11 @@ class TestDomainInvitationAdmin(TestCase): tests have available superuser, client, and admin """ - @classmethod - def setUpClass(cls): - cls.factory = RequestFactory() - cls.admin = ListHeaderAdmin(model=DomainInvitationAdmin, admin_site=AdminSite()) - cls.superuser = create_superuser() - def setUp(self): + self.factory = RequestFactory() + self.admin = ListHeaderAdmin(model=DomainInvitationAdmin, admin_site=AdminSite()) + self.superuser = create_superuser() + self.domain = Domain.objects.create(name="example.com") """Create a client object""" self.client = Client(HTTP_HOST="localhost:8080") @@ -145,9 +143,6 @@ class TestDomainInvitationAdmin(TestCase): """Delete all DomainInvitation objects""" DomainInvitation.objects.all().delete() Contact.objects.all().delete() - - @classmethod - def tearDownClass(self): User.objects.all().delete() @less_console_noise_decorator @@ -168,6 +163,7 @@ class TestDomainInvitationAdmin(TestCase): ) self.assertContains(response, "Show more") + @less_console_noise_decorator def test_get_filters(self): """Ensures that our filters are displaying correctly""" with less_console_noise(): @@ -192,6 +188,59 @@ class TestDomainInvitationAdmin(TestCase): self.assertContains(response, invited_html, count=1) self.assertContains(response, retrieved_html, count=1) + @less_console_noise_decorator + def test_save_model_user_exists(self): + """Test saving a domain invitation when the user exists. + + Should attempt to retrieve the domain invitation.""" + # Create a user with the same email + User.objects.create_user(email="test@example.com", password="password", username="username") + + # Create a domain invitation instance + invitation = DomainInvitation(email="test@example.com", domain=self.domain) + + admin_instance = DomainInvitationAdmin(DomainInvitation, admin_site=None) + + # Create a request object + request = self.factory.post("/admin/registrar/DomainInvitation/add/") + request.user = self.superuser + + # Patch the retrieve method + with patch.object(DomainInvitation, "retrieve") as mock_retrieve: + admin_instance.save_model(request, invitation, form=None, change=False) + + # Assert retrieve was called + mock_retrieve.assert_called_once() + + # Assert the invitation was saved + self.assertEqual(DomainInvitation.objects.count(), 1) + self.assertEqual(DomainInvitation.objects.first().email, "test@example.com") + + @less_console_noise_decorator + def test_save_model_user_does_not_exist(self): + """Test saving a domain invitation when the user does not exist. + + Should not attempt to retrieve the domain invitation.""" + # Create a domain invitation instance + invitation = DomainInvitation(email="nonexistent@example.com", domain=self.domain) + + admin_instance = DomainInvitationAdmin(DomainInvitation, admin_site=None) + + # Create a request object + request = self.factory.post("/admin/registrar/DomainInvitation/add/") + request.user = self.superuser + + # Patch the retrieve method to ensure it is not called + with patch.object(DomainInvitation, "retrieve") as mock_retrieve: + admin_instance.save_model(request, invitation, form=None, change=False) + + # Assert retrieve was not called + mock_retrieve.assert_not_called() + + # Assert the invitation was saved + self.assertEqual(DomainInvitation.objects.count(), 1) + self.assertEqual(DomainInvitation.objects.first().email, "nonexistent@example.com") + class TestUserPortfolioPermissionAdmin(TestCase): """Tests for the PortfolioInivtationAdmin class""" From cc1a8fd44a480c462af8e8f40e556f0af8cd6a1c Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 17:24:19 -0500 Subject: [PATCH 5/7] formatted tests for linter --- src/registrar/tests/test_admin.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index c2a8b5e64..3195f8237 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -191,10 +191,10 @@ class TestDomainInvitationAdmin(TestCase): @less_console_noise_decorator def test_save_model_user_exists(self): """Test saving a domain invitation when the user exists. - + Should attempt to retrieve the domain invitation.""" # Create a user with the same email - User.objects.create_user(email="test@example.com", password="password", username="username") + User.objects.create_user(email="test@example.com", username="username") # Create a domain invitation instance invitation = DomainInvitation(email="test@example.com", domain=self.domain) @@ -219,7 +219,7 @@ class TestDomainInvitationAdmin(TestCase): @less_console_noise_decorator def test_save_model_user_does_not_exist(self): """Test saving a domain invitation when the user does not exist. - + Should not attempt to retrieve the domain invitation.""" # Create a domain invitation instance invitation = DomainInvitation(email="nonexistent@example.com", domain=self.domain) From ab491d1507f8be970d979439e926ad2c9c9a3550 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 17:36:10 -0500 Subject: [PATCH 6/7] simplified conditions on save, accounting for multiple users with same email address --- src/registrar/admin.py | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8c16c8bbe..849cb6100 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1439,18 +1439,12 @@ class DomainInvitationAdmin(ListHeaderAdmin): Override the save_model method. On creation of a new domain invitation, attempt to retrieve the invitation, - which will be successful if a user exists for that email; otherwise, will - raise a RuntimeError, and in this case can continue to create the invitation. + which will be successful if a single User exists for that email; otherwise, will + just continue to create the invitation. """ - # NOTE: is a future ticket accounting for a 'not member of this org' scenario - # to mirror the logic in DomainAddUser view? - if not change: # Domain Invitation creation - try: - User.objects.get(email=obj.email) - obj.retrieve() - except User.DoesNotExist: - # Proceed with invitation as new as exception indicates user does not exist - pass + if not change and User.objects.filter(email=obj.email).count() == 1: + # Domain Invitation creation for an existing User + obj.retrieve() # Call the parent save method to save the object super().save_model(request, obj, form, change) From d71485eaaf98f9cc372e519e4df9a47fcfbb7982 Mon Sep 17 00:00:00 2001 From: Rachid Mrad <107004823+rachidatecs@users.noreply.github.com> Date: Wed, 8 Jan 2025 13:56:19 -0500 Subject: [PATCH 7/7] Update src/registrar/tests/test_admin_request.py Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com> --- src/registrar/tests/test_admin_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index eeb14380f..6b170e4cd 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1570,7 +1570,7 @@ class TestDomainRequestAdmin(MockEppLib): @less_console_noise_decorator def test_other_websites_has_few_readonly_links(self): """Tests if the readonly other_websites field has links. - Test markup for l5 or less websites.""" + Test markup for 5 or less websites.""" # Create a domain request with 4 current websites domain_request = completed_domain_request(