(Not on a sandbox) Ticket #1501: Delete button for DomainApplication records #1590

zandercymatics · 2024-01-04T21:35:05Z

Ticket

Resolves #1501

Changes

Added some styles in _tables.scss for positioning, increased width of card on the home page
Modified home.html to include the delete button
Added unit tests in test_views.py
Added a new delete view in application.py for DomainApplication w/ a has_permission override to forbid certain domains from being deleted
Modified index.py to add a context for the modal button and if there are any applications which can be deleted
Added an abstract permission view for DomainApplication
Adds some logic for displaying multiple unfinished applications

Context for reviewers

This ticket adds a delete button for DomainApplications on the home page, and some logic where when you start multiple new domain applications, they will be numbered rather than having duplicate names.

Regarding the delete button, it is only visible if the domain is in the state of started or withdrawn, otherwise it cannot be deleted.

Setup

For designers
Everything you need should already exists on my sandbox - one domainapplication in STARTED and one in WITHDRAWN. If this is not the case (i.e. there is not delete button for any of the DomainApplications), then you will need to navigate to /admin and add yourself as a submitter to a DomainApplication with either of those states.

Afterwards, just follow the workflow defined in the figma.

For developers
IMPORTANT: Since this PR adds a scss file and makes some of those changes, you will likely need to do collectstatic if that local cache is not refreshing for you.

This PR should be fairly simple to review. When testing locally, all you need to do is boot up the application and you will have an assortment to test with.

Afterwards, start two domain applications, but do not finish either of them. Note that they will be numbered...

On sandboxes, you should already have this, but if you do not - grab a few applications and set yourself as a submitter. Then, delete them.

Code Review Verification Steps

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
Added at least 2 developers as PR reviewers (only 1 will need to approve)
Messaged on Slack or in standup to notify the team that a PR is ready for review
Changes to “how we do things” are documented in READMEs and or onboarding guide
If any model was updated to modify/add/delete columns, makemigrations was ran and the assoicated migrations file has been commited.

Ensured code standards are met (Original Developer)

All new functions and methods are commented using plain language
Did dependency updates in Pipfile also get changed 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)

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)
Add at least 1 designer as PR reviewer

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

Pulled this branch locally and tested it
Reviewed this code and left comments
Checked that all code is adequately covered by tests
Made it clear which comments need to be addressed before this work is merged
If any model was updated to modify/add/delete columns, makemigrations was ran and the assoicated migrations file has been commited.

Ensured code standards are met (Code reviewer)

All new functions and methods are commented using plain language
Interactions with external systems are wrapped in try/except
Error handling exists for unusual or missing values
(Rarely needed) Did dependency updates in Pipfile also get changed in requirements.txt?

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist

As a designer reviewer, I have

Verified that the changes match the design intention

Checked that the design translated visually
Checked behavior
Checked different states (empty, one, some, error)
Checked for landmarks, page heading structure, and links
Tried to break the intended flow

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

Screenshots

github-actions · 2024-01-04T21:40:07Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T16:59:49Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T17:03:38Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T18:32:17Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T19:38:06Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T19:54:43Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T21:50:30Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-05T22:07:52Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics · 2024-01-05T22:29:30Z

src/registrar/assets/sass/_theme/_usa-modal.scss

+.usa-modal {
+    .usa-modal__heading.blue-header{
+        color: $dhs-blue;
+    }
+}


This is the same color as the figma but its not consistent with the other modals in our app (consistency difference between that and the figma). Thoughts?

@zandercymatics sometimes I forget to update the color when pulling in the USWDS base modal component because the color is so similar to our .gov palette. Go ahead and match the color we have for our styles on this and any modals going forward.

I double checked and I sometimes mistakenly use medium-50 when it should be dark-60. My bad!

Not a problem!

src/registrar/templates/home.html

zandercymatics · 2024-01-05T22:33:57Z

src/registrar/tests/test_views.py

+        # Given that we are including a subset of items that can be deleted while excluding the rest,
+        # subTest is appropriate here as otherwise we would need many duplicate tests for the same reason.
+        draft_domain = DraftDomain.objects.create(name="igorville.gov")
+        for status in DomainApplication.ApplicationStatus:
+            if status not in [
+                DomainApplication.ApplicationStatus.STARTED,
+                DomainApplication.ApplicationStatus.WITHDRAWN,
+            ]:
+                with self.subTest(status=status):
+                    application = DomainApplication.objects.create(
+                        creator=self.user, requested_domain=draft_domain, status=status
+                    )


This can be re-implemented to exclude the for loop but it doesn't make sense to do here in this context because it would make the test brittle. This is based off of other unit tests using subTest which test on status as well (FSM checks)

allly-b

Sorry for approving before, I forgot to check that contacts were being deleted. That needs to happen here so we don't create orphaned contacts on our production environment.

github-actions · 2024-01-19T19:08:05Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-19T20:21:07Z

🥳 Successfully deployed to developer sandbox za.

gabo0oo

Looks great!

github-actions · 2024-01-19T22:59:28Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-19T23:26:59Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-22T16:42:50Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-22T16:57:33Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics · 2024-01-22T17:02:28Z

src/registrar/views/application.py

+    def post(self, request, *args, **kwargs):
+        # Grab all orphaned contacts
+        application: DomainApplication = self.get_object()
+        contacts_to_delete, duplicates = self._get_orphaned_contacts(application)
+
+        # Delete the DomainApplication
+        response = super().post(request, *args, **kwargs)
+
+        # Delete orphaned contacts - but only for if they are not associated with a user
+        Contact.objects.filter(id__in=contacts_to_delete, user=None).delete()
+
+        # After a delete occurs, do a second sweep on any returned duplicates.
+        # This determines if any of these three fields share a contact, which is used for
+        # the edge case where the same user may be an AO, and a submitter, for example.
+        if len(duplicates) > 0:
+            duplicates_to_delete, _ = self._get_orphaned_contacts(application)
+            Contact.objects.filter(id__in=duplicates_to_delete, user=None).delete()
+
+        return response


This is being done at the form level, as the ticket which handled the joins was done at this level too. From my understanding, we are trying to avoid modifying model-level behavior where possible, and this could be labelled as falling under that triage.

For a future ticket (probably during some sort of refactor or reconciliation), I would recommend some sort of helper, utility, class, etc that handles orphaned records more generically. This can be done either at the model or form level, but that on its own is another beast (metaprogramming) - but it can be done. I do not think this should be done now, though. Only if we have a need for it

zandercymatics · 2024-01-22T18:16:50Z

src/registrar/views/application.py

+        # Check if the desired item still exists in the DB
+        if check_db:
+            ao = self._get_contacts_by_id([ao.id]).first() if ao is not None else None
+            submitter = self._get_contacts_by_id([submitter.id]).first() if submitter is not None else None
+            other_contacts = self._get_contacts_by_id(other_contact_ids)
+
+        # Pair each contact with its related name
+        checked_contacts = [(ao, "authorizing_official"), (submitter, "submitted_applications")]
+        checked_contacts.extend((contact, "contact_applications") for contact in other_contacts)


Not the most elegant thing ever but it works. Let me know if this needs changing

Agreed but makes sense here, just would update the comment to make it easier to read why this is happening

Implemented changes

allly-b

Looks good, thanks for the change. A couple minor suggestions and for sure remove the availability check change before merging. That really should be linked to the associated ticket in it's own PR

allly-b · 2024-01-22T19:32:17Z

src/registrar/tests/test_views.py

+
+        # Create the site and contacts to delete (orphaned)
+        contact = Contact.objects.create(
+            first_name="Henry",


total tangent It would have been funny if this contact was named Oliver Twist or Bruce Wayne, since it gets orphaned.

allly-b · 2024-01-22T23:49:38Z

src/registrar/models/utility/domain_helper.py

@@ -57,6 +57,9 @@ def _validate_domain_string(domain, blank_ok):
            # If blank ok is true, just return the domain
            return domain

+        if domain.startswith("www."):


blocking, fix before merge This is part of another ticket and shouldn't be combined into this PR

allly-b · 2024-01-22T23:54:09Z

src/registrar/views/application.py

+        other_contact_ids = application.other_contacts.all().values_list("id", flat=True)
+
+        # Check if the desired item still exists in the DB
+        if check_db:


comment: are you saying here that you can have a foreign key value filled in with an id and then have it not exist in the actual foreign table (contact). That shouldn't be possible if things were set up right. Was this check done just to be cautious or because you saw this happen

(More info below) basically, there is a scenario where it is possible to have dangling contacts even after a delete. This occurs when the contact is shared among two contact fields (like AO and submitter). From the join functions perspective, it looks fine. But from our perspective (as long as its not shared anywhere else), we know that it will still be orphaned after its deleted. After we determine (with duplicates) that we ran into this edge case, we do the same join check, but check by getting updated values from the db. If it still exists in the DB and its an orphan, we caught that edge case

src/registrar/views/application.py

allly-b · 2024-01-23T00:01:37Z

src/registrar/views/application.py

+        # Check if the desired item still exists in the DB
+        if check_db:
+            ao = self._get_contacts_by_id([ao.id]).first() if ao is not None else None
+            submitter = self._get_contacts_by_id([submitter.id]).first() if submitter is not None else None
+            other_contacts = self._get_contacts_by_id(other_contact_ids)
+
+        # Pair each contact with its related name
+        checked_contacts = [(ao, "authorizing_official"), (submitter, "submitted_applications")]
+        checked_contacts.extend((contact, "contact_applications") for contact in other_contacts)


Agreed but makes sense here, just would update the comment to make it easier to read why this is happening

allly-b · 2024-01-23T00:03:05Z

src/registrar/views/application.py

+        return response
+
+    def _get_orphaned_contacts(self, application: DomainApplication, check_db=True):
+        """Collects all orphaned contacts"""


change before merging: add info on what this returns since it's not initially clear

allly-b · 2024-01-23T00:16:42Z

src/registrar/views/application.py

+        return contacts
+
+    def _get_duplicates(self, objects):
+        """Given a list of objects, return a list of which items were duplicates"""


fix before merging: this is only ever used for contacts, I would be more specific here this is for contacts, especially since you use the variable "contact" below. Also, why not just convert objects to a set and then back to a list? That will automatically remove duplicates in a list normally, though I have never tried with a query result so curious if you ran into an issue there.

The reason why I am not converting it to a set is because I actually want to get a list of duplicates, not remove them. This is because duplicates let me detect an edge case:

A contact (which is not joined on any other field) is attached to both the authorizing_official field and the submitter field. When the delete is ran, it "looks" like this field is not an orphan (because it technically isn't) - but that isn't correct, because it will be after the delete.

This is basically a check to see if we run into that case, i.e. if the contact is shared among any of the fields on Application. If it is, there is a good chance that we hit this edge case - so we use this as a flag to determine if we should do a second join check or not (which will occur after delete, so it will now pick up if that contact became orphaned from the delete)

src/registrar/tests/test_views.py

Co-authored-by: Alysia Broddrick <109625347+abroddrick@users.noreply.github.com>

github-actions · 2024-01-23T15:26:04Z

🥳 Successfully deployed to developer sandbox za.

…com/cisagov/manage.get.gov into za/1501-users-delete-domain-records

github-actions · 2024-01-23T15:38:26Z

🥳 Successfully deployed to developer sandbox za.

github-actions · 2024-01-23T15:50:00Z

🥳 Successfully deployed to developer sandbox za.

zandercymatics added 2 commits January 4, 2024 14:12

Update home.html

5ebae38

Delete functionality

403bfb6

zandercymatics requested a review from allly-b as a code owner January 4, 2024 21:35

zandercymatics marked this pull request as draft January 4, 2024 21:35

zandercymatics added 2 commits January 5, 2024 09:54

Conditional deletion

a251d1c

Increase padding

2042fd1

Fix styling

0bbbc89

Add unit tests

90760e6

Add padding for mobile view

693058c

zandercymatics marked this pull request as ready for review January 5, 2024 19:57

CSS changes

0ae2cb7

Update _usa-modal.scss

d9151fe

zandercymatics commented Jan 5, 2024

View reviewed changes

src/registrar/templates/home.html Outdated Show resolved Hide resolved

zandercymatics commented Jan 5, 2024

View reviewed changes

zandercymatics changed the title ~~(Draft) Ticket #1501: Delete button for DomainApplication records~~ (On getgov-za) Ticket #1501: Delete button for DomainApplication records Jan 5, 2024

zandercymatics requested review from neilmb, therealslimhsiehdy, CocoByte, rachidatecs, dave-kennedy-ecs and erinysong January 5, 2024 22:35

allly-b previously requested changes Jan 19, 2024

View reviewed changes

Fix screenreader bug

928fa94

Fix Vo for delete

11b48eb

gabo0oo approved these changes Jan 19, 2024

View reviewed changes

zandercymatics added 2 commits January 19, 2024 15:54

Logic to remove orphaned contacts

d1daefd

Remove prints

5fca4a1

zandercymatics requested a review from allly-b January 19, 2024 23:22

zandercymatics mentioned this pull request Jan 22, 2024

(On getgov-za) Ticket #1484: Domain manager delete #1633

Merged

46 tasks

Add test case for delete

a119cab

Finish test cases

8a75c8f

zandercymatics commented Jan 22, 2024

View reviewed changes

allly-b approved these changes Jan 23, 2024

View reviewed changes

zandercymatics and others added 2 commits January 23, 2024 08:18

Update src/registrar/views/application.py

c07e83b

Co-authored-by: Alysia Broddrick <109625347+abroddrick@users.noreply.github.com>

Update src/registrar/tests/test_views.py

e5fbd65

Co-authored-by: Alysia Broddrick <109625347+abroddrick@users.noreply.github.com>

zandercymatics added 2 commits January 23, 2024 08:26

PR suggestions

7c4a0a6

Merge branch 'za/1501-users-delete-domain-records' of https://github.…

0d238e8

…com/cisagov/manage.get.gov into za/1501-users-delete-domain-records

Linting

688257a

zandercymatics merged commit 2d13f37 into main Jan 23, 2024
9 checks passed

zandercymatics deleted the za/1501-users-delete-domain-records branch January 23, 2024 15:48

(Not on a sandbox) Ticket #1501: Delete button for DomainApplication records #1590

(Not on a sandbox) Ticket #1501: Delete button for DomainApplication records #1590

Conversation

zandercymatics commented Jan 4, 2024 • edited by gabo0oo 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

Ensured code standards are met (Code reviewer)

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

Screenshots

github-actions bot commented Jan 4, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

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 Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

allly-b left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2024

github-actions bot commented Jan 19, 2024

gabo0oo left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 19, 2024

github-actions bot commented Jan 19, 2024

github-actions bot commented Jan 22, 2024

github-actions bot commented Jan 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allly-b 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

github-actions bot commented Jan 23, 2024

zandercymatics commented Jan 4, 2024 •

edited by gabo0oo

Loading

zandercymatics Jan 5, 2024 •

edited

Loading

zandercymatics Jan 23, 2024 •

edited

Loading