chore(occupancy): PR #178 second-pass P3 follow-ups (schedule records, event endpoints, repository queries) #211

New issue

Open

opened 2026-10-02 17:59:00 +00:00 by gabogg · 1 comment

gabogg commented

2026-10-02 17:59:00 +00:00

Owner

These P3 findings from the second review pass on #178 (holiday and event context for investor analytics, #161) were deferred. None of them blocked the merge.

Standards

Deleting an exception edits a frozen record (app/db/occupancy_repository.py:941).
- Finding: It clears is_exception/exception_name but leaves is_open=0 and the hours. The glossary says the Business-Day Schedule Record "remains fixed".
- Acceptance: deleting a dated schedule exception does not mutate frozen business-day schedule records, or follows an explicit audited correction model.
Repository fallback is_holiday = is_open when the field is omitted (app/db/occupancy_repository.py:805).
- Finding: Unused by current API contracts, and it makes open exceptions holidays in direct repository calls.
- Acceptance: all callers pass explicit is_holiday, and the fallback defaults to non-holiday (is_holiday = 0) safely without breaking legacy tests.
Timed events accept and store start_date/end_date on PUT.
- Finding: They are unused for timed events, but nothing rejects them.
- Acceptance: validate and reject or strip day-range fields on timed-event PUT updates.
Event timezone relabelled on every update (app/services/occupancy_service.py:421).
- Finding: Updating an event re-evaluates/overwrites the stored timezone string even when not specified.
- Acceptance: preserve the original event timezone unless explicitly updated.
_schedule_epochs closed-day branch (app/services/occupancy_service.py:726).
- Finding: Uses default "04:00" rather than configured reset time.
- Acceptance: thread the configured daily reset time into _schedule_epochs closed-day bounds computation.
Quarantine loop N-queries per row (app/services/occupancy_service.py:1491).
- Finding: The quarantine loop executes one is_event_day_async query per row.
- Acceptance: batch event-day checks across the quarantine inspection range in a single query.
domain_errors() wraps EventItem.model_validate.
- Finding: A broken schema or malformed DB record surfaces as 422 instead of 500.
- Acceptance: validate DB entities outside of user-input domain_errors() validation wrappers.
Duplicated code across event queries and derivations.
- Finding: Timed-event date expansion (:2612, :2921), Event Day SQL predicate (4 places), exclusion blocks, and holiday_hours derivation are repeated.
- Acceptance: consolidate into shared helper functions or SQL views.
Repeated reset_time fallback in repository (app/db/occupancy_repository.py).
- Finding: Seven repository methods repeat if reset_time is None: cfg=...; configured_reset_time(cfg) and repository expands timed events into business days.
- Acceptance: standardize reset_time resolution in repository or require caller to pass it.
Speculative generality.
- Finding: Hidden start/end export aliases; HolidayCreateOrUpdate | HolidayItem union with getattr(item, "reason"); isinstance(str) check on always-string field.
- Acceptance: clean up redundant aliases, union types, and type checks.

Spec

OccupancyLiveResponse.events has no consumer (app/schemas/occupancy_models.py:901).
- Finding: The field was added to the live response model but has no active frontend consumer.
- Acceptance: wire to live monitoring UI or deprecate the field.

Refs #178, #161.

These P3 findings from the second review pass on #178 (holiday and event context for investor analytics, #161) were deferred. None of them blocked the merge. ### Standards 1. **Deleting an exception edits a frozen record** (`app/db/occupancy_repository.py:941`). - Finding: It clears `is_exception`/`exception_name` but leaves `is_open=0` and the hours. The glossary says the Business-Day Schedule Record "remains fixed". - Acceptance: deleting a dated schedule exception does not mutate frozen business-day schedule records, or follows an explicit audited correction model. 2. **Repository fallback `is_holiday = is_open` when the field is omitted** (`app/db/occupancy_repository.py:805`). - Finding: Unused by current API contracts, and it makes open exceptions holidays in direct repository calls. - Acceptance: all callers pass explicit `is_holiday`, and the fallback defaults to non-holiday (`is_holiday = 0`) safely without breaking legacy tests. 3. **Timed events accept and store `start_date`/`end_date` on PUT**. - Finding: They are unused for timed events, but nothing rejects them. - Acceptance: validate and reject or strip day-range fields on timed-event PUT updates. 4. **Event timezone relabelled on every update** (`app/services/occupancy_service.py:421`). - Finding: Updating an event re-evaluates/overwrites the stored timezone string even when not specified. - Acceptance: preserve the original event timezone unless explicitly updated. 5. **`_schedule_epochs` closed-day branch** (`app/services/occupancy_service.py:726`). - Finding: Uses default `"04:00"` rather than configured reset time. - Acceptance: thread the configured daily reset time into `_schedule_epochs` closed-day bounds computation. 6. **Quarantine loop N-queries per row** (`app/services/occupancy_service.py:1491`). - Finding: The quarantine loop executes one `is_event_day_async` query per row. - Acceptance: batch event-day checks across the quarantine inspection range in a single query. 7. **`domain_errors()` wraps `EventItem.model_validate`**. - Finding: A broken schema or malformed DB record surfaces as 422 instead of 500. - Acceptance: validate DB entities outside of user-input `domain_errors()` validation wrappers. 8. **Duplicated code across event queries and derivations**. - Finding: Timed-event date expansion (`:2612`, `:2921`), Event Day SQL predicate (4 places), exclusion blocks, and `holiday_hours` derivation are repeated. - Acceptance: consolidate into shared helper functions or SQL views. 9. **Repeated `reset_time` fallback in repository** (`app/db/occupancy_repository.py`). - Finding: Seven repository methods repeat `if reset_time is None: cfg=...; configured_reset_time(cfg)` and repository expands timed events into business days. - Acceptance: standardize `reset_time` resolution in repository or require caller to pass it. 10. **Speculative generality**. - Finding: Hidden `start`/`end` export aliases; `HolidayCreateOrUpdate | HolidayItem` union with `getattr(item, "reason")`; `isinstance(str)` check on always-string field. - Acceptance: clean up redundant aliases, union types, and type checks. ### Spec 11. **`OccupancyLiveResponse.events` has no consumer** (`app/schemas/occupancy_models.py:901`). - Finding: The field was added to the live response model but has no active frontend consumer. - Acceptance: wire to live monitoring UI or deprecate the field. Refs #178, #161.

gabogg added the

ready-for-agent

label

2026-10-02 17:59:00 +00:00

gabogg referenced this issue

2026-10-02 18:04:26 +00:00

feat(statistics): holiday and event context for investor analytics #178

~~gabogg referenced this issue 2026-10-02 18:17:14 +00:00~~

research(tests): faster test suite and right-sized testing for agentic workflows #217

gabogg commented

2026-10-03 06:53:20 +00:00

Author

Owner

Item 1 (deleting an exception edits a frozen record) is resolved in PR #178 pass 3 (commit dad4d93): re-saving and deleting schedule exceptions now syncs only is_holiday through the audited correction path (_sync_record_holiday_async), leaving frozen record hours and open status untouched and auditable.

Item 1 (deleting an exception edits a frozen record) is resolved in PR #178 pass 3 (commit dad4d93): re-saving and deleting schedule exceptions now syncs only is_holiday through the audited correction path (_sync_record_holiday_async), leaving frozen record hours and open status untouched and auditable.

gabogg referenced this issue

2026-10-03 06:56:16 +00:00

feat(statistics): holiday and event context for investor analytics #178

gabogg referenced this issue

2026-10-03 06:58:30 +00:00

feat(statistics): holiday and event context for investor analytics #178