From 324bea9868fb785f0b6d4f8f8fcc655241c2ac65 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 5 Sep 2023 15:19:39 -0400 Subject: [PATCH 01/26] check is_active on rejected and ineligible, delete domain and domain info and remove references, intercept and show friendly error in admin --- src/registrar/admin.py | 84 +++++++++++++++------- src/registrar/models/domain.py | 9 ++- src/registrar/models/domain_application.py | 37 ++++++++-- src/registrar/models/domain_information.py | 4 +- 4 files changed, 100 insertions(+), 34 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4696a15bf..a46873f51 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -104,6 +104,7 @@ class MyUserAdmin(BaseUserAdmin): inlines = [UserContactInline] list_display = ( + "username", "email", "first_name", "last_name", @@ -202,6 +203,13 @@ class ContactAdmin(ListHeaderAdmin): search_help_text = "Search by firstname, lastname or email." +class DomainInformationAdmin(ListHeaderAdmin): + """Custom contact admin class to add search.""" + + search_fields = ["domain__name"] + search_help_text = "Search by domain name." + + class DomainApplicationAdmin(ListHeaderAdmin): """Customize the applications listing view.""" @@ -234,7 +242,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Detail view fieldsets = [ - (None, {"fields": ["status", "investigator", "creator"]}), + (None, {"fields": ["status", "investigator", "creator", "approved_domain"]}), ( "Type of organization", { @@ -306,29 +314,57 @@ class DomainApplicationAdmin(ListHeaderAdmin): # Get the original application from the database original_obj = models.DomainApplication.objects.get(pk=obj.pk) - if obj.status != original_obj.status: - status_method_mapping = { - models.DomainApplication.STARTED: None, - models.DomainApplication.SUBMITTED: obj.submit, - models.DomainApplication.IN_REVIEW: obj.in_review, - models.DomainApplication.ACTION_NEEDED: obj.action_needed, - models.DomainApplication.APPROVED: obj.approve, - models.DomainApplication.WITHDRAWN: obj.withdraw, - models.DomainApplication.REJECTED: obj.reject, - models.DomainApplication.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() + if ( + obj + and original_obj.status == models.DomainApplication.APPROVED + and ( + obj.status == models.DomainApplication.REJECTED + or obj.status == models.DomainApplication.INELIGIBLE + ) + and not obj.domain_is_not_active() + ): + # If an admin tried to set an approved application to + # rejected or ineligible 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. - super().save_model(request, obj, form, change) + # 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.STARTED: None, + models.DomainApplication.SUBMITTED: obj.submit, + models.DomainApplication.IN_REVIEW: obj.in_review, + models.DomainApplication.ACTION_NEEDED: obj.action_needed, + models.DomainApplication.APPROVED: obj.approve, + models.DomainApplication.WITHDRAWN: obj.withdraw, + models.DomainApplication.REJECTED: obj.reject, + models.DomainApplication.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: # Clear the success message messages.set_level(request, messages.ERROR) @@ -382,7 +418,7 @@ admin.site.register(models.User, MyUserAdmin) admin.site.register(models.UserDomainRole, AuditedAdmin) admin.site.register(models.Contact, ContactAdmin) admin.site.register(models.DomainInvitation, AuditedAdmin) -admin.site.register(models.DomainInformation, AuditedAdmin) +admin.site.register(models.DomainInformation, DomainInformationAdmin) admin.site.register(models.Domain, DomainAdmin) admin.site.register(models.Host, MyHostAdmin) admin.site.register(models.Nameserver, MyHostAdmin) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 59563d3d8..4988ae36b 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -301,8 +301,14 @@ class Domain(TimeStampedModel, DomainHelper): # TODO: implement a check -- should be performant so it can be called for # any number of domains on a status page # this is NOT as simple as checking if Domain.Status.OK is in self.statuses + + # NOTE: This was stubbed in all along for 852 and 811 return False + def delete_request(self): + """Delete from host. Possibly a duplicate of _delete_host?""" + pass + def transfer(self): """Going somewhere. Not implemented.""" raise NotImplementedError() @@ -338,9 +344,6 @@ class Domain(TimeStampedModel, DomainHelper): help_text="Very basic info about the lifecycle of this domain object", ) - def isActive(self): - return self.state == Domain.State.CREATED - # ForeignKey on UserDomainRole creates a "permissions" member for # all of the user-roles that are in place for this domain diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index b1230b703..db962fda0 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -411,7 +411,7 @@ class DomainApplication(TimeStampedModel): blank=True, help_text="The approved domain", related_name="domain_application", - on_delete=models.PROTECT, + on_delete=models.SET_NULL, ) requested_domain = models.OneToOneField( @@ -477,6 +477,11 @@ class DomainApplication(TimeStampedModel): except Exception: return "" + def domain_is_not_active(self): + if self.approved_domain: + return not self.approved_domain.is_active() + return True + def _send_status_update_email( self, new_status, email_template, email_template_subject ): @@ -600,11 +605,22 @@ class DomainApplication(TimeStampedModel): "emails/domain_request_withdrawn_subject.txt", ) - @transition(field="status", source=[IN_REVIEW, APPROVED], target=REJECTED) + @transition( + field="status", + source=[IN_REVIEW, APPROVED], + target=REJECTED, + conditions=[domain_is_not_active], + ) def reject(self): """Reject an application that has been submitted. - As a side effect, an email notification is sent, similar to in_review""" + As a side effect this will delete the domain and domain_information + (will cascade), and send an email notification""" + + if self.status == self.APPROVED: + self.approved_domain.delete_request() + self.approved_domain.delete() + self.approved_domain = None self._send_status_update_email( "action needed", @@ -612,14 +628,25 @@ class DomainApplication(TimeStampedModel): "emails/status_change_rejected_subject.txt", ) - @transition(field="status", source=[IN_REVIEW, APPROVED], target=INELIGIBLE) + @transition( + field="status", + source=[IN_REVIEW, APPROVED], + target=INELIGIBLE, + conditions=[domain_is_not_active], + ) def reject_with_prejudice(self): """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/applications and from submitting new aplications. We do this by setting an ineligible status on the user, which the - permissions classes test against""" + permissions classes test against. This will also delete the domain + and domain_information (will cascade)""" + + if self.status == self.APPROVED: + self.approved_domain.delete_request() + self.approved_domain.delete() + self.approved_domain = None self.creator.restrict_user() diff --git a/src/registrar/models/domain_information.py b/src/registrar/models/domain_information.py index b12039e73..3361185b8 100644 --- a/src/registrar/models/domain_information.py +++ b/src/registrar/models/domain_information.py @@ -13,7 +13,7 @@ logger = logging.getLogger(__name__) class DomainInformation(TimeStampedModel): """A registrant's domain information for that domain, exported from - DomainApplication. We use these field from DomainApplication with few exceptation + DomainApplication. We use these field from DomainApplication with few exceptions which are 'removed' via pop at the bottom of this file. Most of design for domain management's user information are based on application, but we cannot change the application once approved, so copying them that way we can make changes @@ -156,7 +156,7 @@ class DomainInformation(TimeStampedModel): domain = models.OneToOneField( "registrar.Domain", - on_delete=models.PROTECT, + on_delete=models.CASCADE, blank=True, null=True, # Access this information via Domain as "domain.domain_info" From 5fd84b534d9898ae1a93c711311e97959f481351 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 6 Sep 2023 15:19:32 -0400 Subject: [PATCH 02/26] Unit test the unallowed transitions, error message on admin, and side effects of successful transitions --- src/registrar/tests/test_admin.py | 155 +++++++++++++++++++++++++++++ src/registrar/tests/test_models.py | 41 +++++++- 2 files changed, 195 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index fc5478dd9..9cde6d5cf 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1,5 +1,8 @@ from django.test import TestCase, RequestFactory, Client from django.contrib.admin.sites import AdminSite +from unittest.mock import patch +from contextlib import ExitStack +from django.contrib import messages from registrar.admin import ( DomainApplicationAdmin, @@ -10,6 +13,7 @@ from registrar.admin import ( from registrar.models import ( DomainApplication, DomainInformation, + Domain, User, DomainInvitation, ) @@ -432,8 +436,159 @@ class TestDomainApplicationAdmin(TestCase): request, "Cannot edit an application with a restricted creator.", ) + + def test_error_when_saving_approved_to_rejected_and_domain_is_active(self): + # Create an instance of the model + application = completed_application(status=DomainApplication.APPROVED) + domain = Domain.objects.create(name=application.requested_domain.name) + application.approved_domain = domain + application.save() + # Create a request object with a superuser + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + request.user = self.superuser + + # Define a custom implementation for is_active + def custom_is_active(self): + return True # Override to return True + + # Use ExitStack to combine patch contexts + with ExitStack() as stack: + # Patch Domain.is_active and django.contrib.messages.error simultaneously + stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) + stack.enter_context(patch.object(messages, 'error')) + + # Simulate saving the model + application.status = DomainApplication.REJECTED + self.admin.save_model(request, application, None, True) + + # Assert that the error message was called with the correct argument + messages.error.assert_called_once_with( + request, + "This action is not permitted, the domain " + + "is already active.", + ) + + def test_side_effects_when_saving_approved_to_rejected(self): + # Create an instance of the model + application = completed_application(status=DomainApplication.APPROVED) + domain = Domain.objects.create(name=application.requested_domain.name) + domain_information = DomainInformation.objects.create(creator=self.superuser, domain=domain) + application.approved_domain = domain + application.save() + + # Create a request object with a superuser + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + request.user = self.superuser + + # Define a custom implementation for is_active + def custom_is_active(self): + return False # Override to return False + + # Use ExitStack to combine patch contexts + with ExitStack() as stack: + # Patch Domain.is_active and django.contrib.messages.error simultaneously + stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) + stack.enter_context(patch.object(messages, 'error')) + + # Simulate saving the model + application.status = DomainApplication.REJECTED + self.admin.save_model(request, application, None, True) + + # Assert that the error message was never called + messages.error.assert_not_called() + + self.assertEqual(application.approved_domain, None) + + # Assert that Domain got Deleted + with self.assertRaises(Domain.DoesNotExist): + domain.refresh_from_db() + + # Assert that DomainInformation got Deleted + with self.assertRaises(DomainInformation.DoesNotExist): + domain_information.refresh_from_db() + + def test_error_when_saving_approved_to_ineligible_and_domain_is_active(self): + # Create an instance of the model + application = completed_application(status=DomainApplication.APPROVED) + domain = Domain.objects.create(name=application.requested_domain.name) + application.approved_domain = domain + application.save() + + # Create a request object with a superuser + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + request.user = self.superuser + + # Define a custom implementation for is_active + def custom_is_active(self): + return True # Override to return True + + # Use ExitStack to combine patch contexts + with ExitStack() as stack: + # Patch Domain.is_active and django.contrib.messages.error simultaneously + stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) + stack.enter_context(patch.object(messages, 'error')) + + # Simulate saving the model + application.status = DomainApplication.INELIGIBLE + self.admin.save_model(request, application, None, True) + + # Assert that the error message was called with the correct argument + messages.error.assert_called_once_with( + request, + "This action is not permitted, the domain " + + "is already active.", + ) + + def test_side_effects_when_saving_approved_to_ineligible(self): + # Create an instance of the model + application = completed_application(status=DomainApplication.APPROVED) + domain = Domain.objects.create(name=application.requested_domain.name) + domain_information = DomainInformation.objects.create(creator=self.superuser, domain=domain) + application.approved_domain = domain + application.save() + + # Create a request object with a superuser + request = self.factory.post( + "/admin/registrar/domainapplication/{}/change/".format(application.pk) + ) + request.user = self.superuser + + # Define a custom implementation for is_active + def custom_is_active(self): + return False # Override to return False + + # Use ExitStack to combine patch contexts + with ExitStack() as stack: + # Patch Domain.is_active and django.contrib.messages.error simultaneously + stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) + stack.enter_context(patch.object(messages, 'error')) + + # Simulate saving the model + application.status = DomainApplication.INELIGIBLE + self.admin.save_model(request, application, None, True) + + # Assert that the error message was never called + messages.error.assert_not_called() + + self.assertEqual(application.approved_domain, None) + + # Assert that Domain got Deleted + with self.assertRaises(Domain.DoesNotExist): + domain.refresh_from_db() + + # Assert that DomainInformation got Deleted + with self.assertRaises(DomainInformation.DoesNotExist): + domain_information.refresh_from_db() + def tearDown(self): + Domain.objects.all().delete() DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() User.objects.all().delete() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index ca1191061..a6a6fb3c9 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -1,5 +1,6 @@ from django.test import TestCase from django.db.utils import IntegrityError +from unittest.mock import patch from registrar.models import ( Contact, @@ -439,7 +440,26 @@ class TestDomainApplication(TestCase): application = completed_application(status=DomainApplication.INELIGIBLE) with self.assertRaises(TransitionNotAllowed): - application.reject_with_prejudice() + application.reject() + + def test_transition_not_allowed_approved_rejected_when_domain_is_active(self): + """Create an application with status approved, create a matching domain that + is active, and call reject against transition rules""" + + application = completed_application(status=DomainApplication.APPROVED) + domain = Domain.objects.create(name=application.requested_domain.name) + application.approved_domain = domain + application.save() + + # Define a custom implementation for is_active + def custom_is_active(self): + return True # Override to return True + + # Use patch to temporarily replace is_active with the custom implementation + with patch.object(Domain, 'is_active', custom_is_active): + # Now, when you call is_active on Domain, it will return True + with self.assertRaises(TransitionNotAllowed): + application.reject() def test_transition_not_allowed_started_ineligible(self): """Create an application with status started and call reject @@ -494,6 +514,25 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject_with_prejudice() + + def test_transition_not_allowed_approved_ineligible_when_domain_is_active(self): + """Create an application with status approved, create a matching domain that + is active, and call reject_with_prejudice against transition rules""" + + application = completed_application(status=DomainApplication.APPROVED) + domain = Domain.objects.create(name=application.requested_domain.name) + application.approved_domain = domain + application.save() + + # Define a custom implementation for is_active + def custom_is_active(self): + return True # Override to return True + + # Use patch to temporarily replace is_active with the custom implementation + with patch.object(Domain, 'is_active', custom_is_active): + # Now, when you call is_active on Domain, it will return True + with self.assertRaises(TransitionNotAllowed): + application.reject_with_prejudice() class TestPermissions(TestCase): From 0568928f1a79e235b31a8cca4693828dd8241ebf Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 6 Sep 2023 15:25:17 -0400 Subject: [PATCH 03/26] lint --- src/registrar/models/domain_application.py | 6 +- src/registrar/tests/test_admin.py | 93 +++++++++++----------- src/registrar/tests/test_models.py | 16 ++-- 3 files changed, 58 insertions(+), 57 deletions(-) diff --git a/src/registrar/models/domain_application.py b/src/registrar/models/domain_application.py index db962fda0..14244311d 100644 --- a/src/registrar/models/domain_application.py +++ b/src/registrar/models/domain_application.py @@ -614,8 +614,8 @@ class DomainApplication(TimeStampedModel): def reject(self): """Reject an application that has been submitted. - As a side effect this will delete the domain and domain_information - (will cascade), and send an email notification""" + As side effects this will delete the domain and domain_information + (will cascade), and send an email notification.""" if self.status == self.APPROVED: self.approved_domain.delete_request() @@ -641,7 +641,7 @@ class DomainApplication(TimeStampedModel): any existing domains/applications 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)""" + and domain_information (will cascade) when they exist.""" if self.status == self.APPROVED: self.approved_domain.delete_request() diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 9cde6d5cf..2b347287d 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -1,6 +1,5 @@ from django.test import TestCase, RequestFactory, Client from django.contrib.admin.sites import AdminSite -from unittest.mock import patch from contextlib import ExitStack from django.contrib import messages @@ -436,7 +435,7 @@ class TestDomainApplicationAdmin(TestCase): request, "Cannot edit an application with a restricted creator.", ) - + def test_error_when_saving_approved_to_rejected_and_domain_is_active(self): # Create an instance of the model application = completed_application(status=DomainApplication.APPROVED) @@ -449,33 +448,34 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) request.user = self.superuser - + # Define a custom implementation for is_active def custom_is_active(self): return True # Override to return True - - # Use ExitStack to combine patch contexts + + # Use ExitStack to combine patch contexts with ExitStack() as stack: # Patch Domain.is_active and django.contrib.messages.error simultaneously - stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) - stack.enter_context(patch.object(messages, 'error')) - + stack.enter_context(patch.object(Domain, "is_active", custom_is_active)) + stack.enter_context(patch.object(messages, "error")) + # Simulate saving the model - application.status = DomainApplication.REJECTED + application.status = DomainApplication.REJECTED self.admin.save_model(request, application, None, True) # Assert that the error message was called with the correct argument messages.error.assert_called_once_with( request, - "This action is not permitted, the domain " - + "is already active.", + "This action is not permitted, the domain " + "is already active.", ) - + def test_side_effects_when_saving_approved_to_rejected(self): # Create an instance of the model application = completed_application(status=DomainApplication.APPROVED) domain = Domain.objects.create(name=application.requested_domain.name) - domain_information = DomainInformation.objects.create(creator=self.superuser, domain=domain) + domain_information = DomainInformation.objects.create( + creator=self.superuser, domain=domain + ) application.approved_domain = domain application.save() @@ -484,34 +484,34 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) request.user = self.superuser - + # Define a custom implementation for is_active def custom_is_active(self): return False # Override to return False - - # Use ExitStack to combine patch contexts + + # Use ExitStack to combine patch contexts with ExitStack() as stack: # Patch Domain.is_active and django.contrib.messages.error simultaneously - stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) - stack.enter_context(patch.object(messages, 'error')) - + stack.enter_context(patch.object(Domain, "is_active", custom_is_active)) + stack.enter_context(patch.object(messages, "error")) + # Simulate saving the model - application.status = DomainApplication.REJECTED + application.status = DomainApplication.REJECTED self.admin.save_model(request, application, None, True) # Assert that the error message was never called messages.error.assert_not_called() - + self.assertEqual(application.approved_domain, None) - + # Assert that Domain got Deleted with self.assertRaises(Domain.DoesNotExist): domain.refresh_from_db() - + # Assert that DomainInformation got Deleted with self.assertRaises(DomainInformation.DoesNotExist): domain_information.refresh_from_db() - + def test_error_when_saving_approved_to_ineligible_and_domain_is_active(self): # Create an instance of the model application = completed_application(status=DomainApplication.APPROVED) @@ -524,33 +524,34 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) request.user = self.superuser - + # Define a custom implementation for is_active def custom_is_active(self): return True # Override to return True - - # Use ExitStack to combine patch contexts + + # Use ExitStack to combine patch contexts with ExitStack() as stack: # Patch Domain.is_active and django.contrib.messages.error simultaneously - stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) - stack.enter_context(patch.object(messages, 'error')) - + stack.enter_context(patch.object(Domain, "is_active", custom_is_active)) + stack.enter_context(patch.object(messages, "error")) + # Simulate saving the model - application.status = DomainApplication.INELIGIBLE + application.status = DomainApplication.INELIGIBLE self.admin.save_model(request, application, None, True) # Assert that the error message was called with the correct argument messages.error.assert_called_once_with( request, - "This action is not permitted, the domain " - + "is already active.", + "This action is not permitted, the domain " + "is already active.", ) - + def test_side_effects_when_saving_approved_to_ineligible(self): # Create an instance of the model application = completed_application(status=DomainApplication.APPROVED) domain = Domain.objects.create(name=application.requested_domain.name) - domain_information = DomainInformation.objects.create(creator=self.superuser, domain=domain) + domain_information = DomainInformation.objects.create( + creator=self.superuser, domain=domain + ) application.approved_domain = domain application.save() @@ -559,34 +560,34 @@ class TestDomainApplicationAdmin(TestCase): "/admin/registrar/domainapplication/{}/change/".format(application.pk) ) request.user = self.superuser - + # Define a custom implementation for is_active def custom_is_active(self): return False # Override to return False - - # Use ExitStack to combine patch contexts + + # Use ExitStack to combine patch contexts with ExitStack() as stack: # Patch Domain.is_active and django.contrib.messages.error simultaneously - stack.enter_context(patch.object(Domain, 'is_active', custom_is_active)) - stack.enter_context(patch.object(messages, 'error')) - + stack.enter_context(patch.object(Domain, "is_active", custom_is_active)) + stack.enter_context(patch.object(messages, "error")) + # Simulate saving the model - application.status = DomainApplication.INELIGIBLE + application.status = DomainApplication.INELIGIBLE self.admin.save_model(request, application, None, True) # Assert that the error message was never called messages.error.assert_not_called() - + self.assertEqual(application.approved_domain, None) - + # Assert that Domain got Deleted with self.assertRaises(Domain.DoesNotExist): domain.refresh_from_db() - + # Assert that DomainInformation got Deleted with self.assertRaises(DomainInformation.DoesNotExist): domain_information.refresh_from_db() - + def tearDown(self): Domain.objects.all().delete() DomainInformation.objects.all().delete() diff --git a/src/registrar/tests/test_models.py b/src/registrar/tests/test_models.py index a6a6fb3c9..2c6f78ef5 100644 --- a/src/registrar/tests/test_models.py +++ b/src/registrar/tests/test_models.py @@ -441,7 +441,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject() - + def test_transition_not_allowed_approved_rejected_when_domain_is_active(self): """Create an application with status approved, create a matching domain that is active, and call reject against transition rules""" @@ -450,13 +450,13 @@ class TestDomainApplication(TestCase): domain = Domain.objects.create(name=application.requested_domain.name) application.approved_domain = domain application.save() - + # Define a custom implementation for is_active def custom_is_active(self): return True # Override to return True - + # Use patch to temporarily replace is_active with the custom implementation - with patch.object(Domain, 'is_active', custom_is_active): + with patch.object(Domain, "is_active", custom_is_active): # Now, when you call is_active on Domain, it will return True with self.assertRaises(TransitionNotAllowed): application.reject() @@ -514,7 +514,7 @@ class TestDomainApplication(TestCase): with self.assertRaises(TransitionNotAllowed): application.reject_with_prejudice() - + def test_transition_not_allowed_approved_ineligible_when_domain_is_active(self): """Create an application with status approved, create a matching domain that is active, and call reject_with_prejudice against transition rules""" @@ -523,13 +523,13 @@ class TestDomainApplication(TestCase): domain = Domain.objects.create(name=application.requested_domain.name) application.approved_domain = domain application.save() - + # Define a custom implementation for is_active def custom_is_active(self): return True # Override to return True - + # Use patch to temporarily replace is_active with the custom implementation - with patch.object(Domain, 'is_active', custom_is_active): + with patch.object(Domain, "is_active", custom_is_active): # Now, when you call is_active on Domain, it will return True with self.assertRaises(TransitionNotAllowed): application.reject_with_prejudice() From 37aea7d62a78a5d05e5b0dbef945083e67ff7a17 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 6 Sep 2023 16:31:03 -0400 Subject: [PATCH 04/26] migrations --- ...ainapplication_approved_domain_and_more.py | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py diff --git a/src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py b/src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py new file mode 100644 index 000000000..3a5957686 --- /dev/null +++ b/src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py @@ -0,0 +1,37 @@ +# Generated by Django 4.2.1 on 2023-09-06 20:30 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + dependencies = [ + ("registrar", "0030_alter_user_status"), + ] + + operations = [ + 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_application", + to="registrar.domain", + ), + ), + migrations.AlterField( + model_name="domaininformation", + name="domain", + field=models.OneToOneField( + blank=True, + help_text="Domain to which this information belongs", + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="domain_info", + to="registrar.domain", + ), + ), + ] From 0ed550b327e6a3da67b855af8b481b188d8e071d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 7 Sep 2023 14:04:20 -0400 Subject: [PATCH 05/26] lint --- src/registrar/tests/test_admin.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 2bf12b48c..1c440ff70 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -17,7 +17,6 @@ from registrar.models import ( Domain, User, DomainInvitation, - Domain, ) from .common import ( completed_application, From 494c5161426cec63025955e4363902607fbdac40 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 7 Sep 2023 14:54:19 -0400 Subject: [PATCH 06/26] Set perms on staff fixtures to be able to change the user status --- src/registrar/admin.py | 52 +++++++++++++++++++++++++++++++++++++-- src/registrar/fixtures.py | 2 +- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 5ccba7cad..dc7e45895 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -134,10 +134,51 @@ class MyUserAdmin(BaseUserAdmin): ("Important dates", {"fields": ("last_login", "date_joined")}), ) + analyst_fieldsets = ( + ( + None, + {"fields": ("password", "status")}, + ), + ("Personal Info", {"fields": ("first_name", "last_name", "email")}), + ( + "Permissions", + { + "fields": ( + "is_active", + "is_staff", + "is_superuser", + ) + }, + ), + ("Important dates", {"fields": ("last_login", "date_joined")}), + ) + + analyst_readonly_fields = [ + "password", + "Personal Info", + "first_name", + "last_name", + "email", + "Permissions", + "is_active", + "is_staff", + "is_superuser", + "Important dates", + "last_login", + "date_joined", + ] + def get_list_display(self, request): if not request.user.is_superuser: # Customize the list display for staff users - return ("email", "first_name", "last_name", "is_staff", "is_superuser") + return ( + "email", + "first_name", + "last_name", + "is_staff", + "is_superuser", + "status", + ) # Use the default list display for non-staff users return super().get_list_display(request) @@ -146,11 +187,18 @@ class MyUserAdmin(BaseUserAdmin): if not request.user.is_superuser: # If the user doesn't have permission to change the model, # show a read-only fieldset - return ((None, {"fields": []}),) + return self.analyst_fieldsets # If the user has permission to change the model, show all fields return super().get_fieldsets(request, obj) + def get_readonly_fields(self, request, obj=None): + if request.user.is_superuser: + return () # No read-only fields for superusers + elif request.user.is_staff: + return self.analyst_readonly_fields # Read-only fields for staff + return () # No read-only fields for other users + class HostIPInline(admin.StackedInline): diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index 76b01abf7..63ef1dea9 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -138,7 +138,7 @@ class UserFixture: "permissions": ["change_domainapplication"], }, {"app_label": "registrar", "model": "domain", "permissions": ["view_domain"]}, - {"app_label": "registrar", "model": "user", "permissions": ["view_user"]}, + {"app_label": "registrar", "model": "user", "permissions": ["change_user"]}, ] @classmethod From e14ac58cd7891143a7b5ace22ff5965a422c2ee3 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Thu, 7 Sep 2023 15:32:45 -0400 Subject: [PATCH 07/26] lint and tests for the staff perm changes --- src/registrar/tests/test_admin.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 1c440ff70..e23eafc84 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -689,6 +689,7 @@ class MyUserAdminTest(TestCase): "last_name", "is_staff", "is_superuser", + "status", ) self.assertEqual(list_display, expected_list_display) @@ -705,7 +706,12 @@ class MyUserAdminTest(TestCase): request = self.client.request().wsgi_request request.user = create_user() fieldsets = self.admin.get_fieldsets(request) - expected_fieldsets = ((None, {"fields": []}),) + expected_fieldsets = ( + (None, {"fields": ("password", "status")}), + ("Personal Info", {"fields": ("first_name", "last_name", "email")}), + ("Permissions", {"fields": ("is_active", "is_staff", "is_superuser")}), + ("Important dates", {"fields": ("last_login", "date_joined")}), + ) self.assertEqual(fieldsets, expected_fieldsets) def tearDown(self): From 08c50fba63101f6562100e210bb446df932e414d Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 8 Sep 2023 16:56:56 -0400 Subject: [PATCH 08/26] Copy edits on the error msg --- src/registrar/admin.py | 2 +- src/registrar/tests/test_admin.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index dc7e45895..21dbd7f08 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -405,7 +405,7 @@ class DomainApplicationAdmin(ListHeaderAdmin): messages.error( request, - "This action is not permitted, the domain " + "This action is not permitted. The domain " + "is already active.", ) diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index e23eafc84..82568781e 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -469,7 +469,7 @@ class TestDomainApplicationAdmin(TestCase): # Assert that the error message was called with the correct argument messages.error.assert_called_once_with( request, - "This action is not permitted, the domain " + "is already active.", + "This action is not permitted. The domain " + "is already active.", ) def test_side_effects_when_saving_approved_to_rejected(self): @@ -545,7 +545,7 @@ class TestDomainApplicationAdmin(TestCase): # Assert that the error message was called with the correct argument messages.error.assert_called_once_with( request, - "This action is not permitted, the domain " + "is already active.", + "This action is not permitted. The domain " + "is already active.", ) def test_side_effects_when_saving_approved_to_ineligible(self): From 50e1a484d5ac05a6fce6cce171a00d86d7c82484 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 11 Sep 2023 14:23:16 -0400 Subject: [PATCH 09/26] change stubbed is_active to check against Domain.State.CREATED --- src/registrar/models/domain.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 4988ae36b..15aac3cd9 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -301,9 +301,7 @@ class Domain(TimeStampedModel, DomainHelper): # TODO: implement a check -- should be performant so it can be called for # any number of domains on a status page # this is NOT as simple as checking if Domain.Status.OK is in self.statuses - - # NOTE: This was stubbed in all along for 852 and 811 - return False + return self.state == Domain.State.CREATED def delete_request(self): """Delete from host. Possibly a duplicate of _delete_host?""" From e4a2c6d03028ba4e3fef29ec60a7ce1257e1904d Mon Sep 17 00:00:00 2001 From: zandercymatics <141044360+zandercymatics@users.noreply.github.com> Date: Wed, 13 Sep 2023 10:53:07 -0600 Subject: [PATCH 10/26] Fix sorting bug --- src/registrar/templates/domain_users.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/registrar/templates/domain_users.html b/src/registrar/templates/domain_users.html index 09d391dc8..22b9d18d1 100644 --- a/src/registrar/templates/domain_users.html +++ b/src/registrar/templates/domain_users.html @@ -20,7 +20,7 @@ {% for permission in domain.permissions.all %} - + {{ permission.user.email }} {{ permission.role|title }} @@ -57,7 +57,7 @@ {% for invitation in domain.invitations.all %} - + {{ invitation.email }} {{ invitation.created_at|date }} From 33106895192fca9cbbd82470982d175085ca5154 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 13 Sep 2023 16:20:14 -0400 Subject: [PATCH 11/26] Add DraftDomain to superusers and staff, Add filter to domains (state), Update user table definition to show first, last AND email by default, Use inline editors to expose DomainInformation, Make Domain Information fields available to CISA Analyst --- src/registrar/admin.py | 293 +++++++++++++++++++++++------------ src/registrar/fixtures.py | 10 ++ src/registrar/models/user.py | 2 +- 3 files changed, 204 insertions(+), 101 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 78b19191e..6bad9d9d0 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -189,102 +189,6 @@ class MyHostAdmin(AuditedAdmin): inlines = [HostIPInline] -class DomainAdmin(ListHeaderAdmin): - """Custom domain admin class to add extra buttons.""" - - # Columns - list_display = [ - "name", - "organization_type", - "state", - ] - - def organization_type(self, obj): - return obj.domain_info.organization_type - - organization_type.admin_order_field = ( # type: ignore - "domain_info__organization_type" - ) - - # Filters - list_filter = ["domain_info__organization_type"] - - search_fields = ["name"] - search_help_text = "Search by domain name." - change_form_template = "django/admin/domain_change_form.html" - readonly_fields = ["state"] - - def response_change(self, request, obj): - # Create dictionary of action functions - ACTION_FUNCTIONS = { - "_place_client_hold": self.do_place_client_hold, - "_remove_client_hold": self.do_remove_client_hold, - "_edit_domain": self.do_edit_domain, - } - - # Check which action button was pressed and call the corresponding function - for action, function in ACTION_FUNCTIONS.items(): - if action in request.POST: - return function(request, obj) - - # If no matching action button is found, return the super method - return super().response_change(request, obj) - - def do_place_client_hold(self, request, obj): - try: - obj.place_client_hold() - obj.save() - except Exception as err: - self.message_user(request, err, messages.ERROR) - else: - self.message_user( - request, - ( - "%s is in client hold. This domain is no longer accessible on" - " the public internet." - ) - % obj.name, - ) - return HttpResponseRedirect(".") - - def do_remove_client_hold(self, request, obj): - try: - obj.remove_client_hold() - obj.save() - except Exception as err: - self.message_user(request, err, messages.ERROR) - else: - self.message_user( - request, - ("%s is ready. This domain is accessible on the public internet.") - % obj.name, - ) - return HttpResponseRedirect(".") - - def do_edit_domain(self, request, obj): - # We want to know, globally, when an edit action occurs - request.session["analyst_action"] = "edit" - # Restricts this action to this domain (pk) only - request.session["analyst_action_location"] = obj.id - return HttpResponseRedirect(reverse("domain", args=(obj.id,))) - - def change_view(self, request, object_id): - # If the analyst was recently editing a domain page, - # delete any associated session values - if "analyst_action" in request.session: - del request.session["analyst_action"] - del request.session["analyst_action_location"] - return super().change_view(request, object_id) - - def has_change_permission(self, request, obj=None): - # Fixes a bug wherein users which are only is_staff - # can access 'change' when GET, - # but cannot access this page when it is a request of type POST. - if request.user.is_staff: - return True - return super().has_change_permission(request, obj) - - class ContactAdmin(ListHeaderAdmin): """Custom contact admin class to add search.""" @@ -380,6 +284,84 @@ class DomainInformationAdmin(ListHeaderAdmin): ] search_help_text = "Search by domain." + # TODO: on merging with 446, replace type_of_work with + # about_your_organization and remove more_organization_information + fieldsets = [ + (None, {"fields": ["creator", "domain_application"]}), + ( + "Type of organization", + { + "fields": [ + "organization_type", + "federally_recognized_tribe", + "state_recognized_tribe", + "tribe_name", + "federal_agency", + "federal_type", + "is_election_board", + "type_of_work", + "more_organization_information", + ] + }, + ), + ( + "Organization name and mailing address", + { + "fields": [ + "organization_name", + "address_line1", + "address_line2", + "city", + "state_territory", + "zipcode", + "urbanization", + ] + }, + ), + ("Authorizing official", {"fields": ["authorizing_official"]}), + (".gov domain", {"fields": ["domain"]}), + ("Your contact information", {"fields": ["submitter"]}), + ("Other employees from your organization?", {"fields": ["other_contacts"]}), + ( + "No other employees from your organization?", + {"fields": ["no_other_contacts_rationale"]}, + ), + ("Anything else we should know?", {"fields": ["anything_else"]}), + ( + "Requirements for operating .gov domains", + {"fields": ["is_policy_acknowledged"]}, + ), + ] + + # Read only that we'll leverage for CISA Analysts + analyst_readonly_fields = [ + "creator", + "type_of_work", + "more_organization_information", + "address_line1", + "address_line2", + "zipcode", + "domain", + "submitter", + "no_other_contacts_rationale", + "anything_else", + "is_policy_acknowledged", + ] + + def get_readonly_fields(self, request, obj=None): + """Set the read-only state on form elements. + We have 1 conditions that determine which fields are read-only: + admin user permissions. + """ + + readonly_fields = list(self.readonly_fields) + + if request.user.is_superuser: + return readonly_fields + else: + readonly_fields.extend([field for field in self.analyst_readonly_fields]) + return readonly_fields + class DomainApplicationAdminForm(forms.ModelForm): """Custom form to limit transitions to available transitions""" @@ -416,10 +398,6 @@ class DomainApplicationAdmin(ListHeaderAdmin): """Custom domain applications admin class.""" - # Set multi-selects 'read-only' (hide selects and show data) - # based on user perms and application creator's status - # form = DomainApplicationForm - # Columns list_display = [ "requested_domain", @@ -589,6 +567,120 @@ class DomainApplicationAdmin(ListHeaderAdmin): return super().change_view(request, object_id, form_url, extra_context) +class DomainInformationInline(admin.StackedInline): + """Edit a domain information on the domain page. + We had issues inheriting from both StackedInline + and the source DomainInformationAdmin since these + classes conflict, so we'll just pull what we need + from DomainInformationAdmin""" + + model = models.DomainInformation + + fieldsets = DomainInformationAdmin.fieldsets + analyst_readonly_fields = DomainInformationAdmin.analyst_readonly_fields + + def get_readonly_fields(self, request, obj=None): + return DomainInformationAdmin.get_readonly_fields(self, request, obj=None) + + +class DomainAdmin(ListHeaderAdmin): + """Custom domain admin class to add extra buttons.""" + + inlines = [DomainInformationInline] + + # Columns + list_display = [ + "name", + "organization_type", + "state", + ] + + def organization_type(self, obj): + return obj.domain_info.organization_type + + organization_type.admin_order_field = ( # type: ignore + "domain_info__organization_type" + ) + + # Filters + list_filter = ["domain_info__organization_type", "state"] + + search_fields = ["name"] + search_help_text = "Search by domain name." + change_form_template = "django/admin/domain_change_form.html" + readonly_fields = ["state"] + + def response_change(self, request, obj): + # Create dictionary of action functions + ACTION_FUNCTIONS = { + "_place_client_hold": self.do_place_client_hold, + "_remove_client_hold": self.do_remove_client_hold, + "_edit_domain": self.do_edit_domain, + } + + # Check which action button was pressed and call the corresponding function + for action, function in ACTION_FUNCTIONS.items(): + if action in request.POST: + return function(request, obj) + + # If no matching action button is found, return the super method + return super().response_change(request, obj) + + def do_place_client_hold(self, request, obj): + try: + obj.place_client_hold() + obj.save() + except Exception as err: + self.message_user(request, err, messages.ERROR) + else: + self.message_user( + request, + ( + "%s is in client hold. This domain is no longer accessible on" + " the public internet." + ) + % obj.name, + ) + return HttpResponseRedirect(".") + + def do_remove_client_hold(self, request, obj): + try: + obj.remove_client_hold() + obj.save() + except Exception as err: + self.message_user(request, err, messages.ERROR) + else: + self.message_user( + request, + ("%s is ready. This domain is accessible on the public internet.") + % obj.name, + ) + return HttpResponseRedirect(".") + + def do_edit_domain(self, request, obj): + # We want to know, globally, when an edit action occurs + request.session["analyst_action"] = "edit" + # Restricts this action to this domain (pk) only + request.session["analyst_action_location"] = obj.id + return HttpResponseRedirect(reverse("domain", args=(obj.id,))) + + def change_view(self, request, object_id): + # If the analyst was recently editing a domain page, + # delete any associated session values + if "analyst_action" in request.session: + del request.session["analyst_action"] + del request.session["analyst_action_location"] + return super().change_view(request, object_id) + + def has_change_permission(self, request, obj=None): + # Fixes a bug wherein users which are only is_staff + # can access 'change' when GET, + # but cannot access this page when it is a request of type POST. + if request.user.is_staff: + return True + return super().has_change_permission(request, obj) + + admin.site.unregister(LogEntry) # Unregister the default registration admin.site.register(LogEntry, CustomLogEntryAdmin) admin.site.register(models.User, MyUserAdmin) @@ -597,6 +689,7 @@ admin.site.register(models.Contact, ContactAdmin) admin.site.register(models.DomainInvitation, DomainInvitationAdmin) admin.site.register(models.DomainInformation, DomainInformationAdmin) admin.site.register(models.Domain, DomainAdmin) +admin.site.register(models.DraftDomain, ListHeaderAdmin) admin.site.register(models.Host, MyHostAdmin) admin.site.register(models.Nameserver, MyHostAdmin) admin.site.register(models.Website, WebsiteAdmin) diff --git a/src/registrar/fixtures.py b/src/registrar/fixtures.py index 30924b8bf..b5c25a170 100644 --- a/src/registrar/fixtures.py +++ b/src/registrar/fixtures.py @@ -143,12 +143,22 @@ class UserFixture: "permissions": ["view_logentry"], }, {"app_label": "registrar", "model": "contact", "permissions": ["view_contact"]}, + { + "app_label": "registrar", + "model": "domaininformation", + "permissions": ["change_domaininformation"], + }, { "app_label": "registrar", "model": "domainapplication", "permissions": ["change_domainapplication"], }, {"app_label": "registrar", "model": "domain", "permissions": ["view_domain"]}, + { + "app_label": "registrar", + "model": "draftdomain", + "permissions": ["change_draftdomain"], + }, {"app_label": "registrar", "model": "user", "permissions": ["view_user"]}, ] diff --git a/src/registrar/models/user.py b/src/registrar/models/user.py index 5cf1dd71f..5b04c628d 100644 --- a/src/registrar/models/user.py +++ b/src/registrar/models/user.py @@ -45,7 +45,7 @@ class User(AbstractUser): def __str__(self): # this info is pulled from Login.gov if self.first_name or self.last_name: - return f"{self.first_name or ''} {self.last_name or ''}" + return f"{self.first_name or ''} {self.last_name or ''} {self.email or ''}" elif self.email: return self.email else: From 9f3a8d0a9d784f108abbf678fcbea930a5d1e379 Mon Sep 17 00:00:00 2001 From: rachidatecs <107004823+rachidatecs@users.noreply.github.com> Date: Thu, 14 Sep 2023 11:36:45 -0400 Subject: [PATCH 12/26] Update src/registrar/models/domain.py Co-authored-by: Alysia Broddrick <109625347+abroddrick@users.noreply.github.com> --- src/registrar/models/domain.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 15aac3cd9..2159bee1c 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -305,6 +305,7 @@ class Domain(TimeStampedModel, DomainHelper): def delete_request(self): """Delete from host. Possibly a duplicate of _delete_host?""" + # TODO fix in ticket #901 pass def transfer(self): From ea036eac1b9445d012680051ed9b72ffb844bd3b Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 15 Sep 2023 17:11:47 -0400 Subject: [PATCH 13/26] fix migrations --- ...ainapplication_approved_domain_and_more.py | 37 ------------------- .../0031_transitiondomain_and_more.py | 27 +++++++++++++- 2 files changed, 26 insertions(+), 38 deletions(-) delete mode 100644 src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py diff --git a/src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py b/src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py deleted file mode 100644 index 3a5957686..000000000 --- a/src/registrar/migrations/0031_alter_domainapplication_approved_domain_and_more.py +++ /dev/null @@ -1,37 +0,0 @@ -# Generated by Django 4.2.1 on 2023-09-06 20:30 - -from django.db import migrations, models -import django.db.models.deletion - - -class Migration(migrations.Migration): - dependencies = [ - ("registrar", "0030_alter_user_status"), - ] - - operations = [ - 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_application", - to="registrar.domain", - ), - ), - migrations.AlterField( - model_name="domaininformation", - name="domain", - field=models.OneToOneField( - blank=True, - help_text="Domain to which this information belongs", - null=True, - on_delete=django.db.models.deletion.CASCADE, - related_name="domain_info", - to="registrar.domain", - ), - ), - ] diff --git a/src/registrar/migrations/0031_transitiondomain_and_more.py b/src/registrar/migrations/0031_transitiondomain_and_more.py index 79bf7eab4..b51fc27d3 100644 --- a/src/registrar/migrations/0031_transitiondomain_and_more.py +++ b/src/registrar/migrations/0031_transitiondomain_and_more.py @@ -1,6 +1,7 @@ -# Generated by Django 4.2.1 on 2023-09-13 22:25 +# Generated by Django 4.2.1 on 2023-09-15 21:05 from django.db import migrations, models +import django.db.models.deletion import django_fsm @@ -105,6 +106,30 @@ class Migration(migrations.Migration): protected=True, ), ), + 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_application", + to="registrar.domain", + ), + ), + migrations.AlterField( + model_name="domaininformation", + name="domain", + field=models.OneToOneField( + blank=True, + help_text="Domain to which this information belongs", + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="domain_info", + to="registrar.domain", + ), + ), migrations.AlterField( model_name="publiccontact", name="contact_type", From e3d59e0e62c80e7a26e76199307dbc6ff6a83bd7 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 15 Sep 2023 17:34:14 -0400 Subject: [PATCH 14/26] edit latest migration --- .../0031_transitiondomain_and_more.py | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/src/registrar/migrations/0031_transitiondomain_and_more.py b/src/registrar/migrations/0031_transitiondomain_and_more.py index b51fc27d3..e378a33de 100644 --- a/src/registrar/migrations/0031_transitiondomain_and_more.py +++ b/src/registrar/migrations/0031_transitiondomain_and_more.py @@ -11,6 +11,23 @@ class Migration(migrations.Migration): ] operations = [ + migrations.AlterField( + model_name="domain", + name="state", + field=django_fsm.FSMField( + choices=[ + ("unknown", "Unknown"), + ("dns needed", "Dns Needed"), + ("ready", "Ready"), + ("on hold", "On Hold"), + ("deleted", "Deleted"), + ], + default="unknown", + help_text="Very basic info about the lifecycle of this domain object", + max_length=21, + protected=True, + ), + ), migrations.CreateModel( name="TransitionDomain", fields=[ @@ -89,23 +106,6 @@ class Migration(migrations.Migration): blank=True, help_text="Information about your organization", null=True ), ), - migrations.AlterField( - model_name="domain", - name="state", - field=django_fsm.FSMField( - choices=[ - ("unknown", "Unknown"), - ("dns needed", "Dns Needed"), - ("ready", "Ready"), - ("on hold", "On Hold"), - ("deleted", "Deleted"), - ], - default="unknown", - help_text="Very basic info about the lifecycle of this domain object", - max_length=21, - protected=True, - ), - ), migrations.AlterField( model_name="domainapplication", name="approved_domain", From 229b29066c0043aa71d811fa76dfaa39e5dfe9a9 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Fri, 15 Sep 2023 17:49:54 -0400 Subject: [PATCH 15/26] lint --- src/registrar/admin.py | 7 ------- src/registrar/tests/test_admin.py | 1 - 2 files changed, 8 deletions(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index b781d5f9c..56f1c093a 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -488,13 +488,6 @@ class DomainApplicationAdminForm(forms.ModelForm): self.fields["status"].widget.choices = available_transitions -class DomainInformationAdmin(ListHeaderAdmin): - """Custom contact admin class to add search.""" - - search_fields = ["domain__name"] - search_help_text = "Search by domain name." - - class DomainApplicationAdmin(ListHeaderAdmin): """Custom domain applications admin class.""" diff --git a/src/registrar/tests/test_admin.py b/src/registrar/tests/test_admin.py index 54a03fd7b..9ff9ce451 100644 --- a/src/registrar/tests/test_admin.py +++ b/src/registrar/tests/test_admin.py @@ -16,7 +16,6 @@ from registrar.models import ( Domain, DomainApplication, DomainInformation, - Domain, User, DomainInvitation, ) From 68012389a8c2bb9e936847272e54ac08a2c4f960 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 18 Sep 2023 15:21:57 -0400 Subject: [PATCH 16/26] fix typo in exception message --- src/registrar/models/domain.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index b0bf00082..dda295a60 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -341,7 +341,7 @@ class Domain(TimeStampedModel, DomainHelper): if "statuses" not in self._cache: self._fetch_cache() if "statuses" not in self._cache: - raise Exception("Can't retreive status from domain info") + raise Exception("Can't retrieve status from domain info") else: return self._cache["statuses"] From 76a2a1a6fd2be1422b89973241910a439843e18a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 13:32:57 -0400 Subject: [PATCH 17/26] refactor status getter, add unit test --- src/registrar/models/domain.py | 11 +++---- src/registrar/tests/common.py | 4 ++- src/registrar/tests/test_models_domain.py | 40 ++++++++++++++++------- 3 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index dda295a60..c20e92b63 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -336,14 +336,11 @@ class Domain(TimeStampedModel, DomainHelper): A domain's status indicates various properties. See Domain.Status. """ - # implementation note: the Status object from EPP stores the string in - # a dataclass property `state`, not to be confused with the `state` field here if "statuses" not in self._cache: - self._fetch_cache() - if "statuses" not in self._cache: - raise Exception("Can't retrieve status from domain info") - else: - return self._cache["statuses"] + try: + return self._get_property("statuses") + except KeyError: + logger.error("Can't retrieve status from domain info") @statuses.setter # type: ignore def statuses(self, statuses: list[str]): diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index e21431321..d9fdc0b52 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -547,17 +547,19 @@ class MockEppLib(TestCase): class fakedEppObject(object): """""" - def __init__(self, auth_info=..., cr_date=..., contacts=..., hosts=...): + def __init__(self, auth_info=..., cr_date=..., contacts=..., hosts=..., statuses=...,): self.auth_info = auth_info self.cr_date = cr_date self.contacts = contacts self.hosts = hosts + self.statuses = statuses mockDataInfoDomain = fakedEppObject( "fakepw", cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), contacts=[common.DomainContact(contact="123", type="security")], hosts=["fake.host.com"], + statuses=[common.Status(state='serverTransferProhibited', description=None, lang='en'), common.Status(state='inactive', description=None, lang='en')], ) infoDomainNoContact = fakedEppObject( "security", diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 9aaac7321..02bb04e26 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -34,6 +34,8 @@ class TestDomainCache(MockEppLib): # (see InfoDomainResult) self.assertEquals(domain._cache["auth_info"], self.mockDataInfoDomain.auth_info) self.assertEquals(domain._cache["cr_date"], self.mockDataInfoDomain.cr_date) + status_list = [status.state for status in self.mockDataInfoDomain.statuses] + self.assertEquals(domain._cache["statuses"], status_list) self.assertFalse("avail" in domain._cache.keys()) # using a setter should clear the cache @@ -49,7 +51,8 @@ class TestDomainCache(MockEppLib): ), call(commands.InfoContact(id="123", auth_info=None), cleaned=True), call(commands.InfoHost(name="fake.host.com"), cleaned=True), - ] + ], + any_order=False # Ensure calls are in the specified order ) def test_cache_used_when_avail(self): @@ -168,22 +171,37 @@ class TestDomainCreation(TestCase): with self.assertRaisesRegex(IntegrityError, "name"): Domain.objects.create(name="igorville.gov") - @skip("cannot activate a domain without mock registry") - def test_get_status(self): - """Returns proper status based on `state`.""" - domain = Domain.objects.create(name="igorville.gov") - domain.save() - self.assertEqual(None, domain.status) - domain.activate() - domain.save() - self.assertIn("ok", domain.status) - def tearDown(self) -> None: DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() Domain.objects.all().delete() +class TestDomainStatuses(MockEppLib): + + def test_get_status(self): + """Getter returns statuses""" + domain, _ = Domain.objects.get_or_create(name="igorville2.gov") + _ = domain.statuses + status_list = [status.state for status in self.mockDataInfoDomain.statuses] + self.assertEquals(domain._cache["statuses"], status_list) + + # print(self.mockedSendFunction.call_args_list) + + # Called in _fetch_cache + self.mockedSendFunction.assert_has_calls( + [ + call(commands.InfoDomain(name='igorville2.gov', auth_info=None), cleaned=True), + call(commands.InfoContact(id='123', auth_info=None), cleaned=True), + call(commands.InfoHost(name='fake.host.com'), cleaned=True) + ], + any_order=False # Ensure calls are in the specified order + ) + + def tearDown(self) -> None: + Domain.objects.all().delete() + + class TestRegistrantContacts(MockEppLib): """Rule: Registrants may modify their WHOIS data""" From 03e7e9d67dc0a1e3d57f557575cbe60b9f644b2f Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 14:17:17 -0400 Subject: [PATCH 18/26] use public_sire_url helper in application_wizard error alert --- src/registrar/forms/application_wizard.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/registrar/forms/application_wizard.py b/src/registrar/forms/application_wizard.py index 93ec18aad..cfadb7f8b 100644 --- a/src/registrar/forms/application_wizard.py +++ b/src/registrar/forms/application_wizard.py @@ -12,6 +12,7 @@ from django.utils.safestring import mark_safe from api.views import DOMAIN_API_MESSAGES from registrar.models import Contact, DomainApplication, DraftDomain, Domain +from registrar.templatetags.url_helpers import public_site_url from registrar.utility import errors logger = logging.getLogger(__name__) @@ -181,7 +182,6 @@ class TribalGovernmentForm(RegistrarForm): self.cleaned_data["federally_recognized_tribe"] or self.cleaned_data["state_recognized_tribe"] ): - todo_url = reverse("todo") raise forms.ValidationError( # no sec because we are using it to include an internal URL # into a link. There should be no user-facing input in the @@ -190,10 +190,10 @@ class TribalGovernmentForm(RegistrarForm): "You can’t complete this application yet. " "Only tribes recognized by the U.S. federal government " "or by a U.S. state government are eligible for .gov " - 'domains. Please use our contact form to ' + 'domains. Use our contact form to ' "tell us more about your tribe and why you want a .gov " "domain. We’ll review your information and get back " - "to you.".format(todo_url) + "to you.".format(public_site_url('contact')) ), code="invalid", ) From 01bcde2586028c277aa455a2942d0394f872e37a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 14:33:57 -0400 Subject: [PATCH 19/26] lint --- src/registrar/forms/application_wizard.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/registrar/forms/application_wizard.py b/src/registrar/forms/application_wizard.py index cfadb7f8b..fc50455b6 100644 --- a/src/registrar/forms/application_wizard.py +++ b/src/registrar/forms/application_wizard.py @@ -6,7 +6,6 @@ from phonenumber_field.formfields import PhoneNumberField # type: ignore from django import forms from django.core.validators import RegexValidator, MaxLengthValidator -from django.urls import reverse from django.utils.safestring import mark_safe from api.views import DOMAIN_API_MESSAGES From f4f89ef7054f571225f6a038e839ed194a384684 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 14:38:21 -0400 Subject: [PATCH 20/26] linting --- src/registrar/forms/application_wizard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/forms/application_wizard.py b/src/registrar/forms/application_wizard.py index fc50455b6..516683247 100644 --- a/src/registrar/forms/application_wizard.py +++ b/src/registrar/forms/application_wizard.py @@ -192,7 +192,7 @@ class TribalGovernmentForm(RegistrarForm): 'domains. Use our contact form to ' "tell us more about your tribe and why you want a .gov " "domain. We’ll review your information and get back " - "to you.".format(public_site_url('contact')) + "to you.".format(public_site_url("contact")) ), code="invalid", ) From d0effe6018f5d96c255e346f2b21d0b2b7b0427a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 14:47:43 -0400 Subject: [PATCH 21/26] lint --- src/registrar/admin.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index 4df2401c3..cbbbe1077 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -702,7 +702,7 @@ class DomainAdmin(ListHeaderAdmin): # If no matching action button is found, return the super method return super().response_change(request, obj) - + def do_delete_domain(self, request, obj): try: obj.deleted() From 425045783af1d8bd3617bba950ee972a4ede2532 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 14:50:58 -0400 Subject: [PATCH 22/26] replace type_of_work with # about_your_organization and remove more_organization_information --- 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 cbbbe1077..d0f5c0dbf 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -333,8 +333,6 @@ class DomainInformationAdmin(ListHeaderAdmin): ] search_help_text = "Search by domain." - # TODO: on merging with 446, replace type_of_work with - # about_your_organization and remove more_organization_information fieldsets = [ (None, {"fields": ["creator", "domain_application"]}), ( @@ -348,8 +346,7 @@ class DomainInformationAdmin(ListHeaderAdmin): "federal_agency", "federal_type", "is_election_board", - "type_of_work", - "more_organization_information", + "about_your_organization", ] }, ), From 021f71f2f5b2b4418499429fbe3ece114806d155 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 17:46:47 -0400 Subject: [PATCH 23/26] clean up refactor, refactor an unclosed patch in a test that was causing subsequent tests to fail, lint --- src/registrar/models/domain.py | 12 +++--- src/registrar/tests/common.py | 16 +++++++- src/registrar/tests/test_models_domain.py | 49 ++++++++++++----------- 3 files changed, 46 insertions(+), 31 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index c20e92b63..440bed0aa 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -336,11 +336,11 @@ class Domain(TimeStampedModel, DomainHelper): A domain's status indicates various properties. See Domain.Status. """ - if "statuses" not in self._cache: - try: - return self._get_property("statuses") - except KeyError: - logger.error("Can't retrieve status from domain info") + try: + return self._get_property("statuses") + except KeyError: + logger.error("Can't retrieve status from domain info") + return [] @statuses.setter # type: ignore def statuses(self, statuses: list[str]): @@ -954,7 +954,7 @@ class Domain(TimeStampedModel, DomainHelper): # remove null properties (to distinguish between "a value of None" and null) cleaned = {k: v for k, v in cache.items() if v is not ...} - + print(f"cleaned {cleaned}") # statuses can just be a list no need to keep the epp object if "statuses" in cleaned.keys(): cleaned["statuses"] = [status.state for status in cleaned["statuses"]] diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index d9fdc0b52..5258a88f3 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -547,7 +547,14 @@ class MockEppLib(TestCase): class fakedEppObject(object): """""" - def __init__(self, auth_info=..., cr_date=..., contacts=..., hosts=..., statuses=...,): + def __init__( + self, + auth_info=..., + cr_date=..., + contacts=..., + hosts=..., + statuses=..., + ): self.auth_info = auth_info self.cr_date = cr_date self.contacts = contacts @@ -559,7 +566,10 @@ class MockEppLib(TestCase): cr_date=datetime.datetime(2023, 5, 25, 19, 45, 35), contacts=[common.DomainContact(contact="123", type="security")], hosts=["fake.host.com"], - statuses=[common.Status(state='serverTransferProhibited', description=None, lang='en'), common.Status(state='inactive', description=None, lang='en')], + statuses=[ + common.Status(state="serverTransferProhibited", description="", lang="en"), + common.Status(state="inactive", description="", lang="en"), + ], ) infoDomainNoContact = fakedEppObject( "security", @@ -597,6 +607,7 @@ class MockEppLib(TestCase): def setUp(self): """mock epp send function as this will fail locally""" + print("Set up EPP MOCK") self.mockSendPatch = patch("registrar.models.domain.registry.send") self.mockedSendFunction = self.mockSendPatch.start() self.mockedSendFunction.side_effect = self.mockSend @@ -660,4 +671,5 @@ class MockEppLib(TestCase): ) def tearDown(self): + print("tear down EPP MOCK") self.mockSendPatch.stop() diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 02bb04e26..f0c01d239 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -5,7 +5,7 @@ This file tests the various ways in which the registrar interacts with the regis """ from django.test import TestCase from django.db.utils import IntegrityError -from unittest.mock import patch, call +from unittest.mock import call import datetime from registrar.models import Domain @@ -52,7 +52,7 @@ class TestDomainCache(MockEppLib): call(commands.InfoContact(id="123", auth_info=None), cleaned=True), call(commands.InfoHost(name="fake.host.com"), cleaned=True), ], - any_order=False # Ensure calls are in the specified order + any_order=False, # Ensure calls are in the specified order ) def test_cache_used_when_avail(self): @@ -109,16 +109,14 @@ class TestDomainCache(MockEppLib): domain._get_property("hosts") self.assertEqual(domain._cache["hosts"], [expectedHostsDict]) + def tearDown(self) -> None: + Domain.objects.all().delete() + super().tearDown() -class TestDomainCreation(TestCase): + +class TestDomainCreation(MockEppLib): """Rule: An approved domain application must result in a domain""" - def setUp(self): - """ - Background: - Given that a valid domain application exists - """ - def test_approved_application_creates_domain_locally(self): """ Scenario: Analyst approves a domain application @@ -126,8 +124,6 @@ class TestDomainCreation(TestCase): Then a Domain exists in the database with the same `name` But a domain object does not exist in the registry """ - patcher = patch("registrar.models.domain.Domain._get_or_create_domain") - mocked_domain_creation = patcher.start() draft_domain, _ = DraftDomain.objects.get_or_create(name="igorville.gov") user, _ = User.objects.get_or_create() application = DomainApplication.objects.create( @@ -140,7 +136,7 @@ class TestDomainCreation(TestCase): # should hav information present for this domain domain = Domain.objects.get(name="igorville.gov") self.assertTrue(domain) - mocked_domain_creation.assert_not_called() + self.mockedSendFunction.assert_not_called() @skip("not implemented yet") def test_accessing_domain_properties_creates_domain_in_registry(self): @@ -175,32 +171,39 @@ class TestDomainCreation(TestCase): DomainInformation.objects.all().delete() DomainApplication.objects.all().delete() Domain.objects.all().delete() + super().tearDown() class TestDomainStatuses(MockEppLib): - def test_get_status(self): """Getter returns statuses""" - domain, _ = Domain.objects.get_or_create(name="igorville2.gov") + domain, _ = Domain.objects.get_or_create(name="chicken-liver.gov") + # trigger getter _ = domain.statuses + print("checkpoint 1") + print(domain._cache["statuses"]) + print(domain._cache) + status_list = [status.state for status in self.mockDataInfoDomain.statuses] self.assertEquals(domain._cache["statuses"], status_list) - - # print(self.mockedSendFunction.call_args_list) - + # Called in _fetch_cache self.mockedSendFunction.assert_has_calls( [ - call(commands.InfoDomain(name='igorville2.gov', auth_info=None), cleaned=True), - call(commands.InfoContact(id='123', auth_info=None), cleaned=True), - call(commands.InfoHost(name='fake.host.com'), cleaned=True) + call( + commands.InfoDomain(name="chicken-liver.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoContact(id="123", auth_info=None), cleaned=True), + call(commands.InfoHost(name="fake.host.com"), cleaned=True), ], - any_order=False # Ensure calls are in the specified order + any_order=False, # Ensure calls are in the specified order ) - + def tearDown(self) -> None: Domain.objects.all().delete() - + super().tearDown() + class TestRegistrantContacts(MockEppLib): """Rule: Registrants may modify their WHOIS data""" From ca6fa62ca43760cc96bcf628e9bef9485df22a3a Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 17:48:21 -0400 Subject: [PATCH 24/26] clean up print statements --- src/registrar/models/domain.py | 2 +- src/registrar/tests/common.py | 2 -- src/registrar/tests/test_models_domain.py | 4 ---- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index 440bed0aa..d7b012282 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -954,7 +954,7 @@ class Domain(TimeStampedModel, DomainHelper): # remove null properties (to distinguish between "a value of None" and null) cleaned = {k: v for k, v in cache.items() if v is not ...} - print(f"cleaned {cleaned}") + # statuses can just be a list no need to keep the epp object if "statuses" in cleaned.keys(): cleaned["statuses"] = [status.state for status in cleaned["statuses"]] diff --git a/src/registrar/tests/common.py b/src/registrar/tests/common.py index 5258a88f3..66d9c2db1 100644 --- a/src/registrar/tests/common.py +++ b/src/registrar/tests/common.py @@ -607,7 +607,6 @@ class MockEppLib(TestCase): def setUp(self): """mock epp send function as this will fail locally""" - print("Set up EPP MOCK") self.mockSendPatch = patch("registrar.models.domain.registry.send") self.mockedSendFunction = self.mockSendPatch.start() self.mockedSendFunction.side_effect = self.mockSend @@ -671,5 +670,4 @@ class MockEppLib(TestCase): ) def tearDown(self): - print("tear down EPP MOCK") self.mockSendPatch.stop() diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index f0c01d239..0bed3845b 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -180,10 +180,6 @@ class TestDomainStatuses(MockEppLib): domain, _ = Domain.objects.get_or_create(name="chicken-liver.gov") # trigger getter _ = domain.statuses - print("checkpoint 1") - print(domain._cache["statuses"]) - print(domain._cache) - status_list = [status.state for status in self.mockDataInfoDomain.statuses] self.assertEquals(domain._cache["statuses"], status_list) From 1653784a759502a946e63188e8bd7a6431d6e8f6 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 18:50:27 -0400 Subject: [PATCH 25/26] Add search on draft domain --- src/registrar/admin.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/registrar/admin.py b/src/registrar/admin.py index d0f5c0dbf..d78947c85 100644 --- a/src/registrar/admin.py +++ b/src/registrar/admin.py @@ -780,6 +780,13 @@ class DomainAdmin(ListHeaderAdmin): return super().has_change_permission(request, obj) +class DraftDomainAdmin(ListHeaderAdmin): + """Custom draft domain admin class.""" + + search_fields = ["name"] + search_help_text = "Search by draft domain name." + + admin.site.unregister(LogEntry) # Unregister the default registration admin.site.register(LogEntry, CustomLogEntryAdmin) admin.site.register(models.User, MyUserAdmin) @@ -788,7 +795,7 @@ admin.site.register(models.Contact, ContactAdmin) admin.site.register(models.DomainInvitation, DomainInvitationAdmin) admin.site.register(models.DomainInformation, DomainInformationAdmin) admin.site.register(models.Domain, DomainAdmin) -admin.site.register(models.DraftDomain, ListHeaderAdmin) +admin.site.register(models.DraftDomain, DraftDomainAdmin) admin.site.register(models.Host, MyHostAdmin) admin.site.register(models.Nameserver, MyHostAdmin) admin.site.register(models.Website, WebsiteAdmin) From a0b0fe27763a574c34076cdc87c2311dd5e1e490 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 20 Sep 2023 13:32:09 -0400 Subject: [PATCH 26/26] test the exception, partial implementation of test_accessing_domain_properties_creates_domain_in_registry --- src/registrar/models/domain.py | 10 ++-- src/registrar/tests/test_models_domain.py | 70 +++++++++++++++++++++-- 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index d7b012282..13405d9bb 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -332,7 +332,7 @@ class Domain(TimeStampedModel, DomainHelper): @Cache def statuses(self) -> list[str]: """ - Get or set the domain `status` elements from the registry. + Get the domain `status` elements from the registry. A domain's status indicates various properties. See Domain.Status. """ @@ -344,9 +344,11 @@ class Domain(TimeStampedModel, DomainHelper): @statuses.setter # type: ignore def statuses(self, statuses: list[str]): - # TODO: there are a long list of rules in the RFC about which statuses - # can be combined; check that here and raise errors for invalid combinations - - # some statuses cannot be set by the client at all + """ + We will not implement this. Statuses are set by the registry + when we run delete and client hold, and these are the only statuses + we will be triggering. + """ raise NotImplementedError() @Cache diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 0bed3845b..d35b0ba96 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -5,7 +5,7 @@ This file tests the various ways in which the registrar interacts with the regis """ from django.test import TestCase from django.db.utils import IntegrityError -from unittest.mock import call +from unittest.mock import patch, call import datetime from registrar.models import Domain @@ -138,17 +138,44 @@ class TestDomainCreation(MockEppLib): self.assertTrue(domain) self.mockedSendFunction.assert_not_called() - @skip("not implemented yet") def test_accessing_domain_properties_creates_domain_in_registry(self): """ Scenario: A registrant checks the status of a newly approved domain Given that no domain object exists in the registry When a property is accessed Then Domain sends `commands.CreateDomain` to the registry - And `domain.state` is set to `CREATED` + And `domain.state` is set to `UNKNOWN` And `domain.is_active()` returns False """ - raise + domain = Domain.objects.create(name="beef-tongue.gov") + # trigger getter + _ = domain.statuses + + # contacts = PublicContact.objects.filter(domain=domain, + # type=PublicContact.ContactTypeChoices.REGISTRANT).get() + + # Called in _fetch_cache + self.mockedSendFunction.assert_has_calls( + [ + # TODO: due to complexity of the test, will return to it in + # a future ticket + # call( + # commands.CreateDomain(name="beef-tongue.gov", + # id=contact.registry_id, auth_info=None), + # cleaned=True, + # ), + call( + commands.InfoDomain(name="beef-tongue.gov", auth_info=None), + cleaned=True, + ), + call(commands.InfoContact(id="123", auth_info=None), cleaned=True), + call(commands.InfoHost(name="fake.host.com"), cleaned=True), + ], + any_order=False, # Ensure calls are in the specified order + ) + + self.assertEqual(domain.state, Domain.State.UNKNOWN) + self.assertEqual(domain.is_active(), False) @skip("assertion broken with mock addition") def test_empty_domain_creation(self): @@ -175,8 +202,10 @@ class TestDomainCreation(MockEppLib): class TestDomainStatuses(MockEppLib): + """Domain statuses are set by the registry""" + def test_get_status(self): - """Getter returns statuses""" + """Domain 'statuses' getter returns statuses by calling epp""" domain, _ = Domain.objects.get_or_create(name="chicken-liver.gov") # trigger getter _ = domain.statuses @@ -196,6 +225,37 @@ class TestDomainStatuses(MockEppLib): any_order=False, # Ensure calls are in the specified order ) + def test_get_status_returns_empty_list_when_value_error(self): + """Domain 'statuses' getter returns an empty list + when value error""" + domain, _ = Domain.objects.get_or_create(name="pig-knuckles.gov") + + def side_effect(self): + raise KeyError + + patcher = patch("registrar.models.domain.Domain._get_property") + mocked_get = patcher.start() + mocked_get.side_effect = side_effect + + # trigger getter + _ = domain.statuses + + with self.assertRaises(KeyError): + _ = domain._cache["statuses"] + self.assertEquals(_, []) + + patcher.stop() + + @skip("not implemented yet") + def test_place_client_hold_sets_status(self): + """Domain 'place_client_hold' method causes the registry to change statuses""" + raise + + @skip("not implemented yet") + def test_revert_client_hold_sets_status(self): + """Domain 'revert_client_hold' method causes the registry to change statuses""" + raise + def tearDown(self) -> None: Domain.objects.all().delete() super().tearDown()