fix(deployment): drop unused INotificationRepository from ArtifactDeploymentService (review 2026-10-21 04/U4)
This commit is contained in:
@@ -50,12 +50,6 @@ public class ArtifactDeploymentService
|
|||||||
/// <param name="deploymentRepo">Repository for deployment records.</param>
|
/// <param name="deploymentRepo">Repository for deployment records.</param>
|
||||||
/// <param name="templateRepo">Repository for templates.</param>
|
/// <param name="templateRepo">Repository for templates.</param>
|
||||||
/// <param name="externalSystemRepo">Repository for external systems.</param>
|
/// <param name="externalSystemRepo">Repository for external systems.</param>
|
||||||
/// <param name="notificationRepo">
|
|
||||||
/// Retained on the signature for DI/source compatibility but
|
|
||||||
/// intentionally NOT consumed. Notification lists and SMTP configuration are
|
|
||||||
/// central-only and are never shipped to sites, so the artifact path must not read
|
|
||||||
/// the notification repository at all.
|
|
||||||
/// </param>
|
|
||||||
/// <param name="communicationService">Service for communicating with sites.</param>
|
/// <param name="communicationService">Service for communicating with sites.</param>
|
||||||
/// <param name="auditService">Service for audit logging.</param>
|
/// <param name="auditService">Service for audit logging.</param>
|
||||||
/// <param name="options">Deployment manager options.</param>
|
/// <param name="options">Deployment manager options.</param>
|
||||||
@@ -75,7 +69,6 @@ public class ArtifactDeploymentService
|
|||||||
IDeploymentManagerRepository deploymentRepo,
|
IDeploymentManagerRepository deploymentRepo,
|
||||||
ITemplateEngineRepository templateRepo,
|
ITemplateEngineRepository templateRepo,
|
||||||
IExternalSystemRepository externalSystemRepo,
|
IExternalSystemRepository externalSystemRepo,
|
||||||
INotificationRepository notificationRepo,
|
|
||||||
CommunicationService communicationService,
|
CommunicationService communicationService,
|
||||||
IAuditService auditService,
|
IAuditService auditService,
|
||||||
IOptions<DeploymentManagerOptions> options,
|
IOptions<DeploymentManagerOptions> options,
|
||||||
@@ -87,9 +80,6 @@ public class ArtifactDeploymentService
|
|||||||
_deploymentRepo = deploymentRepo;
|
_deploymentRepo = deploymentRepo;
|
||||||
_templateRepo = templateRepo;
|
_templateRepo = templateRepo;
|
||||||
_externalSystemRepo = externalSystemRepo;
|
_externalSystemRepo = externalSystemRepo;
|
||||||
// notificationRepo is deliberately not stored — notification
|
|
||||||
// lists and SMTP configs are central-only and are never fetched for shipping to sites.
|
|
||||||
_ = notificationRepo;
|
|
||||||
_communicationService = communicationService;
|
_communicationService = communicationService;
|
||||||
_auditService = auditService;
|
_auditService = auditService;
|
||||||
_options = options.Value;
|
_options = options.Value;
|
||||||
|
|||||||
@@ -50,7 +50,6 @@ public class SitesPageTests : BunitContext
|
|||||||
Substitute.For<IDeploymentManagerRepository>(),
|
Substitute.For<IDeploymentManagerRepository>(),
|
||||||
Substitute.For<ITemplateEngineRepository>(),
|
Substitute.For<ITemplateEngineRepository>(),
|
||||||
Substitute.For<IExternalSystemRepository>(),
|
Substitute.For<IExternalSystemRepository>(),
|
||||||
Substitute.For<INotificationRepository>(),
|
|
||||||
comms,
|
comms,
|
||||||
Substitute.For<IAuditService>(),
|
Substitute.For<IAuditService>(),
|
||||||
Options.Create(new DeploymentManagerOptions()),
|
Options.Create(new DeploymentManagerOptions()),
|
||||||
|
|||||||
+22
-12
@@ -26,7 +26,6 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
private readonly IDeploymentManagerRepository _deploymentRepo;
|
private readonly IDeploymentManagerRepository _deploymentRepo;
|
||||||
private readonly ITemplateEngineRepository _templateRepo;
|
private readonly ITemplateEngineRepository _templateRepo;
|
||||||
private readonly IExternalSystemRepository _externalSystemRepo;
|
private readonly IExternalSystemRepository _externalSystemRepo;
|
||||||
private readonly INotificationRepository _notificationRepo;
|
|
||||||
private readonly IAuditService _audit;
|
private readonly IAuditService _audit;
|
||||||
private readonly ArtifactRevisionHashService _hashService = new();
|
private readonly ArtifactRevisionHashService _hashService = new();
|
||||||
private readonly ISecretStore _secretStore;
|
private readonly ISecretStore _secretStore;
|
||||||
@@ -41,7 +40,6 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
// The notification repo is retained only so the
|
// The notification repo is retained only so the
|
||||||
// tests can assert the artifact path NEVER touches it (it is no longer a
|
// tests can assert the artifact path NEVER touches it (it is no longer a
|
||||||
// constructor dependency of ArtifactDeploymentService).
|
// constructor dependency of ArtifactDeploymentService).
|
||||||
_notificationRepo = Substitute.For<INotificationRepository>();
|
|
||||||
_audit = Substitute.For<IAuditService>();
|
_audit = Substitute.For<IAuditService>();
|
||||||
// Unconfigured GetAsync returns null (NSubstitute default), i.e. "not in the store" —
|
// Unconfigured GetAsync returns null (NSubstitute default), i.e. "not in the store" —
|
||||||
// exactly the fail-closed posture a test wants unless it explicitly seeds a row.
|
// exactly the fail-closed posture a test wants unless it explicitly seeds a row.
|
||||||
@@ -208,11 +206,10 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
await _templateRepo.Received(1).GetAllSharedScriptsAsync(Arg.Any<CancellationToken>());
|
await _templateRepo.Received(1).GetAllSharedScriptsAsync(Arg.Any<CancellationToken>());
|
||||||
await _externalSystemRepo.Received(1).GetAllExternalSystemsAsync(Arg.Any<CancellationToken>());
|
await _externalSystemRepo.Received(1).GetAllExternalSystemsAsync(Arg.Any<CancellationToken>());
|
||||||
await _externalSystemRepo.Received(1).GetAllDatabaseConnectionsAsync(Arg.Any<CancellationToken>());
|
await _externalSystemRepo.Received(1).GetAllDatabaseConnectionsAsync(Arg.Any<CancellationToken>());
|
||||||
// Notification lists and SMTP configuration are
|
// Notification lists and SMTP configuration are central-only — the service has no
|
||||||
// central-only — the artifact path must NEVER fetch them, and the per-site
|
// notification repository at all (see
|
||||||
// command must NEVER carry them.
|
// Constructor_TakesNoNotificationRepository), and the per-site command must NEVER
|
||||||
await _notificationRepo.DidNotReceive().GetAllNotificationListsAsync(Arg.Any<CancellationToken>());
|
// carry them.
|
||||||
await _notificationRepo.DidNotReceive().GetAllSmtpConfigurationsAsync(Arg.Any<CancellationToken>());
|
|
||||||
Assert.All(recorder.Received, cmd =>
|
Assert.All(recorder.Received, cmd =>
|
||||||
{
|
{
|
||||||
Assert.Null(cmd.NotificationLists);
|
Assert.Null(cmd.NotificationLists);
|
||||||
@@ -278,9 +275,7 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
await _templateRepo.Received(1).GetAllSharedScriptsAsync(Arg.Any<CancellationToken>());
|
await _templateRepo.Received(1).GetAllSharedScriptsAsync(Arg.Any<CancellationToken>());
|
||||||
await _externalSystemRepo.Received(1).GetAllExternalSystemsAsync(Arg.Any<CancellationToken>());
|
await _externalSystemRepo.Received(1).GetAllExternalSystemsAsync(Arg.Any<CancellationToken>());
|
||||||
await _externalSystemRepo.Received(1).GetAllDatabaseConnectionsAsync(Arg.Any<CancellationToken>());
|
await _externalSystemRepo.Received(1).GetAllDatabaseConnectionsAsync(Arg.Any<CancellationToken>());
|
||||||
// Central-only — never fetched, never shipped.
|
// Central-only — never fetched (no notification repository), never shipped.
|
||||||
await _notificationRepo.DidNotReceive().GetAllNotificationListsAsync(Arg.Any<CancellationToken>());
|
|
||||||
await _notificationRepo.DidNotReceive().GetAllSmtpConfigurationsAsync(Arg.Any<CancellationToken>());
|
|
||||||
Assert.All(recorder.Received, cmd =>
|
Assert.All(recorder.Received, cmd =>
|
||||||
{
|
{
|
||||||
Assert.Null(cmd.NotificationLists);
|
Assert.Null(cmd.NotificationLists);
|
||||||
@@ -1105,6 +1100,21 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
Assert.DoesNotContain("Ciphertext", json, StringComparison.OrdinalIgnoreCase);
|
Assert.DoesNotContain("Ciphertext", json, StringComparison.OrdinalIgnoreCase);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// review 2026-10-21 04/U4: notification lists and SMTP configs are central-only and never
|
||||||
|
/// shipped, so the service must not depend on the notification repository at all — a
|
||||||
|
/// dead DI edge kept a spurious DeploymentManager→Notification dependency alive.
|
||||||
|
/// </summary>
|
||||||
|
[Fact]
|
||||||
|
public void Constructor_TakesNoNotificationRepository()
|
||||||
|
{
|
||||||
|
var parameterTypes = typeof(ArtifactDeploymentService).GetConstructors()
|
||||||
|
.SelectMany(c => c.GetParameters())
|
||||||
|
.Select(p => p.ParameterType);
|
||||||
|
|
||||||
|
Assert.DoesNotContain(typeof(INotificationRepository), parameterTypes);
|
||||||
|
}
|
||||||
|
|
||||||
private ArtifactDeploymentService CreateService(ILogger<ArtifactDeploymentService>? logger = null)
|
private ArtifactDeploymentService CreateService(ILogger<ArtifactDeploymentService>? logger = null)
|
||||||
{
|
{
|
||||||
var comms = new CommunicationService(
|
var comms = new CommunicationService(
|
||||||
@@ -1112,7 +1122,7 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
NullLogger<CommunicationService>.Instance);
|
NullLogger<CommunicationService>.Instance);
|
||||||
|
|
||||||
return new ArtifactDeploymentService(
|
return new ArtifactDeploymentService(
|
||||||
_siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo, _notificationRepo,
|
_siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo,
|
||||||
comms, _audit,
|
comms, _audit,
|
||||||
Options.Create(new DeploymentManagerOptions()),
|
Options.Create(new DeploymentManagerOptions()),
|
||||||
logger ?? NullLogger<ArtifactDeploymentService>.Instance,
|
logger ?? NullLogger<ArtifactDeploymentService>.Instance,
|
||||||
@@ -1132,7 +1142,7 @@ public class ArtifactDeploymentServiceTests : TestKit
|
|||||||
comms.SetCommunicationActor(commActor);
|
comms.SetCommunicationActor(commActor);
|
||||||
|
|
||||||
return new ArtifactDeploymentService(
|
return new ArtifactDeploymentService(
|
||||||
_siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo, _notificationRepo,
|
_siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo,
|
||||||
comms, _audit,
|
comms, _audit,
|
||||||
Options.Create(new DeploymentManagerOptions
|
Options.Create(new DeploymentManagerOptions
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -39,7 +39,6 @@ public class PendingUpdatesEvaluatorTests
|
|||||||
private readonly ISiteRepository _siteRepo = Substitute.For<ISiteRepository>();
|
private readonly ISiteRepository _siteRepo = Substitute.For<ISiteRepository>();
|
||||||
private readonly ITemplateEngineRepository _templateRepo = Substitute.For<ITemplateEngineRepository>();
|
private readonly ITemplateEngineRepository _templateRepo = Substitute.For<ITemplateEngineRepository>();
|
||||||
private readonly IExternalSystemRepository _externalSystemRepo = Substitute.For<IExternalSystemRepository>();
|
private readonly IExternalSystemRepository _externalSystemRepo = Substitute.For<IExternalSystemRepository>();
|
||||||
private readonly INotificationRepository _notificationRepo = Substitute.For<INotificationRepository>();
|
|
||||||
private readonly IDeploymentManagerRepository _deploymentRepo = Substitute.For<IDeploymentManagerRepository>();
|
private readonly IDeploymentManagerRepository _deploymentRepo = Substitute.For<IDeploymentManagerRepository>();
|
||||||
private readonly IFlatteningPipeline _pipeline = Substitute.For<IFlatteningPipeline>();
|
private readonly IFlatteningPipeline _pipeline = Substitute.For<IFlatteningPipeline>();
|
||||||
private readonly IAuditService _audit = Substitute.For<IAuditService>();
|
private readonly IAuditService _audit = Substitute.For<IAuditService>();
|
||||||
@@ -63,7 +62,7 @@ public class PendingUpdatesEvaluatorTests
|
|||||||
NullLogger<CommunicationService>.Instance);
|
NullLogger<CommunicationService>.Instance);
|
||||||
|
|
||||||
_artifactService = new ArtifactDeploymentService(
|
_artifactService = new ArtifactDeploymentService(
|
||||||
_siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo, _notificationRepo,
|
_siteRepo, _deploymentRepo, _templateRepo, _externalSystemRepo,
|
||||||
comms, _audit,
|
comms, _audit,
|
||||||
Options.Create(new DeploymentManagerOptions()),
|
Options.Create(new DeploymentManagerOptions()),
|
||||||
NullLogger<ArtifactDeploymentService>.Instance,
|
NullLogger<ArtifactDeploymentService>.Instance,
|
||||||
|
|||||||
@@ -81,7 +81,6 @@ public class ManagementActorTests : TestKit, IDisposable
|
|||||||
Substitute.For<IDeploymentManagerRepository>(),
|
Substitute.For<IDeploymentManagerRepository>(),
|
||||||
Substitute.For<ITemplateEngineRepository>(),
|
Substitute.For<ITemplateEngineRepository>(),
|
||||||
Substitute.For<IExternalSystemRepository>(),
|
Substitute.For<IExternalSystemRepository>(),
|
||||||
Substitute.For<INotificationRepository>(),
|
|
||||||
comms,
|
comms,
|
||||||
Substitute.For<IAuditService>(),
|
Substitute.For<IAuditService>(),
|
||||||
Options.Create(new DeploymentManagerOptions()),
|
Options.Create(new DeploymentManagerOptions()),
|
||||||
|
|||||||
Reference in New Issue
Block a user