From f8f0d7bced1f03d45453fc03c0b6e1df2d8d205b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 10:17:52 -0700 Subject: [PATCH 01/43] Slight perf improvement for fixtures --- src/registrar/fixtures_applications.py | 30 +++++++++++++-- src/registrar/fixtures_users.py | 12 +++++- src/registrar/models/domain_application.py | 19 ++++++++- src/registrar/utility/errors.py | 45 ++++++++++++++++++++++ 4 files changed, 100 insertions(+), 6 deletions(-) diff --git a/src/registrar/fixtures_applications.py b/src/registrar/fixtures_applications.py index 3e4e0e362..29e95e02b 100644 --- a/src/registrar/fixtures_applications.py +++ b/src/registrar/fixtures_applications.py @@ -1,7 +1,7 @@ import logging import random from faker import Faker - +from django.db import transaction from registrar.models import ( User, DomainApplication, @@ -184,6 +184,14 @@ class DomainApplicationFixture: logger.warning(e) return + # Lumped under .atomic to ensure we don't make redundant DB calls. + # This bundles them all together, and then saves it in a single call. + with transaction.atomic(): + cls._create_applications(users) + + @classmethod + def _create_applications(cls, users): + """Creates DomainApplications given a list of users""" for user in users: logger.debug("Loading domain applications for %s" % user) for app in cls.DA: @@ -211,8 +219,16 @@ class DomainFixture(DomainApplicationFixture): logger.warning(e) return + # Lumped under .atomic to ensure we don't make redundant DB calls. + # This bundles them all together, and then saves it in a single call. + with transaction.atomic(): + # approve each user associated with `in review` status domains + DomainFixture._approve_applications(users) + + @staticmethod + def _approve_applications(users): + """Approves all provided applications if they are in the state in_review""" for user in users: - # approve one of each users in review status domains application = DomainApplication.objects.filter( creator=user, status=DomainApplication.ApplicationStatus.IN_REVIEW ).last() @@ -220,5 +236,13 @@ class DomainFixture(DomainApplicationFixture): # We don't want fixtures sending out real emails to # fake email addresses, so we just skip that and log it instead + + # All approvals require an investigator, so if there is none, + # assign one. + if application.investigator is None: + # All "users" in fixtures have admin perms per prior config. + # No need to check for that. + application.investigator = random.choice(users) + application.approve(send_email=False) - application.save() + application.save() \ No newline at end of file diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index f03fb025d..830adb5bf 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -1,5 +1,6 @@ import logging from faker import Faker +from django.db import transaction from registrar.models import ( User, @@ -186,5 +187,12 @@ class UserFixture: @classmethod def load(cls): - cls.load_users(cls, cls.ADMINS, "full_access_group") - cls.load_users(cls, cls.STAFF, "cisa_analysts_group") + # Lumped under .atomic to ensure we don't make redundant DB calls. + # This bundles them all together, and then saves it in a single call. + # This is slightly different then bulk_create or bulk_update, in that + # you still get the same behaviour of .save(), but those incremental + # steps now do not need to close/reopen a db connection, + # instead they share one. + with transaction.atomic(): + cls.load_users(cls, cls.ADMINS, "full_access_group") + cls.load_users(cls, cls.STAFF, "cisa_analysts_group") diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 17bc71cbe..5f204d3e3 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -8,6 +8,7 @@ from django.db import models from django_fsm import FSMField, transition # type: ignore from django.utils import timezone from registrar.models.domain import Domain +from registrar.utility.errors import ApplicationStatusError, FSMErrorCodes from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError @@ -736,8 +737,23 @@ class DomainApplication(TimeStampedModel): # create the domain Domain = apps.get_model("registrar.Domain") + + # == Check that the application is valid == # if Domain.objects.filter(name=self.requested_domain.name).exists(): - raise ValueError("Cannot approve. Requested domain is already in use.") + raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) + + # Check if an investigator is assigned. No approval is possible without one. + # TODO - add form level error + # TODO - maybe add modal if approver is not investigator as superuser + if self.investigator is None: + raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_NO_INVESTIGATOR) + + # Investigators must be staff users. + # This is handled elsewhere, but we should check here as a precaution. + if not self.investigator.is_staff: + raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) + + # == Create the domain and related components == # created_domain = Domain.objects.create(name=self.requested_domain.name) self.approved_domain = created_domain @@ -751,6 +767,7 @@ class DomainApplication(TimeStampedModel): user=self.creator, domain=created_domain, role=UserDomainRole.Roles.MANAGER ) + # == Send out an email == # self._send_status_update_email( "application approved", "emails/status_change_approved.txt", diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 03cb81893..21bdeefc6 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -70,6 +70,51 @@ class GenericError(Exception): def get_error_message(self, code=None): return self._error_mapping.get(code) +# (Q for reviewers) What should this be called? +# Not a fan of this name. +class FSMErrorCodes(IntEnum): + """Used when doing FSM transitions. + Overview of generic error codes: + - 1 APPROVE_DOMAIN_IN_USE The domain is already in use + - 2 APPROVE_NO_INVESTIGATOR No investigator is assigned when approving + - 3 APPROVE_INVESTIGATOR_NOT_STAFF Investigator is a non-staff user + """ + APPROVE_DOMAIN_IN_USE = 1 + APPROVE_NO_INVESTIGATOR = 2 + APPROVE_INVESTIGATOR_NOT_STAFF = 3 + + +class ApplicationStatusError(Exception): + """ + Used to raise exceptions when doing FSM Transitions. + Uses `FSMErrorCodes` as an enum. + """ + + _error_mapping = { + FSMErrorCodes.APPROVE_DOMAIN_IN_USE: ( + "Cannot approve. Requested domain is already in use." + ), + FSMErrorCodes.APPROVE_NO_INVESTIGATOR: ( + "Cannot approve. No investigator was assigned." + ), + FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF: ( + "Cannot approve. Investigator is not a staff user." + ), + } + + def __init__(self, *args, code=None, **kwargs): + super().__init__(*args, **kwargs) + self.code = code + if self.code in self._error_mapping: + self.message = self._error_mapping.get(self.code) + + def __str__(self): + return f"{self.message}" + + @classmethod + def get_error_message(self, code=None): + return self._error_mapping.get(code) + class NameserverErrorCodes(IntEnum): """Used in the NameserverError class for From 1a20583b487dc965af13d322e1ac54188603942d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 11:25:23 -0700 Subject: [PATCH 02/43] Update admin.py --- src/registrar/admin.py | 105 +++++++++++++++++++++-------------------- 1 file changed, 55 insertions(+), 50 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0ab6c3022..349d0ddc1 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -953,57 +953,10 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): - if obj and obj.creator.status != models.User.RESTRICTED: - if change: # Check if the application is being edited - # Get the original application from the database - original_obj = models.DomainApplication.objects.get(pk=obj.pk) - if ( - obj - and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED - and obj.status != models.DomainApplication.ApplicationStatus.APPROVED - and not obj.domain_is_not_active() - ): - # If an admin tried to set an approved application to - # another status and the related domain is already - # active, shortcut the action and throw a friendly - # error message. This action would still not go through - # shortcut or not as the rules are duplicated on the model, - # but the error would be an ugly Django error screen. - - # Clear the success message - messages.set_level(request, messages.ERROR) - - messages.error( - request, - "This action is not permitted. The domain is already active.", - ) - - else: - if obj.status != original_obj.status: - status_method_mapping = { - models.DomainApplication.ApplicationStatus.STARTED: None, - models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, - models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, - models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, - models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, - models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, - models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, - models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), - } - selected_method = status_method_mapping.get(obj.status) - if selected_method is None: - logger.warning("Unknown status selected in django admin") - else: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we roll back the - # status to what it was before the admin user changed it and - # let the fsm method set it. - obj.status = original_obj.status - selected_method() - - super().save_model(request, obj, form, change) - else: + # If the user is restricted or we're saving an invalid model, + # forbid this action. + if not obj or obj.creator.status == models.User.RESTRICTED: # Clear the success message messages.set_level(request, messages.ERROR) @@ -1012,6 +965,58 @@ class DomainApplicationAdmin(ListHeaderAdmin): "This action is not permitted for applications with a restricted creator.", ) + return None + + if change: # Check if the application is being edited + # Get the original application from the database + original_obj = models.DomainApplication.objects.get(pk=obj.pk) + + if ( + obj + and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED + and obj.status != models.DomainApplication.ApplicationStatus.APPROVED + and not obj.domain_is_not_active() + ): + # If an admin tried to set an approved application to + # another status and the related domain is already + # active, shortcut the action and throw a friendly + # error message. This action would still not go through + # shortcut or not as the rules are duplicated on the model, + # but the error would be an ugly Django error screen. + + # Clear the success message + messages.set_level(request, messages.ERROR) + + messages.error( + request, + "This action is not permitted. The domain is already active.", + ) + + else: + if obj.status != original_obj.status: + status_method_mapping = { + models.DomainApplication.ApplicationStatus.STARTED: None, + models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, + models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, + models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, + models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, + models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, + models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, + models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), + } + selected_method = status_method_mapping.get(obj.status) + if selected_method is None: + logger.warning("Unknown status selected in django admin") + else: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we roll back the + # status to what it was before the admin user changed it and + # let the fsm method set it. + obj.status = original_obj.status + selected_method() + + super().save_model(request, obj, form, change) + def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. We have 2 conditions that determine which fields are read-only: From 527f586419b9b986a8b88ecc3dd0edd9373b64b3 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 12:03:04 -0700 Subject: [PATCH 03/43] Additional refactoring --- src/registrar/admin.py | 24 ++++++++++++++++++---- src/registrar/models/domain_application.py | 5 +---- src/registrar/utility/errors.py | 2 ++ 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 349d0ddc1..f58a481df 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -16,6 +16,7 @@ from dateutil.relativedelta import relativedelta # type: ignore from epplibwrapper.errors import ErrorCode, RegistryError from registrar.models import Contact, Domain, DomainApplication, DraftDomain, User, Website from registrar.utility import csv_export +from registrar.utility.errors import ApplicationStatusError from registrar.views.utility.mixins import OrderableFieldsMixin from django.contrib.admin.views.main import ORDER_VAR from . import models @@ -953,6 +954,8 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): + # TODO - there is an existing bug in these in that they redirect + # to the table rather than back, on message display # If the user is restricted or we're saving an invalid model, # forbid this action. @@ -966,8 +969,8 @@ class DomainApplicationAdmin(ListHeaderAdmin): ) return None - - if change: # Check if the application is being edited + + if change: # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) @@ -1013,9 +1016,22 @@ class DomainApplicationAdmin(ListHeaderAdmin): # status to what it was before the admin user changed it and # let the fsm method set it. obj.status = original_obj.status - selected_method() - super().save_model(request, obj, form, change) + # Try to perform the status change. + # Catch ApplicationStatusError's and return the message, + # as these are typically user errors. + try: + selected_method() + except ApplicationStatusError as err: + # Clear the success message, if any + messages.set_level(request, messages.ERROR) + + messages.error( + request, + err.message, + ) + + super().save_model(request, obj, form, change) def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 5f204d3e3..5de6dc482 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -638,10 +638,7 @@ class DomainApplication(TimeStampedModel): # can raise more informative exceptions # requested_domain could be None here - if not hasattr(self, "requested_domain"): - raise ValueError("Requested domain is missing.") - - if self.requested_domain is None: + if not hasattr(self, "requested_domain") or self.requested_domain is None: raise ValueError("Requested domain is missing.") DraftDomain = apps.get_model("registrar.DraftDomain") diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 21bdeefc6..1d82ea49a 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -84,6 +84,8 @@ class FSMErrorCodes(IntEnum): APPROVE_INVESTIGATOR_NOT_STAFF = 3 +# (Q for reviewers) What should this be called? +# Not a fan of this name. class ApplicationStatusError(Exception): """ Used to raise exceptions when doing FSM Transitions. From cf7bf4d30491d5894a0220dda5ea0cf94d709285 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 13:14:36 -0700 Subject: [PATCH 04/43] Add form level errors --- src/registrar/admin.py | 27 ++++++++++++++++++++++++++- src/registrar/utility/errors.py | 4 ++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index f58a481df..9c3b8075e 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -16,7 +16,7 @@ from dateutil.relativedelta import relativedelta # type: ignore from epplibwrapper.errors import ErrorCode, RegistryError from registrar.models import Contact, Domain, DomainApplication, DraftDomain, User, Website from registrar.utility import csv_export -from registrar.utility.errors import ApplicationStatusError +from registrar.utility.errors import ApplicationStatusError, FSMErrorCodes from registrar.views.utility.mixins import OrderableFieldsMixin from django.contrib.admin.views.main import ORDER_VAR from . import models @@ -808,6 +808,31 @@ class DomainApplicationAdminForm(forms.ModelForm): if not application.creator.is_restricted(): self.fields["status"].widget.choices = available_transitions + def clean(self): + # clean is called from clean_forms, which is called from is_valid + # after clean_fields. it is used to determine form level errors. + # is_valid is typically called from view during a post + cleaned_data = super().clean() + investigator = cleaned_data.get("investigator") + status = cleaned_data.get("status") + requested_domain = cleaned_data.get("requested_domain") + + # Check if an investigator is assigned. No approval is possible without one. + # TODO - add form level error + # TODO - maybe add modal if approver is not investigator as superuser + if investigator is None: + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_NO_INVESTIGATOR) + self.add_error("investigator", error_message) + else: + # Investigators must be staff users. + # This is handled elsewhere, but we should check here as a precaution. + if not investigator.is_staff: + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) + self.add_error("investigator", error_message) + #if status == DomainApplication.ApplicationStatus.APPROVED and investigator != request.user: + #raise ValidationError("Only the assigned investigator can approve this application.") + + return cleaned_data class DomainApplicationAdmin(ListHeaderAdmin): """Custom domain applications admin class.""" diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 1d82ea49a..8d77ecd2e 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -114,8 +114,8 @@ class ApplicationStatusError(Exception): return f"{self.message}" @classmethod - def get_error_message(self, code=None): - return self._error_mapping.get(code) + def get_error_message(cls, code=None): + return cls._error_mapping.get(code) class NameserverErrorCodes(IntEnum): From c02e99b972fe42599c7b618321288d5f9e9b1281 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 14:21:08 -0700 Subject: [PATCH 05/43] Add logic for same investigator is approver --- src/registrar/admin.py | 66 ++++++++++++++++------ src/registrar/models/domain_application.py | 2 - src/registrar/utility/errors.py | 5 ++ 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 9c3b8075e..a33b8eaa9 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -786,8 +786,8 @@ class DomainApplicationAdminForm(forms.ModelForm): fields = "__all__" def __init__(self, *args, **kwargs): + self.request = kwargs.pop("request", None) super().__init__(*args, **kwargs) - application = kwargs.get("instance") if application and application.pk: current_state = application.status @@ -813,27 +813,42 @@ class DomainApplicationAdminForm(forms.ModelForm): # after clean_fields. it is used to determine form level errors. # is_valid is typically called from view during a post cleaned_data = super().clean() - investigator = cleaned_data.get("investigator") status = cleaned_data.get("status") - requested_domain = cleaned_data.get("requested_domain") + investigator = cleaned_data.get("investigator") - # Check if an investigator is assigned. No approval is possible without one. - # TODO - add form level error - # TODO - maybe add modal if approver is not investigator as superuser - if investigator is None: - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_NO_INVESTIGATOR) - self.add_error("investigator", error_message) - else: - # Investigators must be staff users. - # This is handled elsewhere, but we should check here as a precaution. - if not investigator.is_staff: - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) - self.add_error("investigator", error_message) - #if status == DomainApplication.ApplicationStatus.APPROVED and investigator != request.user: - #raise ValidationError("Only the assigned investigator can approve this application.") + if status == DomainApplication.ApplicationStatus.APPROVED: + # Checks the "investigators" field for validity. + # That field must obey certain conditions when an application is approved. + # Will call "add_error" if any issues are found. + self._check_investigators_on_approval(investigator) return cleaned_data + def _check_investigators_on_approval(self, investigator): + """Checks the investigator field when an approval occurs""" + + error_message = None + # Check if an investigator is assigned. No approval is possible without one. + if investigator is not None: + + if not investigator.is_staff: + # Investigators must be staff users. + # This is handled elsewhere, but we should check here as a precaution. + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) + elif investigator != self.request.user: + # If the submitting user is not the investigator, block this action. + # This is to enforce accountability. Superusers do not have this restriction. + error_message = ApplicationStatusError.get_error_message( + FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_SUBMITTER + ) + else: + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_NO_INVESTIGATOR) + + # Add the error + if error_message is not None: + self.add_error("investigator", error_message) + + class DomainApplicationAdmin(ListHeaderAdmin): """Custom domain applications admin class.""" @@ -977,6 +992,23 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Table ordering ordering = ["requested_domain__name"] + def get_form(self, request, obj=None, **kwargs): + """ + Workaround to pass the request context to the underlying DomainApplicationAdminForm form object. + This is so we can do things like check the current user against a form value at submission. + """ + # Call the superclass's get_form method to get the form class + da_form = super().get_form(request, obj, **kwargs) + + # Define a wrapper class for the form that includes the request in its initialization. + # This is needed such that we can inject request without otherwise altering it. + class DomainApplicationFormWrapper(da_form): + def __new__(cls, *args, **form_kwargs): + form_kwargs["request"] = request + return da_form(*args, **form_kwargs) + + return DomainApplicationFormWrapper + # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): # TODO - there is an existing bug in these in that they redirect diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 5de6dc482..5f4ce8a01 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -740,8 +740,6 @@ class DomainApplication(TimeStampedModel): raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) # Check if an investigator is assigned. No approval is possible without one. - # TODO - add form level error - # TODO - maybe add modal if approver is not investigator as superuser if self.investigator is None: raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_NO_INVESTIGATOR) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 8d77ecd2e..a4359b0c7 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -78,10 +78,12 @@ class FSMErrorCodes(IntEnum): - 1 APPROVE_DOMAIN_IN_USE The domain is already in use - 2 APPROVE_NO_INVESTIGATOR No investigator is assigned when approving - 3 APPROVE_INVESTIGATOR_NOT_STAFF Investigator is a non-staff user + - 4 APPROVE_INVESTIGATOR_NOT_SUBMITTER The form submitter is not the investigator """ APPROVE_DOMAIN_IN_USE = 1 APPROVE_NO_INVESTIGATOR = 2 APPROVE_INVESTIGATOR_NOT_STAFF = 3 + APPROVE_INVESTIGATOR_NOT_SUBMITTER = 4 # (Q for reviewers) What should this be called? @@ -102,6 +104,9 @@ class ApplicationStatusError(Exception): FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF: ( "Cannot approve. Investigator is not a staff user." ), + FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_SUBMITTER: ( + "Cannot approve. Only the assigned investigator can approve this application." + ) } def __init__(self, *args, code=None, **kwargs): From afeb0f55b1122cc002fd90942620b2652b28cf9e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 14:25:48 -0700 Subject: [PATCH 06/43] Add logic for superuser --- src/registrar/admin.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a33b8eaa9..71c347fed 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -827,15 +827,18 @@ class DomainApplicationAdminForm(forms.ModelForm): def _check_investigators_on_approval(self, investigator): """Checks the investigator field when an approval occurs""" + # Get information about the current user making the request + current_user = self.request.user + is_superuser = current_user.has_perm("registrar.full_access_permission") + error_message = None # Check if an investigator is assigned. No approval is possible without one. if investigator is not None: - if not investigator.is_staff: # Investigators must be staff users. # This is handled elsewhere, but we should check here as a precaution. error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) - elif investigator != self.request.user: + elif investigator != current_user and not is_superuser: # If the submitting user is not the investigator, block this action. # This is to enforce accountability. Superusers do not have this restriction. error_message = ApplicationStatusError.get_error_message( From 4eae72d983442f89fc2c845cbe5a76c86ceac9fe Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 14:51:29 -0700 Subject: [PATCH 07/43] Add todo --- src/registrar/admin.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 71c347fed..d724a91fd 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -816,6 +816,7 @@ class DomainApplicationAdminForm(forms.ModelForm): status = cleaned_data.get("status") investigator = cleaned_data.get("investigator") + # TODO - need some way of determining if a change has actually occurred and to enforce this only then if status == DomainApplication.ApplicationStatus.APPROVED: # Checks the "investigators" field for validity. # That field must obey certain conditions when an application is approved. From b30802d3d737dc36f7c5f9dddca354f4506675b8 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 23 Feb 2024 15:20:25 -0700 Subject: [PATCH 08/43] Fix some unit tests, linting Have a few more to nail down, but this is most of them --- src/registrar/admin.py | 10 +++--- src/registrar/models/domain_application.py | 2 +- src/registrar/tests/common.py | 2 ++ src/registrar/tests/test_models.py | 38 +++++++++++++++------- src/registrar/tests/test_models_domain.py | 5 ++- 5 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d724a91fd..5cfacc172 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -836,7 +836,7 @@ class DomainApplicationAdminForm(forms.ModelForm): # Check if an investigator is assigned. No approval is possible without one. if investigator is not None: if not investigator.is_staff: - # Investigators must be staff users. + # Investigators must be staff users. # This is handled elsewhere, but we should check here as a precaution. error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) elif investigator != current_user and not is_superuser: @@ -847,7 +847,7 @@ class DomainApplicationAdminForm(forms.ModelForm): ) else: error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_NO_INVESTIGATOR) - + # Add the error if error_message is not None: self.add_error("investigator", error_message) @@ -1003,14 +1003,14 @@ class DomainApplicationAdmin(ListHeaderAdmin): """ # Call the superclass's get_form method to get the form class da_form = super().get_form(request, obj, **kwargs) - + # Define a wrapper class for the form that includes the request in its initialization. # This is needed such that we can inject request without otherwise altering it. class DomainApplicationFormWrapper(da_form): def __new__(cls, *args, **form_kwargs): form_kwargs["request"] = request return da_form(*args, **form_kwargs) - + return DomainApplicationFormWrapper # Trigger action when a fieldset is changed @@ -1030,7 +1030,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): ) return None - + if change: # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 5f4ce8a01..19f82101e 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -743,7 +743,7 @@ class DomainApplication(TimeStampedModel): if self.investigator is None: raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_NO_INVESTIGATOR) - # Investigators must be staff users. + # Investigators must be staff users. # This is handled elsewhere, but we should check here as a precaution. if not self.investigator.is_staff: raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 17833d689..35c093ad5 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -530,6 +530,7 @@ def completed_application( user=False, submitter=False, name="city.gov", + investigator=None, ): """A completed domain application.""" if not user: @@ -574,6 +575,7 @@ def completed_application( submitter=submitter, creator=user, status=status, + investigator=investigator, ) if has_about_your_organization: domain_application_kwargs["about_your_organization"] = "e-Government" diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index cb7906d7a..433ad98ab 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -27,29 +27,30 @@ from django_fsm import TransitionNotAllowed @boto3_mocking.patching class TestDomainApplication(TestCase): def setUp(self): + user, _ = User.objects.get_or_create(username="testpancakesyrup", is_staff=True) self.started_application = completed_application( - status=DomainApplication.ApplicationStatus.STARTED, name="started.gov" + status=DomainApplication.ApplicationStatus.STARTED, name="started.gov", investigator=user ) self.submitted_application = completed_application( - status=DomainApplication.ApplicationStatus.SUBMITTED, name="submitted.gov" + status=DomainApplication.ApplicationStatus.SUBMITTED, name="submitted.gov", investigator=user ) self.in_review_application = completed_application( - status=DomainApplication.ApplicationStatus.IN_REVIEW, name="in-review.gov" + status=DomainApplication.ApplicationStatus.IN_REVIEW, name="in-review.gov", investigator=user ) self.action_needed_application = completed_application( - status=DomainApplication.ApplicationStatus.ACTION_NEEDED, name="action-needed.gov" + status=DomainApplication.ApplicationStatus.ACTION_NEEDED, name="action-needed.gov", investigator=user ) self.approved_application = completed_application( - status=DomainApplication.ApplicationStatus.APPROVED, name="approved.gov" + status=DomainApplication.ApplicationStatus.APPROVED, name="approved.gov", investigator=user ) self.withdrawn_application = completed_application( - status=DomainApplication.ApplicationStatus.WITHDRAWN, name="withdrawn.gov" + status=DomainApplication.ApplicationStatus.WITHDRAWN, name="withdrawn.gov", investigator=user ) self.rejected_application = completed_application( - status=DomainApplication.ApplicationStatus.REJECTED, name="rejected.gov" + status=DomainApplication.ApplicationStatus.REJECTED, name="rejected.gov", investigator=user ) self.ineligible_application = completed_application( - status=DomainApplication.ApplicationStatus.INELIGIBLE, name="ineligible.gov" + status=DomainApplication.ApplicationStatus.INELIGIBLE, name="ineligible.gov", investigator=user ) self.mock_client = MockSESClient() @@ -161,7 +162,7 @@ class TestDomainApplication(TestCase): application.submit() self.assertEqual(application.status, application.ApplicationStatus.SUBMITTED) - def check_email_sent(self, application, msg, action, expected_count): + def check_email_sent(self, application: DomainApplication, msg, action, expected_count): """Check if an email was sent after performing an action.""" with self.subTest(msg=msg, action=action): @@ -169,7 +170,14 @@ class TestDomainApplication(TestCase): with less_console_noise(): # Perform the specified action action_method = getattr(application, action) - action_method() + if action == "approve" and not application.investigator: + user, _ = User.objects.get_or_create(username="testwafflesyrup", is_staff=True) + application.investigator = user + application.save() + application.refresh_from_db() + action_method() + else: + action_method() # Check if an email was sent sent_emails = [ @@ -616,7 +624,10 @@ class TestPermissions(TestCase): def test_approval_creates_role(self): draft_domain, _ = DraftDomain.objects.get_or_create(name="igorville.gov") user, _ = User.objects.get_or_create() - application = DomainApplication.objects.create(creator=user, requested_domain=draft_domain) + investigator, _ = User.objects.get_or_create(username="frenchtoast", is_staff=True) + application = DomainApplication.objects.create( + creator=user, requested_domain=draft_domain, investigator=investigator + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client): with less_console_noise(): @@ -650,7 +661,10 @@ class TestDomainInformation(TestCase): self.maxDiff = None draft_domain, _ = DraftDomain.objects.get_or_create(name="igorville.gov") user, _ = User.objects.get_or_create() - application = DomainApplication.objects.create(creator=user, requested_domain=draft_domain, notes="test notes") + investigator, _ = User.objects.get_or_create(username="frenchtoast", is_staff=True) + application = DomainApplication.objects.create( + creator=user, requested_domain=draft_domain, notes="test notes", investigator=investigator + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client): with less_console_noise(): diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 2bd581734..b59d548db 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -270,7 +270,10 @@ class TestDomainCreation(MockEppLib): """ draft_domain, _ = DraftDomain.objects.get_or_create(name="igorville.gov") user, _ = User.objects.get_or_create() - application = DomainApplication.objects.create(creator=user, requested_domain=draft_domain) + investigator, _ = User.objects.get_or_create(username="frenchtoast", is_staff=True) + application = DomainApplication.objects.create( + creator=user, requested_domain=draft_domain, investigator=investigator + ) mock_client = MockSESClient() with boto3_mocking.clients.handler_for("sesv2", mock_client): From 02e0d00544c9ad126c1c2d1ce2f222647bcec6d3 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 08:56:03 -0700 Subject: [PATCH 09/43] Add default investigator --- src/registrar/tests/common.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index b89f0fe1a..f7bca00be 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -560,6 +560,13 @@ def completed_application( email="testy2@town.com", phone="(555) 555 5557", ) + if not investigator: + investigator, _ = User.objects.get_or_create( + username="incrediblyfakeinvestigator", + first_name = "Joe", + last_name = "Bob" + is_staff=True + ) domain_application_kwargs = dict( organization_type="federal", federal_type="executive", From 25a5cd02269d048fc0d4ce3bbc771886b71888ad Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 09:00:33 -0700 Subject: [PATCH 10/43] Linting, unit tests --- src/registrar/fixtures_applications.py | 4 ++-- src/registrar/fixtures_users.py | 4 ++-- src/registrar/tests/common.py | 10 +++++----- src/registrar/utility/errors.py | 22 +++++++++------------- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/src/registrar/fixtures_applications.py b/src/registrar/fixtures_applications.py index 29e95e02b..d1ef25211 100644 --- a/src/registrar/fixtures_applications.py +++ b/src/registrar/fixtures_applications.py @@ -188,7 +188,7 @@ class DomainApplicationFixture: # This bundles them all together, and then saves it in a single call. with transaction.atomic(): cls._create_applications(users) - + @classmethod def _create_applications(cls, users): """Creates DomainApplications given a list of users""" @@ -245,4 +245,4 @@ class DomainFixture(DomainApplicationFixture): application.investigator = random.choice(users) application.approve(send_email=False) - application.save() \ No newline at end of file + application.save() diff --git a/src/registrar/fixtures_users.py b/src/registrar/fixtures_users.py index 830adb5bf..e89809484 100644 --- a/src/registrar/fixtures_users.py +++ b/src/registrar/fixtures_users.py @@ -190,8 +190,8 @@ class UserFixture: # Lumped under .atomic to ensure we don't make redundant DB calls. # This bundles them all together, and then saves it in a single call. # This is slightly different then bulk_create or bulk_update, in that - # you still get the same behaviour of .save(), but those incremental - # steps now do not need to close/reopen a db connection, + # you still get the same behaviour of .save(), but those incremental + # steps now do not need to close/reopen a db connection, # instead they share one. with transaction.atomic(): cls.load_users(cls, cls.ADMINS, "full_access_group") diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index f7bca00be..a195ccb63 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -562,11 +562,11 @@ def completed_application( ) if not investigator: investigator, _ = User.objects.get_or_create( - username="incrediblyfakeinvestigator", - first_name = "Joe", - last_name = "Bob" - is_staff=True - ) + username="incrediblyfakeinvestigator", + first_name="Joe", + last_name="Bob", + is_staff=True, + ) domain_application_kwargs = dict( organization_type="federal", federal_type="executive", diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index a4359b0c7..fba3410f6 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -70,23 +70,25 @@ class GenericError(Exception): def get_error_message(self, code=None): return self._error_mapping.get(code) -# (Q for reviewers) What should this be called? + +# (Q for reviewers) What should this be called? # Not a fan of this name. class FSMErrorCodes(IntEnum): """Used when doing FSM transitions. Overview of generic error codes: - - 1 APPROVE_DOMAIN_IN_USE The domain is already in use + - 1 APPROVE_DOMAIN_IN_USE The domain is already in use - 2 APPROVE_NO_INVESTIGATOR No investigator is assigned when approving - 3 APPROVE_INVESTIGATOR_NOT_STAFF Investigator is a non-staff user - 4 APPROVE_INVESTIGATOR_NOT_SUBMITTER The form submitter is not the investigator """ + APPROVE_DOMAIN_IN_USE = 1 APPROVE_NO_INVESTIGATOR = 2 APPROVE_INVESTIGATOR_NOT_STAFF = 3 APPROVE_INVESTIGATOR_NOT_SUBMITTER = 4 -# (Q for reviewers) What should this be called? +# (Q for reviewers) What should this be called? # Not a fan of this name. class ApplicationStatusError(Exception): """ @@ -95,18 +97,12 @@ class ApplicationStatusError(Exception): """ _error_mapping = { - FSMErrorCodes.APPROVE_DOMAIN_IN_USE: ( - "Cannot approve. Requested domain is already in use." - ), - FSMErrorCodes.APPROVE_NO_INVESTIGATOR: ( - "Cannot approve. No investigator was assigned." - ), - FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF: ( - "Cannot approve. Investigator is not a staff user." - ), + FSMErrorCodes.APPROVE_DOMAIN_IN_USE: ("Cannot approve. Requested domain is already in use."), + FSMErrorCodes.APPROVE_NO_INVESTIGATOR: ("Cannot approve. No investigator was assigned."), + FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF: ("Cannot approve. Investigator is not a staff user."), FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_SUBMITTER: ( "Cannot approve. Only the assigned investigator can approve this application." - ) + ), } def __init__(self, *args, code=None, **kwargs): From 4d4cd8257ce1b4f899683790cdad1e12a9061996 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 09:05:34 -0700 Subject: [PATCH 11/43] Update fixtures_applications.py --- src/registrar/fixtures_applications.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/fixtures_applications.py b/src/registrar/fixtures_applications.py index d1ef25211..e1cfc859e 100644 --- a/src/registrar/fixtures_applications.py +++ b/src/registrar/fixtures_applications.py @@ -242,7 +242,7 @@ class DomainFixture(DomainApplicationFixture): if application.investigator is None: # All "users" in fixtures have admin perms per prior config. # No need to check for that. - application.investigator = random.choice(users) + application.investigator = random.choice(users) # nosec application.approve(send_email=False) application.save() From d416ea5138926ca44d2d6cc3b37352ed06198826 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 10:49:38 -0700 Subject: [PATCH 12/43] Add checks for other fields --- src/registrar/admin.py | 51 +++++++++++++++++---------------- src/registrar/utility/errors.py | 20 ++++++------- 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 95036812d..e68943272 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -824,41 +824,42 @@ class DomainApplicationAdminForm(forms.ModelForm): status = cleaned_data.get("status") investigator = cleaned_data.get("investigator") - # TODO - need some way of determining if a change has actually occurred and to enforce this only then - if status == DomainApplication.ApplicationStatus.APPROVED: - # Checks the "investigators" field for validity. - # That field must obey certain conditions when an application is approved. - # Will call "add_error" if any issues are found. - self._check_investigators_on_approval(investigator) + checked_statuses = [ + DomainApplication.ApplicationStatus.APPROVED, + DomainApplication.ApplicationStatus.IN_REVIEW, + DomainApplication.ApplicationStatus.ACTION_NEEDED, + DomainApplication.ApplicationStatus.REJECTED, + DomainApplication.ApplicationStatus.INELIGIBLE + ] + # Checks the "investigators" field for validity. + # That field must obey certain conditions when an application is approved. + # Will call "add_error" if any issues are found. + #if status in checked_statuses: + #self._check_for_valid_investigator(investigator) return cleaned_data - def _check_investigators_on_approval(self, investigator): - """Checks the investigator field when an approval occurs""" + def _check_for_valid_investigator(self, investigator) -> bool: + """ + Checks if the investigator field is not none, and is staff. + Adds form errors on failure. + """ - # Get information about the current user making the request - current_user = self.request.user - is_superuser = current_user.has_perm("registrar.full_access_permission") + is_valid = False - error_message = None # Check if an investigator is assigned. No approval is possible without one. - if investigator is not None: - if not investigator.is_staff: - # Investigators must be staff users. - # This is handled elsewhere, but we should check here as a precaution. - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) - elif investigator != current_user and not is_superuser: - # If the submitting user is not the investigator, block this action. - # This is to enforce accountability. Superusers do not have this restriction. - error_message = ApplicationStatusError.get_error_message( - FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_SUBMITTER - ) + error_message = None + if investigator is None: + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.NO_INVESTIGATOR) + elif not investigator.is_staff: + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.INVESTIGATOR_NOT_STAFF) else: - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.APPROVE_NO_INVESTIGATOR) + is_valid = True - # Add the error if error_message is not None: self.add_error("investigator", error_message) + + return is_valid class DomainApplicationAdmin(ListHeaderAdmin): diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index fba3410f6..f399299ff 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -77,15 +77,15 @@ class FSMErrorCodes(IntEnum): """Used when doing FSM transitions. Overview of generic error codes: - 1 APPROVE_DOMAIN_IN_USE The domain is already in use - - 2 APPROVE_NO_INVESTIGATOR No investigator is assigned when approving - - 3 APPROVE_INVESTIGATOR_NOT_STAFF Investigator is a non-staff user - - 4 APPROVE_INVESTIGATOR_NOT_SUBMITTER The form submitter is not the investigator + - 2 NO_INVESTIGATOR No investigator is assigned + - 3 INVESTIGATOR_NOT_STAFF Investigator is a non-staff user + - 4 INVESTIGATOR_NOT_SUBMITTER The form submitter is not the investigator """ APPROVE_DOMAIN_IN_USE = 1 - APPROVE_NO_INVESTIGATOR = 2 - APPROVE_INVESTIGATOR_NOT_STAFF = 3 - APPROVE_INVESTIGATOR_NOT_SUBMITTER = 4 + NO_INVESTIGATOR = 2 + INVESTIGATOR_NOT_STAFF = 3 + INVESTIGATOR_NOT_SUBMITTER = 4 # (Q for reviewers) What should this be called? @@ -98,10 +98,10 @@ class ApplicationStatusError(Exception): _error_mapping = { FSMErrorCodes.APPROVE_DOMAIN_IN_USE: ("Cannot approve. Requested domain is already in use."), - FSMErrorCodes.APPROVE_NO_INVESTIGATOR: ("Cannot approve. No investigator was assigned."), - FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF: ("Cannot approve. Investigator is not a staff user."), - FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_SUBMITTER: ( - "Cannot approve. Only the assigned investigator can approve this application." + FSMErrorCodes.NO_INVESTIGATOR: ("No investigator was assigned."), + FSMErrorCodes.INVESTIGATOR_NOT_STAFF: ("Investigator is not a staff user."), + FSMErrorCodes.INVESTIGATOR_NOT_SUBMITTER: ( + "Only the assigned investigator can make this change." ), } From 6542d03bc5a8ec8e3b53c51a72a5de5fab2ed0a0 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 10:50:38 -0700 Subject: [PATCH 13/43] Use FSM instead --- src/registrar/assets/js/get-gov-admin.js | 35 ++++++++++++++++++++++ src/registrar/models/domain_application.py | 26 ++++++++-------- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 29aa9ce03..687346b37 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -49,6 +49,41 @@ function openInNewTab(el, removeAttribute = false){ } createPhantomModalFormButtons(); + + document.addEventListener("DOMContentLoaded", function() { + const statusSelect = document.getElementById('id_status'); + const investigatorSelect = document.getElementById('id_investigator'); // Adjust the ID based on your actual field name + + function checkInvestigatorAndShowMessage() { + // Assuming the first option is the default "---------" or similar + const investigatorSelected = investigatorSelect.selectedIndex > 0; + + // Remove existing messages to prevent duplicates + const existingMessage = document.querySelector('.no-investigator-exists'); + if (existingMessage) { + existingMessage.remove(); + } + + const flexContainerParent = statusSelect.closest('.flex-container'); + if (!investigatorSelected && flexContainerParent) { + const message = document.createElement("div"); + message.classList.add("no-investigator-exists"); + message.classList.add("padding-top-1") + message.classList.add("font-1") + message.textContent = '* An investigator must be added before other options will display.'; + + // Insert the message before the flex-container parent + flexContainerParent.insertAdjacentElement('afterend', message); + } + + } + + // Initial check in case the form is loaded with a selection already + checkInvestigatorAndShowMessage(); + + // Add event listener to re-check whenever the investigator selection changes + investigatorSelect.addEventListener('change', checkInvestigatorAndShowMessage); + }); })(); /** An IIFE for pages in DjangoAdmin which may need custom JS implementation. * Currently only appends target="_blank" to the domain_form object, diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 19f82101e..84a06fa39 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -619,6 +619,14 @@ class DomainApplication(TimeStampedModel): except EmailSendingError: logger.warning("Failed to send confirmation email", exc_info=True) + def investigator_exists_and_is_staff(self): + """Checks if the current investigator is in a valid state for a state transition""" + is_valid = True + # Check if an investigator is assigned. No approval is possible without one. + if self.investigator is None or not self.investigator.is_staff: + is_valid = False + return is_valid + @transition( field="status", source=[ @@ -669,7 +677,7 @@ class DomainApplication(TimeStampedModel): ApplicationStatus.INELIGIBLE, ], target=ApplicationStatus.IN_REVIEW, - conditions=[domain_is_not_active], + conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def in_review(self): """Investigate an application that has been submitted. @@ -696,7 +704,7 @@ class DomainApplication(TimeStampedModel): ApplicationStatus.INELIGIBLE, ], target=ApplicationStatus.ACTION_NEEDED, - conditions=[domain_is_not_active], + conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def action_needed(self): """Send back an application that is under investigation or rejected. @@ -723,6 +731,7 @@ class DomainApplication(TimeStampedModel): ApplicationStatus.REJECTED, ], target=ApplicationStatus.APPROVED, + conditions=[investigator_exists_and_is_staff] ) def approve(self, send_email=True): """Approve an application that has been submitted. @@ -739,15 +748,6 @@ class DomainApplication(TimeStampedModel): if Domain.objects.filter(name=self.requested_domain.name).exists(): raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) - # Check if an investigator is assigned. No approval is possible without one. - if self.investigator is None: - raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_NO_INVESTIGATOR) - - # Investigators must be staff users. - # This is handled elsewhere, but we should check here as a precaution. - if not self.investigator.is_staff: - raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) - # == Create the domain and related components == # created_domain = Domain.objects.create(name=self.requested_domain.name) self.approved_domain = created_domain @@ -788,7 +788,7 @@ class DomainApplication(TimeStampedModel): field="status", source=[ApplicationStatus.IN_REVIEW, ApplicationStatus.ACTION_NEEDED, ApplicationStatus.APPROVED], target=ApplicationStatus.REJECTED, - conditions=[domain_is_not_active], + conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def reject(self): """Reject an application that has been submitted. @@ -814,7 +814,7 @@ class DomainApplication(TimeStampedModel): ApplicationStatus.REJECTED, ], target=ApplicationStatus.INELIGIBLE, - conditions=[domain_is_not_active], + conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def reject_with_prejudice(self): """The applicant is a bad actor, reject with prejudice. From 79d3c1821d838094fd900d483a015ae965109654 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 11:23:40 -0700 Subject: [PATCH 14/43] Cleanup --- src/registrar/admin.py | 17 ++++++++++++----- src/registrar/models/domain_application.py | 9 --------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e68943272..ec6b625fb 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -824,6 +824,10 @@ class DomainApplicationAdminForm(forms.ModelForm): status = cleaned_data.get("status") investigator = cleaned_data.get("investigator") + # Get the old status + initial_status = self.initial.get("status", None) + + # We only care about investigator when in these statuses checked_statuses = [ DomainApplication.ApplicationStatus.APPROVED, DomainApplication.ApplicationStatus.IN_REVIEW, @@ -831,11 +835,13 @@ class DomainApplicationAdminForm(forms.ModelForm): DomainApplication.ApplicationStatus.REJECTED, DomainApplication.ApplicationStatus.INELIGIBLE ] - # Checks the "investigators" field for validity. - # That field must obey certain conditions when an application is approved. - # Will call "add_error" if any issues are found. - #if status in checked_statuses: - #self._check_for_valid_investigator(investigator) + + # If a status change occured, check for validity + if status != initial_status and status in checked_statuses: + # Checks the "investigators" field for validity. + # That field must obey certain conditions when an application is approved. + # Will call "add_error" if any issues are found. + self._check_for_valid_investigator(investigator) return cleaned_data @@ -850,6 +856,7 @@ class DomainApplicationAdminForm(forms.ModelForm): # Check if an investigator is assigned. No approval is possible without one. error_message = None if investigator is None: + # Lets grab the error message from a common location error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.NO_INVESTIGATOR) elif not investigator.is_staff: error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.INVESTIGATOR_NOT_STAFF) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 19f82101e..3337c8c7a 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -739,15 +739,6 @@ class DomainApplication(TimeStampedModel): if Domain.objects.filter(name=self.requested_domain.name).exists(): raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) - # Check if an investigator is assigned. No approval is possible without one. - if self.investigator is None: - raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_NO_INVESTIGATOR) - - # Investigators must be staff users. - # This is handled elsewhere, but we should check here as a precaution. - if not self.investigator.is_staff: - raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_INVESTIGATOR_NOT_STAFF) - # == Create the domain and related components == # created_domain = Domain.objects.create(name=self.requested_domain.name) self.approved_domain = created_domain From 3a9683cf490e12d3da019e178b7c9c91a9f71ab6 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 13:02:12 -0700 Subject: [PATCH 15/43] Add custom field transitions Kudos to rachid --- src/registrar/admin.py | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index e68943272..c5e177bf4 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -803,9 +803,14 @@ class DomainApplicationAdminForm(forms.ModelForm): # first option in status transitions is current state available_transitions = [(current_state, application.get_status_display())] - transitions = get_available_FIELD_transitions( - application, models.DomainApplication._meta.get_field("status") - ) + if application.investigator is not None: + transitions = get_available_FIELD_transitions( + application, models.DomainApplication._meta.get_field("status") + ) + else: + transitions = self.get_custom_field_transitions( + application, models.DomainApplication._meta.get_field("status") + ) for transition in transitions: available_transitions.append((transition.target, transition.target.label)) @@ -816,6 +821,17 @@ class DomainApplicationAdminForm(forms.ModelForm): if not application.creator.is_restricted(): self.fields["status"].widget.choices = available_transitions + def get_custom_field_transitions(self, instance, field): + """Custom implementation of get_available_FIELD_transitions + in the FSM. Allows us to still display fields filtered out by a condition.""" + curr_state = field.get_state(instance) + transitions = field.transitions[instance.__class__] + + for name, transition in transitions.items(): + meta = transition._django_fsm + if meta.has_transition(curr_state): + yield meta.get_transition(curr_state) + def clean(self): # clean is called from clean_forms, which is called from is_valid # after clean_fields. it is used to determine form level errors. From 66ba6919190c7593d56120b2a62da0d0e08c022b Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 13:03:36 -0700 Subject: [PATCH 16/43] Revert admin change --- src/registrar/assets/js/get-gov-admin.js | 35 ------------------------ 1 file changed, 35 deletions(-) diff --git a/src/registrar/assets/js/get-gov-admin.js b/src/registrar/assets/js/get-gov-admin.js index 687346b37..29aa9ce03 100644 --- a/src/registrar/assets/js/get-gov-admin.js +++ b/src/registrar/assets/js/get-gov-admin.js @@ -49,41 +49,6 @@ function openInNewTab(el, removeAttribute = false){ } createPhantomModalFormButtons(); - - document.addEventListener("DOMContentLoaded", function() { - const statusSelect = document.getElementById('id_status'); - const investigatorSelect = document.getElementById('id_investigator'); // Adjust the ID based on your actual field name - - function checkInvestigatorAndShowMessage() { - // Assuming the first option is the default "---------" or similar - const investigatorSelected = investigatorSelect.selectedIndex > 0; - - // Remove existing messages to prevent duplicates - const existingMessage = document.querySelector('.no-investigator-exists'); - if (existingMessage) { - existingMessage.remove(); - } - - const flexContainerParent = statusSelect.closest('.flex-container'); - if (!investigatorSelected && flexContainerParent) { - const message = document.createElement("div"); - message.classList.add("no-investigator-exists"); - message.classList.add("padding-top-1") - message.classList.add("font-1") - message.textContent = '* An investigator must be added before other options will display.'; - - // Insert the message before the flex-container parent - flexContainerParent.insertAdjacentElement('afterend', message); - } - - } - - // Initial check in case the form is loaded with a selection already - checkInvestigatorAndShowMessage(); - - // Add event listener to re-check whenever the investigator selection changes - investigatorSelect.addEventListener('change', checkInvestigatorAndShowMessage); - }); })(); /** An IIFE for pages in DjangoAdmin which may need custom JS implementation. * Currently only appends target="_blank" to the domain_form object, From 932f32b3dcb7979cc3392fb1781bff85199b5abc Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:09:08 -0700 Subject: [PATCH 17/43] Add some test cases Still need to add a few more --- src/registrar/tests/test_models.py | 166 +++++++++++++++++++++++++++++ 1 file changed, 166 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 433ad98ab..a4eac03e4 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -227,6 +227,33 @@ class TestDomainApplication(TestCase): application = completed_application(status=DomainApplication.ApplicationStatus.APPROVED) self.check_email_sent(application, msg, "reject_with_prejudice", 0) + def test_submit_transition_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator. + For submit, this should be valid in all cases. + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.in_review_application.investigator = None + self.action_needed_application.investigator = None + + # Save changes + self.in_review_application.save() + self.action_needed_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + try: + application.submit() + except TransitionNotAllowed: + self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_submit_transition_allowed(self): """ Test that calling submit from allowable statuses does raises TransitionNotAllowed. @@ -247,6 +274,27 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + + def test_submit_transition_allowed_twice(self): + """ + Test that rotating between submit and in_review doesn't throw an error + """ + with boto3_mocking.clients.handler_for("sesv2", self.mock_client): + with less_console_noise(): + try: + # Make a submission + self.in_review_application.submit() + + # Rerun the old method to get back to the original state + self.in_review_application.in_review() + + # Make another submission + self.in_review_application.submit() + except TransitionNotAllowed: + self.fail("TransitionNotAllowed was raised, but it was not expected.") + + self.assertEqual(self.in_review_application.status, DomainApplication.ApplicationStatus.SUBMITTED) + def test_submit_transition_not_allowed(self): """ Test that calling submit against transition rules raises TransitionNotAllowed. @@ -286,6 +334,36 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_in_review_transition_not_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator + """ + + test_cases = [ + (self.action_needed_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + (self.ineligible_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.approved_application.investigator = None + self.action_needed_application.investigator = None + self.rejected_application.investigator = None + self.ineligible_application.investigator = None + + # Save changes + self.approved_application.save() + self.action_needed_application.save() + self.rejected_application.save() + self.ineligible_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + with self.assertRaises(exception_type): + application.in_review() + def test_in_review_transition_not_allowed(self): """ Test that calling in_review against transition rules raises TransitionNotAllowed. @@ -321,6 +399,37 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_action_needed_transition_not_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + (self.ineligible_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.in_review_application.investigator = None + self.approved_application.investigator = None + self.action_needed_application.investigator = None + self.rejected_application.investigator = None + self.ineligible_application.investigator = None + + # Save changes + self.in_review_application.save() + self.action_needed_application.save() + self.rejected_application.save() + self.ineligible_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + with self.assertRaises(exception_type): + application.action_needed() + def test_action_needed_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. @@ -357,6 +466,33 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_approved_transition_not_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.in_review_application.investigator = None + self.action_needed_application.investigator = None + self.rejected_application.investigator = None + + # Save changes + self.in_review_application.save() + self.action_needed_application.save() + self.rejected_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + with self.assertRaises(exception_type): + application.approve() + def test_approved_skips_sending_email(self): """ Test that calling .approve with send_email=False doesn't actually send @@ -407,6 +543,36 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_withdraw_transition_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator. + For withdraw, this should be valid in all cases. + """ + + test_cases = [ + (self.submitted_application, TransitionNotAllowed), + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.submitted_application.investigator = None + self.in_review_application.investigator = None + self.action_needed_application.investigator = None + + # Save changes + self.submitted_application.save() + self.in_review_application.save() + self.action_needed_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + try: + application.withdraw() + except TransitionNotAllowed: + self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_withdraw_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. From d978211c6d7d84fd00ce4bcdde53b698e628cbe2 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:13:02 -0700 Subject: [PATCH 18/43] Update test_models.py --- src/registrar/tests/test_models.py | 57 ++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index a4eac03e4..4c07abe64 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -611,6 +611,33 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_reject_transition_not_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.in_review_application.investigator = None + self.action_needed_application.investigator = None + self.approved_application.investigator = None + + # Save changes + self.in_review_application.save() + self.action_needed_application.save() + self.approved_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + with self.assertRaises(exception_type): + application.reject() + def test_reject_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. @@ -650,6 +677,36 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_reject_with_prejudice_transition_not_allowed_with_no_investigator(self): + """ + Tests for attempting to transition without an investigator + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + ] + + # Set all investigators to none + self.in_review_application.investigator = None + self.action_needed_application.investigator = None + self.approved_application.investigator = None + self.rejected_application.investigator = None + + # Save changes + self.in_review_application.save() + self.action_needed_application.save() + self.approved_application.save() + self.rejected_application.save() + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + with self.assertRaises(exception_type): + application.reject_with_prejudice() + def test_reject_with_prejudice_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. From d60ff230208e8fba9df3845b1a66cf87eba1de12 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:47:42 -0700 Subject: [PATCH 19/43] Simplify FSM tests --- src/registrar/tests/common.py | 7 + src/registrar/tests/test_models.py | 261 ++++++++++++++--------------- 2 files changed, 133 insertions(+), 135 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index a195ccb63..81a8cee6e 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -601,6 +601,13 @@ def completed_application( return application +def set_applications_investigators(application_list: list[DomainApplication], investigator_user: User): + """Helper method that sets the investigator field of all provided applications""" + for application in application_list: + application.investigator = investigator_user + application.save() + + def multiple_unalphabetical_domain_objects( domain_type=AuditedAdminMockData.APPLICATION, ): diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 4c07abe64..9d3c7fc27 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -17,7 +17,7 @@ from registrar.models import ( import boto3_mocking from registrar.models.transition_domain import TransitionDomain from registrar.models.verified_by_staff import VerifiedByStaff # type: ignore -from .common import MockSESClient, less_console_noise, completed_application +from .common import MockSESClient, less_console_noise, completed_application, set_applications_investigators from django_fsm import TransitionNotAllowed @@ -27,32 +27,43 @@ from django_fsm import TransitionNotAllowed @boto3_mocking.patching class TestDomainApplication(TestCase): def setUp(self): - user, _ = User.objects.get_or_create(username="testpancakesyrup", is_staff=True) self.started_application = completed_application( - status=DomainApplication.ApplicationStatus.STARTED, name="started.gov", investigator=user + status=DomainApplication.ApplicationStatus.STARTED, name="started.gov" ) self.submitted_application = completed_application( - status=DomainApplication.ApplicationStatus.SUBMITTED, name="submitted.gov", investigator=user + status=DomainApplication.ApplicationStatus.SUBMITTED, name="submitted.gov" ) self.in_review_application = completed_application( - status=DomainApplication.ApplicationStatus.IN_REVIEW, name="in-review.gov", investigator=user + status=DomainApplication.ApplicationStatus.IN_REVIEW, name="in-review.gov" ) self.action_needed_application = completed_application( - status=DomainApplication.ApplicationStatus.ACTION_NEEDED, name="action-needed.gov", investigator=user + status=DomainApplication.ApplicationStatus.ACTION_NEEDED, name="action-needed.gov" ) self.approved_application = completed_application( - status=DomainApplication.ApplicationStatus.APPROVED, name="approved.gov", investigator=user + status=DomainApplication.ApplicationStatus.APPROVED, name="approved.gov" ) self.withdrawn_application = completed_application( - status=DomainApplication.ApplicationStatus.WITHDRAWN, name="withdrawn.gov", investigator=user + status=DomainApplication.ApplicationStatus.WITHDRAWN, name="withdrawn.gov" ) self.rejected_application = completed_application( - status=DomainApplication.ApplicationStatus.REJECTED, name="rejected.gov", investigator=user + status=DomainApplication.ApplicationStatus.REJECTED, name="rejected.gov" ) self.ineligible_application = completed_application( - status=DomainApplication.ApplicationStatus.INELIGIBLE, name="ineligible.gov", investigator=user + status=DomainApplication.ApplicationStatus.INELIGIBLE, name="ineligible.gov" ) - + + # Store all aplpication statuses in a variable for ease of use + self.all_applications = [ + self.started_application, + self.submitted_application, + self.in_review_application, + self.action_needed_application, + self.approved_application, + self.withdrawn_application, + self.rejected_application, + self.ineligible_application, + ] + self.mock_client = MockSESClient() def tearDown(self): @@ -227,6 +238,17 @@ class TestDomainApplication(TestCase): application = completed_application(status=DomainApplication.ApplicationStatus.APPROVED) self.check_email_sent(application, msg, "reject_with_prejudice", 0) + def assert_fsm_transition_raises_error(self, test_cases, method_to_run): + """Given a list of test cases, check if each transition throws the intended error""" + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + with self.assertRaises(exception_type): + # Retrieve the method by name from the application object and call it + method = getattr(application, method_to_run) + # Call the method + method() + def test_submit_transition_allowed_with_no_investigator(self): """ Tests for attempting to transition without an investigator. @@ -239,12 +261,30 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.in_review_application.investigator = None - self.action_needed_application.investigator = None + set_applications_investigators(self.all_applications, None) - # Save changes - self.in_review_application.save() - self.action_needed_application.save() + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + try: + application.submit() + except TransitionNotAllowed: + self.fail("TransitionNotAllowed was raised, but it was not expected.") + + def test_submit_transition_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition with an investigator user that is not staff. + For submit, this should be valid in all cases. + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): for application, exception_type in test_cases: @@ -306,12 +346,7 @@ class TestDomainApplication(TestCase): (self.ineligible_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.submit() + self.assert_fsm_transition_raises_error(test_cases, "submit") def test_in_review_transition_allowed(self): """ @@ -347,22 +382,28 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.approved_application.investigator = None - self.action_needed_application.investigator = None - self.rejected_application.investigator = None - self.ineligible_application.investigator = None + set_applications_investigators(self.all_applications, None) - # Save changes - self.approved_application.save() - self.action_needed_application.save() - self.rejected_application.save() - self.ineligible_application.save() + self.assert_fsm_transition_raises_error(test_cases, "in_review") + + def test_in_review_transition_not_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition with an investigator that is not staff. + This should throw an exception. + """ - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.in_review() + test_cases = [ + (self.action_needed_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + (self.ineligible_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) + + self.assert_fsm_transition_raises_error(test_cases, "in_review") def test_in_review_transition_not_allowed(self): """ @@ -374,12 +415,7 @@ class TestDomainApplication(TestCase): (self.withdrawn_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.in_review() + self.assert_fsm_transition_raises_error(test_cases, "in_review") def test_action_needed_transition_allowed(self): """ @@ -412,23 +448,27 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.in_review_application.investigator = None - self.approved_application.investigator = None - self.action_needed_application.investigator = None - self.rejected_application.investigator = None - self.ineligible_application.investigator = None + set_applications_investigators(self.all_applications, None) - # Save changes - self.in_review_application.save() - self.action_needed_application.save() - self.rejected_application.save() - self.ineligible_application.save() + self.assert_fsm_transition_raises_error(test_cases, "action_needed") - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.action_needed() + def test_action_needed_transition_not_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition with an investigator that is not staff + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + (self.ineligible_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) + + self.assert_fsm_transition_raises_error(test_cases, "action_needed") def test_action_needed_transition_not_allowed(self): """ @@ -440,11 +480,8 @@ class TestDomainApplication(TestCase): (self.action_needed_application, TransitionNotAllowed), (self.withdrawn_application, TransitionNotAllowed), ] - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.action_needed() + + self.assert_fsm_transition_raises_error(test_cases, "action_needed") def test_approved_transition_allowed(self): """ @@ -478,20 +515,26 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.in_review_application.investigator = None - self.action_needed_application.investigator = None - self.rejected_application.investigator = None + set_applications_investigators(self.all_applications, None) - # Save changes - self.in_review_application.save() - self.action_needed_application.save() - self.rejected_application.save() + self.assert_fsm_transition_raises_error(test_cases, "approve") - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.approve() + def test_approved_transition_not_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition with an investigator that is not staff + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) + + self.assert_fsm_transition_raises_error(test_cases, "approve") def test_approved_skips_sending_email(self): """ @@ -516,13 +559,7 @@ class TestDomainApplication(TestCase): (self.withdrawn_application, TransitionNotAllowed), (self.ineligible_application, TransitionNotAllowed), ] - - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.approve() + self.assert_fsm_transition_raises_error(test_cases, "approve") def test_withdraw_transition_allowed(self): """ @@ -556,14 +593,7 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.submitted_application.investigator = None - self.in_review_application.investigator = None - self.action_needed_application.investigator = None - - # Save changes - self.submitted_application.save() - self.in_review_application.save() - self.action_needed_application.save() + set_applications_investigators(self.all_applications, None) with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): for application, exception_type in test_cases: @@ -585,12 +615,7 @@ class TestDomainApplication(TestCase): (self.ineligible_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.withdraw() + self.assert_fsm_transition_raises_error(test_cases, "withdraw") def test_reject_transition_allowed(self): """ @@ -623,20 +648,9 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.in_review_application.investigator = None - self.action_needed_application.investigator = None - self.approved_application.investigator = None + set_applications_investigators(self.all_applications, None) - # Save changes - self.in_review_application.save() - self.action_needed_application.save() - self.approved_application.save() - - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.reject() + self.assert_fsm_transition_raises_error(test_cases, "reject") def test_reject_transition_not_allowed(self): """ @@ -650,12 +664,7 @@ class TestDomainApplication(TestCase): (self.ineligible_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.reject() + self.assert_fsm_transition_raises_error(test_cases, "reject") def test_reject_with_prejudice_transition_allowed(self): """ @@ -690,22 +699,9 @@ class TestDomainApplication(TestCase): ] # Set all investigators to none - self.in_review_application.investigator = None - self.action_needed_application.investigator = None - self.approved_application.investigator = None - self.rejected_application.investigator = None + set_applications_investigators(self.all_applications, None) - # Save changes - self.in_review_application.save() - self.action_needed_application.save() - self.approved_application.save() - self.rejected_application.save() - - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.reject_with_prejudice() + self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") def test_reject_with_prejudice_transition_not_allowed(self): """ @@ -718,12 +714,7 @@ class TestDomainApplication(TestCase): (self.ineligible_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - with self.assertRaises(exception_type): - application.reject_with_prejudice() + self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") def test_transition_not_allowed_approved_in_review_when_domain_is_active(self): """Create an application with status approved, create a matching domain that From 3b4e470f0c197557664210f86600e46bb227c39f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:52:35 -0700 Subject: [PATCH 20/43] Even more unit testing Almost done with it! --- src/registrar/tests/test_models.py | 60 +++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 9d3c7fc27..bcd68d539 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -314,7 +314,6 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") - def test_submit_transition_allowed_twice(self): """ Test that rotating between submit and in_review doesn't throw an error @@ -603,6 +602,30 @@ class TestDomainApplication(TestCase): except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_withdraw_transition_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition when investigator is not staff. + For withdraw, this should be valid in all cases. + """ + + test_cases = [ + (self.submitted_application, TransitionNotAllowed), + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) + + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + try: + application.withdraw() + except TransitionNotAllowed: + self.fail("TransitionNotAllowed was raised, but it was not expected.") + def test_withdraw_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. @@ -652,6 +675,23 @@ class TestDomainApplication(TestCase): self.assert_fsm_transition_raises_error(test_cases, "reject") + def test_reject_transition_not_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition when investigator is not staff + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) + + self.assert_fsm_transition_raises_error(test_cases, "reject") + def test_reject_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. @@ -703,6 +743,24 @@ class TestDomainApplication(TestCase): self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") + def test_reject_with_prejudice_not_allowed_with_investigator_not_staff(self): + """ + Tests for attempting to transition when investigator is not staff + """ + + test_cases = [ + (self.in_review_application, TransitionNotAllowed), + (self.action_needed_application, TransitionNotAllowed), + (self.approved_application, TransitionNotAllowed), + (self.rejected_application, TransitionNotAllowed), + ] + + # Set all investigators to a user with no staff privs + user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) + set_applications_investigators(self.all_applications, user) + + self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") + def test_reject_with_prejudice_transition_not_allowed(self): """ Test that calling action_needed against transition rules raises TransitionNotAllowed. From 3d6f63846270a40c1362fda1b5f4dbf92a517247 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 15:02:54 -0700 Subject: [PATCH 21/43] Simplify test cases further, linting --- src/registrar/admin.py | 6 +- src/registrar/models/domain_application.py | 2 +- src/registrar/tests/test_models.py | 116 ++++++--------------- 3 files changed, 33 insertions(+), 91 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 847e8595c..176c0bb87 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -849,7 +849,7 @@ class DomainApplicationAdminForm(forms.ModelForm): DomainApplication.ApplicationStatus.IN_REVIEW, DomainApplication.ApplicationStatus.ACTION_NEEDED, DomainApplication.ApplicationStatus.REJECTED, - DomainApplication.ApplicationStatus.INELIGIBLE + DomainApplication.ApplicationStatus.INELIGIBLE, ] # If a status change occured, check for validity @@ -864,7 +864,7 @@ class DomainApplicationAdminForm(forms.ModelForm): def _check_for_valid_investigator(self, investigator) -> bool: """ Checks if the investigator field is not none, and is staff. - Adds form errors on failure. + Adds form errors on failure. """ is_valid = False @@ -881,7 +881,7 @@ class DomainApplicationAdminForm(forms.ModelForm): if error_message is not None: self.add_error("investigator", error_message) - + return is_valid diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 84a06fa39..51f832ec2 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -731,7 +731,7 @@ class DomainApplication(TimeStampedModel): ApplicationStatus.REJECTED, ], target=ApplicationStatus.APPROVED, - conditions=[investigator_exists_and_is_staff] + conditions=[investigator_exists_and_is_staff], ) def approve(self, send_email=True): """Approve an application that has been submitted. diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index bcd68d539..8e9b9dc4e 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -51,7 +51,7 @@ class TestDomainApplication(TestCase): self.ineligible_application = completed_application( status=DomainApplication.ApplicationStatus.INELIGIBLE, name="ineligible.gov" ) - + # Store all aplpication statuses in a variable for ease of use self.all_applications = [ self.started_application, @@ -63,7 +63,7 @@ class TestDomainApplication(TestCase): self.rejected_application, self.ineligible_application, ] - + self.mock_client = MockSESClient() def tearDown(self): @@ -249,6 +249,19 @@ class TestDomainApplication(TestCase): # Call the method method() + def assert_fsm_transition_does_not_raise_error(self, test_cases, method_to_run): + """Given a list of test cases, ensure that none of them throw transition errors""" + with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): + for application, exception_type in test_cases: + with self.subTest(application=application, exception_type=exception_type): + try: + # Retrieve the method by name from the application object and call it + method = getattr(application, method_to_run) + # Call the method + method() + except exception_type: + self.fail(f"{exception_type} was raised, but it was not expected.") + def test_submit_transition_allowed_with_no_investigator(self): """ Tests for attempting to transition without an investigator. @@ -263,13 +276,7 @@ class TestDomainApplication(TestCase): # Set all investigators to none set_applications_investigators(self.all_applications, None) - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.submit() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "submit") def test_submit_transition_allowed_with_investigator_not_staff(self): """ @@ -286,13 +293,7 @@ class TestDomainApplication(TestCase): user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) set_applications_investigators(self.all_applications, user) - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.submit() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "submit") def test_submit_transition_allowed(self): """ @@ -305,14 +306,7 @@ class TestDomainApplication(TestCase): (self.withdrawn_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.submit() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "submit") def test_submit_transition_allowed_twice(self): """ @@ -331,7 +325,7 @@ class TestDomainApplication(TestCase): self.in_review_application.submit() except TransitionNotAllowed: self.fail("TransitionNotAllowed was raised, but it was not expected.") - + self.assertEqual(self.in_review_application.status, DomainApplication.ApplicationStatus.SUBMITTED) def test_submit_transition_not_allowed(self): @@ -359,14 +353,7 @@ class TestDomainApplication(TestCase): (self.ineligible_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.in_review() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "in_review") def test_in_review_transition_not_allowed_with_no_investigator(self): """ @@ -384,7 +371,7 @@ class TestDomainApplication(TestCase): set_applications_investigators(self.all_applications, None) self.assert_fsm_transition_raises_error(test_cases, "in_review") - + def test_in_review_transition_not_allowed_with_investigator_not_staff(self): """ Tests for attempting to transition with an investigator that is not staff. @@ -426,13 +413,8 @@ class TestDomainApplication(TestCase): (self.rejected_application, TransitionNotAllowed), (self.ineligible_application, TransitionNotAllowed), ] - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.action_needed() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + + self.assert_fsm_transition_does_not_raise_error(test_cases, "action_needed") def test_action_needed_transition_not_allowed_with_no_investigator(self): """ @@ -493,14 +475,7 @@ class TestDomainApplication(TestCase): (self.rejected_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.approve() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "approve") def test_approved_transition_not_allowed_with_no_investigator(self): """ @@ -570,14 +545,7 @@ class TestDomainApplication(TestCase): (self.action_needed_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.withdraw() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "withdraw") def test_withdraw_transition_allowed_with_no_investigator(self): """ @@ -594,13 +562,7 @@ class TestDomainApplication(TestCase): # Set all investigators to none set_applications_investigators(self.all_applications, None) - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.withdraw() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "withdraw") def test_withdraw_transition_allowed_with_investigator_not_staff(self): """ @@ -618,13 +580,7 @@ class TestDomainApplication(TestCase): user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) set_applications_investigators(self.all_applications, user) - with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.withdraw() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "withdraw") def test_withdraw_transition_not_allowed(self): """ @@ -650,14 +606,7 @@ class TestDomainApplication(TestCase): (self.approved_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.reject() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "reject") def test_reject_transition_not_allowed_with_no_investigator(self): """ @@ -717,14 +666,7 @@ class TestDomainApplication(TestCase): (self.rejected_application, TransitionNotAllowed), ] - with boto3_mocking.clients.handler_for("sesv2", self.mock_client): - with less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): - try: - application.reject_with_prejudice() - except TransitionNotAllowed: - self.fail("TransitionNotAllowed was raised, but it was not expected.") + self.assert_fsm_transition_does_not_raise_error(test_cases, "reject_with_prejudice") def test_reject_with_prejudice_transition_not_allowed_with_no_investigator(self): """ From 92c85dbb16b621fd20010e8ab8c4daee87970f31 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Mon, 26 Feb 2024 15:10:03 -0700 Subject: [PATCH 22/43] Linting --- src/registrar/tests/test_models.py | 11 ++--------- src/registrar/utility/errors.py | 4 +--- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 8e9b9dc4e..73d86dfd7 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -52,7 +52,7 @@ class TestDomainApplication(TestCase): status=DomainApplication.ApplicationStatus.INELIGIBLE, name="ineligible.gov" ) - # Store all aplpication statuses in a variable for ease of use + # Store all application statuses in a variable for ease of use self.all_applications = [ self.started_application, self.submitted_application, @@ -181,14 +181,7 @@ class TestDomainApplication(TestCase): with less_console_noise(): # Perform the specified action action_method = getattr(application, action) - if action == "approve" and not application.investigator: - user, _ = User.objects.get_or_create(username="testwafflesyrup", is_staff=True) - application.investigator = user - application.save() - application.refresh_from_db() - action_method() - else: - action_method() + action_method() # Check if an email was sent sent_emails = [ diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index f399299ff..10e1809aa 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -100,9 +100,7 @@ class ApplicationStatusError(Exception): FSMErrorCodes.APPROVE_DOMAIN_IN_USE: ("Cannot approve. Requested domain is already in use."), FSMErrorCodes.NO_INVESTIGATOR: ("No investigator was assigned."), FSMErrorCodes.INVESTIGATOR_NOT_STAFF: ("Investigator is not a staff user."), - FSMErrorCodes.INVESTIGATOR_NOT_SUBMITTER: ( - "Only the assigned investigator can make this change." - ), + FSMErrorCodes.INVESTIGATOR_NOT_SUBMITTER: ("Only the assigned investigator can make this change."), } def __init__(self, *args, code=None, **kwargs): From a81d4d18458fb926450e9d24184bf773e78b914a Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 08:21:28 -0700 Subject: [PATCH 23/43] Update admin.py --- src/registrar/admin.py | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 176c0bb87..5ad274ab1 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -794,7 +794,6 @@ class DomainApplicationAdminForm(forms.ModelForm): fields = "__all__" def __init__(self, *args, **kwargs): - self.request = kwargs.pop("request", None) super().__init__(*args, **kwargs) application = kwargs.get("instance") if application and application.pk: @@ -833,6 +832,10 @@ class DomainApplicationAdminForm(forms.ModelForm): yield meta.get_transition(curr_state) def clean(self): + """ + Override of the default clean on the form. + This is so we can inject custom form-level error messages. + """ # clean is called from clean_forms, which is called from is_valid # after clean_fields. it is used to determine form level errors. # is_valid is typically called from view during a post @@ -1028,23 +1031,6 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Table ordering ordering = ["requested_domain__name"] - def get_form(self, request, obj=None, **kwargs): - """ - Workaround to pass the request context to the underlying DomainApplicationAdminForm form object. - This is so we can do things like check the current user against a form value at submission. - """ - # Call the superclass's get_form method to get the form class - da_form = super().get_form(request, obj, **kwargs) - - # Define a wrapper class for the form that includes the request in its initialization. - # This is needed such that we can inject request without otherwise altering it. - class DomainApplicationFormWrapper(da_form): - def __new__(cls, *args, **form_kwargs): - form_kwargs["request"] = request - return da_form(*args, **form_kwargs) - - return DomainApplicationFormWrapper - # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): # TODO - there is an existing bug in these in that they redirect From 2ef573ddfaf01f5aa1ded9b394be3cf977009244 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 08:49:22 -0700 Subject: [PATCH 24/43] Fix indent --- src/registrar/tests/test_models_domain.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 626cdae8f..e50cc357b 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -329,16 +329,16 @@ class TestDomainCreation(MockEppLib): creator=user, requested_domain=draft_domain, investigator=investigator ) - mock_client = MockSESClient() - with boto3_mocking.clients.handler_for("sesv2", mock_client): - # skip using the submit method - application.status = DomainApplication.ApplicationStatus.SUBMITTED - # transition to approve state - application.approve() - # should have information present for this domain - domain = Domain.objects.get(name="igorville.gov") - self.assertTrue(domain) - self.mockedSendFunction.assert_not_called() + mock_client = MockSESClient() + with boto3_mocking.clients.handler_for("sesv2", mock_client): + # skip using the submit method + application.status = DomainApplication.ApplicationStatus.SUBMITTED + # transition to approve state + application.approve() + # should have information present for this domain + domain = Domain.objects.get(name="igorville.gov") + self.assertTrue(domain) + self.mockedSendFunction.assert_not_called() def test_accessing_domain_properties_creates_domain_in_registry(self): """ From 6b5d6009d196a54cd58014699b23f1e94e8f1ace Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 09:36:06 -0700 Subject: [PATCH 25/43] Add some none returns - needs cleaning --- src/registrar/admin.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6a7198556..50ee54547 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1110,6 +1110,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): request, "This action is not permitted. The domain is already active.", ) + return None elif ( obj and obj.status == models.DomainApplication.ApplicationStatus.REJECTED @@ -1126,7 +1127,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): request, "A rejection reason is required.", ) - + return None else: if obj.status != original_obj.status: status_method_mapping = { @@ -1162,6 +1163,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): request, err.message, ) + return None super().save_model(request, obj, form, change) From db8c363d43b6df4b107abcb6132690e918fef3a2 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 11:34:05 -0700 Subject: [PATCH 26/43] Save_model refactor --- src/registrar/admin.py | 194 +++++++++++++++++++++++++---------------- 1 file changed, 121 insertions(+), 73 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 50ee54547..68103968f 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1070,8 +1070,20 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): - # TODO - there is an existing bug in these in that they redirect - # to the table rather than back, on message display + """Custom save_model definition that handles edge cases""" + + # == Check that the obj is in a valid state == # + + # If obj is none, something went very wrong. + # The form should have blocked this, so lets forbid it. + if not obj: + logger.error(f"Invalid value for obj ({obj})") + messages.set_level(request, messages.ERROR) + messages.error( + request, + "Could not save DomainApplication. Something went wrong.", + ) + return None # If the user is restricted or we're saving an invalid model, # forbid this action. @@ -1086,86 +1098,122 @@ class DomainApplicationAdmin(ListHeaderAdmin): return None - if change: - # Get the original application from the database - original_obj = models.DomainApplication.objects.get(pk=obj.pk) + # == Check if we're making a change or not == # - if ( - obj - and original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED - and obj.status != models.DomainApplication.ApplicationStatus.APPROVED - and not obj.domain_is_not_active() - ): - # If an admin tried to set an approved application to - # another status and the related domain is already - # active, shortcut the action and throw a friendly - # error message. This action would still not go through - # shortcut or not as the rules are duplicated on the model, - # but the error would be an ugly Django error screen. + # If we're not making a change (adding a record), run save model as we do normally + if not change: + return super().save_model(request, obj, form, change) - # Clear the success message - messages.set_level(request, messages.ERROR) + # == Handle non-status changes == # + + # Get the original application from the database. + original_obj = models.DomainApplication.objects.get(pk=obj.pk) + if obj.status == original_obj.status: + # If the status hasn't changed, let the base function take care of it + return super().save_model(request, obj, form, change) - messages.error( - request, - "This action is not permitted. The domain is already active.", + # == Handle status changes == # + + # Run some checks on the current object for invalid status changes + obj, should_save = self._handle_status_change(request, obj, original_obj) + + # We should only save if we don't display any errors in the step above. + if should_save: + return super().save_model(request, obj, form, change) + + def _handle_status_change(self, request, obj, original_obj): + """ + Checks for various conditions when a status change is triggered. + In the event that it is valid, the status will be mapped to + the appropriate method. + + In the event that we should not status change, an error message + will be displayed. + + Returns a tuple: (obj: DomainApplication, should_proceed: bool) + """ + + should_proceed = True + error_message = None + + # Get the method that should be run given the status + selected_method = self.get_status_method_mapping(obj) + if selected_method is None: + logger.warning("Unknown status selected in django admin") + + # If the status is not mapped properly, saving could cause + # weird issues down the line. Instead, we should block this. + should_proceed = False + return should_proceed + + original_is_approved_and_current_is_not = ( + original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED, + obj.status != models.DomainApplication.ApplicationStatus.APPROVED + ) + if (original_is_approved_and_current_is_not and not obj.domain_is_not_active()): + # If an admin tried to set an approved application to + # another status and the related domain is already + # active, shortcut the action and throw a friendly + # error message. This action would still not go through + # shortcut or not as the rules are duplicated on the model, + # but the error would be an ugly Django error screen. + error_message = "This action is not permitted. The domain is already active." + elif ( + obj.status == models.DomainApplication.ApplicationStatus.REJECTED + and not obj.rejection_reason + ): + # This condition should never be triggered. + # The opposite of this condition is acceptable (rejected -> other status and rejection_reason) + # because we clean up the rejection reason in the transition in the model. + error_message = "A rejection reason is required." + else: + # This is an fsm in model which will throw an error if the + # transition condition is violated, so we roll back the + # status to what it was before the admin user changed it and + # let the fsm method set it. + obj.status = original_obj.status + + # Try to perform the status change. + # Catch ApplicationStatusError's and return the message, + # as these are typically user errors. + try: + selected_method() + except ApplicationStatusError as err: + logger.warning( + f"User error encountered when trying to change status: {err}" ) - return None - elif ( - obj - and obj.status == models.DomainApplication.ApplicationStatus.REJECTED - and not obj.rejection_reason - ): - # This condition should never be triggered. - # The opposite of this condition is acceptable (rejected -> other status and rejection_reason) - # because we clean up the rejection reason in the transition in the model. + error_message = err.message - # Clear the success message - messages.set_level(request, messages.ERROR) + if error_message is not None: + # Clear the success message + messages.set_level(request, messages.ERROR) + # Display the error + messages.error( + request, + error_message, + ) - messages.error( - request, - "A rejection reason is required.", - ) - return None - else: - if obj.status != original_obj.status: - status_method_mapping = { - models.DomainApplication.ApplicationStatus.STARTED: None, - models.DomainApplication.ApplicationStatus.SUBMITTED: obj.submit, - models.DomainApplication.ApplicationStatus.IN_REVIEW: obj.in_review, - models.DomainApplication.ApplicationStatus.ACTION_NEEDED: obj.action_needed, - models.DomainApplication.ApplicationStatus.APPROVED: obj.approve, - models.DomainApplication.ApplicationStatus.WITHDRAWN: obj.withdraw, - models.DomainApplication.ApplicationStatus.REJECTED: obj.reject, - models.DomainApplication.ApplicationStatus.INELIGIBLE: (obj.reject_with_prejudice), - } - selected_method = status_method_mapping.get(obj.status) - if selected_method is None: - logger.warning("Unknown status selected in django admin") - else: - # This is an fsm in model which will throw an error if the - # transition condition is violated, so we roll back the - # status to what it was before the admin user changed it and - # let the fsm method set it. - obj.status = original_obj.status + # If an error message exists, we shouldn't proceed + should_proceed = False - # Try to perform the status change. - # Catch ApplicationStatusError's and return the message, - # as these are typically user errors. - try: - selected_method() - except ApplicationStatusError as err: - # Clear the success message, if any - messages.set_level(request, messages.ERROR) + return (obj, should_proceed) - messages.error( - request, - err.message, - ) - return None + def get_status_method_mapping(self, application): + """Returns what method should be ran given an application object""" + # Define a per-object mapping + status_method_mapping = { + models.DomainApplication.ApplicationStatus.STARTED: None, + models.DomainApplication.ApplicationStatus.SUBMITTED: application.submit, + models.DomainApplication.ApplicationStatus.IN_REVIEW: application.in_review, + models.DomainApplication.ApplicationStatus.ACTION_NEEDED: application.action_needed, + models.DomainApplication.ApplicationStatus.APPROVED: application.approve, + models.DomainApplication.ApplicationStatus.WITHDRAWN: application.withdraw, + models.DomainApplication.ApplicationStatus.REJECTED: application.reject, + models.DomainApplication.ApplicationStatus.INELIGIBLE: (application.reject_with_prejudice), + } - super().save_model(request, obj, form, change) + # Grab the method + return status_method_mapping.get(application.status, None) def get_readonly_fields(self, request, obj=None): """Set the read-only state on form elements. From 3268fa44db4badac0db111fc640ae0c0d87de3b9 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 11:35:52 -0700 Subject: [PATCH 27/43] Linting --- src/registrar/admin.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 68103968f..df39a36cc 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1071,8 +1071,8 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Trigger action when a fieldset is changed def save_model(self, request, obj, form, change): """Custom save_model definition that handles edge cases""" - - # == Check that the obj is in a valid state == # + + # == Check that the obj is in a valid state == # # If obj is none, something went very wrong. # The form should have blocked this, so lets forbid it. @@ -1105,7 +1105,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): return super().save_model(request, obj, form, change) # == Handle non-status changes == # - + # Get the original application from the database. original_obj = models.DomainApplication.objects.get(pk=obj.pk) if obj.status == original_obj.status: @@ -1124,7 +1124,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): def _handle_status_change(self, request, obj, original_obj): """ Checks for various conditions when a status change is triggered. - In the event that it is valid, the status will be mapped to + In the event that it is valid, the status will be mapped to the appropriate method. In the event that we should not status change, an error message @@ -1148,9 +1148,9 @@ class DomainApplicationAdmin(ListHeaderAdmin): original_is_approved_and_current_is_not = ( original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED, - obj.status != models.DomainApplication.ApplicationStatus.APPROVED + obj.status != models.DomainApplication.ApplicationStatus.APPROVED, ) - if (original_is_approved_and_current_is_not and not obj.domain_is_not_active()): + if original_is_approved_and_current_is_not and not obj.domain_is_not_active(): # If an admin tried to set an approved application to # another status and the related domain is already # active, shortcut the action and throw a friendly @@ -1158,10 +1158,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): # shortcut or not as the rules are duplicated on the model, # but the error would be an ugly Django error screen. error_message = "This action is not permitted. The domain is already active." - elif ( - obj.status == models.DomainApplication.ApplicationStatus.REJECTED - and not obj.rejection_reason - ): + elif obj.status == models.DomainApplication.ApplicationStatus.REJECTED and not obj.rejection_reason: # This condition should never be triggered. # The opposite of this condition is acceptable (rejected -> other status and rejection_reason) # because we clean up the rejection reason in the transition in the model. @@ -1179,9 +1176,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): try: selected_method() except ApplicationStatusError as err: - logger.warning( - f"User error encountered when trying to change status: {err}" - ) + logger.warning(f"An error encountered when trying to change status: {err}") error_message = err.message if error_message is not None: From 7d8cd5498bef44fb2985ffe587d3da7d1fe13b2e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 12:55:16 -0700 Subject: [PATCH 28/43] Merge conflict --- src/registrar/admin.py | 180 +++++++++++++++++------------------------ 1 file changed, 75 insertions(+), 105 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index aaa3b1f03..68e9bc4bb 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -93,9 +93,14 @@ class DomainApplicationAdminForm(forms.ModelForm): # first option in status transitions is current state available_transitions = [(current_state, application.get_status_display())] - transitions = get_available_FIELD_transitions( - application, models.DomainApplication._meta.get_field("status") - ) + if application.investigator is not None: + transitions = get_available_FIELD_transitions( + application, models.DomainApplication._meta.get_field("status") + ) + else: + transitions = self.get_custom_field_transitions( + application, models.DomainApplication._meta.get_field("status") + ) for transition in transitions: available_transitions.append((transition.target, transition.target.label)) @@ -106,6 +111,73 @@ class DomainApplicationAdminForm(forms.ModelForm): if not application.creator.is_restricted(): self.fields["status"].widget.choices = available_transitions + def get_custom_field_transitions(self, instance, field): + """Custom implementation of get_available_FIELD_transitions + in the FSM. Allows us to still display fields filtered out by a condition.""" + curr_state = field.get_state(instance) + transitions = field.transitions[instance.__class__] + + for name, transition in transitions.items(): + meta = transition._django_fsm + if meta.has_transition(curr_state): + yield meta.get_transition(curr_state) + + def clean(self): + """ + Override of the default clean on the form. + This is so we can inject custom form-level error messages. + """ + # clean is called from clean_forms, which is called from is_valid + # after clean_fields. it is used to determine form level errors. + # is_valid is typically called from view during a post + cleaned_data = super().clean() + status = cleaned_data.get("status") + investigator = cleaned_data.get("investigator") + + # Get the old status + initial_status = self.initial.get("status", None) + + # We only care about investigator when in these statuses + checked_statuses = [ + DomainApplication.ApplicationStatus.APPROVED, + DomainApplication.ApplicationStatus.IN_REVIEW, + DomainApplication.ApplicationStatus.ACTION_NEEDED, + DomainApplication.ApplicationStatus.REJECTED, + DomainApplication.ApplicationStatus.INELIGIBLE, + ] + + # If a status change occured, check for validity + if status != initial_status and status in checked_statuses: + # Checks the "investigators" field for validity. + # That field must obey certain conditions when an application is approved. + # Will call "add_error" if any issues are found. + self._check_for_valid_investigator(investigator) + + return cleaned_data + + def _check_for_valid_investigator(self, investigator) -> bool: + """ + Checks if the investigator field is not none, and is staff. + Adds form errors on failure. + """ + + is_valid = False + + # Check if an investigator is assigned. No approval is possible without one. + error_message = None + if investigator is None: + # Lets grab the error message from a common location + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.NO_INVESTIGATOR) + elif not investigator.is_staff: + error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.INVESTIGATOR_NOT_STAFF) + else: + is_valid = True + + if error_message is not None: + self.add_error("investigator", error_message) + + return is_valid + # Based off of this excellent example: https://djangosnippets.org/snippets/10471/ class MultiFieldSortableChangeList(admin.views.main.ChangeList): @@ -867,108 +939,6 @@ class DomainInformationAdmin(ListHeaderAdmin): return readonly_fields # Read-only fields for analysts -class DomainApplicationAdminForm(forms.ModelForm): - """Custom form to limit transitions to available transitions""" - - class Meta: - model = models.DomainApplication - fields = "__all__" - - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) - application = kwargs.get("instance") - if application and application.pk: - current_state = application.status - - # first option in status transitions is current state - available_transitions = [(current_state, application.get_status_display())] - - if application.investigator is not None: - transitions = get_available_FIELD_transitions( - application, models.DomainApplication._meta.get_field("status") - ) - else: - transitions = self.get_custom_field_transitions( - application, models.DomainApplication._meta.get_field("status") - ) - - for transition in transitions: - available_transitions.append((transition.target, transition.target.label)) - - # only set the available transitions if the user is not restricted - # from editing the domain application; otherwise, the form will be - # readonly and the status field will not have a widget - if not application.creator.is_restricted(): - self.fields["status"].widget.choices = available_transitions - - def get_custom_field_transitions(self, instance, field): - """Custom implementation of get_available_FIELD_transitions - in the FSM. Allows us to still display fields filtered out by a condition.""" - curr_state = field.get_state(instance) - transitions = field.transitions[instance.__class__] - - for name, transition in transitions.items(): - meta = transition._django_fsm - if meta.has_transition(curr_state): - yield meta.get_transition(curr_state) - - def clean(self): - """ - Override of the default clean on the form. - This is so we can inject custom form-level error messages. - """ - # clean is called from clean_forms, which is called from is_valid - # after clean_fields. it is used to determine form level errors. - # is_valid is typically called from view during a post - cleaned_data = super().clean() - status = cleaned_data.get("status") - investigator = cleaned_data.get("investigator") - - # Get the old status - initial_status = self.initial.get("status", None) - - # We only care about investigator when in these statuses - checked_statuses = [ - DomainApplication.ApplicationStatus.APPROVED, - DomainApplication.ApplicationStatus.IN_REVIEW, - DomainApplication.ApplicationStatus.ACTION_NEEDED, - DomainApplication.ApplicationStatus.REJECTED, - DomainApplication.ApplicationStatus.INELIGIBLE, - ] - - # If a status change occured, check for validity - if status != initial_status and status in checked_statuses: - # Checks the "investigators" field for validity. - # That field must obey certain conditions when an application is approved. - # Will call "add_error" if any issues are found. - self._check_for_valid_investigator(investigator) - - return cleaned_data - - def _check_for_valid_investigator(self, investigator) -> bool: - """ - Checks if the investigator field is not none, and is staff. - Adds form errors on failure. - """ - - is_valid = False - - # Check if an investigator is assigned. No approval is possible without one. - error_message = None - if investigator is None: - # Lets grab the error message from a common location - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.NO_INVESTIGATOR) - elif not investigator.is_staff: - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.INVESTIGATOR_NOT_STAFF) - else: - is_valid = True - - if error_message is not None: - self.add_error("investigator", error_message) - - return is_valid - - class DomainApplicationAdmin(ListHeaderAdmin): """Custom domain applications admin class.""" From a4f51c6477bdfa9bf3bbd59d94dbe8d418dbde0f Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 14:34:35 -0700 Subject: [PATCH 29/43] Update admin.py --- src/registrar/admin.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 68e9bc4bb..c440d0ea3 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1203,11 +1203,10 @@ class DomainApplicationAdmin(ListHeaderAdmin): should_proceed = False return should_proceed - original_is_approved_and_current_is_not = ( - original_obj.status == models.DomainApplication.ApplicationStatus.APPROVED, - obj.status != models.DomainApplication.ApplicationStatus.APPROVED, + application_is_not_approved = ( + obj.status != models.DomainApplication.ApplicationStatus.APPROVED ) - if original_is_approved_and_current_is_not and not obj.domain_is_not_active(): + if application_is_not_approved and not obj.domain_is_not_active(): # If an admin tried to set an approved application to # another status and the related domain is already # active, shortcut the action and throw a friendly From 820d6f3f58dfc492b5e84157ff99e14d8bc16cf1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 27 Feb 2024 14:38:12 -0700 Subject: [PATCH 30/43] Linting, once more --- src/registrar/admin.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index c440d0ea3..598b612b5 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -1203,9 +1203,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): should_proceed = False return should_proceed - application_is_not_approved = ( - obj.status != models.DomainApplication.ApplicationStatus.APPROVED - ) + application_is_not_approved = obj.status != models.DomainApplication.ApplicationStatus.APPROVED if application_is_not_approved and not obj.domain_is_not_active(): # If an admin tried to set an approved application to # another status and the related domain is already From 2ba62bf1741646d4ac5383dca36f1a79c805dc3d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 28 Feb 2024 10:00:13 -0700 Subject: [PATCH 31/43] Update name --- src/registrar/admin.py | 10 +++++----- src/registrar/models/domain_application.py | 4 ++-- src/registrar/utility/errors.py | 6 +----- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 598b612b5..6dc6cd94d 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -16,7 +16,7 @@ from dateutil.relativedelta import relativedelta # type: ignore from epplibwrapper.errors import ErrorCode, RegistryError from registrar.models import Contact, Domain, DomainApplication, DraftDomain, User, Website from registrar.utility import csv_export -from registrar.utility.errors import ApplicationStatusError, FSMErrorCodes +from registrar.utility.errors import FSMApplicationError, FSMErrorCodes from registrar.views.utility.mixins import OrderableFieldsMixin from django.contrib.admin.views.main import ORDER_VAR from registrar.widgets import NoAutocompleteFilteredSelectMultiple @@ -167,9 +167,9 @@ class DomainApplicationAdminForm(forms.ModelForm): error_message = None if investigator is None: # Lets grab the error message from a common location - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.NO_INVESTIGATOR) + error_message = FSMApplicationError.get_error_message(FSMErrorCodes.NO_INVESTIGATOR) elif not investigator.is_staff: - error_message = ApplicationStatusError.get_error_message(FSMErrorCodes.INVESTIGATOR_NOT_STAFF) + error_message = FSMApplicationError.get_error_message(FSMErrorCodes.INVESTIGATOR_NOT_STAFF) else: is_valid = True @@ -1225,11 +1225,11 @@ class DomainApplicationAdmin(ListHeaderAdmin): obj.status = original_obj.status # Try to perform the status change. - # Catch ApplicationStatusError's and return the message, + # Catch FSMApplicationError's and return the message, # as these are typically user errors. try: selected_method() - except ApplicationStatusError as err: + except FSMApplicationError as err: logger.warning(f"An error encountered when trying to change status: {err}") error_message = err.message diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index 29c4d63a5..6076497ad 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -9,7 +9,7 @@ from django.db import models from django_fsm import FSMField, transition # type: ignore from django.utils import timezone from registrar.models.domain import Domain -from registrar.utility.errors import ApplicationStatusError, FSMErrorCodes +from registrar.utility.errors import FSMApplicationError, FSMErrorCodes from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError @@ -791,7 +791,7 @@ class DomainApplication(TimeStampedModel): # == Check that the application is valid == # if Domain.objects.filter(name=self.requested_domain.name).exists(): - raise ApplicationStatusError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) + raise FSMApplicationError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) # == Create the domain and related components == # created_domain = Domain.objects.create(name=self.requested_domain.name) diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index 10e1809aa..f5804ac2f 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -71,8 +71,6 @@ class GenericError(Exception): return self._error_mapping.get(code) -# (Q for reviewers) What should this be called? -# Not a fan of this name. class FSMErrorCodes(IntEnum): """Used when doing FSM transitions. Overview of generic error codes: @@ -88,9 +86,7 @@ class FSMErrorCodes(IntEnum): INVESTIGATOR_NOT_SUBMITTER = 4 -# (Q for reviewers) What should this be called? -# Not a fan of this name. -class ApplicationStatusError(Exception): +class FSMApplicationError(Exception): """ Used to raise exceptions when doing FSM Transitions. Uses `FSMErrorCodes` as an enum. From a0fba816c237d25224735ec89cd0763b5c07a6b7 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 1 Mar 2024 13:31:01 -0700 Subject: [PATCH 32/43] PR suggestions --- src/registrar/assets/sass/_theme/_admin.scss | 5 ----- src/registrar/utility/errors.py | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/registrar/assets/sass/_theme/_admin.scss b/src/registrar/assets/sass/_theme/_admin.scss index b57c6a015..7f332efd2 100644 --- a/src/registrar/assets/sass/_theme/_admin.scss +++ b/src/registrar/assets/sass/_theme/_admin.scss @@ -275,11 +275,6 @@ h1, h2, h3, } } -// Hides the "clear" button on autocomplete, as we already have one to use -.select2-selection__clear { - display: none; -} - // Fixes a display issue where the list was entirely white, or had too much whitespace .select2-dropdown { display: inline-grid !important; diff --git a/src/registrar/utility/errors.py b/src/registrar/utility/errors.py index f5804ac2f..00c65ce57 100644 --- a/src/registrar/utility/errors.py +++ b/src/registrar/utility/errors.py @@ -94,7 +94,7 @@ class FSMApplicationError(Exception): _error_mapping = { FSMErrorCodes.APPROVE_DOMAIN_IN_USE: ("Cannot approve. Requested domain is already in use."), - FSMErrorCodes.NO_INVESTIGATOR: ("No investigator was assigned."), + FSMErrorCodes.NO_INVESTIGATOR: ("Investigator is required for this status."), FSMErrorCodes.INVESTIGATOR_NOT_STAFF: ("Investigator is not a staff user."), FSMErrorCodes.INVESTIGATOR_NOT_SUBMITTER: ("Only the assigned investigator can make this change."), } From f042bb72f55b99204011cf5ac5a2c4028ac1414e Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Fri, 1 Mar 2024 15:01:21 -0700 Subject: [PATCH 33/43] Remove check for only status changes --- src/registrar/admin.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 6dc6cd94d..0d9045920 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -134,9 +134,6 @@ class DomainApplicationAdminForm(forms.ModelForm): status = cleaned_data.get("status") investigator = cleaned_data.get("investigator") - # Get the old status - initial_status = self.initial.get("status", None) - # We only care about investigator when in these statuses checked_statuses = [ DomainApplication.ApplicationStatus.APPROVED, @@ -147,7 +144,7 @@ class DomainApplicationAdminForm(forms.ModelForm): ] # If a status change occured, check for validity - if status != initial_status and status in checked_statuses: + if status in checked_statuses: # Checks the "investigators" field for validity. # That field must obey certain conditions when an application is approved. # Will call "add_error" if any issues are found. From 9dec0753f28cfd40b3df60214edd0d645fb8d50c Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Thu, 7 Mar 2024 10:42:30 -0700 Subject: [PATCH 34/43] Revert change where only status change is forbidden --- src/registrar/admin.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 0d9045920..6dc6cd94d 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -134,6 +134,9 @@ class DomainApplicationAdminForm(forms.ModelForm): status = cleaned_data.get("status") investigator = cleaned_data.get("investigator") + # Get the old status + initial_status = self.initial.get("status", None) + # We only care about investigator when in these statuses checked_statuses = [ DomainApplication.ApplicationStatus.APPROVED, @@ -144,7 +147,7 @@ class DomainApplicationAdminForm(forms.ModelForm): ] # If a status change occured, check for validity - if status in checked_statuses: + if status != initial_status and status in checked_statuses: # Checks the "investigators" field for validity. # That field must obey certain conditions when an application is approved. # Will call "add_error" if any issues are found. From d83bd361cba26b8ddb81bd08c29b4b960005f4cd Mon Sep 17 00:00:00 2001 From: CocoByte Date: Mon, 11 Mar 2024 13:16:27 -0600 Subject: [PATCH 35/43] Fix migrations (rename domainApplication model instead of delete/create) --- .../migrations/0073_domainrequest_and_more.py | 685 ------------------ .../0073_rename_domainapplication.py | 16 + ...information_domain_application_and_more.py | 114 +++ ...roups_v08.py => 0075_create_groups_v08.py} | 2 +- 4 files changed, 131 insertions(+), 686 deletions(-) delete mode 100644 src/registrar/migrations/0073_domainrequest_and_more.py create mode 100644 src/registrar/migrations/0073_rename_domainapplication.py create mode 100644 src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py rename src/registrar/migrations/{0074_create_groups_v08.py => 0075_create_groups_v08.py} (93%) diff --git a/src/registrar/migrations/0073_domainrequest_and_more.py b/src/registrar/migrations/0073_domainrequest_and_more.py deleted file mode 100644 index 88608b03a..000000000 --- a/src/registrar/migrations/0073_domainrequest_and_more.py +++ /dev/null @@ -1,685 +0,0 @@ -# Generated by Django 4.2.10 on 2024-03-07 21:52 - -from django.conf import settings -from django.db import migrations, models -import django.db.models.deletion -import django_fsm - - -class Migration(migrations.Migration): - - dependencies = [ - ("registrar", "0072_alter_publiccontact_fax_alter_publiccontact_voice"), - ] - - operations = [ - migrations.CreateModel( - name="DomainRequest", - fields=[ - ("id", models.BigAutoField(auto_created=True, primary_key=True, serialize=False, verbose_name="ID")), - ("created_at", models.DateTimeField(auto_now_add=True)), - ("updated_at", models.DateTimeField(auto_now=True)), - ( - "status", - django_fsm.FSMField( - choices=[ - ("started", "Started"), - ("submitted", "Submitted"), - ("in review", "In review"), - ("action needed", "Action needed"), - ("approved", "Approved"), - ("withdrawn", "Withdrawn"), - ("rejected", "Rejected"), - ("ineligible", "Ineligible"), - ], - default="started", - max_length=50, - ), - ), - ( - "rejection_reason", - models.TextField( - blank=True, - choices=[ - ("purpose_not_met", "Purpose requirements not met"), - ("requestor_not_eligible", "Requestor not eligible to make request"), - ("org_has_domain", "Org already has a .gov domain"), - ("contacts_not_verified", "Org contacts couldn't be verified"), - ("org_not_eligible", "Org not eligible for a .gov domain"), - ("naming_not_met", "Naming requirements not met"), - ("other", "Other/Unspecified"), - ], - null=True, - ), - ), - ( - "organization_type", - models.CharField( - blank=True, - choices=[ - ("federal", "Federal"), - ("interstate", "Interstate"), - ("state_or_territory", "State or territory"), - ("tribal", "Tribal"), - ("county", "County"), - ("city", "City"), - ("special_district", "Special district"), - ("school_district", "School district"), - ], - help_text="Type of organization", - max_length=255, - null=True, - ), - ), - ( - "federally_recognized_tribe", - models.BooleanField(help_text="Is the tribe federally recognized", null=True), - ), - ( - "state_recognized_tribe", - models.BooleanField(help_text="Is the tribe recognized by a state", null=True), - ), - ("tribe_name", models.CharField(blank=True, help_text="Name of tribe", null=True)), - ( - "federal_agency", - models.CharField( - blank=True, - choices=[ - ( - "Administrative Conference of the United States", - "Administrative Conference of the United States", - ), - ("Advisory Council on Historic Preservation", "Advisory Council on Historic Preservation"), - ("American Battle Monuments Commission", "American Battle Monuments Commission"), - ("AMTRAK", "AMTRAK"), - ("Appalachian Regional Commission", "Appalachian Regional Commission"), - ( - "Appraisal Subcommittee of the Federal Financial Institutions Examination Council", - "Appraisal Subcommittee of the Federal Financial Institutions Examination Council", - ), - ("Appraisal Subcommittee", "Appraisal Subcommittee"), - ("Architect of the Capitol", "Architect of the Capitol"), - ("Armed Forces Retirement Home", "Armed Forces Retirement Home"), - ( - "Barry Goldwater Scholarship and Excellence in Education Foundation", - "Barry Goldwater Scholarship and Excellence in Education Foundation", - ), - ( - "Barry Goldwater Scholarship and Excellence in Education Program", - "Barry Goldwater Scholarship and Excellence in Education Program", - ), - ("Central Intelligence Agency", "Central Intelligence Agency"), - ("Chemical Safety Board", "Chemical Safety Board"), - ( - "Christopher Columbus Fellowship Foundation", - "Christopher Columbus Fellowship Foundation", - ), - ( - "Civil Rights Cold Case Records Review Board", - "Civil Rights Cold Case Records Review Board", - ), - ( - "Commission for the Preservation of America's Heritage Abroad", - "Commission for the Preservation of America's Heritage Abroad", - ), - ("Commission of Fine Arts", "Commission of Fine Arts"), - ( - "Committee for Purchase From People Who Are Blind or Severely Disabled", - "Committee for Purchase From People Who Are Blind or Severely Disabled", - ), - ("Commodity Futures Trading Commission", "Commodity Futures Trading Commission"), - ("Congressional Budget Office", "Congressional Budget Office"), - ("Consumer Financial Protection Bureau", "Consumer Financial Protection Bureau"), - ("Consumer Product Safety Commission", "Consumer Product Safety Commission"), - ( - "Corporation for National & Community Service", - "Corporation for National & Community Service", - ), - ( - "Corporation for National and Community Service", - "Corporation for National and Community Service", - ), - ( - "Council of Inspectors General on Integrity and Efficiency", - "Council of Inspectors General on Integrity and Efficiency", - ), - ("Court Services and Offender Supervision", "Court Services and Offender Supervision"), - ("Cyberspace Solarium Commission", "Cyberspace Solarium Commission"), - ( - "DC Court Services and Offender Supervision Agency", - "DC Court Services and Offender Supervision Agency", - ), - ("DC Pre-trial Services", "DC Pre-trial Services"), - ("Defense Nuclear Facilities Safety Board", "Defense Nuclear Facilities Safety Board"), - ("Delta Regional Authority", "Delta Regional Authority"), - ("Denali Commission", "Denali Commission"), - ("Department of Agriculture", "Department of Agriculture"), - ("Department of Commerce", "Department of Commerce"), - ("Department of Defense", "Department of Defense"), - ("Department of Education", "Department of Education"), - ("Department of Energy", "Department of Energy"), - ("Department of Health and Human Services", "Department of Health and Human Services"), - ("Department of Homeland Security", "Department of Homeland Security"), - ( - "Department of Housing and Urban Development", - "Department of Housing and Urban Development", - ), - ("Department of Justice", "Department of Justice"), - ("Department of Labor", "Department of Labor"), - ("Department of State", "Department of State"), - ("Department of the Interior", "Department of the Interior"), - ("Department of the Treasury", "Department of the Treasury"), - ("Department of Transportation", "Department of Transportation"), - ("Department of Veterans Affairs", "Department of Veterans Affairs"), - ("Director of National Intelligence", "Director of National Intelligence"), - ("Dwight D. Eisenhower Memorial Commission", "Dwight D. Eisenhower Memorial Commission"), - ("Election Assistance Commission", "Election Assistance Commission"), - ("Environmental Protection Agency", "Environmental Protection Agency"), - ("Equal Employment Opportunity Commission", "Equal Employment Opportunity Commission"), - ("Executive Office of the President", "Executive Office of the President"), - ("Export-Import Bank of the United States", "Export-Import Bank of the United States"), - ("Export/Import Bank of the U.S.", "Export/Import Bank of the U.S."), - ("Farm Credit Administration", "Farm Credit Administration"), - ("Farm Credit System Insurance Corporation", "Farm Credit System Insurance Corporation"), - ("Federal Communications Commission", "Federal Communications Commission"), - ("Federal Deposit Insurance Corporation", "Federal Deposit Insurance Corporation"), - ("Federal Election Commission", "Federal Election Commission"), - ("Federal Energy Regulatory Commission", "Federal Energy Regulatory Commission"), - ( - "Federal Financial Institutions Examination Council", - "Federal Financial Institutions Examination Council", - ), - ("Federal Housing Finance Agency", "Federal Housing Finance Agency"), - ("Federal Judiciary", "Federal Judiciary"), - ("Federal Labor Relations Authority", "Federal Labor Relations Authority"), - ("Federal Maritime Commission", "Federal Maritime Commission"), - ( - "Federal Mediation and Conciliation Service", - "Federal Mediation and Conciliation Service", - ), - ( - "Federal Mine Safety and Health Review Commission", - "Federal Mine Safety and Health Review Commission", - ), - ( - "Federal Permitting Improvement Steering Council", - "Federal Permitting Improvement Steering Council", - ), - ("Federal Reserve Board of Governors", "Federal Reserve Board of Governors"), - ("Federal Reserve System", "Federal Reserve System"), - ("Federal Trade Commission", "Federal Trade Commission"), - ("General Services Administration", "General Services Administration"), - ("gov Administration", "gov Administration"), - ("Government Accountability Office", "Government Accountability Office"), - ("Government Publishing Office", "Government Publishing Office"), - ("Gulf Coast Ecosystem Restoration Council", "Gulf Coast Ecosystem Restoration Council"), - ("Harry S Truman Scholarship Foundation", "Harry S Truman Scholarship Foundation"), - ("Harry S. Truman Scholarship Foundation", "Harry S. Truman Scholarship Foundation"), - ("Institute of Museum and Library Services", "Institute of Museum and Library Services"), - ("Institute of Peace", "Institute of Peace"), - ("Inter-American Foundation", "Inter-American Foundation"), - ( - "International Boundary and Water Commission: United States and Mexico", - "International Boundary and Water Commission: United States and Mexico", - ), - ( - "International Boundary Commission: United States and Canada", - "International Boundary Commission: United States and Canada", - ), - ( - "International Joint Commission: United States and Canada", - "International Joint Commission: United States and Canada", - ), - ( - "James Madison Memorial Fellowship Foundation", - "James Madison Memorial Fellowship Foundation", - ), - ("Japan-United States Friendship Commission", "Japan-United States Friendship Commission"), - ("Japan-US Friendship Commission", "Japan-US Friendship Commission"), - ( - "John F. Kennedy Center for Performing Arts", - "John F. Kennedy Center for Performing Arts", - ), - ( - "John F. Kennedy Center for the Performing Arts", - "John F. Kennedy Center for the Performing Arts", - ), - ("Legal Services Corporation", "Legal Services Corporation"), - ("Legislative Branch", "Legislative Branch"), - ("Library of Congress", "Library of Congress"), - ("Marine Mammal Commission", "Marine Mammal Commission"), - ( - "Medicaid and CHIP Payment and Access Commission", - "Medicaid and CHIP Payment and Access Commission", - ), - ("Medical Payment Advisory Commission", "Medical Payment Advisory Commission"), - ("Medicare Payment Advisory Commission", "Medicare Payment Advisory Commission"), - ("Merit Systems Protection Board", "Merit Systems Protection Board"), - ("Millennium Challenge Corporation", "Millennium Challenge Corporation"), - ( - "Morris K. Udall and Stewart L. Udall Foundation", - "Morris K. Udall and Stewart L. Udall Foundation", - ), - ( - "National Aeronautics and Space Administration", - "National Aeronautics and Space Administration", - ), - ( - "National Archives and Records Administration", - "National Archives and Records Administration", - ), - ("National Capital Planning Commission", "National Capital Planning Commission"), - ("National Council on Disability", "National Council on Disability"), - ("National Credit Union Administration", "National Credit Union Administration"), - ("National Endowment for the Arts", "National Endowment for the Arts"), - ("National Endowment for the Humanities", "National Endowment for the Humanities"), - ( - "National Foundation on the Arts and the Humanities", - "National Foundation on the Arts and the Humanities", - ), - ("National Gallery of Art", "National Gallery of Art"), - ("National Indian Gaming Commission", "National Indian Gaming Commission"), - ("National Labor Relations Board", "National Labor Relations Board"), - ("National Mediation Board", "National Mediation Board"), - ("National Science Foundation", "National Science Foundation"), - ( - "National Security Commission on Artificial Intelligence", - "National Security Commission on Artificial Intelligence", - ), - ("National Transportation Safety Board", "National Transportation Safety Board"), - ( - "Networking Information Technology Research and Development", - "Networking Information Technology Research and Development", - ), - ("Non-Federal Agency", "Non-Federal Agency"), - ("Northern Border Regional Commission", "Northern Border Regional Commission"), - ("Nuclear Regulatory Commission", "Nuclear Regulatory Commission"), - ("Nuclear Safety Oversight Committee", "Nuclear Safety Oversight Committee"), - ("Nuclear Waste Technical Review Board", "Nuclear Waste Technical Review Board"), - ( - "Occupational Safety & Health Review Commission", - "Occupational Safety & Health Review Commission", - ), - ( - "Occupational Safety and Health Review Commission", - "Occupational Safety and Health Review Commission", - ), - ("Office of Compliance", "Office of Compliance"), - ("Office of Congressional Workplace Rights", "Office of Congressional Workplace Rights"), - ("Office of Government Ethics", "Office of Government Ethics"), - ( - "Office of Navajo and Hopi Indian Relocation", - "Office of Navajo and Hopi Indian Relocation", - ), - ("Office of Personnel Management", "Office of Personnel Management"), - ("Open World Leadership Center", "Open World Leadership Center"), - ("Overseas Private Investment Corporation", "Overseas Private Investment Corporation"), - ("Peace Corps", "Peace Corps"), - ("Pension Benefit Guaranty Corporation", "Pension Benefit Guaranty Corporation"), - ("Postal Regulatory Commission", "Postal Regulatory Commission"), - ("Presidio Trust", "Presidio Trust"), - ( - "Privacy and Civil Liberties Oversight Board", - "Privacy and Civil Liberties Oversight Board", - ), - ("Public Buildings Reform Board", "Public Buildings Reform Board"), - ( - "Public Defender Service for the District of Columbia", - "Public Defender Service for the District of Columbia", - ), - ("Railroad Retirement Board", "Railroad Retirement Board"), - ("Securities and Exchange Commission", "Securities and Exchange Commission"), - ("Selective Service System", "Selective Service System"), - ("Small Business Administration", "Small Business Administration"), - ("Smithsonian Institution", "Smithsonian Institution"), - ("Social Security Administration", "Social Security Administration"), - ("Social Security Advisory Board", "Social Security Advisory Board"), - ("Southeast Crescent Regional Commission", "Southeast Crescent Regional Commission"), - ("Southwest Border Regional Commission", "Southwest Border Regional Commission"), - ("State Justice Institute", "State Justice Institute"), - ("State, Local, and Tribal Government", "State, Local, and Tribal Government"), - ("Stennis Center for Public Service", "Stennis Center for Public Service"), - ("Surface Transportation Board", "Surface Transportation Board"), - ("Tennessee Valley Authority", "Tennessee Valley Authority"), - ("The Executive Office of the President", "The Executive Office of the President"), - ("The Intelligence Community", "The Intelligence Community"), - ("The Legislative Branch", "The Legislative Branch"), - ("The Supreme Court", "The Supreme Court"), - ( - "The United States World War One Centennial Commission", - "The United States World War One Centennial Commission", - ), - ("U.S. Access Board", "U.S. Access Board"), - ("U.S. Agency for Global Media", "U.S. Agency for Global Media"), - ("U.S. Agency for International Development", "U.S. Agency for International Development"), - ("U.S. Capitol Police", "U.S. Capitol Police"), - ("U.S. Chemical Safety Board", "U.S. Chemical Safety Board"), - ( - "U.S. China Economic and Security Review Commission", - "U.S. China Economic and Security Review Commission", - ), - ( - "U.S. Commission for the Preservation of Americas Heritage Abroad", - "U.S. Commission for the Preservation of Americas Heritage Abroad", - ), - ("U.S. Commission of Fine Arts", "U.S. Commission of Fine Arts"), - ("U.S. Commission on Civil Rights", "U.S. Commission on Civil Rights"), - ( - "U.S. Commission on International Religious Freedom", - "U.S. Commission on International Religious Freedom", - ), - ("U.S. Courts", "U.S. Courts"), - ("U.S. Department of Agriculture", "U.S. Department of Agriculture"), - ("U.S. Interagency Council on Homelessness", "U.S. Interagency Council on Homelessness"), - ("U.S. International Trade Commission", "U.S. International Trade Commission"), - ("U.S. Nuclear Waste Technical Review Board", "U.S. Nuclear Waste Technical Review Board"), - ("U.S. Office of Special Counsel", "U.S. Office of Special Counsel"), - ("U.S. Peace Corps", "U.S. Peace Corps"), - ("U.S. Postal Service", "U.S. Postal Service"), - ("U.S. Semiquincentennial Commission", "U.S. Semiquincentennial Commission"), - ("U.S. Trade and Development Agency", "U.S. Trade and Development Agency"), - ( - "U.S.-China Economic and Security Review Commission", - "U.S.-China Economic and Security Review Commission", - ), - ("Udall Foundation", "Udall Foundation"), - ("United States AbilityOne", "United States AbilityOne"), - ("United States Access Board", "United States Access Board"), - ( - "United States African Development Foundation", - "United States African Development Foundation", - ), - ("United States Agency for Global Media", "United States Agency for Global Media"), - ("United States Arctic Research Commission", "United States Arctic Research Commission"), - ( - "United States Global Change Research Program", - "United States Global Change Research Program", - ), - ("United States Holocaust Memorial Museum", "United States Holocaust Memorial Museum"), - ("United States Institute of Peace", "United States Institute of Peace"), - ( - "United States Interagency Council on Homelessness", - "United States Interagency Council on Homelessness", - ), - ( - "United States International Development Finance Corporation", - "United States International Development Finance Corporation", - ), - ( - "United States International Trade Commission", - "United States International Trade Commission", - ), - ("United States Postal Service", "United States Postal Service"), - ("United States Senate", "United States Senate"), - ( - "United States Trade and Development Agency", - "United States Trade and Development Agency", - ), - ( - "Utah Reclamation Mitigation and Conservation Commission", - "Utah Reclamation Mitigation and Conservation Commission", - ), - ("Vietnam Education Foundation", "Vietnam Education Foundation"), - ("Western Hemisphere Drug Policy Commission", "Western Hemisphere Drug Policy Commission"), - ( - "Woodrow Wilson International Center for Scholars", - "Woodrow Wilson International Center for Scholars", - ), - ("World War I Centennial Commission", "World War I Centennial Commission"), - ], - help_text="Federal agency", - null=True, - ), - ), - ( - "federal_type", - models.CharField( - blank=True, - choices=[("executive", "Executive"), ("judicial", "Judicial"), ("legislative", "Legislative")], - help_text="Federal government branch", - max_length=50, - null=True, - ), - ), - ( - "is_election_board", - models.BooleanField(blank=True, help_text="Is your organization an election office?", null=True), - ), - ( - "organization_name", - models.CharField(blank=True, db_index=True, help_text="Organization name", null=True), - ), - ( - "address_line1", - models.CharField(blank=True, help_text="Street address", null=True, verbose_name="Address line 1"), - ), - ( - "address_line2", - models.CharField( - blank=True, - help_text="Street address line 2 (optional)", - null=True, - verbose_name="Address line 2", - ), - ), - ("city", models.CharField(blank=True, help_text="City", null=True)), - ( - "state_territory", - models.CharField( - blank=True, - choices=[ - ("AL", "Alabama (AL)"), - ("AK", "Alaska (AK)"), - ("AS", "American Samoa (AS)"), - ("AZ", "Arizona (AZ)"), - ("AR", "Arkansas (AR)"), - ("CA", "California (CA)"), - ("CO", "Colorado (CO)"), - ("CT", "Connecticut (CT)"), - ("DE", "Delaware (DE)"), - ("DC", "District of Columbia (DC)"), - ("FL", "Florida (FL)"), - ("GA", "Georgia (GA)"), - ("GU", "Guam (GU)"), - ("HI", "Hawaii (HI)"), - ("ID", "Idaho (ID)"), - ("IL", "Illinois (IL)"), - ("IN", "Indiana (IN)"), - ("IA", "Iowa (IA)"), - ("KS", "Kansas (KS)"), - ("KY", "Kentucky (KY)"), - ("LA", "Louisiana (LA)"), - ("ME", "Maine (ME)"), - ("MD", "Maryland (MD)"), - ("MA", "Massachusetts (MA)"), - ("MI", "Michigan (MI)"), - ("MN", "Minnesota (MN)"), - ("MS", "Mississippi (MS)"), - ("MO", "Missouri (MO)"), - ("MT", "Montana (MT)"), - ("NE", "Nebraska (NE)"), - ("NV", "Nevada (NV)"), - ("NH", "New Hampshire (NH)"), - ("NJ", "New Jersey (NJ)"), - ("NM", "New Mexico (NM)"), - ("NY", "New York (NY)"), - ("NC", "North Carolina (NC)"), - ("ND", "North Dakota (ND)"), - ("MP", "Northern Mariana Islands (MP)"), - ("OH", "Ohio (OH)"), - ("OK", "Oklahoma (OK)"), - ("OR", "Oregon (OR)"), - ("PA", "Pennsylvania (PA)"), - ("PR", "Puerto Rico (PR)"), - ("RI", "Rhode Island (RI)"), - ("SC", "South Carolina (SC)"), - ("SD", "South Dakota (SD)"), - ("TN", "Tennessee (TN)"), - ("TX", "Texas (TX)"), - ("UM", "United States Minor Outlying Islands (UM)"), - ("UT", "Utah (UT)"), - ("VT", "Vermont (VT)"), - ("VI", "Virgin Islands (VI)"), - ("VA", "Virginia (VA)"), - ("WA", "Washington (WA)"), - ("WV", "West Virginia (WV)"), - ("WI", "Wisconsin (WI)"), - ("WY", "Wyoming (WY)"), - ("AA", "Armed Forces Americas (AA)"), - ("AE", "Armed Forces Africa, Canada, Europe, Middle East (AE)"), - ("AP", "Armed Forces Pacific (AP)"), - ], - help_text="State, territory, or military post", - max_length=2, - null=True, - ), - ), - ( - "zipcode", - models.CharField(blank=True, db_index=True, help_text="Zip code", max_length=10, null=True), - ), - ( - "urbanization", - models.CharField(blank=True, help_text="Urbanization (required for Puerto Rico only)", null=True), - ), - ( - "about_your_organization", - models.TextField(blank=True, help_text="Information about your organization", null=True), - ), - ("purpose", models.TextField(blank=True, help_text="Purpose of your domain", null=True)), - ( - "no_other_contacts_rationale", - models.TextField(blank=True, help_text="Reason for listing no additional contacts", null=True), - ), - ("anything_else", models.TextField(blank=True, help_text="Anything else?", null=True)), - ( - "is_policy_acknowledged", - models.BooleanField(blank=True, help_text="Acknowledged .gov acceptable use policy", null=True), - ), - ("submission_date", models.DateField(blank=True, default=None, help_text="Date submitted", null=True)), - ("notes", models.TextField(blank=True, help_text="Notes about this request", null=True)), - ( - "alternative_domains", - models.ManyToManyField(blank=True, related_name="alternatives+", to="registrar.website"), - ), - ( - "approved_domain", - models.OneToOneField( - blank=True, - help_text="The approved domain", - null=True, - on_delete=django.db.models.deletion.SET_NULL, - related_name="domain_request", - to="registrar.domain", - ), - ), - ( - "authorizing_official", - models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="authorizing_official", - to="registrar.contact", - ), - ), - ( - "creator", - models.ForeignKey( - on_delete=django.db.models.deletion.PROTECT, - related_name="domain_requests_created", - to=settings.AUTH_USER_MODEL, - ), - ), - ( - "current_websites", - models.ManyToManyField( - blank=True, related_name="current+", to="registrar.website", verbose_name="websites" - ), - ), - ( - "investigator", - models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.SET_NULL, - related_name="domain_requests_investigating", - to=settings.AUTH_USER_MODEL, - ), - ), - ( - "other_contacts", - models.ManyToManyField( - blank=True, - related_name="contact_domain_requests", - to="registrar.contact", - verbose_name="contacts", - ), - ), - ( - "requested_domain", - models.OneToOneField( - blank=True, - help_text="The requested domain", - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="domain_request", - to="registrar.draftdomain", - ), - ), - ( - "submitter", - models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="submitted_domain_requests", - to="registrar.contact", - ), - ), - ], - options={ - "abstract": False, - }, - ), - migrations.RemoveField( - model_name="domaininformation", - name="domain_application", - ), - migrations.AlterField( - model_name="domaininformation", - name="other_contacts", - field=models.ManyToManyField( - blank=True, - related_name="contact_domain_requests_information", - to="registrar.contact", - verbose_name="contacts", - ), - ), - migrations.AlterField( - model_name="domaininformation", - name="submitter", - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="submitted_domain_requests_information", - to="registrar.contact", - ), - ), - migrations.DeleteModel( - name="DomainApplication", - ), - migrations.AddField( - model_name="domaininformation", - name="domain_request", - field=models.OneToOneField( - blank=True, - help_text="Associated domain request", - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="DomainRequest_info", - to="registrar.domainrequest", - ), - ), - ] diff --git a/src/registrar/migrations/0073_rename_domainapplication.py b/src/registrar/migrations/0073_rename_domainapplication.py new file mode 100644 index 000000000..488111fab --- /dev/null +++ b/src/registrar/migrations/0073_rename_domainapplication.py @@ -0,0 +1,16 @@ +# Generated by Django 4.2.7 on 2024-01-29 22:21 + +from django.db import migrations + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0072_alter_publiccontact_fax_alter_publiccontact_voice"), + ] + + operations = [ + migrations.RenameModel( + old_name="DomainApplication", + new_name="DomainRequest", + ) + ] diff --git a/src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py b/src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py new file mode 100644 index 000000000..d654b0d1b --- /dev/null +++ b/src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py @@ -0,0 +1,114 @@ +# Generated by Django 4.2.10 on 2024-03-11 19:12 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0073_rename_domainapplication"), + ] + + operations = [ + migrations.RemoveField( + model_name="domaininformation", + name="domain_application", + ), + migrations.AddField( + model_name="domaininformation", + name="domain_request", + field=models.OneToOneField( + blank=True, + help_text="Associated domain request", + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="DomainRequest_info", + to="registrar.domainrequest", + ), + ), + migrations.AlterField( + model_name="domaininformation", + name="other_contacts", + field=models.ManyToManyField( + blank=True, + related_name="contact_domain_requests_information", + to="registrar.contact", + verbose_name="contacts", + ), + ), + migrations.AlterField( + model_name="domaininformation", + name="submitter", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="submitted_domain_requests_information", + to="registrar.contact", + ), + ), + migrations.AlterField( + model_name="domainrequest", + name="approved_domain", + field=models.OneToOneField( + blank=True, + help_text="The approved domain", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="domain_request", + to="registrar.domain", + ), + ), + migrations.AlterField( + model_name="domainrequest", + name="creator", + field=models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, + related_name="domain_requests_created", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterField( + model_name="domainrequest", + name="investigator", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="domain_requests_investigating", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterField( + model_name="domainrequest", + name="other_contacts", + field=models.ManyToManyField( + blank=True, related_name="contact_domain_requests", to="registrar.contact", verbose_name="contacts" + ), + ), + migrations.AlterField( + model_name="domainrequest", + name="requested_domain", + field=models.OneToOneField( + blank=True, + help_text="The requested domain", + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="domain_request", + to="registrar.draftdomain", + ), + ), + migrations.AlterField( + model_name="domainrequest", + name="submitter", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="submitted_domain_requests", + to="registrar.contact", + ), + ), + ] diff --git a/src/registrar/migrations/0074_create_groups_v08.py b/src/registrar/migrations/0075_create_groups_v08.py similarity index 93% rename from src/registrar/migrations/0074_create_groups_v08.py rename to src/registrar/migrations/0075_create_groups_v08.py index 0c28cee52..a8e70b88a 100644 --- a/src/registrar/migrations/0074_create_groups_v08.py +++ b/src/registrar/migrations/0075_create_groups_v08.py @@ -25,7 +25,7 @@ def create_groups(apps, schema_editor) -> Any: class Migration(migrations.Migration): dependencies = [ - ("registrar", "0073_domainrequest_and_more"), + ("registrar", "0074_remove_domaininformation_domain_application_and_more"), ] operations = [ From 5d41eae80178d5ab914de354640a7607a3f00b0e Mon Sep 17 00:00:00 2001 From: CocoByte Date: Tue, 12 Mar 2024 00:24:04 -0600 Subject: [PATCH 36/43] more fixes (works locally) --- ...ename_domaininformation_domain_application_and_more.py} | 7 ++++--- src/registrar/migrations/0075_create_groups_v08.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) rename src/registrar/migrations/{0074_remove_domaininformation_domain_application_and_more.py => 0074_rename_domaininformation_domain_application_and_more.py} (96%) diff --git a/src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py b/src/registrar/migrations/0074_rename_domaininformation_domain_application_and_more.py similarity index 96% rename from src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py rename to src/registrar/migrations/0074_rename_domaininformation_domain_application_and_more.py index d654b0d1b..b0309bfd9 100644 --- a/src/registrar/migrations/0074_remove_domaininformation_domain_application_and_more.py +++ b/src/registrar/migrations/0074_rename_domaininformation_domain_application_and_more.py @@ -12,11 +12,12 @@ class Migration(migrations.Migration): ] operations = [ - migrations.RemoveField( + migrations.RenameField( model_name="domaininformation", - name="domain_application", + old_name="domain_application", + new_name="domain_request", ), - migrations.AddField( + migrations.AlterField( model_name="domaininformation", name="domain_request", field=models.OneToOneField( diff --git a/src/registrar/migrations/0075_create_groups_v08.py b/src/registrar/migrations/0075_create_groups_v08.py index a8e70b88a..db07ffe78 100644 --- a/src/registrar/migrations/0075_create_groups_v08.py +++ b/src/registrar/migrations/0075_create_groups_v08.py @@ -25,7 +25,7 @@ def create_groups(apps, schema_editor) -> Any: class Migration(migrations.Migration): dependencies = [ - ("registrar", "0074_remove_domaininformation_domain_application_and_more"), + ("registrar", "0074_rename_domaininformation_domain_application_and_more"), ] operations = [ From d4ddd6bb38fac8d09b8d1008af8505de234ea0bc Mon Sep 17 00:00:00 2001 From: CocoByte Date: Tue, 12 Mar 2024 12:37:31 -0600 Subject: [PATCH 37/43] more fixes --- ...inapplication_approved_domain_and_more.py} | 135 +++++++++--------- .../0073_rename_domainapplication.py | 16 --- ...omainapplication_domainrequest_and_more.py | 22 +++ .../migrations/0075_create_groups_v08.py | 2 +- 4 files changed, 88 insertions(+), 87 deletions(-) rename src/registrar/migrations/{0074_rename_domaininformation_domain_application_and_more.py => 0073_alter_domainapplication_approved_domain_and_more.py} (84%) delete mode 100644 src/registrar/migrations/0073_rename_domainapplication.py create mode 100644 src/registrar/migrations/0074_rename_domainapplication_domainrequest_and_more.py diff --git a/src/registrar/migrations/0074_rename_domaininformation_domain_application_and_more.py b/src/registrar/migrations/0073_alter_domainapplication_approved_domain_and_more.py similarity index 84% rename from src/registrar/migrations/0074_rename_domaininformation_domain_application_and_more.py rename to src/registrar/migrations/0073_alter_domainapplication_approved_domain_and_more.py index b0309bfd9..1eacf0e56 100644 --- a/src/registrar/migrations/0074_rename_domaininformation_domain_application_and_more.py +++ b/src/registrar/migrations/0073_alter_domainapplication_approved_domain_and_more.py @@ -1,4 +1,4 @@ -# Generated by Django 4.2.10 on 2024-03-11 19:12 +# Generated by Django 4.2.10 on 2024-03-12 16:50 from django.conf import settings from django.db import migrations, models @@ -8,25 +8,82 @@ import django.db.models.deletion class Migration(migrations.Migration): dependencies = [ - ("registrar", "0073_rename_domainapplication"), + ("registrar", "0072_alter_publiccontact_fax_alter_publiccontact_voice"), ] operations = [ - migrations.RenameField( - model_name="domaininformation", - old_name="domain_application", - new_name="domain_request", + migrations.AlterField( + model_name="domainapplication", + name="approved_domain", + field=models.OneToOneField( + blank=True, + help_text="The approved domain", + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="domain_request", + to="registrar.domain", + ), + ), + migrations.AlterField( + model_name="domainapplication", + name="creator", + field=models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, + related_name="domain_requests_created", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterField( + model_name="domainapplication", + name="investigator", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.SET_NULL, + related_name="domain_requests_investigating", + to=settings.AUTH_USER_MODEL, + ), + ), + migrations.AlterField( + model_name="domainapplication", + name="other_contacts", + field=models.ManyToManyField( + blank=True, related_name="contact_domain_requests", to="registrar.contact", verbose_name="contacts" + ), + ), + migrations.AlterField( + model_name="domainapplication", + name="requested_domain", + field=models.OneToOneField( + blank=True, + help_text="The requested domain", + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="domain_request", + to="registrar.draftdomain", + ), + ), + migrations.AlterField( + model_name="domainapplication", + name="submitter", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="submitted_domain_requests", + to="registrar.contact", + ), ), migrations.AlterField( model_name="domaininformation", - name="domain_request", + name="domain_application", field=models.OneToOneField( blank=True, help_text="Associated domain request", null=True, on_delete=django.db.models.deletion.PROTECT, related_name="DomainRequest_info", - to="registrar.domainrequest", + to="registrar.domainapplication", ), ), migrations.AlterField( @@ -50,66 +107,4 @@ class Migration(migrations.Migration): to="registrar.contact", ), ), - migrations.AlterField( - model_name="domainrequest", - name="approved_domain", - field=models.OneToOneField( - blank=True, - help_text="The approved domain", - null=True, - on_delete=django.db.models.deletion.SET_NULL, - related_name="domain_request", - to="registrar.domain", - ), - ), - migrations.AlterField( - model_name="domainrequest", - name="creator", - field=models.ForeignKey( - on_delete=django.db.models.deletion.PROTECT, - related_name="domain_requests_created", - to=settings.AUTH_USER_MODEL, - ), - ), - migrations.AlterField( - model_name="domainrequest", - name="investigator", - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.SET_NULL, - related_name="domain_requests_investigating", - to=settings.AUTH_USER_MODEL, - ), - ), - migrations.AlterField( - model_name="domainrequest", - name="other_contacts", - field=models.ManyToManyField( - blank=True, related_name="contact_domain_requests", to="registrar.contact", verbose_name="contacts" - ), - ), - migrations.AlterField( - model_name="domainrequest", - name="requested_domain", - field=models.OneToOneField( - blank=True, - help_text="The requested domain", - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="domain_request", - to="registrar.draftdomain", - ), - ), - migrations.AlterField( - model_name="domainrequest", - name="submitter", - field=models.ForeignKey( - blank=True, - null=True, - on_delete=django.db.models.deletion.PROTECT, - related_name="submitted_domain_requests", - to="registrar.contact", - ), - ), ] diff --git a/src/registrar/migrations/0073_rename_domainapplication.py b/src/registrar/migrations/0073_rename_domainapplication.py deleted file mode 100644 index 488111fab..000000000 --- a/src/registrar/migrations/0073_rename_domainapplication.py +++ /dev/null @@ -1,16 +0,0 @@ -# Generated by Django 4.2.7 on 2024-01-29 22:21 - -from django.db import migrations - - -class Migration(migrations.Migration): - dependencies = [ - ("registrar", "0072_alter_publiccontact_fax_alter_publiccontact_voice"), - ] - - operations = [ - migrations.RenameModel( - old_name="DomainApplication", - new_name="DomainRequest", - ) - ] diff --git a/src/registrar/migrations/0074_rename_domainapplication_domainrequest_and_more.py b/src/registrar/migrations/0074_rename_domainapplication_domainrequest_and_more.py new file mode 100644 index 000000000..2dae12340 --- /dev/null +++ b/src/registrar/migrations/0074_rename_domainapplication_domainrequest_and_more.py @@ -0,0 +1,22 @@ +# Generated by Django 4.2.10 on 2024-03-12 16:53 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("registrar", "0073_alter_domainapplication_approved_domain_and_more"), + ] + + operations = [ + migrations.RenameModel( + old_name="DomainApplication", + new_name="DomainRequest", + ), + migrations.RenameField( + model_name="domaininformation", + old_name="domain_application", + new_name="domain_request", + ), + ] diff --git a/src/registrar/migrations/0075_create_groups_v08.py b/src/registrar/migrations/0075_create_groups_v08.py index db07ffe78..b0b2ed740 100644 --- a/src/registrar/migrations/0075_create_groups_v08.py +++ b/src/registrar/migrations/0075_create_groups_v08.py @@ -25,7 +25,7 @@ def create_groups(apps, schema_editor) -> Any: class Migration(migrations.Migration): dependencies = [ - ("registrar", "0074_rename_domaininformation_domain_application_and_more"), + ("registrar", "0074_rename_domainapplication_domainrequest_and_more"), ] operations = [ From ca1a79a97f2c1f858ae4b1f49ecfe094bc531ec1 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:35:39 -0600 Subject: [PATCH 38/43] Update fixtures_domain_requests.py --- src/registrar/fixtures_domain_requests.py | 27 ++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/registrar/fixtures_domain_requests.py b/src/registrar/fixtures_domain_requests.py index a78bdd1ab..a37e29d6b 100644 --- a/src/registrar/fixtures_domain_requests.py +++ b/src/registrar/fixtures_domain_requests.py @@ -1,6 +1,7 @@ import logging import random from faker import Faker +from django.db import transaction from registrar.models import ( User, @@ -184,6 +185,14 @@ class DomainRequestFixture: logger.warning(e) return + # Lumped under .atomic to ensure we don't make redundant DB calls. + # This bundles them all together, and then saves it in a single call. + with transaction.atomic(): + cls._create_domain_requests(users) + + @classmethod + def _create_domain_requests(cls, users): + """Creates DomainRequests given a list of users""" for user in users: logger.debug("Loading domain requests for %s" % user) for app in cls.DA: @@ -211,8 +220,16 @@ class DomainFixture(DomainRequestFixture): logger.warning(e) return + # Lumped under .atomic to ensure we don't make redundant DB calls. + # This bundles them all together, and then saves it in a single call. + with transaction.atomic(): + # approve each user associated with `in review` status domains + DomainFixture._approve_domain_requests(users) + + @staticmethod + def _approve_domain_requests(users): + """Approves all provided domain requests if they are in the state in_review""" for user in users: - # approve one of each users in review status domains domain_request = DomainRequest.objects.filter( creator=user, status=DomainRequest.DomainRequestStatus.IN_REVIEW ).last() @@ -220,5 +237,13 @@ class DomainFixture(DomainRequestFixture): # We don't want fixtures sending out real emails to # fake email addresses, so we just skip that and log it instead + + # All approvals require an investigator, so if there is none, + # assign one. + if domain_request.investigator is None: + # All "users" in fixtures have admin perms per prior config. + # No need to check for that. + domain_request.investigator = random.choice(users) # nosec + domain_request.approve(send_email=False) domain_request.save() From a5a3c13653905ae19e9f788229876c4bf311cf49 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:47:30 -0600 Subject: [PATCH 39/43] Merge conflictts pt 1 --- src/registrar/admin.py | 22 +++++------ src/registrar/models/domain_request.py | 54 +++++++++++++------------- src/registrar/tests/common.py | 8 ++-- src/registrar/tests/test_models.py | 32 +++++++-------- 4 files changed, 58 insertions(+), 58 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index a7871d5c6..7dc13df17 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -139,17 +139,17 @@ class DomainRequestAdminForm(forms.ModelForm): # We only care about investigator when in these statuses checked_statuses = [ - DomainApplication.ApplicationStatus.APPROVED, - DomainApplication.ApplicationStatus.IN_REVIEW, - DomainApplication.ApplicationStatus.ACTION_NEEDED, - DomainApplication.ApplicationStatus.REJECTED, - DomainApplication.ApplicationStatus.INELIGIBLE, + DomainRequest.DomainRequestStatus.APPROVED, + DomainRequest.DomainRequestStatus.IN_REVIEW, + DomainRequest.DomainRequestStatus.ACTION_NEEDED, + DomainRequest.DomainRequestStatus.REJECTED, + DomainRequest.DomainRequestStatus.INELIGIBLE, ] # If a status change occured, check for validity if status != initial_status and status in checked_statuses: # Checks the "investigators" field for validity. - # That field must obey certain conditions when an application is approved. + # That field must obey certain conditions when an domain_request is approved. # Will call "add_error" if any issues are found. self._check_for_valid_investigator(investigator) @@ -1163,7 +1163,7 @@ class DomainRequestAdmin(ListHeaderAdmin): # == Handle non-status changes == # - # Get the original application from the database. + # Get the original domain_request from the database. original_obj = models.DomainRequest.objects.get(pk=obj.pk) if obj.status == original_obj.status: # If the status hasn't changed, let the base function take care of it @@ -1203,9 +1203,9 @@ class DomainRequestAdmin(ListHeaderAdmin): should_proceed = False return should_proceed - application_is_not_approved = obj.status != models.DomainRequest.DomainRequestStatus.APPROVED - if application_is_not_approved and not obj.domain_is_not_active(): - # If an admin tried to set an approved application to + request_is_not_approved = obj.status != models.DomainRequest.DomainRequestStatus.APPROVED + if request_is_not_approved and not obj.domain_is_not_active(): + # If an admin tried to set an approved domain_request to # another status and the related domain is already # active, shortcut the action and throw a friendly # error message. This action would still not go through @@ -1248,7 +1248,7 @@ class DomainRequestAdmin(ListHeaderAdmin): return (obj, should_proceed) def get_status_method_mapping(self, domain_request): - """Returns what method should be ran given an application object""" + """Returns what method should be ran given an DomainRequest object""" # Define a per-object mapping status_method_mapping = { models.DomainRequest.DomainRequestStatus.STARTED: None, diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 4a20514a7..4f7e79c39 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -9,7 +9,7 @@ from django.db import models from django_fsm import FSMField, transition # type: ignore from django.utils import timezone from registrar.models.domain import Domain -from registrar.utility.errors import FSMdomain_requestError, FSMErrorCodes +from registrar.utility.errors import FSMApplicationError, FSMErrorCodes from .utility.time_stamped_model import TimeStampedModel from ..utility.email import send_templated_email, EmailSendingError @@ -19,7 +19,7 @@ logger = logging.getLogger(__name__) class DomainRequest(TimeStampedModel): - """A registrant's request for a new domain.""" + """A registrant's domain request for a new domain.""" # Constants for choice fields class DomainRequestStatus(models.TextChoices): @@ -116,7 +116,7 @@ class DomainRequest(TimeStampedModel): class OrganizationChoicesVerbose(models.TextChoices): """ Secondary organization choices - For use in the domain_request form and on the templates + For use in the domain request form and on the templates Keys need to match OrganizationChoices """ @@ -367,7 +367,7 @@ class DomainRequest(TimeStampedModel): NAMING_REQUIREMENTS = "naming_not_met", "Naming requirements not met" OTHER = "other", "Other/Unspecified" - # #### Internal fields about the domain_request ##### + # #### Internal fields about the domain request ##### status = FSMField( choices=DomainRequestStatus.choices, # possible states as an array of constants default=DomainRequestStatus.STARTED, # sensible default @@ -380,7 +380,7 @@ class DomainRequest(TimeStampedModel): blank=True, ) - # This is the domain_request user who created this domain_request. The contact + # This is the domain request user who created this domain request. The contact # information that they gave is in the `submitter` field creator = models.ForeignKey( "registrar.User", @@ -500,7 +500,7 @@ class DomainRequest(TimeStampedModel): on_delete=models.PROTECT, ) - # "+" means no reverse relation to lookup domain_requests from Website + # "+" means no reverse relation to lookup domain requests from Website current_websites = models.ManyToManyField( "registrar.Website", blank=True, @@ -513,7 +513,7 @@ class DomainRequest(TimeStampedModel): null=True, blank=True, help_text="The approved domain", - related_name="domain_domain_request", + related_name="domain_request", on_delete=models.SET_NULL, ) @@ -522,7 +522,7 @@ class DomainRequest(TimeStampedModel): null=True, blank=True, help_text="The requested domain", - related_name="domain_domain_request", + related_name="domain_request", on_delete=models.PROTECT, ) alternative_domains = models.ManyToManyField( @@ -531,8 +531,8 @@ class DomainRequest(TimeStampedModel): related_name="alternatives+", ) - # This is the contact information provided by the applicant. The - # domain_request user who created it is in the `creator` field. + # This is the contact information provided by the domain requestor. The + # user who created the domain request is in the `creator` field. submitter = models.ForeignKey( "registrar.Contact", null=True, @@ -572,7 +572,7 @@ class DomainRequest(TimeStampedModel): help_text="Acknowledged .gov acceptable use policy", ) - # submission date records when domain_request is submitted + # submission date records when domain request is submitted submission_date = models.DateField( null=True, blank=True, @@ -591,7 +591,7 @@ class DomainRequest(TimeStampedModel): if self.requested_domain and self.requested_domain.name: return self.requested_domain.name else: - return f"{self.status} domain_request created by {self.creator}" + return f"{self.status} domain request created by {self.creator}" except Exception: return "" @@ -665,7 +665,7 @@ class DomainRequest(TimeStampedModel): target=DomainRequestStatus.SUBMITTED, ) def submit(self): - """Submit an domain_request that is started. + """Submit an domain request that is started. As a side effect, an email notification is sent.""" @@ -713,7 +713,7 @@ class DomainRequest(TimeStampedModel): conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def in_review(self): - """Investigate an domain_request that has been submitted. + """Investigate an domain request that has been submitted. This action is logged. @@ -745,7 +745,7 @@ class DomainRequest(TimeStampedModel): conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def action_needed(self): - """Send back an domain_request that is under investigation or rejected. + """Send back an domain request that is under investigation or rejected. This action is logged. @@ -783,7 +783,7 @@ class DomainRequest(TimeStampedModel): This has substantial side-effects because it creates another database object for the approved Domain and makes the user who created the - domain_request into an admin on that domain. It also triggers an email + domain request into an admin on that domain. It also triggers an email notification.""" # create the domain @@ -791,7 +791,7 @@ class DomainRequest(TimeStampedModel): # == Check that the domain_request is valid == # if Domain.objects.filter(name=self.requested_domain.name).exists(): - raise FSMdomain_requestError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) + raise FSMApplicationError(code=FSMErrorCodes.APPROVE_DOMAIN_IN_USE) # == Create the domain and related components == # created_domain = Domain.objects.create(name=self.requested_domain.name) @@ -799,7 +799,7 @@ class DomainRequest(TimeStampedModel): # copy the information from DomainRequest into domaininformation DomainInformation = apps.get_model("registrar.DomainInformation") - DomainInformation.create_from_da(domain_domain_request=self, domain=created_domain) + DomainInformation.create_from_da(domain_request=self, domain=created_domain) # create the permission for the user UserDomainRole = apps.get_model("registrar.UserDomainRole") @@ -812,7 +812,7 @@ class DomainRequest(TimeStampedModel): # == Send out an email == # self._send_status_update_email( - "domain_request approved", + "domain request approved", "emails/status_change_approved.txt", "emails/status_change_approved_subject.txt", send_email, @@ -824,7 +824,7 @@ class DomainRequest(TimeStampedModel): target=DomainRequestStatus.WITHDRAWN, ) def withdraw(self): - """Withdraw an domain_request that has been submitted.""" + """Withdraw an domain request that has been submitted.""" self._send_status_update_email( "withdraw", @@ -839,7 +839,7 @@ class DomainRequest(TimeStampedModel): conditions=[domain_is_not_active, investigator_exists_and_is_staff], ) def reject(self): - """Reject an domain_request that has been submitted. + """Reject an domain request that has been submitted. As side effects this will delete the domain and domain_information (will cascade), and send an email notification.""" @@ -868,7 +868,7 @@ class DomainRequest(TimeStampedModel): """The applicant is a bad actor, reject with prejudice. No email As a side effect, but we block the applicant from editing - any existing domains/domain_requests and from submitting new aplications. + any existing domains/domain requests and from submitting new aplications. We do this by setting an ineligible status on the user, which the permissions classes test against. This will also delete the domain and domain_information (will cascade) when they exist.""" @@ -881,7 +881,7 @@ class DomainRequest(TimeStampedModel): # ## Form policies ### # # These methods control what questions need to be answered by applicants - # during the domain_request flow. They are policies about the domain_request so + # during the domain request flow. They are policies about the domain request so # they appear here. def show_organization_federal(self) -> bool: @@ -917,15 +917,15 @@ class DomainRequest(TimeStampedModel): ] def has_rationale(self) -> bool: - """Does this domain_request have no_other_contacts_rationale?""" + """Does this domain request have no_other_contacts_rationale?""" return bool(self.no_other_contacts_rationale) def has_other_contacts(self) -> bool: - """Does this domain_request have other contacts listed?""" + """Does this domain request have other contacts listed?""" return self.other_contacts.exists() def is_federal(self) -> Union[bool, None]: - """Is this domain_request for a federal agency? + """Is this domain request for a federal agency? organization_type can be both null and blank, """ @@ -957,4 +957,4 @@ class DomainRequest(TimeStampedModel): data[field.name] = field.value_from_object(self) for field in opts.many_to_many: data[field.name] = field.value_from_object(self) - return data \ No newline at end of file + return data diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 13f15a6d0..587935d54 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -599,11 +599,11 @@ def completed_domain_request( return domain_request -def set_domain_request_investigators(application_list: list[DomainRequest], investigator_user: User): +def set_domain_request_investigators(domain_request_list: list[DomainRequest], investigator_user: User): """Helper method that sets the investigator field of all provided domain_requests""" - for application in application_list: - application.investigator = investigator_user - application.save() + for request in domain_request_list: + request.investigator = investigator_user + request.save() def multiple_unalphabetical_domain_objects( diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index b46676e26..7956b5dc0 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -52,8 +52,8 @@ class TestDomainRequest(TestCase): status=DomainRequest.DomainRequestStatus.INELIGIBLE, name="ineligible.gov" ) - # Store all application statuses in a variable for ease of use - self.all_applications = [ + # Store all domain request statuses in a variable for ease of use + self.all_domain_requests = [ self.started_domain_request, self.submitted_domain_request, self.in_review_domain_request, @@ -269,7 +269,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_does_not_raise_error(test_cases, "submit") @@ -286,7 +286,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_domain_request_investigators(self.all_applications, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_does_not_raise_error(test_cases, "submit") @@ -363,7 +363,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_raises_error(test_cases, "in_review") @@ -382,7 +382,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_applications, user) + set_applications_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "in_review") @@ -424,7 +424,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_raises_error(test_cases, "action_needed") @@ -442,7 +442,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_applications, user) + set_applications_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "action_needed") @@ -484,7 +484,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_raises_error(test_cases, "approve") @@ -501,7 +501,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_applications, user) + set_applications_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "approve") @@ -555,7 +555,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_does_not_raise_error(test_cases, "withdraw") @@ -573,7 +573,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_applications, user) + set_applications_investigators(self.all_domain_requests, user) self.assert_fsm_transition_does_not_raise_error(test_cases, "withdraw") @@ -615,7 +615,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_raises_error(test_cases, "reject") @@ -632,7 +632,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_applications, user) + set_applications_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "reject") @@ -676,7 +676,7 @@ class TestDomainRequest(TestCase): ] # Set all investigators to none - set_domain_request_investigators(self.all_applications, None) + set_domain_request_investigators(self.all_domain_requests, None) self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") @@ -694,7 +694,7 @@ class TestDomainRequest(TestCase): # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_applications, user) + set_applications_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") From 5661174a2dc6b6bff916d35e93247d349e4695e6 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:48:46 -0600 Subject: [PATCH 40/43] Update admin.py --- src/registrar/admin.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 7dc13df17..b321b9107 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -149,7 +149,7 @@ class DomainRequestAdminForm(forms.ModelForm): # If a status change occured, check for validity if status != initial_status and status in checked_statuses: # Checks the "investigators" field for validity. - # That field must obey certain conditions when an domain_request is approved. + # That field must obey certain conditions when an domain request is approved. # Will call "add_error" if any issues are found. self._check_for_valid_investigator(investigator) @@ -1163,7 +1163,7 @@ class DomainRequestAdmin(ListHeaderAdmin): # == Handle non-status changes == # - # Get the original domain_request from the database. + # Get the original domain request from the database. original_obj = models.DomainRequest.objects.get(pk=obj.pk) if obj.status == original_obj.status: # If the status hasn't changed, let the base function take care of it @@ -1205,7 +1205,7 @@ class DomainRequestAdmin(ListHeaderAdmin): request_is_not_approved = obj.status != models.DomainRequest.DomainRequestStatus.APPROVED if request_is_not_approved and not obj.domain_is_not_active(): - # If an admin tried to set an approved domain_request to + # If an admin tried to set an approved domain request to # another status and the related domain is already # active, shortcut the action and throw a friendly # error message. This action would still not go through @@ -1248,7 +1248,7 @@ class DomainRequestAdmin(ListHeaderAdmin): return (obj, should_proceed) def get_status_method_mapping(self, domain_request): - """Returns what method should be ran given an DomainRequest object""" + """Returns what method should be ran given an domain request object""" # Define a per-object mapping status_method_mapping = { models.DomainRequest.DomainRequestStatus.STARTED: None, From 0f0ce6be53d488568e6b2070127791de55777016 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:54:03 -0600 Subject: [PATCH 41/43] Additional merge conflicts --- src/registrar/models/domain_request.py | 2 +- src/registrar/tests/common.py | 2 +- src/registrar/tests/test_models.py | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/registrar/models/domain_request.py b/src/registrar/models/domain_request.py index 4f7e79c39..2282af726 100644 --- a/src/registrar/models/domain_request.py +++ b/src/registrar/models/domain_request.py @@ -777,7 +777,7 @@ class DomainRequest(TimeStampedModel): conditions=[investigator_exists_and_is_staff], ) def approve(self, send_email=True): - """Approve an domain_request that has been submitted. + """Approve an domain request that has been submitted. This action cleans up the rejection status if moving away from rejected. diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 587935d54..f5ba815f4 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -600,7 +600,7 @@ def completed_domain_request( return domain_request def set_domain_request_investigators(domain_request_list: list[DomainRequest], investigator_user: User): - """Helper method that sets the investigator field of all provided domain_requests""" + """Helper method that sets the investigator field of all provided domain requests""" for request in domain_request_list: request.investigator = investigator_user request.save() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index 7956b5dc0..de6d021d9 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -245,11 +245,11 @@ class TestDomainRequest(TestCase): def assert_fsm_transition_does_not_raise_error(self, test_cases, method_to_run): """Given a list of test cases, ensure that none of them throw transition errors""" with boto3_mocking.clients.handler_for("sesv2", self.mock_client), less_console_noise(): - for application, exception_type in test_cases: - with self.subTest(application=application, exception_type=exception_type): + for domain_request, exception_type in test_cases: + with self.subTest(domain_request=domain_request, exception_type=exception_type): try: # Retrieve the method by name from the application object and call it - method = getattr(application, method_to_run) + method = getattr(domain_request, method_to_run) # Call the method method() except exception_type: @@ -920,7 +920,7 @@ class TestDomainInformation(TestCase): self.maxDiff = None draft_domain, _ = DraftDomain.objects.get_or_create(name="igorville.gov") user, _ = User.objects.get_or_create() - domain_request = DomainApplication.objects.create(creator=user, requested_domain=draft_domain, notes="test notes") + domain_request = DomainRequest.objects.create(creator=user, requested_domain=draft_domain, notes="test notes") with boto3_mocking.clients.handler_for("sesv2", self.mock_client): with less_console_noise(): From 7f5bfa07a6c0d01f4f00fbbaed89264b5eb8a469 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:58:35 -0600 Subject: [PATCH 42/43] Additional --- src/registrar/tests/common.py | 1 + src/registrar/tests/test_models.py | 56 +++++++++++++++--------------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index f5ba815f4..24151261f 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -599,6 +599,7 @@ def completed_domain_request( return domain_request + def set_domain_request_investigators(domain_request_list: list[DomainRequest], investigator_user: User): """Helper method that sets the investigator field of all provided domain requests""" for request in domain_request_list: diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index de6d021d9..c7564b77b 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -248,7 +248,7 @@ class TestDomainRequest(TestCase): for domain_request, exception_type in test_cases: with self.subTest(domain_request=domain_request, exception_type=exception_type): try: - # Retrieve the method by name from the application object and call it + # Retrieve the method by name from the DomainRequest object and call it method = getattr(domain_request, method_to_run) # Call the method method() @@ -374,15 +374,15 @@ class TestDomainRequest(TestCase): """ test_cases = [ - (self.action_needed_application, TransitionNotAllowed), - (self.approved_application, TransitionNotAllowed), - (self.rejected_application, TransitionNotAllowed), - (self.ineligible_application, TransitionNotAllowed), + (self.action_needed_domain_request, TransitionNotAllowed), + (self.approved_domain_request, TransitionNotAllowed), + (self.rejected_domain_request, TransitionNotAllowed), + (self.ineligible_domain_request, TransitionNotAllowed), ] # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_domain_requests, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "in_review") @@ -434,15 +434,15 @@ class TestDomainRequest(TestCase): """ test_cases = [ - (self.in_review_application, TransitionNotAllowed), - (self.approved_application, TransitionNotAllowed), - (self.rejected_application, TransitionNotAllowed), - (self.ineligible_application, TransitionNotAllowed), + (self.in_review_domain_request, TransitionNotAllowed), + (self.approved_domain_request, TransitionNotAllowed), + (self.rejected_domain_request, TransitionNotAllowed), + (self.ineligible_domain_request, TransitionNotAllowed), ] # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_domain_requests, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "action_needed") @@ -494,14 +494,14 @@ class TestDomainRequest(TestCase): """ test_cases = [ - (self.in_review_application, TransitionNotAllowed), - (self.action_needed_application, TransitionNotAllowed), - (self.rejected_application, TransitionNotAllowed), + (self.in_review_domain_request, TransitionNotAllowed), + (self.action_needed_domain_request, TransitionNotAllowed), + (self.rejected_domain_request, TransitionNotAllowed), ] # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_domain_requests, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "approve") @@ -566,14 +566,14 @@ class TestDomainRequest(TestCase): """ test_cases = [ - (self.submitted_application, TransitionNotAllowed), - (self.in_review_application, TransitionNotAllowed), - (self.action_needed_application, TransitionNotAllowed), + (self.submitted_domain_request, TransitionNotAllowed), + (self.in_review_domain_request, TransitionNotAllowed), + (self.action_needed_domain_request, TransitionNotAllowed), ] # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_domain_requests, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_does_not_raise_error(test_cases, "withdraw") @@ -625,14 +625,14 @@ class TestDomainRequest(TestCase): """ test_cases = [ - (self.in_review_application, TransitionNotAllowed), - (self.action_needed_application, TransitionNotAllowed), - (self.approved_application, TransitionNotAllowed), + (self.in_review_domain_request, TransitionNotAllowed), + (self.action_needed_domain_request, TransitionNotAllowed), + (self.approved_domain_request, TransitionNotAllowed), ] # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_domain_requests, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "reject") @@ -686,15 +686,15 @@ class TestDomainRequest(TestCase): """ test_cases = [ - (self.in_review_application, TransitionNotAllowed), - (self.action_needed_application, TransitionNotAllowed), - (self.approved_application, TransitionNotAllowed), - (self.rejected_application, TransitionNotAllowed), + (self.in_review_domain_request, TransitionNotAllowed), + (self.action_needed_domain_request, TransitionNotAllowed), + (self.approved_domain_request, TransitionNotAllowed), + (self.rejected_domain_request, TransitionNotAllowed), ] # Set all investigators to a user with no staff privs user, _ = User.objects.get_or_create(username="pancakesyrup", is_staff=False) - set_applications_investigators(self.all_domain_requests, user) + set_domain_request_investigators(self.all_domain_requests, user) self.assert_fsm_transition_raises_error(test_cases, "reject_with_prejudice") From 26dabb3ab0b9d49526a8f9fe6dc958b345f8efd4 Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Tue, 12 Mar 2024 16:10:19 -0600 Subject: [PATCH 43/43] Update test_models.py --- src/registrar/tests/test_models.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index c7564b77b..4d5315b80 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -917,10 +917,12 @@ class TestDomainInformation(TestCase): @boto3_mocking.patching def test_approval_creates_info(self): - self.maxDiff = None draft_domain, _ = DraftDomain.objects.get_or_create(name="igorville.gov") user, _ = User.objects.get_or_create() - domain_request = DomainRequest.objects.create(creator=user, requested_domain=draft_domain, notes="test notes") + investigator, _ = User.objects.get_or_create(username="frenchtoast", is_staff=True) + domain_request = DomainRequest.objects.create( + creator=user, requested_domain=draft_domain, notes="test notes", investigator=investigator + ) with boto3_mocking.clients.handler_for("sesv2", self.mock_client): with less_console_noise():