From 8065e6effa94020df8dbe56bb4f84664bb56c5da Mon Sep 17 00:00:00 2001 From: "alexei.dolgolyov" Date: Fri, 29 May 2026 14:57:41 +0300 Subject: [PATCH] feat(immich): multi-time-point scheduling for scheduled/periodic/memory Replace the single comma-separated text box with an add/remove list of native HH:MM pickers for the Immich scheduled-assets, periodic-summary, and memory slots. The backend already stored comma-separated *_times and scheduled one cron job per time; this makes entering several times per day discoverable and hardens the read/write path. Backend: - services/time_list.py: normalize_time_list (validate / dedup / sort / cap at 24, raising TimeListError) + lenient parse_hhmm_list; the scheduler now uses the shared parser (drops its private copy). - tracking_configs API normalizes *_times on every write (422 on bad input) and rejects enabling a slot whose times list is empty. - scheduler warns when an enabled slot has zero or dropped fire times, restoring the observability lost with the old per-call warning. Frontend: - TimeListEditor.svelte: add/remove native time rows, dedupe + sort on emit, per-day cap, collapses on-screen duplicates, aria-labelled rows; syncs from the value prop only on external changes (untrack guard) so keyboard entry isn't clobbered mid-edit. - Descriptor-driven save guard: an enabled feature section must have at least one time. - i18n (en/ru) keys; refreshed help text; removed dead invalidTimeList. Tests: time_list normalization/parsing (incl. non-ASCII/odd shapes) and the enabled-implies-times validation. --- .../src/lib/components/TimeListEditor.svelte | 252 ++++++++++++++++++ frontend/src/lib/i18n/en.json | 11 +- frontend/src/lib/i18n/ru.json | 11 +- frontend/src/lib/providers/immich.ts | 6 +- frontend/src/lib/providers/types.ts | 5 +- .../src/routes/tracking-configs/+page.svelte | 83 +++--- .../api/tracking_configs.py | 65 ++++- .../services/scheduler.py | 45 ++-- .../services/time_list.py | 99 +++++++ packages/server/tests/test_time_list.py | 116 ++++++++ .../tests/test_tracking_config_validation.py | 114 ++++++++ 11 files changed, 718 insertions(+), 89 deletions(-) create mode 100644 frontend/src/lib/components/TimeListEditor.svelte create mode 100644 packages/server/src/notify_bridge_server/services/time_list.py create mode 100644 packages/server/tests/test_time_list.py create mode 100644 packages/server/tests/test_tracking_config_validation.py diff --git a/frontend/src/lib/components/TimeListEditor.svelte b/frontend/src/lib/components/TimeListEditor.svelte new file mode 100644 index 0000000..7d10d67 --- /dev/null +++ b/frontend/src/lib/components/TimeListEditor.svelte @@ -0,0 +1,252 @@ + + +
+ {#each rows as _, i (i)} +
+ + +
+ {/each} + + {#if rows.length === 0} +

+ + {t('trackingConfig.noTimes')} +

+ {/if} + + {#if atMax} +

{t('trackingConfig.maxTimesReached').replace('{n}', String(max))}

+ {:else} + + {/if} +
+ + diff --git a/frontend/src/lib/i18n/en.json b/frontend/src/lib/i18n/en.json index e4101da..6f55e2c 100644 --- a/frontend/src/lib/i18n/en.json +++ b/frontend/src/lib/i18n/en.json @@ -728,8 +728,13 @@ "deleted": "deleted", "providerType": "Provider Type", "sortRandom": "Random", - "timesInlineHelp": "HH:MM, comma-separated", - "invalidTimeList": "Use HH:MM format, e.g. 09:00 or 09:00, 18:30", + "timesInlineHelp": "One or more times per day", + "addTime": "Add time", + "removeTime": "Remove time", + "timeRowLabel": "Time {n}", + "noTimes": "No times set — add at least one", + "maxTimesReached": "Maximum {n} times reached", + "timesRequiredFor": "Add at least one time for \"{slot}\"", "previewTemplate": "Preview template", "previewSampleNote": "Rendered with sample data — not your real assets. Shows the shipped default template.", "editTemplate": "Edit template", @@ -1011,7 +1016,7 @@ "maxAssets": "Maximum number of asset details to include in a single notification message.", "periodicStartDate": "Reference date in the app timezone. The first summary fires at the next configured HH:MM on/after this date, then every N days.", "intervalDays": "Days between successive summaries. 1 = daily, 7 = weekly.", - "times": "Time(s) of day to send notifications, in HH:MM format. Use commas for multiple times: 09:00,18:00", + "times": "Time(s) of day to send notifications. Add as many time points per day as you need.", "albumMode": "Per album: separate notification per album. Combined: one notification with all albums. Random: pick one album randomly.", "scheduledAlbumMode": "How albums are grouped in scheduled deliveries. Default: Per album (one notification per tracked album).", "memoryAlbumMode": "How albums are grouped in memory deliveries. Default: Combined (a single notification aggregating matches from all tracked albums).", diff --git a/frontend/src/lib/i18n/ru.json b/frontend/src/lib/i18n/ru.json index e6bc3e3..2b63e44 100644 --- a/frontend/src/lib/i18n/ru.json +++ b/frontend/src/lib/i18n/ru.json @@ -728,8 +728,13 @@ "deleted": "удалён", "providerType": "Тип провайдера", "sortRandom": "Случайный", - "timesInlineHelp": "ЧЧ:ММ, через запятую", - "invalidTimeList": "Используйте формат ЧЧ:ММ, например 09:00 или 09:00, 18:30", + "timesInlineHelp": "Одно или несколько значений времени в день", + "addTime": "Добавить время", + "removeTime": "Удалить время", + "timeRowLabel": "Время {n}", + "noTimes": "Время не задано — добавьте хотя бы одно", + "maxTimesReached": "Достигнут максимум: {n}", + "timesRequiredFor": "Добавьте хотя бы одно время для «{slot}»", "previewTemplate": "Предпросмотр шаблона", "previewSampleNote": "Отрисовано на демо-данных, не на ваших реальных фото. Показан шаблон по умолчанию.", "editTemplate": "Редактировать шаблон", @@ -1011,7 +1016,7 @@ "maxAssets": "Максимальное количество ассетов в одном уведомлении.", "periodicStartDate": "Опорная дата в часовом поясе приложения. Первая сводка отправится в ближайшее заданное время ЧЧ:ММ, начиная с этой даты, затем каждые N дней.", "intervalDays": "Период между сводками в днях. 1 = ежедневно, 7 = еженедельно.", - "times": "Время отправки уведомлений в формате ЧЧ:ММ. Для нескольких значений через запятую: 09:00,18:00", + "times": "Время отправки уведомлений. Добавьте сколько угодно значений времени в день.", "albumMode": "По альбому: отдельное уведомление для каждого. Объединённый: одно уведомление со всеми. Случайный: выбирается один альбом.", "scheduledAlbumMode": "Как альбомы группируются в запланированных отправках. По умолчанию: По альбому (одно уведомление на каждый отслеживаемый альбом).", "memoryAlbumMode": "Как альбомы группируются в воспоминаниях. По умолчанию: Объединённый (одно уведомление со всеми совпадениями из всех альбомов).", diff --git a/frontend/src/lib/providers/immich.ts b/frontend/src/lib/providers/immich.ts index 3639019..c725591 100644 --- a/frontend/src/lib/providers/immich.ts +++ b/frontend/src/lib/providers/immich.ts @@ -68,14 +68,14 @@ export const immichDescriptor: ProviderDescriptor = { fields: [ { key: 'periodic_interval_days', label: 'trackingConfig.intervalDays', type: 'number', min: 1, defaultValue: 1, hint: 'hints.intervalDays' }, { key: 'periodic_start_date', label: 'trackingConfig.startDate', type: 'date', defaultValue: todayIso, hint: 'hints.periodicStartDate' }, - { key: 'periodic_times', label: 'trackingConfig.times', type: 'time-list', defaultValue: '12:00', hint: 'hints.times', inlineHelp: 'trackingConfig.timesInlineHelp', validateFormat: true }, + { key: 'periodic_times', label: 'trackingConfig.times', type: 'time-list', defaultValue: '12:00', hint: 'hints.times', inlineHelp: 'trackingConfig.timesInlineHelp' }, ], }, { key: 'scheduled', legend: 'trackingConfig.scheduledAssets', legendHint: 'hints.scheduledAssets', enabledField: 'scheduled_enabled', enabledDefault: false, fields: [ - { key: 'scheduled_times', label: 'trackingConfig.times', type: 'time-list', defaultValue: '09:00', hint: 'hints.times', inlineHelp: 'trackingConfig.timesInlineHelp', validateFormat: true }, + { key: 'scheduled_times', label: 'trackingConfig.times', type: 'time-list', defaultValue: '09:00', hint: 'hints.times', inlineHelp: 'trackingConfig.timesInlineHelp' }, { key: 'scheduled_collection_mode', label: 'trackingConfig.albumMode', type: 'grid-select', gridItems: 'albumModeItems', gridColumns: 3, defaultValue: 'per_collection', hint: 'hints.scheduledAlbumMode' }, { key: 'scheduled_limit', label: 'trackingConfig.maxAssets', type: 'number', min: 1, max: 100, defaultValue: 10, hint: 'hints.maxAssets' }, { key: 'scheduled_asset_type', label: 'trackingConfig.assetType', type: 'grid-select', gridItems: 'assetTypeItems', gridColumns: 3, defaultValue: 'all' }, @@ -87,7 +87,7 @@ export const immichDescriptor: ProviderDescriptor = { key: 'memory', legend: 'trackingConfig.memoryMode', legendHint: 'hints.memoryMode', enabledField: 'memory_enabled', enabledDefault: false, fields: [ - { key: 'memory_times', label: 'trackingConfig.times', type: 'time-list', defaultValue: '09:00', hint: 'hints.times', inlineHelp: 'trackingConfig.timesInlineHelp', validateFormat: true }, + { key: 'memory_times', label: 'trackingConfig.times', type: 'time-list', defaultValue: '09:00', hint: 'hints.times', inlineHelp: 'trackingConfig.timesInlineHelp' }, { key: 'memory_collection_mode', label: 'trackingConfig.albumMode', type: 'grid-select', gridItems: 'albumModeItems', gridColumns: 3, defaultValue: 'combined', hint: 'hints.memoryAlbumMode' }, { key: 'memory_limit', label: 'trackingConfig.maxAssets', type: 'number', min: 1, max: 100, defaultValue: 10, hint: 'hints.maxAssets' }, { key: 'memory_asset_type', label: 'trackingConfig.assetType', type: 'grid-select', gridItems: 'assetTypeItems', gridColumns: 3, defaultValue: 'all' }, diff --git a/frontend/src/lib/providers/types.ts b/frontend/src/lib/providers/types.ts index 2183cda..abe960e 100644 --- a/frontend/src/lib/providers/types.ts +++ b/frontend/src/lib/providers/types.ts @@ -67,7 +67,8 @@ export interface ExtraTrackingField { * - `toggle` — on/off switch * - `date` — HTML date picker (YYYY-MM-DD) * - `time` — HTML time picker (HH:MM) - * - `time-list` — comma-separated HH:MM list, validated on blur + * - `time-list` — add/remove list of HH:MM pickers (TimeListEditor), + * serialized as a comma-separated string */ type: 'number' | 'grid-select' | 'toggle' | 'date' | 'time' | 'time-list'; /** Grid-select item source function name from grid-items.ts. */ @@ -78,8 +79,6 @@ export interface ExtraTrackingField { inlineHelp?: string; min?: number; max?: number; - /** For time-list: show live validation + auto-normalize on blur. */ - validateFormat?: boolean; /** * Default value. Can be a function for dynamic values (e.g. today's date) * evaluated each time the form is reset. diff --git a/frontend/src/routes/tracking-configs/+page.svelte b/frontend/src/routes/tracking-configs/+page.svelte index 3ef934d..b741389 100644 --- a/frontend/src/routes/tracking-configs/+page.svelte +++ b/frontend/src/routes/tracking-configs/+page.svelte @@ -27,6 +27,7 @@ import ErrorBanner from '$lib/components/ErrorBanner.svelte'; import Button from '$lib/components/Button.svelte'; import MetaStrip, { type MetaTile } from '$lib/components/MetaStrip.svelte'; + import TimeListEditor from '$lib/components/TimeListEditor.svelte'; /** Grid-select item source lookup — maps descriptor string name to actual function. */ const gridItemSources: Record any[]> = { @@ -34,44 +35,12 @@ }; /** - * HH:MM, comma-separated: "09:00" or "09:00, 18:30" — the only format cron - * dispatch accepts. Matched on blur for time-list fields; invalid values - * are surfaced inline next to the input. + * Max distinct dispatch times per slot — mirrors the backend cap + * (`MAX_DISPATCH_TIMES` in services/time_list.py). The TimeListEditor + * disables "+ Add time" once this many rows are filled; the server enforces + * the same limit on write. */ - const TIME_LIST_RE = /^\s*(?:[01]\d|2[0-3]):[0-5]\d(?:\s*,\s*(?:[01]\d|2[0-3]):[0-5]\d)*\s*$/; - - /** Per-field error messages surfaced inline under time-list inputs. */ - let timeListErrors = $state>({}); - - /** Normalize "9:0 , 18:30" → "09:00,18:30" on blur, clear error when valid. */ - function normalizeTimeList(key: string) { - const raw = String(form[key] ?? '').trim(); - if (!raw) { timeListErrors = { ...timeListErrors, [key]: '' }; return; } - if (!TIME_LIST_RE.test(raw)) { - // Try a lenient normalization: split on commas, zero-pad each part. - const parts = raw.split(',').map(p => p.trim()).filter(Boolean); - const fixed: string[] = []; - let ok = true; - for (const p of parts) { - const m = /^(\d{1,2}):(\d{1,2})$/.exec(p); - if (!m) { ok = false; break; } - const hh = Number(m[1]); - const mm = Number(m[2]); - if (!Number.isFinite(hh) || !Number.isFinite(mm) || hh < 0 || hh > 23 || mm < 0 || mm > 59) { ok = false; break; } - fixed.push(`${String(hh).padStart(2, '0')}:${String(mm).padStart(2, '0')}`); - } - if (ok) { - form[key] = fixed.join(','); - timeListErrors = { ...timeListErrors, [key]: '' }; - return; - } - timeListErrors = { ...timeListErrors, [key]: t('trackingConfig.invalidTimeList') }; - return; - } - // Canonicalise spacing. - form[key] = raw.split(',').map(s => s.trim()).join(','); - timeListErrors = { ...timeListErrors, [key]: '' }; - } + const MAX_DISPATCH_TIMES = 24; /** * Quiet-hours preview: "22:00 → 07:00 next day (9h)" or "Quiet period is 0 @@ -280,6 +249,18 @@ async function save(e: SubmitEvent) { e.preventDefault(); error = ''; + // Descriptor-driven guard: an enabled feature section that uses a + // time-list must have at least one time, otherwise it saves but the + // scheduler creates no cron job and the slot silently never fires. + for (const section of descriptor?.featureSections ?? []) { + const timeField = section.fields.find((f) => f.type === 'time-list'); + if (!timeField) continue; + if (form[section.enabledField] && !String(form[timeField.key] ?? '').trim()) { + const msg = t('trackingConfig.timesRequiredFor').replace('{slot}', t(section.legend)); + error = msg; snackError(msg); + return; + } + } try { if (editing) await api(`/tracking-configs/${editing}`, { method: 'PUT', body: JSON.stringify(form) }); else await api('/tracking-configs', { method: 'POST', body: JSON.stringify(form) }); @@ -413,22 +394,24 @@ {:else if field.type === 'grid-select' && field.gridItems} - {:else} - {@const inputType = field.type === 'date' ? 'date' - : field.type === 'time' ? 'time' - : field.type === 'time-list' ? 'text' - : 'number'} - {@const hasError = field.type === 'time-list' && !!timeListErrors[field.key]} - normalizeTimeList(field.key) : undefined} - placeholder={field.type === 'time-list' || field.type === 'time' ? String(typeof field.defaultValue === 'function' ? field.defaultValue() : (field.defaultValue ?? '')) : ''} - class="w-full px-2 py-1 border rounded-md text-sm bg-[var(--color-background)] {hasError ? 'border-[var(--color-error-fg)]' : 'border-[var(--color-border)]'}" /> + {:else if field.type === 'time-list'} + form[field.key] = v} + max={MAX_DISPATCH_TIMES} /> {#if field.inlineHelp}

{t(field.inlineHelp)}

{/if} - {#if hasError} -

{timeListErrors[field.key]}

+ {:else} + {@const inputType = field.type === 'date' ? 'date' + : field.type === 'time' ? 'time' + : 'number'} + + {#if field.inlineHelp} +

{t(field.inlineHelp)}

{/if} {/if} diff --git a/packages/server/src/notify_bridge_server/api/tracking_configs.py b/packages/server/src/notify_bridge_server/api/tracking_configs.py index 0bdf70b..e1cf504 100644 --- a/packages/server/src/notify_bridge_server/api/tracking_configs.py +++ b/packages/server/src/notify_bridge_server/api/tracking_configs.py @@ -11,11 +11,66 @@ from ..auth.dependencies import get_current_user from ..database.engine import get_session from ..database.models import TrackingConfig, User from ..services.scheduler import reschedule_immich_dispatch_jobs +from ..services.time_list import TimeListError, normalize_time_list _LOGGER = logging.getLogger(__name__) router = APIRouter(prefix="/api/tracking-configs", tags=["tracking-configs"]) +# Immich dispatch slots that fire on a wall-clock schedule. Each has a +# ``{kind}_enabled`` flag and a ``{kind}_times`` comma-separated HH:MM list. +_DISPATCH_KINDS = ("periodic", "scheduled", "memory") +# TrackingConfig fields holding comma-separated ``HH:MM`` dispatch schedules. +# Normalized (validated, de-duplicated, sorted, capped) on every write so the +# scheduler only ever reads clean values and cron jobs stay deterministic. +_TIME_LIST_FIELDS = tuple(f"{k}_times" for k in _DISPATCH_KINDS) + + +def _normalize_time_fields(values: dict) -> None: + """Canonicalize any ``*_times`` keys present in ``values`` (in place). + + Raises HTTP 422 with a field-scoped message when an entry is malformed or + the list exceeds the per-day cap, so the client surfaces exactly which slot + was rejected instead of the input being silently dropped at schedule time. + """ + for field in _TIME_LIST_FIELDS: + if values.get(field) is None: + continue + try: + values[field] = normalize_time_list(values[field]) + except TimeListError as err: + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"{field}: {err}", + ) from err + + +def _validate_enabled_have_times(values: dict, existing: TrackingConfig | None) -> None: + """Reject enabling a dispatch slot with no fire times. + + An enabled slot whose ``{kind}_times`` normalizes to empty would save fine + but the scheduler creates zero cron jobs for it — a silently dead slot that + shows as "enabled" in the UI yet never fires. We fail the write with a 422 + instead. Only kinds the request actually touches (enabled flag or times) are + checked, so unrelated edits to a pre-existing config aren't blocked; the + effective state is the request value merged over ``existing``. + """ + for kind in _DISPATCH_KINDS: + enabled_key, times_key = f"{kind}_enabled", f"{kind}_times" + if enabled_key not in values and times_key not in values: + continue + enabled = values.get( + enabled_key, getattr(existing, enabled_key, False) if existing else False + ) + times = values.get( + times_key, getattr(existing, times_key, "") if existing else "" + ) + if enabled and not (times or "").strip(): + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, + detail=f"{times_key}: add at least one time when {kind} is enabled", + ) + class TrackingConfigCreate(BaseModel): provider_type: str @@ -124,7 +179,10 @@ async def create_config( user: User = Depends(get_current_user), session: AsyncSession = Depends(get_session), ): - config = TrackingConfig(user_id=user.id, **body.model_dump()) + data = body.model_dump() + _normalize_time_fields(data) + _validate_enabled_have_times(data, None) + config = TrackingConfig(user_id=user.id, **data) session.add(config) await session.commit() await session.refresh(config) @@ -150,7 +208,10 @@ async def update_config( session: AsyncSession = Depends(get_session), ): config = await _get(session, config_id, user.id) - for field, value in body.model_dump(exclude_unset=True).items(): + updates = body.model_dump(exclude_unset=True) + _normalize_time_fields(updates) + _validate_enabled_have_times(updates, config) + for field, value in updates.items(): setattr(config, field, value) session.add(config) await session.commit() diff --git a/packages/server/src/notify_bridge_server/services/scheduler.py b/packages/server/src/notify_bridge_server/services/scheduler.py index 39defdd..81620b9 100644 --- a/packages/server/src/notify_bridge_server/services/scheduler.py +++ b/packages/server/src/notify_bridge_server/services/scheduler.py @@ -7,6 +7,8 @@ from zoneinfo import ZoneInfo, ZoneInfoNotFoundError from apscheduler.schedulers.asyncio import AsyncIOScheduler +from .time_list import parse_hhmm_list + _LOGGER = logging.getLogger(__name__) @@ -896,30 +898,6 @@ _IMMICH_DISPATCH_KINDS = ("scheduled", "periodic", "memory") _IMMICH_DISPATCH_PREFIX = "immich_dispatch_" -def _parse_hhmm_list(raw: str) -> list[tuple[int, int]]: - """Parse ``"09:00,18:30"`` → ``[(9, 0), (18, 30)]``, skipping bad entries. - - A typo in one slot must not prevent the others from scheduling — we log - and move on rather than raising. - """ - out: list[tuple[int, int]] = [] - for part in (raw or "").split(","): - part = part.strip() - if not part: - continue - try: - h_str, m_str = part.split(":", 1) - hour, minute = int(h_str), int(m_str) - except ValueError: - _LOGGER.warning("Skipping invalid time literal %r", part) - continue - if not (0 <= hour <= 23 and 0 <= minute <= 59): - _LOGGER.warning("Skipping out-of-range time %r", part) - continue - out.append((hour, minute)) - return out - - async def _run_immich_dispatch(tracker_id: int, kind: str) -> None: """APScheduler entry point — wraps the dispatch helper to swallow errors.""" from .scheduled_dispatch import dispatch_scheduled_for_tracker @@ -994,7 +972,24 @@ async def _load_immich_dispatch_jobs() -> None: if not getattr(tc, f"{kind}_enabled", False): continue times_raw = getattr(tc, f"{kind}_times", "") or "" - for hour, minute in _parse_hhmm_list(times_raw): + parsed = parse_hhmm_list(times_raw) + # Observability for misconfigured/legacy data: warn when some tokens + # were unparseable (the lenient parser drops them silently) and when + # an enabled slot resolves to zero fire times (it will never fire). + raw_tokens = [p for p in times_raw.split(",") if p.strip()] + if len(parsed) < len(raw_tokens): + _LOGGER.warning( + "Tracker %d %s: dropped %d unparseable time(s) from %r", + tracker.id, kind, len(raw_tokens) - len(parsed), times_raw, + ) + if not parsed: + _LOGGER.warning( + "Tracker %d has %s enabled but no valid fire times (%r); " + "slot will not fire", + tracker.id, kind, times_raw, + ) + continue + for hour, minute in parsed: job_id = f"{_IMMICH_DISPATCH_PREFIX}{kind}_{tracker.id}_{hour:02d}{minute:02d}" scheduler.add_job( _run_immich_dispatch, diff --git a/packages/server/src/notify_bridge_server/services/time_list.py b/packages/server/src/notify_bridge_server/services/time_list.py new file mode 100644 index 0000000..75af713 --- /dev/null +++ b/packages/server/src/notify_bridge_server/services/time_list.py @@ -0,0 +1,99 @@ +"""Parsing + normalization for comma-separated ``HH:MM`` dispatch time lists. + +The Immich scheduled / periodic / memory slots fire at one or more wall-clock +times per day, stored on ``TrackingConfig`` as a comma-separated ``HH:MM`` +string (e.g. ``"09:00,18:30"``). Two consumers share this module: + +* the API layer (``api.tracking_configs``) calls :func:`normalize_time_list` + to validate + canonicalize user input before persisting — rejecting malformed + entries, de-duplicating, sorting ascending, and capping the count. +* the scheduler (``services.scheduler``) calls :func:`parse_hhmm_list` to turn a + stored (already-normalized) string into ``(hour, minute)`` tuples, tolerating + legacy or hand-edited values by skipping anything unparseable rather than + letting one bad slot stop the others from firing. +""" + +from __future__ import annotations + +# A generous ceiling: 24 distinct fire times per day is already an unusual +# config. The cap mainly exists so a pathological paste can't spawn hundreds of +# cron jobs for a single tracker. +MAX_DISPATCH_TIMES = 24 + + +class TimeListError(ValueError): + """Raised by :func:`normalize_time_list` on malformed input or over-cap. + + Subclasses ``ValueError`` so callers that only care about "bad input" can + catch the broad type; the API layer catches this specifically to map it to + an HTTP 422 with the human-readable message. + """ + + +def _parse_one(token: str) -> tuple[int, int] | None: + """Parse a single ``HH:MM`` token; return ``None`` if malformed/out-of-range. + + Strict on shape (exactly one ``:``, both parts 1-2 plain ASCII digits) but + lenient on width so ``"9:0"`` parses to ``(9, 0)`` — :func:`normalize_time_list` + re-emits it zero-padded. ``int()`` alone is too permissive here: it would + accept signs (``"+9"``), PEP 515 underscores (``"1_0"``), and non-ASCII + decimal digits (Arabic-Indic ``"٠٩"``), none of which are valid wall-clock + literals — so we gate on ``str.isdigit()``/ASCII before converting. + """ + h_str, sep, m_str = token.partition(":") + if not sep: + return None + for part in (h_str, m_str): + if not (1 <= len(part) <= 2 and part.isascii() and part.isdigit()): + return None + hour, minute = int(h_str), int(m_str) + if not (0 <= hour <= 23 and 0 <= minute <= 59): + return None + return hour, minute + + +def parse_hhmm_list(raw: str) -> list[tuple[int, int]]: + """Lenient parse: ``"09:00,18:30"`` → ``[(9, 0), (18, 30)]``. + + Skips blank/invalid entries rather than raising — the scheduler must keep + firing the valid times even if one slot was hand-edited to garbage. Order is + preserved and duplicates are *not* collapsed (the scheduler keys jobs by + time, so a duplicate simply replaces its own job id). + """ + out: list[tuple[int, int]] = [] + for part in (raw or "").split(","): + part = part.strip() + if not part: + continue + hm = _parse_one(part) + if hm is not None: + out.append(hm) + return out + + +def normalize_time_list(raw: str, *, max_count: int = MAX_DISPATCH_TIMES) -> str: + """Validate + canonicalize a comma-separated ``HH:MM`` list. + + Returns the canonical form: zero-padded, de-duplicated, sorted ascending, + and comma-joined with no spaces — e.g. ``" 9:0, 18:30 ,09:00"`` → + ``"09:00,18:30"``. An empty/whitespace input returns ``""`` (the valid + "no scheduled fires" state). + + Raises :class:`TimeListError` when any entry is not a valid ``HH:MM`` time, + or when the de-duplicated count exceeds ``max_count``. The caller maps this + to an HTTP 422 so the user gets a clear message instead of silent dropping. + """ + seen: set[tuple[int, int]] = set() + for part in (raw or "").split(","): + part = part.strip() + if not part: + continue + hm = _parse_one(part) + if hm is None: + raise TimeListError(f"Invalid time {part!r}; use HH:MM (00:00-23:59)") + seen.add(hm) + if len(seen) > max_count: + raise TimeListError( + f"Too many times ({len(seen)}); at most {max_count} are allowed" + ) + return ",".join(f"{h:02d}:{m:02d}" for h, m in sorted(seen)) diff --git a/packages/server/tests/test_time_list.py b/packages/server/tests/test_time_list.py new file mode 100644 index 0000000..594afdb --- /dev/null +++ b/packages/server/tests/test_time_list.py @@ -0,0 +1,116 @@ +"""Unit tests for the shared HH:MM dispatch time-list parser/normalizer.""" + +from __future__ import annotations + +import pytest + +from notify_bridge_server.services.time_list import ( + MAX_DISPATCH_TIMES, + TimeListError, + normalize_time_list, + parse_hhmm_list, +) + + +class TestNormalizeTimeList: + def test_single_time_passthrough(self): + assert normalize_time_list("09:00") == "09:00" + + def test_zero_pads_short_parts(self): + assert normalize_time_list("9:0") == "09:00" + + def test_trims_surrounding_and_inner_whitespace(self): + assert normalize_time_list(" 09:00 , 18:30 ") == "09:00,18:30" + + def test_sorts_ascending(self): + assert normalize_time_list("18:30,09:00,12:15") == "09:00,12:15,18:30" + + def test_deduplicates(self): + # Duplicates collapse even when written with different padding. + assert normalize_time_list("09:00,9:00,09:00") == "09:00" + + def test_empty_string_returns_empty(self): + assert normalize_time_list("") == "" + + def test_whitespace_only_returns_empty(self): + assert normalize_time_list(" ") == "" + + def test_trailing_comma_ignored(self): + assert normalize_time_list("09:00,") == "09:00" + + def test_midnight_and_last_minute_are_valid(self): + assert normalize_time_list("00:00,23:59") == "00:00,23:59" + + @pytest.mark.parametrize( + "bad", + ["24:00", "09:60", "noon", "9", "09-00", "-1:00", "09:5a", "1:2:3"], + ) + def test_invalid_entry_raises(self, bad): + with pytest.raises(TimeListError): + normalize_time_list(bad) + + @pytest.mark.parametrize( + "bad", + [ + "+9:00", # sign not allowed + "1_0:00", # PEP 515 underscore not allowed + "09:0_0", # underscore in minutes + "٩:00", # Arabic-Indic digit (non-ASCII) + "٠٩:٠٠", # Arabic-Indic "09:00" + "09: 00", # inner whitespace in part + "9 :00", # inner whitespace in part + "009:00", # 3-digit hour part + ], + ) + def test_rejects_non_ascii_or_oddly_shaped_digits(self, bad): + # int() alone would accept these; the parser must not. + with pytest.raises(TimeListError): + normalize_time_list(bad) + + def test_one_bad_entry_rejects_whole_list(self): + # Strict on writes: a single bad slot fails the request rather than + # being silently dropped. + with pytest.raises(TimeListError): + normalize_time_list("09:00,bad,18:30") + + def test_at_cap_is_allowed(self): + times = ",".join(f"{h:02d}:00" for h in range(MAX_DISPATCH_TIMES)) + assert normalize_time_list(times) == times + + def test_over_cap_raises(self): + # 25 distinct times (minute-granular) exceeds the default cap of 24. + times = ",".join(f"00:{m:02d}" for m in range(MAX_DISPATCH_TIMES + 1)) + with pytest.raises(TimeListError): + normalize_time_list(times) + + def test_custom_max_count(self): + with pytest.raises(TimeListError): + normalize_time_list("09:00,10:00,11:00", max_count=2) + + def test_duplicates_do_not_count_against_cap(self): + # Three entries but one distinct → fits under a cap of 1. + assert normalize_time_list("09:00,09:00,9:00", max_count=1) == "09:00" + + +class TestParseHhmmList: + def test_basic(self): + assert parse_hhmm_list("09:00,18:30") == [(9, 0), (18, 30)] + + def test_preserves_order_and_duplicates(self): + # Lenient parser keeps input order and does not collapse duplicates — + # the scheduler keys jobs by time so a dup just replaces its own id. + assert parse_hhmm_list("18:30,09:00,18:30") == [(18, 30), (9, 0), (18, 30)] + + def test_skips_invalid_entries(self): + assert parse_hhmm_list("09:00,bad,18:30") == [(9, 0), (18, 30)] + + def test_empty_returns_empty_list(self): + assert parse_hhmm_list("") == [] + assert parse_hhmm_list(" ") == [] + + def test_skips_out_of_range(self): + assert parse_hhmm_list("24:00,09:00") == [(9, 0)] + + def test_skips_non_ascii_and_oddly_shaped(self): + # Lenient parser drops the odd shapes but keeps the valid neighbour. + assert parse_hhmm_list("+9:00,1_0:00,٠٩:٠٠,18:30") == [(18, 30)] diff --git a/packages/server/tests/test_tracking_config_validation.py b/packages/server/tests/test_tracking_config_validation.py new file mode 100644 index 0000000..7e07c38 --- /dev/null +++ b/packages/server/tests/test_tracking_config_validation.py @@ -0,0 +1,114 @@ +"""Tests for the tracking-config write-time guards. + +Covers the cross-field rule that an *enabled* Immich dispatch slot +(periodic / scheduled / memory) must carry at least one fire time — otherwise +the config saves but the scheduler creates zero cron jobs and the slot is +silently dead. +""" + +from __future__ import annotations + +import pytest +from fastapi import HTTPException + +from notify_bridge_server.api.tracking_configs import ( + _normalize_time_fields, + _validate_enabled_have_times, +) +from notify_bridge_server.database.models import TrackingConfig + + +class TestNormalizeTimeFields: + def test_normalizes_each_field_in_place(self): + values = { + "periodic_times": "18:30, 09:00", + "scheduled_times": "9:0", + "memory_times": "", + } + _normalize_time_fields(values) + assert values["periodic_times"] == "09:00,18:30" + assert values["scheduled_times"] == "09:00" + assert values["memory_times"] == "" + + def test_ignores_absent_fields(self): + values = {"name": "x"} + _normalize_time_fields(values) # no KeyError, no-op + assert values == {"name": "x"} + + def test_malformed_raises_422(self): + with pytest.raises(HTTPException) as exc: + _normalize_time_fields({"scheduled_times": "25:00"}) + assert exc.value.status_code == 422 + assert "scheduled_times" in str(exc.value.detail) + + +class TestValidateEnabledHaveTimes: + # --- create path (existing=None) --- + def test_create_enabled_with_times_ok(self): + _validate_enabled_have_times( + {"scheduled_enabled": True, "scheduled_times": "09:00"}, None + ) + + def test_create_enabled_without_times_raises(self): + with pytest.raises(HTTPException) as exc: + _validate_enabled_have_times( + {"scheduled_enabled": True, "scheduled_times": ""}, None + ) + assert exc.value.status_code == 422 + assert "scheduled_times" in str(exc.value.detail) + + def test_create_disabled_without_times_ok(self): + _validate_enabled_have_times( + {"memory_enabled": False, "memory_times": ""}, None + ) + + def test_all_three_kinds_checked(self): + for kind in ("periodic", "scheduled", "memory"): + with pytest.raises(HTTPException): + _validate_enabled_have_times( + {f"{kind}_enabled": True, f"{kind}_times": ""}, None + ) + + # --- update path (effective state = values merged over existing) --- + def test_update_enable_without_providing_times_uses_existing_empty(self): + existing = TrackingConfig(provider_type="immich", name="t", scheduled_times="") + with pytest.raises(HTTPException): + _validate_enabled_have_times({"scheduled_enabled": True}, existing) + + def test_update_enable_with_existing_times_ok(self): + existing = TrackingConfig( + provider_type="immich", name="t", scheduled_times="09:00" + ) + _validate_enabled_have_times({"scheduled_enabled": True}, existing) + + def test_update_clear_times_while_enabled_in_existing_raises(self): + existing = TrackingConfig( + provider_type="immich", + name="t", + scheduled_enabled=True, + scheduled_times="09:00", + ) + with pytest.raises(HTTPException): + _validate_enabled_have_times({"scheduled_times": ""}, existing) + + def test_update_unrelated_field_does_not_trigger_check(self): + # A pre-existing enabled-but-empty config must not block edits that don't + # touch the slot's enabled flag or times (only touched kinds are checked). + existing = TrackingConfig( + provider_type="immich", + name="t", + scheduled_enabled=True, + scheduled_times="", + ) + _validate_enabled_have_times({"name": "renamed"}, existing) + + def test_update_disabling_slot_clears_requirement(self): + existing = TrackingConfig( + provider_type="immich", + name="t", + scheduled_enabled=True, + scheduled_times="09:00", + ) + _validate_enabled_have_times( + {"scheduled_enabled": False, "scheduled_times": ""}, existing + )