#2729: Update Add a Domain Manager page - [ES] #2857

erinysong · 2024-09-26T17:53:14Z

Ticket

Resolves #2729

Changes

Update Add Domain Manager form so emails cannot be added as an org's domain manager if it is already associated with another org (either as domain manager or invited user).
Add breadcrumb to Add Domain Manager page directing user back to Domain Manager page.
Edit copy on Add Domain Manager page description.
Update fixtures to assign each user only 1 portfolio (as opposed to all portfolios previously).

Context for reviewers

Setup

Testing that users cannot add users who are domain managers/invited to other organizations

Go to getgov-es and set up at least the following items:

Turn on the organization feature waffle flag
2 portfolio objects
1 user assigned to the first portfolio and assigned as a domain manager to one of the portfolio's domains
A 2nd user assigned to the second portfolio

Go to the domain page that you are assigned domain manager. Go to the Domain Managers page and add the email as the second user who you added to the other portfolio. You should not be able to add this user as a domain manager and view the error message indicating so.
Turn off the organization feature waffle flag. You should be able to now add this 2nd user as a domain manager now that portfolios no longer exist after turning off the org feature waffle flag.

Testing Other ACs

On the Add a Domain Manager page, verify that the breadcrumb menu allows you go back to the Domain Managers page. You should verify this when org feature waffle flag is both on and off.
Verify that the Add a Domain Manager page content matches the updated copy on Figma (can be found on the ticket AC)

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-26T17:57:43Z

🥳 Successfully deployed to developer sandbox es.

github-actions · 2024-09-26T21:49:46Z

🥳 Successfully deployed to developer sandbox es.

github-actions · 2024-09-26T21:56:26Z

🥳 Successfully deployed to developer sandbox es.

erinysong · 2024-09-26T22:08:21Z

src/registrar/utility/errors.py

@@ -23,6 +23,15 @@ class InvalidDomainError(ValueError):
    pass


+class OutsideOrgMemberError(ValueError):


We can delete this error class and all references to it once we allow users to be members of multiple orgs as per the ticket ACs

github-actions · 2024-09-26T22:12:17Z

🥳 Successfully deployed to developer sandbox es.

erinysong · 2024-09-26T22:25:19Z

src/registrar/views/domain.py

@@ -831,6 +857,8 @@ def _send_domain_invitation_email(self, email: str, requestor: User, add_success
                    "requestor_email": requestor_email,
                },
            )
+            if add_success:


Moved this into the try statement since we shouldn't be sending emails if we do run into Exceptions that aren't EmailSendingError - feel free to let me know if there was a reason we did this previously though!

abroddrick · 2024-09-30T17:12:39Z

src/registrar/utility/errors.py

+class OutsideOrgMemberError(ValueError):
+    """
+    Error raised when an org member tries adding a user from a different .gov org.
+    To be deleted when users can be members of multiple orgs.


love the comment here too! great!

therealslimhsiehdy

The HTML file looks good and follows the Figma design:

Running into some issues with getting the suggested error with the organization_feature flag on (users from 2nd portfolio, approved user), and even with the organization_feature flag off (still unable to add any user as domain manager). I turned off the disable_email_sending flag as well even though the emails I'm using are on the allow list, so something may be funky with the errors.py file I'm assuming 😭

github-actions · 2024-10-08T22:44:14Z

🥳 Successfully deployed to developer sandbox es.

github-actions · 2024-10-08T23:14:58Z

🥳 Successfully deployed to developer sandbox es.

github-actions · 2024-10-09T22:20:23Z

🥳 Successfully deployed to developer sandbox es.

github-actions · 2024-10-09T23:00:48Z

🥳 Successfully deployed to developer sandbox es.

github-actions · 2024-10-09T23:26:00Z

🥳 Successfully deployed to developer sandbox es.

therealslimhsiehdy

LGTM -- caveat that to merge the test that is failing must be fixed!

Tested out with flag on a user that was already in a different org (red banner), one that was in same org (successful, green banner)

With flag turned off, was able to add user that was previously in a different org

gabydisarli · 2024-10-15T22:03:06Z

Hi Erin! The content on the Domain managers page (both org model + non-org model) needs to be updated. Please refer to this section of the content document for this page.

erinysong · 2024-10-15T23:19:51Z

Thank you for bringing to attention the new content @gabydisarli! Just updated content with the following

Add a domain manager - domain view

Add a domain manager - org model view

Domain manager page content updates

github-actions · 2024-10-15T23:26:53Z

🥳 Successfully deployed to developer sandbox es.

gabydisarli · 2024-10-16T18:06:20Z

Thanks for making these content updates, Erin! On the domain managers page, can you remove the final line of the last bullet points "Add another domain manager..."

gabydisarli · 2024-10-16T18:22:15Z

Can we change this text to: <[email protected]> is already a member of another .gov organization.

erinysong · 2024-10-16T21:52:13Z

@gabydisarli good catches and thank you! Just pushed those content fixes and the updates should be deployed on getgov-es soon!

github-actions · 2024-10-16T21:54:02Z

🥳 Successfully deployed to developer sandbox es.

erinysong added 3 commits September 26, 2024 10:24

Block users invited to other orgs from being domain managers

caad00d

Add domain manager breadcrumb nav

42de7f2

Update add domain manager page content

8b61eb1

Add error code for outside org members being added

b0fe698

erinysong added 2 commits September 26, 2024 14:51

Fix linting. Revert to original email sending logic

d710696

Readd outside org member error handling

d66ff33

Fix linting

3d1781c

erinysong commented Sep 26, 2024

View reviewed changes

erinysong marked this pull request as ready for review September 26, 2024 22:20

erinysong requested a review from abroddrick as a code owner September 26, 2024 22:20

erinysong requested review from lizpearl, therealslimhsiehdy, Matt-Spence, CocoByte, rachidatecs, dave-kennedy-ecs, asaki222, zandercymatics and a team September 26, 2024 22:20

erinysong commented Sep 26, 2024

View reviewed changes

AnnaGingle requested review from AnnaGingle and removed request for a team September 30, 2024 15:18

abroddrick reviewed Sep 30, 2024

View reviewed changes

therealslimhsiehdy self-assigned this Sep 30, 2024

Revert to try else catch

117900c

therealslimhsiehdy reviewed Sep 30, 2024

View reviewed changes

Secure portfolio check on requestor org

889c0a2

Reverse email send check

aec1a4f

Re-revert email_success check

76fc713

Add domain manager after email sending error

dd29cbf

therealslimhsiehdy approved these changes Oct 10, 2024

View reviewed changes

erinysong requested a review from a team October 10, 2024 21:38

gabydisarli requested review from gabydisarli and removed request for a team October 15, 2024 18:42

erinysong mentioned this pull request Oct 15, 2024

#2859 multiple portfolio flags bug - [KO] #2917

Merged

51 tasks

This comment was marked as resolved.

Sign in to view

Add domain manager page content updates

4393b5a

Update domain manager page content

961a289

erinysong added 2 commits October 16, 2024 14:46

Add content updates

3cb341d

Resolve merge conflict in fixtures

62b3cbf

gabydisarli approved these changes Oct 17, 2024

View reviewed changes

erinysong merged commit ce7064d into main Oct 17, 2024
10 checks passed

erinysong deleted the es/2729-domain-manager-updates branch October 17, 2024 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2729: Update Add a Domain Manager page - [ES] #2857

#2729: Update Add a Domain Manager page - [ES] #2857

erinysong commented Sep 26, 2024 •

edited by gabydisarli

Loading

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

erinysong Sep 26, 2024 •

edited

Loading

github-actions bot commented Sep 26, 2024

erinysong Sep 26, 2024

abroddrick Sep 30, 2024

therealslimhsiehdy left a comment

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

therealslimhsiehdy left a comment

gabydisarli commented Oct 15, 2024

This comment was marked as resolved.

This comment was marked as resolved.

erinysong commented Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 15, 2024

gabydisarli commented Oct 16, 2024

gabydisarli commented Oct 16, 2024

erinysong commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

		@@ -23,6 +23,15 @@ class InvalidDomainError(ValueError):
		pass


		class OutsideOrgMemberError(ValueError):

#2729: Update Add a Domain Manager page - [ES] #2857

#2729: Update Add a Domain Manager page - [ES] #2857

Conversation

erinysong commented Sep 26, 2024 • edited by gabydisarli 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 26, 2024

github-actions bot commented Sep 26, 2024

github-actions bot commented Sep 26, 2024

erinysong Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Sep 26, 2024

erinysong Sep 26, 2024

Choose a reason for hiding this comment

abroddrick Sep 30, 2024

Choose a reason for hiding this comment

therealslimhsiehdy left a comment

Choose a reason for hiding this comment

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

therealslimhsiehdy left a comment

Choose a reason for hiding this comment

gabydisarli commented Oct 15, 2024

This comment was marked as resolved.

This comment was marked as resolved.

erinysong commented Oct 15, 2024 • edited Loading

github-actions bot commented Oct 15, 2024

gabydisarli commented Oct 16, 2024

gabydisarli commented Oct 16, 2024

erinysong commented Oct 16, 2024

github-actions bot commented Oct 16, 2024

erinysong commented Sep 26, 2024 •

edited by gabydisarli

Loading

erinysong Sep 26, 2024 •

edited

Loading

erinysong commented Oct 15, 2024 •

edited

Loading