From 5108694c2843b4da77a09ea749ca4761b5f6c93e Mon Sep 17 00:00:00 2001 From: tansdf Date: Fri, 18 Sep 2026 22:32:57 +0300 Subject: [PATCH 1/3] Switch maintenance saves to per-item upsert and delete. A full-list replace revalidated every window, so one owner-less legacy ICS event blocked saving anything else. Client persists now run one at a time so drag and modal save cannot share the WebSocket waiter. --- app/maintenance/api.py | 39 ++---- app/maintenance/store.py | 57 +++++++-- app/routes.py | 57 +++++---- static/js/maintenance.js | 181 ++++++++++++++------------- tests/test_maintenance/test_api.py | 34 +---- tests/test_maintenance/test_store.py | 155 +++++++++++++++++++++-- tests/test_routes_auth.py | 85 +++++++++---- 7 files changed, 393 insertions(+), 215 deletions(-) diff --git a/app/maintenance/api.py b/app/maintenance/api.py index 5e3492c5..549cb450 100644 --- a/app/maintenance/api.py +++ b/app/maintenance/api.py @@ -1,5 +1,4 @@ from datetime import datetime, timezone -from typing import Any from fastapi import HTTPException @@ -41,18 +40,13 @@ def validate_owner_id( raise HTTPException(status_code=400, detail="invalid owner_id") -def owner_id_from_payload(payload: dict) -> str: - owner_id = payload.get("owner_id") - if owner_id: - return str(owner_id) - raise HTTPException(status_code=400, detail="owner_id is required") - - def window_from_ws_item( - payload: dict, + payload, assignable_user_ids: set[str], existing_owner_id: str | None = None, ) -> dict: + if not isinstance(payload, dict): + raise HTTPException(status_code=400, detail="window must be an object") if "start" not in payload or "end" not in payload: raise HTTPException(status_code=400, detail="start and end are required") starts_at = parse_iso_to_utc(payload["start"]) @@ -69,7 +63,10 @@ def window_from_ws_item( if not window_id: raise HTTPException(status_code=400, detail="id is required") - owner_id = owner_id_from_payload(payload) + owner_id = payload.get("owner_id") + if not owner_id: + raise HTTPException(status_code=400, detail="owner_id is required") + owner_id = str(owner_id) validate_owner_id(owner_id, assignable_user_ids, existing_owner_id) return { @@ -80,25 +77,3 @@ def window_from_ws_item( "comment": comment, "owner_id": owner_id, } - - -def windows_from_ws_payload( - data: list, - assignable_user_ids: set[str], - existing_by_id: dict[str, dict], -) -> list[dict[str, Any]]: - windows = [] - for item in data: - if not isinstance(item, dict): - raise HTTPException(status_code=400, detail="each window must be an object") - windows.append(window_from_ws_item( - item, - assignable_user_ids, - existing_by_id.get(str(item.get("id")), {}).get("owner_id"), - )) - return windows - - -def removed_windows(existing: list[dict[str, Any]], saved: list[dict[str, Any]]) -> list[dict[str, Any]]: - saved_ids = {w["id"] for w in saved} - return [w for w in existing if w["id"] not in saved_ids] diff --git a/app/maintenance/store.py b/app/maintenance/store.py index 6803787e..21413980 100644 --- a/app/maintenance/store.py +++ b/app/maintenance/store.py @@ -8,7 +8,8 @@ from app.config.config import get_config from app.config.environment import get_environment_config from app.logging import logger -from app.maintenance.models import MaintenanceWindow +from app.maintenance.api import window_from_ws_item +from app.maintenance.models import MaintenanceWindow, _parse_iso from app.time import unix_sleep_to_timedelta @@ -41,10 +42,54 @@ def load_windows(self) -> list[dict[str, Any]]: with self._lock: return self._read_windows_from_disk() - def save_windows(self, windows: list[dict[str, Any]]) -> bool: + def upsert_window( + self, + payload, + assignable_user_ids: set[str], + ) -> tuple[bool, list[dict[str, Any]], list[dict[str, Any]]]: with self._lock: - retained = self._filter_retained_windows(windows) - return self._write_windows_unlocked(retained) + existing = self._read_windows_from_disk() + existing_owner_id = None + if isinstance(payload, dict) and payload.get("id"): + window_id = str(payload["id"]) + for existing_window in existing: + if existing_window["id"] == window_id: + existing_owner_id = existing_window.get("owner_id") + break + window = window_from_ws_item(payload, assignable_user_ids, existing_owner_id) + merged = [] + replaced = False + for existing_window in existing: + if existing_window["id"] == window["id"]: + merged.append(window) + replaced = True + else: + merged.append(existing_window) + if not replaced: + merged.append(window) + retained = self._filter_retained_windows(merged) + if not self._write_windows_unlocked(retained): + return False, existing, existing + return True, existing, retained + + def delete_window( + self, window_id: str + ) -> tuple[bool, list[dict[str, Any]], list[dict[str, Any]], dict[str, Any] | None]: + with self._lock: + existing = self._read_windows_from_disk() + deleted = None + remaining = [] + for existing_window in existing: + if existing_window["id"] == str(window_id): + deleted = existing_window + else: + remaining.append(existing_window) + if deleted is None: + return True, existing, existing, None + retained = self._filter_retained_windows(remaining) + if not self._write_windows_unlocked(retained): + return False, existing, existing, None + return True, existing, retained, deleted def windows_list(self) -> list[MaintenanceWindow]: windows = self.load_windows() @@ -193,9 +238,7 @@ def _parse_datetime(self, dt_str: str | None) -> datetime | None: if not dt_str: return None try: - if dt_str.endswith("Z"): - dt_str = dt_str[:-1] + "+00:00" - return datetime.fromisoformat(dt_str) + return _parse_iso(dt_str) except ValueError: return None diff --git a/app/routes.py b/app/routes.py index 28fbc5a3..a6c5c3d2 100644 --- a/app/routes.py +++ b/app/routes.py @@ -18,7 +18,6 @@ from app.config.config import get_config, reload_config from app.im.chain.ui_chains_store import ui_chains_store from app.logging import logger -from app.maintenance.api import removed_windows, windows_from_ws_payload from app.maintenance.store import get_maintenance_store from app.metrics import generate_metrics_response from app.middleware import ( @@ -34,8 +33,11 @@ _MSG_AUTHENTICATION_REQUIRED = "Authentication required" -async def _maintenance_save_side_effects(app, existing, saved, deleted): - await app.state.maintenance_manager.apply_save_side_effects(existing, saved, deleted) +async def _send_maintenance_saved(websocket, success, detail=None): + message = {"event": "maintenance_saved", "success": success} + if detail is not None: + message["detail"] = detail + await websocket.send_text(json.dumps(message)) def create_router(http_prefix: str, fastapi_app: FastAPI | None = None, auth_manager=None) -> APIRouter: @@ -426,43 +428,44 @@ async def websocket_endpoint(websocket: WebSocket): await websocket.send_text(json.dumps({"event": "maintenance_data", "data": windows})) elif event_type == "save_maintenance": if auth_manager and _get_acting_user_from_websocket(websocket) is None: - await websocket.send_text(json.dumps({ - "event": "maintenance_saved", - "success": False, - "detail": _MSG_AUTHENTICATION_REQUIRED, - })) + await _send_maintenance_saved(websocket, False, _MSG_AUTHENTICATION_REQUIRED) else: - windows_payload = message.get("data", []) + payload = message.get("data") store = get_maintenance_store() - existing = store.load_windows() - existing_by_id = {w["id"]: w for w in existing} assignable_user_ids = { str(user["user_id"]) for user in _get_assignable_users(websocket.app.state.messenger) } try: - windows = windows_from_ws_payload( - windows_payload, + success, existing_before, saved = store.upsert_window( + payload, assignable_user_ids, - existing_by_id, ) except HTTPException as exc: - await websocket.send_text(json.dumps({ - "event": "maintenance_saved", - "success": False, - "detail": exc.detail, - })) + await _send_maintenance_saved(websocket, False, exc.detail) else: - deleted = removed_windows(existing, windows) - success = store.save_windows(windows) - await websocket.send_text(json.dumps({ - "event": "maintenance_saved", - "success": success, - })) + await _send_maintenance_saved(websocket, success) if success: _maintenance_save_task = asyncio.create_task( - _maintenance_save_side_effects( - websocket.app, existing, windows, deleted + websocket.app.state.maintenance_manager.apply_save_side_effects( + existing_before, saved, [] + ) + ) + elif event_type == "delete_maintenance": + if auth_manager and _get_acting_user_from_websocket(websocket) is None: + await _send_maintenance_saved(websocket, False, _MSG_AUTHENTICATION_REQUIRED) + else: + window_id = message.get("id") + if not window_id: + await _send_maintenance_saved(websocket, False, "id is required") + else: + store = get_maintenance_store() + success, existing_before, saved, deleted = store.delete_window(str(window_id)) + await _send_maintenance_saved(websocket, success) + if success and deleted: + _maintenance_save_task = asyncio.create_task( + websocket.app.state.maintenance_manager.apply_save_side_effects( + existing_before, saved, [deleted] ) ) diff --git a/static/js/maintenance.js b/static/js/maintenance.js index c48b16e3..468cbd10 100644 --- a/static/js/maintenance.js +++ b/static/js/maintenance.js @@ -31,8 +31,6 @@ let cachedWindows = []; let windowsPromiseResolve = null; let savePromiseResolve = null; let currentWindowId = null; -let pendingSelectStart = null; -let pendingSelectEnd = null; let modalMatchers = []; let ownerSelector = null; let configTimezone = "UTC"; @@ -74,10 +72,6 @@ function isMaintenanceWindowActive(startIso, endIso, now = new Date()) { return start.getTime() <= now.getTime() && now.getTime() < end.getTime(); } -function countActiveMaintenanceWindows(windows, now = new Date()) { - return windows.filter((w) => isMaintenanceWindowActive(w.start, w.end, now)).length; -} - function formatTimeLeft(endIso, now = new Date()) { const ms = new Date(endIso).getTime() - now.getTime(); if (ms <= 0) return "ended"; @@ -343,32 +337,13 @@ async function loadWindows() { }); } -async function saveWindows(windows) { - cachedWindows = windows; - const socket = getSocket(); - if (socket?.readyState !== WebSocket.OPEN) { - console.error("WebSocket not connected, cannot save maintenance windows"); - return false; - } - return new Promise((resolve) => { - savePromiseResolve = resolve; - socket.send(JSON.stringify({event: "save_maintenance", data: windows})); - setTimeout(() => { - if (savePromiseResolve === resolve) { - savePromiseResolve = null; - resolve(false); - } - }, 5000); - }); -} - let windowModalPersistInFlight = false; +let maintenancePersistQueue = Promise.resolve(); -async function getWindowsForEdit() { - if (cachedWindows.length > 0) { - return [...cachedWindows]; - } - return [...await loadWindows()]; +function runMaintenanceMutation(work) { + const run = maintenancePersistQueue.then(work); + maintenancePersistQueue = run.then(() => undefined, () => undefined); + return run; } function setWindowModalPersistInFlight(inFlight) { @@ -377,9 +352,25 @@ function setWindowModalPersistInFlight(inFlight) { document.getElementById("maintenance-window-delete-btn")?.toggleAttribute("disabled", inFlight); } -async function persistMaintenanceWindows(windows, previousWindows) { - refreshCalendarEvents(windows); - const saved = await saveWindows(windows); +async function persistMaintenanceMutation(message, nextWindows, previousWindows) { + cachedWindows = nextWindows; + refreshCalendarEvents(nextWindows); + const socket = getSocket(); + let saved = false; + if (socket?.readyState !== WebSocket.OPEN) { + console.error("WebSocket not connected, cannot save maintenance windows"); + } else { + saved = await new Promise((resolve) => { + savePromiseResolve = resolve; + socket.send(JSON.stringify(message)); + setTimeout(() => { + if (savePromiseResolve === resolve) { + savePromiseResolve = null; + resolve(false); + } + }, 5000); + }); + } if (!saved) { cachedWindows = previousWindows; refreshCalendarEvents(previousWindows); @@ -479,8 +470,6 @@ function buildMainCalendarOptions(events, firstDay, timezone) { events, select(info) { - pendingSelectStart = info.start; - pendingSelectEnd = info.end; openWindowModal(); document.getElementById("maintenance-window-start").value = formatDateTime(info.start, getTz()); if (info.end) { @@ -537,22 +526,33 @@ function buildMonthCalendarOptions(events, firstDay, timezone) { } async function handleEventTimeChange(info) { - const previousWindows = cachedWindows; - const windows = await getWindowsForEdit(); - const index = windows.findIndex((w) => w.id === info.event.id); - if (index === -1) { - info.revert(); - return; - } - windows[index] = { - ...windows[index], - start: info.event.start.toISOString(), - end: info.event.end?.toISOString() ?? windows[index].end, - }; - const saved = await persistMaintenanceWindows(windows, previousWindows); - if (!saved) { - info.revert(); - } + await runMaintenanceMutation(async () => { + const previousWindows = cachedWindows; + const windows = [...cachedWindows]; + const index = windows.findIndex((w) => w.id === info.event.id); + if (index === -1) { + info.revert(); + return; + } + if (!windows[index].owner_id) { + info.revert(); + showNotification("Owner is required"); + return; + } + windows[index] = { + ...windows[index], + start: info.event.start.toISOString(), + end: info.event.end?.toISOString() ?? windows[index].end, + }; + const saved = await persistMaintenanceMutation( + {event: "save_maintenance", data: windows[index]}, + windows, + previousWindows, + ); + if (!saved) { + info.revert(); + } + }); } function setMatcherInputError(reason) { @@ -690,10 +690,8 @@ function openWindowModal(windowData = null) { } else { currentWindowId = null; title.textContent = "New maintenance"; - if (!pendingSelectStart) { - startInput.value = ""; - endInput.value = ""; - } + startInput.value = ""; + endInput.value = ""; commentInput.value = ""; modalMatchers = []; deleteBtn.classList.add("hidden"); @@ -707,8 +705,6 @@ function openWindowModal(windowData = null) { function closeWindowModal() { document.getElementById("maintenance-window-modal")?.classList.remove("visible"); currentWindowId = null; - pendingSelectStart = null; - pendingSelectEnd = null; modalMatchers = []; } @@ -769,47 +765,54 @@ async function saveWindowModal() { if (windowModalPersistInFlight) return; const input = validateWindowModalInput(); if (!input) return; + const windowId = currentWindowId; - const previousWindows = cachedWindows; - const windows = await getWindowsForEdit(); - if (currentWindowId) { - const index = windows.findIndex((w) => w.id === currentWindowId); - if (index === -1) return; - windows[index] = { - ...windows[index], - start: input.start, - end: input.end, - matchers: input.matchers, - comment: input.comment, - owner_id: input.owner_id, - }; - } else { - windows.push({ - id: crypto.randomUUID(), - start: input.start, - end: input.end, - matchers: input.matchers, - comment: input.comment, - owner_id: input.owner_id, + setWindowModalPersistInFlight(true); + try { + await runMaintenanceMutation(async () => { + const previousWindows = cachedWindows; + const windows = [...cachedWindows]; + let savedWindow; + if (windowId) { + const index = windows.findIndex((w) => w.id === windowId); + if (index === -1) return; + savedWindow = {...windows[index], ...input}; + windows[index] = savedWindow; + } else { + savedWindow = {id: crypto.randomUUID(), ...input}; + windows.push(savedWindow); + } + closeWindowModal(); + await persistMaintenanceMutation( + {event: "save_maintenance", data: savedWindow}, + windows, + previousWindows, + ); }); + } finally { + setWindowModalPersistInFlight(false); } - - setWindowModalPersistInFlight(true); - closeWindowModal(); - await persistMaintenanceWindows(windows, previousWindows); - setWindowModalPersistInFlight(false); } async function deleteWindowModal() { if (!currentWindowId || windowModalPersistInFlight) return; - - const previousWindows = cachedWindows; - const windows = (await getWindowsForEdit()).filter((w) => w.id !== currentWindowId); + const windowId = currentWindowId; setWindowModalPersistInFlight(true); - closeWindowModal(); - await persistMaintenanceWindows(windows, previousWindows); - setWindowModalPersistInFlight(false); + try { + await runMaintenanceMutation(async () => { + const previousWindows = cachedWindows; + const windows = cachedWindows.filter((w) => w.id !== windowId); + closeWindowModal(); + await persistMaintenanceMutation( + {event: "delete_maintenance", id: windowId}, + windows, + previousWindows, + ); + }); + } finally { + setWindowModalPersistInFlight(false); + } } function refreshMaintenanceModalDateTimes({previousTimezone, configTimezone: tz, userTimezone: userTz}) { diff --git a/tests/test_maintenance/test_api.py b/tests/test_maintenance/test_api.py index 905eff66..31d9f1ac 100644 --- a/tests/test_maintenance/test_api.py +++ b/tests/test_maintenance/test_api.py @@ -2,10 +2,8 @@ from fastapi import HTTPException from app.maintenance.api import ( - owner_id_from_payload, validate_owner_id, window_from_ws_item, - windows_from_ws_payload, ) @@ -25,16 +23,6 @@ def _window_payload(**overrides): ASSIGNABLE = {"U123", "U555"} -def test_owner_id_from_payload_uses_explicit_value(): - assert owner_id_from_payload({"owner_id": "U999"}) == "U999" - - -def test_owner_id_from_payload_required(): - with pytest.raises(HTTPException) as exc: - owner_id_from_payload({}) - assert exc.value.detail == "owner_id is required" - - def test_validate_owner_id_accepts_assignable_user(): validate_owner_id("U123", ASSIGNABLE) @@ -77,21 +65,7 @@ def test_window_from_ws_item_allows_existing_owner_not_assignable(): assert window["owner_id"] == "U999" -def test_windows_from_ws_payload_validates_list(): - windows = windows_from_ws_payload( - [_window_payload()], - assignable_user_ids=ASSIGNABLE, - existing_by_id={}, - ) - assert len(windows) == 1 - assert windows[0]["owner_id"] == "U123" - - -def test_windows_from_ws_payload_uses_existing_by_id(): - existing_by_id = {"w1": {"id": "w1", "owner_id": "U999"}} - windows = windows_from_ws_payload( - [_window_payload(owner_id="U999")], - assignable_user_ids=ASSIGNABLE, - existing_by_id=existing_by_id, - ) - assert windows[0]["owner_id"] == "U999" +def test_window_from_ws_item_rejects_list(): + with pytest.raises(HTTPException) as exc: + window_from_ws_item([_window_payload()], assignable_user_ids=ASSIGNABLE) + assert exc.value.detail == "window must be an object" diff --git a/tests/test_maintenance/test_store.py b/tests/test_maintenance/test_store.py index 5de4cb1a..a89aed4a 100644 --- a/tests/test_maintenance/test_store.py +++ b/tests/test_maintenance/test_store.py @@ -2,10 +2,15 @@ from pathlib import Path from unittest.mock import patch +import pytest +from fastapi import HTTPException + from app.config.validation import IncidentTimeouts from app.maintenance.models import MaintenanceWindow from app.maintenance.store import MaintenanceStore +ASSIGNABLE = {"U123"} + def _make_store(tmp_path: Path) -> MaintenanceStore: with patch("app.maintenance.store.get_environment_config") as mock_env: @@ -20,6 +25,11 @@ def _mock_closed_retention(closed: str = "7d"): return mock_config +def _seed_windows(store: MaintenanceStore, windows: list[dict]) -> None: + store._ensure_dir() + assert store._write_windows_unlocked(windows) is True + + def _sample_window(window_id: str = "w1") -> dict: now = datetime.now(timezone.utc) start = now + timedelta(hours=1) @@ -47,7 +57,8 @@ def test_save_and_load_windows_round_trip(tmp_path: Path): windows[1]["matchers"] = ['service="elastic"', 'env="prod"'] windows[1]["id"] = "w2" - assert store.save_windows(windows) is True + assert store.upsert_window(windows[0], ASSIGNABLE)[0] is True + assert store.upsert_window(windows[1], ASSIGNABLE)[0] is True loaded = store.load_windows() assert len(loaded) == 2 by_id = {w["id"]: w for w in loaded} @@ -62,7 +73,7 @@ def test_list_returns_maintenance_window_objects(tmp_path: Path): store = _make_store(tmp_path) mock_config = _mock_closed_retention("7d") try: - store.save_windows([_sample_window()]) + assert store.upsert_window(_sample_window(), ASSIGNABLE)[0] is True windows = store.windows_list() assert len(windows) == 1 assert isinstance(windows[0], MaintenanceWindow) @@ -99,7 +110,7 @@ def test_prune_expired_windows(tmp_path: Path): mock_config.stop() -def test_save_retains_recently_ended_windows_until_closed_timeout(tmp_path: Path): +def test_upsert_retains_recently_ended_windows_until_closed_timeout(tmp_path: Path): store = _make_store(tmp_path) mock_config = _mock_closed_retention("7d") now = datetime.now(timezone.utc) @@ -110,6 +121,7 @@ def test_save_retains_recently_ended_windows_until_closed_timeout(tmp_path: Path "end": (now - timedelta(days=1)).isoformat(), "matchers": ['alertname="A"'], "comment": "recent maintenance", + "owner_id": "U123", } old = { "id": "old-ended", @@ -118,11 +130,10 @@ def test_save_retains_recently_ended_windows_until_closed_timeout(tmp_path: Path "matchers": ['alertname="B"'], "comment": "old maintenance", } - - assert store.save_windows([recent, old]) is True - - loaded = store.load_windows() - assert [window["id"] for window in loaded] == ["recent-ended"] + _seed_windows(store, [recent, old]) + ok, _existing, saved = store.upsert_window(recent, ASSIGNABLE) + assert ok is True + assert [window["id"] for window in saved] == ["recent-ended"] finally: mock_config.stop() @@ -139,3 +150,131 @@ def test_skips_event_without_matchers(tmp_path: Path): b"SUMMARY:test\r\nEND:VEVENT\r\nEND:VCALENDAR\r\n" ) assert store.load_windows() == [] + + +def test_load_window_without_owner_id(tmp_path: Path): + store = _make_store(tmp_path) + store._ensure_dir() + with open(store._file, "wb") as f: + f.write( + b"BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//test//EN\r\n" + b"BEGIN:VEVENT\r\nUID:legacy\r\n" + b"DTSTART:20260620T080000Z\r\nDTEND:20260620T120000Z\r\n" + b"SUMMARY:test\r\n" + b"X-MATCHER:alertname=\"A\"\r\n" + b"END:VEVENT\r\nEND:VCALENDAR\r\n" + ) + loaded = store.load_windows() + assert len(loaded) == 1 + assert loaded[0]["id"] == "legacy" + assert loaded[0]["owner_id"] is None + + +def test_upsert_window_keeps_ownerless_sibling(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + legacy = _sample_window("legacy") + legacy["owner_id"] = None + _seed_windows(store, [legacy]) + ok, _existing, _saved = store.upsert_window(_sample_window("new"), ASSIGNABLE) + assert ok is True + by_id = {w["id"]: w for w in store.load_windows()} + assert by_id["legacy"]["owner_id"] is None + assert by_id["new"]["owner_id"] == "U123" + ics = Path(store._file).read_bytes() + assert ics.count(b"X-OWNER-ID") == 1 + assert b"X-OWNER-ID:U123" in ics + finally: + mock_config.stop() + + +def test_delete_window_leaves_remaining(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + _seed_windows(store, [_sample_window("w1"), _sample_window("w2")]) + ok, _existing, _saved, deleted = store.delete_window("w1") + assert ok is True + assert deleted["id"] == "w1" + assert [w["id"] for w in store.load_windows()] == ["w2"] + finally: + mock_config.stop() + + +def test_delete_missing_window_is_success(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + _seed_windows(store, [_sample_window("w1")]) + ok, _existing, _saved, deleted = store.delete_window("missing") + assert ok is True + assert deleted is None + assert [w["id"] for w in store.load_windows()] == ["w1"] + finally: + mock_config.stop() + + +def test_upsert_drops_expired_siblings(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + now = datetime.now(timezone.utc) + try: + recent = { + "id": "recent-ended", + "start": (now - timedelta(days=1, hours=2)).isoformat(), + "end": (now - timedelta(days=1)).isoformat(), + "matchers": ['alertname="A"'], + "comment": "recent maintenance", + } + old = { + "id": "old-ended", + "start": (now - timedelta(days=8, hours=2)).isoformat(), + "end": (now - timedelta(days=8)).isoformat(), + "matchers": ['alertname="B"'], + "comment": "old maintenance", + } + store._write_windows_unlocked([recent, old]) + ok, _existing, saved = store.upsert_window(_sample_window("new"), ASSIGNABLE) + assert ok is True + assert {w["id"] for w in saved} == {"recent-ended", "new"} + finally: + mock_config.stop() + + +def test_upsert_rejects_invalid_payload_without_writing(tmp_path: Path): + store = _make_store(tmp_path) + legacy = _sample_window("legacy") + legacy["owner_id"] = None + _seed_windows(store, [legacy]) + before = Path(store._file).read_bytes() + with pytest.raises(HTTPException) as exc: + store.upsert_window(legacy, ASSIGNABLE) + assert exc.value.detail == "owner_id is required" + assert Path(store._file).read_bytes() == before + loaded = store.load_windows() + assert loaded[0]["id"] == "legacy" + assert loaded[0]["owner_id"] is None + + +def test_upsert_rejects_list_without_writing(tmp_path: Path): + store = _make_store(tmp_path) + with pytest.raises(HTTPException) as exc: + store.upsert_window([_sample_window()], ASSIGNABLE) + assert exc.value.detail == "window must be an object" + assert store.load_windows() == [] + assert not Path(store._file).exists() + + +def test_upsert_grandfathers_stored_owner(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + existing = _sample_window("w1") + existing["owner_id"] = "U999" + _seed_windows(store, [existing]) + ok, _before, saved = store.upsert_window(existing, ASSIGNABLE) + assert ok is True + assert saved[0]["owner_id"] == "U999" + finally: + mock_config.stop() diff --git a/tests/test_routes_auth.py b/tests/test_routes_auth.py index fc10948d..8aa6040d 100644 --- a/tests/test_routes_auth.py +++ b/tests/test_routes_auth.py @@ -3,7 +3,7 @@ import pytest import time from datetime import datetime, timedelta, timezone -from fastapi import FastAPI +from fastapi import FastAPI, HTTPException from fastapi.testclient import TestClient from app.routes import create_router @@ -49,16 +49,17 @@ def _mock_auth_manager(*, authenticated: bool): MATCHER = 'alertname = "Test"' -def _maintenance_ws_payload(): +def _maintenance_ws_window(): start = datetime.now(timezone.utc).replace(microsecond=0) end = start + timedelta(hours=1) - return [{ + return { "id": "w1", "start": start.isoformat(), "end": end.isoformat(), "matchers": [MATCHER], "comment": "planned work", - }] + "owner_id": "U1", + } @pytest.fixture @@ -131,10 +132,10 @@ def test_save_maintenance_rejected_when_unauthenticated(self, config, messenger) ws.receive_json() ws.send_json({ "event": "save_maintenance", - "data": _maintenance_ws_payload(), + "data": _maintenance_ws_window(), }) message = ws.receive_json() - mock_store.return_value.save_windows.assert_not_called() + mock_store.return_value.upsert_window.assert_not_called() assert message == { "event": "maintenance_saved", "success": False, @@ -160,44 +161,84 @@ def test_request_maintenance_rejected_when_unauthenticated(self, config, messeng def test_save_maintenance_allowed_when_authenticated(self, config, messenger): auth_manager = _mock_auth_manager(authenticated=True) app = _build_app(config, messenger, auth_manager) + window = _maintenance_ws_window() with patch("app.routes.get_config", return_value=config), \ - patch("app.routes.get_maintenance_store") as mock_store, \ - patch("app.routes.windows_from_ws_payload") as mock_validate: - mock_store.return_value.load_windows.return_value = [] - mock_store.return_value.save_windows.return_value = True - mock_validate.return_value = _maintenance_ws_payload() + patch("app.routes.get_maintenance_store") as mock_store: + mock_store.return_value.upsert_window.return_value = (True, [], [window]) with TestClient(app, cookies={SESSION_COOKIE: "valid-session"}) as client: with client.websocket_connect("/ws") as ws: ws.receive_json() ws.send_json({ "event": "save_maintenance", - "data": _maintenance_ws_payload(), + "data": window, }) message = ws.receive_json() _wait_for_background_tasks() - mock_store.return_value.save_windows.assert_called_once() + mock_store.return_value.upsert_window.assert_called_once() app.state.maintenance_manager.apply_save_side_effects.assert_awaited_once() assert message == {"event": "maintenance_saved", "success": True} - def test_save_maintenance_reconciles_removed_windows(self, config, messenger): + def test_save_maintenance_rejects_list_payload(self, config, messenger): auth_manager = _mock_auth_manager(authenticated=True) app = _build_app(config, messenger, auth_manager) - existing = _maintenance_ws_payload() with patch("app.routes.get_config", return_value=config), \ - patch("app.routes.get_maintenance_store") as mock_store, \ - patch("app.routes.windows_from_ws_payload") as mock_validate: - mock_store.return_value.load_windows.return_value = existing - mock_store.return_value.save_windows.return_value = True - mock_validate.return_value = [] + patch("app.routes.get_maintenance_store") as mock_store: + mock_store.return_value.upsert_window.side_effect = HTTPException( + status_code=400, + detail="window must be an object", + ) with TestClient(app, cookies={SESSION_COOKIE: "valid-session"}) as client: with client.websocket_connect("/ws") as ws: ws.receive_json() ws.send_json({ "event": "save_maintenance", - "data": [], + "data": [_maintenance_ws_window()], + }) + message = ws.receive_json() + mock_store.return_value.upsert_window.assert_called_once() + assert message == { + "event": "maintenance_saved", + "success": False, + "detail": "window must be an object", + } + + def test_delete_maintenance_rejected_when_unauthenticated(self, config, messenger): + auth_manager = _mock_auth_manager(authenticated=False) + app = _build_app(config, messenger, auth_manager) + with patch("app.routes.get_config", return_value=config), \ + patch("app.routes.get_maintenance_store") as mock_store: + with TestClient(app) as client: + with client.websocket_connect("/ws") as ws: + ws.receive_json() + ws.send_json({ + "event": "delete_maintenance", + "id": "w1", + }) + message = ws.receive_json() + mock_store.return_value.delete_window.assert_not_called() + assert message == { + "event": "maintenance_saved", + "success": False, + "detail": "Authentication required", + } + + def test_delete_maintenance_reconciles_removed_window(self, config, messenger): + auth_manager = _mock_auth_manager(authenticated=True) + app = _build_app(config, messenger, auth_manager) + existing = [_maintenance_ws_window()] + with patch("app.routes.get_config", return_value=config), \ + patch("app.routes.get_maintenance_store") as mock_store: + mock_store.return_value.delete_window.return_value = (True, existing, [], existing[0]) + with TestClient(app, cookies={SESSION_COOKIE: "valid-session"}) as client: + with client.websocket_connect("/ws") as ws: + ws.receive_json() + ws.send_json({ + "event": "delete_maintenance", + "id": "w1", }) message = ws.receive_json() _wait_for_background_tasks() + mock_store.return_value.delete_window.assert_called_once_with("w1") side_effects = app.state.maintenance_manager.apply_save_side_effects side_effects.assert_awaited_once() assert side_effects.await_args.args[2] == existing @@ -206,7 +247,7 @@ def test_save_maintenance_reconciles_removed_windows(self, config, messenger): def test_request_maintenance_allowed_when_authenticated(self, config, messenger): auth_manager = _mock_auth_manager(authenticated=True) app = _build_app(config, messenger, auth_manager) - payload = _maintenance_ws_payload() + payload = [_maintenance_ws_window()] with patch("app.routes.get_config", return_value=config), \ patch("app.routes.get_maintenance_store") as mock_store: mock_store.return_value.load_windows.return_value = payload From a610e674f0aea6cead6993e121461ad3b1161dcb Mon Sep 17 00:00:00 2001 From: tansdf Date: Sat, 19 Sep 2026 12:08:45 +0300 Subject: [PATCH 2/3] Switch UI chain shifts to per-item upsert and delete. --- app/im/chain/ui_chains_store.py | 77 +++- app/routes.py | 52 ++- static/js/chains.js | 443 +++++++++-------------- static/js/websocket.js | 11 +- tests/test_chain/test_ui_chains_store.py | 160 +++++++- tests/test_routes_auth.py | 77 +++- 6 files changed, 496 insertions(+), 324 deletions(-) diff --git a/app/im/chain/ui_chains_store.py b/app/im/chain/ui_chains_store.py index 6ffa1912..8f61d09c 100644 --- a/app/im/chain/ui_chains_store.py +++ b/app/im/chain/ui_chains_store.py @@ -1,5 +1,6 @@ import json import os +import threading from datetime import datetime, timedelta, timezone from typing import Any @@ -93,6 +94,7 @@ class UIChainsStore: def __init__(self): env_config = get_environment_config() self.ui_chains_dir = os.path.join(env_config.data_path, "ui_chains") + self._lock = threading.Lock() self._ensure_directory_exists() def _ensure_directory_exists(self) -> None: @@ -106,25 +108,27 @@ def _calendar_path(self, chain_name: str) -> str: def load_shifts(self, chain_name: str) -> list[dict[str, Any]]: if not chain_name: return [] - shifts = self._read_shifts_from_disk(chain_name) - logger.debug("Loaded ui chains", extra={"chain": chain_name, "count": len(shifts)}) - return self.recalculate_priorities(shifts) + with self._lock: + shifts = self._read_shifts_from_disk(chain_name) + logger.debug("Loaded ui chains", extra={"chain": chain_name, "count": len(shifts)}) + return self.recalculate_priorities(shifts) def prune_expired_shifts(self, chain_name: str, now: datetime | None = None) -> int: if not chain_name: return 0 - shifts = self._read_shifts_from_disk(chain_name) - if not shifts: - return 0 - retained, expired = self._partition_by_retention(shifts, now) - if not expired: - return 0 - self._write_shifts(chain_name, retained) - logger.info( - "Pruned expired ui chain shifts", - extra={"chain": chain_name, "removed": len(expired)}, - ) - return len(expired) + with self._lock: + shifts = self._read_shifts_from_disk(chain_name) + if not shifts: + return 0 + retained, expired = self._partition_by_retention(shifts, now) + if not expired: + return 0 + self._write_shifts(chain_name, retained) + logger.info( + "Pruned expired ui chain shifts", + extra={"chain": chain_name, "removed": len(expired)}, + ) + return len(expired) def prune_all(self, now: datetime | None = None) -> int: if not os.path.exists(self.ui_chains_dir): @@ -262,12 +266,45 @@ def get_steps_for_now(self, chain_name: str, now: datetime | None = None) -> lis steps = active[0].get("steps") return steps if isinstance(steps, list) else [] - def save_shifts(self, chain_name: str, shifts: list[dict[str, Any]]) -> bool: + def upsert_shift(self, chain_name: str, payload) -> tuple[bool, list[dict[str, Any]]]: if not chain_name: - return False - shifts = self.filter_retained_shifts(shifts) - shifts = self.recalculate_priorities(shifts) - return self._write_shifts(chain_name, shifts) + return False, [] + with self._lock: + existing = self._read_shifts_from_disk(chain_name) + if not isinstance(payload, dict) or not payload.get("id"): + return False, existing + shift = {**payload, "id": str(payload["id"])} + merged = [] + replaced = False + for existing_shift in existing: + if existing_shift.get("id") == shift["id"]: + if replaced: + continue + merged.append(shift) + replaced = True + else: + merged.append(existing_shift) + if not replaced: + merged.append(shift) + return self._commit_shifts(chain_name, existing, merged) + + def delete_shift(self, chain_name: str, shift_id: str) -> tuple[bool, list[dict[str, Any]]]: + if not chain_name: + return False, [] + with self._lock: + existing = self._read_shifts_from_disk(chain_name) + remaining = [shift for shift in existing if shift.get("id") != str(shift_id)] + if len(remaining) == len(existing): + return True, self.recalculate_priorities(existing) + return self._commit_shifts(chain_name, existing, remaining) + + def _commit_shifts( + self, chain_name: str, existing: list[dict[str, Any]], shifts: list[dict[str, Any]] + ) -> tuple[bool, list[dict[str, Any]]]: + recalculated = self.recalculate_priorities(self.filter_retained_shifts(shifts)) + if not self._write_shifts(chain_name, recalculated): + return False, existing + return True, recalculated def _chain_to_ical_event(self, chain: dict[str, Any]) -> Event | None: try: diff --git a/app/routes.py b/app/routes.py index a6c5c3d2..f56fb0b0 100644 --- a/app/routes.py +++ b/app/routes.py @@ -33,10 +33,12 @@ _MSG_AUTHENTICATION_REQUIRED = "Authentication required" -async def _send_maintenance_saved(websocket, success, detail=None): - message = {"event": "maintenance_saved", "success": success} +async def _send_saved_event(websocket, event, success, detail=None, data=None): + message = {"event": event, "success": success} if detail is not None: message["detail"] = detail + if data is not None: + message["data"] = data await websocket.send_text(json.dumps(message)) @@ -405,16 +407,34 @@ async def websocket_endpoint(websocket: WebSocket): await websocket.send_text(json.dumps({"event": "ui_chains_data", "data": shifts})) elif event_type == "save_ui_chains": if auth_manager and _get_acting_user_from_websocket(websocket) is None: - await websocket.send_text(json.dumps({ - "event": "ui_chains_saved", - "success": False, - "detail": _MSG_AUTHENTICATION_REQUIRED, - })) + await _send_saved_event(websocket, "ui_chains_saved", False, _MSG_AUTHENTICATION_REQUIRED) else: chain_name = message.get("chain_name", "") - shifts = message.get("data", []) - success = ui_chains_store.save_shifts(chain_name, shifts) - await websocket.send_text(json.dumps({"event": "ui_chains_saved", "success": success})) + payload = message.get("data") + if not isinstance(payload, dict): + await _send_saved_event(websocket, "ui_chains_saved", False, "shift must be an object") + elif not payload.get("id"): + await _send_saved_event(websocket, "ui_chains_saved", False, "id is required") + else: + success, saved = ui_chains_store.upsert_shift(chain_name, payload) + if success: + await _send_saved_event(websocket, "ui_chains_saved", True, data=saved) + else: + await _send_saved_event(websocket, "ui_chains_saved", False) + elif event_type == "delete_ui_chain": + if auth_manager and _get_acting_user_from_websocket(websocket) is None: + await _send_saved_event(websocket, "ui_chains_saved", False, _MSG_AUTHENTICATION_REQUIRED) + else: + chain_name = message.get("chain_name", "") + shift_id = message.get("id") + if not shift_id: + await _send_saved_event(websocket, "ui_chains_saved", False, "id is required") + else: + success, saved = ui_chains_store.delete_shift(chain_name, str(shift_id)) + if success: + await _send_saved_event(websocket, "ui_chains_saved", True, data=saved) + else: + await _send_saved_event(websocket, "ui_chains_saved", False) elif event_type == "request_maintenance": if auth_manager and _get_acting_user_from_websocket(websocket) is None: await websocket.send_text(json.dumps({ @@ -428,7 +448,7 @@ async def websocket_endpoint(websocket: WebSocket): await websocket.send_text(json.dumps({"event": "maintenance_data", "data": windows})) elif event_type == "save_maintenance": if auth_manager and _get_acting_user_from_websocket(websocket) is None: - await _send_maintenance_saved(websocket, False, _MSG_AUTHENTICATION_REQUIRED) + await _send_saved_event(websocket, "maintenance_saved", False, _MSG_AUTHENTICATION_REQUIRED) else: payload = message.get("data") store = get_maintenance_store() @@ -442,9 +462,9 @@ async def websocket_endpoint(websocket: WebSocket): assignable_user_ids, ) except HTTPException as exc: - await _send_maintenance_saved(websocket, False, exc.detail) + await _send_saved_event(websocket, "maintenance_saved", False, exc.detail) else: - await _send_maintenance_saved(websocket, success) + await _send_saved_event(websocket, "maintenance_saved", success) if success: _maintenance_save_task = asyncio.create_task( websocket.app.state.maintenance_manager.apply_save_side_effects( @@ -453,15 +473,15 @@ async def websocket_endpoint(websocket: WebSocket): ) elif event_type == "delete_maintenance": if auth_manager and _get_acting_user_from_websocket(websocket) is None: - await _send_maintenance_saved(websocket, False, _MSG_AUTHENTICATION_REQUIRED) + await _send_saved_event(websocket, "maintenance_saved", False, _MSG_AUTHENTICATION_REQUIRED) else: window_id = message.get("id") if not window_id: - await _send_maintenance_saved(websocket, False, "id is required") + await _send_saved_event(websocket, "maintenance_saved", False, "id is required") else: store = get_maintenance_store() success, existing_before, saved, deleted = store.delete_window(str(window_id)) - await _send_maintenance_saved(websocket, success) + await _send_saved_event(websocket, "maintenance_saved", success) if success and deleted: _maintenance_save_task = asyncio.create_task( websocket.app.state.maintenance_manager.apply_save_side_effects( diff --git a/static/js/chains.js b/static/js/chains.js index 51992fd5..d5662d27 100644 --- a/static/js/chains.js +++ b/static/js/chains.js @@ -33,6 +33,7 @@ let initialized = false; let cachedChains = []; let chainsPromiseResolve = null; let savePromiseResolve = null; +let timedOutChainName = null; function getRepeatIntervalDays(repeat) { switch (repeat) { @@ -237,10 +238,6 @@ function prepareEventsForCalendar(chains) { }); } -function getExpandedChains(chains) { - return prepareEventsForCalendar(chains); -} - function applyHarnessOverlapOffset(harness) { if (!harness) return; @@ -322,99 +319,43 @@ function recalculatePriorities(chains) { }); } -function recalculatePrioritiesForChainIds(chains, chainIds) { - const impactedIds = new Set(chainIds.filter(Boolean)); - - if (impactedIds.size === 0) { - return chains; - } - - return chains.map(chain => { - if (!impactedIds.has(chain.id)) { - return chain; - } - - const overlapping = findOverlappingChainsForChain(chains, chain, chain.id); - return { - ...chain, - priority: calculateNewPriority(chain, overlapping) - }; - }); -} - -function applyPriorityToEvent(event, chain) { - event.setExtendedProp('priority', chain.priority ?? 2); - if (event.el) { - styleMountedEvent(event.el, event); - } -} - -function syncCalendarEventPriorities(chains, chainIds, preferredEvent = null) { - if (!calendar) { - return; - } - - const impactedIds = new Set(chainIds.filter(Boolean)); - if (preferredEvent) { - const preferredId = preferredEvent.extendedProps?.originalId || preferredEvent.id; - impactedIds.add(preferredId); - } - - const chainById = new Map(chains.map(chain => [chain.id, chain])); - const allEvents = calendar.getEvents(); - - for (const event of allEvents) { - if (event.extendedProps?.isOccurrence) { - continue; - } - - const originalId = event.extendedProps?.originalId || event.id; - if (!impactedIds.has(originalId)) { - continue; - } - - const chain = chainById.get(originalId); - if (!chain) { - continue; - } - - applyPriorityToEvent(event, chain); - } - - if (preferredEvent) { - const preferredId = preferredEvent.extendedProps?.originalId || preferredEvent.id; - const preferredChain = chainById.get(preferredId); - if (preferredChain) { - applyPriorityToEvent(preferredEvent, preferredChain); - } - } -} - globalThis.handleUiChainsData = function(data) { if (chainsPromiseResolve) { cachedChains = data; - cachedChains = recalculatePriorities(cachedChains); chainsPromiseResolve(cachedChains); chainsPromiseResolve = null; } }; -globalThis.handleUiChainsSaved = function(success) { +function applyServerChains(chains) { + cachedChains = chains; + refreshCalendarEvents(chains); +} + +globalThis.handleUiChainsSaved = function(success, detail, data) { if (savePromiseResolve) { - savePromiseResolve(success); + savePromiseResolve({success: !!success, data}); savePromiseResolve = null; + } else if (success && Array.isArray(data) && timedOutChainName === getSelectedChain()) { + applyServerChains(data); + } + timedOutChainName = null; + if (!success) { + showError(detail || "Failed to save shifts"); } }; -globalThis.handleUiChainsError = function() { +globalThis.handleUiChainsError = function(detail) { if (chainsPromiseResolve) { chainsPromiseResolve([]); chainsPromiseResolve = null; } if (savePromiseResolve) { - savePromiseResolve(false); + savePromiseResolve({success: false}); savePromiseResolve = null; } + timedOutChainName = null; + showError(detail || "UI chains error"); }; const CHAINS_TOGGLE_HTML = @@ -521,9 +462,6 @@ async function openChainsModal() { async function loadChains() { const socket = getSocket(); if (!socket || socket.readyState !== WebSocket.OPEN) { - if (cachedChains.length > 0) { - return recalculatePriorities(cachedChains); - } return cachedChains; } @@ -534,41 +472,66 @@ async function loadChains() { setTimeout(() => { if (chainsPromiseResolve === resolve) { chainsPromiseResolve = null; - if (cachedChains.length > 0) { - resolve(recalculatePriorities(cachedChains)); - } else { - resolve(cachedChains); - } + resolve(cachedChains); } }, 5000); }); } -async function saveChains(chains) { +let chainModalPersistInFlight = false; +let chainsPersistQueue = Promise.resolve(); + +function runChainsMutation(work) { + const run = chainsPersistQueue.then(work); + chainsPersistQueue = run.then(() => undefined, () => undefined); + return run; +} + +function setChainModalPersistInFlight(inFlight) { + chainModalPersistInFlight = inFlight; + document.getElementById("save-chain-btn")?.toggleAttribute("disabled", inFlight); + document.getElementById("delete-chain-btn")?.toggleAttribute("disabled", inFlight); +} + +async function persistChainMutation(message, nextChains) { if (!getSelectedChain()) { - showError('Select a chain first'); - return; + showError("Select a chain first"); + return false; } - const recalculatedChains = recalculatePriorities(chains); - chains.splice(0, chains.length, ...recalculatedChains); - cachedChains = recalculatedChains; + const previousChains = cachedChains; + timedOutChainName = null; + applyServerChains(recalculatePriorities(nextChains)); const socket = getSocket(); + let result = {success: false}; if (!socket || socket.readyState !== WebSocket.OPEN) { - console.error('WebSocket not connected, cannot save ui chains'); - return; + showError("WebSocket not connected, cannot save shifts"); + } else { + result = await new Promise((resolve) => { + savePromiseResolve = resolve; + socket.send(JSON.stringify(message)); + setTimeout(() => { + if (savePromiseResolve === resolve) { + savePromiseResolve = null; + timedOutChainName = message.chain_name; + resolve({success: false, timedOut: true}); + } + }, 5000); + }); } - - return new Promise((resolve) => { - savePromiseResolve = resolve; - socket.send(JSON.stringify({event: "save_ui_chains", chain_name: getSelectedChain(), data: recalculatedChains})); - - setTimeout(() => { - if (savePromiseResolve === resolve) { - savePromiseResolve = null; - resolve(false); - } - }, 5000); - }); + const selected = getSelectedChain() === message.chain_name; + if (!result.success) { + if (result.timedOut) { + showError("Failed to save shifts"); + } + if (selected) { + applyServerChains(previousChains); + } + return false; + } + if (selected && Array.isArray(result.data)) { + applyServerChains(result.data); + } + return true; } function generateId() { @@ -1002,43 +965,6 @@ function findFutureRepeatEvents(chains, start, excludeId = null) { } -async function updateEventPriority(droppedEvent) { - const droppedStart = droppedEvent.start; - const droppedEnd = droppedEvent.end; - - const chains = await loadChains(); - const droppedOriginalId = droppedEvent.extendedProps?.originalId || droppedEvent.id; - const droppedChainIndex = chains.findIndex(c => c.id === droppedOriginalId); - - if (droppedChainIndex === -1) { - droppedEvent.setExtendedProp('priority', 2); - return; - } - - const droppedChain = chains[droppedChainIndex]; - const previousOverlapping = findOverlappingChainsForChain(chains, droppedChain, droppedChain.id); - const updatedDroppedChain = { - ...droppedChain, - start: droppedStart.toISOString(), - end: droppedEnd ? droppedEnd.toISOString() : null - }; - - chains[droppedChainIndex] = updatedDroppedChain; - - const newOverlapping = findOverlappingChainsForChain(chains, updatedDroppedChain, updatedDroppedChain.id); - const impactedIds = [ - updatedDroppedChain.id, - ...previousOverlapping.map(chain => chain.id), - ...newOverlapping.map(chain => chain.id) - ]; - - const updatedChains = recalculatePrioritiesForChainIds(chains, impactedIds); - syncCalendarEventPriorities(updatedChains, impactedIds, droppedEvent); - - chains.splice(0, chains.length, ...updatedChains); - await saveChains(chains); -} - function toggleRepeatUntilVisibility() { const repeatSelect = document.getElementById('chain-repeat'); const untilGroup = document.getElementById('chain-until-group'); @@ -1093,7 +1019,8 @@ function getChainModalInputs() { }; } -function refreshCalendarEvents(expandedChains) { +function refreshCalendarEvents(chains) { + const expandedChains = prepareEventsForCalendar(chains); if (calendar) { calendar.removeAllEvents(); calendar.addEventSource(expandedChains); @@ -1119,16 +1046,26 @@ async function handleEventTimeChange(info) { return; } - await updateEventPriority(info.event); - - const chains = await loadChains(); - const index = chains.findIndex(c => c.id === originalId); - if (index !== -1) { - chains[index].start = info.event.start.toISOString(); - chains[index].end = info.event.end ? info.event.end.toISOString() : null; - chains[index].priority = info.event.extendedProps?.priority ?? 2; - await persistChainsAndRerender(chains); - } + await runChainsMutation(async () => { + const chains = [...cachedChains]; + const index = chains.findIndex((c) => c.id === originalId); + if (index === -1) { + info.revert(); + return; + } + chains[index] = { + ...chains[index], + start: info.event.start.toISOString(), + end: info.event.end ? info.event.end.toISOString() : null, + }; + const saved = await persistChainMutation( + {event: "save_ui_chains", chain_name: getSelectedChain(), data: chains[index]}, + chains, + ); + if (!saved) { + info.revert(); + } + }); } function closeChainEditModal() { @@ -1137,11 +1074,6 @@ function closeChainEditModal() { currentChainId = null; } -async function persistChainsAndRerender(chains) { - await saveChains(chains); - refreshCalendarEvents(getExpandedChains(chains)); -} - function stripTrailingWaitSteps(steps) { if (steps.length === 0) { return steps; @@ -1206,118 +1138,84 @@ function validateChainInput() { } async function saveChain() { + if (chainModalPersistInFlight) return; const input = validateChainInput(); if (!input) return; - const { start, end, repeat, repeatEnd, steps } = input; - const chains = await loadChains(); + const shiftId = currentChainId; - if (currentChainId) { - const candidateChain = { - id: currentChainId, - start, - end: end || null, - repeat: repeat || null, - repeatEnd: repeat ? (repeatEnd || null) : null - }; - const overlapping = findOverlappingChainsForChain(chains, candidateChain, currentChainId); - if (overlapping.length >= 2) { - showOverlapError(); - return; - } - - if (repeat) { - const futureRepeatEvents = findFutureRepeatEvents(chains, start, currentChainId); - if (futureRepeatEvents.length > 0) { - showError('Cannot create REPEAT event: another REPEAT event exists in the future'); - return; - } - } - - const index = chains.findIndex(c => c.id === currentChainId); - if (index !== -1) { - const existingChain = chains[index]; - const previousOverlapping = findOverlappingChainsForChain(chains, existingChain, currentChainId); - const updatedChain = { - ...chains[index], + setChainModalPersistInFlight(true); + try { + await runChainsMutation(async () => { + const chains = [...cachedChains]; + const schedule = { start, end: end || null, repeat: repeat || null, repeatEnd: repeat ? (repeatEnd || null) : null, - steps: steps.length > 0 ? steps : null }; - chains[index] = updatedChain; - const impactedIds = [ - currentChainId, - ...previousOverlapping.map(chain => chain.id), - ...overlapping.map(chain => chain.id) - ]; - const recalculatedChains = recalculatePrioritiesForChainIds(chains, impactedIds); - chains.splice(0, chains.length, ...recalculatedChains); - } - - await persistChainsAndRerender(chains); - closeChainEditModal(); - return; - } else { - const candidateChain = { - start, - end: end || null, - repeat: repeat || null, - repeatEnd: repeat ? (repeatEnd || null) : null - }; - const overlapping = findOverlappingChainsForChain(chains, candidateChain); - if (overlapping.length >= 2) { - showOverlapError(); - return; - } - - if (repeat) { - const futureRepeatEvents = findFutureRepeatEvents(chains, start); - if (futureRepeatEvents.length > 0) { - showError('Cannot create REPEAT event: another REPEAT event exists in the future'); + const overlapping = findOverlappingChainsForChain(chains, schedule, shiftId); + if (overlapping.length >= 2) { + showOverlapError(); return; } - } - - const newChain = { - id: generateId(), - title: '', - start, - end: end || null, - repeat: repeat || null, - repeatEnd: repeat ? (repeatEnd || null) : null, - steps: steps.length > 0 ? steps : null - }; - const newPriority = calculateNewPriority(newChain, overlapping); - newChain.priority = newPriority; - - for (const overlappingChain of overlapping) { - const overlappingIndex = chains.findIndex(c => c.id === overlappingChain.id); - if (overlappingIndex !== -1) { - const otherOverlapping = [newChain, ...overlapping.filter(c => c.id !== overlappingChain.id)]; - const otherPriority = calculateNewPriority(overlappingChain, otherOverlapping); - chains[overlappingIndex].priority = otherPriority; + if (repeat) { + const futureRepeatEvents = findFutureRepeatEvents(chains, start, shiftId); + if (futureRepeatEvents.length > 0) { + showError("Cannot create REPEAT event: another REPEAT event exists in the future"); + return; + } } - } - - chains.push(newChain); + let savedShift; + if (shiftId) { + const index = chains.findIndex((c) => c.id === shiftId); + if (index === -1) return; + savedShift = { + ...chains[index], + ...schedule, + steps: steps.length > 0 ? steps : null, + }; + chains[index] = savedShift; + } else { + savedShift = { + id: generateId(), + title: "", + ...schedule, + steps: steps.length > 0 ? steps : null, + }; + chains.push(savedShift); + } + const saved = await persistChainMutation( + {event: "save_ui_chains", chain_name: getSelectedChain(), data: savedShift}, + chains, + ); + if (saved) { + closeChainEditModal(); + } + }); + } finally { + setChainModalPersistInFlight(false); } - - await persistChainsAndRerender(chains); - closeChainEditModal(); } async function deleteChain() { - if (!currentChainId) return; + if (!currentChainId || chainModalPersistInFlight) return; + const shiftId = currentChainId; + setChainModalPersistInFlight(true); try { - const chains = await loadChains(); - const filtered = chains.filter(c => c.id !== currentChainId); - await persistChainsAndRerender(filtered); - closeChainEditModal(); - } catch (error) { - console.error('Failed to delete chain:', error); + await runChainsMutation(async () => { + const chains = cachedChains.filter((c) => c.id !== shiftId); + const saved = await persistChainMutation( + {event: "delete_ui_chain", chain_name: getSelectedChain(), id: shiftId}, + chains, + ); + if (saved) { + closeChainEditModal(); + } + }); + } finally { + setChainModalPersistInFlight(false); } } @@ -1480,7 +1378,7 @@ async function updateCalendarTimezone() { monthCalendar.destroy(); const chains = await loadChains(); - const expandedChains = getExpandedChains(chains); + const expandedChains = prepareEventsForCalendar(chains); const calendarOptions = buildMainCalendarOptions(expandedChains, firstDay, timezone); const monthOptions = buildMonthCalendarOptions(expandedChains, firstDay, timezone); @@ -1531,18 +1429,23 @@ function updateCurrentWeekHighlight() { async function setRepeatEndFromEvent(event, isLastOccurrence) { const originalId = event.extendedProps?.originalId || event.id; - const chains = await loadChains(); - const index = chains.findIndex(c => c.id === originalId); - if (index === -1) { - return; - } - if (isLastOccurrence) { - chains[index].repeatEnd = null; - } else { - const eventEnd = event.end || new Date(event.start.getTime() + 24 * 60 * 60 * 1000); - chains[index].repeatEnd = eventEnd.toISOString(); - } - await persistChainsAndRerender(chains); + await runChainsMutation(async () => { + const chains = [...cachedChains]; + const index = chains.findIndex((c) => c.id === originalId); + if (index === -1) { + return; + } + if (isLastOccurrence) { + chains[index] = {...chains[index], repeatEnd: null}; + } else { + const eventEnd = event.end || new Date(event.start.getTime() + 24 * 60 * 60 * 1000); + chains[index] = {...chains[index], repeatEnd: eventEnd.toISOString()}; + } + await persistChainMutation( + {event: "save_ui_chains", chain_name: getSelectedChain(), data: chains[index]}, + chains, + ); + }); } function mountRepeatEndButton(el, event) { @@ -1812,7 +1715,7 @@ async function initializeCalendars() { } const chains = await loadChains(); - const expandedChains = getExpandedChains(chains); + const expandedChains = prepareEventsForCalendar(chains); calendar = new FullCalendar.Calendar(calendarEl, buildMainCalendarOptions(expandedChains, firstDay, timezone)); @@ -1902,13 +1805,7 @@ export const ChainsManager = { if (monthCalendar) monthCalendar.updateSize(); }, 100); } else { - const chains = await loadChains(); - const expandedChains = getExpandedChains(chains); - calendar.removeAllEventSources(); - calendar.addEventSource(expandedChains); - monthCalendar.removeAllEventSources(); - monthCalendar.addEventSource(expandedChains); - updateEventStyles(); + refreshCalendarEvents(await loadChains()); } }); } diff --git a/static/js/websocket.js b/static/js/websocket.js index d5c5ce9b..7f64641f 100644 --- a/static/js/websocket.js +++ b/static/js/websocket.js @@ -97,8 +97,12 @@ const WEBSOCKET_DATA_HANDLERS = { maintenance_data: "handleMaintenanceData", }; +const WEBSOCKET_SAVED_HANDLERS = { + maintenance_saved: "handleMaintenanceSaved", + ui_chains_saved: "handleUiChainsSaved", +}; + const WEBSOCKET_MESSAGE_FIELD_HANDLERS = { - ui_chains_saved: ["handleUiChainsSaved", "success"], ui_chains_error: ["handleUiChainsError", "detail"], maintenance_error: ["handleMaintenanceError", "detail"], }; @@ -109,8 +113,9 @@ function dispatchOptionalGlobalHandler(message) { globalThis[dataHandler](message.data); return true; } - if (message.event === "maintenance_saved") { - globalThis.handleMaintenanceSaved(message.success, message.detail); + const savedHandler = WEBSOCKET_SAVED_HANDLERS[message.event]; + if (savedHandler) { + globalThis[savedHandler](message.success, message.detail, message.data); return true; } const fieldHandler = WEBSOCKET_MESSAGE_FIELD_HANDLERS[message.event]; diff --git a/tests/test_chain/test_ui_chains_store.py b/tests/test_chain/test_ui_chains_store.py index 00fae1de..ec2f32bd 100644 --- a/tests/test_chain/test_ui_chains_store.py +++ b/tests/test_chain/test_ui_chains_store.py @@ -137,7 +137,7 @@ def test_prune_expired_shifts_keeps_repeating_shift_without_repeat_end(tmp_path: "repeatEnd": None, "steps": [{"user": "oncall"}], } - store.save_shifts("primary", [repeating_shift]) + store.upsert_shift("primary", repeating_shift) removed = store.prune_expired_shifts("primary", now) assert removed == 0 @@ -169,7 +169,7 @@ def test_prune_expired_shifts_removes_repeating_shift_with_past_repeat_end(tmp_p mock_config.stop() -def test_save_shifts_filters_expired_shifts(tmp_path: Path): +def test_upsert_drops_expired_siblings(tmp_path: Path): store = _make_store(tmp_path) mock_config = _mock_closed_retention("7d") fixed_now = datetime(2026, 6, 16, 12, 0, tzinfo=timezone.utc) @@ -187,16 +187,20 @@ def test_save_shifts_filters_expired_shifts(tmp_path: Path): "end": "2026-06-10T12:00:00Z", "steps": [{"user": "bob"}], } + _write_shifts_unfiltered(store, "primary", [old_shift, recent_shift]) def filter_at_fixed_now(shifts, now=None): return UIChainsStore.filter_retained_shifts(store, shifts, fixed_now) store.filter_retained_shifts = filter_at_fixed_now - store.save_shifts("primary", [old_shift, recent_shift]) - - remaining = store.load_shifts("primary") - assert len(remaining) == 1 - assert remaining[0]["id"] == "recent" + ok, saved = store.upsert_shift("primary", { + "id": "new", + "start": "2026-06-15T10:00:00Z", + "end": "2026-06-15T12:00:00Z", + "steps": [{"user": "carol"}], + }) + assert ok is True + assert {shift["id"] for shift in saved} == {"recent", "new"} finally: mock_config.stop() @@ -267,3 +271,145 @@ def test_does_chain_overlap_range_repeating_daily(tmp_path: Path): outside_range = datetime(2026, 6, 10, 8, 0, tzinfo=timezone.utc) outside_end = datetime(2026, 6, 10, 18, 0, tzinfo=timezone.utc) assert store._does_chain_overlap_range(chain, outside_range, outside_end) is False + + +def test_upsert_shift_recalculates_sibling_priority(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + ok, _saved = store.upsert_shift("primary", { + "id": "a", + "start": "2026-12-10T10:00:00Z", + "end": "2026-12-10T12:00:00Z", + "steps": [{"user": "alice"}], + }) + assert ok is True + ok, saved = store.upsert_shift("primary", { + "id": "b", + "start": "2026-12-10T11:00:00Z", + "end": "2026-12-10T13:00:00Z", + "steps": [{"user": "bob"}], + }) + assert ok is True + by_id = {shift["id"]: shift["priority"] for shift in saved} + assert by_id["b"] == 1 + assert by_id["a"] == 2 + finally: + mock_config.stop() + + +def test_upsert_shift_keeps_sibling_without_steps(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + legacy = { + "id": "legacy", + "start": "2026-12-10T08:00:00Z", + "end": "2026-12-10T09:00:00Z", + } + _write_shifts_unfiltered(store, "primary", [legacy]) + ok, saved = store.upsert_shift("primary", { + "id": "new", + "start": "2026-12-11T10:00:00Z", + "end": "2026-12-11T12:00:00Z", + "steps": [{"user": "alice"}], + }) + assert ok is True + by_id = {shift["id"]: shift for shift in saved} + assert by_id["legacy"]["steps"] is None + assert by_id["new"]["steps"] == [{"user": "alice"}] + ics = Path(store._calendar_path("primary")).read_bytes() + assert ics.count(b"DESCRIPTION") == 1 + finally: + mock_config.stop() + + +def test_upsert_rejects_list_without_writing(tmp_path: Path): + store = _make_store(tmp_path) + ok, saved = store.upsert_shift("primary", [{"id": "s1"}]) + assert ok is False + assert saved == [] + assert store.load_shifts("primary") == [] + assert not (tmp_path / "ui_chains" / "primary.ics").exists() + + +def test_delete_shift_leaves_remaining(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + store.upsert_shift("primary", { + "id": "w1", + "start": "2026-12-10T10:00:00Z", + "end": "2026-12-10T12:00:00Z", + "steps": [{"user": "alice"}], + }) + store.upsert_shift("primary", { + "id": "w2", + "start": "2026-12-11T10:00:00Z", + "end": "2026-12-11T12:00:00Z", + "steps": [{"user": "bob"}], + }) + ok, saved = store.delete_shift("primary", "w1") + assert ok is True + assert [shift["id"] for shift in saved] == ["w2"] + assert [shift["id"] for shift in store.load_shifts("primary")] == ["w2"] + finally: + mock_config.stop() + + +def test_delete_missing_shift_is_success(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + store.upsert_shift("primary", { + "id": "w1", + "start": "2026-12-10T10:00:00Z", + "end": "2026-12-10T12:00:00Z", + "steps": [{"user": "alice"}], + }) + ok, saved = store.delete_shift("primary", "missing") + assert ok is True + assert [shift["id"] for shift in saved] == ["w1"] + assert [shift["id"] for shift in store.load_shifts("primary")] == ["w1"] + finally: + mock_config.stop() + + +def test_upsert_shift_collapses_duplicate_ids(tmp_path: Path): + store = _make_store(tmp_path) + mock_config = _mock_closed_retention("7d") + try: + _write_shifts_unfiltered(store, "primary", [ + { + "id": "dup", + "start": "2026-12-10T10:00:00Z", + "end": "2026-12-10T12:00:00Z", + "steps": [{"user": "alice"}], + }, + { + "id": "dup", + "start": "2026-12-10T14:00:00Z", + "end": "2026-12-10T16:00:00Z", + "steps": [{"user": "bob"}], + }, + { + "id": "keep", + "start": "2026-12-11T10:00:00Z", + "end": "2026-12-11T12:00:00Z", + "steps": [{"user": "carol"}], + }, + ]) + ok, saved = store.upsert_shift("primary", { + "id": "dup", + "start": "2026-12-12T10:00:00Z", + "end": "2026-12-12T12:00:00Z", + "steps": [{"user": "dana"}], + }) + assert ok is True + ids = [shift["id"] for shift in saved] + assert ids.count("dup") == 1 + assert "keep" in ids + assert {shift["id"]: shift["steps"] for shift in saved}["dup"] == [{"user": "dana"}] + assert [shift["id"] for shift in store.load_shifts("primary")].count("dup") == 1 + finally: + mock_config.stop() diff --git a/tests/test_routes_auth.py b/tests/test_routes_auth.py index 8aa6040d..91747908 100644 --- a/tests/test_routes_auth.py +++ b/tests/test_routes_auth.py @@ -273,10 +273,10 @@ def test_save_ui_chains_rejected_when_unauthenticated(self, config, messenger): ws.send_json({ "event": "save_ui_chains", "chain_name": "primary", - "data": [], + "data": {"id": "shift-1"}, }) message = ws.receive_json() - mock_store.save_shifts.assert_not_called() + mock_store.upsert_shift.assert_not_called() assert message == { "event": "ui_chains_saved", "success": False, @@ -303,11 +303,34 @@ def test_request_ui_chains_rejected_when_unauthenticated(self, config, messenger } def test_save_ui_chains_allowed_when_authenticated(self, config, messenger): + auth_manager = _mock_auth_manager(authenticated=True) + app = _build_app(config, messenger, auth_manager) + shift = { + "id": "shift-1", + "start": "2026-06-10T10:00:00+00:00", + "end": "2026-06-10T12:00:00+00:00", + } + saved = [{**shift, "priority": 2}] + with patch("app.routes.get_config", return_value=config), \ + patch("app.routes.ui_chains_store") as mock_store: + mock_store.upsert_shift.return_value = (True, saved) + with TestClient(app, cookies={SESSION_COOKIE: "valid-session"}) as client: + with client.websocket_connect("/ws") as ws: + ws.receive_json() + ws.send_json({ + "event": "save_ui_chains", + "chain_name": "primary", + "data": shift, + }) + message = ws.receive_json() + mock_store.upsert_shift.assert_called_once_with("primary", shift) + assert message == {"event": "ui_chains_saved", "success": True, "data": saved} + + def test_save_ui_chains_rejects_list_payload(self, config, messenger): auth_manager = _mock_auth_manager(authenticated=True) app = _build_app(config, messenger, auth_manager) with patch("app.routes.get_config", return_value=config), \ patch("app.routes.ui_chains_store") as mock_store: - mock_store.save_shifts.return_value = True with TestClient(app, cookies={SESSION_COOKIE: "valid-session"}) as client: with client.websocket_connect("/ws") as ws: ws.receive_json() @@ -317,5 +340,49 @@ def test_save_ui_chains_allowed_when_authenticated(self, config, messenger): "data": [{"id": "shift-1"}], }) message = ws.receive_json() - mock_store.save_shifts.assert_called_once_with("primary", [{"id": "shift-1"}]) - assert message == {"event": "ui_chains_saved", "success": True} + mock_store.upsert_shift.assert_not_called() + assert message == { + "event": "ui_chains_saved", + "success": False, + "detail": "shift must be an object", + } + + def test_delete_ui_chain_rejected_when_unauthenticated(self, config, messenger): + auth_manager = _mock_auth_manager(authenticated=False) + app = _build_app(config, messenger, auth_manager) + with patch("app.routes.get_config", return_value=config), \ + patch("app.routes.ui_chains_store") as mock_store: + with TestClient(app) as client: + with client.websocket_connect("/ws") as ws: + ws.receive_json() + ws.send_json({ + "event": "delete_ui_chain", + "chain_name": "primary", + "id": "shift-1", + }) + message = ws.receive_json() + mock_store.delete_shift.assert_not_called() + assert message == { + "event": "ui_chains_saved", + "success": False, + "detail": "Authentication required", + } + + def test_delete_ui_chain_returns_remaining_shifts(self, config, messenger): + auth_manager = _mock_auth_manager(authenticated=True) + app = _build_app(config, messenger, auth_manager) + remaining = [{"id": "shift-2", "priority": 2}] + with patch("app.routes.get_config", return_value=config), \ + patch("app.routes.ui_chains_store") as mock_store: + mock_store.delete_shift.return_value = (True, remaining) + with TestClient(app, cookies={SESSION_COOKIE: "valid-session"}) as client: + with client.websocket_connect("/ws") as ws: + ws.receive_json() + ws.send_json({ + "event": "delete_ui_chain", + "chain_name": "primary", + "id": "shift-1", + }) + message = ws.receive_json() + mock_store.delete_shift.assert_called_once_with("primary", "shift-1") + assert message == {"event": "ui_chains_saved", "success": True, "data": remaining} From ce8e604c72c4afa88d26669f3b6c00e997e12e12 Mon Sep 17 00:00:00 2001 From: tansdf Date: Sat, 19 Sep 2026 12:18:24 +0300 Subject: [PATCH 3/3] sonar --- app/routes.py | 7 ++++--- tests/test_maintenance/test_api.py | 3 ++- tests/test_maintenance/test_store.py | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/routes.py b/app/routes.py index f56fb0b0..10004621 100644 --- a/app/routes.py +++ b/app/routes.py @@ -31,6 +31,7 @@ _MSG_INCIDENT_NOT_FOUND = "Incident not found" _MSG_UNIQ_ID_REQUIRED = "uniq_id is required" _MSG_AUTHENTICATION_REQUIRED = "Authentication required" +_MSG_ID_REQUIRED = "id is required" async def _send_saved_event(websocket, event, success, detail=None, data=None): @@ -414,7 +415,7 @@ async def websocket_endpoint(websocket: WebSocket): if not isinstance(payload, dict): await _send_saved_event(websocket, "ui_chains_saved", False, "shift must be an object") elif not payload.get("id"): - await _send_saved_event(websocket, "ui_chains_saved", False, "id is required") + await _send_saved_event(websocket, "ui_chains_saved", False, _MSG_ID_REQUIRED) else: success, saved = ui_chains_store.upsert_shift(chain_name, payload) if success: @@ -428,7 +429,7 @@ async def websocket_endpoint(websocket: WebSocket): chain_name = message.get("chain_name", "") shift_id = message.get("id") if not shift_id: - await _send_saved_event(websocket, "ui_chains_saved", False, "id is required") + await _send_saved_event(websocket, "ui_chains_saved", False, _MSG_ID_REQUIRED) else: success, saved = ui_chains_store.delete_shift(chain_name, str(shift_id)) if success: @@ -477,7 +478,7 @@ async def websocket_endpoint(websocket: WebSocket): else: window_id = message.get("id") if not window_id: - await _send_saved_event(websocket, "maintenance_saved", False, "id is required") + await _send_saved_event(websocket, "maintenance_saved", False, _MSG_ID_REQUIRED) else: store = get_maintenance_store() success, existing_before, saved, deleted = store.delete_window(str(window_id)) diff --git a/tests/test_maintenance/test_api.py b/tests/test_maintenance/test_api.py index 31d9f1ac..0080bf0f 100644 --- a/tests/test_maintenance/test_api.py +++ b/tests/test_maintenance/test_api.py @@ -66,6 +66,7 @@ def test_window_from_ws_item_allows_existing_owner_not_assignable(): def test_window_from_ws_item_rejects_list(): + payload = [_window_payload()] with pytest.raises(HTTPException) as exc: - window_from_ws_item([_window_payload()], assignable_user_ids=ASSIGNABLE) + window_from_ws_item(payload, assignable_user_ids=ASSIGNABLE) assert exc.value.detail == "window must be an object" diff --git a/tests/test_maintenance/test_store.py b/tests/test_maintenance/test_store.py index a89aed4a..3b7b4475 100644 --- a/tests/test_maintenance/test_store.py +++ b/tests/test_maintenance/test_store.py @@ -259,8 +259,9 @@ def test_upsert_rejects_invalid_payload_without_writing(tmp_path: Path): def test_upsert_rejects_list_without_writing(tmp_path: Path): store = _make_store(tmp_path) + payload = [_sample_window()] with pytest.raises(HTTPException) as exc: - store.upsert_window([_sample_window()], ASSIGNABLE) + store.upsert_window(payload, ASSIGNABLE) assert exc.value.detail == "window must be an object" assert store.load_windows() == [] assert not Path(store._file).exists()