Skip to content

Conversation

@trmartin4
Copy link
Member

@trmartin4 trmartin4 commented Oct 29, 2025

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-27593

📔 Objective

We would like to document that the HandlebarsMailService has been deprecated in favor of the Mailer, and also add documentation in code about the support guarantees of our disk overrides of mail templates, so that there is no confusion in the future about this feature and its expectations.

Creating a "home" for the documentation that made sense required moving the mail-related services out of the shared Services folder and into their own "feature" folder. This is our current best practice and allows contextual, close-to-code READMEs for developer and AI usage.

📸 Screenshots

image

⏰ 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

@trmartin4 trmartin4 changed the title Added docs for legacy mail service. Add docs for legacy mail service Oct 29, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Oct 29, 2025

Logo
Checkmarx One – Scan Summary & Details1e052032-6601-4bfe-931b-4984957e42dd

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

@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.86%. Comparing base (8f2f204) to head (9ab5bfb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6513      +/-   ##
==========================================
+ Coverage   51.85%   51.86%   +0.01%     
==========================================
  Files        1901     1901              
  Lines       84074    84051      -23     
  Branches     7509     7501       -8     
==========================================
  Hits        43594    43594              
+ Misses      38786    38763      -23     
  Partials     1694     1694              

☔ 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.

@trmartin4 trmartin4 changed the title Add docs for legacy mail service [PM-27593] Add docs for legacy mail service Oct 29, 2025
@trmartin4 trmartin4 marked this pull request as ready for review October 29, 2025 13:52
@trmartin4 trmartin4 requested a review from a team as a code owner October 29, 2025 13:52
@trmartin4 trmartin4 requested review from coroiu and kspearrin October 29, 2025 13:52
@claude
Copy link

claude bot commented Oct 29, 2025

Claude finished @trmartin4's task —— View job


PR Review: Add docs for legacy mail service

Summary

This PR documents the deprecation of HandlebarsMailService in favor of the new Mailer implementation and clarifies the support guarantees for disk-based email template overrides used by self-hosted customers.

Key Changes:

  • Files reorganized from src/Core/Services/ to src/Core/Platform/Mail/ with preserved namespaces (Bit.Core.Services.Mail.*)
  • New README.md added with deprecation warning and template override documentation
  • Using statements updated across codebase to reference new namespace locations

Critical Issues

None found.


Suggested Improvements

1. Documentation Clarity - README.md structure

The README could be improved for better readability:

Current: src/Core/Platform/Mail/README.md:3-10

> [!CAUTION]
> Adding new methods to the `HandlebarsMailService` is no longer the recommended approach...

# Overriding email templates from disk

The `HandlebarsMailService` supports loading...

Suggestion: Consider adding a clear heading for the deprecation section to match the structure of the template override section:

# Mail Service

## Deprecation Notice

> [!CAUTION]
> Adding new methods to the `HandlebarsMailService` is no longer the recommended approach to building and sending emails. Instead, the [Mailer](../Mailer/README.md) implementation is preferred, along with the [MJML](../../MailTemplates/README.md) pipeline.

## Overriding email templates from disk

The `HandlebarsMailService` supports loading the mail template from disk...

This provides better visual hierarchy and makes it clearer that there are two distinct topics being covered.

2. Verify linked documentation exists

Location: src/Core/Platform/Mail/README.md:4

The README references [Mailer](../Mailer/README.md) and [MJML](../../MailTemplates/README.md). While these paths look correct based on the directory structure, it would be good to verify these files exist and contain relevant documentation for developers transitioning away from HandlebarsMailService.

Verification needed
# Check if referenced docs exist
ls -la src/Core/Platform/Mailer/README.md
ls -la src/Core/MailTemplates/README.md

If these don't exist yet, consider either:

  • Creating stub documentation
  • Using different link targets
  • Noting in the PR description that these docs are pending

3. Minor wording improvement - README.md:10

Current wording: src/Core/Platform/Mail/README.md:10

**Bitwarden is not responsible for maintaining backward compatibility between releases in order to support any overridden emails.**

Suggestion: The double emphasis (bold + caution block) is appropriate, but consider slightly softening the language while maintaining clarity:

**Bitwarden does not maintain backward compatibility between releases for overridden email templates. Customers using template overrides are responsible for updating their customizations when templates change.**

This maintains the important disclaimer while being slightly more constructive about the customer's responsibility.


Good Practices Observed

  • File moves preserve namespace compatibility (avoiding breaking changes)
  • Comprehensive update of using statements across all affected files
  • Tests updated to reference new namespace locations
  • Documentation placed at appropriate location in source tree
  • Clear caution callout for deprecation notice

Action Items

  • (Optional) Consider adding section heading for deprecation notice in README.md
  • (Optional) Verify that referenced Mailer and MJML documentation exists or will be created
  • (Optional) Consider minor wording refinement in template override disclaimer

Additional Notes

The namespace preservation strategy (Bit.Core.Services.Mail.* kept despite file moves to Platform/Mail/) is a good architectural decision that:

  • Avoids breaking changes for consumers
  • Maintains backward compatibility
  • Allows gradual migration path

This is consistent with the goal of documenting deprecation rather than forcing immediate migration.


@@ -1,5 +1,5 @@
using Bit.Core.Models.Mail;
using Bit.Core.Services;
Copy link
Member

Choose a reason for hiding this comment

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

Why does mailer go to it's own namespace and not just in ..Services.Mail, for example?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants