From 68012389a8c2bb9e936847272e54ac08a2c4f960 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Mon, 18 Sep 2023 15:21:57 -0400 Subject: [PATCH 1/5] 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 2/5] 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 021f71f2f5b2b4418499429fbe3ece114806d155 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 19 Sep 2023 17:46:47 -0400 Subject: [PATCH 3/5] 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 4/5] 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 a0b0fe27763a574c34076cdc87c2311dd5e1e490 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Wed, 20 Sep 2023 13:32:09 -0400 Subject: [PATCH 5/5] 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()