feat(statistics): Closed Days are not business days (#103) #110

Merged

gabogg merged 3 commits from feat/statistics-closed-days into master 2026-09-25 18:08:35 +00:00

Conversation 4 Commits 3 Files changed 7 +574 -75

gabogg commented 2026-09-25 17:38:23 +00:00

Owner

Copy link

Closes #103

Summary

A Closed Day (a business day the weekday schedule or a dated exception declares closed) is no longer a business day for statistics. A closure never makes a week or month partial, and passages counted on it (staff, guards) stay out of the deck's figures. Step 1 of the pre-deck work agreed on 2026-09-25.

Problem

The admin can already declare closures (weekday "open" checkbox, dated entry marked closed), but statistics ignored is_open. A closed day with no counted data made its period partial, like an outage, and a closed day with staff passages counted as a normal business day.

Architectural impact

• OccupancyManager.get_closed_business_days_async resolves closures with the same precedence as the live schedule: a dated exception overrides the weekday schedule; open by default.
• OccupancyRepository.get_counted_day_flow_async / get_counted_camera_flow_async take optional skip_windows (half-open cycle spans) so Closed Day passages are left out in SQL.
• Contract additions (all additive): PeriodQuality.closed_days, DailyStatistics.is_closed, StatisticsSummary.closed, comparison reason CLOSED_PERIOD.
• Behaviour: business_days excludes Closed Days; period visitors, egress, daily average, busiest/best day, peak, weekend share, entrance totals/shares, comparison references and usual-weekday samples exclude them. A fully closed period returns closed: true with zero figures (summary) or an empty list (entrances) instead of 404, and is listed in the picker with business_days: 0.
• Glossary: new Closed Day term in CONTEXT.md. API docs updated.
• No schema migration.

Known limitation: schedules have no history, so a change to the weekly schedule also re-classifies past days. Same limitation already applies to opening hours (dwell, dayparts).

Verification

• New tests/test_statistics_closed_days.py (4 tests): exception-over-weekday precedence; a week with a closed Wednesday is complete (6 business days), its staff passages stay out of visitors, daily average, entrances and the previous-week reference; a closed day returns closed: true, CLOSED_PERIOD comparisons and is listed in the picker; a closed weekday is never a usual-weekday sample. All 4 fail without the service changes.
• pytest: 332 passed. Ruff lint/format and pre-commit passed. scripts/check_docs.py: 33 files, 72 operations.

Checklist

• Closed Days excluded from business_days; never partial.
• Closed Day passages excluded from all statistics figures and comparison references.
• Never a Usual Weekday Baseline sample.
• Closed period returns a closed status instead of an error.
• Picker and daily rows expose closures.
• Glossary and API docs updated.

🤖 Generated with Claude Code

Closes #103 ## Summary A **Closed Day** (a business day the weekday schedule or a dated exception declares closed) is no longer a business day for statistics. A closure never makes a week or month partial, and passages counted on it (staff, guards) stay out of the deck's figures. Step 1 of the pre-deck work agreed on 2026-09-25. ## Problem The admin can already declare closures (weekday "open" checkbox, dated entry marked closed), but statistics ignored `is_open`. A closed day with no counted data made its period partial, like an outage, and a closed day with staff passages counted as a normal business day. ## Architectural impact - `OccupancyManager.get_closed_business_days_async` resolves closures with the same precedence as the live schedule: a dated exception overrides the weekday schedule; open by default. - `OccupancyRepository.get_counted_day_flow_async` / `get_counted_camera_flow_async` take optional `skip_windows` (half-open cycle spans) so Closed Day passages are left out in SQL. - Contract additions (all additive): `PeriodQuality.closed_days`, `DailyStatistics.is_closed`, `StatisticsSummary.closed`, comparison reason `CLOSED_PERIOD`. - Behaviour: `business_days` excludes Closed Days; period visitors, egress, daily average, busiest/best day, peak, weekend share, entrance totals/shares, comparison references and usual-weekday samples exclude them. A fully closed period returns `closed: true` with zero figures (summary) or an empty list (entrances) instead of 404, and is listed in the picker with `business_days: 0`. - Glossary: new **Closed Day** term in `CONTEXT.md`. API docs updated. - No schema migration. **Known limitation:** schedules have no history, so a change to the weekly schedule also re-classifies past days. Same limitation already applies to opening hours (dwell, dayparts). ## Verification - New `tests/test_statistics_closed_days.py` (4 tests): exception-over-weekday precedence; a week with a closed Wednesday is complete (6 business days), its staff passages stay out of visitors, daily average, entrances and the previous-week reference; a closed day returns `closed: true`, `CLOSED_PERIOD` comparisons and is listed in the picker; a closed weekday is never a usual-weekday sample. All 4 fail without the service changes. - `pytest`: 332 passed. Ruff lint/format and pre-commit passed. `scripts/check_docs.py`: 33 files, 72 operations. ## Checklist - [x] Closed Days excluded from `business_days`; never partial. - [x] Closed Day passages excluded from all statistics figures and comparison references. - [x] Never a Usual Weekday Baseline sample. - [x] Closed period returns a closed status instead of an error. - [x] Picker and daily rows expose closures. - [x] Glossary and API docs updated. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 17:38:23 +00:00

feat(statistics): Closed Days are not business days ... b62694ce2b

A day the weekday schedule or a dated exception declares closed no longer
counts as a business day. It never makes a week or month partial, and
passages counted on it (staff, guards) stay out of period figures, entrance
totals, comparison references and usual-weekday samples. Closed Days are
reported as `closed_days` on period quality and `is_closed` on daily rows;
a fully closed period returns `closed: true` and comparisons against it use
the reason CLOSED_PERIOD. Adds the Closed Day glossary term.

Closes #103

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg commented 2026-09-25 17:46:10 +00:00

Author

Owner

Copy link

Code review — pass 1 (two-axis)

Reviewed git diff origin/master...origin/feat/statistics-closed-days (b62694c) against #103. Standards and Spec ran independently and are reported separately, not reranked. Severity: P1 must fix · P2 fix before merge · P3 minor. Per review policy, pass 1 findings are all fixed on the branch.

Standards

Ruff check/format clean; pytest 331 passed, 1 skipped. No hard violations of AGENTS.md or the code standards: SQL stays in the repository, functions are typed, I/O is async.

Bug

• P2 app/services/analytics_service.py:543: the picker lists a period when covered_days or closed_days. A week with one Closed Day and no counted data (an outage) is listed with business_days: 6, covered_days: 0; selecting it fails because validate_statistics_period_async (:160) raises LookupError (404). Condition should be quality.covered_days or quality.business_days == 0.

Judgement calls (smells)

• P2 Duplicated Code, occupancy_service.py:221-238 vs get_active_schedule_info_async: "a dated exception beats the weekday schedule, open by default" now exists twice (the docstring says so: "Uses the same precedence…"). The daily series takes is_closed from the old copy (analytics_service.py:662) while quality and figures use the new one (:498). If they drift, the summary's not row.is_closed filter (:246) and business_days disagree. Make the old path call the new resolver.
• P3 Duplicated Code: the [start, end) day list [start + timedelta(days=offset) for offset in range((end - start).days)] is built twice (analytics_service.py:169, :496). Extract a helper.
• P3 Primitive Obsession / Data Clumps: skip_windows: Sequence[tuple[float, float]] (occupancy_repository.py:38, 1233, 1256) is a bare tuple for a cycle span; a bounds type with start/next_start already exists.
• P3 performance: each get_closed_business_days_async reads the whole occupancy_holidays and schedule tables. One summary resolves closures 3–4 times (quality, reference quality, _closed_windows_async); the picker once per period. The baseline (:122) resolves closures and then still calls get_holiday_by_date_async per candidate (:128). Resolve once per request and pass the set along.
• P3 hard-coded values in tests: close_weekday(6) (test:72) is an unnamed Sunday; date(2026, 9, 7) (:70) and range(7) (:77, :98) are bare literals. Name them (calendar.SUNDAY).
• P3 tests bypass the repository: declare_exception and close_weekday (test:42-58) write raw SQL, though the repository has upsert_holiday / update_daily_schedule_async (occupancy_repository.py:597, ~644). AGENTS.md §3: test real flows.

CONTEXT.md

• P3 CONTEXT.md:101: the definition contradicts itself — "A business day on which…" then "It is not a business day for statistics". Start with "A calendar day…". Otherwise domain language only, meeting the glossary-only rule.

SQL correctness (checked, OK)

The skip-window filter is correct: AND NOT (ts >= ? AND ts < ?) sits in the LEFT JOIN … ON clause in the camera query (zero-flow cameras still appear) and in WHERE in the day-flow query. Spans are half-open and aligned with cycle bounds; the range predicate can still use the index; a month has at most 31 terms, well under SQLite's parameter limit.

Spec

Verdict: one P2 to fix before merge; the rest is P3. Full suite passes (331 passed, 1 skipped) and the 4 new tests pass in a throwaway worktree.

(c) Implemented, but wrong in one case

• P2: the picker lists periods that return 404. app/services/analytics_service.py:~544. Spec: "Picker quality and daily rows expose it: closed_days on PeriodQuality…". The picker filter is now quality.covered_days or quality.closed_days, which lists an outage period (no data on any open day) as soon as it contains one Closed Day. Selecting it then fails, because validate_statistics_period_async (:160) raises when business_days and covered_days == 0. Confirmed with a probe: a week with no open-day data and one closed Wednesday is listed as business_days=6, covered_days=0, and its summary returns 404 NOT_FOUND. Any site with a closed weekday in its weekly schedule would show every outage week/month in the picker. Fix: if quality.covered_days or quality.business_days == 0.

(a) Missing or partial

• P3: the reference side of a comparison has no business-day count (VisitorComparison). Spec: "Comparisons keep comparing totals; the badge explains fewer business days." quality.closed_days only describes the current period; the response doesn't say the previous/last-year period had fewer business days, so the badge would need a separate picker call. Acceptable if the deck does that, but decide explicitly.
• P3: inconsistent reasons when the reference period is fully closed: entrance previous reports NO_CAMERA_DATA_PREVIOUS_PERIOD, not CLOSED_PERIOD (:~425); previous_month_daily_average_reason falls back to NO_DATA_PREVIOUS_PERIOD (:~352); the closed summary's previous has no reference_start/reference_end (:236).

(b) Scope creep

None. CLOSED_PERIOD and StatisticsSummary.closed are what "the summary returns a closed status" needs.

Surfaces checked and found correct

• Weekend share (numerator and denominator both from covered), peak, busiest/best day, daily_average, previous_month_daily_average, and the entrance current/previous/last-year rows (via skip_windows).
• Usual-weekday samples, which also feed the presenter hourly and daypart baselines (:791, :1057).
• Day view busiest hour and top entrance (only run for an open day).
• DailyStatistics.is_closed and get_closed_business_days_async agree on precedence ("a dated entry overrides the weekday schedule").
• P3 edge case: NEW-camera detection. first_event_epoch (occupancy_repository.py:1271) still counts Closed Day events; a camera whose first events fell only on a previous period's Closed Day is labelled NO_REFERENCE_DATA, not NEW. Arguably correct.

Test coverage

• P3: busiest_day/peak exclusion not really tested (tests/test_statistics_closed_days.py:~130): the closed Wednesday has 3 passages vs 10, so busiest_day != wednesday passes even without the exclusion. Give the Closed Day the highest count.
• P3: untested figures: weekend share, egress, peak, month best_day / previous_month_daily_average, the last-year reference, the picker limit counting fully closed days.

---

Summary: Standards 9 (2×P2, 7×P3), worst: picker lists outage periods that 404 (also flagged by Spec). Spec 7 (1×P2, 6×P3), worst: the same picker bug.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed `git diff origin/master...origin/feat/statistics-closed-days` (b62694c) against #103. Standards and Spec ran independently and are reported separately, not reranked. Severity: **P1** must fix · **P2** fix before merge · **P3** minor. Per review policy, pass 1 findings are all fixed on the branch. ## Standards Ruff check/format clean; `pytest` 331 passed, 1 skipped. No hard violations of AGENTS.md or the code standards: SQL stays in the repository, functions are typed, I/O is async. ### Bug - **P2 `app/services/analytics_service.py:543`**: the picker lists a period when `covered_days or closed_days`. A week with one Closed Day and no counted data (an outage) is listed with `business_days: 6, covered_days: 0`; selecting it fails because `validate_statistics_period_async` (:160) raises `LookupError` (404). Condition should be `quality.covered_days or quality.business_days == 0`. ### Judgement calls (smells) - **P2 Duplicated Code, `occupancy_service.py:221-238` vs `get_active_schedule_info_async`**: "a dated exception beats the weekday schedule, open by default" now exists twice (the docstring says so: "Uses the same precedence…"). The daily series takes `is_closed` from the old copy (`analytics_service.py:662`) while quality and figures use the new one (`:498`). If they drift, the summary's `not row.is_closed` filter (:246) and `business_days` disagree. Make the old path call the new resolver. - **P3 Duplicated Code**: the `[start, end)` day list `[start + timedelta(days=offset) for offset in range((end - start).days)]` is built twice (`analytics_service.py:169`, `:496`). Extract a helper. - **P3 Primitive Obsession / Data Clumps**: `skip_windows: Sequence[tuple[float, float]]` (`occupancy_repository.py:38, 1233, 1256`) is a bare tuple for a cycle span; a bounds type with `start`/`next_start` already exists. - **P3 performance**: each `get_closed_business_days_async` reads the whole `occupancy_holidays` and schedule tables. One summary resolves closures 3–4 times (quality, reference quality, `_closed_windows_async`); the picker once per period. The baseline (:122) resolves closures and then still calls `get_holiday_by_date_async` per candidate (:128). Resolve once per request and pass the set along. - **P3 hard-coded values in tests**: `close_weekday(6)` (test:72) is an unnamed Sunday; `date(2026, 9, 7)` (:70) and `range(7)` (:77, :98) are bare literals. Name them (`calendar.SUNDAY`). - **P3 tests bypass the repository**: `declare_exception` and `close_weekday` (test:42-58) write raw SQL, though the repository has `upsert_holiday` / `update_daily_schedule_async` (`occupancy_repository.py:597, ~644`). AGENTS.md §3: test real flows. ### CONTEXT.md - **P3 `CONTEXT.md:101`**: the definition contradicts itself — "A business day on which…" then "It is not a business day for statistics". Start with "A calendar day…". Otherwise domain language only, meeting the glossary-only rule. ### SQL correctness (checked, OK) The skip-window filter is correct: `AND NOT (ts >= ? AND ts < ?)` sits in the `LEFT JOIN … ON` clause in the camera query (zero-flow cameras still appear) and in `WHERE` in the day-flow query. Spans are half-open and aligned with cycle bounds; the range predicate can still use the index; a month has at most 31 terms, well under SQLite's parameter limit. ## Spec **Verdict:** one P2 to fix before merge; the rest is P3. Full suite passes (331 passed, 1 skipped) and the 4 new tests pass in a throwaway worktree. ### (c) Implemented, but wrong in one case - **P2: the picker lists periods that return 404.** `app/services/analytics_service.py:~544`. Spec: *"Picker quality and daily rows expose it: `closed_days` on `PeriodQuality`…"*. The picker filter is now `quality.covered_days or quality.closed_days`, which lists an outage period (no data on any open day) as soon as it contains one Closed Day. Selecting it then fails, because `validate_statistics_period_async` (`:160`) raises when `business_days and covered_days == 0`. Confirmed with a probe: a week with no open-day data and one closed Wednesday is listed as `business_days=6, covered_days=0`, and its summary returns `404 NOT_FOUND`. Any site with a closed weekday in its weekly schedule would show every outage week/month in the picker. Fix: `if quality.covered_days or quality.business_days == 0`. ### (a) Missing or partial - **P3: the reference side of a comparison has no business-day count** (`VisitorComparison`). Spec: *"Comparisons keep comparing totals; the badge explains fewer business days."* `quality.closed_days` only describes the current period; the response doesn't say the previous/last-year period had fewer business days, so the badge would need a separate picker call. Acceptable if the deck does that, but decide explicitly. - **P3: inconsistent reasons when the reference period is fully closed:** entrance `previous` reports `NO_CAMERA_DATA_PREVIOUS_PERIOD`, not `CLOSED_PERIOD` (`:~425`); `previous_month_daily_average_reason` falls back to `NO_DATA_PREVIOUS_PERIOD` (`:~352`); the closed summary's `previous` has no `reference_start`/`reference_end` (`:236`). ### (b) Scope creep None. `CLOSED_PERIOD` and `StatisticsSummary.closed` are what *"the summary returns a closed status"* needs. ### Surfaces checked and found correct - Weekend share (numerator and denominator both from `covered`), peak, busiest/best day, `daily_average`, `previous_month_daily_average`, and the entrance current/previous/last-year rows (via `skip_windows`). - Usual-weekday samples, which also feed the presenter hourly and daypart baselines (`:791`, `:1057`). - Day view busiest hour and top entrance (only run for an open day). - `DailyStatistics.is_closed` and `get_closed_business_days_async` agree on precedence (*"a dated entry overrides the weekday schedule"*). - **P3 edge case: NEW-camera detection.** `first_event_epoch` (`occupancy_repository.py:1271`) still counts Closed Day events; a camera whose first events fell only on a previous period's Closed Day is labelled `NO_REFERENCE_DATA`, not `NEW`. Arguably correct. ### Test coverage - **P3: `busiest_day`/peak exclusion not really tested** (`tests/test_statistics_closed_days.py:~130`): the closed Wednesday has 3 passages vs 10, so `busiest_day != wednesday` passes even without the exclusion. Give the Closed Day the highest count. - **P3: untested figures:** weekend share, egress, peak, month `best_day` / `previous_month_daily_average`, the last-year reference, the picker `limit` counting fully closed days. --- **Summary:** Standards 9 (2×P2, 7×P3), worst: picker lists outage periods that 404 (also flagged by Spec). Spec 7 (1×P2, 6×P3), worst: the same picker bug. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 17:57:06 +00:00

fix(statistics): address review of Closed Days ... 7f1f7a6453

- The picker no longer lists an outage period just because it contains a
  Closed Day; only covered or fully closed periods are selectable.
- One schedule-precedence rule (`is_open_by_schedule`) serves the live
  schedule and the new `ScheduleCalendar`, loaded once per request and
  passed down instead of re-read per quality/comparison call.
- Comparisons carry `reference_business_days`; fully closed reference
  periods report CLOSED_PERIOD on entrances and the previous-month daily
  average, and the closed summary names its reference period.
- Skip spans are typed as `CycleBounds`; `business_dates` replaces the
  duplicated day-list comprehension.
- Tests use repository writers and named constants, give the Closed Day
  the highest counts, and cover egress, peak, weekend share, month figures,
  last-year references and the outage picker case.
- Glossary: a Closed Day is "a calendar day", not "a business day".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg commented 2026-09-25 17:57:20 +00:00

Author

Owner

Copy link

Pass-1 findings addressed in 7f1f7a6. Full suite: 334 passed; ruff and docs check clean.

Standards

Finding	Fix
P2 picker lists outage periods that 404	Filter is now covered_days or business_days == 0. New test_an_outage_week_with_a_closed_day_is_not_selectable fails with the old filter.
P2 precedence rule duplicated	is_open_by_schedule(exception, weekday) in occupancy_service.py is the single rule; get_active_schedule_info_async (daily-row is_closed) and ScheduleCalendar.is_closed both call it.
P3 duplicated day-list comprehension	business_dates(start, end) module helper.
P3 bare (float, float) skip spans	skip_cycles: Sequence[CycleBounds]; SQL built by _skip_cycles_sql.
P3 closures re-read 3–4× per request	ScheduleCalendar is loaded once per summary / entrances / picker request and passed down (quality, comparisons, closed cycles, baseline). The baseline uses calendar.exception_for instead of one holiday query per candidate.
P3 magic literals in tests	calendar.WEDNESDAY/SATURDAY/SUNDAY, DAYS_PER_WEEK, named count constants.
P3 tests bypass the repository	add_holiday_async and update_daily_schedule_async.
P3 glossary contradiction	"A calendar day on which…"; "It is not a business day".

Spec

Finding	Fix
P2 picker/404	Same fix as above.
P3 reference has no business-day count	VisitorComparison.reference_business_days on every summary and entrance comparison (maintainer decision: carry it in the comparison). Documented in the API README.
P3 inconsistent reasons for fully closed references	Entrance previous → CLOSED_PERIOD; previous_month_daily_average_reason → CLOSED_PERIOD; the closed summary's previous carries its reference dates.
P3 busiest day/peak exclusion not really tested	The Closed Day now has the highest in/out counts; asserts busiest day = Saturday and peak = open-day max.
P3 untested figures	Egress, peak, weekend share, month best_day / daily average / previous_month_daily_average_reason, last-year reference, closed-day picker listing — all asserted.
P3 NEW-camera detection counts Closed Day events	No change: agreed with the reviewer that it's arguably correct (a camera that saw staff on a Closed Day already existed).

🤖 Generated with Claude Code

Pass-1 findings addressed in `7f1f7a6`. Full suite: 334 passed; ruff and docs check clean. ### Standards | Finding | Fix | |---|---| | P2 picker lists outage periods that 404 | Filter is now `covered_days or business_days == 0`. New `test_an_outage_week_with_a_closed_day_is_not_selectable` fails with the old filter. | | P2 precedence rule duplicated | `is_open_by_schedule(exception, weekday)` in `occupancy_service.py` is the single rule; `get_active_schedule_info_async` (daily-row `is_closed`) and `ScheduleCalendar.is_closed` both call it. | | P3 duplicated day-list comprehension | `business_dates(start, end)` module helper. | | P3 bare `(float, float)` skip spans | `skip_cycles: Sequence[CycleBounds]`; SQL built by `_skip_cycles_sql`. | | P3 closures re-read 3–4× per request | `ScheduleCalendar` is loaded once per summary / entrances / picker request and passed down (quality, comparisons, closed cycles, baseline). The baseline uses `calendar.exception_for` instead of one holiday query per candidate. | | P3 magic literals in tests | `calendar.WEDNESDAY/SATURDAY/SUNDAY`, `DAYS_PER_WEEK`, named count constants. | | P3 tests bypass the repository | `add_holiday_async` and `update_daily_schedule_async`. | | P3 glossary contradiction | "A calendar day on which…"; "It is not a business day". | ### Spec | Finding | Fix | |---|---| | P2 picker/404 | Same fix as above. | | P3 reference has no business-day count | `VisitorComparison.reference_business_days` on every summary and entrance comparison (maintainer decision: carry it in the comparison). Documented in the API README. | | P3 inconsistent reasons for fully closed references | Entrance `previous` → `CLOSED_PERIOD`; `previous_month_daily_average_reason` → `CLOSED_PERIOD`; the closed summary's `previous` carries its reference dates. | | P3 busiest day/peak exclusion not really tested | The Closed Day now has the highest in/out counts; asserts busiest day = Saturday and peak = open-day max. | | P3 untested figures | Egress, peak, weekend share, month `best_day` / daily average / `previous_month_daily_average_reason`, last-year reference, closed-day picker listing — all asserted. | | P3 NEW-camera detection counts Closed Day events | No change: agreed with the reviewer that it's arguably correct (a camera that saw staff on a Closed Day already existed). | 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg commented 2026-09-25 18:04:04 +00:00

Author

Owner

Copy link

Code review — pass 2 (two-axis)

Re-reviewed at 7f1f7a6. Each axis verified every pass-1 finding against the code, then reviewed the fix commit for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue.

Standards

Head 7f1f7a6. Ruff check/format clean; pytest 333 passed, 1 skipped (throwaway worktree).

Pass-1 findings

Finding	Verdict	Evidence
P2 picker lists outage periods that 404	✅ fixed	analytics_service.py:595 filters on covered_days or business_days == 0; regression test covers it.
P2 precedence rule duplicated	◐ partial	is_open_by_schedule (occupancy_service.py:100) holds the open-by-default rule, but "a dated exception beats the weekday schedule" is still written twice: the live path keeps its own if holiday: … return (:272-274) and calls the helper only as (holiday, None) / (None, sched) (:274, :306), never using its precedence branch. The daily series still reads is_closed from that live path, once per day (analytics_service.py:651,714); quality figures use ScheduleCalendar. Live behaviour unchanged.
P3 duplicated day list	✅ fixed	business_dates (analytics_service.py:67)
P3 bare (float, float) skip spans	✅ fixed	skip_cycles: Sequence[CycleBounds], _skip_cycles_sql (occupancy_repository.py:38)
P3 closures re-read per request	✅ fixed	One get_schedule_calendar_async per request (:235, :393, :587), passed down; baseline no longer queries holidays one by one (:131-136).
P3 hard-coded values in tests	◐ partial	calendar.* and DAYS_PER_WEEK used. Still bare: date(2026, 9, 7) (test:105), 4 * … + 2 * … (:133), timedelta(days=2) (:221), (1, 2, 3) (:299).
P3 tests bypass the repository	✅ fixed	add_holiday_async, update_daily_schedule_async (test:62-73).
P3 glossary contradiction	✅ fixed	"A calendar day on which…".

New findings

• P3 Speculative Generality / inconsistent optional parameter (analytics_service.py:537): get_statistics_period_quality_async(..., calendar: ScheduleCalendar | None = None) — every caller passes it, so the load fallback is dead code, while validate_…, statistics_visitor_comparison_async and _closed_cycles require it. Make it required. (The baseline's optional parameter at :123 is justified: :843, :1109 call without it.)
• P3 layering / type choice (occupancy_service.py:100-124): ScheduleCalendar and is_open_by_schedule are pure schedule logic in the large OccupancyManager module (Divergent Change risk). Precedent: CycleBounds in app/facility_time.py; a small sibling module fits. A NamedTuple with behaviour carries tuple semantics — the calendar or await … fallbacks (:126, :545) only work because a 2-tuple is truthy. @dataclass(frozen=True) (used in the CLI) is clearer.
• P3 Primitive Obsession (occupancy_service.py:112-113): exceptions: dict[str, dict[str, Any]] keyed by ISO strings holding raw DB rows. Key by date, keep only is_open.
• P3 tests bypass the repository (test:303-311): raw INSERT INTO occupancy_calibration_logs, though record_calibration_log_async exists (occupancy_repository.py:1686). Same pattern already on master (test_statistics_baselines.py:40).
• P3 nit (occupancy_service.py:247): return annotation "ScheduleCalendar" quoted though the class is defined above.

Merge readiness (this axis): Reviewer rates it ready (the partial P2 paths currently agree). Per policy, the partial P2 is fixed before merge; P3s → follow-up issue.

Spec

Pass-1 findings

Finding	Verdict	Evidence
P2 picker lists outage periods that 404	✅ fixed	7f1f7a6 analytics_service.py:596: covered_days or business_days == 0, matching the validator at :173. test_an_outage_week_with_a_closed_day_is_not_selectable fails when the old or closed_days filter is restored (mutation run).
P3 reference side has no business-day count	◐ partial	VisitorComparison.reference_business_days (schemas/statistics.py:121) set in statistics_visitor_comparison_async (:200-206) and both entrance comparisons (:431-446); missing on the closed summary's previous (N1).
P3 inconsistent reasons for fully closed references	✅ fixed	Entrance previous → CLOSED_PERIOD when previous_quality.business_days == 0 (:475-480); previous_month_daily_average_reason → CLOSED_PERIOD (:380-385); closed summary previous has reference_start/reference_end (:248-252). Month test fails when the branch is mutated out.
P3 busiest day / peak exclusion not really tested	✅ fixed	CLOSED_DAY_PASSAGES = 50 exceeds any open day; asserts busiest day = Saturday and peak = OPEN_WEEKEND_VISITORS (test:160-162).
P3 untested figures	✅ fixed	Egress, weekend share, peak, last-year reference (:155-166), month best_day, daily average, previous_month_daily_average_reason (:287-292), closed-day picker row (:214-215). Picker limit counting of closed periods not asserted alone (negligible).
P3 NEW-camera detection counts Closed Day events	⊘ declined-with-reason	Documented in the author's reply.

New findings

• P3 (N1): closed summary's previous has no reference_business_days (analytics_service.py:248-252). Maintainer decision: "every comparison carries the reference period's business-day count (reference_business_days)"; docs/api/README.md:132 says "Every comparison carries reference_business_days". This one comparison is built by hand, so the field is null. Low impact, but contradicts the documented contract; test :237-238 doesn't check it.
• P3 (note): usual_weekday comparisons have no reference_business_days (:334-349). Arguably fine (a sample mean with source_dates), but "every comparison" in the README is too broad: say "every period comparison" or set it to len(source_dates).

Re-hunt found no other P1/P2: summary figures all come from covered (drops is_closed rows; daily-row is_closed uses the same is_open_by_schedule rule as ScheduleCalendar); entrance current/previous/last-year rows and has_year_data pass _closed_cycles; usual-weekday samples (summary, presenter hourly, dayparts) exclude closed and exception days; picker and validator agree.

Full suite 333 passed, 1 skipped (throwaway worktree). Two mutation checks confirm the new tests catch regressions.

Merge readiness (this axis): Ready. No P1/P2; N1 is a one-line fix or a follow-up.

---

Summary: Standards: 1 partial P2 (live schedule path still writes the exception-over-weekday precedence itself) + 6 P3. Spec: no P1/P2; worst P3 is the closed summary's previous lacking reference_business_days.

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at `7f1f7a6`. Each axis verified every pass-1 finding against the code, then reviewed the fix commit for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue. ## Standards Head `7f1f7a6`. Ruff check/format clean; `pytest` 333 passed, 1 skipped (throwaway worktree). ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 picker lists outage periods that 404 | ✅ fixed | `analytics_service.py:595` filters on `covered_days or business_days == 0`; regression test covers it. | | P2 precedence rule duplicated | ◐ partial | `is_open_by_schedule` (`occupancy_service.py:100`) holds the open-by-default rule, but "a dated exception beats the weekday schedule" is still written twice: the live path keeps its own `if holiday: … return` (`:272-274`) and calls the helper only as `(holiday, None)` / `(None, sched)` (`:274`, `:306`), never using its precedence branch. The daily series still reads `is_closed` from that live path, once per day (`analytics_service.py:651,714`); quality figures use `ScheduleCalendar`. Live behaviour unchanged. | | P3 duplicated day list | ✅ fixed | `business_dates` (`analytics_service.py:67`) | | P3 bare `(float, float)` skip spans | ✅ fixed | `skip_cycles: Sequence[CycleBounds]`, `_skip_cycles_sql` (`occupancy_repository.py:38`) | | P3 closures re-read per request | ✅ fixed | One `get_schedule_calendar_async` per request (`:235`, `:393`, `:587`), passed down; baseline no longer queries holidays one by one (`:131-136`). | | P3 hard-coded values in tests | ◐ partial | `calendar.*` and `DAYS_PER_WEEK` used. Still bare: `date(2026, 9, 7)` (test:105), `4 * … + 2 * …` (:133), `timedelta(days=2)` (:221), `(1, 2, 3)` (:299). | | P3 tests bypass the repository | ✅ fixed | `add_holiday_async`, `update_daily_schedule_async` (test:62-73). | | P3 glossary contradiction | ✅ fixed | "A calendar day on which…". | ### New findings - **P3 Speculative Generality / inconsistent optional parameter** (`analytics_service.py:537`): `get_statistics_period_quality_async(..., calendar: ScheduleCalendar | None = None)` — every caller passes it, so the load fallback is dead code, while `validate_…`, `statistics_visitor_comparison_async` and `_closed_cycles` require it. Make it required. (The baseline's optional parameter at `:123` is justified: `:843`, `:1109` call without it.) - **P3 layering / type choice** (`occupancy_service.py:100-124`): `ScheduleCalendar` and `is_open_by_schedule` are pure schedule logic in the large `OccupancyManager` module (Divergent Change risk). Precedent: `CycleBounds` in `app/facility_time.py`; a small sibling module fits. A `NamedTuple` with behaviour carries tuple semantics — the `calendar or await …` fallbacks (`:126`, `:545`) only work because a 2-tuple is truthy. `@dataclass(frozen=True)` (used in the CLI) is clearer. - **P3 Primitive Obsession** (`occupancy_service.py:112-113`): `exceptions: dict[str, dict[str, Any]]` keyed by ISO strings holding raw DB rows. Key by `date`, keep only `is_open`. - **P3 tests bypass the repository** (test:303-311): raw `INSERT INTO occupancy_calibration_logs`, though `record_calibration_log_async` exists (`occupancy_repository.py:1686`). Same pattern already on master (`test_statistics_baselines.py:40`). - **P3 nit** (`occupancy_service.py:247`): return annotation `"ScheduleCalendar"` quoted though the class is defined above. **Merge readiness (this axis):** Reviewer rates it ready (the partial P2 paths currently agree). Per policy, the partial P2 is fixed before merge; P3s → follow-up issue. ## Spec ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 picker lists outage periods that 404 | ✅ fixed | `7f1f7a6` `analytics_service.py:596`: `covered_days or business_days == 0`, matching the validator at `:173`. `test_an_outage_week_with_a_closed_day_is_not_selectable` fails when the old `or closed_days` filter is restored (mutation run). | | P3 reference side has no business-day count | ◐ partial | `VisitorComparison.reference_business_days` (`schemas/statistics.py:121`) set in `statistics_visitor_comparison_async` (`:200-206`) and both entrance comparisons (`:431-446`); missing on the closed summary's `previous` (N1). | | P3 inconsistent reasons for fully closed references | ✅ fixed | Entrance `previous` → `CLOSED_PERIOD` when `previous_quality.business_days == 0` (`:475-480`); `previous_month_daily_average_reason` → `CLOSED_PERIOD` (`:380-385`); closed summary `previous` has `reference_start`/`reference_end` (`:248-252`). Month test fails when the branch is mutated out. | | P3 busiest day / peak exclusion not really tested | ✅ fixed | `CLOSED_DAY_PASSAGES = 50` exceeds any open day; asserts busiest day = Saturday and peak = `OPEN_WEEKEND_VISITORS` (test:160-162). | | P3 untested figures | ✅ fixed | Egress, weekend share, peak, last-year reference (:155-166), month `best_day`, daily average, `previous_month_daily_average_reason` (:287-292), closed-day picker row (:214-215). Picker `limit` counting of closed periods not asserted alone (negligible). | | P3 NEW-camera detection counts Closed Day events | ⊘ declined-with-reason | Documented in the author's reply. | ### New findings - **P3 (N1): closed summary's `previous` has no `reference_business_days`** (`analytics_service.py:248-252`). Maintainer decision: *"every comparison carries the reference period's business-day count (`reference_business_days`)"*; `docs/api/README.md:132` says *"Every comparison carries `reference_business_days`"*. This one comparison is built by hand, so the field is `null`. Low impact, but contradicts the documented contract; test :237-238 doesn't check it. - **P3 (note): `usual_weekday` comparisons have no `reference_business_days`** (`:334-349`). Arguably fine (a sample mean with `source_dates`), but "every comparison" in the README is too broad: say "every period comparison" or set it to `len(source_dates)`. Re-hunt found no other P1/P2: summary figures all come from `covered` (drops `is_closed` rows; daily-row `is_closed` uses the same `is_open_by_schedule` rule as `ScheduleCalendar`); entrance current/previous/last-year rows and `has_year_data` pass `_closed_cycles`; usual-weekday samples (summary, presenter hourly, dayparts) exclude closed and exception days; picker and validator agree. Full suite 333 passed, 1 skipped (throwaway worktree). Two mutation checks confirm the new tests catch regressions. **Merge readiness (this axis):** Ready. No P1/P2; N1 is a one-line fix or a follow-up. --- **Summary:** Standards: 1 partial P2 (live schedule path still writes the exception-over-weekday precedence itself) + 6 P3. Spec: no P1/P2; worst P3 is the closed summary's `previous` lacking `reference_business_days`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 18:08:27 +00:00

follow-up(statistics): P3 cleanups from #110 review (Closed Days) #111

gabogg added 1 commit 2026-09-25 18:08:28 +00:00

fix(statistics): one open/closed rule in the live schedule path ... dd2bc10088

`get_active_schedule_info_async` now reads the dated exception and the
weekday schedule together and decides open/closed through
`is_open_by_schedule`, so the exception-over-weekday precedence exists only
there. The closed summary's `previous` carries `reference_business_days`,
and the API docs scope that field to period comparisons.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

gabogg commented 2026-09-25 18:08:28 +00:00

Author

Owner

Copy link

Pass-2 blockers fixed in the latest commit: the live schedule path now decides open/closed only through is_open_by_schedule (partial P2 → fixed), and the closed summary's previous carries reference_business_days (N1). Full suite 334 passed. Remaining P3s filed as #111. Merging.

🤖 Generated with Claude Code

Pass-2 blockers fixed in the latest commit: the live schedule path now decides open/closed only through `is_open_by_schedule` (partial P2 → fixed), and the closed summary's `previous` carries `reference_business_days` (N1). Full suite 334 passed. Remaining P3s filed as #111. Merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg merged commit 2870d0406d into master 2026-09-25 18:08:35 +00:00

gabogg deleted branch feat/statistics-closed-days 2026-09-25 18:08:35 +00:00

gabogg referenced this pull request from a commit 2026-09-25 18:08:36 +00:00

feat(statistics): Closed Days are not business days (#103) (#110)

gabogg referenced this pull request 2026-09-25 18:22:56 +00:00

feat(schedule): each business day remembers the schedule it was measured under #113

gabogg referenced this pull request 2026-09-25 19:46:48 +00:00

feat(statistics): usual weekday baseline matches opening hours; configurable sampling (#105) #122

gabogg referenced this pull request 2026-09-26 10:25:45 +00:00

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

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!110

No description provided.