#3019: Portfolio and domain invitation emails [AD] #3252

dave-kennedy-ecs · 2024-12-19T21:14:44Z

Ticket

Resolves #3019

Changes

On (Portfolio) Add New Member, sends an email when creating a PortfolioInvitation.
In this ticket, completed the PortfolioAddNewMember view and form. Refactored the view to properly handle email sending exceptions.
Refactored 3 forms to inherit from BasePortfolioMemberForm, and for BasePortfolioMemberForm to inherit ModelForm.
On (Domain) Add Domain Manager, streamlined view to properly handle exceptions in one flow. When appropriate, sends PortfolioInvitation email to the newly added Domain Manager.
In Django Admin, addition of PortfolioInvitation sends email to invited email address. In the event of email sending error, new Portfolio Invitation is not saved.
Refactored email sending methods for portfolio invitation email and domain manager email into a separate utility file so that they can be executed from multiple views.

Context for reviewers

Setup

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

Ensured code standards are met (Original Developer)

If any updated dependencies on Pipfile, also update dependencies 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)

Tag @dotgov-designers in this PR's Reviewers for 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

Code review best practices

Screenshots

github-actions · 2024-12-19T21:19:20Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-12-19T22:02:25Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-12-20T00:18:00Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-12-20T00:56:14Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-12-20T21:03:20Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-12-21T00:28:08Z

🥳 Successfully deployed to developer sandbox bob.

src/registrar/views/portfolios.py

github-actions · 2024-12-30T16:02:35Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2024-12-30T16:10:14Z

🥳 Successfully deployed to developer sandbox bob.

src/registrar/templates/portfolio_members_add_new.html

src/registrar/admin.py

src/registrar/assets/src/js/getgov/portfolio-member-page.js

src/registrar/tests/test_admin.py

src/registrar/tests/test_forms.py

CocoByte

I am running into issues with portfolio invitation emails not getting sent (I am able to send domain request invitation emails). This happens for both existing and non-existing user e-mails.

abroddrick

Didn't do a full review, just briefly skimmed the code only. No major comments, looks good and I like the implementation, I'll trust Nicolle do the full review though of course as I see she already hit some snags

abroddrick · 2024-12-31T01:40:34Z

src/registrar/admin.py

+        if has_errors:
+            # Re-render the change form if there are errors or warnings
+            # Prepare context for rendering the change form
+            opts = self.model._meta


non blocking question why "opts"?

If confusing, can change. Was modeling code after super class, django.contrib.admin.options.ModelAdmin, in which many of their methods get the options from _meta.

@dave-kennedy-ecs It looks like opts is being used on line 1673 after pulling from obj (its using obj._meta instead of self.model._meta). The value for the var definition here is never actually being used, only the _meta property from obj. Seems like a typo and makes it a bit hard to follow -- can you delete the opts definition on this line?

src/registrar/admin.py

src/registrar/tests/test_forms.py

src/registrar/views/portfolios.py

github-actions · 2024-12-31T13:21:03Z

🥳 Successfully deployed to developer sandbox bob.

dave-kennedy-ecs · 2024-12-31T13:56:39Z

@CocoByte @abroddrick I have restaged this PR on AD sandbox. Email sending appears to be broken on BOB sandbox. I have now successfully tested this branch on multiple sandboxes so think the problem is not with the branch, but with the sandbox. Please evaluate the functionality on the AD sandbox, and I have opened a ticket to investigate email sending on BOB sandbox.

zandercymatics

Still verifying the functionality, but here is the initial code review. Great work! Most of my suggestions are nitpicks so keep that in mind, no need to address everything if you don't view it as worth the time investment

src/registrar/admin.py

zandercymatics · 2025-01-02T19:34:04Z

src/registrar/admin.py

+        if has_errors:
+            # Re-render the change form if there are errors or warnings
+            # Prepare context for rendering the change form
+            opts = self.model._meta


@dave-kennedy-ecs It looks like opts is being used on line 1673 after pulling from obj (its using obj._meta instead of self.model._meta). The value for the var definition here is never actually being used, only the _meta property from obj. Seems like a typo and makes it a bit hard to follow -- can you delete the opts definition on this line?

zandercymatics · 2025-01-02T19:49:11Z

src/registrar/models/user_portfolio_permission.py

+        common_forbidden_perms = (
+            set.intersection(*[set(cls.FORBIDDEN_PORTFOLIO_ROLE_PERMISSIONS.get(role, [])) for role in roles])
+            if roles
+            else set()


Thanks for adding this check on roles. If you recall, how did you run into this issue?

I believe I had this error when attempting to save a userportfoliopermission through django admin without specifying a role. May have run into it in one of the views though. Can't remember if both scenarios.

Ah weird. Thanks

src/registrar/templates/django/admin/portfolio_invitation_change_form.html

src/registrar/utility/email_invitations.py

zandercymatics · 2025-01-02T20:58:29Z

src/registrar/views/domain.py

+        # COMMENT: this code does not take into account multiple portfolios flag
+
+        # COMMENT: shouldn't this code be based on the organization of the domain, not the org
+        # of the requestor? requestor could have multiple portfolios


If I'm reading the original logic right, I think we shouldn't actually be doing this check at all if the multiple portfolios flag is enabled. Its basically enforcing that rule by proxy (hence why its checking on org requestor). See image

src/registrar/views/domain.py

zandercymatics · 2025-01-02T21:13:46Z

src/registrar/views/domain.py

+            domain=self.object,
+            is_member_of_different_org=member_of_different_org,
+        )
+        DomainInvitation.objects.get_or_create(email=email, domain=self.object)


Shouldn't this be create?

It should be get_or_create to account for an existing, canceled DomainInvitation

github-actions · 2025-01-02T22:01:26Z

🥳 Successfully deployed to developer sandbox bob.

zandercymatics

LGTM though note the comments above

github-actions · 2025-01-02T23:59:40Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2025-01-03T00:59:27Z

🥳 Successfully deployed to developer sandbox bob.

github-actions · 2025-01-03T01:07:12Z

🥳 Successfully deployed to developer sandbox bob.

rachidatecs and others added 16 commits December 12, 2024 17:38

wip

a2e6238

refactored _send_domain_invitation_email in domain.py view

d766aa9

first refactor attempt, moving send_domain_invitation_email to a helper

d2d44bf

refactored form_valid to break into smaller chunks (to satisfy linter)

b4826ea

portfolio invitation emails refactored and implemented in helper

dd32cb9

moving around logic, prepping for refactor

ddf7d92

wip

d888c61

work in progress tracing current form implementation

f09627b

refactor form and view for add portfolio member

060f2d5

wip

ae3ce1f

portfolio invitations working in admin view as well as user view

7bbee19

error cleanup

5cb6c3f

static content on invitation and permissions change forms

d2343b7

merge main and integrate ModelForm for PortfolioInvitation

7658810

PortfolioMemberForm inheriting from BasePortfolioMemberForm

08b985d

new portfolio member form inheriting from base

be657e1

dave-kennedy-ecs requested a review from abroddrick as a code owner December 19, 2024 21:14

dave-kennedy-ecs changed the title ~~[DRAFT] #3019: Portfolio and domain invitation emails~~ [DRAFT] #3019: Portfolio and domain invitation emails [BOB] Dec 19, 2024

Fix background color bug

4e4e2c0

a little bit of linting

1fe97eb

cleanup and comments

955d0a7

Fix broken tests

d27352b

portfolio views tests

1b16d6d

abroddrick reviewed Dec 21, 2024

View reviewed changes

src/registrar/views/portfolios.py Show resolved Hide resolved

dave-kennedy-ecs added 2 commits December 30, 2024 10:56

friendlier error message

d7493cc

Merge branch 'main' into bob/3019-portfolio-invitation-email

a8340f5

dave-kennedy-ecs added 2 commits December 30, 2024 11:04

fixed a test

267364e

linted

e845fe1

CocoByte self-assigned this Dec 30, 2024

CocoByte reviewed Dec 31, 2024

View reviewed changes

abroddrick reviewed Dec 31, 2024

View reviewed changes

dave-kennedy-ecs added 2 commits December 31, 2024 07:47

Merge branch 'main' into bob/3019-portfolio-invitation-email

b86d8bc

performed some cleanup

2007daa

dave-kennedy-ecs changed the title ~~#3019: Portfolio and domain invitation emails [BOB]~~ #3019: Portfolio and domain invitation emails [AD] Dec 31, 2024

zandercymatics self-assigned this Jan 2, 2025

zandercymatics reviewed Jan 2, 2025

View reviewed changes

dave-kennedy-ecs added 2 commits January 2, 2025 16:56

simplified logic on check for existing permission in admin

b39c143

more cleanup in admin

0c07e93

zandercymatics approved these changes Jan 2, 2025

View reviewed changes

updated logic to account for domain portfolio assignment

c5325a6

dave-kennedy-ecs added 2 commits January 2, 2025 19:48

fixed tests

c70a16d

merge of main

84f6445

removed some extraneous log statements

8b29171

dave-kennedy-ecs merged commit 1745f6a into main Jan 3, 2025
10 checks passed

dave-kennedy-ecs deleted the bob/3019-portfolio-invitation-email branch January 3, 2025 01:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3019: Portfolio and domain invitation emails [AD] #3252

#3019: Portfolio and domain invitation emails [AD] #3252

dave-kennedy-ecs commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 21, 2024

github-actions bot commented Dec 30, 2024

github-actions bot commented Dec 30, 2024

CocoByte left a comment

abroddrick left a comment •

edited

Loading

abroddrick Dec 31, 2024

dave-kennedy-ecs Dec 31, 2024

zandercymatics Jan 2, 2025

github-actions bot commented Dec 31, 2024

dave-kennedy-ecs commented Dec 31, 2024 •

edited

Loading

zandercymatics left a comment

zandercymatics Jan 2, 2025

zandercymatics Jan 2, 2025

dave-kennedy-ecs Jan 2, 2025

zandercymatics Jan 2, 2025

zandercymatics Jan 2, 2025 •

edited

Loading

zandercymatics Jan 2, 2025

dave-kennedy-ecs Jan 3, 2025

github-actions bot commented Jan 2, 2025

zandercymatics left a comment

github-actions bot commented Jan 2, 2025

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

#3019: Portfolio and domain invitation emails [AD] #3252

#3019: Portfolio and domain invitation emails [AD] #3252

Conversation

dave-kennedy-ecs commented Dec 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

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

References

Screenshots

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 19, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 20, 2024

github-actions bot commented Dec 21, 2024

github-actions bot commented Dec 30, 2024

github-actions bot commented Dec 30, 2024

CocoByte left a comment

Choose a reason for hiding this comment

abroddrick left a comment • edited Loading

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 Dec 31, 2024

dave-kennedy-ecs commented Dec 31, 2024 • edited Loading

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

zandercymatics Jan 2, 2025 • edited Loading

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 Jan 2, 2025

zandercymatics left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 2, 2025

github-actions bot commented Jan 3, 2025

github-actions bot commented Jan 3, 2025

dave-kennedy-ecs commented Dec 19, 2024 •

edited

Loading

abroddrick left a comment •

edited

Loading

dave-kennedy-ecs commented Dec 31, 2024 •

edited

Loading

zandercymatics Jan 2, 2025 •

edited

Loading