diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 4b57cdd05..fb507cd5c 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -412,19 +412,12 @@ func createRun(opts *CreateOptions) (err error) { return } - // Post-creation mutations for Issues 2.0 fields - err = applyIssueType(apiClient, baseRepo, newIssue, opts) + var updateOpts api.DeferredUpdateIssueOptions + updateOpts, err = deferredUpdateIssueOptions(apiClient, baseRepo, newIssue, opts) if err != nil { return } - - err = applyParent(apiClient, baseRepo, newIssue, opts) - if err != nil { - return - } - - err = applyRelationships(apiClient, baseRepo, newIssue, opts) - if err != nil { + if err = api.DeferredUpdateIssue(apiClient, updateOpts); err != nil { return } @@ -441,63 +434,50 @@ func generatePreviewURL(apiClient *api.Client, baseRepo ghrepo.Interface, tb prS return prShared.WithPrAndIssueQueryParams(apiClient, baseRepo, openURL, tb, projectsV1Support) } -// applyIssueType resolves the --type flag and sets the issue type via mutation. -func applyIssueType(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, opts *CreateOptions) error { - if opts.IssueType == "" { - return nil +// 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) { + updateOpts := api.DeferredUpdateIssueOptions{ + IssueID: issue.ID, + Hostname: baseRepo.RepoHost(), } - // Use pre-resolved ID from interactive flow if available - typeID := opts.issueTypeID - if typeID == "" { - var err error - typeID, err = issueShared.ResolveIssueTypeName(client, baseRepo, opts.IssueType) - if err != nil { - return err + if opts.IssueType != "" { + typeID := opts.issueTypeID + if typeID == "" { + var err error + typeID, err = issueShared.ResolveIssueTypeName(client, baseRepo, opts.IssueType) + if err != nil { + return updateOpts, err + } } + updateOpts.IssueTypeID = typeID } - return api.UpdateIssueIssueType(client, baseRepo.RepoHost(), issue.ID, typeID) -} - -// applyParent resolves the --parent flag and adds the issue as a sub-issue. -func applyParent(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, opts *CreateOptions) error { - if opts.Parent == "" { - return nil + if opts.Parent != "" { + parentID, err := issueShared.ResolveIssueRef(client, baseRepo, opts.Parent) + if err != nil { + return updateOpts, fmt.Errorf("resolving --parent reference %q: %w", opts.Parent, err) + } + updateOpts.ParentID = parentID } - parentID, err := issueShared.ResolveIssueRef(client, baseRepo, opts.Parent) - if err != nil { - return fmt.Errorf("resolving parent: %w", err) - } - - return api.AddSubIssue(client, baseRepo.RepoHost(), parentID, issue.ID, false) -} - -// applyRelationships resolves the --blocked-by and --blocking flags and creates relationships. -func applyRelationships(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, opts *CreateOptions) error { - hostname := baseRepo.RepoHost() - for _, ref := range opts.BlockedBy { - blockingID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) if err != nil { - return fmt.Errorf("resolving --blocked-by reference %q: %w", ref, err) - } - if err := api.AddBlockedBy(client, hostname, issue.ID, blockingID); err != nil { - return err + return updateOpts, fmt.Errorf("resolving --blocked-by reference %q: %w", ref, err) } + updateOpts.AddBlockedByIDs = append(updateOpts.AddBlockedByIDs, id) } for _, ref := range opts.Blocking { - // --blocking swaps the args: the OTHER issue is blocked by THIS issue - blockedID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) if err != nil { - return fmt.Errorf("resolving --blocking reference %q: %w", ref, err) - } - if err := api.AddBlockedBy(client, hostname, blockedID, issue.ID); err != nil { - return err + return updateOpts, fmt.Errorf("resolving --blocking reference %q: %w", ref, err) } + updateOpts.AddBlockingIDs = append(updateOpts.AddBlockingIDs, id) } - return nil + return updateOpts, nil } diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 603bb7350..bd39dfd9b 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -885,58 +885,44 @@ func Test_createRun(t *testing.T) { "id": "ISSUE_ID_123", "URL": "https://github.com/OWNER/REPO/issues/123" } } } }`)) - // IssueNodeID + AddBlockedBy for --blocked-by 200 + // IssueNodeID lookups for each ref, routed by number so they + // don't depend on parallel ordering. r.Register( - httpmock.GraphQL(`query IssueNodeID\b`), - httpmock.StringResponse(` - { "data": { "repository": { "issue": { "id": "BLOCKER_ID_200" } } } }`)) + issueNodeIDByNumberMatcher(200), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "BLOCKER_ID_200" } } } }`)) r.Register( - httpmock.GraphQL(`mutation AddBlockedBy\b`), - httpmock.GraphQLMutation(` - { "data": { "addBlockedBy": { "issue": { "id": "ISSUE_ID_123" } } } }`, - func(inputs map[string]interface{}) { - assert.Equal(t, "ISSUE_ID_123", inputs["issueId"]) - assert.Equal(t, "BLOCKER_ID_200", inputs["blockingIssueId"]) - })) - // IssueNodeID + AddBlockedBy for --blocked-by 201 + issueNodeIDByNumberMatcher(201), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "BLOCKER_ID_201" } } } }`)) r.Register( - httpmock.GraphQL(`query IssueNodeID\b`), - httpmock.StringResponse(` - { "data": { "repository": { "issue": { "id": "BLOCKER_ID_201" } } } }`)) + issueNodeIDByNumberMatcher(300), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "BLOCKED_ID_300" } } } }`)) r.Register( - httpmock.GraphQL(`mutation AddBlockedBy\b`), - httpmock.GraphQLMutation(` - { "data": { "addBlockedBy": { "issue": { "id": "ISSUE_ID_123" } } } }`, - func(inputs map[string]interface{}) { - assert.Equal(t, "ISSUE_ID_123", inputs["issueId"]) - assert.Equal(t, "BLOCKER_ID_201", inputs["blockingIssueId"]) - })) - // IssueNodeID + AddBlockedBy for --blocking 300 (swapped args) + issueNodeIDByNumberMatcher(301), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "BLOCKED_ID_301" } } } }`)) + // AddBlockedBy mutations, routed by their inputs so they + // also don't depend on parallel ordering. + // --blocked-by N: this issue is blocked by N r.Register( - httpmock.GraphQL(`query IssueNodeID\b`), - httpmock.StringResponse(` - { "data": { "repository": { "issue": { "id": "BLOCKED_ID_300" } } } }`)) + httpmock.GraphQLMutationMatcher(`mutation AddBlockedBy\b`, func(input map[string]interface{}) bool { + return input["issueId"] == "ISSUE_ID_123" && input["blockingIssueId"] == "BLOCKER_ID_200" + }), + httpmock.StringResponse(`{ "data": { "addBlockedBy": { "issue": { "id": "ISSUE_ID_123" } } } }`)) r.Register( - httpmock.GraphQL(`mutation AddBlockedBy\b`), - httpmock.GraphQLMutation(` - { "data": { "addBlockedBy": { "issue": { "id": "BLOCKED_ID_300" } } } }`, - func(inputs map[string]interface{}) { - assert.Equal(t, "BLOCKED_ID_300", inputs["issueId"]) - assert.Equal(t, "ISSUE_ID_123", inputs["blockingIssueId"]) - })) - // IssueNodeID + AddBlockedBy for --blocking 301 (swapped args) + httpmock.GraphQLMutationMatcher(`mutation AddBlockedBy\b`, func(input map[string]interface{}) bool { + return input["issueId"] == "ISSUE_ID_123" && input["blockingIssueId"] == "BLOCKER_ID_201" + }), + httpmock.StringResponse(`{ "data": { "addBlockedBy": { "issue": { "id": "ISSUE_ID_123" } } } }`)) + // --blocking N: N is blocked by this issue (args swapped) r.Register( - httpmock.GraphQL(`query IssueNodeID\b`), - httpmock.StringResponse(` - { "data": { "repository": { "issue": { "id": "BLOCKED_ID_301" } } } }`)) + httpmock.GraphQLMutationMatcher(`mutation AddBlockedBy\b`, func(input map[string]interface{}) bool { + return input["issueId"] == "BLOCKED_ID_300" && input["blockingIssueId"] == "ISSUE_ID_123" + }), + httpmock.StringResponse(`{ "data": { "addBlockedBy": { "issue": { "id": "BLOCKED_ID_300" } } } }`)) r.Register( - httpmock.GraphQL(`mutation AddBlockedBy\b`), - httpmock.GraphQLMutation(` - { "data": { "addBlockedBy": { "issue": { "id": "BLOCKED_ID_301" } } } }`, - func(inputs map[string]interface{}) { - assert.Equal(t, "BLOCKED_ID_301", inputs["issueId"]) - assert.Equal(t, "ISSUE_ID_123", inputs["blockingIssueId"]) - })) + httpmock.GraphQLMutationMatcher(`mutation AddBlockedBy\b`, func(input map[string]interface{}) bool { + return input["issueId"] == "BLOCKED_ID_301" && input["blockingIssueId"] == "ISSUE_ID_123" + }), + httpmock.StringResponse(`{ "data": { "addBlockedBy": { "issue": { "id": "BLOCKED_ID_301" } } } }`)) }, wantsStdout: "https://github.com/OWNER/REPO/issues/123\n", wantsStderr: "\nCreating issue in OWNER/REPO\n\n", @@ -1716,3 +1702,30 @@ func TestProjectsV1Deprecation(t *testing.T) { }) }) } + +// issueNodeIDByNumberMatcher matches an IssueNodeID GraphQL query whose +// number variable equals the given value. Used by tests that issue +// multiple IssueNodeID lookups and need stubs to route by issue number +// rather than by registration order. +func issueNodeIDByNumberMatcher(number int) httpmock.Matcher { + queryMatcher := httpmock.GraphQL(`query IssueNodeID\b`) + return func(req *http.Request) bool { + if !queryMatcher(req) { + return false + } + body, err := io.ReadAll(req.Body) + if err != nil { + return false + } + req.Body = io.NopCloser(bytes.NewReader(body)) + var b struct { + Variables struct { + Number int `json:"number"` + } `json:"variables"` + } + if err := json.Unmarshal(body, &b); err != nil { + return false + } + return b.Variables.Number == number + } +}