diff --git a/CHANGELOG.md b/CHANGELOG.md index 964c7087..9c174ba3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,25 @@ ## Unreleased +- Fix three correctness regressions from the + scheduling + sync work. (1) A cloud-pull update for a + task whose wire payload omits `scheduled`/`deadline` + (legacy peer, PWA, iOS on an older build) no longer + silently clears the local dates; `None` on the wire + is now "leave unchanged", matching the rest of the + PATCH convention. (2) The JSON backend's `list_tasks` + / `list_archived` normalise legacy task dicts so + `scheduled`/`deadline` always come back as `None` + instead of missing keys — the SQL and markdown + backends already did this, the JSON one diverged. + (3) The PAUSED-on-pull clear from v2.5.1 was too + aggressive: it wiped the local pause flag on every + cloud-origin sync, including unrelated edits (notes + appended, customer renamed). Now PAUSED only clears + when the cloud actually touched the entry's `end` — + resume or stop — so editing notes on the iPhone for + a paused entry leaves the desktop pause flag alone. + - Make the 5-minute cloud-sync poller's `_broadcast_sync_changes` actually refresh the kanban. It was sending `resource: "tasks"` while the frontend's diff --git a/kaisho/backends/json_backend/__init__.py b/kaisho/backends/json_backend/__init__.py index 296efc94..e6025358 100644 --- a/kaisho/backends/json_backend/__init__.py +++ b/kaisho/backends/json_backend/__init__.py @@ -129,6 +129,23 @@ def _guess_inbox_type(text: str) -> str: return "note" +def _normalize_task(task: dict) -> dict: + """Ensure every task dict carries the full + documented shape — used on the read path so legacy + rows (written before a field was added) match what + SQL and markdown backends emit and what + ``frontend/src/types.ts:Task`` expects. + + Today the only fields needing a missing → None fill + are ``scheduled`` and ``deadline``. The helper is + deliberately additive so future schema growth lands + in one place. + """ + task.setdefault("scheduled", None) + task.setdefault("deadline", None) + return task + + # ==================================================================== # TaskBackend # ==================================================================== @@ -156,7 +173,10 @@ def list_tasks( include_done=False, ) -> list[dict]: """Return tasks matching the given filters.""" - tasks = _read_json(self._tasks_file) + tasks = [ + _normalize_task(t) + for t in _read_json(self._tasks_file) + ] if not include_done: tasks = [ t for t in tasks @@ -198,7 +218,10 @@ def list_all_tags(self) -> list[dict]: def list_archived(self) -> list[dict]: """Return all archived tasks.""" - return _read_json(self._archive_file) + return [ + _normalize_task(t) + for t in _read_json(self._archive_file) + ] # -- mutations ----------------------------------------------- diff --git a/kaisho/backends/sql/__init__.py b/kaisho/backends/sql/__init__.py index 27f59993..32d01105 100644 --- a/kaisho/backends/sql/__init__.py +++ b/kaisho/backends/sql/__init__.py @@ -1410,6 +1410,15 @@ def _match_clock_by_content(session, fields): def _apply_clock_fields(row: ClockRow, fields: dict) -> None: """Set ClockRow columns from a sync payload. The caller enforces last-writer-wins before calling this.""" + # Snapshot ``row.end`` before the new value lands so + # we can tell whether the cloud-origin change touched + # the entry's timing — see PAUSED handling below. + prev_end = row.end + new_end = ( + fields["end"] or None if "end" in fields + else row.end + ) + for col in ( "customer", "description", "start", "task_id", "contract", "notes", @@ -1417,21 +1426,25 @@ def _apply_clock_fields(row: ClockRow, fields: dict) -> None: if col in fields: setattr(row, col, fields[col] or "") if "end" in fields: - row.end = fields["end"] or None + row.end = new_end if "invoiced" in fields: row.invoiced = bool(fields["invoiced"]) row.sync_id = fields["sync_id"] row.updated_at = ( fields.get("updated_at") or _local_now().isoformat() ) - # ``paused`` is a desktop-only UI affordance and never - # crosses the wire. Any cloud-origin pull is therefore - # authoritative evidence that the entry is *not* paused - # — otherwise the running-timer card stays stuck on a - # stale Resume affordance for an entry the cloud has - # since resumed or stopped on another device. Mirrors - # the same clear in ``services/clocks.py:apply_sync_payload``. - row.paused = False + # ``paused`` is a desktop-only UI affordance that + # never crosses the wire. Only clear it when the + # cloud actually touched this entry's timing — resume + # clears ``end``, stop sets it. A non-timing change + # (notes appended on the PWA, customer renamed, tag + # adjusted) leaves ``end`` unchanged and must NOT + # wipe the local pause flag: the user paused on this + # device and still intends to resume. Mirrors the + # narrower clear in + # ``services/clocks.py:apply_sync_payload``. + if prev_end != new_end: + row.paused = False # ==================================================================== diff --git a/kaisho/services/clocks.py b/kaisho/services/clocks.py index c6acb010..776f027d 100644 --- a/kaisho/services/clocks.py +++ b/kaisho/services/clocks.py @@ -750,6 +750,11 @@ def apply_sync_payload( else None ) + # Snapshot the previous ``end`` *before* the new value + # lands so we can tell whether the cloud-origin + # change actually touched the entry's timing. + prev_end = clock.end + clock.start = start clock.end = end if end is not None: @@ -780,13 +785,17 @@ def apply_sync_payload( props.pop("INVOICED", None) props["SYNC_ID"] = fields["sync_id"] props["UPDATED_AT"] = fields["updated_at"] - # ``PAUSED`` is a desktop-only UI affordance and never - # crosses the wire. Any cloud-origin pull is therefore - # authoritative evidence that the entry is *not* paused - # — otherwise the running-timer card stays stuck on a - # stale Resume affordance for an entry the cloud has - # since resumed or stopped on another device. - props.pop("PAUSED", None) + # ``PAUSED`` is a desktop-only UI affordance that never + # crosses the wire. Only clear it when the cloud + # actually touched the entry's timing — resume sets + # ``end`` from a value to None, stop sets it from None + # (or a previous value) to a new one. A non-timing + # change (notes appended on the PWA, customer renamed, + # tag adjusted) leaves ``end`` unchanged and must NOT + # wipe the local pause flag — the user paused on this + # device and still intends to resume. + if prev_end != end: + props.pop("PAUSED", None) notes = fields.get("notes") or "" heading.body = ( diff --git a/kaisho/services/cloud_sync.py b/kaisho/services/cloud_sync.py index 5633a5e3..5efd910e 100644 --- a/kaisho/services/cloud_sync.py +++ b/kaisho/services/cloud_sync.py @@ -1698,14 +1698,27 @@ def _apply_task_update( status (only when changed, since move_task is the write that moves between sections in the org file). """ + # Pass scheduled / deadline through as-is. ``None`` + # means "leave the local value alone" -- which is + # exactly the right behaviour for two cases that we + # can't distinguish at this layer: a wire payload + # from an older peer that doesn't know about these + # fields, and a fresh wire payload whose field was + # never set on the cloud side. Coercing ``None`` to + # ``""`` (the previous code) would silently clear a + # local date whenever either happened. The + # trade-off: a cloud-side clear by a newer peer is + # also seen as "leave alone" until the user wakes / + # resets the date locally. Acceptable until the wire + # carries an explicit "cleared" marker. backend.tasks.update_task( existing["id"], title=incoming["title"], customer=incoming["customer"], body=incoming["body"], github_url=incoming["github_url"], - scheduled=incoming.get("scheduled") or "", - deadline=incoming.get("deadline") or "", + scheduled=incoming.get("scheduled"), + deadline=incoming.get("deadline"), ) if incoming.get("tags") != existing.get("tags"): backend.tasks.set_tags( diff --git a/tests/test_clocks_paused_clear_on_pull.py b/tests/test_clocks_paused_clear_on_pull.py index 96b967be..73b24eb5 100644 --- a/tests/test_clocks_paused_clear_on_pull.py +++ b/tests/test_clocks_paused_clear_on_pull.py @@ -110,3 +110,49 @@ def test_pull_clears_paused_for_resumed_running_entry( # And the clock is now running (no end). clock = org.headings[0].logbook[0] assert clock.end is None + + +def test_pull_preserves_paused_on_non_timing_edit( + tmp_path, +): + """A cloud-origin update that does NOT touch the + entry's timing (notes appended, customer renamed, + tag adjusted) must leave the local PAUSED flag + alone. The user paused on this device and still + intends to resume — an unrelated remote edit must + not silently wipe that intent.""" + clocks_file = tmp_path / "clocks.org" + sync_id = "abc-notes" + _seed_paused_entry(clocks_file, sync_id) + + clocks_svc.update_clock_entry_by_sync_id( + clocks_file=clocks_file, + sync_id=sync_id, + fields={ + "sync_id": sync_id, + "customer": "KAISHO", + "description": "paused-entry", + "start": "2026-06-09T10:29:00", + # End matches the seed exactly — the cloud + # only changed the notes. + "end": "2026-06-09T10:30:00", + "updated_at": "2099-01-01T11:00:01", + "invoiced": False, + "task_id": None, + "contract": None, + "notes": "remote added a note", + }, + ) + + org = parse_org_file(clocks_file, CLOCK_KEYWORDS) + assert ( + org.headings[0].properties.get("PAUSED") + == "true" + ), ( + "PAUSED must survive a cloud pull that did not " + "change the entry's end timestamp — only resume " + "or stop should clear the local pause intent" + ) + # And the note made it through. + body_text = "\n".join(org.headings[0].body) + assert "remote added a note" in body_text diff --git a/tests/test_sql_backend_sync.py b/tests/test_sql_backend_sync.py index e9bf46d4..d5b3aaa7 100644 --- a/tests/test_sql_backend_sync.py +++ b/tests/test_sql_backend_sync.py @@ -88,6 +88,42 @@ def test_apply_clears_local_paused_flag(tmp_path): assert after["paused"] is False +def test_apply_keeps_paused_when_end_unchanged(tmp_path): + """A cloud-origin update that does NOT touch ``end`` + (notes appended, customer renamed) must leave the + local ``paused`` flag alone. The user paused on this + device and still intends to resume; an unrelated + remote edit must not silently wipe that intent. + + Mirror of + ``test_pull_preserves_paused_on_non_timing_edit`` for + the org backend.""" + clocks = _clocks(tmp_path) + clocks.apply_sync_payload(_payload(end=None)) + clocks.stop(paused=True) + paused_row = clocks.list_entries(period="all")[0] + assert paused_row["paused"] is True + paused_end = paused_row["end"] + + # Cloud sends a newer update with the SAME end + # timestamp — only the notes / description moved. + clocks.apply_sync_payload(_payload( + end=paused_end, + description="remote updated description", + updated_at="2099-01-01T13:00:01", + )) + after = clocks.list_entries(period="all")[0] + assert after["paused"] is True, ( + "paused must survive a cloud pull that did not " + "change end -- only resume or stop should clear " + "the local pause intent" + ) + assert ( + after["description"] + == "remote updated description" + ) + + def test_delete_entry_by_sync_id(tmp_path): clocks = _clocks(tmp_path) clocks.apply_sync_payload(_payload()) diff --git a/tests/test_task_scheduled_deadline.py b/tests/test_task_scheduled_deadline.py index 45198645..0e77bbde 100644 --- a/tests/test_task_scheduled_deadline.py +++ b/tests/test_task_scheduled_deadline.py @@ -206,6 +206,43 @@ def test_json_add_and_read_dates(tmp_path): assert raw[0]["scheduled"] == "2099-06-15" assert raw[0]["deadline"] == "2099-06-20" + # Round-trip via a fresh backend instance so the dates + # survive a save/load cycle (the markdown test does + # this; the JSON one was missing it before the audit). + fresh = _json_backend(tmp_path) + again = fresh.list_tasks(include_done=True)[0] + assert again["scheduled"] == "2099-06-15" + assert again["deadline"] == "2099-06-20" + + +def test_json_legacy_row_normalises_missing_dates( + tmp_path, +): + """A JSON file written before scheduled/deadline + existed must come back with both keys present and + set to None — otherwise the dict shape diverges from + what SQL and markdown emit and from what + ``frontend/src/types.ts:Task`` expects.""" + legacy = [{ + "id": "abc", + "customer": "Acme", + "title": "Old task", + "status": "TODO", + "tags": [], + "body": "", + "github_url": "", + "created": "2026-01-01T10:00:00", + }] + (tmp_path / "tasks.json").write_text( + json.dumps(legacy), encoding="utf-8", + ) + backend = _json_backend(tmp_path) + task = backend.list_tasks(include_done=True)[0] + assert "scheduled" in task + assert "deadline" in task + assert task["scheduled"] is None + assert task["deadline"] is None + # -- Markdown backend ------------------------------------- @@ -287,3 +324,62 @@ def test_wire_handles_missing_dates_as_none(): back = wire_to_task(wire) assert back["scheduled"] is None assert back["deadline"] is None + + +# -- Cloud pull does not clobber local dates -------------- + +class _BackendShim: + """Minimal stand-in for the kaisho backend façade so + cloud-sync helpers can be called against a single + sub-backend in tests. The production code expects + ``backend.tasks`` (and other domain attributes); this + wrapper exposes just what ``_apply_task_update`` + needs.""" + + def __init__(self, tasks_backend): + self.tasks = tasks_backend + + +def test_cloud_pull_does_not_clear_local_dates(tmp_path): + """A wire entry from an older peer (or a fresh peer + that never set the dates) carries ``scheduled: None`` + / ``deadline: None``. The cloud-pull path must + interpret ``None`` as "leave the local value alone", + not as "clear". Coercing ``None`` → ``""`` (the + previous code) silently wiped a local-set date every + time a remote edited the task for any other reason.""" + from kaisho.services.cloud_sync import _apply_task_update + tasks = _org_backend(tmp_path) + backend = _BackendShim(tasks) + task = tasks.add_task( + customer="Acme", + title="Locally scheduled", + scheduled="2099-06-15", + deadline="2099-06-20", + ) + existing = tasks.list_tasks(include_done=True)[0] + assert existing["scheduled"] == "2099-06-15" + + incoming = { + "sync_id": task["sync_id"], + "customer": "Acme", + "title": "Locally scheduled", + "status": "TODO", + "tags": [], + "body": "edited remotely", + "github_url": "", + "scheduled": None, + "deadline": None, + "updated_at": "2099-12-31T00:00:00", + } + _apply_task_update(backend, incoming, existing) + + after = tasks.list_tasks(include_done=True)[0] + assert after["scheduled"] == "2099-06-15", ( + "cloud pull with scheduled=None must NOT wipe " + "the local scheduled date" + ) + assert after["deadline"] == "2099-06-20" + # The remote did change something — confirm the + # legitimate field went through. + assert after["body"] == "edited remotely"