#2823: update delete domain process - [MS] #3185

Matt-Spence · 2024-12-03T21:22:59Z

Ticket

Resolves #2823

Changes

Adds new functionality to the "_delete_domain" function that removes subdomains and non-registrant contacts.
Small change to Registry errors to provide a way to bubble up domain names so they can be displayed to users.
New/updated tests

Context for reviewers

Currently domains can't be deleted by the "Delete from the Registry" button in admin unless all hosts and contacts for that domain are deleted first. This PR adds logic to automagically delete contacts and hosts, unless a contact is shared with another domain.

Setup

Poor-police.gov has been created on ms as an example of a domain that has a subdomain used by another domain (you can find this in hosts). You can use that to check that deletion fails in this case, or roll your own if you prefer.

Other than that any domain that has subdomains (usually as nameservers) can be used to test that deletion works properly. After deleting you should be able to click "check status in registry" and see a "pendingDelete" status.

Code Review Verification Steps

Go to MS -> Admin -> Pick a domain that has nameservers and/or contacts -> Put On Hold -> Delete From Registry. Try this for a few permutations including have a domain that shares a host with another domain (this should fail).

As the original developer, I have

Satisfied acceptance criteria and met development standards

Met the acceptance criteria, or will meet them in a subsequent PR
Created/modified automated tests
Update documentation in READMEs and/or onboarding guide

Ensured code standards are met (Original Developer)

If any updated dependencies on Pipfile, also update dependencies in requirements.txt.
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

Tag @dotgov-designers in this PR's Reviewers for design review. If code is not user-facing, delete design reviewer checklist
Verify new pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Verified code meets all checks above. Address any checks that are not satisfied
Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
Checked that all code is adequately covered by tests
Verify migrations are valid and do not conflict with existing migrationsX

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Meets all designs and user flows provided by design/product
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
(Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior. Comment any found issues or broken flows.
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links

Validated user-facing changes as a designer

Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers (check off which ones were used)
- Chrome
- Microsoft Edge
- FireFox
- Safari
(Rarely needed) Tested as both an analyst and applicant user

References

Code review best practices

Screenshots

…-domain-process

github-actions · 2024-12-03T21:28:04Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-04T17:29:40Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-04T18:55:31Z

🥳 Successfully deployed to developer sandbox ms.

…-domain-process

github-actions · 2024-12-04T20:33:18Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-04T22:33:17Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-04T22:36:54Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-04T23:15:45Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-05T16:23:50Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-05T17:06:28Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-05T17:35:27Z

🥳 Successfully deployed to developer sandbox ms.

…-domain-process

github-actions · 2024-12-05T18:56:48Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-09T19:17:04Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-09T22:30:18Z

🥳 Successfully deployed to developer sandbox ms.

zandercymatics

Code looks good. Only real blocker is the raise error on _delete_hosts_if_not_used which may have some knock-on effects on nameserver. Might not be a problem in practice though

Will test this within a day or so (no need to monitor your sandbox though) as I am nibbling this. Posting this review so you have this earlier rather than later

zandercymatics · 2024-12-13T15:08:24Z

src/registrar/admin.py

-                message = error_messages.get(err.code, err)
+                message = error_messages[err.code]


I think this is an artifact of some testing I did that involved messing with the error messages. I thought I rewrote everything the way it should have been but it appears I missed this. Will revert this change.

zandercymatics · 2024-12-13T15:11:09Z

src/registrar/models/domain.py

+
+        logger.info("Deleting subdomains for %s", self.name)
+        # check if any subdomains are in use by another domain
+        hosts = Host.objects.filter(name__regex=r".+{}".format(self.name))


Nice check on subdomain

zandercymatics · 2024-12-13T15:15:21Z

src/registrar/models/domain.py

+        for host in hosts:
+            if host.domain != self:
+                logger.error("Unable to delete host: %s is in use by another domain: %s", host.name, host.domain)
+                raise RegistryError(
+                    code=ErrorCode.OBJECT_ASSOCIATION_PROHIBITS_OPERATION,
+                    note=f"Host {host.name} is in use by {host.domain}",
+                )
+


I think EPP will throw an error in this case so it should be OK, but there is a potential scenario here where a nameserver exists on EPP but not on our hosts table (sandboxes for example).

I think the way you handled this is totally fine though. Just curious to hear your perspective and thoughts on that

That's a good point that I had considered, but in general there's not a great way to handle it. I haven't found a method in EPP that lets us list object based on their relationships, so we have to rely on our database foreign key relationships being an accurate-enough reflection. That said, it would be a relatively simple task to add more error handling if need be.

Did some digging. There is a function in our epplib library called ListDomainsByNsset and ListDomainsByContact as well as a few others.

The process involved in using these though would go way outside the scope of the ticket. The commands may work on their own by default but you'd need to extend what we currently have here. And if they don't work on their own, you'd need to modify the repo which Dave and I did before. For it being a rare edgecase we can just send out these commands manually if we run into this scenario

zandercymatics · 2024-12-13T15:19:08Z

src/registrar/models/domain.py

+        if responseCode != ErrorCode.COMMAND_COMPLETED_SUCCESSFULLY:
+            raise NameserverError(code=nsErrorCodes.BAD_DATA)
+
+        self._delete_hosts_if_not_used(hostsToDelete=deleted_values)


(Nitpick / optional) A comment explaining as to why delete hosts is called after 1062 would be a nice to have but not needed

zandercymatics · 2024-12-13T15:20:38Z

src/registrar/models/domain.py

@@ -1026,10 +1033,49 @@ def _remove_client_hold(self):
            # if registry error occurs, log the error, and raise it as well
            logger.error(f"registry error removing client hold: {err}")
            raise (err)
-
+        
    def _delete_domain(self):


Nice work on this function. I also like how you doing error handling / raising, very clean

zandercymatics · 2024-12-13T15:26:32Z

src/registrar/models/domain.py

+            raise e
+
    def _fix_unknown_state(self, cleaned):


(potentially blocking) This will have some knock-on effects for the nameservers setter as it seems to assume that no error is raised by this helper function

Good catch, I'll add a try/catch to the nameservers setter.

zandercymatics · 2024-12-13T15:46:51Z

src/registrar/tests/common.py

+    def mockDeleteHostCommands(self, _request, cleaned):
+        host = getattr(_request, "name", None)
+        if "sharedhost.com" in host:
+            print("raising registry error")


Remember to remove before merging

Another good catch, I thought I caught all of these!

zandercymatics · 2024-12-13T15:47:07Z

src/registrar/tests/common.py

@@ -1748,6 +1765,7 @@ def mockInfoDomainCommands(self, _request, cleaned):
            "subdomainwoip.gov": (self.mockDataInfoDomainSubdomainNoIP, None),
            "ddomain3.gov": (self.InfoDomainWithContacts, None),
            "igorville.gov": (self.InfoDomainWithContacts, None),
+            "sharingiscaring.gov": (self.infoDomainSharedHost, None),


zandercymatics · 2024-12-13T16:17:41Z

src/registrar/tests/test_models_domain.py

+                    cleaned=True,
+                ),
+            ],
+            any_order=True,


Shouldn't this be false?

The ordering of these matters in some cases, but the database doesn't always return things in the same order so for things like the contacts it can break the test. I'll break that part out though, because in general you have the right idea and I had meant to do so anyway.

Ah gotcha. Yeah that'd be totally fine then

zandercymatics · 2024-12-13T16:19:48Z

src/registrar/tests/test_reports.py

+            "[email protected],True,[email protected],2022-04-01,2024-02-01,Viewer Requester,Manager,False,0,\n"
+            "[email protected],False,[email protected],2022-04-01,Invalid date,None,"
+            'Manager,True,2,"adomain2.gov,cdomain1.gov"\n'
            "[email protected],False,[email protected],Unretrieved,Invited,None,Manager,False,0,\n"
            "[email protected],False,[email protected],Unretrieved,Invited,None,Viewer,False,0,\n"
            "[email protected],False,[email protected],Unretrieved,Invited,Viewer,None,False,0,\n"


I think this fix is on latest now so you don't need to add it manually

….com/cisagov/manage.get.gov into ms/2823-update-delete-domain-process

zandercymatics

looks great

zandercymatics · 2024-12-13T22:24:18Z

src/registrar/models/domain.py

+        except:
+            # the error will be logged in the erring function and we don't 
+            # need this part to succeed in order to continue.s
+            pass


Would suggest logging the error + some minor grammar things in the comment, but other than that looks good

…in-process

github-actions · 2024-12-13T22:42:07Z

🥳 Successfully deployed to developer sandbox ms.

zandercymatics

LGTM. Was able to reproduce the subdomain error, though its actually a non-issue, imo.

When adding nameserver records, I am getting the Error deleting this Domain: This subdomain is being used as a hostname on another domain: error that you mentioned.

This error would only occur when nameservers were added. So if I deleted the domain in dns needed or if I deleted those nameserver records when it was in "ready", this error would disappear. This actually makes total sense - the underlying error is OBJECT_ASSOCIATION_PROHIBITS_OPERATION. This tells me that the messaging was faulty rather than your code

In unrelated news, on your sandbox I seem to be getting a 500 on team-loss-within.gov for a missing contact. This is "on hold". If you set this record to on hold manually from deleted then that would make sense, though otherwise that is strange

zandercymatics · 2024-12-16T16:09:30Z

That said, I am curious as to what @dave-kennedy-ecs thinks

zandercymatics · 2024-12-16T16:10:01Z

@Matt-Spence Also forgot to mention, but your linter is failing

github-actions · 2024-12-16T16:33:37Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-16T16:42:00Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-12-16T19:04:28Z

🥳 Successfully deployed to developer sandbox ms.

Matt-Spence and others added 4 commits November 22, 2024 10:51

Delete contacts and subdomains on delete domain

7cf8b8a

Rework delete from epp

6891f5c

Merge remote-tracking branch 'origin/main' into ms/2823-update-delete…

ede01e3

…-domain-process

update deletion process and tests

b5e4f8b

Matt-Spence requested a review from allly-b as a code owner December 3, 2024 21:23

minor fixes to tests

27868a0

minor test fix

9437b73

Matt-Spence added 2 commits December 4, 2024 13:21

Merge remote-tracking branch 'origin/main' into ms/2823-update-delete…

74eeae1

…-domain-process

linter fixes

89253a1

Matt-Spence added 2 commits December 4, 2024 16:28

include hostname in error messages for shared hosts

f25bb9b

add back in less console noise decorator

dad4226

admin fix

6fdb763

revert accidental admin change

e8fdf0c

temp test changes

3f79b56

fix a test

2e84171

Matt-Spence added 2 commits December 5, 2024 12:28

Merge remote-tracking branch 'origin/main' into ms/2823-update-delete…

43484ec

…-domain-process

fix broken test

aaaa4f2

Matt-Spence added 2 commits December 5, 2024 13:55

add error message to registry errors

8b473d5

up log level

3dbafb5

use update function to delete hosts

e87c4f7

Matt-Spence and others added 6 commits December 11, 2024 10:11

consolidate delete domain function

1a9b671

working _delete_domain

e0eb70a

refactor delete to use same logic as nameserver setter

533e577

fix tests

51da456

pushing changes to pull on another device

1fb8b22

clean up

63c2b79

Matt-Spence changed the title ~~[DRAFT] #2823: update delete domain process - [MS]~~ #2823: update delete domain process - [MS] Dec 12, 2024

zandercymatics self-assigned this Dec 13, 2024

zandercymatics reviewed Dec 13, 2024

View reviewed changes

Matt-Spence and others added 3 commits December 13, 2024 15:23

review changes

21007ce

review changes and linting

32789fa

Merge branch 'ms/2823-update-delete-domain-process' of https://github…

db4f9c5

….com/cisagov/manage.get.gov into ms/2823-update-delete-domain-process

zandercymatics reviewed Dec 13, 2024

View reviewed changes

Matt-Spence added 2 commits December 13, 2024 16:35

log exception in nameserver setter.

bb2072d

Merge remote-tracking branch 'origin' into ms/2823-update-delete-doma…

f2d7be1

…in-process

zandercymatics approved these changes Dec 16, 2024

View reviewed changes

linter fixes

0178d57

linter fixes

f039f31

Merge branch 'main' into ms/2823-update-delete-domain-process

8016dd9

Matt-Spence merged commit c02b036 into main Dec 16, 2024
9 of 10 checks passed

Matt-Spence deleted the ms/2823-update-delete-domain-process branch December 16, 2024 19:04

		message = error_messages.get(err.code, err)
		message = error_messages[err.code]

#2823: update delete domain process - [MS] #3185

#2823: update delete domain process - [MS] #3185

Conversation

Matt-Spence commented Dec 3, 2024 • edited Loading

Ticket

Changes

Context for reviewers

Setup

Code Review Verification Steps

As the original developer, I have

Satisfied acceptance criteria and met development standards

Ensured code standards are met (Original Developer)

Validated user-facing changes (if applicable)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Validated user-facing changes as a developer

As a designer reviewer, I have

Verified that the changes match the design intention

Validated user-facing changes as a designer

References

Screenshots

github-actions bot commented Dec 3, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 5, 2024

github-actions bot commented Dec 9, 2024

github-actions bot commented Dec 9, 2024

zandercymatics left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2024

zandercymatics left a comment

Choose a reason for hiding this comment

zandercymatics commented Dec 16, 2024

zandercymatics commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

github-actions bot commented Dec 16, 2024

Matt-Spence commented Dec 3, 2024 •

edited

Loading

zandercymatics Dec 13, 2024 •

edited

Loading

zandercymatics Dec 13, 2024 •

edited

Loading