diff --git a/android/ffi/src/lib.rs b/android/ffi/src/lib.rs index 96cff38..6cdffb4 100644 --- a/android/ffi/src/lib.rs +++ b/android/ffi/src/lib.rs @@ -336,17 +336,19 @@ impl ThoughtSync { /// Rename a label. Every note carrying it follows, because notes reference it /// by id and never by name. /// - /// Renaming onto a name that already exists does NOT merge, and does not fail - /// either — the core's find-or-create matching only runs on the create path, so - /// this leaves two labels whose names differ by case at most. + /// Renaming onto a name another tag already holds MERGES the two, and the OLDER + /// row is the survivor — it keeps its id and colour and takes the new spelling. + /// Matching is case-insensitive, like `find_or_create_label`. /// - /// The SERVER disagrees: `labels.py` answers that PATCH with 409 "a label with - /// that name already exists". So the local stores (this and the desktop, which - /// calls the same `store::rename_label`) are more permissive than the REST path - /// the web uses, and a duplicate made offline will meet that 409 on sync. Not - /// introduced here — it predates Android having a rename at all — but a UI over - /// this has to decide what it shows, so it is written down rather than found. - /// Merging is the deliberate, irreversible operation and stays a separate button. + /// So this call can return a label whose id is NOT the one passed in, and it can + /// make another label stop existing. A UI over it should say so before calling: + /// the merge cannot be undone by repeating it, and here it is reachable by a + /// typo in a text field. The web asks first (`stores/labels.ts`); this binding + /// deliberately does not, because a confirmation belongs to the surface that has + /// a person in front of it, not to the store. + /// + /// `store::rename_label` and the server's PATCH implement the same rule, so the + /// phone, the desktop and the web agree on which row survives. pub fn rename_label(&self, id: String, name: String) -> Result { let conn = self.db.conn().map_err(CoreError::store)?; local::store::rename_label(&conn, &id, &name) @@ -963,6 +965,75 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + /// Renaming a tag onto a name another tag already holds MERGES the two, and the + /// OLDER row is the survivor. + /// + /// Before this, the bare UPDATE met `idx_labels_name` — unique on `lower(name)` — + /// and the user got a raw "UNIQUE constraint failed" from SQLite. Merging is what + /// a person means by typing an existing tag's name onto this one. + #[test] + fn renaming_onto_an_existing_tag_merges_into_the_older_one() { + let dir = scratch_dir(); + let app = ThoughtSync::new(dir.clone()).expect("a fresh data dir should open"); + + let older = app.create_label("grocery".to_string()).expect("older"); + // `created_at` is RFC3339 to the MILLISECOND. Without a gap the two rows can + // share a timestamp, and then the tie-break is under test instead of the age + // rule this test is about. + std::thread::sleep(std::time::Duration::from_millis(5)); + let newer = app.create_label("errands".to_string()).expect("newer"); + + let one = app.create_note(draft("milk")).expect("note one"); + let two = app.create_note(draft("stamps")).expect("note two"); + app.set_note_labels(one.id.clone(), vec![older.id.clone()]) + .expect("tag one"); + app.set_note_labels(two.id.clone(), vec![newer.id.clone()]) + .expect("tag two"); + + // The YOUNGER one is renamed onto the older's name, in a different case — + // matching is case-insensitive, and the survivor takes the spelling asked for. + let survivor = app + .rename_label(newer.id.clone(), "Grocery".to_string()) + .expect("a rename onto an existing name merges instead of failing"); + + assert_eq!(survivor.id, older.id, "the older row is the one that survives"); + assert_eq!(survivor.name, "Grocery", "spelled the way the caller asked"); + + let all = app.list_labels().expect("list"); + assert_eq!(all.len(), 1, "the two became one"); + assert_eq!(all[0].id, older.id); + assert_eq!(all[0].count, Some(2), "carrying every note from both sides"); + + std::fs::remove_dir_all(&dir).ok(); + } + + /// The mirror of the test above. Renaming the OLDER one onto the younger's name + /// still leaves the older row standing — it just changes its name. + /// + /// This is the whole reason age decides rather than "whoever already held the + /// name": otherwise the survivor depends on which way round someone typed it, + /// and two devices tidying the same pair would disagree about which id exists. + #[test] + fn the_rename_merge_survivor_does_not_depend_on_the_direction() { + let dir = scratch_dir(); + let app = ThoughtSync::new(dir.clone()).expect("a fresh data dir should open"); + + let older = app.create_label("grocery".to_string()).expect("older"); + std::thread::sleep(std::time::Duration::from_millis(5)); + let newer = app.create_label("errands".to_string()).expect("newer"); + + let survivor = app + .rename_label(older.id.clone(), "errands".to_string()) + .expect("rename"); + + assert_eq!(survivor.id, older.id, "age wins in this direction too"); + assert_eq!(survivor.name, "errands"); + assert_ne!(survivor.id, newer.id, "the younger row is the one that went"); + assert_eq!(app.list_labels().expect("list").len(), 1); + + std::fs::remove_dir_all(&dir).ok(); + } + /// A crude RFC3339 sanity check that doesn't pull a date crate into this /// crate's dev-dependencies to assert one field is well-formed. fn chrono_free_parse(raw: &str) -> usize { diff --git a/core/src/local/store.rs b/core/src/local/store.rs index ccdffe3..dddc51d 100644 --- a/core/src/local/store.rs +++ b/core/src/local/store.rs @@ -820,7 +820,57 @@ pub fn create_label(conn: &Connection, name: &str) -> rusqlite::Result