Skip to content

#3523: Standardize all failed email logger error messages - [RH] #3937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+175 −78
Open

#3523: Standardize all failed email logger error messages - [RH] #3937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
943d196
Update all failed email logger error messages
therealslimhsiehdy Jul 1, 2025
7365748
Fix error message unit tests
therealslimhsiehdy Jul 1, 2025
ff1ae77
Merge branch 'main' of https://github.com/cisagov/manage.get.gov into…
therealslimhsiehdy Jul 1, 2025
dbc5646
Forgot to remove a commented out line
therealslimhsiehdy Jul 1, 2025
0ee9acb
Fix logic for metadata and address other needed changes
therealslimhsiehdy Jul 2, 2025
a73f998
Merge branch 'main' of https://github.com/cisagov/manage.get.gov into…
therealslimhsiehdy Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions src/registrar/management/commands/email_current_metadata_report.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from django.conf import settings
from registrar.utility import csv_export
from io import StringIO
from ...utility.email import send_templated_email
from ...utility.email import send_templated_email, EmailSendingError


logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -46,9 +46,11 @@ def handle(self, **options):

logger.info("Generating report...")
try:
self.email_current_metadata_report(zip_filename, email_to)
success = self.email_current_metadata_report(zip_filename, email_to)
if not success:
# TODO - #1317: Notify operations when auto report generation fails
raise EmailSendingError("Report was generated but failed to send via email.")
except Exception as err:
# TODO - #1317: Notify operations when auto report generation fails
raise err
else:
logger.info(f"Success! Created {zip_filename} and successfully sent out an email!")
Expand Down Expand Up @@ -78,13 +80,24 @@ def email_current_metadata_report(self, zip_filename, email_to):
encrypted_zip_in_bytes = self.get_encrypted_zip(zip_filename, reports, password)

# Send the metadata file that is zipped
send_templated_email(
template_name="emails/metadata_body.txt",
subject_template_name="emails/metadata_subject.txt",
to_addresses=email_to,
context={"current_date_str": datetime.now().strftime("%Y-%m-%d")},
attachment_file=encrypted_zip_in_bytes,
)
try:
send_templated_email(
template_name="emails/metadata_body.txt",
subject_template_name="emails/metadata_subject.txt",
to_addresses=email_to,
context={"current_date_str": datetime.now().strftime("%Y-%m-%d")},
attachment_file=encrypted_zip_in_bytes,
)
return True
except EmailSendingError as err:
logger.error(
"Failed to send metadata email:\n"
f" Subject: metadata_subject.txt\n"
f" To: {email_to}\n"
f" Error: {err}",
exc_info=True,
)
return False

def get_encrypted_zip(self, zip_filename, reports, password):
"""Helper function for encrypting the attachment file"""
Expand Down
9 changes: 6 additions & 3 deletions src/registrar/management/commands/send_domain_invitations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,12 @@ def send_email(self, email_data):
)
except EmailSendingError as err:
logger.error(
f"email did not send successfully to {email_data['email']} "
f"for {[domain for domain in email_data['domains']]}"
f": {err}"
"Failed to send transition domain invitation email:\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got the following error when trying to test this script and prompting an error. I did this locally by automatically throwing an EmailSendingError in the try block's first line. For setup, I created a transition domain and tried running the script to prompt the email error but let me know if I did anything different from its expected use!

Traceback (most recent call last):
  File "/app/registrar/management/commands/send_domain_invitations.py", line 124, in send_email
    raise EmailSendingError
registrar.utility.email.EmailSendingError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/app/./manage.py", line 20, in <module>
    main()
  File "/app/./manage.py", line 16, in main
    execute_from_command_line(sys.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 442, in execute_from_command_line
    utility.execute()
  File "/usr/local/lib/python3.10/site-packages/django/core/management/__init__.py", line 436, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 412, in run_from_argv
    self.execute(*args, **cmd_options)
  File "/usr/local/lib/python3.10/site-packages/django/core/management/base.py", line 458, in execute
    output = self.handle(*args, **options)
  File "/app/registrar/management/commands/send_domain_invitations.py", line 66, in handle
    self.send_emails()
  File "/app/registrar/management/commands/send_domain_invitations.py", line 115, in send_emails
    self.send_email(email_data)
  File "/app/registrar/management/commands/send_domain_invitations.py", line 141, in send_email
    f"  Subject: {email_data['subject']}\n"
KeyError: 'subject'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my gosh great catch - I will edit the line to f" Subject: transition_domain_invitation_subject.txt\n" as we have the subject above, and the way I was trying to pull that info is incorrect

f" Subject: transition_domain_invitation_subject.txt\n"
f" To: {email_data['email']}\n"
f" Domains: {', '.join(email_data['domains'])}\n"
f" Error: {err}",
exc_info=True,
)
# if email failed to send, set error in domains_with_errors for each
# domain in the email so that transition domain email_sent is not set
Expand Down
12 changes: 10 additions & 2 deletions src/registrar/management/commands/send_expiring_soon_domains_notification.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,17 @@ def handle(self, *args, **options):
context=context,
)
logger.info(f"Sent email for domain {domain.name} to managers and CC’d org admins")
except EmailSendingError as e:
except EmailSendingError as err:
if not dryrun:
logger.warning(f"Failed to send email for domain {domain.name}. Reason: {e}")
logger.error(
"Failed to send expiring soon email(s):\n"
f" Subject: {subject_template}\n"
f" To: {', '.join(domain_manager_emails)}\n"
f" CC: {', '.join(portfolio_admin_emails)}\n"
f" Domain: {domain.name}\n"
f" Error: {err}",
exc_info=True,
)
all_emails_sent = False

if all_emails_sent:
Expand Down
13 changes: 11 additions & 2 deletions src/registrar/models/domain_request.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,8 +1032,17 @@ def _send_status_update_email(
wrap_email=wrap_email,
)
logger.info(f"The {new_status} email sent to: {recipient.email}")
except EmailSendingError:
logger.warning("Failed to send confirmation email", exc_info=True)
except EmailSendingError as err:
logger.error(
"Failed to send status update to creator email:\n"
f" Type: {new_status}\n"
f" Subject: {email_template_subject}\n"
f" To: {recipient.email}\n"
f" CC: {', '.join(cc_addresses)}\n"
f" BCC: {bcc_address}"
f" Error: {err}",
exc_info=True,
)

def investigator_exists_and_is_staff(self):
"""Checks if the current investigator is in a valid state for a state transition"""
Expand Down
42 changes: 26 additions & 16 deletions src/registrar/tests/test_email_invitations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -940,18 +940,22 @@ def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_se
permissions.user.email = "[email protected]"
permissions.portfolio.organization_name = "Test Portfolio"

mock_get_requestor_email.return_value = "[email protected]"
mock_get_requestor_email.return_value = MagicMock(name="mock.email")

# Call function
result = send_portfolio_member_permission_update_email(requestor, permissions)

# Assertions
mock_logger.warning.assert_called_once_with(
"Could not send email organization member update notification to %s for portfolio: %s",
permissions.user.email,
permissions.portfolio.organization_name,
exc_info=True,
expected_message = (
"Failed to send organization member update notification email:\n"
f" Requestor Email: {mock_get_requestor_email.return_value}\n"
f" Subject: portfolio_update_subject.txt\n"
f" To: {permissions.user.email}\n"
f" Portfolio: {permissions.portfolio}\n"
f" Error: Email failed"
)

mock_logger.error.assert_called_once_with(expected_message, exc_info=True)
self.assertFalse(result)

@patch("registrar.utility.email_invitations._get_requestor_email", side_effect=Exception("Unexpected error"))
Expand Down Expand Up @@ -1013,18 +1017,22 @@ def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_se
permissions.user.email = "[email protected]"
permissions.portfolio.organization_name = "Test Portfolio"

mock_get_requestor_email.return_value = "[email protected]"
mock_get_requestor_email.return_value = MagicMock(name="mock.email")

# Call function
result = send_portfolio_member_permission_remove_email(requestor, permissions)

# Assertions
mock_logger.warning.assert_called_once_with(
"Could not send email organization member removal notification to %s for portfolio: %s",
permissions.user.email,
permissions.portfolio.organization_name,
exc_info=True,
expected_message = (
"Failed to send portfolio member removal email:\n"
f" Requestor Email: {mock_get_requestor_email.return_value}\n"
f" Subject: portfolio_removal_subject.txt\n"
f" To: {permissions.user.email}\n"
f" Portfolio: {permissions.portfolio}\n"
f" Error: Email failed"
)

mock_logger.error.assert_called_once_with(expected_message, exc_info=True)
self.assertFalse(result)

@patch("registrar.utility.email_invitations._get_requestor_email", side_effect=Exception("Unexpected error"))
Expand Down Expand Up @@ -1092,10 +1100,12 @@ def test_send_email_failure(self, mock_logger, mock_get_requestor_email, mock_se
result = send_portfolio_invitation_remove_email(requestor, invitation)

# Assertions
mock_logger.warning.assert_called_once_with(
"Could not send email organization member removal notification to %s for portfolio: %s",
invitation.email,
invitation.portfolio.organization_name,
mock_logger.error.assert_called_once_with(
"Failed to send portfolio invitation removal email:\n"
f" Subject: portfolio_removal_subject.txt\n"
f" To: {invitation.email}\n"
f" Portfolio: {invitation.portfolio.organization_name}\n"
f" Error: Email failed",
exc_info=True,
)
self.assertFalse(result)
Expand Down
122 changes: 84 additions & 38 deletions src/registrar/utility/email_invitations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ def _send_domain_invitation_email(email, requestor_email, domains, requested_use
)
except EmailSendingError as err:
domain_names = ", ".join([domain.name for domain in domains])
logger.error(
"Failed to send domain invitation email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: domain_invitation_subject.txt\n"
f" To: {email}\n"
f" Domains: {domain_names}\n"
f" Error: {err}",
exc_info=True,
)
raise EmailSendingError(f"Could not send email invitation to {email} for domains: {domain_names}") from err


Expand Down Expand Up @@ -173,9 +182,15 @@ def _send_domain_invitation_update_emails_to_domain_managers(
"date": date.today(),
},
)
except EmailSendingError:
logger.warning(
f"Could not send email manager notification to {user.email} for domain: {domain.name}", exc_info=True
except EmailSendingError as err:
logger.error(
"Failed to send domain manager update notification email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: domain_manager_notification_subject.txt\n"
f" To: {user.email}\n"
f" Domain: {domain.name}\n"
f" Error: {err}",
exc_info=True,
)
all_emails_sent = False
return all_emails_sent
Expand Down Expand Up @@ -220,11 +235,15 @@ def send_domain_manager_removal_emails_to_domain_managers(
"date": date.today(),
},
)
except EmailSendingError:
logger.warning(
"Could not send notification email to %s for domain %s",
user.email,
domain.name,
except EmailSendingError as err:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be out of scope but I noticed when we fail to email domain managers that a manager has been added, we add an alert saying so
image

But when we remove a domain manager and fail to email the other domain managers, that update doesn't show
image

Are the alert error messages also in scope of this ticket or is that separate? We can create another ticket if it is out of scope!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create another ticket as this one is focused just on logs and not the error displays! Another great catch btw!!! 💯

logger.error(
"Failed to send domain manager deleted notification email:\n"
f" User that did the removing: {removed_by_user}\n"
f" Domain manager removed: {manager_removed_email}\n"
f" Subject: domain_manager_deleted_notification_subject.txt\n"
f" To: {user.email}\n"
f" Domain: {domain.name}\n"
f" Error: {err}",
exc_info=True,
)
all_emails_sent = False
Expand Down Expand Up @@ -265,6 +284,15 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_i
},
)
except EmailSendingError as err:
logger.error(
"Failed to send portfolio invitation email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: portfolio_invitation_subject.txt\n"
f" To: {email}\n"
f" Portfolio: {portfolio}\n"
f" Error: {err}",
exc_info=True,
)
raise EmailSendingError(
f"Could not sent email invitation to {email} for portfolio {portfolio}. Portfolio invitation not saved."
) from err
Expand Down Expand Up @@ -319,11 +347,14 @@ def send_portfolio_update_emails_to_portfolio_admins(editor, portfolio, updated_
"updated_info": updated_page,
},
)
except EmailSendingError:
logger.warning(
"Could not send email organization admin notification to %s " "for portfolio: %s",
user.email,
portfolio,
except EmailSendingError as err:
logger.error(
"Failed to send portfolio org update notification email:\n"
f" Requested User: {user}\n"
f" Subject: portfolio_org_update_notification_subject.txt\n"
f" To: {user.email}\n"
f" Portfolio: {portfolio}\n"
f" Error: {err}",
exc_info=True,
)
all_emails_sent = False
Expand Down Expand Up @@ -362,11 +393,14 @@ def send_portfolio_member_permission_update_email(requestor, permissions: UserPo
"date": date.today(),
},
)
except EmailSendingError:
logger.warning(
"Could not send email organization member update notification to %s " "for portfolio: %s",
permissions.user.email,
permissions.portfolio.organization_name,
except EmailSendingError as err:
logger.error(
"Failed to send organization member update notification email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: portfolio_update_subject.txt\n"
f" To: {permissions.user.email}\n"
f" Portfolio: {permissions.portfolio}\n"
f" Error: {err}",
exc_info=True,
)
return False
Expand Down Expand Up @@ -403,11 +437,14 @@ def send_portfolio_member_permission_remove_email(requestor, permissions: UserPo
"requestor_email": requestor_email,
},
)
except EmailSendingError:
logger.warning(
"Could not send email organization member removal notification to %s " "for portfolio: %s",
permissions.user.email,
permissions.portfolio.organization_name,
except EmailSendingError as err:
logger.error(
"Failed to send portfolio member removal email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: portfolio_removal_subject.txt\n"
f" To: {permissions.user.email}\n"
f" Portfolio: {permissions.portfolio}\n"
f" Error: {err}",
exc_info=True,
)
return False
Expand Down Expand Up @@ -444,11 +481,13 @@ def send_portfolio_invitation_remove_email(requestor, invitation: PortfolioInvit
"requestor_email": requestor_email,
},
)
except EmailSendingError:
logger.warning(
"Could not send email organization member removal notification to %s " "for portfolio: %s",
invitation.email,
invitation.portfolio.organization_name,
except EmailSendingError as err:
logger.error(
"Failed to send portfolio invitation removal email:\n"
f" Subject: portfolio_removal_subject.txt\n"
f" To: {invitation.email}\n"
f" Portfolio: {invitation.portfolio.organization_name}\n"
f" Error: {err}",
exc_info=True,
)
return False
Expand Down Expand Up @@ -497,11 +536,15 @@ def _send_portfolio_admin_addition_emails_to_portfolio_admins(email: str, reques
"date": date.today(),
},
)
except EmailSendingError:
logger.warning(
"Could not send email organization admin notification to %s " "for portfolio: %s",
user.email,
portfolio.organization_name,
except EmailSendingError as err:
logger.error(
"Failed to send portfolio admin addition notification email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: portfolio_admin_addition_notification_subject.txt\n"
f" To: {user.email}\n"
f" Portfolio: {portfolio}\n"
f" Portfolio Admin: {user}\n"
f" Error: {err}",
exc_info=True,
)
all_emails_sent = False
Expand Down Expand Up @@ -550,11 +593,14 @@ def _send_portfolio_admin_removal_emails_to_portfolio_admins(email: str, request
"date": date.today(),
},
)
except EmailSendingError:
logger.warning(
"Could not send email organization admin notification to %s " "for portfolio: %s",
user.email,
portfolio.organization_name,
except EmailSendingError as err:
logger.error(
"Failed to send portfolio admin removal notification email:\n"
f" Requestor Email: {requestor_email}\n"
f" Subject: portfolio_admin_removal_notification_subject.txt\n"
f" To: {user.email}\n"
f" Portfolio: {portfolio.organization_name}\n"
f" Error: {err}",
exc_info=True,
)
all_emails_sent = False
Expand Down
12 changes: 7 additions & 5 deletions src/registrar/views/domain.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,11 +375,13 @@ def email_domain_managers(self, domain: Domain, template: str, subject_template:
context["recipient"] = manager
try:
send_templated_email(template, subject_template, to_addresses=[manager.email], context=context)
except EmailSendingError:
logger.warning(
"Could not send notification email to %s for domain %s",
manager.email,
domain.name,
except EmailSendingError as err:
logger.error(
"Failed to send notification email:\n"
f" Subject: {subject_template}\n"
f" To: {manager.email}\n"
f" Domain: {domain.name}\n"
f" Error: {err}",
exc_info=True,
)

Expand Down
Loading