diff --git a/README.md b/README.md index fd126ff78350f9307c72cbfbbddced6663ac73a8..c63beeb56a12aa18a44954d08901ac762d144930 100644 --- a/README.md +++ b/README.md @@ -31,8 +31,10 @@ Commands: create Create a new task [aliases: add] list List tasks [aliases: ls] show Show task details + log Append a work log entry to a task update Update a task done Mark task(s) as closed [aliases: close] + rm Delete task(s) reopen Reopen task(s) dep Manage dependencies / blockers label Manage labels diff --git a/SKILL.md b/SKILL.md index 0e2ad1f508ddd5c64f866e6e83ea8bb48ae02743..1c56ffbb53a1290423590055fe76b57f6cbb028a 100644 --- a/SKILL.md +++ b/SKILL.md @@ -80,6 +80,13 @@ td update td-a1b2c3 -p high -e low -t "Revised title" -d "Added context" td done td-a1b2c3 td-d4e5f6 # one or many td reopen td-a1b2c3 +# Delete tasks (always non-interactive) +td rm td-a1b2c3 # delete one or many IDs +td rm td-a1b2c3 td-d4e5f6 +td rm --recursive td-parent # required for deleting task trees +td rm --force td-blocker # suppress dependent-unblocked warnings +td --json rm td-a1b2c3 # machine-readable deleted/unblocked IDs + # Blocked by something else td dep add td-child td-blocker # child waits for blocker to close td dep rm td-child td-blocker diff --git a/src/cli.rs b/src/cli.rs index ca6a491193d17c853f6fe5e9898289dde822669e..d76541b86e33b670d55119c7c61edba9259ded86 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -119,6 +119,21 @@ pub enum Command { ids: Vec, }, + /// Delete task(s) + Rm { + /// Skip warnings about dependents becoming unblocked + #[arg(short, long)] + force: bool, + + /// Delete the whole subtree (task and descendants) + #[arg(short = 'r', long)] + recursive: bool, + + /// Task IDs + #[arg(required = true)] + ids: Vec, + }, + /// Reopen task(s) Reopen { /// Task IDs diff --git a/src/cmd/mod.rs b/src/cmd/mod.rs index bfe45d07b66f621d59a83415e2c56d3b882ca392..3a6410e1dcdf4e68a643fe4143d1dd6e76be10a6 100644 --- a/src/cmd/mod.rs +++ b/src/cmd/mod.rs @@ -11,6 +11,7 @@ mod log; mod next; mod ready; mod reopen; +mod rm; mod search; mod show; mod skill; @@ -109,6 +110,14 @@ pub fn dispatch(cli: &Cli) -> Result<()> { let root = require_root()?; done::run(&root, ids, cli.json) } + Command::Rm { + force, + recursive, + ids, + } => { + let root = require_root()?; + rm::run(&root, ids, *recursive, *force, cli.json) + } Command::Reopen { ids } => { let root = require_root()?; reopen::run(&root, ids, cli.json) diff --git a/src/cmd/rm.rs b/src/cmd/rm.rs new file mode 100644 index 0000000000000000000000000000000000000000..b8b1779ed966a85b0e7fcf9f258ef5de994a0fbc --- /dev/null +++ b/src/cmd/rm.rs @@ -0,0 +1,141 @@ +use anyhow::{bail, Result}; +use serde::Serialize; +use std::collections::BTreeSet; +use std::path::Path; + +use crate::db; + +#[derive(Serialize)] +struct RmResult { + requested_ids: Vec, + deleted_ids: Vec, + unblocked_ids: Vec, +} + +pub fn run(root: &Path, ids: &[String], recursive: bool, force: bool, json: bool) -> Result<()> { + let mut conn = db::open(root)?; + let tx = conn.transaction()?; + + let mut to_delete = BTreeSet::new(); + for id in ids { + if !db::task_exists(&tx, id)? { + bail!("task '{id}' not found"); + } + + if recursive { + for subtree_id in load_subtree_ids(&tx, id)? { + to_delete.insert(subtree_id); + } + } else { + let child_count: i64 = tx.query_row( + "SELECT COUNT(*) FROM tasks WHERE parent = ?1", + [id], + |row| row.get(0), + )?; + if child_count > 0 { + bail!("task '{id}' has children; use --recursive to delete subtree"); + } + to_delete.insert(id.clone()); + } + } + + let deleted_ids: Vec = to_delete.into_iter().collect(); + let unblocked_ids = detach_dependents(&tx, &deleted_ids)?; + + if !deleted_ids.is_empty() { + delete_tasks(&tx, &deleted_ids)?; + } + + tx.commit()?; + + if !force && !unblocked_ids.is_empty() { + eprintln!( + "warning: removed blockers from {}", + unblocked_ids.join(", ") + ); + } + + if json { + let out = RmResult { + requested_ids: ids.to_vec(), + deleted_ids, + unblocked_ids, + }; + println!("{}", serde_json::to_string(&out)?); + } else { + let c = crate::color::stdout_theme(); + for id in &deleted_ids { + println!("{}deleted{} {id}", c.green, c.reset); + } + } + + Ok(()) +} + +fn load_subtree_ids(tx: &rusqlite::Transaction, root_id: &str) -> Result> { + let mut stmt = tx.prepare( + "WITH RECURSIVE subtree(id) AS ( + SELECT id FROM tasks WHERE id = ?1 + UNION ALL + SELECT tasks.id + FROM tasks + JOIN subtree ON tasks.parent = subtree.id + ) + SELECT id FROM subtree", + )?; + let ids = stmt + .query_map([root_id], |row| row.get(0))? + .collect::>>()?; + Ok(ids) +} + +fn detach_dependents(tx: &rusqlite::Transaction, deleted_ids: &[String]) -> Result> { + if deleted_ids.is_empty() { + return Ok(Vec::new()); + } + + let in_placeholders = vec!["?"; deleted_ids.len()].join(", "); + let sql = format!( + "SELECT DISTINCT task_id + FROM blockers + WHERE blocker_id IN ({in_placeholders}) + AND task_id NOT IN ({in_placeholders}) + ORDER BY task_id" + ); + let params = deleted_ids.iter().chain(deleted_ids.iter()); + let mut stmt = tx.prepare(&sql)?; + let unblocked_ids = stmt + .query_map(rusqlite::params_from_iter(params), |row| row.get(0))? + .collect::>>()?; + + if unblocked_ids.is_empty() { + return Ok(unblocked_ids); + } + + let delete_sql = format!( + "DELETE FROM blockers + WHERE blocker_id IN ({in_placeholders}) + AND task_id NOT IN ({in_placeholders})" + ); + let delete_params = deleted_ids.iter().chain(deleted_ids.iter()); + tx.execute(&delete_sql, rusqlite::params_from_iter(delete_params))?; + + let update_placeholders = vec!["?"; unblocked_ids.len()].join(", "); + let update_sql = format!( + "UPDATE tasks + SET updated = ?1 + WHERE id IN ({update_placeholders})" + ); + let ts = db::now_utc(); + let update_params = std::iter::once(&ts).chain(unblocked_ids.iter()); + tx.execute(&update_sql, rusqlite::params_from_iter(update_params))?; + + Ok(unblocked_ids) +} + +fn delete_tasks(tx: &rusqlite::Transaction, deleted_ids: &[String]) -> Result<()> { + let in_placeholders = vec!["?"; deleted_ids.len()].join(", "); + let sql = format!("DELETE FROM tasks WHERE id IN ({in_placeholders})"); + tx.execute(&sql, rusqlite::params_from_iter(deleted_ids.iter()))?; + Ok(()) +} diff --git a/src/migrate.rs b/src/migrate.rs index dc1361690593c4fb939db9f789cf09e57cdfedf6..44183413ca7f13f7aea88131e7047200c1518d0d 100644 --- a/src/migrate.rs +++ b/src/migrate.rs @@ -45,6 +45,12 @@ static MIGRATIONS: &[Migration] = &[ post_hook_up: None, post_hook_down: None, }, + Migration { + up_sql: include_str!("migrations/0005_cascade_fks.up.sql"), + down_sql: include_str!("migrations/0005_cascade_fks.down.sql"), + post_hook_up: None, + post_hook_down: None, + }, ]; /// Read the current schema version from the database. diff --git a/src/migrations/0005_cascade_fks.down.sql b/src/migrations/0005_cascade_fks.down.sql new file mode 100644 index 0000000000000000000000000000000000000000..6a3589651faab6029d0b77fa4caaef49de7456eb --- /dev/null +++ b/src/migrations/0005_cascade_fks.down.sql @@ -0,0 +1,30 @@ +-- Revert labels/blockers foreign keys to definitions without ON DELETE CASCADE. + +CREATE TABLE labels_old ( + task_id TEXT, + label TEXT, + PRIMARY KEY (task_id, label), + FOREIGN KEY (task_id) REFERENCES tasks(id) +); + +INSERT INTO labels_old (task_id, label) + SELECT task_id, label FROM labels; + +DROP TABLE labels; + +ALTER TABLE labels_old RENAME TO labels; + +CREATE TABLE blockers_old ( + task_id TEXT, + blocker_id TEXT, + PRIMARY KEY (task_id, blocker_id), + FOREIGN KEY (task_id) REFERENCES tasks(id), + FOREIGN KEY (blocker_id) REFERENCES tasks(id) +); + +INSERT INTO blockers_old (task_id, blocker_id) + SELECT task_id, blocker_id FROM blockers; + +DROP TABLE blockers; + +ALTER TABLE blockers_old RENAME TO blockers; diff --git a/src/migrations/0005_cascade_fks.up.sql b/src/migrations/0005_cascade_fks.up.sql new file mode 100644 index 0000000000000000000000000000000000000000..a94ac947b42b638111dfb2553dca3a398bcd5a40 --- /dev/null +++ b/src/migrations/0005_cascade_fks.up.sql @@ -0,0 +1,34 @@ +-- Add ON DELETE CASCADE to labels/task_id and blockers/task_id+blocker_id. +-- SQLite has no ALTER TABLE ADD CONSTRAINT, so we rebuild both tables. + +-- Drop dangling label rows before introducing stricter FK behavior. +DELETE FROM labels WHERE task_id NOT IN (SELECT id FROM tasks); + +CREATE TABLE labels_new ( + task_id TEXT, + label TEXT, + PRIMARY KEY (task_id, label), + FOREIGN KEY (task_id) REFERENCES tasks(id) ON DELETE CASCADE +); + +INSERT INTO labels_new (task_id, label) + SELECT task_id, label FROM labels; + +DROP TABLE labels; + +ALTER TABLE labels_new RENAME TO labels; + +CREATE TABLE blockers_new ( + task_id TEXT, + blocker_id TEXT, + PRIMARY KEY (task_id, blocker_id), + FOREIGN KEY (task_id) REFERENCES tasks(id) ON DELETE CASCADE, + FOREIGN KEY (blocker_id) REFERENCES tasks(id) ON DELETE CASCADE +); + +INSERT INTO blockers_new (task_id, blocker_id) + SELECT task_id, blocker_id FROM blockers; + +DROP TABLE blockers; + +ALTER TABLE blockers_new RENAME TO blockers; diff --git a/tests/cli_migrate.rs b/tests/cli_migrate.rs index 900cff2019ed932f28c92b005cd6c12db47c7a0b..75cd6cedb86216927897ed593f22ebbe21d01625 100644 --- a/tests/cli_migrate.rs +++ b/tests/cli_migrate.rs @@ -20,8 +20,8 @@ fn fresh_init_sets_latest_version() { let version: u32 = conn .pragma_query_value(None, "user_version", |row| row.get(0)) .unwrap(); - // Version should be 4 (migration 0001 + 0002 + 0003 + 0004). - assert_eq!(version, 4); + // Version should be 5 (migration 0001 + 0002 + 0003 + 0004 + 0005). + assert_eq!(version, 5); } #[test] @@ -33,7 +33,8 @@ fn legacy_db_is_migrated_on_open() { // Create a v0 database with the old schema (no effort column). let conn = rusqlite::Connection::open(td_dir.join("tasks.db")).unwrap(); conn.execute_batch( - "CREATE TABLE tasks ( + "PRAGMA foreign_keys = OFF; + CREATE TABLE tasks ( id TEXT PRIMARY KEY, title TEXT NOT NULL, description TEXT DEFAULT '', @@ -81,7 +82,7 @@ fn legacy_db_is_migrated_on_open() { let version: u32 = conn .pragma_query_value(None, "user_version", |row| row.get(0)) .unwrap(); - assert_eq!(version, 4); + assert_eq!(version, 5); } #[test] @@ -128,6 +129,89 @@ fn blocker_fk_rejects_nonexistent_blocker_id() { ); } +#[test] +fn labels_fk_cascades_on_task_delete() { + let tmp = init_tmp(); + let conn = rusqlite::Connection::open(tmp.path().join(".td/tasks.db")).unwrap(); + conn.execute_batch("PRAGMA foreign_keys = ON").unwrap(); + + conn.execute( + "INSERT INTO tasks (id, title, created, updated) \ + VALUES ('td-labeled', 'Labeled task', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO labels (task_id, label) VALUES ('td-labeled', 'urgent')", + [], + ) + .unwrap(); + + conn.execute("DELETE FROM tasks WHERE id = 'td-labeled'", []) + .unwrap(); + + let label_count: i32 = conn + .query_row( + "SELECT COUNT(*) FROM labels WHERE task_id = 'td-labeled'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!( + label_count, 0, + "labels should be deleted via ON DELETE CASCADE" + ); +} + +#[test] +fn blockers_fk_cascades_on_task_delete() { + let tmp = init_tmp(); + let conn = rusqlite::Connection::open(tmp.path().join(".td/tasks.db")).unwrap(); + conn.execute_batch("PRAGMA foreign_keys = ON").unwrap(); + + conn.execute( + "INSERT INTO tasks (id, title, created, updated) \ + VALUES ('td-a', 'Task A', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO tasks (id, title, created, updated) \ + VALUES ('td-b', 'Task B', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO tasks (id, title, created, updated) \ + VALUES ('td-c', 'Task C', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z')", + [], + ) + .unwrap(); + + // td-b appears as both task_id and blocker_id across these rows. + conn.execute( + "INSERT INTO blockers (task_id, blocker_id) VALUES ('td-b', 'td-a')", + [], + ) + .unwrap(); + conn.execute( + "INSERT INTO blockers (task_id, blocker_id) VALUES ('td-c', 'td-b')", + [], + ) + .unwrap(); + + conn.execute("DELETE FROM tasks WHERE id = 'td-b'", []) + .unwrap(); + + let blocker_count: i32 = conn + .query_row("SELECT COUNT(*) FROM blockers", [], |r| r.get(0)) + .unwrap(); + assert_eq!( + blocker_count, 0, + "rows referencing a deleted task should be deleted via ON DELETE CASCADE" + ); +} + #[test] fn migration_cleans_dangling_blocker_ids() { let tmp = TempDir::new().unwrap(); @@ -137,7 +221,8 @@ fn migration_cleans_dangling_blocker_ids() { // Create a v2 database (pre-0003) with a dangling blocker_id. let conn = rusqlite::Connection::open(td_dir.join("tasks.db")).unwrap(); conn.execute_batch( - "CREATE TABLE tasks ( + "PRAGMA foreign_keys = OFF; + CREATE TABLE tasks ( id TEXT PRIMARY KEY, title TEXT NOT NULL, description TEXT DEFAULT '', @@ -198,3 +283,81 @@ fn migration_cleans_dangling_blocker_ids() { .unwrap(); assert_eq!(blocker, "td-b"); } + +#[test] +fn migration_cleans_dangling_labels() { + let tmp = TempDir::new().unwrap(); + let td_dir = tmp.path().join(".td"); + std::fs::create_dir_all(&td_dir).unwrap(); + + // Create a v4 database (pre-0005) with a dangling label row. + let conn = rusqlite::Connection::open(td_dir.join("tasks.db")).unwrap(); + conn.execute_batch( + "PRAGMA foreign_keys = OFF; + CREATE TABLE tasks ( + id TEXT PRIMARY KEY, + title TEXT NOT NULL, + description TEXT DEFAULT '', + type TEXT DEFAULT 'task', + priority INTEGER DEFAULT 2, + status TEXT DEFAULT 'open', + parent TEXT DEFAULT '', + created TEXT NOT NULL, + updated TEXT NOT NULL, + effort INTEGER NOT NULL DEFAULT 2 + ); + CREATE TABLE labels ( + task_id TEXT, label TEXT, + PRIMARY KEY (task_id, label), + FOREIGN KEY (task_id) REFERENCES tasks(id) + ); + CREATE TABLE blockers ( + task_id TEXT, blocker_id TEXT, + PRIMARY KEY (task_id, blocker_id), + FOREIGN KEY (task_id) REFERENCES tasks(id), + FOREIGN KEY (blocker_id) REFERENCES tasks(id) + ); + CREATE TABLE task_logs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + task_id TEXT NOT NULL, + timestamp TEXT NOT NULL, + body TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES tasks(id) ON DELETE CASCADE + ); + INSERT INTO tasks (id, title, created, updated) + VALUES ('td-real', 'Real task', '2024-01-01T00:00:00Z', '2024-01-01T00:00:00Z'); + INSERT INTO labels (task_id, label) VALUES ('td-real', 'kept'); + INSERT INTO labels (task_id, label) VALUES ('td-gone', 'orphan'); + PRAGMA user_version = 4;", + ) + .unwrap(); + drop(conn); + + // Running any command triggers migration to v5. + td().args(["--json", "list"]) + .current_dir(&tmp) + .assert() + .success(); + + let conn = rusqlite::Connection::open(td_dir.join("tasks.db")).unwrap(); + let kept_count: i32 = conn + .query_row( + "SELECT COUNT(*) FROM labels WHERE task_id = 'td-real' AND label = 'kept'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!(kept_count, 1, "valid label should survive migration"); + + let orphan_count: i32 = conn + .query_row( + "SELECT COUNT(*) FROM labels WHERE task_id = 'td-gone'", + [], + |r| r.get(0), + ) + .unwrap(); + assert_eq!( + orphan_count, 0, + "dangling label should be removed during migration" + ); +} diff --git a/tests/cli_rm.rs b/tests/cli_rm.rs new file mode 100644 index 0000000000000000000000000000000000000000..4eb16dde53a06ca80db3c6f8738e29e73f653053 --- /dev/null +++ b/tests/cli_rm.rs @@ -0,0 +1,192 @@ +use assert_cmd::Command; +use predicates::prelude::*; +use tempfile::TempDir; + +fn td() -> Command { + Command::cargo_bin("td").unwrap() +} + +fn init_tmp() -> TempDir { + let tmp = TempDir::new().unwrap(); + td().arg("init").current_dir(&tmp).assert().success(); + tmp +} + +fn create_task(dir: &TempDir, title: &str) -> String { + let out = td() + .args(["--json", "create", title]) + .current_dir(dir) + .output() + .unwrap(); + let v: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + v["id"].as_str().unwrap().to_string() +} + +fn get_task_json(dir: &TempDir, id: &str) -> serde_json::Value { + let out = td() + .args(["--json", "show", id]) + .current_dir(dir) + .output() + .unwrap(); + serde_json::from_slice(&out.stdout).unwrap() +} + +#[test] +fn rm_deletes_task() { + let tmp = init_tmp(); + let id = create_task(&tmp, "Delete me"); + + td().args(["rm", &id]) + .current_dir(&tmp) + .assert() + .success() + .stdout(predicate::str::contains("deleted")); + + td().args(["show", &id]) + .current_dir(&tmp) + .assert() + .failure() + .stderr(predicate::str::contains("not found")); +} + +#[test] +fn rm_deletes_multiple_ids() { + let tmp = init_tmp(); + let id1 = create_task(&tmp, "First"); + let id2 = create_task(&tmp, "Second"); + + td().args(["rm", &id1, &id2]) + .current_dir(&tmp) + .assert() + .success(); + + td().args(["show", &id1]) + .current_dir(&tmp) + .assert() + .failure(); + td().args(["show", &id2]) + .current_dir(&tmp) + .assert() + .failure(); +} + +#[test] +fn rm_requires_recursive_for_parent_task() { + let tmp = init_tmp(); + let parent = create_task(&tmp, "Parent"); + td().args(["create", "Child", "--parent", &parent]) + .current_dir(&tmp) + .assert() + .success(); + + td().args(["rm", &parent]) + .current_dir(&tmp) + .assert() + .failure() + .stderr(predicate::str::contains("use --recursive")); +} + +#[test] +fn rm_recursive_deletes_subtree() { + let tmp = init_tmp(); + let parent = create_task(&tmp, "Parent"); + + td().args(["create", "Child", "--parent", &parent]) + .current_dir(&tmp) + .assert() + .success(); + let child_id = format!("{parent}.1"); + + td().args(["create", "Grandchild", "--parent", &child_id]) + .current_dir(&tmp) + .assert() + .success(); + let grandchild_id = format!("{child_id}.1"); + + td().args(["rm", "--recursive", &parent]) + .current_dir(&tmp) + .assert() + .success(); + + td().args(["show", &parent]) + .current_dir(&tmp) + .assert() + .failure(); + td().args(["show", &child_id]) + .current_dir(&tmp) + .assert() + .failure(); + td().args(["show", &grandchild_id]) + .current_dir(&tmp) + .assert() + .failure(); +} + +#[test] +fn rm_detaches_dependents_and_warns() { + let tmp = init_tmp(); + let dependent = create_task(&tmp, "Dependent"); + let blocker = create_task(&tmp, "Blocker"); + + td().args(["dep", "add", &dependent, &blocker]) + .current_dir(&tmp) + .assert() + .success(); + + td().args(["rm", &blocker]) + .current_dir(&tmp) + .assert() + .success() + .stderr(predicate::str::contains("warning")) + .stderr(predicate::str::contains(&dependent)); + + let dependent_task = get_task_json(&tmp, &dependent); + let blockers = dependent_task["blockers"].as_array().unwrap(); + assert!(blockers.is_empty()); +} + +#[test] +fn rm_force_suppresses_unblocked_warning() { + let tmp = init_tmp(); + let dependent = create_task(&tmp, "Dependent"); + let blocker = create_task(&tmp, "Blocker"); + + td().args(["dep", "add", &dependent, &blocker]) + .current_dir(&tmp) + .assert() + .success(); + + td().args(["rm", "--force", &blocker]) + .current_dir(&tmp) + .assert() + .success() + .stderr(predicate::str::is_empty()); +} + +#[test] +fn rm_json_includes_deleted_and_unblocked_ids() { + let tmp = init_tmp(); + let dependent = create_task(&tmp, "Dependent"); + let blocker = create_task(&tmp, "Blocker"); + + td().args(["dep", "add", &dependent, &blocker]) + .current_dir(&tmp) + .assert() + .success(); + + let out = td() + .args(["--json", "rm", &blocker]) + .current_dir(&tmp) + .output() + .unwrap(); + assert!(out.status.success()); + + let v: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + let requested = v["requested_ids"].as_array().unwrap(); + let deleted = v["deleted_ids"].as_array().unwrap(); + let unblocked = v["unblocked_ids"].as_array().unwrap(); + + assert_eq!(requested, &vec![serde_json::Value::String(blocker.clone())]); + assert_eq!(deleted, &vec![serde_json::Value::String(blocker)]); + assert_eq!(unblocked, &vec![serde_json::Value::String(dependent)]); +}