refactor(pr edit, issue edit): use login-based assignee mutation for flag flows
When ActorAssignees is true (github.com), the --add-assignee and --remove-assignee flag flows now pass logins directly to ReplaceActorsForAssignableByLogin instead of bulk fetching all assignable actors and resolving logins to node IDs. Changes: - New AssigneeLogins() method on Editable that computes the final login set (defaults + add - remove) without ID resolution - UpdateIssue: call AssigneeLogins + ByLogin when ActorAssignees is true - EditableOptionsFetch: skip assignee bulk fetch for flag flows on github.com (only fetch on GHES where ID resolution is needed) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Kynan Ware
parent
e6d9019bc9
commit
b3cfe7454c
4 changed files with 67 additions and 43 deletions
|
|
@ -527,17 +527,6 @@ func Test_editRun(t *testing.T) {
|
|||
},
|
||||
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
|
||||
// Should only be one fetch of metadata.
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
|
||||
httpmock.StringResponse(`
|
||||
{ "data": { "repository": { "suggestedActors": {
|
||||
"nodes": [
|
||||
{ "login": "hubot", "id": "HUBOTID", "__typename": "Bot" },
|
||||
{ "login": "MonaLisa", "id": "MONAID", "__typename": "User" }
|
||||
],
|
||||
"pageInfo": { "hasNextPage": false, "endCursor": "Mg" }
|
||||
} } } }
|
||||
`))
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query RepositoryMilestoneList\b`),
|
||||
httpmock.StringResponse(`
|
||||
|
|
@ -618,6 +607,17 @@ func Test_editRun(t *testing.T) {
|
|||
mockIssueGet(t, reg)
|
||||
mockIssueProjectItemsGet(t, reg)
|
||||
mockRepoMetadata(t, reg)
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
|
||||
httpmock.StringResponse(`
|
||||
{ "data": { "repository": { "suggestedActors": {
|
||||
"nodes": [
|
||||
{ "login": "hubot", "id": "HUBOTID", "__typename": "Bot" },
|
||||
{ "login": "monalisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" }
|
||||
],
|
||||
"pageInfo": { "hasNextPage": false }
|
||||
} } } }
|
||||
`))
|
||||
mockIssueUpdate(t, reg)
|
||||
mockIssueUpdateActorAssignees(t, reg)
|
||||
mockIssueUpdateLabels(t, reg)
|
||||
|
|
@ -667,9 +667,9 @@ func Test_editRun(t *testing.T) {
|
|||
{ "data": { "replaceActorsForAssignable": { "__typename": "" } } }`,
|
||||
func(inputs map[string]interface{}) {
|
||||
// Checking that despite the display name being returned
|
||||
// from the EditFieldsSurvey, the ID is still
|
||||
// from the EditFieldsSurvey, the login is still
|
||||
// used in the mutation.
|
||||
require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"})
|
||||
require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "MonaLisa (Mona Display Name)"})
|
||||
}),
|
||||
)
|
||||
},
|
||||
|
|
@ -839,18 +839,6 @@ func mockIssueProjectItemsGet(_ *testing.T, reg *httpmock.Registry) {
|
|||
}
|
||||
|
||||
func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) {
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
|
||||
httpmock.StringResponse(`
|
||||
{ "data": { "repository": { "suggestedActors": {
|
||||
"nodes": [
|
||||
{ "login": "hubot", "id": "HUBOTID", "__typename": "Bot" },
|
||||
{ "login": "monalisa", "id": "MONAID", "name": "Mona Display Name", "__typename": "User" }
|
||||
],
|
||||
"pageInfo": { "hasNextPage": false }
|
||||
} } } }
|
||||
`))
|
||||
|
||||
reg.Register(
|
||||
httpmock.GraphQL(`query RepositoryLabelList\b`),
|
||||
httpmock.StringResponse(`
|
||||
|
|
|
|||
|
|
@ -415,7 +415,7 @@ func Test_editRun(t *testing.T) {
|
|||
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
|
||||
// Non-interactive with Add/Remove doesn't need reviewers/assignees metadata
|
||||
// REST API accepts logins and team slugs directly
|
||||
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true})
|
||||
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true})
|
||||
mockPullRequestUpdate(reg)
|
||||
mockPullRequestUpdateActorAssignees(reg)
|
||||
mockRequestReviewsByLogin(reg)
|
||||
|
|
@ -473,7 +473,7 @@ func Test_editRun(t *testing.T) {
|
|||
Fetcher: testFetcher{},
|
||||
},
|
||||
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
|
||||
mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: true, labels: true, projects: true, milestones: true})
|
||||
mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true})
|
||||
mockPullRequestUpdate(reg)
|
||||
mockPullRequestUpdateActorAssignees(reg)
|
||||
mockPullRequestUpdateLabels(reg)
|
||||
|
|
@ -547,7 +547,7 @@ func Test_editRun(t *testing.T) {
|
|||
},
|
||||
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
|
||||
// Non-interactive with Remove doesn't need reviewers metadata
|
||||
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: true, labels: true, projects: true, milestones: true})
|
||||
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true})
|
||||
mockPullRequestUpdate(reg)
|
||||
mockRequestReviewsByLogin(reg)
|
||||
mockPullRequestUpdateLabels(reg)
|
||||
|
|
@ -932,9 +932,9 @@ func Test_editRun(t *testing.T) {
|
|||
{ "data": { "replaceActorsForAssignable": { "__typename": "" } } }`,
|
||||
func(inputs map[string]interface{}) {
|
||||
// Checking that despite the display name being returned
|
||||
// from the EditFieldsSurvey, the ID is still
|
||||
// from the EditFieldsSurvey, the login is still
|
||||
// used in the mutation.
|
||||
require.Subset(t, inputs["actorIds"], []string{"MONAID", "HUBOTID"})
|
||||
require.Subset(t, inputs["actorLogins"], []interface{}{"hubot", "monalisa (Mona Display Name)"})
|
||||
}),
|
||||
)
|
||||
},
|
||||
|
|
|
|||
|
|
@ -150,6 +150,49 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str
|
|||
return &a, err
|
||||
}
|
||||
|
||||
// AssigneeLogins computes the final list of assignee logins from the current
|
||||
// defaults plus any Add/Remove operations. Unlike AssigneeIds, this does not
|
||||
// resolve logins to node IDs, and is used on github.com where the
|
||||
// ReplaceActorsForAssignable mutation accepts logins directly.
|
||||
func (e Editable) AssigneeLogins(client *api.Client, repo ghrepo.Interface) ([]string, error) {
|
||||
if !e.Assignees.Edited {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
if len(e.Assignees.Add) != 0 || len(e.Assignees.Remove) != 0 {
|
||||
meReplacer := NewMeReplacer(client, repo.RepoHost())
|
||||
copilotReplacer := NewCopilotReplacer(true)
|
||||
|
||||
replaceSpecialAssigneeNames := func(value []string) ([]string, error) {
|
||||
replaced, err := meReplacer.ReplaceSlice(value)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
replaced = copilotReplacer.ReplaceSlice(replaced)
|
||||
return replaced, nil
|
||||
}
|
||||
|
||||
assigneeSet := set.NewStringSet()
|
||||
assigneeSet.AddValues(e.Assignees.DefaultLogins)
|
||||
|
||||
add, err := replaceSpecialAssigneeNames(e.Assignees.Add)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
assigneeSet.AddValues(add)
|
||||
|
||||
remove, err := replaceSpecialAssigneeNames(e.Assignees.Remove)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
assigneeSet.RemoveValues(remove)
|
||||
|
||||
e.Assignees.Value = assigneeSet.ToSlice()
|
||||
}
|
||||
|
||||
return e.Assignees.Value, nil
|
||||
}
|
||||
|
||||
// ProjectIds returns a slice containing IDs of projects v1 that the issue or a PR has to be linked to.
|
||||
func (e Editable) ProjectIds() (*[]string, error) {
|
||||
if !e.Projects.Edited {
|
||||
|
|
@ -474,12 +517,10 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable,
|
|||
if len(editable.Assignees.Add) == 0 && len(editable.Assignees.Remove) == 0 && editable.AssigneeSearchFunc == nil {
|
||||
fetchAssignees = true
|
||||
}
|
||||
// However, if we have Add/Remove operations (non-interactive flow),
|
||||
// we do need to fetch the assignees.
|
||||
// TODO replaceActorsByLoginCleanup
|
||||
// When ActorAssignees is true, noninteractive assignees should use
|
||||
// ReplaceActorsForAssignableByLogin to skip this fetch entirely.
|
||||
if len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0 {
|
||||
// For non-interactive Add/Remove operations, we only need to fetch assignees
|
||||
// on GHES where ID resolution is required. On github.com (ActorAssignees),
|
||||
// logins are passed directly to the mutation.
|
||||
if (len(editable.Assignees.Add) > 0 || len(editable.Assignees.Remove) > 0) && !editable.Assignees.ActorAssignees {
|
||||
fetchAssignees = true
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -66,19 +66,14 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR
|
|||
// other issue fields to ensure consistency with how legacy
|
||||
// user assignees are handled.
|
||||
// https://github.com/cli/cli/pull/10960#discussion_r2086725348
|
||||
// TODO replaceActorsByLoginCleanup
|
||||
// When ActorAssignees is true (github.com), this should pass logins directly
|
||||
// to ReplaceActorsForAssignableByLogin instead of resolving IDs. The ID-based
|
||||
// path can be removed once both interactive and non-interactive edit flows
|
||||
// are migrated to use logins.
|
||||
if options.Assignees.Edited && options.Assignees.ActorAssignees {
|
||||
apiClient := api.NewClientFromHTTP(httpClient)
|
||||
assigneeIds, err := options.AssigneeIds(apiClient, repo)
|
||||
logins, err := options.AssigneeLogins(apiClient, repo)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
err = api.ReplaceActorsForAssignableByID(apiClient, repo, id, *assigneeIds)
|
||||
err = api.ReplaceActorsForAssignableByLogin(apiClient, repo, id, logins)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue