diff --git a/src/DfE.CheckPerformanceData.RulesEngineWorker/Consumers/ZendeskConsumer.cs b/src/DfE.CheckPerformanceData.RulesEngineWorker/Consumers/ZendeskConsumer.cs index 8c612da01..3e0f5e5ad 100644 --- a/src/DfE.CheckPerformanceData.RulesEngineWorker/Consumers/ZendeskConsumer.cs +++ b/src/DfE.CheckPerformanceData.RulesEngineWorker/Consumers/ZendeskConsumer.cs @@ -135,24 +135,49 @@ public override async Task ProcessMessageBodyAsync(string messageBody, Cancellat // Durably claim the "ticket created" transition before calling Zendesk. The claim flips // the status out of RulesProcessed in a single atomic UPDATE, so of two concurrent // deliveries that both saw CrmId == null only one flips a row — the loser gets zero rows - // affected and skips without ever calling Zendesk, so no duplicate ticket is created. - // A redelivery of this worker's own crashed attempt (status already Creating, CrmId - // still null) re-claims and retries rather than stranding the request. + // affected and never calls Zendesk, so no duplicate ticket is created. A failed attempt + // hands its claim back (see the catch below) so its own redelivery can claim afresh. if (!await TryClaimTicketCreationAsync(message.ReferenceNumber, cancellationToken)) { - return; + // A lost claim is only benign when a ticket now exists (a concurrent delivery won and + // finished). Otherwise the row is stuck in ZendeskTicketCreating — an attempt that died + // mid-call, or another worker still in flight — and returning here would ACK the + // message as handled and lose the request with nothing dead-lettered. Throw instead: + // the queue retries, and a row that stays stuck reaches the DLQ with this reason. + var current = await LoadChangeRequestAsync(message.ReferenceNumber, cancellationToken); + if (!string.IsNullOrEmpty(current?.CrmId)) + { + return; + } + + throw new InvalidOperationException( + $"Could not claim ticket creation for Reference={message.ReferenceNumber}: " + + $"WorkerStatus={current?.WorkerStatus?.ToString() ?? "null"}, row found={current is not null}."); } var decision = DeriveDecision(message, changeRequest); var ticket = BuildTicket(message, decision); - var response = await _zendeskService.CreateTicketAsync(ticket); - var files = message.Answers - .Where(a => a.Files is not null) - .SelectMany(a => a.Files!); - foreach (var file in files) + CreateTicketResponseDto response; + try { - await UploadAttachmentToTicketAsync(response.Ticket.Id, message.CheckingWindowId, file, cancellationToken); + response = await _zendeskService.CreateTicketAsync(ticket); + + var files = message.Answers + .Where(a => a.Files is not null) + .SelectMany(a => a.Files!); + foreach (var file in files) + { + await UploadAttachmentToTicketAsync(response.Ticket.Id, message.CheckingWindowId, file, cancellationToken); + } + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + // Hand the claim back so the redelivery can take it. Without this the retry finds the + // row in ZendeskTicketCreating, cannot claim it, and the request is lost — every one of + // a 120-request preprod replay went this way after Zendesk returned 422. + await ReleaseTicketCreationClaimAsync(message.ReferenceNumber, cancellationToken); + throw; } var crmId = response.Ticket.Id.ToString(CultureInfo.InvariantCulture); @@ -174,15 +199,21 @@ await _dbContext.ChangeRequests response.Ticket.Id, message.ReferenceNumber, decision.Status, decision.MatchedRuleId); } - // Atomically flips the request into the in-progress "creating" state: a RulesProcessed row - // (rules-consumer path) or an enquiry row — which never passes the rules engine, so its - // WorkerStatus is NULL and it is claimed only when RequestType is ResultsEnquiry (FR-002). - // Returns true to the single delivery that wins the flip. The flip is exclusive: Postgres - // takes a row lock on the matching row, so of two concurrent deliveries the second re-checks - // its WHERE against the winner's committed row — which is no longer claimable — and - // matches zero rows. The loser therefore skips without ever calling Zendesk, so concurrent - // redelivery cannot create a duplicate ticket. CrmId == null keeps an already-ticketed - // request (whose status is ZendeskTicketCreated) from ever being re-claimed. + // Atomically claims the request for ticket creation. Returns true to the single delivery that + // wins. The flip is exclusive: Postgres takes a row lock on the matching row, so of two + // concurrent deliveries the second re-checks its WHERE against the winner's committed row — + // now ZendeskTicketCreating — and matches zero rows, so it never calls Zendesk and no duplicate + // ticket is created. CrmId == null keeps an already-ticketed request from being re-claimed. + // + // Two states are claimable, both meaning "no ticket exists and nobody is creating one": + // - RulesProcessed: the ordinary path, a rules decision has been recorded; + // - null, for a results enquiry only: an enquiry never passes the rules engine (FR-002), so + // its status is NULL by design and DeriveDecision falls back to Scrutiny for it. A null + // AMENDMENT is deliberately not claimable (SC-005): it means the rules decision was never + // recorded, and it must surface on the DLQ rather than be ticketed under a guess. + // ZendeskTicketCreating is deliberately NOT claimable — that is what makes the claim exclusive + // under concurrency — so a failed attempt must hand its claim back (ReleaseTicketCreationClaimAsync) + // for the redelivery to take. private async Task TryClaimTicketCreationAsync(string referenceNumber, CancellationToken cancellationToken) { var claimed = 0; @@ -201,6 +232,33 @@ await _dbContext.ExecuteInTransactionAsync(async () => return claimed == 1; } + // Undoes TryClaimTicketCreationAsync after a failed create so the redelivery can claim again. + // Guarded on CrmId == null and the Creating state so it can never disturb a row that a + // concurrent delivery has since ticketed. Best effort: if this write itself fails, the original + // exception is the one worth surfacing, and the row's stuck state surfaces on the next attempt. + private async Task ReleaseTicketCreationClaimAsync(string referenceNumber, CancellationToken cancellationToken) + { + try + { + await _dbContext.ExecuteInTransactionAsync(async () => + { + await _dbContext.ChangeRequests + .Where(r => r.ReferenceNumber == referenceNumber + && r.CrmId == null + && r.WorkerStatus == WorkerStatus.ZendeskTicketCreating) + .ExecuteUpdateAsync(s => s + .SetProperty(r => r.WorkerStatus, WorkerStatus.RulesProcessed), + cancellationToken); + }, cancellationToken); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger.LogWarning(ex, + "Could not release the ticket-creation claim for Reference={Reference}; the next attempt will report the stuck row.", + referenceNumber); + } + } + private async Task LoadChangeRequestAsync(string referenceNumber, CancellationToken cancellationToken) { try @@ -352,14 +410,27 @@ private void MapPupilFields(CreateTicketRequestDto dto, RequestDocument message) }); } + // CYPMD_ID is an integer field in Zendesk, and a value that does not parse as one makes + // Zendesk reject the whole ticket (422 "CYPMD ID: is invalid") — which is how a UAT seed + // with "CY0045" ids sank an entire close-exercise replay. Losing the one field is the + // lesser harm: the ticket still carries the UPN, name and DOB to identify the pupil. var cypmdFieldId = GetConfiguredFieldId(ZendeskTicketFieldConstants.CypmdName); if (cypmdFieldId.HasValue) { - dto.Ticket.CustomFields.Add(new CustomFieldDto + if (long.TryParse(message.Pupil.CypmdId, NumberStyles.None, CultureInfo.InvariantCulture, out _)) { - Id = cypmdFieldId.Value, - Value = message.Pupil.CypmdId, - }); + dto.Ticket.CustomFields.Add(new CustomFieldDto + { + Id = cypmdFieldId.Value, + Value = message.Pupil.CypmdId, + }); + } + else + { + _logger.LogWarning( + "CYPMD id for Reference={Reference} is not an integer; omitting the CYPMD_ID field from the ticket.", + message.ReferenceNumber); + } } var upnId = GetConfiguredFieldId(ZendeskTicketFieldConstants.UpnName); diff --git a/src/DfE.CheckPerformanceData.RulesEngineWorker/DfE.CheckPerformanceData.RulesEngineWorker.csproj b/src/DfE.CheckPerformanceData.RulesEngineWorker/DfE.CheckPerformanceData.RulesEngineWorker.csproj index 980899e53..2f0db8366 100644 --- a/src/DfE.CheckPerformanceData.RulesEngineWorker/DfE.CheckPerformanceData.RulesEngineWorker.csproj +++ b/src/DfE.CheckPerformanceData.RulesEngineWorker/DfE.CheckPerformanceData.RulesEngineWorker.csproj @@ -17,6 +17,10 @@ + + + + diff --git a/src/DfE.CheckPerformanceData.RulesEngineWorker/Program.cs b/src/DfE.CheckPerformanceData.RulesEngineWorker/Program.cs index 5af810b09..75aa65c04 100644 --- a/src/DfE.CheckPerformanceData.RulesEngineWorker/Program.cs +++ b/src/DfE.CheckPerformanceData.RulesEngineWorker/Program.cs @@ -17,81 +17,102 @@ using DfE.CheckPerformanceData.RulesEngineWorker.Zendesk; using Microsoft.AspNetCore.Builder; using Microsoft.EntityFrameworkCore; +using Serilog; +using Serilog.Formatting.Compact; -var builder = WebApplication.CreateBuilder(args); -builder.Configuration.AddUserSecrets(); +// Bootstrap logger so a failure before the host is built is still one JSON entry in Logit. +Log.Logger = new LoggerConfiguration() + .WriteTo.Console(new CompactJsonFormatter()) + .CreateBootstrapLogger(); -builder.Services.Configure(builder.Configuration.GetSection("QueueOptions")); -builder.Services.AddScoped(); -builder.Services.AddScoped(); +try +{ + Log.Information("Starting worker"); -builder.Services.AddScoped(); -builder.Services.AddScoped(); + var builder = WebApplication.CreateBuilder(args); + builder.Configuration.AddUserSecrets(); + builder.UseCpdSerilog(); -builder.Services.AddScoped(); -// Search analytics retention: the events sink + the messages service are the two purge -// dependencies of SearchAnalyticsRetentionJob below. Registered sibling to IMetricsSink -// rather than through AddPersistenceDependencies — the worker deliberately opts out of -// the shared registration bundle so its manual DbContext registration (lines below) is -// the single source of truth. -builder.Services.AddScoped(); -builder.Services.AddScoped(); + builder.Services.Configure(builder.Configuration.GetSection("QueueOptions")); + builder.Services.AddScoped(); + builder.Services.AddScoped(); -builder.Services.AddSingleton(); -builder.Services.AddDbContext(options => - options.UseNpgsql(builder.Configuration.GetConnectionString("Postgres"), - sql => sql.EnableRetryOnFailure())); -builder.Services.AddScoped(sp => sp.GetRequiredService()); + builder.Services.AddScoped(); + builder.Services.AddScoped(); -// Whether the real Zendesk client is the one that will actually serve requests decides how -// strict its configuration has to be. With the fake selected — the default, so that a fresh dev -// or test environment never pushes to real Zendesk — blank settings are expected rather than an -// error, and demanding them would stop the worker starting in exactly the environment the fake -// exists to serve. The consumers and the retention jobs share this process, so that is all of -// them, over an integration none of them is using. -builder.Services.AddZendeskApiClient( - builder.Configuration, - requireRealClient: !ZendeskServiceRegistration.ShouldUseFake(builder.Configuration)); -builder.Services.AddInfrastructureDependencies(builder.Configuration); -builder.Services.AddNotifyService(builder.Configuration); + builder.Services.AddScoped(); + // Search analytics retention: the events sink + the messages service are the two purge + // dependencies of SearchAnalyticsRetentionJob below. Registered sibling to IMetricsSink + // rather than through AddPersistenceDependencies — the worker deliberately opts out of + // the shared registration bundle so its manual DbContext registration (lines below) is + // the single source of truth. + builder.Services.AddScoped(); + builder.Services.AddScoped(); -// When Zendesk:UseFake is set the real Zendesk service is replaced with a fake that captures -// "created" tickets into the shared dev outbox table, so the rules-engine pipeline can be -// driven and observed without a real Zendesk. The flag defaults to true so a fresh dev or test -// environment never pushes to real Zendesk; setting it to false routes to the real client. -// Gated on configuration rather than the environment name because the test site also runs as -// Development. -ZendeskServiceRegistration.ConfigureFakeZendesk(builder.Services, builder.Configuration); + builder.Services.AddSingleton(); + builder.Services.AddDbContext(options => + options.UseNpgsql(builder.Configuration.GetConnectionString("Postgres"), + sql => sql.EnableRetryOnFailure())); + builder.Services.AddScoped(sp => sp.GetRequiredService()); -// The worker only needs the rules-engine pieces from the Application layer, not the -// portal's full service graph (which depends on web-only collaborators). -builder.Services.AddSingleton(); -builder.Services.AddSingleton(); -builder.Services.AddSingleton(); + // Whether the real Zendesk client is the one that will actually serve requests decides how + // strict its configuration has to be. With the fake selected — the default, so that a fresh dev + // or test environment never pushes to real Zendesk — blank settings are expected rather than an + // error, and demanding them would stop the worker starting in exactly the environment the fake + // exists to serve. The consumers and the retention jobs share this process, so that is all of + // them, over an integration none of them is using. + builder.Services.AddZendeskApiClient( + builder.Configuration, + requireRealClient: !ZendeskServiceRegistration.ShouldUseFake(builder.Configuration)); + builder.Services.AddInfrastructureDependencies(builder.Configuration); + builder.Services.AddNotifyService(builder.Configuration); -builder.Services.AddRulesProvider(builder.Configuration); + // When Zendesk:UseFake is set the real Zendesk service is replaced with a fake that captures + // "created" tickets into the shared dev outbox table, so the rules-engine pipeline can be + // driven and observed without a real Zendesk. The flag defaults to true so a fresh dev or test + // environment never pushes to real Zendesk; setting it to false routes to the real client. + // Gated on configuration rather than the environment name because the test site also runs as + // Development. + ZendeskServiceRegistration.ConfigureFakeZendesk(builder.Services, builder.Configuration); -builder.Services.AddHostedService(); -builder.Services.AddHostedService(); -builder.Services.AddHostedService(sp => - new DlqRetentionJob( - sp.GetRequiredService(), - sp.GetRequiredService>())); -builder.Services.AddHostedService(sp => - new MetricsRetentionJob( - sp.GetRequiredService(), - sp.GetRequiredService>())); -builder.Services.AddHostedService(sp => - new SearchAnalyticsRetentionJob( - sp.GetRequiredService(), - sp.GetRequiredService>())); -builder.Services.AddHostedService(sp => - new ContentStagingSessionRetentionJob( - sp.GetRequiredService(), - sp.GetRequiredService>())); + // The worker only needs the rules-engine pieces from the Application layer, not the + // portal's full service graph (which depends on web-only collaborators). + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); -builder.Services.AddWorkerHealthChecks(); + builder.Services.AddRulesProvider(builder.Configuration); -var app = builder.Build(); -app.MapWorkerHealthChecks(); -app.Run(); + builder.Services.AddHostedService(); + builder.Services.AddHostedService(); + builder.Services.AddHostedService(sp => + new DlqRetentionJob( + sp.GetRequiredService(), + sp.GetRequiredService>())); + builder.Services.AddHostedService(sp => + new MetricsRetentionJob( + sp.GetRequiredService(), + sp.GetRequiredService>())); + builder.Services.AddHostedService(sp => + new SearchAnalyticsRetentionJob( + sp.GetRequiredService(), + sp.GetRequiredService>())); + builder.Services.AddHostedService(sp => + new ContentStagingSessionRetentionJob( + sp.GetRequiredService(), + sp.GetRequiredService>())); + + builder.Services.AddWorkerHealthChecks(); + + var app = builder.Build(); + app.MapWorkerHealthChecks(); + app.Run(); +} +catch (Exception e) +{ + Log.Fatal(e, "Worker terminated unexpectedly"); +} +finally +{ + Log.CloseAndFlush(); +} diff --git a/src/DfE.CheckPerformanceData.RulesEngineWorker/SerilogHostExtensions.cs b/src/DfE.CheckPerformanceData.RulesEngineWorker/SerilogHostExtensions.cs new file mode 100644 index 000000000..eb564221a --- /dev/null +++ b/src/DfE.CheckPerformanceData.RulesEngineWorker/SerilogHostExtensions.cs @@ -0,0 +1,40 @@ +using Microsoft.AspNetCore.Builder; +using Serilog; +using Serilog.Formatting.Compact; +using Serilog.Templates; +using Serilog.Templates.Themes; + +namespace DfE.CheckPerformanceData.RulesEngineWorker; + +/// +/// Gives the worker the same console output as the web app's UseCpdSerilog: one compact +/// JSON object per event outside Development, so Logit groups a multi-line message and its +/// exception into a single entry rather than one entry per line. Duplicated from the Web project +/// because the worker cannot reference it. +/// +public static class SerilogHostExtensions +{ + public static WebApplicationBuilder UseCpdSerilog(this WebApplicationBuilder builder) + { + builder.Host.UseSerilog((context, services, config) => + { + var isDevelopment = context.HostingEnvironment.IsDevelopment(); + + config + .ReadFrom.Configuration(context.Configuration) + .ReadFrom.Services(services) + .Enrich.FromLogContext() + .WriteTo.Console(isDevelopment + ? new ExpressionTemplate( + "[{@t:HH:mm:ss} {@l:u3}] {SourceContext}\n {@m}\n{@x}", + theme: TemplateTheme.Code) + : new CompactJsonFormatter()); + }); + + // Remove the default Console provider so the worker does not print every event a second + // time as a plain "info: Category[0]" line — the multi-line form Logit splits up. + builder.Logging.ClearProviders(); + + return builder; + } +} diff --git a/src/DfE.CheckPerformanceData.RulesEngineWorker/appsettings.json b/src/DfE.CheckPerformanceData.RulesEngineWorker/appsettings.json index 329e5b39d..6047f1fe8 100644 --- a/src/DfE.CheckPerformanceData.RulesEngineWorker/appsettings.json +++ b/src/DfE.CheckPerformanceData.RulesEngineWorker/appsettings.json @@ -1,8 +1,12 @@ { - "Logging": { - "LogLevel": { + "Serilog": { + "MinimumLevel": { "Default": "Information", - "Microsoft.Hosting.Lifetime": "Information" + "Override": { + "Microsoft": "Warning", + "Microsoft.Hosting.Lifetime": "Information", + "System": "Warning" + } } }, "PollySettings": { @@ -28,6 +32,5 @@ "TargetViewTitle": "School Checking Exercise", "GroupId": 0, "BrandId": 0 - } } diff --git a/tests/DfE.CheckPerformanceData.IntegrationTests/Queue/ZendeskConsumerIdempotencyTests.cs b/tests/DfE.CheckPerformanceData.IntegrationTests/Queue/ZendeskConsumerIdempotencyTests.cs index fea80b00f..1abc6c60f 100644 --- a/tests/DfE.CheckPerformanceData.IntegrationTests/Queue/ZendeskConsumerIdempotencyTests.cs +++ b/tests/DfE.CheckPerformanceData.IntegrationTests/Queue/ZendeskConsumerIdempotencyTests.cs @@ -111,7 +111,13 @@ async Task DeliverAsync() await consumer.ProcessMessageBodyAsync(Message, CancellationToken.None); } - await Task.WhenAll(DeliverAsync(), DeliverAsync()); + // The loser of the claim throws so the queue retries it later (by which time the winner's + // CrmId makes the retry a no-op) — it must never be acked as if handled. Either delivery + // may lose, so both are awaited individually and at most one may throw. + var outcomes = await Task.WhenAll( + Task.Run(async () => { try { await DeliverAsync(); return true; } catch (InvalidOperationException) { return false; } }), + Task.Run(async () => { try { await DeliverAsync(); return true; } catch (InvalidOperationException) { return false; } })); + Assert.True(outcomes.Count(won => won) >= 1); // Only one delivery may win the "ticket created" transition; the loser must skip the // Zendesk call entirely. Exactly one ticket is created for the reference. @@ -123,7 +129,112 @@ async Task DeliverAsync() Assert.Equal(WorkerStatus.ZendeskTicketCreated, saved.WorkerStatus); } - private async Task SeedChangeRequestAsync() + // The retry path. Attempt 1 claims the row (RulesProcessed -> ZendeskTicketCreating) and then + // Zendesk fails. The redelivery must re-claim and create the ticket. It did not: the claim + // insisted on RulesProcessed, so attempt 2 matched no row, returned as if handled, and the + // message was acked away — 120 preprod requests vanished this way with nothing dead-lettered. + [Fact] + public async Task RedeliveryAfterFailedCreate_ReclaimsAndCreatesTheTicket() + { + await ResetChangeRequestsAsync(); + await SeedChangeRequestAsync(); + + var zendesk = Substitute.For(); + var calls = 0; + zendesk.CreateTicketAsync(Arg.Any()) + .Returns(_ => ++calls == 1 + ? throw new ZendeskApiException("Failed to create ticket.") + : new CreateTicketResponseDto { Ticket = new TicketDto { Id = 7100 } }); + + await using (var ctx1 = _fixture.CreateContext()) + { + var consumer = new ZendeskConsumer(Substitute.For(), zendesk, ctx1); + await Assert.ThrowsAsync( + () => consumer.ProcessMessageBodyAsync(Message, CancellationToken.None)); + } + + await using (var ctx2 = _fixture.CreateContext()) + { + var consumer = new ZendeskConsumer(Substitute.For(), zendesk, ctx2); + await consumer.ProcessMessageBodyAsync(Message, CancellationToken.None); + } + + await zendesk.Received(2).CreateTicketAsync(Arg.Any()); + + await using var verify = _fixture.CreateContext(); + var saved = await verify.ChangeRequests.SingleAsync(r => r.ReferenceNumber == Reference); + Assert.Equal("7100", saved.CrmId); + Assert.Equal(WorkerStatus.ZendeskTicketCreated, saved.WorkerStatus); + } + + // A results enquiry never passes the rules engine, so its WorkerStatus is null by design. It + // must be ticketed, with DeriveDecision's Scrutiny fallback, rather than dropped by the claim. + [Fact] + public async Task EnquiryRowWithNoRulesDecision_IsStillTicketed() + { + await ResetChangeRequestsAsync(); + await SeedChangeRequestAsync(workerStatus: null, requestType: RequestType.ResultsEnquiry); + + var zendesk = Substitute.For(); + zendesk.CreateTicketAsync(Arg.Any()) + .Returns(new CreateTicketResponseDto { Ticket = new TicketDto { Id = 7200 } }); + + await using (var ctx = _fixture.CreateContext()) + { + var consumer = new ZendeskConsumer(Substitute.For(), zendesk, ctx); + await consumer.ProcessMessageBodyAsync(Message, CancellationToken.None); + } + + await zendesk.Received(1).CreateTicketAsync(Arg.Any()); + + await using var verify = _fixture.CreateContext(); + var saved = await verify.ChangeRequests.SingleAsync(r => r.ReferenceNumber == Reference); + Assert.Equal("7200", saved.CrmId); + Assert.Equal(WorkerStatus.ZendeskTicketCreated, saved.WorkerStatus); + } + + // An AMENDMENT with no rules decision is not claimable (SC-005): its decision was never + // recorded, so it must not be ticketed under a guess. But it must not be acked away either — + // throwing sends it to the DLQ with a reason an admin can act on. + [Fact] + public async Task AmendmentRowWithNoRulesDecision_ThrowsSoTheMessageIsRetriedNotAcked() + { + await ResetChangeRequestsAsync(); + await SeedChangeRequestAsync(workerStatus: null, requestType: RequestType.Amendment); + + var zendesk = Substitute.For(); + + await using var ctx = _fixture.CreateContext(); + var consumer = new ZendeskConsumer(Substitute.For(), zendesk, ctx); + + await Assert.ThrowsAsync( + () => consumer.ProcessMessageBodyAsync(Message, CancellationToken.None)); + await zendesk.DidNotReceive().CreateTicketAsync(Arg.Any()); + } + + // A row stuck in ZendeskTicketCreating with no ticket id is an earlier attempt that died + // mid-call (or another worker still in flight). Returning quietly acks the message and + // loses the request; throwing makes the queue retry and, if the row stays stuck, dead-letter + // it with a reason an admin can see. + [Fact] + public async Task RowStuckInCreating_ThrowsSoTheMessageIsRetriedNotAcked() + { + await ResetChangeRequestsAsync(); + await SeedChangeRequestAsync(workerStatus: WorkerStatus.ZendeskTicketCreating); + + var zendesk = Substitute.For(); + + await using var ctx = _fixture.CreateContext(); + var consumer = new ZendeskConsumer(Substitute.For(), zendesk, ctx); + + await Assert.ThrowsAsync( + () => consumer.ProcessMessageBodyAsync(Message, CancellationToken.None)); + await zendesk.DidNotReceive().CreateTicketAsync(Arg.Any()); + } + + private async Task SeedChangeRequestAsync( + WorkerStatus? workerStatus = WorkerStatus.RulesProcessed, + RequestType requestType = RequestType.Amendment) { await using var ctx = _fixture.CreateContext(); var window = new CheckingWindow @@ -145,10 +256,10 @@ private async Task SeedChangeRequestAsync() Submitted = DateTime.SpecifyKind(DateTime.UtcNow, DateTimeKind.Unspecified), SubmittedById = Guid.NewGuid(), SubmittedByName = "Test User", - WorkerStatus = WorkerStatus.RulesProcessed, + WorkerStatus = workerStatus, Status = RequestStatus.SubmittedCommitted, ReferenceNumber = Reference, - RequestType = RequestType.Amendment, + RequestType = requestType, RequestTypeDescription = "Not on roll", Outcome = DecisionStatus.Scrutiny, }); diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerEnquiryMessageTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerEnquiryMessageTests.cs index 30484228b..692c62058 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerEnquiryMessageTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerEnquiryMessageTests.cs @@ -66,11 +66,13 @@ await harness.Consumer.ProcessMessageBodyAsync( public async Task Non_enquiry_row_with_null_WorkerStatus_is_not_claimed() { // SC-005: the widened claim must not accidentally claim an amendment with null status. + // Not claimed — but not quietly acked away either: the consumer throws so the queue + // retries and, if the row stays unclaimable, dead-letters it with a reason. var harness = new ConsumerHarness([NewAmendmentRow(Reference)]); harness.StubTicketCreation(); - await harness.Consumer.ProcessMessageBodyAsync( - Serialize(NewEnquiryMessage(Reference)), CancellationToken.None); + await Assert.ThrowsAsync(() => harness.Consumer.ProcessMessageBodyAsync( + Serialize(NewEnquiryMessage(Reference)), CancellationToken.None)); await harness.Zendesk.DidNotReceive().CreateTicketAsync(Arg.Any()); } @@ -246,8 +248,10 @@ public TResult ExecuteAsync(Expression expression, CancellationToken ca if (TryExecuteUpdate(expression, out var affected)) return (TResult)(object)Task.FromResult(affected); - // FirstOrDefaultAsync: returns the first match or null from the in-memory list. - if (expression is MethodCallExpression { Method.Name: "FirstOrDefaultAsync" } call + // FirstOrDefaultAsync: returns the first match or null from the in-memory list. EF + // builds the expression over the synchronous QueryableMethods, so the method is named + // "FirstOrDefault" here, not "FirstOrDefaultAsync". + if (expression is MethodCallExpression { Method.Name: "FirstOrDefault" or "FirstOrDefaultAsync" } call && call.Arguments.Count > 1 && TryGetPredicate(call.Arguments[1], out var firstPredicate)) { diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerTicketCompositionTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerTicketCompositionTests.cs index 986f6cc4e..46c10ed1e 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerTicketCompositionTests.cs +++ b/tests/DfE.CheckPerformanceData.UnitTests/Worker/ZendeskConsumerTicketCompositionTests.cs @@ -145,7 +145,7 @@ public void PupilFields_AreMappedFromConfiguredFieldIds() var ticket = consumer.BuildTicket(NewMessage("Other"), decision).Ticket; Assert.Contains(ticket.CustomFields!, f => f.Id == 10 && (string)f.Value! == "123456"); - Assert.Contains(ticket.CustomFields!, f => f.Id == 11 && (string)f.Value! == "c1"); + Assert.Contains(ticket.CustomFields!, f => f.Id == 11 && (string)f.Value! == "1001"); // UPN comes from the pupil's Upn (upper-cased), not the internal pupil Id. Assert.Contains(ticket.CustomFields!, f => f.Id == 12 && (string)f.Value! == "UPN1"); // Surname/forename are upper-cased before mapping. @@ -155,6 +155,35 @@ public void PupilFields_AreMappedFromConfiguredFieldIds() Assert.Contains(ticket.CustomFields!, f => f.Id == 15 && (string)f.Value! == "2010-01-01"); } + // The Zendesk CYPMD_ID field is an integer field: Zendesk rejects the WHOLE ticket (422 + // "CYPMD ID: is invalid") when the value is not an integer. A pupil with a non-numeric id + // (the UAT seed used "CY0045") must therefore lose that one field, not the ticket. + [Fact] + public void CypmdField_IsOmitted_WhenTheIdIsNotAnInteger() + { + _ticketFieldService.GetFieldIdFromConfig(ZendeskTicketFieldConstants.CypmdName).Returns(11L); + + var consumer = NewConsumer(); + var decision = new Decision(DecisionStatus.Scrutiny, "Other", "OTH-DEF", Array.Empty()); + + var ticket = consumer.BuildTicket(NewMessage("Other", "KS4", cypmdId: "CY0045"), decision).Ticket; + + Assert.DoesNotContain(ticket.CustomFields!, f => f.Id == 11); + } + + [Fact] + public void CypmdField_IsOmitted_WhenTheIdIsEmpty() + { + _ticketFieldService.GetFieldIdFromConfig(ZendeskTicketFieldConstants.CypmdName).Returns(11L); + + var consumer = NewConsumer(); + var decision = new Decision(DecisionStatus.Scrutiny, "Other", "OTH-DEF", Array.Empty()); + + var ticket = consumer.BuildTicket(NewMessage("Other", "KS4", cypmdId: ""), decision).Ticket; + + Assert.DoesNotContain(ticket.CustomFields!, f => f.Id == 11); + } + [Fact] public void UpnField_IsUpperCased() { @@ -507,9 +536,9 @@ public void DecisionReasonRejected_IsOmitted_WhenAutoApproved() // --- helpers --- private static RequestDocument NewMessage(string whatToChange, params AnswerRecord[] answers) => - NewMessage(whatToChange, "KS4", laestab: null, matchRef: 0, entryDate: null, upn: "UPN1", answers); + NewMessage(whatToChange, "KS4", laestab: null, matchRef: 0, entryDate: null, upn: "UPN1", cypmdId: "1001", answers); - private static RequestDocument NewMessage(string whatToChange, string windowType, string? laestab = null, int matchRef = 0, string? entryDate = null, string? upn = "UPN1", params AnswerRecord[] answers) => new() + private static RequestDocument NewMessage(string whatToChange, string windowType, string? laestab = null, int matchRef = 0, string? entryDate = null, string? upn = "UPN1", string cypmdId = "1001", params AnswerRecord[] answers) => new() { ReferenceNumber = "REF", CheckingWindowId = Guid.NewGuid(), @@ -521,7 +550,7 @@ private static RequestDocument NewMessage(string whatToChange, params AnswerReco School = new SchoolDetails { Urn = "123456", Name = "Test School", Laestab = laestab ?? string.Empty }, Pupil = new PupilDetails { - Id = "p1", CypmdId = "c1", Firstname = "Bob", Surname = "Smith", + Id = "p1", CypmdId = cypmdId, Firstname = "Bob", Surname = "Smith", DateOfBirth = "01/01/2010", Sex = "M", Age = 14, Upn = upn, MatchRef = matchRef, EntryDate = entryDate ?? string.Empty, },