feat(sms): make FromNumber optional — support Twilio Messaging-Service-only configs (UI-Med-2)

Code-review finding UI-Med-2: the design doc + delivery adapter treat FromNumber and MessagingServiceSid as either-or, but the entity ctor, EF schema, UI and CLI all hard- required FromNumber — so a Messaging-Service-only Twilio config (a normal production setup) could not be created. Bring the implementation into line with the spec: - Commons: SmsConfiguration.FromNumber -> string? (ctor fromNumber optional); UpdateSmsConfigCommand.FromNumber -> string?. - ConfigurationDatabase: FromNumber.IsRequired(false) + migration SmsFromNumberOptional (ALTER COLUMN nullable, idempotent; Down backfills '' — harmless, MsgSid keeps it deliverable) + regenerated model snapshot. - Transport: SmsConfigDto.FromNumber -> string? (round-trips a Messaging-Service-only config). - CentralUI: form validation requires AccountSid + at-least-one-of(FromNumber, MsgSid); nullable create/edit paths; From-number help text. - CLI: --from-number no longer Required; BuildUpdateSmsConfigCommand validates the either-or. - Adapter: From branch null-forgiving (guarded by the existing incomplete-config check). Tests: ManagementActor MsgSid-only persists null FromNumber; CLI MsgSid-only builds + neither-throws + contract (--from-number not Required); CentralUI MsgSid-only save.
2026-06-19 15:19:40 -04:00
parent a9393c8913
commit 33e1802e6d
14 changed files with 2133 additions and 28 deletions
@@ -238,8 +238,11 @@ public static class NotificationCommands
        new("--id") { Description = "SMS config ID", Required = true };
    private static readonly Option<string> SmsAccountSidOption =
        new("--account-sid") { Description = "Twilio Account SID", Required = true };
-    private static readonly Option<string> SmsFromNumberOption =
-        new("--from-number") { Description = "Sender phone number (E.164)", Required = true };
+    private static readonly Option<string?> SmsFromNumberOption =
+        new("--from-number")
+        {
+            Description = "Sender phone number (E.164). Provide this and/or --messaging-service-sid.",
+        };
    private static readonly Option<string?> SmsMessagingServiceSidOption =
        new("--messaging-service-sid")
        {
@@ -269,10 +272,18 @@ public static class NotificationCommands
    {
        var id = result.GetValue(SmsIdOption);
        var accountSid = result.GetValue(SmsAccountSidOption)!;
-        var fromNumber = result.GetValue(SmsFromNumberOption)!;
+        var fromNumber = result.GetValue(SmsFromNumberOption);
        var messagingServiceSid = result.GetValue(SmsMessagingServiceSidOption);
        var apiBaseUrl = result.GetValue(SmsApiBaseUrlOption);
        var authToken = result.GetValue(SmsAuthTokenOption);
+        // Whole-replace update: the sender identity must be re-supplied. A Twilio config
+        // needs a From number and/or a Messaging Service SID, so reject the case where the
+        // update would leave neither (mirrors the UI + delivery-adapter validation).
+        if (string.IsNullOrWhiteSpace(fromNumber) && string.IsNullOrWhiteSpace(messagingServiceSid))
+        {
+            throw new ArgumentException(
+                "Provide --from-number and/or --messaging-service-sid (at least one sender identity is required).");
+        }
        return new UpdateSmsConfigCommand(id, accountSid, fromNumber, messagingServiceSid, apiBaseUrl, authToken);
    }

@@ -80,6 +80,7 @@
                            <div class="col-12">
                                <label class="form-label">From Number</label>
                                <input type="text" class="form-control" @bind="_fromNumber" placeholder="+15551234567" />
+                                <div class="form-text">Provide a From Number or a Messaging Service SID (at least one is required).</div>
                            </div>
                            <div class="col-12">
                                <label class="form-label">Messaging Service SID</label>
@@ -195,7 +196,7 @@
    {
        _editingSms = sms;
        _accountSid = sms.AccountSid;
-        _fromNumber = sms.FromNumber;
+        _fromNumber = sms.FromNumber ?? string.Empty;
        _messagingServiceSid = sms.MessagingServiceSid;
        _apiBaseUrl = sms.ApiBaseUrl;
        // Never pre-fill the stored secret; blank means "keep existing".
@@ -216,9 +217,12 @@
    private async Task Save()
    {
        _formError = null;
-        if (string.IsNullOrWhiteSpace(_accountSid) || string.IsNullOrWhiteSpace(_fromNumber))
+        // A valid Twilio config needs an Account SID and at least one sender identity:
+        // a From number OR a Messaging Service SID (the latter is used instead of From).
+        if (string.IsNullOrWhiteSpace(_accountSid)
+            || (string.IsNullOrWhiteSpace(_fromNumber) && string.IsNullOrWhiteSpace(_messagingServiceSid)))
        {
-            _formError = "Account SID and From Number are required.";
+            _formError = "Account SID and either a From Number or a Messaging Service SID are required.";
            return;
        }

@@ -229,7 +233,7 @@
            if (_editingSms != null)
            {
                _editingSms.AccountSid = _accountSid.Trim();
-                _editingSms.FromNumber = _fromNumber.Trim();
+                _editingSms.FromNumber = string.IsNullOrWhiteSpace(_fromNumber) ? null : _fromNumber.Trim();
                _editingSms.MessagingServiceSid = string.IsNullOrWhiteSpace(_messagingServiceSid)
                    ? null
                    : _messagingServiceSid.Trim();
@@ -252,7 +256,9 @@
                    return;
                }

-                var sms = new SmsConfigurationEntity(_accountSid.Trim(), _fromNumber.Trim())
+                var sms = new SmsConfigurationEntity(
+                    _accountSid.Trim(),
+                    string.IsNullOrWhiteSpace(_fromNumber) ? null : _fromNumber.Trim())
                {
                    MessagingServiceSid = string.IsNullOrWhiteSpace(_messagingServiceSid)
                        ? null
@@ -11,8 +11,14 @@ public class SmsConfiguration
    /// during configuration, never a valid production value.
    /// </summary>
    public string? AuthToken { get; set; }
-    /// <summary>Gets or sets the sender phone number (E.164) placed in the From field.</summary>
-    public string FromNumber { get; set; }
+    /// <summary>
+    /// Gets or sets the sender phone number (E.164) placed in the From field, or null when
+    /// delivery is via a <see cref="MessagingServiceSid"/> instead. A valid config has a
+    /// FromNumber and/or a MessagingServiceSid; that either-or invariant is enforced at the
+    /// management/UI boundary and again by the delivery adapter (it is not a ctor invariant
+    /// because MessagingServiceSid is a settable property assigned after construction).
+    /// </summary>
+    public string? FromNumber { get; set; }
    /// <summary>Gets or sets the Twilio Messaging Service SID used instead of a From number, or null.</summary>
    public string? MessagingServiceSid { get; set; }
    /// <summary>Gets or sets the Twilio REST API base URL, or null to use the provider default.</summary>
@@ -39,15 +45,17 @@ public class SmsConfiguration
    public TimeSpan RetryDelay { get; set; }

    /// <summary>
-    /// Initializes a new <see cref="SmsConfiguration"/> with required fields and sensible defaults
-    /// for the numeric and timeout fields.
+    /// Initializes a new <see cref="SmsConfiguration"/> with the required Account SID and
+    /// sensible defaults for the numeric and timeout fields. <paramref name="fromNumber"/>
+    /// is optional — a Twilio Messaging-Service-only config sets <see cref="MessagingServiceSid"/>
+    /// instead and leaves the From number null.
    /// </summary>
    /// <param name="accountSid">Twilio Account SID.</param>
-    /// <param name="fromNumber">Sender phone number (E.164) for the From field.</param>
-    public SmsConfiguration(string accountSid, string fromNumber)
+    /// <param name="fromNumber">Sender phone number (E.164) for the From field, or null for a Messaging-Service-only config.</param>
+    public SmsConfiguration(string accountSid, string? fromNumber = null)
    {
        AccountSid = accountSid ?? throw new ArgumentNullException(nameof(accountSid));
-        FromNumber = fromNumber ?? throw new ArgumentNullException(nameof(fromNumber));
+        FromNumber = fromNumber;
        ConnectionTimeoutSeconds = 30;
        MaxRetries = 10;
        RetryDelay = TimeSpan.FromMinutes(1);
@@ -10,4 +10,6 @@ public record DeleteNotificationListCommand(int NotificationListId);
 public record ListSmtpConfigsCommand;
 public record UpdateSmtpConfigCommand(int SmtpConfigId, string Server, int Port, string AuthMode, string FromAddress, string? TlsMode = null, string? Credentials = null);
 public record ListSmsConfigsCommand;
-public record UpdateSmsConfigCommand(int SmsConfigId, string AccountSid, string FromNumber, string? MessagingServiceSid = null, string? ApiBaseUrl = null, string? AuthToken = null);
+// FromNumber is optional: a Twilio Messaging-Service-only config supplies MessagingServiceSid
+// instead. At-least-one-of (FromNumber, MessagingServiceSid) is validated at the CLI/UI boundary.
+public record UpdateSmsConfigCommand(int SmsConfigId, string AccountSid, string? FromNumber, string? MessagingServiceSid = null, string? ApiBaseUrl = null, string? AuthToken = null);
@@ -73,8 +73,10 @@ public class SmsConfigurationConfiguration : IEntityTypeConfiguration<SmsConfigu
        builder.Property(s => s.AuthToken)
            .HasMaxLength(8000);

+        // Optional: a Twilio Messaging-Service-only config has no From number (a valid
+        // config carries a FromNumber and/or a MessagingServiceSid).
        builder.Property(s => s.FromNumber)
-            .IsRequired()
+            .IsRequired(false)
            .HasMaxLength(32);

        builder.Property(s => s.MessagingServiceSid)
@@ -0,0 +1,33 @@
+using Microsoft.EntityFrameworkCore.Migrations;
+
+#nullable disable
+
+namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Migrations
+{
+    /// <summary>
+    /// SMS code-review (CentralUI-NNN / UI-Med-2): makes <c>SmsConfigurations.FromNumber</c>
+    /// optional. A Twilio Messaging-Service-only config supplies a MessagingServiceSid instead
+    /// of a From number; the design doc, the delivery adapter, and now the UI/CLI treat the two
+    /// as either-or, so the column must allow NULL.
+    ///
+    /// ALTER COLUMN to NULL is naturally idempotent (re-running re-applies the same shape).
+    /// </summary>
+    public partial class SmsFromNumberOptional : Migration
+    {
+        /// <inheritdoc />
+        protected override void Up(MigrationBuilder migrationBuilder)
+        {
+            migrationBuilder.Sql("ALTER TABLE [SmsConfigurations] ALTER COLUMN [FromNumber] nvarchar(32) NULL;");
+        }
+
+        /// <inheritdoc />
+        protected override void Down(MigrationBuilder migrationBuilder)
+        {
+            // Reversing to NOT NULL requires backfilling NULLs first. Unlike the recipient
+            // EmailAddress backfill, '' is a harmless reversal here: a Messaging-Service-only
+            // config keeps its MessagingServiceSid and stays deliverable, so no data is lost.
+            migrationBuilder.Sql("UPDATE [SmsConfigurations] SET [FromNumber] = '' WHERE [FromNumber] IS NULL;");
+            migrationBuilder.Sql("ALTER TABLE [SmsConfigurations] ALTER COLUMN [FromNumber] nvarchar(32) NOT NULL;");
+        }
+    }
+}
@@ -871,7 +871,6 @@ namespace ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Migrations
                        .HasColumnType("int");

                    b.Property<string>("FromNumber")
-                        .IsRequired()
                        .HasMaxLength(32)
                        .HasColumnType("nvarchar(32)");

@@ -211,7 +211,9 @@ public sealed class SmsNotificationDeliveryAdapter : INotificationDeliveryAdapte
            }
            else
            {
-                form.Add(new KeyValuePair<string, string>("From", smsConfig.FromNumber));
+                // The line 113 guard guarantees a non-null FromNumber whenever there is no
+                // MessagingServiceSid, so this branch never sends a null From.
+                form.Add(new KeyValuePair<string, string>("From", smsConfig.FromNumber!));
            }

            using var request = new HttpRequestMessage(HttpMethod.Post, requestUri)
@@ -220,7 +220,7 @@ public sealed record SmtpConfigDto(
 /// </summary>
 public sealed record SmsConfigDto(
    string AccountSid,
-    string FromNumber,
+    string? FromNumber,
    string? MessagingServiceSid,
    string? ApiBaseUrl,
    int ConnectionTimeoutSeconds,
@@ -208,11 +208,37 @@ public class NotificationSmsCommandTests
    public void SmsUpdate_OptionalFlags_AreNotRequired()
    {
        var update = SmsUpdate();
+        Assert.False(update.Options.Single(o => o.Name == "--from-number").Required);
        Assert.False(update.Options.Single(o => o.Name == "--messaging-service-sid").Required);
        Assert.False(update.Options.Single(o => o.Name == "--api-base-url").Required);
        Assert.False(update.Options.Single(o => o.Name == "--auth-token").Required);
    }

+    [Fact]
+    public void SmsUpdate_MessagingServiceSidOnly_NoFromNumber_Builds()
+    {
+        // Twilio Messaging-Service-only config: From number omitted, Messaging Service SID
+        // supplied. The either-or validation accepts it and FromNumber maps to null.
+        var parse = Parse(SmsUpdate(),
+            "--id", "3", "--account-sid", "ACmsg", "--messaging-service-sid", "MGonly");
+
+        Assert.Empty(parse.Errors);
+        var cmd = NotificationCommands.BuildUpdateSmsConfigCommand(parse);
+
+        Assert.Null(cmd.FromNumber);
+        Assert.Equal("MGonly", cmd.MessagingServiceSid);
+    }
+
+    [Fact]
+    public void SmsUpdate_NeitherFromNorMessagingService_Throws()
+    {
+        // Neither sender identity supplied => the build rejects it so an SMS config is
+        // never left with no way to send (mirrors the UI + delivery-adapter validation).
+        var parse = Parse(SmsUpdate(), "--id", "4", "--account-sid", "ACnone");
+
+        Assert.Throws<ArgumentException>(() => NotificationCommands.BuildUpdateSmsConfigCommand(parse));
+    }
+
    [Fact]
    public void SmsCommands_ResolveViaRegistry()
    {
@@ -86,16 +86,19 @@ public class UpdateCommandContractTests
    [Fact]
    public void SmsUpdate_CoreFieldsRequired()
    {
-        // `notification sms update` is a whole-replace of the SMS provider config, so its
-        // identity + non-secret core fields must be Required — a missing --account-sid or
-        // --from-number would otherwise send null and wipe a stored value. --auth-token
-        // stays optional (preserve-if-omitted; empty == omitted, never "clear").
+        // --id and --account-sid are unconditionally Required. --from-number is NOT Required:
+        // it is either-or with --messaging-service-sid (Twilio Messaging-Service-only configs),
+        // so the builder validates "at least one sender identity" instead of the option layer.
+        // --auth-token stays optional (preserve-if-omitted; empty == omitted, never "clear").
        var update = UpdateCommand(NotificationCommands.Build(Url, Format, Username, Password), "sms", "update");
-        AssertRequired(update, "--id", "--account-sid", "--from-number");
+        AssertRequired(update, "--id", "--account-sid");

-        var authToken = update.Options.SingleOrDefault(o => o.Name == "--auth-token");
-        Assert.True(authToken != null, "sms update is missing option '--auth-token'.");
-        Assert.False(authToken!.Required, "sms update '--auth-token' must be optional (preserve-if-omitted).");
+        foreach (var name in new[] { "--from-number", "--auth-token" })
+        {
+            var option = update.Options.SingleOrDefault(o => o.Name == name);
+            Assert.True(option != null, $"sms update is missing option '{name}'.");
+            Assert.False(option!.Required, $"sms update '{name}' must be conditionally validated / optional, not Required.");
+        }
    }

    [Fact]
@@ -126,6 +126,39 @@ public class SmsConfigurationPageTests : BunitContext
        });
    }

+    [Fact]
+    public void SavingNewConfig_MessagingServiceSidOnly_NoFromNumber_Saves()
+    {
+        // UI-Med-2: a Twilio Messaging-Service-only config is valid with no From number.
+        // The either-or validation must accept it and persist a null FromNumber.
+        var repo = RepoWith();
+        Services.AddSingleton(repo);
+        WireAuth();
+
+        var cut = Render<SmsConfigurationPage>();
+        cut.WaitForState(() => cut.Markup.Contains("No SMS configuration set."));
+
+        cut.FindAll("button").First(b => b.TextContent.Contains("Add SMS configuration")).Click();
+
+        // Account SID + Messaging Service SID, leaving From Number (index 1) blank.
+        cut.FindAll("input[type=text]")[0].Change("ACmsg_account");        // Account SID
+        cut.FindAll("input[type=text]")[2].Change("MGmessaging_service");  // Messaging Service SID
+        cut.FindAll("input[type=password]").First().Change("new-token");
+
+        cut.FindAll("button").First(b => b.TextContent.Contains("Save")).Click();
+
+        cut.WaitForAssertion(() =>
+        {
+            repo.Received().AddSmsConfigurationAsync(
+                Arg.Is<SmsConfiguration>(c =>
+                    c.AccountSid == "ACmsg_account" &&
+                    c.FromNumber == null &&
+                    c.MessagingServiceSid == "MGmessaging_service" &&
+                    c.AuthToken == "new-token"));
+            repo.Received().SaveChangesAsync();
+        });
+    }
+
    [Fact]
    public void SavingEdit_WithBlankAuthToken_PreservesExistingToken()
    {
@@ -1705,6 +1705,34 @@ public class ManagementActorTests : TestKit, IDisposable
        Assert.Equal("ACnew", existing.AccountSid);
    }

+    [Fact]
+    public void UpdateSmsConfig_MessagingServiceSidOnly_PersistsNullFromNumber()
+    {
+        // UI-Med-2: a Twilio Messaging-Service-only config has no From number. The handler
+        // must accept and persist a null FromNumber alongside a MessagingServiceSid.
+        var notifRepo = Substitute.For<INotificationRepository>();
+        var existing = new Commons.Entities.Notifications.SmsConfiguration("ACold", "+15550000000")
+        {
+            Id = 1,
+            AuthToken = "old-secret",
+        };
+        notifRepo.GetAllSmsConfigurationsAsync(Arg.Any<CancellationToken>())
+            .Returns(new List<Commons.Entities.Notifications.SmsConfiguration> { existing });
+        _services.AddScoped(_ => notifRepo);
+
+        var actor = CreateActor();
+        var envelope = Envelope(
+            new UpdateSmsConfigCommand(1, "ACnew", FromNumber: null, MessagingServiceSid: "MGonly"),
+            "Administrator");
+
+        actor.Tell(envelope);
+
+        var response = ExpectMsg<ManagementSuccess>(TimeSpan.FromSeconds(5));
+        Assert.Equal(envelope.CorrelationId, response.CorrelationId);
+        Assert.Null(existing.FromNumber);
+        Assert.Equal("MGonly", existing.MessagingServiceSid);
+    }
+
    [Fact]
    public void CuratedHandlerFailure_SurfacesTheCuratedMessage()
    {