refactor(statistics): third-pass P3 follow-ups from PRs #167 and #168 #227

Merged

gabogg merged 3 commits from chore/statistics-p3-followups-219-220 into master 2026-10-03 12:58:31 +00:00

Conversation 3 Commits 3 Files changed 16 +234 -97

gabogg commented 2026-10-03 07:18:52 +00:00

Owner

Copy link

Resolves deferred P3 findings from the third review passes on PR #167 and PR #168.

Closes #219
Closes #220 (Note: Closing #220 auto-closes the issue; item 3 was migrated and already resolved in #209).

Summary

Issue #167 follow-ups (#219):

• Renamed _trusted_cycle_multiplier to _plausible_trusted_multiplier in app/services/analytics_service.py to prevent confusion with _trusted_cycle_multiplier_async.
• Updated multiplier helpers _plausible_trusted_multiplier, _is_trusted_log, and _trusted_multiplier to take CalibrationLogRecord rather than Mapping[str, Any].
• Split CalibrationLogRecord into a base table row TypedDict and extended it with BatchCalibrationLogRecord containing matched_cycle_date: str and rank_in_cycle: int.
• Extracted window parameter flattening in app/db/occupancy_repository.py into a shared _window_values_params(windows) helper across all four batch methods.
• Extracted duplicated EXPLAIN QUERY PLAN assertions in tests/test_statistics_periods.py into module-level _assert_indexed_plan(sql, params).
• Renamed DST test in tests/test_statistics_baselines.py to test_hourly_baseline_positional_averaging_for_duplicated_clock_labels and updated docs/audit/statistics-baseline-followups-pr-plan.md.

Issue #168 follow-ups (#220):

• Removed dead i18n key occupancy.zoneLabel from app/static/js/i18n.js (EN and ES).
• Updated command deck fallback for occupancy.noCameraGroup to use "No camera group" instead of "General".
• Item 3 skipped: legacy zone_name writes are handled in #209 (already merged) to avoid merge conflicts.
• Added Node tests for grouped and ungrouped camera display in both admin list (tests/frontend/test_admin_cameras.test.js) and command deck (tests/frontend/test_command_deck_adapter.test.js) in EN and ES, and cleaned up globals in test_admin_cameras.
• Extracted duplicated camera group SELECT query in app/db/occupancy_repository.py to module constant _SELECT_CAMERAS_WITH_GROUP_SQL.
• Replaced inline SQL in tests/test_occupancy.py with occupancy_repo.register_camera_groups_async.
• Updated API documentation in docs/api/README.md to use glossary term "Camera Group" instead of "counting camera group".
• Documented camera response inclusion of is_excluded and resource_group_code in docs/api/README.md.
• Updated CONTEXT.md Busiest Day definition to use the brief's exact text with Closed Day and multi-day scope clauses restored.
• Renamed leftover best_day variable in summary service and references in documentation to busiest_day.
• Defined _T and OmittedIfNone Annotated alias in app/schemas/statistics.py for exclude_if=lambda v: v is None and documented wire omission rule in docs/api/README.md (noting deliberate OpenAPI nullability).

Architectural Impact

• Clean deep module interfaces and typing alignment across occupancy repository, services, schemas, and tests.
• Single-row calibration log reads return accurate CalibrationLogRecord matching table schema without false casts, while batch methods return BatchCalibrationLogRecord.
• Preserves full schema backward compatibility; no breaking database or API changes.

Verification / Test Evidence

• rtk ruff check .: clean (0 issues).
• rtk ruff format --check .: clean (154 files formatted).
• python3 scripts/check_docs.py: clean (43 Markdown files and 90 HTTP operations verified).
• node --test tests/frontend/*.test.js: 250 passed, 0 failed.
• rtk pytest: 602 passed, 0 failed.

Checklist

• Issue #219 item 1: rename _trusted_cycle_multiplier -> _plausible_trusted_multiplier
• Issue #219 item 2: trusted-multiplier helpers take CalibrationLogRecord
• Issue #219 item 3: CalibrationLogRecord table columns base and BatchCalibrationLogRecord for batch reads
• Issue #219 item 4: _window_values_params helper in repository batch methods
• Issue #219 item 5: module-level _assert_indexed_plan helper in query plan tests
• Issue #219 item 6: DST test renamed to positional averaging and doc citation updated
• Issue #220 item 1: remove dead zoneLabel i18n key
• Issue #220 item 2: command deck fallback uses "No camera group"
• [-] Issue #220 item 3: skipped (handled by #209, already merged)
• Issue #220 item 4: frontend node tests for grouped and ungrouped cameras in EN and ES, clean globals
• Issue #220 item 5: extract _SELECT_CAMERAS_WITH_GROUP_SQL constant
• Issue #220 item 6: use register_camera_groups_async in test
• Issue #220 item 7: glossary term Camera Group used in docs
• Issue #220 item 8: document is_excluded and resource_group_code camera response fields
• Issue #220 item 9: CONTEXT.md Busiest Day definition uses exact brief text with restored clauses
• Issue #220 item 10: rename leftover best_day to busiest_day
• Issue #220 item 11: OmittedIfNone Annotated alias in statistics schemas and documented wire omission (OpenAPI nullability deliberate)
• ruff check & format clean
• scripts/check_docs.py clean
• full pytest suite pass (602 passed)
• frontend node tests pass (250 passed)

Resolves deferred P3 findings from the third review passes on PR #167 and PR #168. Closes #219 Closes #220 (Note: Closing #220 auto-closes the issue; item 3 was migrated and already resolved in #209). ### Summary **Issue #167 follow-ups (#219):** - Renamed `_trusted_cycle_multiplier` to `_plausible_trusted_multiplier` in `app/services/analytics_service.py` to prevent confusion with `_trusted_cycle_multiplier_async`. - Updated multiplier helpers `_plausible_trusted_multiplier`, `_is_trusted_log`, and `_trusted_multiplier` to take `CalibrationLogRecord` rather than `Mapping[str, Any]`. - Split `CalibrationLogRecord` into a base table row `TypedDict` and extended it with `BatchCalibrationLogRecord` containing `matched_cycle_date: str` and `rank_in_cycle: int`. - Extracted window parameter flattening in `app/db/occupancy_repository.py` into a shared `_window_values_params(windows)` helper across all four batch methods. - Extracted duplicated EXPLAIN QUERY PLAN assertions in `tests/test_statistics_periods.py` into module-level `_assert_indexed_plan(sql, params)`. - Renamed DST test in `tests/test_statistics_baselines.py` to `test_hourly_baseline_positional_averaging_for_duplicated_clock_labels` and updated `docs/audit/statistics-baseline-followups-pr-plan.md`. **Issue #168 follow-ups (#220):** - Removed dead i18n key `occupancy.zoneLabel` from `app/static/js/i18n.js` (EN and ES). - Updated command deck fallback for `occupancy.noCameraGroup` to use "No camera group" instead of "General". - Item 3 skipped: legacy `zone_name` writes are handled in #209 (already merged) to avoid merge conflicts. - Added Node tests for grouped and ungrouped camera display in both admin list (`tests/frontend/test_admin_cameras.test.js`) and command deck (`tests/frontend/test_command_deck_adapter.test.js`) in EN and ES, and cleaned up globals in `test_admin_cameras`. - Extracted duplicated camera group SELECT query in `app/db/occupancy_repository.py` to module constant `_SELECT_CAMERAS_WITH_GROUP_SQL`. - Replaced inline SQL in `tests/test_occupancy.py` with `occupancy_repo.register_camera_groups_async`. - Updated API documentation in `docs/api/README.md` to use glossary term "Camera Group" instead of "counting camera group". - Documented camera response inclusion of `is_excluded` and `resource_group_code` in `docs/api/README.md`. - Updated `CONTEXT.md` Busiest Day definition to use the brief's exact text with Closed Day and multi-day scope clauses restored. - Renamed leftover `best_day` variable in summary service and references in documentation to `busiest_day`. - Defined `_T` and `OmittedIfNone` Annotated alias in `app/schemas/statistics.py` for `exclude_if=lambda v: v is None` and documented wire omission rule in `docs/api/README.md` (noting deliberate OpenAPI nullability). ### Architectural Impact - Clean deep module interfaces and typing alignment across occupancy repository, services, schemas, and tests. - Single-row calibration log reads return accurate `CalibrationLogRecord` matching table schema without false casts, while batch methods return `BatchCalibrationLogRecord`. - Preserves full schema backward compatibility; no breaking database or API changes. ### Verification / Test Evidence - `rtk ruff check .`: clean (0 issues). - `rtk ruff format --check .`: clean (154 files formatted). - `python3 scripts/check_docs.py`: clean (43 Markdown files and 90 HTTP operations verified). - `node --test tests/frontend/*.test.js`: 250 passed, 0 failed. - `rtk pytest`: 602 passed, 0 failed. ### Checklist - [x] Issue #219 item 1: rename `_trusted_cycle_multiplier` -> `_plausible_trusted_multiplier` - [x] Issue #219 item 2: trusted-multiplier helpers take `CalibrationLogRecord` - [x] Issue #219 item 3: `CalibrationLogRecord` table columns base and `BatchCalibrationLogRecord` for batch reads - [x] Issue #219 item 4: `_window_values_params` helper in repository batch methods - [x] Issue #219 item 5: module-level `_assert_indexed_plan` helper in query plan tests - [x] Issue #219 item 6: DST test renamed to positional averaging and doc citation updated - [x] Issue #220 item 1: remove dead `zoneLabel` i18n key - [x] Issue #220 item 2: command deck fallback uses "No camera group" - [-] Issue #220 item 3: skipped (handled by #209, already merged) - [x] Issue #220 item 4: frontend node tests for grouped and ungrouped cameras in EN and ES, clean globals - [x] Issue #220 item 5: extract `_SELECT_CAMERAS_WITH_GROUP_SQL` constant - [x] Issue #220 item 6: use `register_camera_groups_async` in test - [x] Issue #220 item 7: glossary term Camera Group used in docs - [x] Issue #220 item 8: document `is_excluded` and `resource_group_code` camera response fields - [x] Issue #220 item 9: `CONTEXT.md` Busiest Day definition uses exact brief text with restored clauses - [x] Issue #220 item 10: rename leftover `best_day` to `busiest_day` - [x] Issue #220 item 11: `OmittedIfNone` Annotated alias in statistics schemas and documented wire omission (OpenAPI nullability deliberate) - [x] ruff check & format clean - [x] scripts/check_docs.py clean - [x] full pytest suite pass (602 passed) - [x] frontend node tests pass (250 passed)

gabogg added the

ready-for-agent

label 2026-10-03 07:18:55 +00:00

gabogg added 1 commit 2026-10-03 07:45:12 +00:00

chore(statistics): address third-pass P3 follow-ups from PRs #167 and #168 ... bd835b7e86

- Rename _trusted_cycle_multiplier to _plausible_trusted_multiplier and type with CalibrationLogRecord (closes #219)
- Add rank_in_cycle to CalibrationLogRecord and type is_trusted as int | None (closes #219)
- Extract _cycle_values_params helper across occupancy repository batch queries (closes #219)
- Unify EXPLAIN QUERY PLAN assertions into _assert_indexed_plan helper (closes #219)
- Rename hourly baseline DST fallback test to positional averaging (closes #219)
- Remove dead zoneLabel i18n key and update command deck fallback (closes #220)
- Add Node tests for grouped and ungrouped cameras in EN and ES on admin list and command deck (closes #220)
- Extract _SELECT_CAMERAS_WITH_GROUP_SQL module constant in occupancy repository (closes #220)
- Use register_camera_groups_async in test_occupancy (closes #220)
- Align Camera Group and Busiest Day glossary wording in CONTEXT.md and docs (closes #220)
- Rename leftover best_day variable and documentation references to busiest day (closes #220)
- Introduce OmittedIfNone Annotated type alias in statistics schemas and document in API docs (closes #220)

gabogg changed title from WIP: chore(statistics): third-pass P3 follow-ups from PRs #167 and #168 to chore(statistics): third-pass P3 follow-ups from PRs #167 and #168 2026-10-03 07:46:07 +00:00

gabogg commented 2026-10-03 08:04:47 +00:00

Author

Owner

Copy link

Please perform code review pass 1 of master...bd835b7 against issues #219 and #220.

Scope:

• PR #219 items 1–6 (all items; item 7 is record-only):
  • 1: Rename _trusted_cycle_multiplier -> _plausible_trusted_multiplier in app/services/analytics_service.py.
  • 2: Update multiplier helpers (_plausible_trusted_multiplier, _is_trusted_log, _trusted_multiplier) to take CalibrationLogRecord rather than Mapping[str, Any].
  • 3: Update CalibrationLogRecord in app/schemas/occupancy_models.py with rank_in_cycle: int and nullable is_trusted: int | None.
  • 4: Extract _cycle_values_params(cycles) in app/db/occupancy_repository.py for cycle parameter flattening across all four batch queries.
  • 5: Extract _assert_indexed_plan(sql, params) in tests/test_statistics_periods.py.
  • 6: Rename DST test in tests/test_statistics_baselines.py to test_hourly_baseline_positional_averaging_for_duplicated_clock_labels and update citation in docs/audit/statistics-baseline-followups-pr-plan.md.
• PR #220 items 1–2, 4–11 (item 3 skipped per maintainer instruction, handled by #209; item 12 is record-only):
  • 1: Remove dead key occupancy.zoneLabel from app/static/js/i18n.js (EN/ES).
  • 2: Update command deck fallback for occupancy.noCameraGroup to use "No camera group" in app/static/js/src/ui/command_deck_adapter.js.
  • 4: Add Node tests for grouped and ungrouped cameras in both admin cameras list and command deck in EN and ES.
  • 5: Extract _SELECT_CAMERAS_WITH_GROUP_SQL constant in app/db/occupancy_repository.py.
  • 6: Use register_camera_groups_async in tests/test_occupancy.py instead of raw SQL insert.
  • 7: Standardize documentation on "Camera Group" in docs/api/README.md.
  • 8: Document is_excluded and resource_group_code camera response fields in docs/api/README.md.
  • 9: Restore exact brief wording with Closed Day clause for Busiest Day in CONTEXT.md.
  • 10: Rename leftover best_day to busiest_day in summary service and docs/architecture/rfc-statistics-deck-display-model.md.
  • 11: Define OmittedIfNone Annotated alias in app/schemas/statistics.py and document in docs/api/README.md.

Validation passed:

• rtk ruff check .: clean (0 issues)
• rtk ruff format --check .: clean (150 files formatted)
• python3 scripts/check_docs.py: clean (42 Markdown files, 80 HTTP operations)
• node --test tests/frontend/*.test.js: 244 passed, 0 failed
• rtk pytest: 534 passed, 0 failed
• CI: Successful (run 365)

Record the completed review as a formal review using scripts/wt review post 227, headed:

Code review, pass 1 (master...bd835b7, spec #219, #220)

This addresses findings from prior reviews (#167 and #168), so the repository's three-pass follow-up review policy applies before merge.

Please perform code review pass 1 of master...bd835b7 against issues #219 and #220. Scope: - PR #219 items 1–6 (all items; item 7 is record-only): - 1: Rename `_trusted_cycle_multiplier` -> `_plausible_trusted_multiplier` in `app/services/analytics_service.py`. - 2: Update multiplier helpers (`_plausible_trusted_multiplier`, `_is_trusted_log`, `_trusted_multiplier`) to take `CalibrationLogRecord` rather than `Mapping[str, Any]`. - 3: Update `CalibrationLogRecord` in `app/schemas/occupancy_models.py` with `rank_in_cycle: int` and nullable `is_trusted: int | None`. - 4: Extract `_cycle_values_params(cycles)` in `app/db/occupancy_repository.py` for cycle parameter flattening across all four batch queries. - 5: Extract `_assert_indexed_plan(sql, params)` in `tests/test_statistics_periods.py`. - 6: Rename DST test in `tests/test_statistics_baselines.py` to `test_hourly_baseline_positional_averaging_for_duplicated_clock_labels` and update citation in `docs/audit/statistics-baseline-followups-pr-plan.md`. - PR #220 items 1–2, 4–11 (item 3 skipped per maintainer instruction, handled by #209; item 12 is record-only): - 1: Remove dead key `occupancy.zoneLabel` from `app/static/js/i18n.js` (EN/ES). - 2: Update command deck fallback for `occupancy.noCameraGroup` to use "No camera group" in `app/static/js/src/ui/command_deck_adapter.js`. - 4: Add Node tests for grouped and ungrouped cameras in both admin cameras list and command deck in EN and ES. - 5: Extract `_SELECT_CAMERAS_WITH_GROUP_SQL` constant in `app/db/occupancy_repository.py`. - 6: Use `register_camera_groups_async` in `tests/test_occupancy.py` instead of raw SQL insert. - 7: Standardize documentation on "Camera Group" in `docs/api/README.md`. - 8: Document `is_excluded` and `resource_group_code` camera response fields in `docs/api/README.md`. - 9: Restore exact brief wording with Closed Day clause for Busiest Day in `CONTEXT.md`. - 10: Rename leftover `best_day` to `busiest_day` in summary service and `docs/architecture/rfc-statistics-deck-display-model.md`. - 11: Define `OmittedIfNone` Annotated alias in `app/schemas/statistics.py` and document in `docs/api/README.md`. Validation passed: - `rtk ruff check .`: clean (0 issues) - `rtk ruff format --check .`: clean (150 files formatted) - `python3 scripts/check_docs.py`: clean (42 Markdown files, 80 HTTP operations) - `node --test tests/frontend/*.test.js`: 244 passed, 0 failed - `rtk pytest`: 534 passed, 0 failed - CI: Successful (run 365) Record the completed review as a formal review using `scripts/wt review post 227`, headed: ## Code review, pass 1 (master...bd835b7, spec #219, #220) This addresses findings from prior reviews (#167 and #168), so the repository's three-pass follow-up review policy applies before merge.

gabogg reviewed 2026-10-03 11:26:46 +00:00

gabogg left a comment

Copy link

Code review, pass 1 (origin/master...bd835b7, spec #219 + #220)

Result: 1 P2 (raised on both axes) and 15 P3s. Not mergeable yet. This is pass 1, so fix every finding, merge origin/master (now 9a72ff4) and request a second pass.

• Good news: every item in #219 and #220 is implemented, and #220 item 3 is correctly skipped. The PR doesn't touch zone_name, so it doesn't clash with #234.
• Checks on the merge with 9a72ff4 (it merges cleanly):
  • ruff is clean.
  • 107 statistics and occupancy tests pass.
  • The node tests pass: 250 of 250, including the 24 new ones.

Spec

Per-item status

• #219: items 1–6 are done. Items 3 and 4 are verified against the schema columns and all four _cycle_values_params call sites.
• #220: items 1, 2 and 4–11 are done, and item 3 is skipped. Items 7 and 10 are verified by grep: no "counting camera group" or "best day" is left in the docs or code.

P2

• P2-1. CalibrationLogRecord is a false type on the single-row path (occupancy_repository.py ~2564/2576 on the merge result; raised on both axes). get_calibration_log_for_cycle_async now returns cast(CalibrationLogRecord, dict(row)) from a SELECT * that has neither matched_cycle_date nor rank_in_cycle, yet the TypedDict marks both as required (occupancy_models.py:82-83). The type checker would accept log["rank_in_cycle"] and the code would raise KeyError at runtime. That undercuts #219 item 3 ("the record matches every column the query returns").
  • Fix: a base TypedDict with only the table columns, used by the single-row path, and the batch record extending it with the two query-only keys. NotRequired on those keys also works.

P3

1. #220 item 11 README wording. It points readers at "fields marked OmittedIfNone", which is a Python-internal name. Describe the rule in wire terms, or list the fields. Also note in the PR that OpenAPI still marks those fields nullable, and that this is deliberate.

2. The Busiest Day text in CONTEXT.md lost two clauses by copying the brief verbatim:

   • the scope "multi-day period (week or month)", since the Day view has no busiest day;
   • the rule "no busiest day is named when every covered day is unreliable or closed".

   Default: restore both after the verbatim text, unless the maintainer says otherwise on this PR.

3. module.exports exports renderCountingCamerasList with no test using it (app.js:4504-4510). Test it or drop the export. Also add a one-line comment explaining why app.js has the localStorage guard and the export block (Node test loading, the same as i18n.js:2372).

4. tests/frontend/test_admin_cameras.test.js leaks globals (:18-26, 73-77). It sets globalThis.window and globalThis.localStorage, but finally deletes only document and t, and app.js stays in the require cache. Clean up every global it sets.

5. Closes #220 will auto-close #220 while item 3 lives on in #209, which is already merged. Fine as is, but say so in the PR body.

Standards

P2

• P2-1 (same finding): code-standards §3 requires structured data to be typed truthfully.

P3

6. Wrong branch type. In git-and-workflow.md §1, chore/ is for dependencies, CI and tool config. This PR is refactor/ work. Use refactor(statistics): in the PR title and new commits; the branch name can stay.
7. bd835b7 has no Co-Authored-By trailer.
8. The PR body's checklist says "PR #219 item N" and "PR #220 item N". These are issues, not PRs.
9. Mysterious Name: _cycle_values_params(cycles) (occupancy_repository.py:88). It is also called with counting windows (:1738). Rename it, for example to _window_values_params(windows).
10. Mysterious Name: a public T = TypeVar("T") in app/schemas/statistics.py:11. Rename it _T.
11. _assert_indexed_plan is nested inside the test (tests/test_statistics_periods.py:577). Move it to module level, which fits #219 item 5's "one helper" better.

Not raised: keeping CalibrationLogRecord as a TypedDict is fine, since it predates this PR and holds internal DB rows rather than API payloads.

## Code review, pass 1 (`origin/master...bd835b7`, spec #219 + #220) Result: **1 P2 (raised on both axes) and 15 P3s. Not mergeable yet.** This is pass 1, so fix every finding, merge `origin/master` (now `9a72ff4`) and request a **second pass**. - **Good news:** every item in #219 and #220 is implemented, and #220 item 3 is correctly skipped. The PR doesn't touch `zone_name`, so it doesn't clash with #234. - **Checks on the merge with `9a72ff4`** (it merges cleanly): - ruff is clean. - 107 statistics and occupancy tests pass. - The node tests pass: 250 of 250, including the 24 new ones. ## Spec ### Per-item status - **#219:** items 1–6 are done. Items 3 and 4 are verified against the schema columns and all four `_cycle_values_params` call sites. - **#220:** items 1, 2 and 4–11 are done, and item 3 is skipped. Items 7 and 10 are verified by grep: no "counting camera group" or "best day" is left in the docs or code. ### P2 - **P2-1. `CalibrationLogRecord` is a false type on the single-row path** (`occupancy_repository.py` ~2564/2576 on the merge result; raised on both axes). `get_calibration_log_for_cycle_async` now returns `cast(CalibrationLogRecord, dict(row))` from a `SELECT *` that has neither `matched_cycle_date` nor `rank_in_cycle`, yet the TypedDict marks both as required (`occupancy_models.py:82-83`). The type checker would accept `log["rank_in_cycle"]` and the code would raise `KeyError` at runtime. That undercuts #219 item 3 (*"the record matches every column the query returns"*). - Fix: a base TypedDict with only the table columns, used by the single-row path, and the batch record extending it with the two query-only keys. `NotRequired` on those keys also works. ### P3 1. **#220 item 11 README wording.** It points readers at "fields marked `OmittedIfNone`", which is a Python-internal name. Describe the rule in wire terms, or list the fields. Also note in the PR that OpenAPI still marks those fields nullable, and that this is deliberate. 2. **The `Busiest Day` text in CONTEXT.md lost two clauses** by copying the brief verbatim: - the scope "multi-day period (week or month)", since the Day view has no busiest day; - the rule "no busiest day is named when every covered day is unreliable or closed". **Default: restore both** after the verbatim text, unless the maintainer says otherwise on this PR. 3. **`module.exports` exports `renderCountingCamerasList` with no test using it** (`app.js:4504-4510`). Test it or drop the export. Also add a one-line comment explaining why `app.js` has the `localStorage` guard and the export block (Node test loading, the same as `i18n.js:2372`). 4. **`tests/frontend/test_admin_cameras.test.js` leaks globals** (:18-26, 73-77). It sets `globalThis.window` and `globalThis.localStorage`, but `finally` deletes only `document` and `t`, and app.js stays in the require cache. Clean up every global it sets. 5. **`Closes #220` will auto-close #220 while item 3 lives on in #209**, which is already merged. Fine as is, but say so in the PR body. ## Standards ### P2 - **P2-1 (same finding):** code-standards §3 requires structured data to be typed truthfully. ### P3 6. **Wrong branch type.** In git-and-workflow.md §1, `chore/` is for dependencies, CI and tool config. This PR is `refactor/` work. Use `refactor(statistics):` in the PR title and new commits; the branch name can stay. 7. **`bd835b7` has no `Co-Authored-By` trailer.** 8. **The PR body's checklist says "PR #219 item N" and "PR #220 item N".** These are issues, not PRs. 9. **Mysterious Name: `_cycle_values_params(cycles)`** (`occupancy_repository.py:88`). It is also called with counting windows (`:1738`). Rename it, for example to `_window_values_params(windows)`. 10. **Mysterious Name: a public `T = TypeVar("T")` in `app/schemas/statistics.py:11`.** Rename it `_T`. 11. **`_assert_indexed_plan` is nested inside the test** (`tests/test_statistics_periods.py:577`). Move it to module level, which fits #219 item 5's "one helper" better. Not raised: keeping `CalibrationLogRecord` as a TypedDict is fine, since it predates this PR and holds internal DB rows rather than API payloads.

gabogg referenced this pull request from a commit 2026-10-03 12:15:26 +00:00

refactor(statistics): address pass 1 review findings for PR #227

gabogg added 2 commits 2026-10-03 12:15:26 +00:00

Merge remote-tracking branch 'origin/master' into chore/statistics-p3-followups-219-220 e875a55288

refactor(statistics): address pass 1 review findings for PR #227 ... a96f6bfc4b

- Split CalibrationLogRecord into base table row TypedDict and BatchCalibrationLogRecord with matched_cycle_date and rank_in_cycle
- Remove false cast on single-row get_calibration_log_for_cycle_async
- Restore multi-day scope and closed/unreliable period clauses for Busiest Day in CONTEXT.md
- Describe statistics omission rule in wire terms in docs/api/README.md and note deliberate OpenAPI nullability
- Drop unused renderCountingCamerasList export and document Node test loading guards in app.js
- Clean up leaked globals and require cache in tests/frontend/test_admin_cameras.test.js
- Rename _cycle_values_params to _window_values_params across batch queries
- Rename TypeVar T to _T in app/schemas/statistics.py
- Move _assert_indexed_plan helper to module level in tests/test_statistics_periods.py

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

gabogg changed title from chore(statistics): third-pass P3 follow-ups from PRs #167 and #168 to refactor(statistics): third-pass P3 follow-ups from PRs #167 and #168 2026-10-03 12:15:57 +00:00

gabogg commented 2026-10-03 12:16:14 +00:00

Author

Owner

Copy link

Pass 1 fixes (a96f6bf)

All P2 and P3 findings from review pass 1 (r44) have been resolved. origin/master (9a72ff4) is merged cleanly into the branch.

Finding	Severity	Description	Fix
P2-1	P2 (Spec & Standards)	CalibrationLogRecord required query-only keys (matched_cycle_date, rank_in_cycle), making single-row query a false cast.	Split into base CalibrationLogRecord (table columns) and BatchCalibrationLogRecord (with matched_cycle_date and rank_in_cycle). Single-row returns truthfully typed CalibrationLogRecord.
P3-1	P3 (Spec)	README wording used internal Python name OmittedIfNone.	Described omission behavior in wire terms in docs/api/README.md and noted that OpenAPI nullability is deliberate.
P3-2	P3 (Spec)	Busiest Day definition in CONTEXT.md lost multi-day scope and closed/unreliable period clauses.	Restored both clauses in CONTEXT.md following the brief's verbatim text.
P3-3	P3 (Spec)	Unused renderCountingCamerasList export in app.js; missing comments on Node-loading guards.	Dropped renderCountingCamerasList from module.exports and added explanatory comments on Node test loading for localStorage guard and exports.
P3-4	P3 (Spec)	tests/frontend/test_admin_cameras.test.js leaked globals and app.js in require.cache.	Cleaned up window, localStorage, document, t, and purged app.js from require.cache in finally.
P3-5	P3 (Spec)	Auto-closing #220 while item 3 lives on in #209.	Noted in PR body and checklist that #220 item 3 was migrated and resolved in #209.
P3-6	P3 (Standards)	Branch type prefix in PR title (chore/ instead of refactor/).	Updated PR title to refactor(statistics): ... and used refactor(statistics): prefix for new commits.
P3-7	P3 (Standards)	Missing Co-Authored-By trailer on earlier commit.	Added Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> trailer to fix commit.
P3-8	P3 (Standards)	PR body checklist referred to "PR #219" and "PR #220".	Corrected PR body and checklist to reference Issue #219 and Issue #220.
P3-9	P3 (Standards)	Mysterious Name _cycle_values_params(cycles) also used for counting windows.	Renamed to _window_values_params(windows) across definition and all four call sites.
P3-10	P3 (Standards)	Mysterious Name: public T = TypeVar("T") in app/schemas/statistics.py.	Renamed to private _T = TypeVar("_T").
P3-11	P3 (Standards)	Nested _assert_indexed_plan inside test function.	Moved _assert_indexed_plan to module level in tests/test_statistics_periods.py.

Ready for review pass 2.

## Pass 1 fixes (a96f6bf) All P2 and P3 findings from review pass 1 (r44) have been resolved. `origin/master` (9a72ff4) is merged cleanly into the branch. | Finding | Severity | Description | Fix | |---|---|---|---| | P2-1 | P2 (Spec & Standards) | `CalibrationLogRecord` required query-only keys (`matched_cycle_date`, `rank_in_cycle`), making single-row query a false cast. | Split into base `CalibrationLogRecord` (table columns) and `BatchCalibrationLogRecord` (with `matched_cycle_date` and `rank_in_cycle`). Single-row returns truthfully typed `CalibrationLogRecord`. | | P3-1 | P3 (Spec) | README wording used internal Python name `OmittedIfNone`. | Described omission behavior in wire terms in `docs/api/README.md` and noted that OpenAPI nullability is deliberate. | | P3-2 | P3 (Spec) | `Busiest Day` definition in `CONTEXT.md` lost multi-day scope and closed/unreliable period clauses. | Restored both clauses in `CONTEXT.md` following the brief's verbatim text. | | P3-3 | P3 (Spec) | Unused `renderCountingCamerasList` export in `app.js`; missing comments on Node-loading guards. | Dropped `renderCountingCamerasList` from `module.exports` and added explanatory comments on Node test loading for `localStorage` guard and exports. | | P3-4 | P3 (Spec) | `tests/frontend/test_admin_cameras.test.js` leaked globals and `app.js` in `require.cache`. | Cleaned up `window`, `localStorage`, `document`, `t`, and purged `app.js` from `require.cache` in `finally`. | | P3-5 | P3 (Spec) | Auto-closing #220 while item 3 lives on in #209. | Noted in PR body and checklist that #220 item 3 was migrated and resolved in #209. | | P3-6 | P3 (Standards) | Branch type prefix in PR title (`chore/` instead of `refactor/`). | Updated PR title to `refactor(statistics): ...` and used `refactor(statistics):` prefix for new commits. | | P3-7 | P3 (Standards) | Missing `Co-Authored-By` trailer on earlier commit. | Added `Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>` trailer to fix commit. | | P3-8 | P3 (Standards) | PR body checklist referred to "PR #219" and "PR #220". | Corrected PR body and checklist to reference Issue #219 and Issue #220. | | P3-9 | P3 (Standards) | Mysterious Name `_cycle_values_params(cycles)` also used for counting windows. | Renamed to `_window_values_params(windows)` across definition and all four call sites. | | P3-10 | P3 (Standards) | Mysterious Name: public `T = TypeVar("T")` in `app/schemas/statistics.py`. | Renamed to private `_T = TypeVar("_T")`. | | P3-11 | P3 (Standards) | Nested `_assert_indexed_plan` inside test function. | Moved `_assert_indexed_plan` to module level in `tests/test_statistics_periods.py`. | Ready for review pass 2.

gabogg referenced this pull request 2026-10-03 12:55:32 +00:00

test(frontend): harden admin-cameras app.js loading and fix README omitted-field note #268

gabogg referenced this pull request 2026-10-03 12:55:33 +00:00

fix(calibration): per-cycle calibration log lookup can return a neighbouring cycle's log #127

gabogg reviewed 2026-10-03 12:55:53 +00:00

gabogg left a comment

Copy link

Code review, pass 2 (origin/master...a96f6bf, spec #219 + #220)

Result: no P1, no P2 and 4 P3s. Mergeable. The new P3s are filed as #268. A pre-existing boundary mismatch went to #127, and the commit-history items are accepted as they are.

• Checks:
  • ruff is clean.
  • 58 targeted tests pass (occupancy, statistics periods and baselines, calibration audit trust).
  • The node tests pass 250 of 250.
• Every item in #219 and #220 is still met.

r44	Status
P2-1 false CalibrationLogRecord type	FIXED. The base type has exactly the 26 table columns, BatchCalibrationLogRecord adds the two query-only keys, and the single-row path uses the base type. is_trusted is nullable, like the column.
P3-1 README wording, P3-2 Busiest Day clauses, P3-3 export and guard comments, P3-4 leaked globals, P3-5 auto-close note, P3-8 checklist, P3-9 _window_values_params, P3-10 _T, P3-11 module-level helper	FIXED
P3-6 commit type	PARTIAL. The PR title and a96f6bf use refactor(statistics):, but bd835b7 still says chore(statistics):. Accepted: not worth rewriting history for.
P3-7 Co-Authored-By on bd835b7	NOT FIXED. Accepted for the same reason.

New P3 → #268

• test_admin_cameras.test.js:20-37 sets globals and loads app.js before the try, so if loading throws, the globals leak.
• The typeof localStorage guard at app.js:16-17 never runs in tests, because the only test stubs localStorage first.
• The README says previous_month_daily_average is "always present", but it is omitted when null. This predates the PR.

Pre-existing → #127

The single-row calibration-log lookup's window end is inclusive (<=), while the batched lookup's is exclusive (<). The two disagree on a log stamped exactly at a cycle boundary, and the audit doc claims they match.

## Code review, pass 2 (`origin/master...a96f6bf`, spec #219 + #220) Result: **no P1, no P2 and 4 P3s. Mergeable.** The new P3s are filed as #268. A pre-existing boundary mismatch went to #127, and the commit-history items are accepted as they are. - **Checks:** - ruff is clean. - 58 targeted tests pass (occupancy, statistics periods and baselines, calibration audit trust). - The node tests pass 250 of 250. - Every item in #219 and #220 is still met. | r44 | Status | |---|---| | P2-1 false `CalibrationLogRecord` type | **FIXED**. The base type has exactly the 26 table columns, `BatchCalibrationLogRecord` adds the two query-only keys, and the single-row path uses the base type. `is_trusted` is nullable, like the column. | | P3-1 README wording, P3-2 Busiest Day clauses, P3-3 export and guard comments, P3-4 leaked globals, P3-5 auto-close note, P3-8 checklist, P3-9 `_window_values_params`, P3-10 `_T`, P3-11 module-level helper | FIXED | | P3-6 commit type | PARTIAL. The PR title and `a96f6bf` use `refactor(statistics):`, but `bd835b7` still says `chore(statistics):`. **Accepted:** not worth rewriting history for. | | P3-7 `Co-Authored-By` on `bd835b7` | NOT FIXED. **Accepted** for the same reason. | ### New P3 → #268 - `test_admin_cameras.test.js:20-37` sets globals and loads app.js before the `try`, so if loading throws, the globals leak. - The `typeof localStorage` guard at `app.js:16-17` never runs in tests, because the only test stubs `localStorage` first. - The README says `previous_month_daily_average` is "always present", but it is omitted when null. This predates the PR. ### Pre-existing → #127 The single-row calibration-log lookup's window end is inclusive (`<=`), while the batched lookup's is exclusive (`<`). The two disagree on a log stamped exactly at a cycle boundary, and the audit doc claims they match.

gabogg merged commit 9a608c115a into master 2026-10-03 12:58:31 +00:00

gabogg deleted branch chore/statistics-p3-followups-219-220 2026-10-03 12:58:31 +00:00

gabogg referenced this pull request from a commit 2026-10-03 12:58:32 +00:00

refactor(statistics): third-pass P3 follow-ups from PRs #167 and #168 (#227)

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

No description provided.