Skip to content

fix(report): guard missing case, site worker and worker in GenerateReportV2 - #1079

Merged
renemadsen merged 1 commit into
stablefrom
fix/report-v2-null-case-and-worker-guards
Aug 13, 2026
Merged

renemadsen merged 1 commit into
stablefrom
fix/report-v2-null-case-and-worker-guards

Conversation

@renemadsen

Copy link
Copy Markdown
Member

Follow-up to #1078. Same method, same failure class, found during that investigation.

Problem

GenerateReportV2 resolved the reporting worker like this:

var dbCase = await sdkDbContext.Cases.FirstOrDefaultAsync(...);
var workerId = sdkDbContext.SiteWorkers.First(x => x.SiteId == dbCase.SiteId).WorkerId;
var worker = await sdkDbContext.Workers.FirstOrDefaultAsync(x => x.Id == workerId);

if (dbCase == null)          // never reached when dbCase is null
{
    logger.LogError($"Could not find case with id {planningCase.MicrotingSdkCaseId}");
    continue;
}
...
EmployeeNo = worker.EmployeeNo

Three defects. Each aborts the entire report, because the method wraps its whole body in a single try/catch:

  1. dbCase.SiteId is dereferenced one line above the dbCase == null guard, so a missing case throws NullReferenceException instead of reaching the intended continue. The guard is effectively dead code.
  2. SiteWorkers.First(...) throws Sequence contains no elements for a site with no worker row — the identical failure shape that broke reports for customer 879 via Properties.First(...) in fix(report): don't abort report when a planning's property is unresolvable #1078.
  3. worker came from FirstOrDefaultAsync and was dereferenced unguarded, so a dangling WorkerId throws NullReferenceException.

GenerateReport (V1) already gets the ordering right; V1 has no EmployeeNo, so the worker lookup is V2-only.

Fix

  • Move the dbCase guard above its first use, so the continue is actually reachable.
  • Resolve the site worker with FirstOrDefaultAsync and fall back to a blank EmployeeNo, matching how fix(report): don't abort report when a planning's property is unresolvable #1078 handles an unresolvable property name. EmployeeNo is display-only — WordService.cs:872 interpolates it into a <td> and ExcelService.cs:419 passes it to CreateCell(string), so "" is safe for both and the row still appears in the report.

Also folded in (behavior change — please sanity-check)

The SiteWorkers lookup had no WorkflowState filter and no ordering, so it could return a removed site-worker link, or a nondeterministic one when a site has several worker rows. This is pre-existing — the old .First(...) had the same predicate — but the line was already open and the plugin has a settled idiom for it (GrpcSiteResolver.cs:72-76, BackendConfigurationAssignmentWorkerServiceHelper.cs:925,942). Now filtered to non-removed and ordered by Id.

Consequence: if a site's only SiteWorker row is removed, EmployeeNo now renders blank where it previously showed that removed worker's number. That's the intended reading, but it is a visible change — happy to drop this hunk if you'd rather keep the fix strictly to the crash.

Also added a HashSet so "no active worker assigned to site N" warns once per site rather than once per planning case — otherwise one misconfigured site emits an identical line for every case in the range.

Verification

  • dotnet build BackendConfiguration.Pn.csproj → 0 errors, 92 warnings (byte-identical to the pre-change baseline)
  • Reviewed by subagent — no Critical issues, no regressions introduced. Confirmed the happy path is unchanged in output and query count, that the added await is safe (the enclosing foreach iterates a materialized List, and these queries hit sdkDbContext while the loop source came from itemsPlanningPnDbContext), and that nothing else in the file references the removed locals.
  • Checked against the restored customer 879 database: 0 sites without a worker, 0 orphan SiteWorkers, 0 report cases lacking a worker. So this is preventive for 879 — unlike fix(report): don't abort report when a planning's property is unresolvable #1078, it is not fixing an observed failure there.

Still open

  • The report service has no test coverage in either test project. Neither this nor fix(report): don't abort report when a planning's property is unresolvable #1078 is covered, and this method has now produced two bugs of the same shape.
  • Remaining same-class hazards in this method, not touched here: CheckListTranslations…First(x => x.LanguageId == language.Id), Languages.Single(...), FieldTranslations.FirstAsync, FieldOptionTranslations.FirstAsync, match.Name and clTranslation.Text after a FirstOrDefaultAsync. A sweep would be worth scheduling.

🤖 Generated with Claude Code

https://claude.ai/code/session_01A9DDHNWkLapbuXHjFSqFt9

…portV2

GenerateReportV2 resolved the reporting worker like this:

    var dbCase = await sdkDbContext.Cases.FirstOrDefaultAsync(...);
    var workerId = sdkDbContext.SiteWorkers.First(x => x.SiteId == dbCase.SiteId).WorkerId;
    var worker = await sdkDbContext.Workers.FirstOrDefaultAsync(x => x.Id == workerId);

    if (dbCase == null) { logger.LogError(...); continue; }

Three defects, each of which aborts the entire report because the method
wraps its whole body in a single try/catch:

- dbCase.SiteId is dereferenced before the dbCase null check, so a missing
  case throws NullReferenceException instead of reaching the continue.
- SiteWorkers.First(...) throws "Sequence contains no elements" for a site
  with no worker row -- the same failure shape that took down reports for
  customer 879 via Properties.First(...), fixed in #1078.
- worker came from FirstOrDefaultAsync and was dereferenced unguarded, so a
  dangling WorkerId throws NullReferenceException.

Move the dbCase guard above its first use, so the intended continue is
actually reachable. Resolve the site worker with FirstOrDefaultAsync and
fall back to a blank EmployeeNo, matching how #1078 handles an
unresolvable property name. EmployeeNo is display-only: WordService
interpolates it into a table cell and ExcelService passes it to
CreateCell, so "" is safe for both and the row still appears.

While the lookup was open, filter out removed SiteWorkers and order by Id.
The plugin already does this in GrpcSiteResolver and
BackendConfigurationAssignmentWorkerServiceHelper; without it the report
can show the EmployeeNo of a worker unassigned from that site months ago,
or a different one run-to-run when a site has several worker rows.

Warn once per site rather than once per planning case -- "site has no
active worker" is a per-site condition, so a single misconfigured site
would otherwise emit one identical line per case.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A9DDHNWkLapbuXHjFSqFt9
Copilot AI review requested due to automatic review settings August 3, 2026 14:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens GenerateReportV2 in the backend report service to avoid aborting an entire report when related SDK data (case/site-worker/worker) is missing or removed, aligning behavior with the earlier “degrade gracefully” fix pattern from #1078.

Changes:

  • Ensures the dbCase == null guard is reachable by removing dereferences before the null-check.
  • Replaces throwing site-worker/worker resolution with FirstOrDefaultAsync + safe fallback to blank EmployeeNo, and filters out removed site-worker links with deterministic ordering.
  • Deduplicates “missing worker for site” warnings to log once per site per report run.

@renemadsen
renemadsen merged commit 89db550 into stable Aug 13, 2026
30 checks passed
@renemadsen
renemadsen deleted the fix/report-v2-null-case-and-worker-guards branch August 13, 2026 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants