Skip to content

[PM-36011] feat: add member email change notification email#8044

Open
jrmccannon wants to merge 8 commits into
jmccannon/ac/pm-38926-admin-change-emailfrom
jmccannon/ac/pm-36011-member-email-change-notification
Open

[PM-36011] feat: add member email change notification email#8044
jrmccannon wants to merge 8 commits into
jmccannon/ac/pm-38926-admin-change-emailfrom
jmccannon/ac/pm-36011-member-email-change-notification

Conversation

@jrmccannon

@jrmccannon jrmccannon commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

PM-36011

📔 Objective

Adds the "Your Bitwarden account email was updated" notification email (part of PM-28365), built with the IMailer / MJML pipeline: view model, mail class, MJML source, compiled templates, and a render test.

Screenshot

image

@jrmccannon jrmccannon added the t:feature Change Type - Feature Development label Jul 22, 2026
@jrmccannon jrmccannon changed the title Jmccannon/ac/pm 36011 member email change notification [PM-36011] feat: add member email change notification email Jul 22, 2026
@jrmccannon
jrmccannon force-pushed the jmccannon/ac/pm-38926-admin-change-email branch from 4b4a236 to 1899f50 Compare July 23, 2026 19:54
@jrmccannon
jrmccannon force-pushed the jmccannon/ac/pm-36011-member-email-change-notification branch from b118fb3 to 9ccf265 Compare July 23, 2026 19:56
@jrmccannon
jrmccannon force-pushed the jmccannon/ac/pm-38926-admin-change-email branch from 1899f50 to 0f7dbea Compare July 24, 2026 16:55
@jrmccannon
jrmccannon force-pushed the jmccannon/ac/pm-36011-member-email-change-notification branch 2 times, most recently from ed975cf to 66f8162 Compare July 24, 2026 20:14
@jrmccannon
jrmccannon marked this pull request as ready for review July 24, 2026 20:28
@jrmccannon
jrmccannon requested a review from a team as a code owner July 24, 2026 20:28
@jrmccannon
jrmccannon requested a review from JimmyVo16 July 24, 2026 20:28
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the new "member email changed" notification email: the view/mail model, MJML source and compiled Handlebars templates, the render test, and the wiring into UpdateOrganizationUserCommand. Notifying the previous email address that its account email was changed is the correct security behavior, and the command's test coverage exercises both the notification and no-notification paths. One correctness concern about error handling around the notification send is noted inline.

Code Review Details
  • ⚠️ : Notification-email delivery failure fails the whole update after the email change is already committed (only BadRequestException is caught).
    • src/Core/AdminConsole/OrganizationFeatures/OrganizationUsers/UpdateUser/v2/UpdateOrganizationUserCommand.cs:118

Comment on lines +116 to +118
await changeEmailCommand.ChangeEmailAsync(request.UserToUpdate, request.NewEmail!);

await SendEmailChangedNotificationAsync(previousEmail, request.NewEmail!);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: A notification-email delivery failure will fail the whole update after the email change is already committed.

Details and fix

SendEmailChangedNotificationAsync runs inside TryApplyAccountChangesAsync, whose only catch is BadRequestException (mapped by MapEmailChangeError). Mail delivery services (SendGridMailDeliveryService, AmazonSesMailDeliveryService, MailKitSmtpMailDeliveryService) throw HTTP/SMTP/network exceptions, not BadRequestException.

Because ChangeEmailAsync has already persisted the account change and synced Stripe (per the comment on line 115), a transient send failure would throw an unhandled exception out of UpdateUserAsync and surface to the admin as a failed update — even though the email change actually succeeded. The notification is a best-effort side effect and should not be able to fail the primary operation.

The sibling OrganizationAutoConfirmEnabledNotificationCommand already guards its mailer.SendEmail call in a try/catch that logs and continues. Consider the same here, e.g.:

try
{
    await SendEmailChangedNotificationAsync(previousEmail, request.NewEmail!);
}
catch (Exception ex)
{
    logger.LogError(ex, "Failed to send member email-change notification for organization user {OrganizationUserId}.", request.OrganizationUserToUpdate.Id);
}

(logging without the recipient address to avoid PII in logs).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with this Claude feedback.

Comment on lines +116 to +118
await changeEmailCommand.ChangeEmailAsync(request.UserToUpdate, request.NewEmail!);

await SendEmailChangedNotificationAsync(previousEmail, request.NewEmail!);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I agree with this Claude feedback.

@jrmccannon
jrmccannon force-pushed the jmccannon/ac/pm-36011-member-email-change-notification branch from 66f8162 to c76d02c Compare July 25, 2026 15:00
@jrmccannon
jrmccannon requested a review from eliykat July 25, 2026 15:13
Comment on lines +157 to +160
catch (Exception ex)
{
logger.LogError(ex, "Failed to send member email-change notification for organization user {OrganizationUserId}.", request.OrganizationUserToUpdate.Id);
}
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.62%. Comparing base (98a4e35) to head (b628a98).

Files with missing lines Patch % Lines
...ers/UpdateUser/v2/UpdateOrganizationUserCommand.cs 77.77% 4 Missing ⚠️
Additional details and impacted files
@@                            Coverage Diff                            @@
##           jmccannon/ac/pm-38926-admin-change-email    #8044   +/-   ##
=========================================================================
  Coverage                                     62.61%   62.62%           
=========================================================================
  Files                                          2305     2306    +1     
  Lines                                        100355   100371   +16     
  Branches                                       9046     9046           
=========================================================================
+ Hits                                          62839    62854   +15     
- Misses                                        35337    35338    +1     
  Partials                                       2179     2179           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants