fix(storage): persist close_reason to issues table on close (#551)

CloseIssue was storing the reason only in the events table, not in the
issues.close_reason column. This caused `bd show --json` to return an
empty close_reason even when one was provided.

- Update CloseIssue in queries.go and transaction.go to set close_reason
- Clear close_reason when reopening issues (in manageClosedAt)
- Add tests for close_reason in storage and CLI JSON output
- Document the dual-storage of close_reason (issues + events tables)
This commit is contained in:
cbro
2025-12-14 17:18:01 -05:00
committed by GitHub
parent 3a4da4e08d
commit 2651620a4c
5 changed files with 31 additions and 7 deletions

View File

@@ -333,6 +333,9 @@ func TestCLI_Close(t *testing.T) {
if closed[0]["status"] != "closed" { if closed[0]["status"] != "closed" {
t.Errorf("Expected status 'closed', got: %v", closed[0]["status"]) t.Errorf("Expected status 'closed', got: %v", closed[0]["status"])
} }
if closed[0]["close_reason"] != "Done" {
t.Errorf("Expected close_reason 'Done', got: %v", closed[0]["close_reason"])
}
} }
func TestCLI_DepAdd(t *testing.T) { func TestCLI_DepAdd(t *testing.T) {

View File

@@ -500,10 +500,13 @@ func manageClosedAt(oldIssue *types.Issue, updates map[string]interface{}, setCl
setClauses = append(setClauses, "closed_at = ?") setClauses = append(setClauses, "closed_at = ?")
args = append(args, now) args = append(args, now)
} else if oldIssue.Status == types.StatusClosed { } else if oldIssue.Status == types.StatusClosed {
// Changing from closed to something else: clear closed_at // Changing from closed to something else: clear closed_at and close_reason
updates["closed_at"] = nil updates["closed_at"] = nil
setClauses = append(setClauses, "closed_at = ?") setClauses = append(setClauses, "closed_at = ?")
args = append(args, nil) args = append(args, nil)
updates["close_reason"] = ""
setClauses = append(setClauses, "close_reason = ?")
args = append(args, "")
} }
return setClauses, args return setClauses, args
@@ -806,10 +809,14 @@ func (s *SQLiteStorage) CloseIssue(ctx context.Context, id string, reason string
} }
defer func() { _ = tx.Rollback() }() defer func() { _ = tx.Rollback() }()
// NOTE: close_reason is stored in two places:
// 1. issues.close_reason - for direct queries (bd show --json, exports)
// 2. events.comment - for audit history (when was it closed, by whom)
// Keep both in sync. If refactoring, consider deriving one from the other.
result, err := tx.ExecContext(ctx, ` result, err := tx.ExecContext(ctx, `
UPDATE issues SET status = ?, closed_at = ?, updated_at = ? UPDATE issues SET status = ?, closed_at = ?, updated_at = ?, close_reason = ?
WHERE id = ? WHERE id = ?
`, types.StatusClosed, now, now, id) `, types.StatusClosed, now, now, reason, id)
if err != nil { if err != nil {
return fmt.Errorf("failed to close issue: %w", err) return fmt.Errorf("failed to close issue: %w", err)
} }

View File

@@ -706,6 +706,10 @@ func TestCloseIssue(t *testing.T) {
if closed.ClosedAt == nil { if closed.ClosedAt == nil {
t.Error("ClosedAt should be set") t.Error("ClosedAt should be set")
} }
if closed.CloseReason != "Done" {
t.Errorf("CloseReason not set: got %q, want %q", closed.CloseReason, "Done")
}
} }
func TestClosedAtInvariant(t *testing.T) { func TestClosedAtInvariant(t *testing.T) {
@@ -766,7 +770,7 @@ func TestClosedAtInvariant(t *testing.T) {
t.Fatalf("CloseIssue failed: %v", err) t.Fatalf("CloseIssue failed: %v", err)
} }
// Verify it's closed with closed_at set // Verify it's closed with closed_at and close_reason set
closed, err := store.GetIssue(ctx, issue.ID) closed, err := store.GetIssue(ctx, issue.ID)
if err != nil { if err != nil {
t.Fatalf("GetIssue failed: %v", err) t.Fatalf("GetIssue failed: %v", err)
@@ -774,6 +778,9 @@ func TestClosedAtInvariant(t *testing.T) {
if closed.ClosedAt == nil { if closed.ClosedAt == nil {
t.Fatal("ClosedAt should be set after closing") t.Fatal("ClosedAt should be set after closing")
} }
if closed.CloseReason != "Done" {
t.Errorf("CloseReason should be 'Done', got %q", closed.CloseReason)
}
// Reopen the issue // Reopen the issue
updates := map[string]interface{}{ updates := map[string]interface{}{
@@ -784,7 +791,7 @@ func TestClosedAtInvariant(t *testing.T) {
t.Fatalf("UpdateIssue failed: %v", err) t.Fatalf("UpdateIssue failed: %v", err)
} }
// Verify closed_at was cleared // Verify closed_at and close_reason were cleared
reopened, err := store.GetIssue(ctx, issue.ID) reopened, err := store.GetIssue(ctx, issue.ID)
if err != nil { if err != nil {
t.Fatalf("GetIssue failed: %v", err) t.Fatalf("GetIssue failed: %v", err)
@@ -795,6 +802,9 @@ func TestClosedAtInvariant(t *testing.T) {
if reopened.ClosedAt != nil { if reopened.ClosedAt != nil {
t.Error("ClosedAt should be cleared when reopening issue") t.Error("ClosedAt should be cleared when reopening issue")
} }
if reopened.CloseReason != "" {
t.Errorf("CloseReason should be cleared when reopening issue, got %q", reopened.CloseReason)
}
}) })
t.Run("CreateIssue rejects closed issue without closed_at", func(t *testing.T) { t.Run("CreateIssue rejects closed issue without closed_at", func(t *testing.T) {

View File

@@ -462,13 +462,14 @@ func applyUpdatesToIssue(issue *types.Issue, updates map[string]interface{}) {
} }
// CloseIssue closes an issue within the transaction. // CloseIssue closes an issue within the transaction.
// NOTE: close_reason is stored in both issues table and events table - see SQLiteStorage.CloseIssue.
func (t *sqliteTxStorage) CloseIssue(ctx context.Context, id string, reason string, actor string) error { func (t *sqliteTxStorage) CloseIssue(ctx context.Context, id string, reason string, actor string) error {
now := time.Now() now := time.Now()
result, err := t.conn.ExecContext(ctx, ` result, err := t.conn.ExecContext(ctx, `
UPDATE issues SET status = ?, closed_at = ?, updated_at = ? UPDATE issues SET status = ?, closed_at = ?, updated_at = ?, close_reason = ?
WHERE id = ? WHERE id = ?
`, types.StatusClosed, now, now, id) `, types.StatusClosed, now, now, reason, id)
if err != nil { if err != nil {
return fmt.Errorf("failed to close issue: %w", err) return fmt.Errorf("failed to close issue: %w", err)
} }

View File

@@ -268,6 +268,9 @@ func TestTransactionCloseIssue(t *testing.T) {
if closed.Status != types.StatusClosed { if closed.Status != types.StatusClosed {
t.Errorf("expected status 'closed', got %q", closed.Status) t.Errorf("expected status 'closed', got %q", closed.Status)
} }
if closed.CloseReason != "Done" {
t.Errorf("expected close_reason 'Done', got %q", closed.CloseReason)
}
} }
// TestTransactionDeleteIssue tests deleting an issue within a transaction. // TestTransactionDeleteIssue tests deleting an issue within a transaction.