feat: correct a title from the terminal — `mr edit --title` and `issue edit` (#7)Unverified
The server has shipped `PATCH /merge-requests/{n}` and `PATCH /issues/{n}` since
rickub/rickub!112, but the CLI half was dropped when `cli/` left the monorepo. So
a wrong merge-request title was uncorrectable without a browser, and an issue's
title *and* description were both terminal-uneditable — `issue edit` did not exist
at all.
Both commands now take `--title` and `--body`, each optional, at least one
required:
- `internal/api` gains `UpdateMergeRequest` / `UpdateIssue` over the PATCH routes,
with `MergeRequestPatch` / `IssuePatch` whose fields are POINTERS. That is the
load-bearing detail: a flag the user did not pass is absent from the JSON, not
sent as "". An empty title would trip the API's `title_required`; an empty body
would blank a description nobody asked to blank.
- One resolver (`cmd/edit.go`) serves both commands, for the same reason
cmd/body.go exists — a second reading of "the user didn't pass --title" is how
the two halves drift. `-b -` still reads stdin and a blank body is still refused
locally (issue #94); a blank title is refused too; passing neither flag exits
non-zero and sends NO request rather than collecting the server's 400.
- The success line reports what the server echoed back — the new title, the stored
byte count, or both. A success line that does not prove the write landed is what
made issue #94 invisible.
- `UpdateMergeRequestBody` (the older single-field POST .../body helper) goes: the
PATCH route supersedes it and nothing else called it.
Tests assert the method, the path, and that a flag left off does not appear in the
payload at all — plus the local refusals, which are checked by asserting ZERO
requests reached the server.
Closes #7.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KPPhRdTiUiiSvLjthotkT10e5a942 parent: 93ced54 modified
README.md +6 -2 | @@ -163,8 +163,10 @@ rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for r | ||
| 163 | 163 | rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only] |
| 164 | 164 | rickub pr close <number> [-R owner/repo] |
| 165 | 165 | rickub pr reopen <number> [-R owner/repo] |
| 166 | -rickub pr edit <number> [-R owner/repo] -b "text" | -b - | |
| 167 | - # replace the description (author or repo writer) | |
| 166 | +rickub pr edit <number> [-R owner/repo] [-t "title"] [-b "text" | -b -] | |
| 167 | + # correct the title and/or the description in one call (author or | |
| 168 | + # repo writer); a field you do not name is left untouched, and | |
| 169 | + # passing neither flag is refused without sending a request | |
| 168 | 170 | rickub pr comment <number> [-R owner/repo] -b "text" |
| 169 | 171 | # whole-MR conversation comment (any reader) |
| 170 | 172 | rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N |
| @@ -191,6 +193,8 @@ rickub run watch <number> [-R owner/repo] [--interval 2s] [--timeout 30m] [-- | ||
| 191 | 193 | rickub issue list [-R owner/repo] [--state open|closed|all] [--page N] |
| 192 | 194 | rickub issue view <number> [-R owner/repo] |
| 193 | 195 | rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file] |
| 196 | +rickub issue edit <number> [-R owner/repo] [-t "title"] [-b "body" | -b - < file] | |
| 197 | + # same rules as `pr edit`: name a field to change it, omit it to keep it | |
| 194 | 198 | rickub issue close <number> [-R owner/repo] |
| 195 | 199 | rickub issue reopen <number> [-R owner/repo] |
| 196 | 200 | rickub issue comment <number> [-R owner/repo] -b "text" |
| @@ -163,8 +163,10 @@ rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for r | |||
| 163 | rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only] | 163 | rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only] |
| 164 | rickub pr close <number> [-R owner/repo] | 164 | rickub pr close <number> [-R owner/repo] |
| 165 | rickub pr reopen <number> [-R owner/repo] | 165 | rickub pr reopen <number> [-R owner/repo] |
| 166 | -rickub pr edit <number> [-R owner/repo] -b "text" | -b - | 166 | +rickub pr edit <number> [-R owner/repo] [-t "title"] [-b "text" | -b -] |
| 167 | - # replace the description (author or repo writer) | 167 | + # correct the title and/or the description in one call (author or |
| 168 | + # repo writer); a field you do not name is left untouched, and | ||
| 169 | + # passing neither flag is refused without sending a request | ||
| 168 | rickub pr comment <number> [-R owner/repo] -b "text" | 170 | rickub pr comment <number> [-R owner/repo] -b "text" |
| 169 | # whole-MR conversation comment (any reader) | 171 | # whole-MR conversation comment (any reader) |
| 170 | rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N | 172 | rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N |
| @@ -191,6 +193,8 @@ rickub run watch <number> [-R owner/repo] [--interval 2s] [--timeout 30m] [-- | |||
| 191 | rickub issue list [-R owner/repo] [--state open|closed|all] [--page N] | 193 | rickub issue list [-R owner/repo] [--state open|closed|all] [--page N] |
| 192 | rickub issue view <number> [-R owner/repo] | 194 | rickub issue view <number> [-R owner/repo] |
| 193 | rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file] | 195 | rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file] |
| 196 | +rickub issue edit <number> [-R owner/repo] [-t "title"] [-b "body" | -b - < file] | ||
| 197 | + # same rules as `pr edit`: name a field to change it, omit it to keep it | ||
| 194 | rickub issue close <number> [-R owner/repo] | 198 | rickub issue close <number> [-R owner/repo] |
| 195 | rickub issue reopen <number> [-R owner/repo] | 199 | rickub issue reopen <number> [-R owner/repo] |
| 196 | rickub issue comment <number> [-R owner/repo] -b "text" | 200 | rickub issue comment <number> [-R owner/repo] -b "text" |
added
cmd/edit.go +70 -0 | new file mode 100644 | ||
| @@ -0,0 +1,70 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "fmt" | |
| 5 | + "strings" | |
| 6 | + | |
| 7 | + "github.com/spf13/cobra" | |
| 8 | +) | |
| 9 | + | |
| 10 | +// --- --title/--body resolution for the `edit` commands ----------------------- | |
| 11 | +// | |
| 12 | +// `mr edit` and `issue edit` both drive a PATCH whose two fields are optional | |
| 13 | +// and independent: a field the user did not name must be ABSENT from the request | |
| 14 | +// so the server leaves it alone. Sending "" instead would be a change — a blank | |
| 15 | +// title is refused by the API (title_required) and a blank body would silently | |
| 16 | +// erase a description nobody asked to erase. | |
| 17 | +// | |
| 18 | +// Both commands resolve their flags here, for the same reason cmd/body.go exists: | |
| 19 | +// a second interpretation of "the user didn't pass --title" is how the two halves | |
| 20 | +// drift apart. | |
| 21 | + | |
| 22 | +// resolveEditFields turns an edit command's --title/--body flags into the two | |
| 23 | +// optional halves of a patch. A nil result means "not given, leave it alone". | |
| 24 | +// | |
| 25 | +// The rules, all enforced BEFORE any request is made: | |
| 26 | +// - neither flag given → error, and nothing is sent (the server would answer | |
| 27 | +// 400 empty_update; a local refusal is faster and says what to do instead) | |
| 28 | +// - --title given but blank → error; a title is the one field that cannot be | |
| 29 | +// empty, and a CLI that forwards it just to collect the server's 400 is worse | |
| 30 | +// - --body given → the shared resolver, so `-b -` still reads stdin and a body | |
| 31 | +// that resolves blank is still refused (issue #94) | |
| 32 | +func resolveEditFields(cmd *cobra.Command, title, body string) (*string, *string, error) { | |
| 33 | + titleSet := cmd.Flags().Changed("title") | |
| 34 | + bodySet := cmd.Flags().Changed("body") | |
| 35 | + if !titleSet && !bodySet { | |
| 36 | + return nil, nil, fmt.Errorf("nothing to update: pass --title and/or --body") | |
| 37 | + } | |
| 38 | + | |
| 39 | + var titleOut, bodyOut *string | |
| 40 | + if titleSet { | |
| 41 | + t := strings.TrimSpace(title) | |
| 42 | + if t == "" { | |
| 43 | + return nil, nil, fmt.Errorf("--title must not be blank: a title cannot be empty") | |
| 44 | + } | |
| 45 | + titleOut = &t | |
| 46 | + } | |
| 47 | + if bodySet { | |
| 48 | + b, err := requireBody(cmd, body) | |
| 49 | + if err != nil { | |
| 50 | + return nil, nil, err | |
| 51 | + } | |
| 52 | + bodyOut = &b | |
| 53 | + } | |
| 54 | + return titleOut, bodyOut, nil | |
| 55 | +} | |
| 56 | + | |
| 57 | +// editSummary renders the success line from the state the SERVER echoed back, | |
| 58 | +// not from what was sent: a success line that does not prove the bytes landed is | |
| 59 | +// exactly what made issue #94 invisible. Only the fields the user actually | |
| 60 | +// patched are reported. | |
| 61 | +func editSummary(titleSet, bodySet bool, storedTitle, storedBody string) string { | |
| 62 | + parts := make([]string, 0, 2) | |
| 63 | + if titleSet { | |
| 64 | + parts = append(parts, fmt.Sprintf("title now %q", clean(storedTitle))) | |
| 65 | + } | |
| 66 | + if bodySet { | |
| 67 | + parts = append(parts, fmt.Sprintf("description now %d bytes", len(storedBody))) | |
| 68 | + } | |
| 69 | + return strings.Join(parts, ", ") | |
| 70 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,70 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "fmt" | ||
| 5 | + "strings" | ||
| 6 | + | ||
| 7 | + "github.com/spf13/cobra" | ||
| 8 | +) | ||
| 9 | + | ||
| 10 | +// --- --title/--body resolution for the `edit` commands ----------------------- | ||
| 11 | +// | ||
| 12 | +// `mr edit` and `issue edit` both drive a PATCH whose two fields are optional | ||
| 13 | +// and independent: a field the user did not name must be ABSENT from the request | ||
| 14 | +// so the server leaves it alone. Sending "" instead would be a change — a blank | ||
| 15 | +// title is refused by the API (title_required) and a blank body would silently | ||
| 16 | +// erase a description nobody asked to erase. | ||
| 17 | +// | ||
| 18 | +// Both commands resolve their flags here, for the same reason cmd/body.go exists: | ||
| 19 | +// a second interpretation of "the user didn't pass --title" is how the two halves | ||
| 20 | +// drift apart. | ||
| 21 | + | ||
| 22 | +// resolveEditFields turns an edit command's --title/--body flags into the two | ||
| 23 | +// optional halves of a patch. A nil result means "not given, leave it alone". | ||
| 24 | +// | ||
| 25 | +// The rules, all enforced BEFORE any request is made: | ||
| 26 | +// - neither flag given → error, and nothing is sent (the server would answer | ||
| 27 | +// 400 empty_update; a local refusal is faster and says what to do instead) | ||
| 28 | +// - --title given but blank → error; a title is the one field that cannot be | ||
| 29 | +// empty, and a CLI that forwards it just to collect the server's 400 is worse | ||
| 30 | +// - --body given → the shared resolver, so `-b -` still reads stdin and a body | ||
| 31 | +// that resolves blank is still refused (issue #94) | ||
| 32 | +func resolveEditFields(cmd *cobra.Command, title, body string) (*string, *string, error) { | ||
| 33 | + titleSet := cmd.Flags().Changed("title") | ||
| 34 | + bodySet := cmd.Flags().Changed("body") | ||
| 35 | + if !titleSet && !bodySet { | ||
| 36 | + return nil, nil, fmt.Errorf("nothing to update: pass --title and/or --body") | ||
| 37 | + } | ||
| 38 | + | ||
| 39 | + var titleOut, bodyOut *string | ||
| 40 | + if titleSet { | ||
| 41 | + t := strings.TrimSpace(title) | ||
| 42 | + if t == "" { | ||
| 43 | + return nil, nil, fmt.Errorf("--title must not be blank: a title cannot be empty") | ||
| 44 | + } | ||
| 45 | + titleOut = &t | ||
| 46 | + } | ||
| 47 | + if bodySet { | ||
| 48 | + b, err := requireBody(cmd, body) | ||
| 49 | + if err != nil { | ||
| 50 | + return nil, nil, err | ||
| 51 | + } | ||
| 52 | + bodyOut = &b | ||
| 53 | + } | ||
| 54 | + return titleOut, bodyOut, nil | ||
| 55 | +} | ||
| 56 | + | ||
| 57 | +// editSummary renders the success line from the state the SERVER echoed back, | ||
| 58 | +// not from what was sent: a success line that does not prove the bytes landed is | ||
| 59 | +// exactly what made issue #94 invisible. Only the fields the user actually | ||
| 60 | +// patched are reported. | ||
| 61 | +func editSummary(titleSet, bodySet bool, storedTitle, storedBody string) string { | ||
| 62 | + parts := make([]string, 0, 2) | ||
| 63 | + if titleSet { | ||
| 64 | + parts = append(parts, fmt.Sprintf("title now %q", clean(storedTitle))) | ||
| 65 | + } | ||
| 66 | + if bodySet { | ||
| 67 | + parts = append(parts, fmt.Sprintf("description now %d bytes", len(storedBody))) | ||
| 68 | + } | ||
| 69 | + return strings.Join(parts, ", ") | ||
| 70 | +} | ||
added
cmd/edit_test.go +205 -0 | new file mode 100644 | ||
| @@ -0,0 +1,205 @@ | ||
| 1 | +package cmd | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "encoding/json" | |
| 5 | + "io" | |
| 6 | + "net/http" | |
| 7 | + "net/http/httptest" | |
| 8 | + "strings" | |
| 9 | + "testing" | |
| 10 | +) | |
| 11 | + | |
| 12 | +// editServer records every request it sees and answers a PATCH with the merged | |
| 13 | +// result, so a test can assert both what went on the wire and what the CLI | |
| 14 | +// printed from the response. | |
| 15 | +type editCall struct { | |
| 16 | + Method string | |
| 17 | + Path string | |
| 18 | + Body string | |
| 19 | +} | |
| 20 | + | |
| 21 | +func editServer(t *testing.T, reply string) (*httptest.Server, *[]editCall) { | |
| 22 | + t.Helper() | |
| 23 | + var calls []editCall | |
| 24 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 25 | + b, _ := io.ReadAll(r.Body) | |
| 26 | + calls = append(calls, editCall{Method: r.Method, Path: r.URL.Path, Body: string(b)}) | |
| 27 | + w.Header().Set("Content-Type", "application/json") | |
| 28 | + _, _ = w.Write([]byte(reply)) | |
| 29 | + })) | |
| 30 | + t.Cleanup(srv.Close) | |
| 31 | + return srv, &calls | |
| 32 | +} | |
| 33 | + | |
| 34 | +func editEnv(t *testing.T, srv *httptest.Server) { | |
| 35 | + t.Helper() | |
| 36 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | |
| 37 | + t.Setenv("RICKUB_HOST", srv.URL) | |
| 38 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | |
| 39 | +} | |
| 40 | + | |
| 41 | +// TestPREditTitleOnlyLeavesBodyAlone is issue #7's first acceptance criterion: | |
| 42 | +// renaming a merge request must not touch its description, which on the wire | |
| 43 | +// means the payload carries no "body" key at all. | |
| 44 | +func TestPREditTitleOnlyLeavesBodyAlone(t *testing.T) { | |
| 45 | + srv, calls := editServer(t, `{"number":12,"title":"new title","body":"untouched","state":"open"}`) | |
| 46 | + editEnv(t, srv) | |
| 47 | + | |
| 48 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "new title") | |
| 49 | + if err != nil { | |
| 50 | + t.Fatalf("execute: %v", err) | |
| 51 | + } | |
| 52 | + if len(*calls) != 1 { | |
| 53 | + t.Fatalf("expected exactly one request, got %d", len(*calls)) | |
| 54 | + } | |
| 55 | + c := (*calls)[0] | |
| 56 | + if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/merge-requests/12" { | |
| 57 | + t.Errorf("wrong route: %s %s", c.Method, c.Path) | |
| 58 | + } | |
| 59 | + var payload map[string]any | |
| 60 | + if err := json.Unmarshal([]byte(c.Body), &payload); err != nil { | |
| 61 | + t.Fatalf("payload is not JSON: %v (%s)", err, c.Body) | |
| 62 | + } | |
| 63 | + if payload["title"] != "new title" { | |
| 64 | + t.Errorf("title not sent: %s", c.Body) | |
| 65 | + } | |
| 66 | + if _, ok := payload["body"]; ok { | |
| 67 | + t.Errorf("body must be absent when --body was not passed: %s", c.Body) | |
| 68 | + } | |
| 69 | + if !strings.Contains(out, `title now "new title"`) { | |
| 70 | + t.Errorf("success line should echo the stored title:\n%s", out) | |
| 71 | + } | |
| 72 | + if strings.Contains(out, "description") { | |
| 73 | + t.Errorf("success line should not claim the description changed:\n%s", out) | |
| 74 | + } | |
| 75 | +} | |
| 76 | + | |
| 77 | +// TestPREditBodyFromStdin keeps `-b -` working through the shared resolver and | |
| 78 | +// checks the byte count comes from the server's echo (issue #94's lesson). | |
| 79 | +func TestPREditBodyFromStdin(t *testing.T) { | |
| 80 | + srv, calls := editServer(t, `{"number":12,"title":"kept","body":"piped in","state":"open"}`) | |
| 81 | + editEnv(t, srv) | |
| 82 | + | |
| 83 | + rootCmd.SetIn(strings.NewReader("piped in\n")) | |
| 84 | + defer rootCmd.SetIn(nil) | |
| 85 | + | |
| 86 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "-b", "-") | |
| 87 | + if err != nil { | |
| 88 | + t.Fatalf("execute: %v", err) | |
| 89 | + } | |
| 90 | + c := (*calls)[0] | |
| 91 | + var payload map[string]any | |
| 92 | + _ = json.Unmarshal([]byte(c.Body), &payload) | |
| 93 | + if payload["body"] != "piped in" { | |
| 94 | + t.Errorf("stdin body not sent verbatim: %s", c.Body) | |
| 95 | + } | |
| 96 | + if _, ok := payload["title"]; ok { | |
| 97 | + t.Errorf("title must be absent when --title was not passed: %s", c.Body) | |
| 98 | + } | |
| 99 | + if !strings.Contains(out, "description now 8 bytes") { | |
| 100 | + t.Errorf("success line should report the stored byte count:\n%s", out) | |
| 101 | + } | |
| 102 | +} | |
| 103 | + | |
| 104 | +// TestPREditBothFieldsOnePatch: naming both flags corrects both in ONE call. | |
| 105 | +func TestPREditBothFieldsOnePatch(t *testing.T) { | |
| 106 | + srv, calls := editServer(t, `{"number":12,"title":"t","body":"bb","state":"open"}`) | |
| 107 | + editEnv(t, srv) | |
| 108 | + | |
| 109 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "t", "--body", "bb") | |
| 110 | + if err != nil { | |
| 111 | + t.Fatalf("execute: %v", err) | |
| 112 | + } | |
| 113 | + if len(*calls) != 1 { | |
| 114 | + t.Fatalf("expected one PATCH, got %d requests", len(*calls)) | |
| 115 | + } | |
| 116 | + var payload map[string]any | |
| 117 | + _ = json.Unmarshal([]byte((*calls)[0].Body), &payload) | |
| 118 | + if payload["title"] != "t" || payload["body"] != "bb" { | |
| 119 | + t.Errorf("both fields expected: %s", (*calls)[0].Body) | |
| 120 | + } | |
| 121 | + if !strings.Contains(out, `title now "t"`) || !strings.Contains(out, "description now 2 bytes") { | |
| 122 | + t.Errorf("success line should report both:\n%s", out) | |
| 123 | + } | |
| 124 | +} | |
| 125 | + | |
| 126 | +// TestEditWithNoFieldFlagsSendsNothing: the refusal is LOCAL. A command that | |
| 127 | +// forwarded an empty patch just to collect the server's 400 empty_update would | |
| 128 | +// still be wrong here — the assertion that matters is zero requests. | |
| 129 | +func TestEditWithNoFieldFlagsSendsNothing(t *testing.T) { | |
| 130 | + for _, args := range [][]string{ | |
| 131 | + {"mr", "edit", "12", "-R", "o/r"}, | |
| 132 | + {"issue", "edit", "7", "-R", "o/r"}, | |
| 133 | + } { | |
| 134 | + srv, calls := editServer(t, `{}`) | |
| 135 | + editEnv(t, srv) | |
| 136 | + _, _, err := execute(t, args...) | |
| 137 | + if err == nil { | |
| 138 | + t.Fatalf("%v: expected an error when no field flag is given", args) | |
| 139 | + } | |
| 140 | + if !strings.Contains(err.Error(), "nothing to update") { | |
| 141 | + t.Errorf("%v: unhelpful error: %v", args, err) | |
| 142 | + } | |
| 143 | + if len(*calls) != 0 { | |
| 144 | + t.Errorf("%v: %d request(s) sent; nothing should be sent", args, len(*calls)) | |
| 145 | + } | |
| 146 | + } | |
| 147 | +} | |
| 148 | + | |
| 149 | +// TestEditRefusesBlankFields: a blank title is refused (it is the one field that | |
| 150 | +// cannot be empty) and so is a blank body (issue #94) — both before any request. | |
| 151 | +func TestEditRefusesBlankFields(t *testing.T) { | |
| 152 | + cases := [][]string{ | |
| 153 | + {"mr", "edit", "12", "-R", "o/r", "--title", " "}, | |
| 154 | + {"mr", "edit", "12", "-R", "o/r", "--body", ""}, | |
| 155 | + {"issue", "edit", "7", "-R", "o/r", "--title", ""}, | |
| 156 | + {"issue", "edit", "7", "-R", "o/r", "--body", " "}, | |
| 157 | + } | |
| 158 | + for _, args := range cases { | |
| 159 | + srv, calls := editServer(t, `{}`) | |
| 160 | + editEnv(t, srv) | |
| 161 | + if _, _, err := execute(t, args...); err == nil { | |
| 162 | + t.Errorf("%v: expected a refusal", args) | |
| 163 | + } | |
| 164 | + if len(*calls) != 0 { | |
| 165 | + t.Errorf("%v: %d request(s) sent; a blank field must not reach the server", args, len(*calls)) | |
| 166 | + } | |
| 167 | + } | |
| 168 | +} | |
| 169 | + | |
| 170 | +// TestIssueEditPatchesIssueRoute is the issue half: same rules, the issues route. | |
| 171 | +func TestIssueEditPatchesIssueRoute(t *testing.T) { | |
| 172 | + srv, calls := editServer(t, `{"number":7,"title":"renamed","body":"kept","state":"open"}`) | |
| 173 | + editEnv(t, srv) | |
| 174 | + | |
| 175 | + out, _, err := execute(t, "issue", "edit", "7", "-R", "o/r", "--title", "renamed") | |
| 176 | + if err != nil { | |
| 177 | + t.Fatalf("execute: %v", err) | |
| 178 | + } | |
| 179 | + c := (*calls)[0] | |
| 180 | + if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/issues/7" { | |
| 181 | + t.Errorf("wrong route: %s %s", c.Method, c.Path) | |
| 182 | + } | |
| 183 | + if !strings.Contains(out, `Updated issue #7: title now "renamed"`) { | |
| 184 | + t.Errorf("unexpected success line:\n%s", out) | |
| 185 | + } | |
| 186 | +} | |
| 187 | + | |
| 188 | +// TestEditSurfacesServerRefusalCleanly: a caller who may not touch the resource | |
| 189 | +// gets the server's uniform 404 as a plain CLI error. | |
| 190 | +func TestEditSurfacesServerRefusalCleanly(t *testing.T) { | |
| 191 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| 192 | + w.WriteHeader(http.StatusNotFound) | |
| 193 | + _, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"merge request not found"}}`)) | |
| 194 | + })) | |
| 195 | + defer srv.Close() | |
| 196 | + editEnv(t, srv) | |
| 197 | + | |
| 198 | + _, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "x") | |
| 199 | + if err == nil { | |
| 200 | + t.Fatal("expected an error") | |
| 201 | + } | |
| 202 | + if !strings.Contains(err.Error(), "merge request not found") || !strings.Contains(err.Error(), "not_found") { | |
| 203 | + t.Errorf("error should surface the server's message and code: %v", err) | |
| 204 | + } | |
| 205 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,205 @@ | |||
| 1 | +package cmd | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "encoding/json" | ||
| 5 | + "io" | ||
| 6 | + "net/http" | ||
| 7 | + "net/http/httptest" | ||
| 8 | + "strings" | ||
| 9 | + "testing" | ||
| 10 | +) | ||
| 11 | + | ||
| 12 | +// editServer records every request it sees and answers a PATCH with the merged | ||
| 13 | +// result, so a test can assert both what went on the wire and what the CLI | ||
| 14 | +// printed from the response. | ||
| 15 | +type editCall struct { | ||
| 16 | + Method string | ||
| 17 | + Path string | ||
| 18 | + Body string | ||
| 19 | +} | ||
| 20 | + | ||
| 21 | +func editServer(t *testing.T, reply string) (*httptest.Server, *[]editCall) { | ||
| 22 | + t.Helper() | ||
| 23 | + var calls []editCall | ||
| 24 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 25 | + b, _ := io.ReadAll(r.Body) | ||
| 26 | + calls = append(calls, editCall{Method: r.Method, Path: r.URL.Path, Body: string(b)}) | ||
| 27 | + w.Header().Set("Content-Type", "application/json") | ||
| 28 | + _, _ = w.Write([]byte(reply)) | ||
| 29 | + })) | ||
| 30 | + t.Cleanup(srv.Close) | ||
| 31 | + return srv, &calls | ||
| 32 | +} | ||
| 33 | + | ||
| 34 | +func editEnv(t *testing.T, srv *httptest.Server) { | ||
| 35 | + t.Helper() | ||
| 36 | + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) | ||
| 37 | + t.Setenv("RICKUB_HOST", srv.URL) | ||
| 38 | + t.Setenv("RICKUB_TOKEN", "rickub_pat_test") | ||
| 39 | +} | ||
| 40 | + | ||
| 41 | +// TestPREditTitleOnlyLeavesBodyAlone is issue #7's first acceptance criterion: | ||
| 42 | +// renaming a merge request must not touch its description, which on the wire | ||
| 43 | +// means the payload carries no "body" key at all. | ||
| 44 | +func TestPREditTitleOnlyLeavesBodyAlone(t *testing.T) { | ||
| 45 | + srv, calls := editServer(t, `{"number":12,"title":"new title","body":"untouched","state":"open"}`) | ||
| 46 | + editEnv(t, srv) | ||
| 47 | + | ||
| 48 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "new title") | ||
| 49 | + if err != nil { | ||
| 50 | + t.Fatalf("execute: %v", err) | ||
| 51 | + } | ||
| 52 | + if len(*calls) != 1 { | ||
| 53 | + t.Fatalf("expected exactly one request, got %d", len(*calls)) | ||
| 54 | + } | ||
| 55 | + c := (*calls)[0] | ||
| 56 | + if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/merge-requests/12" { | ||
| 57 | + t.Errorf("wrong route: %s %s", c.Method, c.Path) | ||
| 58 | + } | ||
| 59 | + var payload map[string]any | ||
| 60 | + if err := json.Unmarshal([]byte(c.Body), &payload); err != nil { | ||
| 61 | + t.Fatalf("payload is not JSON: %v (%s)", err, c.Body) | ||
| 62 | + } | ||
| 63 | + if payload["title"] != "new title" { | ||
| 64 | + t.Errorf("title not sent: %s", c.Body) | ||
| 65 | + } | ||
| 66 | + if _, ok := payload["body"]; ok { | ||
| 67 | + t.Errorf("body must be absent when --body was not passed: %s", c.Body) | ||
| 68 | + } | ||
| 69 | + if !strings.Contains(out, `title now "new title"`) { | ||
| 70 | + t.Errorf("success line should echo the stored title:\n%s", out) | ||
| 71 | + } | ||
| 72 | + if strings.Contains(out, "description") { | ||
| 73 | + t.Errorf("success line should not claim the description changed:\n%s", out) | ||
| 74 | + } | ||
| 75 | +} | ||
| 76 | + | ||
| 77 | +// TestPREditBodyFromStdin keeps `-b -` working through the shared resolver and | ||
| 78 | +// checks the byte count comes from the server's echo (issue #94's lesson). | ||
| 79 | +func TestPREditBodyFromStdin(t *testing.T) { | ||
| 80 | + srv, calls := editServer(t, `{"number":12,"title":"kept","body":"piped in","state":"open"}`) | ||
| 81 | + editEnv(t, srv) | ||
| 82 | + | ||
| 83 | + rootCmd.SetIn(strings.NewReader("piped in\n")) | ||
| 84 | + defer rootCmd.SetIn(nil) | ||
| 85 | + | ||
| 86 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "-b", "-") | ||
| 87 | + if err != nil { | ||
| 88 | + t.Fatalf("execute: %v", err) | ||
| 89 | + } | ||
| 90 | + c := (*calls)[0] | ||
| 91 | + var payload map[string]any | ||
| 92 | + _ = json.Unmarshal([]byte(c.Body), &payload) | ||
| 93 | + if payload["body"] != "piped in" { | ||
| 94 | + t.Errorf("stdin body not sent verbatim: %s", c.Body) | ||
| 95 | + } | ||
| 96 | + if _, ok := payload["title"]; ok { | ||
| 97 | + t.Errorf("title must be absent when --title was not passed: %s", c.Body) | ||
| 98 | + } | ||
| 99 | + if !strings.Contains(out, "description now 8 bytes") { | ||
| 100 | + t.Errorf("success line should report the stored byte count:\n%s", out) | ||
| 101 | + } | ||
| 102 | +} | ||
| 103 | + | ||
| 104 | +// TestPREditBothFieldsOnePatch: naming both flags corrects both in ONE call. | ||
| 105 | +func TestPREditBothFieldsOnePatch(t *testing.T) { | ||
| 106 | + srv, calls := editServer(t, `{"number":12,"title":"t","body":"bb","state":"open"}`) | ||
| 107 | + editEnv(t, srv) | ||
| 108 | + | ||
| 109 | + out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "t", "--body", "bb") | ||
| 110 | + if err != nil { | ||
| 111 | + t.Fatalf("execute: %v", err) | ||
| 112 | + } | ||
| 113 | + if len(*calls) != 1 { | ||
| 114 | + t.Fatalf("expected one PATCH, got %d requests", len(*calls)) | ||
| 115 | + } | ||
| 116 | + var payload map[string]any | ||
| 117 | + _ = json.Unmarshal([]byte((*calls)[0].Body), &payload) | ||
| 118 | + if payload["title"] != "t" || payload["body"] != "bb" { | ||
| 119 | + t.Errorf("both fields expected: %s", (*calls)[0].Body) | ||
| 120 | + } | ||
| 121 | + if !strings.Contains(out, `title now "t"`) || !strings.Contains(out, "description now 2 bytes") { | ||
| 122 | + t.Errorf("success line should report both:\n%s", out) | ||
| 123 | + } | ||
| 124 | +} | ||
| 125 | + | ||
| 126 | +// TestEditWithNoFieldFlagsSendsNothing: the refusal is LOCAL. A command that | ||
| 127 | +// forwarded an empty patch just to collect the server's 400 empty_update would | ||
| 128 | +// still be wrong here — the assertion that matters is zero requests. | ||
| 129 | +func TestEditWithNoFieldFlagsSendsNothing(t *testing.T) { | ||
| 130 | + for _, args := range [][]string{ | ||
| 131 | + {"mr", "edit", "12", "-R", "o/r"}, | ||
| 132 | + {"issue", "edit", "7", "-R", "o/r"}, | ||
| 133 | + } { | ||
| 134 | + srv, calls := editServer(t, `{}`) | ||
| 135 | + editEnv(t, srv) | ||
| 136 | + _, _, err := execute(t, args...) | ||
| 137 | + if err == nil { | ||
| 138 | + t.Fatalf("%v: expected an error when no field flag is given", args) | ||
| 139 | + } | ||
| 140 | + if !strings.Contains(err.Error(), "nothing to update") { | ||
| 141 | + t.Errorf("%v: unhelpful error: %v", args, err) | ||
| 142 | + } | ||
| 143 | + if len(*calls) != 0 { | ||
| 144 | + t.Errorf("%v: %d request(s) sent; nothing should be sent", args, len(*calls)) | ||
| 145 | + } | ||
| 146 | + } | ||
| 147 | +} | ||
| 148 | + | ||
| 149 | +// TestEditRefusesBlankFields: a blank title is refused (it is the one field that | ||
| 150 | +// cannot be empty) and so is a blank body (issue #94) — both before any request. | ||
| 151 | +func TestEditRefusesBlankFields(t *testing.T) { | ||
| 152 | + cases := [][]string{ | ||
| 153 | + {"mr", "edit", "12", "-R", "o/r", "--title", " "}, | ||
| 154 | + {"mr", "edit", "12", "-R", "o/r", "--body", ""}, | ||
| 155 | + {"issue", "edit", "7", "-R", "o/r", "--title", ""}, | ||
| 156 | + {"issue", "edit", "7", "-R", "o/r", "--body", " "}, | ||
| 157 | + } | ||
| 158 | + for _, args := range cases { | ||
| 159 | + srv, calls := editServer(t, `{}`) | ||
| 160 | + editEnv(t, srv) | ||
| 161 | + if _, _, err := execute(t, args...); err == nil { | ||
| 162 | + t.Errorf("%v: expected a refusal", args) | ||
| 163 | + } | ||
| 164 | + if len(*calls) != 0 { | ||
| 165 | + t.Errorf("%v: %d request(s) sent; a blank field must not reach the server", args, len(*calls)) | ||
| 166 | + } | ||
| 167 | + } | ||
| 168 | +} | ||
| 169 | + | ||
| 170 | +// TestIssueEditPatchesIssueRoute is the issue half: same rules, the issues route. | ||
| 171 | +func TestIssueEditPatchesIssueRoute(t *testing.T) { | ||
| 172 | + srv, calls := editServer(t, `{"number":7,"title":"renamed","body":"kept","state":"open"}`) | ||
| 173 | + editEnv(t, srv) | ||
| 174 | + | ||
| 175 | + out, _, err := execute(t, "issue", "edit", "7", "-R", "o/r", "--title", "renamed") | ||
| 176 | + if err != nil { | ||
| 177 | + t.Fatalf("execute: %v", err) | ||
| 178 | + } | ||
| 179 | + c := (*calls)[0] | ||
| 180 | + if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/issues/7" { | ||
| 181 | + t.Errorf("wrong route: %s %s", c.Method, c.Path) | ||
| 182 | + } | ||
| 183 | + if !strings.Contains(out, `Updated issue #7: title now "renamed"`) { | ||
| 184 | + t.Errorf("unexpected success line:\n%s", out) | ||
| 185 | + } | ||
| 186 | +} | ||
| 187 | + | ||
| 188 | +// TestEditSurfacesServerRefusalCleanly: a caller who may not touch the resource | ||
| 189 | +// gets the server's uniform 404 as a plain CLI error. | ||
| 190 | +func TestEditSurfacesServerRefusalCleanly(t *testing.T) { | ||
| 191 | + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| 192 | + w.WriteHeader(http.StatusNotFound) | ||
| 193 | + _, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"merge request not found"}}`)) | ||
| 194 | + })) | ||
| 195 | + defer srv.Close() | ||
| 196 | + editEnv(t, srv) | ||
| 197 | + | ||
| 198 | + _, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "x") | ||
| 199 | + if err == nil { | ||
| 200 | + t.Fatal("expected an error") | ||
| 201 | + } | ||
| 202 | + if !strings.Contains(err.Error(), "merge request not found") || !strings.Contains(err.Error(), "not_found") { | ||
| 203 | + t.Errorf("error should surface the server's message and code: %v", err) | ||
| 204 | + } | ||
| 205 | +} | ||
modified
cmd/issue.go +49 -1 | @@ -111,6 +111,24 @@ func init() { | ||
| 111 | 111 | RunE: runIssueLabelsList, |
| 112 | 112 | } |
| 113 | 113 | |
| 114 | + editCmd := &cobra.Command{ | |
| 115 | + Use: "edit <number>", | |
| 116 | + Short: "Correct an issue's title and/or description", | |
| 117 | + Long: "Correct an issue's title (--title) and/or description (--body, or -b - to read it\n" + | |
| 118 | + "from stdin).\n\n" + | |
| 119 | + "Both flags are optional but at least one is required: a field you do not name is\n" + | |
| 120 | + "left exactly as it was, and passing neither is refused without sending a request.\n" + | |
| 121 | + "Naming both corrects both in a single call.\n\n" + | |
| 122 | + "The issue's author, or anyone with write access to the repository, may edit it.", | |
| 123 | + Example: " rickub issue edit 7 --title \"run logs has no --job filter\"\n" + | |
| 124 | + " rickub issue edit 7 -b - <<'EOF'\n the new description\n EOF\n" + | |
| 125 | + " rickub issue edit 7 --title \"…\" --body \"…\"", | |
| 126 | + Args: cobra.ExactArgs(1), | |
| 127 | + RunE: runIssueEdit, | |
| 128 | + } | |
| 129 | + editCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "new title") | |
| 130 | + editCmd.Flags().StringVarP(&issueBody, "body", "b", "", "new description "+bodyFlagHelp) | |
| 131 | + | |
| 114 | 132 | deleteCommentCmd := &cobra.Command{ |
| 115 | 133 | Use: "delete-comment <number> <comment-id>", |
| 116 | 134 | Aliases: []string{"retract"}, |
| @@ -123,7 +141,7 @@ func init() { | ||
| 123 | 141 | RunE: runIssueDeleteComment, |
| 124 | 142 | } |
| 125 | 143 | |
| 126 | - issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd, | |
| 144 | + issueCmd.AddCommand(listCmd, viewCmd, createCmd, editCmd, closeCmd, reopenCmd, commentCmd, labelCmd, | |
| 127 | 145 | milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd) |
| 128 | 146 | rootCmd.AddCommand(issueCmd) |
| 129 | 147 | } |
| @@ -274,6 +292,36 @@ func runIssueCreate(cmd *cobra.Command, _ []string) error { | ||
| 274 | 292 | return nil |
| 275 | 293 | } |
| 276 | 294 | |
| 295 | +func runIssueEdit(cmd *cobra.Command, args []string) error { | |
| 296 | + // Same resolver as `mr edit`: a flag the user did not pass stays nil and is | |
| 297 | + // left out of the PATCH, so the other field survives untouched. | |
| 298 | + title, body, err := resolveEditFields(cmd, issueTitle, issueBody) | |
| 299 | + if err != nil { | |
| 300 | + return err | |
| 301 | + } | |
| 302 | + client, err := newClient() | |
| 303 | + if err != nil { | |
| 304 | + return err | |
| 305 | + } | |
| 306 | + owner, repo, err := resolveRepo(issueRepo) | |
| 307 | + if err != nil { | |
| 308 | + return err | |
| 309 | + } | |
| 310 | + n, err := issueNumber(args[0]) | |
| 311 | + if err != nil { | |
| 312 | + return err | |
| 313 | + } | |
| 314 | + i, err := client.UpdateIssue(cmd.Context(), owner, repo, n, api.IssuePatch{Title: title, Body: body}) | |
| 315 | + if err != nil { | |
| 316 | + return err | |
| 317 | + } | |
| 318 | + if flagJSON { | |
| 319 | + return printJSON(cmd.OutOrStdout(), i) | |
| 320 | + } | |
| 321 | + fmt.Fprintf(cmd.OutOrStdout(), "Updated issue #%d: %s\n", n, editSummary(title != nil, body != nil, i.Title, i.Body)) | |
| 322 | + return nil | |
| 323 | +} | |
| 324 | + | |
| 277 | 325 | func runIssueState(cmd *cobra.Command, args []string, state string) error { |
| 278 | 326 | client, err := newClient() |
| 279 | 327 | if err != nil { |
| @@ -111,6 +111,24 @@ func init() { | |||
| 111 | RunE: runIssueLabelsList, | 111 | RunE: runIssueLabelsList, |
| 112 | } | 112 | } |
| 113 | 113 | ||
| 114 | + editCmd := &cobra.Command{ | ||
| 115 | + Use: "edit <number>", | ||
| 116 | + Short: "Correct an issue's title and/or description", | ||
| 117 | + Long: "Correct an issue's title (--title) and/or description (--body, or -b - to read it\n" + | ||
| 118 | + "from stdin).\n\n" + | ||
| 119 | + "Both flags are optional but at least one is required: a field you do not name is\n" + | ||
| 120 | + "left exactly as it was, and passing neither is refused without sending a request.\n" + | ||
| 121 | + "Naming both corrects both in a single call.\n\n" + | ||
| 122 | + "The issue's author, or anyone with write access to the repository, may edit it.", | ||
| 123 | + Example: " rickub issue edit 7 --title \"run logs has no --job filter\"\n" + | ||
| 124 | + " rickub issue edit 7 -b - <<'EOF'\n the new description\n EOF\n" + | ||
| 125 | + " rickub issue edit 7 --title \"…\" --body \"…\"", | ||
| 126 | + Args: cobra.ExactArgs(1), | ||
| 127 | + RunE: runIssueEdit, | ||
| 128 | + } | ||
| 129 | + editCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "new title") | ||
| 130 | + editCmd.Flags().StringVarP(&issueBody, "body", "b", "", "new description "+bodyFlagHelp) | ||
| 131 | + | ||
| 114 | deleteCommentCmd := &cobra.Command{ | 132 | deleteCommentCmd := &cobra.Command{ |
| 115 | Use: "delete-comment <number> <comment-id>", | 133 | Use: "delete-comment <number> <comment-id>", |
| 116 | Aliases: []string{"retract"}, | 134 | Aliases: []string{"retract"}, |
| @@ -123,7 +141,7 @@ func init() { | |||
| 123 | RunE: runIssueDeleteComment, | 141 | RunE: runIssueDeleteComment, |
| 124 | } | 142 | } |
| 125 | 143 | ||
| 126 | - issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd, | 144 | + issueCmd.AddCommand(listCmd, viewCmd, createCmd, editCmd, closeCmd, reopenCmd, commentCmd, labelCmd, |
| 127 | milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd) | 145 | milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd) |
| 128 | rootCmd.AddCommand(issueCmd) | 146 | rootCmd.AddCommand(issueCmd) |
| 129 | } | 147 | } |
| @@ -274,6 +292,36 @@ func runIssueCreate(cmd *cobra.Command, _ []string) error { | |||
| 274 | return nil | 292 | return nil |
| 275 | } | 293 | } |
| 276 | 294 | ||
| 295 | +func runIssueEdit(cmd *cobra.Command, args []string) error { | ||
| 296 | + // Same resolver as `mr edit`: a flag the user did not pass stays nil and is | ||
| 297 | + // left out of the PATCH, so the other field survives untouched. | ||
| 298 | + title, body, err := resolveEditFields(cmd, issueTitle, issueBody) | ||
| 299 | + if err != nil { | ||
| 300 | + return err | ||
| 301 | + } | ||
| 302 | + client, err := newClient() | ||
| 303 | + if err != nil { | ||
| 304 | + return err | ||
| 305 | + } | ||
| 306 | + owner, repo, err := resolveRepo(issueRepo) | ||
| 307 | + if err != nil { | ||
| 308 | + return err | ||
| 309 | + } | ||
| 310 | + n, err := issueNumber(args[0]) | ||
| 311 | + if err != nil { | ||
| 312 | + return err | ||
| 313 | + } | ||
| 314 | + i, err := client.UpdateIssue(cmd.Context(), owner, repo, n, api.IssuePatch{Title: title, Body: body}) | ||
| 315 | + if err != nil { | ||
| 316 | + return err | ||
| 317 | + } | ||
| 318 | + if flagJSON { | ||
| 319 | + return printJSON(cmd.OutOrStdout(), i) | ||
| 320 | + } | ||
| 321 | + fmt.Fprintf(cmd.OutOrStdout(), "Updated issue #%d: %s\n", n, editSummary(title != nil, body != nil, i.Title, i.Body)) | ||
| 322 | + return nil | ||
| 323 | +} | ||
| 324 | + | ||
| 277 | func runIssueState(cmd *cobra.Command, args []string, state string) error { | 325 | func runIssueState(cmd *cobra.Command, args []string, state string) error { |
| 278 | client, err := newClient() | 326 | client, err := newClient() |
| 279 | if err != nil { | 327 | if err != nil { |
modified
cmd/pr.go +17 -10 | @@ -158,13 +158,21 @@ func init() { | ||
| 158 | 158 | |
| 159 | 159 | editCmd := &cobra.Command{ |
| 160 | 160 | Use: "edit <number>", |
| 161 | - Short: "Replace a merge request's description", | |
| 162 | - Long: "Replace a merge request's description with --body (or -b - to read it from stdin).\n\n" + | |
| 161 | + Short: "Correct a merge request's title and/or description", | |
| 162 | + Long: "Correct a merge request's title (--title) and/or description (--body, or -b - to\n" + | |
| 163 | + "read it from stdin).\n\n" + | |
| 164 | + "Both flags are optional but at least one is required: a field you do not name is\n" + | |
| 165 | + "left exactly as it was, and passing neither is refused without sending a request.\n" + | |
| 166 | + "Naming both corrects both in a single call.\n\n" + | |
| 163 | 167 | "The merge request's author, or anyone with write access to the repository, may edit it.", |
| 168 | + Example: " rickub mr edit 12 --title \"fix the retry budget, not the timeout\"\n" + | |
| 169 | + " rickub mr edit 12 -b - <<'EOF'\n the new description\n EOF\n" + | |
| 170 | + " rickub mr edit 12 --title \"…\" --body \"…\"", | |
| 164 | 171 | Args: cobra.ExactArgs(1), |
| 165 | 172 | RunE: runPREdit, |
| 166 | 173 | } |
| 167 | - editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp+"; required") | |
| 174 | + editCmd.Flags().StringVarP(&prTitle, "title", "t", "", "new title") | |
| 175 | + editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp) | |
| 168 | 176 | |
| 169 | 177 | prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd, |
| 170 | 178 | lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd) |
| @@ -274,9 +282,10 @@ func prStateLabel(mr api.MergeRequest) string { | ||
| 274 | 282 | } |
| 275 | 283 | |
| 276 | 284 | func runPREdit(cmd *cobra.Command, args []string) error { |
| 277 | - // Same resolver as every other --body: `-b -` reads stdin, and a blank result | |
| 278 | - // is refused rather than silently blanking the description. | |
| 279 | - body, err := requireBody(cmd, prBody) | |
| 285 | + // Shared with `issue edit`: a flag the user did not pass stays nil and is | |
| 286 | + // left out of the PATCH, `-b -` still reads stdin, and a blank title or body | |
| 287 | + // is refused here rather than after a round trip. | |
| 288 | + title, body, err := resolveEditFields(cmd, prTitle, prBody) | |
| 280 | 289 | if err != nil { |
| 281 | 290 | return err |
| 282 | 291 | } |
| @@ -292,16 +301,14 @@ func runPREdit(cmd *cobra.Command, args []string) error { | ||
| 292 | 301 | if err != nil { |
| 293 | 302 | return err |
| 294 | 303 | } |
| 295 | - mr, err := client.UpdateMergeRequestBody(cmd.Context(), owner, repo, n, body) | |
| 304 | + mr, err := client.UpdateMergeRequest(cmd.Context(), owner, repo, n, api.MergeRequestPatch{Title: title, Body: body}) | |
| 296 | 305 | if err != nil { |
| 297 | 306 | return err |
| 298 | 307 | } |
| 299 | 308 | if flagJSON { |
| 300 | 309 | return printJSON(cmd.OutOrStdout(), mr) |
| 301 | 310 | } |
| 302 | - // Report the stored length, not just "ok": a success line that does not prove | |
| 303 | - // the bytes landed is exactly what made issue #94 invisible. | |
| 304 | - fmt.Fprintf(cmd.OutOrStdout(), "Updated the description of #%d (%d bytes)\n", n, len(mr.Body)) | |
| 311 | + fmt.Fprintf(cmd.OutOrStdout(), "Updated #%d: %s\n", n, editSummary(title != nil, body != nil, mr.Title, mr.Body)) | |
| 305 | 312 | return nil |
| 306 | 313 | } |
| 307 | 314 | |
| @@ -158,13 +158,21 @@ func init() { | |||
| 158 | 158 | ||
| 159 | editCmd := &cobra.Command{ | 159 | editCmd := &cobra.Command{ |
| 160 | Use: "edit <number>", | 160 | Use: "edit <number>", |
| 161 | - Short: "Replace a merge request's description", | 161 | + Short: "Correct a merge request's title and/or description", |
| 162 | - Long: "Replace a merge request's description with --body (or -b - to read it from stdin).\n\n" + | 162 | + Long: "Correct a merge request's title (--title) and/or description (--body, or -b - to\n" + |
| 163 | + "read it from stdin).\n\n" + | ||
| 164 | + "Both flags are optional but at least one is required: a field you do not name is\n" + | ||
| 165 | + "left exactly as it was, and passing neither is refused without sending a request.\n" + | ||
| 166 | + "Naming both corrects both in a single call.\n\n" + | ||
| 163 | "The merge request's author, or anyone with write access to the repository, may edit it.", | 167 | "The merge request's author, or anyone with write access to the repository, may edit it.", |
| 168 | + Example: " rickub mr edit 12 --title \"fix the retry budget, not the timeout\"\n" + | ||
| 169 | + " rickub mr edit 12 -b - <<'EOF'\n the new description\n EOF\n" + | ||
| 170 | + " rickub mr edit 12 --title \"…\" --body \"…\"", | ||
| 164 | Args: cobra.ExactArgs(1), | 171 | Args: cobra.ExactArgs(1), |
| 165 | RunE: runPREdit, | 172 | RunE: runPREdit, |
| 166 | } | 173 | } |
| 167 | - editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp+"; required") | 174 | + editCmd.Flags().StringVarP(&prTitle, "title", "t", "", "new title") |
| 175 | + editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp) | ||
| 168 | 176 | ||
| 169 | prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd, | 177 | prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd, |
| 170 | lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd) | 178 | lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd) |
| @@ -274,9 +282,10 @@ func prStateLabel(mr api.MergeRequest) string { | |||
| 274 | } | 282 | } |
| 275 | 283 | ||
| 276 | func runPREdit(cmd *cobra.Command, args []string) error { | 284 | func runPREdit(cmd *cobra.Command, args []string) error { |
| 277 | - // Same resolver as every other --body: `-b -` reads stdin, and a blank result | 285 | + // Shared with `issue edit`: a flag the user did not pass stays nil and is |
| 278 | - // is refused rather than silently blanking the description. | 286 | + // left out of the PATCH, `-b -` still reads stdin, and a blank title or body |
| 279 | - body, err := requireBody(cmd, prBody) | 287 | + // is refused here rather than after a round trip. |
| 288 | + title, body, err := resolveEditFields(cmd, prTitle, prBody) | ||
| 280 | if err != nil { | 289 | if err != nil { |
| 281 | return err | 290 | return err |
| 282 | } | 291 | } |
| @@ -292,16 +301,14 @@ func runPREdit(cmd *cobra.Command, args []string) error { | |||
| 292 | if err != nil { | 301 | if err != nil { |
| 293 | return err | 302 | return err |
| 294 | } | 303 | } |
| 295 | - mr, err := client.UpdateMergeRequestBody(cmd.Context(), owner, repo, n, body) | 304 | + mr, err := client.UpdateMergeRequest(cmd.Context(), owner, repo, n, api.MergeRequestPatch{Title: title, Body: body}) |
| 296 | if err != nil { | 305 | if err != nil { |
| 297 | return err | 306 | return err |
| 298 | } | 307 | } |
| 299 | if flagJSON { | 308 | if flagJSON { |
| 300 | return printJSON(cmd.OutOrStdout(), mr) | 309 | return printJSON(cmd.OutOrStdout(), mr) |
| 301 | } | 310 | } |
| 302 | - // Report the stored length, not just "ok": a success line that does not prove | 311 | + fmt.Fprintf(cmd.OutOrStdout(), "Updated #%d: %s\n", n, editSummary(title != nil, body != nil, mr.Title, mr.Body)) |
| 303 | - // the bytes landed is exactly what made issue #94 invisible. | ||
| 304 | - fmt.Fprintf(cmd.OutOrStdout(), "Updated the description of #%d (%d bytes)\n", n, len(mr.Body)) | ||
| 305 | return nil | 312 | return nil |
| 306 | } | 313 | } |
| 307 | 314 | ||
modified
cmd/pr_line_comments_test.go +2 -2 | @@ -325,8 +325,8 @@ func TestPREditSendsBodyAndReportsLength(t *testing.T) { | ||
| 325 | 325 | if err != nil { |
| 326 | 326 | t.Fatalf("execute: %v", err) |
| 327 | 327 | } |
| 328 | - if seen.Path != "/api/v1/repos/rick/portal/merge-requests/12/body" { | |
| 329 | - t.Fatalf("path = %q", seen.Path) | |
| 328 | + if seen.Method != http.MethodPatch || seen.Path != "/api/v1/repos/rick/portal/merge-requests/12" { | |
| 329 | + t.Fatalf("route = %s %q", seen.Method, seen.Path) | |
| 330 | 330 | } |
| 331 | 331 | if seen.Body["body"] != desc { |
| 332 | 332 | t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"]) |
| @@ -325,8 +325,8 @@ func TestPREditSendsBodyAndReportsLength(t *testing.T) { | |||
| 325 | if err != nil { | 325 | if err != nil { |
| 326 | t.Fatalf("execute: %v", err) | 326 | t.Fatalf("execute: %v", err) |
| 327 | } | 327 | } |
| 328 | - if seen.Path != "/api/v1/repos/rick/portal/merge-requests/12/body" { | 328 | + if seen.Method != http.MethodPatch || seen.Path != "/api/v1/repos/rick/portal/merge-requests/12" { |
| 329 | - t.Fatalf("path = %q", seen.Path) | 329 | + t.Fatalf("route = %s %q", seen.Method, seen.Path) |
| 330 | } | 330 | } |
| 331 | if seen.Body["body"] != desc { | 331 | if seen.Body["body"] != desc { |
| 332 | t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"]) | 332 | t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"]) |
modified
cmd/repo_test.go +18 -0 | @@ -7,6 +7,9 @@ import ( | ||
| 7 | 7 | "net/http/httptest" |
| 8 | 8 | "strings" |
| 9 | 9 | "testing" |
| 10 | + | |
| 11 | + "github.com/spf13/cobra" | |
| 12 | + "github.com/spf13/pflag" | |
| 10 | 13 | ) |
| 11 | 14 | |
| 12 | 15 | // execute runs the root command with args against a fresh output buffer, after |
| @@ -39,6 +42,7 @@ func execute(t *testing.T, args ...string) (string, string, error) { | ||
| 39 | 42 | authLoginScope = "all" |
| 40 | 43 | authLoginNoBrowser = false |
| 41 | 44 | prRepo = "" |
| 45 | + prTitle = "" | |
| 42 | 46 | prBody = "" |
| 43 | 47 | prFile = "" |
| 44 | 48 | prLine = 0 |
| @@ -53,6 +57,12 @@ func execute(t *testing.T, args ...string) (string, string, error) { | ||
| 53 | 57 | labelColor = "" |
| 54 | 58 | labelNewName = "" |
| 55 | 59 | |
| 60 | + // Cobra records "the user passed this flag" on the flag object itself, and the | |
| 61 | + // command tree is built once per process by init(). `edit` asks exactly that | |
| 62 | + // question to decide which fields go into its PATCH, so a stale Changed from an | |
| 63 | + // earlier test would make it send a field nobody passed. Clear it everywhere. | |
| 64 | + resetFlagsChanged(rootCmd) | |
| 65 | + | |
| 56 | 66 | var out, errBuf bytes.Buffer |
| 57 | 67 | rootCmd.SetOut(&out) |
| 58 | 68 | rootCmd.SetErr(&errBuf) |
| @@ -61,6 +71,14 @@ func execute(t *testing.T, args ...string) (string, string, error) { | ||
| 61 | 71 | return out.String(), errBuf.String(), err |
| 62 | 72 | } |
| 63 | 73 | |
| 74 | +// resetFlagsChanged clears the Changed bit on every flag in a command tree. | |
| 75 | +func resetFlagsChanged(c *cobra.Command) { | |
| 76 | + c.Flags().VisitAll(func(f *pflag.Flag) { f.Changed = false }) | |
| 77 | + for _, sub := range c.Commands() { | |
| 78 | + resetFlagsChanged(sub) | |
| 79 | + } | |
| 80 | +} | |
| 81 | + | |
| 64 | 82 | func repoListServer(t *testing.T) *httptest.Server { |
| 65 | 83 | return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| 66 | 84 | if r.URL.Path != "/api/v1/users/ricktester/repos" { |
| @@ -7,6 +7,9 @@ import ( | |||
| 7 | "net/http/httptest" | 7 | "net/http/httptest" |
| 8 | "strings" | 8 | "strings" |
| 9 | "testing" | 9 | "testing" |
| 10 | + | ||
| 11 | + "github.com/spf13/cobra" | ||
| 12 | + "github.com/spf13/pflag" | ||
| 10 | ) | 13 | ) |
| 11 | 14 | ||
| 12 | // execute runs the root command with args against a fresh output buffer, after | 15 | // execute runs the root command with args against a fresh output buffer, after |
| @@ -39,6 +42,7 @@ func execute(t *testing.T, args ...string) (string, string, error) { | |||
| 39 | authLoginScope = "all" | 42 | authLoginScope = "all" |
| 40 | authLoginNoBrowser = false | 43 | authLoginNoBrowser = false |
| 41 | prRepo = "" | 44 | prRepo = "" |
| 45 | + prTitle = "" | ||
| 42 | prBody = "" | 46 | prBody = "" |
| 43 | prFile = "" | 47 | prFile = "" |
| 44 | prLine = 0 | 48 | prLine = 0 |
| @@ -53,6 +57,12 @@ func execute(t *testing.T, args ...string) (string, string, error) { | |||
| 53 | labelColor = "" | 57 | labelColor = "" |
| 54 | labelNewName = "" | 58 | labelNewName = "" |
| 55 | 59 | ||
| 60 | + // Cobra records "the user passed this flag" on the flag object itself, and the | ||
| 61 | + // command tree is built once per process by init(). `edit` asks exactly that | ||
| 62 | + // question to decide which fields go into its PATCH, so a stale Changed from an | ||
| 63 | + // earlier test would make it send a field nobody passed. Clear it everywhere. | ||
| 64 | + resetFlagsChanged(rootCmd) | ||
| 65 | + | ||
| 56 | var out, errBuf bytes.Buffer | 66 | var out, errBuf bytes.Buffer |
| 57 | rootCmd.SetOut(&out) | 67 | rootCmd.SetOut(&out) |
| 58 | rootCmd.SetErr(&errBuf) | 68 | rootCmd.SetErr(&errBuf) |
| @@ -61,6 +71,14 @@ func execute(t *testing.T, args ...string) (string, string, error) { | |||
| 61 | return out.String(), errBuf.String(), err | 71 | return out.String(), errBuf.String(), err |
| 62 | } | 72 | } |
| 63 | 73 | ||
| 74 | +// resetFlagsChanged clears the Changed bit on every flag in a command tree. | ||
| 75 | +func resetFlagsChanged(c *cobra.Command) { | ||
| 76 | + c.Flags().VisitAll(func(f *pflag.Flag) { f.Changed = false }) | ||
| 77 | + for _, sub := range c.Commands() { | ||
| 78 | + resetFlagsChanged(sub) | ||
| 79 | + } | ||
| 80 | +} | ||
| 81 | + | ||
| 64 | func repoListServer(t *testing.T) *httptest.Server { | 82 | func repoListServer(t *testing.T) *httptest.Server { |
| 65 | return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | 83 | return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| 66 | if r.URL.Path != "/api/v1/users/ricktester/repos" { | 84 | if r.URL.Path != "/api/v1/users/ricktester/repos" { |
modified
internal/api/endpoints.go +21 -6 | @@ -207,14 +207,16 @@ func lineCommentsPath(owner, repo string, number int) string { | ||
| 207 | 207 | return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments" |
| 208 | 208 | } |
| 209 | 209 | |
| 210 | -// UpdateMergeRequestBody replaces a merge request's description. The MR's author | |
| 211 | -// or any write-access user may do it. | |
| 212 | -func (c *Client) UpdateMergeRequestBody(ctx context.Context, owner, repo string, number int, body string) (*MergeRequest, error) { | |
| 210 | +// UpdateMergeRequest patches a merge request's title and/or description in one | |
| 211 | +// call. Fields left nil in the patch are not sent, so the server keeps them; a | |
| 212 | +// patch with neither field set is refused by the server as 400 empty_update. | |
| 213 | +// The MR's author or any write-access user may do it. | |
| 214 | +func (c *Client) UpdateMergeRequest(ctx context.Context, owner, repo string, number int, in MergeRequestPatch) (*MergeRequest, error) { | |
| 213 | 215 | var mr MergeRequest |
| 214 | 216 | err := c.do(ctx, Request{ |
| 215 | - Method: http.MethodPost, | |
| 216 | - Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/body", | |
| 217 | - Body: map[string]string{"body": body}, | |
| 217 | + Method: http.MethodPatch, | |
| 218 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number), | |
| 219 | + Body: in, | |
| 218 | 220 | }, &mr) |
| 219 | 221 | return &mr, err |
| 220 | 222 | } |
| @@ -367,6 +369,19 @@ func (c *Client) CreateIssue(ctx context.Context, owner, repo, title, body strin | ||
| 367 | 369 | return &i, err |
| 368 | 370 | } |
| 369 | 371 | |
| 372 | +// UpdateIssue patches an issue's title and/or description in one call, with the | |
| 373 | +// same nil-means-untouched rule as UpdateMergeRequest. The issue's author or any | |
| 374 | +// write-access user may do it. | |
| 375 | +func (c *Client) UpdateIssue(ctx context.Context, owner, repo string, number int, in IssuePatch) (*IssueDetail, error) { | |
| 376 | + var i IssueDetail | |
| 377 | + err := c.do(ctx, Request{ | |
| 378 | + Method: http.MethodPatch, | |
| 379 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/issues/" + itoa(number), | |
| 380 | + Body: in, | |
| 381 | + }, &i) | |
| 382 | + return &i, err | |
| 383 | +} | |
| 384 | + | |
| 370 | 385 | // SetIssueState closes ("closed") or reopens ("open") an issue. |
| 371 | 386 | func (c *Client) SetIssueState(ctx context.Context, owner, repo string, number int, state string) (*IssueDetail, error) { |
| 372 | 387 | var i IssueDetail |
| @@ -207,14 +207,16 @@ func lineCommentsPath(owner, repo string, number int) string { | |||
| 207 | return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments" | 207 | return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments" |
| 208 | } | 208 | } |
| 209 | 209 | ||
| 210 | -// UpdateMergeRequestBody replaces a merge request's description. The MR's author | 210 | +// UpdateMergeRequest patches a merge request's title and/or description in one |
| 211 | -// or any write-access user may do it. | 211 | +// call. Fields left nil in the patch are not sent, so the server keeps them; a |
| 212 | -func (c *Client) UpdateMergeRequestBody(ctx context.Context, owner, repo string, number int, body string) (*MergeRequest, error) { | 212 | +// patch with neither field set is refused by the server as 400 empty_update. |
| 213 | +// The MR's author or any write-access user may do it. | ||
| 214 | +func (c *Client) UpdateMergeRequest(ctx context.Context, owner, repo string, number int, in MergeRequestPatch) (*MergeRequest, error) { | ||
| 213 | var mr MergeRequest | 215 | var mr MergeRequest |
| 214 | err := c.do(ctx, Request{ | 216 | err := c.do(ctx, Request{ |
| 215 | - Method: http.MethodPost, | 217 | + Method: http.MethodPatch, |
| 216 | - Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/body", | 218 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number), |
| 217 | - Body: map[string]string{"body": body}, | 219 | + Body: in, |
| 218 | }, &mr) | 220 | }, &mr) |
| 219 | return &mr, err | 221 | return &mr, err |
| 220 | } | 222 | } |
| @@ -367,6 +369,19 @@ func (c *Client) CreateIssue(ctx context.Context, owner, repo, title, body strin | |||
| 367 | return &i, err | 369 | return &i, err |
| 368 | } | 370 | } |
| 369 | 371 | ||
| 372 | +// UpdateIssue patches an issue's title and/or description in one call, with the | ||
| 373 | +// same nil-means-untouched rule as UpdateMergeRequest. The issue's author or any | ||
| 374 | +// write-access user may do it. | ||
| 375 | +func (c *Client) UpdateIssue(ctx context.Context, owner, repo string, number int, in IssuePatch) (*IssueDetail, error) { | ||
| 376 | + var i IssueDetail | ||
| 377 | + err := c.do(ctx, Request{ | ||
| 378 | + Method: http.MethodPatch, | ||
| 379 | + Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/issues/" + itoa(number), | ||
| 380 | + Body: in, | ||
| 381 | + }, &i) | ||
| 382 | + return &i, err | ||
| 383 | +} | ||
| 384 | + | ||
| 370 | // SetIssueState closes ("closed") or reopens ("open") an issue. | 385 | // SetIssueState closes ("closed") or reopens ("open") an issue. |
| 371 | func (c *Client) SetIssueState(ctx context.Context, owner, repo string, number int, state string) (*IssueDetail, error) { | 386 | func (c *Client) SetIssueState(ctx context.Context, owner, repo string, number int, state string) (*IssueDetail, error) { |
| 372 | var i IssueDetail | 387 | var i IssueDetail |
added
internal/api/endpoints_patch_test.go +89 -0 | new file mode 100644 | ||
| @@ -0,0 +1,89 @@ | ||
| 1 | +package api | |
| 2 | + | |
| 3 | +import ( | |
| 4 | + "context" | |
| 5 | + "errors" | |
| 6 | + "strings" | |
| 7 | + "testing" | |
| 8 | +) | |
| 9 | + | |
| 10 | +// TestUpdateMergeRequestAndIssuePatchWire pins the wire contract of the two PATCH | |
| 11 | +// helpers: the method, the path, and — the load-bearing part — that a field the | |
| 12 | +// caller left nil is ABSENT from the JSON rather than sent as "". Sending an empty | |
| 13 | +// title would trip the API's title_required; sending an empty body would blank a | |
| 14 | +// description nobody asked to blank. | |
| 15 | +func TestUpdateMergeRequestAndIssuePatchWire(t *testing.T) { | |
| 16 | + srv, calls, responses := newIssuesServer(t) | |
| 17 | + responses["PATCH /api/v1/repos/o/r/merge-requests/12"] = `{"number":12,"title":"corrected","body":"kept","state":"open"}` | |
| 18 | + responses["PATCH /api/v1/repos/o/r/issues/7"] = `{"number":7,"title":"kept","body":"corrected","state":"open"}` | |
| 19 | + c := New(srv.URL, "rickub_pat_x") | |
| 20 | + ctx := context.Background() | |
| 21 | + | |
| 22 | + title := "corrected" | |
| 23 | + body := "new body" | |
| 24 | + | |
| 25 | + // Title only: the JSON must carry "title" and NOT "body". | |
| 26 | + mr, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title}) | |
| 27 | + if err != nil || mr.Title != "corrected" { | |
| 28 | + t.Fatalf("UpdateMergeRequest: %+v err=%v", mr, err) | |
| 29 | + } | |
| 30 | + last := (*calls)[len(*calls)-1] | |
| 31 | + if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/merge-requests/12" { | |
| 32 | + t.Errorf("wrong route: %s %s", last.Method, last.Path) | |
| 33 | + } | |
| 34 | + if !strings.Contains(last.Body, `"title":"corrected"`) { | |
| 35 | + t.Errorf("title missing from payload: %s", last.Body) | |
| 36 | + } | |
| 37 | + if strings.Contains(last.Body, `"body"`) { | |
| 38 | + t.Errorf("body must not appear when it was not set: %s", last.Body) | |
| 39 | + } | |
| 40 | + | |
| 41 | + // Body only: the mirror image. | |
| 42 | + iss, err := c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &body}) | |
| 43 | + if err != nil || iss.Body != "corrected" { | |
| 44 | + t.Fatalf("UpdateIssue: %+v err=%v", iss, err) | |
| 45 | + } | |
| 46 | + last = (*calls)[len(*calls)-1] | |
| 47 | + if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/issues/7" { | |
| 48 | + t.Errorf("wrong route: %s %s", last.Method, last.Path) | |
| 49 | + } | |
| 50 | + if !strings.Contains(last.Body, `"body":"new body"`) { | |
| 51 | + t.Errorf("body missing from payload: %s", last.Body) | |
| 52 | + } | |
| 53 | + if strings.Contains(last.Body, `"title"`) { | |
| 54 | + t.Errorf("title must not appear when it was not set: %s", last.Body) | |
| 55 | + } | |
| 56 | + | |
| 57 | + // Both: one PATCH carries both fields. | |
| 58 | + if _, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title, Body: &body}); err != nil { | |
| 59 | + t.Fatalf("UpdateMergeRequest both: %v", err) | |
| 60 | + } | |
| 61 | + last = (*calls)[len(*calls)-1] | |
| 62 | + if !strings.Contains(last.Body, `"title":"corrected"`) || !strings.Contains(last.Body, `"body":"new body"`) { | |
| 63 | + t.Errorf("both fields expected in one payload: %s", last.Body) | |
| 64 | + } | |
| 65 | + | |
| 66 | + // An empty string is a deliberate value, not an omission: a caller that | |
| 67 | + // really passes "" must see it on the wire (the server then answers | |
| 68 | + // title_required). This guards the pointer semantics from being "fixed" | |
| 69 | + // into omitempty-on-value. | |
| 70 | + empty := "" | |
| 71 | + _, _ = c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &empty}) | |
| 72 | + last = (*calls)[len(*calls)-1] | |
| 73 | + if !strings.Contains(last.Body, `"body":""`) { | |
| 74 | + t.Errorf("an explicit empty body must be sent, not dropped: %s", last.Body) | |
| 75 | + } | |
| 76 | +} | |
| 77 | + | |
| 78 | +// TestUpdateSurfacesAPIError checks the server's uniform 404 (not the author, no | |
| 79 | +// write access) arrives as a clean *APIError rather than something opaque. | |
| 80 | +func TestUpdateSurfacesAPIError(t *testing.T) { | |
| 81 | + srv, _, _ := newIssuesServer(t) // every unscripted route answers 404 not_found | |
| 82 | + c := New(srv.URL, "rickub_pat_x") | |
| 83 | + title := "nope" | |
| 84 | + _, err := c.UpdateMergeRequest(context.Background(), "o", "r", 99, MergeRequestPatch{Title: &title}) | |
| 85 | + var apiErr *APIError | |
| 86 | + if err == nil || !errors.As(err, &apiErr) || apiErr.Code != "not_found" { | |
| 87 | + t.Fatalf("expected an *APIError not_found, got %v", err) | |
| 88 | + } | |
| 89 | +} | |
| new file mode 100644 | |||
| @@ -0,0 +1,89 @@ | |||
| 1 | +package api | ||
| 2 | + | ||
| 3 | +import ( | ||
| 4 | + "context" | ||
| 5 | + "errors" | ||
| 6 | + "strings" | ||
| 7 | + "testing" | ||
| 8 | +) | ||
| 9 | + | ||
| 10 | +// TestUpdateMergeRequestAndIssuePatchWire pins the wire contract of the two PATCH | ||
| 11 | +// helpers: the method, the path, and — the load-bearing part — that a field the | ||
| 12 | +// caller left nil is ABSENT from the JSON rather than sent as "". Sending an empty | ||
| 13 | +// title would trip the API's title_required; sending an empty body would blank a | ||
| 14 | +// description nobody asked to blank. | ||
| 15 | +func TestUpdateMergeRequestAndIssuePatchWire(t *testing.T) { | ||
| 16 | + srv, calls, responses := newIssuesServer(t) | ||
| 17 | + responses["PATCH /api/v1/repos/o/r/merge-requests/12"] = `{"number":12,"title":"corrected","body":"kept","state":"open"}` | ||
| 18 | + responses["PATCH /api/v1/repos/o/r/issues/7"] = `{"number":7,"title":"kept","body":"corrected","state":"open"}` | ||
| 19 | + c := New(srv.URL, "rickub_pat_x") | ||
| 20 | + ctx := context.Background() | ||
| 21 | + | ||
| 22 | + title := "corrected" | ||
| 23 | + body := "new body" | ||
| 24 | + | ||
| 25 | + // Title only: the JSON must carry "title" and NOT "body". | ||
| 26 | + mr, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title}) | ||
| 27 | + if err != nil || mr.Title != "corrected" { | ||
| 28 | + t.Fatalf("UpdateMergeRequest: %+v err=%v", mr, err) | ||
| 29 | + } | ||
| 30 | + last := (*calls)[len(*calls)-1] | ||
| 31 | + if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/merge-requests/12" { | ||
| 32 | + t.Errorf("wrong route: %s %s", last.Method, last.Path) | ||
| 33 | + } | ||
| 34 | + if !strings.Contains(last.Body, `"title":"corrected"`) { | ||
| 35 | + t.Errorf("title missing from payload: %s", last.Body) | ||
| 36 | + } | ||
| 37 | + if strings.Contains(last.Body, `"body"`) { | ||
| 38 | + t.Errorf("body must not appear when it was not set: %s", last.Body) | ||
| 39 | + } | ||
| 40 | + | ||
| 41 | + // Body only: the mirror image. | ||
| 42 | + iss, err := c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &body}) | ||
| 43 | + if err != nil || iss.Body != "corrected" { | ||
| 44 | + t.Fatalf("UpdateIssue: %+v err=%v", iss, err) | ||
| 45 | + } | ||
| 46 | + last = (*calls)[len(*calls)-1] | ||
| 47 | + if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/issues/7" { | ||
| 48 | + t.Errorf("wrong route: %s %s", last.Method, last.Path) | ||
| 49 | + } | ||
| 50 | + if !strings.Contains(last.Body, `"body":"new body"`) { | ||
| 51 | + t.Errorf("body missing from payload: %s", last.Body) | ||
| 52 | + } | ||
| 53 | + if strings.Contains(last.Body, `"title"`) { | ||
| 54 | + t.Errorf("title must not appear when it was not set: %s", last.Body) | ||
| 55 | + } | ||
| 56 | + | ||
| 57 | + // Both: one PATCH carries both fields. | ||
| 58 | + if _, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title, Body: &body}); err != nil { | ||
| 59 | + t.Fatalf("UpdateMergeRequest both: %v", err) | ||
| 60 | + } | ||
| 61 | + last = (*calls)[len(*calls)-1] | ||
| 62 | + if !strings.Contains(last.Body, `"title":"corrected"`) || !strings.Contains(last.Body, `"body":"new body"`) { | ||
| 63 | + t.Errorf("both fields expected in one payload: %s", last.Body) | ||
| 64 | + } | ||
| 65 | + | ||
| 66 | + // An empty string is a deliberate value, not an omission: a caller that | ||
| 67 | + // really passes "" must see it on the wire (the server then answers | ||
| 68 | + // title_required). This guards the pointer semantics from being "fixed" | ||
| 69 | + // into omitempty-on-value. | ||
| 70 | + empty := "" | ||
| 71 | + _, _ = c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &empty}) | ||
| 72 | + last = (*calls)[len(*calls)-1] | ||
| 73 | + if !strings.Contains(last.Body, `"body":""`) { | ||
| 74 | + t.Errorf("an explicit empty body must be sent, not dropped: %s", last.Body) | ||
| 75 | + } | ||
| 76 | +} | ||
| 77 | + | ||
| 78 | +// TestUpdateSurfacesAPIError checks the server's uniform 404 (not the author, no | ||
| 79 | +// write access) arrives as a clean *APIError rather than something opaque. | ||
| 80 | +func TestUpdateSurfacesAPIError(t *testing.T) { | ||
| 81 | + srv, _, _ := newIssuesServer(t) // every unscripted route answers 404 not_found | ||
| 82 | + c := New(srv.URL, "rickub_pat_x") | ||
| 83 | + title := "nope" | ||
| 84 | + _, err := c.UpdateMergeRequest(context.Background(), "o", "r", 99, MergeRequestPatch{Title: &title}) | ||
| 85 | + var apiErr *APIError | ||
| 86 | + if err == nil || !errors.As(err, &apiErr) || apiErr.Code != "not_found" { | ||
| 87 | + t.Fatalf("expected an *APIError not_found, got %v", err) | ||
| 88 | + } | ||
| 89 | +} | ||
modified
internal/api/models.go +20 -0 | @@ -91,6 +91,26 @@ type MergeRequestCreate struct { | ||
| 91 | 91 | Draft *bool `json:"draft,omitempty"` |
| 92 | 92 | } |
| 93 | 93 | |
| 94 | +// MergeRequestPatch is the PATCH-a-merge-request request body (title and/or | |
| 95 | +// description). Both fields are POINTERS, and that is the load-bearing detail: | |
| 96 | +// a field the caller did not set is absent from the JSON entirely, so the server | |
| 97 | +// leaves it as it was. Sending "" instead would be a *change* — an empty title | |
| 98 | +// trips the API's title_required, and an empty body would blank a description | |
| 99 | +// the user never meant to touch. | |
| 100 | +// | |
| 101 | +// The server refuses a patch carrying neither field (400 empty_update); the CLI | |
| 102 | +// refuses it earlier, before a request is made. | |
| 103 | +type MergeRequestPatch struct { | |
| 104 | + Title *string `json:"title,omitempty"` | |
| 105 | + Body *string `json:"body,omitempty"` | |
| 106 | +} | |
| 107 | + | |
| 108 | +// IssuePatch is MergeRequestPatch's twin for an issue, with the same rules. | |
| 109 | +type IssuePatch struct { | |
| 110 | + Title *string `json:"title,omitempty"` | |
| 111 | + Body *string `json:"body,omitempty"` | |
| 112 | +} | |
| 113 | + | |
| 94 | 114 | // Comment is a conversation comment on an MR or an issue. |
| 95 | 115 | // |
| 96 | 116 | // ID is what makes it addressable — `mr delete-comment` / `issue delete-comment` |
| @@ -91,6 +91,26 @@ type MergeRequestCreate struct { | |||
| 91 | Draft *bool `json:"draft,omitempty"` | 91 | Draft *bool `json:"draft,omitempty"` |
| 92 | } | 92 | } |
| 93 | 93 | ||
| 94 | +// MergeRequestPatch is the PATCH-a-merge-request request body (title and/or | ||
| 95 | +// description). Both fields are POINTERS, and that is the load-bearing detail: | ||
| 96 | +// a field the caller did not set is absent from the JSON entirely, so the server | ||
| 97 | +// leaves it as it was. Sending "" instead would be a *change* — an empty title | ||
| 98 | +// trips the API's title_required, and an empty body would blank a description | ||
| 99 | +// the user never meant to touch. | ||
| 100 | +// | ||
| 101 | +// The server refuses a patch carrying neither field (400 empty_update); the CLI | ||
| 102 | +// refuses it earlier, before a request is made. | ||
| 103 | +type MergeRequestPatch struct { | ||
| 104 | + Title *string `json:"title,omitempty"` | ||
| 105 | + Body *string `json:"body,omitempty"` | ||
| 106 | +} | ||
| 107 | + | ||
| 108 | +// IssuePatch is MergeRequestPatch's twin for an issue, with the same rules. | ||
| 109 | +type IssuePatch struct { | ||
| 110 | + Title *string `json:"title,omitempty"` | ||
| 111 | + Body *string `json:"body,omitempty"` | ||
| 112 | +} | ||
| 113 | + | ||
| 94 | // Comment is a conversation comment on an MR or an issue. | 114 | // Comment is a conversation comment on an MR or an issue. |
| 95 | // | 115 | // |
| 96 | // ID is what makes it addressable — `mr delete-comment` / `issue delete-comment` | 116 | // ID is what makes it addressable — `mr delete-comment` / `issue delete-comment` |