Merge pull request cli#12838 from cli/remove-state-reason-duplicate-detection

Remove unnecessary StateReason and StateReasonDuplicate feature detection

Author: williammartin

internal/featuredetection/feature_detection.go

@@ -23,15 +23,11 @@ type Detector interface {
 }
 
 type IssueFeatures struct {
-	StateReason          bool
-	StateReasonDuplicate bool
-	ActorIsAssignable    bool
+	ActorIsAssignable bool
 }
 
 var allIssueFeatures = IssueFeatures{
-	StateReason:          true,
-	StateReasonDuplicate: true,
-	ActorIsAssignable:    true,
+	ActorIsAssignable: true,
 }
 
 type PullRequestFeatures struct {
@@ -139,47 +135,9 @@ func (d *detector) IssueFeatures() (IssueFeatures, error) {
 		return allIssueFeatures, nil
 	}
 
-	features := IssueFeatures{
-		StateReason:          false,
-		StateReasonDuplicate: false,
-		ActorIsAssignable:    false, // replaceActorsForAssignable GraphQL mutation unavailable on GHES
-	}
-
-	var featureDetection struct {
-		Issue struct {
-			Fields []struct {
-				Name string
-			} `graphql:"fields(includeDeprecated: true)"`
-		} `graphql:"Issue: __type(name: \"Issue\")"`
-		IssueClosedStateReason struct {
-			EnumValues []struct {
-				Name string
-			} `graphql:"enumValues(includeDeprecated: true)"`
-		} `graphql:"IssueClosedStateReason: __type(name: \"IssueClosedStateReason\")"`
-	}
-
-	gql := api.NewClientFromHTTP(d.httpClient)
-	err := gql.Query(d.host, "Issue_fields", &featureDetection, nil)
-	if err != nil {
-		return features, err
-	}
-
-	for _, field := range featureDetection.Issue.Fields {
-		if field.Name == "stateReason" {
-			features.StateReason = true
-		}
-	}
-
-	if features.StateReason {
-		for _, enumValue := range featureDetection.IssueClosedStateReason.EnumValues {
-			if enumValue.Name == "DUPLICATE" {
-				features.StateReasonDuplicate = true
-				break
-			}
-		}
-	}
-
-	return features, nil
+	return IssueFeatures{
+		ActorIsAssignable: false, // replaceActorsForAssignable GraphQL mutation unavailable on GHES
+	}, nil
 }
 
 func (d *detector) PullRequestFeatures() (PullRequestFeatures, error) {

internal/featuredetection/feature_detection_test.go

@@ -23,73 +23,23 @@ func TestIssueFeatures(t *testing.T) {
 			name:     "github.com",
 			hostname: "github.com",
 			wantFeatures: IssueFeatures{
-				StateReason:          true,
-				StateReasonDuplicate: true,
-				ActorIsAssignable:    true,
+				ActorIsAssignable: true,
 			},
 			wantErr: false,
 		},
 		{
 			name:     "ghec data residency (ghe.com)",
 			hostname: "stampname.ghe.com",
 			wantFeatures: IssueFeatures{
-				StateReason:          true,
-				StateReasonDuplicate: true,
-				ActorIsAssignable:    true,
+				ActorIsAssignable: true,
 			},
 			wantErr: false,
 		},
 		{
-			name:     "GHE empty response",
-			hostname: "git.my.org",
-			queryResponse: map[string]string{
-				`query Issue_fields\b`: `{"data": {}}`,
-			},
-			wantFeatures: IssueFeatures{
-				StateReason:          false,
-				StateReasonDuplicate: false,
-				ActorIsAssignable:    false,
-			},
-			wantErr: false,
-		},
-		{
-			name:     "GHE has state reason field without duplicate enum",
+			name:     "GHE",
 			hostname: "git.my.org",
-			queryResponse: map[string]string{
-				`query Issue_fields\b`: heredoc.Doc(`
-					{ "data": { "Issue": { "fields": [
-						{"name": "stateReason"}
-					] }, "IssueClosedStateReason": { "enumValues": [
-						{"name": "COMPLETED"},
-						{"name": "NOT_PLANNED"}
-					] } } }
-				`),
-			},
-			wantFeatures: IssueFeatures{
-				StateReason:          true,
-				StateReasonDuplicate: false,
-				ActorIsAssignable:    false,
-			},
-			wantErr: false,
-		},
-		{
-			name:     "GHE has duplicate state reason enum value",
-			hostname: "git.my.org",
-			queryResponse: map[string]string{
-				`query Issue_fields\b`: heredoc.Doc(`
-					{ "data": { "Issue": { "fields": [
-						{"name": "stateReason"}
-					] }, "IssueClosedStateReason": { "enumValues": [
-						{"name": "COMPLETED"},
-						{"name": "NOT_PLANNED"},
-						{"name": "DUPLICATE"}
-					] } } }
-				`),
-			},
 			wantFeatures: IssueFeatures{
-				StateReason:          true,
-				StateReasonDuplicate: true,
-				ActorIsAssignable:    false,
+				ActorIsAssignable: false,
 			},
 			wantErr: false,
 		},

pkg/cmd/issue/close/close.go

@@ -3,11 +3,9 @@ package close
 import (
 	"fmt"
 	"net/http"
-	"time"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/v2/api"
-	fd "github.com/cli/cli/v2/internal/featuredetection"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/cmd/issue/shared"
 	prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared"
@@ -26,8 +24,6 @@ type CloseOptions struct {
 	Comment     string
 	Reason      string
 	DuplicateOf string
-
-	Detector fd.Detector
 }
 
 func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Command {
@@ -165,7 +161,7 @@ func closeRun(opts *CloseOptions) error {
 		}
 	}
 
-	err = apiClose(httpClient, baseRepo, issue, opts.Detector, closeReason, duplicateIssueID)
+	err = apiClose(httpClient, baseRepo, issue, closeReason, duplicateIssueID)
 	if err != nil {
 		return err
 	}
@@ -175,36 +171,11 @@ func closeRun(opts *CloseOptions) error {
 	return nil
 }
 
-func apiClose(httpClient *http.Client, repo ghrepo.Interface, issue *api.Issue, detector fd.Detector, reason string, duplicateIssueID string) error {
+func apiClose(httpClient *http.Client, repo ghrepo.Interface, issue *api.Issue, reason string, duplicateIssueID string) error {
 	if issue.IsPullRequest() {
 		return api.PullRequestClose(httpClient, repo, issue.ID)
 	}
 
-	if reason != "" || duplicateIssueID != "" {
-		if detector == nil {
-			cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
-			detector = fd.NewDetector(cachedClient, repo.RepoHost())
-		}
-		features, err := detector.IssueFeatures()
-		if err != nil {
-			return err
-		}
-		// TODO stateReasonCleanup
-		if !features.StateReason {
-			// If StateReason is not supported silently close issue without setting StateReason.
-			if duplicateIssueID != "" {
-				return fmt.Errorf("closing as duplicate is not supported on %s", repo.RepoHost())
-			}
-			reason = ""
-		} else if reason == "duplicate" && !features.StateReasonDuplicate {
-			if duplicateIssueID != "" {
-				return fmt.Errorf("closing as duplicate is not supported on %s", repo.RepoHost())
-			}
-			// If DUPLICATE is not supported silently close issue without setting StateReason.
-			reason = ""
-		}
-	}
-
 	switch reason {
 	case "":
 		// If no reason is specified do not set it.

pkg/cmd/issue/close/close_test.go

@@ -5,7 +5,6 @@ import (
 	"net/http"
 	"testing"
 
-	fd "github.com/cli/cli/v2/internal/featuredetection"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/cmd/issue/argparsetest"
 	"github.com/cli/cli/v2/pkg/cmdutil"
@@ -16,15 +15,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-type issueFeaturesDetectorMock struct {
-	fd.EnabledDetectorMock
-	issueFeatures fd.IssueFeatures
-}
-
-func (md *issueFeaturesDetectorMock) IssueFeatures() (fd.IssueFeatures, error) {
-	return md.issueFeatures, nil
-}
-
 func TestNewCmdClose(t *testing.T) {
 	// Test shared parsing of issue number / URL.
 	argparsetest.TestArgParsing(t, NewCmdClose)
@@ -194,7 +184,6 @@ func TestCloseRun(t *testing.T) {
 			opts: &CloseOptions{
 				IssueNumber: 13,
 				Reason:      "not planned",
-				Detector:    &fd.EnabledDetectorMock{},
 			},
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
@@ -222,7 +211,6 @@ func TestCloseRun(t *testing.T) {
 			opts: &CloseOptions{
 				IssueNumber: 13,
 				Reason:      "duplicate",
-				Detector:    &fd.EnabledDetectorMock{},
 			},
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
@@ -250,7 +238,6 @@ func TestCloseRun(t *testing.T) {
 			opts: &CloseOptions{
 				IssueNumber: 13,
 				DuplicateOf: "99",
-				Detector:    &fd.EnabledDetectorMock{},
 			},
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
@@ -282,71 +269,6 @@ func TestCloseRun(t *testing.T) {
 			},
 			wantStderr: "✓ Closed issue OWNER/REPO#13 (The title of the issue)\n",
 		},
-		{
-			name: "close issue with duplicate reason when duplicate is not supported",
-			opts: &CloseOptions{
-				IssueNumber: 13,
-				Reason:      "duplicate",
-				Detector: &issueFeaturesDetectorMock{
-					issueFeatures: fd.IssueFeatures{
-						StateReason:          true,
-						StateReasonDuplicate: false,
-					},
-				},
-			},
-			httpStubs: func(reg *httpmock.Registry) {
-				reg.Register(
-					httpmock.GraphQL(`query IssueByNumber\b`),
-					httpmock.StringResponse(`
-            { "data": { "repository": {
-              "hasIssuesEnabled": true,
-              "issue": { "id": "THE-ID", "number": 13, "title": "The title of the issue"}
-            } } }`),
-				)
-				reg.Register(
-					httpmock.GraphQL(`mutation IssueClose\b`),
-					httpmock.GraphQLMutation(`{"id": "THE-ID"}`,
-						func(inputs map[string]interface{}) {
-							assert.Equal(t, 1, len(inputs))
-							assert.Equal(t, "THE-ID", inputs["issueId"])
-						}),
-				)
-			},
-			wantStderr: "✓ Closed issue OWNER/REPO#13 (The title of the issue)\n",
-		},
-		{
-			name: "close issue as duplicate when duplicate is not supported",
-			opts: &CloseOptions{
-				IssueNumber: 13,
-				DuplicateOf: "99",
-				Detector: &issueFeaturesDetectorMock{
-					issueFeatures: fd.IssueFeatures{
-						StateReason:          true,
-						StateReasonDuplicate: false,
-					},
-				},
-			},
-			httpStubs: func(reg *httpmock.Registry) {
-				reg.Register(
-					httpmock.GraphQL(`query IssueByNumber\b`),
-					httpmock.StringResponse(`
-            { "data": { "repository": {
-              "hasIssuesEnabled": true,
-              "issue": { "id": "THE-ID", "number": 13, "title": "The title of the issue"}
-            } } }`),
-				)
-				reg.Register(
-					httpmock.GraphQL(`query IssueByNumber\b`),
-					httpmock.StringResponse(`
-            { "data": { "repository": {
-              "hasIssuesEnabled": true,
-              "issue": { "id": "DUPLICATE-ID", "number": 99}
-            } } }`),
-				)
-			},
-			wantErr: true,
-			errMsg:  "closing as duplicate is not supported on github.com",
-		},
 		{
 			name: "duplicate of cannot point to same issue",
 			opts: &CloseOptions{
@@ -412,33 +334,6 @@ func TestCloseRun(t *testing.T) {
 			wantErr: true,
 			errMsg:  "invalid value for `--duplicate-of`: invalid issue format: \"not-an-issue\"",
 		},
-		{
-			name: "close issue with reason when reason is not supported",
-			opts: &CloseOptions{
-				IssueNumber: 13,
-				Reason:      "not planned",
-				Detector:    &fd.DisabledDetectorMock{},
-			},
-			httpStubs: func(reg *httpmock.Registry) {
-				reg.Register(
-					httpmock.GraphQL(`query IssueByNumber\b`),
-					httpmock.StringResponse(`
-            { "data": { "repository": {
-              "hasIssuesEnabled": true,
-              "issue": { "id": "THE-ID", "number": 13, "title": "The title of the issue"}
-            } } }`),
-				)
-				reg.Register(
-					httpmock.GraphQL(`mutation IssueClose\b`),
-					httpmock.GraphQLMutation(`{"id": "THE-ID"}`,
-						func(inputs map[string]interface{}) {
-							assert.Equal(t, 1, len(inputs))
-							assert.Equal(t, "THE-ID", inputs["issueId"])
-						}),
-				)
-			},
-			wantStderr: "✓ Closed issue OWNER/REPO#13 (The title of the issue)\n",
-		},
 		{
 			name: "issue already closed",
 			opts: &CloseOptions{

pkg/cmd/issue/list/list.go

@@ -147,15 +147,7 @@ func listRun(opts *ListOptions) error {
 		cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
 		opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
 	}
-	features, err := opts.Detector.IssueFeatures()
-	if err != nil {
-		return err
-	}
-	fields := defaultFields
-	// TODO stateReasonCleanup
-	if features.StateReason {
-		fields = append(defaultFields, "stateReason")
-	}
+	fields := append(defaultFields, "stateReason")
 
 	filterOptions := prShared.FilterOptions{
 		Entity:    "issue",