diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 23f332d7048..1538a6b4036 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -304,8 +304,10 @@ func createRun(opts *CreateOptions) (err error) { } } - // Interactive issue type selection - if opts.IssueType == "" { + // Interactive issue type selection. Setting a type requires triage + // access, so don't offer the picker to a viewer who cannot apply it: + // the mutation would only fail once the issue already exists. + if opts.IssueType == "" && repo.ViewerCanTriage() { issueTypes, typesErr := api.RepoIssueTypes(apiClient, baseRepo) if typesErr == nil && len(issueTypes) > 0 { typeNames := make([]string, len(issueTypes)) @@ -393,6 +395,15 @@ func createRun(opts *CreateOptions) (err error) { } return opts.Browser.Browse(openURL) } else if action == prShared.SubmitAction { + // Resolve the deferred fields before creating anything. These lookups + // can fail on user error (an unknown --type, a bad issue reference) and + // such a failure must not leave a created issue behind. + var updateOpts api.DeferredUpdateIssueOptions + updateOpts, err = resolveDeferredUpdateIssueOptions(apiClient, baseRepo, opts) + if err != nil { + return + } + params := map[string]interface{}{ "title": tb.Title, "body": tb.Body, @@ -412,13 +423,14 @@ func createRun(opts *CreateOptions) (err error) { return } - var updateOpts api.DeferredUpdateIssueOptions - updateOpts, err = deferredUpdateIssueOptions(apiClient, baseRepo, newIssue, opts) - if err != nil { - return - } - if err = api.DeferredUpdateIssue(apiClient, updateOpts); err != nil { - return + updateOpts.IssueID = newIssue.ID + // The issue exists by now, so failing to apply the deferred fields is + // not a failure of the whole operation. Returning the error here would + // make PreserveInput write a recovery file and print "operation + // failed", implying the issue was never created. + if updateErr := api.DeferredUpdateIssue(apiClient, updateOpts); updateErr != nil { + fmt.Fprintf(opts.IO.ErrOut, "%s Issue created, but not all fields could be set: %s\n", + opts.IO.ColorScheme().WarningIcon(), updateErr) } fmt.Fprintln(opts.IO.Out, newIssue.URL) @@ -434,12 +446,12 @@ func generatePreviewURL(apiClient *api.Client, baseRepo ghrepo.Interface, tb prS return prShared.WithPrAndIssueQueryParams(apiClient, baseRepo, openURL, tb, projectsV1Support) } -// deferredUpdateIssueOptions resolves the user-supplied --type / --parent / -// --blocked-by / --blocking flags into the IDs that DeferredUpdateIssue -// expects. -func deferredUpdateIssueOptions(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, opts *CreateOptions) (api.DeferredUpdateIssueOptions, error) { +// resolveDeferredUpdateIssueOptions resolves the user-supplied --type / +// --parent / --blocked-by / --blocking flags into the IDs that +// DeferredUpdateIssue expects. It performs reads only and needs no issue ID, so +// callers run it before creating the issue and set IssueID afterwards. +func resolveDeferredUpdateIssueOptions(client *api.Client, baseRepo ghrepo.Interface, opts *CreateOptions) (api.DeferredUpdateIssueOptions, error) { updateOpts := api.DeferredUpdateIssueOptions{ - IssueID: issue.ID, Hostname: baseRepo.RepoHost(), } diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index bd39dfd9bdb..a04b9163b5d 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -787,6 +787,106 @@ func Test_createRun(t *testing.T) { wantsStdout: "https://github.com/OWNER/REPO/issues/123\n", wantsStderr: "\nCreating issue in OWNER/REPO\n\n", }, + { + // Setting a type requires triage access. Without it the picker is + // skipped entirely, so the types are never even fetched. + name: "interactive does not prompt for type without triage access", + opts: CreateOptions{ + Interactive: true, + Detector: &fd.EnabledDetectorMock{}, + Title: "feature request", + Body: "would be nice to have", + }, + promptStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(message, defaultValue string, options []string) (int, error) { + switch message { + case "What's next?": + return prompter.IndexFor(options, "Submit") + default: + return 0, fmt.Errorf("unexpected select prompt: %s", message) + } + } + }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query IssueRepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true, + "viewerPermission": "READ" + } } }`)) + // A failed types fetch also silently skips the picker, so + // asserting on the prompt alone would pass even ungated. + // Exclude proves the gate short-circuits before the fetch. + r.Exclude(t, httpmock.GraphQL(`query RepositoryIssueTypes\b`)) + r.Register( + httpmock.GraphQL(`mutation IssueCreate\b`), + httpmock.StringResponse(` + { "data": { "createIssue": { "issue": { + "id": "ISSUE_ID_123", + "URL": "https://github.com/OWNER/REPO/issues/123" + } } } }`)) + }, + wantsStdout: "https://github.com/OWNER/REPO/issues/123\n", + wantsStderr: "\nCreating issue in OWNER/REPO\n\n", + }, + { + // Triage is the lowest role that can set a type, so the picker is + // still offered to it. + name: "interactive prompts for type with triage access", + opts: CreateOptions{ + Interactive: true, + Detector: &fd.EnabledDetectorMock{}, + Title: "feature request", + Body: "would be nice to have", + }, + promptStubs: func(pm *prompter.PrompterMock) { + pm.SelectFunc = func(message, defaultValue string, options []string) (int, error) { + switch message { + case "Issue type": + return prompter.IndexFor(options, "Feature") + case "What's next?": + return prompter.IndexFor(options, "Submit") + default: + return 0, fmt.Errorf("unexpected select prompt: %s", message) + } + } + }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query IssueRepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true, + "viewerPermission": "TRIAGE" + } } }`)) + r.Register( + httpmock.GraphQL(`query RepositoryIssueTypes\b`), + httpmock.StringResponse(` + { "data": { "repository": { "issueTypes": { "nodes": [ + { "id": "IT_1", "name": "Bug", "description": "", "color": "d73a4a" }, + { "id": "IT_2", "name": "Feature", "description": "", "color": "0075ca" } + ] } } } }`)) + r.Register( + httpmock.GraphQL(`mutation IssueCreate\b`), + httpmock.StringResponse(` + { "data": { "createIssue": { "issue": { + "id": "ISSUE_ID_123", + "URL": "https://github.com/OWNER/REPO/issues/123" + } } } }`)) + r.Register( + httpmock.GraphQL(`mutation UpdateIssueIssueType\b`), + httpmock.GraphQLMutation(` + { "data": { "updateIssueIssueType": { "issue": { "id": "ISSUE_ID_123" } } } }`, + func(inputs map[string]interface{}) { + assert.Equal(t, "IT_2", inputs["issueTypeId"]) + })) + }, + wantsStdout: "https://github.com/OWNER/REPO/issues/123\n", + wantsStderr: "\nCreating issue in OWNER/REPO\n\n", + }, { name: "create with type not found", opts: CreateOptions{ @@ -795,6 +895,91 @@ func Test_createRun(t *testing.T) { Body: "bug body", IssueType: "Bugz", }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query IssueRepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } }`)) + r.Register( + httpmock.GraphQL(`query RepositoryIssueTypes\b`), + httpmock.StringResponse(` + { "data": { "repository": { "issueTypes": { "nodes": [ + { "id": "IT_1", "name": "Bug", "description": "", "color": "d73a4a" }, + { "id": "IT_2", "name": "Feature", "description": "", "color": "0075ca" }, + { "id": "IT_3", "name": "Task", "description": "", "color": "e4e669" } + ] } } } }`)) + // An unknown type is user error, so it must fail before the + // issue exists rather than leave one behind. + r.Exclude(t, httpmock.GraphQL(`mutation IssueCreate\b`)) + }, + wantsErr: `type "Bugz" not found; available types: Bug, Feature, Task`, + }, + { + // A reference that cannot be resolved is user error too, and is + // resolved in the same pre-create step. + name: "create with unresolvable parent", + opts: CreateOptions{ + Detector: &fd.EnabledDetectorMock{}, + Title: "child issue", + Body: "child body", + Parent: "999", + }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query IssueRepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } }`)) + r.Register( + httpmock.GraphQL(`query IssueNodeID\b`), + httpmock.StringResponse(` + { "errors": [ { "type": "NOT_FOUND", "message": "Could not resolve to an Issue with the number of 999." } ] }`)) + r.Exclude(t, httpmock.GraphQL(`mutation IssueCreate\b`)) + }, + wantsErr: `resolving --parent reference "999": GraphQL: Could not resolve to an Issue with the number of 999.`, + }, + { + // Refs are resolved in a loop, so a later ref failing after an + // earlier one succeeded must still abort before the issue exists. + name: "create with unresolvable blocked-by among several", + opts: CreateOptions{ + Detector: &fd.EnabledDetectorMock{}, + Title: "blocked issue", + Body: "blocked body", + BlockedBy: []string{"200", "999"}, + }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query IssueRepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "id": "REPOID", + "hasIssuesEnabled": true + } } }`)) + r.Register( + issueNodeIDByNumberMatcher(200), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "BLOCKER_ID_200" } } } }`)) + r.Register( + issueNodeIDByNumberMatcher(999), + httpmock.StringResponse(` + { "errors": [ { "type": "NOT_FOUND", "message": "Could not resolve to an Issue with the number of 999." } ] }`)) + r.Exclude(t, httpmock.GraphQL(`mutation IssueCreate\b`)) + }, + wantsErr: `resolving --blocked-by reference "999": GraphQL: Could not resolve to an Issue with the number of 999.`, + }, + { + name: "create with type that cannot be set", + opts: CreateOptions{ + Detector: &fd.EnabledDetectorMock{}, + Title: "bug title", + Body: "bug body", + IssueType: "Bug", + }, httpStubs: func(_ *testing.T, r *httpmock.Registry) { r.Register( httpmock.GraphQL(`query IssueRepositoryInfo\b`), @@ -814,12 +999,22 @@ func Test_createRun(t *testing.T) { httpmock.GraphQL(`query RepositoryIssueTypes\b`), httpmock.StringResponse(` { "data": { "repository": { "issueTypes": { "nodes": [ - { "id": "IT_1", "name": "Bug", "description": "", "color": "d73a4a" }, - { "id": "IT_2", "name": "Feature", "description": "", "color": "0075ca" }, - { "id": "IT_3", "name": "Task", "description": "", "color": "e4e669" } + { "id": "IT_1", "name": "Bug", "description": "", "color": "d73a4a" } ] } } } }`)) + // The issue exists by now; the type cannot be applied because + // the user lacks permission on the repository. + r.Register( + httpmock.GraphQL(`mutation UpdateIssueIssueType\b`), + httpmock.StringResponse(` + { "errors": [ + { + "type": "FORBIDDEN", + "message": "monalisa does not have the correct permissions to execute `+"`UpdateIssueIssueType`"+`" + } + ] }`)) }, - wantsErr: `type "Bugz" not found; available types: Bug, Feature, Task`, + wantsStdout: "https://github.com/OWNER/REPO/issues/123\n", + wantsStderr: "\nCreating issue in OWNER/REPO\n\n! Issue created, but not all fields could be set: GraphQL: monalisa does not have the correct permissions to execute `UpdateIssueIssueType`\n", }, { name: "create with parent",