tags: renaming onto an existing tag merges them, and the older row survives
Android / Build, or is the channel already serving this? (push) Successful in 4s
CI & Build / Build now, or wait for Android? (push) Successful in 3s
CI & Build / Python lint (push) Successful in 4s
Desktop (Tauri) / Build, or is the channel already serving this? (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 7s
CI & Build / Python tests (push) Successful in 12s
CI & Build / integration (push) Successful in 21s
CI & Build / Build & push image (push) Skipped
Desktop (Tauri) / Tauri desktop (Linux) (push) Failing after 2m35s
Desktop (Tauri) / Windows installer (cross-compiled) (push) Successful in 2m46s
Desktop (Tauri) / Update manifest (push) Skipped
Android / Kotlin + Rust (APK) (push) Canceled after 5m37s
Android / Build, or is the channel already serving this? (push) Successful in 4s
CI & Build / Build now, or wait for Android? (push) Successful in 3s
CI & Build / Python lint (push) Successful in 4s
Desktop (Tauri) / Build, or is the channel already serving this? (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 7s
CI & Build / Python tests (push) Successful in 12s
CI & Build / integration (push) Successful in 21s
CI & Build / Build & push image (push) Skipped
Desktop (Tauri) / Tauri desktop (Linux) (push) Failing after 2m35s
Desktop (Tauri) / Windows installer (cross-compiled) (push) Successful in 2m46s
Desktop (Tauri) / Update manifest (push) Skipped
Android / Kotlin + Rust (APK) (push) Canceled after 5m37s
The three surfaces did not agree on what renaming a tag onto a name another
one already holds should do, and none of the three answers was good.
I described this wrongly first time and the correction matters. The local
store does NOT silently create a duplicate: `idx_labels_name` is unique on
`lower(name)`, so the bare UPDATE in `rename_label` failed, and the user got
a raw SQLite "UNIQUE constraint failed" as their error message. The server
meanwhile answered 409 "a tag with that name already exists" — and only on
an EXACT match, because its constraint is on the raw name while every
client's index is on `lower(name)`.
That last part is the sharper bug. The server would happily hold "Groceries"
beside "groceries"; no synced client can store both. Creating that pair on
the web armed a pull that fails later, on a phone, in a path with no UI.
Operator's call: a rename onto an existing name means merge — typing an
existing tag's name onto this one says they are the same thing.
* `store::rename_label` and the server's PATCH now implement one rule.
THE OLDER ROW SURVIVES and takes the new spelling. Age rather than "the
one that already held the name", so that renaming A→B and B→A land on
the same survivor; otherwise the outcome depends on which way round
someone typed it, and two devices tidying the same pair disagree about
which id still exists. Ties go to the incumbent, so it stays
deterministic.
* The core reuses `merge_labels` rather than reimplementing the move. That
is the only place that knows to mark every affected NOTE dirty before
the delete cascades the membership rows away, which is what makes a
merge reach the server at all.
* The server's rename and its `/merge` route now share one `_merge_into`
helper, for the same reason.
* Both server lookups became case-INSENSITIVE, matching every client. The
create path is included: it was the one actually minting the unstorable
pair, so fixing only the rename would have left the door open.
* The web asks before merging, naming both note counts. A merge cannot be
undone by repeating it and is now reachable by a typo in a text field —
the same reasoning as the delete confirmation in #2116. The confirmation
lives in the shared store, so the desktop gets it too; the FFI does not
ask, because that belongs to the surface with a person in front of it.
* The web store detects the merge from the LIST, not the response: the
survivor may be the row we asked to rename, so an unchanged id proves
nothing.
Tests: three integration tests over a real database (both rename directions
land on the older row; a case-varied create returns the existing tag) and
two through the Android FFI, which is the binding the phone will use.
Also fixes a straggler from 8c7553d — the delete confirmation still said
"the label".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K3MMqUtzX1TJgA1oypvm1c
This commit is contained in:
@@ -543,3 +543,90 @@ async def test_a_revision_inside_the_window_blocks_another(db, owner):
|
||||
await db.commit()
|
||||
|
||||
assert await should_snapshot(db, note.id, note.body, "draft revised") is False
|
||||
|
||||
|
||||
async def test_renaming_a_tag_onto_an_existing_one_merges_into_the_older(app_client, db):
|
||||
"""Renaming a tag onto a name another tag holds merges them, and the OLDER row
|
||||
is the survivor — whichever side the caller happened to be renaming.
|
||||
|
||||
Runs against a real database because the whole question is about `created_at`
|
||||
ordering and the note_labels rows moving, neither of which a unit test sees.
|
||||
|
||||
Age decides, rather than "the one that already held the name", so that renaming
|
||||
A→B and renaming B→A land on the same row. If the incumbent won, the survivor
|
||||
would depend on which way round someone typed it, and two clients racing the
|
||||
same tidy-up would disagree about which id still exists.
|
||||
"""
|
||||
reg = await app_client.post(
|
||||
"/api/auth/register",
|
||||
json={"email": "tags@example.test", "password": "a-long-enough-password"},
|
||||
)
|
||||
assert reg.status_code == 201
|
||||
|
||||
# Two separate requests, so two transactions and two distinct `func.now()`s.
|
||||
older = await (await app_client.post("/api/labels", json={"name": "grocery"})).get_json()
|
||||
newer = await (await app_client.post("/api/labels", json={"name": "errands"})).get_json()
|
||||
|
||||
one = await (await app_client.post("/api/notes", json={"body": "milk"})).get_json()
|
||||
two = await (await app_client.post("/api/notes", json={"body": "stamps"})).get_json()
|
||||
await app_client.put(f"/api/notes/{one['id']}/labels", json={"label_ids": [older["id"]]})
|
||||
await app_client.put(f"/api/notes/{two['id']}/labels", json={"label_ids": [newer["id"]]})
|
||||
|
||||
# Rename the YOUNGER onto the older's name, with different casing — matching is
|
||||
# case-insensitive, and the survivor must end up spelled the way we asked.
|
||||
resp = await app_client.patch(f"/api/labels/{newer['id']}", json={"name": "Grocery"})
|
||||
assert resp.status_code == 200
|
||||
survivor = await resp.get_json()
|
||||
|
||||
assert survivor["id"] == older["id"], "the older row is the one that keeps existing"
|
||||
assert survivor["name"] == "Grocery", "the survivor takes the spelling that was asked for"
|
||||
|
||||
listing = (await (await app_client.get("/api/labels")).get_json())["labels"]
|
||||
assert len(listing) == 1, "the two became one"
|
||||
assert listing[0]["id"] == older["id"]
|
||||
assert listing[0]["count"] == 2, "it carries every note from both sides"
|
||||
|
||||
|
||||
async def test_the_rename_merge_survivor_does_not_depend_on_the_direction(app_client, db):
|
||||
"""The mirror of the test above: rename the OLDER onto the younger's name. The
|
||||
older still survives — it just changes its name — so the two directions agree."""
|
||||
reg = await app_client.post(
|
||||
"/api/auth/register",
|
||||
json={"email": "tags2@example.test", "password": "a-long-enough-password"},
|
||||
)
|
||||
assert reg.status_code == 201
|
||||
|
||||
older = await (await app_client.post("/api/labels", json={"name": "grocery"})).get_json()
|
||||
newer = await (await app_client.post("/api/labels", json={"name": "errands"})).get_json()
|
||||
|
||||
resp = await app_client.patch(f"/api/labels/{older['id']}", json={"name": "errands"})
|
||||
assert resp.status_code == 200
|
||||
survivor = await resp.get_json()
|
||||
|
||||
assert survivor["id"] == older["id"], "age wins in this direction too"
|
||||
assert survivor["name"] == "errands"
|
||||
assert newer["id"] != older["id"]
|
||||
|
||||
listing = (await (await app_client.get("/api/labels")).get_json())["labels"]
|
||||
assert [lb["id"] for lb in listing] == [older["id"]]
|
||||
|
||||
|
||||
async def test_creating_a_tag_that_differs_only_in_case_returns_the_existing_one(app_client, db):
|
||||
"""A case-sensitive match here used to mint "Groceries" beside "groceries". No
|
||||
synced client can hold both — their `labels` index is unique on `lower(name)` —
|
||||
so the pair was a pull that would fail later, on a phone, with no UI in the path.
|
||||
"""
|
||||
reg = await app_client.post(
|
||||
"/api/auth/register",
|
||||
json={"email": "tags3@example.test", "password": "a-long-enough-password"},
|
||||
)
|
||||
assert reg.status_code == 201
|
||||
|
||||
first = await app_client.post("/api/labels", json={"name": "groceries"})
|
||||
assert first.status_code == 201
|
||||
second = await app_client.post("/api/labels", json={"name": "Groceries"})
|
||||
assert second.status_code == 200, "an existing tag is returned, not a second one made"
|
||||
|
||||
assert (await second.get_json())["id"] == (await first.get_json())["id"]
|
||||
listing = (await (await app_client.get("/api/labels")).get_json())["labels"]
|
||||
assert len(listing) == 1
|
||||
|
||||
Reference in New Issue
Block a user