diff --git a/internal/models/item.go b/internal/models/item.go index b1ba96c7c..f0aa61748 100644 --- a/internal/models/item.go +++ b/internal/models/item.go @@ -1835,6 +1835,11 @@ type ItemListParams struct { // an outgoing parent/implements item_link. Incoming links do not count. Unparented bool IncludeArchived bool + // IncludeDeletedCollections returns items whose collection is + // soft-deleted. Every read leaves them out by default (BUG-3425): a + // collection's delete leaves its items live. Only the account data + // export sets it, so a user's export still holds everything they own. + IncludeDeletedCollections bool // NonTerminal, when true, restricts results to items whose resolved // done-field value is NOT one of their collection's terminal options. // Each collection is evaluated against its OWN terminal_options (falling diff --git a/internal/server/bug3333_deleted_collection_grant_test.go b/internal/server/bug3333_deleted_collection_grant_test.go index 6d1ef54a4..0022389a8 100644 --- a/internal/server/bug3333_deleted_collection_grant_test.go +++ b/internal/server/bug3333_deleted_collection_grant_test.go @@ -120,9 +120,12 @@ func TestBUG3333_DeletedCollectionNotVisibleThroughAGrant(t *testing.T) { } } - after := []string{"Doc in a granted collection", "Task in an assigned collection"} + // Nothing from either deleted collection lists: BUG-3425 leaves a + // soft-deleted collection's items out of every ListItems reader, the + // whole-collection grant's doc included (it listed here before BUG-3425). + after := []string{"Task in an assigned collection"} if got := listTitles(); !bug3333Equal(got, after) { - t.Errorf("after deleting the collections the member lists %v, want %v (nothing from the item-granted collection)", got, after) + t.Errorf("after deleting the collections the member lists %v, want %v (nothing from either deleted collection)", got, after) } for _, it := range []*models.Item{hidden, granted} { if canOpen(it) { diff --git a/internal/server/bug3425_deleted_collection_reads_test.go b/internal/server/bug3425_deleted_collection_reads_test.go new file mode 100644 index 000000000..45b3997ce --- /dev/null +++ b/internal/server/bug3425_deleted_collection_reads_test.go @@ -0,0 +1,104 @@ +package server + +import ( + "net/http" + "strings" + "testing" + "time" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3425: the workspace graph builds its nodes from ListItems, so a +// soft-deleted collection's items showed up as nodes for an unrestricted +// viewer. The live task is the control. +func TestGraphExcludesItemsOfSoftDeletedCollections(t *testing.T) { + srv := testServer(t) + slug := createWSWithCollections(t, srv) + ws, err := srv.store.GetWorkspaceBySlug(slug) + if err != nil || ws == nil { + t.Fatalf("workspace: %v", err) + } + doomColl, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{Name: "Drafts", Slug: "drafts", Prefix: "DRF"}) + if err != nil { + t.Fatalf("create collection: %v", err) + } + + keep := createItem(t, srv, slug, "tasks", map[string]interface{}{ + "title": "Kept task", "fields": map[string]interface{}{"status": "open"}, + }) + doomed := createItem(t, srv, slug, "drafts", map[string]interface{}{"title": "Draft in a doomed collection"}) + createBlocksLink(t, srv, slug, keep.Slug, doomed.ID) + + before := getGraph(t, srv, slug, "") + if graphNode(before, keep.Ref) == nil || graphNode(before, doomed.Ref) == nil { + t.Fatalf("control: both nodes should be in the graph before the delete") + } + + if err := srv.store.DeleteCollection(doomColl.ID, ""); err != nil { + t.Fatalf("soft-delete collection: %v", err) + } + after := getGraph(t, srv, slug, "") + if graphNode(after, doomed.Ref) != nil { + t.Errorf("graph still has a node for an item of a soft-deleted collection") + } + if graphNode(after, keep.Ref) == nil { + t.Errorf("graph lost the live node") + } + if len(after.Edges) != 0 { + t.Errorf("graph still has an edge to the deleted collection's item: %+v", after.Edges) + } +} + +// The account data export keeps a user's items from collections they deleted, +// exactly as before BUG-3425 (it opts back in with IncludeDeletedCollections). +func TestExportAccount_KeepsItemsOfSoftDeletedCollections(t *testing.T) { + srv := testServer(t) + owner, err := srv.store.CreateUser(models.UserCreate{ + Email: "export-3425@example.com", Name: "Export Owner", Username: "export-3425", + Password: "pw-test-12345", + }) + if err != nil { + t.Fatalf("create owner: %v", err) + } + ws, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Export3425", OwnerID: owner.ID}) + if err != nil { + t.Fatalf("create workspace: %v", err) + } + if err := srv.store.AddWorkspaceMember(ws.ID, owner.ID, "owner"); err != nil { + t.Fatalf("add member: %v", err) + } + liveColl, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{Name: "Notes", Slug: "notes", Prefix: "NOT"}) + if err != nil { + t.Fatalf("create live collection: %v", err) + } + doomColl, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{Name: "Drafts", Slug: "drafts", Prefix: "DRF"}) + if err != nil { + t.Fatalf("create doomed collection: %v", err) + } + if _, err := srv.store.CreateItem(ws.ID, liveColl.ID, models.ItemCreate{Title: "Export live marker"}); err != nil { + t.Fatalf("create live item: %v", err) + } + if _, err := srv.store.CreateItem(ws.ID, doomColl.ID, models.ItemCreate{Title: "Export doomed marker"}); err != nil { + t.Fatalf("create doomed item: %v", err) + } + if err := srv.store.DeleteCollection(doomColl.ID, ""); err != nil { + t.Fatalf("soft-delete collection: %v", err) + } + sessTok, err := srv.store.CreateSession(owner.ID, "go-test", "192.0.2.1", "", 24*time.Hour) + if err != nil { + t.Fatalf("CreateSession: %v", err) + } + + rr := doRequestWithCookie(srv, "GET", "/api/v1/auth/export", nil, sessTok) + if rr.Code != http.StatusOK { + t.Fatalf("export: %d: %s", rr.Code, rr.Body.String()) + } + body := rr.Body.String() + if !strings.Contains(body, "Export live marker") { + t.Fatalf("control: the export lost the live item") + } + if !strings.Contains(body, "Export doomed marker") { + t.Errorf("the account export dropped an item of a soft-deleted collection") + } +} diff --git a/internal/server/handlers_account.go b/internal/server/handlers_account.go index b74196448..beb80ad73 100644 --- a/internal/server/handlers_account.go +++ b/internal/server/handlers_account.go @@ -391,7 +391,9 @@ func (s *Server) handleExportAccount(w http.ResponseWriter, r *http.Request) { wsData["collections"] = collections // Stream items per workspace (each workspace loaded individually, then GC'd) - items, err := s.store.ListItems(ws.ID, models.ItemListParams{IncludeArchived: true}) + // IncludeDeletedCollections: a user's own export keeps the items + // of collections they deleted, as it always has (BUG-3425). + items, err := s.store.ListItems(ws.ID, models.ItemListParams{IncludeArchived: true, IncludeDeletedCollections: true}) if err != nil { slog.Error("export: failed to list items", "workspace", ws.Slug, "error", err) wsData["items"] = []interface{}{} diff --git a/internal/store/agent_roles.go b/internal/store/agent_roles.go index e5bab56b3..5c417e177 100644 --- a/internal/store/agent_roles.go +++ b/internal/store/agent_roles.go @@ -62,12 +62,17 @@ func (s *Store) GetAgentRole(workspaceID, idOrSlug string) (*models.AgentRole, e } func (s *Store) ListAgentRoles(workspaceID string) ([]models.AgentRole, error) { + // item_count leaves out a soft-deleted collection's items: the role board + // lists its items through ListItems, which does (BUG-3425), and the count + // must agree. The check sits in the LEFT JOIN's ON so a role with no + // items still lists. (A comment inside this SQL broke it on Postgres.) rows, err := s.db.Query(s.q(` SELECT r.id, r.workspace_id, r.slug, r.name, r.description, r.icon, r.tools, r.sort_order, r.created_at, r.updated_at, COUNT(i.id) as item_count FROM agent_roles r LEFT JOIN items i ON i.agent_role_id = r.id AND i.deleted_at IS NULL + AND EXISTS (SELECT 1 FROM collections rc WHERE rc.id = i.collection_id AND rc.deleted_at IS NULL) WHERE r.workspace_id = ? GROUP BY r.id ORDER BY r.sort_order ASC, r.name ASC @@ -190,6 +195,7 @@ func (s *Store) GetRoleBreakdown(workspaceID string) ([]RoleBreakdown, error) { rows, err := s.db.Query(s.q(fmt.Sprintf(` SELECT i.agent_role_id, COUNT(*) as cnt, %s as users FROM items i + JOIN collections rc ON rc.id = i.collection_id AND rc.deleted_at IS NULL LEFT JOIN users u ON u.id = i.assigned_user_id WHERE i.workspace_id = ? AND i.deleted_at IS NULL AND NOT %s diff --git a/internal/store/bug3425_deleted_collection_reads_test.go b/internal/store/bug3425_deleted_collection_reads_test.go new file mode 100644 index 000000000..54958b60a --- /dev/null +++ b/internal/store/bug3425_deleted_collection_reads_test.go @@ -0,0 +1,258 @@ +package store + +import ( + "strings" + "testing" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// BUG-3425: soft-deleting a collection sets deleted_at on the collection row +// only, so its items stay live. ListItems, its FTS path and the graph's edges +// joined collections without checking c.deleted_at, so an unrestricted reader +// (who has no collection filter to catch it) kept seeing them. Every test +// keeps a live item beside the doomed one, so a query that returns nothing +// cannot pass. + +type bug3425World struct { + s *Store + ws *models.Workspace + liveColl, doomColl *models.Collection + live, doomed *models.Item +} + +func newBug3425World(t *testing.T) bug3425World { + t.Helper() + s := testStore(t) + ws := createTestWorkspace(t, s, "Deleted collection reads") + w := bug3425World{s: s, ws: ws} + w.liveColl = createTestCollection(t, s, ws.ID, "Notes") + w.doomColl = createTestCollection(t, s, ws.ID, "Drafts") + w.live = createTestItem(t, s, ws.ID, w.liveColl.ID, "Zephyr live row", "") + w.doomed = createTestItem(t, s, ws.ID, w.doomColl.ID, "Zephyr doomed row", "") + return w +} + +func (w bug3425World) deleteDoomed(t *testing.T) { + t.Helper() + if err := w.s.DeleteCollection(w.doomColl.ID, ""); err != nil { + t.Fatalf("soft-delete collection: %v", err) + } +} + +func hasItem(items []models.Item, id string) bool { + for _, it := range items { + if it.ID == id { + return true + } + } + return false +} + +func TestListItems_ExcludesSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + + before, err := w.s.ListItems(w.ws.ID, models.ItemListParams{}) + if err != nil { + t.Fatalf("ListItems before: %v", err) + } + if !hasItem(before, w.doomed.ID) || !hasItem(before, w.live.ID) { + t.Fatalf("control: both items should be listed before the delete") + } + + w.deleteDoomed(t) + after, err := w.s.ListItems(w.ws.ID, models.ItemListParams{}) + if err != nil { + t.Fatalf("ListItems after: %v", err) + } + if hasItem(after, w.doomed.ID) { + t.Errorf("ListItems still lists an item from a soft-deleted collection") + } + if !hasItem(after, w.live.ID) { + t.Errorf("ListItems lost the live item") + } + + // The account data export keeps exporting everything the user owns. + optOut, err := w.s.ListItems(w.ws.ID, models.ItemListParams{IncludeArchived: true, IncludeDeletedCollections: true}) + if err != nil { + t.Fatalf("ListItems opt-out: %v", err) + } + if !hasItem(optOut, w.doomed.ID) || !hasItem(optOut, w.live.ID) { + t.Errorf("IncludeDeletedCollections must keep the deleted collection's items") + } +} + +func TestListItemsSearch_ExcludesSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + search := func() []models.Item { + t.Helper() + got, err := w.s.ListItems(w.ws.ID, models.ItemListParams{Search: "Zephyr"}) + if err != nil { + t.Fatalf("ListItems search: %v", err) + } + return got + } + + if before := search(); !hasItem(before, w.doomed.ID) || !hasItem(before, w.live.ID) { + t.Fatalf("control: search should find both items before the delete") + } + w.deleteDoomed(t) + after := search() + if hasItem(after, w.doomed.ID) { + t.Errorf("search still finds an item from a soft-deleted collection") + } + if !hasItem(after, w.live.ID) { + t.Errorf("search lost the live item") + } + optOut, err := w.s.ListItems(w.ws.ID, models.ItemListParams{Search: "Zephyr", IncludeDeletedCollections: true}) + if err != nil { + t.Fatalf("ListItems search opt-out: %v", err) + } + if !hasItem(optOut, w.doomed.ID) { + t.Errorf("search ignores IncludeDeletedCollections") + } +} + +func TestListWorkspaceGraphLinks_ExcludesEdgesTouchingSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + s, ws := w.s, w.ws + partner := createTestItem(t, s, ws.ID, w.liveColl.ID, "Partner", "") + + link := func(src, tgt *models.Item, typ string) { + t.Helper() + if _, err := s.CreateItemLink(ws.ID, models.ItemLinkCreate{TargetID: tgt.ID, LinkType: typ}, src.ID); err != nil { + t.Fatalf("link %s -> %s: %v", src.Title, tgt.Title, err) + } + } + wiki := func(src, tgt *models.Item) { + t.Helper() + body := "See [[" + refOf(tgt) + "]]." + if _, err := s.UpdateItem(src.ID, models.ItemUpdate{Content: &body}); err != nil { + t.Fatalf("wiki %s -> %s: %v", src.Title, tgt.Title, err) + } + } + // Live control, then each edge shape touching the doomed collection from + // either end. + link(w.live, partner, "blocks") + link(w.doomed, partner, "related") + link(partner, w.doomed, "blocks") + wiki(w.live, partner) + wiki(w.doomed, w.live) + wiki(partner, w.doomed) + + touches := func(links []GraphLink, id string) []string { + var out []string + for _, l := range links { + if l.SourceID == id || l.TargetID == id { + out = append(out, l.Type) + } + } + return out + } + liveEdges := func(links []GraphLink) (blocks, wikiLink bool) { + for _, l := range links { + if l.SourceID == w.live.ID && l.TargetID == partner.ID { + blocks = blocks || l.Type == "blocks" + wikiLink = wikiLink || l.Type == "wiki-link" + } + } + return + } + + before, err := s.ListWorkspaceGraphLinks(ws.ID) + if err != nil { + t.Fatalf("graph links before: %v", err) + } + // Control: every doomed-collection edge shape is present before the + // delete (item_links both ways, wiki both ways). + if got := touches(before, w.doomed.ID); len(got) != 4 { + t.Fatalf("control: want 4 edges touching the doomed item before the delete, got %v", got) + } + + w.deleteDoomed(t) + after, err := s.ListWorkspaceGraphLinks(ws.ID) + if err != nil { + t.Fatalf("graph links after: %v", err) + } + if got := touches(after, w.doomed.ID); len(got) != 0 { + t.Errorf("graph still has edges touching a soft-deleted collection's item: %s", strings.Join(got, ", ")) + } + if b, wl := liveEdges(after); !b || !wl { + t.Errorf("graph lost a live edge: blocks=%v wiki-link=%v", b, wl) + } +} + +// The title-rename cascade deliberately still rewrites link text inside a +// soft-deleted collection's items, so a restored collection's links keep +// working. BUG-3425 must not narrow it. +func TestTitleRenameCascade_StillRewritesSoftDeletedCollectionSources(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + body := "See [[Zephyr live row]]." + if _, err := w.s.UpdateItem(w.doomed.ID, models.ItemUpdate{Content: &body}); err != nil { + t.Fatalf("seed link: %v", err) + } + w.deleteDoomed(t) + + title := "Zephyr renamed row" + if _, err := w.s.UpdateItem(w.live.ID, models.ItemUpdate{Title: &title}); err != nil { + t.Fatalf("rename target: %v", err) + } + got, err := w.s.GetItem(w.doomed.ID) + if err != nil || got == nil { + t.Fatalf("read source: %v", err) + } + if !strings.Contains(got.Content, "[[Zephyr renamed row]]") { + t.Errorf("cascade no longer rewrites a soft-deleted collection's source: %q", got.Content) + } +} + +// Codex r1: the role lanes list their items through ListItems, so the role +// COUNTS must leave out a soft-deleted collection's items too, or a lane +// reports more items than it shows. +func TestRoleCounts_ExcludeSoftDeletedCollections(t *testing.T) { + t.Parallel() + w := newBug3425World(t) + role, err := w.s.CreateAgentRole(w.ws.ID, models.AgentRoleCreate{Name: "Implementer"}) + if err != nil { + t.Fatalf("create role: %v", err) + } + for _, it := range []*models.Item{w.live, w.doomed} { + if _, err := w.s.UpdateItem(it.ID, models.ItemUpdate{AgentRoleID: &role.ID}); err != nil { + t.Fatalf("assign role to %s: %v", it.Title, err) + } + } + counts := func() (listed, breakdown int) { + t.Helper() + roles, err := w.s.ListAgentRoles(w.ws.ID) + if err != nil { + t.Fatalf("ListAgentRoles: %v", err) + } + for _, r := range roles { + if r.ID == role.ID { + listed = r.ItemCount + } + } + rb, err := w.s.GetRoleBreakdown(w.ws.ID) + if err != nil { + t.Fatalf("GetRoleBreakdown: %v", err) + } + for _, b := range rb { + if b.RoleID != nil && *b.RoleID == role.ID { + breakdown = b.ItemCount + } + } + return + } + + if l, b := counts(); l != 2 || b != 2 { + t.Fatalf("control: want 2 and 2 before the delete, got ListAgentRoles=%d GetRoleBreakdown=%d", l, b) + } + w.deleteDoomed(t) + if l, b := counts(); l != 1 || b != 1 { + t.Errorf("after the delete: want 1 and 1 (the live item only), got ListAgentRoles=%d GetRoleBreakdown=%d", l, b) + } +} diff --git a/internal/store/export_soft_deleted_collection_test.go b/internal/store/export_soft_deleted_collection_test.go index b1a3a4bc9..78cc6a7f9 100644 --- a/internal/store/export_soft_deleted_collection_test.go +++ b/internal/store/export_soft_deleted_collection_test.go @@ -135,7 +135,8 @@ func TestRoundTripPreservesItemsUnderSoftDeletedCollection(t *testing.T) { t.Fatalf("ImportWorkspace: %v", err) } - items, err := s.ListItems(imported.ID, models.ItemListParams{}) + // The collection is soft-deleted on both ends, so readers hide its items (BUG-3425); this asserts storage. + items, err := s.ListItems(imported.ID, models.ItemListParams{IncludeDeletedCollections: true}) if err != nil { t.Fatalf("ListItems: %v", err) } @@ -215,7 +216,8 @@ func TestRoundTripPreservesDependentsUnderSoftDeletedCollection(t *testing.T) { if err != nil { t.Fatalf("ImportWorkspace: %v", err) } - items, err := s.ListItems(imported.ID, models.ItemListParams{}) + // The collection is soft-deleted on both ends, so readers hide its items (BUG-3425); this asserts storage. + items, err := s.ListItems(imported.ID, models.ItemListParams{IncludeDeletedCollections: true}) if err != nil { t.Fatalf("ListItems: %v", err) } diff --git a/internal/store/graph.go b/internal/store/graph.go index dff137c58..7ece536e1 100644 --- a/internal/store/graph.go +++ b/internal/store/graph.go @@ -48,6 +48,8 @@ func graphEdgeType(linkType string) string { // Both queries exclude edges whose source or target item is soft-deleted, // matching GetParentMap's posture (BUG-734) — a dangling edge to an // archived item would render as a node the items query never returned. +// For the same reason they exclude an endpoint whose COLLECTION is +// soft-deleted, since ListItems leaves those items out (BUG-3425). // // Wiki-link rows are deduplicated per (source, target) pair — an item // that mentions [[TASK-5]] three times is still one edge — and @@ -62,6 +64,8 @@ func (s *Store) ListWorkspaceGraphLinks(workspaceID string) ([]GraphLink, error) FROM item_links il JOIN items src ON src.id = il.source_id AND src.deleted_at IS NULL JOIN items tgt ON tgt.id = il.target_id AND tgt.deleted_at IS NULL + JOIN collections sc ON sc.id = src.collection_id AND sc.deleted_at IS NULL + JOIN collections tc ON tc.id = tgt.collection_id AND tc.deleted_at IS NULL WHERE il.workspace_id = ? `), workspaceID) if err != nil { @@ -89,6 +93,8 @@ func (s *Store) ListWorkspaceGraphLinks(workspaceID string) ([]GraphLink, error) FROM item_wiki_links wl JOIN items src ON src.id = wl.source_item_id AND src.deleted_at IS NULL JOIN items tgt ON tgt.id = wl.target_item_id AND tgt.deleted_at IS NULL + JOIN collections sc ON sc.id = src.collection_id AND sc.deleted_at IS NULL + JOIN collections tc ON tc.id = tgt.collection_id AND tc.deleted_at IS NULL WHERE src.workspace_id = ? AND wl.target_item_id IS NOT NULL AND wl.target_workspace_id IS NULL diff --git a/internal/store/items.go b/internal/store/items.go index 8a33b8b1f..a72a48847 100644 --- a/internal/store/items.go +++ b/internal/store/items.go @@ -1299,6 +1299,11 @@ func (s *Store) ListItems(workspaceID string, params models.ItemListParams) ([]m if !params.IncludeArchived { query += " AND i.deleted_at IS NULL" } + // A soft-deleted collection leaves its items live (BUG-3425): every + // reader leaves them out unless it opts in. + if !params.IncludeDeletedCollections { + query += " AND c.deleted_at IS NULL" + } if params.CollectionSlug != "" { query += " AND c.slug = ?" @@ -2076,6 +2081,10 @@ func (s *Store) listItemsFTS(workspaceID string, params models.ItemListParams) ( // the OR-combined plainto_tsquery in the dialect (BUG-842). args = []interface{}{workspaceID, sanitizeFTSQuery(params.Search)} } + // As in ListItems (BUG-3425). + if !params.IncludeDeletedCollections { + query += " AND c.deleted_at IS NULL" + } if params.CollectionSlug != "" { query += " AND c.slug = ?" @@ -4653,11 +4662,11 @@ func (s *Store) childrenDoneFiltersForParent(parentItemID string) []collectionDo // breakdowns) that need to evaluate "is done?" for every item regardless // of which collection it belongs to. // -// Includes soft-deleted collections: callers (e.g. GetRoleBreakdown) -// count items in the workspace without filtering by collection -// deleted_at, so excluding soft-deleted collections here would leave -// their items without a matching per-collection clause and cause them -// to always register as non-terminal. +// Includes soft-deleted collections, so that a caller counting items +// without a collection deleted_at filter still finds a per-collection +// clause for every item (an item with none would always register as +// non-terminal). GetRoleBreakdown now excludes those items itself +// (BUG-3425); keeping them here is harmless for it. func (s *Store) doneFiltersForWorkspace(workspaceID string) []collectionDoneFilter { rows, err := s.db.Query( s.q(`SELECT id, schema, settings FROM collections WHERE workspace_id = ?`),