fix(plantext): parse through the shared base parser - #1724
Merged
Merged
Conversation
Deletes PlanTextHelper and both dead BreakTimeCalculator tables in favour of PlanTextParser / PlanRegistrationPlanText in the base, which the background service now shares. One of the dead tables already held the corrected break values up to "4.75". The fix had been written against a copy nobody called, which is how the live one-hour ceiling survived review: a break of 1.5 became zero and PlanHours was inflated by the whole break. ContentHandoverService loses ~175 lines of private parsing and formatting that disagreed with the live parser on the same input -- "0.1" was five minutes in one and six in the other -- and whose own comments described a ceiling in a table that was never reached. Its three PlanText helpers are internal rather than private so the tests can drive them directly through InternalsVisibleTo, as ComputeShiftPauseSecondsTests already does, rather than by reflection. The handover fallback segment now renders times the same way the grid does. It emitted a zero-padded "08:00" where everything else wrote "8:00", so handover text and grid text disagreed; they now agree, and a break off the five-minute grid round-trips instead of collapsing. Tests cover what the call sites assume rather than re-testing the base grammar: that text which is not a shift leaves the sheet's hours column alone, that shift text owns PlanHours, that a regenerated PlanText reads back as the same shift, and that handover moves a 90-minute break intact. Includes the design note the change was built from. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical handover compatibility and admin-save flag issues remain unresolved, along with a moderate zero-break formatting inconsistency.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
This PR centralizes PlanText parsing through the shared base parser, fixes extended-break handling, removes duplicate logic, and adds integration coverage.
Changes:
- Updates the shared parser dependency and call sites.
- Simplifies handover and helper parsing.
- Replaces obsolete tests and registers new tests in CI.
- Documents parser behavior and rollout considerations.
| File | Summary |
|---|---|
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/TimePlanning.Pn.csproj |
Updates the shared parser dependency. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningPlanningService/TimePlanningPlanningService.cs |
Uses shared PlanText generation. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/ContentHandoverService/ContentHandoverService.cs |
Uses shared parsing and formatting for handover operations. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanTextHelper.cs |
Removes the obsolete local parser. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/PlanRegistrationHelper.cs |
Routes parsing through the shared implementation. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Infrastructure/Helpers/GoogleSheetHelper.cs |
Uses shared parsing during sheet imports. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanTextIntegrationTests.cs |
Adds parser integration and handover coverage. |
eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/PlanTextHelperTests.cs |
Removes obsolete helper tests. |
docs/superpowers/specs/2026-09-22-plantext-parsing-design.md |
Documents parser behavior and migration scope. |
.github/workflows/dotnet-core-pr.yml |
Registers the replacement test fixture. |
.github/workflows/dotnet-core-master.yml |
Registers the replacement test fixture. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1516
to
+1517
| && PlanTextParser.TryParseSegment(seg, out var shift) | ||
| && shift.StartMinutes == targetStart && shift.EndMinutes == targetEnd) |
| planning.PlannedBreakOfShift5 = model.PlannedBreakOfShift5; | ||
| planning.PlannedEndOfShift5 = model.PlannedEndOfShift5; | ||
| planning.PlanText = PlanTextHelper.GeneratePlanText(planning); | ||
| planning.PlanText = PlanRegistrationPlanText.Generate(planning); |
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.
Problem
PlanTextHelper.BreakTimeCalculatorwas a 25-entry string whitelist whose largest key was"1". Everything longer hit_ => 0silently, andRecalculatePlanHoursthen deducted nothing — inflatingPlanHoursby the full break.Four copies of that table existed. Two were dead — and one of the dead ones already held the corrected values up to
"4.75". The fix had been written against a copy nobody called. The comments inContentHandoverServicepointed readers at that dead method, which is plausibly how the live ceiling survived review.A sweep of all 227 tenant schemas found ~3,900 live rows losing their break outright:
1.5/1,5(2,436),1.0/1,0(642),0,50(372), plain2–7, and prose-suffixed tokens like2 helligdagand½ + AT.What changes here
PlanTextHelperand both dead tables are deleted; everything routes throughPlanTextParser/PlanRegistrationPlanTextin the base, which the background service now shares (service PR).ContentHandoverServiceloses ~175 lines of private parsing that disagreed with the live parser on the same input —"0.1"was 5 minutes in one and 6 in the other,"1.5"was 0 versus 90 — and whoseFormatBreakAsCanonicalHoursdeliberately emitted values it knew the live parser would drop. Its three PlanText helpers are nowinternalrather thanprivate, so tests drive them directly via the existingInternalsVisibleTo(asComputeShiftPauseSecondsTestsalready does) instead of by reflection.The handover fallback segment now renders times as
8:00rather than zero-padded08:00. That's a fix, not a regression:MinutesToTimeStringalready wrote8:00, so the fallback was the sole outlier and handover text disagreed with grid text. They now agree.Review caught a data-loss bug in the base, fixed upstream
The first version of
ParseIntoassignedPlanHourswheneverPlanTextwas non-empty. ButGoogleSheetHelperassignsPlanHoursfrom the sheet's hours column immediately before parsing, so a row readingFerie,Helligdag, a bare8or7,4had its real hours overwritten with zero — which then seedsSumFlexEndand drifts every following day. The read path runs on every period load for a sheet site, so it wasn't import-only. Fixed in #906:PlanHoursis derived only when the text actually described a shift.Two things NOT fixed here, flagged deliberately
Legacy single-digit-fraction rows have stale columns. The old parser read
8.5as 08:05; the new one reads 08:30. Rows already storePlannedStartOfShift1 = 485whilePlanTextstill says8.5-16. Handover matches segments by start/end, so those won't match, and the sender's columns get cleared while itsPlanTextkeeps the segment — the shift can reappear on the sender and exist on the receiver simultaneously. Wants either a re-derive pass before deploy or a start-only fallback match.PlanChangedByAdminmay latch on the first admin save. Detection isentry.State == EntityState.Modified, evaluated afterPlanTextis regenerated. Rows whose break is outside the old 5–60 grid now render differently, so even a no-op save marks the entity modified and permanently severs that day from sheet sync. Moving the snapshot would change admin-change detection semantics — andCalculatePauseAutoBreakCalculationActiveruns between the two points — so I have not rewritten that inside a parsing PR.Tests
PlanTextHelperTestsreplaced byPlanTextIntegrationTests, registered in the shard filter in both workflow files (this repo filters by class name; an unregistered class silently never runs —ShardCoverageTestswill confirm).It deliberately does not re-test the base grammar, which has ~130 cases of its own. It covers what the call sites assume: non-shift text leaving the hours column alone, shift text owning
PlanHours, a regeneratedPlanTextreading back as the same shift, and handover moving a 90-minute break intact.🤖 Generated with Claude Code