-
Notifications
You must be signed in to change notification settings - Fork 26
#3940: Update messages for adding a domain manager - [aa] #4225
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.
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
Changes from 14 commits
f1131f0
9afe1b8
de57ae3
45c9077
60f35c4
9ca35d4
d3cf495
3f1803b
c6b5187
09d9f36
9832cf9
da07008
2103075
9ac5424
458c8bf
7f5cd18
14f8d97
465e6cc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1884,7 +1884,7 @@ def save_model(self, request, obj, form, change): | |
if requested_user is not None: | ||
portfolio_invitation.retrieve() | ||
portfolio_invitation.save() | ||
messages.success(request, f"{requested_email} has been invited to the organization: {domain_org}") | ||
messages.success(request, f"{requested_email} has been invited to become a member of {domain_org}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ dm-add-alert005: Noticed that when we add a domain manager who is not yet a member of the domain's organization, we have 2 error messages. The first one is the correctly updated alert message, but I was wondering if we should be seeing that bottom one as well? It doesn't match any of the error messages from 006-010 ![]() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure if the messages and alert type (success + failure) may be confusing or contradictory but @witha-k @SamiyahKey will defer to your judgment since you're more well-versed in this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is intended per @witha-k 's conversations with me. Tagging @SamiyahKey as backup. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it still makes sense to keep both. It's letting the user know that whomever they added was not a member of this organization (which is why a domain invitation could not be sent) and that an portfolio invitation to join the organization was sent instead. Otherwise, the user might not know why a domain invitation was not sent. And if we don't keep the success message, they might not know that a portfolio invitation was sent instead... |
||
|
||
if not send_domain_invitation_email( | ||
email=requested_email, | ||
|
@@ -1893,7 +1893,7 @@ def save_model(self, request, obj, form, change): | |
is_member_of_different_org=member_of_a_different_org, | ||
requested_user=requested_user, | ||
): | ||
messages.warning(request, "Could not send email confirmation to existing domain managers.") | ||
messages.warning(request, "Could not send email notification to existing domain managers.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert011: alert message updated |
||
if requested_user is not None: | ||
# Domain Invitation creation for an existing User | ||
obj.retrieve() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -399,7 +399,7 @@ def test_add_domain_invitation_success_when_user_not_portfolio_member( | |
# Assert success message | ||
mock_messages_success.assert_has_calls( | ||
[ | ||
call(request, "[email protected] has been invited to the organization: new portfolio"), | ||
call(request, "[email protected] has been invited to become a member of new portfolio"), | ||
call(request, "[email protected] has been invited to the domain: example.com"), | ||
] | ||
) | ||
|
@@ -656,7 +656,7 @@ def test_add_domain_invitation_when_user_not_portfolio_member_raises_exception_s | |
|
||
# Assert success message | ||
mock_messages_success.assert_called_once_with( | ||
request, "[email protected] has been invited to the organization: new portfolio" | ||
request, "[email protected] has been invited to become a member of new portfolio" | ||
) | ||
|
||
# Assert error message | ||
|
@@ -784,7 +784,7 @@ def test_add_domain_invitation_success_when_email_not_portfolio_member( | |
# Assert success message | ||
mock_messages_success.assert_has_calls( | ||
[ | ||
call(request, "[email protected] has been invited to the organization: new portfolio"), | ||
call(request, "[email protected] has been invited to become a member of new portfolio"), | ||
call(request, "[email protected] has been invited to the domain: example.com"), | ||
] | ||
) | ||
|
@@ -1018,7 +1018,7 @@ def test_add_domain_invitation_when_user_not_portfolio_email_raises_exception_se | |
|
||
# Assert success message | ||
mock_messages_success.assert_called_once_with( | ||
request, "[email protected] has been invited to the organization: new portfolio" | ||
request, "[email protected] has been invited to become a member of new portfolio" | ||
) | ||
|
||
# Assert error message | ||
|
@@ -1503,7 +1503,7 @@ def test_save_exception_email_sending_error(self, mock_messages_error, mock_send | |
self.client.force_login(self.superuser) | ||
|
||
# Mock the email sending function to raise EmailSendingError | ||
mock_send_email.side_effect = EmailSendingError("Email service unavailable") | ||
mock_send_email.side_effect = EmailSendingError("Email service unavailable.") | ||
|
||
# Create an instance of the admin class | ||
admin_instance = PortfolioInvitationAdmin(PortfolioInvitation, admin_site=None) | ||
|
@@ -1521,9 +1521,16 @@ def test_save_exception_email_sending_error(self, mock_messages_error, mock_send | |
|
||
# Call the save_model method | ||
admin_instance.save_model(request, portfolio_invitation, None, None) | ||
msg = ( | ||
"Email service unavailable. Try again, and if the problem persists, " | ||
'<a href="https://get.gov/contact" class="usa-link" target="_blank">contact us</a>.' | ||
) | ||
|
||
# Assert that messages.error was called with the correct message | ||
mock_messages_error.assert_called_once_with(request, "Email service unavailable") | ||
mock_messages_error.assert_called_once_with( | ||
request, | ||
msg, | ||
) | ||
|
||
@less_console_noise_decorator | ||
@patch("registrar.admin.send_portfolio_invitation_email") | ||
|
@@ -1585,8 +1592,17 @@ def test_save_exception_generic_error(self, mock_messages_error, mock_send_email | |
# Call the save_model method | ||
admin_instance.save_model(request, portfolio_invitation, None, None) | ||
|
||
msg = ( | ||
"An unexpected error occurred: [email protected] could not be added to this domain. " | ||
'Try again, and if the problem persists, <a href="https://get.gov/contact" ' | ||
'class="usa-link" target="_blank">contact us</a>.' | ||
) | ||
|
||
# Assert that messages.error was called with the correct message | ||
mock_messages_error.assert_called_once_with(request, "Could not send email invitation.") | ||
mock_messages_error.assert_called_once_with( | ||
request, | ||
msg, | ||
) | ||
|
||
@less_console_noise_decorator | ||
@patch("registrar.admin.send_portfolio_admin_addition_emails") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -523,7 +523,10 @@ def test_send_portfolio_invitation_email_failure(self, mock_send_templated_email | |
with self.assertRaises(EmailSendingError) as context: | ||
send_portfolio_invitation_email(self.email, self.requestor, self.portfolio, is_admin_invitation) | ||
|
||
self.assertIn("Could not sent email invitation to", str(context.exception)) | ||
self.assertIn( | ||
"An unexpected error occurred: [email protected] could not be added to this domain.", | ||
str(context.exception), | ||
) | ||
|
||
@less_console_noise_decorator | ||
@patch( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,7 +103,7 @@ def _send_domain_invitation_email(email, requestor_email, domains, requested_use | |
f" Error: {err}", | ||
exc_info=True, | ||
) | ||
raise EmailSendingError(f"Could not send email invitation to {email} for domains: {domain_names}") from err | ||
raise EmailSendingError(f"An unexpected error occurred: {email} could not be added to this domain.") from err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also noticed the spreadsheet also includes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert006: Feel free to ignore below but flagging just for documentation that this error can be prompted without inviting the user to an organization I noticed the Mural said this event should result in 2 error messages: dm-add-alert005 and dm-add-alert006 (I think the dm-add-alert006 alert message on the Mural also is different from the message in the spreadsheet, but assuming spreadsheet is the source of truth). However, when I prompt dm-add-alert006, I do not get dm-add-alert005. But would it make sense to show dm-add-alert005 here? In this case we didn't invite the user to the organization (no domain invitation was made) so I may also be prompting this message in a different way than originally intended. ![]() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also keeping that screenshot to verify we successfully concat the contact us message There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert013: Correct alert message update |
||
|
||
|
||
def send_domain_invitation_email( | ||
|
@@ -289,9 +289,7 @@ def send_portfolio_invitation_email(email: str, requestor, portfolio, is_admin_i | |
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 | ||
raise EmailSendingError(f"An unexpected error occurred: {email} could not be added to this domain.") from err | ||
erinysong marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert001: typo fixed |
||
|
||
all_admin_emails_sent = True | ||
# send emails to portfolio admins | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,12 +54,6 @@ def __init__(self, email=None, domain=None, portfolio=None): | |
# Default message if no additional info is provided | ||
message = "Can't send invitation email. No email is associated with your user account." | ||
|
||
# Customize message based on provided arguments | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert002: Remove email, domain, and portfolio from error messages and replace with single, generic message. |
||
if email and domain: | ||
message = f"Can't send email to '{email}' on domain '{domain}'. No email exists for the requestor." | ||
elif email and portfolio: | ||
message = f"Can't send email to '{email}' for portfolio '{portfolio}'. No email exists for the requestor." | ||
|
||
super().__init__(message) | ||
|
||
|
||
|
@@ -71,9 +65,9 @@ class OutsideOrgMemberError(InvitationError): | |
|
||
def __init__(self, email=None): | ||
# Default message if no additional info is provided | ||
message = "Can not invite member of a .gov organization to a different organization." | ||
message = "Can not invite member to this organization." | ||
if email: | ||
message = f"{email} is already a member of another .gov organization." | ||
message = f"{email} is not a member of this organization." | ||
abe-alam-ecs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
super().__init__(message) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -446,7 +446,7 @@ def _process_added_domains(self, added_domain_ids, member, requestor, portfolio) | |
is_member_of_different_org=member_of_a_different_org, | ||
requested_user=member, | ||
): | ||
messages.warning(self.request, "Could not send email confirmation to existing domain managers.") | ||
messages.warning(self.request, "Could not send email notification to existing domain managers.") | ||
# Bulk create UserDomainRole instances for added domains | ||
UserDomainRole.objects.bulk_create( | ||
[ | ||
|
@@ -777,7 +777,7 @@ def _process_added_domains(self, added_domain_ids, email, requestor, portfolio): | |
domains=added_domains, | ||
is_member_of_different_org=member_of_a_different_org, | ||
): | ||
messages.warning(self.request, "Could not send email confirmation to existing domain managers.") | ||
messages.warning(self.request, "Could not send email notification to existing domain managers.") | ||
|
||
# Update existing invitations from CANCELED to INVITED | ||
existing_invitations = DomainInvitation.objects.filter(domain__in=added_domains, email=email) | ||
|
@@ -1179,7 +1179,7 @@ def _handle_exceptions(self, exception, portfolio, email): | |
elif isinstance(exception, MissingEmailError): | ||
messages.error(self.request, str(exception)) | ||
logger.error( | ||
f"Can't send email to '{email}' for portfolio '{portfolio}'. No email exists for the requestor.", | ||
"Can't send invitation email. No email is associated with your account.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert007: Update alert message language |
||
exc_info=True, | ||
) | ||
else: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
MissingEmailError, | ||
OutsideOrgMemberError, | ||
) | ||
from django.utils.html import format_html | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -61,18 +62,28 @@ def handle_invitation_exceptions(request, exception, email): | |
"""Handle exceptions raised during the process.""" | ||
if isinstance(exception, EmailSendingError): | ||
logger.warning(exception, exc_info=True) | ||
messages.error(request, str(exception)) | ||
messages.error(request, with_contact_link(str(exception))) | ||
elif isinstance(exception, MissingEmailError): | ||
messages.error(request, str(exception)) | ||
logger.error(exception, exc_info=True) | ||
elif isinstance(exception, OutsideOrgMemberError): | ||
messages.error(request, str(exception)) | ||
elif isinstance(exception, AlreadyDomainManagerError): | ||
messages.error(request, str(exception)) | ||
messages.error(request, with_contact_link(str(exception))) | ||
elif isinstance(exception, AlreadyDomainInvitedError): | ||
messages.error(request, str(exception)) | ||
elif isinstance(exception, IntegrityError): | ||
messages.error(request, f"{email} is already a manager for this domain") | ||
messages.error(request, f"An unexpected error occurred: {email} could not be added to this domain.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert003: Correct generic IntegrityError message There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert012: Correct generic IntegrityError message |
||
else: | ||
logger.warning("Could not send email invitation (Other Exception)", exc_info=True) | ||
messages.error(request, "Could not send email invitation.") | ||
messages.error( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ dm-add-alert004: Update to generic error message |
||
request, with_contact_link(f"An unexpected error occurred: {email} could not be added to this domain.") | ||
) | ||
|
||
|
||
def with_contact_link(error_message: str, contact_url: str = "https://get.gov/contact") -> str: | ||
abe-alam-ecs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return format_html( | ||
'{} Try again, and if the problem persists, <a href="{}" class="usa-link" target="_blank">contact us</a>.', | ||
error_message, | ||
contact_url, | ||
) |
Uh oh!
There was an error while loading. Please reload this page.