gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 113 Pull requests 8 Projects Releases Packages Wiki Activity Actions

feat(schedule): complete schedule follow-ups #228

Open
gabogg wants to merge 7 commits from chore/schedule-p3-followups-186 into master
pull from: chore/schedule-p3-followups-186
merge into: gabogg:master
Conversation 10 Commits 7 Files changed 17 +738 -88
gabogg commented 2026-10-03 07:21:28 +00:00
Owner
Copy link

Completes the schedule follow-ups in #186. Completed days now keep their schedule through outages and later weekly edits, and calibration reads an explicitly stored Original Schedule that corrections and overwrites preserve.

Architectural impact:

  • ScheduleAuditAction defines the action vocabulary for schemas, repository writers and generated CHECKs. A validated CHECK helper handles sources and actions; the generic action migration rebuilds outdated constraints while retaining rows, indexes, triggers and the audit ID sequence.
  • Schedule records store original_value JSON. The atomic upgrade backfills the oldest audit old/new value, or the record when unaudited. The upgrade runs once so future imported drafts can remain unfixed until approval. Decoding preserves the stored recorded flag.
  • Default stamping covers every completed business day from the earliest flow data across all cameras through yesterday. The monitor backfills missed days on startup and business-day transitions without overwriting existing records.
  • Nameless exceptions retain empty names; the frontend supplies the localized EN/ES default. Service docstrings, stamping help and API documentation describe the behavior.

Coordination: shared write-path invariants posted on PR #178; origin/master refreshed during implementation.

Verification:

  • Ruff lint and format checks passed.
  • python scripts/check_docs.py passed (42 Markdown files, 80 HTTP operations).
  • Full pytest suite passed through the repository commit hook (544 tests); 19 targeted schedule/migration and legacy-default tests also passed.
  • Frontend suite passed (244 tests).

Checklist:

  • Implement all eight triage decisions
  • Add migration, original-value, monitor recovery and localization regressions
  • Update stamping help and API documentation
  • Full pytest
  • First review pass

Closes #186

Completes the schedule follow-ups in #186. Completed days now keep their schedule through outages and later weekly edits, and calibration reads an explicitly stored Original Schedule that corrections and overwrites preserve. Architectural impact: - `ScheduleAuditAction` defines the action vocabulary for schemas, repository writers and generated CHECKs. A validated CHECK helper handles sources and actions; the generic action migration rebuilds outdated constraints while retaining rows, indexes, triggers and the audit ID sequence. - Schedule records store `original_value` JSON. The atomic upgrade backfills the oldest audit old/new value, or the record when unaudited. The upgrade runs once so future imported drafts can remain unfixed until approval. Decoding preserves the stored `recorded` flag. - Default stamping covers every completed business day from the earliest flow data across all cameras through yesterday. The monitor backfills missed days on startup and business-day transitions without overwriting existing records. - Nameless exceptions retain empty names; the frontend supplies the localized EN/ES default. Service docstrings, stamping help and API documentation describe the behavior. Coordination: shared write-path invariants posted on PR #178; origin/master refreshed during implementation. Verification: - Ruff lint and format checks passed. - `python scripts/check_docs.py` passed (42 Markdown files, 80 HTTP operations). - Full pytest suite passed through the repository commit hook (544 tests); 19 targeted schedule/migration and legacy-default tests also passed. - Frontend suite passed (244 tests). Checklist: - [x] Implement all eight triage decisions - [x] Add migration, original-value, monitor recovery and localization regressions - [x] Update stamping help and API documentation - [x] Full pytest - [ ] First review pass Closes #186
gabogg changed title from WIP: chore(schedule): complete schedule follow-ups to chore(schedule): complete schedule follow-ups 2026-10-03 07:37:53 +00:00
chore(schedule): persist original schedules and complete #186 follow-ups
All checks were successful
CI / lint-and-test (pull_request) Successful in 2m41s
Details
87e3bb4777
gabogg commented 2026-10-03 07:38:21 +00:00
Author
Owner
Copy link

Requesting code review pass 1 for 016a012...87e3bb4, spec #186. All eight triage decisions are implemented, including ADR 0008 explicit Original Schedule storage and upgrade backfill. Ruff lint/format, scripts/check_docs.py, the full pytest suite (544 tests through the commit hook), and frontend tests (244) pass. This is a follow-up PR, so the repository three-pass review path applies. Coordination notes were posted on overlapping PR #178; it remains open, and origin/master is current.

Requesting code review pass 1 for 016a012...87e3bb4, spec #186. All eight triage decisions are implemented, including ADR 0008 explicit Original Schedule storage and upgrade backfill. Ruff lint/format, scripts/check_docs.py, the full pytest suite (544 tests through the commit hook), and frontend tests (244) pass. This is a follow-up PR, so the repository three-pass review path applies. Coordination notes were posted on overlapping PR #178; it remains open, and origin/master is current.
Merge origin/master into chore/schedule-p3-followups-186
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m56s
Details
f4f2264296
gabogg commented 2026-10-03 10:57:12 +00:00
Author
Owner
Copy link

Merged origin/master (b64e805) into chore/schedule-p3-followups-186. All merge conflicts were resolved preserving both #178 holiday/event context and #228 schedule follow-up behaviors:

Conflict Resolutions

  1. app/facility_time.py:
    • BusinessDaySchedule.from_record: Merged both is_holiday=bool(row.get("is_holiday", 0)) (#178) and recorded=bool(row.get("recorded", True)) (#228), ensuring stored recorded flags and holiday classifications are accurately decoded.
  2. app/db/database.py:
    • init_schema: Preserved dynamic check and ALTER TABLE addition of is_holiday column on occupancy_business_day_schedules.
    • Used sql_check_choices("action", ScheduleAuditAction) for occupancy_business_day_schedule_audit.
    • Executed _migrate_schedule_audit_actions and _backfill_original_schedules before the versioned holiday migration.
    • In 20261002_holiday_backfill: Included original_value with json.dumps(stamp_dict) when inserting unstamped past holidays into occupancy_business_day_schedules, so stamped historical records always have an explicit Original Schedule.
  3. app/db/occupancy_repository.py:
    • _BUSINESS_DAY_SCHEDULE_UPSERT_SQL & ensure_business_day_schedule_record_async: Aligned columns to include all 11 fields (day_date, is_open, open_time, close_time, source, is_exception, exception_name, is_holiday, created_at, updated_at, original_value) matching _business_day_schedule_params.
    • add_holiday_async: Preserved nameless exception empty string handling (str(holiday.get("name") or "").strip()) while adopting holiday_date variable and holiday identity tracking.
    • get_original_business_day_schedule_async: Read explicitly stored original_value JSON directly from occupancy_business_day_schedules (ADR 0008) while querying and overriding is_holiday dynamically from the current business-day schedule record (SELECT original_value, is_holiday FROM occupancy_business_day_schedules WHERE day_date = ?), maintaining the business-day record as the single source of holiday truth without a second holiday source.
  4. app/services/occupancy_service.py:
    • add_schedule_exception_async & delete_schedule_exception_async: Retained freeze-before-mutation docstrings while keeping completed-day validation and actor/reason audit parameter propagation.
  5. app/static/js/app.js:
    • renderHolidaysList: Combined localized fallback for nameless schedule exceptions (h.name || t('occupancy.scheduleExceptionSingular')) with the HOLIDAY / NON-HOLIDAY badge display.
  6. Tests:
    • tests/test_schedule_migrations.py: Added explicit column list on occupancy_business_day_schedules insert in test_original_backfill_uses_oldest_audit_or_record_and_runs_once to account for is_holiday.
    • tests/test_occupancy.py: Aligned test_holiday_schedule_evaluation to check unnamed_info["is_exception"] and not unnamed_info["is_open"] for closed unnamed schedule exceptions.

Verification

  • ruff check .: passed (clean)
  • ruff format --check .: passed (clean, 152 files)
  • python3 scripts/check_docs.py: passed (42 Markdown files, 89 HTTP operations)
  • pytest: passed full suite through pre-commit hook (564 passed)
  • node --test tests/frontend/*.test.js: passed (245 tests passed)

Requesting code review pass 1 for b64e805...f4f2264, spec #186.

Merged `origin/master` (`b64e805`) into `chore/schedule-p3-followups-186`. All merge conflicts were resolved preserving both #178 holiday/event context and #228 schedule follow-up behaviors: ### Conflict Resolutions 1. **`app/facility_time.py`**: - `BusinessDaySchedule.from_record`: Merged both `is_holiday=bool(row.get("is_holiday", 0))` (#178) and `recorded=bool(row.get("recorded", True))` (#228), ensuring stored recorded flags and holiday classifications are accurately decoded. 2. **`app/db/database.py`**: - `init_schema`: Preserved dynamic check and `ALTER TABLE` addition of `is_holiday` column on `occupancy_business_day_schedules`. - Used `sql_check_choices("action", ScheduleAuditAction)` for `occupancy_business_day_schedule_audit`. - Executed `_migrate_schedule_audit_actions` and `_backfill_original_schedules` before the versioned holiday migration. - In `20261002_holiday_backfill`: Included `original_value` with `json.dumps(stamp_dict)` when inserting unstamped past holidays into `occupancy_business_day_schedules`, so stamped historical records always have an explicit Original Schedule. 3. **`app/db/occupancy_repository.py`**: - `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL` & `ensure_business_day_schedule_record_async`: Aligned columns to include all 11 fields (`day_date, is_open, open_time, close_time, source, is_exception, exception_name, is_holiday, created_at, updated_at, original_value`) matching `_business_day_schedule_params`. - `add_holiday_async`: Preserved nameless exception empty string handling (`str(holiday.get("name") or "").strip()`) while adopting `holiday_date` variable and holiday identity tracking. - `get_original_business_day_schedule_async`: Read explicitly stored `original_value` JSON directly from `occupancy_business_day_schedules` (ADR 0008) while querying and overriding `is_holiday` dynamically from the current business-day schedule record (`SELECT original_value, is_holiday FROM occupancy_business_day_schedules WHERE day_date = ?`), maintaining the business-day record as the single source of holiday truth without a second holiday source. 4. **`app/services/occupancy_service.py`**: - `add_schedule_exception_async` & `delete_schedule_exception_async`: Retained freeze-before-mutation docstrings while keeping completed-day validation and `actor`/`reason` audit parameter propagation. 5. **`app/static/js/app.js`**: - `renderHolidaysList`: Combined localized fallback for nameless schedule exceptions (`h.name || t('occupancy.scheduleExceptionSingular')`) with the `HOLIDAY` / `NON-HOLIDAY` badge display. 6. **Tests**: - `tests/test_schedule_migrations.py`: Added explicit column list on `occupancy_business_day_schedules` insert in `test_original_backfill_uses_oldest_audit_or_record_and_runs_once` to account for `is_holiday`. - `tests/test_occupancy.py`: Aligned `test_holiday_schedule_evaluation` to check `unnamed_info["is_exception"] and not unnamed_info["is_open"]` for closed unnamed schedule exceptions. ### Verification - `ruff check .`: passed (clean) - `ruff format --check .`: passed (clean, 152 files) - `python3 scripts/check_docs.py`: passed (42 Markdown files, 89 HTTP operations) - `pytest`: passed full suite through pre-commit hook (564 passed) - `node --test tests/frontend/*.test.js`: passed (245 tests passed) Requesting code review pass 1 for b64e805...f4f2264, spec #186.
Merge branch 'origin/master' into chore/schedule-p3-followups-186
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m1s
Details
f2a4078679
gabogg commented 2026-10-03 11:32:00 +00:00
Author
Owner
Copy link

Merged origin/master (9a72ff4) into chore/schedule-p3-followups-186. All merge conflicts were resolved preserving #177 next-day settings activation and audit flow, #234 BEGIN IMMEDIATE schema transaction wrapping, #178 single source of holiday identity, and #228 schedule follow-up behaviors:

Conflict Resolutions

  1. docs/api/README.md:

    • Integrated #177 documentation on effective-dated reset boundaries (occupancy_effective_resets), contiguous business cycles (end_D + \epsilon == start_{D+1}), prospective changes at tomorrow's effective reset boundary (facility_now().date() + timedelta(days=1)), and pending activation notices returned by mutation endpoints.
    • Preserved #228 documentation on ADR 0008 explicit Original Schedule preservation, completed business-day stamping from earliest flow data, monitor backfill on startup/rollover, and localized defaults for nameless exceptions.
    • Synchronized documented operations with OpenAPI schema (all 90 registered HTTP operations pass validation).

  2. app/db/database.py:

    • Preserved #234 BEGIN IMMEDIATE transaction wrapping around _run_schema and inline ALTER TABLE counting_cameras DROP COLUMN zone_name.
    • Retained #177 tables (occupancy_effective_resets, occupancy_pending_changes, occupancy_settings_audit) and its CANCELLATION audit action migration.
    • Preserved #228 migrations: _migrate_schedule_audit_actions(c) and _backfill_original_schedules(c).

  3. app/services/occupancy_service.py:

    • In stamp_business_day_schedules_async: Adopted #177's effective reset schedule lookup (reset = await self.get_reset_schedule_async()) while preserving #228's earliest flow data fallback logic and docstrings.
    • In add_schedule_exception_async and delete_schedule_exception_async: Retained #177's pending change staging, completed-day validation, and audit flow.
    • In update_weekly_schedule_async: Maintained #177's prospective staging behavior and removed extraneous ensure_started_day_schedule_async() call which was firing pending changes with wall-clock time during test simulations of past dates.

Verification

  • ruff check .: passed (clean)
  • ruff format --check .: passed (clean, 155 files)
  • python3 scripts/check_docs.py: passed (Checked 43 Markdown files and 90 HTTP operations)
  • pytest: passed full test suite through commit pre-commit hook (and targeted 31/31 in test_next_day_settings_activation.py and 38/38 in schedule/holiday tests)
  • node --test tests/frontend/*.test.js: passed (250 tests passed, 0 failures)

Requesting code review pass 1 for 9a72ff4...f2a4078, spec #186.

Merged origin/master (`9a72ff4`) into `chore/schedule-p3-followups-186`. All merge conflicts were resolved preserving #177 next-day settings activation and audit flow, #234 `BEGIN IMMEDIATE` schema transaction wrapping, #178 single source of holiday identity, and #228 schedule follow-up behaviors: ### Conflict Resolutions 1. **`docs/api/README.md`**: - Integrated #177 documentation on effective-dated reset boundaries (`occupancy_effective_resets`), contiguous business cycles ($end_D + \epsilon == start_{D+1}$), prospective changes at tomorrow's effective reset boundary (`facility_now().date() + timedelta(days=1)`), and pending activation notices returned by mutation endpoints. - Preserved #228 documentation on ADR 0008 explicit Original Schedule preservation, completed business-day stamping from earliest flow data, monitor backfill on startup/rollover, and localized defaults for nameless exceptions. - Synchronized documented operations with OpenAPI schema (all 90 registered HTTP operations pass validation). 2. **`app/db/database.py`**: - Preserved #234 `BEGIN IMMEDIATE` transaction wrapping around `_run_schema` and inline `ALTER TABLE counting_cameras DROP COLUMN zone_name`. - Retained #177 tables (`occupancy_effective_resets`, `occupancy_pending_changes`, `occupancy_settings_audit`) and its `CANCELLATION` audit action migration. - Preserved #228 migrations: `_migrate_schedule_audit_actions(c)` and `_backfill_original_schedules(c)`. 3. **`app/services/occupancy_service.py`**: - In `stamp_business_day_schedules_async`: Adopted #177's effective reset schedule lookup (`reset = await self.get_reset_schedule_async()`) while preserving #228's earliest flow data fallback logic and docstrings. - In `add_schedule_exception_async` and `delete_schedule_exception_async`: Retained #177's pending change staging, completed-day validation, and audit flow. - In `update_weekly_schedule_async`: Maintained #177's prospective staging behavior and removed extraneous `ensure_started_day_schedule_async()` call which was firing pending changes with wall-clock time during test simulations of past dates. ### Verification - `ruff check .`: passed (clean) - `ruff format --check .`: passed (clean, 155 files) - `python3 scripts/check_docs.py`: passed (Checked 43 Markdown files and 90 HTTP operations) - `pytest`: passed full test suite through commit pre-commit hook (and targeted 31/31 in `test_next_day_settings_activation.py` and 38/38 in schedule/holiday tests) - `node --test tests/frontend/*.test.js`: passed (250 tests passed, 0 failures) Requesting code review pass 1 for 9a72ff4...f2a4078, spec #186.
gabogg reviewed 2026-10-03 12:21:51 +00:00
gabogg left a comment
Copy link

Code review, pass 1 (origin/master...f2a4078, spec #186 + its triage decisions)

Result: 2 P2s and 10 P3s. Not mergeable yet. This is pass 1, so fix every finding and request a second pass. Both P2s come from the two rounds of conflict resolution with #178 and #177, not from the original #186 work.

  • The migration is sound. I probed a DB with the pre-#178 schema: audit CHECK without FREEZE, no is_holiday or original_value, plus a custom index and trigger.
    • init_schema rebuilt the CHECK and accepted a FREEZE insert.
    • Row ids, the id sequence, the index and the trigger were all kept, and no _old table was left behind.
    • Original Schedules were backfilled from the oldest audit old_value, or from the record when a day had no audit row.
    • A second run was a no-op.
    • The SAVEPOINTs nest correctly inside #234's BEGIN IMMEDIATE.
    • original_value is written once and never touched by ON CONFLICT or a correction.
  • Items 3–7 and decisions 2, 6, 7, 8.1 and 8.3 are implemented as specified.
  • #178 holiday identity is preserved: the record's is_holiday overlays the Original Schedule.
  • Checks: ruff is clean, and the targeted tests pass (38).

Spec

P2

  • P2-1. The merge lost decision 8.2 for nameless exceptions that aren't holidays (app.js ~1870 and ~1888). The spec says "A nameless exception … replaced by a frontend-localized default".
    • What happens: the live card keys on live.is_holiday. Since #178, is_holiday means holiday identity only, and OccupancyLiveResponse doesn't expose is_exception. So a nameless, open, non-holiday exception (10:00–18:00) shows the backend's weekday label ("Lunes (10:00 - 18:00)") with the working-hours title, where master showed "Excepción de horario".
    • Merge f4f2264 switched the test assertion to is_exception, but the frontend wasn't switched to match. The frontend test only covers is_holiday: true.
    • Fix: expose is_exception (or key on the exception name being present) in the live response, use it in the card, and add a frontend test for a nameless non-holiday exception.
  • P2-2. The merge dropped the date from the exceptions list (renderHolidaysList, app.js ~2481). The <span>${escapeHtml(h.holiday_date)}</span> date label disappeared in merge f4f2264. Neither 87e3bb4 nor master removed it. With several nameless exceptions, every row reads "Schedule exception", and an admin can't tell which date each row is before pressing DEL or toggling HOLIDAY. Restore it.

P3

  1. Item 1 is only partly done. The spec says "the repository uses it instead of literals". Bare 'CORRECTION' remains at occupancy_repository.py:876 (_sync_record_holiday_async, from #178). Use ScheduleAuditAction there. The one-time migration literals at database.py:698 and :767 ('CORRECTION', 'STAMP') should use it too.

  2. Decision 8.2 is not applied consistently:

    • The #178 holiday backfill still defaults nameless holidays to "Feriado" (exc_name = … or "Feriado").
    • Existing rows that already store "Excepción de horario" aren't normalized.
    • week_view.js:710 and month_view.js:705 now render "Holiday: " with a blank name.

    Use the same localized default everywhere, or leave a note pointing to #73.

  3. The frontend rebuilds the backend's schedule_label format (app.js:1870). Two places now own the format name (open - close). Duplicated Code.

Standards

P3

  1. The monitor restamps the whole history at every boundary (monitor_service.py:116). Each business-day boundary restamps from the first flow day, with one SELECT per day under BEGIN IMMEDIATE, and skips the ten-year cap for the default range. It's correct, but the write lock grows with history. Start from the latest recorded day. Plausible, not measured.
  2. Nothing documents that the upsert doesn't touch original_value (_business_day_schedule_params(original=…)). original is used only on the INSERT path, and the upsert's DO UPDATE correctly leaves original_value alone. Add a one-line comment on the upsert SQL so nobody adds it later.
  3. Test gap (tests/test_schedule_migrations.py): nothing tests the rollback path or trigger replay. Index replay and the id sequence are covered.
  4. Wrong branch type (git-and-workflow.md §1). chore/ is for dependencies, CI and tool config. A new schema column, a data backfill and new monitor behavior are fix/ or feat/ work. Use the right type in new commits and the PR title.
  5. The merge commits have no Co-Authored-By trailer.
## Code review, pass 1 (`origin/master...f2a4078`, spec #186 + its triage decisions) Result: **2 P2s and 10 P3s. Not mergeable yet.** This is pass 1, so fix every finding and request a **second pass**. Both P2s come from the two rounds of conflict resolution with #178 and #177, not from the original #186 work. - **The migration is sound.** I probed a DB with the pre-#178 schema: audit CHECK without FREEZE, no `is_holiday` or `original_value`, plus a custom index and trigger. - `init_schema` rebuilt the CHECK and accepted a FREEZE insert. - Row ids, the id sequence, the index and the trigger were all kept, and no `_old` table was left behind. - Original Schedules were backfilled from the oldest audit `old_value`, or from the record when a day had no audit row. - A second run was a no-op. - The SAVEPOINTs nest correctly inside #234's `BEGIN IMMEDIATE`. - `original_value` is written once and never touched by `ON CONFLICT` or a correction. - **Items 3–7 and decisions 2, 6, 7, 8.1 and 8.3 are implemented as specified.** - **#178 holiday identity is preserved:** the record's `is_holiday` overlays the Original Schedule. - **Checks:** ruff is clean, and the targeted tests pass (38). ## Spec ### P2 - **P2-1. The merge lost decision 8.2 for nameless exceptions that aren't holidays** (`app.js` ~1870 and ~1888). The spec says *"A nameless exception … replaced by a frontend-localized default"*. - **What happens:** the live card keys on `live.is_holiday`. Since #178, `is_holiday` means holiday identity only, and `OccupancyLiveResponse` doesn't expose `is_exception`. So a nameless, open, non-holiday exception (10:00–18:00) shows the backend's weekday label ("Lunes (10:00 - 18:00)") with the working-hours title, where master showed "Excepción de horario". - Merge `f4f2264` switched the test assertion to `is_exception`, but the frontend wasn't switched to match. The frontend test only covers `is_holiday: true`. - **Fix:** expose `is_exception` (or key on the exception name being present) in the live response, use it in the card, and add a frontend test for a nameless non-holiday exception. - **P2-2. The merge dropped the date from the exceptions list** (`renderHolidaysList`, `app.js` ~2481). The `<span>${escapeHtml(h.holiday_date)}</span>` date label disappeared in merge `f4f2264`. Neither `87e3bb4` nor master removed it. With several nameless exceptions, every row reads "Schedule exception", and an admin can't tell which date each row is before pressing DEL or toggling HOLIDAY. Restore it. ### P3 1. **Item 1 is only partly done.** The spec says *"the repository uses it instead of literals"*. Bare `'CORRECTION'` remains at `occupancy_repository.py:876` (`_sync_record_holiday_async`, from #178). Use `ScheduleAuditAction` there. The one-time migration literals at `database.py:698` and `:767` (`'CORRECTION'`, `'STAMP'`) should use it too. 2. **Decision 8.2 is not applied consistently:** - The #178 holiday backfill still defaults nameless holidays to `"Feriado"` (`exc_name = … or "Feriado"`). - Existing rows that already store "Excepción de horario" aren't normalized. - `week_view.js:710` and `month_view.js:705` now render "Holiday: " with a blank name. Use the same localized default everywhere, or leave a note pointing to #73. 3. **The frontend rebuilds the backend's `schedule_label` format** (`app.js:1870`). Two places now own the format `name (open - close)`. Duplicated Code. ## Standards ### P3 4. **The monitor restamps the whole history at every boundary** (`monitor_service.py:116`). Each business-day boundary restamps from the first flow day, with one SELECT per day under `BEGIN IMMEDIATE`, and skips the ten-year cap for the default range. It's correct, but the write lock grows with history. Start from the latest recorded day. Plausible, not measured. 5. **Nothing documents that the upsert doesn't touch `original_value`** (`_business_day_schedule_params(original=…)`). `original` is used only on the INSERT path, and the upsert's `DO UPDATE` correctly leaves `original_value` alone. Add a one-line comment on the upsert SQL so nobody adds it later. 6. **Test gap** (`tests/test_schedule_migrations.py`): nothing tests the rollback path or trigger replay. Index replay and the id sequence are covered. 7. **Wrong branch type** (git-and-workflow.md §1). `chore/` is for dependencies, CI and tool config. A new schema column, a data backfill and new monitor behavior are `fix/` or `feat/` work. Use the right type in new commits and the PR title. 8. **The merge commits have no `Co-Authored-By` trailer.**
gabogg changed title from chore(schedule): complete schedule follow-ups to feat(schedule): complete schedule follow-ups 2026-10-03 12:47:06 +00:00 
- P2-1: expose is_exception and exception_name in OccupancyLiveResponse, and key live card localized nameless default on is_exception
- P2-2: restore holiday_date span in renderHolidaysList
- P3-1: use ScheduleAuditAction instead of CORRECTION/STAMP string literals
- P3-2: use consistent empty default for nameless exceptions in holiday backfill and deck views
- P3-3: remove duplicated schedule_label format from live card
- P3-4: start monitor boundary restamp from latest recorded day
- P3-5: document that schedule upsert leaves original_value untouched
- P3-6: add unit tests for trigger replay and rollback path on migration failure

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Merge branch 'origin/master' into chore/schedule-p3-followups-186
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m4s
Details
026e4ae32b 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-10-03 12:48:08 +00:00
Author
Owner
Copy link

Pass 1 fixes (026e4ae)

All P2 and P3 findings from review pass 1 (r48) have been resolved. The branch has also merged origin/master (026e4ae, including fix commit cffb435).

Findings and Resolutions

Finding Severity Resolution Summary Key Changes / Commits
P2-1 Live card lost decision 8.2 for nameless exceptions that aren't holidays P2 Exposed is_exception: bool and exception_name: str | None in OccupancyLiveResponse. Keyed live card default label on live.is_exception instead of live.is_holiday. Added frontend test for nameless non-holiday exceptions. app/schemas/occupancy_models.py, app/services/occupancy_service.py, app/static/js/app.js, tests/frontend/test_schedule_exception.test.js
P2-2 Exceptions list dropped the date label P2 Restored the date label badge <span>${escapeHtml(h.holiday_date)}</span> in renderHolidaysList. app/static/js/app.js
P3-1 Bare 'CORRECTION' and 'STAMP' literals P3 Replaced bare string literals with ScheduleAuditAction.CORRECTION and ScheduleAuditAction.STAMP. app/db/occupancy_repository.py, app/db/database.py
P3-2 Inconsistent nameless exception handling across views and backfill P3 Backfill uses str(h["name"] or "") without injecting "Feriado". Cleaned week_view.js and month_view.js to avoid dangling colons when holiday name is empty. Preserved existing rows per ADR 0006/0007 / ADR #34 until #73 sweep. app/db/database.py, app/static/js/src/ui/statistics_deck/views/week_view.js, month_view.js
P3-3 Duplicated schedule_label format in frontend P3 Reused backend's clean (open - close) formatting for nameless exceptions; frontend now prepends the localized default rather than rebuilding the full label string. app/services/occupancy_service.py, app/static/js/app.js
P3-4 Monitor boundary restamp scans full history P3 Added get_latest_recorded_schedule_day_async(before=...). Restamp now starts from the latest recorded day before the current schedule day instead of the earliest flow day. app/db/occupancy_repository.py, app/services/occupancy_service.py, app/services/monitor_service.py
P3-5 Document that upsert leaves original_value untouched P3 Added explicit comment above _BUSINESS_DAY_SCHEDULE_UPSERT_SQL documenting that original_value is only set on initial INSERT and never modified by DO UPDATE. app/db/occupancy_repository.py
P3-6 Test gap for migration rollback path and trigger replay P3 Added trigger execution replay checks in test_action_check_upgrade_preserves_history_indexes_and_sequence, and added test_action_check_upgrade_rollback_on_failure_preserves_state for SAVEPOINT rollback on CHECK violation. tests/test_schedule_migrations.py
P3-7 Branch/commit prefix convention P3 Used fix(schedule): ... commit type and updated PR #228 title to feat(schedule): complete schedule follow-ups. PR #228 title, commit cffb435
P3-8 Missing Co-Authored-By trailer on commits P3 Added Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> trailer to fix commit cffb435 and master merge commit 026e4ae. Git commit log

All automated checks (ruff, pytest, npm test) pass cleanly.

Ready for review pass 2.

## Pass 1 fixes (026e4ae) All P2 and P3 findings from review pass 1 (r48) have been resolved. The branch has also merged `origin/master` (`026e4ae`, including fix commit `cffb435`). ### Findings and Resolutions | Finding | Severity | Resolution Summary | Key Changes / Commits | |---|---|---|---| | **P2-1** Live card lost decision 8.2 for nameless exceptions that aren't holidays | P2 | Exposed `is_exception: bool` and `exception_name: str \| None` in `OccupancyLiveResponse`. Keyed live card default label on `live.is_exception` instead of `live.is_holiday`. Added frontend test for nameless non-holiday exceptions. | `app/schemas/occupancy_models.py`, `app/services/occupancy_service.py`, `app/static/js/app.js`, `tests/frontend/test_schedule_exception.test.js` | | **P2-2** Exceptions list dropped the date label | P2 | Restored the date label badge `<span>${escapeHtml(h.holiday_date)}</span>` in `renderHolidaysList`. | `app/static/js/app.js` | | **P3-1** Bare `'CORRECTION'` and `'STAMP'` literals | P3 | Replaced bare string literals with `ScheduleAuditAction.CORRECTION` and `ScheduleAuditAction.STAMP`. | `app/db/occupancy_repository.py`, `app/db/database.py` | | **P3-2** Inconsistent nameless exception handling across views and backfill | P3 | Backfill uses `str(h["name"] or "")` without injecting `"Feriado"`. Cleaned `week_view.js` and `month_view.js` to avoid dangling colons when holiday name is empty. Preserved existing rows per ADR 0006/0007 / ADR #34 until #73 sweep. | `app/db/database.py`, `app/static/js/src/ui/statistics_deck/views/week_view.js`, `month_view.js` | | **P3-3** Duplicated `schedule_label` format in frontend | P3 | Reused backend's clean `(open - close)` formatting for nameless exceptions; frontend now prepends the localized default rather than rebuilding the full label string. | `app/services/occupancy_service.py`, `app/static/js/app.js` | | **P3-4** Monitor boundary restamp scans full history | P3 | Added `get_latest_recorded_schedule_day_async(before=...)`. Restamp now starts from the latest recorded day before the current schedule day instead of the earliest flow day. | `app/db/occupancy_repository.py`, `app/services/occupancy_service.py`, `app/services/monitor_service.py` | | **P3-5** Document that upsert leaves `original_value` untouched | P3 | Added explicit comment above `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL` documenting that `original_value` is only set on initial INSERT and never modified by `DO UPDATE`. | `app/db/occupancy_repository.py` | | **P3-6** Test gap for migration rollback path and trigger replay | P3 | Added trigger execution replay checks in `test_action_check_upgrade_preserves_history_indexes_and_sequence`, and added `test_action_check_upgrade_rollback_on_failure_preserves_state` for SAVEPOINT rollback on CHECK violation. | `tests/test_schedule_migrations.py` | | **P3-7** Branch/commit prefix convention | P3 | Used `fix(schedule): ...` commit type and updated PR #228 title to `feat(schedule): complete schedule follow-ups`. | PR #228 title, commit `cffb435` | | **P3-8** Missing `Co-Authored-By` trailer on commits | P3 | Added `Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>` trailer to fix commit `cffb435` and master merge commit `026e4ae`. | Git commit log | All automated checks (`ruff`, `pytest`, `npm test`) pass cleanly. Ready for review pass 2.
gabogg reviewed 2026-10-03 18:03:13 +00:00
gabogg left a comment
Copy link

Code review, pass 2 (origin/master...026e4ae, spec #186 + its triage decisions)

Summary: 1 P2 (a merge conflict), plus P3s. #228 is a follow-up PR (from #186), so pass 2 fixes everything; request pass 3 afterwards.

Both r48 P2s are fixed (the live card keys on is_exception; the date span is restored). The spec reviewer found no new P1 or P2.

Must fix:

  • Standards N-P2-1: merge conflict with master 9a608c1 in the app/db/database.py import block. Keep master's import list and add ScheduleAuditAction. Do not keep DEFAULT_WEEKDAY_HOURS: it is unused after #233, so ruff fails with F401.
  • All P3s from both axes. Spec P3-3 (CSV exports an empty name for nameless holidays) and Spec P3-4 (the backend-only "(Cerrado)" suffix) belong to #73: add a note there instead of fixing them here.

Standards axis

Result: 0 P1, 1 P2 (merge conflict), 6 P3. All ten pass-1 findings are fixed or acceptably closed. This PR addresses follow-up issue #186, so git-and-workflow.md "Review passes" step 3 applies: fix every P1, P2 and P3 here, then request a third pass.

Checks run on the merged tree (origin/master 9a608c1 + 026e4ae, with the conflict resolved as described in N-P2-1):

  • ruff check and ruff format --check are clean.
  • pytest passes: test_business_day_schedules.py and test_schedule_migrations.py (20), plus test_occupancy.py and test_holiday_state_reset.py (22).
  • node --test passes for test_schedule_exception and test_command_deck_adapter (27).

Previous findings (r48)

# Finding Status Evidence
P2-1 Live card lost the localized default for nameless non-holiday exceptions FIXED OccupancyLiveResponse.is_exception/exception_name added (occupancy_models.py ~1009). get_active_schedule_info_async returns them (occupancy_service.py:1450-1451). The card keys on isException (app.js:1870-1884). A frontend test covers the nameless non-holiday case in en and es.
P2-2 Exceptions list dropped its date label FIXED <span …>${escapeHtml(h.holiday_date)}</span> is present at app.js:2493 and is now identical to master. No test covers it (see N-P3-5).
P3-1 Bare audit-action literals FIXED The only quoted FREEZE/STAMP/OVERWRITE/CORRECTION strings left in app/**/*.py are the enum definitions (occupancy_models.py:384-387).
P3-2 Nameless default applied inconsistently FIXED The backfill uses "" (database.py:743). week_view.js:710 and month_view.js:705 no longer leave a dangling colon. Normalizing legacy rows is already covered by #73 (maintainer comment of 2026-10-03: "legacy rows stay as they are", fold into area (c)).
P3-3 Frontend rebuilds the schedule_label format PARTIAL The frontend no longer builds name (open - close). It still parses the backend label with indexOf('(') (app.js:1875-1878), and that code path can never do anything useful (see N-P3-1).
P3-4 Monitor restamps the whole history at each boundary FIXED monitor_service.py:116-128 starts from get_latest_recorded_schedule_day_async(before=schedule_day). This still meets the #186 decision ("On startup the monitor backfills days it missed while down"). Older history goes through the admin default stamp.
P3-5 Document that the upsert never touches original_value FIXED There is a comment above _BUSINESS_DAY_SCHEDULE_UPSERT_SQL (occupancy_repository.py:73).
P3-6 No tests for migration rollback or trigger replay FIXED test_action_check_upgrade_preserves_history_indexes_and_sequence now checks that custom_audit_trigger fires once per new action. test_action_check_upgrade_rollback_on_failure_preserves_state covers the SAVEPOINT rollback.
P3-7 Wrong branch/commit type FIXED (in effect) The PR title is feat(schedule): complete schedule follow-ups. That title becomes the merge commit <title> (#228), and the fix commit uses fix(schedule). The branch name stays chore/…, which can't change without a new PR.
P3-8 Merge commits lack Co-Authored-By PARTIAL (acceptable) cffb435 and 026e4ae have the trailer. 87e3bb4, f4f2264 and f2a4078 still don't, and fixing them would need a history rewrite. Accept this; don't force-push.

New findings

P2

N-P2-1. Merge conflict with master in app/db/database.py:15-24 (import block). CONFIRMED.

  • Cause: git merge-tree --write-tree origin/master 026e4ae32b reports a single conflict, in the import block. Master (#233) swapped DEFAULT_HOLIDAY_HOURS and DEFAULT_WEEKDAY_HOURS for DEFAULT_DAILY_SCHEDULE and seed_default_occupancy_config. The PR keeps DEFAULT_WEEKDAY_HOURS and adds ScheduleAuditAction.
  • Failure scenario: the PR can't merge as it stands. If the conflict is resolved by keeping both sides, DEFAULT_WEEKDAY_HOURS is left unused in the merged file, because master's only use (default_days) is gone. Ruff then fails with F401, and so do pre-commit and CI.
  • Fix: keep only ScheduleAuditAction from the PR side.
  • Verified: I applied that resolution to an export of the merged tree. Ruff was clean and the targeted tests above passed.
  • Other auto-merged files: I checked occupancy_repository, occupancy_models, occupancy_service, app.js, i18n, docs/api and test_occupancy and found no semantic clashes.
    • Master's reset_test_occupancy_state deletes occupancy_business_day_schedules and its audit rows, so the new original_value column is reset along with them.
    • Master's OccupancyManager.reset() doesn't interact with the new manager method, which holds no state.
    • Master's seed_default_config doesn't touch the schedule tables.
P3

N-P3-1. The live-card label code has a branch that can never run (app.js:1870-1878). CONFIRMED by reading the code.

  • Dead slice: the backend label for an exception with no name is exactly hours (label = f"{name} {hours}".strip() if name else hours, occupancy_service.py:1440). So parenIdx is always 0, and both the slice and the parenIdx === -1 fallback do nothing. The frontend still knows the backend's ( format, which is what P3-3 asked to remove.
    • Fix: use `${t('occupancy.scheduleExceptionSingular')} ${live.schedule_label}`.
  • Dead fallback: isException = live.is_exception || (live.is_holiday && live.is_exception !== false) is a compatibility fallback for a payload without is_exception. That can't happen, because the backend always sends the field with a default of False (Speculative Generality).
  • Redundant fallback: excName = live.exception_name || live.holiday_name is also redundant, because holiday_name is exception_name whenever the day is an exception.

N-P3-2. The manager method is a pure Middle Man, and the monitor stitches the backfill together itself (occupancy_service.py:1332-1336, monitor_service.py:116-128). PLAUSIBLE (smell).

  • OccupancyManager.get_latest_recorded_schedule_day_async only forwards to the repo.
  • The monitor then works out the start date and calls stamp_business_day_schedules_async with five arguments. "Backfill missed days" is domain policy that now lives in the polling loop.
  • Fix: add one deep method, occupancy_manager.backfill_missed_schedule_days_async(), that finds the latest recorded day and stamps from there. Call it from the monitor and drop the pass-through.

N-P3-3. Duplicated normalization in add_holiday_async (occupancy_repository.py:919 and :927). CONFIRMED.

  • str(holiday.get("name") or "").strip() is computed once for the INSERT and again for holiday_name in new_dict. Those are the same two sites #73's maintainer note calls out.
  • Fix: compute holiday_name once, before the INSERT, and use it in both places.

N-P3-4. The test name doesn't match what the test does (tests/test_business_day_schedules.py, test_get_latest_recorded_schedule_day_and_monitor_restamp). CONFIRMED.

  • The test only exercises get_latest_recorded_schedule_day_async. It never runs the monitor or a restamp.
  • Fix: rename it, for example to test_latest_recorded_schedule_day_excludes_the_given_day, or add the monitor assertion. A useful one is that a gap older than the latest recorded day is deliberately not refilled.

N-P3-5. No frontend tests for the other pass-1 UI fixes. CONFIRMED. Nothing in tests/frontend covers:

  • the restored date span in renderHolidaysList, the P2-2 regression;
  • the localized name in that list for nameless rows;
  • the week and month context entries without a dangling : ;
  • the formatBusinessDayScheduleAuditValue nameless default.

P2-2 was a silent merge regression, so a small vm-based test like test_schedule_exception.test.js would guard it against the next merge.

N-P3-6. The monitor stamps with wall-clock time instead of the now it already took (monitor_service.py:116-128, occupancy_service.py ~1356). PLAUSIBLE, unmeasured.

  • ensure_started_day_schedule_async(now) receives the loop's now. stamp_business_day_schedules_async instead calls active_business_day(reset_time=reset) with the current time.
  • Effect: this is harmless in practice, since there are only milliseconds between the two calls. Near a boundary, though, the two calls can disagree on schedule_day, and the code is easier to reason about with one clock.
  • Fix: pass now through, or fold this into the N-P3-2 method.

Verdict

Not mergeable yet.

  1. Resolve the database.py import conflict (N-P2-1).
  2. This is a follow-up PR (#186), so fix N-P3-1 to N-P3-6 as well, and request a third pass.

Nothing in the fix commit regresses the pass-1 behavior.

Spec axis

Verdict: both pass-1 P2s are fixed. No new P1 or P2. The only merge blocker is a trivial import conflict with master 9a608c1.

Previous findings (r48)

Finding Status Evidence
P2-1 live card keyed on is_holiday FIXED OccupancyLiveResponse now has is_exception and exception_name (occupancy_models.py:1009-1010), filled from get_active_schedule_info_async (occupancy_service.py:1447-1448, 3059-3060). The card keys on isException (app.js:1870-1900). For a nameless exception the backend label is (10:00 - 18:00) and the frontend adds the localized default in front of it. tests/frontend/test_schedule_exception.test.js covers a nameless non-holiday exception, a nameless holiday and a named exception in en and es: 4/4 pass.
P2-2 date dropped from exceptions list FIXED <span …>${escapeHtml(h.holiday_date)}</span> is back in renderHolidaysList (app.js ~2492) and matches master. A nameless row also shows the localized default.
P3-1 bare action literals FIXED ScheduleAuditAction is used in occupancy_repository.py (878, 1466, 1507, 1551) and in the database.py migrations.
P3-2 8.2 consistency PARTIAL (accepted) The backfill uses "", and the week and month views drop the dangling colon. Existing "Excepción de horario" rows are deferred to #73, as the fix reply says.
P3-3 frontend rebuilds label FIXED (reduced) The frontend now only adds the default in front of the backend's (…) part.
P3-4 full-history restamp FIXED The monitor now starts from get_latest_recorded_schedule_day_async(before=schedule_day) (monitor_service.py:116-127).
P3-5 upsert comment FIXED occupancy_repository.py:73
P3-6 migration test gaps FIXED The rollback and trigger replay tests were added. test_schedule_migrations.py: 8/8 pass.
P3-7 branch type FIXED (title) The PR title is now feat(schedule). The branch name is unchanged, which is acceptable.
P3-8 Co-Authored-By FIXED cffb435 and 026e4ae carry the trailer. f2a4078 does not, and history won't be rewritten.

Acceptance re-check (spec #186 + triage)

  • Items 1, 4: done. A single StrEnum generates the CHECK through sql_check_choices, which validates its input.
  • Items 2 and 3, decision 7: done.
    • The generic CHECK-rebuild migration is in place.
    • from_record keeps the stored recorded flag.
    • original_value is written once by every writer: the freeze, the stamp upsert, the correction (with previous_fallback) and the #178 holiday stamp. Neither ON CONFLICT nor a correction touches it.
    • Calibration reads original_value, not the audit order.
    • ADR 0008 and the CONTEXT.md term are on master.
  • Item 5: docstrings present.
  • Decision 6: done.
    • The default stamp runs from MIN(people_counting_events.timestamp_epoch) up to active_day − 1.
    • The test asserts that counted+3 is recorded.
    • The monitor backfills on startup and at each boundary.
    • Staged flow_history_camera_hours rows are correctly not counted as flow data, because they are "never read by statistics" until the import lands (#170).
  • Decision 8: done. 8.1 and 8.3 were accepted as-is, and 8.2 now uses the localized default.

Master clash (9a608c1: #233, #227)

  • The textual conflict is only in the import block of app/db/database.py. Master moved the default seeding to seed_default_occupancy_config, so DEFAULT_WEEKDAY_HOURS is no longer used in database.py. Resolution: keep master's import list and add ScheduleAuditAction. Keeping the PR side's DEFAULT_WEEKDAY_HOURS would leave an unused import.
  • CONFIRMED that nothing breaks once resolved: I merged with that resolution in a scratch copy.
    • ruff is clean.
    • These suites pass: test_holiday_state_reset (2), test_business_day_schedules (12), test_schedule_migrations (8), test_occupancy (20), test_calibration_reconciliation (3).
    • Master's tests/occupancy_reset.py already clears both schedule tables.
    • OccupancyManager.reset() (#233) adds no state that the PR depends on.
    • #227's statistics changes don't touch the schedule-name paths.

New findings

P1

None.

P2

None.

P3
  1. The test name overclaims (tests/test_business_day_schedules.py, test_get_latest_recorded_schedule_day_and_monitor_restamp). It tests only the repository helper and never runs the monitor restamp. The boundary path, where the latest record is yesterday and only that day is preserved, is covered only indirectly by test_monitor_backfills_missed_days_and_preserves_existing. Rename the test or add a boundary case. CONFIRMED.
  2. The monitor backfill only looks forward (monitor_service.py:116-127).
    • It starts at the latest record before today, so a gap before that record is never filled automatically. Example: a correction on D-2 while the monitor task was dead for D-5..D-3 inside a live app process.
    • This matches the spec's "days it missed while down" when the process is down, and the admin default stamp still covers the gap.
    • Document the behaviour in the monitor comment or the API doc. PLAUSIBLE, edge case only.
  3. Nameless holidays now export an empty name in CSV (analytics_service.py:1911 d.holiday_name or ""). Before, the cell read "Excepción de horario" (the repository default) or the ISO date. The CSV is not localized by the frontend, so these cells are now blank. This is consistent with decision 8.2 being frontend-only. Fold it into the #73 sweep. CONFIRMED by reading the code.
  4. The live-card fallback is backend-only Spanish: a closed nameless exception renders as "Schedule exception (Cerrado)" in English. (Cerrado) is pre-existing backend text, now more visible. Belongs to #73. CONFIRMED by reading the code.

Checks run

  • At the PR head: node test_schedule_exception 4/4; pytest test_business_day_schedules 12/12, test_schedule_migrations 8/8.
  • On the merged copy: the 5 suites and ruff listed above.
## Code review, pass 2 (`origin/master...026e4ae`, spec #186 + its triage decisions) **Summary: 1 P2 (a merge conflict), plus P3s. #228 is a follow-up PR (from #186), so pass 2 fixes everything; request pass 3 afterwards.** Both r48 P2s are fixed (the live card keys on `is_exception`; the date span is restored). The spec reviewer found no new P1 or P2. Must fix: - **Standards N-P2-1: merge conflict with master 9a608c1** in the `app/db/database.py` import block. Keep master's import list and add `ScheduleAuditAction`. Do **not** keep `DEFAULT_WEEKDAY_HOURS`: it is unused after #233, so ruff fails with F401. - All P3s from both axes. Spec P3-3 (CSV exports an empty name for nameless holidays) and Spec P3-4 (the backend-only "(Cerrado)" suffix) belong to #73: add a note there instead of fixing them here. ### Standards axis **Result: 0 P1, 1 P2 (merge conflict), 6 P3.** All ten pass-1 findings are fixed or acceptably closed. This PR addresses follow-up issue #186, so git-and-workflow.md "Review passes" step 3 applies: fix every P1, P2 and P3 here, then request a **third pass**. Checks run on the merged tree (origin/master 9a608c1 + 026e4ae, with the conflict resolved as described in N-P2-1): - `ruff check` and `ruff format --check` are clean. - pytest passes: `test_business_day_schedules.py` and `test_schedule_migrations.py` (20), plus `test_occupancy.py` and `test_holiday_state_reset.py` (22). - `node --test` passes for `test_schedule_exception` and `test_command_deck_adapter` (27). #### Previous findings (r48) | # | Finding | Status | Evidence | |---|---|---|---| | P2-1 | Live card lost the localized default for nameless non-holiday exceptions | FIXED | `OccupancyLiveResponse.is_exception/exception_name` added (occupancy_models.py ~1009). `get_active_schedule_info_async` returns them (occupancy_service.py:1450-1451). The card keys on `isException` (app.js:1870-1884). A frontend test covers the nameless non-holiday case in en and es. | | P2-2 | Exceptions list dropped its date label | FIXED | `<span …>${escapeHtml(h.holiday_date)}</span>` is present at app.js:2493 and is now identical to master. No test covers it (see N-P3-5). | | P3-1 | Bare audit-action literals | FIXED | The only quoted `FREEZE/STAMP/OVERWRITE/CORRECTION` strings left in `app/**/*.py` are the enum definitions (occupancy_models.py:384-387). | | P3-2 | Nameless default applied inconsistently | FIXED | The backfill uses `""` (database.py:743). week_view.js:710 and month_view.js:705 no longer leave a dangling colon. Normalizing legacy rows is already covered by #73 (maintainer comment of 2026-10-03: "legacy rows stay as they are", fold into area (c)). | | P3-3 | Frontend rebuilds the `schedule_label` format | PARTIAL | The frontend no longer builds `name (open - close)`. It still parses the backend label with `indexOf('(')` (app.js:1875-1878), and that code path can never do anything useful (see N-P3-1). | | P3-4 | Monitor restamps the whole history at each boundary | FIXED | monitor_service.py:116-128 starts from `get_latest_recorded_schedule_day_async(before=schedule_day)`. This still meets the #186 decision ("On startup the monitor backfills days it missed while down"). Older history goes through the admin default stamp. | | P3-5 | Document that the upsert never touches `original_value` | FIXED | There is a comment above `_BUSINESS_DAY_SCHEDULE_UPSERT_SQL` (occupancy_repository.py:73). | | P3-6 | No tests for migration rollback or trigger replay | FIXED | `test_action_check_upgrade_preserves_history_indexes_and_sequence` now checks that `custom_audit_trigger` fires once per new action. `test_action_check_upgrade_rollback_on_failure_preserves_state` covers the SAVEPOINT rollback. | | P3-7 | Wrong branch/commit type | FIXED (in effect) | The PR title is `feat(schedule): complete schedule follow-ups`. That title becomes the merge commit `<title> (#228)`, and the fix commit uses `fix(schedule)`. The branch name stays `chore/…`, which can't change without a new PR. | | P3-8 | Merge commits lack `Co-Authored-By` | PARTIAL (acceptable) | cffb435 and 026e4ae have the trailer. 87e3bb4, f4f2264 and f2a4078 still don't, and fixing them would need a history rewrite. Accept this; don't force-push. | #### New findings ##### P2 **N-P2-1. Merge conflict with master in `app/db/database.py:15-24` (import block). CONFIRMED.** - **Cause:** `git merge-tree --write-tree origin/master 026e4ae32b` reports a single conflict, in the import block. Master (#233) swapped `DEFAULT_HOLIDAY_HOURS` and `DEFAULT_WEEKDAY_HOURS` for `DEFAULT_DAILY_SCHEDULE` and `seed_default_occupancy_config`. The PR keeps `DEFAULT_WEEKDAY_HOURS` and adds `ScheduleAuditAction`. - **Failure scenario:** the PR can't merge as it stands. If the conflict is resolved by keeping both sides, `DEFAULT_WEEKDAY_HOURS` is left unused in the merged file, because master's only use (`default_days`) is gone. Ruff then fails with F401, and so do pre-commit and CI. - **Fix:** keep only `ScheduleAuditAction` from the PR side. - **Verified:** I applied that resolution to an export of the merged tree. Ruff was clean and the targeted tests above passed. - **Other auto-merged files:** I checked occupancy_repository, occupancy_models, occupancy_service, app.js, i18n, docs/api and test_occupancy and found no semantic clashes. - Master's `reset_test_occupancy_state` deletes `occupancy_business_day_schedules` and its audit rows, so the new `original_value` column is reset along with them. - Master's `OccupancyManager.reset()` doesn't interact with the new manager method, which holds no state. - Master's `seed_default_config` doesn't touch the schedule tables. ##### P3 **N-P3-1. The live-card label code has a branch that can never run (app.js:1870-1878). CONFIRMED by reading the code.** - **Dead slice:** the backend label for an exception with no name is exactly `hours` (`label = f"{name} {hours}".strip() if name else hours`, occupancy_service.py:1440). So `parenIdx` is always 0, and both the slice and the `parenIdx === -1` fallback do nothing. The frontend still knows the backend's `(` format, which is what P3-3 asked to remove. - **Fix:** use `` `${t('occupancy.scheduleExceptionSingular')} ${live.schedule_label}` ``. - **Dead fallback:** `isException = live.is_exception || (live.is_holiday && live.is_exception !== false)` is a compatibility fallback for a payload without `is_exception`. That can't happen, because the backend always sends the field with a default of `False` (Speculative Generality). - **Redundant fallback:** `excName = live.exception_name || live.holiday_name` is also redundant, because `holiday_name` is `exception_name` whenever the day is an exception. **N-P3-2. The manager method is a pure Middle Man, and the monitor stitches the backfill together itself (occupancy_service.py:1332-1336, monitor_service.py:116-128). PLAUSIBLE (smell).** - `OccupancyManager.get_latest_recorded_schedule_day_async` only forwards to the repo. - The monitor then works out the start date and calls `stamp_business_day_schedules_async` with five arguments. "Backfill missed days" is domain policy that now lives in the polling loop. - **Fix:** add one deep method, `occupancy_manager.backfill_missed_schedule_days_async()`, that finds the latest recorded day and stamps from there. Call it from the monitor and drop the pass-through. **N-P3-3. Duplicated normalization in `add_holiday_async` (occupancy_repository.py:919 and :927). CONFIRMED.** - `str(holiday.get("name") or "").strip()` is computed once for the INSERT and again for `holiday_name` in `new_dict`. Those are the same two sites #73's maintainer note calls out. - **Fix:** compute `holiday_name` once, before the INSERT, and use it in both places. **N-P3-4. The test name doesn't match what the test does (`tests/test_business_day_schedules.py`, `test_get_latest_recorded_schedule_day_and_monitor_restamp`). CONFIRMED.** - The test only exercises `get_latest_recorded_schedule_day_async`. It never runs the monitor or a restamp. - **Fix:** rename it, for example to `test_latest_recorded_schedule_day_excludes_the_given_day`, or add the monitor assertion. A useful one is that a gap older than the latest recorded day is deliberately not refilled. **N-P3-5. No frontend tests for the other pass-1 UI fixes. CONFIRMED.** Nothing in `tests/frontend` covers: - the restored date span in `renderHolidaysList`, the P2-2 regression; - the localized name in that list for nameless rows; - the week and month context entries without a dangling `: `; - the `formatBusinessDayScheduleAuditValue` nameless default. P2-2 was a silent merge regression, so a small vm-based test like `test_schedule_exception.test.js` would guard it against the next merge. **N-P3-6. The monitor stamps with wall-clock time instead of the `now` it already took (monitor_service.py:116-128, occupancy_service.py ~1356). PLAUSIBLE, unmeasured.** - `ensure_started_day_schedule_async(now)` receives the loop's `now`. `stamp_business_day_schedules_async` instead calls `active_business_day(reset_time=reset)` with the current time. - **Effect:** this is harmless in practice, since there are only milliseconds between the two calls. Near a boundary, though, the two calls can disagree on `schedule_day`, and the code is easier to reason about with one clock. - **Fix:** pass `now` through, or fold this into the N-P3-2 method. #### Verdict **Not mergeable yet.** 1. Resolve the database.py import conflict (N-P2-1). 2. This is a follow-up PR (#186), so fix N-P3-1 to N-P3-6 as well, and request a **third pass**. Nothing in the fix commit regresses the pass-1 behavior. ### Spec axis Verdict: **both pass-1 P2s are fixed. No new P1 or P2. The only merge blocker is a trivial import conflict with master 9a608c1.** #### Previous findings (r48) | Finding | Status | Evidence | |---|---|---| | P2-1 live card keyed on is_holiday | FIXED | `OccupancyLiveResponse` now has `is_exception` and `exception_name` (occupancy_models.py:1009-1010), filled from `get_active_schedule_info_async` (occupancy_service.py:1447-1448, 3059-3060). The card keys on `isException` (app.js:1870-1900). For a nameless exception the backend label is `(10:00 - 18:00)` and the frontend adds the localized default in front of it. tests/frontend/test_schedule_exception.test.js covers a nameless non-holiday exception, a nameless holiday and a named exception in en and es: 4/4 pass. | | P2-2 date dropped from exceptions list | FIXED | `<span …>${escapeHtml(h.holiday_date)}</span>` is back in `renderHolidaysList` (app.js ~2492) and matches master. A nameless row also shows the localized default. | | P3-1 bare action literals | FIXED | `ScheduleAuditAction` is used in occupancy_repository.py (878, 1466, 1507, 1551) and in the database.py migrations. | | P3-2 8.2 consistency | PARTIAL (accepted) | The backfill uses `""`, and the week and month views drop the dangling colon. Existing "Excepción de horario" rows are deferred to #73, as the fix reply says. | | P3-3 frontend rebuilds label | FIXED (reduced) | The frontend now only adds the default in front of the backend's `(…)` part. | | P3-4 full-history restamp | FIXED | The monitor now starts from `get_latest_recorded_schedule_day_async(before=schedule_day)` (monitor_service.py:116-127). | | P3-5 upsert comment | FIXED | occupancy_repository.py:73 | | P3-6 migration test gaps | FIXED | The rollback and trigger replay tests were added. test_schedule_migrations.py: 8/8 pass. | | P3-7 branch type | FIXED (title) | The PR title is now `feat(schedule)`. The branch name is unchanged, which is acceptable. | | P3-8 Co-Authored-By | FIXED | cffb435 and 026e4ae carry the trailer. f2a4078 does not, and history won't be rewritten. | #### Acceptance re-check (spec #186 + triage) - **Items 1, 4:** done. A single StrEnum generates the CHECK through `sql_check_choices`, which validates its input. - **Items 2 and 3, decision 7:** done. - The generic CHECK-rebuild migration is in place. - `from_record` keeps the stored `recorded` flag. - `original_value` is written once by every writer: the freeze, the stamp upsert, the correction (with `previous_fallback`) and the #178 holiday stamp. Neither `ON CONFLICT` nor a correction touches it. - Calibration reads `original_value`, not the audit order. - ADR 0008 and the CONTEXT.md term are on master. - **Item 5:** docstrings present. - **Decision 6:** done. - The default stamp runs from `MIN(people_counting_events.timestamp_epoch)` up to `active_day − 1`. - The test asserts that counted+3 is recorded. - The monitor backfills on startup and at each boundary. - Staged `flow_history_camera_hours` rows are correctly not counted as flow data, because they are "never read by statistics" until the import lands (#170). - **Decision 8:** done. 8.1 and 8.3 were accepted as-is, and 8.2 now uses the localized default. #### Master clash (9a608c1: #233, #227) - **The textual conflict is only in the import block of `app/db/database.py`.** Master moved the default seeding to `seed_default_occupancy_config`, so `DEFAULT_WEEKDAY_HOURS` is no longer used in database.py. **Resolution:** keep master's import list and add `ScheduleAuditAction`. Keeping the PR side's `DEFAULT_WEEKDAY_HOURS` would leave an unused import. - **CONFIRMED that nothing breaks once resolved:** I merged with that resolution in a scratch copy. - ruff is clean. - These suites pass: test_holiday_state_reset (2), test_business_day_schedules (12), test_schedule_migrations (8), test_occupancy (20), test_calibration_reconciliation (3). - Master's `tests/occupancy_reset.py` already clears both schedule tables. - `OccupancyManager.reset()` (#233) adds no state that the PR depends on. - #227's statistics changes don't touch the schedule-name paths. #### New findings ##### P1 None. ##### P2 None. ##### P3 1. **The test name overclaims** (tests/test_business_day_schedules.py, `test_get_latest_recorded_schedule_day_and_monitor_restamp`). It tests only the repository helper and never runs the monitor restamp. The boundary path, where the latest record is yesterday and only that day is preserved, is covered only indirectly by `test_monitor_backfills_missed_days_and_preserves_existing`. Rename the test or add a boundary case. CONFIRMED. 2. **The monitor backfill only looks forward** (monitor_service.py:116-127). - It starts at the latest record before today, so a gap *before* that record is never filled automatically. Example: a correction on D-2 while the monitor task was dead for D-5..D-3 inside a live app process. - This matches the spec's "days it missed while down" when the process is down, and the admin default stamp still covers the gap. - Document the behaviour in the monitor comment or the API doc. PLAUSIBLE, edge case only. 3. **Nameless holidays now export an empty name in CSV** (analytics_service.py:1911 `d.holiday_name or ""`). Before, the cell read "Excepción de horario" (the repository default) or the ISO date. The CSV is not localized by the frontend, so these cells are now blank. This is consistent with decision 8.2 being frontend-only. Fold it into the #73 sweep. CONFIRMED by reading the code. 4. **The live-card fallback is backend-only Spanish:** a closed nameless exception renders as "Schedule exception (Cerrado)" in English. `(Cerrado)` is pre-existing backend text, now more visible. Belongs to #73. CONFIRMED by reading the code. #### Checks run - At the PR head: node test_schedule_exception 4/4; pytest test_business_day_schedules 12/12, test_schedule_migrations 8/8. - On the merged copy: the 5 suites and ruff listed above. 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
fix(schedule): address review pass 2 findings
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m19s
Details
7334922141 
- Standards N-P3-1: clean up dead branches in live-card label logic in app.js
- Standards N-P3-2 & N-P3-6: consolidate backfill in OccupancyManager.backfill_missed_schedule_days_async with loop's now passed through to active_business_day
- Standards N-P3-3: deduplicate holiday_name normalization in add_holiday_async
- Standards N-P3-4 / Spec P3-1: rename test to test_latest_recorded_schedule_day_excludes_the_given_day and add test_backfill_missed_schedule_days_preserves_older_gaps
- Standards N-P3-5: add frontend tests for restored date span, nameless row localization, audit value formatting, and context entry strings without dangling colon

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-10-03 18:20:57 +00:00
Author
Owner
Copy link

Pass 2 fixes (7334922)

All P2 and P3 findings from review pass 2 (r56) have been resolved. The branch has merged origin/master (commit 893189c), and pass 2 fixes are committed in 7334922. Items Spec P3-3 and Spec P3-4 have been deferred and logged on issue #73.

Findings and Resolutions

Finding Severity Resolution Summary Key Changes / Commits
N-P2-1 Merge conflict with master in app/db/database.py import block P2 Resolved conflict by retaining master's import list and adding ScheduleAuditAction without DEFAULT_WEEKDAY_HOURS (avoiding F401). app/db/database.py, commit 893189c
N-P3-1 Dead branches and redundant fallbacks in live-card label logic P3 Cleaned updateOccupancyLiveCard in app.js: removed dead string slice/fallback, dead compatibility fallback for live.is_exception, and redundant fallback for excName. Simplified working hours text to prepending t('occupancy.scheduleExceptionSingular') to live.schedule_label. app/static/js/app.js, commit 7334922
N-P3-2 & N-P3-6 Middle Man backfill and wall-clock vs now clock skew P3 Consolidated backfill policy into OccupancyManager.backfill_missed_schedule_days_async(now). Dropped the middle-man method get_latest_recorded_schedule_day_async from manager. Passed now through to active_business_day(epoch=now, ...) in stamp_business_day_schedules_async so monitor and stamping use the identical clock reference. app/services/occupancy_service.py, app/services/monitor_service.py, commit 7334922
N-P3-3 Duplicated normalization in add_holiday_async P3 Normalized holiday_name = str(holiday.get("name") or "").strip() once before the INSERT and reused for both the SQL parameter and new_dict. app/db/occupancy_repository.py, commit 7334922
N-P3-4 / Spec P3-1 Test name overclaimed repository helper P3 Renamed test to test_latest_recorded_schedule_day_excludes_the_given_day calling occupancy_repo directly, and added test_backfill_missed_schedule_days_preserves_older_gaps to verify that monitor forward backfill deliberately preserves older gaps unrefilled. tests/test_business_day_schedules.py, commit 7334922
N-P3-5 Missing frontend tests for pass-1 UI fixes P3 Added tests covering restored date span (<span class="font-mono text-cyan-300...">), nameless row localized fallback in renderHolidaysList, nameless default in formatBusinessDayScheduleAuditValue, and week/month context entry strings without dangling colons in EN and ES. Also added view integration tests in test_statistics_deck_week.test.js and test_statistics_deck_month.test.js. tests/frontend/test_schedule_exception.test.js, tests/frontend/test_statistics_deck_week.test.js, tests/frontend/test_statistics_deck_month.test.js, commit 7334922
Spec P3-2 Forward-only backfill behavior documentation P3 Documented forward-only backfill behavior and reliance on manual admin stamping for older gaps in OccupancyManager.backfill_missed_schedule_days_async docstring and monitor loop comments. app/services/occupancy_service.py, app/services/monitor_service.py, commit 7334922
Spec P3-3 & Spec P3-4 CSV empty name and live-card (Cerrado) backend suffix P3 Deferred to #73 sweep per instructions; documented and posted as follow-up items in comment on #73. Comment on issue #73

All automated checks (ruff, pytest, node --test) pass cleanly.

Ready for review pass 3.

## Pass 2 fixes (7334922) All P2 and P3 findings from review pass 2 (r56) have been resolved. The branch has merged `origin/master` (commit `893189c`), and pass 2 fixes are committed in `7334922`. Items Spec P3-3 and Spec P3-4 have been deferred and logged on issue #73. ### Findings and Resolutions | Finding | Severity | Resolution Summary | Key Changes / Commits | |---|---|---|---| | **N-P2-1** Merge conflict with master in `app/db/database.py` import block | P2 | Resolved conflict by retaining master's import list and adding `ScheduleAuditAction` without `DEFAULT_WEEKDAY_HOURS` (avoiding F401). | `app/db/database.py`, commit `893189c` | | **N-P3-1** Dead branches and redundant fallbacks in live-card label logic | P3 | Cleaned `updateOccupancyLiveCard` in `app.js`: removed dead string slice/fallback, dead compatibility fallback for `live.is_exception`, and redundant fallback for `excName`. Simplified working hours text to prepending `t('occupancy.scheduleExceptionSingular')` to `live.schedule_label`. | `app/static/js/app.js`, commit `7334922` | | **N-P3-2 & N-P3-6** Middle Man backfill and wall-clock vs `now` clock skew | P3 | Consolidated backfill policy into `OccupancyManager.backfill_missed_schedule_days_async(now)`. Dropped the middle-man method `get_latest_recorded_schedule_day_async` from manager. Passed `now` through to `active_business_day(epoch=now, ...)` in `stamp_business_day_schedules_async` so monitor and stamping use the identical clock reference. | `app/services/occupancy_service.py`, `app/services/monitor_service.py`, commit `7334922` | | **N-P3-3** Duplicated normalization in `add_holiday_async` | P3 | Normalized `holiday_name = str(holiday.get("name") or "").strip()` once before the INSERT and reused for both the SQL parameter and `new_dict`. | `app/db/occupancy_repository.py`, commit `7334922` | | **N-P3-4 / Spec P3-1** Test name overclaimed repository helper | P3 | Renamed test to `test_latest_recorded_schedule_day_excludes_the_given_day` calling `occupancy_repo` directly, and added `test_backfill_missed_schedule_days_preserves_older_gaps` to verify that monitor forward backfill deliberately preserves older gaps unrefilled. | `tests/test_business_day_schedules.py`, commit `7334922` | | **N-P3-5** Missing frontend tests for pass-1 UI fixes | P3 | Added tests covering restored date span (`<span class="font-mono text-cyan-300...">`), nameless row localized fallback in `renderHolidaysList`, nameless default in `formatBusinessDayScheduleAuditValue`, and week/month context entry strings without dangling colons in EN and ES. Also added view integration tests in `test_statistics_deck_week.test.js` and `test_statistics_deck_month.test.js`. | `tests/frontend/test_schedule_exception.test.js`, `tests/frontend/test_statistics_deck_week.test.js`, `tests/frontend/test_statistics_deck_month.test.js`, commit `7334922` | | **Spec P3-2** Forward-only backfill behavior documentation | P3 | Documented forward-only backfill behavior and reliance on manual admin stamping for older gaps in `OccupancyManager.backfill_missed_schedule_days_async` docstring and monitor loop comments. | `app/services/occupancy_service.py`, `app/services/monitor_service.py`, commit `7334922` | | **Spec P3-3 & Spec P3-4** CSV empty name and live-card `(Cerrado)` backend suffix | P3 | Deferred to #73 sweep per instructions; documented and posted as follow-up items in comment on #73. | Comment on issue #73 | All automated checks (`ruff`, `pytest`, `node --test`) pass cleanly. Ready for review pass 3.
gabogg commented 2026-10-03 20:11:35 +00:00
Author
Owner
Copy link

Coordinating #171 pass-1 approval fixes: if #228 merges before publication fixes finish, approval will write original_value explicitly via a shared occupancy repository helper, replacing STAMP audit inference. Please notify #171 when original_value lands.

Coordinating #171 pass-1 approval fixes: if #228 merges before publication fixes finish, approval will write original_value explicitly via a shared occupancy repository helper, replacing STAMP audit inference. Please notify #171 when original_value lands.
gabogg reviewed 2026-10-03 20:24:34 +00:00
gabogg left a comment
Copy link

Code review, pass 3 (origin/master...7334922, spec #186)

Summary: 2 P2s, a few P3s. Every r56 finding is fixed. Pass 3 of a follow-up PR: fix the P2s, request pass 4. P3s are filed as #275.

Must fix:

  • P2-1 (Standards): merge master 5a02cd9, which now contains #222. A trial merge conflicts in app/db/occupancy_repository.py (add_holiday_async), in app/services/occupancy_service.py (the two calendar exception_names sites, around 1251 and 1290) and in tests/test_occupancy.py (test_holiday_schedule_evaluation). Resolve it this way, which was tested on a scratch merge (ruff clean, 58/58 targeted backend tests, 115/115 Node tests):

    • Take #222's null semantics: blank names become None and stay out of exception_names. #184 item 7 says an unnamed exception has a null name in the API and in records.
    • Keep #228's holiday_name variable in the repository.
    • Keep #228's label rule: a nameless exception label shows only the hours, and the frontend adds "Schedule exception"/"Excepción de horario" (decision 8.2). In #222's test, expect schedule_label == "(Cerrado)", keep the exception_name/holiday_name is None assertions, and delete the weekday_row/expected_day_name lines that become unused (F841). Change #228's holiday_name == "" assertions to is None.
    • Make the #178 holiday stamp migration (app/db/database.py around 717-719, exc_name = str(h["name"] or "")) write NULL with .strip() or None. Migrated and live rows must agree. Check that test_schedule_migrations doesn't assert "".

  • P2-2 (Spec): the monitor's "missed days" backfill stamps history it never missed. backfill_missed_schedule_days_async (occupancy_service.py around 1332-1350) passes start_date=None when no record exists before the active day. stamp_business_day_schedules_async then uses the full admin default range, from the earliest flow data to yesterday. Two probes confirm it:

    • (a) A DB with flow data from D-400 and no records. On the monitor's first start, it auto-stamps 400 days as STAMP by system.
    • (b) After the #178 migration stamps one past holiday at D-100, D-99 to D-1 get auto-stamped while D-400 to D-101 stay unrecorded. The cut-off is wherever the latest holiday happens to fall.

    This breaks CONTEXT.md (a historical day without a record follows the current schedule until an administrator stamps or corrects it) and the 2026-10-02 decision (the automatic path covers only days the monitor missed while down). It also freezes today's weekly plan as each day's Original Schedule for good (ADR 0008). Fix:

    • With no anchor, do nothing.
    • Anchor on the latest day the monitor itself froze (the latest FREEZE audit row or a record from an automatic source), not on any record. Admin or migration stamps must never open an automatic range.
    • Add tests for "no records, flow exists" and "only a migration-stamped holiday exists".

Standards axis

Verdict on r56

All six standards findings (N-P2-1 and N-P3-1 to N-P3-6) are FIXED. Evidence:

  • The import block keeps master's list plus ScheduleAuditAction.
  • The live card uses Boolean(live.is_exception) and has no slice parsing.
  • The backfill policy lives in OccupancyManager.backfill_missed_schedule_days_async, and the monitor makes one call to it.
  • holiday_name is normalized once.
  • The renamed repository test calls the repository directly, and a test was added that keeps older gaps.
  • Frontend tests cover the pass-1 UI fixes.
  • now is passed through to active_business_day.

Checks at the head:

  • ruff check and format: clean.
  • pytest:
    • test_business_day_schedules: 13
    • test_next_day_settings_activation: 31
    • test_schedule_migrations: 8
    • test_occupancy: 20
    • test_holiday_state_reset: 2
    • test_statistics_periods: 9
    • test_statistics_baselines: 16
  • Node: 105/105.

New findings

  • P2-1: above.
  • P3-1: tests/frontend/test_schedule_exception.test.js has a tautological test, "week and month context entries render without dangling colon for nameless holidays". It rebuilds the template literal and asserts on constants, so it runs no production code. The test_statistics_deck_week/_month cases already guard this. Delete it.
  • P3-2: monitor_service.py:118 and the docstring at occupancy_service.py:1338 cite "(#186, Spec P3-2)". A review-finding ID means nothing to later readers. Keep #186 only.

Spec axis

Verdict on r56

Every finding is FIXED. Spec P3-3 and P3-4 are deferred to #73 by decision.

Acceptance criteria (#186 plus the 2026-10-02 decisions)

Probes on a git-archive export:

  • Items 1–5 (the single action definition, the generic CHECK migration, the recorded flag, the validated CHECK helper, the docstrings) are met.
  • Item 6:
    • Completed days keep their schedule through weekly edits.
    • Existing records are never overwritten.
    • The admin default range (earliest flow data to yesterday) matches the decision.
    • The monitor backfill is partial; see P2-2.
  • Item 7:
    • original_value is written on every insert path.
    • Corrections and overwrites preserve it. Probe: stamp, then correct, then a weekly edit, then an overwrite. The calibration still reads the original 09:00-20:00.
    • The upgrade backfill handles FREEZE then CORRECTION, a correction on an unrecorded day, and a record with no audit row.
    • The backfill runs once (sentinel probe).
  • Item 8 and ADR 0008: met.

Interaction with #222: see P2-1. The frontend treats null and "" the same everywhere, so the UI doesn't change.

New findings

  • P2-2: above.
  • P3-3: get_first_flow_epoch_async (occupancy_repository.py around 2297) takes an unfiltered MIN(timestamp_epoch) across all cameras. The default range skips the 10-year guard, so one event with a bad camera clock (from 1970, say) makes the admin default stamp write tens of thousands of days in one transaction. Keep a sanity bound, or ignore implausible epochs. PLAUSIBLE.
## Code review, pass 3 (`origin/master...7334922`, spec #186) **Summary: 2 P2s, a few P3s. Every r56 finding is fixed. Pass 3 of a follow-up PR: fix the P2s, request pass 4. P3s are filed as #275.** Must fix: - **P2-1 (Standards): merge master 5a02cd9, which now contains #222.** A trial merge conflicts in `app/db/occupancy_repository.py` (`add_holiday_async`), in `app/services/occupancy_service.py` (the two calendar `exception_names` sites, around 1251 and 1290) and in `tests/test_occupancy.py` (`test_holiday_schedule_evaluation`). Resolve it this way, which was tested on a scratch merge (ruff clean, 58/58 targeted backend tests, 115/115 Node tests): - Take #222's null semantics: blank names become `None` and stay out of `exception_names`. #184 item 7 says an unnamed exception has a null name in the API and in records. - Keep #228's `holiday_name` variable in the repository. - Keep #228's label rule: a nameless exception label shows only the hours, and the frontend adds "Schedule exception"/"Excepción de horario" (decision 8.2). In #222's test, expect `schedule_label == "(Cerrado)"`, keep the `exception_name`/`holiday_name is None` assertions, and delete the `weekday_row`/`expected_day_name` lines that become unused (F841). Change #228's `holiday_name == ""` assertions to `is None`. - Make the #178 holiday stamp migration (`app/db/database.py` around 717-719, `exc_name = str(h["name"] or "")`) write NULL with `.strip() or None`. Migrated and live rows must agree. Check that `test_schedule_migrations` doesn't assert `""`. - **P2-2 (Spec): the monitor's "missed days" backfill stamps history it never missed.** `backfill_missed_schedule_days_async` (occupancy_service.py around 1332-1350) passes `start_date=None` when no record exists before the active day. `stamp_business_day_schedules_async` then uses the full admin default range, from the earliest flow data to yesterday. Two probes confirm it: - (a) A DB with flow data from D-400 and no records. On the monitor's first start, it auto-stamps 400 days as STAMP by `system`. - (b) After the #178 migration stamps one past holiday at D-100, D-99 to D-1 get auto-stamped while D-400 to D-101 stay unrecorded. The cut-off is wherever the latest holiday happens to fall. This breaks CONTEXT.md (a historical day without a record follows the current schedule until an administrator stamps or corrects it) and the 2026-10-02 decision (the automatic path covers only days the monitor missed while down). It also freezes today's weekly plan as each day's Original Schedule for good (ADR 0008). Fix: - With no anchor, do nothing. - Anchor on the latest day the monitor itself froze (the latest `FREEZE` audit row or a record from an automatic source), not on any record. Admin or migration stamps must never open an automatic range. - Add tests for "no records, flow exists" and "only a migration-stamped holiday exists". ### Standards axis #### Verdict on r56 All six standards findings (N-P2-1 and N-P3-1 to N-P3-6) are FIXED. Evidence: - The import block keeps master's list plus `ScheduleAuditAction`. - The live card uses `Boolean(live.is_exception)` and has no slice parsing. - The backfill policy lives in `OccupancyManager.backfill_missed_schedule_days_async`, and the monitor makes one call to it. - `holiday_name` is normalized once. - The renamed repository test calls the repository directly, and a test was added that keeps older gaps. - Frontend tests cover the pass-1 UI fixes. - `now` is passed through to `active_business_day`. Checks at the head: - ruff check and format: clean. - pytest: - test_business_day_schedules: 13 - test_next_day_settings_activation: 31 - test_schedule_migrations: 8 - test_occupancy: 20 - test_holiday_state_reset: 2 - test_statistics_periods: 9 - test_statistics_baselines: 16 - Node: 105/105. #### New findings - **P2-1:** above. - **P3-1:** `tests/frontend/test_schedule_exception.test.js` has a tautological test, "week and month context entries render without dangling colon for nameless holidays". It rebuilds the template literal and asserts on constants, so it runs no production code. The `test_statistics_deck_week`/`_month` cases already guard this. Delete it. - **P3-2:** `monitor_service.py:118` and the docstring at `occupancy_service.py:1338` cite "(#186, Spec P3-2)". A review-finding ID means nothing to later readers. Keep `#186` only. ### Spec axis #### Verdict on r56 Every finding is FIXED. Spec P3-3 and P3-4 are deferred to #73 by decision. #### Acceptance criteria (#186 plus the 2026-10-02 decisions) Probes on a git-archive export: - Items 1–5 (the single action definition, the generic CHECK migration, the `recorded` flag, the validated CHECK helper, the docstrings) are met. - Item 6: - Completed days keep their schedule through weekly edits. - Existing records are never overwritten. - The admin default range (earliest flow data to yesterday) matches the decision. - The monitor backfill is **partial**; see P2-2. - Item 7: - `original_value` is written on every insert path. - Corrections and overwrites preserve it. Probe: stamp, then correct, then a weekly edit, then an overwrite. The calibration still reads the original 09:00-20:00. - The upgrade backfill handles FREEZE then CORRECTION, a correction on an unrecorded day, and a record with no audit row. - The backfill runs once (sentinel probe). - Item 8 and ADR 0008: met. Interaction with #222: see P2-1. The frontend treats null and `""` the same everywhere, so the UI doesn't change. #### New findings - **P2-2:** above. - **P3-3:** `get_first_flow_epoch_async` (occupancy_repository.py around 2297) takes an unfiltered `MIN(timestamp_epoch)` across all cameras. The default range skips the 10-year guard, so one event with a bad camera clock (from 1970, say) makes the admin default stamp write tens of thousands of days in one transaction. Keep a sanity bound, or ignore implausible epochs. PLAUSIBLE.
gabogg commented 2026-10-03 21:13:29 +00:00
Author
Owner
Copy link

#171 pass-1 fixes are pushed at e0d10d2 and merged into #172 at d8f3c48. Approval now calls OccupancyRepository.stamp_business_day_schedule_conn_async, extracted from the normal stamp writer, inside its atomic publication transaction. Only purely imported days without lived records are written; the helper protects non-IMPORTED existing records. When original_value lands, this shared connection-level helper is the approval seam that must explicitly fix original_value. #228 is still open at this handoff; the coordination remains recorded in #171 body/reply.

#171 pass-1 fixes are pushed at e0d10d2 and merged into #172 at d8f3c48. Approval now calls OccupancyRepository.stamp_business_day_schedule_conn_async, extracted from the normal stamp writer, inside its atomic publication transaction. Only purely imported days without lived records are written; the helper protects non-IMPORTED existing records. When original_value lands, this shared connection-level helper is the approval seam that must explicitly fix original_value. #228 is still open at this handoff; the coordination remains recorded in #171 body/reply.
All checks were successful
CI / lint-and-test (pull_request) Successful in 3m19s
Details
This pull request has changes conflicting with the target branch.
  • app/db/occupancy_repository.py
  • app/services/occupancy_service.py
  • tests/test_occupancy.py
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin chore/schedule-p3-followups-186:chore/schedule-p3-followups-186
git switch chore/schedule-p3-followups-186

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master
git merge --no-ff chore/schedule-p3-followups-186
git switch chore/schedule-p3-followups-186
git rebase master
git switch master
git merge --ff-only chore/schedule-p3-followups-186
git switch chore/schedule-p3-followups-186
git rebase master
git switch master
git merge --no-ff chore/schedule-p3-followups-186
git switch master
git merge --squash chore/schedule-p3-followups-186
git switch master
git merge --ff-only chore/schedule-p3-followups-186
git switch master
git merge chore/schedule-p3-followups-186
git push origin master
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
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
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!228
No description provided.