fix(sms): code-review fixes — Admin-gate provider-config updates, guard secret-clear/data-loss, type-aware UI

Findings from the per-module code review of the SMS feature (code-reviews/):

- ManagementService (High): UpdateSmsConfig + UpdateSmtpConfig were Designer-gated
  while both /notifications/{sms,smtp} pages enforce RequireAdmin — a Designer
  blocked in the UI could still rotate a production credential via CLI. Moved both
  to the Administrator arm so the actor gate matches the UI.
- ManagementService (Medium): UpdateSmsConfig treated --auth-token "" as a value,
  silently clearing the stored Twilio token. Guard on IsNullOrWhiteSpace so empty ==
  omitted (SMTP Credentials keeps its null-only guard — empty is valid for no-auth).
- CentralUI (Medium): NotificationLists recipient badge rendered Name <Email>
  unconditionally, showing "Name <>" for SMS lists. Now type-aware (phone for SMS).
- ConfigurationDatabase (Medium): AddSmsNotifications.Down() backfilled NULL emails
  to '' — silent data loss for SMS-only recipients. Added a pre-drop guard that
  refuses rollback while such rows exist.
- NotificationOutbox (Low): SMS body truncation could split a surrogate pair at the
  cap boundary; back off one code unit to stay well-formed.
- Commons (Low): NotificationRecipient public ctor name-guard now matches the
  ForEmail factory (IsNullOrWhiteSpace). Documented SmsConfiguration.MaxRetries/
  RetryDelay as RESERVED (dispatcher reuses the shared SMTP-derived retry policy).
This commit is contained in:
Joseph Doherty
2026-06-19 15:02:02 -04:00
parent d6ead8ae62
commit cd8e4872f6
6 changed files with 77 additions and 11 deletions
@@ -20,7 +20,17 @@ public class NotificationRecipient
/// <param name="emailAddress">Email address of the recipient.</param>
public NotificationRecipient(string name, string emailAddress)
{
Name = name ?? throw new ArgumentNullException(nameof(name));
// Match the ForEmail factory's guard so the invariant ("a recipient always
// has a non-blank display name") holds regardless of construction path. EF
// materializes via the private parameterless ctor + property injection — an
// SMS-only recipient has a null EmailAddress — so this ctor is only reached
// by code that genuinely intends the email path.
if (string.IsNullOrWhiteSpace(name))
{
throw new ArgumentException("Name must not be empty.", nameof(name));
}
Name = name;
EmailAddress = emailAddress ?? throw new ArgumentNullException(nameof(emailAddress));
}
@@ -17,11 +17,25 @@ public class SmsConfiguration
public string? MessagingServiceSid { get; set; }
/// <summary>Gets or sets the Twilio REST API base URL, or null to use the provider default.</summary>
public string? ApiBaseUrl { get; set; }
/// <summary>Gets or sets the connection timeout in seconds.</summary>
/// <summary>Gets or sets the connection timeout in seconds. Honored per-send by the SMS delivery adapter.</summary>
public int ConnectionTimeoutSeconds { get; set; }
/// <summary>Gets or sets the maximum number of delivery retries before parking.</summary>
/// <summary>
/// Gets or sets the maximum number of delivery retries before parking.
/// <para>
/// RESERVED: the Notification Outbox dispatcher currently derives the retry policy
/// (max-retries + interval) from the central SMTP configuration for <em>all</em>
/// notification types — see the "retry reuses central SMTP max-retry-count and fixed
/// interval" design decision and <c>NotificationOutboxActor.ResolveRetryPolicyAsync</c>.
/// This per-SMS value is persisted/transported for forward-compatibility but is NOT
/// yet read at dispatch time. Honoring it per-type is a deferred enhancement (it would
/// supersede the shared-SMTP-policy decision and is not a silent behavioral change).
/// </para>
/// </summary>
public int MaxRetries { get; set; }
/// <summary>Gets or sets the delay between retry attempts.</summary>
/// <summary>
/// Gets or sets the delay between retry attempts. RESERVED — see <see cref="MaxRetries"/>:
/// the dispatcher currently uses the shared SMTP-derived retry interval for all types.
/// </summary>
public TimeSpan RetryDelay { get; set; }
/// <summary>