diff --git a/alembic/versions/0027_checklist_items_into_body.py b/alembic/versions/0027_checklist_items_into_body.py new file mode 100644 index 0000000..ed6e5d2 --- /dev/null +++ b/alembic/versions/0027_checklist_items_into_body.py @@ -0,0 +1,120 @@ +"""fold note_items into the note body and drop the table + +Revision ID: 0027 +Revises: 0026 +Create Date: 2026-08-24 + +M304. A checklist item becomes a `- [ ] milk` line of `notes.body`, and `note_items` +goes. The reason is positional, not cosmetic: a row had a position in a table and no +position in the text, so a separate list could only ever render AFTER the prose. With +the items in the body, a list can sit between two paragraphs — which is the thing that +could not be built before and no amount of restyling would have delivered. + +## This migration rewrites real content + +Every note that has items gets its body appended to. The Google Keep import is genuine +content on this instance, not fixtures, so the rules here are strict: + + * Rows are read BEFORE the table is dropped, in this one transaction. + * The existing body is never rewritten, only appended to. + * The layout — a blank line between prose and the list, nothing between consecutive + items — is byte-for-byte what `_note_markdown` has always exported and what + `derive::append_item` produces on every client. All three landing on the same text + is what lets the clients migrate their own SQLite stores independently and still + agree with the server, with no sync required to reconcile them. + +## The fold is inlined on purpose + +`notes/checklist.py` has this same function and this migration deliberately does not +import it. A migration has to keep producing what it produced the day it ran; if the +app's spacing rule ever changes, this file must not change with it. + +## `updated_at` is left alone, and that is load-bearing + +Raw SQL, so SQLAlchemy's `onupdate` never fires. Two reasons, and the second matters +more than the first. Every client folds the same rows the same way, so the new body is +news to nobody. And a client holding an UNPUSHED body edit still has the newer +`updated_at`, so when it pulls the migrated note last-write-wins keeps its edit instead +of the migration silently winning. + +The `notes` row's own `sync_revision` trigger (migration 0015) does fire, so every +migrated note becomes pullable once. That is wanted: it is what makes a client whose +local fold somehow differed converge on the server's text. + +## The downgrade is not a true inverse, and says so + +It recreates an empty `note_items` and leaves the bodies alone. Nothing is lost — +every item is still there as text, which is where this migration put it — but the old +code would show those notes as prose with no checklist. A faithful inverse is not +possible: once the items are lines, nothing distinguishes a line this migration wrote +from one somebody typed, and a downgrade that guessed would eat hand-written task +lists. The real rollback is a database restore. + +Recreating the table is not decoration, though. Migration 0015's downgrade runs +`DROP TRIGGER IF EXISTS trg_note_items_bump_note ON note_items`, and `IF EXISTS` +covers the trigger, not the table — against a missing table that statement errors. So +this is what keeps the migration chain runnable all the way back down. +""" +import re + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.dialects.postgresql import UUID + +revision = "0027" +down_revision = "0026" +branch_labels = None +depends_on = None + +_TASK_RE = re.compile(r"^\s*[-*] +\[[ xX]\](?: +.*)?$") + + +def _append_item(body: str, text: str, checked: bool) -> str: + mark = "x" if checked else " " + text = (text or "").strip() + line = f"- [{mark}] {text}" if text else f"- [{mark}]" + trimmed = (body or "").rstrip("\n") + if not trimmed.strip(): + return line + follows_a_list = bool(_TASK_RE.match(trimmed.split("\n")[-1])) + return f"{trimmed}\n{line}" if follows_a_list else f"{trimmed}\n\n{line}" + + +def upgrade(): + bind = op.get_bind() + rows = bind.execute( + sa.text("SELECT note_id, text, checked FROM note_items ORDER BY note_id, position, created_at") + ).fetchall() + + grouped: dict = {} + for note_id, text, checked in rows: + grouped.setdefault(note_id, []).append((text, bool(checked))) + + for note_id, items in grouped.items(): + body = bind.execute(sa.text("SELECT body FROM notes WHERE id = :id"), {"id": note_id}).scalar() + # An item whose note is already gone has nothing to fold into. The foreign key + # should make this impossible; skipping costs nothing and failing here would + # leave the database half-migrated. + if body is None: + continue + for text, checked in items: + body = _append_item(body, text, checked) + bind.execute(sa.text("UPDATE notes SET body = :body WHERE id = :id"), {"body": body, "id": note_id}) + + op.drop_table("note_items") + + +def downgrade(): + # Column-for-column as migration 0006 created it, index name included: 0015's + # downgrade names both the table and its trigger, so a near-enough copy is not + # good enough. + op.create_table( + "note_items", + sa.Column("id", UUID(as_uuid=True), primary_key=True), + sa.Column("note_id", UUID(as_uuid=True), sa.ForeignKey("notes.id", ondelete="CASCADE"), nullable=False), + sa.Column("text", sa.Text(), nullable=False), + sa.Column("checked", sa.Boolean(), nullable=False, server_default=sa.false()), + sa.Column("position", sa.Integer(), nullable=False, server_default="0"), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False, server_default=sa.func.now()), + ) + op.create_index("ix_note_items_note", "note_items", ["note_id"]) diff --git a/src/thoughtsync/models/all.py b/src/thoughtsync/models/all.py index 443b9db..eb77fac 100644 --- a/src/thoughtsync/models/all.py +++ b/src/thoughtsync/models/all.py @@ -9,7 +9,6 @@ from . import ( # noqa: F401 label, note, note_attachment, - note_item, note_link_preview, note_revision, saved_filter, diff --git a/src/thoughtsync/models/note.py b/src/thoughtsync/models/note.py index 28bd907..5c9bb6f 100644 --- a/src/thoughtsync/models/note.py +++ b/src/thoughtsync/models/note.py @@ -46,7 +46,6 @@ class Note(Base): display_title: Mapped[str] = mapped_column(Text(), nullable=False, server_default="") body: Mapped[str] = mapped_column(Text(), nullable=False, server_default="") color: Mapped[str] = mapped_column(Text(), nullable=False, server_default="default") - # 'text' (freeform body) or 'list' (a checklist of note_items). # Manual drag order (higher = earlier); 0 until the user reorders. position: Mapped[int] = mapped_column(Integer(), nullable=False, server_default="0") pinned: Mapped[bool] = mapped_column(Boolean(), nullable=False, server_default=func.false()) diff --git a/src/thoughtsync/models/note_item.py b/src/thoughtsync/models/note_item.py deleted file mode 100644 index 2ebfe75..0000000 --- a/src/thoughtsync/models/note_item.py +++ /dev/null @@ -1,29 +0,0 @@ -from __future__ import annotations - -import uuid -from datetime import datetime - -from sqlalchemy import Boolean, DateTime, ForeignKey, Integer, Text, func -from sqlalchemy.dialects.postgresql import UUID -from sqlalchemy.orm import Mapped, mapped_column - -from . import Base - - -class NoteItem(Base): - """A single checklist item on a note. - - Any note can have them. There is no note "kind" gating this — a checklist is - something a note HAS, not something a note IS (M13 step 2). - """ - - __tablename__ = "note_items" - - id: Mapped[uuid.UUID] = mapped_column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - note_id: Mapped[uuid.UUID] = mapped_column( - UUID(as_uuid=True), ForeignKey("notes.id", ondelete="CASCADE"), nullable=False - ) - text: Mapped[str] = mapped_column(Text(), nullable=False) - checked: Mapped[bool] = mapped_column(Boolean(), nullable=False, server_default=func.false()) - position: Mapped[int] = mapped_column(Integer(), nullable=False, server_default="0") - created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), nullable=False, server_default=func.now()) diff --git a/src/thoughtsync/notes/__init__.py b/src/thoughtsync/notes/__init__.py index f8e07a3..1a744ca 100644 --- a/src/thoughtsync/notes/__init__.py +++ b/src/thoughtsync/notes/__init__.py @@ -31,10 +31,16 @@ from ..labeling import reconcile_manual_labels, resolve_owned_label_ids from ..models.label import Label, NoteLabel from ..models.note import Note from ..models.note_attachment import NoteAttachment -from ..models.note_item import NoteItem from ..models.note_link_preview import NoteLinkPreview from ..models.note_revision import NoteRevision from ..revisions import should_snapshot +from .checklist import ( + append_item, + parse_items, + remove_item, + set_item_checked, + set_item_text, +) from ..responses import json_error, not_found, parse_uuid from ..retention import purge_note from ..settings import get_setting @@ -70,7 +76,7 @@ from .tags import ( parse_tags, ) from .recurrence import REMINDER_RECURRENCES, next_occurrence, normalize_recurrence -from .serialize import _items_for_notes, _labels_for_notes, _serialize_note, _serialize_notes +from .serialize import _labels_for_notes, _serialize_note, _serialize_notes __all__ = [ "bp", @@ -225,7 +231,6 @@ async def export_notes(): ).all() ids = [n.id for n in notes_list] labels_map = await _labels_for_notes(db, ids) - items_map = await _items_for_notes(db, ids) att_rows = ( (await db.scalars(select(NoteAttachment).where(NoteAttachment.note_id.in_(ids)))).all() if ids else [] ) @@ -247,7 +252,6 @@ async def export_notes(): with zipfile.ZipFile(buf, "w", zipfile.ZIP_DEFLATED) as zf: for n in notes_list: labels = labels_map.get(n.id, []) - items = items_map.get(n.id, []) atts = att_by_note.get(n.id, []) short = str(n.id)[:8] payload["notes"].append( @@ -263,13 +267,12 @@ async def export_notes(): "created_at": n.created_at.isoformat() if n.created_at else None, "updated_at": n.updated_at.isoformat() if n.updated_at else None, "labels": [lb["name"] for lb in labels], - "items": [{"text": it["text"], "checked": it["checked"]} for it in items], "attachments": [ {"file": f"attachments/{short}/{os.path.basename(a.path)}", "mime": a.mime} for a in atts ], } ) - zf.writestr(f"notes/{_slugify(n.display_title)}-{short}.md", _note_markdown(n, labels, items)) + zf.writestr(f"notes/{_slugify(n.display_title)}-{short}.md", _note_markdown(n, labels)) for a in atts: src = Config.media_root() / a.path if src.is_file(): @@ -385,24 +388,6 @@ async def reorder_notes(): return jsonify({"ok": True}) -async def _name_for(db, note: Note, item_texts: list[str] | None = None) -> str: - """The note's display name, consulting its checklist only when the body is silent. - - `item_texts` short-circuits the query for callers that already hold the items - (create, import). Everyone else pays one narrow SELECT, and only when the body - produced nothing — which is the uncommon case. - """ - name = derive_display_title(note.body) - if name: - return name - if item_texts is not None: - return derive_display_title("", item_texts[0] if item_texts else None) - first = await db.scalar( - select(NoteItem.text).where(NoteItem.note_id == note.id).order_by(NoteItem.position).limit(1) - ) - return derive_display_title("", first) - - @bp.post("") @login_required async def create_note(): @@ -419,17 +404,20 @@ async def create_note(): Note.owner_id == g.user_id, Note.deleted_at.is_(None) ) ) + # Items still arrive separately — a client holds a list, not a blob — but they + # are folded into the body, which is where a checklist lives now (M304). + for text in item_texts: + body = append_item(body, text) note = Note( owner_id=g.user_id, - display_title=derive_display_title(body, item_texts[0] if item_texts else None), + display_title=derive_display_title(body), body=body, color=normalize_color(data.get("color")), position=int(max_pos) + 1, ) db.add(note) - await db.flush() # assign note.id before writing items/links - for pos, text in enumerate(item_texts): - db.add(NoteItem(note_id=note.id, text=text, position=pos)) + await db.flush() # assign note.id before writing links + # The FOLDED body: an item can carry a #tag too. await _reconcile_tags(db, note) await db.commit() await db.refresh(note) @@ -484,7 +472,7 @@ async def update_note(note_id: str): if "recurrence" in data: note.recurrence = normalize_recurrence(data["recurrence"]) if "body" in data: - note.display_title = await _name_for(db, note) + note.display_title = derive_display_title(note.body) await _reconcile_tags(db, note) # Version history: snapshot the PRE-edit body, once per editing session # rather than once per write — see revisions.should_snapshot. Writing often @@ -543,7 +531,7 @@ async def restore_revision(note_id: str, rev_id: str): # the revision — with the same body ripple as a normal edit. db.add(NoteRevision(note_id=note.id, body=note.body)) note.body = rev.body - note.display_title = await _name_for(db, note) + note.display_title = derive_display_title(note.body) await _reconcile_tags(db, note) await db.commit() await db.refresh(note) @@ -575,11 +563,35 @@ async def set_note_labels(note_id: str): return jsonify(await _serialize_note(db, note)) -async def _get_item(db, note: Note, item_id: str) -> NoteItem | None: - iid = parse_uuid(item_id) - if iid is None: +def _item_index(item_id: str) -> int | None: + """An item's id is its ordinal (see serialize.items_of). Anything else is a stale + id from a client that has not reloaded, and the answer to those is 404.""" + try: + index = int(item_id) + except (TypeError, ValueError): return None - return await db.scalar(select(NoteItem).where(NoteItem.id == iid, NoteItem.note_id == note.id)) + return index if index >= 0 else None + + +async def _rewrite_body(db, note: Note, body: str): + """Every item mutation is a body edit, so all of them land here. + + One place means one place that snapshots a revision, re-derives `#tags`, recomputes + the name and queues link unfurls — rather than three routes each remembering to. + Deliberately the same sequence the PATCH route runs for a body change, because it + IS a body change. + """ + old_body = note.body + if await should_snapshot(db, note.id, old_body, body): + db.add(NoteRevision(note_id=note.id, body=old_body)) + note.body = body + note.display_title = derive_display_title(body) + await _reconcile_tags(db, note) + await db.commit() + await db.refresh(note) + if note.body != old_body: + schedule_unfurls(note.id, note.body) + return jsonify(await _serialize_note(db, note)) @bp.post("//items") @@ -591,70 +603,49 @@ async def add_item(note_id: str): note = await _get_owned(db, note_id) if note is None: return not_found() - max_pos = await db.scalar( - select(func.coalesce(func.max(NoteItem.position), -1)).where(NoteItem.note_id == note.id) - ) - db.add(NoteItem(note_id=note.id, text=text, position=int(max_pos) + 1)) - await db.commit() - return jsonify(await _serialize_note(db, note)), 201 + response = await _rewrite_body(db, note, append_item(note.body, text)) + return response, 201 @bp.patch("//items/") @login_required async def update_item(note_id: str, item_id: str): data = await request.get_json(silent=True) or {} + index = _item_index(item_id) + if index is None: + return not_found() async with session_scope() as db: note = await _get_owned(db, note_id) if note is None: return not_found() - item = await _get_item(db, note, item_id) - if item is None: + if index >= len(parse_items(note.body)): return not_found() + body = note.body if "text" in data and isinstance(data["text"], str): - item.text = data["text"] + body = set_item_text(body, index, data["text"]) if "checked" in data: - item.checked = bool(data["checked"]) - await db.commit() - return jsonify(await _serialize_note(db, note)) + body = set_item_checked(body, index, bool(data["checked"])) + return await _rewrite_body(db, note, body) @bp.delete("//items/") @login_required async def delete_item(note_id: str, item_id: str): + index = _item_index(item_id) + if index is None: + return not_found() async with session_scope() as db: note = await _get_owned(db, note_id) if note is None: return not_found() - item = await _get_item(db, note, item_id) - if item is None: + if index >= len(parse_items(note.body)): return not_found() - await db.delete(item) - await db.commit() - return jsonify(await _serialize_note(db, note)) + return await _rewrite_body(db, note, remove_item(note.body, index)) -@bp.post("//items/reorder") -@login_required -async def reorder_items(note_id: str): - data = await request.get_json(silent=True) or {} - order = data.get("item_ids") - if not isinstance(order, list): - return json_error("item_ids must be a list", 400) - async with session_scope() as db: - note = await _get_owned(db, note_id) - if note is None: - return not_found() - existing = { - str(i.id): i for i in (await db.scalars(select(NoteItem).where(NoteItem.note_id == note.id))).all() - } - pos = 0 - for iid in order: - item = existing.get(str(iid)) - if item is not None: - item.position = pos - pos += 1 - await db.commit() - return jsonify(await _serialize_note(db, note)) +# The reorder route is gone with M304. Reordering a checklist is moving a line, which +# is something a text editor already does and no client ever called this for — the +# only reference to it in the tree was a test asserting the route existed. @bp.post("//attachments") diff --git a/src/thoughtsync/notes/checklist.py b/src/thoughtsync/notes/checklist.py new file mode 100644 index 0000000..1c92286 --- /dev/null +++ b/src/thoughtsync/notes/checklist.py @@ -0,0 +1,150 @@ +"""Checklist items — a note's body IS its checklist (M304). + +A `- [ ] milk` line is the item. There is no `note_items` table beside the body any +more, which is what lets a list sit BETWEEN two paragraphs: rows had a position in a +table and no position in the text, so a separate list could only ever render after +the prose no matter how it was styled. + +The same shape as `tags.py`, one strength further along. Tags are derived from the +body too, but they MATERIALISE into `note_labels` rows because the board queries by +label. Items materialise into nothing, because nothing queries them — their only +readers are the card, the editor and `display_title`. So `parse_items` is the whole +storage layer for a checklist, and the rewriters below are how one is edited. + +THE GRAMMAR IS SHARED. Three implementations exist and they have to agree, because a +difference between any two of them is a checklist that changes shape when it syncs: + + core/src/local/derive.rs the native clients (desktop + Android) + src/thoughtsync/notes/checklist.py this file, the server + frontend/src/notes/markdown.ts the browser + + optional indent, `-` or `*`, one-or-more spaces, `[ ]`/`[x]`/`[X]`, + then either end-of-line or one-or-more spaces and the text. + +`*` is accepted because markdown.ts already takes it for a plain bullet, and a rule +that allowed `* item` but not `* [ ] item` would be one nobody could guess. `- [ ]` +with nothing after it IS an item with empty text — that is what pressing Enter on a +list leaves behind, and refusing to parse it would make a half-typed list stop being +a list. `- [X]` parses as checked and renders back lowercase, so one canonical form +survives a round trip. +""" +from __future__ import annotations + +import re +from dataclasses import dataclass + +# Anchored at both ends: a `[ ]` mid-sentence is prose, and `- [ ]x` (no space after +# the brackets) is a sentence that happens to start with brackets, not a marker. +_TASK_RE = re.compile(r"^(?P\s*)(?P[-*]) +\[(?P[ xX])\](?: +(?P.*))?$") + + +@dataclass(frozen=True) +class Item: + """One checklist item. Its position in the parsed list is its identity — the same + thing `position` meant when these were rows, and all the wire ever carried.""" + + text: str + checked: bool + + +def parse_items(body: str | None) -> list[Item]: + """Every checklist item in `body`, in the order they appear.""" + out: list[Item] = [] + for line in (body or "").split("\n"): + match = _TASK_RE.match(line) + if match: + out.append(Item(text=match.group("text") or "", checked=match.group("mark") in "xX")) + return out + + +def render_item(text: str, checked: bool, indent: str = "", bullet: str = "-") -> str: + """One item as the line that stores it. + + Always lowercase `x`, whatever was parsed: one canonical output is what makes a + round trip stable, so `- [X]` normalises the first time it is touched and never + again. + """ + mark = "x" if checked else " " + if not text: + return f"{indent}{bullet} [{mark}]" + return f"{indent}{bullet} [{mark}] {text}" + + +def strip_marker(line: str) -> str: + """The text of a line with its task marker removed, or the line as it was. + + For naming a note: a list-only note is named by its first item, and calling one + "- [ ] milk" would be showing someone the storage instead of the note. + """ + match = _TASK_RE.match(line) + return (match.group("text") or "") if match else line + + +def _rewrite(body: str, index: int, replace) -> str: + """Rewrite the `index`-th task line with `replace`, or drop it when `replace` + returns None. + + A body with fewer task lines than that is returned UNCHANGED rather than raising: + the index comes from a client that may be a moment behind the server, and a stale + request should do nothing rather than 500. + """ + lines = body.split("\n") + target = None + seen = 0 + for n, line in enumerate(lines): + if _TASK_RE.match(line): + if seen == index: + target = n + break + seen += 1 + if target is None: + return body + + match = _TASK_RE.match(lines[target]) + replacement = replace(match) + if replacement is None: + del lines[target] + else: + lines[target] = replacement + return "\n".join(lines) + + +def set_item_checked(body: str, index: int, checked: bool) -> str: + """Tick or untick the `index`-th item, keeping its text, indent and bullet.""" + return _rewrite( + body, + index, + lambda m: render_item(m.group("text") or "", checked, m.group("indent"), m.group("bullet")), + ) + + +def set_item_text(body: str, index: int, text: str) -> str: + """Replace the text of the `index`-th item, keeping its state and its bullet.""" + return _rewrite( + body, + index, + lambda m: render_item(text.strip(), m.group("mark") in "xX", m.group("indent"), m.group("bullet")), + ) + + +def remove_item(body: str, index: int) -> str: + """Delete the `index`-th item, line and all.""" + return _rewrite(body, index, lambda _m: None) + + +def append_item(body: str, text: str, checked: bool = False) -> str: + """Add an item at the end of the body. + + A blank line between prose and the list, nothing between consecutive items — + the layout `import_export._note_markdown` has always used when writing a checklist + out. That is not cosmetic: it is what the Alembic migration folds existing + `note_items` rows into AND what `derive::append_item` produces on every client, so + all three land on identical bodies. An export taken before the migration and one + taken after therefore differ in nothing. + """ + line = render_item(text.strip(), checked) + trimmed = body.rstrip("\n") + if not trimmed.strip(): + return line + follows_a_list = bool(_TASK_RE.match(trimmed.split("\n")[-1])) + return f"{trimmed}\n{line}" if follows_a_list else f"{trimmed}\n\n{line}" diff --git a/src/thoughtsync/notes/helpers.py b/src/thoughtsync/notes/helpers.py index 7ba8c5f..c197a19 100644 --- a/src/thoughtsync/notes/helpers.py +++ b/src/thoughtsync/notes/helpers.py @@ -3,6 +3,8 @@ board-filter narrowing, the owner-scoped fetch, and filename/slug sanitizers use both the attachment routes and the importer.""" from __future__ import annotations +from .checklist import strip_marker + import os import re @@ -19,29 +21,36 @@ VALID_FILTERS = {"active", "archived", "trash"} DISPLAY_TITLE_CAP = 200 -def derive_display_title(body: str | None, first_item: str | None = None) -> str: - """The note's display NAME: the first non-empty line of the body, else the first - checklist item's text (both trimmed and length-capped). +def derive_display_title(body: str | None) -> str: + """The note's display NAME: the first line of the body that says anything. - There is no explicit title to prefer any more (M13 step 3) — a note is a body plus - optional items, and its name is simply the first thing written in it. Persisted as - notes.display_title so search results and export filenames have something to say. + There is no explicit title to prefer any more (M13 step 3) — a note is a body, and + its name is simply the first thing written in it. Persisted as notes.display_title + so search results and export filenames have something to say. - The item fallback is what step 2 bought: a note that is only a checklist would - otherwise have no name at all, which is exactly the hole that made removing the - title unsafe before checklists stopped being their own kind of thing. + The old `first_item` fallback is gone with M304: items ARE body lines now, so a + list-only note is named by its first item without anyone having to arrange it. What + replaced the fallback is stripping the task marker — calling that note "- [ ] milk" + would show someone the storage instead of the note — and skipping an EMPTY item, so + a half-typed list does not leave a note with no name. - Deterministic — a literal first line, never generated. + Mirrors `display_title` in core/src/local/store.rs. Deterministic — a literal first + line, never generated. """ for line in (body or "").splitlines(): - stripped = line.strip() + stripped = strip_marker(line.strip()).strip() if stripped: return stripped[:DISPLAY_TITLE_CAP] - return (first_item or "").strip()[:DISPLAY_TITLE_CAP] + return "" def is_empty_note(body: str | None, items: list | None = None) -> bool: - """Nothing worth keeping: no body text and no checklist items.""" + """Nothing worth keeping: no body text and no checklist items. + + `items` is still a separate argument because create still accepts them separately — + the importer holds a list, not a blob — and they are folded into the body only + after this check has decided the note is worth making at all. + """ return not (body or "").strip() and not items diff --git a/src/thoughtsync/notes/import_export.py b/src/thoughtsync/notes/import_export.py index c3b659f..93cbe73 100644 --- a/src/thoughtsync/notes/import_export.py +++ b/src/thoughtsync/notes/import_export.py @@ -20,7 +20,6 @@ from ..config import Config from ..models.label import NoteLabel from ..models.note import Note from ..models.note_attachment import NoteAttachment -from ..models.note_item import NoteItem from .helpers import ( ALLOWED_IMAGE_MIMES, _attachment_ext, @@ -28,11 +27,12 @@ from .helpers import ( derive_display_title, is_empty_note, ) +from .checklist import append_item from .tags import _find_or_create_label, _reconcile_tags from .recurrence import normalize_recurrence -def _note_markdown(note: Note, labels: list, items: list) -> str: +def _note_markdown(note: Note, labels: list) -> str: """One note as a human-readable Markdown file with a small frontmatter block. The authoritative machine format is notes.json; this is for reading/portability.""" fm = ["---"] @@ -54,11 +54,9 @@ def _note_markdown(note: Note, labels: list, items: list) -> str: # are written, body first, with a blank line between them when there is. if note.body: fm.append(note.body) - if items: - if note.body: - fm.append("") - for it in items: - fm.append(f"- [{'x' if it['checked'] else ' '}] {it['text']}") + # No separate items block any more. The body already ends with those exact lines + # (M304) — this function is where their layout was decided, and appending them a + # second time would double every checklist in an export. return "\n".join(fm) + "\n" @@ -294,9 +292,17 @@ async def _create_imported_note( if is_empty_note(body, item_texts): return False + # Items fold into the body, which is where a checklist lives now (M304). Done + # before the Note is built so display_title and _reconcile_tags both see the + # finished text — an imported item can carry a #tag like any other line. + for it in items: + text = (it.get("text") or "").strip() + if text: + body = append_item(body, text, bool(it.get("checked"))) + note = Note( owner_id=owner_id, - display_title=derive_display_title(body, item_texts[0] if item_texts else None), + display_title=derive_display_title(body), body=body, color=normalize_color(spec.get("color")), pinned=bool(spec.get("pinned")), @@ -316,12 +322,7 @@ async def _create_imported_note( if spec.get("updated_at"): note.updated_at = spec["updated_at"] db.add(note) - await db.flush() # assign note.id before items/labels/attachments/links - - for pos, it in enumerate(items): - text = (it.get("text") or "").strip() - if text: - db.add(NoteItem(note_id=note.id, text=text, checked=bool(it.get("checked")), position=pos)) + await db.flush() # assign note.id before labels/attachments/links # Explicit (picker-style) labels are manual — via_tag=False. Inline #tags in the # body are handled by _reconcile_tags below, same as a normal create. diff --git a/src/thoughtsync/notes/serialize.py b/src/thoughtsync/notes/serialize.py index cceff35..67587ed 100644 --- a/src/thoughtsync/notes/serialize.py +++ b/src/thoughtsync/notes/serialize.py @@ -8,8 +8,8 @@ from sqlalchemy import select from ..models.label import Label, NoteLabel from ..models.note import Note from ..models.note_attachment import NoteAttachment -from ..models.note_item import NoteItem from ..models.note_link_preview import NoteLinkPreview +from .checklist import parse_items async def _labels_for_notes(db, note_ids: list) -> dict: @@ -30,23 +30,23 @@ async def _labels_for_notes(db, note_ids: list) -> dict: return result -def _serialize_item(item: NoteItem) -> dict: - return {"id": str(item.id), "text": item.text, "checked": item.checked, "position": item.position} +def items_of(body: str | None) -> list[dict]: + """The note's checklist, read out of its body. No query, because there is no table. + Still emitted in the payload after M304, and that is not a second source of truth: + it is DERIVED on the way out, so it cannot disagree with the body it came from. It + saves every consumer that only wants to draw checkboxes from carrying a parser, and + the ones that do carry one (the native clients, the browser) are free to ignore it + and read the body. -async def _items_for_notes(db, note_ids: list) -> dict: - """Map note_id -> [checklist items] in one query, ordered by position.""" - result: dict = {} - if not note_ids: - return result - items = ( - await db.scalars( - select(NoteItem).where(NoteItem.note_id.in_(note_ids)).order_by(NoteItem.position, NoteItem.created_at) - ) - ).all() - for item in items: - result.setdefault(item.note_id, []).append(_serialize_item(item)) - return result + The id is the item's ORDINAL, which is what the rewriters in `checklist.py` take, + so a client holding one can act on it directly. It also shifts when an item is + removed — every mutation returns the reloaded note for exactly that reason. + """ + return [ + {"id": str(i), "text": item.text, "checked": item.checked, "position": i} + for i, item in enumerate(parse_items(body)) + ] def _attachment_url(note_id, att_id) -> str: @@ -108,8 +108,7 @@ async def _serialize_note(db, note: Note) -> dict: data = note.serialize() labels = await _labels_for_notes(db, [note.id]) data["labels"] = labels.get(note.id, []) - items = await _items_for_notes(db, [note.id]) - data["items"] = items.get(note.id, []) + data["items"] = items_of(note.body) attachments = await _attachments_for_notes(db, [note.id]) data["attachments"] = attachments.get(note.id, []) previews = await _previews_for_notes(db, [note.id]) @@ -120,14 +119,14 @@ async def _serialize_note(db, note: Note) -> dict: async def _serialize_notes(db, notes: list) -> list: ids = [n.id for n in notes] labels_map = await _labels_for_notes(db, ids) - items_map = await _items_for_notes(db, ids) + attach_map = await _attachments_for_notes(db, ids) preview_map = await _previews_for_notes(db, ids) out = [] for n in notes: data = n.serialize() data["labels"] = labels_map.get(n.id, []) - data["items"] = items_map.get(n.id, []) + data["items"] = items_of(n.body) data["attachments"] = attach_map.get(n.id, []) data["previews"] = preview_map.get(n.id, []) out.append(data) diff --git a/src/thoughtsync/retention.py b/src/thoughtsync/retention.py index 6c6dd88..ea4166a 100644 --- a/src/thoughtsync/retention.py +++ b/src/thoughtsync/retention.py @@ -32,7 +32,6 @@ from .db import session_scope from .models.label import NoteLabel from .models.note import Note from .models.note_attachment import NoteAttachment -from .models.note_item import NoteItem from .models.note_link_preview import NoteLinkPreview from .models.note_revision import NoteRevision from .settings import get_setting @@ -85,7 +84,6 @@ async def purge_note(db, note: Note, edited_at: datetime | None = None) -> None: # place would make the note reappear whole on the next sweep. logger.warning("couldn't remove attachment file %s during purge", a.path, exc_info=True) await db.execute(sa_delete(NoteAttachment).where(NoteAttachment.note_id == note.id)) - await db.execute(sa_delete(NoteItem).where(NoteItem.note_id == note.id)) await db.execute(sa_delete(NoteLabel).where(NoteLabel.note_id == note.id)) await db.execute(sa_delete(NoteLinkPreview).where(NoteLinkPreview.note_id == note.id)) await db.execute(sa_delete(NoteRevision).where(NoteRevision.note_id == note.id)) diff --git a/src/thoughtsync/sync.py b/src/thoughtsync/sync.py index ad46c6d..f5e141a 100644 --- a/src/thoughtsync/sync.py +++ b/src/thoughtsync/sync.py @@ -24,7 +24,6 @@ from .db import session_scope from .labeling import reconcile_manual_labels, resolve_owned_label_ids from .models.label import Label, NoteLabel from .models.note import Note -from .models.note_item import NoteItem from .models.note_revision import NoteRevision from .revisions import should_snapshot from .notes import ( @@ -63,8 +62,8 @@ MAX_PUSH = 1000 # per-batch change cap # # One bump for the pair: they landed in the same protocol generation, and nothing ever # ran against a half-applied v2. -SYNC_PROTOCOL_VERSION = 2 -MIN_CLIENT_PROTOCOL_VERSION = 2 +SYNC_PROTOCOL_VERSION = 3 +MIN_CLIENT_PROTOCOL_VERSION = 3 # Named capabilities beyond the base protocol. An ADDITIVE change earns a name # here rather than a min-version bump, so a newer client meeting an older server @@ -213,49 +212,12 @@ def _assign_note_fields(note: Note, ch: dict) -> None: note.position = ch["position"] -def _first_item_text(ch: dict) -> str: - """The first non-blank checklist item in a pushed change, or "". - - Read straight from the payload rather than the database because the note's name is - computed BEFORE `_apply_note_items` has written anything — and a note whose body is - empty is named by its first item (M13 step 3). - """ - items = ch.get("items") - if not isinstance(items, list): - return "" - for it in items: - if isinstance(it, dict): - text = (it.get("text") or "").strip() - if text: - return text - return "" - - -async def _apply_note_items(db, note: Note, ch: dict) -> None: - """Replace the note's checklist items with the client's (items sync inline). - - Applies to ANY note. This used to delete every item when the note wasn't - `kind == "list"`, which was survivable only because nothing could produce a note - holding both a body and items. M13 makes that the normal shape — a checklist is - something a note HAS, not something a note IS — and against that shape the old - guard was a data-loss path: the first sync after adding a checklist to a note - would have wiped it. - - Removed ahead of the UI that can create the state, deliberately, so there is no - window in which the two disagree. - """ - items = ch.get("items") - if not isinstance(items, list): - # Absent means "not telling us", not "empty". A client that omits the key - # leaves what the server has; only an explicit [] clears it. - return - await db.execute(sa_delete(NoteItem).where(NoteItem.note_id == note.id)) - for pos, it in enumerate(items): - if not isinstance(it, dict): - continue - text = (it.get("text") or "").strip() - if text: - db.add(NoteItem(note_id=note.id, text=text, checked=bool(it.get("checked")), position=pos)) +# `_first_item_text` and `_apply_note_items` lived here until M304. Both existed for +# one reason — a checklist was a table beside the body — and both are gone with it. A +# pushed change carries its items as `- [ ] ` lines inside `body`, so applying them is +# applying the body, and naming the note is reading its first line. A client that still +# sends an `items` array is a v2 client, and the version floor below turns it away +# before any of this runs. async def _apply_note_manual_labels(db, note: Note, ch: dict) -> None: @@ -315,7 +277,7 @@ async def _apply_note(db, ch: dict) -> dict: old_body = note.body _assign_note_fields(note, ch) - note.display_title = derive_display_title(note.body, _first_item_text(ch)) + note.display_title = derive_display_title(note.body) if edited_at is not None: note.updated_at = edited_at # Non-destructive LWW: snapshot the overwritten server body into history — @@ -327,7 +289,6 @@ async def _apply_note(db, ch: dict) -> dict: if not creating and await should_snapshot(db, note.id, old_body, note.body): db.add(NoteRevision(note_id=note.id, body=old_body)) await db.flush() # assign note.id before items/labels/links - await _apply_note_items(db, note, ch) await _reconcile_tags(db, note) await _apply_note_manual_labels(db, note, ch) await db.flush() diff --git a/tests/test_integration.py b/tests/test_integration.py index f94d0bf..3b2ff66 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -26,21 +26,20 @@ from thoughtsync import ratelimit from thoughtsync.app import create_app from thoughtsync.db import dispose_engine, session_scope from thoughtsync.models.note import Note -from thoughtsync.models.note_item import NoteItem from thoughtsync.models.user import User from thoughtsync.settings import get_setting, live, refresh_live, reset_live, set_settings +from thoughtsync.notes.checklist import parse_items, set_item_checked from thoughtsync.notes.helpers import derive_display_title from thoughtsync.models.note_link_preview import NoteLinkPreview from thoughtsync.models.note_revision import NoteRevision from thoughtsync.revisions import REVISION_WINDOW_MINUTES, should_snapshot -from thoughtsync.sync import _apply_note_items from thoughtsync.unfurl_queue import _unfurl_new_urls, detect_urls pytestmark = pytest.mark.integration # Every table the tests touch, child-first so FKs never block the truncate. # RESTART IDENTITY + CASCADE keeps this honest if a table gains children later. -_TABLES = "notes, note_items, note_revisions, note_labels, note_link_previews, labels, users" +_TABLES = "notes, note_revisions, note_labels, note_link_previews, labels, users" @pytest_asyncio.fixture @@ -160,65 +159,55 @@ async def test_the_search_vector_was_rebuilt_over_the_name(db, owner): assert body_only == 1 -async def test_a_note_keeps_both_its_body_and_its_items(db, owner): - """The shape M13 step 2 made normal: a note HAS a checklist, it isn't one.""" - note = Note(owner_id=owner.id, body="weekend shop", display_title="weekend shop") - db.add(note) - await db.flush() - db.add_all( - [ - NoteItem(note_id=note.id, text="milk", position=0), - NoteItem(note_id=note.id, text="eggs", position=1), - ] - ) - await db.commit() +async def test_a_note_keeps_its_prose_on_both_sides_of_its_list(db, owner): + """The shape M304 made expressible at all. - items = ( - await db.scalars(select(NoteItem).where(NoteItem.note_id == note.id).order_by(NoteItem.position)) - ).all() - assert [i.text for i in items] == ["milk", "eggs"] - assert (await db.scalar(select(Note.body).where(Note.id == note.id))) == "weekend shop" - - -async def test_sync_no_longer_deletes_items_from_a_note_with_a_body(db, owner): - """The data-loss path step 2 removed, pinned against a real database. - - `_apply_note_items` used to delete every item when the note wasn't `kind = "list"`. - Nothing can produce that state any more, but this is the regression that would - have silently eaten a checklist, and it deserves a test that would catch its - return. + The old model could not hold this: a row had a position in a table and none in the + text, so a checklist could only ever render AFTER the body. Prose, list, prose is + the case that proves the storage changed, not just the styling. """ - note = Note(owner_id=owner.id, body="packing", display_title="packing") + body = "weekend shop\n\n- [ ] milk\n- [x] eggs\n\nback before six" + note = Note(owner_id=owner.id, body=body, display_title=derive_display_title(body)) db.add(note) - await db.flush() - db.add(NoteItem(note_id=note.id, text="socks", position=0)) await db.commit() - # A change that says nothing about items must LEAVE them alone — absent means - # "not telling us", not "empty". - await _apply_note_items(db, note, {"body": "packing"}) - await db.commit() - assert (await db.scalar(select(NoteItem.text).where(NoteItem.note_id == note.id))) == "socks" - - # An explicit list replaces them. - await _apply_note_items(db, note, {"items": [{"text": "charger", "checked": True}]}) - await db.commit() - rows = (await db.scalars(select(NoteItem).where(NoteItem.note_id == note.id))).all() - assert [(r.text, r.checked) for r in rows] == [("charger", True)] + stored = await db.scalar(select(Note.body).where(Note.id == note.id)) + assert [(i.text, i.checked) for i in parse_items(stored)] == [("milk", False), ("eggs", True)] + assert stored.splitlines()[0] == "weekend shop" + assert stored.splitlines()[-1] == "back before six" -async def test_a_note_with_only_items_still_has_a_name(db, owner): - """The hole that made removing the title unsafe until step 2 closed it.""" - note = Note(owner_id=owner.id, body="", display_title="") +async def test_ticking_an_item_is_a_body_edit(db, owner): + """What replaced `_apply_note_items`: there is no separate thing left to apply. + + The regression that function guarded against — a sync silently eating a checklist + off a note that also had a body — cannot recur, because there is nothing to delete. + A pushed body either has the lines or it does not. + """ + body = "packing\n\n- [ ] socks" + note = Note(owner_id=owner.id, body=body, display_title="packing") db.add(note) - await db.flush() - db.add(NoteItem(note_id=note.id, text="milk", position=0)) await db.commit() - first = await db.scalar( - select(NoteItem.text).where(NoteItem.note_id == note.id).order_by(NoteItem.position).limit(1) - ) - note.display_title = derive_display_title(note.body, first) + note.body = set_item_checked(note.body, 0, True) + await db.commit() + + stored = await db.scalar(select(Note.body).where(Note.id == note.id)) + assert stored == "packing\n\n- [x] socks" + assert parse_items(stored)[0].checked + # The prose is untouched — a tick rewrites one line, not the note. + assert stored.splitlines()[0] == "packing" + + +async def test_a_note_with_only_a_list_still_has_a_name(db, owner): + """The hole that made removing the title unsafe, still closed — by a different + mechanism. There is no item table to fall back to any more; the name comes from + the first line with its marker stripped, because calling the note "- [ ] milk" + would show someone the storage instead of the note. + """ + body = "- [ ] milk\n- [ ] eggs" + note = Note(owner_id=owner.id, body=body, display_title=derive_display_title(body)) + db.add(note) await db.commit() assert (await db.scalar(select(Note.display_title).where(Note.id == note.id))) == "milk" diff --git a/tests/test_notes.py b/tests/test_notes.py index 30b9181..abb3d4a 100644 --- a/tests/test_notes.py +++ b/tests/test_notes.py @@ -5,6 +5,14 @@ import pytest from thoughtsync.app import create_app from thoughtsync.common import coerce_bool, parse_dt from thoughtsync.models.note import NOTE_COLORS, Note +from thoughtsync.notes.checklist import ( + append_item, + parse_items, + remove_item, + set_item_checked, + set_item_text, + strip_marker, +) from thoughtsync.unfurl_queue import detect_urls from thoughtsync.notes import ( _attachment_ext, @@ -42,7 +50,7 @@ def test_all_note_routes_registered(app): "reorder_notes", "create_note", "get_note", "update_note", "list_revisions", "restore_revision", "set_note_labels", "add_item", "update_item", "delete_item", - "reorder_items", "upload_attachment", "get_attachment", + "upload_attachment", "get_attachment", "delete_attachment", "unfurl_link", "delete_preview", "trash_note", "restore_note", "delete_note", ) @@ -406,3 +414,120 @@ def test_detect_urls_ignores_non_http(): assert detect_urls("ftp://example.com and mailto:a@b.c and bare example.com") == [] assert detect_urls(None) == [] assert detect_urls("") == [] + + +# --- checklist items: the body IS the checklist (M304) ----------------------- +# +# The same table of cases as core/src/local/derive.rs. Deliberately duplicated +# rather than shared: the point of three implementations is that each is checked +# against the same grammar, and a test that only ran once would not catch the two +# drifting apart. + + +def test_parse_items_reads_a_list_out_of_prose(): + body = "shopping\n\n- [ ] milk\n- [x] eggs" + assert [(i.text, i.checked) for i in parse_items(body)] == [("milk", False), ("eggs", True)] + + +def test_parse_items_between_paragraphs(): + # The case a side table could not express, which is the whole reason for M304. + assert [i.text for i in parse_items("before\n- [ ] middle\nafter")] == ["middle"] + + +@pytest.mark.parametrize( + "body", + [ + "-[ ] no space after the dash", + "- [] empty brackets", + "- [ ]no space after the brackets", + "- [y] not a mark", + "a [ ] mid sentence", + "[ ] no bullet at all", + ], +) +def test_parse_items_rejects_near_misses(body): + assert parse_items(body) == [] + + +def test_parse_items_accepts_star_bullets_and_indentation(): + # `*` because markdown.ts already takes it for a plain bullet. + body = "* [ ] star\n - [x] indented" + assert [(i.text, i.checked) for i in parse_items(body)] == [("star", False), ("indented", True)] + + +def test_an_empty_item_is_still_an_item(): + # What pressing Enter on a list leaves behind. + assert [i.text for i in parse_items("- [ ]")] == [""] + assert [i.text for i in parse_items("- [ ] ")] == [""] + + +def test_uppercase_x_parses_and_normalises_on_rewrite(): + assert parse_items("- [X] done")[0].checked + assert set_item_checked("- [X] done", 0, True) == "- [x] done" + + +def test_rewriters_preserve_indent_bullet_and_neighbours(): + assert set_item_checked(" * [ ] milk", 0, True) == " * [x] milk" + assert set_item_text("- [x] old", 0, "new") == "- [x] new" + assert remove_item("keep\n- [ ] drop\n- [ ] stay", 0) == "keep\n- [ ] stay" + # Addressed by ITEM, not by line. + assert set_item_checked("note\n- [ ] a\nprose\n- [ ] b", 1, True) == "note\n- [ ] a\nprose\n- [x] b" + + +def test_a_stale_index_does_nothing(): + # The index comes from a client that may be a moment behind. A late request + # should be inert, not a 500. + body = "- [ ] only" + assert set_item_checked(body, 7, True) == body + assert remove_item(body, 7) == body + assert set_item_text(body, 7, "x") == body + + +def test_a_plain_body_is_returned_unchanged(): + body = "just prose\nwith two lines" + assert set_item_checked(body, 0, True) == body + assert remove_item(body, 0) == body + + +def test_append_item_spacing(): + # Prose, blank line, list — the layout _note_markdown has always exported, and + # what the migration folds existing rows into. + assert append_item("a note", "milk") == "a note\n\n- [ ] milk" + # Nothing between consecutive items. + assert append_item("a note\n\n- [ ] milk", "eggs") == "a note\n\n- [ ] milk\n- [ ] eggs" + # A list-only note starts at the first line. + assert append_item("", "milk") == "- [ ] milk" + assert append_item("\n\n", "milk") == "- [ ] milk" + # Carries state, which is what the migrations need of it. + assert append_item("", "done", True) == "- [x] done" + + +def test_strip_marker_and_display_title(): + assert strip_marker("- [x] milk") == "milk" + assert strip_marker("just prose") == "just prose" + # A list-only note is named by its first item, without the marker. + assert derive_display_title("- [ ] milk\n- [ ] eggs") == "milk" + # An EMPTY item does not name the note "" — a half-typed list still has a name. + assert derive_display_title("- [ ]\n- [ ] eggs") == "eggs" + + +def test_the_migration_folds_exactly_like_the_app(): + """0027 inlines its own copy of append_item, deliberately — a migration has to keep + producing what it produced the day it ran, so it must not follow the app if the + app's spacing ever changes. This is what keeps the copy honest until then.""" + import importlib.util + from pathlib import Path + + path = Path(__file__).resolve().parents[1] / "alembic" / "versions" / "0027_checklist_items_into_body.py" + spec = importlib.util.spec_from_file_location("_m0027", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + + for body, text, checked in [ + ("a note", "milk", False), + ("a note\n\n- [ ] milk", "eggs", True), + ("", "milk", False), + ("\n\n", "milk", False), + ("prose\n", " padded ", True), + ]: + assert module._append_item(body, text, checked) == append_item(body, text, checked)