Skip to content

Conversation

@enmande
Copy link
Contributor

@enmande enmande commented Jan 2, 2026

🎟️ Tracking

PM-29584

📔 Objective

Adds an email template and view for sending notifications to users that one or more of their emergency contacts (Emergency Access user feature grantees) has been removed.

Includes HTML and text-only email templates with unit tests to illustrate instantiation and consumption with the newer IMailer strategy (not the legacy Handlebars mailer service).

🥇 Bonus: moves EmergencyAccess into the UserFeatures space. As I understand, this has been a desired relocation to align with other Auth architecture. The positioning of the mail templates also mirrors this move to set up future feature-scoped development.

📸 Screenshots

HTML: Single contact removed

PM-29584__single-html

HTML: Multiple contacts removed

PM-29584__multi__html

Text

PM-29584__multi-text

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2026

Logo
Checkmarx One – Scan Summary & Details39bf0267-4255-4cde-abc7-9e75a232cbda

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.27%. Comparing base (e705fe3) to head (b884efc).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6793   +/-   ##
=======================================
  Coverage   55.26%   55.27%           
=======================================
  Files        1939     1940    +1     
  Lines       86224    86227    +3     
  Branches     7705     7705           
=======================================
+ Hits        47655    47658    +3     
  Misses      36777    36777           
  Partials     1792     1792           

☔ View full report in Codecov by Sentry.
📢 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.

@enmande enmande added the ai-review Request a Claude code review label Jan 2, 2026
@enmande enmande added ai-review Request a Claude code review and removed ai-review Request a Claude code review labels Jan 9, 2026
@claude
Copy link
Contributor

claude bot commented Jan 9, 2026

Claude finished @enmande's task in 1m 48s —— View job


Code Review Complete

This PR adds email notification capability for emergency contact removal and refactors EmergencyAccess into the UserFeatures namespace. The implementation is well-structured with comprehensive test coverage.

Summary

Risk Assessment: Low
Test Coverage: Excellent - comprehensive unit tests with clear documentation
Breaking Changes: None (namespace changes handled correctly)

Findings

🎨 Minor Improvements (2)

  1. Mutable Subject Property

    • File: src/Core/Auth/UserFeatures/EmergencyAccess/Mail/EmergencyAccessRemoveGranteesMailView.cs:13
    • Issue: The Subject property has a public setter, allowing modification after initialization
    • Suggestion: Consider making it init-only for immutability:
    public override string Subject { get; init; } = "Emergency contacts removed";
    • Impact: Low - currently no code path modifies it, but immutability would prevent future bugs
  2. Empty Collection Handling

    • Context: The email template doesn't validate that RemovedGranteeNames contains at least one item
    • Suggestion: Consider adding validation in the mail view or documenting the expectation that callers must ensure non-empty collections
    • Impact: Low - tests show proper usage, but defensive validation would prevent edge cases

Positive Observations

  • Security: No sensitive data exposure; email is informational only with no authentication tokens
  • Architecture: Clean namespace refactoring from Auth.Services to Auth.UserFeatures.EmergencyAccess
  • Testing: Excellent test documentation showing exactly how to use the new IMailer pattern
  • Patterns: Correctly follows the modern IMailer approach (not legacy Handlebars)
  • Zero-Knowledge: No vault data exposed; only user-provided names included

Notes

  • This PR provides the email template infrastructure but doesn't implement the actual sending logic (as documented in tests)
  • The MJML template correctly references the Handlebars variables
  • DI registration properly updated in ServiceCollectionExtensions.cs
  • All namespace changes properly propagated through controllers and jobs

Recommendation: Approve with optional consideration of the minor improvements above.

@enmande
Copy link
Contributor Author

enmande commented Jan 12, 2026

Re: Claude comments:

🎨 Minor Improvements (2)

  1. Mutable Subject Property

    • File: src/Core/Auth/UserFeatures/EmergencyAccess/Mail/EmergencyAccessRemoveGranteesMailView.cs:13
    • Issue: The Subject property has a public setter, allowing modification after initialization
    • Suggestion: Consider making it init-only for immutability:
    public override string Subject { get; init; } = "Emergency contacts removed";
    • Impact: Low - currently no code path modifies it, but immutability would prevent future bugs

Overriding Subject requires signature matching (i.e., get and set accessors, not get and init). This might be a handy update to the inherited, but out of scope here.

  1. Empty Collection Handling

    • Context: The email template doesn't validate that RemovedGranteeNames contains at least one item
    • Suggestion: Consider adding validation in the mail view or documenting the expectation that callers must ensure non-empty collections
    • Impact: Low - tests show proper usage, but defensive validation would prevent edge cases

RemovedGranteeNames is a required field. The view cannot be instantiated without a collection. Yes that means the collection can be empty. However, for keeping the template presentational, this should not fall to the template to enforce; it should be interrogated/handled at view creation time in the caller (command).

@enmande enmande marked this pull request as ready for review January 12, 2026 16:29
@enmande enmande requested a review from a team as a code owner January 12, 2026 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants