fix(praktikant): only split days whose tiers encode a normal-time boundary - #1678
Merged
renemadsen merged 1 commit intoAug 7, 2026
Merged
Conversation
…ndary The normal-time/overtime split was gated on the preset NAME alone. Preset definitions are copy-at-create-time snapshots, so a customer who created the praktikant rule set before the tier correction still holds the OLD tiers in their database, and no migration updates them. The name still matched, so the split fired on stale data with the wrong boundary. Concretely, Staldarbejde SATURDAY used to be [21600 SAT_NORMAL, null SAT_ANIMAL_AFTERNOON]. Two tiers with a non-null first one satisfied the old gate, so the split ran with normalSeconds = 21600 and emitted the overflow using the old tier 2 pay code — a fixed kr/dag animal-care supplement. A Saturday worked 04:00-12:00 went from SAT_NORMAL 28800 to SAT_NORMAL 21600 + SAT_ANIMAL_AFTERNOON 7200: an afternoon supplement for work that ended at noon. A 00:00-12:00 Saturday flipped six full hours. That is the exact harm the identity gate was written to prevent. It checked the name, but the data behind the name can predate the correction. The split now additionally requires the day rule's tiers to match the shape it assumes: 26640 / 33840 OVERTIME_50 / null OVERTIME_80. Stale rows fall through to their historical bands-only treatment, unchanged. Tier 1's pay code is deliberately not constrained — it legitimately varies (SAT_NORMAL, ANIMAL_SUN_HOLIDAY, NORMAL). The same guard is applied to the Grundlovsdag path. A data migration rewriting the old rows is the durable fix; until then this keeps existing customers on the behaviour they have been paid against. 11 tests added, built from the pre-correction tier values as they exist in customer databases. No existing test changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in praktikant pay-line routing by preventing the normal-time/overtime split (and the Grundlovsdag noon split) from triggering on stale customer DB rows whose preset name matches, but whose tier encoding predates the corrected “normal-time boundary + overtime ladder” shape.
Changes:
- Adds a tier-shape predicate (
HasNormalTimeBoundaryShape) and uses it to gate the split logic so only corrected-tier rows are reinterpreted as having a normal-time boundary. - Applies the same shape guard to the Grundlovsdag special-case path to avoid misplacing the noon split on legacy tier ladders.
- Adds legacy fixtures plus targeted routing/unit tests to lock down the regression and ensure corrected presets still take the intended split paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs | Gates the normal-time split and Grundlovsdag logic on the new tier-shape predicate to avoid reinterpreting stale preset snapshots. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PayRuleSetLock.cs | Introduces HasNormalTimeBoundaryShape and related constants to detect the corrected praktikant tier encoding. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PraktikantPayLineRoutingTests.cs | Adds regression tests covering legacy tier snapshots plus unit tests for the shape predicate itself. |
| eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/Helpers/PraktikantFixtures.cs | Adds legacy-tier fixtures representing pre-correction customer DB rows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
renemadsen
deleted the
fix/praktikant-split-requires-corrected-tier-shape
branch
August 7, 2026 17:25
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.
Regression fix for #1676. That PR changed attribution for customers who already had the praktikant rule set — in the wrong direction.
What went wrong
Preset definitions are copy-at-create-time snapshots: choosing a preset writes its values into DB rows, and nothing re-reads the catalogue afterwards. There is no migration updating existing rows — I checked.
So a customer who created Staldarbejde before the tier correction still holds:
The split was gated on the preset name, which didn't change. Two tiers with a non-null first one satisfied the gate, so it fired with
normalSeconds = 21600instead of 26640, and emitted the overflow using the old tier 2 pay code — a fixed kr/dag animal-care supplement, neverOVERTIME_50/OVERTIME_80.An afternoon supplement for work that finished at noon. This is precisely the harm the identity gate exists to prevent — it checked the name, but the data behind the name can predate the correction. Every test in #1676 used freshly-built corrected fixtures, so none could see it.
Aggravating: the customer can't fix it themselves (the preset is locked against edit/delete and filtered out of the create dialog as already-existing), and the edit modal renders the catalogue, not the DB row — so an admin sees the corrected tiers while the engine runs the old ones.
The fix
The split now also requires the day rule's tiers to match the shape it assumes —
26640/33840 OVERTIME_50/null OVERTIME_80— applied both to the split branch and the Grundlovsdag path. Stale rows fall through to their historical bands-only treatment, unchanged.Tier 1's pay code is deliberately not constrained: it legitimately varies across day types (
SAT_NORMAL,ANIMAL_SUN_HOLIDAY,NORMAL). Verified that every corrected day rule reaching either gate still matches, so no legitimate current-data case is excluded.Tests
11 added, built from the pre-correction tier values as they exist in customer databases (
StaldarbejdeLegacyTiers()/AndetArbejdeLegacyTiers(), commented as such), plus 4 direct unit tests of the predicate. No existing test changed — which is the signal that this constrains behaviour rather than reinterpreting it.Still owed
A data migration rewriting the old rows to the corrected tiers is the durable fix; until it ships, existing customers keep the behaviour they have been paid against — including the Sunday/Holiday overtime gap #1676 set out to close, which single-tier stale rows never reached anyway.
🤖 Generated with Claude Code