fix(backend): return real results from Service.GetBlocking (#2015)
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 <agent@kandev.dev> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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{})
|
||||
|
||||
Reference in New Issue
Block a user