fix(schedule): localize calibration labels and preserve unnamed exceptions #222
No reviewers
Labels
No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gabogg/hikcentral!222
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/schedule-p3-followups-184"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Unnamed schedule exceptions remain null through POST, pending listings, and next-day activation. Empty and whitespace-only request names return 422; legacy blank active and pending names read as null. A shared Annotated alias defines name validation. The legacy NOT NULL column stores unnamed values as empty strings without a migration.
Localize calibration/trust labels and baseline offset in EN/ES while preserving uppercase styling. Rename the sibling trust key to
trustExceptionMinEntriesLabel; parse HTML for attribute-order-independent uppercase tests.Complete #258: use “Working hours - {schedule}”, business cycle terminology in English, and jornada terminology in Spanish. Operator/admin HUDs, status strips, reset time, completeness, cycle/date headings, and retroactive date validation all use i18n.
Closes #184 (items 1, 2, 3 and 7). Items 4 and 6 belong to #73; item 5 belongs to #178.
Closes #258.
Architectural impact
Name validation stays in schemas, persistence stays in the repository, and services preserve null staged names. No database migration or scheduling-boundary change. Existing localized-label test parser remains unchanged because review r47 accepts it.
Verification
Checklist
WIP: chore(schedule): localize calibration labels and validate exception namesto chore(schedule): localize calibration labels and validate exception namesPlease perform code review pass 1 of
master...67d9fdcagainst issue #184 and its maintainer triage comment.Scope: items 1, 2, 3 and 7 only. Check EN/ES calibration labels and uppercase rendering, attribute-order-independent uppercase tests with inherited button styling, the
trustExceptionMinEntriesLabelrename, andHolidayItem.namerejecting""while omitted/null names remain null. Items 4 and 6 are assigned to #73; item 5 is assigned to #178.Validation passed: Ruff lint/format,
scripts/check_docs.py(42 Markdown files, 80 HTTP operations), the full pytest suite (543 tests) through normal commit hooks, and 35 focused i18n/occupancy tests.Record the completed review as a formal review using
scripts/wt review post 222, headed:## Code review, pass 1 (master...67d9fdc, spec #184)This addresses findings from a prior review, so the repository's three-pass follow-up review policy applies before merge.
Scope added before pass 1 (maintainer decision, 2026-10-03): also fix #258 (the business cycle wording left over from PR #243 review pass 2), and add
Closes #258to the PR body. Merge order: #243 merges first. Merge origin/master after it lands, then fix #258 on top. Most important item: the working-hours label atoccupancy_service.py~774 should read 'Working hours - {schedule}', not 'Business cycle working hours'. Update the test that locks it in (tests/test_business_cycle_labels.py:55).- Fix 'Business cycle working hours' to 'Working hours - {schedule}' in occupancy_service.py and update test_business_cycle_labels.py - Align HolidayCreateOrUpdate name validation with HolidayItem (optional name, min_length=1) post-merge with origin/master - Replace remaining English 'operating cycle' with 'business cycle' in CONTEXT.md, README.md, docs, and i18n dictionaries - Standardize Spanish translations to use 'jornada' (operationalCycle, trustSpikeWindowLabel, nocturnalCycleAndCalibTitle, peakTooltip, operatingCycle, modalRetroactiveDateLabel, driftHistorySpan, logCycleHeader, trustTooltips) - Add frontend test coverage for canonical English business cycle and Spanish jornada keysPlease perform code review pass 1 of master...f7623d5 against issues #184 and #258.
Scope:
trustExceptionMinEntriesLabelrename, andHolidayItem.name/HolidayCreateOrUpdate.namerejecting""withmin_length=1while omitted/null names remain null.occupancy_service.pyis"Working hours - {schedule}", locked in viatests/test_business_cycle_labels.py.CONTEXT.md,README.md,docs/architecture/system-overview.md,i18n.js,index.html, andcommand_deck_adapter.js.operationalCycle,trustSpikeWindowLabel,nocturnalCycleAndCalibTitle,kpi.operationalCycleTitle,tactical.peakTooltip,tactical.operatingCycle,modalRetroactiveDateLabel,driftHistorySpan,logCycleHeader,trustTooltipTrusted,trustTooltipExcluded).tests/frontend/test_business_cycle_i18n.test.js.Validation passed:
scripts/check_docs.pypassed (42 Markdown files and 89 HTTP operations).Record the completed review as a formal review using
scripts/wt review post 222, headed:Code review, pass 1 (master...f7623d5, spec #184)
This addresses findings from prior reviews (#184 and #258), so the repository's three-pass follow-up review policy applies before merge.
Code review, pass 1 (
origin/master...f7623d5, spec #184 + #258)Result: 2 P2s and 11 P3s. Not mergeable yet. This is pass 1, so fix every finding and request a second pass.
trustExceptionMinEntriesLabelis renamed and guarded.node --test tests/frontend/*.test.js).Spec
P2
name: None(occupancy_models.py:443,466). ButScheduleExceptionStagedResult.nameis stillstr(occupancy_models.py:1181), andoccupancy_service.py:562passespayload.get("name", "Holiday")through, which isNonebecausemodel_dump()always includes the key.POST /api/occupancy/holidays {"holiday_date":"2099-12-25"}raises aValidationErrorwhile building the staged result, so the request returns 500. Before this PR it returned 422.ScheduleExceptionStagedResult.namestr | None, and add a POST test with no name. The current tests cover only""and the bare schema.P3
command_deck_adapter.js:259hard-codesOPERATING_CYCLE:with no i18n.'OPERATING_CYCLE:'while the English key saysBUSINESS_CYCLE:.tests/frontend/test_command_deck_adapter.test.js:345,413,506assert the avoided term.data-i18nremains atindex.html:649"CYCLE RESET TIME" (a sibling of the newly localized BASELINE OFFSET), :141 "CYCLE:", :283 "CYCLE COMPLETENESS" and :1163 "CYCLE / DATE". The PR body says index.html and the adapter were swept.app.js:4265alert('…fecha de ciclo operativa (YYYY-MM-DD).')is hard-coded, belongs to the same retroactive-date flow asmodalRetroactiveDateLabel, and slipped past the grep because of the feminine ending. Localize it.Standards
P2
HolidayCreateOrUpdate.name: straccepted""through the API, and the UI never sends one. NowHolidayItemhasmin_length=1, and it is built from DB rows (occupancy_service.py:702-705,occupancy_controller.py:291-294). Any existing row or pending payload withname=""makesGET /holidaysraise and return 500.""tonullwhen reading, or migrate once. Whitespace-only names such as" "should be rejected or normalized too.P3
chore/is for dependencies, CI and tool config, and this is fix and refactor work. Usefix(schedule):orrefactor(i18n):in new commits and the PR title.5a91d79's body has bullets cut off with "...".fd0e511is an empty commit; drop it if you rewrite, otherwise leave it.nameField block appears in bothHolidayItemandHolidayCreateOrUpdate(occupancy_models.py:443,466). Use anAnnotatedalias.payload.get("name", "Holiday")(occupancy_service.py:723) contradicts "null if unnamed", and it uses "Holiday", the term CONTEXT.md avoids. Remove it with P2-1.LocalizedLabelParserintest_i18n.pyis heavy test-only machinery. It's acceptable because it tests itself, but keep it in mind if it grows.Allow null staged exception names and remove invented name fallbacks. Use one Annotated name field with trimmed, nonempty request validation; normalize legacy blank active and pending names to null on API reads. Store unnamed exceptions as empty strings in the legacy NOT NULL column so next-day activation succeeds without a schema migration. Localize operator/admin HUD labels, status-strip fallbacks, reset time, completeness, cycle/date headings, and retroactive missing-date alerts. Update the three stale adapter assertions and add English/Spanish rendering regressions plus real SQLite/ASGI name and activation tests. Complete scope of the earlier i18n commit: retain Working hours - {schedule}, optional exception names, canonical business cycle wording in English, and jornada wording in Spanish. This commit and the revised PR description record the full changes without the truncated historical commit bullets. Addresses PR #222 review pass 1 (r47), #184 item 7, and remaining #258 copy.chore(schedule): localize calibration labels and validate exception namesto fix(schedule): localize calibration labels and preserve unnamed exceptionsPass 1 fixes (
6120c50)Addressed review r47 in PR #222.
str | None; no invented fallback. Real ASGI tests cover omitted/null names through next-day activation and GET.tactical.operatingCyclevia i18n.BUSINESS_CYCLE:.data-i18nand EN/ES translations.analytics.retroactiveDateRequired; EN/ES runtime alert tests cover it.fix(schedule):.ScheduleExceptionNameAnnotated alias, preserving nullable validation and OpenAPI description.Verification: full pytest/pre-commit hooks pass (621 backend tests); Node suite passes 253/253; Ruff lint/format and documentation checks pass (43 Markdown files, 90 HTTP operations).
Please perform review pass 2 against
6120c50for #184 and #258. This follow-up PR remains subject to the three-pass review policy; no merge requested.Code review, pass 2 (
origin/master...6120c50, spec #184 + #258)Summary: 1 P2, several P3s. #222 is a follow-up PR (P3s from #184), so pass 2 fixes everything; request pass 3 afterwards.
Both r47 P2s are fixed (nameless POST, blank stored names).
Must fix:
occupancy_service.py:1250and:1288userow.get("name") or day.isoformat(), and the result is stamped intooccupancy_business_day_schedules.exception_nameand the live label ("2026-09-23 (Cerrado)"). This breaks #184 item 7: no name is stored as null, and the frontend supplies the display name. Fix: strip the name, useor None, and update the test that pins the date label.BusinessDayScheduleCorrectionstill requires a name on admin corrections of exception days; leave that rule as is and state it in the code.9a608c1: the import block intests/test_occupancy.py. Keep both sides:CameraGroupRegistration, DirectionType, HolidayCreateOrUpdate, HolidayItem, TimespanPreset.Standards axis
Result: 0 P1, 0 P2, 3 P3. The merge conflict is trivial and nothing clashes semantically. Mergeable on the standards axis once the conflict is resolved.
Checks
test_occupancy.py,test_next_day_settings_activation.py,test_i18n.pyandtest_business_cycle_labels.py. 85 passed.node --test tests/frontend/*.test.js).git merge-treeresult 3462e5a) with the conflict hand-resolved and rantest_occupancy.py,test_next_day_settings_activation.py,test_holiday_state_reset.py(new on master) andtest_i18n.py. 78 passed, Node passed 255 of 255, and ruff is clean.Merge conflict (Forgejo: not mergeable against
9a608c1)tests/test_occupancy.py, the import block at lines 11-12.from app.schemas.occupancy_models import CameraGroupRegistration, DirectionType, TimespanPreset.HolidayCreateOrUpdateandHolidayItem.CameraGroupRegistration, DirectionType, HolidayCreateOrUpdate, HolidayItem, TimespanPreset.zoneLabel, which the PR doesn't touch.module.exportsand alocalStorageguard. The PR's app.js change is a singlealertline.tests/test_holiday_state_reset.pyuses a named exception ("Closure"), so the PR's unnamed-name change doesn't affect it. It passes on the merged tree.OccupancyManager.reset()doesn't interact with the PR.Previous findings (r47)
ScheduleExceptionStagedResult.name: str | None(occupancy_models.py:1189). Service :562 and :723 passpayload.get("name").test_unnamed_exception_post_and_activation({}and{"name": null}) asserts a 200 response,name is None, null in the pending payload, activation, and GET returning ACTIVE with null. Passes.""names break GETHolidayItem.normalize_stored_name(occupancy_models.py:468-472) maps blank and whitespace-only names to None. A probe confirmedHolidayItem(name=" ").name is None.HolidayCreateOrUpdatestrips and then appliesmin_length=1, so""," "and"\t\n"return 422 withVALIDATION_ERROR(test_blank_exception_name_post_rejected, which also asserts that nothing is staged). The legacy GET test covers an active row and a pending row.OPERATING_CYCLE:HUDtr('tactical.operatingCycle','BUSINESS_CYCLE:').BUSINESS_CYCLE:.data-i18n. Each key exists in both ES and EN (i18n.js 85/1239, 143/1297, 1052/2206, 341/1495).alert(t('analytics.retroactiveDateRequired')). Key exists in ES (1051) and EN (2205).fix(schedule):. The branch name is stillchore/, which r47 accepted (it asked only for new commits and the title).6120c50body. The empty commit was kept, which r47 allowed when history isn't rewritten.nameField blockScheduleExceptionNameAnnotated alias (occupancy_models.py:440-447). The OpenAPI schema keeps the description,minLength: 1, and null (probe).payload.get("name","Holiday")fallbackor ""). The add path (:914/:926) no longer injects "Excepción de horario". No"Holiday")fallback is left inapp/.LocalizedLabelParserweightNew findings
P3
ScheduleCalendar.exception_namesusesstr(row.get("name") or day.isoformat())(occupancy_service.py:1250 and :1288)."", freezing (stamping) an unnamed closure recordsexception_name == "2024-02-05"in the business-day schedule.schedule_labelbecomes"2024-02-05 (Cerrado)"(probe on the PR head).name: nullon/holidaysandexception_name: "<date>"on the business-day schedule and audit endpoints. Unnamed rows created before this PR keep the stored literal "Excepción de horario".BusinessDayScheduleCorrection(occupancy_models.py:395) requires a name for exception records, so some name has to fill the gap. But neither the code nor the PR body states that rule.exception_namefor an unnamed exception. The test at test_occupancy.py:177 already pins the label."", a special value standing in for null that every caller has to know about (Primitive Obsession).""(occupancy_repository.py:914, :926, :3713).add_holiday_asyncreturns a dict withname == "". OnlyHolidayItemconverts it back to null."": theexception_namesfallback and any future reader of the raw rows. The comment at repository :827-828 documents this.""as a real name.""value.HolidayItem.normalize_stored_name(occupancy_models.py:471) is misleading. It reads "...while requests reject blank strings", butHolidayItemitself accepts blank strings. OnlyHolidayCreateOrUpdaterejects them.add_schedule_exception_asyncstill acceptsHolidayCreateOrUpdate | HolidayItem(occupancy_service.py:496). A caller that passes aHolidayItemtherefore turns""into null silently instead of being rejected. There is no HTTP path for this today, because the controller bindsHolidayCreateOrUpdate.Nits (no action required)
tests/test_occupancy.py:22-33: the two new pure-schema tests are placed above the module's autousesetup_test_dbfixture, so they each run a fullinit_db()they don't need. They are also right next to the conflicting import block.test_legacy_blank_exception_names_are_null_on_getusestarget_date="2099-12-24"with a payloadholiday_dateof"2099-12-25"and a hard-codedeffective_boundary_epoch=4101840000. The mismatch is harmless but reads like a typo.Verdict summary
tests/test_occupancy.py, and the merged tree passes the targeted tests.Spec axis
Previous findings (r47)
ScheduleExceptionStagedResult.nameis nowstr | None(occupancy_models.py:1189), and the"Holiday"fallbacks are gone (occupancy_service.py:562,723). New ASGI testtest_unnamed_exception_post_and_activationcovers both{}and{"name": null}through POST, staging, activation and GET. My probe of the completed-day path (past date + reason, no name) returns 200 withname: null. The classification PUT also returnsname: null.HolidayItem.normalize_stored_name(occupancy_models.py:468-472) maps blank or whitespace-only strings toNone. It runs before the min_length check: I probedHolidayItem(name=" ").nameand gotNone.HolidayCreateOrUpdatestrips and then rejects""," "and"\t\n"with 422VALIDATION_ERROR, and nothing is staged. The new test covers both active and pending legacy rows on real SQLite. OpenAPI showsanyOf[string minLength 1, null].OPERATING_CYCLE:, fallbacks, Node assertions, bare HTML labels, ES alert, Annotated alias, stale fallback)command_deck_adapter.js:259,343,1563,1611all go throughtactical.operatingCycle/BUSINESS_CYCLE:. index.html:141,283,649,1163 havedata-i18n. app.js:4265 usesanalytics.retroactiveDateRequired.Checks
git merge-treeagainst9a608c1has one textual conflict, in thetests/test_occupancy.pyimport block. Master addedCameraGroupRegistrationand the PR addedHolidayCreateOrUpdate, HolidayItem, so the fix is to take the union of both lists.zoneLabelremoval, noCameraGroup fallback,reset()) don't touch this PR's lines.New findings
P2
occupancy_service.py:1250and:1288buildexception_names[day] = str(row.get("name") or day.isoformat()). On master this fallback was almost never reached, because the repo stored "Excepción de horario". This PR stores""(occupancy_repository.py:914,926,3713), so every nameless exception now takes the date fallback./api/occupancy/holidays {"holiday_date": <past date>, "is_open": false, "reason": "probe"}./holidayscorrectly returnsname: null.occupancy_business_day_schedulesis stamped withexception_name = '2026-09-23', and GET/schedule/days/2026-09-23returns"exception_name": "2026-09-23".holiday_name/exception_name= the date, and the label is"2026-09-23 (Cerrado)". The PR's own test locks this in (test_occupancy.py,f"{holiday_date_str} (Cerrado)"). The live card would read "Schedule Exception: 2026-09-23".exception_nameasNonewhen the stored name is blank:row.get("name") or None, after the strip, soexception_namesdrops the key and.get(day)returns None. Then check thatget_active_schedule_info_async'sname = schedule.exception_name or weekday namefallback is acceptable, or leave a pointer to #73.P3
timespanToday: "Jornada Activa (Ciclo 24H)"(EN: "Business Cycle (24H)").timespanDesc"... jornada comercial vs ciclo completo de 24 horas" (EN: "full business cycle").'Sin ciclos registrados'in the calibration-log table. This only shows whencommon.noDatais missing." ", reads asnullon/holidays. But the same row is truthy at occupancy_service.py:1250, soexception_namebecomes" ". This is legacy data only, and the P2-A fix (strip, thenor None) also covers it.Acceptance criteria
trustExceptionMinEntriesLabelin EN and ES): MET.min_length=1, null for no name, tested): MET at the/holidaysAPI boundary. PARTIAL for derived payloads and stored data (see P2-A).Verdict
Both pass-1 P2s are genuinely fixed. There is one new P2 (P2-A, an invented ISO-date name for unnamed exceptions in business-day records and live info) and two P3s. Rebase onto master for the trivial import conflict.
Pass 2 fixes (
bd56307)Addressed review r55 in PR #222.
or day.isoformat()inoccupancy_service.py(get_schedule_calendar_asyncand_planned_schedule_for_day_async). Stripped names and set toNoneif blank so stampedexception_name, database rows, and live schedule payload remain null. Updatedtests/test_occupancy.pyto assert null names and weekday fallback inschedule_label, and addedtest_unnamed_exception_stamped_and_live_remain_nullintests/test_next_day_settings_activation.py.BusinessDayScheduleCorrectiondocstring (app/schemas/occupancy_models.py) andCONTEXT.mdthat manual admin corrections of exception days require an explicit non-emptyexception_namefor audit accountability and historical clarity.""for unnamed exceptionsHolidayItem.normalize_stored_name"Response/read model: legacy blank names read as null; request validation lives in HolidayCreateOrUpdate."timespanTodayto"Jornada (24H)"(i18n.js:79), updatedtimespanDescto"jornada comercial vs jornada completa de 24 horas"(i18n.js:228), and updated empty table fallback inapp.js:4702to"Sin jornadas registradas". Addedoccupancy.timespanTodayassertion to Spanish tests intest_business_cycle_i18n.test.js. Left "Ciclos de Puertas" intact.row.get("name")beforeor Noneinoccupancy_service.py, ensuring stored whitespace strings evaluate to null across calendar planning and stamping.9a608c1)origin/master(9a608c1) cleanly; resolved the import conflict intests/test_occupancy.pykeeping all imports:CameraGroupRegistration, DirectionType, HolidayCreateOrUpdate, HolidayItem, TimespanPreset.target_date="2099-12-25"typo intest_legacy_blank_exception_names_are_null_on_getto match payload.Verification:
pytest.ruff check .,ruff format --check .).Please perform review pass 3 against
bd56307.Code review, pass 3 (
origin/master...bd56307, spec #184 + #258)Summary: no P1/P2. Every r55 finding is fixed. Ready to merge. P3s are filed as #274; the frontend fallback for unnamed exceptions is noted on #73.
Must fix: none.
Checks on a git-archive export of
bd56307:{}, then the pending listing, next-day activation, the freeze, a past-day stamp, GET/schedule/days/{d}, the audit row and/live. The name is null on every path, including legacy rows stored as"", whitespace or tab. An admin correction withis_exceptionand no name returns 422, as now documented.Standards axis
Verdict on r55
occupancy_service.py:1254-1256,:1283,:1295), with a new ASGI testoccupancy_models.py:398-403) and CONTEXT.md:131 agree285b130)""stored for "no name"""or a date.New findings (P3)
HolidayItem.normalize_stored_name(occupancy_models.py:493-496), occupancy_service.py:1254 and :1283. The one at :1283 also depends onA or B if C else Dprecedence. Extract one helper.test_unnamed_exception_stamped_and_live_remain_null(test_next_day_settings_activation.py:1555) never checks live info. Assert that/api/occupancy/livehasholiday_name is None, or rename the test.2026-01-15. Derive the date relative to now.Spec axis
All acceptance criteria are met: #184 items 1, 2, 3 and 7, plus #258 (the working-hours label, "business cycle" in English, "jornada" in Spanish). There is no scope creep.
New findings (P3)
operationalCycleDesc) for the business cycle. Reword :230 to "...Mantiene unida la jornada nocturna". This predates the PR.''and the request returns 422. This is the intended rule. Add a client-side hint before submitting.