Trim the mail notification doc comments

Author: maxkpower

bitwarden_license/src/Services/Pam/Services/ILeaseRevokedMailNotifier.cs

@@ -4,9 +4,8 @@
 namespace Bit.Services.Pam.Services;
 
 /// <summary>
-/// Emails a lease holder that an operator ended their active access — the out-of-band twin of the
-/// <c>RefreshAccessRequest</c> push that <see cref="IRequesterNotifier" /> sends on the same path. The push has
-/// already re-locked the item on any client that happened to be open; this tells the person mid-task why.
+/// Emails a lease holder that an operator ended their active access: the out-of-band twin of the
+/// <c>RefreshAccessRequest</c> push from <see cref="IRequesterNotifier" /> on the same path.
 /// </summary>
 /// <remarks>
 /// A courtesy, not a security control. The lease is dead server-side before anything here runs, so nothing this
@@ -19,13 +18,9 @@ public interface ILeaseRevokedMailNotifier
 {
     /// <summary>
     /// Tells <paramref name="lease" />'s holder that their access ended, but only when
-    /// <paramref name="endAction" /> is <see cref="AccessLeaseAction.Revoked" />.
+    /// <paramref name="endAction" /> is <see cref="AccessLeaseAction.Revoked" />. The name is neutral because this
+    /// is handed every early end, so the rule that a holder is never mailed about their own action lives here.
     /// </summary>
-    /// <remarks>
-    /// Handed every early end rather than only the revocations so the rule that a holder is never mailed about
-    /// their own action lives in one place. Mailing someone "your access was revoked" seconds after they ended it
-    /// themselves is the kind of notification that teaches people to ignore the channel.
-    /// </remarks>
     /// <param name="lease">The lease just ended. Its <c>Action</c> may not be stamped yet at the call site.</param>
     /// <param name="endAction">
     /// How it ended, passed rather than read from <paramref name="lease" /> for that reason:

bitwarden_license/src/Services/Pam/Services/LeaseRevokedMailNotifier.cs

@@ -38,7 +38,7 @@ public async Task NotifyLeaseEndedAsync(AccessLease lease, AccessLeaseAction end
         }
 
         // Duplicates the guard inside IAccessMailNotifier to keep the organization read off every revocation in the
-        // flag-off state — which is every revocation on self-host.
+        // flag-off state, which is every revocation on self-host.
         if (!_featureService.IsEnabled(FeatureFlagKeys.Pam))
         {
             return;

bitwarden_license/test/Services/Pam.Test/Services/LeaseRevokedMailNotifierTests.cs

@@ -41,10 +41,6 @@ public async Task NotifyLeaseEndedAsync_Revoked_MailsTheHolderWithTheWindowItCut
         Assert.Equal($"{_vaultUrl}/pam/requests/{lease.AccessRequestId}", mail.View.Url);
     }
 
-    /// <summary>
-    /// The point of the whole feature. A holder who ends their own access already knows, and a mail thirty seconds
-    /// behind their own click is what teaches people to filter the channel.
-    /// </summary>
     [Theory]
     [BitAutoData(AccessLeaseAction.Cancelled)]
     [BitAutoData(AccessLeaseAction.None)]

src/Core/Pam/Models/Mail/AccessLeaseRevoked/AccessLeaseRevokedView.cs

@@ -7,22 +7,19 @@ namespace Bit.Core.Pam.Models.Mail.AccessLeaseRevoked;
 /// The lease holder's notice that someone else revoked their active access before its window ran out.
 /// </summary>
 /// <remarks>
-/// This is a courtesy, not a control: the lease is already dead and the client has already re-locked by the time
-/// this is composed. It exists so someone mid-task learns why their item locked itself.
-///
-/// Sent only when an operator revoked the lease. A holder who ends their own access is not mailed about it, so
-/// there is no field here for who ended it — this view only ever describes the one case.
+/// A courtesy, not a control: the lease is already dead and the client has already re-locked by the time this is
+/// composed. It is sent only when an operator revoked the lease, so there is no field for who ended it.
 ///
 /// Bounded by zero knowledge in the same way its siblings are: the collection and cipher are named only by
-/// ciphertext the server cannot read. The revocation reason is left out for the reason <c>AccessRequest.Reason</c>
-/// is — free text that would name the very system being accessed, rendered into an HTML mail body.
+/// ciphertext the server cannot read, and the revocation reason is withheld for the reason
+/// <c>AccessRequest.Reason</c> is, being free text that would name the very system being accessed.
 /// <see cref="Url" /> carries the holder to it.
 /// </remarks>
 public class AccessLeaseRevokedView : BaseMailView
 {
     /// <summary>
-    /// UTC is spelled out in the rendered string. There is no per-recipient timezone on this path, so an
-    /// unqualified instant would be read as local time and would misstate how much of the window was cut short.
+    /// UTC is spelled out in the rendered string: there is no per-recipient timezone on this path, so an unqualified
+    /// instant would be read as local time and misstate how much of the window was cut short.
     /// </summary>
     private const string _windowFormat = "d MMM yyyy 'at' HH:mm 'UTC'";
 
@@ -31,24 +28,22 @@ public class AccessLeaseRevokedView : BaseMailView
     /// <summary>The request the ended lease was minted from, which is what <see cref="Url" /> addresses.</summary>
     public required Guid AccessRequestId { get; init; }
 
-    /// <summary>Plaintext, unlike the collection and cipher names this mail deliberately omits.</summary>
     public required string OrganizationName { get; init; }
 
     /// <summary>
     /// When the lease would have ended on its own, in UTC. Always in the future at send time:
-    /// <c>RevokeAccessLeaseCommand</c> refuses a lease whose window has already closed, so a revoke is always an
-    /// early end and this is what it cut short.
+    /// <c>RevokeAccessLeaseCommand</c> refuses a lease whose window has already closed.
     /// </summary>
     public required DateTime NotAfter { get; init; }
 
     public string ScheduledEnd => NotAfter.ToString(_windowFormat, CultureInfo.InvariantCulture);
 
     /// <summary>
-    /// The holder's own view of the request this lease came from, where the revocation is recorded as a decision
-    /// with whatever reason the operator gave. The user-scoped PAM pages mount at <c>pam</c>
-    /// (<c>apps/web/src/app/oss-routing.module.ts:687</c>) and the request page is <c>requests/:id</c> beneath it
-    /// (<c>access-requests-routing.module.ts:47</c>). The organization-scoped admin surface under
-    /// <c>/organizations/:organizationId/pam</c> is a different route tree and does not serve this page.
+    /// The request this lease came from, where the revocation is recorded with whatever reason the operator gave.
+    /// The user-scoped PAM pages mount at <c>pam</c> (<c>apps/web/src/app/oss-routing.module.ts:687</c>) with the
+    /// request page at <c>requests/:id</c> beneath it (<c>access-requests-routing.module.ts:47</c>). The
+    /// organization-scoped tree under <c>/organizations/:organizationId/pam</c> is a different route tree and does
+    /// not serve this page.
     /// </summary>
     public string Url => $"{WebVaultUrl}/pam/requests/{AccessRequestId}";
 }

test/Core.Test/Pam/Models/Mail/AccessLeaseRevoked/AccessLeaseRevokedViewTests.cs

@@ -12,11 +12,7 @@ public class AccessLeaseRevokedViewTests
 {
     private static readonly Guid _requestId = Guid.Parse("6f1b2d84-0c37-4a91-8e55-1d7c93a4b208");
 
-    /// <summary>
-    /// <see cref="HandlebarMailRenderer" /> resolves both templates from the view's full class name and only fails
-    /// when a mail is actually sent, which no other spec in this feature reaches. This is the spec that catches a
-    /// misnamed or misplaced <c>.hbs</c>.
-    /// </summary>
+    /// <summary>The only spec that renders, so the only one that catches a misnamed or misplaced <c>.hbs</c>.</summary>
     [Fact]
     public async Task RenderAsync_SaysAccessWasRevokedAndPointsAtTheRequest()
     {
@@ -32,10 +28,7 @@ public async Task RenderAsync_SaysAccessWasRevokedAndPointsAtTheRequest()
         }
     }
 
-    /// <summary>
-    /// A revoked lease is over. Copy that implies otherwise sends someone back to a "Start access" button that
-    /// will not help them, so the body has to say both that it cannot be resumed and what to do instead.
-    /// </summary>
+    /// <summary>A revoked lease is over; copy implying otherwise sends the holder to a button that cannot help.</summary>
     [Fact]
     public async Task RenderAsync_SaysTheAccessCannotBeResumedAndThatANewRequestIsNeeded()
     {
@@ -50,11 +43,7 @@ public async Task RenderAsync_SaysTheAccessCannotBeResumedAndThatANewRequestIsNe
         }
     }
 
-    /// <summary>
-    /// The reason an operator gave is free text that may name the very system being accessed, so it is linked to
-    /// rather than rendered — the same call the request's reason and the approver's comment get. Nothing on the
-    /// view can carry it.
-    /// </summary>
+    /// <summary>The reason is free text that may name the system being accessed, so it is linked to, not rendered.</summary>
     [Fact]
     public void View_HasNoPlaceToCarryTheRevocationReason() =>
         Assert.DoesNotContain(
@@ -63,9 +52,6 @@ public void View_HasNoPlaceToCarryTheRevocationReason() =>
                 || property.Name.Contains("Comment", StringComparison.OrdinalIgnoreCase)
                 || property.Name.Contains("Detail", StringComparison.OrdinalIgnoreCase));
 
-    /// <summary>
-    /// An organization name is member-supplied text, so the HTML body must escape it rather than interpolate it raw.
-    /// </summary>
     [Fact]
     public async Task RenderAsync_EscapesTheOrganizationNameInTheHtmlBody()
     {
@@ -93,10 +79,7 @@ public void Subject_SaysTheAccessWasRevokedWithoutNamingTheItemOrTheReason() =>
         NotAfter = new DateTime(2026, 9, 1, 17, 0, 0, DateTimeKind.Utc),
     };
 
-    /// <summary>
-    /// Both templates wrap their copy across source lines, so a sentence these specs assert on is not contiguous in
-    /// the rendered output. Collapsing runs of whitespace keeps them about the wording rather than the line breaks.
-    /// </summary>
+    /// <summary>Both templates wrap copy across source lines; collapsing whitespace keeps the specs about wording.</summary>
     private static string Reflow(string body) => Regex.Replace(body, @"\s+", " ");
 
     private static Task<(string html, string txt)> RenderAsync(BaseMailView view) =>