fixes from review comments

src/registrar/tests/test_admin_domain.py

@@ -26,7 +26,6 @@ from .common import (
     GenericTestHelper,
 )
 from unittest.mock import ANY, call, patch
-from unittest import skip
 
 import boto3_mocking  # type: ignore
 import logging
@@ -172,44 +171,6 @@ class TestDomainAdminAsStaff(MockEppLib):
         _domain_request.delete()
         _creator.delete()
 
-    @skip("Why did this test stop working, and is is a good test")
-    def test_place_and_remove_hold(self):
-        domain = create_ready_domain()
-        # get admin page and assert Place Hold button
-        response = self.client.get(
-            "/admin/registrar/domain/{}/change/".format(domain.pk),
-            follow=True,
-        )
-        self.assertEqual(response.status_code, 200)
-        self.assertContains(response, domain.name)
-        self.assertContains(response, "Place hold")
-        self.assertNotContains(response, "Remove hold")
-
-        # submit place_client_hold and assert Remove Hold button
-        response = self.client.post(
-            "/admin/registrar/domain/{}/change/".format(domain.pk),
-            {"_place_client_hold": "Place hold", "name": domain.name},
-            follow=True,
-        )
-        self.assertEqual(response.status_code, 200)
-        self.assertContains(response, domain.name)
-        self.assertContains(response, "Remove hold")
-        self.assertNotContains(response, "Place hold")
-
-        # submit remove client hold and assert Place hold button
-        response = self.client.post(
-            "/admin/registrar/domain/{}/change/".format(domain.pk),
-            {"_remove_client_hold": "Remove hold", "name": domain.name},
-            follow=True,
-        )
-        self.assertEqual(response.status_code, 200)
-        self.assertContains(response, domain.name)
-        self.assertContains(response, "Place hold")
-        self.assertNotContains(response, "Remove hold")
-
-        # clean up this test's data
-        domain.delete()
-
     @less_console_noise_decorator
     def test_deletion_is_successful(self):
         """
@@ -371,7 +332,7 @@ class TestDomainAdminAsStaff(MockEppLib):
         domain.delete()
 
 
-class TestDomainAdminWClient(TestCase):
+class TestDomainAdminWithClient(TestCase):
     """Test DomainAdmin class as super user.
 
     Notes:
@@ -510,7 +471,7 @@ class TestDomainAdminWClient(TestCase):
         self.assertContains(response, domain.name)
 
         # Contains some test tools
-        self.test_helper = GenericTestHelper(
+        test_helper = GenericTestHelper(
             factory=self.factory,
             user=self.superuser,
             admin=self.admin,
@@ -524,7 +485,7 @@ class TestDomainAdminWClient(TestCase):
             ("first_ready_at", 'Date when this domain first moved into "ready" state; date will never change'),
             ("deleted_at", 'Will appear blank unless the domain is in "deleted" state'),
         ]
-        self.test_helper.assert_response_contains_distinct_values(response, expected_values)
+        test_helper.assert_response_contains_distinct_values(response, expected_values)
 
     @less_console_noise_decorator
     def test_helper_text_state(self):
@@ -533,11 +494,11 @@ class TestDomainAdminWClient(TestCase):
         """
 
         # Add domain data
-        self.ready_domain, _ = Domain.objects.get_or_create(name="fakeready.gov", state=Domain.State.READY)
+        ready_domain, _ = Domain.objects.get_or_create(name="fakeready.gov", state=Domain.State.READY)
-        self.unknown_domain, _ = Domain.objects.get_or_create(name="fakeunknown.gov", state=Domain.State.UNKNOWN)
+        unknown_domain, _ = Domain.objects.get_or_create(name="fakeunknown.gov", state=Domain.State.UNKNOWN)
-        self.dns_domain, _ = Domain.objects.get_or_create(name="fakedns.gov", state=Domain.State.DNS_NEEDED)
+        dns_domain, _ = Domain.objects.get_or_create(name="fakedns.gov", state=Domain.State.DNS_NEEDED)
-        self.hold_domain, _ = Domain.objects.get_or_create(name="fakehold.gov", state=Domain.State.ON_HOLD)
+        hold_domain, _ = Domain.objects.get_or_create(name="fakehold.gov", state=Domain.State.ON_HOLD)
-        self.deleted_domain, _ = Domain.objects.get_or_create(name="fakedeleted.gov", state=Domain.State.DELETED)
+        deleted_domain, _ = Domain.objects.get_or_create(name="fakedeleted.gov", state=Domain.State.DELETED)
 
         # We don't need to check for all text content, just a portion of it
         expected_unknown_domain_message = "The creator of the associated domain request has not logged in to"
@@ -545,11 +506,11 @@ class TestDomainAdminWClient(TestCase):
         expected_hold_message = "While on hold, this domain"
         expected_deleted_message = "This domain was permanently removed from the registry."
         expected_messages = [
-            (self.ready_domain, "This domain has name servers and is ready for use."),
+            (ready_domain, "This domain has name servers and is ready for use."),
-            (self.unknown_domain, expected_unknown_domain_message),
+            (unknown_domain, expected_unknown_domain_message),
-            (self.dns_domain, expected_dns_message),
+            (dns_domain, expected_dns_message),
-            (self.hold_domain, expected_hold_message),
+            (hold_domain, expected_hold_message),
-            (self.deleted_domain, expected_deleted_message),
+            (deleted_domain, expected_deleted_message),
         ]
 
         for domain, message in expected_messages:
@@ -809,7 +770,7 @@ class TestDomainAdminWebTest(MockEppLib, WebTest):
         domain_change_page = self.app.get(reverse("admin:registrar_domain_change", args=[domain.pk]))
 
         self.assertContains(domain_change_page, "fake.gov")
-        # click the "Manage" link
+        # click the "Delete" link
         confirmation_page = domain_change_page.click("Delete", index=0)
 
         content_slice = "When a domain is deleted:"
@@ -861,7 +822,3 @@ class TestDomainAdminWebTest(MockEppLib, WebTest):
             # Web test has issues grabbing up to date data from the db, so we can test
             # the returned view instead
             self.assertContains(response, '<div class="readonly">On hold</div>')
-
-    @skip("Waiting on epp lib to implement")
-    def test_place_and_remove_hold_epp(self):
-        raise

src/registrar/tests/test_admin_request.py

@@ -77,6 +77,7 @@ class TestDomainRequestAdmin(MockEppLib):
         DomainRequest.objects.all().delete()
         Contact.objects.all().delete()
         Website.objects.all().delete()
+        SeniorOfficial.objects.all().delete()
         self.mock_client.EMAILS_SENT.clear()
 
     @classmethod
@@ -111,13 +112,6 @@ class TestDomainRequestAdmin(MockEppLib):
 
         self.assertEqual(current_sort_order, expected_sort_order)
 
-        DomainInformation.objects.all().delete()
-        domain_request.delete()
-        contact.delete()
-        so_mary.delete()
-        so_alex.delete()
-        so_zoup.delete()
-
     @less_console_noise_decorator
     def test_has_model_description(self):
         """Tests if this model has a model description on the table view"""
@@ -267,7 +261,7 @@ class TestDomainRequestAdmin(MockEppLib):
         # Make sure the page loaded, and that we're on the right page
         self.assertEqual(response.status_code, 200)
         self.assertContains(response, domain_request.requested_domain.name)
-        self.test_helper.assertContains(response, "<span>Show details</span>")
+        self.assertContains(response, "<span>Show details</span>")
 
     @less_console_noise_decorator
     def test_analyst_can_see_and_edit_alternative_domain(self):

src/registrar/tests/test_management_scripts.py

@@ -1184,10 +1184,9 @@ class TestTransferFederalAgencyType(TestCase):
         User.objects.all().delete()
         Contact.objects.all().delete()
         Website.objects.all().delete()
-        self.amtrak.delete()
+        FederalAgency.objects.filter(
-        self.legislative_branch.delete()
+            id__in=[self.amtrak.id, self.legislative_branch.id, self.library_of_congress.id, self.gov_admin.id]
-        self.library_of_congress.delete()
+        ).delete()
-        self.gov_admin.delete()
 
     def run_transfer_federal_agency_type(self):
         """

src/registrar/tests/test_views.py

@@ -65,12 +65,6 @@ class TestWithUser(MockEppLib):
         super().setUp()
         self.client = Client()
 
-    def tearDown(self):
-        # delete any domain requests too
-        super().tearDown()
-        # DomainRequest.objects.all().delete()
-        # DomainInformation.objects.all().delete()
-
     @classmethod
     def tearDownClass(cls):
         super().tearDownClass()
@@ -486,11 +480,11 @@ class HomeTests(TestWithUser):
             "You’re about to start your .gov domain request.",
         )
 
+    @less_console_noise_decorator
     def test_domain_request_form_with_ineligible_user(self):
         """Domain request form not accessible for an ineligible user.
         This test should be solid enough since all domain request wizard
         views share the same permissions class"""
-        with less_console_noise():
         username = "restricted_user"
         first_name = "First"
         last_name = "Last"