From 60040a71f1f92b7adbe99d0aa9c8d480de5794b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Tue, 28 Jul 2026 23:03:43 +0100 Subject: [PATCH] fix(backend): return real results from Service.GetBlocking (#2015) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetBlocking unconditionally returned an empty slice behind a TODO, so any caller asking "which tasks does this task block?" silently got a wrong answer. The reverse-lookup query the TODO asked for already exists on the blocker repository as ListTasksBlockedBy (used by the office scheduler and the handoff context builder) and is already part of the BlockerRepository interface, so GetBlocking now delegates to it. Like GetBlockers, it reports the raw blocker edges without filtering archived tasks — every other ListTasksBlockedBy consumer does the same. ListTasksBlockedBy also gains a deterministic ORDER BY (mirroring ListTaskBlockers) and normalizes an empty result to a non-nil slice. Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 5 (1M context) --- .../office/repository/sqlite/blockers.go | 8 +- .../office/repository/sqlite/blockers_test.go | 65 ++++++++++ .../internal/task/service/service_office.go | 15 ++- .../task/service/service_office_test.go | 118 ++++++++++++++++++ 4 files changed, 200 insertions(+), 6 deletions(-) diff --git a/apps/backend/internal/office/repository/sqlite/blockers.go b/apps/backend/internal/office/repository/sqlite/blockers.go index 03cfbbd445..f7f14899a2 100644 --- a/apps/backend/internal/office/repository/sqlite/blockers.go +++ b/apps/backend/internal/office/repository/sqlite/blockers.go @@ -40,13 +40,19 @@ func (r *Repository) DeleteTaskBlocker(ctx context.Context, taskID, blockerTaskI } // ListTasksBlockedBy returns task IDs that are blocked by the given task. +// This is the reverse direction of ListTaskBlockers; results are ordered by +// insertion time so callers see a stable list. func (r *Repository) ListTasksBlockedBy(ctx context.Context, blockerTaskID string) ([]string, error) { var ids []string err := r.ro.SelectContext(ctx, &ids, r.ro.Rebind( - `SELECT task_id FROM task_blockers WHERE blocker_task_id = ?`), blockerTaskID) + `SELECT task_id FROM task_blockers WHERE blocker_task_id = ? ORDER BY created_at, task_id`), + blockerTaskID) if err != nil { return nil, err } + if ids == nil { + ids = []string{} + } return ids, nil } diff --git a/apps/backend/internal/office/repository/sqlite/blockers_test.go b/apps/backend/internal/office/repository/sqlite/blockers_test.go index 75724e6b4e..bede188bca 100644 --- a/apps/backend/internal/office/repository/sqlite/blockers_test.go +++ b/apps/backend/internal/office/repository/sqlite/blockers_test.go @@ -2,6 +2,7 @@ package sqlite_test import ( "context" + "slices" "testing" "github.com/kandev/kandev/internal/office/models" @@ -39,6 +40,70 @@ func TestTaskBlocker_CRUD(t *testing.T) { } } +func TestListTasksBlockedBy(t *testing.T) { + repo := newTestRepo(t) + ctx := context.Background() + + // No edges yet: empty, non-nil result. + ids, err := repo.ListTasksBlockedBy(ctx, "task-3") + if err != nil { + t.Fatalf("ListTasksBlockedBy: %v", err) + } + if ids == nil { + t.Error("expected non-nil empty slice, got nil") + } + if len(ids) != 0 { + t.Errorf("expected no blocked tasks, got %v", ids) + } + + // task-1 and task-2 are blocked by task-3; task-2 is also blocked by task-4. + for _, b := range []*models.TaskBlocker{ + {TaskID: "task-1", BlockerTaskID: "task-3"}, + {TaskID: "task-2", BlockerTaskID: "task-3"}, + {TaskID: "task-2", BlockerTaskID: "task-4"}, + } { + if err := repo.CreateTaskBlocker(ctx, b); err != nil { + t.Fatalf("create blocker: %v", err) + } + } + + // One blocked task. + ids, err = repo.ListTasksBlockedBy(ctx, "task-4") + if err != nil { + t.Fatalf("ListTasksBlockedBy: %v", err) + } + if !slices.Equal(ids, []string{"task-2"}) { + t.Errorf("blocked by task-4 = %v, want [task-2]", ids) + } + + // Several blocked tasks, ordered by insertion time. + ids, err = repo.ListTasksBlockedBy(ctx, "task-3") + if err != nil { + t.Fatalf("ListTasksBlockedBy: %v", err) + } + if !slices.Equal(ids, []string{"task-1", "task-2"}) { + t.Errorf("blocked by task-3 = %v, want [task-1 task-2]", ids) + } + + // A task that only appears on the blocked side blocks nothing. + ids, err = repo.ListTasksBlockedBy(ctx, "task-1") + if err != nil { + t.Fatalf("ListTasksBlockedBy: %v", err) + } + if len(ids) != 0 { + t.Errorf("blocked by task-1 = %v, want none", ids) + } + + // Deleting an edge removes it from the reverse lookup. + if err := repo.DeleteTaskBlocker(ctx, "task-1", "task-3"); err != nil { + t.Fatalf("DeleteTaskBlocker: %v", err) + } + ids, _ = repo.ListTasksBlockedBy(ctx, "task-3") + if !slices.Equal(ids, []string{"task-2"}) { + t.Errorf("blocked by task-3 after delete = %v, want [task-2]", ids) + } +} + func TestGetChildSummaries(t *testing.T) { repo := newSearchTestRepo(t) ctx := context.Background() diff --git a/apps/backend/internal/task/service/service_office.go b/apps/backend/internal/task/service/service_office.go index 955ac39d64..dd8e629625 100644 --- a/apps/backend/internal/task/service/service_office.go +++ b/apps/backend/internal/task/service/service_office.go @@ -147,15 +147,20 @@ func (s *Service) GetBlockers(ctx context.Context, taskID string) ([]string, err // GetBlocking returns all task IDs that the given task is blocking. // This is the reverse lookup: find all tasks where blockerTaskID = taskID. +// Like GetBlockers, it returns the raw blocker edges: archived tasks are not +// filtered out, so callers that only want active tasks must filter themselves. func (s *Service) GetBlocking(ctx context.Context, taskID string) ([]string, error) { if s.blockers == nil { return nil, fmt.Errorf("blocker repository not configured") } - // We need to search across all tasks. For now, we use a pragmatic approach: - // query all tasks and check. This will be optimized with a dedicated query later. - // For the initial implementation, we return an empty list. - // TODO: Add a GetBlockingTasks query to the blocker repository - return []string{}, nil + ids, err := s.blockers.ListTasksBlockedBy(ctx, taskID) + if err != nil { + return nil, err + } + if ids == nil { + return []string{}, nil + } + return ids, nil } // checkCircularBlocker checks if adding blockerTaskID as a blocker to taskID diff --git a/apps/backend/internal/task/service/service_office_test.go b/apps/backend/internal/task/service/service_office_test.go index b2d337f203..b514c1709f 100644 --- a/apps/backend/internal/task/service/service_office_test.go +++ b/apps/backend/internal/task/service/service_office_test.go @@ -3,6 +3,8 @@ package service import ( "context" "database/sql" + "slices" + "sort" "strings" "testing" @@ -372,6 +374,122 @@ func TestBlocker_CRUD(t *testing.T) { } } +func TestGetBlocking_ReverseLookup(t *testing.T) { + svc, _, _ := createTestService(t) + svc.SetBlockerRepository(&mockBlockerRepo{}) + ctx := context.Background() + + // No blocker edges at all. + ids, err := svc.GetBlocking(ctx, "task-1") + if err != nil { + t.Fatalf("GetBlocking: %v", err) + } + if ids == nil { + t.Error("expected non-nil empty slice, got nil") + } + if len(ids) != 0 { + t.Errorf("expected no blocked tasks, got %v", ids) + } + + // One blocked task: task-2 is blocked by task-1. + if err := svc.AddBlocker(ctx, "task-2", "task-1"); err != nil { + t.Fatalf("AddBlocker: %v", err) + } + ids, err = svc.GetBlocking(ctx, "task-1") + if err != nil { + t.Fatalf("GetBlocking: %v", err) + } + if len(ids) != 1 || ids[0] != "task-2" { + t.Errorf("expected [task-2], got %v", ids) + } + + // The reverse direction must not be confused with the forward one: + // task-2 blocks nothing, and task-1 is the blocker of task-2. + if ids, err = svc.GetBlocking(ctx, "task-2"); err != nil || len(ids) != 0 { + t.Errorf("GetBlocking(task-2) = %v, %v; want empty", ids, err) + } + if ids, err = svc.GetBlockers(ctx, "task-2"); err != nil || len(ids) != 1 || ids[0] != "task-1" { + t.Errorf("GetBlockers(task-2) = %v, %v; want [task-1]", ids, err) + } + + // Several blocked tasks. + for _, blocked := range []string{"task-3", "task-4"} { + if err := svc.AddBlocker(ctx, blocked, "task-1"); err != nil { + t.Fatalf("AddBlocker(%s): %v", blocked, err) + } + } + ids, err = svc.GetBlocking(ctx, "task-1") + if err != nil { + t.Fatalf("GetBlocking: %v", err) + } + sort.Strings(ids) + want := []string{"task-2", "task-3", "task-4"} + if !slices.Equal(ids, want) { + t.Errorf("expected %v, got %v", want, ids) + } + + // Removing an edge removes it from the reverse lookup too. + if err := svc.RemoveBlocker(ctx, "task-3", "task-1"); err != nil { + t.Fatalf("RemoveBlocker: %v", err) + } + ids, _ = svc.GetBlocking(ctx, "task-1") + sort.Strings(ids) + if !slices.Equal(ids, []string{"task-2", "task-4"}) { + t.Errorf("expected [task-2 task-4] after removal, got %v", ids) + } +} + +func TestGetBlocking_NoRepositoryConfigured(t *testing.T) { + svc, _, _ := createTestService(t) + ctx := context.Background() + + if _, err := svc.GetBlocking(ctx, "task-1"); err == nil { + t.Fatal("expected error when blocker repository is not configured") + } +} + +// Archiving a task does not drop it from the reverse lookup: GetBlocking +// mirrors GetBlockers and reports the raw blocker edges. Callers that only +// want active tasks filter themselves. +func TestGetBlocking_IncludesArchivedTasks(t *testing.T) { + svc, repo := setupOfficeTest(t) + svc.SetBlockerRepository(&mockBlockerRepo{}) + ctx := context.Background() + + blocker, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", Title: "Blocker", ProjectID: "proj-1", + }) + if err != nil { + t.Fatalf("create blocker: %v", err) + } + blocked, err := svc.CreateTask(ctx, &CreateTaskRequest{ + WorkspaceID: "ws-1", Title: "Blocked", ProjectID: "proj-1", + BlockedBy: []string{blocker.ID}, + }) + if err != nil { + t.Fatalf("create blocked task: %v", err) + } + + ids, err := svc.GetBlocking(ctx, blocker.ID) + if err != nil { + t.Fatalf("GetBlocking: %v", err) + } + if len(ids) != 1 || ids[0] != blocked.ID { + t.Fatalf("expected [%s], got %v", blocked.ID, ids) + } + + if err := repo.ArchiveTask(ctx, blocked.ID); err != nil { + t.Fatalf("ArchiveTask: %v", err) + } + ids, err = svc.GetBlocking(ctx, blocker.ID) + if err != nil { + t.Fatalf("GetBlocking after archive: %v", err) + } + if len(ids) != 1 || ids[0] != blocked.ID { + t.Errorf("expected archived task %s to still be reported, got %v", blocked.ID, ids) + } +} + func TestBlocker_CircularDetection(t *testing.T) { svc, _, _ := createTestService(t) svc.SetBlockerRepository(&mockBlockerRepo{})