From 1035ff551b0e6de477414dbb744cc4719fc4af20 Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 11 Oct 2026 23:14:40 +0200 Subject: [PATCH] fix(milestone): address milestones by title or ID everywhere milestone view v2.0 failed with "invalid resource number" although its usage promised , and issue create --milestone v1.0 failed with a strconv error. Every milestone argument (view, edit, delete, close, reopen, milestone issue add|remove|list) and issue create|edit -m/--milestone now accept a title or an ID: a number is an ID first, then a title; --by-name matches numeric titles only. --milestone-name stays as a hidden name of --milestone, and issue edit gains --remove-milestone (as does --milestone 0). Signed-off-by: bircni --- internal/app/human_output_test.go | 4 +- internal/app/issue_edit.go | 26 +++---- internal/app/issue_inputs_test.go | 4 ++ internal/app/issues.go | 44 +++++++++--- internal/app/metadata.go | 47 +++++-------- internal/app/milestone_bulk_test.go | 10 ++- internal/app/milestone_description_test.go | 2 + internal/app/milestone_issues.go | 46 ++++-------- internal/app/milestone_names_test.go | 82 ++++++++++++++++++++-- internal/app/milestone_state.go | 51 ++++---------- internal/app/validation_test.go | 10 ++- internal/services/milestone_selection.go | 41 ++++++++--- 12 files changed, 220 insertions(+), 147 deletions(-) diff --git a/internal/app/human_output_test.go b/internal/app/human_output_test.go index 5e14edf4..dd60f7e4 100644 --- a/internal/app/human_output_test.go +++ b/internal/app/human_output_test.go @@ -151,8 +151,8 @@ func TestMultiItemHumanOutputPagesOnce(t *testing.T) { } server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { - case "/api/v1/repos/alice/project/milestones/3": - _, _ = w.Write([]byte(`{"id":3,"title":"v1"}`)) + case "/api/v1/repos/alice/project/milestones": + writeMilestones(w, r, `[{"id":3,"title":"v1"}]`) case "/api/v1/repos/alice/project/issues": _, _ = w.Write([]byte("[" + issue(1) + "," + issue(2) + "]")) case "/api/v1/repos/alice/project/issues/1", "/api/v1/repos/alice/project/issues/2": diff --git a/internal/app/issue_edit.go b/internal/app/issue_edit.go index 48227ad6..6036ad43 100644 --- a/internal/app/issue_edit.go +++ b/internal/app/issue_edit.go @@ -20,8 +20,8 @@ func (a *application) issueEditCommand() *cobra.Command { var useEditor bool var deadline string var ref string - var milestone int64 - var milestoneName string + var milestone string + var removeMilestone bool var metadata metadataFlags cmd := &cobra.Command{Use: "edit ...", Short: "Edit issue content, assignees, and milestone", Args: usageArgs(cobra.MinimumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) (runErr error) { var draft terminal.Draft @@ -46,10 +46,14 @@ func (a *application) issueEditCommand() *cobra.Command { if useEditor && len(numbers) != 1 { return clierr.Wrap(clierr.Usage, errors.New("--editor requires one issue")) } - if err := validateMilestoneFlags(cmd, milestoneName, milestone); err != nil { + milestone, removeMilestone, err := milestoneFlag(cmd, milestone) + if err != nil { return err } opts := services.IssueEdit{} + if removeMilestone { + opts.Milestone = new(int64) + } if cmd.Flags().Changed("ref") { opts.Ref = &ref } @@ -72,25 +76,22 @@ func (a *application) issueEditCommand() *cobra.Command { } opts.Body = &body } - if cmd.Flags().Changed("milestone") { - opts.Milestone = &milestone - } if err := metadata.validate(cmd); err != nil { return err } - if opts == (services.IssueEdit{}) && !useEditor && !metadata.changed(cmd) && !cmd.Flags().Changed("milestone-name") { + if opts == (services.IssueEdit{}) && !useEditor && !metadata.changed(cmd) && milestone == "" { return clierr.Wrap(clierr.Usage, errors.New("supply an edit flag; see tea issue edit --help")) } target, err := a.resolve(cmd.Context(), true) if err != nil { return err } - if cmd.Flags().Changed("milestone-name") { - milestone, err = (services.Milestones{Client: target.client, Repository: target.Repo}).ResolveName(cmd.Context(), milestoneName) + if milestone != "" { + id, err := milestoneID(cmd, target, milestone) if err != nil { return err } - opts.Milestone = &milestone + opts.Milestone = &id } if opts.Labels, opts.Assignees, opts.Changes, err = metadata.resolve(cmd, target); err != nil { return err @@ -117,8 +118,9 @@ func (a *application) issueEditCommand() *cobra.Command { cmd.Flags().StringVar(&body, "body", "", "Issue body (empty clears)") cmd.Flags().StringVar(&file, "body-file", "", "Read body from file, or - for stdin") cmd.Flags().BoolVar(&useEditor, "editor", false, "Edit the current issue body in the configured editor") - cmd.Flags().StringVar(&milestoneName, "milestone-name", "", "Milestone by exact title; conflicts with --milestone") - cmd.Flags().Int64Var(&milestone, "milestone", 0, "Milestone ID; 0 clears") + cmd.Flags().StringVarP(&milestone, "milestone", "m", "", "Move the issue to a milestone by `title` or ID") + cmd.Flags().BoolVar(&removeMilestone, "remove-milestone", false, "Remove the issue from its milestone") + flagAliases(cmd, map[string]string{"milestone-name": "milestone"}) metadata.register(cmd) return cmd } diff --git a/internal/app/issue_inputs_test.go b/internal/app/issue_inputs_test.go index feaa4e06..084c7a10 100644 --- a/internal/app/issue_inputs_test.go +++ b/internal/app/issue_inputs_test.go @@ -16,6 +16,10 @@ import ( func TestIssueCreationMetadataAndReferenceClearing(t *testing.T) { calls := 0 server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/repos/owner/repo/milestones" { + writeMilestones(w, r, `[{"id":4,"title":"Release"}]`) + return + } calls++ if calls == 1 { require.Equal(t, http.MethodPost, r.Method) diff --git a/internal/app/issues.go b/internal/app/issues.go index 7984163f..78b2fa64 100644 --- a/internal/app/issues.go +++ b/internal/app/issues.go @@ -80,7 +80,31 @@ func issueTemplate(ctx context.Context, service services.Issues, name string) (s return services.IssueTemplate{}, fmt.Errorf("issue template %q does not exist; available: %s", name, strings.Join(names, ", ")) } -// validateMilestoneFlags checks --milestone and --milestone-name before any request. +// milestoneFlag reads -m/--milestone, a milestone title or ID, before any request: value is +// "" when the flag is absent, and remove reports 0 or --remove-milestone, which clear it. +func milestoneFlag(cmd *cobra.Command, value string) (selected string, remove bool, err error) { + remove, _ = cmd.Flags().GetBool("remove-milestone") + switch { + case !cmd.Flags().Changed("milestone"): + return "", remove, nil + case remove: + return "", false, clierr.Wrap(clierr.Usage, errors.New("--milestone and --remove-milestone cannot be combined")) + case strings.TrimSpace(value) == "": + return "", false, clierr.Wrap(clierr.Usage, errors.New("--milestone needs a title or ID; use --remove-milestone to clear it")) + case strings.TrimSpace(value) == "0": + return "", true, nil + } + return strings.TrimSpace(value), false, nil +} + +// milestoneID resolves a milestone title or ID in the target repository. +func milestoneID(cmd *cobra.Command, t target, raw string) (int64, error) { + milestone, err := (services.Milestones{Client: t.client, Repository: t.Repo}).FindOne(cmd.Context(), raw, false) + return milestone.ID, err +} + +// validateMilestoneFlags checks the --milestone ID and --milestone-name flags of pr create and pr +// edit before any request. func validateMilestoneFlags(cmd *cobra.Command, name string, id int64) error { if cmd.Flags().Changed("milestone-name") { if cmd.Flags().Changed("milestone") { @@ -301,15 +325,15 @@ func (a *application) issueCommand() *cobra.Command { var useEditor bool var ref string var assignees, labelNames, attachmentPaths []string - var milestone int64 - var milestoneName string + var milestone string create := &cobra.Command{Use: "create", Short: "Create a repository issue", Args: argsExact(0), RunE: func(cmd *cobra.Command, _ []string) (runErr error) { var draft terminal.Draft defer finishDraft(&draft, &runErr) if err := validateBodySources(cmd); err != nil { return err } - if err := validateMilestoneFlags(cmd, milestoneName, milestone); err != nil { + milestone, _, err := milestoneFlag(cmd, milestone) + if err != nil { return err } if err := requireNames("label", "", labelNames); err != nil { @@ -381,9 +405,9 @@ func (a *application) issueCommand() *cobra.Command { return err } } - if cmd.Flags().Changed("milestone-name") { - milestone, err = (services.Milestones{Client: target.client, Repository: target.Repo}).ResolveName(cmd.Context(), milestoneName) - if err != nil { + var milestoneValue int64 + if milestone != "" { + if milestoneValue, err = milestoneID(cmd, target, milestone); err != nil { return err } } @@ -397,7 +421,7 @@ func (a *application) issueCommand() *cobra.Command { } } service := services.Issues{Client: target.client, Repository: target.Repo} - item, err := service.Create(cmd.Context(), services.IssueOptions{Title: title, Body: body, Deadline: due, Ref: ref, Assignees: assignees, Milestone: milestone, Labels: labelIDs}) + item, err := service.Create(cmd.Context(), services.IssueOptions{Title: title, Body: body, Deadline: due, Ref: ref, Assignees: assignees, Milestone: milestoneValue, Labels: labelIDs}) if err != nil { return err } @@ -416,8 +440,8 @@ func (a *application) issueCommand() *cobra.Command { create.Flags().StringVar(&ref, "ref", "", "Referenced branch or tag") create.Flags().StringSliceVar(&labelNames, "label", nil, "Apply labels by exact name; use repo: or org: to disambiguate") create.Flags().StringSliceVar(&assignees, "assignee", nil, "Assign users by username") - create.Flags().StringVar(&milestoneName, "milestone-name", "", "Milestone by exact title; conflicts with --milestone") - create.Flags().Int64Var(&milestone, "milestone", 0, "Milestone ID") + create.Flags().StringVarP(&milestone, "milestone", "m", "", "Add the issue to a milestone by `title` or ID") + flagAliases(create, map[string]string{"milestone-name": "milestone"}) create.Flags().StringVar(&deadline, "deadline", "", "Due date: YYYY-MM-DD, RFC3339, or Unix timestamp") create.Flags().StringVar(&body, "body", "", "Issue body") create.Flags().StringVar(&bodyFile, "body-file", "", "Read body from a file, or - for stdin") diff --git a/internal/app/metadata.go b/internal/app/metadata.go index 88cc6752..6e822839 100644 --- a/internal/app/metadata.go +++ b/internal/app/metadata.go @@ -205,8 +205,8 @@ func (a *application) milestoneCommand() *cobra.Command { if verb == "create" { command.Args = argsExact(0) } else { - command.Use += " " - command.Flags().BoolVar(&byName, "by-name", false, "Select by exact milestone title instead of ID") + command.Use += " " + command.Flags().BoolVar(&byName, "by-name", false, "Match a numeric argument as a milestone title rather than an ID") command.Args = argsExact(1) } if verb == "create" || verb == "edit" { @@ -228,12 +228,8 @@ func (a *application) milestoneCommand() *cobra.Command { } } opts := services.MilestoneOptions{} - var id int64 - if verb != "create" { - var err error - if id, err = milestoneSelector(args[0], byName); err != nil { - return err - } + if verb != "create" && strings.TrimSpace(args[0]) == "" { + return clierr.Wrap(clierr.Usage, errors.New("milestone title or ID cannot be empty")) } var interactiveDeadline *time.Time descriptionChanged := false @@ -297,38 +293,27 @@ func (a *application) milestoneCommand() *cobra.Command { if err != nil { return err } - if byName { - id, err = s.ResolveName(cmd.Context(), args[0]) - if err != nil { + var current services.Milestone + if verb != "create" { + if current, err = s.FindOne(cmd.Context(), args[0], byName); err != nil { return err } } + id := current.ID if verb == "delete" { if err := a.confirm(cmd, "Delete milestone "+args[0]); err != nil { return err } - if a.options.json != "" { - item, err := s.View(cmd.Context(), id) - if err != nil { - return err - } - if err := s.Delete(cmd.Context(), id); err != nil { - return err - } - return writeItem(a, cmd, item, milestoneRow) + if err := s.Delete(cmd.Context(), id); err != nil { + return err } - return s.Delete(cmd.Context(), id) + if a.options.json != "" { + return writeItem(a, cmd, current, milestoneRow) + } + return nil } if useEditor { - initial := "" - if verb == "edit" { - current, viewErr := s.View(cmd.Context(), id) - if viewErr != nil { - return viewErr - } - initial = current.Description - } - editorDraft, err = a.editDraft(cmd, initial) + editorDraft, err = a.editDraft(cmd, current.Description) if err != nil { return err } @@ -340,7 +325,7 @@ func (a *application) milestoneCommand() *cobra.Command { } var item services.Milestone if verb == "view" { - item, err = s.View(cmd.Context(), id) + item = current } else if verb == "create" { item, err = s.Create(cmd.Context(), opts) } else { diff --git a/internal/app/milestone_bulk_test.go b/internal/app/milestone_bulk_test.go index eba69b7c..c8f6cf30 100644 --- a/internal/app/milestone_bulk_test.go +++ b/internal/app/milestone_bulk_test.go @@ -17,9 +17,13 @@ import ( func TestBulkMilestoneStateContinuesAfterFailureAndKeepsJSONClean(t *testing.T) { mutations := []string{} server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/v1/repos/alice/project/milestones" { + writeMilestones(w, r, `[{"id":9,"title":"v9"},{"id":10,"title":"v10"},{"id":11,"title":"v11"}]`) + return + } id := strings.TrimPrefix(r.URL.Path, "/api/v1/repos/alice/project/milestones/") if r.Method == http.MethodGet { - _, _ = fmt.Fprintf(w, `{"id":%s,"title":"Milestone %s"}`, id, id) + _, _ = fmt.Fprintf(w, `{"id":%s,"title":"v%s"}`, id, id) return } mutations = append(mutations, id) @@ -32,7 +36,7 @@ func TestBulkMilestoneStateContinuesAfterFailureAndKeepsJSONClean(t *testing.T) defer server.Close() deps, out, diagnostics := serverDeps(t, server.URL) err := Execute(context.Background(), deps, []string{"milestone", "close", "9", "10", "11", "-R", "alice/project", "--json=id,state"}) - require.ErrorContains(t, err, "milestone 10") + require.ErrorContains(t, err, "milestone v10") require.Equal(t, clierr.Failure, clierr.ExitCode(err)) require.Equal(t, []string{"9", "10", "11"}, mutations) require.JSONEq(t, `[{"id":9,"state":"closed"},{"id":11,"state":"closed"}]`, out.String()) @@ -40,7 +44,7 @@ func TestBulkMilestoneStateContinuesAfterFailureAndKeepsJSONClean(t *testing.T) mutations = nil out.Reset() err = Execute(context.Background(), deps, []string{"milestone", "close", "9", "bad", "-R", "alice/project"}) - require.Equal(t, clierr.Usage, clierr.ExitCode(err)) + require.ErrorContains(t, err, `milestone "bad" not found in alice/project`) require.Empty(t, mutations) require.Empty(t, out.String()) } diff --git a/internal/app/milestone_description_test.go b/internal/app/milestone_description_test.go index c58391d9..f1ba4e56 100644 --- a/internal/app/milestone_description_test.go +++ b/internal/app/milestone_description_test.go @@ -43,6 +43,8 @@ func TestMilestoneCreateDescriptionFileAndEditor(t *testing.T) { func TestMilestoneEditEditorSeedsDescription(t *testing.T) { server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch { + case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/milestones": + writeMilestones(w, r, `[{"id":4,"title":"Ship","description":"current notes","state":"open"}]`) case r.Method == http.MethodGet && r.URL.Path == "/api/v1/repos/alice/project/milestones/4": _, _ = w.Write([]byte(`{"id":4,"title":"Ship","description":"current notes","state":"open"}`)) case r.Method == http.MethodPatch && r.URL.Path == "/api/v1/repos/alice/project/milestones/4": diff --git a/internal/app/milestone_issues.go b/internal/app/milestone_issues.go index af708ff9..cd705fab 100644 --- a/internal/app/milestone_issues.go +++ b/internal/app/milestone_issues.go @@ -4,7 +4,6 @@ package app import ( - "context" "errors" "strconv" "strings" @@ -14,35 +13,19 @@ import ( "github.com/spf13/cobra" ) -// milestoneSelector validates a milestone ID or, with byName, a title before any request. -func milestoneSelector(selector string, byName bool) (int64, error) { - if byName { - if strings.TrimSpace(selector) == "" { - return 0, clierr.Wrap(clierr.Usage, errors.New("milestone name cannot be empty")) - } - return 0, nil +// findMilestone resolves a milestone argument, a title or an ID, in the target repository. +func findMilestone(cmd *cobra.Command, t target, raw string, byName bool) (services.Milestone, error) { + if strings.TrimSpace(raw) == "" { + return services.Milestone{}, clierr.Wrap(clierr.Usage, errors.New("milestone title or ID cannot be empty")) } - return number(selector) -} - -// existingMilestone resolves a title, or confirms that an ID exists. -func existingMilestone(ctx context.Context, milestones services.Milestones, selector string, id int64, byName bool) (int64, error) { - if byName { - return milestones.ResolveName(ctx, selector) - } - _, err := milestones.View(ctx, id) - return id, err + return (services.Milestones{Client: t.client, Repository: t.Repo}).FindOne(cmd.Context(), raw, byName) } func (a *application) milestoneIssueCommand() *cobra.Command { group := &cobra.Command{Use: "issue", Short: "Manage milestone issues and pull requests"} opts := services.ListOptions{} var byName bool - list := &cobra.Command{Use: "list ", Short: "List issues and pull requests in a milestone", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { - id, err := milestoneSelector(args[0], byName) - if err != nil { - return err - } + list := &cobra.Command{Use: "list ", Short: "List issues and pull requests in a milestone", Args: argsExact(1), RunE: func(cmd *cobra.Command, args []string) error { if err := oneOf("state", opts.State, "open", "closed", "all"); err != nil { return err } @@ -53,11 +36,11 @@ func (a *application) milestoneIssueCommand() *cobra.Command { if err != nil { return err } - id, err = existingMilestone(cmd.Context(), services.Milestones{Client: target.client, Repository: target.Repo}, args[0], id, byName) + milestone, err := findMilestone(cmd, target, args[0], byName) if err != nil { return err } - opts.Milestones = strconv.FormatInt(id, 10) + opts.Milestones = strconv.FormatInt(milestone.ID, 10) items, more, err := (services.Issues{Client: target.client, Repository: target.Repo}).List(cmd.Context(), opts) if err != nil { return err @@ -70,15 +53,11 @@ func (a *application) milestoneIssueCommand() *cobra.Command { pageFlags(list, &opts.Limit, &opts.Page) list.Flags().StringVar(&opts.State, "state", "open", "open, closed, or all") list.Flags().StringVar(&opts.Kind, "kind", "all", "all, issues, or pulls") - list.Flags().BoolVar(&byName, "by-name", false, "Select by exact milestone title instead of ID") + list.Flags().BoolVar(&byName, "by-name", false, "Match a numeric argument as a milestone title rather than an ID") group.AddCommand(list) for _, verb := range []string{"add", "remove"} { var byName bool - command := &cobra.Command{Use: verb + " ...", Short: titleCase(verb) + " issues or pull requests in a milestone", Args: usageArgs(cobra.MinimumNArgs(2)), RunE: func(cmd *cobra.Command, args []string) error { - id, err := milestoneSelector(args[0], byName) - if err != nil { - return err - } + command := &cobra.Command{Use: verb + " ...", Short: titleCase(verb) + " issues or pull requests in a milestone", Args: usageArgs(cobra.MinimumNArgs(2)), RunE: func(cmd *cobra.Command, args []string) error { numbers, err := resourceNumbers(args[1:]) if err != nil { return err @@ -87,10 +66,11 @@ func (a *application) milestoneIssueCommand() *cobra.Command { if err != nil { return err } - id, err = existingMilestone(cmd.Context(), services.Milestones{Client: target.client, Repository: target.Repo}, args[0], id, byName) + milestone, err := findMilestone(cmd, target, args[0], byName) if err != nil { return err } + id := milestone.ID service := services.Issues{Client: target.client, Repository: target.Repo} return batch(a, cmd, numbers, issueLabel, func(n int64) (services.Issue, error) { if verb == "remove" { @@ -99,7 +79,7 @@ func (a *application) milestoneIssueCommand() *cobra.Command { return service.Edit(cmd.Context(), n, services.IssueEdit{Milestone: &id}) }, issueRow) }} - command.Flags().BoolVar(&byName, "by-name", false, "Select by exact milestone title instead of ID") + command.Flags().BoolVar(&byName, "by-name", false, "Match a numeric argument as a milestone title rather than an ID") group.AddCommand(command) } return group diff --git a/internal/app/milestone_names_test.go b/internal/app/milestone_names_test.go index 3b366aad..09561890 100644 --- a/internal/app/milestone_names_test.go +++ b/internal/app/milestone_names_test.go @@ -6,6 +6,7 @@ package app import ( "context" "encoding/json" + "fmt" "net/http" "testing" @@ -58,12 +59,13 @@ func TestMilestoneDeleteReturnsJSONSnapshotOnlyAfterSuccess(t *testing.T) { calls := []string{} fail := false server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "/api/v1/repos/alice/project/milestones/9", r.URL.Path) calls = append(calls, r.Method) if r.Method == http.MethodGet { - _, _ = w.Write([]byte(`{"id":9,"title":"Release one","state":"open"}`)) + require.Equal(t, "/api/v1/repos/alice/project/milestones", r.URL.Path) + writeMilestones(w, r, `[{"id":9,"title":"Release one","state":"open"}]`) return } + require.Equal(t, "/api/v1/repos/alice/project/milestones/9", r.URL.Path) require.Equal(t, http.MethodDelete, r.Method) if fail { w.WriteHeader(403) @@ -76,7 +78,7 @@ func TestMilestoneDeleteReturnsJSONSnapshotOnlyAfterSuccess(t *testing.T) { args := []string{"milestone", "delete", "9", "-R", "alice/project", "--yes", "--json=id,title"} require.NoError(t, Execute(context.Background(), deps, args)) require.JSONEq(t, `{"id":9,"title":"Release one"}`, out.String()) - require.Equal(t, []string{"GET", "DELETE"}, calls) + require.Equal(t, []string{"GET", "GET", "DELETE"}, calls) fail = true out.Reset() require.Error(t, Execute(context.Background(), deps, args)) @@ -113,5 +115,77 @@ func TestMilestoneIssueCommandsSelectByName(t *testing.T) { require.NoError(t, Execute(context.Background(), deps, []string{"milestone", "issue", "add", "Release one", "3", "--by-name", "-R", "alice/project", "--json=number"})) require.NotContains(t, calls, "GET /api/v1/repos/alice/project/milestones/9") err := Execute(context.Background(), deps, []string{"milestone", "issue", "remove", " ", "3", "--by-name", "-R", "alice/project"}) - require.ErrorContains(t, err, "milestone name cannot be empty") + require.ErrorContains(t, err, "milestone title or ID cannot be empty") +} + +// writeMilestones answers a milestone listing with page, a JSON array, as its first and only page. +func writeMilestones(w http.ResponseWriter, r *http.Request, page string) { + if r.URL.Query().Get("page") != "1" { + page = `[]` + } + _, _ = w.Write([]byte(page)) +} + +// payloadField returns the JSON request body's field name as text. +func payloadField(t *testing.T, r *http.Request, name string) string { + t.Helper() + var payload map[string]any + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + return fmt.Sprint(payload[name]) +} + +func TestMilestonesAcceptTitlesOrIDs(t *testing.T) { + var calls []string + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Method + " " + r.URL.Path { + case "GET /api/v1/repos/alice/project/milestones": + writeMilestones(w, r, `[{"id":3,"title":"v1.0","state":"open"},{"id":4,"title":"2026","state":"closed"},{"id":5,"title":"v2.0"}]`) + case "GET /api/v1/repos/alice/project/milestones/3", "GET /api/v1/repos/alice/project/milestones/4": + _, _ = w.Write([]byte(`{"id":3,"title":"v1.0","state":"open"}`)) + case "PATCH /api/v1/repos/alice/project/milestones/3", "PATCH /api/v1/repos/alice/project/milestones/4": + calls = append(calls, r.Method+" "+r.URL.Path) + _, _ = w.Write([]byte(`{"id":3,"title":"v1.0","state":"closed"}`)) + case "POST /api/v1/repos/alice/project/issues": + requirePayload(t, `{"title":"x","milestone":3}`, r.Body) + calls = append(calls, r.Method+" "+r.URL.Path) + _, _ = w.Write([]byte(`{"number":8}`)) + case "PATCH /api/v1/repos/alice/project/issues/8": + calls = append(calls, r.Method+" "+r.URL.Path+" milestone="+payloadField(t, r, "milestone")) + _, _ = w.Write([]byte(`{"number":8}`)) + default: + t.Errorf("unexpected %s %s", r.Method, r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + defer server.Close() + deps, out, _ := serverDeps(t, server.URL) + run := func(args ...string) error { + return Execute(context.Background(), deps, append(args, "-R", "alice/project", "--json", "--yes")) + } + require.NoError(t, run("milestone", "view", "v1.0")) + require.Contains(t, out.String(), `"id":3`) + require.NoError(t, run("milestone", "close", "V1.0", "2026")) + require.NoError(t, run("issue", "create", "--title", "x", "-m", "v1.0")) + require.NoError(t, run("issue", "edit", "8", "--milestone-name", "3")) + require.NoError(t, run("issue", "edit", "8", "--remove-milestone")) + require.NoError(t, run("issue", "edit", "8", "--milestone", "0")) + require.Equal(t, []string{ + "PATCH /api/v1/repos/alice/project/milestones/3", + "PATCH /api/v1/repos/alice/project/milestones/4", + "POST /api/v1/repos/alice/project/issues", + "PATCH /api/v1/repos/alice/project/issues/8 milestone=3", + "PATCH /api/v1/repos/alice/project/issues/8 milestone=0", + "PATCH /api/v1/repos/alice/project/issues/8 milestone=0", + }, calls) + for _, test := range []struct { + args []string + want string + }{ + {[]string{"milestone", "view", "v9"}, `milestone "v9" not found in alice/project`}, + {[]string{"milestone", "close", "v1.0", "3"}, "milestone 3 is selected more than once"}, + {[]string{"issue", "create", "--title", "x", "--milestone", "nope"}, `milestone "nope" not found in alice/project`}, + } { + require.ErrorContains(t, run(test.args...), test.want) + } + require.Len(t, calls, 6) } diff --git a/internal/app/milestone_state.go b/internal/app/milestone_state.go index 89d201a5..b1a18fa1 100644 --- a/internal/app/milestone_state.go +++ b/internal/app/milestone_state.go @@ -5,6 +5,8 @@ package app import ( "errors" + "slices" + "strings" "gitea.dev/tea/internal/clierr" "gitea.dev/tea/internal/services" @@ -13,54 +15,31 @@ import ( func (a *application) milestoneStateCommand(verb string, service func(*cobra.Command) (services.Milestones, error)) *cobra.Command { var byName bool - cmd := &cobra.Command{Use: verb + " ...", Short: titleCase(verb) + " one or more milestones", Args: usageArgs(cobra.MinimumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { - ids := make(map[string]int64, len(args)) - seenIDs := map[int64]bool{} - selectID := func(argument string, id int64) error { - if seenIDs[id] { - return clierr.Wrap(clierr.Usage, errors.New("selectors resolve to the same milestone")) - } - seenIDs[id], ids[argument] = true, id - return nil - } - for _, argument := range args { - if _, seen := ids[argument]; seen { - return clierr.Wrap(clierr.Usage, errors.New("milestone selectors must be unique")) - } - id, err := milestoneSelector(argument, byName) - if err != nil { - return err - } - ids[argument] = 0 - if !byName { - if err := selectID(argument, id); err != nil { - return err - } - } + cmd := &cobra.Command{Use: verb + " ...", Short: titleCase(verb) + " one or more milestones", Args: usageArgs(cobra.MinimumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { + if slices.ContainsFunc(args, func(arg string) bool { return strings.TrimSpace(arg) == "" }) { + return clierr.Wrap(clierr.Usage, errors.New("milestone title or ID cannot be empty")) } milestones, err := service(cmd) if err != nil { return err } - if byName { - for _, argument := range args { - id, err := milestones.ResolveName(cmd.Context(), argument) - if err != nil { - return err - } - if err := selectID(argument, id); err != nil { - return err - } + selected, err := milestones.Find(cmd.Context(), args, byName) + if err != nil { + return err + } + for i, milestone := range selected { + if slices.ContainsFunc(selected[:i], func(other services.Milestone) bool { return other.ID == milestone.ID }) { + return clierr.Wrap(clierr.Usage, errors.New("milestone "+args[i]+" is selected more than once")) } } state := "closed" if verb == "reopen" { state = "open" } - return batch(a, cmd, args, func(argument string) string { return "milestone " + argument }, func(argument string) (services.Milestone, error) { - return milestones.Edit(cmd.Context(), ids[argument], services.MilestoneOptions{State: &state}) + return batch(a, cmd, selected, func(milestone services.Milestone) string { return "milestone " + milestone.Title }, func(milestone services.Milestone) (services.Milestone, error) { + return milestones.Edit(cmd.Context(), milestone.ID, services.MilestoneOptions{State: &state}) }, milestoneRow) }} - cmd.Flags().BoolVar(&byName, "by-name", false, "Select by exact milestone titles instead of IDs") + cmd.Flags().BoolVar(&byName, "by-name", false, "Match numeric arguments as milestone titles rather than IDs") return cmd } diff --git a/internal/app/validation_test.go b/internal/app/validation_test.go index d9ea5fdb..b6dcafad 100644 --- a/internal/app/validation_test.go +++ b/internal/app/validation_test.go @@ -51,12 +51,12 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"issue", "create", "--no-input"}, "missing required input: supply --title"}, {[]string{"issue", "create", "--title", "safe", "--json=unknown"}, "unknown JSON field \"unknown\""}, {[]string{"issue", "create", "--title", "safe", "--json", "--jq", "["}, "invalid --jq expression"}, - {[]string{"issue", "create", "--milestone=-1"}, "--milestone must be a positive ID, or 0 for none"}, + {[]string{"issue", "create", "--milestone="}, "--milestone needs a title or ID; use --remove-milestone to clear it"}, {[]string{"issue", "create", "--assignee", "alice, "}, "assignee names cannot be empty"}, {[]string{"issue", "create", "--template="}, "--template cannot be empty"}, {[]string{"issue", "create", "--title", "x", "--attachment", "."}, ". is not a regular file"}, - {[]string{"issue", "create", "--milestone-name="}, "--milestone-name cannot be empty"}, - {[]string{"issue", "create", "--milestone-name=Release", "--milestone=1"}, "--milestone and --milestone-name are mutually exclusive"}, + {[]string{"issue", "create", "--milestone-name="}, "--milestone needs a title or ID"}, + {[]string{"issue", "edit", "1", "--milestone", "v1", "--remove-milestone"}, "--milestone and --remove-milestone cannot be combined"}, {[]string{"issue", "edit", "1"}, "supply an edit flag"}, {[]string{"issue", "edit", "1", "--editor=false"}, "supply an edit flag"}, {[]string{"issue", "edit", "1", "--add-labels="}, "supply an edit flag"}, @@ -73,9 +73,7 @@ func TestUsageErrorsPrecedeRequests(t *testing.T) { {[]string{"label", "create", "--name", "bug", "--color", "wrong"}, "--color requires six hexadecimal digits"}, {[]string{"label", "edit", "1"}, "supply label fields to edit"}, {[]string{"label", "list", "--org", "team", "-R", "a/b"}, "--org and --repo are mutually exclusive"}, - {[]string{"milestone", "close", "9", "09"}, "selectors resolve to the same milestone"}, - {[]string{"milestone", "close", " ", "--by-name"}, "milestone name cannot be empty"}, - {[]string{"milestone", "reopen", "v1", "v1", "--by-name"}, "milestone selectors must be unique"}, + {[]string{"milestone", "close", " ", "--by-name"}, "milestone title or ID cannot be empty"}, {[]string{"milestone", "create", "--title", "v1", "--deadline", "invalid"}, "invalid --deadline"}, {[]string{"milestone", "create", "--title", "x", "--state", "merged", "--no-input"}, "--state must be open or closed"}, {[]string{"milestone", "edit", "1"}, "supply milestone fields to edit"}, diff --git a/internal/services/milestone_selection.go b/internal/services/milestone_selection.go index b97e2455..f81ad4fa 100644 --- a/internal/services/milestone_selection.go +++ b/internal/services/milestone_selection.go @@ -40,22 +40,43 @@ func (s Milestones) All(ctx context.Context) ([]Milestone, error) { return listAll(func(page int) ([]Milestone, error) { return s.List(ctx, "all", 100, page) }) } -// FilterIDs resolves the milestones that filter an issue listing, each an ID or a title, to their -// IDs. Gitea drops filter values that name no milestone and lists every issue when none is left, -// so an unknown milestone is an error here. -func (s Milestones) FilterIDs(ctx context.Context, values []string) ([]string, error) { +// Find returns the milestones that raws address among the open and closed milestones, each by ID +// or title (D10); byName matches numeric values as titles only. +func (s Milestones) Find(ctx context.Context, raws []string, byName bool) ([]Milestone, error) { items, err := s.All(ctx) if err != nil { return nil, err } - finder := NameOrID[Milestone]{Kind: "milestone", Scope: s.Repository, ID: func(m Milestone) int64 { return m.ID }, Name: func(m Milestone) string { return m.Title }} - ids := make([]string, len(values)) - for i, value := range values { - milestone, err := finder.Find(items, value) - if err != nil { + finder := NameOrID[Milestone]{Kind: "milestone", Scope: s.Repository, ID: func(m Milestone) int64 { return m.ID }, Name: func(m Milestone) string { return m.Title }, ByName: byName} + result := make([]Milestone, len(raws)) + for i, raw := range raws { + if result[i], err = finder.Find(items, raw); err != nil { return nil, err } - ids[i] = strconv.FormatInt(milestone.ID, 10) + } + return result, nil +} + +// FindOne returns the milestone that raw addresses like Find. +func (s Milestones) FindOne(ctx context.Context, raw string, byName bool) (Milestone, error) { + items, err := s.Find(ctx, []string{raw}, byName) + if err != nil { + return Milestone{}, err + } + return items[0], nil +} + +// FilterIDs resolves the milestones that filter an issue listing, each an ID or a title, to their +// IDs. Gitea drops filter values that name no milestone and lists every issue when none is left, +// so an unknown milestone is an error here. +func (s Milestones) FilterIDs(ctx context.Context, values []string) ([]string, error) { + items, err := s.Find(ctx, values, false) + if err != nil { + return nil, err + } + ids := make([]string, len(items)) + for i, item := range items { + ids[i] = strconv.FormatInt(item.ID, 10) } return ids, nil }