#3530, #3531: Email notifications [MEOWARD] #3671
Conversation
…signment, and updated tests
dave-kennedy-ecs
requested review from
lizpearl,
therealslimhsiehdy,
Matt-Spence,
CocoByte,
rachidatecs,
allly-b,
Katherine-MN,
erinysong,
zandercymatics and
a team
|
🥳 Successfully deployed to developer sandbox meoward. |
|
Verified on meoward that managers receive an email when 1. a domain manager is removed from the org model domains page and 2. a portfolio member is removed from the portfolio/organization from the members page |
There was a problem hiding this comment.
Both scenarios (user domain role deletion, portfolio permission deletion) do not trigger emails when done from DJA. I forget whether this was a requirement (NOT triggering emails) or an oversight. Worth a quick cc @Katherine-MN
| invitations__status=DomainInvitation.DomainInvitationStatus.INVITED, | ||
| ).distinct() | ||
| # Get list of portfolio domains that the member is a manager of | ||
| domains = Domain.objects.filter( |
There was a problem hiding this comment.
I do not think 'domains' should return anything here, and 'invited_domains' should return anything for a User, but I agree that there are so many edge cases that covering our bases makes sense.
| all_emails_sent = True | ||
| # Get each domain manager from list | ||
| user_domain_roles = UserDomainRole.objects.filter(domain=domain) | ||
| if manager_removed: |
There was a problem hiding this comment.
I'd think if manager_removed returns false, something bad happened prior to this function call. Put an else and add a logger.error in there?
There was a problem hiding this comment.
I realize this PR was merged, but just following up Rachid's note above!
Yes, that was a requirement. If a user is removed from a domain or portfolio via Django admin, it should not trigger an email. Can only trigger "removal" emails directly in the registrar. cc: @dave-kennedy-ecs
There was a problem hiding this comment.
Update: Just saw Rachid's comments after submitting. I don't have strong opinions on the return values mentioned above, so will defer to your two's judgments.
LGTM! Tested and verified this work on sandbox and code + refactoring also look good and clean, good work!
There was a problem hiding this comment.
Looks and works great! following up with @dave-kennedy-ecs on an email formatting issue that is probably caused by my email software
|
🥳 Successfully deployed to developer sandbox meoward. |
Ticket
Resolves #3530
Resolves #3531
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
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.
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
References
Screenshots