fix(lock): use UTC for the freeze, and stop three fail-open paths and two silent skips

[renemadsen]

Five correctness fixes found by a post-merge audit of the "Afstemt" reconciled-day-lock feature, plus the UI wiring needed to make one of them actually reach a user.

1. The freeze compares against UTC

DayLockHelper.CanReconcile used DateTime.Now. The comment justified it as "PlanRegistration.Date is a local midnight" — that premise is wrong. PlanRegistration.Date is a calendar-day label with the time zeroed, not an instant in any timezone, so there is no local midnight for a local clock to be consistent with. Writers do not even agree on how they build the label (TimePlanningFlexService derives one from DateTime.Now), which is a reason to pin this comparison to one clock rather than follow theirs.

Now DateTime.UtcNow. The rewritten comment also states the direction honestly rather than as a law: at a positive offset (CET/CEST, where this product runs) UTC is the conservative choice for a freeze; at a negative offset the same expression would be the permissive one and would break I2. That case does not arise here, and the answer if it ever does is an explicit business timezone.

CanReconcile is plugin-only — the service repo's twin DayLockHelper omits it and has no DateTime.Now — so this creates no twin drift. No blanket sweep: the other DateTime.Now uses in this repo are unrelated to the lock and were left, including ReconciledAt, which is a display instant rather than a comparison input.

2. Three fail-open null branches

A rule whose entire purpose is "this day cannot be written" must not waive itself when it cannot resolve the site.

• RefuseIfNotWritableAsync (renamed from CheckDayLockAsync, because it now refuses on two grounds and only one of them is a day lock — and to disambiguate it from the same-named method in TimePlanningPlanningService): a null site id returned null, which is this method's "proceed" answer, so a missing site id permitted the write. It now refuses with a localized message. The kiosk overload has no null guard of its own and dereferences the same id with ! a few hundred lines later, so a null used to sail past the lock and then throw a raw InvalidOperationException.
• Import: a sheet whose site has no MicrotingUid produced a null boundary, and a null boundary makes IsLocked false for every row — so an unresolvable site imported a whole sheet with no lock check at all. It is now skipped, counted and reported.
• GoogleSheetHelper: the null check on the lock line was provably dead (sitesByKey filters MicrotingUid != null and workers is built only from it), but had it ever fired it would have been the one line in the loop that degrades silently while its neighbours throw. It now matches them.

3. Swallowed exception detail restored

PlanRegistrationHelper had //SentrySdk.CaptureException(e); commented out at two sites. Those try blocks contain planRegistration.Update calls, so DbUpdateException, DbUpdateConcurrencyException and MySqlException were all reported to Sentry as the string "Could not parse PlanText" while the actual type and stack went to LogTrace. Both sites now capture the exception itself, once per channel, and the first logs at Error with a structured message instead of guessing.

4. The xlsx import no longer reports plain success while dropping rows

Locked days were skipped silently and the count went only to Console.WriteLine. Someone importing a corrected timesheet over a reconciled month saw "success", saw the old numbers, and re-imported forever.

The result now names each skip with its reason, the way ReconcileThrough already distinguishes its own. Two counters, composed as whole sentences so they join in any language without having to agree grammatically. The counts are also logged server-side in a finally, so they survive the exception path and the malformed-file early returns — that is the channel ops actually has, and it replaces the Console.WriteLine this change removes. An unresolvable sheet additionally raises a Sentry warning, matching what GoogleSheetHelper already does for its sheet problems.

The UI had to be wired for any of this to be visible: onSuccessItem ignored the response body entirely and toasted a hardcoded string. It now renders the server's message, falling back to the existing translated string.

5. UpdateAssignedSite — verified, documented, no guard added

Flagged by the audit as an unguarded write path. It is not one. It only reads, creates and writes rows with Date >= today (UTC midnight), while a locked day is always strictly in the past: Reconciled has exactly one writer, and the two callers that set it true both gate on CanReconcile. The ranges are disjoint. Comment only, so the next auditor does not re-raise it — and note that fix 1 is what makes it provable, since under DateTime.Now a server at a positive offset late in the UTC day could accept a day equal to UtcNow.Date.

Testing

C# — new and updated cases in three fixtures that are already in the shard filters, so no workflow change was needed:

• ReconcileServiceTests: new UpdateWorkingHour_Kiosk_WithNoSiteId_IsRefused_AndWritesNothing (fix 2a). Pre-fix this threw a raw InvalidOperationException.
• WorkingHoursImportRemovedRowTests: new Import_ASheetWhoseWorkerHasNoMicrotingUid_IsSkippedAndReported (fixes 2b and 4), plus assertions on the reported skip count and on the zero-skip path.
• DayLockHelperTests and two ReconcileServiceTests cases updated from Now to UtcNow. These are corrections, not weakened assertions: left on DateTime.Now, the boundary cases fail on a dev machine at either a positive or a negative offset.

TypeScript — never compiled locally. This repo carries only the plugin module, with no node_modules and no host app, so tsc/ng build cannot run here. CI is the first compile of the upload-modal change.

No e2e coverage of the upload flow exists at all — not before this change and not after. The new failure branch is therefore untested end to end. Flagging rather than implying otherwise.

Behaviour notes

• The UTC change is behaviour-neutral in production. No deployment sets TZ — this repo's Dockerfile sets none and installs no tzdata, and the mcr.microsoft.com/dotnet/aspnet base image defaults to UTC. It only makes dev machines behave like the container.
• The SiteNotFound resx entry has a blast radius beyond this feature. The key was already in use but had no resource entry, so it rendered as the raw string SiteNotFound. Adding it changes user-visible text at seven pre-existing call sites (TimePlanningWorkingHoursService.cs:1231, 1440; TimePlanningPlanningService.cs:507, 519, 530, 1209, 1221). "Medarbejder ikke fundet." will now appear in flows unrelated to the lock. An improvement, but nobody should be surprised by it.
• The second Sentry site is a control-flow change, not just a logging one: catch (Exception) became catch (Exception e) when (e is not DayLockedException). The net outcome is unchanged — the rejected entry stays tracked either way, so the sole caller's own Update at TimePlanningWorkingHoursService.cs:720 rethrows whether or not this catch swallowed it. Without the filter, uncommenting the capture would have started reporting routine lock refusals to Sentry as incidents.
• The import's failure path used to show a green success toast. The endpoint returns OperationResult with HTTP 200 regardless of its Success flag, so ng2-file-upload routes server-reported failures to onSuccessItem. The modal now branches on the body rather than the transport.

Follow-ups recorded and deliberately not fixed here

• GoogleSheetHelper.cs:288-292 still has a null-tolerant MicrotingUid check whose comment says such a site "cannot be resolved; skip it here" — the same pattern the new comment at :359-364 argues against. Not a bug (it guards the one-minute timeline dictionary, not the lock), but the file now reasons in opposite directions about the same property.
• unresolvableSheetsSkipped counts sheets skipped, not rows lost, so it overstates the loss for a wholly empty tab named after such a worker.
• The import endpoint returning OperationDataResult with a typed model — the way ReconcileThrough does — would let the UI choose toast severity properly. Today the success toast stays green even when it carries skips, because the only signal the component receives is a localized string and sniffing it for skip text would break on the first copy edit or non-Danish locale.
• E2E coverage for the upload flow, including the failure branch.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

There is at least one correctness issue in the updated code path (UTC midnight derivation built from multiple UtcNow reads) that can yield an incorrect day boundary at runtime.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens the “Afstemt” (reconciled-day) lock and import pipeline by making the lock’s “freeze” comparison UTC-based, eliminating several fail-open/null-path behaviors, restoring Sentry exception detail, and surfacing partial-import outcomes to users via the upload modal.

Changes:

• Switch lock eligibility (CanReconcile) to DateTime.UtcNow and align/clarify related lock logic and comments.
• Prevent fail-open writes/imports when a site/worker cannot be resolved; report skipped sheets/rows and log counters reliably.
• Improve observability (capture real exceptions to Sentry) and update the Angular upload modal to respect OperationResult.success and display server messages.

File summaries

File	Description
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs	Renames and hardens write-guard; import now counts/reports skips, logs summary in finally, and captures Sentry warnings.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningSettingService/TimeSettingService.cs	Refactors UpdateAssignedSite planning-fill logic and documents why it doesn’t need a day-lock guard.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningPlanningService/TimePlanningPlanningService.cs	Clarifies ReconciledAt as display/audit time and intentionally leaves it as DateTime.Now.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.resx	Adds new localized strings for site-not-found and import summaries (default).
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.Designer.cs	Regenerates strongly-typed resource accessors for new translation keys.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.da.resx	Adds Danish translations for the new keys.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanRegistrationHelper.cs	Restores Sentry exception capture and improves error logging semantics while filtering out DayLockedException.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/GoogleSheetHelper.cs	Removes dead null-tolerant lock-check and makes the lock check strict.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/DayLockHelper.cs	Updates CanReconcile to use UTC and expands rationale in comments.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursImportRemovedRowTests.cs	Adds assertions for “no skips” message and new coverage for unresolvable-sheet skip/report behavior.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs	Aligns date-boundary tests with UTC behavior and adds kiosk null-site refusal coverage.
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DayLockHelperTests.cs	Updates reconciliation boundary tests to use UTC consistently.
eform-client/src/app/plugins/modules/time-planning-pn/modules/working-hours/components/working-hours-actions/working-hours-upload-modal/working-hours-upload-modal.component.ts	Parses OperationResult from HTTP 200 responses; shows server message and treats success=false as an error.

Review details

Files not reviewed (1)

• eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.Designer.cs: Generated file

• Files reviewed: 12/13 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.