From 2651cd4abaebbe84c3ed762ce3bf99f28ab86122 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Thu, 2 Jan 2025 16:36:36 -0500 Subject: [PATCH 01/82] 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/82] 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/82] 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/82] 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/82] 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/82] 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/82] 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/82] 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/82] 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/82] 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 1bc83a1c3e44aeb9d0d78b07aff4eab5db48caa9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 10 Jan 2025 15:32:00 -0500 Subject: [PATCH 11/82] normalize summary box --- .../assets/src/sass/_theme/_admin.scss | 21 +++++++++++-------- .../assets/src/sass/_theme/_base.scss | 1 - .../assets/src/sass/_theme/_summary-box.scss | 15 +++++++++++++ .../assets/src/sass/_theme/styles.scss | 1 + 4 files changed, 28 insertions(+), 10 deletions(-) create mode 100644 src/registrar/assets/src/sass/_theme/_summary-box.scss diff --git a/src/registrar/assets/src/sass/_theme/_admin.scss b/src/registrar/assets/src/sass/_theme/_admin.scss index a71804d77..afdb39551 100644 --- a/src/registrar/assets/src/sass/_theme/_admin.scss +++ b/src/registrar/assets/src/sass/_theme/_admin.scss @@ -812,16 +812,19 @@ div.dja__model-description{ text-decoration: underline !important; } -//-- Override some styling for the USWDS summary box (per design quidance for ticket #2055 -.usa-summary-box { - background: #{$dhs-blue-10}; - border-color: #{$dhs-blue-30}; - max-width: 72ex; - word-wrap: break-word; -} +//-- Override some styling for the USWDS summary box (per design guidance for ticket #2055) +// Keep it scoped to admin.scss +.dashboard { + .usa-summary-box { + background: #{$dhs-blue-10}; + border-color: #{$dhs-blue-30}; + max-width: 72ex; + word-wrap: break-word; + } -.usa-summary-box h3 { - color: #{$dhs-blue-60}; + .usa-summary-box h3 { + color: #{$dhs-blue-60}; + } } .module caption, .inline-group h2 { diff --git a/src/registrar/assets/src/sass/_theme/_base.scss b/src/registrar/assets/src/sass/_theme/_base.scss index d73becd75..0e726ff37 100644 --- a/src/registrar/assets/src/sass/_theme/_base.scss +++ b/src/registrar/assets/src/sass/_theme/_base.scss @@ -59,7 +59,6 @@ body { } h2 { - color: color('primary-dark'); margin-top: units(2); margin-bottom: units(2); } diff --git a/src/registrar/assets/src/sass/_theme/_summary-box.scss b/src/registrar/assets/src/sass/_theme/_summary-box.scss new file mode 100644 index 000000000..91693ff02 --- /dev/null +++ b/src/registrar/assets/src/sass/_theme/_summary-box.scss @@ -0,0 +1,15 @@ +// USWDS override to basically match the header size to a standard h3 size +// This get complicated because USWDS sets a size on the container then a relative +// size on the header. We'll need to reset the container size, override the header size, +// then 'fix' the content size. +.usa-summary-box { + font-size: 1rem; + .usa-summary-box__heading { + font-size: 1.17em; + } + p, li, dd { + font-size: 1.06rem; + } +} + + \ No newline at end of file diff --git a/src/registrar/assets/src/sass/_theme/styles.scss b/src/registrar/assets/src/sass/_theme/styles.scss index 78d27b2e0..493ebd542 100644 --- a/src/registrar/assets/src/sass/_theme/styles.scss +++ b/src/registrar/assets/src/sass/_theme/styles.scss @@ -17,6 +17,7 @@ @forward "forms"; @forward "search"; @forward "tooltips"; +@forward "summary-box"; @forward "fieldsets"; @forward "alerts"; @forward "tables"; From 7dbb9de3b9925bf955a683132094ca7835e213d7 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 10 Jan 2025 16:18:11 -0500 Subject: [PATCH 12/82] use typeset mixin instead of hardcoded values in summary-box --- src/registrar/assets/src/sass/_theme/_summary-box.scss | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_summary-box.scss b/src/registrar/assets/src/sass/_theme/_summary-box.scss index 91693ff02..fdaf165e2 100644 --- a/src/registrar/assets/src/sass/_theme/_summary-box.scss +++ b/src/registrar/assets/src/sass/_theme/_summary-box.scss @@ -5,10 +5,12 @@ .usa-summary-box { font-size: 1rem; .usa-summary-box__heading { - font-size: 1.17em; + // 1.17em / 18.72px + @include typeset('sans', 'sm', 6); } p, li, dd { - font-size: 1.06rem; + // 1.06rem / 16.96px + @include typeset('sans', 'sm', 5); } } From ae6c461ddb33f67c0b7bea042a4ca6594e30e669 Mon Sep 17 00:00:00 2001 From: David Kennedy Date: Mon, 13 Jan 2025 08:00:02 -0500 Subject: [PATCH 13/82] 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 14/82] 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 0e9d69a26f8eeab84c0c7a73e3f97b86fc10848d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 13 Jan 2025 15:43:37 -0500 Subject: [PATCH 15/82] clean up summary boxes --- .../assets/src/sass/_theme/_admin.scss | 19 ------------ .../assets/src/sass/_theme/_base.scss | 10 ------- .../assets/src/sass/_theme/_summary-box.scss | 25 +++++++--------- .../assets/src/sass/_theme/_typography.scss | 6 ++-- .../admin/domain_delete_confirmation.html | 2 +- .../domain_delete_selected_confirmation.html | 2 +- src/registrar/templates/domain_detail.html | 25 +++++++--------- src/registrar/templates/domain_dnssec.html | 30 ++++++++++--------- .../domain_request_awaiting_review.html | 4 +-- .../includes/profile_information.html | 2 +- .../includes/request_status_manage.html | 24 +++++++-------- 11 files changed, 56 insertions(+), 93 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_admin.scss b/src/registrar/assets/src/sass/_theme/_admin.scss index afdb39551..7ffd6d6b1 100644 --- a/src/registrar/assets/src/sass/_theme/_admin.scss +++ b/src/registrar/assets/src/sass/_theme/_admin.scss @@ -516,10 +516,6 @@ input[type=submit].button--dja-toolbar:focus, input[type=submit].button--dja-too max-width: 68ex; } -.usa-summary-box__dhs-color { - color: $dhs-blue-70; -} - details.dja-detail-table { display: inline-table; background-color: var(--body-bg); @@ -812,21 +808,6 @@ div.dja__model-description{ text-decoration: underline !important; } -//-- Override some styling for the USWDS summary box (per design guidance for ticket #2055) -// Keep it scoped to admin.scss -.dashboard { - .usa-summary-box { - background: #{$dhs-blue-10}; - border-color: #{$dhs-blue-30}; - max-width: 72ex; - word-wrap: break-word; - } - - .usa-summary-box h3 { - color: #{$dhs-blue-60}; - } -} - .module caption, .inline-group h2 { text-transform: capitalize; } diff --git a/src/registrar/assets/src/sass/_theme/_base.scss b/src/registrar/assets/src/sass/_theme/_base.scss index 0e726ff37..60018511f 100644 --- a/src/registrar/assets/src/sass/_theme/_base.scss +++ b/src/registrar/assets/src/sass/_theme/_base.scss @@ -129,16 +129,6 @@ grid column to the max-width of the searchbar, which was calculated to be 33rem. word-break: break-word; } -.dotgov-status-box { - background-color: color('primary-lightest'); - border-color: color('accent-cool-lighter'); -} - -.dotgov-status-box--action-need { - background-color: color('warning-lighter'); - border-color: color('warning'); -} - footer { border-top: 1px solid color('primary-darker'); } diff --git a/src/registrar/assets/src/sass/_theme/_summary-box.scss b/src/registrar/assets/src/sass/_theme/_summary-box.scss index fdaf165e2..112829e6c 100644 --- a/src/registrar/assets/src/sass/_theme/_summary-box.scss +++ b/src/registrar/assets/src/sass/_theme/_summary-box.scss @@ -1,17 +1,12 @@ -// USWDS override to basically match the header size to a standard h3 size -// This get complicated because USWDS sets a size on the container then a relative -// size on the header. We'll need to reset the container size, override the header size, -// then 'fix' the content size. -.usa-summary-box { - font-size: 1rem; - .usa-summary-box__heading { - // 1.17em / 18.72px - @include typeset('sans', 'sm', 6); - } - p, li, dd { - // 1.06rem / 16.96px - @include typeset('sans', 'sm', 5); - } -} +@use "uswds-core" as *; +.usa-summary-box { + background-color: color('primary-lightest'); + border-color: color('accent-cool-lighter'); +} + +.usa-summary-box--action-needed { + background-color: color('warning-lighter'); + border-color: color('warning'); +} \ No newline at end of file diff --git a/src/registrar/assets/src/sass/_theme/_typography.scss b/src/registrar/assets/src/sass/_theme/_typography.scss index db19a595b..3fb61ccfd 100644 --- a/src/registrar/assets/src/sass/_theme/_typography.scss +++ b/src/registrar/assets/src/sass/_theme/_typography.scss @@ -10,17 +10,19 @@ address, max-width: measure(5); } +h1, h2, h3, h4, h5, h6 { + color: color('primary-darker'); +} + h1 { @include typeset('sans', '2xl', 2); margin: 0 0 units(2); - color: color('primary-darker'); } h2 { font-weight: font-weight('semibold'); line-height: line-height('heading', 3); margin: units(4) 0 units(1); - color: color('primary-darker'); } .header--body { diff --git a/src/registrar/templates/django/admin/domain_delete_confirmation.html b/src/registrar/templates/django/admin/domain_delete_confirmation.html index 5a9bef5b0..3228188d7 100644 --- a/src/registrar/templates/django/admin/domain_delete_confirmation.html +++ b/src/registrar/templates/django/admin/domain_delete_confirmation.html @@ -8,7 +8,7 @@ aria-labelledby="summary-box-description" >
-

+

When a domain is deleted:

diff --git a/src/registrar/templates/django/admin/domain_delete_selected_confirmation.html b/src/registrar/templates/django/admin/domain_delete_selected_confirmation.html index 3e0a32a4d..68f726708 100644 --- a/src/registrar/templates/django/admin/domain_delete_selected_confirmation.html +++ b/src/registrar/templates/django/admin/domain_delete_selected_confirmation.html @@ -9,7 +9,7 @@ aria-labelledby="summary-box-description" >
-

+

When a domain is deleted:

diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index a5b8e52cb..56c46ed0f 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -23,19 +23,15 @@

{{ domain.name }}

-

- - Status: - - - +

+ Status: {# UNKNOWN domains would not have an expiration date and thus would show 'Expired' #} {% if domain.is_expired and domain.state != domain.State.UNKNOWN %} Expired @@ -46,9 +42,10 @@ {% else %} {{ domain.state|title }} {% endif %} - +

+ {% if domain.get_state_help_text %} -
+

{% if has_domain_renewal_flag and domain.is_expiring and is_domain_manager %} This domain will expire soon. Renew to maintain access. {% elif has_domain_renewal_flag and domain.is_expiring and is_portfolio_user %} @@ -56,13 +53,11 @@ {% else %} {{ domain.get_state_help_text }} {% endif %} -

+

{% endif %} -

+
-
-
- +
{% include "includes/domain_dates.html" %} diff --git a/src/registrar/templates/domain_dnssec.html b/src/registrar/templates/domain_dnssec.html index a795fb2fc..a55bd8d23 100644 --- a/src/registrar/templates/domain_dnssec.html +++ b/src/registrar/templates/domain_dnssec.html @@ -33,23 +33,25 @@
{% csrf_token %} - {% if has_dnssec_records %} +
-

-

To fully disable DNSSEC

-
    -
  • Click “Disable DNSSEC” below.
    • -
  • Wait until the Time to Live (TTL) expires on your DNSSEC records managed by your DNS hosting provider. This is often less than 24 hours, but confirm with your provider.
    • -
  • After the TTL expiration, disable DNSSEC at your DNS hosting provider.
    • -
-

Warning: If you disable DNSSEC at your DNS hosting provider before TTL expiration, this may cause your domain to appear offline.

+

To fully disable DNSSEC

+ +
+
    +
  • Click “Disable DNSSEC” below.
    • +
  • Wait until the Time to Live (TTL) expires on your DNSSEC records managed by your DNS hosting provider. This is often less than 24 hours, but confirm with your provider.
    • +
  • After the TTL expiration, disable DNSSEC at your DNS hosting provider.
    • +
+

Warning: If you disable DNSSEC at your DNS hosting provider before TTL expiration, this may cause your domain to appear offline.

+
+

DNSSEC is enabled on your domain

@@ -60,7 +62,7 @@ data-open-modal >Disable DNSSEC - {% else %} +
@@ -69,7 +71,7 @@
Enable DNSSEC
- {% endif %} +
+

Next steps in this process

We received your .gov domain request. Our next step is to review your request. This usually takes 30 business days. We’ll email you if we have questions and when we complete our review. Contact us with any questions.

{% if show_withdraw_text %} -

+

Need to make changes?

diff --git a/src/registrar/templates/includes/profile_information.html b/src/registrar/templates/includes/profile_information.html index 3e7c827f1..257e8d1dc 100644 --- a/src/registrar/templates/includes/profile_information.html +++ b/src/registrar/templates/includes/profile_information.html @@ -12,7 +12,7 @@ Your contact information
-
    +
    • Full name: {{ user.get_formatted_name }}
    • Organization email: {{ user.email }}
    • Title or role in your organization: {{ user.title }}
      • diff --git a/src/registrar/templates/includes/request_status_manage.html b/src/registrar/templates/includes/request_status_manage.html index b4738d0d0..17083b360 100644 --- a/src/registrar/templates/includes/request_status_manage.html +++ b/src/registrar/templates/includes/request_status_manage.html @@ -39,22 +39,20 @@ {% block status_summary %}
      -
      -

      - - Status: - - {{ DomainRequest.get_status_display|default:"ERROR Please contact technical support/dev" }} -

      +
      +
      +

      + Status: + {{ DomainRequest.get_status_display|default:"ERROR Please contact technical support/dev" }} +

      +
      +
      -
      -
      {% endblock status_summary %} {% block status_metadata %} @@ -142,7 +140,7 @@
      {% block request_summary_header %} -

      Summary of your domain request

      +

      Summary of your domain request

      {% endblock request_summary_header%} {% block request_summary %} 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 16/82] 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 17/82] 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 18/82] 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 20e8ad5e1e3f276a10aec6507243ffd8a7121597 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 14 Jan 2025 14:08:28 -0500 Subject: [PATCH 19/82] header revision wip --- .../assets/src/sass/_theme/_admin.scss | 2 +- .../assets/src/sass/_theme/_base.scss | 8 ++- .../src/sass/_theme/_register-form.scss | 18 +---- .../assets/src/sass/_theme/_typography.scss | 6 +- src/registrar/templates/domain_detail.html | 2 +- .../templates/includes/domain_dates.html | 2 +- .../domain_request_awaiting_review.html | 4 +- .../includes/member_domain_management.html | 2 +- .../includes/member_permissions.html | 6 +- .../includes/request_status_manage.html | 69 +++++++------------ .../templates/includes/summary_item.html | 6 +- .../portfolio_member_permissions.html | 8 +-- .../templates/portfolio_members_add_new.html | 9 +-- 13 files changed, 54 insertions(+), 88 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_admin.scss b/src/registrar/assets/src/sass/_theme/_admin.scss index 7ffd6d6b1..34ab3024d 100644 --- a/src/registrar/assets/src/sass/_theme/_admin.scss +++ b/src/registrar/assets/src/sass/_theme/_admin.scss @@ -188,7 +188,7 @@ html[data-theme="dark"] { } #branding h1, -h1, h2, h3, +.dashboard h1, .dashboard h2, .dashboard h3, .module h2 { font-weight: font-weight('bold'); } diff --git a/src/registrar/assets/src/sass/_theme/_base.scss b/src/registrar/assets/src/sass/_theme/_base.scss index 60018511f..76c971709 100644 --- a/src/registrar/assets/src/sass/_theme/_base.scss +++ b/src/registrar/assets/src/sass/_theme/_base.scss @@ -270,4 +270,10 @@ abbr[title] { .maxw-fit-content { max-width: fit-content; -} \ No newline at end of file +} + +.summary-item__title { + // match h3 at 18.72px and semibold + font-size: 1.17em; + font-weight: 600; +} diff --git a/src/registrar/assets/src/sass/_theme/_register-form.scss b/src/registrar/assets/src/sass/_theme/_register-form.scss index fcc5b5ae6..ddd162383 100644 --- a/src/registrar/assets/src/sass/_theme/_register-form.scss +++ b/src/registrar/assets/src/sass/_theme/_register-form.scss @@ -64,26 +64,10 @@ margin-top: units(3); } - .summary-item hr, +.summary-item hr, .review__step hr { border: none; //reset border-top: 1px solid color('primary-dark'); margin-top: 0; margin-bottom: units(0.5); } - -.review__step__title a:visited { - color: color('primary'); -} - -.review__step__name { - color: color('primary-dark'); - font-weight: font-weight('semibold'); - margin-bottom: units(0.5); -} - -.review__step__subheading { - color: color('primary-dark'); - font-weight: font-weight('semibold'); - margin-bottom: units(0.5); -} diff --git a/src/registrar/assets/src/sass/_theme/_typography.scss b/src/registrar/assets/src/sass/_theme/_typography.scss index 3fb61ccfd..457c1a367 100644 --- a/src/registrar/assets/src/sass/_theme/_typography.scss +++ b/src/registrar/assets/src/sass/_theme/_typography.scss @@ -20,11 +20,13 @@ h1 { } h2 { - font-weight: font-weight('semibold'); - line-height: line-height('heading', 3); margin: units(4) 0 units(1); } +h3, h4 { + font-weight: font-weight('semibold'); +} + .header--body { margin-top: units(2); font-weight: font-weight('semibold'); diff --git a/src/registrar/templates/domain_detail.html b/src/registrar/templates/domain_detail.html index 56c46ed0f..2776ac40a 100644 --- a/src/registrar/templates/domain_detail.html +++ b/src/registrar/templates/domain_detail.html @@ -30,7 +30,7 @@
      -

      +

      Status: {# UNKNOWN domains would not have an expiration date and thus would show 'Expired' #} {% if domain.is_expired and domain.state != domain.State.UNKNOWN %} diff --git a/src/registrar/templates/includes/domain_dates.html b/src/registrar/templates/includes/domain_dates.html index c05e202e1..b14c091d0 100644 --- a/src/registrar/templates/includes/domain_dates.html +++ b/src/registrar/templates/includes/domain_dates.html @@ -1,5 +1,5 @@ {% if domain.expiration_date or domain.created_at %} -

      +

      {% if domain.expiration_date %} Expires: {{ domain.expiration_date|date }} diff --git a/src/registrar/templates/includes/domain_request_awaiting_review.html b/src/registrar/templates/includes/domain_request_awaiting_review.html index 7d04639aa..852d6bd60 100644 --- a/src/registrar/templates/includes/domain_request_awaiting_review.html +++ b/src/registrar/templates/includes/domain_request_awaiting_review.html @@ -1,12 +1,12 @@ {% load url_helpers %} -

      +

      Next steps in this process

      We received your .gov domain request. Our next step is to review your request. This usually takes 30 business days. We’ll email you if we have questions and when we complete our review. Contact us with any questions.

      {% if show_withdraw_text %} -

      +

      Need to make changes?

      diff --git a/src/registrar/templates/includes/member_domain_management.html b/src/registrar/templates/includes/member_domain_management.html index 6bf3f1320..1e5b29994 100644 --- a/src/registrar/templates/includes/member_domain_management.html +++ b/src/registrar/templates/includes/member_domain_management.html @@ -1,4 +1,4 @@ -

      Assigned domains

      +

      Assigned domains

      {% if domain_count > 0 %}

      {{domain_count}}

      {% else %} diff --git a/src/registrar/templates/includes/member_permissions.html b/src/registrar/templates/includes/member_permissions.html index 8cf75cfbf..4833b5e4b 100644 --- a/src/registrar/templates/includes/member_permissions.html +++ b/src/registrar/templates/includes/member_permissions.html @@ -1,4 +1,4 @@ -

      Member access

      +

      Member access

      {% if permissions.roles and 'organization_admin' in permissions.roles %}

      Admin access

      {% elif permissions.roles and 'organization_member' in permissions.roles %} @@ -7,7 +7,7 @@

      ⎯

      {% endif %} -

      Organization domain requests

      +

      Organization domain requests

      {% if member_has_edit_request_portfolio_permission %}

      View all requests plus create requests

      {% elif member_has_view_all_requests_portfolio_permission %} @@ -16,7 +16,7 @@

      No access

      {% endif %} -

      Organization members

      +

      Organization members

      {% if member_has_edit_members_portfolio_permission %}

      View all members plus manage members

      {% elif member_has_view_members_portfolio_permission %} diff --git a/src/registrar/templates/includes/request_status_manage.html b/src/registrar/templates/includes/request_status_manage.html index 17083b360..f50d31bfc 100644 --- a/src/registrar/templates/includes/request_status_manage.html +++ b/src/registrar/templates/includes/request_status_manage.html @@ -59,12 +59,12 @@ {% if portfolio %} {% if DomainRequest.creator %} -

      - Created by: {{DomainRequest.creator.email|default:DomainRequest.creator.get_formatted_name }} +

      + Created by: {{DomainRequest.creator.email|default:DomainRequest.creator.get_formatted_name }}

      {% else %} -

      - No creator found: this is an error, please email help@get.gov. +

      + No creator found: this is an error, please email help@get.gov.

      {% endif %} {% endif %} @@ -75,49 +75,32 @@ There is some code repetition, but it gives us more flexibility rather than a dense reduction. Leave it this way until we've solidified our requirements. {% endcomment %} - {% if DomainRequest.status == statuses.STARTED %} - {% with first_started_date=DomainRequest.get_first_status_started_date|date:"F j, Y" %} -

      + +

      + {% if DomainRequest.status == statuses.STARTED %} + {% with first_started_date=DomainRequest.get_first_status_started_date|date:"F j, Y" %} {% comment %} A newly created domain request will not have a value for last_status update. This is because the status never really updated. However, if this somehow goes back to started we can default to displaying that new date. {% endcomment %} - Started on: {{last_status_update|default:first_started_date}} -

      - {% endwith %} - {% elif DomainRequest.status == statuses.SUBMITTED %} -

      - Submitted on: {{last_submitted|default:first_submitted }} -

      -

      - Last updated on: {{DomainRequest.updated_at|date:"F j, Y"}} -

      - {% elif DomainRequest.status == statuses.ACTION_NEEDED %} -

      - Submitted on: {{last_submitted|default:first_submitted }} -

      -

      - Last updated on: {{DomainRequest.updated_at|date:"F j, Y"}} -

      - {% elif DomainRequest.status == statuses.REJECTED %} -

      - Submitted on: {{last_submitted|default:first_submitted }} -

      -

      - Rejected on: {{last_status_update}} -

      - {% elif DomainRequest.status == statuses.WITHDRAWN %} -

      - Submitted on: {{last_submitted|default:first_submitted }} -

      -

      - Withdrawn on: {{last_status_update}} -

      - {% else %} - {% comment %} Shown for in_review, approved, ineligible {% endcomment %} -

      - Last updated on: {{DomainRequest.updated_at|date:"F j, Y"}} + Started on: {{last_status_update|default:first_started_date}} + {% endwith %} + {% elif DomainRequest.status == statuses.SUBMITTED %} + Submitted on: {{last_submitted|default:first_submitted }}
      + Last updated on: {{DomainRequest.updated_at|date:"F j, Y"}} + {% elif DomainRequest.status == statuses.ACTION_NEEDED %} + Submitted on: {{last_submitted|default:first_submitted }}
      + Last updated on: {{DomainRequest.updated_at|date:"F j, Y"}} + {% elif DomainRequest.status == statuses.REJECTED %} + Submitted on: {{last_submitted|default:first_submitted }}
      + Rejected on: {{last_status_update}} + {% elif DomainRequest.status == statuses.WITHDRAWN %} + Submitted on: {{last_submitted|default:first_submitted }}
      + Withdrawn on: {{last_status_update}} + {% else %} + {% comment %} Shown for in_review, approved, ineligible {% endcomment %} + Last updated on: {{DomainRequest.updated_at|date:"F j, Y"}}

      {% endif %} {% endwith %} @@ -125,7 +108,7 @@ {% block status_blurb %} {% if DomainRequest.is_awaiting_review %} -

      {% include "includes/domain_request_awaiting_review.html" with show_withdraw_text=DomainRequest.is_withdrawable %}

      + {% include "includes/domain_request_awaiting_review.html" with show_withdraw_text=DomainRequest.is_withdrawable %} {% endif %} {% endblock status_blurb %} diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index bbdfc8dee..0b0cea3c1 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -10,8 +10,6 @@

      @@ -41,8 +39,6 @@

      Contact {{forloop.counter}} @@ -143,7 +139,7 @@

      Admin access permissions

      Member permissions available for admin-level acccess.

      -

      Organization domain requests

      {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} {% input_with_errors form.domain_request_permission_admin %} {% endwith %} -

      Organization members

      {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} @@ -116,7 +114,7 @@

      Basic member permissions

      Member permissions available for basic-level acccess.

      -

      Organization domain requests

      +

      Organization domain requests

      {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} {% input_with_errors form.domain_request_permission_member %} {% endwith %} diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index 092a9af31..d44233f49 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -68,15 +68,13 @@

      Admin access permissions

      Member permissions available for admin-level acccess.

      -

      Organization domain requests

      {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} {% input_with_errors form.domain_request_permission_admin %} {% endwith %} -

      Organization members

      {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} @@ -127,8 +125,7 @@

      Invite this member to the organization?

      -

      Member information and permissions

      +

      Member information and permissions

      Email

      From 3337e5dc9c011df846ec449d201c14961a0b03a9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 14 Jan 2025 14:46:47 -0500 Subject: [PATCH 20/82] Header refactor wip --- .../js/getgov/table-edit-member-domains.js | 6 ++-- .../assets/src/sass/_theme/_base.scss | 6 ---- .../assets/src/sass/_theme/_forms.scss | 18 +++++----- .../src/sass/_theme/_register-form.scss | 4 --- .../assets/src/sass/_theme/_typography.scss | 33 +++++++------------ src/registrar/templates/domain_dnssec.html | 6 ++-- .../templates/includes/input_read_only.html | 4 +-- .../portfolio_request_review_steps.html | 2 +- .../includes/request_review_steps.html | 10 +++--- .../includes/request_status_manage.html | 4 +-- .../templates/includes/summary_item.html | 6 ++-- .../portfolio_member_permissions.html | 2 +- .../templates/portfolio_members_add_new.html | 2 +- .../templates/portfolio_organization.html | 8 ++--- src/registrar/tests/test_views_portfolio.py | 12 +++---- 15 files changed, 50 insertions(+), 73 deletions(-) diff --git a/src/registrar/assets/src/js/getgov/table-edit-member-domains.js b/src/registrar/assets/src/js/getgov/table-edit-member-domains.js index 86aa39c37..4f0b1d610 100644 --- a/src/registrar/assets/src/js/getgov/table-edit-member-domains.js +++ b/src/registrar/assets/src/js/getgov/table-edit-member-domains.js @@ -259,7 +259,7 @@ export class EditMemberDomainsTable extends BaseTable { // Append unassigned domains section if (this.removedDomains.length) { const unassignedHeader = document.createElement('h3'); - unassignedHeader.classList.add('header--body', 'text-primary', 'margin-bottom-1'); + unassignedHeader.classList.add('margin-bottom-1'); unassignedHeader.textContent = 'Unassigned domains'; domainAssignmentSummary.appendChild(unassignedHeader); domainAssignmentSummary.appendChild(unassignedDomainsList); @@ -268,7 +268,7 @@ export class EditMemberDomainsTable extends BaseTable { // Append assigned domains section if (this.addedDomains.length) { const assignedHeader = document.createElement('h3'); - assignedHeader.classList.add('header--body', 'text-primary', 'margin-bottom-1'); + assignedHeader.classList.add('margin-bottom-1'); assignedHeader.textContent = 'Assigned domains'; domainAssignmentSummary.appendChild(assignedHeader); domainAssignmentSummary.appendChild(assignedDomainsList); @@ -276,7 +276,7 @@ export class EditMemberDomainsTable extends BaseTable { // Append total assigned domains section const totalHeader = document.createElement('h3'); - totalHeader.classList.add('header--body', 'text-primary', 'margin-bottom-1'); + totalHeader.classList.add('margin-bottom-1'); totalHeader.textContent = 'Total assigned domains'; domainAssignmentSummary.appendChild(totalHeader); const totalCount = document.createElement('p'); diff --git a/src/registrar/assets/src/sass/_theme/_base.scss b/src/registrar/assets/src/sass/_theme/_base.scss index 76c971709..fa4005cfd 100644 --- a/src/registrar/assets/src/sass/_theme/_base.scss +++ b/src/registrar/assets/src/sass/_theme/_base.scss @@ -271,9 +271,3 @@ abbr[title] { .maxw-fit-content { max-width: fit-content; } - -.summary-item__title { - // match h3 at 18.72px and semibold - font-size: 1.17em; - font-weight: 600; -} diff --git a/src/registrar/assets/src/sass/_theme/_forms.scss b/src/registrar/assets/src/sass/_theme/_forms.scss index 4138c5878..10fcd870c 100644 --- a/src/registrar/assets/src/sass/_theme/_forms.scss +++ b/src/registrar/assets/src/sass/_theme/_forms.scss @@ -2,6 +2,14 @@ @use "cisa_colors" as *; @use "typography" as *; +// Normalize typography in forms +.usa-form, +.usa-form fieldset { + font-size: 1rem; + .usa-legend { + font-size: 1rem; + } +} .usa-form .usa-button { margin-top: units(3); } @@ -69,16 +77,6 @@ legend.float-left-tablet + button.float-right-tablet { } } -.read-only-label { - @extend .h4--sm-05; - font-weight: bold; - color: color('primary-dark'); -} - -.read-only-value { - margin-top: units(0); -} - .bg-gray-1 .usa-radio { background: color('gray-1'); } diff --git a/src/registrar/assets/src/sass/_theme/_register-form.scss b/src/registrar/assets/src/sass/_theme/_register-form.scss index ddd162383..9b29029b8 100644 --- a/src/registrar/assets/src/sass/_theme/_register-form.scss +++ b/src/registrar/assets/src/sass/_theme/_register-form.scss @@ -12,11 +12,7 @@ margin-top: units(1); } -// header--body is used on the summary page and -// should not be styled like the register form headers .register-form-step h3 { - color: color('primary-dark'); - letter-spacing: $letter-space--xs; margin-top: units(3); margin-bottom: 0; diff --git a/src/registrar/assets/src/sass/_theme/_typography.scss b/src/registrar/assets/src/sass/_theme/_typography.scss index 457c1a367..b796ad20d 100644 --- a/src/registrar/assets/src/sass/_theme/_typography.scss +++ b/src/registrar/assets/src/sass/_theme/_typography.scss @@ -15,6 +15,7 @@ h1, h2, h3, h4, h5, h6 { } h1 { + font-size: 2.125rem; @include typeset('sans', '2xl', 2); margin: 0 0 units(2); } @@ -23,35 +24,23 @@ h2 { margin: units(4) 0 units(1); } -h3, h4 { +h3, .h3 { + font-size: 1.25rem; font-weight: font-weight('semibold'); } -.header--body { - margin-top: units(2); +h4, .h4 { + font-size: 1.15rem; font-weight: font-weight('semibold'); - // The units mixin can only get us close, so it's between - // hardcoding the value and using in markup - font-size: 16.96px; -} - -.h4--sm-05 { - font-size: size('body', 'sm'); - font-weight: normal; - color: color('primary'); - margin-bottom: units(0.5); -} - -// Normalize typography in forms -.usa-form, -.usa-form fieldset { - font-size: 1rem; - .usa-legend { - font-size: 1rem; - } } .p--blockquote { padding-left: units(1); border-left: 2px solid color('base-lighter'); } + +.summary-item__title { + // match h3 at 18.72px and semibold + font-size: 1.25em; + font-weight: 600; +} diff --git a/src/registrar/templates/domain_dnssec.html b/src/registrar/templates/domain_dnssec.html index a55bd8d23..3beb2548e 100644 --- a/src/registrar/templates/domain_dnssec.html +++ b/src/registrar/templates/domain_dnssec.html @@ -33,7 +33,7 @@
      {% csrf_token %} - + {% if has_dnssec_records %}
      Disable DNSSEC - + {% else %}
      @@ -71,7 +71,7 @@
      Enable DNSSEC
      - + {% endif %}
      {{ field.label }}

      +

      {{ field.label }}

      {% if label_description %}

      {{ label_description }}

      {% endif %} @@ -11,4 +11,4 @@ Template include for read-only form fields This allows us to customize the displayed value. For instance, Select fields will display the id by default. {% endcomment %} -

      {{ value|default:field.value }}

      +

      {{ value|default:field.value }}

      diff --git a/src/registrar/templates/includes/portfolio_request_review_steps.html b/src/registrar/templates/includes/portfolio_request_review_steps.html index eecc5005a..c9bd99607 100644 --- a/src/registrar/templates/includes/portfolio_request_review_steps.html +++ b/src/registrar/templates/includes/portfolio_request_review_steps.html @@ -46,7 +46,7 @@ {% endwith %} {% if domain_request.alternative_domains.all %} -

      Alternative domains

      +

      Alternative domains

        {% for site in domain_request.alternative_domains.all %}
      • {{ site.website }}
        • diff --git a/src/registrar/templates/includes/request_review_steps.html b/src/registrar/templates/includes/request_review_steps.html index dada2dffb..42b5cda46 100644 --- a/src/registrar/templates/includes/request_review_steps.html +++ b/src/registrar/templates/includes/request_review_steps.html @@ -88,7 +88,7 @@ {% endwith %} {% if domain_request.alternative_domains.all %} -

        Alternative domains

        +

        Alternative domains

          {% for site in domain_request.alternative_domains.all %}
        • {{ site.website }}
          • @@ -132,8 +132,8 @@ {% with title=form_titles|get_item:step %} {% if domain_request.has_additional_details %} {% include "includes/summary_item.html" with title="Additional Details" value=" " heading_level=heading_level editable=is_editable edit_link=domain_request_url %} -

          CISA Regional Representative

          -
            +

            CISA Regional Representative

            +
              {% if domain_request.cisa_representative_first_name %}
            • {{domain_request.cisa_representative_first_name}} {{domain_request.cisa_representative_last_name}}
              • {% if domain_request.cisa_representative_email %} @@ -144,8 +144,8 @@ {% endif %}
            -

            Anything else

            -
              +

              Anything else

              +
                {% if domain_request.anything_else %} {{domain_request.anything_else}} {% else %} diff --git a/src/registrar/templates/includes/request_status_manage.html b/src/registrar/templates/includes/request_status_manage.html index f50d31bfc..b1c1a638d 100644 --- a/src/registrar/templates/includes/request_status_manage.html +++ b/src/registrar/templates/includes/request_status_manage.html @@ -198,7 +198,7 @@ {# We always show this field even if None #} {% if DomainRequest %} -

                CISA Regional Representative

                +

                CISA Regional Representative

                  {% if DomainRequest.cisa_representative_first_name %} {{ DomainRequest.get_formatted_cisa_rep_name }} @@ -206,7 +206,7 @@ No {% endif %}
                -

                Anything else

                +

                Anything else

                  {% if DomainRequest.anything_else %} {{DomainRequest.anything_else}} diff --git a/src/registrar/templates/includes/summary_item.html b/src/registrar/templates/includes/summary_item.html index 0b0cea3c1..46ffd2fb0 100644 --- a/src/registrar/templates/includes/summary_item.html +++ b/src/registrar/templates/includes/summary_item.html @@ -20,7 +20,7 @@

      {% endif %} {% if sub_header_text %} -

      {{ sub_header_text }}

      +

      {{ sub_header_text }}

      {% endif %} {% if permissions %} {% include "includes/member_permissions.html" with permissions=value %} @@ -38,7 +38,7 @@ {% for item in value %}
      -

      Contact {{forloop.counter}} @@ -115,7 +115,7 @@ {% endif %} {% endif %} {% if value.invitations.all %} -

      Invited domain managers

      +

      Invited domain managers

        {% for item in value.invitations.all %}
      • {{ item.email }}
        • diff --git a/src/registrar/templates/portfolio_member_permissions.html b/src/registrar/templates/portfolio_member_permissions.html index e4b3e3aa4..21f5593ec 100644 --- a/src/registrar/templates/portfolio_member_permissions.html +++ b/src/registrar/templates/portfolio_member_permissions.html @@ -103,7 +103,7 @@

        Organization members

        + margin-top-4">Organization members {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} {% input_with_errors form.member_permission_admin %} {% endwith %} diff --git a/src/registrar/templates/portfolio_members_add_new.html b/src/registrar/templates/portfolio_members_add_new.html index d44233f49..16dbded23 100644 --- a/src/registrar/templates/portfolio_members_add_new.html +++ b/src/registrar/templates/portfolio_members_add_new.html @@ -76,7 +76,7 @@

        Organization members

        + margin-top-4">Organization members {% with group_classes="usa-form-editable usa-form-editable--no-border bg-gray-1 padding-top-0" %} {% input_with_errors form.member_permission_admin %} {% endwith %} diff --git a/src/registrar/templates/portfolio_organization.html b/src/registrar/templates/portfolio_organization.html index 55064d902..e6bd19ec2 100644 --- a/src/registrar/templates/portfolio_organization.html +++ b/src/registrar/templates/portfolio_organization.html @@ -37,8 +37,8 @@ {% include "includes/required_fields.html" %}
        {% csrf_token %} -

        Organization name

        -

        +

        Organization name

        +

        {{ portfolio.federal_agency }}

        {% input_with_errors form.address_line1 %} @@ -53,8 +53,8 @@
        {% else %} -

        Organization name

        -

        +

        Organization name

        +

        {{ portfolio.federal_agency }}

        {% if form.address_line1.value is not None %} diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 9bc97874d..76ccbb839 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -211,11 +211,11 @@ class TestPortfolio(WebTest): # Assert the response is a 200 self.assertEqual(response.status_code, 200) # The label for Federal agency will always be a h4 - self.assertContains(response, '

        Organization name

        ') + self.assertContains(response, '

        Organization name

        ') # The read only label for city will be a h4 - self.assertContains(response, '

        City

        ') + self.assertContains(response, '

        City

        ') self.assertNotContains(response, 'for="id_city"') - self.assertContains(response, '

        Los Angeles

        ') + self.assertContains(response, '

        Los Angeles

        ') @less_console_noise_decorator def test_portfolio_organization_page_edit_access(self): @@ -236,10 +236,10 @@ class TestPortfolio(WebTest): # Assert the response is a 200 self.assertEqual(response.status_code, 200) # The label for Federal agency will always be a h4 - self.assertContains(response, '

        Organization name

        ') + self.assertContains(response, '

        Organization name

        ') # The read only label for city will be a h4 - self.assertNotContains(response, '

        City

        ') - self.assertNotContains(response, '

        Los Angeles

        ') + self.assertNotContains(response, '

        City

        ') + self.assertNotContains(response, '

        Los Angeles

        ') self.assertContains(response, 'for="id_city"') @less_console_noise_decorator From 5fce7c7b4e888615a86a9f83eb5a9dadb5d405c3 Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 14 Jan 2025 14:33:30 -0600 Subject: [PATCH 21/82] initial fix --- src/registrar/tests/test_admin.py | 13 +- src/registrar/views/transfer_user.py | 275 ++++++++++++++++++++------- 2 files changed, 215 insertions(+), 73 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 3195f8237..e38c47f74 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2734,7 +2734,7 @@ class TestTransferUser(WebTest): @less_console_noise_decorator def test_transfer_user_transfers_user_portfolio_roles_no_error_when_duplicates(self): - """Assert that duplicate portfolio user roles do not throw errorsd""" + """Assert that duplicate portfolio user roles do not throw errors""" portfolio1 = Portfolio.objects.create(organization_name="Hotel California", creator=self.user2) UserPortfolioPermission.objects.create( user=self.user1, portfolio=portfolio1, roles=[UserPortfolioRoleChoices.ORGANIZATION_ADMIN] @@ -2759,7 +2759,7 @@ class TestTransferUser(WebTest): messages.error.assert_not_called() - @less_console_noise_decorator + # @less_console_noise_decorator def test_transfer_user_transfers_domain_request_creator_and_investigator(self): """Assert that domain request fields get transferred""" domain_request = completed_domain_request(user=self.user2, name="wasteland.gov", investigator=self.user2) @@ -2776,6 +2776,7 @@ class TestTransferUser(WebTest): self.assertEquals(domain_request.creator, self.user1) self.assertEquals(domain_request.investigator, self.user1) + @less_console_noise_decorator def test_transfer_user_transfers_domain_information_creator(self): """Assert that domain fields get transferred""" @@ -2866,7 +2867,7 @@ class TestTransferUser(WebTest): with self.assertRaises(User.DoesNotExist): self.user2.refresh_from_db() - @less_console_noise_decorator + # @less_console_noise_decorator def test_transfer_user_throws_transfer_and_delete_success_messages(self): """Test that success messages for data transfer and user deletion are displayed.""" # Ensure the setup for VerifiedByStaff @@ -2882,6 +2883,10 @@ class TestTransferUser(WebTest): submit_form["selected_user"] = self.user2.pk after_submit = submit_form.submit().follow() + print("mock_success_message.call_args_list:") + for call in mock_success_message.call_args_list: + print(call) + self.assertContains(after_submit, "

        Change user

        ") mock_success_message.assert_any_call( @@ -2898,7 +2903,7 @@ class TestTransferUser(WebTest): def test_transfer_user_throws_error_message(self): """Test that an error message is thrown if the transfer fails.""" with patch( - "registrar.views.TransferUserView.transfer_user_fields_and_log", side_effect=Exception("Simulated Error") + "registrar.views.TransferUserView.transfer_related_fields_and_log", side_effect=Exception("Simulated Error") ): with patch("django.contrib.messages.error") as mock_error: # Access the transfer user page diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index 69a0f4997..fa4c59a3f 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -1,7 +1,10 @@ import logging +from django.db import transaction +from django.db.models import Manager,ForeignKey, OneToOneField, ManyToManyField, ManyToOneRel from django.shortcuts import render, get_object_or_404, redirect from django.views import View +from registrar import models from registrar.models.domain import Domain from registrar.models.domain_information import DomainInformation from registrar.models.domain_request import DomainRequest @@ -21,22 +24,8 @@ logger = logging.getLogger(__name__) class TransferUserView(View): """Transfer user methods that set up the transfer_user template and handle the forms on it.""" - JOINS = [ - (DomainRequest, "creator"), - (DomainInformation, "creator"), - (Portfolio, "creator"), - (DomainRequest, "investigator"), - (UserDomainRole, "user"), - (VerifiedByStaff, "requestor"), - (UserPortfolioPermission, "user"), - ] - - # Future-proofing in case joined fields get added on the user model side - # This was tested in the first portfolio model iteration and works - USER_FIELDS: List[Any] = [] - def get(self, request, user_id): - """current_user referes to the 'source' user where the button that redirects to this view was clicked. + """current_user refers to the 'source' user where the button that redirects to this view was clicked. other_users exclude current_user and populate a dropdown, selected_user is the selection in the dropdown. This also querries the relevant domains and domain requests, and the admin context needed for the sidenav.""" @@ -71,86 +60,234 @@ class TransferUserView(View): def post(self, request, user_id): """This handles the transfer from selected_user to current_user then deletes selected_user. - - NOTE: We have a ticket to refactor this into a more solid lookup for related fields in #2645""" - + """ current_user = get_object_or_404(User, pk=user_id) selected_user_id = request.POST.get("selected_user") selected_user = get_object_or_404(User, pk=selected_user_id) try: - change_logs = [] + # Make this atomic so that we don't get any partial transfers + with transaction.atomic(): + change_logs = [] - # Transfer specific fields - self.transfer_user_fields_and_log(selected_user, current_user, change_logs) + self._delete_duplicate_user_domain_roles_and_log(selected_user, current_user, change_logs) - # Perform the updates and log the changes - for model_class, field_name in self.JOINS: - self.update_joins_and_log(model_class, field_name, selected_user, current_user, change_logs) + self._delete_duplicate_user_portfolio_permissions_and_log(selected_user, current_user, change_logs) + # Dynamically handle related fields + self.transfer_related_fields_and_log(selected_user, current_user, change_logs) - # Success message if any related objects were updated - if change_logs: - success_message = f"Data transferred successfully for the following objects: {change_logs}" - messages.success(request, success_message) + # Success message if any related objects were updated + logger.debug(f"change_logs: {change_logs}") + if change_logs: + logger.debug(f"change_logs: {change_logs}") + success_message = f"Data transferred successfully for the following objects: {change_logs}" + messages.success(request, success_message) - selected_user.delete() - messages.success(request, f"Deleted {selected_user} {selected_user.username}") + logger.debug("Deleting old user") + selected_user.delete() + messages.success(request, f"Deleted {selected_user} {selected_user.username}") except Exception as e: messages.error(request, f"An error occurred during the transfer: {e}") + logger.error(f"An error occurred during the transfer: {e}", exc_info=True) return redirect("admin:registrar_user_change", object_id=user_id) - - @classmethod - def update_joins_and_log(cls, model_class, field_name, selected_user, current_user, change_logs): + + def _delete_duplicate_user_portfolio_permissions_and_log(self, selected_user, current_user, change_logs): """ - Helper function to update the user join fields for a given model and log the changes. + Check and remove duplicate UserPortfolioPermission objects from the selected_user based on portfolios associated with the current_user. + """ + try: + # Fetch portfolios associated with the current_user + current_user_portfolios = UserPortfolioPermission.objects.filter(user=current_user).values_list('portfolio_id', flat=True) + + # Identify duplicates in selected_user for these portfolios + duplicates = ( + UserPortfolioPermission.objects + .filter(user=selected_user, portfolio_id__in=current_user_portfolios) + ) + + duplicate_count = duplicates.count() + + if duplicate_count > 0: + # Log the specific duplicates before deletion for better traceability + duplicate_permissions = list(duplicates) + logger.debug(f"Duplicate permissions to be removed: {duplicate_permissions}") + + duplicates.delete() + logger.info(f"Removed {duplicate_count} duplicate UserPortfolioPermission(s) from user_id {selected_user.id} for portfolios already associated with user_id {current_user.id}") + change_logs.append(f"Removed {duplicate_count} duplicate UserPortfolioPermission(s) from user_id {selected_user.id} for portfolios already associated with user_id {current_user.id}") + + except Exception as e: + logger.error(f"Failed to check and remove duplicate UserPortfolioPermissions: {e}", exc_info=True) + raise + + def _delete_duplicate_user_domain_roles_and_log(self, selected_user, current_user, change_logs): + """ + Check and remove duplicate UserDomainRole objects from the selected_user based on domains associated with the current_user. + Retain one instance per domain to maintain data integrity. """ - filter_kwargs = {field_name: selected_user} - updated_objects = model_class.objects.filter(**filter_kwargs) + try: + # Fetch domains associated with the current_user + current_user_domains = UserDomainRole.objects.filter(user=current_user).values_list('domain_id', flat=True) - for obj in updated_objects: - # Check for duplicate UserDomainRole before updating - if model_class == UserDomainRole: - if model_class.objects.filter(user=current_user, domain=obj.domain).exists(): - continue # Skip the update to avoid a duplicate + # Identify duplicates in selected_user for these domains + duplicates = ( + UserDomainRole.objects + .filter(user=selected_user, domain_id__in=current_user_domains) + ) - if model_class == UserPortfolioPermission: - if model_class.objects.filter(user=current_user, portfolio=obj.portfolio).exists(): - continue # Skip the update to avoid a duplicate + duplicate_count = duplicates.count() - # Update the field on the object and save it + if duplicate_count > 0: + duplicates.delete() + logger.info( + f"Removed {duplicate_count} duplicate UserDomainRole(s) from user_id {selected_user.id} " + f"for domains already associated with user_id {current_user.id}" + ) + change_logs.append( + f"Removed {duplicate_count} duplicate UserDomainRole(s) from user_id {selected_user.id} " + f"for domains already associated with user_id {current_user.id}" + ) + + except Exception as e: + logger.error(f"Failed to check and remove duplicate UserDomainRoles: {e}", exc_info=True) + raise + + def transfer_related_fields_and_log(self, selected_user, current_user, change_logs): + """ + Dynamically find all related fields to the User model and transfer them from selected_user to current_user. + Handles ForeignKey, OneToOneField, ManyToManyField, and ManyToOneRel relationships. + """ + user_model = User + + # Handle forward relationships + for related_field in user_model._meta.get_fields(): + if related_field.is_relation and related_field.related_model: + if isinstance(related_field, ForeignKey): + self._handle_foreign_key(related_field, selected_user, current_user, change_logs) + elif isinstance(related_field, OneToOneField): + self._handle_one_to_one(related_field, selected_user, current_user, change_logs) + elif isinstance(related_field, ManyToManyField): + self._handle_many_to_many(related_field, selected_user, current_user, change_logs) + elif isinstance(related_field, ManyToOneRel): + self._handle_many_to_one_rel(related_field, selected_user, current_user, change_logs) + + # # Handle reverse relationships + for related_object in user_model._meta.related_objects: + if isinstance(related_object, ManyToOneRel): + self._handle_many_to_one_rel(related_object, selected_user, current_user, change_logs) + elif isinstance(related_object.field, OneToOneField): + self._handle_one_to_one_reverse(related_object, selected_user, current_user, change_logs) + elif isinstance(related_object.field, ForeignKey): + self._handle_foreign_key_reverse(related_object, selected_user, current_user, change_logs) + elif isinstance(related_object.field, ManyToManyField): + self._handle_many_to_many_reverse(related_object, selected_user, current_user, change_logs) + + def _handle_foreign_key(self, related_field: ForeignKey, selected_user, current_user, change_logs): + related_name = related_field.get_accessor_name() + # for foreign key relationships, getattr returns a manager + related_manager = getattr(selected_user, related_name, None) + + if related_manager: + # get all the related objects + related_queryset = related_manager.all() + for obj in related_queryset: + # set the foreign key to the current user + setattr(obj, related_field.field.name, current_user) + obj.save() + log_entry = f'Transferred {related_field.field.name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) + + def _handle_one_to_one(self, related_field: OneToOneField, selected_user, current_user, change_logs): + related_name = related_field.get_accessor_name() + # for one to one relationships, getattr returns the related object + related_object = getattr(selected_user, related_name, None) + + if related_object: + # set the one to one field to the current user + setattr(related_object, related_field.field.name, current_user) + related_object.save() + log_entry = f'Transferred {related_field.field.name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) + + def _handle_many_to_many(self, related_field: ManyToManyField, selected_user, current_user, change_logs): + # for many to many relationships, getattr returns a manager + related_manager = getattr(selected_user, related_field.name, None) + if related_manager: + # get all the related objects + related_queryset = related_manager.all() + # add the related objects to the current user + getattr(current_user, related_field.name).add(*related_queryset) + log_entry = f'Transferred {related_field.name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) + + def _handle_many_to_one_rel(self, related_object: ManyToOneRel, selected_user: User, current_user: User, change_logs: List[str]): + """ + Handles ManyToOneRel relationships, where multiple objects relate to the User via a ForeignKey. + """ + related_model = related_object.related_model + related_name = related_object.field.name + + # for many to one relationships, we need a queryset + related_queryset = related_model.objects.filter(**{related_name: selected_user}) + for obj in related_queryset: + setattr(obj, related_name, current_user) + obj.save() + log_entry = f'Transferred {related_name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) + + def _handle_one_to_one_reverse(self, related_object: OneToOneField, selected_user: User, current_user: User, change_logs: List[str]): + """ + Handles reverse OneToOneField relationships. + """ + related_model = related_object.related_model + field_name = related_object.field.name + + try: + related_instance = related_model.objects.filter(**{field_name: selected_user}).first() + setattr(related_instance, field_name, current_user) + related_instance.save() + log_entry = f'Transferred {field_name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) + except related_model.DoesNotExist: + logger.warning(f"No related instance found for reverse OneToOneField {field_name} for {selected_user}") + + def _handle_foreign_key_reverse(self, related_object: ForeignKey, selected_user: User, current_user: User, change_logs: List[str]): + """ + Handles reverse ForeignKey relationships. + """ + related_model = related_object.related_model + field_name = related_object.field.name + + related_queryset = related_model.objects.filter(**{field_name: selected_user}) + for obj in related_queryset: setattr(obj, field_name, current_user) obj.save() + log_entry = f'Transferred {field_name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) - # Log the change - cls.log_change(obj, field_name, selected_user, current_user, change_logs) - - @classmethod - def transfer_user_fields_and_log(cls, selected_user, current_user, change_logs): + def _handle_many_to_many_reverse(self, related_object: ManyToManyField, selected_user: User, current_user: User, change_logs: List[str]): """ - Transfers portfolio fields from the selected_user to the current_user. - Logs the changes for each transferred field. + Handles reverse ManyToManyField relationships. """ - for field in cls.USER_FIELDS: - field_value = getattr(selected_user, field, None) + related_model = related_object.related_model + field_name = related_object.field.name - if field_value: - setattr(current_user, field, field_value) - cls.log_change(current_user, field, field_value, field_value, change_logs) - - current_user.save() - - @classmethod - def log_change(cls, obj, field_name, field_value, new_value, change_logs): - """Logs the change for a specific field on an object""" - log_entry = f'Changed {field_name} from "{field_value}" to "{new_value}" on {obj}' - - logger.info(log_entry) - - # Collect the related object for the success message - change_logs.append(log_entry) + related_manager = related_model.objects.filter(**{field_name: selected_user}) + if related_manager: + related_qs = related_manager.all() + getattr(current_user, field_name).add(*related_qs) + log_entry = f'Transferred {field_name} from {selected_user} to {current_user}' + logger.info(log_entry) + change_logs.append(log_entry) @classmethod def get_domains(cls, user): From bc9bc8fbaee837b9e23ac822cb55d590eb3ac95b Mon Sep 17 00:00:00 2001 From: matthewswspence Date: Tue, 14 Jan 2025 14:58:30 -0600 Subject: [PATCH 22/82] refactor transfer user function --- src/registrar/tests/test_admin.py | 9 +- src/registrar/views/transfer_user.py | 139 ++++++++++++--------------- 2 files changed, 65 insertions(+), 83 deletions(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index e38c47f74..0cc75d6e2 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -2759,7 +2759,7 @@ class TestTransferUser(WebTest): messages.error.assert_not_called() - # @less_console_noise_decorator + @less_console_noise_decorator def test_transfer_user_transfers_domain_request_creator_and_investigator(self): """Assert that domain request fields get transferred""" domain_request = completed_domain_request(user=self.user2, name="wasteland.gov", investigator=self.user2) @@ -2776,7 +2776,6 @@ class TestTransferUser(WebTest): self.assertEquals(domain_request.creator, self.user1) self.assertEquals(domain_request.investigator, self.user1) - @less_console_noise_decorator def test_transfer_user_transfers_domain_information_creator(self): """Assert that domain fields get transferred""" @@ -2867,7 +2866,7 @@ class TestTransferUser(WebTest): with self.assertRaises(User.DoesNotExist): self.user2.refresh_from_db() - # @less_console_noise_decorator + @less_console_noise_decorator def test_transfer_user_throws_transfer_and_delete_success_messages(self): """Test that success messages for data transfer and user deletion are displayed.""" # Ensure the setup for VerifiedByStaff @@ -2892,8 +2891,8 @@ class TestTransferUser(WebTest): mock_success_message.assert_any_call( ANY, ( - "Data transferred successfully for the following objects: ['Changed requestor " - + 'from "Furiosa Jabassa " to "Max Rokatanski " on immortan.joe@citadel.com\']' + "Data transferred successfully for the following objects: ['Transferred requestor " + + "from Furiosa Jabassa to Max Rokatanski ']" ), ) diff --git a/src/registrar/views/transfer_user.py b/src/registrar/views/transfer_user.py index fa4c59a3f..e10a5f3fc 100644 --- a/src/registrar/views/transfer_user.py +++ b/src/registrar/views/transfer_user.py @@ -1,6 +1,6 @@ import logging from django.db import transaction -from django.db.models import Manager,ForeignKey, OneToOneField, ManyToManyField, ManyToOneRel +from django.db.models import Manager, ForeignKey, OneToOneField, ManyToManyField, ManyToOneRel from django.shortcuts import render, get_object_or_404, redirect from django.views import View @@ -59,8 +59,7 @@ class TransferUserView(View): return render(request, "admin/transfer_user.html", context) def post(self, request, user_id): - """This handles the transfer from selected_user to current_user then deletes selected_user. - """ + """This handles the transfer from selected_user to current_user then deletes selected_user.""" current_user = get_object_or_404(User, pk=user_id) selected_user_id = request.POST.get("selected_user") selected_user = get_object_or_404(User, pk=selected_user_id) @@ -73,7 +72,7 @@ class TransferUserView(View): self._delete_duplicate_user_domain_roles_and_log(selected_user, current_user, change_logs) self._delete_duplicate_user_portfolio_permissions_and_log(selected_user, current_user, change_logs) - # Dynamically handle related fields + # Dynamically handle related fields self.transfer_related_fields_and_log(selected_user, current_user, change_logs) # Success message if any related objects were updated @@ -92,19 +91,20 @@ class TransferUserView(View): logger.error(f"An error occurred during the transfer: {e}", exc_info=True) return redirect("admin:registrar_user_change", object_id=user_id) - + def _delete_duplicate_user_portfolio_permissions_and_log(self, selected_user, current_user, change_logs): """ Check and remove duplicate UserPortfolioPermission objects from the selected_user based on portfolios associated with the current_user. """ try: # Fetch portfolios associated with the current_user - current_user_portfolios = UserPortfolioPermission.objects.filter(user=current_user).values_list('portfolio_id', flat=True) + current_user_portfolios = UserPortfolioPermission.objects.filter(user=current_user).values_list( + "portfolio_id", flat=True + ) # Identify duplicates in selected_user for these portfolios - duplicates = ( - UserPortfolioPermission.objects - .filter(user=selected_user, portfolio_id__in=current_user_portfolios) + duplicates = UserPortfolioPermission.objects.filter( + user=selected_user, portfolio_id__in=current_user_portfolios ) duplicate_count = duplicates.count() @@ -115,9 +115,13 @@ class TransferUserView(View): logger.debug(f"Duplicate permissions to be removed: {duplicate_permissions}") duplicates.delete() - logger.info(f"Removed {duplicate_count} duplicate UserPortfolioPermission(s) from user_id {selected_user.id} for portfolios already associated with user_id {current_user.id}") - change_logs.append(f"Removed {duplicate_count} duplicate UserPortfolioPermission(s) from user_id {selected_user.id} for portfolios already associated with user_id {current_user.id}") - + logger.info( + f"Removed {duplicate_count} duplicate UserPortfolioPermission(s) from user_id {selected_user.id} for portfolios already associated with user_id {current_user.id}" + ) + change_logs.append( + f"Removed {duplicate_count} duplicate UserPortfolioPermission(s) from user_id {selected_user.id} for portfolios already associated with user_id {current_user.id}" + ) + except Exception as e: logger.error(f"Failed to check and remove duplicate UserPortfolioPermissions: {e}", exc_info=True) raise @@ -130,13 +134,10 @@ class TransferUserView(View): try: # Fetch domains associated with the current_user - current_user_domains = UserDomainRole.objects.filter(user=current_user).values_list('domain_id', flat=True) + current_user_domains = UserDomainRole.objects.filter(user=current_user).values_list("domain_id", flat=True) # Identify duplicates in selected_user for these domains - duplicates = ( - UserDomainRole.objects - .filter(user=selected_user, domain_id__in=current_user_domains) - ) + duplicates = UserDomainRole.objects.filter(user=selected_user, domain_id__in=current_user_domains) duplicate_count = duplicates.count() @@ -150,7 +151,7 @@ class TransferUserView(View): f"Removed {duplicate_count} duplicate UserDomainRole(s) from user_id {selected_user.id} " f"for domains already associated with user_id {current_user.id}" ) - + except Exception as e: logger.error(f"Failed to check and remove duplicate UserDomainRoles: {e}", exc_info=True) raise @@ -187,65 +188,48 @@ class TransferUserView(View): def _handle_foreign_key(self, related_field: ForeignKey, selected_user, current_user, change_logs): related_name = related_field.get_accessor_name() - # for foreign key relationships, getattr returns a manager related_manager = getattr(selected_user, related_name, None) - if related_manager: - # get all the related objects + if related_manager.count() > 0: related_queryset = related_manager.all() for obj in related_queryset: - # set the foreign key to the current user setattr(obj, related_field.field.name, current_user) obj.save() - log_entry = f'Transferred {related_field.field.name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) - + self.log_change(selected_user, current_user, related_field.field.name, change_logs) + def _handle_one_to_one(self, related_field: OneToOneField, selected_user, current_user, change_logs): related_name = related_field.get_accessor_name() - # for one to one relationships, getattr returns the related object related_object = getattr(selected_user, related_name, None) if related_object: - # set the one to one field to the current user setattr(related_object, related_field.field.name, current_user) related_object.save() - log_entry = f'Transferred {related_field.field.name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) + self.log_change(selected_user, current_user, related_field.field.name, change_logs) def _handle_many_to_many(self, related_field: ManyToManyField, selected_user, current_user, change_logs): - # for many to many relationships, getattr returns a manager related_manager = getattr(selected_user, related_field.name, None) - if related_manager: - # get all the related objects + if related_manager.count() > 0: related_queryset = related_manager.all() - # add the related objects to the current user getattr(current_user, related_field.name).add(*related_queryset) - log_entry = f'Transferred {related_field.name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) + self.log_change(selected_user, current_user, related_field.name, change_logs) - def _handle_many_to_one_rel(self, related_object: ManyToOneRel, selected_user: User, current_user: User, change_logs: List[str]): - """ - Handles ManyToOneRel relationships, where multiple objects relate to the User via a ForeignKey. - """ + def _handle_many_to_one_rel( + self, related_object: ManyToOneRel, selected_user: User, current_user: User, change_logs: List[str] + ): related_model = related_object.related_model related_name = related_object.field.name - # for many to one relationships, we need a queryset related_queryset = related_model.objects.filter(**{related_name: selected_user}) - for obj in related_queryset: - setattr(obj, related_name, current_user) - obj.save() - log_entry = f'Transferred {related_name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) - def _handle_one_to_one_reverse(self, related_object: OneToOneField, selected_user: User, current_user: User, change_logs: List[str]): - """ - Handles reverse OneToOneField relationships. - """ + if related_queryset.count() > 0: + for obj in related_queryset: + setattr(obj, related_name, current_user) + obj.save() + self.log_change(selected_user, current_user, related_name, change_logs) + + def _handle_one_to_one_reverse( + self, related_object: OneToOneField, selected_user: User, current_user: User, change_logs: List[str] + ): related_model = related_object.related_model field_name = related_object.field.name @@ -253,41 +237,40 @@ class TransferUserView(View): related_instance = related_model.objects.filter(**{field_name: selected_user}).first() setattr(related_instance, field_name, current_user) related_instance.save() - log_entry = f'Transferred {field_name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) + self.log_change(selected_user, current_user, field_name, change_logs) except related_model.DoesNotExist: logger.warning(f"No related instance found for reverse OneToOneField {field_name} for {selected_user}") - - def _handle_foreign_key_reverse(self, related_object: ForeignKey, selected_user: User, current_user: User, change_logs: List[str]): - """ - Handles reverse ForeignKey relationships. - """ + + def _handle_foreign_key_reverse( + self, related_object: ForeignKey, selected_user: User, current_user: User, change_logs: List[str] + ): related_model = related_object.related_model field_name = related_object.field.name related_queryset = related_model.objects.filter(**{field_name: selected_user}) - for obj in related_queryset: - setattr(obj, field_name, current_user) - obj.save() - log_entry = f'Transferred {field_name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) - def _handle_many_to_many_reverse(self, related_object: ManyToManyField, selected_user: User, current_user: User, change_logs: List[str]): - """ - Handles reverse ManyToManyField relationships. - """ + if related_queryset.count() > 0: + for obj in related_queryset: + setattr(obj, field_name, current_user) + obj.save() + self.log_change(selected_user, current_user, field_name, change_logs) + + def _handle_many_to_many_reverse( + self, related_object: ManyToManyField, selected_user: User, current_user: User, change_logs: List[str] + ): related_model = related_object.related_model field_name = related_object.field.name - related_manager = related_model.objects.filter(**{field_name: selected_user}) - if related_manager: - related_qs = related_manager.all() - getattr(current_user, field_name).add(*related_qs) - log_entry = f'Transferred {field_name} from {selected_user} to {current_user}' - logger.info(log_entry) - change_logs.append(log_entry) + related_queryset = related_model.objects.filter(**{field_name: selected_user}) + if related_queryset.count() > 0: + getattr(current_user, field_name).add(*related_queryset) + self.log_change(selected_user, current_user, field_name, change_logs) + + @classmethod + def log_change(cls, selected_user, current_user, field_name, change_logs): + log_entry = f"Transferred {field_name} from {selected_user} to {current_user}" + logger.info(log_entry) + change_logs.append(log_entry) @classmethod def get_domains(cls, user): From 52ba794d4210be85cf32666039f00e0ef40b5544 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 14 Jan 2025 16:40:50 -0500 Subject: [PATCH 23/82] cleanup --- src/registrar/assets/src/sass/_theme/_tooltips.scss | 2 +- src/registrar/templates/portfolio_member_domains_edit.html | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/src/sass/_theme/_tooltips.scss b/src/registrar/assets/src/sass/_theme/_tooltips.scss index 58beb8ae6..65bfbb483 100644 --- a/src/registrar/assets/src/sass/_theme/_tooltips.scss +++ b/src/registrar/assets/src/sass/_theme/_tooltips.scss @@ -66,9 +66,9 @@ text-align: center; font-size: inherit; //inherit tooltip fontsize of .93rem max-width: fit-content; + display: block; @include at-media('desktop') { width: 70vw; } - display: block; } } \ No newline at end of file diff --git a/src/registrar/templates/portfolio_member_domains_edit.html b/src/registrar/templates/portfolio_member_domains_edit.html index 3169076ee..5f2185f84 100644 --- a/src/registrar/templates/portfolio_member_domains_edit.html +++ b/src/registrar/templates/portfolio_member_domains_edit.html @@ -88,13 +88,13 @@
        -

        Unassigned domains

        +

        Unassigned domains

        • item1
        • item2
        -

        Assigned domains

        +

        Assigned domains