From ebdc8eacf2368e9a3933376b8ac7f2fba02d85fe Mon Sep 17 00:00:00 2001 From: Kristian Wind Date: Tue, 18 Aug 2026 09:03:59 +0200 Subject: [PATCH] feat(admin): deleting an account stops deleting other people's work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tasks.created_by` was ON DELETE CASCADE, so retiring a colleague removed every task they had ever written — including in projects somebody else owns. The interface said so in plain words before you pressed the button, which is a warning rather than a fix. It was the last remaining way to lose other people's work by accident. SET NULL instead: the task stays and loses its author, the same shape `assignee_id` and `completed_by` already have. SQLite cannot alter a foreign key action, so the table is rebuilt — and with foreign keys on, the drop in the middle of a rebuild is a cascading delete that takes every row referencing `tasks` with it. `PRAGMA foreign_keys` is a no-op inside a transaction, so migrations now run on a connection of their own, and a migration can opt out of enforcement with `-- verdande:rebuild-tables`. `PRAGMA foreign_key_check` runs before the commit, so a rebuild that leaves a dangling reference fails the migration rather than writing it. The test fills a database at 0007, upgrades it, and counts. Without the marker it loses a task, a label link and a comment — which is exactly what it is for. The admin list now says two numbers instead of one: what disappears, and what stays behind unattributed. They are different sentences and the second is a reassurance. Co-Authored-By: Claude Opus 5 --- internal/httpapi/users_handlers.go | 13 +- internal/httpapi/users_test.go | 56 ++++-- .../0008_tasks_created_by_set_null.sql | 101 +++++++++++ internal/store/store.go | 115 +++++++++--- internal/store/store_test.go | 163 ++++++++++++++++++ internal/store/tasks.go | 60 +++++-- internal/store/users.go | 92 +++++++--- .../routes/indstillinger/brugere/+page.svelte | 20 ++- 8 files changed, 541 insertions(+), 79 deletions(-) create mode 100644 internal/store/migrations/0008_tasks_created_by_set_null.sql diff --git a/internal/httpapi/users_handlers.go b/internal/httpapi/users_handlers.go index e461bc9..cdbea07 100644 --- a/internal/httpapi/users_handlers.go +++ b/internal/httpapi/users_handlers.go @@ -30,11 +30,13 @@ type adminUserJSON struct { Self bool `json:"self"` CreatedAt string `json:"created_at"` LastSeenAt string `json:"last_seen_at,omitempty"` - // ProjectCount and TaskCount are what a delete would destroy. Sent with the - // list rather than fetched when the button is pressed, so the confirmation can - // name real numbers without a round trip in the middle of a decision. - ProjectCount int `json:"project_count"` - TaskCount int `json:"task_count"` + // ProjectCount and TaskCount are what a delete would destroy, and + // AuthoredElsewhere is what it would leave behind without an author. Sent with + // the list rather than fetched when the button is pressed, so the confirmation + // can name real numbers without a round trip in the middle of a decision. + ProjectCount int `json:"project_count"` + TaskCount int `json:"task_count"` + AuthoredElsewhere int `json:"authored_elsewhere"` } type pendingInviteJSON struct { @@ -70,6 +72,7 @@ func (s *Server) handleListUsers(w http.ResponseWriter, r *http.Request) { IsAdmin: u.IsAdmin, Self: u.ID == me.ID, CreatedAt: u.CreatedAt.Format(time.RFC3339), ProjectCount: u.ProjectCount, TaskCount: u.TaskCount, + AuthoredElsewhere: u.AuthoredElsewhere, } // Zero means never signed in, which is not a time and must not be sent as // one — 1970 in an interface reads as a bug rather than as "never". diff --git a/internal/httpapi/users_test.go b/internal/httpapi/users_test.go index 0b49091..13bb8bf 100644 --- a/internal/httpapi/users_test.go +++ b/internal/httpapi/users_test.go @@ -2,6 +2,7 @@ package httpapi import ( "net/http" + "net/url" "testing" ) @@ -125,8 +126,8 @@ func TestASecondAdministratorCanBeDemoted(t *testing.T) { } } -// Deleting an account takes more than its own projects, and the list says so -// before anybody presses the button. +// Deleting an account takes its own projects and leaves everything else behind. +// The list says both numbers before anybody presses the button. func TestTheDeleteCountsSayWhatWouldGo(t *testing.T) { ts := newTestServer(t) ts.bootstrap(t) @@ -143,9 +144,10 @@ func TestTheDeleteCountsSayWhatWouldGo(t *testing.T) { "content": "deres egen", "project_id": theirs["id"].(string), }) - // And a task they wrote in a project somebody else owns. tasks.created_by - // cascades, so this one goes too — and a count that missed it would understate - // the damage in exactly the case that matters. + // And a task they wrote in a project somebody else owns. This one survives — + // `created_by` is ON DELETE SET NULL — and it is counted separately, because + // "this much disappears" and "this much stays unattributed" are two different + // things to tell somebody who is about to press delete. _, mine := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Fælles"}) sharedID := mine["id"].(string) ts.do(t, "POST", "/api/v1/projects/"+sharedID+"/invites", map[string]any{ @@ -168,18 +170,52 @@ func TestTheDeleteCountsSayWhatWouldGo(t *testing.T) { if found["project_count"] != float64(1) { t.Errorf("project_count = %v, want 1 — the Inbox must not be counted", found["project_count"]) } - if found["task_count"] != float64(2) { - t.Errorf("task_count = %v, want 2 — the task in the shared project cascades too", + if found["task_count"] != float64(1) { + t.Errorf("task_count = %v, want 1 — only the task in their own project goes", found["task_count"]) } + if found["authored_elsewhere"] != float64(1) { + t.Errorf("authored_elsewhere = %v, want 1 — the task in the shared project stays", + found["authored_elsewhere"]) + } - // And the delete really does take it. + // And the delete leaves it standing, without an author. if resp, _ := ts.do(t, "DELETE", "/api/v1/users/"+otherUser.ID, nil); resp.StatusCode != http.StatusNoContent { t.Fatalf("delete: status %d", resp.StatusCode) } _, tasks := ts.do(t, "GET", "/api/v1/tasks?project_id="+sharedID, nil) - if got, _ := tasks["tasks"].([]any); len(got) != 0 { - t.Errorf("a task written by the deleted account survived in a shared project: %v", got) + got, _ := tasks["tasks"].([]any) + if len(got) != 1 { + t.Fatalf("a task written by the deleted account should survive in a shared project, got %v", got) + } + surviving := got[0].(map[string]any) + if surviving["content"] != "skrevet i et delt projekt" { + t.Errorf("the surviving task is not the one that was written: %v", surviving["content"]) + } + if surviving["created_by"] != "" { + t.Errorf("created_by = %q, want empty — the author is gone, the work is not", + surviving["created_by"]) + } +} + +// The tasks table was rebuilt to change created_by's delete action, and a rebuild +// is where an index quietly stops matching its table: tasks_fts is external-content +// and keyed on tasks.rowid, which a rebuilt table hands out afresh. Search is the +// only place that would notice. +func TestSearchStillFindsTasksAfterTheTableRebuild(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + _, project := ts.do(t, "POST", "/api/v1/projects", map[string]any{"name": "Have"}) + ts.do(t, "POST", "/api/v1/tasks", map[string]any{ + "content": "beskær det grønne hegn", "project_id": project["id"].(string), + }) + + for _, q := range []string{"grønne", "gronne", "hegn"} { + _, found := ts.do(t, "GET", "/api/v1/search?q="+url.QueryEscape(q), nil) + if got, _ := found["tasks"].([]any); len(got) != 1 { + t.Errorf("search for %q found %d tasks, want 1", q, len(got)) + } } } diff --git a/internal/store/migrations/0008_tasks_created_by_set_null.sql b/internal/store/migrations/0008_tasks_created_by_set_null.sql new file mode 100644 index 0000000..9c11e92 --- /dev/null +++ b/internal/store/migrations/0008_tasks_created_by_set_null.sql @@ -0,0 +1,101 @@ +-- `tasks.created_by` stops cascading. +-- +-- It was ON DELETE CASCADE, which made deleting an account delete every task that +-- account ever wrote — including in projects somebody else owns. Retiring a +-- colleague therefore removed their contributions from shared work rather than +-- leaving it behind. It is the last remaining way to lose other people's work by +-- accident, and the administrator's page saying so in plain words is a warning, not +-- a fix. +-- +-- SET NULL instead: the task stays and loses its author. `created_by` becomes +-- nullable to make that expressible, which is the same shape `assignee_id` and +-- `completed_by` already have for the same reason. +-- +-- SQLite cannot alter a foreign key action, so the table is rebuilt. The FTS +-- triggers come along: they are dropped first so that DROP TABLE and ALTER TABLE +-- see a schema they can parse, and recreated below. `tasks_fts` is an +-- external-content table keyed on `tasks.rowid`, and a rebuilt table hands out new +-- rowids, so the index is rebuilt at the end rather than left pointing at rows that +-- have moved. +-- +-- verdande:rebuild-tables + +DROP TRIGGER tasks_fts_insert; +DROP TRIGGER tasks_fts_delete; +DROP TRIGGER tasks_fts_update; + +CREATE TABLE tasks_rebuilt ( + id TEXT PRIMARY KEY, + project_id TEXT NOT NULL REFERENCES projects (id) ON DELETE CASCADE, + section_id TEXT REFERENCES sections (id) ON DELETE SET NULL, + parent_id TEXT REFERENCES tasks (id) ON DELETE CASCADE, + content TEXT NOT NULL, + description TEXT NOT NULL DEFAULT '', + priority INTEGER NOT NULL DEFAULT 4, + due_date TEXT, + due_datetime INTEGER, + due_timezone TEXT, + duration_min INTEGER, + recurrence_rule TEXT, + assignee_id TEXT REFERENCES users (id) ON DELETE SET NULL, + -- Nullable, and SET NULL: an account can go without its work going with it. + created_by TEXT REFERENCES users (id) ON DELETE SET NULL, + completed_at INTEGER, + completed_by TEXT REFERENCES users (id) ON DELETE SET NULL, + sort_order REAL NOT NULL DEFAULT 0, + created_at INTEGER NOT NULL, + updated_at INTEGER NOT NULL, + deleted_at INTEGER, + fold TEXT GENERATED ALWAYS AS ( + replace(replace(replace(replace(replace(replace( + content || ' ' || description, + 'ø', 'o'), 'Ø', 'o'), + 'æ', 'ae'), 'Æ', 'ae'), + 'å', 'aa'), 'Å', 'aa') + ) STORED, + CHECK (priority BETWEEN 1 AND 4) +); + +INSERT INTO tasks_rebuilt + (id, project_id, section_id, parent_id, content, description, priority, + due_date, due_datetime, due_timezone, duration_min, recurrence_rule, + assignee_id, created_by, completed_at, completed_by, sort_order, + created_at, updated_at, deleted_at) +SELECT + id, project_id, section_id, parent_id, content, description, priority, + due_date, due_datetime, due_timezone, duration_min, recurrence_rule, + assignee_id, created_by, completed_at, completed_by, sort_order, + created_at, updated_at, deleted_at +FROM tasks; + +DROP TABLE tasks; + +ALTER TABLE tasks_rebuilt RENAME TO tasks; + +CREATE INDEX idx_tasks_project ON tasks (project_id, sort_order) + WHERE deleted_at IS NULL AND completed_at IS NULL; +CREATE INDEX idx_tasks_due ON tasks (due_date) + WHERE deleted_at IS NULL AND completed_at IS NULL AND due_date IS NOT NULL; +CREATE INDEX idx_tasks_assignee ON tasks (assignee_id) + WHERE deleted_at IS NULL AND completed_at IS NULL AND assignee_id IS NOT NULL; +CREATE INDEX idx_tasks_parent ON tasks (parent_id) WHERE parent_id IS NOT NULL; + +CREATE TRIGGER tasks_fts_insert AFTER INSERT ON tasks BEGIN + INSERT INTO tasks_fts (rowid, content, description, fold, id, project_id) + VALUES (new.rowid, new.content, new.description, new.fold, new.id, new.project_id); +END; + +CREATE TRIGGER tasks_fts_delete AFTER DELETE ON tasks BEGIN + INSERT INTO tasks_fts (tasks_fts, rowid, content, description, fold, id, project_id) + VALUES ('delete', old.rowid, old.content, old.description, old.fold, old.id, old.project_id); +END; + +CREATE TRIGGER tasks_fts_update AFTER UPDATE ON tasks BEGIN + INSERT INTO tasks_fts (tasks_fts, rowid, content, description, fold, id, project_id) + VALUES ('delete', old.rowid, old.content, old.description, old.fold, old.id, old.project_id); + INSERT INTO tasks_fts (rowid, content, description, fold, id, project_id) + VALUES (new.rowid, new.content, new.description, new.fold, new.id, new.project_id); +END; + +-- The rowids the index was built against are gone with the old table. +INSERT INTO tasks_fts (tasks_fts) VALUES ('rebuild'); diff --git a/internal/store/store.go b/internal/store/store.go index 1f42b19..0187bba 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -25,15 +25,18 @@ type DB struct { path string } -// Open prepares the database at path and brings the schema up to date. +// dsn is the connection string. // // SQLite is a single file, but it is being driven by an HTTP server with concurrent // readers and a WebSocket fan-out, so the pragmas below matter more than usual: // WAL lets readers run while a write is in flight, busy_timeout turns the "database // is locked" race into a short wait, and foreign_keys is off by default in SQLite // and has to be asked for on every connection. -func Open(path string) (*DB, error) { - dsn := "file:" + filepath.ToSlash(path) + "?" + strings.Join([]string{ +// +// Its own function so the migration tests can open a database with exactly the +// pragmas production runs with. +func dsn(path string) string { + return "file:" + filepath.ToSlash(path) + "?" + strings.Join([]string{ "_pragma=journal_mode(WAL)", "_pragma=busy_timeout(5000)", "_pragma=foreign_keys(ON)", @@ -56,8 +59,11 @@ func Open(path string) (*DB, error) { // spilling them into the data volume. "_pragma=temp_store(MEMORY)", }, "&") +} - sqlDB, err := sql.Open("sqlite", dsn) +// Open prepares the database at path and brings the schema up to date. +func Open(path string) (*DB, error) { + sqlDB, err := sql.Open("sqlite", dsn(path)) if err != nil { return nil, fmt.Errorf("open sqlite: %w", err) } @@ -91,11 +97,36 @@ func Open(path string) (*DB, error) { func (db *DB) Path() string { return db.path } +// rebuildMarker opts a migration out of foreign key enforcement for the length of +// its transaction. +// +// SQLite cannot alter a column's foreign key action, so changing one means +// rebuilding the table: copy it, drop the original, rename the copy back. With +// foreign keys on, the drop is a cascading delete — every row in every table that +// references `tasks` would go with it, which is the opposite of what a migration +// that exists to *stop* losing rows should do. `PRAGMA foreign_keys` is a no-op +// inside a transaction, so it has to be set on the connection before BEGIN; that +// is why migrations run on a connection of their own rather than on the pool. +// +// `PRAGMA foreign_key_check` runs before the commit, so a rebuild that leaves a +// dangling reference fails the migration instead of writing it. +const rebuildMarker = "-- verdande:rebuild-tables" + // migrate applies every embedded migration that has not run yet, in filename order, // each inside its own transaction. A migration that fails leaves the schema at the // last version that succeeded rather than half-applied. +// +// Everything here runs on one connection: a migration marked with rebuildMarker +// needs a pragma to hold across its transaction, and a pragma only applies to the +// connection it was issued on. func (db *DB) migrate(ctx context.Context) error { - if _, err := db.ExecContext(ctx, ` + conn, err := db.Conn(ctx) + if err != nil { + return err + } + defer conn.Close() + + if _, err := conn.ExecContext(ctx, ` CREATE TABLE IF NOT EXISTS schema_migrations ( name TEXT PRIMARY KEY, applied_at INTEGER NOT NULL @@ -104,7 +135,7 @@ func (db *DB) migrate(ctx context.Context) error { } applied := map[string]bool{} - rows, err := db.QueryContext(ctx, `SELECT name FROM schema_migrations`) + rows, err := conn.QueryContext(ctx, `SELECT name FROM schema_migrations`) if err != nil { return fmt.Errorf("read schema_migrations: %w", err) } @@ -136,27 +167,69 @@ func (db *DB) migrate(ctx context.Context) error { if err != nil { return err } - - tx, err := db.BeginTx(ctx, nil) - if err != nil { + if err := db.applyMigration(ctx, conn, name, string(body)); err != nil { return err } - if _, err := tx.ExecContext(ctx, string(body)); err != nil { - tx.Rollback() - return fmt.Errorf("migration %s: %w", name, err) - } - if _, err := tx.ExecContext(ctx, - `INSERT INTO schema_migrations (name, applied_at) VALUES (?, unixepoch())`, name); err != nil { - tx.Rollback() - return fmt.Errorf("record migration %s: %w", name, err) - } - if err := tx.Commit(); err != nil { - return fmt.Errorf("commit migration %s: %w", name, err) - } } return nil } +func (db *DB) applyMigration(ctx context.Context, conn *sql.Conn, name, body string) error { + if strings.Contains(body, rebuildMarker) { + if _, err := conn.ExecContext(ctx, `PRAGMA foreign_keys = OFF`); err != nil { + return fmt.Errorf("migration %s: disable foreign keys: %w", name, err) + } + // Back on whatever happens below: this connection goes back to the pool. + defer conn.ExecContext(ctx, `PRAGMA foreign_keys = ON`) + } + + tx, err := conn.BeginTx(ctx, nil) + if err != nil { + return err + } + if _, err := tx.ExecContext(ctx, body); err != nil { + tx.Rollback() + return fmt.Errorf("migration %s: %w", name, err) + } + if strings.Contains(body, rebuildMarker) { + if err := foreignKeyCheck(ctx, tx); err != nil { + tx.Rollback() + return fmt.Errorf("migration %s: %w", name, err) + } + } + if _, err := tx.ExecContext(ctx, + `INSERT INTO schema_migrations (name, applied_at) VALUES (?, unixepoch())`, name); err != nil { + tx.Rollback() + return fmt.Errorf("record migration %s: %w", name, err) + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("commit migration %s: %w", name, err) + } + return nil +} + +// foreignKeyCheck reports the first row that references something that is not +// there. It names the table and the rowid, because a rebuild that drops rows on the +// floor is otherwise indistinguishable from one that worked. +func foreignKeyCheck(ctx context.Context, tx *sql.Tx) error { + rows, err := tx.QueryContext(ctx, `PRAGMA foreign_key_check`) + if err != nil { + return fmt.Errorf("foreign key check: %w", err) + } + defer rows.Close() + if rows.Next() { + var table, parent string + var rowid sql.NullInt64 + var fkid int + if err := rows.Scan(&table, &rowid, &parent, &fkid); err != nil { + return fmt.Errorf("foreign key check: %w", err) + } + return fmt.Errorf("foreign key check: %s row %d references a missing %s", + table, rowid.Int64, parent) + } + return rows.Err() +} + // Tx runs fn inside a transaction, rolling back on error or panic. func (db *DB) Tx(ctx context.Context, fn func(*sql.Tx) error) error { tx, err := db.BeginTx(ctx, nil) diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 0013413..fc6c222 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -2,9 +2,13 @@ package store import ( "context" + "database/sql" "fmt" + "io/fs" "path/filepath" + "sort" "testing" + "time" ) // openTest gives each test its own database file. In-memory SQLite would be faster, @@ -410,3 +414,162 @@ func ftsCount(t *testing.T, db *DB, query string) int { } return n } + +// The rebuild in 0008 is the one migration that does not just add: it copies the +// tasks table, drops the original and renames the copy back. Every test above this +// one starts from an empty database, where a rebuild that loses rows looks exactly +// like a rebuild that works — so this one carries data across it. +// +// What can go wrong, in the order it would: the drop cascades and takes the child +// rows with it; the copy misses a column; the FTS index is left keyed on rowids the +// rebuilt table no longer hands out; and finally the thing the migration is for, +// which is that deleting an account stops deleting other people's work. +func TestTheTasksRebuildCarriesTheDataAcross(t *testing.T) { + ctx := context.Background() + path := filepath.Join(t.TempDir(), "upgrade.db") + + // A database at 0007: the schema as it shipped in v0.10.2. + sqlDB, err := sql.Open("sqlite", dsn(path)) + if err != nil { + t.Fatalf("open: %v", err) + } + defer sqlDB.Close() + if _, err := sqlDB.ExecContext(ctx, ` + CREATE TABLE IF NOT EXISTS schema_migrations ( + name TEXT PRIMARY KEY, + applied_at INTEGER NOT NULL + )`); err != nil { + t.Fatal(err) + } + entries, err := fs.Glob(migrationFS, "migrations/*.sql") + if err != nil { + t.Fatal(err) + } + sort.Strings(entries) + for _, entry := range entries { + name := filepath.Base(entry) + if name >= "0008" { + break + } + body, err := migrationFS.ReadFile(entry) + if err != nil { + t.Fatal(err) + } + if _, err := sqlDB.ExecContext(ctx, string(body)); err != nil { + t.Fatalf("apply %s: %v", name, err) + } + if _, err := sqlDB.ExecContext(ctx, + `INSERT INTO schema_migrations (name, applied_at) VALUES (?, unixepoch())`, name); err != nil { + t.Fatal(err) + } + } + + // Two people, one project each, and a task in each project written by the + // other — which is the shape the migration exists for. Plus a label, a comment + // and a sub-task, so the child tables have something to lose. + now := time.Now().Unix() + for _, stmt := range []string{ + `INSERT INTO users (id, email, name, password_hash, avatar_color, is_admin, created_at, updated_at) + VALUES ('u1', 'en@example.dk', 'En', 'x', 'graphite', 1, ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `), + ('u2', 'to@example.dk', 'To', 'x', 'graphite', 0, ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `)`, + `INSERT INTO projects (id, owner_id, name, color, created_at, updated_at) + VALUES ('p1', 'u1', 'Ens', 'graphite', ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `), + ('p2', 'u2', 'Tos', 'graphite', ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `)`, + `INSERT INTO tasks (id, project_id, content, description, created_by, priority, sort_order, created_at, updated_at) + VALUES ('t1', 'p1', 'grøn hæk', 'i ens eget', 'u1', 2, 1.0, ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `), + ('t2', 'p2', 'skrevet af en anden', '', 'u1', 4, 2.0, ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `)`, + `INSERT INTO tasks (id, project_id, parent_id, content, created_by, sort_order, created_at, updated_at) + VALUES ('t3', 'p1', 't1', 'undertask', 'u1', 3.0, ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `)`, + `INSERT INTO labels (id, user_id, name, created_at) VALUES ('l1', 'u1', 'have', ` + fmt.Sprint(now) + `)`, + `INSERT INTO task_labels (task_id, label_id) VALUES ('t1', 'l1')`, + `INSERT INTO comments (id, task_id, user_id, body, created_at, updated_at) + VALUES ('c1', 't1', 'u1', 'en kommentar', ` + fmt.Sprint(now) + `, ` + fmt.Sprint(now) + `)`, + } { + if _, err := sqlDB.ExecContext(ctx, stmt); err != nil { + t.Fatalf("seed: %v\n%s", err, stmt) + } + } + sqlDB.Close() + + // Now the upgrade, through the real runner. + db, err := Open(path) + if err != nil { + t.Fatalf("upgrade: %v", err) + } + defer db.Close() + + var tasks, labels, comments int + if err := db.QueryRow(`SELECT count(*) FROM tasks`).Scan(&tasks); err != nil { + t.Fatal(err) + } + if err := db.QueryRow(`SELECT count(*) FROM task_labels`).Scan(&labels); err != nil { + t.Fatal(err) + } + if err := db.QueryRow(`SELECT count(*) FROM comments`).Scan(&comments); err != nil { + t.Fatal(err) + } + if tasks != 3 || labels != 1 || comments != 1 { + t.Fatalf("after the rebuild: %d tasks, %d labels, %d comments — want 3, 1, 1. "+ + "A drop that cascaded would look exactly like this", tasks, labels, comments) + } + + // Every column came across, including the generated one and the sub-task's + // self-reference. + var content, description, parent string + var priority int + if err := db.QueryRow( + `SELECT content, description, priority FROM tasks WHERE id = 't1'`). + Scan(&content, &description, &priority); err != nil { + t.Fatal(err) + } + if content != "grøn hæk" || description != "i ens eget" || priority != 2 { + t.Errorf("t1 came across as %q/%q/%d", content, description, priority) + } + if err := db.QueryRow(`SELECT parent_id FROM tasks WHERE id = 't3'`).Scan(&parent); err != nil { + t.Fatal(err) + } + if parent != "t1" { + t.Errorf("the sub-task lost its parent: %q", parent) + } + + // The FTS index was rebuilt against the new rowids. Searching the fold column + // is the sharpest check: it is generated, so a copy that skipped it would still + // have the row and not the index entry. + for _, q := range []string{"grøn", "gron", "haek"} { + var n int + if err := db.QueryRow(`SELECT count(*) FROM tasks_fts WHERE tasks_fts MATCH ?`, q).Scan(&n); err != nil { + t.Fatalf("search %q: %v", q, err) + } + if n != 1 { + t.Errorf("search for %q found %d rows, want 1 — the index is keyed on rowids "+ + "the rebuilt table no longer hands out", q, n) + } + } + + // And the point of the whole thing. + if err := db.DeleteUser(ctx, "u1"); err != nil { + t.Fatalf("delete user: %v", err) + } + var survived int + if err := db.QueryRow(`SELECT count(*) FROM tasks WHERE id = 't2'`).Scan(&survived); err != nil { + t.Fatal(err) + } + if survived != 1 { + t.Fatal("a task written in somebody else's project went with the account") + } + var author sql.NullString + if err := db.QueryRow(`SELECT created_by FROM tasks WHERE id = 't2'`).Scan(&author); err != nil { + t.Fatal(err) + } + if author.Valid { + t.Errorf("created_by = %q, want null", author.String) + } + // Their own project still goes, and everything in it with it. + var own int + if err := db.QueryRow(`SELECT count(*) FROM tasks WHERE project_id = 'p1'`).Scan(&own); err != nil { + t.Fatal(err) + } + if own != 0 { + t.Errorf("%d tasks survived in the deleted account's own project", own) + } +} diff --git a/internal/store/tasks.go b/internal/store/tasks.go index 846c4c5..72f405f 100644 --- a/internal/store/tasks.go +++ b/internal/store/tasks.go @@ -29,11 +29,23 @@ type Task struct { AssigneeID string CompletedAt *time.Time CompletedBy string - CreatedBy string - SortOrder float64 - Labels []string - CreatedAt time.Time - UpdatedAt time.Time + // CreatedBy is empty when the author's account has been deleted: `created_by` + // is ON DELETE SET NULL so the task survives the person who wrote it. + CreatedBy string + SortOrder float64 + // SubtaskCount and SubtaskDone are what a row can say without being opened: + // that there is something underneath it, and how much of it is left. Counted + // for every task in a list, in the same query — a hundred extra lookups to draw + // a badge is not a badge worth having. + SubtaskCount int + SubtaskDone int + // AttachmentCount is the same idea for files. A task with a drawing on it looks + // exactly like one without, and the whole reason to attach something is that + // somebody should find it. + AttachmentCount int + Labels []string + CreatedAt time.Time + UpdatedAt time.Time } func (t *Task) Completed() bool { return t.CompletedAt != nil } @@ -173,7 +185,7 @@ func (db *DB) ListTasks(ctx context.Context, userID string, f TaskFilter) ([]Tas SELECT t.id, t.project_id, t.section_id, t.parent_id, t.content, t.description, t.priority, t.due_date, t.due_datetime, t.due_timezone, t.duration_min, t.recurrence_rule, t.assignee_id, t.completed_at, t.completed_by, - t.created_by, t.sort_order, t.created_at, t.updated_at + t.created_by, t.sort_order, t.created_at, t.updated_at,` + taskCounts + ` FROM tasks t JOIN projects p ON p.id = t.project_id WHERE ` + strings.Join(where, " AND ") + ` @@ -264,11 +276,11 @@ func (db *DB) GetTask(ctx context.Context, taskID, userID string) (*Task, error) return nil, err } rows, err := db.QueryContext(ctx, ` - SELECT id, project_id, section_id, parent_id, content, description, - priority, due_date, due_datetime, due_timezone, duration_min, - recurrence_rule, assignee_id, completed_at, completed_by, - created_by, sort_order, created_at, updated_at - FROM tasks WHERE id = ? AND deleted_at IS NULL`, taskID) + SELECT t.id, t.project_id, t.section_id, t.parent_id, t.content, t.description, + t.priority, t.due_date, t.due_datetime, t.due_timezone, t.duration_min, + t.recurrence_rule, t.assignee_id, t.completed_at, t.completed_by, + t.created_by, t.sort_order, t.created_at, t.updated_at,`+taskCounts+` + FROM tasks t WHERE t.id = ? AND t.deleted_at IS NULL`, taskID) if err != nil { return nil, err } @@ -317,9 +329,29 @@ func (db *DB) taskLabels(ctx context.Context, taskID string) ([]string, error) { return out, rows.Err() } +// taskCounts are what a row can say about itself without being opened. +// +// Correlated subqueries rather than a second round of lookups: a list of fifty +// tasks would otherwise be a hundred more queries, and the whole point is that a +// row shows this without anybody asking for it. `idx_tasks_parent` covers the +// first two; the attachment count is a small table with a task_id on it. +// +// Sub-tasks are counted whether they are open or closed, and the finished ones +// again separately, because "3" and "1 of 3" are different sentences and only the +// second one says whether there is anything left to do. +const taskCounts = ` + (SELECT count(*) FROM tasks c + WHERE c.parent_id = t.id AND c.deleted_at IS NULL), + (SELECT count(*) FROM tasks c + WHERE c.parent_id = t.id AND c.deleted_at IS NULL AND c.completed_at IS NOT NULL), + (SELECT count(*) FROM attachments a WHERE a.task_id = t.id)` + func scanTask(rows *sql.Rows) (Task, error) { var t Task var sectionID, parentID, dueDate, dueTZ, recurrence, assignee, completedBy sql.NullString + // createdBy is null on a task whose author's account has been deleted. The work + // stays; it loses its author. + var createdBy sql.NullString var dueDatetime, completedAt sql.NullInt64 var duration sql.NullInt64 var created, updated int64 @@ -327,13 +359,15 @@ func scanTask(rows *sql.Rows) (Task, error) { err := rows.Scan(&t.ID, &t.ProjectID, §ionID, &parentID, &t.Content, &t.Description, &t.Priority, &dueDate, &dueDatetime, &dueTZ, &duration, &recurrence, &assignee, &completedAt, &completedBy, - &t.CreatedBy, &t.SortOrder, &created, &updated) + &createdBy, &t.SortOrder, &created, &updated, + &t.SubtaskCount, &t.SubtaskDone, &t.AttachmentCount) if err != nil { return t, err } t.SectionID = sectionID.String t.ParentID = parentID.String + t.CreatedBy = createdBy.String t.DueDate = dueDate.String t.DueTimezone = dueTZ.String t.RecurrenceRule = recurrence.String @@ -384,7 +418,7 @@ func (db *DB) CreateTask(ctx context.Context, t *Task, labelNames []string) erro t.ID, t.ProjectID, nullString(t.SectionID), nullString(t.ParentID), t.Content, t.Description, t.Priority, nullString(t.DueDate), nullTime(t.DueDatetime), nullString(t.DueTimezone), nullInt(t.DurationMin), - nullString(t.RecurrenceRule), nullString(t.AssigneeID), t.CreatedBy, + nullString(t.RecurrenceRule), nullString(t.AssigneeID), nullString(t.CreatedBy), t.SortOrder, now.Unix(), now.Unix()) if err != nil { return err diff --git a/internal/store/users.go b/internal/store/users.go index 7c3de83..58ec5fd 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -3,6 +3,7 @@ package store import ( "context" "database/sql" + "encoding/json" "errors" "fmt" "strings" @@ -28,8 +29,15 @@ type User struct { Timezone string Locale string IsAdmin bool - CreatedAt time.Time - UpdatedAt time.Time + // SidebarCollapsed is the sidebar headings this person has folded away, by + // key. On the account rather than in localStorage, for the same reason a + // project group carries `collapsed` on its row: folding a heading is a + // statement about the work — "I am not in Etiketter at the moment" — and that + // is as true on the laptop as on the desktop. The sidebar's *width* is the + // opposite case and is stored per browser. + SidebarCollapsed []string + CreatedAt time.Time + UpdatedAt time.Time } // NormalizeEmail is the one place an address is canonicalised. Addresses arrive @@ -96,7 +104,7 @@ func (db *DB) UserByID(ctx context.Context, id string) (*User, error) { } const userColumns = `id, email, name, password_hash, totp_secret, totp_enabled, - avatar_color, timezone, locale, is_admin, created_at, updated_at` + avatar_color, timezone, locale, is_admin, sidebar_collapsed, created_at, updated_at` func (db *DB) scanUser(ctx context.Context, query string, args ...any) (*User, error) { var u User @@ -104,9 +112,11 @@ func (db *DB) scanUser(ctx context.Context, query string, args ...any) (*User, e var totpEnabled, isAdmin int var created, updated int64 + var collapsed string + err := db.QueryRowContext(ctx, query, args...).Scan( &u.ID, &u.Email, &u.Name, &u.PasswordHash, &secret, &totpEnabled, - &u.AvatarColor, &u.Timezone, &u.Locale, &isAdmin, &created, &updated) + &u.AvatarColor, &u.Timezone, &u.Locale, &isAdmin, &collapsed, &created, &updated) if errors.Is(err, sql.ErrNoRows) { return nil, ErrNotFound } @@ -117,11 +127,41 @@ func (db *DB) scanUser(ctx context.Context, query string, args ...any) (*User, e u.TOTPSecret = secret.String u.TOTPEnabled = totpEnabled == 1 u.IsAdmin = isAdmin == 1 + // A malformed value is read as "nothing folded" rather than as an error: the + // worst case is a sidebar that opens fully, which is the state it shipped in. + u.SidebarCollapsed = []string{} + _ = json.Unmarshal([]byte(collapsed), &u.SidebarCollapsed) u.CreatedAt = time.Unix(created, 0).UTC() u.UpdatedAt = time.Unix(updated, 0).UTC() return &u, nil } +// SetSidebarCollapsed records which sidebar headings are folded. +// +// Its own method rather than a field on UpdateProfile: that one backs a form with +// a save button and validates three fields together, and this is a toggle that +// writes on every click. Sharing the path would mean a fold sending a name and a +// timezone it was never asked to change. +func (db *DB) SetSidebarCollapsed(ctx context.Context, userID string, sections []string) error { + if sections == nil { + sections = []string{} + } + raw, err := json.Marshal(sections) + if err != nil { + return err + } + res, err := db.ExecContext(ctx, + `UPDATE users SET sidebar_collapsed = ?, updated_at = ? WHERE id = ?`, + string(raw), time.Now().Unix(), userID) + if err != nil { + return err + } + if n, _ := res.RowsAffected(); n == 0 { + return ErrNotFound + } + return nil +} + // Person is the public half of a user: what somebody else is allowed to see when // their name has to appear next to a task. No address, no hash, no timezone. type Person struct { @@ -175,12 +215,17 @@ type UserSummary struct { // ProjectCount excludes the Inbox: every account has one, so counting it would // make "1 project" mean "nothing". ProjectCount int - // TaskCount is everything a delete would destroy, which is not the same as - // "their tasks": it is the tasks in projects they own *plus* every task they - // wrote anywhere, because tasks.created_by cascades. A number that counted - // only the first would understate the damage in exactly the case that matters - // — a colleague leaving a shared project. + // TaskCount is everything a delete would destroy: the tasks in the projects + // they own, which go with those projects. Tasks they merely *wrote* in + // somebody else's project are not counted, because they no longer go — + // `created_by` is ON DELETE SET NULL, so that work stays and loses its author. TaskCount int + // AuthoredElsewhere is what a delete leaves behind unattributed: tasks they + // wrote in projects other people own. Separate from TaskCount because it is a + // different sentence — "this much disappears" and "this much stays without a + // name on it" — and running them together was what made the old number sound + // like a threat it no longer is. + AuthoredElsewhere int } // ListUsers returns every account on the instance. Administrators only — this is @@ -192,8 +237,9 @@ func (db *DB) ListUsers(ctx context.Context) ([]UserSummary, error) { (SELECT count(*) FROM projects p WHERE p.owner_id = u.id AND p.is_inbox = 0 AND p.deleted_at IS NULL), (SELECT count(*) FROM tasks t JOIN projects p ON p.id = t.project_id - WHERE t.deleted_at IS NULL - AND (p.owner_id = u.id OR t.created_by = u.id)) + WHERE t.deleted_at IS NULL AND p.owner_id = u.id), + (SELECT count(*) FROM tasks t JOIN projects p ON p.id = t.project_id + WHERE t.deleted_at IS NULL AND t.created_by = u.id AND p.owner_id <> u.id) FROM users u ORDER BY u.is_admin DESC, u.name`) if err != nil { @@ -207,7 +253,7 @@ func (db *DB) ListUsers(ctx context.Context) ([]UserSummary, error) { var isAdmin int var created, lastSeen int64 if err := rows.Scan(&s.ID, &s.Email, &s.Name, &s.AvatarColor, &isAdmin, &created, - &lastSeen, &s.ProjectCount, &s.TaskCount); err != nil { + &lastSeen, &s.ProjectCount, &s.TaskCount, &s.AuthoredElsewhere); err != nil { return nil, err } s.IsAdmin = isAdmin == 1 @@ -249,20 +295,18 @@ func (db *DB) SetUserAdmin(ctx context.Context, userID string, admin bool) error // there is no way back — which is why the handler sends the counts to the // interface first. // -// What goes is wider than it looks. `projects.owner_id` cascades, so their -// projects go and every task in them goes too. But `tasks.created_by` cascades as -// well, so **every task they ever wrote goes, including in projects somebody else -// owns**. Retiring a colleague's account therefore removes their contributions -// from shared projects rather than leaving them behind unattributed. +// What goes: `projects.owner_id` cascades, so their projects go and every task in +// them goes too. That is the whole of it, and the handler sends the count to the +// interface first. // -// That is the schema as it stands rather than a decision made here. Changing it — -// `created_by` as ON DELETE SET NULL, so the work stays and loses its author — -// means rebuilding the tasks table, which carries the FTS triggers with it. Worth -// doing; not worth doing quietly as part of adding a screen. +// What stays: everything they wrote in somebody else's project. `tasks.created_by` +// used to cascade as well, which meant retiring a colleague's account removed +// their contributions from shared work — see migration 0008, which rebuilt the +// table to make it ON DELETE SET NULL. The task survives and loses its author, +// which is the honest record of what happened. // -// What survives: tasks merely *assigned* to them, which are unassigned by -// `assignee_id ON DELETE SET NULL`, and completions, by the same rule on -// `completed_by`. +// Tasks merely *assigned* to them are unassigned by `assignee_id ON DELETE SET +// NULL`, and completions by the same rule on `completed_by`. func (db *DB) DeleteUser(ctx context.Context, userID string) error { res, err := db.ExecContext(ctx, `DELETE FROM users WHERE id = ?`, userID) if err != nil { diff --git a/web/src/routes/indstillinger/brugere/+page.svelte b/web/src/routes/indstillinger/brugere/+page.svelte index 5bc7b4f..b542d84 100644 --- a/web/src/routes/indstillinger/brugere/+page.svelte +++ b/web/src/routes/indstillinger/brugere/+page.svelte @@ -68,18 +68,26 @@ /** * Deleting an account is the only thing in this app that cannot be undone. * - * Everything else is soft-deleted with a trash behind it. This is not, and it - * takes more than the person's own work: `tasks.created_by` cascades, so what - * they wrote in projects somebody else owns goes too. The confirmation says the - * numbers out loud, which is why the list carries them. + * Everything else is soft-deleted with a trash behind it. This is not: the + * person's own projects go, and every task in them. What they wrote in somebody + * else's project stays and loses its author — `tasks.created_by` is ON DELETE + * SET NULL since migration 0008. Both numbers are said out loud, because they + * are two different sentences and the second one is a reassurance rather than a + * warning. */ async function remove(user) { const parts = [`Slet ${user.name} <${user.email}> for altid?`, '']; if (user.project_count || user.task_count) { parts.push( `Det sletter ${count(user.project_count, 'projekt', 'projekter')} og ` + - `${count(user.task_count, 'opgave', 'opgaver')} — også opgaver, personen har ` + - `skrevet i projekter, andre ejer.`, + `${count(user.task_count, 'opgave', 'opgaver')} i dem.`, + '' + ); + } + if (user.authored_elsewhere) { + parts.push( + `${count(user.authored_elsewhere, 'opgave', 'opgaver')} i andres projekter ` + + `bliver stående uden forfatter.`, '' ); }