From 79077cce31efa1e469ba426507e9e7d44c263890 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 16 Dec 2024 13:23:14 -0700 Subject: [PATCH 01/18] basic logic --- .../js/getgov-admin/domain-request-form.js | 5 +++ ...0_alter_suborganization_unique_together.py | 17 +++++++++ src/registrar/models/domain.py | 1 + src/registrar/models/domain_request.py | 37 ++++++++++++++++++- src/registrar/models/suborganization.py | 3 ++ 5 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 src/registrar/migrations/0140_alter_suborganization_unique_together.py diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index a815a59a1..6b79a2419 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -629,6 +629,10 @@ export function initRejectedEmail() { }); } +function handleSuborganizationSelection() { + console.log("cats are cool") +} + /** * A function for dynamic DomainRequest fields */ @@ -636,5 +640,6 @@ export function initDynamicDomainRequestFields(){ const domainRequestPage = document.getElementById("domainrequest_form"); if (domainRequestPage) { handlePortfolioSelection(); + handleSuborganizationSelection(); } } diff --git a/src/registrar/migrations/0140_alter_suborganization_unique_together.py b/src/registrar/migrations/0140_alter_suborganization_unique_together.py new file mode 100644 index 000000000..e59ecdf2b --- /dev/null +++ b/src/registrar/migrations/0140_alter_suborganization_unique_together.py @@ -0,0 +1,17 @@ +# Generated by Django 4.2.10 on 2024-12-16 17:02 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0139_alter_domainrequest_action_needed_reason"), + ] + + operations = [ + migrations.AlterUniqueTogether( + name="suborganization", + unique_together={("name", "portfolio")}, + ), + ] diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index cc600e1ce..2ea78ff10 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -237,6 +237,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 44d8511b0..46e188a0a 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -690,6 +690,18 @@ class DomainRequest(TimeStampedModel): # Update the cached values after saving self._cache_status_and_status_reasons() + def create_requested_suborganization(self): + """Creates the requested suborganization. + Adds the name, portfolio, city, and state_territory fields. + Returns the created suborganization.""" + Suborganization = apps.get_model("registrar.Suborganization") + return Suborganization.objects.create( + name=self.requested_suborganization, + portfolio=self.portfolio, + city=self.suborganization_city, + state_territory=self.suborganization_state_territory, + ) + def send_custom_status_update_email(self, status): """Helper function to send out a second status email when the status remains the same, but the reason has changed.""" @@ -785,6 +797,7 @@ class DomainRequest(TimeStampedModel): def delete_and_clean_up_domain(self, called_from): try: + # Clean up the approved domain domain_state = self.approved_domain.state # Only reject if it exists on EPP if domain_state != Domain.State.UNKNOWN: @@ -796,6 +809,19 @@ class DomainRequest(TimeStampedModel): logger.error(err) logger.error(f"Can't query an approved domain while attempting {called_from}") + # Clean up any created suborgs, assuming its for this record only + if self.sub_organization is not None: + request_suborg_count = self.request_sub_organization.count() + domain_suborgs = self.DomainRequest_info.filter( + sub_organization=self.sub_organization + ).count() + if request_suborg_count == 1 and domain_suborgs.count() <= 1: + # if domain_suborgs.count() == 1: + # domain_info = domain_suborgs.first() + # domain_info.sub_organization = None + # domain_info.save() + self.sub_organization.delete() + def _send_status_update_email( self, new_status, @@ -984,6 +1010,7 @@ class DomainRequest(TimeStampedModel): if self.status == self.DomainRequestStatus.APPROVED: self.delete_and_clean_up_domain("action_needed") + elif self.status == self.DomainRequestStatus.REJECTED: self.rejection_reason = None @@ -1014,8 +1041,16 @@ class DomainRequest(TimeStampedModel): domain request into an admin on that domain. It also triggers an email notification.""" + should_save = False if self.federal_agency is None: self.federal_agency = FederalAgency.objects.filter(agency="Non-Federal Agency").first() + should_save = True + + if self.is_requesting_new_suborganization(): + self.sub_organization = self.create_requested_suborganization() + should_save = True + + if should_save: self.save() # create the domain @@ -1148,7 +1183,7 @@ class DomainRequest(TimeStampedModel): def is_requesting_new_suborganization(self) -> bool: """Determines if a user is trying to request a new suborganization using the domain request form, rather than one that already exists. - Used for the RequestingEntity page. + Used for the RequestingEntity page and on DomainInformation.create_from_da(). Returns True if a sub_organization does not exist and if requested_suborganization, suborganization_city, and suborganization_state_territory all exist. diff --git a/src/registrar/models/suborganization.py b/src/registrar/models/suborganization.py index 087490244..fb32ad48a 100644 --- a/src/registrar/models/suborganization.py +++ b/src/registrar/models/suborganization.py @@ -9,6 +9,9 @@ class Suborganization(TimeStampedModel): Suborganization under an organization (portfolio) """ + class Meta: + unique_together = ["name", "portfolio"] + name = models.CharField( unique=True, max_length=1000, From 4572b52aad52154d000e0d9399d0987dbcfd48e9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 17 Dec 2024 08:34:58 -0700 Subject: [PATCH 02/18] Removal logic --- src/registrar/forms/domain_request_wizard.py | 17 ++++++- ...0_alter_suborganization_unique_together.py | 17 ------- src/registrar/models/domain_request.py | 46 ++++++++++++++----- src/registrar/models/suborganization.py | 3 -- 4 files changed, 50 insertions(+), 33 deletions(-) delete mode 100644 src/registrar/migrations/0140_alter_suborganization_unique_together.py diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index 572ef6399..eed0866f3 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -17,6 +17,7 @@ from registrar.models import Contact, DomainRequest, DraftDomain, Domain, Federa from registrar.templatetags.url_helpers import public_site_url from registrar.utility.enums import ValidationReturnType from registrar.utility.constants import BranchChoices +from django.core.exceptions import ValidationError logger = logging.getLogger(__name__) @@ -78,6 +79,20 @@ class RequestingEntityForm(RegistrarForm): # Otherwise just return the suborg as normal return self.cleaned_data.get("sub_organization") + def clean_requested_suborganization(self): + name = self.cleaned_data.get('requested_suborganization') + if name and Suborganization.objects.filter( + name__iexact=name, + portfolio=self.domain_request.portfolio, + name__isnull=False, + portfolio__isnull=False + ).exists(): + raise ValidationError( + "This suborganization already exists. " + "Choose a new name, or select it directly if you would like to use it." + ) + return name + def full_clean(self): """Validation logic to remove the custom suborganization value before clean is triggered. Without this override, the form will throw an 'invalid option' error.""" @@ -114,7 +129,7 @@ class RequestingEntityForm(RegistrarForm): if requesting_entity_is_suborganization == "True": if is_requesting_new_suborganization: # Validate custom suborganization fields - if not cleaned_data.get("requested_suborganization"): + if not cleaned_data.get("requested_suborganization") and "requested_suborganization" not in self.errors: self.add_error("requested_suborganization", "Enter the name of your suborganization.") if not cleaned_data.get("suborganization_city"): self.add_error("suborganization_city", "Enter the city where your suborganization is located.") diff --git a/src/registrar/migrations/0140_alter_suborganization_unique_together.py b/src/registrar/migrations/0140_alter_suborganization_unique_together.py deleted file mode 100644 index e59ecdf2b..000000000 --- a/src/registrar/migrations/0140_alter_suborganization_unique_together.py +++ /dev/null @@ -1,17 +0,0 @@ -# Generated by Django 4.2.10 on 2024-12-16 17:02 - -from django.db import migrations - - -class Migration(migrations.Migration): - - dependencies = [ - ("registrar", "0139_alter_domainrequest_action_needed_reason"), - ] - - operations = [ - migrations.AlterUniqueTogether( - name="suborganization", - unique_together={("name", "portfolio")}, - ), - ] diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 46e188a0a..d78cd587f 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -12,6 +12,7 @@ from registrar.models.utility.generic_helper import CreateOrUpdateOrganizationTy from registrar.utility.errors import FSMDomainRequestError, FSMErrorCodes from registrar.utility.constants import BranchChoices from auditlog.models import LogEntry +from django.core.exceptions import ValidationError from registrar.utility.waffle import flag_is_active_for_user @@ -796,6 +797,7 @@ class DomainRequest(TimeStampedModel): return True def delete_and_clean_up_domain(self, called_from): + # Delete the approved domain try: # Clean up the approved domain domain_state = self.approved_domain.state @@ -808,19 +810,39 @@ class DomainRequest(TimeStampedModel): except Exception as err: logger.error(err) logger.error(f"Can't query an approved domain while attempting {called_from}") + + # Delete the suborg as long as this is the only place it is used + self._cleanup_dangling_suborg() - # Clean up any created suborgs, assuming its for this record only - if self.sub_organization is not None: - request_suborg_count = self.request_sub_organization.count() - domain_suborgs = self.DomainRequest_info.filter( - sub_organization=self.sub_organization - ).count() - if request_suborg_count == 1 and domain_suborgs.count() <= 1: - # if domain_suborgs.count() == 1: - # domain_info = domain_suborgs.first() - # domain_info.sub_organization = None - # domain_info.save() - self.sub_organization.delete() + def _cleanup_dangling_suborg(self): + """Deletes the existing suborg if its only being used by the deleted record""" + # Nothing to delete, so we just smile and walk away + if self.sub_organization is None: + return + + Suborganization = apps.get_model("registrar.Suborganization") + + # Stored as so because we need to set the reference to none first, + # so we can't just use the self.sub_organization property + suborg = Suborganization.objects.get(id=self.sub_organization.id) + requests = suborg.request_sub_organization + domain_infos = suborg.information_sub_organization + + # Check if this is the only reference to the suborganization + if requests.count() != 1 or domain_infos.count() > 1: + return + + # Remove the suborganization reference from request. + self.sub_organization = None + self.save() + + # Remove the suborganization reference from domain if it exists. + if domain_infos.count() == 1: + domain_infos.update(sub_organization=None) + + # Delete the now-orphaned suborganization + logger.info(f"_cleanup_dangling_suborg() -> Deleting orphan suborganization: {suborg}") + suborg.delete() def _send_status_update_email( self, diff --git a/src/registrar/models/suborganization.py b/src/registrar/models/suborganization.py index fb32ad48a..087490244 100644 --- a/src/registrar/models/suborganization.py +++ b/src/registrar/models/suborganization.py @@ -9,9 +9,6 @@ class Suborganization(TimeStampedModel): Suborganization under an organization (portfolio) """ - class Meta: - unique_together = ["name", "portfolio"] - name = models.CharField( unique=True, max_length=1000, From 682594263cb9ce6d2832e728a554fa38f47f867a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:00:15 -0700 Subject: [PATCH 03/18] Clear button --- src/registrar/admin.py | 7 ++++ .../js/getgov-admin/domain-request-form.js | 39 ++++++++++++++++++- .../helpers-portfolio-dynamic-fields.js | 17 +++++++- .../admin/domain_request_change_form.html | 11 +----- .../includes/domain_request_fieldset.html | 14 +++++++ 5 files changed, 76 insertions(+), 12 deletions(-) create mode 100644 src/registrar/templates/django/admin/includes/domain_request_fieldset.html diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0e8e4847a..6cbdd4878 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1817,6 +1817,11 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): if self.value() == "0": return queryset.filter(Q(is_election_board=False) | Q(is_election_board=None)) + @admin.display(description="Suborganization options") + def reject_suborganization_button(self, obj): + """Custom field to display the reject suborganization button""" + return "" + @admin.display(description=_("Generic Org Type")) def converted_generic_org_type(self, obj): return obj.converted_generic_org_type_display @@ -1985,6 +1990,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "requested_suborganization", "suborganization_city", "suborganization_state_territory", + "reject_suborganization_button", "creator", ] }, @@ -2106,6 +2112,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "alternative_domains", "is_election_board", "status_history", + "reject_suborganization_button", ) # Read only that we'll leverage for CISA Analysts diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index 6b79a2419..2f98684f5 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -630,7 +630,44 @@ export function initRejectedEmail() { } function handleSuborganizationSelection() { - console.log("cats are cool") + const requestedSuborganizationField = document.getElementById("id_requested_suborganization"); + const suborganizationCity = document.getElementById("id_suborganization_city"); + const suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); + // The reject button is wrapped in a fieldset with a label + const rejectButtonFieldset = document.querySelector(".field-reject_suborganization_button"); + const rejectButton = document.querySelector("#clear-requested-suborganization"); + + // Ensure that every variable is present before proceeding + if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory || !rejectButtonFieldset || !rejectButton) { + console.warn("handleSuborganizationSelection() => Could not find required fields.") + return; + } + console.log("Test") + function updateRejectButtonFieldset() { + if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { + showElement(rejectButtonFieldset); + }else { + console.log("Hiding") + console.log(requestedSuborganizationField) + console.log(requestedSuborganizationField.value) + console.log(suborganizationCity.value) + console.log(suborganizationStateTerritory.value) + hideElement(rejectButtonFieldset) + } + } + + function handleRejectButton() { + // Clear the text fields + requestedSuborganizationField.value = ""; + suborganizationCity.value = ""; + suborganizationStateTerritory.value = ""; + // Update button visibility after clearing + updateRejectButtonFieldset(); + } + rejectButton.addEventListener("click", handleRejectButton) + requestedSuborganizationField.addEventListener("blur", updateRejectButtonFieldset); + suborganizationCity.addEventListener("blur", updateRejectButtonFieldset); + suborganizationStateTerritory.addEventListener("change", updateRejectButtonFieldset); } /** diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index 39f30b87f..924176c31 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -87,6 +87,7 @@ export function handlePortfolioSelection() { const portfolioUrbanizationField = document.querySelector(".field-portfolio_urbanization"); const portfolioUrbanization = portfolioUrbanizationField.querySelector(".readonly"); const portfolioJsonUrl = document.getElementById("portfolio_json_url")?.value || null; + const rejectRequestedSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); let isPageLoading = true; /** @@ -507,11 +508,25 @@ export function handlePortfolioSelection() { showElement(requestedSuborganizationField); showElement(suborganizationCity); showElement(suborganizationStateTerritory); + + let requestedSuborganizationField = document.getElementById("id_requested_suborganization"); + let suborganizationCity = document.getElementById("id_suborganization_city"); + let suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); + if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory) { + return; + } + + if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { + showElement(rejectRequestedSuborganizationButtonFieldset); + }else { + hideElement(rejectRequestedSuborganizationButtonFieldset); + } } else { // Hide suborganization request fields if suborganization is selected hideElement(requestedSuborganizationField); hideElement(suborganizationCity); - hideElement(suborganizationStateTerritory); + hideElement(suborganizationStateTerritory); + hideElement(rejectRequestedSuborganizationButtonFieldset); } } diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index 46965f236..ee79aaeef 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -20,16 +20,7 @@ {% url 'get-rejection-email-for-user-json' as url_2 %} {% for fieldset in adminform %} - {% comment %} - TODO: this will eventually need to be changed to something like this - if we ever want to customize this file: - {% include "django/admin/includes/domain_information_fieldset.html" %} - - Use detail_table_fieldset as an example, or just extend it. - - original_object is just a variable name for "DomainInformation" or "DomainRequest" - {% endcomment %} - {% include "django/admin/includes/detail_table_fieldset.html" with original_object=original %} + {% include "django/admin/includes/domain_request_fieldset.html" with original_object=original %} {% endfor %} {% endblock %} diff --git a/src/registrar/templates/django/admin/includes/domain_request_fieldset.html b/src/registrar/templates/django/admin/includes/domain_request_fieldset.html new file mode 100644 index 000000000..3186fff1d --- /dev/null +++ b/src/registrar/templates/django/admin/includes/domain_request_fieldset.html @@ -0,0 +1,14 @@ +{% extends "django/admin/includes/email_clipboard_fieldset.html" %} +{% load custom_filters %} +{% load static url_helpers %} +{% load i18n static %} + + +{% block field_readonly %} + {{ block.super }} + {% if field.field.name == "reject_suborganization_button" %} + + Clear all requested suborganization fields + + {% endif %} +{% endblock field_readonly %} \ No newline at end of file From 9e339c2cd9a29cfb91615f3aaa77356d84d070ac Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 13:19:37 -0700 Subject: [PATCH 04/18] cleanup --- .../assets/src/js/getgov-admin/domain-request-form.js | 7 +------ .../getgov-admin/helpers-portfolio-dynamic-fields.js | 11 ++++++----- src/registrar/models/domain.py | 1 - 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index 2f98684f5..242ae25c8 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -642,16 +642,11 @@ function handleSuborganizationSelection() { console.warn("handleSuborganizationSelection() => Could not find required fields.") return; } - console.log("Test") + function updateRejectButtonFieldset() { if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { showElement(rejectButtonFieldset); }else { - console.log("Hiding") - console.log(requestedSuborganizationField) - console.log(requestedSuborganizationField.value) - console.log(suborganizationCity.value) - console.log(suborganizationStateTerritory.value) hideElement(rejectButtonFieldset) } } diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index 924176c31..5b67f8fad 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -87,7 +87,7 @@ export function handlePortfolioSelection() { const portfolioUrbanizationField = document.querySelector(".field-portfolio_urbanization"); const portfolioUrbanization = portfolioUrbanizationField.querySelector(".readonly"); const portfolioJsonUrl = document.getElementById("portfolio_json_url")?.value || null; - const rejectRequestedSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); + const rejectSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); let isPageLoading = true; /** @@ -508,7 +508,8 @@ export function handlePortfolioSelection() { showElement(requestedSuborganizationField); showElement(suborganizationCity); showElement(suborganizationStateTerritory); - + + // Initially show / hide the clear button only if there is data to clear let requestedSuborganizationField = document.getElementById("id_requested_suborganization"); let suborganizationCity = document.getElementById("id_suborganization_city"); let suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); @@ -517,16 +518,16 @@ export function handlePortfolioSelection() { } if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { - showElement(rejectRequestedSuborganizationButtonFieldset); + showElement(rejectSuborganizationButtonFieldset); }else { - hideElement(rejectRequestedSuborganizationButtonFieldset); + hideElement(rejectSuborganizationButtonFieldset); } } else { // Hide suborganization request fields if suborganization is selected hideElement(requestedSuborganizationField); hideElement(suborganizationCity); hideElement(suborganizationStateTerritory); - hideElement(rejectRequestedSuborganizationButtonFieldset); + hideElement(rejectSuborganizationButtonFieldset); } } diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 2ea78ff10..cc600e1ce 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -237,7 +237,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in From de8d252136abd911dd3d4439f0f086dacc0a9660 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 14:05:06 -0700 Subject: [PATCH 05/18] unit tests --- src/registrar/models/domain_request.py | 2 +- src/registrar/tests/common.py | 16 +++ src/registrar/tests/test_models_requests.py | 142 ++++++++++++++++++++ 3 files changed, 159 insertions(+), 1 deletion(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index d78cd587f..c0d4c7e57 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -810,7 +810,7 @@ class DomainRequest(TimeStampedModel): except Exception as err: logger.error(err) logger.error(f"Can't query an approved domain while attempting {called_from}") - + # Delete the suborg as long as this is the only place it is used self._cleanup_dangling_suborg() diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index e1f4f5a27..1c345b83b 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -1034,6 +1034,10 @@ def completed_domain_request( # noqa action_needed_reason=None, portfolio=None, organization_name=None, + sub_organization=None, + requested_suborganization=None, + suborganization_city=None, + suborganization_state_territory=None, ): """A completed domain request.""" if not user: @@ -1098,6 +1102,18 @@ def completed_domain_request( # noqa if portfolio: domain_request_kwargs["portfolio"] = portfolio + if sub_organization: + domain_request_kwargs["sub_organization"] = sub_organization + + if requested_suborganization: + domain_request_kwargs["requested_suborganization"] = requested_suborganization + + if suborganization_city: + domain_request_kwargs["suborganization_city"] = suborganization_city + + if suborganization_state_territory: + domain_request_kwargs["suborganization_state_territory"] = suborganization_state_territory + domain_request, _ = DomainRequest.objects.get_or_create(**domain_request_kwargs) if has_other_contacts: diff --git a/src/registrar/tests/test_models_requests.py b/src/registrar/tests/test_models_requests.py index da474224c..983a12b3c 100644 --- a/src/registrar/tests/test_models_requests.py +++ b/src/registrar/tests/test_models_requests.py @@ -15,6 +15,7 @@ from registrar.models import ( FederalAgency, AllowedEmail, Portfolio, + Suborganization, ) import boto3_mocking @@ -23,6 +24,8 @@ from registrar.utility.errors import FSMDomainRequestError from .common import ( MockSESClient, + create_user, + create_superuser, less_console_noise, completed_domain_request, set_domain_request_investigators, @@ -1070,3 +1073,142 @@ class TestDomainRequest(TestCase): ) self.assertEqual(domain_request2.generic_org_type, domain_request2.converted_generic_org_type) self.assertEqual(domain_request2.federal_agency, domain_request2.converted_federal_agency) + + +class TestDomainRequestSuborganization(TestCase): + """Tests for the suborganization fields on domain requests""" + + def setUp(self): + super().setUp() + self.user = create_user() + self.superuser = create_superuser() + + def tearDown(self): + super().tearDown() + DomainInformation.objects.all().delete() + DomainRequest.objects.all().delete() + Domain.objects.all().delete() + Suborganization.objects.all().delete() + Portfolio.objects.all().delete() + + @less_console_noise_decorator + def test_approve_creates_requested_suborganization(self): + """Test that approving a domain request with a requested suborganization creates it""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + requested_suborganization="Boom", + suborganization_city="Explody town", + suborganization_state_territory=DomainRequest.StateTerritoryChoices.OHIO, + ) + domain_request.investigator = self.superuser + domain_request.save() + + domain_request.approve() + + created_suborg = Suborganization.objects.filter( + name="Boom", + city="Explody town", + state_territory=DomainRequest.StateTerritoryChoices.OHIO, + portfolio=portfolio, + ).first() + + self.assertIsNotNone(created_suborg) + self.assertEqual(domain_request.sub_organization, created_suborg) + + @less_console_noise_decorator + def test_approve_without_requested_suborganization_makes_no_changes(self): + """Test that approving without a requested suborganization doesn't create one""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + ) + domain_request.investigator = self.superuser + domain_request.save() + + initial_suborg_count = Suborganization.objects.count() + domain_request.approve() + + self.assertEqual(Suborganization.objects.count(), initial_suborg_count) + self.assertIsNone(domain_request.sub_organization) + + @less_console_noise_decorator + def test_approve_with_existing_suborganization_makes_no_changes(self): + """Test that approving with an existing suborganization doesn't create a new one""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + existing_suborg = Suborganization.objects.create(name="Existing Division", portfolio=portfolio) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=existing_suborg, + ) + domain_request.investigator = self.superuser + domain_request.save() + + initial_suborg_count = Suborganization.objects.count() + domain_request.approve() + + self.assertEqual(Suborganization.objects.count(), initial_suborg_count) + self.assertEqual(domain_request.sub_organization, existing_suborg) + + @less_console_noise_decorator + def test_cleanup_dangling_suborg_with_single_reference(self): + """Test that a suborganization is deleted when it's only referenced once""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + suborg = Suborganization.objects.create(name="Test Division", portfolio=portfolio) + + domain_request = completed_domain_request( + name="test.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=suborg, + ) + domain_request.approve() + + # set it back to in review + domain_request.in_review() + domain_request.refresh_from_db() + + # Verify the suborganization was deleted + self.assertFalse(Suborganization.objects.filter(id=suborg.id).exists()) + self.assertIsNone(domain_request.sub_organization) + + @less_console_noise_decorator + def test_cleanup_dangling_suborg_with_multiple_references(self): + """Test that a suborganization is preserved when it has multiple references""" + portfolio = Portfolio.objects.create(organization_name="Test Org", creator=self.user) + suborg = Suborganization.objects.create(name="Test Division", portfolio=portfolio) + + # Create two domain requests using the same suborganization + domain_request1 = completed_domain_request( + name="test1.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=suborg, + ) + domain_request2 = completed_domain_request( + name="test2.gov", + portfolio=portfolio, + status=DomainRequest.DomainRequestStatus.IN_REVIEW, + sub_organization=suborg, + ) + + domain_request1.approve() + domain_request2.approve() + + # set one back to in review + domain_request1.in_review() + domain_request1.refresh_from_db() + + # Verify the suborganization still exists + self.assertTrue(Suborganization.objects.filter(id=suborg.id).exists()) + self.assertEqual(domain_request1.sub_organization, suborg) + self.assertEqual(domain_request2.sub_organization, suborg) From e1ad261e9c7eb939827254f7a068f18fa705c4ef Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 19 Dec 2024 15:23:49 -0700 Subject: [PATCH 06/18] Validation logic for /admin --- src/registrar/forms/domain_request_wizard.py | 2 +- src/registrar/models/domain_request.py | 40 ++++++++++++++++++++ src/registrar/models/suborganization.py | 1 - 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index eed0866f3..d84ceeb15 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -90,7 +90,7 @@ class RequestingEntityForm(RegistrarForm): raise ValidationError( "This suborganization already exists. " "Choose a new name, or select it directly if you would like to use it." - ) + ) return name def full_clean(self): diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index c0d4c7e57..ff3af829b 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -672,6 +672,46 @@ class DomainRequest(TimeStampedModel): # Store original values for caching purposes. Used to compare them on save. self._cache_status_and_status_reasons() + def clean(self): + super().clean() + # Validation logic for a suborganization request + if self.is_requesting_new_suborganization(): + # Raise an error if this suborganization already exists + Suborganization = apps.get_model("registrar.Suborganization") + if ( + self.requested_suborganization + and Suborganization.objects.filter( + name__iexact=self.requested_suborganization, + portfolio=self.portfolio, + name__isnull=False, + portfolio__isnull=False, + ).exists() + ): + raise ValidationError( + "This suborganization already exists. " + "Choose a new name, or select it directly if you would like to use it." + ) + elif self.portfolio and not self.sub_organization: + # You cannot create a new suborganization without these fields + required_suborg_fields = { + "requested_suborganization": self.requested_suborganization, + "suborganization_city": self.suborganization_city, + "suborganization_state_territory": self.suborganization_state_territory, + } + + if any(bool(value) for value in required_suborg_fields.values()): + # Find which fields are empty + errors_dict = { + field_name: [f"This field is required when creating a new suborganization."] + for field_name, value in required_suborg_fields.items() + if not value + } + # Adds a validation error to each missing field + raise ValidationError({ + k: ValidationError(v, code='required') + for k, v in errors_dict.items() + }) + def save(self, *args, **kwargs): """Save override for custom properties""" self.sync_organization_type() diff --git a/src/registrar/models/suborganization.py b/src/registrar/models/suborganization.py index 087490244..78689799c 100644 --- a/src/registrar/models/suborganization.py +++ b/src/registrar/models/suborganization.py @@ -1,5 +1,4 @@ from django.db import models - from registrar.models.domain_request import DomainRequest from .utility.time_stamped_model import TimeStampedModel From 9cb3ecebf4952c9779bc444ce9fb2e742d3cbf2e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 20 Dec 2024 10:08:00 -0700 Subject: [PATCH 07/18] Cleanup js after merge --- .../src/js/getgov-admin/domain-request-form.js | 15 +++++++++++++-- .../helpers-portfolio-dynamic-fields.js | 12 +++++++++++- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index 242ae25c8..928495cb6 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -629,7 +629,18 @@ export function initRejectedEmail() { }); } -function handleSuborganizationSelection() { + +/** + * A function that handles the suborganzation and requested suborganization fields and buttons. + * - Fieldwise: Hooks to the sub_organization, suborganization_city, and suborganization_state_territory fields. + * On change, this function checks if any of these fields are not empty: + * sub_organization, suborganization_city, and suborganization_state_territory. + * If they aren't, then we show the "clear" button. If they are, then we hide it because we don't need it. + * + * - Buttonwise: Hooks to the #clear-requested-suborganization button. + * On click, this will clear the input value of sub_organization, suborganization_city, and suborganization_state_territory. +*/ +function handleSuborgFieldsAndButtons() { const requestedSuborganizationField = document.getElementById("id_requested_suborganization"); const suborganizationCity = document.getElementById("id_suborganization_city"); const suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); @@ -672,6 +683,6 @@ export function initDynamicDomainRequestFields(){ const domainRequestPage = document.getElementById("domainrequest_form"); if (domainRequestPage) { handlePortfolioSelection(); - handleSuborganizationSelection(); + handleSuborgFieldsAndButtons(); } } diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index 91fd2b6e3..ce5db34c1 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -471,6 +471,16 @@ export function handlePortfolioSelection( if (suborganizationCity) showElement(suborganizationCity); if (suborganizationStateTerritory) showElement(suborganizationStateTerritory); + // Handle rejectSuborganizationButtonFieldset (display of the clear requested suborg button). + // Basically, this button should only be visible when we have data for suborg, city, and state_territory. + // The function handleSuborgFieldsAndButtons() in domain-request-form.js handles doing this same logic + // but on field input for city, state_territory, and the suborg field. + // If it doesn't exist, don't do anything. + if (!rejectSuborganizationButtonFieldset){ + console.warn("updateSuborganizationFieldsDisplay() => Could not update rejectSuborganizationButtonFieldset") + return; + } + // Initially show / hide the clear button only if there is data to clear let requestedSuborganizationField = document.getElementById("id_requested_suborganization"); let suborganizationCity = document.getElementById("id_suborganization_city"); @@ -489,7 +499,7 @@ export function handlePortfolioSelection( if (requestedSuborganizationField) hideElement(requestedSuborganizationField); if (suborganizationCity) hideElement(suborganizationCity); if (suborganizationStateTerritory) hideElement(suborganizationStateTerritory); - hideElement(rejectSuborganizationButtonFieldset); + if (rejectSuborganizationButtonFieldset) hideElement(rejectSuborganizationButtonFieldset); } } From 3d6d9b0d2fd2672a44293be9f40316b0b64b5d16 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 20 Dec 2024 10:28:10 -0700 Subject: [PATCH 08/18] Update domain_request.py --- src/registrar/models/domain_request.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index ff3af829b..381d28703 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -673,6 +673,16 @@ class DomainRequest(TimeStampedModel): self._cache_status_and_status_reasons() def clean(self): + """ + Validates suborganization-related fields in two scenarios: + 1. New suborganization request: Prevents duplicate names within same portfolio + 2. Partial suborganization data: Enforces a all-or-nothing rule for city/state/name fields + when portfolio exists without selected suborganization + + Add new domain request validation rules here to ensure they're + enforced during both model save and form submission. + Not presently used on the domain request wizard, though. + """ super().clean() # Validation logic for a suborganization request if self.is_requesting_new_suborganization(): From 19a1d60c13034052293467636a56ee41ccb0d089 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 20 Dec 2024 10:43:20 -0700 Subject: [PATCH 09/18] Update test_admin_request.py --- src/registrar/tests/test_admin_request.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index df0902719..9bdc1fbb4 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1644,6 +1644,7 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "is_election_board", "status_history", + "reject_suborganization_button", "id", "created_at", "updated_at", @@ -1718,6 +1719,7 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "is_election_board", "status_history", + "reject_suborganization_button", "federal_agency", "creator", "about_your_organization", @@ -1761,6 +1763,7 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "is_election_board", "status_history", + "reject_suborganization_button", ] self.assertEqual(readonly_fields, expected_fields) From 5d601d69913143a78399c545c1ad669ace385454 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 20 Dec 2024 11:30:21 -0700 Subject: [PATCH 10/18] linting --- src/registrar/forms/domain_request_wizard.py | 16 ++++++++-------- src/registrar/models/domain_request.py | 9 +++------ src/registrar/tests/test_reports.py | 1 + src/registrar/utility/csv_export.py | 2 +- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/registrar/forms/domain_request_wizard.py b/src/registrar/forms/domain_request_wizard.py index d84ceeb15..5eea13ed8 100644 --- a/src/registrar/forms/domain_request_wizard.py +++ b/src/registrar/forms/domain_request_wizard.py @@ -80,17 +80,17 @@ class RequestingEntityForm(RegistrarForm): return self.cleaned_data.get("sub_organization") def clean_requested_suborganization(self): - name = self.cleaned_data.get('requested_suborganization') - if name and Suborganization.objects.filter( - name__iexact=name, - portfolio=self.domain_request.portfolio, - name__isnull=False, - portfolio__isnull=False - ).exists(): + name = self.cleaned_data.get("requested_suborganization") + if ( + name + and Suborganization.objects.filter( + name__iexact=name, portfolio=self.domain_request.portfolio, name__isnull=False, portfolio__isnull=False + ).exists() + ): raise ValidationError( "This suborganization already exists. " "Choose a new name, or select it directly if you would like to use it." - ) + ) return name def full_clean(self): diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 381d28703..f4e2a35a1 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -679,9 +679,9 @@ class DomainRequest(TimeStampedModel): 2. Partial suborganization data: Enforces a all-or-nothing rule for city/state/name fields when portfolio exists without selected suborganization - Add new domain request validation rules here to ensure they're + Add new domain request validation rules here to ensure they're enforced during both model save and form submission. - Not presently used on the domain request wizard, though. + Not presently used on the domain request wizard, though. """ super().clean() # Validation logic for a suborganization request @@ -717,10 +717,7 @@ class DomainRequest(TimeStampedModel): if not value } # Adds a validation error to each missing field - raise ValidationError({ - k: ValidationError(v, code='required') - for k, v in errors_dict.items() - }) + raise ValidationError({k: ValidationError(v, code="required") for k, v in errors_dict.items()}) def save(self, *args, **kwargs): """Save override for custom properties""" diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 995782eea..64cfa95b0 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -869,6 +869,7 @@ class MemberExportTest(MockDbForIndividualTests, MockEppLib): csv_file.seek(0) # Read the content into a variable csv_content = csv_file.read() + self.maxDiff = None expected_content = ( # Header "Email,Organization admin,Invited by,Joined date,Last active,Domain requests," diff --git a/src/registrar/utility/csv_export.py b/src/registrar/utility/csv_export.py index 66809777b..f05f14d99 100644 --- a/src/registrar/utility/csv_export.py +++ b/src/registrar/utility/csv_export.py @@ -417,7 +417,7 @@ class MemberExport(BaseExport): # Adding a order_by increases output predictability. # Doesn't matter as much for normal use, but makes tests easier. # We should also just be ordering by default anyway. - members = permissions.union(invitations).order_by("email_display") + members = permissions.union(invitations).order_by("email_display", "member_display", "first_name", "last_name") return convert_queryset_to_dict(members, is_model=False) @classmethod From c0684539fa74b334de02a5e77b75968de0eded99 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Wed, 25 Dec 2024 20:35:18 -0700 Subject: [PATCH 11/18] fix bug in domain request link --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f43261885..874512fdc 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1995,7 +1995,7 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): @admin.display(description=_("Requested Domain")) def custom_requested_domain(self, obj): # Example: Show different icons based on `status` - url = reverse("admin:registrar_domainrequest_changelist") + f"?portfolio={obj.id}" + url = reverse("admin:registrar_domainrequest_changelist") + f"{obj.id}" text = obj.requested_domain if obj.portfolio: return format_html(' {}', url, text) From e978297569cedc34e7fe95ddbb261a7cfbd21795 Mon Sep 17 00:00:00 2001 From: CocoByte Date: Thu, 26 Dec 2024 15:05:38 -0700 Subject: [PATCH 12/18] Fix suborg dropdown behavior --- .../getgov-admin/helpers-portfolio-dynamic-fields.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index ce5db34c1..f045fca47 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -482,14 +482,14 @@ export function handlePortfolioSelection( } // Initially show / hide the clear button only if there is data to clear - let requestedSuborganizationField = document.getElementById("id_requested_suborganization"); - let suborganizationCity = document.getElementById("id_suborganization_city"); - let suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); - if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory) { + let requestedSuborganizationFieldInput = document.getElementById("id_requested_suborganization"); + let suborganizationCityInput = document.getElementById("id_suborganization_city"); + let suborganizationStateTerritoryInput = document.getElementById("id_suborganization_state_territory"); + if (!requestedSuborganizationFieldInput || !suborganizationCityInput || !suborganizationStateTerritoryInput) { return; } - if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { + if (requestedSuborganizationFieldInput.value || suborganizationCityInput.value || suborganizationStateTerritoryInput.value) { showElement(rejectSuborganizationButtonFieldset); }else { hideElement(rejectSuborganizationButtonFieldset); From 6f6e2dbc8b3a567e2c3cbfea5d354a6261ee103d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 2 Jan 2025 09:27:42 -0700 Subject: [PATCH 13/18] Add clear button --- src/package-lock.json | 53 +++---------------- src/registrar/admin.py | 10 ---- .../js/getgov-admin/domain-request-form.js | 19 ++++--- .../helpers-portfolio-dynamic-fields.js | 37 ++++++------- src/registrar/models/domain.py | 1 + .../admin/domain_request_change_form.html | 11 +++- .../admin/includes/detail_table_fieldset.html | 19 +++++++ .../includes/domain_request_fieldset.html | 14 ----- src/registrar/tests/test_admin_request.py | 3 -- 9 files changed, 62 insertions(+), 105 deletions(-) delete mode 100644 src/registrar/templates/django/admin/includes/domain_request_fieldset.html diff --git a/src/package-lock.json b/src/package-lock.json index 22fb31857..d78b5132f 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -6921,16 +6921,6 @@ "validate-npm-package-license": "^3.0.1" } }, - "node_modules/normalize-package-data/node_modules/semver": { - "version": "5.7.2", - "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.2.tgz", - "integrity": "sha512-cBznnQ9KjJqU67B52RMC65CMarK2600WFnbkcaiwWq3xy/5haFJlshgnpjovMVJ+Hff49d8GEn0b87C5pDQ10g==", - "dev": true, - "license": "ISC", - "bin": { - "semver": "bin/semver" - } - }, "node_modules/normalize-path": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/normalize-path/-/normalize-path-3.0.0.tgz", @@ -7307,39 +7297,6 @@ "node": ">= 12" } }, - "node_modules/pa11y/node_modules/lru-cache": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-6.0.0.tgz", - "integrity": "sha512-Jo6dJ04CmSjuznwJSS3pUeWmd/H0ffTlkXXgwZi+eq1UCmqQwCh+eLsYOYCwY991i2Fah4h1BEMCx4qThGbsiA==", - "license": "ISC", - "dependencies": { - "yallist": "^4.0.0" - }, - "engines": { - "node": ">=10" - } - }, - "node_modules/pa11y/node_modules/semver": { - "version": "7.3.8", - "resolved": "https://registry.npmjs.org/semver/-/semver-7.3.8.tgz", - "integrity": "sha512-NB1ctGL5rlHrPJtFDVIVzTyQylMLu9N9VICA6HSFJo8MCGVTMW6gfpicwKmmK/dAjTOrqu5l63JJOpDSrAis3A==", - "license": "ISC", - "dependencies": { - "lru-cache": "^6.0.0" - }, - "bin": { - "semver": "bin/semver.js" - }, - "engines": { - "node": ">=10" - } - }, - "node_modules/pa11y/node_modules/yallist": { - "version": "4.0.0", - "resolved": "https://registry.npmjs.org/yallist/-/yallist-4.0.0.tgz", - "integrity": "sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A==", - "license": "ISC" - }, "node_modules/parse-filepath": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/parse-filepath/-/parse-filepath-1.0.2.tgz", @@ -8888,13 +8845,15 @@ } }, "node_modules/semver": { - "version": "6.3.1", - "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.1.tgz", - "integrity": "sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA==", - "dev": true, + "version": "7.6.3", + "resolved": "https://registry.npmjs.org/semver/-/semver-7.6.3.tgz", + "integrity": "sha512-oVekP1cKtI+CTDvHWYFUcMtsK/00wmAEfyqKfNdARm8u1wNVhSgaX7A8d4UuIlUI5e84iEwOhs7ZPYRmzU9U6A==", "license": "ISC", "bin": { "semver": "bin/semver.js" + }, + "engines": { + "node": ">=10" } }, "node_modules/semver-greatest-satisfied-range": { diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 874512fdc..1476cee7e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2003,14 +2003,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): custom_requested_domain.admin_order_field = "requested_domain__name" # type: ignore - # ------ Converted fields ------ - # These fields map to @Property methods and - # require these custom definitions to work properly - @admin.display(description="Suborganization options") - def reject_suborganization_button(self, obj): - """Custom field to display the reject suborganization button""" - return "" - @admin.display(description=_("Generic Org Type")) def converted_generic_org_type(self, obj): return obj.converted_generic_org_type_display @@ -2182,7 +2174,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "requested_suborganization", "suborganization_city", "suborganization_state_territory", - "reject_suborganization_button", "creator", ] }, @@ -2304,7 +2295,6 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): "alternative_domains", "is_election_board", "status_history", - "reject_suborganization_button", ) # Read only that we'll leverage for CISA Analysts diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index 928495cb6..d82b2de8b 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -644,21 +644,20 @@ function handleSuborgFieldsAndButtons() { const requestedSuborganizationField = document.getElementById("id_requested_suborganization"); const suborganizationCity = document.getElementById("id_suborganization_city"); const suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); - // The reject button is wrapped in a fieldset with a label - const rejectButtonFieldset = document.querySelector(".field-reject_suborganization_button"); const rejectButton = document.querySelector("#clear-requested-suborganization"); + console.log("test12345678") // Ensure that every variable is present before proceeding - if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory || !rejectButtonFieldset || !rejectButton) { + if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory || !rejectButton) { console.warn("handleSuborganizationSelection() => Could not find required fields.") return; } - function updateRejectButtonFieldset() { + function handleRejectButtonVisibility() { if (requestedSuborganizationField.value || suborganizationCity.value || suborganizationStateTerritory.value) { - showElement(rejectButtonFieldset); + showElement(rejectButton); }else { - hideElement(rejectButtonFieldset) + hideElement(rejectButton) } } @@ -668,12 +667,12 @@ function handleSuborgFieldsAndButtons() { suborganizationCity.value = ""; suborganizationStateTerritory.value = ""; // Update button visibility after clearing - updateRejectButtonFieldset(); + handleRejectButtonVisibility(); } rejectButton.addEventListener("click", handleRejectButton) - requestedSuborganizationField.addEventListener("blur", updateRejectButtonFieldset); - suborganizationCity.addEventListener("blur", updateRejectButtonFieldset); - suborganizationStateTerritory.addEventListener("change", updateRejectButtonFieldset); + requestedSuborganizationField.addEventListener("blur", handleRejectButtonVisibility); + suborganizationCity.addEventListener("blur", handleRejectButtonVisibility); + suborganizationStateTerritory.addEventListener("change", handleRejectButtonVisibility); } /** diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index f045fca47..55b42d10d 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -49,7 +49,13 @@ export function handlePortfolioSelection( const portfolioUrbanizationField = document.querySelector(".field-portfolio_urbanization"); const portfolioUrbanization = portfolioUrbanizationField.querySelector(".readonly"); const portfolioJsonUrl = document.getElementById("portfolio_json_url")?.value || null; - const rejectSuborganizationButtonFieldset = document.querySelector(".field-reject_suborganization_button"); + // These requested suborganization fields only exist on the domain request page + const rejectSuborganizationButton = document.querySelector("#clear-requested-suborganization"); + const requestedSuborganizationFieldInput = document.getElementById("id_requested_suborganization"); + const suborganizationCityInput = document.getElementById("id_suborganization_city"); + const suborganizationStateTerritoryInput = document.getElementById("id_suborganization_state_territory"); + + // Global var to track page load let isPageLoading = true; /** @@ -471,35 +477,26 @@ export function handlePortfolioSelection( if (suborganizationCity) showElement(suborganizationCity); if (suborganizationStateTerritory) showElement(suborganizationStateTerritory); - // Handle rejectSuborganizationButtonFieldset (display of the clear requested suborg button). + // == LOGIC FOR THE DOMAIN REQUEST PAGE == // + // Handle rejectSuborganizationButton (display of the clear requested suborg button). // Basically, this button should only be visible when we have data for suborg, city, and state_territory. // The function handleSuborgFieldsAndButtons() in domain-request-form.js handles doing this same logic // but on field input for city, state_territory, and the suborg field. // If it doesn't exist, don't do anything. - if (!rejectSuborganizationButtonFieldset){ - console.warn("updateSuborganizationFieldsDisplay() => Could not update rejectSuborganizationButtonFieldset") - return; - } - - // Initially show / hide the clear button only if there is data to clear - let requestedSuborganizationFieldInput = document.getElementById("id_requested_suborganization"); - let suborganizationCityInput = document.getElementById("id_suborganization_city"); - let suborganizationStateTerritoryInput = document.getElementById("id_suborganization_state_territory"); - if (!requestedSuborganizationFieldInput || !suborganizationCityInput || !suborganizationStateTerritoryInput) { - return; - } - - if (requestedSuborganizationFieldInput.value || suborganizationCityInput.value || suborganizationStateTerritoryInput.value) { - showElement(rejectSuborganizationButtonFieldset); - }else { - hideElement(rejectSuborganizationButtonFieldset); + if (rejectSuborganizationButton){ + if (requestedSuborganizationFieldInput?.value || suborganizationCityInput?.value || suborganizationStateTerritoryInput?.value) { + showElement(rejectSuborganizationButton); + }else { + hideElement(rejectSuborganizationButton); + } } } else { // Hide suborganization request fields if suborganization is selected if (requestedSuborganizationField) hideElement(requestedSuborganizationField); if (suborganizationCity) hideElement(suborganizationCity); if (suborganizationStateTerritory) hideElement(suborganizationStateTerritory); - if (rejectSuborganizationButtonFieldset) hideElement(rejectSuborganizationButtonFieldset); + // == LOGIC FOR THE DOMAIN REQUEST PAGE == // + if (rejectSuborganizationButton) hideElement(rejectSuborganizationButton); } } diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 6eb2fac07..003714dfb 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,6 +244,7 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" + return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/templates/django/admin/domain_request_change_form.html b/src/registrar/templates/django/admin/domain_request_change_form.html index ee79aaeef..46965f236 100644 --- a/src/registrar/templates/django/admin/domain_request_change_form.html +++ b/src/registrar/templates/django/admin/domain_request_change_form.html @@ -20,7 +20,16 @@ {% url 'get-rejection-email-for-user-json' as url_2 %} {% for fieldset in adminform %} - {% include "django/admin/includes/domain_request_fieldset.html" with original_object=original %} + {% comment %} + TODO: this will eventually need to be changed to something like this + if we ever want to customize this file: + {% include "django/admin/includes/domain_information_fieldset.html" %} + + Use detail_table_fieldset as an example, or just extend it. + + original_object is just a variable name for "DomainInformation" or "DomainRequest" + {% endcomment %} + {% include "django/admin/includes/detail_table_fieldset.html" with original_object=original %} {% endfor %} {% endblock %} diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index d5b1130ab..0641c47c6 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -321,6 +321,25 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) {% else %} {% endif %} + {% elif field.field.name == "requested_suborganization" %} + {{ field.field }} + + + + + + + + Clear requested suborganization + + + {% else %} {{ field.field }} {% endif %} diff --git a/src/registrar/templates/django/admin/includes/domain_request_fieldset.html b/src/registrar/templates/django/admin/includes/domain_request_fieldset.html deleted file mode 100644 index 3186fff1d..000000000 --- a/src/registrar/templates/django/admin/includes/domain_request_fieldset.html +++ /dev/null @@ -1,14 +0,0 @@ -{% extends "django/admin/includes/email_clipboard_fieldset.html" %} -{% load custom_filters %} -{% load static url_helpers %} -{% load i18n static %} - - -{% block field_readonly %} - {{ block.super }} - {% if field.field.name == "reject_suborganization_button" %} - - Clear all requested suborganization fields - - {% endif %} -{% endblock field_readonly %} \ No newline at end of file diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index b464eb538..439f4fab0 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1644,7 +1644,6 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "is_election_board", "status_history", - "reject_suborganization_button", "id", "created_at", "updated_at", @@ -1719,7 +1718,6 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "is_election_board", "status_history", - "reject_suborganization_button", "federal_agency", "creator", "about_your_organization", @@ -1760,7 +1758,6 @@ class TestDomainRequestAdmin(MockEppLib): "alternative_domains", "is_election_board", "status_history", - "reject_suborganization_button", ] self.assertEqual(readonly_fields, expected_fields) From c59cb24f51ff5849c2af1076a1779fce95dccfcc Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 2 Jan 2025 09:34:38 -0700 Subject: [PATCH 14/18] Cleanup --- .../admin/includes/detail_table_fieldset.html | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html index 0641c47c6..bebdd6ea2 100644 --- a/src/registrar/templates/django/admin/includes/detail_table_fieldset.html +++ b/src/registrar/templates/django/admin/includes/detail_table_fieldset.html @@ -326,18 +326,15 @@ This is using a custom implementation fieldset.html (see admin/fieldset.html) - - - - - - Clear requested suborganization - + + + + Clear requested suborganization {% else %} From 610d00b7a01080b85869dea1207c8b00c7858aa5 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 2 Jan 2025 10:55:06 -0700 Subject: [PATCH 15/18] Add unit tests --- src/registrar/admin.py | 3 + .../js/getgov-admin/domain-request-form.js | 1 - .../helpers-portfolio-dynamic-fields.js | 1 + src/registrar/models/domain.py | 1 - src/registrar/models/domain_request.py | 28 ++++-- src/registrar/tests/test_admin_request.py | 97 +++++++++++++++++++ 6 files changed, 119 insertions(+), 12 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 1476cee7e..52e214bb9 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -2003,6 +2003,9 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin): custom_requested_domain.admin_order_field = "requested_domain__name" # type: ignore + # ------ Converted fields ------ + # These fields map to @Property methods and + # require these custom definitions to work properly @admin.display(description=_("Generic Org Type")) def converted_generic_org_type(self, obj): return obj.converted_generic_org_type_display diff --git a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js index d82b2de8b..b3d14839e 100644 --- a/src/registrar/assets/src/js/getgov-admin/domain-request-form.js +++ b/src/registrar/assets/src/js/getgov-admin/domain-request-form.js @@ -645,7 +645,6 @@ function handleSuborgFieldsAndButtons() { const suborganizationCity = document.getElementById("id_suborganization_city"); const suborganizationStateTerritory = document.getElementById("id_suborganization_state_territory"); const rejectButton = document.querySelector("#clear-requested-suborganization"); - console.log("test12345678") // Ensure that every variable is present before proceeding if (!requestedSuborganizationField || !suborganizationCity || !suborganizationStateTerritory || !rejectButton) { diff --git a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js index 55b42d10d..9a60e1684 100644 --- a/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js +++ b/src/registrar/assets/src/js/getgov-admin/helpers-portfolio-dynamic-fields.js @@ -495,6 +495,7 @@ export function handlePortfolioSelection( if (requestedSuborganizationField) hideElement(requestedSuborganizationField); if (suborganizationCity) hideElement(suborganizationCity); if (suborganizationStateTerritory) hideElement(suborganizationStateTerritory); + // == LOGIC FOR THE DOMAIN REQUEST PAGE == // if (rejectSuborganizationButton) hideElement(rejectSuborganizationButton); } diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 003714dfb..6eb2fac07 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -244,7 +244,6 @@ class Domain(TimeStampedModel, DomainHelper): is called in the validate function on the request/domain page throws- RegistryError or InvalidDomainError""" - return True if not cls.string_could_be_domain(domain): logger.warning("Not a valid domain: %s" % str(domain)) # throw invalid domain error so that it can be caught in diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index f4e2a35a1..533a22ced 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -697,10 +697,18 @@ class DomainRequest(TimeStampedModel): portfolio__isnull=False, ).exists() ): - raise ValidationError( + # Add a field-level error to requested_suborganization. + # To pass in field-specific errors, we need to embed a dict of + # field: validationerror then pass that into a validation error itself. + # This is slightly confusing, but it just adds it at that level. + msg = ( "This suborganization already exists. " "Choose a new name, or select it directly if you would like to use it." ) + errors = { + "requested_suborganization": ValidationError(msg) + } + raise ValidationError(errors) elif self.portfolio and not self.sub_organization: # You cannot create a new suborganization without these fields required_suborg_fields = { @@ -708,16 +716,16 @@ class DomainRequest(TimeStampedModel): "suborganization_city": self.suborganization_city, "suborganization_state_territory": self.suborganization_state_territory, } - + # If at least one value is populated, enforce a all-or-nothing rule if any(bool(value) for value in required_suborg_fields.values()): - # Find which fields are empty - errors_dict = { - field_name: [f"This field is required when creating a new suborganization."] - for field_name, value in required_suborg_fields.items() - if not value - } - # Adds a validation error to each missing field - raise ValidationError({k: ValidationError(v, code="required") for k, v in errors_dict.items()}) + # Find which fields are empty and throw an error on the field + errors = {} + for field_name, value in required_suborg_fields.items(): + if not value: + errors[field_name] = ValidationError( + "This field is required when creating a new suborganization.", + ) + raise ValidationError(errors) def save(self, *args, **kwargs): """Save override for custom properties""" diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 439f4fab0..a294c127f 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -1,5 +1,7 @@ from datetime import datetime +from django.forms import ValidationError from django.utils import timezone +from waffle.testutils import override_flag import re from django.test import RequestFactory, Client, TestCase, override_settings from django.contrib.admin.sites import AdminSite @@ -25,6 +27,7 @@ from registrar.models import ( Portfolio, AllowedEmail, ) +from registrar.models.suborganization import Suborganization from .common import ( MockSESClient, completed_domain_request, @@ -82,6 +85,7 @@ class TestDomainRequestAdmin(MockEppLib): Contact.objects.all().delete() Website.objects.all().delete() SeniorOfficial.objects.all().delete() + Suborganization.objects.all().delete() Portfolio.objects.all().delete() self.mock_client.EMAILS_SENT.clear() @@ -91,6 +95,99 @@ class TestDomainRequestAdmin(MockEppLib): User.objects.all().delete() AllowedEmail.objects.all().delete() + @override_flag("organization_feature", active=True) + @less_console_noise_decorator + def test_clean_validates_duplicate_suborganization(self): + """Tests that clean() prevents duplicate suborganization names within the same portfolio""" + # Create a portfolio and existing suborganization + portfolio = Portfolio.objects.create( + organization_name="Test Portfolio", + creator=self.superuser + ) + + # Create an existing suborganization + Suborganization.objects.create( + name="Existing Suborg", + portfolio=portfolio + ) + + # Create a domain request trying to use the same suborganization name + # (intentionally lowercase) + domain_request = completed_domain_request( + name="test1234.gov", + portfolio=portfolio, + requested_suborganization="existing suborg", + suborganization_city="Rome", + suborganization_state_territory=DomainRequest.StateTerritoryChoices.OHIO, + ) + + # Assert that the validation error is raised + with self.assertRaises(ValidationError) as err: + domain_request.clean() + + self.assertIn( + "This suborganization already exists", + str(err.exception) + ) + + # Test that a different name is allowed. Should not raise a error. + domain_request.requested_suborganization = "New Suborg" + domain_request.clean() + + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + def test_clean_validates_partial_suborganization_fields(self): + """Tests that clean() enforces all-or-nothing rule for suborganization fields""" + portfolio = Portfolio.objects.create( + organization_name="Test Portfolio", + creator=self.superuser + ) + + # Create domain request with only city filled out + domain_request = completed_domain_request( + name="test1234.gov", + portfolio=portfolio, + suborganization_city="Test City", + ) + + # Assert validation error is raised with correct missing fields + with self.assertRaises(ValidationError) as err: + domain_request.clean() + + error_dict = err.exception.error_dict + expected_missing = ["requested_suborganization", "suborganization_state_territory"] + + # Verify correct fields are flagged as required + self.assertEqual( + sorted(error_dict.keys()), + sorted(expected_missing) + ) + + # Verify error message + for field in expected_missing: + self.assertEqual( + str(error_dict[field][0].message), + "This field is required when creating a new suborganization." + ) + + # When all data is passed in, this should validate correctly + domain_request.requested_suborganization = "Complete Suborg" + domain_request.suborganization_state_territory = DomainRequest.StateTerritoryChoices.OHIO + # Assert that no ValidationError is raised + try: + domain_request.clean() + except ValidationError as e: + self.fail(f"ValidationError was raised unexpectedly: {e}") + + # Also ensure that no validation error is raised if nothing is passed in at all + domain_request.suborganization_city = None + domain_request.requested_suborganization = None + domain_request.suborganization_state_territory = None + try: + domain_request.clean() + except ValidationError as e: + self.fail(f"ValidationError was raised unexpectedly: {e}") + @less_console_noise_decorator def test_domain_request_senior_official_is_alphabetically_sorted(self): """Tests if the senior offical dropdown is alphanetically sorted in the django admin display""" From ef8d8f17bf29a83ed3a18e2fa1c8b710be4d9836 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:51:05 -0700 Subject: [PATCH 16/18] Add unit test for requesting entity page --- src/package-lock.json | 55 ++++++++++++++++++--- src/registrar/models/domain_request.py | 4 +- src/registrar/tests/test_admin_request.py | 44 ++++++----------- src/registrar/tests/test_views_portfolio.py | 40 +++++++++++++++ 4 files changed, 103 insertions(+), 40 deletions(-) diff --git a/src/package-lock.json b/src/package-lock.json index d78b5132f..e93413312 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -6921,6 +6921,16 @@ "validate-npm-package-license": "^3.0.1" } }, + "node_modules/normalize-package-data/node_modules/semver": { + "version": "5.7.2", + "resolved": "https://registry.npmjs.org/semver/-/semver-5.7.2.tgz", + "integrity": "sha512-cBznnQ9KjJqU67B52RMC65CMarK2600WFnbkcaiwWq3xy/5haFJlshgnpjovMVJ+Hff49d8GEn0b87C5pDQ10g==", + "dev": true, + "license": "ISC", + "bin": { + "semver": "bin/semver" + } + }, "node_modules/normalize-path": { "version": "3.0.0", "resolved": "https://registry.npmjs.org/normalize-path/-/normalize-path-3.0.0.tgz", @@ -7297,6 +7307,39 @@ "node": ">= 12" } }, + "node_modules/pa11y/node_modules/lru-cache": { + "version": "6.0.0", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-6.0.0.tgz", + "integrity": "sha512-Jo6dJ04CmSjuznwJSS3pUeWmd/H0ffTlkXXgwZi+eq1UCmqQwCh+eLsYOYCwY991i2Fah4h1BEMCx4qThGbsiA==", + "license": "ISC", + "dependencies": { + "yallist": "^4.0.0" + }, + "engines": { + "node": ">=10" + } + }, + "node_modules/pa11y/node_modules/semver": { + "version": "7.3.8", + "resolved": "https://registry.npmjs.org/semver/-/semver-7.3.8.tgz", + "integrity": "sha512-NB1ctGL5rlHrPJtFDVIVzTyQylMLu9N9VICA6HSFJo8MCGVTMW6gfpicwKmmK/dAjTOrqu5l63JJOpDSrAis3A==", + "license": "ISC", + "dependencies": { + "lru-cache": "^6.0.0" + }, + "bin": { + "semver": "bin/semver.js" + }, + "engines": { + "node": ">=10" + } + }, + "node_modules/pa11y/node_modules/yallist": { + "version": "4.0.0", + "resolved": "https://registry.npmjs.org/yallist/-/yallist-4.0.0.tgz", + "integrity": "sha512-3wdGidZyq5PB084XLES5TpOSRA3wjXAlIWMhum2kRcv/41Sn2emQ0dycQW4uZXLejwKvg6EsvbdlVL+FYEct7A==", + "license": "ISC" + }, "node_modules/parse-filepath": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/parse-filepath/-/parse-filepath-1.0.2.tgz", @@ -8845,15 +8888,13 @@ } }, "node_modules/semver": { - "version": "7.6.3", - "resolved": "https://registry.npmjs.org/semver/-/semver-7.6.3.tgz", - "integrity": "sha512-oVekP1cKtI+CTDvHWYFUcMtsK/00wmAEfyqKfNdARm8u1wNVhSgaX7A8d4UuIlUI5e84iEwOhs7ZPYRmzU9U6A==", + "version": "6.3.1", + "resolved": "https://registry.npmjs.org/semver/-/semver-6.3.1.tgz", + "integrity": "sha512-BR7VvDCVHO+q2xBEWskxS6DJE1qRnb7DxzUrogb71CWoSficBxYsiAGd+Kl0mmq/MprG9yArRkyrQxTO6XjMzA==", + "dev": true, "license": "ISC", "bin": { "semver": "bin/semver.js" - }, - "engines": { - "node": ">=10" } }, "node_modules/semver-greatest-satisfied-range": { @@ -10623,4 +10664,4 @@ } } } -} +} \ No newline at end of file diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 533a22ced..3d3aac769 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -705,9 +705,7 @@ class DomainRequest(TimeStampedModel): "This suborganization already exists. " "Choose a new name, or select it directly if you would like to use it." ) - errors = { - "requested_suborganization": ValidationError(msg) - } + errors = {"requested_suborganization": ValidationError(msg)} raise ValidationError(errors) elif self.portfolio and not self.sub_organization: # You cannot create a new suborganization without these fields diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index a294c127f..8a2f49ee7 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -100,17 +100,11 @@ class TestDomainRequestAdmin(MockEppLib): def test_clean_validates_duplicate_suborganization(self): """Tests that clean() prevents duplicate suborganization names within the same portfolio""" # Create a portfolio and existing suborganization - portfolio = Portfolio.objects.create( - organization_name="Test Portfolio", - creator=self.superuser - ) - + portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.superuser) + # Create an existing suborganization - Suborganization.objects.create( - name="Existing Suborg", - portfolio=portfolio - ) - + Suborganization.objects.create(name="Existing Suborg", portfolio=portfolio) + # Create a domain request trying to use the same suborganization name # (intentionally lowercase) domain_request = completed_domain_request( @@ -124,11 +118,8 @@ class TestDomainRequestAdmin(MockEppLib): # Assert that the validation error is raised with self.assertRaises(ValidationError) as err: domain_request.clean() - - self.assertIn( - "This suborganization already exists", - str(err.exception) - ) + + self.assertIn("This suborganization already exists", str(err.exception)) # Test that a different name is allowed. Should not raise a error. domain_request.requested_suborganization = "New Suborg" @@ -138,11 +129,8 @@ class TestDomainRequestAdmin(MockEppLib): @override_flag("organization_feature", active=True) def test_clean_validates_partial_suborganization_fields(self): """Tests that clean() enforces all-or-nothing rule for suborganization fields""" - portfolio = Portfolio.objects.create( - organization_name="Test Portfolio", - creator=self.superuser - ) - + portfolio = Portfolio.objects.create(organization_name="Test Portfolio", creator=self.superuser) + # Create domain request with only city filled out domain_request = completed_domain_request( name="test1234.gov", @@ -153,21 +141,17 @@ class TestDomainRequestAdmin(MockEppLib): # Assert validation error is raised with correct missing fields with self.assertRaises(ValidationError) as err: domain_request.clean() - + error_dict = err.exception.error_dict expected_missing = ["requested_suborganization", "suborganization_state_territory"] - + # Verify correct fields are flagged as required - self.assertEqual( - sorted(error_dict.keys()), - sorted(expected_missing) - ) - + self.assertEqual(sorted(error_dict.keys()), sorted(expected_missing)) + # Verify error message for field in expected_missing: self.assertEqual( - str(error_dict[field][0].message), - "This field is required when creating a new suborganization." + str(error_dict[field][0].message), "This field is required when creating a new suborganization." ) # When all data is passed in, this should validate correctly @@ -178,7 +162,7 @@ class TestDomainRequestAdmin(MockEppLib): domain_request.clean() except ValidationError as e: self.fail(f"ValidationError was raised unexpectedly: {e}") - + # Also ensure that no validation error is raised if nothing is passed in at all domain_request.suborganization_city = None domain_request.requested_suborganization = None diff --git a/src/registrar/tests/test_views_portfolio.py b/src/registrar/tests/test_views_portfolio.py index 01383ae77..2f02f9ed9 100644 --- a/src/registrar/tests/test_views_portfolio.py +++ b/src/registrar/tests/test_views_portfolio.py @@ -2268,6 +2268,46 @@ class TestRequestingEntity(WebTest): User.objects.all().delete() super().tearDown() + @less_console_noise_decorator + @override_flag("organization_feature", active=True) + @override_flag("organization_requests", active=True) + def test_form_validates_duplicate_suborganization(self): + """Tests that form validation prevents duplicate suborganization names within the same portfolio""" + # Create an existing suborganization + suborganization = Suborganization.objects.create(name="Existing Suborg", portfolio=self.portfolio) + + # Start the domain request process + response = self.app.get(reverse("domain-request:start")) + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + + # Navigate past the intro page + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + form = response.forms[0] + response = form.submit().follow() + + # Fill out the requesting entity form + form = response.forms[0] + form["portfolio_requesting_entity-requesting_entity_is_suborganization"] = "True" + form["portfolio_requesting_entity-is_requesting_new_suborganization"] = "True" + form["portfolio_requesting_entity-requested_suborganization"] = suborganization.name.lower() + form["portfolio_requesting_entity-suborganization_city"] = "Eggnog" + form["portfolio_requesting_entity-suborganization_state_territory"] = DomainRequest.StateTerritoryChoices.OHIO + + # Submit form and verify error + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + response = form.submit() + self.assertContains(response, "This suborganization already exists") + + # Test that a different name is allowed + form["portfolio_requesting_entity-requested_suborganization"] = "New Suborg" + session_id = self.app.cookies[settings.SESSION_COOKIE_NAME] + self.app.set_cookie(settings.SESSION_COOKIE_NAME, session_id) + response = form.submit().follow() + + # Verify successful submission by checking we're on the next page + self.assertContains(response, "Current websites") + @override_flag("organization_feature", active=True) @override_flag("organization_requests", active=True) @less_console_noise_decorator From 3b7746a409db5ba14ecfdc1b2010eb59db8d755b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 2 Jan 2025 11:51:42 -0700 Subject: [PATCH 17/18] Update package-lock.json --- src/package-lock.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/package-lock.json b/src/package-lock.json index e93413312..22fb31857 100644 --- a/src/package-lock.json +++ b/src/package-lock.json @@ -10664,4 +10664,4 @@ } } } -} \ No newline at end of file +} From 419179f69d3ea9135104cb3b2e4d801a07834df4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 2 Jan 2025 12:05:03 -0700 Subject: [PATCH 18/18] Remove junk --- src/registrar/models/suborganization.py | 1 + src/registrar/tests/test_admin_request.py | 2 +- src/registrar/tests/test_reports.py | 1 - 3 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/models/suborganization.py b/src/registrar/models/suborganization.py index 78689799c..087490244 100644 --- a/src/registrar/models/suborganization.py +++ b/src/registrar/models/suborganization.py @@ -1,4 +1,5 @@ from django.db import models + from registrar.models.domain_request import DomainRequest from .utility.time_stamped_model import TimeStampedModel diff --git a/src/registrar/tests/test_admin_request.py b/src/registrar/tests/test_admin_request.py index 8a2f49ee7..efb1331df 100644 --- a/src/registrar/tests/test_admin_request.py +++ b/src/registrar/tests/test_admin_request.py @@ -26,8 +26,8 @@ from registrar.models import ( SeniorOfficial, Portfolio, AllowedEmail, + Suborganization, ) -from registrar.models.suborganization import Suborganization from .common import ( MockSESClient, completed_domain_request, diff --git a/src/registrar/tests/test_reports.py b/src/registrar/tests/test_reports.py index 88e408ed2..4a41238c7 100644 --- a/src/registrar/tests/test_reports.py +++ b/src/registrar/tests/test_reports.py @@ -870,7 +870,6 @@ class MemberExportTest(MockDbForIndividualTests, MockEppLib): csv_file.seek(0) # Read the content into a variable csv_content = csv_file.read() - self.maxDiff = None expected_content = ( # Header "Email,Organization admin,Invited by,Joined date,Last active,Domain requests,"