chore(statistics): PR #168 third-pass P3 follow-ups (camera group leftovers, busiest day wording, omitted-field schema) #220

New issue

Closed

opened 2026-10-02 23:51:47 +00:00 by gabogg · 1 comment

gabogg commented

2026-10-02 23:51:47 +00:00

Owner

Deferred P3 findings from the third review pass on #168 (period and Closed Day follow-ups, plus #200 camera_group and #201 busiest_day). None of them blocked the merge. Under the repo rule, a follow-up PR's third-pass P3s become one linked issue.

Camera Group (#200 leftovers)

Dead i18n key. occupancy.zoneLabel is still defined in EN and ES but unused. The #200 brief said "Rename the key".
- Acceptance: the key is removed, and nothing references it.
The old default returns as a fallback. The command deck uses tr('occupancy.noCameraGroup', 'General'), so a missing key shows "General" again.
- Acceptance: the fallback is the "No camera group" wording, or no fallback.
Camera inserts still write the legacy column. The camera INSERTs write a literal 'General' into zone_name, where the brief said "stops writing zone_name". The column default already covers it.
- Acceptance: the inserts omit zone_name (and #209 later drops the column).
The frontend has no test for an ungrouped camera. The brief asks for "a grouped case and an ungrouped case", but nothing asserts that "No camera group" / "Sin grupo de cámaras" is rendered by the admin list or the command deck.
- Acceptance: a Node test for each surface, in EN and ES.
Duplicated SQL. The SELECT c.*, g.resource_group_name AS camera_group … LEFT JOIN counting_camera_groups text appears 3 times in the repository.
- Acceptance: one module constant or helper.
Raw INSERT in a test. tests/test_occupancy.py writes counting_camera_groups with inline SQL, although register_camera_groups_async exists.
- Acceptance: the test uses the repository method.
Glossary wording. docs/api/README.md says "counting camera group"; the glossary term is Camera Group.
- Acceptance: the docs use the glossary term.
Unrequested response change. The camera responses now fill is_excluded and resource_group_code; before, they served the defaults. It's harmless, but not described.
- Acceptance: document it in the API docs and the PR notes, or revert it.

Busiest Day (#201 leftovers)

The definition is paraphrased. CONTEXT.md's Busiest Day doesn't use the brief's text and drops the explicit Closed Day clause: "the reliable day in a period with the most visitors; never an unreliable (excluded) day or a Closed Day".
- Acceptance: the glossary entry uses the brief's text.
Leftover "best day" names. The local variable best_day in the summary service, and "best day" in docs/architecture/rfc-statistics-deck-display-model.md.
- Acceptance: both renamed to busiest day, per the _Avoid_: best day rule.

Comparisons and the PR record

Duplicated lambda. exclude_if=lambda v: v is None is repeated 14 times in app/schemas/statistics.py. The OpenAPI schema also still marks those fields nullable, although the wire omits them.
- Acceptance: one Annotated alias, e.g. OmittedIfNone, plus one line in docs/api/README.md saying such fields are omitted rather than null.
The PR description had the omission rule backwards ("source_dates on period comparisons and reference_business_days on usual_weekday"). The code and README are right.
- Acceptance: none in code. Kept as a record.

Refs #168, #99, #111, #120, #200, #201, #209.

🤖 Generated with Claude Code

Deferred P3 findings from the third review pass on #168 (period and Closed Day follow-ups, plus #200 `camera_group` and #201 `busiest_day`). None of them blocked the merge. Under the repo rule, a follow-up PR's third-pass P3s become one linked issue. ## Camera Group (#200 leftovers) 1. **Dead i18n key.** `occupancy.zoneLabel` is still defined in EN and ES but unused. The #200 brief said "Rename the key". - *Acceptance:* the key is removed, and nothing references it. 2. **The old default returns as a fallback.** The command deck uses `tr('occupancy.noCameraGroup', 'General')`, so a missing key shows "General" again. - *Acceptance:* the fallback is the "No camera group" wording, or no fallback. 3. **Camera inserts still write the legacy column.** The camera INSERTs write a literal `'General'` into `zone_name`, where the brief said "stops writing zone_name". The column default already covers it. - *Acceptance:* the inserts omit `zone_name` (and #209 later drops the column). 4. **The frontend has no test for an ungrouped camera.** The brief asks for "a grouped case and an ungrouped case", but nothing asserts that "No camera group" / "Sin grupo de cámaras" is rendered by the admin list or the command deck. - *Acceptance:* a Node test for each surface, in EN and ES. 5. **Duplicated SQL.** The `SELECT c.*, g.resource_group_name AS camera_group … LEFT JOIN counting_camera_groups` text appears 3 times in the repository. - *Acceptance:* one module constant or helper. 6. **Raw INSERT in a test.** `tests/test_occupancy.py` writes `counting_camera_groups` with inline SQL, although `register_camera_groups_async` exists. - *Acceptance:* the test uses the repository method. 7. **Glossary wording.** `docs/api/README.md` says "counting camera group"; the glossary term is **Camera Group**. - *Acceptance:* the docs use the glossary term. 8. **Unrequested response change.** The camera responses now fill `is_excluded` and `resource_group_code`; before, they served the defaults. It's harmless, but not described. - *Acceptance:* document it in the API docs and the PR notes, or revert it. ## Busiest Day (#201 leftovers) 9. **The definition is paraphrased.** CONTEXT.md's Busiest Day doesn't use the brief's text and drops the explicit Closed Day clause: "the reliable day in a period with the most visitors; never an unreliable (excluded) day or a Closed Day". - *Acceptance:* the glossary entry uses the brief's text. 10. **Leftover "best day" names.** The local variable `best_day` in the summary service, and "best day" in `docs/architecture/rfc-statistics-deck-display-model.md`. - *Acceptance:* both renamed to busiest day, per the `_Avoid_: best day` rule. ## Comparisons and the PR record 11. **Duplicated lambda.** `exclude_if=lambda v: v is None` is repeated 14 times in `app/schemas/statistics.py`. The OpenAPI schema also still marks those fields nullable, although the wire omits them. - *Acceptance:* one `Annotated` alias, e.g. `OmittedIfNone`, plus one line in `docs/api/README.md` saying such fields are omitted rather than null. 12. **The PR description had the omission rule backwards** ("source_dates on period comparisons and reference_business_days on usual_weekday"). The code and README are right. - *Acceptance:* none in code. Kept as a record. Refs #168, #99, #111, #120, #200, #201, #209. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added the

ready-for-agent

low-priority

labels

2026-10-02 23:51:47 +00:00

gabogg referenced this issue from a pull request that will close it,

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

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

gabogg referenced this issue

2026-10-03 07:29:09 +00:00

chore(db): drop the legacy counting_cameras.zone_name column #209

gabogg commented

2026-10-03 07:29:12 +00:00

Author

Owner

Item 3 (the camera INSERTs writing 'General' into zone_name) is superseded by #209, which is now unblocked and removes the column and every reference to it. Skip item 3 here to avoid conflicting edits.

Item 3 (the camera INSERTs writing `'General'` into `zone_name`) is superseded by #209, which is now unblocked and removes the column and every reference to it. Skip item 3 here to avoid conflicting edits.

gabogg referenced this issue

2026-10-03 07:41:52 +00:00

refactor(db): remove obsolete camera zone column #234

gabogg referenced this issue from a commit

2026-10-03 07:45:12 +00:00

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

gabogg referenced this issue

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

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

gabogg referenced this issue

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

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