server: the body is the checklist here too, and note_items is dropped
CI & Build / Build now, or wait for Android? (push) Successful in 2s
CI & Build / Python lint (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 6s
CI & Build / Python tests (push) Failing after 8s
CI & Build / integration (push) Successful in 20s
CI & Build / Build & push image (push) Skipped
CI & Build / Build now, or wait for Android? (push) Successful in 2s
CI & Build / Python lint (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 6s
CI & Build / Python tests (push) Failing after 8s
CI & Build / integration (push) Successful in 20s
CI & Build / Build & push image (push) Skipped
M304 steps 3 and the server half of 4. The client half landed in 668f7fa; these belong in one deploy, and the protocol floor below is what enforces that. notes/checklist.py is the Python half of a grammar that now exists three times — here, core/src/local/derive.rs, and (next) frontend/src/notes/markdown.ts. That triplication is the deliberate cost: the alternative is a round trip to the server before a phone can draw a checkbox. Each copy names the other two, and each is tested against the same table of cases, including the near-misses that must stay prose: `-[ ] x`, `- []`, `- [ ]x`, a `[ ]` mid-sentence. Routes: add/update/delete items stop touching rows and rewrite note.body, all through one _rewrite_body that runs the same sequence the PATCH route runs for a body change — because it IS a body change. Revisions, #tag reconciliation, the name, and link unfurls therefore happen in one place rather than three routes each remembering to. The reorder route is gone (rule 22). Reordering a checklist is moving a line, and no client ever called it — the only reference in the tree was a test asserting the route existed. The API still returns `items`, DERIVED from the body on the way out. That is not a second source of truth and it cannot disagree with the body it came from; it keeps the web client working across the rest of this milestone and saves any consumer that only wants to draw checkboxes from carrying a parser. Export drops its separate items block, in both formats. The body already ends with those exact lines, so writing them again would double every checklist in an export and then double it again on re-import. Import still ACCEPTS items, because a Keep takeout has a list and not a blob; it folds them in before the Note is built, so display_title and _reconcile_tags both see the finished text. Protocol 3 on both sides now. A v2 client is refused rather than half-served — which matters more than I first said: _apply_note_items returned early on an absent `items` key, so an un-bumped v3 client against a v2 server would not have LOST the rows, it would have kept them and then had the migration fold them a second time. Duplicated lists rather than missing ones. The floor prevents both. Migration 0027 folds every existing row into its note's body and drops the table. It inlines its own copy of the fold on purpose — a migration has to keep producing what it produced the day it ran — and a test pins that copy against the app's until they are allowed to diverge. updated_at is deliberately untouched: a client holding an unpushed edit keeps the newer timestamp, so last-write-wins keeps its work instead of the migration silently winning. The downgrade is honest rather than faithful. It recreates an empty note_items and leaves the bodies alone, because once items are lines nothing distinguishes one this migration wrote from one somebody typed, and a downgrade that guessed would eat hand-written lists. Recreating the table is still necessary: 0015's downgrade drops a trigger ON note_items, and IF EXISTS covers the trigger, not the table.
This commit is contained in:
+41
-52
@@ -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"
|
||||
|
||||
+126
-1
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user