#2355: Custom rejection reason emails - [BACKUP] - MIGRATION #2869

zandercymatics · 2024-09-30T15:50:20Z

Ticket

Resolves #2355

Changes

Adds the ability to edit rejection reason emails, like what we're doing with action needed
Consolidates the custom email logic to make it easier to change in the future + make additions

Context for reviewers

This PR makes the rejection reason emails editable, like what we're doing with action needed.
The only (current) difference between the two is that for rejection you can send emails out in the state "other". The only reason for this is that we had content for it. However, this can be changed if it is deemed that it needs changing.

As it turns out, the old logic for action needed email wasn't as flexible as we would have hoped. The infra had to be changed to accommodate for both rejection reason and action needed. This is the main reason for the line count

Setup

Pick a domain request in django admin
Set the status to rejected
Send out an email, the email field should auto-update for each selection
After sending, note that the content has changed to denote this fact.
Pick another domain request and do the same thing - but this time edit the email

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 associated 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 associated 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-09-30T15:54:48Z

🥳 Successfully deployed to developer sandbox hotgov.

zandercymatics · 2024-09-30T16:07:09Z

src/registrar/assets/js/get-gov-admin.js

-/** An IIFE for admin in DjangoAdmin to listen to changes on the domain request
- * status select and to show/hide the rejection reason
-*/
-(function (){


This logic is now consolidated inside of the CustomizableEmailBase class.
Otherwise, if we add a third customizable field in the future, we'd have to remember to change this on top of the other changes.

github-actions · 2024-09-30T16:09:30Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-09-30T16:25:09Z

🥳 Successfully deployed to developer sandbox hotgov.

zandercymatics · 2024-09-30T16:32:21Z

src/registrar/assets/js/get-gov-admin.js

@@ -500,114 +437,316 @@ function initializeWidgetOnList(list, parentId) {
    }
 })();

+class CustomizableEmailBase {


As I was implementing this, I noticed a lot of required code repetition in many different places.
If you weren't very familiar with this part of the codebase it would be painful to work with.

Not only would this cause code drift between rejected and action needed, it would make adding a third custom status email (like for ineligible) difficult.

We already do something similar for the domain / domain request tables

github-actions · 2024-09-30T18:37:56Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-09-30T20:39:53Z

🥳 Successfully deployed to developer sandbox hotgov.

zandercymatics · 2024-09-30T21:08:25Z

src/registrar/models/domain_request.py

-        # Send out an email if an action needed reason exists
-        if self.action_needed_reason and self.action_needed_reason != self.ActionNeededReasons.OTHER:
-            email_content = self.action_needed_reason_email
-            self._send_action_needed_reason_email(send_email, email_content)


This is now centralized in def save().

The reason why this is not being done exclusively in action_needed is because one of the ACs conflicts with how the FSM works. More specifically, we want to send out a second email when you directly change the action_needed_reason even if the state is already action_needed.

Rather than duplicating logic twice, of which it is already hard to track, we can centralize everything into save for now. The long term solution for this would probably require a refactor of how we're doing emails in general.

github-actions · 2024-09-30T21:09:44Z

🥳 Successfully deployed to developer sandbox hotgov.

Remove refactors + simplify logic

github-actions · 2024-10-01T15:29:23Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-01T15:32:30Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-08T18:55:27Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-08T18:57:23Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-09T14:38:29Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-09T14:40:02Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-09T14:41:42Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-09T14:45:41Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-09T15:09:13Z

🥳 Successfully deployed to developer sandbox hotgov.

rachidatecs · 2024-10-09T15:19:03Z

src/registrar/admin.py

@@ -1939,6 +1927,15 @@ def save_model(self, request, obj, form, change):
            if should_save:
                return super().save_model(request, obj, form, change)

+    def _handle_custom_emails(self, obj):


Much cleaner

rachidatecs · 2024-10-09T15:24:25Z

src/registrar/assets/js/get-gov-admin.js

+        });
+    }
+
+    initializeModalConfirm() {


Whoa! Unlike you not to do something about this repetitive code!

rachidatecs · 2024-10-09T15:28:23Z

src/registrar/assets/js/get-gov-admin.js

+     * @property {string} sessionVariableName - The session variable name. Used for show/hide on textAreaFormGroup/dropdownFormGroup.
+     * @property {string} apiErrorMessage - The error message that the ajax call returns.
+     */
+    constructor(config) {        


If one of the config props does not come through, JS execution will lock up. OPTIONAL: check that all expected config props are there otherwise return early? At the start of the DOM loaded listener.

Sure, makes sense

rachidatecs · 2024-10-09T22:06:19Z

src/registrar/models/domain_request.py

+
+        # Only send out an email if the underlying reason itself changed or if no email was sent previously.
+        if status_info.get("cached_reason") != status_info.get("reason") or status_info.get("cached_reason") is None:
+            bcc_address = settings.DEFAULT_FROM_EMAIL if settings.IS_PRODUCTION else ""


I feel like you missed an opportunity here

Can you explain more of your thought process?

rachidatecs · 2024-10-09T22:07:15Z

src/registrar/models/domain_request.py

+                bcc_address=bcc_address,
+                custom_email_content=status_info.get("email"),
+                wrap_email=status_information.get("wrap_email"),
+            )


OPTIONAL: Do we want an else block here that logs that send_custom_status_update_email met unexpected conditions?

Like a try/catch, or just checking for the values of status_info / bcc_address? Not sure I understand

The function gets called. Inside the function, stuff happens only when certain conditions are right, otherwise it's just a ghost function silently running with no effect. I think we should log something to the effect of 'ran silently' which indicates an issue with our app or data

Ah true. Yeah I can add more logging here

Ok, added logging on some of the fails. Disagree on logging on the if status_info.get("cached_reason") != status_info.get("reason") or status_info.get("cached_reason") is None: bit since we aren't supposed to be sending an email in this state anyway . Its not really a failure in this state, but intentional. It'll just create cluttered logs on every save for the model

rachidatecs · 2024-10-09T22:18:44Z

src/registrar/templates/django/admin/includes/detail_table_fieldset.html

+        <button
+            aria-label="Edit email in textarea"
+            type="button"
+            class="usa-button usa-button--unstyled usa-button--dja-link-color usa-button__small-text margin-left-1 text-no-underline field-rejection_reason_email__edit flex-align-self-start"


Since these buttons/ modals will never exist on the same page at the same time, I think there's an opportunity to significantly simplify the HTML and the JS by using generic handles (email-edit instead of field-rejection_reason_email__edit).

On the JS: You can now trim down config by moving a lot of the handles to the parent class

On the template: You can probably pull out the modals into an include that's a variant of modal.html, if not modal.html itself (that would be ideal)

It's not insignificant and your code has nothing wrong with it, so I won' block if you decide not to do it, but we should capture this plan for the next custom email ticket if it exists.

Since these buttons/ modals will never exist on the same page at the same time

Ah, I think they do actually! Basically the blocks for detail_table_fieldset.html are nested inside in a for loop on each field present on the page. You can verify this via inspect element or by breaking the js somewhere to prevent hiding dom elements.

However, I do really like your train of thought here. We could achieve what your describing by assigning those generic names as classes and having the js infer what the right values are given their expected dom position

You can probably pull out the modals into an include that's a variant of modal.html, if not modal.html itself

You're totally right about that. I'll look at it and see if this is easily achievable (it might be)

Okay. I went through and simplified the javascript (though more can be done with the formgroups). Let me know what you think.

For the templates + further refinement we can definitely do that in a follow-on like you said

rachidatecs · 2024-10-09T22:19:50Z

src/registrar/tests/test_admin_request.py

@@ -632,6 +637,7 @@ def assert_email_is_accurate(
        with less_console_noise():
            # Access the arguments passed to send_email
            call_args = self.mock_client.EMAILS_SENT
+            logger.info(f"what are the call args? {call_args}")


BLOCKING: Remove

Great catch

rachidatecs · 2024-10-09T22:21:49Z

src/registrar/tests/test_admin_request.py

+        """When a rejection reason is set, an email is sent out and [email protected]
+        is BCC'd in production"""
+        # Create fake creator
+        EMAIL = "[email protected]"


Q: Why caps? Good practice for const? I've rarely seen them in the project.

Honestly its just because I copy + pasted another test over as a base, but I do think what you said is correct. What I do like about it here is that its visually distinct when skimming the test though

rachidatecs · 2024-10-09T22:25:33Z

src/registrar/tests/test_models_requests.py

@@ -337,13 +337,6 @@ def test_withdraw_sends_email(self):
            domain_request, msg, "withdraw", 1, expected_content="withdrawn", expected_email=user.email
        )

-    @less_console_noise_decorator


Q: This test should still work. Is that not the case? If so why?

Looked at it again and figured it out. Just had to modify check_email_sent for this function specifically (just moved the logic out for this one). Thanks for pointing it out.

Basically this test was failing because reject wasn't calling the send email function anymore, as we now handle that on save and with javascript. I thought it wasn't relevant initially, but thinking on it now the fact that we have some save logic means we should keep it anyway

rachidatecs · 2024-10-09T22:26:23Z

src/registrar/utility/admin_helpers.py

+        domain_request,
+        file_path="emails/status_change_rejected.txt",
+        reason=rejection_reason,
+        # excluded_reasons=[DomainRequest.RejectionReasons.OTHER]


No opinion on this

rachidatecs · 2024-10-09T22:31:58Z

500 Err on backup when I try to access a domain request

rachidatecs · 2024-10-09T22:48:15Z

Get to a in-review domain request as an admin
Change the creator from eg Paul to Rachid
Select a status with a custom email
Note: The email text will still address Paul, but the email triggered on Save will go to Rachid. I do NOT think this shoud be addressed, just something to be aware of.

github-actions · 2024-10-10T15:10:40Z

🥳 Successfully deployed to developer sandbox hotgov.

zandercymatics · 2024-10-10T16:00:02Z

Note: The email text will still address Paul, but the email triggered on Save will go to Rachid. I do NOT think this shoud be addressed, just something to be aware of.

Yeah good callout, basically to solve this we'd need to pass in the creator field since whats in the db doesn't match whats on the form in this state. This would complicate the code a bit, and I don't think its that harmful because most the time in prod the creator isn't being changed on email send + they can change it anyway if they need to

github-actions · 2024-10-10T16:05:13Z

🥳 Successfully deployed to developer sandbox hotgov.

github-actions · 2024-10-10T16:09:10Z

🥳 Successfully deployed to developer sandbox hotgov.

witha-c

Looks good. Adding a note here that I noticed a small part of the email content in the email footer was different from our content google docs and sent the list to @Katherine-Osos to decide whether to update the docs or the emails.

zandercymatics · 2024-10-10T20:11:46Z

@witha-c Awesome, thanks! I didn't modify their content at all - so that must've been inaccurate for some time! Good find

github-actions · 2024-10-10T20:17:00Z

🥳 Successfully deployed to developer sandbox hotgov.

zandercymatics added 6 commits September 24, 2024 14:22

Refactor

61b1cee

migration

48b9206

Initial architecture for rejection reason

9b23262

Cleanup + send email logic

1ce0724

Typo

e37a95e

check for page

c084ec6

zandercymatics requested a review from abroddrick as a code owner September 30, 2024 15:50

Remove unhelpful logic

ff5212f

zandercymatics commented Sep 30, 2024

View reviewed changes

Fix back button bug

7a5eda0

zandercymatics commented Sep 30, 2024

View reviewed changes

zandercymatics changed the title ~~(Draft) Ticket #2355: Custom rejection reason emails~~ (Draft) #2355: Custom rejection reason emails Sep 30, 2024

zandercymatics changed the title ~~(Draft) #2355: Custom rejection reason emails~~ (Draft) #2355: Custom rejection reason emails - [HOTGOV] Sep 30, 2024

zandercymatics changed the title ~~(Draft) #2355: Custom rejection reason emails - [HOTGOV]~~ (Draft) #2355: Custom rejection reason emails - [HOTGOV] - MIGRATION Sep 30, 2024

cleanup

cfa1879

cleanup

7cc5231

Simplify backend logic

6bb907c

zandercymatics commented Sep 30, 2024

View reviewed changes

zandercymatics added 2 commits October 1, 2024 09:24

Simplify PR

5a96855

Remove refactors + simplify logic

Update domain_request.py

47d226f

zandercymatics added 2 commits October 1, 2024 09:31

Merge branch 'main' into hotgov/2355-rejection-reason-emails

62156ac

add migration

162369f

zandercymatics added 3 commits October 9, 2024 08:33

Merge branch 'main' into hotgov/2355-rejection-reason-emails

fc8090d

Update test_models.py

7c69205

Update test_models.py

3ba4293

Remove space

bd603f6

remove bad test + lint

ec1df25

rachidatecs reviewed Oct 9, 2024

View reviewed changes

Fix test + some minor cleanup stuff

dbfc548

simplify javascript

3e54bb1

lint

3fb9ab5

rachidatecs self-requested a review October 10, 2024 17:52

rachidatecs approved these changes Oct 10, 2024

View reviewed changes

witha-c approved these changes Oct 10, 2024

View reviewed changes

Merge branch 'main' into hotgov/2355-rejection-reason-emails

8b70554

zandercymatics merged commit a1d9904 into main Oct 10, 2024
10 checks passed

zandercymatics deleted the hotgov/2355-rejection-reason-emails branch October 10, 2024 20:51

#2355: Custom rejection reason emails - [BACKUP] - MIGRATION #2869

#2355: Custom rejection reason emails - [BACKUP] - MIGRATION #2869

Conversation

zandercymatics commented Sep 30, 2024 • edited by witha-c 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 Sep 30, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

zandercymatics Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

zandercymatics Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024

github-actions bot commented Oct 1, 2024

github-actions bot commented Oct 1, 2024

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 8, 2024

github-actions bot commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

github-actions bot commented Oct 9, 2024

github-actions bot commented Oct 9, 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

Choose a reason for hiding this comment

zandercymatics Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zandercymatics Oct 10, 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

zandercymatics Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rachidatecs commented Oct 9, 2024

rachidatecs commented Oct 9, 2024

github-actions bot commented Oct 10, 2024

zandercymatics commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024

witha-c left a comment

Choose a reason for hiding this comment

zandercymatics commented Oct 10, 2024 • edited Loading

github-actions bot commented Oct 10, 2024

zandercymatics commented Sep 30, 2024 •

edited by witha-c

Loading

zandercymatics Sep 30, 2024 •

edited

Loading

zandercymatics Sep 30, 2024 •

edited

Loading

zandercymatics Oct 10, 2024 •

edited

Loading

zandercymatics Oct 10, 2024 •

edited

Loading

zandercymatics Oct 10, 2024 •

edited

Loading

zandercymatics Oct 10, 2024 •

edited

Loading

zandercymatics Oct 10, 2024 •

edited

Loading

zandercymatics commented Oct 10, 2024 •

edited

Loading