Skip to content

Fix rolling retrospective window lengths - #513

Merged
thomasluizon merged 4 commits into
mainfrom
fix/ticket-469-window
Sep 9, 2026
Merged

Fix rolling retrospective window lengths#513
thomasluizon merged 4 commits into
mainfrom
fix/ticket-469-window

Conversation

@thomasluizon

@thomasluizon thomasluizon commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Closes thomasluizon/orbit-tickets#469

Summary

Correct the inclusive start offsets so month, quarter, semester, and year span exactly 30, 90, 180, and 365 dates ending today. Add date count coverage at the resolver boundary and update direct month consumers to assert the same inclusive contract. The week calculation remains unchanged.

Blast radius

Every production call to RetrospectivePeriodRange.Resolve was checked:

  • HabitsController retrospective responses for rolling periods use one fewer input date.
  • GamificationController rolling recap responses use one fewer input date. Closed calendar months are unchanged.
  • GetRetrospectiveTool chat tool responses for rolling periods use one fewer input date.
  • HabitTools MCP retrospective responses for rolling periods use one fewer input date.
  • CacheInvalidationHelper removes cache entries keyed by the corrected rolling start dates.
  • ProcessUserChatCommand resolves only week, so its behavior is unchanged.

Assumptions

  • Treated the two existing direct consumer assertions as resolver test scope because they encoded the old 31 date month expectation and blocked the required full suite. Rejected leaving those tests inconsistent with the corrected resolver.

Test evidence

  • Before test or implementation changes, dotnet test tests\Orbit.Application.Tests --filter "FullyQualifiedName~RetrospectivePeriodRangeTests" passed all 17 tests with the defect present.
  • With the new count theory and the defect still present, dotnet test tests\Orbit.Application.Tests --no-restore --filter "FullyQualifiedName~RetrospectivePeriodRangeTests" failed all four new cases for the intended reason: month returned 31 instead of 30, quarter 91 instead of 90, semester 181 instead of 180, and year 366 instead of 365.
  • After the resolver fix, the same focused command passed all 21 tests.
  • dotnet test --no-restore --filter "FullyQualifiedName~MonthPeriod_DerivesThirtyDayRange|FullyQualifiedName~GetRecap_RollingMonth_KeepsThirtyDayWindow" passed the application and infrastructure consumer tests.
  • dotnet build Orbit.slnx passed with 0 errors. The final dotnet build Orbit.slnx --no-restore also passed with 0 errors.
  • Final dotnet test passed 32 analyzer tests, 563 domain tests, 3295 application tests, and 2195 infrastructure tests.

@thomasluizon

Copy link
Copy Markdown
Owner Author

Implementation approach:

  1. Strengthen tests/Orbit.Application.Tests/Queries/Habits/RetrospectivePeriodRangeTests.cs with inclusive date count assertions for month, quarter, semester, and year.
  2. Run the focused tests against the current resolver and record the expected count failures.
  3. Change only src/Orbit.Application/Habits/Queries/RetrospectivePeriodRange.cs so each inclusive rolling range contains its promised number of dates.
  4. Rerun focused tests, build, analyzer tests, and the full test suite.

This keeps coverage at the resolver boundary where the defect originates. Endpoint only assertions are retained for general behavior, while the new count assertions directly guard the inclusive range contract. The week branch stays untouched because it is calendar anchored rather than rolling.

@pullfrog pullfrog Bot 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.

Important

This PR does not yet contain the stated fix. Merging it would leave the rolling retrospective windows one date too long and close ticket 469 without changing behavior.

Reviewed changes Reviewed the sole initialization commit and the current retrospective range behavior it leaves untouched.

  • Empty initialization commit: 6e950d4 changes zero files, so the resolver and its permissive tests remain unchanged.

⚠️ The window length bug remains unchanged

RetrospectivePeriodRange.Resolve still subtracts 30, 90, 180, and 365 days while including both endpoints. As a result, the month, quarter, semester, and year windows still contain 31, 91, 181, and 366 dates across every consumer named in the PR description.

Technical details
# Implement the rolling window correction

## Affected sites
- `src/Orbit.Application/Habits/Queries/RetrospectivePeriodRange.cs:28`: inclusive rolling offsets remain one day too large.
- `tests/Orbit.Application.Tests/Queries/Habits/RetrospectivePeriodRangeTests.cs:16`: coverage checks only endpoint ordering and cannot detect incorrect window lengths.

## Required outcome
- Make month, quarter, semester, and year resolve to exactly 30, 90, 180, and 365 inclusive dates.
- Keep the week calculation unchanged.
- Add exact date count assertions that fail against the current resolver.

Pullfrog  | Fix it ➔View workflow run | Using GPT Sol𝕏

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes Reviewed the two substantive commits added since the prior Pullfrog review.

  • Corrected inclusive offsets: Updated month, quarter, semester, and year starts so the shared resolver returns exactly 30, 90, 180, and 365 dates while leaving week behavior unchanged.
  • Added regression coverage: Added exact date-count assertions at the resolver boundary and aligned the chat and gamification month consumer tests with the inclusive 30-date contract.

Pullfrog  | View workflow run | Using GPT Sol𝕏

@thomasluizon

Copy link
Copy Markdown
Owner Author

@pullfrog review

@sonarqubecloud

sonarqubecloud Bot commented Sep 9, 2026

Copy link
Copy Markdown

@pullfrog pullfrog Bot 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.

✅ No new issues found.

Reviewed changes Reviewed the current head end to end, including the shared resolver, all production consumers, cache invalidation, closed month handling, regression tests, and sibling client assumptions.

  • Corrected inclusive windows: Month, quarter, semester, and year now resolve to exactly 30, 90, 180, and 365 user-local dates while week behavior remains unchanged.
  • Strengthened regression coverage: Resolver tests assert exact inclusive counts, and chat and gamification consumer tests pin the 30-date month contract.
  • Preserved consumer consistency: API, chat, MCP, and cache invalidation paths share the corrected resolver; closed calendar months bypass it, and the sibling client does not hardcode competing window lengths.

Pullfrog  | View workflow run | Using GPT Sol𝕏

@thomasluizon
thomasluizon merged commit cf797f0 into main Sep 9, 2026
23 checks passed
@thomasluizon
thomasluizon deleted the fix/ticket-469-window branch September 9, 2026 13:46
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.

1 participant