From 2651cd4abaebbe84c3ed762ce3bf99f28ab86122 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 2 Jan 2025 16:36:36 -0500 Subject: [PATCH 01/40] initial approach, from domain.py, not quite working --- src/registrar/forms/domain_request_wizard.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 289b3da0b..45bd575f9 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -296,6 +296,14 @@ class OrganizationContactForm(RegistrarForm): label="Urbanization (required for Puerto Rico only)", ) + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + # Set initial value for federal agency combo box and specify combobox template + if self.domain_request and self.domain_request.federal_agency: + self.fields["federal_agency"].initial = self.domain_request.federal_agency + self.fields["federal_agency"].widget.attrs["data-default-value"] = self.domain_request.federal_agency.pk + self.fields["federal_agency"].widget.template_name = "django/forms/widgets/combobox.html", + def clean_federal_agency(self): """Require something to be selected when this is a federal agency.""" federal_agency = self.cleaned_data.get("federal_agency", None) From f359a636d02103c0dd5421a5c25be4314fbc3a27 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 13:45:37 -0500 Subject: [PATCH 02/40] combobox in domain request organization contact --- src/registrar/forms/domain_request_wizard.py | 17 ++++++++++++++--- src/registrar/forms/utility/combobox.py | 5 +++++ 2 files changed, 19 insertions(+), 3 deletions(-) create mode 100644 src/registrar/forms/utility/combobox.py diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 45bd575f9..f58d62c0e 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -7,6 +7,7 @@ from django import forms from django.core.validators import RegexValidator, MaxLengthValidator from django.utils.safestring import mark_safe +from registrar.forms.utility.combobox import ComboboxWidget from registrar.forms.utility.wizard_form_helper import ( RegistrarForm, RegistrarFormSet, @@ -257,6 +258,7 @@ class OrganizationContactForm(RegistrarForm): required=False, queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), empty_label="--Select--", + widget=ComboboxWidget, ) organization_name = forms.CharField( label="Organization name", @@ -298,11 +300,20 @@ class OrganizationContactForm(RegistrarForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - # Set initial value for federal agency combo box and specify combobox template - if self.domain_request and self.domain_request.federal_agency: + + # Initialize federal_agency combobox widget + # Domain requests forms have prefix associated with step + prefix = kwargs.get("prefix", "") + prefixed_name = f"{prefix}-federal_agency" if prefix else "federal_agency" + + # For combobox widget, need to set the data-default-value to selected value + if self.is_bound and self.data.get(prefixed_name): + # If form is bound (from a POST), use submitted value + self.fields["federal_agency"].widget.attrs["data-default-value"] = self.data.get(prefixed_name) + elif self.domain_request and self.domain_request.federal_agency: + # If form is not bound, set initial self.fields["federal_agency"].initial = self.domain_request.federal_agency self.fields["federal_agency"].widget.attrs["data-default-value"] = self.domain_request.federal_agency.pk - self.fields["federal_agency"].widget.template_name = "django/forms/widgets/combobox.html", def clean_federal_agency(self): """Require something to be selected when this is a federal agency.""" diff --git a/src/registrar/forms/utility/combobox.py b/src/registrar/forms/utility/combobox.py new file mode 100644 index 000000000..b7db16ccc --- /dev/null +++ b/src/registrar/forms/utility/combobox.py @@ -0,0 +1,5 @@ +from django.forms import Select + + +class ComboboxWidget(Select): + template_name = "django/forms/widgets/combobox.html" \ No newline at end of file From 4480e3255375120f3ca6612c0f15309ebd57aa9a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 14:15:13 -0500 Subject: [PATCH 03/40] updated combobox widget to set proper data-default-value and set domain suborganization form to use new combobox widget --- src/registrar/forms/domain.py | 18 +++--------------- src/registrar/forms/domain_request_wizard.py | 17 ----------------- .../django/forms/widgets/combobox.html | 1 + 3 files changed, 4 insertions(+), 32 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index b43d91a58..1e4068125 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -4,6 +4,7 @@ import logging from django import forms from django.core.validators import MinValueValidator, MaxValueValidator, RegexValidator, MaxLengthValidator from django.forms import formset_factory +from registrar.forms.utility.combobox import ComboboxWidget from registrar.models import DomainRequest, FederalAgency from phonenumber_field.widgets import RegionalPhoneNumberWidget from registrar.models.suborganization import Suborganization @@ -161,9 +162,10 @@ class DomainSuborganizationForm(forms.ModelForm): """Form for updating the suborganization""" sub_organization = forms.ModelChoiceField( + label = "Suborganization name", queryset=Suborganization.objects.none(), required=False, - widget=forms.Select(), + widget=ComboboxWidget, ) class Meta: @@ -178,20 +180,6 @@ class DomainSuborganizationForm(forms.ModelForm): portfolio = self.instance.portfolio if self.instance else None self.fields["sub_organization"].queryset = Suborganization.objects.filter(portfolio=portfolio) - # Set initial value - if self.instance and self.instance.sub_organization: - self.fields["sub_organization"].initial = self.instance.sub_organization - - # Set custom form label - self.fields["sub_organization"].label = "Suborganization name" - - # Use the combobox rather than the regular select widget - self.fields["sub_organization"].widget.template_name = "django/forms/widgets/combobox.html" - - # Set data-default-value attribute - if self.instance and self.instance.sub_organization: - self.fields["sub_organization"].widget.attrs["data-default-value"] = self.instance.sub_organization.pk - class BaseNameserverFormset(forms.BaseFormSet): def clean(self): diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index f58d62c0e..e090ac8d2 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -298,23 +298,6 @@ class OrganizationContactForm(RegistrarForm): label="Urbanization (required for Puerto Rico only)", ) - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - - # Initialize federal_agency combobox widget - # Domain requests forms have prefix associated with step - prefix = kwargs.get("prefix", "") - prefixed_name = f"{prefix}-federal_agency" if prefix else "federal_agency" - - # For combobox widget, need to set the data-default-value to selected value - if self.is_bound and self.data.get(prefixed_name): - # If form is bound (from a POST), use submitted value - self.fields["federal_agency"].widget.attrs["data-default-value"] = self.data.get(prefixed_name) - elif self.domain_request and self.domain_request.federal_agency: - # If form is not bound, set initial - self.fields["federal_agency"].initial = self.domain_request.federal_agency - self.fields["federal_agency"].widget.attrs["data-default-value"] = self.domain_request.federal_agency.pk - def clean_federal_agency(self): """Require something to be selected when this is a federal agency.""" federal_agency = self.cleaned_data.get("federal_agency", None) diff --git a/src/registrar/templates/django/forms/widgets/combobox.html b/src/registrar/templates/django/forms/widgets/combobox.html index 7ff31945b..4fe796347 100644 --- a/src/registrar/templates/django/forms/widgets/combobox.html +++ b/src/registrar/templates/django/forms/widgets/combobox.html @@ -11,6 +11,7 @@ for now we just carry the attribute to both the parent element and the select. {{ name }}="{{ value }}" {% endif %} {% endfor %} +data-default-value="{% for group_name, group_choices, group_index in widget.optgroups %}{% for option in group_choices %}{% if option.selected %}{{ option.value }}{% endif %}{% endfor %}{% endfor %}" > {% include "django/forms/widgets/select.html" %} From 3146dc07fadd3d9b92e5932b7964f57943cb271a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 14:22:36 -0500 Subject: [PATCH 04/40] applied widget to state territory --- src/registrar/forms/domain_request_wizard.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index e090ac8d2..fcba68de8 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -282,6 +282,7 @@ class OrganizationContactForm(RegistrarForm): error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, + widget=ComboboxWidget, ) zipcode = forms.CharField( label="Zip code", From 560baed2e1e2eb95aa48364b2e3673084814a741 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 6 Jan 2025 14:39:44 -0500 Subject: [PATCH 05/40] linted --- src/registrar/forms/domain.py | 2 +- src/registrar/forms/utility/combobox.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 1e4068125..87a52d142 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -162,7 +162,7 @@ class DomainSuborganizationForm(forms.ModelForm): """Form for updating the suborganization""" sub_organization = forms.ModelChoiceField( - label = "Suborganization name", + label="Suborganization name", queryset=Suborganization.objects.none(), required=False, widget=ComboboxWidget, diff --git a/src/registrar/forms/utility/combobox.py b/src/registrar/forms/utility/combobox.py index b7db16ccc..277aec4f3 100644 --- a/src/registrar/forms/utility/combobox.py +++ b/src/registrar/forms/utility/combobox.py @@ -2,4 +2,4 @@ from django.forms import Select class ComboboxWidget(Select): - template_name = "django/forms/widgets/combobox.html" \ No newline at end of file + template_name = "django/forms/widgets/combobox.html" From bb5f61fd989d3ac6588a1f7a4e30f01ac99556fe Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 10 Jan 2025 06:50:36 -0500 Subject: [PATCH 06/40] updated behavior of combobox, added state territory combobox --- src/registrar/forms/domain_request_wizard.py | 1 + src/registrar/templates/django/forms/widgets/select.html | 1 + 2 files changed, 2 insertions(+) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index fcba68de8..c38eb01a2 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -57,6 +57,7 @@ class RequestingEntityForm(RegistrarForm): label="State, territory, or military post", required=False, choices=[("", "--Select--")] + DomainRequest.StateTerritoryChoices.choices, + widget=ComboboxWidget, ) def __init__(self, *args, **kwargs): diff --git a/src/registrar/templates/django/forms/widgets/select.html b/src/registrar/templates/django/forms/widgets/select.html index cc62eb91d..86ca4ddcc 100644 --- a/src/registrar/templates/django/forms/widgets/select.html +++ b/src/registrar/templates/django/forms/widgets/select.html @@ -3,6 +3,7 @@ {# hint: spacing in the class string matters #} class="usa-select{% if classes %} {{ classes }}{% endif %}" {% include "django/forms/widgets/attrs.html" %} + data-default-value="{% for group_name, group_choices, group_index in widget.optgroups %}{% for option in group_choices %}{% if option.selected %}{{ option.value }}{% endif %}{% endfor %}{% endfor %}" > {% for group, options, index in widget.optgroups %} {% if group %}{% endif %} From 0e2d62f86f6cee2d63f0813b59f4a4852ce4d729 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 10 Jan 2025 06:59:26 -0500 Subject: [PATCH 07/40] small improvement --- src/registrar/templates/django/forms/widgets/combobox.html | 2 +- src/registrar/templates/django/forms/widgets/select.html | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/django/forms/widgets/combobox.html b/src/registrar/templates/django/forms/widgets/combobox.html index 4fe796347..02cd4e35e 100644 --- a/src/registrar/templates/django/forms/widgets/combobox.html +++ b/src/registrar/templates/django/forms/widgets/combobox.html @@ -13,5 +13,5 @@ for now we just carry the attribute to both the parent element and the select. {% endfor %} data-default-value="{% for group_name, group_choices, group_index in widget.optgroups %}{% for option in group_choices %}{% if option.selected %}{{ option.value }}{% endif %}{% endfor %}{% endfor %}" > - {% include "django/forms/widgets/select.html" %} + {% include "django/forms/widgets/select.html" with is_combobox=True %} diff --git a/src/registrar/templates/django/forms/widgets/select.html b/src/registrar/templates/django/forms/widgets/select.html index 86ca4ddcc..db6deafe2 100644 --- a/src/registrar/templates/django/forms/widgets/select.html +++ b/src/registrar/templates/django/forms/widgets/select.html @@ -3,7 +3,9 @@ {# hint: spacing in the class string matters #} class="usa-select{% if classes %} {{ classes }}{% endif %}" {% include "django/forms/widgets/attrs.html" %} - data-default-value="{% for group_name, group_choices, group_index in widget.optgroups %}{% for option in group_choices %}{% if option.selected %}{{ option.value }}{% endif %}{% endfor %}{% endfor %}" + {% if is_combobox %} + data-default-value="{% for group_name, group_choices, group_index in widget.optgroups %}{% for option in group_choices %}{% if option.selected %}{{ option.value }}{% endif %}{% endfor %}{% endfor %}" + {% endif %} > {% for group, options, index in widget.optgroups %} {% if group %}{% endif %} From 6283cad872ef79472ba6dcf60e33cc1479fa90e7 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 10 Jan 2025 07:22:38 -0500 Subject: [PATCH 08/40] state territory in org name address domain form --- src/registrar/forms/domain.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 87a52d142..6cf6416c1 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -457,6 +457,17 @@ class DomainOrgNameAddressForm(forms.ModelForm): }, ) + state_territory = forms.ChoiceField( + label="State, territory, or military post", + required=True, + choices=DomainInformation.StateTerritoryChoices.choices, + widget=ComboboxWidget( + attrs={ + "required": True, + } + ), + ) + class Meta: model = DomainInformation fields = [ @@ -479,20 +490,10 @@ class DomainOrgNameAddressForm(forms.ModelForm): }, } widgets = { - # We need to set the required attributed for State/territory - # because for this fields we are creating an individual - # instance of the Select. For the other fields we use the for loop to set - # the class's required attribute to true. "organization_name": forms.TextInput, "address_line1": forms.TextInput, "address_line2": forms.TextInput, "city": forms.TextInput, - "state_territory": forms.Select( - attrs={ - "required": True, - }, - choices=DomainInformation.StateTerritoryChoices.choices, - ), "urbanization": forms.TextInput, } From d41174547d7ca5fd11bc0929103a58bb40c137bc Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 10 Jan 2025 07:35:44 -0500 Subject: [PATCH 09/40] fixing test_export again --- src/registrar/tests/test_reports.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 4a41238c7..93c07df13 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -900,6 +900,7 @@ class MemberExportTest(MockDbForIndividualTests, MockEppLib): # spaces and leading/trailing whitespace csv_content = csv_content.replace(",,", "").replace(",", "").replace(" ", "").replace("\r\n", "\n").strip() expected_content = expected_content.replace(",,", "").replace(",", "").replace(" ", "").strip() + self.maxDiff=None self.assertEqual(csv_content, expected_content) From b4505b38630eb2df71d33d2c50bcc960995cc790 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 10 Jan 2025 07:38:56 -0500 Subject: [PATCH 10/40] lint --- src/registrar/tests/test_reports.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 93c07df13..b11500ea9 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -900,7 +900,7 @@ class MemberExportTest(MockDbForIndividualTests, MockEppLib): # spaces and leading/trailing whitespace csv_content = csv_content.replace(",,", "").replace(",", "").replace(" ", "").replace("\r\n", "\n").strip() expected_content = expected_content.replace(",,", "").replace(",", "").replace(" ", "").strip() - self.maxDiff=None + self.maxDiff = None self.assertEqual(csv_content, expected_content) From ae6c461ddb33f67c0b7bea042a4ca6594e30e669 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 13 Jan 2025 08:00:02 -0500 Subject: [PATCH 11/40] added federal agency on domain page, and suborg on requesting entity page --- src/registrar/forms/domain.py | 9 +++++++++ src/registrar/forms/domain_request_wizard.py | 1 + 2 files changed, 10 insertions(+) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 6cf6416c1..dd8c719cd 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -444,6 +444,15 @@ class DomainSecurityEmailForm(forms.Form): class DomainOrgNameAddressForm(forms.ModelForm): """Form for updating the organization name and mailing address.""" + # for federal agencies we also want to know the top-level agency. + excluded_agencies = ["gov Administration", "Non-Federal Agency"] + federal_agency = forms.ModelChoiceField( + label="Federal agency", + required=False, + queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), + empty_label="--Select--", + widget=ComboboxWidget, + ) zipcode = forms.CharField( label="Zip code", validators=[ diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 898a02d58..2365d323d 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -45,6 +45,7 @@ class RequestingEntityForm(RegistrarForm): required=False, queryset=Suborganization.objects.none(), empty_label="--Select--", + widget=ComboboxWidget, ) requested_suborganization = forms.CharField( label="Requested suborganization", From d54e57b0840b01d0760669e143916ebe522e5a91 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 13 Jan 2025 08:15:50 -0500 Subject: [PATCH 12/40] update federal agency drop down not to have any exclusions --- src/registrar/forms/domain.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index dd8c719cd..7089409de 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -445,11 +445,10 @@ class DomainOrgNameAddressForm(forms.ModelForm): """Form for updating the organization name and mailing address.""" # for federal agencies we also want to know the top-level agency. - excluded_agencies = ["gov Administration", "Non-Federal Agency"] federal_agency = forms.ModelChoiceField( label="Federal agency", required=False, - queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), + queryset=FederalAgency.objects.all(), empty_label="--Select--", widget=ComboboxWidget, ) From 828c47de4528fc7bec86639087f91cfe628e835a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:06:44 -0700 Subject: [PATCH 13/40] Better table + simplify get_or_create function --- src/registrar/admin.py | 3 ++ src/registrar/models/domain.py | 68 ++++++++++++++-------------------- 2 files changed, 30 insertions(+), 41 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 849cb6100..ed0b776ce 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3835,6 +3835,9 @@ class PublicContactAdmin(ListHeaderAdmin, ImportExportModelAdmin): change_form_template = "django/admin/email_clipboard_change_form.html" autocomplete_fields = ["domain"] + list_display = ("domain", "email", "name", "contact_type", "id") + search_fields = ["email", "name", "id"] + search_help_text = "Search by email, name or id." def changeform_view(self, request, object_id=None, form_url="", extra_context=None): if extra_context is None: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6bd8278a1..cb912cb37 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -4,9 +4,9 @@ import ipaddress import re from datetime import date, timedelta from typing import Optional +from django.db import transaction from django_fsm import FSMField, transition, TransitionNotAllowed # type: ignore - -from django.db import models +from django.db import models, IntegrityError from django.utils import timezone from typing import Any from registrar.models.host import Host @@ -2077,49 +2077,35 @@ class Domain(TimeStampedModel, DomainHelper): def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" - db_contact = PublicContact.objects.filter( - registry_id=public_contact.registry_id, - contact_type=public_contact.contact_type, - domain=self, - ) - - # If we find duplicates, log it and delete the oldest ones. - if db_contact.count() > 1: - logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.") - - newest_duplicate = db_contact.order_by("-created_at").first() - - duplicates_to_delete = db_contact.exclude(id=newest_duplicate.id) # type: ignore - - # Delete all duplicates - duplicates_to_delete.delete() - - # Do a second filter to grab the latest content - db_contact = PublicContact.objects.filter( + try: + with transaction.atomic(): + contact, _ = PublicContact.objects.get_or_create( + registry_id=public_contact.registry_id, + contact_type=public_contact.contact_type, + domain=self, + defaults={ + "email": public_contact.email, + "voice": public_contact.voice, + "fax": public_contact.fax, + "name": public_contact.name, + "org": public_contact.org, + "pw": public_contact.pw, + "city": public_contact.city, + "pc": public_contact.pc, + "cc": public_contact.cc, + "sp": public_contact.sp, + "street1": public_contact.street1, + "street2": public_contact.street2, + "street3": public_contact.street3, + } + ) + except IntegrityError: + contact = PublicContact.objects.get( registry_id=public_contact.registry_id, contact_type=public_contact.contact_type, domain=self, ) - - # Save to DB if it doesn't exist already. - if db_contact.count() == 0: - # Doesn't run custom save logic, just saves to DB - public_contact.save(skip_epp_save=True) - logger.info(f"Created a new PublicContact: {public_contact}") - # Append the item we just created - return public_contact - - existing_contact = db_contact.get() - - # Does the item we're grabbing match what we have in our DB? - if existing_contact.email != public_contact.email or existing_contact.registry_id != public_contact.registry_id: - existing_contact.delete() - public_contact.save() - logger.warning("Requested PublicContact is out of sync " "with DB.") - return public_contact - - # If it already exists, we can assume that the DB instance was updated during set, so we should just use that. - return existing_contact + return contact def _registrant_to_public_contact(self, registry_id: str): """EPPLib returns the registrant as a string, From e2486c127da6627b4bdcdac15bfeeaf81597d687 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 14 Jan 2025 08:42:22 -0500 Subject: [PATCH 14/40] organization edit form - added state territory combobox --- src/registrar/forms/portfolio.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 0a8c4d623..4e2e7bdf1 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -6,6 +6,7 @@ from django.core.validators import RegexValidator from django.core.validators import MaxLengthValidator from django.utils.safestring import mark_safe +from registrar.forms.utility.combobox import ComboboxWidget from registrar.models import ( PortfolioInvitation, UserPortfolioPermission, @@ -33,6 +34,12 @@ class PortfolioOrgAddressForm(forms.ModelForm): "required": "Enter a 5-digit or 9-digit zip code, like 12345 or 12345-6789.", }, ) + state_territory = forms.ChoiceField( + label="State, territory, or military post", + required=True, + choices=DomainInformation.StateTerritoryChoices.choices, + widget=ComboboxWidget, + ) class Meta: model = Portfolio @@ -53,19 +60,9 @@ class PortfolioOrgAddressForm(forms.ModelForm): "zipcode": {"required": "Enter a 5-digit or 9-digit zip code, like 12345 or 12345-6789."}, } widgets = { - # We need to set the required attributed for State/territory - # because for this fields we are creating an individual - # instance of the Select. For the other fields we use the for loop to set - # the class's required attribute to true. "address_line1": forms.TextInput, "address_line2": forms.TextInput, "city": forms.TextInput, - "state_territory": forms.Select( - attrs={ - "required": True, - }, - choices=DomainInformation.StateTerritoryChoices.choices, - ), # "urbanization": forms.TextInput, } From c0c9817c99459a0ecfef76a59fe12a9ea9483bfa Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 14 Jan 2025 10:33:00 -0700 Subject: [PATCH 15/40] Update admin.py --- src/registrar/admin.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index ed0b776ce..8773f7ef8 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3835,9 +3835,9 @@ class PublicContactAdmin(ListHeaderAdmin, ImportExportModelAdmin): change_form_template = "django/admin/email_clipboard_change_form.html" autocomplete_fields = ["domain"] - list_display = ("domain", "email", "name", "contact_type", "id") - search_fields = ["email", "name", "id"] - search_help_text = "Search by email, name or id." + list_display = ("name", "contact_type", "email", "domain", "registry_id") + search_fields = ["email", "name", "registry_id"] + search_help_text = "Search by email, name or registry id." def changeform_view(self, request, object_id=None, form_url="", extra_context=None): if extra_context is None: From f86a065a607e1bbc0d3705193559ded32891d3d3 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 14 Jan 2025 15:04:56 -0700 Subject: [PATCH 16/40] Update domain.py --- src/registrar/models/domain.py | 85 +++++++++++++++++++++++----------- 1 file changed, 57 insertions(+), 28 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cb912cb37..5e186efff 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1329,7 +1329,7 @@ class Domain(TimeStampedModel, DomainHelper): def get_default_administrative_contact(self): """Gets the default administrative contact.""" - logger.info("get_default_security_contact() -> Adding administrative security contact") + logger.info("get_default_administrative_contact() -> Adding administrative security contact") contact = PublicContact.get_default_administrative() contact.domain = self return contact @@ -1848,7 +1848,6 @@ class Domain(TimeStampedModel, DomainHelper): """ try: self._add_missing_contacts_if_unknown(cleaned) - except Exception as e: logger.error( "%s couldn't _add_missing_contacts_if_unknown, error was %s." @@ -1866,7 +1865,6 @@ class Domain(TimeStampedModel, DomainHelper): is in an UNKNOWN state, that is an error state) Note: The transition state change happens at the end of the function """ - missingAdmin = True missingSecurity = True missingTech = True @@ -1890,6 +1888,11 @@ class Domain(TimeStampedModel, DomainHelper): if missingTech: technical_contact = self.get_default_technical_contact() technical_contact.save() + + logger.info( + "_add_missing_contacts_if_unknown => " + f"missingAdmin: {missingAdmin}, missingSecurity: {missingSecurity}, missingTech: {missingTech}" + ) def _fetch_cache(self, fetch_hosts=False, fetch_contacts=False): """Contact registry for info about a domain.""" @@ -2077,35 +2080,61 @@ class Domain(TimeStampedModel, DomainHelper): def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" - try: - with transaction.atomic(): - contact, _ = PublicContact.objects.get_or_create( - registry_id=public_contact.registry_id, - contact_type=public_contact.contact_type, - domain=self, - defaults={ - "email": public_contact.email, - "voice": public_contact.voice, - "fax": public_contact.fax, - "name": public_contact.name, - "org": public_contact.org, - "pw": public_contact.pw, - "city": public_contact.city, - "pc": public_contact.pc, - "cc": public_contact.cc, - "sp": public_contact.sp, - "street1": public_contact.street1, - "street2": public_contact.street2, - "street3": public_contact.street3, - } - ) - except IntegrityError: - contact = PublicContact.objects.get( + db_contact = PublicContact.objects.filter( + registry_id=public_contact.registry_id, + contact_type=public_contact.contact_type, + domain=self, + ) + + # If we find duplicates, log it and delete the oldest ones. + if db_contact.count() > 1: + logger.warning("_get_or_create_public_contact() -> Duplicate contacts found. Deleting duplicate.") + + newest_duplicate = db_contact.order_by("-created_at").first() + + duplicates_to_delete = db_contact.exclude(id=newest_duplicate.id) # type: ignore + + # Delete all duplicates + duplicates_to_delete.delete() + + # Do a second filter to grab the latest content + db_contact = PublicContact.objects.filter( registry_id=public_contact.registry_id, contact_type=public_contact.contact_type, domain=self, ) - return contact + + # Save to DB if it doesn't exist already. + if db_contact.count() == 0: + # Doesn't run custom save logic, just saves to DB + try: + public_contact.save(skip_epp_save=True) + logger.info(f"Created a new PublicContact: {public_contact}") + except IntegrityError as err: + logger.error( + "_get_or_create_public_contact() => tried to create a duplicate public contact: " + f"{err}", exc_info=True + ) + return PublicContact.objects.get( + registry_id=public_contact.registry_id, + contact_type=public_contact.contact_type, + domain=self, + ) + + # Append the item we just created + return public_contact + + existing_contact = db_contact.get() + + # Does the item we're grabbing match what we have in our DB? + if existing_contact.email != public_contact.email or existing_contact.registry_id != public_contact.registry_id: + existing_contact.delete() + public_contact.save() + logger.warning("Requested PublicContact is out of sync " "with DB.") + return public_contact + + # If it already exists, we can assume that the DB instance was updated during set, so we should just use that. + return existing_contact def _registrant_to_public_contact(self, registry_id: str): """EPPLib returns the registrant as a string, From ea509f63efc2880052900ab80b587106200b453b Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 14 Jan 2025 18:03:24 -0500 Subject: [PATCH 17/40] uswds edits to combobox, default values for comboboxes, cleanup of combobox js --- src/registrar/assets/js/uswds-edited.js | 14 ++++++-- .../assets/src/js/getgov/combobox.js | 36 ------------------- src/registrar/forms/domain.py | 15 ++++---- src/registrar/forms/portfolio.py | 8 ++--- 4 files changed, 21 insertions(+), 52 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index f59417b41..b597f2d2b 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -29,6 +29,8 @@ * - tooltip dynamic content updated to include nested element (for better sizing control) * - modal exposed to window to be accessible in other js files * - fixed bug in createHeaderButton which added newlines to header button tooltips + * - modified combobox to allow for blank values in list + * - modified aria label for X button in combobox to reflect modified behavior of button */ if ("document" in window.self) { @@ -1218,9 +1220,11 @@ const enhanceComboBox = _comboBoxEl => { input.setAttribute(key, value); })); comboBoxEl.insertAdjacentElement("beforeend", input); + // DOTGOV - modified the aria-label of the clear input button to Reset selection to reflect changed button behavior + // comboBoxEl.insertAdjacentHTML("beforeend", Sanitizer.escapeHTML` - +   @@ -1356,8 +1360,12 @@ const displayList = el => { for (let i = 0, len = selectEl.options.length; i < len; i += 1) { const optionEl = selectEl.options[i]; const optionId = `${listOptionBaseId}${options.length}`; - if (optionEl.value && (disableFiltering || isPristine || !inputValue || regex.test(optionEl.text))) { - if (selectEl.value && optionEl.value === selectEl.value) { + // DOTGOV: modified combobox to allow for options with blank value + //if (optionEl.value && (disableFiltering || isPristine || !inputValue || regex.test(optionEl.text))) { + if ((disableFiltering || isPristine || !inputValue || regex.test(optionEl.text))) { + // DOTGOV: modified combobox to allow blank option value selections to be considered selected + //if (selectEl.value && optionEl.value === selectEl.value) { + if (selectEl.value && optionEl.value === selectEl.value || (!selectEl.value && !optionEl.value)) { selectedItemId = optionId; } if (disableFiltering && !firstFoundId && regex.test(optionEl.text)) { diff --git a/src/registrar/assets/src/js/getgov/combobox.js b/src/registrar/assets/src/js/getgov/combobox.js index 36b7aa0ad..95c5ff7e8 100644 --- a/src/registrar/assets/src/js/getgov/combobox.js +++ b/src/registrar/assets/src/js/getgov/combobox.js @@ -28,19 +28,6 @@ export function loadInitialValuesForComboBoxes() { // Override the default clear button behavior such that it no longer clears the input, // it just resets to the data-initial-value. // Due to the nature of how uswds works, this is slightly hacky. - // Use a MutationObserver to watch for changes in the dropdown list - const dropdownList = comboBox.querySelector(`#${input.id}--list`); - const observer = new MutationObserver(function(mutations) { - mutations.forEach(function(mutation) { - if (mutation.type === "childList") { - addBlankOption(clearInputButton, dropdownList, initialValue); - } - }); - }); - - // Configure the observer to watch for changes in the dropdown list - const config = { childList: true, subtree: true }; - observer.observe(dropdownList, config); // Input event listener to detect typing input.addEventListener("input", () => { @@ -87,27 +74,4 @@ export function loadInitialValuesForComboBoxes() { showElement(clearInputButton) } } - - function addBlankOption(clearInputButton, dropdownList, initialValue) { - if (dropdownList && !dropdownList.querySelector('[data-value=""]') && !isTyping) { - const blankOption = document.createElement("li"); - blankOption.setAttribute("role", "option"); - blankOption.setAttribute("data-value", ""); - blankOption.classList.add("usa-combo-box__list-option"); - if (!initialValue){ - blankOption.classList.add("usa-combo-box__list-option--selected") - } - blankOption.textContent = "⎯"; - - dropdownList.insertBefore(blankOption, dropdownList.firstChild); - blankOption.addEventListener("click", (e) => { - e.preventDefault(); - e.stopPropagation(); - overrideDefaultClearButton = false; - // Trigger the default clear behavior - clearInputButton.click(); - overrideDefaultClearButton = true; - }); - } - } } diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 7089409de..f577ed0ab 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -164,6 +164,7 @@ class DomainSuborganizationForm(forms.ModelForm): sub_organization = forms.ModelChoiceField( label="Suborganization name", queryset=Suborganization.objects.none(), + empty_label="⎯ (No suborganization)", required=False, widget=ComboboxWidget, ) @@ -468,12 +469,11 @@ class DomainOrgNameAddressForm(forms.ModelForm): state_territory = forms.ChoiceField( label="State, territory, or military post", required=True, - choices=DomainInformation.StateTerritoryChoices.choices, - widget=ComboboxWidget( - attrs={ - "required": True, - } - ), + choices=[("", "--Select--")] + DomainInformation.StateTerritoryChoices.choices, + error_messages={ + "required": ("Select the state, territory, or military post where your organization is located.") + }, + widget=ComboboxWidget(), ) class Meta: @@ -493,9 +493,6 @@ class DomainOrgNameAddressForm(forms.ModelForm): "organization_name": {"required": "Enter the name of your organization."}, "address_line1": {"required": "Enter the street address of your organization."}, "city": {"required": "Enter the city where your organization is located."}, - "state_territory": { - "required": "Select the state, territory, or military post where your organization is located." - }, } widgets = { "organization_name": forms.TextInput, diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 4e2e7bdf1..13d956fe4 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -37,7 +37,10 @@ class PortfolioOrgAddressForm(forms.ModelForm): state_territory = forms.ChoiceField( label="State, territory, or military post", required=True, - choices=DomainInformation.StateTerritoryChoices.choices, + choices=[("", "--Select--")] + DomainInformation.StateTerritoryChoices.choices, + error_messages={ + "required": ("Select the state, territory, or military post where your organization is located.") + }, widget=ComboboxWidget, ) @@ -54,9 +57,6 @@ class PortfolioOrgAddressForm(forms.ModelForm): error_messages = { "address_line1": {"required": "Enter the street address of your organization."}, "city": {"required": "Enter the city where your organization is located."}, - "state_territory": { - "required": "Select the state, territory, or military post where your organization is located." - }, "zipcode": {"required": "Enter a 5-digit or 9-digit zip code, like 12345 or 12345-6789."}, } widgets = { From 1ca52b49bbe719f4c4b1cf101df1f20ce038f06e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 14 Jan 2025 19:23:29 -0500 Subject: [PATCH 18/40] additional edits to uswds and to combobox.js to accomodate for empty values in options --- src/registrar/assets/js/uswds-edited.js | 53 +++++++++++++------ .../assets/src/js/getgov/combobox.js | 2 +- 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index b597f2d2b..590033a87 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -1037,7 +1037,7 @@ const noop = () => {}; * @param {string} value The new value of the element */ const changeElementValue = function (el) { - let value = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : ""; + let value = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : ""; const elementToChange = el; elementToChange.value = value; const event = new CustomEvent("change", { @@ -1167,13 +1167,21 @@ const enhanceComboBox = _comboBoxEl => { placeholder }); } - if (defaultValue) { - for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - const optionEl = selectEl.options[i]; - if (optionEl.value === defaultValue) { - selectedOption = optionEl; - break; - } + // DOTGOV - allowing for defaultValue to be empty + //if (defaultValue) { + // for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + // const optionEl = selectEl.options[i]; + // if (optionEl.value === defaultValue) { + // selectedOption = optionEl; + // break; + // } + // } + //} + for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + const optionEl = selectEl.options[i]; + if ((optionEl.value === defaultValue) || (!optionEl.value && !defaultValue)) { + selectedOption = optionEl; + break; } } @@ -1500,16 +1508,27 @@ const resetSelection = el => { } = getComboBoxContext(el); const selectValue = selectEl.value; const inputValue = (inputEl.value || "").toLowerCase(); - if (selectValue) { - for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - const optionEl = selectEl.options[i]; - if (optionEl.value === selectValue) { - if (inputValue !== optionEl.text) { - changeElementValue(inputEl, optionEl.text); - } - comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); - return; + // DOTGOV - allow for option value to be empty string + //if (selectValue) { + // for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + // const optionEl = selectEl.options[i]; + // if (optionEl.value === selectValue) { + // if (inputValue !== optionEl.text) { + // changeElementValue(inputEl, optionEl.text); + // } + // comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); + // return; + // } + // } + //} + for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + const optionEl = selectEl.options[i]; + if ((!selectValue && !optionEl.value) || optionEl.value === selectValue) { + if (inputValue !== optionEl.text) { + changeElementValue(inputEl, optionEl.text); } + comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); + return; } } if (inputValue) { diff --git a/src/registrar/assets/src/js/getgov/combobox.js b/src/registrar/assets/src/js/getgov/combobox.js index 95c5ff7e8..e0ecc92ad 100644 --- a/src/registrar/assets/src/js/getgov/combobox.js +++ b/src/registrar/assets/src/js/getgov/combobox.js @@ -48,7 +48,7 @@ export function loadInitialValuesForComboBoxes() { // Change the default input behaviour - have it reset to the data default instead clearInputButton.addEventListener("click", (e) => { - if (overrideDefaultClearButton && initialValue) { + if (overrideDefaultClearButton) { e.preventDefault(); e.stopPropagation(); input.click(); From 24e0243df3004f91ca2f99722ff90f1385ee4180 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 14 Jan 2025 20:03:58 -0500 Subject: [PATCH 19/40] fixed bug with portfolio requesting entity --- src/registrar/assets/src/js/getgov/requesting-entity.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/requesting-entity.js b/src/registrar/assets/src/js/getgov/requesting-entity.js index 3bcdcd35c..e784419b4 100644 --- a/src/registrar/assets/src/js/getgov/requesting-entity.js +++ b/src/registrar/assets/src/js/getgov/requesting-entity.js @@ -9,15 +9,16 @@ export function handleRequestingEntityFieldset() { const formPrefix = "portfolio_requesting_entity"; const radioFieldset = document.getElementById(`id_${formPrefix}-requesting_entity_is_suborganization__fieldset`); const radios = radioFieldset?.querySelectorAll(`input[name="${formPrefix}-requesting_entity_is_suborganization"]`); - const select = document.getElementById(`id_${formPrefix}-sub_organization`); - const selectParent = select?.parentElement; + const input = document.getElementById(`id_${formPrefix}-sub_organization`); + const inputGrandParent = input?.parentElement?.parentElement; + const select = input?.previousElementSibling; const suborgContainer = document.getElementById("suborganization-container"); const suborgDetailsContainer = document.getElementById("suborganization-container__details"); const suborgAddtlInstruction = document.getElementById("suborganization-addtl-instruction"); const subOrgCreateNewOption = document.getElementById("option-to-add-suborg")?.value; // Make sure all crucial page elements exist before proceeding. // This more or less ensures that we are on the Requesting Entity page, and not elsewhere. - if (!radios || !select || !selectParent || !suborgContainer || !suborgDetailsContainer) return; + if (!radios || !input || !select || !inputGrandParent || !suborgContainer || !suborgDetailsContainer) return; // requestingSuborganization: This just broadly determines if they're requesting a suborg at all // requestingNewSuborganization: This variable determines if the user is trying to *create* a new suborganization or not. @@ -28,7 +29,7 @@ export function handleRequestingEntityFieldset() { if (radio != null) requestingSuborganization = radio?.checked && radio.value === "True"; requestingSuborganization ? showElement(suborgContainer) : hideElement(suborgContainer); if (select.options.length == 2) { // --Select-- and other are the only options - hideElement(selectParent); // Hide the select drop down and indicate requesting new suborg + hideElement(inputGrandParent); // Hide the combo box and indicate requesting new suborg hideElement(suborgAddtlInstruction); // Hide additional instruction related to the list requestingNewSuborganization.value = "True"; } else { From fabeddaa619a23192c517f831fd80a72c84e066f Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Tue, 14 Jan 2025 20:56:45 -0500 Subject: [PATCH 20/40] modified combobox to handle error class --- src/registrar/assets/js/uswds-edited.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 590033a87..60502050f 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -31,6 +31,7 @@ * - fixed bug in createHeaderButton which added newlines to header button tooltips * - modified combobox to allow for blank values in list * - modified aria label for X button in combobox to reflect modified behavior of button + * - modified combobox to handle error class */ if ("document" in window.self) { @@ -1223,6 +1224,11 @@ const enhanceComboBox = _comboBoxEl => { input.setAttribute("class", INPUT_CLASS); input.setAttribute("type", "text"); input.setAttribute("role", "combobox"); + // DOTGOV - handle error class for combobox + // Check if 'usa-input--error' exists in selectEl and add it to input if true + if (selectEl.classList.contains('usa-input--error')) { + input.classList.add('usa-input--error'); + } additionalAttributes.forEach(attr => Object.keys(attr).forEach(key => { const value = Sanitizer.escapeHTML`${attr[key]}`; input.setAttribute(key, value); From 74ef30b52d222171daa01c1681bdc42c797ace92 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 15 Jan 2025 12:44:47 -0500 Subject: [PATCH 21/40] fixed the 'other' problem in requesting entity form --- .../assets/src/js/getgov/requesting-entity.js | 5 ----- src/registrar/forms/domain_request_wizard.py | 14 ++++++++++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/requesting-entity.js b/src/registrar/assets/src/js/getgov/requesting-entity.js index e784419b4..5f3be8c79 100644 --- a/src/registrar/assets/src/js/getgov/requesting-entity.js +++ b/src/registrar/assets/src/js/getgov/requesting-entity.js @@ -38,11 +38,6 @@ export function handleRequestingEntityFieldset() { requestingNewSuborganization.value === "True" ? showElement(suborgDetailsContainer) : hideElement(suborgDetailsContainer); } - // Add fake "other" option to sub_organization select - if (select && !Array.from(select.options).some(option => option.value === "other")) { - select.add(new Option(subOrgCreateNewOption, "other")); - } - if (requestingNewSuborganization.value === "True") { select.value = "other"; } diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 2365d323d..636a41760 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -63,13 +63,19 @@ class RequestingEntityForm(RegistrarForm): ) def __init__(self, *args, **kwargs): - """Override of init to add the suborganization queryset""" + """Override of init to add the suborganization queryset and 'other' option""" super().__init__(*args, **kwargs) if self.domain_request.portfolio: - self.fields["sub_organization"].queryset = Suborganization.objects.filter( - portfolio=self.domain_request.portfolio - ) + # Fetch the queryset for the portfolio + queryset = Suborganization.objects.filter(portfolio=self.domain_request.portfolio) + # set the queryset appropriately so that post can validate against queryset + self.fields["sub_organization"].queryset = queryset + + # Modify the choices to include "other" so that form can display options properly + self.fields["sub_organization"].choices = [("", "--Select--")] + [ + (obj.id, str(obj)) for obj in queryset + ] + [("other", "Other (enter your suborganization manually)")] def clean_sub_organization(self): """On suborganization clean, set the suborganization value to None if the user is requesting From 6a49f9e373a6db7f99a50f402cb4938ab6c7597a Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 16 Jan 2025 14:13:51 -0500 Subject: [PATCH 22/40] requesting entity working - code still wip --- src/registrar/assets/js/uswds-edited.js | 71 +++------ src/registrar/assets/src/js/getgov/main.js | 2 +- src/registrar/forms/domain_request_wizard.py | 150 ++++++++++++++++--- src/registrar/views/domain_request.py | 10 +- 4 files changed, 163 insertions(+), 70 deletions(-) diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 60502050f..9e3922c9c 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -1038,7 +1038,7 @@ const noop = () => {}; * @param {string} value The new value of the element */ const changeElementValue = function (el) { - let value = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : ""; + let value = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : ""; const elementToChange = el; elementToChange.value = value; const event = new CustomEvent("change", { @@ -1168,22 +1168,14 @@ const enhanceComboBox = _comboBoxEl => { placeholder }); } - // DOTGOV - allowing for defaultValue to be empty - //if (defaultValue) { - // for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - // const optionEl = selectEl.options[i]; - // if (optionEl.value === defaultValue) { - // selectedOption = optionEl; - // break; - // } - // } - //} - for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - const optionEl = selectEl.options[i]; - if ((optionEl.value === defaultValue) || (!optionEl.value && !defaultValue)) { - selectedOption = optionEl; - break; - } + if (defaultValue) { + for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + const optionEl = selectEl.options[i]; + if (optionEl.value === defaultValue) { + selectedOption = optionEl; + break; + } + } } /** @@ -1234,11 +1226,9 @@ const enhanceComboBox = _comboBoxEl => { input.setAttribute(key, value); })); comboBoxEl.insertAdjacentElement("beforeend", input); - // DOTGOV - modified the aria-label of the clear input button to Reset selection to reflect changed button behavior - // comboBoxEl.insertAdjacentHTML("beforeend", Sanitizer.escapeHTML` - +   @@ -1374,12 +1364,8 @@ const displayList = el => { for (let i = 0, len = selectEl.options.length; i < len; i += 1) { const optionEl = selectEl.options[i]; const optionId = `${listOptionBaseId}${options.length}`; - // DOTGOV: modified combobox to allow for options with blank value - //if (optionEl.value && (disableFiltering || isPristine || !inputValue || regex.test(optionEl.text))) { - if ((disableFiltering || isPristine || !inputValue || regex.test(optionEl.text))) { - // DOTGOV: modified combobox to allow blank option value selections to be considered selected - //if (selectEl.value && optionEl.value === selectEl.value) { - if (selectEl.value && optionEl.value === selectEl.value || (!selectEl.value && !optionEl.value)) { + if (optionEl.value && (disableFiltering || isPristine || !inputValue || regex.test(optionEl.text))) { + if (selectEl.value && optionEl.value === selectEl.value) { selectedItemId = optionId; } if (disableFiltering && !firstFoundId && regex.test(optionEl.text)) { @@ -1514,28 +1500,17 @@ const resetSelection = el => { } = getComboBoxContext(el); const selectValue = selectEl.value; const inputValue = (inputEl.value || "").toLowerCase(); - // DOTGOV - allow for option value to be empty string - //if (selectValue) { - // for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - // const optionEl = selectEl.options[i]; - // if (optionEl.value === selectValue) { - // if (inputValue !== optionEl.text) { - // changeElementValue(inputEl, optionEl.text); - // } - // comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); - // return; - // } - // } - //} - for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - const optionEl = selectEl.options[i]; - if ((!selectValue && !optionEl.value) || optionEl.value === selectValue) { - if (inputValue !== optionEl.text) { - changeElementValue(inputEl, optionEl.text); - } - comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); - return; - } + if (selectValue) { + for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + const optionEl = selectEl.options[i]; + if (optionEl.value === selectValue) { + if (inputValue !== optionEl.text) { + changeElementValue(inputEl, optionEl.text); + } + comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); + return; + } + } } if (inputValue) { changeElementValue(inputEl); diff --git a/src/registrar/assets/src/js/getgov/main.js b/src/registrar/assets/src/js/getgov/main.js index 6ff402aa4..9f3156cf7 100644 --- a/src/registrar/assets/src/js/getgov/main.js +++ b/src/registrar/assets/src/js/getgov/main.js @@ -31,7 +31,7 @@ initializeUrbanizationToggle(); userProfileListener(); finishUserSetupListener(); -loadInitialValuesForComboBoxes(); +//loadInitialValuesForComboBoxes(); handleRequestingEntityFieldset(); diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 636a41760..e40355d61 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -77,6 +77,20 @@ class RequestingEntityForm(RegistrarForm): (obj.id, str(obj)) for obj in queryset ] + [("other", "Other (enter your suborganization manually)")] + @classmethod + def from_database(cls, obj: DomainRequest | None): + """Returns a dict of form field values gotten from `obj`. + Overrides RegistrarForm method in order to set sub_organization to 'other' + on GETs of the RequestingEntityForm.""" + if obj is None: + return {} + # get the domain request as a dict, per usual method + domain_request_dict = {name: getattr(obj, name) for name in cls.declared_fields.keys()} # type: ignore + # set sub_organization to 'other' if is_requesting_new_suborganization is True + if obj.is_requesting_new_suborganization(): + domain_request_dict["sub_organization"] = "other" + return domain_request_dict + def clean_sub_organization(self): """On suborganization clean, set the suborganization value to None if the user is requesting a custom suborganization (as it doesn't exist yet)""" @@ -102,42 +116,132 @@ class RequestingEntityForm(RegistrarForm): ) return name + # def full_clean(self): + # """Validation logic to remove the custom suborganization value before clean is triggered. + # Without this override, the form will throw an 'invalid option' error.""" + # # Remove the custom other field before cleaning + # data = self.data.copy() if self.data else None + + # # Remove the 'other' value from suborganization if it exists. + # # This is a special value that tracks if the user is requesting a new suborg. + # suborganization = self.data.get("portfolio_requesting_entity-sub_organization") + # if suborganization and "other" in suborganization: + # data["portfolio_requesting_entity-sub_organization"] = "" + + # # Set the modified data back to the form + # self.data = data + + # # Call the parent's full_clean method + # super().full_clean() + def full_clean(self): - """Validation logic to remove the custom suborganization value before clean is triggered. + """Validation logic to temporarily remove the custom suborganization value before clean is triggered. Without this override, the form will throw an 'invalid option' error.""" - # Remove the custom other field before cleaning - data = self.data.copy() if self.data else None + logger.debug("full_clean") + # Ensure self.data is not None before proceeding + if self.data: + # handle case where form has been submitted + logger.debug("form was submitted") + # Create a copy of the data for manipulation + data = self.data.copy() - # Remove the 'other' value from suborganization if it exists. - # This is a special value that tracks if the user is requesting a new suborg. - suborganization = self.data.get("portfolio_requesting_entity-sub_organization") - if suborganization and "other" in suborganization: - data["portfolio_requesting_entity-sub_organization"] = "" + # Retrieve sub_organization + suborganization = data.get("portfolio_requesting_entity-sub_organization") - # Set the modified data back to the form - self.data = data + logger.debug(f"suborganization submitted as {suborganization}") + # # Determine if "other" should be stored in _original_suborganization + # if not suborganization: + # logger.debug("suborg stored as other") + # self._original_suborganization = "other" + # else: + self._original_suborganization = suborganization + + # If the original value was "other", clear it for validation + if self._original_suborganization == "other": + data["portfolio_requesting_entity-sub_organization"] = "" + + # Set the modified data back to the form + self.data = data + else: + # handle case of a GET + suborganization = None + if self.initial and "sub_organization" in self.initial: + print("suborg in self.initial") + suborganization = self.initial["sub_organization"] + print(self.initial["sub_organization"]) + print(suborganization) + # Check if is_requesting_new_suborganization is True + is_requesting_new_suborganization = False + if self.initial and "is_requesting_new_suborganization" in self.initial: + # Call the method if it exists + print(self.initial["is_requesting_new_suborganization"]()) + is_requesting_new_suborganization = self.initial["is_requesting_new_suborganization"]() + + # Determine if "other" should be set + if is_requesting_new_suborganization and suborganization is None: + print("presetting to other") + self._original_suborganization = "other" + else: + self._original_suborganization = suborganization + print("self.data does not exist") + print(self.initial) + # # Handle the initial GET request case + # self._original_suborganization = None # Call the parent's full_clean method super().full_clean() + # Restore "other" if there are errors + if self.errors: + logger.debug(f"errors detected: {self.errors}; resetting original_sub_organization") + self.data["portfolio_requesting_entity-sub_organization"] = self._original_suborganization + + + # def clean(self): + # """Custom clean implementation to handle our desired logic flow for suborganization. + # Given that these fields often rely on eachother, we need to do this in the parent function.""" + # cleaned_data = super().clean() + + # # Do some custom error validation if the requesting entity is a suborg. + # # Otherwise, just validate as normal. + # suborganization = self.cleaned_data.get("sub_organization") + # is_requesting_new_suborganization = self.cleaned_data.get("is_requesting_new_suborganization") + + # # Get the value of the yes/no checkbox from RequestingEntityYesNoForm. + # # Since self.data stores this as a string, we need to convert "True" => True. + # requesting_entity_is_suborganization = self.data.get( + # "portfolio_requesting_entity-requesting_entity_is_suborganization" + # ) + # if requesting_entity_is_suborganization == "True": + # if is_requesting_new_suborganization: + # # Validate custom suborganization fields + # if not cleaned_data.get("requested_suborganization") and "requested_suborganization" not in self.errors: + # self.add_error("requested_suborganization", "Enter the name of your suborganization.") + # if not cleaned_data.get("suborganization_city"): + # self.add_error("suborganization_city", "Enter the city where your suborganization is located.") + # if not cleaned_data.get("suborganization_state_territory"): + # self.add_error( + # "suborganization_state_territory", + # "Select the state, territory, or military post where your suborganization is located.", + # ) + # elif not suborganization: + # self.add_error("sub_organization", "Suborganization is required.") + + # return cleaned_data + def clean(self): - """Custom clean implementation to handle our desired logic flow for suborganization. - Given that these fields often rely on eachother, we need to do this in the parent function.""" + """Custom clean implementation to handle our desired logic flow for suborganization.""" cleaned_data = super().clean() - # Do some custom error validation if the requesting entity is a suborg. - # Otherwise, just validate as normal. - suborganization = self.cleaned_data.get("sub_organization") - is_requesting_new_suborganization = self.cleaned_data.get("is_requesting_new_suborganization") - - # Get the value of the yes/no checkbox from RequestingEntityYesNoForm. - # Since self.data stores this as a string, we need to convert "True" => True. + # Get the cleaned data + suborganization = cleaned_data.get("sub_organization") + is_requesting_new_suborganization = cleaned_data.get("is_requesting_new_suborganization") requesting_entity_is_suborganization = self.data.get( "portfolio_requesting_entity-requesting_entity_is_suborganization" ) + if requesting_entity_is_suborganization == "True": if is_requesting_new_suborganization: - # Validate custom suborganization fields if not cleaned_data.get("requested_suborganization") and "requested_suborganization" not in self.errors: self.add_error("requested_suborganization", "Enter the name of your suborganization.") if not cleaned_data.get("suborganization_city"): @@ -150,6 +254,12 @@ class RequestingEntityForm(RegistrarForm): elif not suborganization: self.add_error("sub_organization", "Suborganization is required.") + # If there are errors, restore the "other" value for rendering + if self.errors and getattr(self, "_original_suborganization", None) == "other": + self.cleaned_data["sub_organization"] = self._original_suborganization + elif not self.data and getattr(self, "_original_suborganization", None) == "other": + self.cleaned_data["sub_organization"] = self._original_suborganization + return cleaned_data diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 9754b0d0c..e0225aab3 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -368,7 +368,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): and from the database if `use_db` is True (provided that record exists). An empty form will be provided if neither of those are true. """ - + logger.debug(f"get_forms({step},{use_post},{use_db},{files})") kwargs = { "files": files, "prefix": self.steps.current, @@ -385,6 +385,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): for form in forms: data = form.from_database(self.domain_request) if self.has_pk() else None + logger.debug(data) if use_post: instantiated.append(form(self.request.POST, **kwargs)) elif use_db: @@ -562,6 +563,13 @@ class RequestingEntity(DomainRequestWizard): template_name = "domain_request_requesting_entity.html" forms = [forms.RequestingEntityYesNoForm, forms.RequestingEntityForm] + #for debugging: + def get(self, request, *args, **kwargs): + """This method handles GET requests.""" + logger.debug("in get") + + return super().get(request, *args, **kwargs) + def save(self, forms: list): """Override of save to clear or associate certain suborganization data depending on what the user wishes to do. For instance, we want to add a suborganization From 088140a37d6dd96f3e98fa896fd5f15b02c31b1e Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 16 Jan 2025 14:24:43 -0500 Subject: [PATCH 23/40] clean up of requesting entity --- src/registrar/forms/domain_request_wizard.py | 72 +------------------- src/registrar/views/domain_request.py | 9 --- 2 files changed, 2 insertions(+), 79 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index e40355d61..e452c01b1 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -116,44 +116,18 @@ class RequestingEntityForm(RegistrarForm): ) return name - # def full_clean(self): - # """Validation logic to remove the custom suborganization value before clean is triggered. - # Without this override, the form will throw an 'invalid option' error.""" - # # Remove the custom other field before cleaning - # data = self.data.copy() if self.data else None - - # # Remove the 'other' value from suborganization if it exists. - # # This is a special value that tracks if the user is requesting a new suborg. - # suborganization = self.data.get("portfolio_requesting_entity-sub_organization") - # if suborganization and "other" in suborganization: - # data["portfolio_requesting_entity-sub_organization"] = "" - - # # Set the modified data back to the form - # self.data = data - - # # Call the parent's full_clean method - # super().full_clean() def full_clean(self): """Validation logic to temporarily remove the custom suborganization value before clean is triggered. Without this override, the form will throw an 'invalid option' error.""" - logger.debug("full_clean") # Ensure self.data is not None before proceeding if self.data: # handle case where form has been submitted - logger.debug("form was submitted") # Create a copy of the data for manipulation data = self.data.copy() - # Retrieve sub_organization + # Retrieve sub_organization and store in _original_suborganization suborganization = data.get("portfolio_requesting_entity-sub_organization") - - logger.debug(f"suborganization submitted as {suborganization}") - # # Determine if "other" should be stored in _original_suborganization - # if not suborganization: - # logger.debug("suborg stored as other") - # self._original_suborganization = "other" - # else: self._original_suborganization = suborganization # If the original value was "other", clear it for validation @@ -166,69 +140,27 @@ class RequestingEntityForm(RegistrarForm): # handle case of a GET suborganization = None if self.initial and "sub_organization" in self.initial: - print("suborg in self.initial") suborganization = self.initial["sub_organization"] - print(self.initial["sub_organization"]) - print(suborganization) + # Check if is_requesting_new_suborganization is True is_requesting_new_suborganization = False if self.initial and "is_requesting_new_suborganization" in self.initial: # Call the method if it exists - print(self.initial["is_requesting_new_suborganization"]()) is_requesting_new_suborganization = self.initial["is_requesting_new_suborganization"]() # Determine if "other" should be set if is_requesting_new_suborganization and suborganization is None: - print("presetting to other") self._original_suborganization = "other" else: self._original_suborganization = suborganization - print("self.data does not exist") - print(self.initial) - # # Handle the initial GET request case - # self._original_suborganization = None # Call the parent's full_clean method super().full_clean() # Restore "other" if there are errors if self.errors: - logger.debug(f"errors detected: {self.errors}; resetting original_sub_organization") self.data["portfolio_requesting_entity-sub_organization"] = self._original_suborganization - - # def clean(self): - # """Custom clean implementation to handle our desired logic flow for suborganization. - # Given that these fields often rely on eachother, we need to do this in the parent function.""" - # cleaned_data = super().clean() - - # # Do some custom error validation if the requesting entity is a suborg. - # # Otherwise, just validate as normal. - # suborganization = self.cleaned_data.get("sub_organization") - # is_requesting_new_suborganization = self.cleaned_data.get("is_requesting_new_suborganization") - - # # Get the value of the yes/no checkbox from RequestingEntityYesNoForm. - # # Since self.data stores this as a string, we need to convert "True" => True. - # requesting_entity_is_suborganization = self.data.get( - # "portfolio_requesting_entity-requesting_entity_is_suborganization" - # ) - # if requesting_entity_is_suborganization == "True": - # if is_requesting_new_suborganization: - # # Validate custom suborganization fields - # if not cleaned_data.get("requested_suborganization") and "requested_suborganization" not in self.errors: - # self.add_error("requested_suborganization", "Enter the name of your suborganization.") - # if not cleaned_data.get("suborganization_city"): - # self.add_error("suborganization_city", "Enter the city where your suborganization is located.") - # if not cleaned_data.get("suborganization_state_territory"): - # self.add_error( - # "suborganization_state_territory", - # "Select the state, territory, or military post where your suborganization is located.", - # ) - # elif not suborganization: - # self.add_error("sub_organization", "Suborganization is required.") - - # return cleaned_data - def clean(self): """Custom clean implementation to handle our desired logic flow for suborganization.""" cleaned_data = super().clean() diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index e0225aab3..5fccfc2f2 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -368,7 +368,6 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): and from the database if `use_db` is True (provided that record exists). An empty form will be provided if neither of those are true. """ - logger.debug(f"get_forms({step},{use_post},{use_db},{files})") kwargs = { "files": files, "prefix": self.steps.current, @@ -385,7 +384,6 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): for form in forms: data = form.from_database(self.domain_request) if self.has_pk() else None - logger.debug(data) if use_post: instantiated.append(form(self.request.POST, **kwargs)) elif use_db: @@ -562,13 +560,6 @@ class PortfolioDomainRequestWizard(DomainRequestWizard): class RequestingEntity(DomainRequestWizard): template_name = "domain_request_requesting_entity.html" forms = [forms.RequestingEntityYesNoForm, forms.RequestingEntityForm] - - #for debugging: - def get(self, request, *args, **kwargs): - """This method handles GET requests.""" - logger.debug("in get") - - return super().get(request, *args, **kwargs) def save(self, forms: list): """Override of save to clear or associate certain suborganization data From 4649198867a48408be1ca09452ecbdc62988811a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 Jan 2025 12:53:16 -0700 Subject: [PATCH 24/40] Update domain.py --- src/registrar/models/domain.py | 52 ++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 5e186efff..a388196ca 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1665,28 +1665,6 @@ class Domain(TimeStampedModel, DomainHelper): ) return err.code - def _fetch_contacts(self, contact_data): - """Fetch contact info.""" - choices = PublicContact.ContactTypeChoices - # We expect that all these fields get populated, - # so we can create these early, rather than waiting. - contacts_dict = { - choices.ADMINISTRATIVE: None, - choices.SECURITY: None, - choices.TECHNICAL: None, - } - for domainContact in contact_data: - req = commands.InfoContact(id=domainContact.contact) - data = registry.send(req, cleaned=True).res_data[0] - - # Map the object we recieved from EPP to a PublicContact - mapped_object = self.map_epp_contact_to_public_contact(data, domainContact.contact, domainContact.type) - - # Find/create it in the DB - in_db = self._get_or_create_public_contact(mapped_object) - contacts_dict[in_db.contact_type] = in_db.registry_id - return contacts_dict - def _get_or_create_contact(self, contact: PublicContact): """Try to fetch info about a contact. Create it if it does not exist.""" logger.info("_get_or_create_contact() -> Fetching contact info") @@ -1869,8 +1847,8 @@ class Domain(TimeStampedModel, DomainHelper): missingSecurity = True missingTech = True - if len(cleaned.get("_contacts")) < 3: - for contact in cleaned.get("_contacts"): + if len(cleaned.get("contacts")) < 3: + for contact in cleaned.get("contacts"): if contact.type == PublicContact.ContactTypeChoices.ADMINISTRATIVE: missingAdmin = False if contact.type == PublicContact.ContactTypeChoices.SECURITY: @@ -1890,7 +1868,7 @@ class Domain(TimeStampedModel, DomainHelper): technical_contact.save() logger.info( - "_add_missing_contacts_if_unknown => " + "_add_missing_contacts_if_unknown => In function. Values are " f"missingAdmin: {missingAdmin}, missingSecurity: {missingSecurity}, missingTech: {missingTech}" ) @@ -2070,6 +2048,30 @@ class Domain(TimeStampedModel, DomainHelper): if contacts and isinstance(contacts, list) and len(contacts) > 0: cleaned_contacts = self._fetch_contacts(contacts) return cleaned_contacts + + def _fetch_contacts(self, contact_data): + """Fetch contact info.""" + choices = PublicContact.ContactTypeChoices + # We expect that all these fields get populated, + # so we can create these early, rather than waiting. + contacts_dict = { + choices.ADMINISTRATIVE: None, + choices.SECURITY: None, + choices.TECHNICAL: None, + } + for domainContact in contact_data: + req = commands.InfoContact(id=domainContact.contact) + data = registry.send(req, cleaned=True).res_data[0] + logger.info(f"_fetch_contacts => this is the data: {data}") + + # Map the object we recieved from EPP to a PublicContact + mapped_object = self.map_epp_contact_to_public_contact(data, domainContact.contact, domainContact.type) + logger.info(f"_fetch_contacts => mapped_object: {mapped_object}") + + # Find/create it in the DB + in_db = self._get_or_create_public_contact(mapped_object) + contacts_dict[in_db.contact_type] = in_db.registry_id + return contacts_dict def _get_hosts(self, hosts): cleaned_hosts = [] From 511e9610e27b014f7a505f9c2f6c4c441823d615 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 Jan 2025 12:55:50 -0700 Subject: [PATCH 25/40] Update domain.py --- src/registrar/models/domain.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index a388196ca..ad549879b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1847,8 +1847,10 @@ class Domain(TimeStampedModel, DomainHelper): missingSecurity = True missingTech = True - if len(cleaned.get("contacts")) < 3: - for contact in cleaned.get("contacts"): + # Potential collision - mismatch between _contacts and contacts? + # But the ID wouldnt match in this case because the default is being grabbed? + if len(cleaned.get("_contacts")) < 3: + for contact in cleaned.get("_contacts"): if contact.type == PublicContact.ContactTypeChoices.ADMINISTRATIVE: missingAdmin = False if contact.type == PublicContact.ContactTypeChoices.SECURITY: From f9fa8772e585fc30d25d439532b3a1ea8b0a5ed5 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 16 Jan 2025 15:09:32 -0500 Subject: [PATCH 26/40] cleanup and lint --- src/registrar/assets/js/uswds-edited.js | 36 ++++----- .../assets/src/js/getgov/combobox.js | 77 ------------------- src/registrar/assets/src/js/getgov/main.js | 3 - src/registrar/forms/domain_request_wizard.py | 15 ++-- src/registrar/views/domain_request.py | 2 +- 5 files changed, 27 insertions(+), 106 deletions(-) delete mode 100644 src/registrar/assets/src/js/getgov/combobox.js diff --git a/src/registrar/assets/js/uswds-edited.js b/src/registrar/assets/js/uswds-edited.js index 9e3922c9c..9d4dd2e51 100644 --- a/src/registrar/assets/js/uswds-edited.js +++ b/src/registrar/assets/js/uswds-edited.js @@ -29,8 +29,6 @@ * - tooltip dynamic content updated to include nested element (for better sizing control) * - modal exposed to window to be accessible in other js files * - fixed bug in createHeaderButton which added newlines to header button tooltips - * - modified combobox to allow for blank values in list - * - modified aria label for X button in combobox to reflect modified behavior of button * - modified combobox to handle error class */ @@ -1169,13 +1167,13 @@ const enhanceComboBox = _comboBoxEl => { }); } if (defaultValue) { - for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - const optionEl = selectEl.options[i]; - if (optionEl.value === defaultValue) { - selectedOption = optionEl; - break; - } - } + for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + const optionEl = selectEl.options[i]; + if (optionEl.value === defaultValue) { + selectedOption = optionEl; + break; + } + } } /** @@ -1501,16 +1499,16 @@ const resetSelection = el => { const selectValue = selectEl.value; const inputValue = (inputEl.value || "").toLowerCase(); if (selectValue) { - for (let i = 0, len = selectEl.options.length; i < len; i += 1) { - const optionEl = selectEl.options[i]; - if (optionEl.value === selectValue) { - if (inputValue !== optionEl.text) { - changeElementValue(inputEl, optionEl.text); - } - comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); - return; - } - } + for (let i = 0, len = selectEl.options.length; i < len; i += 1) { + const optionEl = selectEl.options[i]; + if (optionEl.value === selectValue) { + if (inputValue !== optionEl.text) { + changeElementValue(inputEl, optionEl.text); + } + comboBoxEl.classList.add(COMBO_BOX_PRISTINE_CLASS); + return; + } + } } if (inputValue) { changeElementValue(inputEl); diff --git a/src/registrar/assets/src/js/getgov/combobox.js b/src/registrar/assets/src/js/getgov/combobox.js deleted file mode 100644 index e0ecc92ad..000000000 --- a/src/registrar/assets/src/js/getgov/combobox.js +++ /dev/null @@ -1,77 +0,0 @@ -import { hideElement, showElement } from './helpers.js'; - -export function loadInitialValuesForComboBoxes() { - var overrideDefaultClearButton = true; - var isTyping = false; - - document.addEventListener('DOMContentLoaded', (event) => { - handleAllComboBoxElements(); - }); - - function handleAllComboBoxElements() { - const comboBoxElements = document.querySelectorAll(".usa-combo-box"); - comboBoxElements.forEach(comboBox => { - const input = comboBox.querySelector("input"); - const select = comboBox.querySelector("select"); - if (!input || !select) { - console.warn("No combobox element found"); - return; - } - // Set the initial value of the combobox - let initialValue = select.getAttribute("data-default-value"); - let clearInputButton = comboBox.querySelector(".usa-combo-box__clear-input"); - if (!clearInputButton) { - console.warn("No clear element found"); - return; - } - - // Override the default clear button behavior such that it no longer clears the input, - // it just resets to the data-initial-value. - // Due to the nature of how uswds works, this is slightly hacky. - - // Input event listener to detect typing - input.addEventListener("input", () => { - isTyping = true; - }); - - // Blur event listener to reset typing state - input.addEventListener("blur", () => { - isTyping = false; - }); - - // Hide the reset button when there is nothing to reset. - // Do this once on init, then everytime a change occurs. - updateClearButtonVisibility(select, initialValue, clearInputButton) - select.addEventListener("change", () => { - updateClearButtonVisibility(select, initialValue, clearInputButton) - }); - - // Change the default input behaviour - have it reset to the data default instead - clearInputButton.addEventListener("click", (e) => { - if (overrideDefaultClearButton) { - e.preventDefault(); - e.stopPropagation(); - input.click(); - // Find the dropdown option with the desired value - const dropdownOptions = document.querySelectorAll(".usa-combo-box__list-option"); - if (dropdownOptions) { - dropdownOptions.forEach(option => { - if (option.getAttribute("data-value") === initialValue) { - // Simulate a click event on the dropdown option - option.click(); - } - }); - } - } - }); - }); - } - - function updateClearButtonVisibility(select, initialValue, clearInputButton) { - if (select.value === initialValue) { - hideElement(clearInputButton); - }else { - showElement(clearInputButton) - } - } -} diff --git a/src/registrar/assets/src/js/getgov/main.js b/src/registrar/assets/src/js/getgov/main.js index 9f3156cf7..a077da929 100644 --- a/src/registrar/assets/src/js/getgov/main.js +++ b/src/registrar/assets/src/js/getgov/main.js @@ -3,7 +3,6 @@ import { initDomainValidators } from './domain-validators.js'; import { initFormsetsForms, triggerModalOnDsDataForm, nameserversFormListener } from './formset-forms.js'; import { initializeUrbanizationToggle } from './urbanization.js'; import { userProfileListener, finishUserSetupListener } from './user-profile.js'; -import { loadInitialValuesForComboBoxes } from './combobox.js'; import { handleRequestingEntityFieldset } from './requesting-entity.js'; import { initDomainsTable } from './table-domains.js'; import { initDomainRequestsTable } from './table-domain-requests.js'; @@ -31,8 +30,6 @@ initializeUrbanizationToggle(); userProfileListener(); finishUserSetupListener(); -//loadInitialValuesForComboBoxes(); - handleRequestingEntityFieldset(); initDomainsTable(); diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index e452c01b1..12d3b74bf 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -73,12 +73,14 @@ class RequestingEntityForm(RegistrarForm): self.fields["sub_organization"].queryset = queryset # Modify the choices to include "other" so that form can display options properly - self.fields["sub_organization"].choices = [("", "--Select--")] + [ - (obj.id, str(obj)) for obj in queryset - ] + [("other", "Other (enter your suborganization manually)")] + self.fields["sub_organization"].choices = ( + [("", "--Select--")] + + [(obj.id, str(obj)) for obj in queryset] + + [("other", "Other (enter your suborganization manually)")] + ) @classmethod - def from_database(cls, obj: DomainRequest | None): + def from_database(cls, obj: DomainRequest | Contact | None): """Returns a dict of form field values gotten from `obj`. Overrides RegistrarForm method in order to set sub_organization to 'other' on GETs of the RequestingEntityForm.""" @@ -86,9 +88,11 @@ class RequestingEntityForm(RegistrarForm): return {} # get the domain request as a dict, per usual method domain_request_dict = {name: getattr(obj, name) for name in cls.declared_fields.keys()} # type: ignore + # set sub_organization to 'other' if is_requesting_new_suborganization is True - if obj.is_requesting_new_suborganization(): + if isinstance(obj, DomainRequest) and obj.is_requesting_new_suborganization(): domain_request_dict["sub_organization"] = "other" + return domain_request_dict def clean_sub_organization(self): @@ -116,7 +120,6 @@ class RequestingEntityForm(RegistrarForm): ) return name - def full_clean(self): """Validation logic to temporarily remove the custom suborganization value before clean is triggered. Without this override, the form will throw an 'invalid option' error.""" diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index 5fccfc2f2..bff3e5c00 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -560,7 +560,7 @@ class PortfolioDomainRequestWizard(DomainRequestWizard): class RequestingEntity(DomainRequestWizard): template_name = "domain_request_requesting_entity.html" forms = [forms.RequestingEntityYesNoForm, forms.RequestingEntityForm] - + def save(self, forms: list): """Override of save to clear or associate certain suborganization data depending on what the user wishes to do. For instance, we want to add a suborganization From 1ece895ac6a6c51d34f41b03a3210ea428185d1b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 Jan 2025 15:15:36 -0700 Subject: [PATCH 27/40] Add test for race condition --- src/registrar/models/domain.py | 29 ++++++++------ src/registrar/tests/test_models_domain.py | 48 +++++++++++++++++++++++ 2 files changed, 64 insertions(+), 13 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index ad549879b..94ab21bde 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1939,6 +1939,8 @@ class Domain(TimeStampedModel, DomainHelper): Additionally, capture and cache old hosts and contacts from cache if they don't exist in cleaned """ + + # object reference issue between self._cache vs cleaned? old_cache_hosts = self._cache.get("hosts") old_cache_contacts = self._cache.get("contacts") @@ -2111,19 +2113,20 @@ class Domain(TimeStampedModel, DomainHelper): # Save to DB if it doesn't exist already. if db_contact.count() == 0: # Doesn't run custom save logic, just saves to DB - try: - public_contact.save(skip_epp_save=True) - logger.info(f"Created a new PublicContact: {public_contact}") - except IntegrityError as err: - logger.error( - "_get_or_create_public_contact() => tried to create a duplicate public contact: " - f"{err}", exc_info=True - ) - return PublicContact.objects.get( - registry_id=public_contact.registry_id, - contact_type=public_contact.contact_type, - domain=self, - ) + public_contact.save(skip_epp_save=True) + # try: + # public_contact.save(skip_epp_save=True) + # logger.info(f"Created a new PublicContact: {public_contact}") + # except IntegrityError as err: + # logger.error( + # "_get_or_create_public_contact() => tried to create a duplicate public contact: " + # f"{err}", exc_info=True + # ) + # return PublicContact.objects.get( + # registry_id=public_contact.registry_id, + # contact_type=public_contact.contact_type, + # domain=self, + # ) # Append the item we just created return public_contact diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 15a88a608..56d5cf2be 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -348,6 +348,54 @@ class TestDomainCache(MockEppLib): class TestDomainCreation(MockEppLib): """Rule: An approved domain request must result in a domain""" + def test_get_security_email_during_unknown_state_race_condition(self): + """ + Scenario: A domain is accessed for the first time + Given a domain in UNKNOWN state with a security contact in registry + When get_security_email is called during state transition + Then the security contact is fetched from registry + And only one security contact exists in database + And the security email matches the registry contact + And no duplicate contact creation is attempted + """ + domain, _ = Domain.objects.get_or_create(name="defaultsecurity.gov") + + # Store original method + original_filter = PublicContact.objects.filter + + def mock_filter(*args, **kwargs): + # First call returns empty queryset to simulate contact not existing + result = original_filter(*args, **kwargs) + if kwargs.get('contact_type') == PublicContact.ContactTypeChoices.SECURITY: + # Create the duplicate contact after the check but before the save + duplicate = PublicContact( + domain=domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY, + registry_id="defaultSec", + email="dotgov@cisa.dhs.gov", + name="Registry Customer Service" + ) + duplicate.save(skip_epp_save=True) + return result + + with patch.object(PublicContact.objects, 'filter', side_effect=mock_filter): + try: + security_email = domain.get_security_email() + except IntegrityError: + self.fail( + "IntegrityError was raised during contact creation due to a race condition. " + "This indicates that concurrent contact creation is not working in some cases. " + "The error occurs when two processes try to create the same contact simultaneously. " + "Expected behavior: gracefully handle duplicate creation and return existing contact." + ) + + # Verify only one contact exists + security_contacts = PublicContact.objects.filter( + domain=domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY + ) + self.assertEqual(security_contacts.count(), 1) + self.assertEqual(security_email, "dotgov@cisa.dhs.gov") @boto3_mocking.patching def test_approved_domain_request_creates_domain_locally(self): From 4de586005e0ed88583d343dd989d1b1bc1a09a63 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 Jan 2025 15:40:03 -0700 Subject: [PATCH 28/40] Test part 2 --- src/registrar/models/domain.py | 32 +++++++++++-------- src/registrar/tests/test_models_domain.py | 39 +++++++++++++++-------- 2 files changed, 44 insertions(+), 27 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 94ab21bde..aaafd6aa8 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2086,11 +2086,13 @@ class Domain(TimeStampedModel, DomainHelper): def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" + logger.info(f"in function") db_contact = PublicContact.objects.filter( registry_id=public_contact.registry_id, contact_type=public_contact.contact_type, domain=self, ) + logger.info(f"db_contact {db_contact}") # If we find duplicates, log it and delete the oldest ones. if db_contact.count() > 1: @@ -2113,20 +2115,22 @@ class Domain(TimeStampedModel, DomainHelper): # Save to DB if it doesn't exist already. if db_contact.count() == 0: # Doesn't run custom save logic, just saves to DB - public_contact.save(skip_epp_save=True) - # try: - # public_contact.save(skip_epp_save=True) - # logger.info(f"Created a new PublicContact: {public_contact}") - # except IntegrityError as err: - # logger.error( - # "_get_or_create_public_contact() => tried to create a duplicate public contact: " - # f"{err}", exc_info=True - # ) - # return PublicContact.objects.get( - # registry_id=public_contact.registry_id, - # contact_type=public_contact.contact_type, - # domain=self, - # ) + try: + public_contact.save(skip_epp_save=True) + logger.info(f"Created a new PublicContact: {public_contact}") + # In rare cases, _add_missing_contacts_if_unknown will cause a race condition with this function. + # This is because it calls .save(), which is called here. + # + except IntegrityError as err: + logger.error( + "_get_or_create_public_contact() => tried to create a duplicate public contact: " + f"{err}", exc_info=True + ) + return PublicContact.objects.get( + registry_id=public_contact.registry_id, + contact_type=public_contact.contact_type, + domain=self, + ) # Append the item we just created return public_contact diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 56d5cf2be..bb1974464 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -348,26 +348,30 @@ class TestDomainCache(MockEppLib): class TestDomainCreation(MockEppLib): """Rule: An approved domain request must result in a domain""" - def test_get_security_email_during_unknown_state_race_condition(self): + + def test_get_security_email_race_condition(self): """ - Scenario: A domain is accessed for the first time - Given a domain in UNKNOWN state with a security contact in registry - When get_security_email is called during state transition - Then the security contact is fetched from registry + Scenario: Two processes try to create the same security contact simultaneously + Given a domain in UNKNOWN state + When a race condition occurs during contact creation + Then no IntegrityError is raised And only one security contact exists in database - And the security email matches the registry contact - And no duplicate contact creation is attempted + And the correct security email is returned """ domain, _ = Domain.objects.get_or_create(name="defaultsecurity.gov") # Store original method original_filter = PublicContact.objects.filter - + self.first_call = True def mock_filter(*args, **kwargs): - # First call returns empty queryset to simulate contact not existing + """ Simulates the race condition by creating a + duplicate contact between filter check and save + """ result = original_filter(*args, **kwargs) - if kwargs.get('contact_type') == PublicContact.ContactTypeChoices.SECURITY: - # Create the duplicate contact after the check but before the save + + # Return empty queryset for first call. Otherwise just proceed as normal. + if self.first_call: + self.first_call = False duplicate = PublicContact( domain=domain, contact_type=PublicContact.ContactTypeChoices.SECURITY, @@ -376,11 +380,20 @@ class TestDomainCreation(MockEppLib): name="Registry Customer Service" ) duplicate.save(skip_epp_save=True) + return PublicContact.objects.none() + return result with patch.object(PublicContact.objects, 'filter', side_effect=mock_filter): try: - security_email = domain.get_security_email() + public_contact = PublicContact( + domain=domain, + contact_type=PublicContact.ContactTypeChoices.SECURITY, + registry_id="defaultSec", + email="dotgov@cisa.dhs.gov", + name="Registry Customer Service" + ) + returned_public_contact = domain._get_or_create_public_contact(public_contact) except IntegrityError: self.fail( "IntegrityError was raised during contact creation due to a race condition. " @@ -395,7 +408,7 @@ class TestDomainCreation(MockEppLib): contact_type=PublicContact.ContactTypeChoices.SECURITY ) self.assertEqual(security_contacts.count(), 1) - self.assertEqual(security_email, "dotgov@cisa.dhs.gov") + self.assertEqual(returned_public_contact, security_contacts.get()) @boto3_mocking.patching def test_approved_domain_request_creates_domain_locally(self): From cfcdd8826cb799701207ebab68e8aa06f11fe889 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 16 Jan 2025 16:04:10 -0700 Subject: [PATCH 29/40] Squash bug --- src/registrar/models/domain.py | 5 +++-- src/registrar/tests/test_models_domain.py | 1 + 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index aaafd6aa8..f7e76cbc4 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -2116,8 +2116,9 @@ class Domain(TimeStampedModel, DomainHelper): if db_contact.count() == 0: # Doesn't run custom save logic, just saves to DB try: - public_contact.save(skip_epp_save=True) - logger.info(f"Created a new PublicContact: {public_contact}") + with transaction.atomic(): + public_contact.save(skip_epp_save=True) + logger.info(f"Created a new PublicContact: {public_contact}") # In rare cases, _add_missing_contacts_if_unknown will cause a race condition with this function. # This is because it calls .save(), which is called here. # diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index bb1974464..8fde89fd0 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -349,6 +349,7 @@ class TestDomainCache(MockEppLib): class TestDomainCreation(MockEppLib): """Rule: An approved domain request must result in a domain""" + @less_console_noise_decorator def test_get_security_email_race_condition(self): """ Scenario: Two processes try to create the same security contact simultaneously From 2ad8e0268adebcf32b0d5092374a0c012f8c18d6 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 16 Jan 2025 19:05:37 -0500 Subject: [PATCH 30/40] changes to domain request data full report and test --- src/registrar/tests/common.py | 8 ++++++ src/registrar/tests/test_reports.py | 25 +++++++++++------- src/registrar/utility/csv_export.py | 39 +++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index c0083068d..2eff53d39 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -578,6 +578,10 @@ class MockDb(TestCase): creator=cls.custom_superuser, federal_agency=cls.federal_agency_3, organization_type="federal" ) + cls.suborganization_1, _ = Suborganization.objects.get_or_create( + name="SubOrg 1", portfolio=cls.portfolio_1, city="Nashville", state_territory="TN", + ) + current_date = get_time_aware_date(datetime(2024, 4, 2)) # Create start and end dates using timedelta @@ -848,6 +852,7 @@ class MockDb(TestCase): status=DomainRequest.DomainRequestStatus.IN_REVIEW, name="city2.gov", portfolio=cls.portfolio_1, + sub_organization=cls.suborganization_1, ) cls.domain_request_3 = completed_domain_request( status=DomainRequest.DomainRequestStatus.STARTED, @@ -863,6 +868,9 @@ class MockDb(TestCase): cls.domain_request_5 = completed_domain_request( status=DomainRequest.DomainRequestStatus.APPROVED, name="city5.gov", + requested_suborganization="requested_suborg", + suborganization_city="SanFran", + suborganization_state_territory="CA", ) cls.domain_request_6 = completed_domain_request( status=DomainRequest.DomainRequestStatus.STARTED, diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index b11500ea9..cabaa048d 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -729,6 +729,7 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): # "Submitted at", "Status", "Domain type", + "Portfolio", "Federal type", "Federal agency", "Organization name", @@ -736,6 +737,10 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): "City", "State/territory", "Region", + "Suborganization", + "Requested suborg", + "Suborg city", + "Suborg state/territory", "Creator first name", "Creator last name", "Creator email", @@ -765,27 +770,29 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): expected_content = ( # Header - "Domain request,Status,Domain type,Federal type,Federal agency,Organization name,Election office," - "City,State/territory,Region,Creator first name,Creator last name,Creator email," + "Domain request,Status,Domain type,Portfolio,Federal type,Federal agency,Organization name," + "Election office,City,State/territory,Region,Suborganization,Requested suborg,Suborg city," + "Suborg state/territory,Creator first name,Creator last name,Creator email," "Creator approved domains count,Creator active requests count,Alternative domains,SO first name," "SO last name,SO email,SO title/role,Request purpose,Request additional details,Other contacts," "CISA regional representative,Current websites,Investigator\n" # Content - "city5.gov,Approved,Federal,Executive,,Testorg,N/A,,NY,2,,,,1,0,city1.gov,Testy,Tester,testy@town.com," - "Chief Tester,Purpose of the site,There is more,Testy Tester testy2@town.com,,city.com,\n" - "city2.gov,In review,Federal,Executive,Portfolio 1 Federal Agency,,N/A,,NY,2,,,,0,1,city1.gov,,,,," - "Purpose of the site,There is more,Testy Tester testy2@town.com,,city.com,\n" - "city3.gov,Submitted,Federal,Executive,Portfolio 1 Federal Agency,,N/A,,NY,2,,,,0,1," + "city5.gov,Approved,Federal,No,Executive,,Testorg,N/A,,NY,2,requested_suborg,SanFran,CA,,,,,1,0," + "city1.gov,Testy,Tester,testy@town.com,Chief Tester,Purpose of the site,There is more," + "Testy Tester testy2@town.com,,city.com,\n" + "city2.gov,In review,Federal,Yes,Executive,Portfolio 1 Federal Agency,,N/A,,,2,SubOrg 1,,,,,,,0," + "1,city1.gov,,,,,Purpose of the site,There is more,Testy Tester testy2@town.com,,city.com,\n" + "city3.gov,Submitted,Federal,Yes,Executive,Portfolio 1 Federal Agency,,N/A,,,2,,,,,,,,0,1," '"cheeseville.gov, city1.gov, igorville.gov",,,,,Purpose of the site,CISA-first-name CISA-last-name | ' 'There is more,"Meow Tester24 te2@town.com, Testy1232 Tester24 te2@town.com, ' 'Testy Tester testy2@town.com",' 'test@igorville.com,"city.com, https://www.example2.com, https://www.example.com",\n' - "city4.gov,Submitted,City,Executive,,Testorg,Yes,,NY,2,,,,0,1,city1.gov,Testy," + "city4.gov,Submitted,City,No,Executive,,Testorg,Yes,,NY,2,,,,,,,,0,1,city1.gov,Testy," "Tester,testy@town.com," "Chief Tester,Purpose of the site,CISA-first-name CISA-last-name | There is more," "Testy Tester testy2@town.com," "cisaRep@igorville.gov,city.com,\n" - "city6.gov,Submitted,Federal,Executive,Portfolio 1 Federal Agency,,N/A,,NY,2,,,,0,1,city1.gov,,,,," + "city6.gov,Submitted,Federal,Yes,Executive,Portfolio 1 Federal Agency,,N/A,,,2,,,,,,,,0,1,city1.gov,,,,," "Purpose of the site,CISA-first-name CISA-last-name | There is more,Testy Tester testy2@town.com," "cisaRep@igorville.gov,city.com,\n" ) diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 6e5773ebb..1bb53a7a3 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -1660,6 +1660,27 @@ class DomainRequestExport(BaseExport): default=F("organization_name"), output_field=CharField(), ), + "converted_city": Case( + # When portfolio is present, use its value instead + When(portfolio__isnull=False, then=F("portfolio__city")), + # Otherwise, return the natively assigned value + default=F("city"), + output_field=CharField(), + ), + "converted_state_territory": Case( + # When portfolio is present, use its value instead + When(portfolio__isnull=False, then=F("portfolio__state_territory")), + # Otherwise, return the natively assigned value + default=F("state_territory"), + output_field=CharField(), + ), + "converted_suborganization_name": Case( + # When sub_organization is present, use its name + When(sub_organization__isnull=False, then=F("sub_organization__name")), + # Otherwise, return empty string + default=Value(""), + output_field=CharField(), + ), "converted_so_email": Case( # When portfolio is present, use its value instead When(portfolio__isnull=False, then=F("portfolio__senior_official__email")), @@ -1786,6 +1807,10 @@ class DomainRequestExport(BaseExport): status = model.get("status") status_display = DomainRequest.DomainRequestStatus.get_status_label(status) if status else None + # Handle the portfolio field. Display as a Yes/No + portfolio = model.get("portfolio") + portfolio_display = "Yes" if portfolio is not None else "No" + # Handle the region field. state_territory = model.get("state_territory") region = get_region(state_territory) if state_territory else None @@ -1819,6 +1844,7 @@ class DomainRequestExport(BaseExport): "Election office": human_readable_election_board, "Federal type": human_readable_federal_type, "Domain type": human_readable_org_type, + "Portfolio": portfolio_display, "Request additional details": additional_details, # Annotated fields - passed into the request dict. "Creator approved domains count": model.get("creator_approved_domains_count", 0), @@ -1827,6 +1853,10 @@ class DomainRequestExport(BaseExport): "Other contacts": model.get("all_other_contacts"), "Current websites": model.get("all_current_websites"), # Untouched FK fields - passed into the request dict. + "Suborganization": model.get("converted_suborganization_name"), + "Requested suborg": model.get("requested_suborganization"), + "Suborg city": model.get("suborganization_city"), + "Suborg state/territory": model.get("suborganization_state_territory"), "Federal agency": model.get("converted_federal_agency"), "SO first name": model.get("converted_senior_official_first_name"), "SO last name": model.get("converted_senior_official_last_name"), @@ -1838,8 +1868,8 @@ class DomainRequestExport(BaseExport): "Investigator": model.get("investigator__email"), # Untouched fields "Organization name": model.get("converted_organization_name"), - "City": model.get("city"), - "State/territory": model.get("state_territory"), + "City": model.get("converted_city"), + "State/territory": model.get("converted_state_territory"), "Request purpose": model.get("purpose"), "CISA regional representative": model.get("cisa_representative_email"), "Last submitted date": model.get("last_submitted_date"), @@ -2006,6 +2036,7 @@ class DomainRequestDataFull(DomainRequestExport): "Last status update", "Status", "Domain type", + "Portfolio", "Federal type", "Federal agency", "Organization name", @@ -2013,6 +2044,10 @@ class DomainRequestDataFull(DomainRequestExport): "City", "State/territory", "Region", + "Suborganization", + "Requested suborg", + "Suborg city", + "Suborg state/territory", "Creator first name", "Creator last name", "Creator email", From 75665ff378c5216fc7281f0637c24db8cb956cf2 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 16 Jan 2025 19:31:11 -0500 Subject: [PATCH 31/40] fixed other tests and linted --- src/registrar/tests/common.py | 5 ++++- src/registrar/tests/test_management_scripts.py | 10 +++++++++- src/registrar/tests/test_reports.py | 4 ++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 2eff53d39..bb65ef6b1 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -579,7 +579,10 @@ class MockDb(TestCase): ) cls.suborganization_1, _ = Suborganization.objects.get_or_create( - name="SubOrg 1", portfolio=cls.portfolio_1, city="Nashville", state_territory="TN", + name="SubOrg 1", + portfolio=cls.portfolio_1, + city="Nashville", + state_territory="TN", ) current_date = get_time_aware_date(datetime(2024, 4, 2)) diff --git a/src/registrar/tests/test_management_scripts.py b/src/registrar/tests/test_management_scripts.py index 655068493..536d1e760 100644 --- a/src/registrar/tests/test_management_scripts.py +++ b/src/registrar/tests/test_management_scripts.py @@ -2101,6 +2101,10 @@ class TestPatchSuborganizations(MockDbForIndividualTests): 1. Fewest spaces 2. Most leading capitals """ + # Delete any other suborganizations defined in the initial test dataset + DomainRequest.objects.all().delete() + Suborganization.objects.all().delete() + Suborganization.objects.create(name="Test Organization ", portfolio=self.portfolio_1) Suborganization.objects.create(name="test organization", portfolio=self.portfolio_1) Suborganization.objects.create(name="Test Organization", portfolio=self.portfolio_1) @@ -2114,6 +2118,10 @@ class TestPatchSuborganizations(MockDbForIndividualTests): @less_console_noise_decorator def test_hardcoded_record(self): """Tests that our hardcoded records update as we expect them to""" + # Delete any other suborganizations defined in the initial test dataset + DomainRequest.objects.all().delete() + Suborganization.objects.all().delete() + # Create orgs with old and new name formats old_name = "USDA/OC" new_name = "USDA, Office of Communications" @@ -2123,7 +2131,7 @@ class TestPatchSuborganizations(MockDbForIndividualTests): self.run_patch_suborganizations() - # Verify only the new one remains + # Verify only the new one of the two remains self.assertEqual(Suborganization.objects.count(), 1) remaining = Suborganization.objects.first() self.assertEqual(remaining.name, new_name) diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index cabaa048d..9d410e430 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -792,8 +792,8 @@ class ExportDataTest(MockDbForIndividualTests, MockEppLib): "Chief Tester,Purpose of the site,CISA-first-name CISA-last-name | There is more," "Testy Tester testy2@town.com," "cisaRep@igorville.gov,city.com,\n" - "city6.gov,Submitted,Federal,Yes,Executive,Portfolio 1 Federal Agency,,N/A,,,2,,,,,,,,0,1,city1.gov,,,,," - "Purpose of the site,CISA-first-name CISA-last-name | There is more,Testy Tester testy2@town.com," + "city6.gov,Submitted,Federal,Yes,Executive,Portfolio 1 Federal Agency,,N/A,,,2,,,,,,,,0,1,city1.gov," + ",,,,Purpose of the site,CISA-first-name CISA-last-name | There is more,Testy Tester testy2@town.com," "cisaRep@igorville.gov,city.com,\n" ) From a0d6e1ec3356e9765e855b9c5a0766c734cacad8 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 17 Jan 2025 10:06:40 -0700 Subject: [PATCH 32/40] cleanup --- src/registrar/models/domain.py | 24 +++++-------- src/registrar/tests/test_models_domain.py | 42 ++++++++++++----------- 2 files changed, 31 insertions(+), 35 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index f7e76cbc4..2ace5f22d 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1847,10 +1847,9 @@ class Domain(TimeStampedModel, DomainHelper): missingSecurity = True missingTech = True - # Potential collision - mismatch between _contacts and contacts? - # But the ID wouldnt match in this case because the default is being grabbed? - if len(cleaned.get("_contacts")) < 3: - for contact in cleaned.get("_contacts"): + contacts = cleaned.get("_contacts", []) + if len(contacts) < 3: + for contact in contacts: if contact.type == PublicContact.ContactTypeChoices.ADMINISTRATIVE: missingAdmin = False if contact.type == PublicContact.ContactTypeChoices.SECURITY: @@ -1868,9 +1867,9 @@ class Domain(TimeStampedModel, DomainHelper): if missingTech: technical_contact = self.get_default_technical_contact() technical_contact.save() - + logger.info( - "_add_missing_contacts_if_unknown => In function. Values are " + "_add_missing_contacts_if_unknown => Adding contacts. Values are " f"missingAdmin: {missingAdmin}, missingSecurity: {missingSecurity}, missingTech: {missingTech}" ) @@ -2052,7 +2051,7 @@ class Domain(TimeStampedModel, DomainHelper): if contacts and isinstance(contacts, list) and len(contacts) > 0: cleaned_contacts = self._fetch_contacts(contacts) return cleaned_contacts - + def _fetch_contacts(self, contact_data): """Fetch contact info.""" choices = PublicContact.ContactTypeChoices @@ -2086,13 +2085,11 @@ class Domain(TimeStampedModel, DomainHelper): def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" - logger.info(f"in function") db_contact = PublicContact.objects.filter( registry_id=public_contact.registry_id, contact_type=public_contact.contact_type, domain=self, ) - logger.info(f"db_contact {db_contact}") # If we find duplicates, log it and delete the oldest ones. if db_contact.count() > 1: @@ -2119,13 +2116,10 @@ class Domain(TimeStampedModel, DomainHelper): with transaction.atomic(): public_contact.save(skip_epp_save=True) logger.info(f"Created a new PublicContact: {public_contact}") - # In rare cases, _add_missing_contacts_if_unknown will cause a race condition with this function. - # This is because it calls .save(), which is called here. - # except IntegrityError as err: logger.error( - "_get_or_create_public_contact() => tried to create a duplicate public contact: " - f"{err}", exc_info=True + "_get_or_create_public_contact() => tried to create a duplicate public contact: " f"{err}", + exc_info=True, ) return PublicContact.objects.get( registry_id=public_contact.registry_id, @@ -2142,7 +2136,7 @@ class Domain(TimeStampedModel, DomainHelper): if existing_contact.email != public_contact.email or existing_contact.registry_id != public_contact.registry_id: existing_contact.delete() public_contact.save() - logger.warning("Requested PublicContact is out of sync " "with DB.") + logger.warning("Requested PublicContact is out of sync with our DB.") return public_contact # If it already exists, we can assume that the DB instance was updated during set, so we should just use that. diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 8fde89fd0..083725a55 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -350,27 +350,28 @@ class TestDomainCreation(MockEppLib): """Rule: An approved domain request must result in a domain""" @less_console_noise_decorator - def test_get_security_email_race_condition(self): + def test_get_or_create_public_contact_race_condition(self): """ Scenario: Two processes try to create the same security contact simultaneously Given a domain in UNKNOWN state When a race condition occurs during contact creation Then no IntegrityError is raised And only one security contact exists in database - And the correct security email is returned + And the correct public contact is returned + + CONTEXT: We ran into an intermittent but somewhat rare issue where IntegrityError + was raised when creating PublicContact. + Per our logs, this seemed to appear during periods of high app activity. """ domain, _ = Domain.objects.get_or_create(name="defaultsecurity.gov") - - # Store original method - original_filter = PublicContact.objects.filter - self.first_call = True - def mock_filter(*args, **kwargs): - """ Simulates the race condition by creating a - duplicate contact between filter check and save - """ - result = original_filter(*args, **kwargs) - # Return empty queryset for first call. Otherwise just proceed as normal. + self.first_call = True + + def mock_filter(*args, **kwargs): + """Simulates a race condition by creating a + duplicate contact between the first filter and save. + """ + # Return an empty queryset for the first call. Otherwise just proceed as normal. if self.first_call: self.first_call = False duplicate = PublicContact( @@ -378,21 +379,21 @@ class TestDomainCreation(MockEppLib): contact_type=PublicContact.ContactTypeChoices.SECURITY, registry_id="defaultSec", email="dotgov@cisa.dhs.gov", - name="Registry Customer Service" + name="Registry Customer Service", ) duplicate.save(skip_epp_save=True) return PublicContact.objects.none() - return result - - with patch.object(PublicContact.objects, 'filter', side_effect=mock_filter): + return PublicContact.objects.filter(*args, **kwargs) + + with patch.object(PublicContact.objects, "filter", side_effect=mock_filter): try: public_contact = PublicContact( domain=domain, contact_type=PublicContact.ContactTypeChoices.SECURITY, registry_id="defaultSec", email="dotgov@cisa.dhs.gov", - name="Registry Customer Service" + name="Registry Customer Service", ) returned_public_contact = domain._get_or_create_public_contact(public_contact) except IntegrityError: @@ -403,13 +404,14 @@ class TestDomainCreation(MockEppLib): "Expected behavior: gracefully handle duplicate creation and return existing contact." ) - # Verify only one contact exists + # Verify that only one contact exists and its correctness security_contacts = PublicContact.objects.filter( - domain=domain, - contact_type=PublicContact.ContactTypeChoices.SECURITY + domain=domain, contact_type=PublicContact.ContactTypeChoices.SECURITY ) self.assertEqual(security_contacts.count(), 1) self.assertEqual(returned_public_contact, security_contacts.get()) + self.assertEqual(returned_public_contact.registry_id, "defaultSec") + self.assertEqual(returned_public_contact.email, "dotgov@cisa.dhs.gov") @boto3_mocking.patching def test_approved_domain_request_creates_domain_locally(self): From 8c204d4e8e2c392baddd35508dad83387a786358 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 17 Jan 2025 11:10:24 -0700 Subject: [PATCH 33/40] Remove unrelated changes --- src/registrar/admin.py | 3 -- src/registrar/models/domain.py | 56 +++++++++++++++++----------------- 2 files changed, 28 insertions(+), 31 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 8773f7ef8..849cb6100 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -3835,9 +3835,6 @@ class PublicContactAdmin(ListHeaderAdmin, ImportExportModelAdmin): change_form_template = "django/admin/email_clipboard_change_form.html" autocomplete_fields = ["domain"] - list_display = ("name", "contact_type", "email", "domain", "registry_id") - search_fields = ["email", "name", "registry_id"] - search_help_text = "Search by email, name or registry id." def changeform_view(self, request, object_id=None, form_url="", extra_context=None): if extra_context is None: diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 2ace5f22d..d024efe5f 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1665,6 +1665,30 @@ class Domain(TimeStampedModel, DomainHelper): ) return err.code + def _fetch_contacts(self, contact_data): + """Fetch contact info.""" + choices = PublicContact.ContactTypeChoices + # We expect that all these fields get populated, + # so we can create these early, rather than waiting. + contacts_dict = { + choices.ADMINISTRATIVE: None, + choices.SECURITY: None, + choices.TECHNICAL: None, + } + for domainContact in contact_data: + req = commands.InfoContact(id=domainContact.contact) + data = registry.send(req, cleaned=True).res_data[0] + logger.info(f"_fetch_contacts => this is the data: {data}") + + # Map the object we recieved from EPP to a PublicContact + mapped_object = self.map_epp_contact_to_public_contact(data, domainContact.contact, domainContact.type) + logger.info(f"_fetch_contacts => mapped_object: {mapped_object}") + + # Find/create it in the DB + in_db = self._get_or_create_public_contact(mapped_object) + contacts_dict[in_db.contact_type] = in_db.registry_id + return contacts_dict + def _get_or_create_contact(self, contact: PublicContact): """Try to fetch info about a contact. Create it if it does not exist.""" logger.info("_get_or_create_contact() -> Fetching contact info") @@ -1826,6 +1850,7 @@ class Domain(TimeStampedModel, DomainHelper): """ try: self._add_missing_contacts_if_unknown(cleaned) + except Exception as e: logger.error( "%s couldn't _add_missing_contacts_if_unknown, error was %s." @@ -1843,6 +1868,7 @@ class Domain(TimeStampedModel, DomainHelper): is in an UNKNOWN state, that is an error state) Note: The transition state change happens at the end of the function """ + missingAdmin = True missingSecurity = True missingTech = True @@ -1938,8 +1964,6 @@ class Domain(TimeStampedModel, DomainHelper): Additionally, capture and cache old hosts and contacts from cache if they don't exist in cleaned """ - - # object reference issue between self._cache vs cleaned? old_cache_hosts = self._cache.get("hosts") old_cache_contacts = self._cache.get("contacts") @@ -2052,30 +2076,6 @@ class Domain(TimeStampedModel, DomainHelper): cleaned_contacts = self._fetch_contacts(contacts) return cleaned_contacts - def _fetch_contacts(self, contact_data): - """Fetch contact info.""" - choices = PublicContact.ContactTypeChoices - # We expect that all these fields get populated, - # so we can create these early, rather than waiting. - contacts_dict = { - choices.ADMINISTRATIVE: None, - choices.SECURITY: None, - choices.TECHNICAL: None, - } - for domainContact in contact_data: - req = commands.InfoContact(id=domainContact.contact) - data = registry.send(req, cleaned=True).res_data[0] - logger.info(f"_fetch_contacts => this is the data: {data}") - - # Map the object we recieved from EPP to a PublicContact - mapped_object = self.map_epp_contact_to_public_contact(data, domainContact.contact, domainContact.type) - logger.info(f"_fetch_contacts => mapped_object: {mapped_object}") - - # Find/create it in the DB - in_db = self._get_or_create_public_contact(mapped_object) - contacts_dict[in_db.contact_type] = in_db.registry_id - return contacts_dict - def _get_hosts(self, hosts): cleaned_hosts = [] if hosts and isinstance(hosts, list): @@ -2118,7 +2118,7 @@ class Domain(TimeStampedModel, DomainHelper): logger.info(f"Created a new PublicContact: {public_contact}") except IntegrityError as err: logger.error( - "_get_or_create_public_contact() => tried to create a duplicate public contact: " f"{err}", + f"_get_or_create_public_contact() => tried to create a duplicate public contact: {err}", exc_info=True, ) return PublicContact.objects.get( @@ -2136,7 +2136,7 @@ class Domain(TimeStampedModel, DomainHelper): if existing_contact.email != public_contact.email or existing_contact.registry_id != public_contact.registry_id: existing_contact.delete() public_contact.save() - logger.warning("Requested PublicContact is out of sync with our DB.") + logger.warning("Requested PublicContact is out of sync with DB.") return public_contact # If it already exists, we can assume that the DB instance was updated during set, so we should just use that. From ba2788014b5118364c1c0054b3587e68e56d7601 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 17 Jan 2025 17:38:00 -0500 Subject: [PATCH 34/40] removed default empty options from Checkboxes as they are only needed for Selects --- src/registrar/assets/src/js/getgov/requesting-entity.js | 3 +-- src/registrar/forms/domain.py | 3 +-- src/registrar/forms/domain_request_wizard.py | 9 +++------ src/registrar/forms/portfolio.py | 2 +- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/requesting-entity.js b/src/registrar/assets/src/js/getgov/requesting-entity.js index 5f3be8c79..833eab2f8 100644 --- a/src/registrar/assets/src/js/getgov/requesting-entity.js +++ b/src/registrar/assets/src/js/getgov/requesting-entity.js @@ -15,7 +15,6 @@ export function handleRequestingEntityFieldset() { const suborgContainer = document.getElementById("suborganization-container"); const suborgDetailsContainer = document.getElementById("suborganization-container__details"); const suborgAddtlInstruction = document.getElementById("suborganization-addtl-instruction"); - const subOrgCreateNewOption = document.getElementById("option-to-add-suborg")?.value; // Make sure all crucial page elements exist before proceeding. // This more or less ensures that we are on the Requesting Entity page, and not elsewhere. if (!radios || !input || !select || !inputGrandParent || !suborgContainer || !suborgDetailsContainer) return; @@ -28,7 +27,7 @@ export function handleRequestingEntityFieldset() { function toggleSuborganization(radio=null) { if (radio != null) requestingSuborganization = radio?.checked && radio.value === "True"; requestingSuborganization ? showElement(suborgContainer) : hideElement(suborgContainer); - if (select.options.length == 2) { // --Select-- and other are the only options + if (select.options.length == 1) { // other is the only option hideElement(inputGrandParent); // Hide the combo box and indicate requesting new suborg hideElement(suborgAddtlInstruction); // Hide additional instruction related to the list requestingNewSuborganization.value = "True"; diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index b3dae0d3a..5eeae232d 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -450,7 +450,6 @@ class DomainOrgNameAddressForm(forms.ModelForm): label="Federal agency", required=False, queryset=FederalAgency.objects.all(), - empty_label="--Select--", widget=ComboboxWidget, ) zipcode = forms.CharField( @@ -469,7 +468,7 @@ class DomainOrgNameAddressForm(forms.ModelForm): state_territory = forms.ChoiceField( label="State, territory, or military post", required=True, - choices=[("", "--Select--")] + DomainInformation.StateTerritoryChoices.choices, + choices=DomainInformation.StateTerritoryChoices.choices, error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 12d3b74bf..7f80c9d34 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -44,7 +44,6 @@ class RequestingEntityForm(RegistrarForm): label="Suborganization name", required=False, queryset=Suborganization.objects.none(), - empty_label="--Select--", widget=ComboboxWidget, ) requested_suborganization = forms.CharField( @@ -58,7 +57,7 @@ class RequestingEntityForm(RegistrarForm): suborganization_state_territory = forms.ChoiceField( label="State, territory, or military post", required=False, - choices=[("", "--Select--")] + DomainRequest.StateTerritoryChoices.choices, + choices=DomainRequest.StateTerritoryChoices.choices, widget=ComboboxWidget, ) @@ -74,8 +73,7 @@ class RequestingEntityForm(RegistrarForm): # Modify the choices to include "other" so that form can display options properly self.fields["sub_organization"].choices = ( - [("", "--Select--")] - + [(obj.id, str(obj)) for obj in queryset] + [(obj.id, str(obj)) for obj in queryset] + [("other", "Other (enter your suborganization manually)")] ) @@ -328,7 +326,6 @@ class OrganizationContactForm(RegistrarForm): # uncomment to see if modelChoiceField can be an arg later required=False, queryset=FederalAgency.objects.exclude(agency__in=excluded_agencies), - empty_label="--Select--", widget=ComboboxWidget, ) organization_name = forms.CharField( @@ -349,7 +346,7 @@ class OrganizationContactForm(RegistrarForm): ) state_territory = forms.ChoiceField( label="State, territory, or military post", - choices=[("", "--Select--")] + DomainRequest.StateTerritoryChoices.choices, + choices=DomainRequest.StateTerritoryChoices.choices, error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 13d956fe4..9d6c9ecdf 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -37,7 +37,7 @@ class PortfolioOrgAddressForm(forms.ModelForm): state_territory = forms.ChoiceField( label="State, territory, or military post", required=True, - choices=[("", "--Select--")] + DomainInformation.StateTerritoryChoices.choices, + choices=DomainInformation.StateTerritoryChoices.choices, error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, From 463632baf769c901c1cec198fdcda293744d632d Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 17 Jan 2025 17:42:00 -0500 Subject: [PATCH 35/40] lint --- src/registrar/forms/domain_request_wizard.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 7f80c9d34..e67595c21 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -72,10 +72,9 @@ class RequestingEntityForm(RegistrarForm): self.fields["sub_organization"].queryset = queryset # Modify the choices to include "other" so that form can display options properly - self.fields["sub_organization"].choices = ( - [(obj.id, str(obj)) for obj in queryset] - + [("other", "Other (enter your suborganization manually)")] - ) + self.fields["sub_organization"].choices = [(obj.id, str(obj)) for obj in queryset] + [ + ("other", "Other (enter your suborganization manually)") + ] @classmethod def from_database(cls, obj: DomainRequest | Contact | None): From 96618ee2bc6ff7d1a2a9cee33cac68d38ec10679 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Fri, 17 Jan 2025 20:03:04 -0500 Subject: [PATCH 36/40] fixing tests --- src/registrar/forms/domain_request_wizard.py | 3 -- src/registrar/tests/test_views_portfolio.py | 30 +++++++++++++++----- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index e67595c21..7c9dcb180 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -95,7 +95,6 @@ class RequestingEntityForm(RegistrarForm): def clean_sub_organization(self): """On suborganization clean, set the suborganization value to None if the user is requesting a custom suborganization (as it doesn't exist yet)""" - # If it's a new suborganization, return None (equivalent to selecting nothing) if self.cleaned_data.get("is_requesting_new_suborganization"): return None @@ -129,7 +128,6 @@ class RequestingEntityForm(RegistrarForm): # Retrieve sub_organization and store in _original_suborganization suborganization = data.get("portfolio_requesting_entity-sub_organization") self._original_suborganization = suborganization - # If the original value was "other", clear it for validation if self._original_suborganization == "other": data["portfolio_requesting_entity-sub_organization"] = "" @@ -171,7 +169,6 @@ class RequestingEntityForm(RegistrarForm): requesting_entity_is_suborganization = self.data.get( "portfolio_requesting_entity-requesting_entity_is_suborganization" ) - if requesting_entity_is_suborganization == "True": if is_requesting_new_suborganization: if not cleaned_data.get("requested_suborganization") and "requested_suborganization" not in self.errors: diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 78a4dae82..e2de3328f 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2879,7 +2879,7 @@ class TestRequestingEntity(WebTest): form["portfolio_requesting_entity-requesting_entity_is_suborganization"] = True form["portfolio_requesting_entity-is_requesting_new_suborganization"] = True - form["portfolio_requesting_entity-sub_organization"] = "" + form["portfolio_requesting_entity-sub_organization"] = "other" form["portfolio_requesting_entity-requested_suborganization"] = "moon" form["portfolio_requesting_entity-suborganization_city"] = "kepler" @@ -2933,7 +2933,7 @@ class TestRequestingEntity(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) - @less_console_noise_decorator + # @less_console_noise_decorator def test_requesting_entity_page_errors(self): """Tests that we get the expected form errors on requesting entity""" domain_request = completed_domain_request(user=self.user, portfolio=self.portfolio) @@ -2942,18 +2942,34 @@ class TestRequestingEntity(WebTest): session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + # For 2 the tests below, it is required to submit a form without submitting a value + # for the select/combobox. WebTest will not do this; by default, WebTest will submit + # the first choice in a select. So, need to manipulate the form to remove the + # particular select/combobox that will not be submitted, and then post the form. + form_action = f"/request/{domain_request.pk}/portfolio_requesting_entity/" + # Test missing suborganization selection form["portfolio_requesting_entity-requesting_entity_is_suborganization"] = True - form["portfolio_requesting_entity-sub_organization"] = "" - - response = form.submit() + form["portfolio_requesting_entity-is_requesting_new_suborganization"] = False + # remove sub_organization from the form submission + form_data = form.submit_fields() + form_data = [(key, value) for key, value in form_data if key != "portfolio_requesting_entity-sub_organization"] + response = self.app.post(form_action, dict(form_data)) self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) self.assertContains(response, "Suborganization is required.", status_code=200) # Test missing custom suborganization details + form["portfolio_requesting_entity-requesting_entity_is_suborganization"] = True form["portfolio_requesting_entity-is_requesting_new_suborganization"] = True - response = form.submit() - self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + form["portfolio_requesting_entity-sub_organization"] = "other" + # remove suborganization_state_territory from the form submission + form_data = form.submit_fields() + form_data = [ + (key, value) + for key, value in form_data + if key != "portfolio_requesting_entity-suborganization_state_territory" + ] + response = self.app.post(form_action, dict(form_data)) self.assertContains(response, "Enter the name of your suborganization.", status_code=200) self.assertContains(response, "Enter the city where your suborganization is located.", status_code=200) self.assertContains( From 6bf52f00150daeaee062c795382cb8f07282c0f7 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 20 Jan 2025 07:10:12 -0500 Subject: [PATCH 37/40] cleanup --- src/registrar/forms/domain.py | 1 - src/registrar/tests/test_views_portfolio.py | 2 +- src/registrar/views/domain_request.py | 1 + 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 5eeae232d..711315632 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -164,7 +164,6 @@ class DomainSuborganizationForm(forms.ModelForm): sub_organization = forms.ModelChoiceField( label="Suborganization name", queryset=Suborganization.objects.none(), - empty_label="⎯ (No suborganization)", required=False, widget=ComboboxWidget, ) diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index e2de3328f..69502d683 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2933,7 +2933,7 @@ class TestRequestingEntity(WebTest): @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) - # @less_console_noise_decorator + @less_console_noise_decorator def test_requesting_entity_page_errors(self): """Tests that we get the expected form errors on requesting entity""" domain_request = completed_domain_request(user=self.user, portfolio=self.portfolio) diff --git a/src/registrar/views/domain_request.py b/src/registrar/views/domain_request.py index bff3e5c00..9754b0d0c 100644 --- a/src/registrar/views/domain_request.py +++ b/src/registrar/views/domain_request.py @@ -368,6 +368,7 @@ class DomainRequestWizard(DomainRequestWizardPermissionView, TemplateView): and from the database if `use_db` is True (provided that record exists). An empty form will be provided if neither of those are true. """ + kwargs = { "files": files, "prefix": self.steps.current, From e9268bea8b3883a6ae62d578617b168c5f36d107 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 22 Jan 2025 13:17:22 -0500 Subject: [PATCH 38/40] updated required attribute on state territory --- src/registrar/forms/domain.py | 2 +- src/registrar/forms/portfolio.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 711315632..20091a325 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -471,7 +471,7 @@ class DomainOrgNameAddressForm(forms.ModelForm): error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, - widget=ComboboxWidget(), + widget=ComboboxWidget(attrs={"required":True}), ) class Meta: diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 9d6c9ecdf..379e8053c 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -41,7 +41,7 @@ class PortfolioOrgAddressForm(forms.ModelForm): error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, - widget=ComboboxWidget, + widget=ComboboxWidget(attrs={"required":True}), ) class Meta: From 5a30d28b34307e62432e8a7ed841baaa921b03a6 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 22 Jan 2025 13:20:11 -0700 Subject: [PATCH 39/40] cleanup logs --- src/registrar/models/domain.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index d024efe5f..cb481db7a 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1329,14 +1329,14 @@ class Domain(TimeStampedModel, DomainHelper): def get_default_administrative_contact(self): """Gets the default administrative contact.""" - logger.info("get_default_administrative_contact() -> Adding administrative security contact") + logger.info("get_default_administrative_contact() -> Adding default administrative contact") contact = PublicContact.get_default_administrative() contact.domain = self return contact def get_default_technical_contact(self): """Gets the default technical contact.""" - logger.info("get_default_security_contact() -> Adding technical security contact") + logger.info("get_default_security_contact() -> Adding default technical contact") contact = PublicContact.get_default_technical() contact.domain = self return contact From 6828a1f1ff022a4da0c58b8e17f34d1d6320f439 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Wed, 22 Jan 2025 15:33:08 -0500 Subject: [PATCH 40/40] lint --- src/registrar/forms/domain.py | 2 +- src/registrar/forms/portfolio.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain.py b/src/registrar/forms/domain.py index 20091a325..05eb90db3 100644 --- a/src/registrar/forms/domain.py +++ b/src/registrar/forms/domain.py @@ -471,7 +471,7 @@ class DomainOrgNameAddressForm(forms.ModelForm): error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, - widget=ComboboxWidget(attrs={"required":True}), + widget=ComboboxWidget(attrs={"required": True}), ) class Meta: diff --git a/src/registrar/forms/portfolio.py b/src/registrar/forms/portfolio.py index 379e8053c..e57b56c4f 100644 --- a/src/registrar/forms/portfolio.py +++ b/src/registrar/forms/portfolio.py @@ -41,7 +41,7 @@ class PortfolioOrgAddressForm(forms.ModelForm): error_messages={ "required": ("Select the state, territory, or military post where your organization is located.") }, - widget=ComboboxWidget(attrs={"required":True}), + widget=ComboboxWidget(attrs={"required": True}), ) class Meta: