fix(jira): select open ticket before closing

Codex-Session: codex resume 01a1c1de-e73c-7622-949e-fbbbd741b6c5

pkg/reporting/trackers/jira/jira.go

@@ -7,7 +7,6 @@ import (
 	"net/http"
 	"net/url"
 	"strings"
-	"sync"
 	"text/template"
 
 	"github.com/andygrunwald/go-jira"
@@ -171,9 +170,6 @@ type Integration struct {
 	Formatter
 	jira    *jira.Client
 	options *Options
-
-	once         *sync.Once
-	transitionID string
 }
 
 // Options contains the configuration options for jira client
@@ -256,7 +252,6 @@ func New(options *Options) (*Integration, error) {
 	integration := &Integration{
 		jira:    jiraClient,
 		options: options,
-		once:    &sync.Once{},
 	}
 	return integration, nil
 }
@@ -404,40 +399,50 @@ func (i *Integration) CloseIssue(event *output.ResultEvent) error {
 		return nil
 	}
 
-	issue, err := i.FindExistingIssue(event, false)
+	issue, err := i.FindExistingIssue(event, true)
 	if err != nil {
 		return err
-	} else if issue.ID != "" {
-		// Lazy load the transitions ID in case it's not set.
-		// StatusNot is a target status name (matches JQL usage below and the
-		// field's semantics); find the transition that moves to that status.
-		i.once.Do(func() {
-			transitions, _, err := i.jira.Issue.GetTransitions(issue.ID)
-			if err != nil {
-				return
-			}
-			for _, transition := range transitions {
-				if transition.To.Name == i.options.StatusNot {
-					i.transitionID = transition.ID
-					break
-				}
-			}
-		})
-		if i.transitionID == "" {
-			return nil
-		}
-		transition := jira.CreateTransitionPayload{
-			Transition: jira.TransitionPayload{
-				ID: i.transitionID,
-			},
-		}
-
-		_, err = i.jira.Issue.DoTransitionWithPayload(issue.ID, transition)
-		if err != nil {
-			return err
-		}
 	}
-	return nil
+	if issue.ID == "" {
+		return nil
+	}
+	return i.CloseIssueByID(issue.ID)
+}
+
+// CloseIssueByID transitions a known Jira issue to the configured target status.
+func (i *Integration) CloseIssueByID(issueID string) error {
+	if i.options.StatusNot == "" {
+		return nil
+	}
+	if issueID == "" {
+		return fmt.Errorf("jira issue ID is required")
+	}
+
+	transitions, _, err := i.jira.Issue.GetTransitions(issueID)
+	if err != nil {
+		return fmt.Errorf("get Jira issue %s transitions: %w", issueID, err)
+	}
+	for _, transition := range transitions {
+		if transition.To.Name != i.options.StatusNot {
+			continue
+		}
+		_, err = i.jira.Issue.DoTransitionWithPayload(issueID, jira.CreateTransitionPayload{
+			Transition: jira.TransitionPayload{ID: transition.ID},
+		})
+		if err != nil {
+			return fmt.Errorf("transition Jira issue %s: %w", issueID, err)
+		}
+		return nil
+	}
+
+	currentIssue, _, err := i.jira.Issue.Get(issueID, nil)
+	if err != nil {
+		return fmt.Errorf("get Jira issue %s status: %w", issueID, err)
+	}
+	if currentIssue.Fields != nil && currentIssue.Fields.Status != nil && currentIssue.Fields.Status.Name == i.options.StatusNot {
+		return nil
+	}
+	return fmt.Errorf("jira issue %s has no transition to %q", issueID, i.options.StatusNot)
 }
 
 // FindExistingIssue checks if the issue already exists and returns its ID
@@ -495,12 +500,12 @@ func (i *Integration) FindExistingIssue(event *output.ResultEvent, useStatus boo
 		}, nil
 	}
 
-	searchOptions := &jira.SearchOptionsV2{
+	searchOptions := &jira.SearchOptions{
 		MaxResults: 1, // if any issue exists, then we won't create a new one
 		Fields:     []string{"summary", "description", "issuetype", "status", "priority", "project"},
 	}
 
-	issues, resp, err := i.jira.Issue.SearchV2JQL(jql, searchOptions)
+	issues, resp, err := i.jira.Issue.Search(jql, searchOptions)
 	if err != nil {
 		var data string
 		if resp != nil && resp.Body != nil {
@@ -510,7 +515,7 @@ func (i *Integration) FindExistingIssue(event *output.ResultEvent, useStatus boo
 		return jira.Issue{}, fmt.Errorf("%w => %s", err, data)
 	}
 
-	switch resp.Total {
+	switch len(issues) {
 	case 0:
 		return jira.Issue{}, nil
 	case 1:

pkg/reporting/trackers/jira/jira_test.go

@@ -1,7 +1,9 @@
 package jira
 
 import (
+	"fmt"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"strings"
 	"testing"
@@ -15,6 +17,188 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+func TestCloseIssueUsesServerSearchResult(t *testing.T) {
+	transitionCalls := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search"):
+			_, _ = w.Write([]byte(`{"issues":[{"id":"10001","key":"TEST-1","fields":{"status":{"name":"Open"}}}]}`))
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search/jql"):
+			http.Error(w, "", http.StatusForbidden)
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/transitions"):
+			_, _ = w.Write([]byte(`{"transitions":[{"id":"5","to":{"name":"Done"}}]}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/transitions"):
+			transitionCalls++
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			http.Error(w, "unexpected endpoint", http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+
+	integration, err := New(&Options{URL: server.URL, AccountID: "test", Token: "test", ProjectName: "TEST", StatusNot: "Done"})
+	require.NoError(t, err)
+	err = integration.CloseIssue(&output.ResultEvent{TemplateID: "pdcp-verify", Host: "example.test", Info: model.Info{Name: "Test issue"}})
+	require.NoError(t, err)
+	require.Equal(t, 1, transitionCalls)
+}
+
+func TestCloseIssueFindsOpenIssueWhenOlderMatchIsDone(t *testing.T) {
+	transitioned := ""
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search"):
+			if strings.Contains(r.URL.Query().Get("jql"), `status != "Done"`) {
+				_, _ = w.Write([]byte(`{"issues":[{"id":"10002","key":"TEST-2"}]}`))
+			} else {
+				_, _ = w.Write([]byte(`{"issues":[{"id":"10001","key":"TEST-1"}]}`))
+			}
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/issue/10001/transitions"):
+			_, _ = w.Write([]byte(`{"transitions":[]}`))
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/issue/10001"):
+			_, _ = w.Write([]byte(`{"fields":{"status":{"name":"Done"}}}`))
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/issue/10002/transitions"):
+			_, _ = w.Write([]byte(`{"transitions":[{"id":"5","to":{"name":"Done"}}]}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/issue/10002/transitions"):
+			transitioned = "10002"
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			http.Error(w, "unexpected endpoint", http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+
+	integration, err := New(&Options{URL: server.URL, AccountID: "test", Token: "test", ProjectName: "TEST", StatusNot: "Done"})
+	require.NoError(t, err)
+	err = integration.CloseIssue(&output.ResultEvent{TemplateID: "pdcp-verify", Host: "example.test", Info: model.Info{Name: "Test issue"}})
+	require.NoError(t, err)
+	require.Equal(t, "10002", transitioned)
+}
+
+func TestCloseIssueSearchesConfiguredProjectWhenEventHasAnotherJiraID(t *testing.T) {
+	searchCalls, transitionCalls := 0, 0
+	searchJQL := ""
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search"):
+			searchCalls++
+			searchJQL = r.URL.Query().Get("jql")
+			_, _ = w.Write([]byte(`{"issues":[{"id":"123","key":"TEST-1","fields":{"status":{"name":"Open"}}}]}`))
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search/jql"):
+			http.Error(w, "", http.StatusForbidden)
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/issue/123/transitions"):
+			_, _ = w.Write([]byte(`{"transitions":[{"id":"5","to":{"name":"Done"}}]}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/issue/123/transitions"):
+			transitionCalls++
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			http.Error(w, "unexpected endpoint", http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+
+	integration, err := New(&Options{URL: server.URL, AccountID: "test", Token: "test", ProjectName: "TEST", StatusNot: "Done"})
+	require.NoError(t, err)
+	err = integration.CloseIssue(&output.ResultEvent{
+		TemplateID: "pdcp-verify",
+		Host:       "example.test",
+		IssueTrackers: map[string]output.IssueTrackerMetadata{
+			"jira": {IssueID: "10001", IssueURL: server.URL + "/browse/OTHER-1"},
+		},
+	})
+	require.NoError(t, err)
+	require.Equal(t, 1, searchCalls)
+	require.Contains(t, searchJQL, `project = "TEST"`)
+	require.Equal(t, 1, transitionCalls)
+}
+
+func TestCloseIssueByIDTargetsOnlyCreatedTicket(t *testing.T) {
+	searchCalls, transitionCalls := 0, 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && strings.Contains(r.URL.Path, "/search"):
+			searchCalls++
+			http.Error(w, "", http.StatusForbidden)
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/issue/10001/transitions"):
+			_, _ = w.Write([]byte(`{"transitions":[{"id":"5","to":{"name":"Done"}}]}`))
+		case r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, "/issue/10001/transitions"):
+			transitionCalls++
+			w.WriteHeader(http.StatusNoContent)
+		default:
+			http.Error(w, "unexpected endpoint", http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+
+	integration, err := New(&Options{URL: server.URL, AccountID: "test", Token: "test", ProjectName: "TEST", StatusNot: "Done"})
+	require.NoError(t, err)
+	err = integration.CloseIssueByID("10001")
+	require.NoError(t, err)
+	require.Zero(t, searchCalls)
+	require.Equal(t, 1, transitionCalls)
+}
+
+func TestCloseIssueReportsTransitionLookupFailure(t *testing.T) {
+	transitionLookups := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search"):
+			_, _ = w.Write([]byte(`{"issues":[{"id":"10001","key":"TEST-1","fields":{"status":{"name":"Open"}}}]}`))
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search/jql"):
+			http.Error(w, "", http.StatusForbidden)
+		case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/transitions"):
+			transitionLookups++
+			http.Error(w, "", http.StatusForbidden)
+		default:
+			http.Error(w, "unexpected endpoint", http.StatusNotFound)
+		}
+	}))
+	defer server.Close()
+
+	integration, err := New(&Options{URL: server.URL, AccountID: "test", Token: "test", ProjectName: "TEST", StatusNot: "Done"})
+	require.NoError(t, err)
+	err = integration.CloseIssue(&output.ResultEvent{TemplateID: "pdcp-verify", Host: "example.test", Info: model.Info{Name: "Test issue"}})
+	require.ErrorContains(t, err, fmt.Sprint(http.StatusForbidden))
+	require.Equal(t, 1, transitionLookups)
+}
+
+func TestCloseIssueWithoutTargetTransition(t *testing.T) {
+	for _, tc := range []struct {
+		status  string
+		wantErr bool
+	}{
+		{status: "Open", wantErr: true},
+		{status: "Done", wantErr: false},
+	} {
+		t.Run(tc.status, func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				switch {
+				case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/search"):
+					_, _ = w.Write([]byte(`{"issues":[{"id":"10001","key":"TEST-1","fields":{"status":{"name":"Open"}}}]}`))
+				case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/transitions"):
+					_, _ = w.Write([]byte(`{"transitions":[]}`))
+				case r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/issue/10001"):
+					_, _ = fmt.Fprintf(w, `{"fields":{"status":{"name":%q}}}`, tc.status)
+				default:
+					http.Error(w, "unexpected endpoint", http.StatusNotFound)
+				}
+			}))
+			defer server.Close()
+
+			integration, err := New(&Options{URL: server.URL, AccountID: "test", Token: "test", StatusNot: "Done"})
+			require.NoError(t, err)
+			err = integration.CloseIssue(&output.ResultEvent{IssueTrackers: map[string]output.IssueTrackerMetadata{
+				"jira": {IssueID: "10001"},
+			}})
+			if tc.wantErr {
+				require.ErrorContains(t, err, "no transition")
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 type recordingTransport struct {
 	inner http.RoundTripper
 	paths []string