#2307: send notification emails on changes to domain - [MS] #2827

Matt-Spence · 2024-09-19T20:43:08Z

Ticket

Resolves #2307

Changes

Notifications are sent to all domain managers when the following are changed on a domain:
- Nameservers
- DNSSec
- Senior Official
- Org name/Address
- Security Email
New additions to email utility to enable CC'ed addresses
New email and subject template reflecting content from here
Content changes to domain managers page as outlined here
Tests
Edits to some old tests that broke because of the new email sending logic (nothing major)

Context for reviewers

Setup

Code Review Verification Steps

Go to getgov-ms and add yourself as a domain manager to any domain. Make sure your email is in the whitelist, then go to that domain and make changes. Observe whether emails are sent, and their content.

Repeat for various permutations (multiple domain managers, some managers in/out of whitelist, domains in/out of portfolios, etc).

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-19T21:46:34Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-19T21:46:48Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-19T21:50:22Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-19T22:02:37Z

🥳 Successfully deployed to developer sandbox ms.

…tion-emails

github-actions · 2024-09-19T22:18:45Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-20T18:16:30Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-20T18:25:19Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-20T18:33:30Z

🥳 Successfully deployed to developer sandbox ms.

Matt-Spence · 2024-09-25T20:33:08Z

src/registrar/config/settings.py

+        if not hasattr(record, "server_time"):
+            record.server_time = self.formatTime(record, self.datefmt)


This was a minor fix to an edge case where this attribute doesn't exist, causing log formatting to revert back to its old "one log per \n" behavior. I can include it as a separate PR if we want, but thought it was small enough to sneak in.

Looks great

github-actions · 2024-09-25T20:36:11Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-25T20:49:08Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-25T21:10:06Z

🥳 Successfully deployed to developer sandbox ms.

Matt-Spence · 2024-09-25T22:27:16Z

src/registrar/views/domain.py

@@ -118,6 +119,7 @@ def post(self, request, *args, **kwargs):
        if form.is_valid():
            return self.form_valid(form)
        else:
+            logger.debug(f"Form errors: {form.errors}")


Not strictly necessary, but I found this super useful while debugging this PR so thought I might leave it in.

github-actions · 2024-09-26T16:03:06Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-26T16:24:49Z

🥳 Successfully deployed to developer sandbox ms.

github-actions · 2024-09-26T21:47:43Z

🥳 Successfully deployed to developer sandbox ms.

zandercymatics

Nice work, I've noted a few changes that should be made though

zandercymatics · 2024-10-01T15:52:16Z

src/registrar/tests/test_emails.py

@@ -61,11 +61,27 @@ def test_disable_email_flag(self):
        # Assert that an email wasn't sent
        self.assertFalse(self.mock_client.send_email.called)

+    @boto3_mocking.patching
+    def test_email_with_cc(self):


(optional) Just in case, I'd also recommend test with the decorator @override_settings(IS_PRODUCTION=True) and check the bcc_address as well as the cc_address The behaviour of that function changes when in production so it would catch any edge cases related to that

zandercymatics · 2024-10-01T15:54:15Z

src/registrar/tests/test_emails.py

+        with boto3_mocking.clients.handler_for("sesv2", self.mock_client_class):
+            send_templated_email(
+                "emails/update_to_approved_domain.txt",
+                "emails/update_to_approved_domain_subject.txt",
+                "doesnotexist@igorville.com",
+                context={"domain": "test", "user": "test", "date": 1, "changes": "test"},
+                bcc_address=None,
+                cc_addresses=["testy2@town.com", "mayor@igorville.gov"],
+            )
+
+        # check that an email was sent
+        self.assertTrue(self.mock_client.send_email.called)


You should also include a test here that checks on the content of cc_addresses.
We have a few tests that test on the bcc_addresses - should be mostly the same logic for it

zandercymatics · 2024-10-01T15:54:58Z

src/registrar/tests/test_emails.py

    @boto3_mocking.patching
    @less_console_noise_decorator
    def test_submission_confirmation(self):
        """Submission confirmation email works."""
-        domain_request = completed_domain_request()
+        domain_request = completed_domain_request(user=User.objects.create(username="test", email="testy@town.com"))


(Q) Why was this changed? Its not a bad thing though, just curious

By default the completed_domain_request function creates a user with no email. Technically what was happening previously was that emails were being "sent" (the send_email function was being called) but without any to_address because when the domain request was submitted, it would grab an empty string from the user's email. That was a valid case previously (not sure if that was intentional) but now I added a check to make sure that there is some email address in to, bcc, or cc. As a result, these tests broke and to fix it I had to add a user object with an email.

Definitely a bug! Good catch

zandercymatics · 2024-10-01T15:56:10Z

src/registrar/tests/test_views_domain.py

+        self.security_contact, _ = PublicContact.objects.get_or_create(
+            domain=self.domain,
+            contact_type=PublicContact.ContactTypeChoices.SECURITY,
+            email="security@igorville.gov",
+        )


Are you using this anywhere? If not can you delete it?

Nope, that was left over from a previous iteration of one of the tests! Good catch.

zandercymatics · 2024-10-01T15:56:28Z

src/registrar/tests/test_views_domain.py

+            AllowedEmail(email="info@example.com"),
+            AllowedEmail(email="doesnotexist@igorville.com"),
+        ]
+        AllowedEmail.objects.bulk_create(allowed_emails)


Nice use of bulk_create

zandercymatics · 2024-10-01T17:03:19Z

src/registrar/views/domain.py

+        # forms of these types should not send notifications if they're part of a portfolio/Organization
+        check_for_portfolio = {
+            DomainOrgNameAddressForm,
+            SeniorOfficialContactForm,
+        }
+


(A) Because it was in the AC's :). Actually though, I think these cases will be handled separately in the org model work.

zandercymatics · 2024-10-01T17:09:22Z

src/registrar/views/domain.py

+            # don't notify for any other types of forms
+            should_notify = False
+            logger.info(f"Not notifying for {form.__class__}")
+        if (should_notify and form.has_changed()) or force_send:


(nitpick/optional) Personally, I think the should_notify variable in this context is moreso better described as can_notify rather than should_notify

I actually disagree, we technically can notify on changes with portfolios (as in, there's no technical barrier) but we have made a decision that we shouldn't. I like the attention to naming though! Good naming is important for whatever unfortunate soul has to maintain this code later on!

zandercymatics · 2024-10-01T17:13:25Z

src/registrar/views/domain.py

+                "changes": form_label_dict[form.__class__],
+            }
+            self.email_domain_managers(
+                self.object,


self.object is typically the model itself.
Later on its being used as domain_name. Is self.object a string in this context?

zandercymatics · 2024-10-01T17:14:38Z

src/registrar/views/domain.py

+        Will log a warning if the email fails to send for any reason, but will not raise an error.
+        """
+        try:
+            domain = Domain.objects.get(name=domain_name)


Per above, I think self.object should already be the object assuming its not just a string.
I'd suggest grabbing that directly rather than a filter. If that returns none then you can log as normal

Yeah I agree. This works technically because (I think) when you treat a domain model as a string it uses the name, but it's clunky at best.

src/registrar/views/domain.py

Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com>

github-actions · 2024-10-02T16:42:41Z

🥳 Successfully deployed to developer sandbox ms.

…/cisagov/manage.get.gov into ms/2307-send-notification-emails

github-actions · 2024-10-02T17:00:22Z

🥳 Successfully deployed to developer sandbox ms.

zandercymatics

It works! One thing I noticed though is that I am receiving the email by being directly CC'd rather than it just being to my address, I assume that is not intentional?

Not blocking the review on it, but just be sure to resolve it given that design is in agreement (since they'll need to review this anyhow)

Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com>

Matt-Spence · 2024-10-02T20:21:30Z

It works! One thing I noticed though is that I am receiving the email by being directly CC'd rather than it just being to my address, I assume that is not intentional?

Not blocking the review on it, but just be sure to resolve it given that design is in agreement (since they'll need to review this anyhow)

Actually it is intentional, as the AC's specifically asked for domain managers to be CC'ed on the same email. It is a little awkward for cases where there's only one domain manager, but I'm not sure it matters much either way. I'll defer to design.

zandercymatics · 2024-10-02T20:28:53Z

@Matt-Spence Fair enough

github-actions · 2024-10-02T20:30:36Z

🥳 Successfully deployed to developer sandbox ms.

Matt-Spence added 12 commits September 10, 2024 15:37

send notification on nameserver changes

9c090ae

test on nameservers

3475a76

add temp email template and subject

6c60556

debug logs

1b7408a

MOAR LOGS

9609db5

MOAR DEBUG LOGS

0ffff70

more debug messages

d6996bd

change to info level

579a8ac

tested up to email sending

9bbcb98

another stab at email sending

b8d697e

add email sending to all required forms

f254e04

test simpler way to organize which emails to send

4ce28fd

Matt-Spence requested a review from abroddrick as a code owner September 19, 2024 20:43

Matt-Spence and others added 3 commits September 19, 2024 16:05

cleanup and add email content

2f270cd

more cleanup

e0025df

fix email template/subject name

68b8c7d

add super call for form valid on security email form

58d4215

Merge remote-tracking branch 'origin/main' into ms/2307-send-notifica…

859da46

…tion-emails

another refactor

63eea77

fix minor bug in logger formatting

07c1060

typo fix

cb70056

map forms to labels

5dbc356

Matt-Spence commented Sep 25, 2024

View reviewed changes

minor email refactor

7a4ed7a

fix email test

0c9db26

Matt-Spence commented Sep 25, 2024

View reviewed changes

abroddrick added the carryover Carryover from a previous sprint label Sep 26, 2024

fix tests

2e3ff9e

Matt-Spence changed the title ~~(draft) #2307: send notification emails on changes to domain - [MS]~~ #2307: send notification emails on changes to domain - [MS] Sep 26, 2024

linter errors

7d9eabb

remove extra logging statements

65d8987

zandercymatics requested changes Oct 1, 2024

View reviewed changes

zandercymatics self-assigned this Oct 1, 2024

Update src/registrar/utility/email.py

9940b16

Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com>

Matt-Spence added 2 commits October 2, 2024 11:54

review changes

f26db71

Merge branch 'ms/2307-send-notification-emails' of https://github.com…

aa8a4a1

…/cisagov/manage.get.gov into ms/2307-send-notification-emails

zandercymatics approved these changes Oct 2, 2024

View reviewed changes

Update src/registrar/views/domain.py

a42ea0a

Co-authored-by: zandercymatics <141044360+zandercymatics@users.noreply.github.com>

Matt-Spence requested a review from a team October 2, 2024 20:23

Katherine-Osos requested review from Katherine-Osos and removed request for a team October 3, 2024 18:04

		if not hasattr(record, "server_time"):
		record.server_time = self.formatTime(record, self.datefmt)

#2307: send notification emails on changes to domain - [MS] #2827

Are you sure you want to change the base?

#2307: send notification emails on changes to domain - [MS] #2827

Conversation

Matt-Spence commented Sep 19, 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

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 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

github-actions bot commented Sep 25, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 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

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

Choose a reason for hiding this comment

github-actions bot commented Oct 2, 2024

github-actions bot commented Oct 2, 2024

zandercymatics left a comment

Choose a reason for hiding this comment

Matt-Spence commented Oct 2, 2024

zandercymatics commented Oct 2, 2024

github-actions bot commented Oct 2, 2024

Matt-Spence commented Sep 19, 2024 •

edited

Loading