refactor(statistics): resolve review follow-ups across statistics domain #132

Merged

gabogg merged 2 commits from refactor/statistics-follow-ups into master 2026-09-26 16:01:23 +00:00

Conversation 1 Commits 2 Files changed 18 +853 -291

gabogg commented 2026-09-26 10:25:45 +00:00

Owner

Copy link

Problem Statement

Multiple P3 review follow-ups across the statistics and occupancy domains accumulated from recent PRs (#89, #90, #92, #110, #116, #122, #126). These tasks addressed overlapping concerns around facility schedule abstractions, usual-weekday baseline rules, database seeding constants, controller error handling DRYing, period quality CTE optimizations, and comparison coverage boundaries.

Resolving these items individually would lead to repetitive merge conflicts across the shared statistics and occupancy service layers.

Architectural Approach

1. Schedule Abstraction Decoupling (app/facility_time.py):

   • Extracted ScheduleCalendar, ScheduleException, WeekdaySchedule, is_open_by_schedule, opening_hours_by_schedule, and configured_holiday_hours out of occupancy_service.py into app/facility_time.py.
   • Converted ScheduleCalendar to a frozen dataclass with exceptions keyed by datetime.date holding typed ScheduleException instances.
   • Introduced active_business_day(epoch, reset_time) helper function.

2. Repository & Schema Typing (app/db/occupancy_repository.py, app/schemas/):

   • Replaced raw tuples and untyped dictionaries with typed models: CountedDayFlow(NamedTuple) and CountedCycleQualityRecord(TypedDict).
   • Replaced hardcoded time strings in database.py with DEFAULT_HOLIDAY_HOURS and DEFAULT_WEEKDAY_HOURS schema constants.
   • Preserved superseded_by and is_current columns in record_calibration_log_async.
   • Handled arbitrary (descending/newest-first) cycle order in get_counted_cycle_quality_range_async using min(dates) and max(dates).
   • Enforced validation rules and missing-row-1 guards in update_config_fields_async.
   • Typed baseline failure reasons via BaselineFailureReason = Literal["INSUFFICIENT_USUAL_WEEKDAYS", "BUCKET_COUNT_MISMATCH", "UNSUPPORTED_BUCKET_SIZE"].

3. Controller & Service Refinement (app/services/analytics_service.py, app/controllers/statistics_controller.py):

   • Consolidated repetitive granularity branch logic into _GRANULARITY_STRATEGIES.
   • Extracted handle_controller_errors() context manager in statistics_controller.py to unify ValueError -> 422 and LookupError -> 404 handling.
   • Cached previous_quality in get_statistics_summary_async to eliminate redundant database reads.
   • Hardened PeriodVisitors.per_day() and compare_visitors against zero covered business days and division-by-zero.
   • Removed dead code branches and sanitized variable naming (previous_row, previous_comparison).

Scope & Addressed Issues

• Issue #128: Dwell multiplier fallback test, usual daypart label API assertions, and dead branch cleanup.
• Issue #124: Default hours constants in db initialization, typed CountedCycleQualityRecord, config field validation tests, and descending cycle range tests.
• Issue #120: Comparison documentation precision, zero covered days hardening in PeriodVisitors and comparisons, and clamped comparison coverage test.
• Issue #111: Extracted ScheduleCalendar and schedule evaluation to facility_time, keyed exceptions by date, converted to frozen dataclass, and cleaned up test fixtures.
• Issue #99: Factored handle_controller_errors context manager in statistics_controller, reused previous_quality in summary calculation, typed BaselineFailureReason, and typed comparison reasons.
• Issue #97: Added active_business_day helper, simplified usual-weekday candidate selection, preserved calibration log columns, and fixed endpoint parameters.
• Issue #96: Extracted _GRANULARITY_STRATEGIES strategy map, defined CountedDayFlow NamedTuple, and added tests for period picker gap_estimated_days and daily series is_holiday.

Closes #128, #124, #120, #111, #99, #97, #96.

Verification Evidence

• Linter & Formatter: Clean pass on ruff check . and ruff format --check ..
• Pre-commit Hooks: Passed all pre-commit hooks (whitespace, end of files, ruff, ruff format, pytest).
• Test Suite: 100% green: 394 passed in pytest.

## Problem Statement Multiple P3 review follow-ups across the statistics and occupancy domains accumulated from recent PRs (#89, #90, #92, #110, #116, #122, #126). These tasks addressed overlapping concerns around facility schedule abstractions, usual-weekday baseline rules, database seeding constants, controller error handling DRYing, period quality CTE optimizations, and comparison coverage boundaries. Resolving these items individually would lead to repetitive merge conflicts across the shared statistics and occupancy service layers. ## Architectural Approach 1. **Schedule Abstraction Decoupling (`app/facility_time.py`)**: - Extracted `ScheduleCalendar`, `ScheduleException`, `WeekdaySchedule`, `is_open_by_schedule`, `opening_hours_by_schedule`, and `configured_holiday_hours` out of `occupancy_service.py` into `app/facility_time.py`. - Converted `ScheduleCalendar` to a frozen dataclass with `exceptions` keyed by `datetime.date` holding typed `ScheduleException` instances. - Introduced `active_business_day(epoch, reset_time)` helper function. 2. **Repository & Schema Typing (`app/db/occupancy_repository.py`, `app/schemas/`)**: - Replaced raw tuples and untyped dictionaries with typed models: `CountedDayFlow(NamedTuple)` and `CountedCycleQualityRecord(TypedDict)`. - Replaced hardcoded time strings in `database.py` with `DEFAULT_HOLIDAY_HOURS` and `DEFAULT_WEEKDAY_HOURS` schema constants. - Preserved `superseded_by` and `is_current` columns in `record_calibration_log_async`. - Handled arbitrary (descending/newest-first) cycle order in `get_counted_cycle_quality_range_async` using `min(dates)` and `max(dates)`. - Enforced validation rules and missing-row-1 guards in `update_config_fields_async`. - Typed baseline failure reasons via `BaselineFailureReason = Literal["INSUFFICIENT_USUAL_WEEKDAYS", "BUCKET_COUNT_MISMATCH", "UNSUPPORTED_BUCKET_SIZE"]`. 3. **Controller & Service Refinement (`app/services/analytics_service.py`, `app/controllers/statistics_controller.py`)**: - Consolidated repetitive granularity branch logic into `_GRANULARITY_STRATEGIES`. - Extracted `handle_controller_errors()` context manager in `statistics_controller.py` to unify `ValueError -> 422` and `LookupError -> 404` handling. - Cached `previous_quality` in `get_statistics_summary_async` to eliminate redundant database reads. - Hardened `PeriodVisitors.per_day()` and `compare_visitors` against zero covered business days and division-by-zero. - Removed dead code branches and sanitized variable naming (`previous_row`, `previous_comparison`). ## Scope & Addressed Issues - **Issue #128**: Dwell multiplier fallback test, usual daypart label API assertions, and dead branch cleanup. - **Issue #124**: Default hours constants in db initialization, typed `CountedCycleQualityRecord`, config field validation tests, and descending cycle range tests. - **Issue #120**: Comparison documentation precision, zero covered days hardening in `PeriodVisitors` and comparisons, and clamped comparison coverage test. - **Issue #111**: Extracted `ScheduleCalendar` and schedule evaluation to `facility_time`, keyed exceptions by date, converted to frozen dataclass, and cleaned up test fixtures. - **Issue #99**: Factored `handle_controller_errors` context manager in `statistics_controller`, reused `previous_quality` in summary calculation, typed `BaselineFailureReason`, and typed comparison reasons. - **Issue #97**: Added `active_business_day` helper, simplified usual-weekday candidate selection, preserved calibration log columns, and fixed endpoint parameters. - **Issue #96**: Extracted `_GRANULARITY_STRATEGIES` strategy map, defined `CountedDayFlow` NamedTuple, and added tests for period picker `gap_estimated_days` and daily series `is_holiday`. Closes #128, #124, #120, #111, #99, #97, #96. ## Verification Evidence - **Linter & Formatter**: Clean pass on `ruff check .` and `ruff format --check .`. - **Pre-commit Hooks**: Passed all pre-commit hooks (whitespace, end of files, ruff, ruff format, pytest). - **Test Suite**: 100% green: 394 passed in `pytest`.

gabogg added 1 commit 2026-09-26 10:25:46 +00:00

refactor(statistics): resolve review follow-ups across statistics domain ... 12e93de9b1

Address overlapping follow-up issues across the statistics and occupancy domains:
- Issue #128: Dwell multiplier fallback test, usual daypart label assertions, dead branch cleanup
- Issue #124: Replaced hardcoded hour seeds with schema constants, typed CountedCycleQualityRecord, tested config field validation and descending cycle ranges
- Issue #120: Docs precision for comparisons, zero covered days hardening in PeriodVisitors and comparisons, clamped coverage test
- Issue #111: Extracted ScheduleCalendar and schedule evaluation into facility_time, keyed exceptions by date, converted ScheduleCalendar to frozen dataclass, cleaned up test fixtures
- Issue #99: Factored handle_controller_errors context manager in statistics_controller, reused previous_quality query, typed BaselineFailureReason and comparison reasons
- Issue #97: Added active_business_day helper, simplified usual-weekday candidate selection, preserved superseded_by/is_current in calibration logs, fixed query parameters
- Issue #96: Extracted _GRANULARITY_STRATEGIES strategy map, defined CountedDayFlow NamedTuple, added gap_estimated_days and is_holiday tests

Closes #128, #124, #120, #111, #99, #97, #96

gabogg changed title from WIP: refactor(statistics): resolve review follow-ups across statistics domain to refactor(statistics): resolve review follow-ups across statistics domain 2026-09-26 11:27:55 +00:00

gabogg commented 2026-09-26 11:28:08 +00:00

Author

Owner

Copy link

Standards

Hard Violations (Documented Standards)

1. Missing Return Type Annotation — statistics_controller.py:22

   • Standard: docs/standards/code-standards.md §2.2 & AGENTS.md §2 (Type Annotations): "All function definitions (parameters and return types) must include explicit type hints."
   • Violation: def handle_controller_errors(): lacks an explicit return type hint (e.g. -> Iterator[None]).

2. Non-Standard HTTP Status Literals — statistics_controller.py:27-32

   • Standard: docs/standards/code-standards.md §3.1 (HTTP Exception Uniformity): requires fastapi.status constants (status.HTTP_...).
   • Violation: Uses raw integer literals status_code=422 and status_code=404 directly instead of status.HTTP_422_UNPROCESSABLE_ENTITY and status.HTTP_404_NOT_FOUND (status was removed from imports).

3. Untyped Dictionaries Across Layers — facility_time.py:309-354

   • Standard: docs/standards/code-standards.md §2.3 (Pydantic Schemas): "Avoid passing untyped, raw dictionaries between service layers when structured schemas are available."
   • Violation: is_open_by_schedule, opening_hours_by_schedule, and configured_holiday_hours accept dict[str, Any] alongside domain dataclasses to accommodate unmapped DB rows from occupancy_service.py.

4. Domain Types Defined in Persistence Layer — occupancy_repository.py:186-204

   • Standard: AGENTS.md §2 & docs/standards/code-standards.md §2.3: "Define domain request/response contracts in app/schemas/."
   • Violation: CountedDayFlow (NamedTuple) and CountedCycleQualityRecord (TypedDict) are defined in the repository file rather than app/schemas/occupancy_models.py.

Judgement Calls (Baseline Smells)

1. Primitive Obsession — facility_time.py:309-322

   def is_open_by_schedule(
       exception: ScheduleException | dict[str, Any] | None,
       weekday: WeekdaySchedule | dict[str, Any] | None,
   ) -> bool:
   

   Smell: Type-branching on isinstance vs .get() to handle raw dictionaries. Upstream callers should instantiate ScheduleException / WeekdaySchedule before passing.

2. Divergent Change — facility_time.py:291-379
   Smell: facility_time.py (authoritative for civil time and cycle boundary math) now hosts facility schedule entities and open/closed evaluation logic.

3. Middle Man — analytics_service.py:584-587

   def statistics_period_end(granularity: Granularity, start: datetime.date) -> datetime.date:
       return _GRANULARITY_STRATEGIES[granularity].period_end(start)
   

   Smell: 4 top-level functions delegate entirely to _GRANULARITY_STRATEGIES. Acceptable trade-off: eliminates the prior Repeated Switches smell while preserving the module's public API.

---

Spec

Missing or Partial Requirements

• #128 Single Ranked Verdict & Reset Removal:

  One ranked verdict decides everything. excluded is derived from the same winning verdict as trusted... Offset resets are not verdicts... Remove them from _CYCLE_VERDICT_PRIORITY_SQL
  Missing: _CYCLE_VERDICT_PRIORITY_SQL still retains MANUAL_ADMIN / MANUAL_OVERRIDE; excluded still reads only automatic nocturnal audits; none of the specified audit/reset test cases or doc updates were added.

• #128 Document Null Label:

  docs/api/README.md ... should say that label is null when source days' clock spans differ.
  Missing: docs/api/README.md was not updated with this requirement.

• #97 / #99 Deduplicate Controller Error Handling:

  Repeated ValueError -> 422 blocks in statistics_controller.py and analytics_controller.py. One helper or exception handler.
  Partial: Added handle_controller_errors context manager only to statistics_controller.py; analytics_controller.py remains untouched.

• #120 Entrance Coverage Assertion:

  Closed Days / exact-threshold test asserts only on the summary, not the entrance comparison.
  Missing: No entrance comparison assertions were added.

• #96 Typed Cycles Argument:

  The cycles: list[tuple[str, float, float]] argument is an anonymous triple that must be passed sorted.
  Partial: CountedDayFlow and CountedCycleQualityRecord were created, but cycles remains an untyped triple.

• #99 Trusted Multiplier Helper:

  Extract _trusted_cycle_multiplier(day, bounds, default).
  Missing: Helper was not extracted.

Scope Creep (Unasked Behaviour)

• Log Table Write Fields:
  In occupancy_repository.py, _CALIBRATION_LOG_INSERT_SQL and _prepare_calibration_log_params were modified to insert superseded_by and is_current. No spec requested schema/insert extensions on this repository method.
• test_update_config_fields_rejects_unknown_keys:
  Added to test_occupancy_config_api.py without being specified in #124.

Incorrectly Implemented Requirements

• #120 Zero-Division Guard in compare_visitors:

  Guard inside per_day()/compare_visitors (treat covered_days == 0 as non-comparable)
  Wrong: compare_visitors returns hardcoded reason="PARTIAL_PERIOD" whenever covered_days == 0. When a reference period is completely closed (business_days == 0), covered_days == 0, but the domain specification requires reason="CLOSED_PERIOD".

• #128 Mutation Test Gaps:

  changing > MIN_PLAUSIBLE_EXIT_MULTIPLIER to > 0 still passes, so add a case at 0.5 or 0.4 to pin the boundary; reverting label=_shared_label... still passes...
  Wrong: In test_implausible_multiplier_falls_back_to_active, res and res_active execute against the same day and existing log (0.5); mutating > 0.5 to > 0 still passes. Similarly, test_usual_daypart_label_through_api_response seeds identical source days, failing to catch a reversion to usable[0].

---

Summary: Standards: 7 findings (worst: untyped dictionaries and missing return type annotation breaking documented type safety standards). Spec: 10 findings (worst: #128 single ranked verdict decision missing from cycle quality queries and #120 incorrect PARTIAL_PERIOD fallback on zero covered days).

## Standards ### Hard Violations (Documented Standards) 1. **Missing Return Type Annotation** — `statistics_controller.py:22` - **Standard**: `docs/standards/code-standards.md` §2.2 & `AGENTS.md` §2 (*Type Annotations*): *"All function definitions (parameters and return types) must include explicit type hints."* - **Violation**: `def handle_controller_errors():` lacks an explicit return type hint (e.g. `-> Iterator[None]`). 2. **Non-Standard HTTP Status Literals** — `statistics_controller.py:27-32` - **Standard**: `docs/standards/code-standards.md` §3.1 (*HTTP Exception Uniformity*): requires `fastapi.status` constants (`status.HTTP_...`). - **Violation**: Uses raw integer literals `status_code=422` and `status_code=404` directly instead of `status.HTTP_422_UNPROCESSABLE_ENTITY` and `status.HTTP_404_NOT_FOUND` (`status` was removed from imports). 3. **Untyped Dictionaries Across Layers** — `facility_time.py:309-354` - **Standard**: `docs/standards/code-standards.md` §2.3 (*Pydantic Schemas*): *"Avoid passing untyped, raw dictionaries between service layers when structured schemas are available."* - **Violation**: `is_open_by_schedule`, `opening_hours_by_schedule`, and `configured_holiday_hours` accept `dict[str, Any]` alongside domain dataclasses to accommodate unmapped DB rows from `occupancy_service.py`. 4. **Domain Types Defined in Persistence Layer** — `occupancy_repository.py:186-204` - **Standard**: `AGENTS.md` §2 & `docs/standards/code-standards.md` §2.3: *"Define domain request/response contracts in app/schemas/."* - **Violation**: `CountedDayFlow` (`NamedTuple`) and `CountedCycleQualityRecord` (`TypedDict`) are defined in the repository file rather than `app/schemas/occupancy_models.py`. ### Judgement Calls (Baseline Smells) 1. **Primitive Obsession** — `facility_time.py:309-322` ```python def is_open_by_schedule( exception: ScheduleException | dict[str, Any] | None, weekday: WeekdaySchedule | dict[str, Any] | None, ) -> bool: ``` *Smell*: Type-branching on `isinstance` vs `.get()` to handle raw dictionaries. Upstream callers should instantiate `ScheduleException` / `WeekdaySchedule` before passing. 2. **Divergent Change** — `facility_time.py:291-379` *Smell*: `facility_time.py` (authoritative for civil time and cycle boundary math) now hosts facility schedule entities and open/closed evaluation logic. 3. **Middle Man** — `analytics_service.py:584-587` ```python def statistics_period_end(granularity: Granularity, start: datetime.date) -> datetime.date: return _GRANULARITY_STRATEGIES[granularity].period_end(start) ``` *Smell*: 4 top-level functions delegate entirely to `_GRANULARITY_STRATEGIES`. Acceptable trade-off: eliminates the prior *Repeated Switches* smell while preserving the module's public API. --- ## Spec ### Missing or Partial Requirements - **#128 Single Ranked Verdict & Reset Removal**: > `One ranked verdict decides everything. excluded is derived from the same winning verdict as trusted... Offset resets are not verdicts... Remove them from _CYCLE_VERDICT_PRIORITY_SQL` *Missing*: `_CYCLE_VERDICT_PRIORITY_SQL` still retains `MANUAL_ADMIN` / `MANUAL_OVERRIDE`; `excluded` still reads only automatic nocturnal audits; none of the specified audit/reset test cases or doc updates were added. - **#128 Document Null Label**: > `docs/api/README.md ... should say that label is null when source days' clock spans differ.` *Missing*: `docs/api/README.md` was not updated with this requirement. - **#97 / #99 Deduplicate Controller Error Handling**: > `Repeated ValueError -> 422 blocks in statistics_controller.py and analytics_controller.py. One helper or exception handler.` *Partial*: Added `handle_controller_errors` context manager only to `statistics_controller.py`; `analytics_controller.py` remains untouched. - **#120 Entrance Coverage Assertion**: > `Closed Days / exact-threshold test asserts only on the summary, not the entrance comparison.` *Missing*: No entrance comparison assertions were added. - **#96 Typed Cycles Argument**: > `The cycles: list[tuple[str, float, float]] argument is an anonymous triple that must be passed sorted.` *Partial*: `CountedDayFlow` and `CountedCycleQualityRecord` were created, but `cycles` remains an untyped triple. - **#99 Trusted Multiplier Helper**: > `Extract _trusted_cycle_multiplier(day, bounds, default).` *Missing*: Helper was not extracted. ### Scope Creep (Unasked Behaviour) - **Log Table Write Fields**: In `occupancy_repository.py`, `_CALIBRATION_LOG_INSERT_SQL` and `_prepare_calibration_log_params` were modified to insert `superseded_by` and `is_current`. No spec requested schema/insert extensions on this repository method. - **`test_update_config_fields_rejects_unknown_keys`**: Added to `test_occupancy_config_api.py` without being specified in #124. ### Incorrectly Implemented Requirements - **#120 Zero-Division Guard in `compare_visitors`**: > `Guard inside per_day()/compare_visitors (treat covered_days == 0 as non-comparable)` *Wrong*: `compare_visitors` returns hardcoded `reason="PARTIAL_PERIOD"` whenever `covered_days == 0`. When a reference period is completely closed (`business_days == 0`), `covered_days == 0`, but the domain specification requires `reason="CLOSED_PERIOD"`. - **#128 Mutation Test Gaps**: > `changing > MIN_PLAUSIBLE_EXIT_MULTIPLIER to > 0 still passes, so add a case at 0.5 or 0.4 to pin the boundary; reverting label=_shared_label... still passes...` *Wrong*: In `test_implausible_multiplier_falls_back_to_active`, `res` and `res_active` execute against the same day and existing log (`0.5`); mutating `> 0.5` to `> 0` still passes. Similarly, `test_usual_daypart_label_through_api_response` seeds identical source days, failing to catch a reversion to `usable[0]`. --- **Summary:** Standards: 7 findings (worst: untyped dictionaries and missing return type annotation breaking documented type safety standards). Spec: 10 findings (worst: #128 single ranked verdict decision missing from cycle quality queries and #120 incorrect `PARTIAL_PERIOD` fallback on zero covered days).

gabogg referenced this pull request 2026-09-26 11:39:33 +00:00

fix(statistics): one ranked cycle verdict; offset resets are not verdicts #137

gabogg referenced this pull request 2026-09-26 11:47:11 +00:00

feat(statistics): data-quality marker inputs for the deck #138

gabogg referenced this pull request from a commit 2026-09-26 16:00:17 +00:00

fix(statistics): address code-review findings for PR #132

gabogg added 1 commit 2026-09-26 16:00:17 +00:00

fix(statistics): address code-review findings for PR #132 d799524b50

gabogg merged commit 8f7a80e76e into master 2026-09-26 16:01:23 +00:00

gabogg deleted branch refactor/statistics-follow-ups 2026-09-26 16:01:23 +00:00

gabogg referenced this pull request from a commit 2026-09-26 16:01:24 +00:00

Merge pull request 'refactor(statistics): resolve review follow-ups across statistics domain' (#132) from refactor/statistics-follow-ups into master

gabogg referenced this pull request 2026-09-26 16:46:20 +00:00

fix(statistics): one ranked cycle verdict; offset resets are not verdicts #137

gabogg referenced this pull request 2026-09-26 21:02:11 +00:00

fix(statistics): one ranked cycle verdict; offset resets are not verdicts #137

gabogg referenced this pull request 2026-09-26 21:02:17 +00:00

feat(statistics): data-quality marker inputs for the deck #138

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

blocked

Waits on the issues listed in its "Blocked by:" line before implementation

  

bug

Category: something is broken; behaviour differs from the spec or docs

  

enhancement

Category: new feature or improvement, including refactors, chores and docs

  

high-priority

Address before ordinary-priority work; independent of triage readiness and blockers

  

low-priority

Address after ordinary-priority work; independent of triage readiness and blockers

  

needs-info

Waiting on reporter for more information

  

needs-triage

Maintainer needs to evaluate this issue

  

ready-for-agent

Fully specified, ready for an AFK agent

  

ready-for-human

Requires human implementation

  

referenced

Previously closed issue reopened or referenced by an open issue; visible history marker

  

research

Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  

wontfix

Will not be actioned

No labels

blocked

bug

enhancement

high-priority

low-priority

needs-info

needs-triage

ready-for-agent

ready-for-human

referenced

research

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gabogg/hikcentral!132

No description provided.