feat(lock): reconciled ('Afstemt') day lock — backend - #1711
Merged
Merged
Conversation
Marking a day Afstemt freezes it and every earlier day for that worker, against web edits, mobile registrations and background recalculation. Earlier days lock without being marked Reconciled themselves, and a day can only be unlocked once every day after it has been unlocked -- the boundary moves, it never gets a hole punched in it. Three findings shaped the design: Reconciled/ReconciledAt already exist on PlanRegistrations and PlanRegistrationVersions (migration 20260127060748) and ship in the pinned Microting.TimePlanningBase 10.0.62. Nothing writes them. So this needs no base-repo change, no migration and no NuGet bump. The lock is derived from MAX(Date) WHERE Reconciled, not stored. That makes "earlier days are locked but not marked" and "unlock only in reverse order" properties of the model rather than rules something has to enforce and keep in sync. There is no choke point: 32 write sites across 11 files in 2 repos, and PnBase.Create/Update/Delete live in the base package. But every write goes through EF change tracking, so a SaveChanges interceptor sees all of them. Call-site guards alone would repeat the MaxDaysEditable mistake, which is enforced only on the read path and in the browser and is bypassable by a crafted POST today. Barring reconcile at or after today is load-bearing beyond the obvious: it is what makes the forward flex cascades -- one running 180 days ahead, one unbounded -- provably unable to reach a locked day. The spec records that so nobody relaxes it without revisiting them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bulk reconcile needs three selection modes -- picked rows, a picked day column, or everything visible in the chosen period. They collapse into one operation with two axes, a target date and a set of workers, rather than three separate features: the date comes from a column header or the picker, and the worker set comes from row selection, defaulting to every visible row. Recorded as a table mapping each requested mode onto that one mechanism. Two consequences worth writing down. The affected cells are previewed in place before committing, so the cascade is something you see rather than infer. And a row already reconciled past the target date is skipped, not moved backwards -- that would be an unlock, which is deliberately the heavier action; skipped rows are counted and reported instead of quietly dropped. Reconciled and TransferredToPayroll stay independent. They are adjacent columns from the same migration, which makes it easy to assume they are related; they are not, in either direction. Mobile rejects the write with the same localized failure as the web paths and renders no lock state. No mobile UI work is in scope. Open questions section now holds nothing blocking -- just the skipped-row reporting surface and whether ReconciledBy is wanted on the record, which would need the base migration this design otherwise avoids. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
15 tasks across three repos, phased so the backend is complete and enforceable at task 6. Revised after review, which found the first draft would not have survived execution. The defects worth recording: The interceptor opened a second DbContext to read the boundary, and built it with ServerVersion.AutoDetect -- which opens a connection and runs a version query. That guard fires on every save, including the four Update calls inside the per-day loop that runs on every dashboard load; 50 workers over a month would have meant thousands of extra connections per page view. It now queries the context it is already inside, which is safe because the boundary query projects into an anonymous type and tracks nothing. The same mistake in miniature: the rewritten context helper used AutoDetect where the factory it replaced hardcodes the version, adding a round-trip per worker per load. Hardcoded to match. Task 5 said to iterate a filtered collection while still projecting the unfiltered one. UpdatePlanRegistrationsInPeriod has a single 715-line loop that fuses writing and projection, so following that instruction would have dropped locked days out of the grid -- silently wrong rather than blocked, which is the worst failure mode for a plan. Now: guard the four Update calls, leave the loop alone. LockedThrough was set inside that loop, so a worker with no rows in the window never got one and a fully locked month would have rendered editable. A `using Pomelo...` line that does not compile -- this repo uses the Microting fork. Three tests that fail in arrange because the seed order put the row inside the range its own Create would be blocked by. A test that seeded WorkflowState = Removed, which PnBase.Create overwrites unconditionally. Three test helpers that are private or do not exist. Seven wrong line numbers. Also corrected: the rationale for guarding setDisabled claimed the cascade re-enables unconditionally. It does not -- every enable is value-guarded. The true reason is that none is guarded by isInTheFuture, which is a pre-existing bug the lock would inherit. An agent that checked the false premise would have skipped a load-bearing step. Spec UI requirements 8.1-8.4 (glyph, tooltip, legend, two-step confirm, bulk preview, typed unlock confirmation) have no tasks and are listed as an open decision rather than quietly dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Design and plan are complete and reviewed; no implementation code exists. Records where the branch is, the one decision still open (spec 8.1-8.4 UI requirements have no tasks -- particularly the typed unlock confirmation, which is the only safeguard on a feature any web user can trigger), what the design settled so it is not re-litigated, and the traps the plan review already caught so they are not reintroduced. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Renamed STATUS -> HANDOFF and folded in what a cold session needs that was previously only in chat: the dev-mode gate, the normal development cycle including the dual review gate, subagent-driven execution, the CI-only testing rule and the shard-filter trap, and a note that stable has moved and that the dependency-alignment map shares this branch by accident. One entry point instead of a status note plus a separate brief. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The lock is derived, never stored: a worker's boundary is MAX(Date) over their non-removed Reconciled PlanRegistration rows. Every day at or before that boundary is locked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every PlanRegistration write is now checked against the worker's reconciled boundary via a SaveChangesInterceptor, on both its original and its current (site, date) slot -- so a write cannot escape the lock by moving a locked row's date past the boundary or reassigning it to another worker. Two writes remain permitted inside the locked range: clearing the flag on the boundary day itself (unlock, which also requires ReconciledAt to be cleared), and a payroll-flag-only write, which spec §11.2 requires so exporting a reconciled period keeps working. Wired into the pooled DbContext registration, TimePlanningDbContextHelper, and both test context builders in TestBaseSetup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reconcile rejects today and later days (I2) and is idempotent; unlock is allowed only on the worker's boundary day; reconcile-through marks one landing day per worker and reports skips by reason; a concurrent boundary move degrades to a per-site skip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…en recalculating - web and mobile edits of a locked day return DayIsReconciled or DayIsLockedByReconciledDay; - the dashboard recompute reverts locked days before any save, so a closed month renders its stored values and never trips the lock; - gap-fill never creates rows inside the lock, and sends a non-persisted placeholder day instead, so the positional grid keeps one column per date; - the working-hours save skips locked rows (the page posts every row) and marks them read-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…th tasks The spec gains section 13, which lists what changed during execution: the payroll-flag exemption, the two-slot check, reverting locked days before any save, placeholder days for the positional grid, and the working-hours skip. Section 6.3 is corrected, and the section 8.3 grid gotcha now uses disableRowClickSelection. The plan gains Task 5B (remaining plugin write paths) and the reviewed UI tasks for section 8.1-8.4 (8A-14A), which settles the handoff's open decision. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… handover inside the lock - The startup pause-id repair skips locked rows, runs on an interceptor-guarded context, and a DayLockedException can no longer take the host down. - Sheet pull, import and the flex follow-up skip locked rows before tracking them. - Flex corrections, absence approval and handover accept are refused up front with a message that says what the day is. - CorruptedPauseIdRepairTests now runs in CI (it was in no shard). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each day carries Reconciled/ReconciledAt, and each row carries LockedThrough, set even when the worker has no rows in the window. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- a test proves both production context paths attach the lock; - unlock refusals name the day to free first; - create-time lock checks for absence and handover requests; - lock refusals are no longer mislabelled or swallowed; - self-contained comments, twin headers, and skip counts logged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Status line, pinned base version, and section 13 now say what is built: the plugin write paths are guarded, unlock refusals name the day, the race window is documented, and the frontend must not ship before the service repo's PR is deployed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 15, 2026
Merged
Reconcile, unlock and bulk reconcile now require the admin role, matching how payroll export and the pay-rule endpoints are gated. Reading a day, editing an open one and the lock's own display are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Supersedes 07b598e ("restrict reconcile and unlock to admins"), already pushed to this branch/PR #1711: the product decision changed again after that commit landed. It is not a role at all — only the first user (the account with the lowest AspNetUsers Id, per FirstUserHelper) may reconcile, unlock, or bulk-reconcile a day. The three controller actions now carry a bare [Authorize] (anonymous still refused by the pipeline); which signed-in caller may proceed is decided in TimePlanningPlanningService instead, since no [Authorize] role can express "the first user". Review follow-ups folded in: the first-user check now runs as the first statement inside each method's existing try (a DB round-trip above the try would have escaped the catch-all as a raw 500 with nothing in Sentry, instead of the OperationResult every other failure here produces); the refusal is extracted once into OnlyTheFirstUserCanReconcileOrUnlock(), mirroring the existing OnlyLatestReconciledDayCanBeUnlocked() precedent; the message key is renamed to match (it also covers unlock and bulk refusals, not just reconcile) with a trailing full stop in both languages, consistent with its resx neighbours; and the test suite gained the first-user behaviour tests (success, a different signed-in user refused with no write, and the userId-0-vs-empty-table edge case) alongside the restored open-day/no-role-restriction pins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The spec and plan still asserted "any web user may reconcile" / "no admin gate", the opposite of what commits 07b598e and 1eb5f77 actually built. Updates §8.6 Permissions (the reversal, why it is a service-layer check and not an [Authorize] role, that the UI hides rather than disables the three controls, and that lock display stays visible to everyone regardless), adds a §13 revision bullet recording when and why the decision reversed a second time and the release-order consequence for PR1/PR4, and points the plan's Global Constraints and Spec coverage lines at §8.6 instead of restating the mechanism. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
stable's #1712 rewrote the sheet pull's column mapping (header name instead of a fixed stride, so `columnSiteMap` keyed by column index became `workers`, a list of (WorkerColumns, Site) pairs) while this branch added the reconciled-day lock guard to the same row loop. The two are orthogonal: the mapping decides WHICH site a pair of columns belongs to, the guard then asks whether THAT site's day is frozen. Neither reads the other's state. Resolution in GoogleSheetHelper.PullEverythingFromGoogleSheet: - Kept stable's mapping verbatim. The resolved file is byte-identical to stable's apart from the three additive lock hunks. - `lockedThroughBySite` is still seeded from `oneMinuteTimelines.Keys`, which stable now builds from `workers` rather than `columnSiteMap.Values`. That set still covers every site the row loop can reach -- `workers` only holds sites with a non-null MicrotingUid, since `sitesByKey` filters them -- which is what the guard needs, because DayLockHelper.IsLocked reads a site missing from the map as having no boundary, i.e. open. Comment updated to say so. - The guard keeps its position: first thing after the test that decides this worker participates in this row (stable's short-row check, which replaced the old `columnSiteMap.TryGetValue` continue), and before the "Processing site" line, the PlanRegistration load and every write. A locked row is still never loaded, so it is never tracked and no later save can flush it. Both of stable's new write paths -- the create leg and the null-tolerant update leg -- sit below it, as does stable's own PlanChangedByAdmin skip, which is narrower (update leg only) and stays. - The two skip counters are folded into stable's single summary line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
ReconcileThrough can dereference a client-supplied null siteIds collection instead of validating the request safely.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Implements the backend for reconciled-day locking across web, mobile, recalculation, imports, and payroll workflows.
Changes:
- Adds derived lock evaluation and
SaveChangesInterceptorenforcement. - Adds reconcile, unlock, bulk-reconcile endpoints with first-user authorization.
- Updates read models, localization, safeguards, and CI tests.
File summaries
| File | Description |
|---|---|
TimePlanningWorkingHoursService.cs |
Applies lock state to working-hours reads and writes. |
TimePlanningPlanningService.cs |
Adds lock-aware planning operations and endpoints. |
ITimePlanningPlanningService.cs |
Exposes reconciliation operations. |
TimePlanningFlexService.cs |
Prevents flex writes to locked days. |
RebusService.cs |
Uses guarded database contexts. |
ContentHandoverService.cs |
Blocks handovers involving locked days. |
AbsenceRequestService.cs |
Blocks absence approval on locked days. |
Translations*.resx |
Adds English and Danish lock messages. |
Translations.Designer.cs |
Adds generated resource accessors. |
TimePlanningPlanningPrDayModel.cs |
Adds reconciliation metadata. |
TimePlanningPlanningModel.cs |
Adds the lock boundary. |
ReconcileThrough*.cs |
Defines bulk reconciliation request/results. |
ReconciledDayLockInterceptor.cs |
Enforces lock invariants on persistence. |
TimePlanningDbContextHelper.cs |
Builds contexts with the interceptor. |
PlanRegistrationHelper.cs |
Skips recalculation and projects lock state. |
GoogleSheetHelper.cs |
Skips locked spreadsheet rows. |
FirstUserHelper.cs |
Implements first-user authorization logic. |
DayLockHelper.cs |
Provides lock derivation and predicates. |
CorruptedPauseIdRepair.cs |
Skips locked rows during startup repair. |
EformTimePlanningPlugin.cs |
Wires interception and guarded repair contexts. |
TimePlanningPlanningController.cs |
Adds secured reconciliation routes. |
WorkingHoursImportRemovedRowTests.cs |
Tests locked-day import behavior. |
TimePlanningFlexServiceRemovedRowTests.cs |
Tests locked flex behavior. |
TestBaseSetup.cs |
Wires the interceptor into test contexts. |
ReconcileServiceTests.cs |
Tests reconciliation, permissions, reads, and writes. |
PayrollExportRemovedPlanRegistrationTests.cs |
Tests payroll exemption behavior. |
DayLockWiringTests.cs |
Verifies production context wiring. |
DayLockInterceptorTests.cs |
Tests persistence enforcement. |
DayLockHelperTests.cs |
Tests lock derivation and filtering. |
CorruptedPauseIdRepairTests.cs |
Tests repair skipping. |
ContentHandoverServiceTests.cs |
Tests handover lock rejection. |
AbsenceRequestServiceTests.cs |
Tests absence lock rejection. |
reconciled-day-lock-design.md |
Documents the design and invariants. |
2026-09-13-HANDOFF-reconciled-day-lock.md |
Records implementation handoff details. |
.github/workflows/dotnet-core-pr.yml |
Adds new tests to CI shards. |
.github/workflows/dotnet-core-master.yml |
Adds new tests to master CI shards. |
Review details
Files not reviewed (1)
- eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Resources/Translations.Designer.cs: Generated file
- Files reviewed: 37/39 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| OnlyTheFirstUserCanReconcileOrUnlock()); | ||
| } | ||
|
|
||
| if (model == null || model.SiteIds.Count == 0) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. It grows one task at a time; each push runs CI on the tests added so far.
Backend phase of the Reconciled ("Afstemt") day lock. Marking a worker's day Afstemt freezes that day, and every earlier day for that worker, against web edits, mobile registrations and background recalculation. A day can only be unlocked once every day after it is unlocked.
docs/superpowers/specs/2026-09-12-reconciled-day-lock-design.mddocs/superpowers/plans/2026-09-12-reconciled-day-lock.mddocs/superpowers/plans/2026-09-13-HANDOFF-reconciled-day-lock.mdNo base-repo change:
Reconciled/ReconciledAtalready exist, and ship inMicroting.TimePlanningBase10.0.63.Tasks in this PR
DayLockHelper): derived, never storedSaveChangesInterceptorenforcing the lock on every write path (+ payroll-flag exemption)The service repo, the host SCSS and the frontend follow as separate PRs.
Release order — read before merging the frontend
The three endpoints go live as soon as this merges, restricted to the first user (the account with the lowest
AspNetUsersId) — see the note below. Background jobs ineform-service-timeplanning-pluginenforce the lock only once that repo's PR is deployed. The frontend PR must not reach production before the service PR is deployed; otherwise jobs could still write days the web shows as closed. Merging this PR alone changes nothing visible: no day is reconciled until someone calls the endpoints.Permissions — changed late, after this branch was already green
Only the first user may reconcile, unlock or bulk-reconcile. This reverses the original "any web user may reconcile" decision at the user's request; spec §8.6 and §13 record the reversal rather than rewriting it.
No
[Authorize]role can express "first user", so the rule is a service-layer check inTimePlanningPlanningService, mirroring the host's own idiom (SitesService.Delete,DeviceUsersService,OnlyTheFirstUserCanDeleteWorkers). The predicate is the host's verbatim, including theuserId > 0half — without it an empty users table would make an unauthenticated caller the first user. The three actions keep a plain[Authorize]so anonymous callers are still refused by the pipeline.Review
Every task passed a spec+quality review and a code-simplifier pass before its commit, and a whole-branch review ran at the end (verdict: ready with fixes; the fixes are in
fix(lock): close the final-review gaps before merge). Rulings taken during implementation are in spec §13.🤖 Generated with Claude Code