Skip to content

Conversation

abe-alam-ecs
Copy link
Contributor

@abe-alam-ecs abe-alam-ecs commented Sep 25, 2025

Ticket

Resolves #3940

Changes

  • Edited error messaging
  • Added helper function for hyperlinking "contact us" to views.

Context for reviewers

There are certain cases in which domain manager updates should not be allowed. Review the story's mural and excel, linked here for convenience.

spreadsheet
Mural

Examples:

  • adding someone who is already a domain manager as a domain manager
  • adding someone who is a part of a different portfolio as a domain manager

Setup

Create a domain that you manage if one does not already exist
Execute workflows in the mural/excel above... for example:

  • Go to a domain you manage (or create one)
  • In the Domain Managers section, click Edit
  • Try adding yourself as another domain manager. It should fail with an error message, "An unexpected error occurred. Try again, and if the problem persists, contact us." with a hyperlink on contact us.

Repeat such tests for the 12 error message updates.

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
  • Update documentation in READMEs and/or onboarding guide - N/A

Ensured code standards are met (Original Developer)

  • If any updated dependencies on Pipfile, also update dependencies in requirements.txt. - N/A
  • Interactions with external systems are wrapped in try/except
  • Error handling exists for unusual or missing values

Validated user-facing changes (if applicable)

  • Follow the process for requesting a design review. If code is not user-facing, delete design reviewer checklist
  • Verify 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)

As a code reviewer, I have

Reviewed, tested, and left feedback about the changes

  • Pulled this branch locally and tested it
  • Verified code meets all checks above. Address any checks that are not satisfied
  • Reviewed this code and left comments. Indicate if comments must be addressed before code is merged
  • Checked that all code is adequately covered by tests
  • Verify migrations are valid and do not conflict with existing migrations

Validated user-facing changes as a developer

Note: Multiple code reviewers can share the checklists above, a second reviewer should not make a duplicate checklist. All checks should be checked before approving, even those labeled N/A.

  • New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
  • Checked keyboard navigability
  • Meets all designs and user flows provided by design/product
  • Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
  • (Rarely needed) Tested as both an analyst and applicant user

As a designer reviewer, I have

Verified that the changes match the design intention

  • Checked that the design translated visually
  • Checked behavior. Comment any found issues or broken flows.
  • Checked different states (empty, one, some, error)
  • Checked for landmarks, page heading structure, and links

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

References

Screenshots

Copy link

🥳 Successfully deployed to developer sandbox aa.

1 similar comment
Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

@abe-alam-ecs abe-alam-ecs requested a review from a team September 26, 2025 15:26
@abe-alam-ecs abe-alam-ecs added the design-review dev ticket needing design review label Sep 26, 2025
Copy link

🥳 Successfully deployed to developer sandbox aa.

@SamiyahKey SamiyahKey requested review from SamiyahKey and removed request for a team September 26, 2025 18:21
Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

@abe-alam-ecs abe-alam-ecs marked this pull request as ready for review September 29, 2025 14:15
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Also noticed the spreadsheet also includes " Try again and contact us if the problem persists." but feel free to ignore if that's outdated!

Copy link
Contributor

@erinysong erinysong Oct 3, 2025

Choose a reason for hiding this comment

The 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).
image

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.

image

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

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

✅ dm-add-alert013: Correct alert message update

Copy link
Contributor

@erinysong erinysong left a comment

Choose a reason for hiding this comment

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

Left some comments but let me know if things have changed from the spreadsheet sections I reference, since you've been working on this more closely!

Copy link

🥳 Successfully deployed to developer sandbox aa.


def __init__(self, email):
super().__init__(f"{email} is already a manager for this domain.")
super().__init__(f"An unexpected error occurred: {email} could not be added to this domain.")
Copy link
Contributor

Choose a reason for hiding this comment

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

recapping our call: I think this error message doesn't need to be changed and can be reverted to the original message!

@github-project-automation github-project-automation bot moved this to 👀 In review in .gov Product Board Oct 3, 2025
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
Copy link
Contributor

@erinysong erinysong Oct 3, 2025

Choose a reason for hiding this comment

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

✅ dm-add-alert001: typo fixed

Copy link

github-actions bot commented Oct 3, 2025

🥳 Successfully deployed to developer sandbox aa.

# 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ dm-add-alert003: Correct generic IntegrityError message

Copy link
Contributor

Choose a reason for hiding this comment

The 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(
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ dm-add-alert004: Update to generic error message

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}")
Copy link
Contributor

@erinysong erinysong Oct 3, 2025

Choose a reason for hiding this comment

The 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

image

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@witha-k witha-k Oct 14, 2025

Choose a reason for hiding this comment

The 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...

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.",
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ dm-add-alert007: Update alert message language

Copy link
Contributor

@erinysong erinysong left a comment

Choose a reason for hiding this comment

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

✅ dm-add-alert010: User gets correct alert
image

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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

✅ dm-add-alert011: alert message updated

Copy link
Contributor

@erinysong erinysong left a comment

Choose a reason for hiding this comment

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

Checked the add manager alerts which had requested changes on the spreadsheet! Had a couple questions that I marked with ❓ and change requests marked with 🔄 . Happy to go over these together if that's easier or if I misunderstood anything!

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link

🥳 Successfully deployed to developer sandbox aa.

Copy link
Contributor

@erinysong erinysong left a comment

Choose a reason for hiding this comment

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

Deferring to design on dm-add-alert005 - thank you for handling everything thoroughly!

Copy link

🥳 Successfully deployed to developer sandbox aa.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

design-review dev ticket needing design review

Projects

Status: 👀 In review

Development

Successfully merging this pull request may close these issues.

Update messages for adding a domain manager

5 participants