Merge pull request #2888 from cisagov/rjm/1263-approved-domain-error

#1263 Request error: Already approved domain - [RJM]

src/registrar/admin.py

@@ -1976,18 +1976,30 @@ class DomainRequestAdmin(ListHeaderAdmin, ImportExportModelAdmin):
 
             # If the status is not mapped properly, saving could cause
             # weird issues down the line. Instead, we should block this.
+            # NEEDS A UNIT TEST
             should_proceed = False
-            return should_proceed
+            return (obj, should_proceed)
 
-        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
-            # shortcut or not as the rules are duplicated on the model,
-            # but the error would be an ugly Django error screen.
+        obj_is_not_approved = obj.status != models.DomainRequest.DomainRequestStatus.APPROVED
+        if obj_is_not_approved and not obj.domain_is_not_active():
+            # REDUNDANT CHECK / ERROR SCREEN AVOIDANCE:
+            # This action (moving a request from approved to
+            # another status) when the domain is already active (READY),
+            # would still not go through even without this check as the rules are
+            # duplicated in the model and the error is raised from the model.
+            # This avoids an ugly Django error screen.
             error_message = "This action is not permitted. The domain is already active."
+        elif (
+            original_obj.status != models.DomainRequest.DomainRequestStatus.APPROVED
+            and obj.status == models.DomainRequest.DomainRequestStatus.APPROVED
+            and original_obj.requested_domain is not None
+            and Domain.objects.filter(name=original_obj.requested_domain.name).exists()
+        ):
+            # REDUNDANT CHECK:
+            # This action (approving a request when the domain exists)
+            # would still not go through even without this check as the rules are
+            # duplicated in the model and the error is raised from the model.
+            error_message = FSMDomainRequestError.get_error_message(FSMErrorCodes.APPROVE_DOMAIN_IN_USE)
         elif obj.status == models.DomainRequest.DomainRequestStatus.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)

src/registrar/tests/test_admin_request.py

@@ -1846,6 +1846,58 @@ class TestDomainRequestAdmin(MockEppLib):
     def test_side_effects_when_saving_approved_to_ineligible(self):
         self.trigger_saving_approved_to_another_state(False, DomainRequest.DomainRequestStatus.INELIGIBLE)
 
+    @less_console_noise
+    def test_error_when_saving_to_approved_and_domain_exists(self):
+        """Redundant admin check on model transition not allowed."""
+        Domain.objects.create(name="wabbitseason.gov")
+
+        new_request = completed_domain_request(
+            status=DomainRequest.DomainRequestStatus.SUBMITTED, name="wabbitseason.gov"
+        )
+
+        # Create a request object with a superuser
+        request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(new_request.pk))
+        request.user = self.superuser
+
+        request.session = {}
+
+        # Use ExitStack to combine patch contexts
+        with ExitStack() as stack:
+            # Patch django.contrib.messages.error
+            stack.enter_context(patch.object(messages, "error"))
+
+            new_request.status = DomainRequest.DomainRequestStatus.APPROVED
+
+            self.admin.save_model(request, new_request, None, True)
+
+            messages.error.assert_called_once_with(
+                request,
+                "Cannot approve. Requested domain is already in use.",
+            )
+
+    @less_console_noise
+    def test_no_error_when_saving_to_approved_and_domain_exists(self):
+        """The negative of the redundant admin check on model transition not allowed."""
+        new_request = completed_domain_request(status=DomainRequest.DomainRequestStatus.SUBMITTED)
+
+        # Create a request object with a superuser
+        request = self.factory.post("/admin/registrar/domainrequest/{}/change/".format(new_request.pk))
+        request.user = self.superuser
+
+        request.session = {}
+
+        # 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(messages, "error"))
+
+            new_request.status = DomainRequest.DomainRequestStatus.APPROVED
+
+            self.admin.save_model(request, new_request, None, True)
+
+            # Assert that the error message was never called
+            messages.error.assert_not_called()
+
     def test_has_correct_filters(self):
         """
         This test verifies that DomainRequestAdmin has the correct filters set up.

src/registrar/tests/test_views_request.py

@@ -82,7 +82,6 @@ class DomainRequestTests(TestWithUser, WebTest):
         response = self.app.get(f"/domain-request/{domain_request.id}")
         # Ensure that the date is still set to None
         self.assertIsNone(domain_request.last_status_update)
-        print(response)
         # We should still grab a date for this field in this event - but it should come from the audit log instead
         self.assertContains(response, "Started on:")
         self.assertContains(response, fixed_date.strftime("%B %-d, %Y"))