Skip to content

Commit 3c00ffd

Browse files
BagToadCopilot
BagToad
and
Copilot
committed
refactor(pr shared): consolidate ActorAssignees and ActorReviewers into ApiActorsSupported
The CLI had two per-entity flags (ActorAssignees on EditableAssignees and IssueMetadataState, ActorReviewers on IssueMetadataState) threaded through different layers of the stack to distinguish github.com from GHES. Both flags were always set from the same source (issueFeatures.ActorIsAssignable) and never had different values, but they were carried independently on different structs. This led to a confusing asymmetry where: - EditableAssignees had ActorAssignees but EditableReviewers had nothing - The PR edit flow piggybacked on editable.Assignees.ActorAssignees to make reviewer mutation decisions, which was misleading - RepoMetadataInput only had ActorAssignees with no reviewer equivalent This commit replaces all per-entity flags with a single ApiActorsSupported bool hoisted to the shared level on Editable, IssueMetadataState, and RepoMetadataInput. Both assignees and reviewers now key off the same signal. Every branch site is marked with // TODO ApiActorsSupported so we can grep for cleanup sites when GHES eventually supports the actor-based mutations (replaceActorsForAssignable, requestReviewsByLogin). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 1df6f84 commit 3c00ffd

15 files changed

Lines changed: 119 additions & 92 deletions

File tree

‎api/queries_issue.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -311,7 +311,7 @@ func IssueCreate(client *Client, repo *Repository, params map[string]interface{}
311311
}
312312
issue := &result.CreateIssue.Issue
313313

314-
// Assign users using login-based mutation when ActorAssignees is true (github.com).
314+
// Assign users using login-based mutation when ApiActorsSupported is true (github.com).
315315
if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 {
316316
err := ReplaceActorsForAssignableByLogin(client, repo, issue.ID, assigneeLogins)
317317
if err != nil {

‎api/queries_pr.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,7 +524,7 @@ func CreatePullRequest(client *Client, repo *Repository, params map[string]inter
524524
}
525525
}
526526

527-
// Assign users using login-based mutation when ActorAssignees is true (github.com).
527+
// Assign users using login-based mutation when ApiActorsSupported is true (github.com).
528528
if assigneeLogins, ok := params["assigneeLogins"].([]string); ok && len(assigneeLogins) > 0 {
529529
err := ReplaceActorsForAssignableByLogin(client, repo, pr.ID, assigneeLogins)
530530
if err != nil {

‎api/queries_repo.go‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -922,14 +922,15 @@ func (m *RepoMetadataResult) Merge(m2 *RepoMetadataResult) {
922922
}
923923

924924
type RepoMetadataInput struct {
925-
Assignees bool
926-
ActorAssignees bool
927-
Reviewers bool
928-
TeamReviewers bool
929-
Labels bool
930-
ProjectsV1 bool
931-
ProjectsV2 bool
932-
Milestones bool
925+
Assignees bool
926+
Reviewers bool
927+
TeamReviewers bool
928+
// TODO ApiActorsSupported
929+
ApiActorsSupported bool
930+
Labels bool
931+
ProjectsV1 bool
932+
ProjectsV2 bool
933+
Milestones bool
933934
}
934935

935936
// RepoMetadata pre-fetches the metadata for attaching to issues and pull requests
@@ -938,7 +939,8 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput
938939
var g errgroup.Group
939940

940941
if input.Assignees || input.Reviewers {
941-
if input.ActorAssignees {
942+
// TODO ApiActorsSupported
943+
if input.ApiActorsSupported {
942944
g.Go(func() error {
943945
actors, err := RepoAssignableActors(client, repo)
944946
if err != nil {

‎pkg/cmd/issue/create/create.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -188,14 +188,14 @@ func createRun(opts *CreateOptions) (err error) {
188188
assigneeSet.AddValues(assignees)
189189

190190
tb := prShared.IssueMetadataState{
191-
Type: prShared.IssueMetadata,
192-
ActorAssignees: issueFeatures.ActorIsAssignable,
193-
Assignees: assigneeSet.ToSlice(),
194-
Labels: opts.Labels,
195-
ProjectTitles: opts.Projects,
196-
Milestones: milestones,
197-
Title: opts.Title,
198-
Body: opts.Body,
191+
Type: prShared.IssueMetadata,
192+
ApiActorsSupported: issueFeatures.ActorIsAssignable, // TODO ApiActorsSupported
193+
Assignees: assigneeSet.ToSlice(),
194+
Labels: opts.Labels,
195+
ProjectTitles: opts.Projects,
196+
Milestones: milestones,
197+
Title: opts.Title,
198+
Body: opts.Body,
199199
}
200200

201201
if opts.RecoverFile != "" {

‎pkg/cmd/issue/edit/edit.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -215,9 +215,9 @@ func editRun(opts *EditOptions) error {
215215

216216
lookupFields := []string{"id", "number", "title", "body", "url"}
217217
if editable.Assignees.Edited {
218-
// TODO actorIsAssignableCleanup
218+
// TODO ApiActorsSupported
219219
if issueFeatures.ActorIsAssignable {
220-
editable.Assignees.ActorAssignees = true
220+
editable.ApiActorsSupported = true
221221
lookupFields = append(lookupFields, "assignedActors")
222222
} else {
223223
lookupFields = append(lookupFields, "assignees")
@@ -280,7 +280,8 @@ func editRun(opts *EditOptions) error {
280280
editable.Body.Default = issue.Body
281281
// We use Actors as the default assignees if Actors are assignable
282282
// on this GitHub host.
283-
if editable.Assignees.ActorAssignees {
283+
// TODO ApiActorsSupported
284+
if editable.ApiActorsSupported {
284285
editable.Assignees.Default = issue.AssignedActors.DisplayNames()
285286
editable.Assignees.DefaultLogins = issue.AssignedActors.Logins()
286287
} else {

‎pkg/cmd/issue/edit/edit_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,7 @@ func Test_editRun(t *testing.T) {
395395
mockIssueProjectItemsGet(t, reg)
396396
mockRepoMetadata(t, reg)
397397
mockIssueUpdate(t, reg)
398-
mockIssueUpdateActorAssignees(t, reg)
398+
mockIssueUpdateApiActors(t, reg)
399399
mockIssueUpdateLabels(t, reg)
400400
mockProjectV2ItemUpdate(t, reg)
401401
},
@@ -444,8 +444,8 @@ func Test_editRun(t *testing.T) {
444444
mockIssueProjectItemsGet(t, reg)
445445
mockIssueUpdate(t, reg)
446446
mockIssueUpdate(t, reg)
447-
mockIssueUpdateActorAssignees(t, reg)
448-
mockIssueUpdateActorAssignees(t, reg)
447+
mockIssueUpdateApiActors(t, reg)
448+
mockIssueUpdateApiActors(t, reg)
449449
mockIssueUpdateLabels(t, reg)
450450
mockIssueUpdateLabels(t, reg)
451451
mockProjectV2ItemUpdate(t, reg)
@@ -608,7 +608,7 @@ func Test_editRun(t *testing.T) {
608608
mockIssueProjectItemsGet(t, reg)
609609
mockRepoMetadata(t, reg)
610610
mockIssueUpdate(t, reg)
611-
mockIssueUpdateActorAssignees(t, reg)
611+
mockIssueUpdateApiActors(t, reg)
612612
mockIssueUpdateLabels(t, reg)
613613
mockProjectV2ItemUpdate(t, reg)
614614
},
@@ -902,7 +902,7 @@ func mockIssueUpdate(t *testing.T, reg *httpmock.Registry) {
902902
)
903903
}
904904

905-
func mockIssueUpdateActorAssignees(t *testing.T, reg *httpmock.Registry) {
905+
func mockIssueUpdateApiActors(t *testing.T, reg *httpmock.Registry) {
906906
reg.Register(
907907
httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`),
908908
httpmock.GraphQLMutation(`

‎pkg/cmd/pr/create/create.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -429,9 +429,9 @@ func createRun(opts *CreateOptions) error {
429429
return err
430430
}
431431

432+
// TODO ApiActorsSupported
432433
if issueFeatures.ActorIsAssignable {
433-
state.ActorReviewers = true
434-
state.ActorAssignees = true
434+
state.ApiActorsSupported = true
435435
}
436436

437437
var openURL string

‎pkg/cmd/pr/create/create_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,7 @@ func Test_createRun(t *testing.T) {
965965
`, func(inputs map[string]interface{}) {
966966
assert.Equal(t, "NEWPULLID", inputs["pullRequestId"])
967967
if _, ok := inputs["assigneeIds"]; ok {
968-
t.Error("did not expect assigneeIds in updatePullRequest when ActorAssignees is true")
968+
t.Error("did not expect assigneeIds in updatePullRequest when ApiActorsSupported is true")
969969
}
970970
assert.Equal(t, []interface{}{"BUGID", "TODOID"}, inputs["labelIds"])
971971
assert.Equal(t, []interface{}{"ROADMAPID"}, inputs["projectIds"])

‎pkg/cmd/pr/edit/edit.go‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -289,9 +289,9 @@ func editRun(opts *EditOptions) error {
289289
editable.Base.Default = pr.BaseRefName
290290
editable.Reviewers.Default = pr.ReviewRequests.DisplayNames()
291291
editable.Reviewers.DefaultLogins = pr.ReviewRequests.Logins()
292-
// TODO actorIsAssignableCleanup
292+
// TODO ApiActorsSupported
293293
if issueFeatures.ActorIsAssignable {
294-
editable.Assignees.ActorAssignees = true
294+
editable.ApiActorsSupported = true
295295
editable.Assignees.Default = pr.AssignedActors.DisplayNames()
296296
editable.Assignees.DefaultLogins = pr.AssignedActors.Logins()
297297
} else {
@@ -438,7 +438,8 @@ func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, pr
438438
// Replace @copilot with the Copilot reviewer login (only on github.com).
439439
// Also use DefaultLogins (not Default display names) for computing the set.
440440
var defaultLogins []string
441-
if editable.Assignees.ActorAssignees {
441+
// TODO ApiActorsSupported
442+
if editable.ApiActorsSupported {
442443
copilotReplacer := shared.NewCopilotReviewerReplacer()
443444
add = copilotReplacer.ReplaceSlice(add)
444445
remove = copilotReplacer.ReplaceSlice(remove)
@@ -457,7 +458,8 @@ func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, pr
457458

458459
// On github.com, use the new GraphQL mutation which supports bots.
459460
// On GHES, fall back to REST API.
460-
if editable.Assignees.ActorAssignees {
461+
// TODO ApiActorsSupported
462+
if editable.ApiActorsSupported {
461463
return updatePullRequestReviewsGraphQL(client, repo, prID, editable)
462464
}
463465
return updatePullRequestReviewsREST(client, repo, number, editable)

‎pkg/cmd/pr/edit/edit_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -417,7 +417,7 @@ func Test_editRun(t *testing.T) {
417417
// REST API accepts logins and team slugs directly
418418
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true})
419419
mockPullRequestUpdate(reg)
420-
mockPullRequestUpdateActorAssignees(reg)
420+
mockPullRequestUpdateApiActors(reg)
421421
mockRequestReviewsByLogin(reg)
422422
mockPullRequestUpdateLabels(reg)
423423
mockProjectV2ItemUpdate(reg)
@@ -475,7 +475,7 @@ func Test_editRun(t *testing.T) {
475475
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
476476
mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true})
477477
mockPullRequestUpdate(reg)
478-
mockPullRequestUpdateActorAssignees(reg)
478+
mockPullRequestUpdateApiActors(reg)
479479
mockPullRequestUpdateLabels(reg)
480480
mockProjectV2ItemUpdate(reg)
481481
},
@@ -551,7 +551,7 @@ func Test_editRun(t *testing.T) {
551551
mockPullRequestUpdate(reg)
552552
mockRequestReviewsByLogin(reg)
553553
mockPullRequestUpdateLabels(reg)
554-
mockPullRequestUpdateActorAssignees(reg)
554+
mockPullRequestUpdateApiActors(reg)
555555
mockProjectV2ItemUpdate(reg)
556556
},
557557
stdout: "https://github.com/OWNER/REPO/pull/123\n",
@@ -756,7 +756,7 @@ func Test_editRun(t *testing.T) {
756756
// (searchFunc handles dynamic fetching, metadata populated in test mock)
757757
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true})
758758
mockPullRequestUpdate(reg)
759-
mockPullRequestUpdateActorAssignees(reg)
759+
mockPullRequestUpdateApiActors(reg)
760760
mockRequestReviewsByLogin(reg)
761761
mockPullRequestUpdateLabels(reg)
762762
mockProjectV2ItemUpdate(reg)
@@ -785,7 +785,7 @@ func Test_editRun(t *testing.T) {
785785
editFields: func(e *shared.Editable, _ string) error {
786786
e.Title.Value = "new title"
787787
e.Body.Value = "new body"
788-
// When ActorAssignees is enabled, the interactive flow returns display names (or logins for non-users)
788+
// When ApiActorsSupported is enabled, the interactive flow returns display names (or logins for non-users)
789789
e.Assignees.Value = []string{"monalisa (Mona Display Name)", "hubot"}
790790
// Populate metadata to simulate what searchFunc would do during prompting
791791
e.Metadata.AssignableActors = []api.AssignableActor{
@@ -808,7 +808,7 @@ func Test_editRun(t *testing.T) {
808808
// assignees: false because searchFunc handles dynamic fetching (metadata populated in test mock)
809809
mockRepoMetadata(reg, mockRepoMetadataOptions{assignees: false, labels: true, projects: true, milestones: true})
810810
mockPullRequestUpdate(reg)
811-
mockPullRequestUpdateActorAssignees(reg)
811+
mockPullRequestUpdateApiActors(reg)
812812
mockPullRequestUpdateLabels(reg)
813813
mockProjectV2ItemUpdate(reg)
814814
},
@@ -876,7 +876,7 @@ func Test_editRun(t *testing.T) {
876876
mockRepoMetadata(reg, mockRepoMetadataOptions{reviewers: false, teamReviewers: false, assignees: false, labels: true, projects: true, milestones: true})
877877
mockPullRequestUpdate(reg)
878878
mockRequestReviewsByLogin(reg)
879-
mockPullRequestUpdateActorAssignees(reg)
879+
mockPullRequestUpdateApiActors(reg)
880880
mockPullRequestUpdateLabels(reg)
881881
mockProjectV2ItemUpdate(reg)
882882
},
@@ -990,7 +990,7 @@ func Test_editRun(t *testing.T) {
990990
return nil
991991
},
992992
editFields: func(e *shared.Editable, _ string) error {
993-
require.False(t, e.Assignees.ActorAssignees)
993+
require.False(t, e.ApiActorsSupported)
994994
require.Nil(t, e.AssigneeSearchFunc)
995995
require.Contains(t, e.Assignees.Options, "monalisa")
996996
require.Contains(t, e.Assignees.Options, "hubot")
@@ -1190,7 +1190,7 @@ type mockRepoMetadataOptions struct {
11901190
}
11911191

11921192
func mockRepoMetadata(reg *httpmock.Registry, opt mockRepoMetadataOptions) {
1193-
// Assignable actors (users/bots) are fetched when reviewers OR assignees edited with ActorAssignees enabled.
1193+
// Assignable actors (users/bots) are fetched when reviewers OR assignees edited with ApiActorsSupported enabled.
11941194
if opt.reviewers || opt.assignees {
11951195
reg.Register(
11961196
httpmock.GraphQL(`query RepositoryAssignableActors\b`),
@@ -1314,7 +1314,7 @@ func mockPullRequestUpdate(reg *httpmock.Registry) {
13141314
httpmock.StringResponse(`{}`))
13151315
}
13161316

1317-
func mockPullRequestUpdateActorAssignees(reg *httpmock.Registry) {
1317+
func mockPullRequestUpdateApiActors(reg *httpmock.Registry) {
13181318
reg.Register(
13191319
httpmock.GraphQL(`mutation ReplaceActorsForAssignable\b`),
13201320
httpmock.GraphQLMutation(`
@@ -1336,7 +1336,7 @@ func mockPullRequestRemoveReviewers(reg *httpmock.Registry) {
13361336
}
13371337

13381338
// mockRequestReviewsByLogin mocks the RequestReviewsByLogin GraphQL mutation
1339-
// used on github.com when ActorAssignees is enabled.
1339+
// used on github.com when ApiActorsSupported is enabled.
13401340
func mockRequestReviewsByLogin(reg *httpmock.Registry) {
13411341
reg.Register(
13421342
httpmock.GraphQL(`mutation RequestReviewsByLogin\b`),

0 commit comments

Comments
 (0)