fix: address verified review findings (pulls-1)

Signed-off-by: bircni <bircni@icloud.com>

docs/migration-1.0.md

@@ -282,6 +282,7 @@ changed meaning:
 | `label edit\|delete` | `--id ID` | label name or ID |
 | `team view\|edit\|delete`, `team member\|repo ...` | team ID | `ORG/TEAM` or team ID |
 | `repo delete` | `--name NAME --owner OWNER` | optional `OWNER/REPO` argument (or `--repo`) |
+| `pulls close\|reopen\|edit` | several indexes | `pr close\|reopen\|edit` take one pull request (number, URL or branch); loop to change several |
 | `issue comment` | optional body argument | `--body`, `--body-file` or `--editor` |
 | `secret set`, `variable set` | `<name> [<value>]`, `--file`, `--stdin` | value from `--body`, `--body-file FILE\|-` or piped standard input |
 

internal/app/pull_create.go

@@ -7,6 +7,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"net/url"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -116,7 +117,7 @@ is not on the remote yet is pushed first, with its upstream set.`, Args: argsExa
 			}
 		}
 		if web {
-			return a.openBrowser(cmd, repositoryURL(t)+"/compare/"+options.Base+"..."+options.Head)
+			return a.openBrowser(cmd, repositoryURL(t)+"/compare/"+compareRef(options.Base)+"..."+compareRef(options.Head))
 		}
 		var commitBody string
 		if options.Title == "" {
@@ -373,3 +374,9 @@ func (a *application) reportCreatedPull(cmd *cobra.Command, t target, item servi
 	a.done(cmd, "Created pull request %s", refLabel(t.Repo, item.Number, item.Title))
 	return a.created(cmd, item, item.URL)
 }
+
+// compareRef escapes a branch name, or OWNER:BRANCH, for a compare URL, where a '#' or '%' in the
+// name would otherwise start a fragment or an invalid escape.
+func compareRef(ref string) string {
+	return strings.ReplaceAll(url.PathEscape(ref), "%2F", "/")
+}

internal/app/pull_create_test.go

@@ -307,3 +307,9 @@ func TestPullCreateHelpListsDefaultsOnce(t *testing.T) {
 	require.Contains(t, out.String(), "(default true)")
 	require.NotContains(t, out.String(), "(default true) (default true)")
 }
+
+func TestCompareRefEscapesBranchNames(t *testing.T) {
+	require.Equal(t, "feature/login", compareRef("feature/login"))
+	require.Equal(t, "fix%2312", compareRef("fix#12"))
+	require.Equal(t, "bob:fix%25x", compareRef("bob:fix%x"))
+}

internal/app/pull_edit.go

@@ -194,6 +194,9 @@ removes or adds that marker.`, Annotations: pullArgumentsAnnotation(nil), Args:
 			if item, err = service.Edit(cmd.Context(), n, services.PullEdit{Title: &title}); err != nil {
 				return err
 			}
+			if item.Draft != undo {
+				return errors.New("the title was changed, but Gitea still " + map[bool]string{true: "does not treat", false: "treats"}[undo] + " the pull request as a draft; this instance may use a different draft prefix, so edit the title with: tea pr edit " + strconv.FormatInt(n, 10) + " --title")
+			}
 			if undo {
 				a.done(cmd, "Converted pull request %s to a draft", refLabel(t.Repo, n, item.Title))
 			} else {

internal/app/pull_edit_test.go

@@ -67,3 +67,14 @@ func TestPullEditAddsAssigneesWithoutExtraReads(t *testing.T) {
 	require.NoError(t, Execute(context.Background(), deps, []string{"pr", "edit", "7", "-R", "alice/project", "--add-assignees", "bob,Alice", "--json=number"}))
 	require.Equal(t, []string{"GET /api/v1/repos/alice/project/issues/7", "PATCH /api/v1/repos/alice/project/pulls/7"}, calls)
 }
+
+func TestPullReadyFailsWhenGiteaStillSeesADraft(t *testing.T) {
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, _ = w.Write([]byte(`{"number":7,"title":"Draft: x","draft":true}`))
+	}))
+	defer server.Close()
+	deps, _, diagnostics := serverDeps(t, server.URL)
+	err := Execute(context.Background(), deps, []string{"pr", "ready", "7", "-R", "alice/project"})
+	require.ErrorContains(t, err, "still treats the pull request as a draft")
+	require.NotContains(t, diagnostics.String(), "Marked")
+}

internal/app/pull_merge_form_test.go

@@ -245,3 +245,19 @@ func TestPullMergeRequiresNumberWithoutInteractive(t *testing.T) {
 	require.Error(t, err)
 	require.Contains(t, err.Error(), "pull request number")
 }
+
+func TestPullMergeReportsNoBranchDeletionForAGitPullRequests(t *testing.T) {
+	agit := `{"number":3,"title":"Add login","state":"open","base":{"ref":"main"},"head":{"ref":"refs/pull/3/head","label":"","sha":"abc123"}}`
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.URL.Path == "/api/v1/repos/alice/project":
+			_, _ = w.Write([]byte(mergeRepository))
+		case r.Method == http.MethodGet:
+			_, _ = w.Write([]byte(agit))
+		}
+	}))
+	defer server.Close()
+	deps, _, diagnostics := serverDeps(t, server.URL)
+	require.NoError(t, Execute(context.Background(), deps, []string{"pr", "merge", "3", "-R", "alice/project", "--yes", "--delete-branch"}))
+	require.NotContains(t, diagnostics.String(), "emote branch")
+}

internal/app/pull_mutations.go

@@ -128,6 +128,8 @@ $ tea pr close 13 -c "Superseded by #15" --delete-branch`
 // head branch of another open pull request, which deleting it would close.
 func (a *application) deleteHead(cmd *cobra.Command, service services.Pulls, item services.Pull, merging, local bool) {
 	switch {
+	case item.Head == "":
+		// An AGit pull request has no head branch to delete.
 	case item.HeadRemoteDeleted && merging:
 		a.done(cmd, "Deleted remote branch %s", item.Head)
 	case item.HeadRemoteDeleted:
@@ -358,7 +360,7 @@ the base branch.`, Annotations: pullArgumentsAnnotation(map[string]string{"tea.c
 		} else {
 			a.done(cmd, "%s pull request %s", mergeVerbs[method], ref)
 			if deleteBranch {
-				a.deleteHead(cmd, service, merged, true, deleteLocal)
+				a.deleteHead(cmd, service, merged, !item.HeadRemoteDeleted, deleteLocal)
 			}
 		}
 		return a.jsonResult(cmd, merged)

internal/app/pull_mutations_test.go

@@ -6,6 +6,7 @@ package app
 import (
 	"context"
 	"net/http"
+	"slices"
 	"strings"
 	"testing"
 
@@ -54,3 +55,60 @@ func TestPullDiffNameOnly(t *testing.T) {
 	require.NoError(t, Execute(context.Background(), deps, []string{"pr", "diff", "2", "-R", "alice/project", "--name-only"}))
 	require.Equal(t, "one.txt\ndocs/two.md\n", out.String())
 }
+
+func TestPullCloseCommentsFirstAndDeletesTheBranchOnlyWhenAsked(t *testing.T) {
+	for _, tc := range []struct {
+		flags   []string
+		deleted bool
+	}{
+		{nil, false},
+		{[]string{"--delete-branch"}, true},
+	} {
+		var requests []string
+		server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			requests = append(requests, r.Method+" "+r.URL.Path)
+			switch {
+			case r.Method == http.MethodGet:
+				_, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"open","base":{"ref":"main"},"head":{"ref":"feature","sha":"abc123","repo":{"full_name":"alice/project","permissions":{"push":true}}}}`))
+			case r.Method == http.MethodPatch:
+				_, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"closed","base":{"ref":"main"},"head":{"ref":"feature","sha":"abc123","repo":{"full_name":"alice/project","permissions":{"push":true}}}}`))
+			case r.Method == http.MethodPost:
+				w.WriteHeader(http.StatusCreated)
+				_, _ = w.Write([]byte(`{"id":1}`))
+			default:
+				w.WriteHeader(http.StatusNoContent)
+			}
+		}))
+		deps, _, _ := serverDeps(t, server.URL)
+		deps.Git = &fakeGit{results: map[string]string{"symbolic-ref --short HEAD": "main"}}
+		require.NoError(t, Execute(context.Background(), deps, append([]string{"pr", "close", "3", "-R", "alice/project", "-c", "Superseded"}, tc.flags...)))
+		server.Close()
+		comment := slices.Index(requests, "POST /api/v1/repos/alice/project/issues/3/comments")
+		edit := slices.Index(requests, "PATCH /api/v1/repos/alice/project/pulls/3")
+		require.True(t, comment >= 0 && comment < edit, requests)
+		require.Equal(t, tc.deleted, slices.Contains(requests, "DELETE /api/v1/repos/alice/project/branches/feature"), requests)
+	}
+}
+
+func TestPullCloseAndReopenRefuseMergedPullRequests(t *testing.T) {
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, http.MethodGet, r.Method)
+		_, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"closed","merged":true}`))
+	}))
+	defer server.Close()
+	for verb, want := range map[string]string{"close": "cannot be closed", "reopen": "cannot be reopened"} {
+		deps, _, _ := serverDeps(t, server.URL)
+		require.ErrorContains(t, Execute(context.Background(), deps, []string{"pr", verb, "3", "-R", "alice/project"}), want)
+	}
+}
+
+func TestPullReopenWarnsWhenAlreadyOpen(t *testing.T) {
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		require.Equal(t, http.MethodGet, r.Method)
+		_, _ = w.Write([]byte(`{"number":3,"title":"Add login","state":"open"}`))
+	}))
+	defer server.Close()
+	deps, _, diagnostics := serverDeps(t, server.URL)
+	require.NoError(t, Execute(context.Background(), deps, []string{"pr", "reopen", "3", "-R", "alice/project"}))
+	require.Contains(t, diagnostics.String(), "is already open")
+}

internal/app/pulls.go

@@ -40,7 +40,7 @@ func (a *application) pullListCommand() *cobra.Command {
 	var web bool
 	cmd := &cobra.Command{Use: "list", Short: "List pull requests", Long: `List the pull requests of a repository, newest first.
 
-The reviewers, reviewer_teams and merged_by JSON fields come from separate requests for
+The reviewers, reviewer_teams, review_decision and merged_by JSON fields come from separate requests for
 each pull request, which tea only makes when --json names them.`, Args: argsExact(0), RunE: func(cmd *cobra.Command, _ []string) error {
 		if id, err := strconv.ParseInt(milestone, 10, 64); err == nil && id < 1 {
 			return clierr.Wrap(clierr.Usage, errors.New("--milestone must be a milestone title or a positive ID"))
@@ -78,10 +78,14 @@ each pull request, which tea only makes when --json names them.`, Args: argsExac
 			return err
 		}
 		items := window.Items
-		if a.jsonSelects("reviewers", "reviewer_teams", "merged_by") {
-			if items, err = service.ViewAll(cmd.Context(), items); err != nil {
-				return err
+		switch {
+		case a.jsonSelects("review_decision"):
+			items, err = service.DecideAll(cmd.Context(), items)
+		case a.jsonSelects("reviewers", "reviewer_teams", "merged_by"):
+			items, err = service.ViewAll(cmd.Context(), items)
 		}
+		if err != nil {
+			return err
 		}
 		noun := map[string]string{"open": "open pull requests", "closed": "closed pull requests", "merged": "merged pull requests", "all": "pull requests"}[state]
 		what := noun + " in " + t.Repo

internal/app/pulls_test.go

@@ -90,3 +90,24 @@ func TestPullListFillsReviewersWhenSelected(t *testing.T) {
 	require.JSONEq(t, `[{"number":13,"reviewers":["bob"]}]`, out.String())
 	require.Equal(t, 1, views)
 }
+
+func TestPullListFillsReviewDecisionWhenSelected(t *testing.T) {
+	server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.Path {
+		case "/api/v1/repos/alice/project/pulls/13/reviews":
+			if r.URL.Query().Get("page") > "1" {
+				_, _ = w.Write([]byte(`[]`))
+				return
+			}
+			_, _ = w.Write([]byte(`[{"id":1,"state":"APPROVED","user":{"login":"bob"}}]`))
+		case "/api/v1/repos/alice/project/pulls/13":
+			_, _ = w.Write([]byte(`{"number":13}`))
+		default:
+			_, _ = w.Write([]byte(`[{"number":13}]`))
+		}
+	}))
+	defer server.Close()
+	deps, out, _ := serverDeps(t, server.URL)
+	require.NoError(t, Execute(context.Background(), deps, []string{"pr", "list", "-R", "alice/project", "--json", "number,review_decision"}))
+	require.JSONEq(t, `[{"number":13,"review_decision":"approved"}]`, out.String())
+}

internal/services/checks.go

@@ -124,6 +124,7 @@ func (s Pulls) WatchChecks(ctx context.Context, n int64, interval time.Duration,
 		return nil, errors.New("check polling interval must be positive")
 	}
 	var previous []Check
+	reported := false
 	for {
 		sha, err := s.checkHead(ctx, n)
 		if err != nil {
@@ -133,7 +134,9 @@ func (s Pulls) WatchChecks(ctx context.Context, n int64, interval time.Duration,
 		if err != nil {
 			return nil, err
 		}
-		if !slices.Equal(items, previous) {
+		// The first poll is reported even when empty, so a watcher can say it is waiting.
+		if !reported || !slices.Equal(items, previous) {
+			reported = true
 			if changed != nil {
 				if err := changed(items); err != nil {
 					return nil, err

internal/services/checks_test.go

@@ -58,6 +58,35 @@ func TestCheckWatchRevalidatesHeadBeforeReturning(t *testing.T) {
 	require.Equal(t, 5, heads)
 }
 
+func TestCheckWatchReportsTheEmptyStateOnce(t *testing.T) {
+	polls := 0
+	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if strings.Contains(r.URL.Path, "/pulls/") {
+			_, _ = w.Write([]byte(`{"head":{"sha":"abc"}}`))
+			return
+		}
+		if r.URL.Query().Get("page") != "1" {
+			_, _ = w.Write([]byte(`[]`))
+			return
+		}
+		polls++
+		if polls < 3 {
+			_, _ = w.Write([]byte(`[]`))
+			return
+		}
+		_, _ = w.Write([]byte(`[{"id":1,"context":"build","status":"success"}]`))
+	}))
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	defer cancel()
+	var reported []int
+	_, err := (Pulls{Client: client, Repository: "owner/repo"}).WatchChecks(ctx, 7, time.Millisecond, func(items []Check) error {
+		reported = append(reported, len(items))
+		return nil
+	})
+	require.NoError(t, err)
+	require.Equal(t, []int{0, 1}, reported)
+}
+
 func TestCheckWatchTreatsSkippedAsPassing(t *testing.T) {
 	snapshots := 0
 	client := newTestClient(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

internal/services/errors.go

@@ -159,6 +159,9 @@ var (
 	unknownUser = regexp.MustCompile(`user does not exist \[uid: \d+, name: ([^\]]+)\]`)
 	// pullTargets finds the branches in "pull request already exists for these targets [... head_branch: x, base_branch: y]".
 	pullTargets = regexp.MustCompile(`head_branch: ([^,\]]+), base_branch: ([^,\]]+)`)
+	// missingBranch finds the branch in the 404 messages `ref "x" does not exist` (create pull
+	// request) and `new base 'x' not exist` (edit pull request).
+	missingBranch = regexp.MustCompile(`^(?:ref "([^"]+)" does not exist|new base '([^']+)' not exist)$`)
 	// wikiTitle finds the page in "wiki page already exists [title: Home.md]".
 	wikiTitle = regexp.MustCompile(`\[title: (.+?)(?:\.md)?\]`)
 )
@@ -186,6 +189,9 @@ func Classify(err error, kind, key, scope string) error {
 		}
 		return conflict
 	}
+	if match := missingBranch.FindStringSubmatch(apiErr.Message); apiErr.Status == http.StatusNotFound && match != nil {
+		return &NotFoundError{Kind: "branch", Key: strconv.Quote(cmp.Or(match[1], match[2])), Scope: scope, Err: err}
+	}
 	if apiErr.Status == http.StatusNotFound && kind != "" {
 		return &NotFoundError{Kind: kind, Key: key, Scope: scope, Err: err}
 	}

internal/services/errors_test.go

@@ -64,3 +64,10 @@ func TestConflictHintsFollowTheReason(t *testing.T) {
 	err := &ConflictError{Reason: ReasonArchived, Scope: "alice/old", Hint: "unarchive it with: tea repo edit alice/old --archived=false"}
 	require.EqualError(t, err, "alice/old is archived (read-only); unarchive it with: tea repo edit alice/old --archived=false")
 }
+
+func TestClassifyNamesAMissingBranchNotThePullRequest(t *testing.T) {
+	for _, message := range []string{`ref "develp" does not exist`, `new base 'develp' not exist`} {
+		err := Classify(&transport.APIError{Status: http.StatusNotFound, Message: message}, "pull request", "#3", "alice/x")
+		require.EqualError(t, err, `branch "develp" not found in alice/x`)
+	}
+}

internal/services/pulls.go

@@ -241,6 +241,26 @@ func (s Pulls) ViewAll(ctx context.Context, items []Pull) ([]Pull, error) {
 	return viewed, err
 }
 
+// DecideAll is ViewAll that also fills each pull request's ReviewDecision from its reviews.
+func (s Pulls) DecideAll(ctx context.Context, items []Pull) ([]Pull, error) {
+	viewed := make([]Pull, len(items))
+	reviews := Reviews{Client: s.Client, Repository: s.Repository}
+	err := concurrently(len(items), func(i int) error {
+		item, err := s.View(ctx, items[i].Number)
+		if err != nil {
+			return err
+		}
+		all, err := reviews.All(ctx, item.Number)
+		if err != nil {
+			return err
+		}
+		item.ReviewDecision = ReviewDecision(LatestReviews(all), len(item.Reviewers)+len(item.ReviewerTeams) > 0)
+		viewed[i] = item
+		return nil
+	})
+	return viewed, err
+}
+
 func (s Pulls) get(ctx context.Context, n int64) (*gitea.PullRequest, error) {
 	item, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) {
 		return c.PullRequests.GetPullRequest(ctx, owner, name, n)