diff --git a/README.md b/README.md index 82dc15b..37656bd 100644 --- a/README.md +++ b/README.md @@ -185,6 +185,7 @@ listed newest first. | key | | |---|---| | `enter` | open the full pull request | +| `B` | from an opened pull request, the runs its build gates produced | | `v` | only pull requests waiting on your review | | `d` | cycle drafts hidden → drafts only → all | | `P` | publish a draft, or put a published pull request back into draft | @@ -205,6 +206,17 @@ first. `w` opens the first linked work item. `c` replies to the first unresolved thread and `R` resolves it, both without leaving the view. `D` opens the diff. +`B` opens the runs behind the pull request's build gates — the build list +again, over those runs rather than the project's recent ones, so `enter` +reads the failing gate's logs and `Q` re-runs it from there. The runs come +from the pull request's policy evaluations rather than from a branch search, +which is what makes them the gates rather than whatever happened to build +that branch: a gate that has not started a run yet is left out, and a pull +request nothing gates says so on the status line instead of opening an empty +list. The status line carries the policy's own verdict over with it, because +it is not the build's — a rejected gate over a green run means the policy was +evaluated against an earlier iteration. + `A` approves, `W` waits for the author and `X` rejects, each on a second press of the same key. They show when the pull request is yours to vote on — named directly, or through a group of yours, whether it was resolved at startup or diff --git a/internal/azdo/policies.go b/internal/azdo/policies.go new file mode 100644 index 0000000..cae41aa --- /dev/null +++ b/internal/azdo/policies.go @@ -0,0 +1,86 @@ +package azdo + +import ( + "fmt" + "net/url" +) + +// buildPolicyTypeID is Azure DevOps's identifier for the build validation +// policy. The evaluations endpoint returns every policy on a pull request — +// reviewer counts, linked work items, comment resolution — and this is the one +// that runs a pipeline, so it is the only one that can name a build. +const buildPolicyTypeID = "0609b952-1397-4640-95ec-e00a01b2c241" + +// evaluationsAPIVersion is APIVersion's preview twin for this endpoint, the +// same arrangement connectionData needs. Policy evaluations are still under +// preview, and asking for a plain version is refused outright: "The requested +// version \"7.1\" of the resource is under preview. The -preview flag must be +// supplied in the api-version for such requests." The .1 is the revision the +// shape read below belongs to; a bare -preview is accepted too, but pinning +// the revision is the point of pinning the version at all. +const evaluationsAPIVersion = APIVersion + "-preview.1" + +// unnamedPolicy is what a build validation with no display name is called. A +// policy is named when it is configured and most are, but an unnamed one still +// gates the pull request and still has a run worth opening. +const unnamedPolicy = "build validation" + +// GateBuild is one build validation policy on a pull request, and the run it +// produced. Status is the policy's own verdict — queued, running, approved, +// rejected — which is not the same as the build's: a rejected gate whose build +// succeeded means the policy was evaluated against an older iteration. +type GateBuild struct { + BuildID int + Policy string + Status string +} + +// PullRequestGateBuilds lists the runs the build validation policies on pr +// have produced, newest evaluation first as the endpoint returns them. +// +// A policy with no run yet is left out rather than returned with a zero id: +// the caller's whole purpose is opening the build, and a row that cannot be +// opened is worse than a shorter list that says so. +func (c *Client) PullRequestGateBuilds(pr PullRequest) ([]GateBuild, error) { + if pr.ProjectID == "" { + return nil, fmt.Errorf("!%d does not say which project its repository belongs to, so its build gates cannot be looked up", pr.ID) + } + + var resp struct { + Value []struct { + Configuration struct { + Type struct { + ID string `json:"id"` + } `json:"type"` + Settings struct { + DisplayName string `json:"displayName"` + } `json:"settings"` + } `json:"configuration"` + Status string `json:"status"` + Context struct { + BuildID int `json:"buildId"` + } `json:"context"` + } `json:"value"` + } + + artifact := fmt.Sprintf("vstfs:///CodeReview/CodeReviewId/%s/%d", pr.ProjectID, pr.ID) + endpoint := fmt.Sprintf("%s/%s/%s/_apis/policy/evaluations?artifactId=%s&api-version=%s", + c.root(), url.PathEscape(c.Org), url.PathEscape(c.Project), + url.QueryEscape(artifact), evaluationsAPIVersion) + if err := c.get(endpoint, &resp); err != nil { + return nil, err + } + + var gates []GateBuild + for _, v := range resp.Value { + if v.Configuration.Type.ID != buildPolicyTypeID || v.Context.BuildID == 0 { + continue + } + name := v.Configuration.Settings.DisplayName + if name == "" { + name = unnamedPolicy + } + gates = append(gates, GateBuild{BuildID: v.Context.BuildID, Policy: name, Status: v.Status}) + } + return gates, nil +} diff --git a/internal/azdo/policies_test.go b/internal/azdo/policies_test.go new file mode 100644 index 0000000..32fc03d --- /dev/null +++ b/internal/azdo/policies_test.go @@ -0,0 +1,89 @@ +package azdo + +import ( + "net/http" + "strings" + "testing" +) + +// gateBody is one pull request's policy evaluations: two build validations — +// one that has produced a run and one still queued behind the policy itself — +// and a reviewer policy that is not a build at all. +const gateBody = `{"value":[ + {"configuration":{"type":{"id":"0609b952-1397-4640-95ec-e00a01b2c241","displayName":"Build"}, + "settings":{"displayName":"CI gate","buildDefinitionId":12}}, + "status":"rejected","context":{"buildId":9001}}, + {"configuration":{"type":{"id":"0609b952-1397-4640-95ec-e00a01b2c241","displayName":"Build"}, + "settings":{"displayName":"Nightly gate","buildDefinitionId":13}}, + "status":"queued","context":{}}, + {"configuration":{"type":{"id":"fa4e907d-c16b-4a4c-9dfa-4906e5d171dd","displayName":"Minimum number of reviewers"}, + "settings":{"minimumApproverCount":2}}, + "status":"approved","context":{}} +]}` + +func TestPullRequestGateBuildsKeepsTheBuildsAPolicyHasRun(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + want := "vstfs:///CodeReview/CodeReviewId/p-guid/512" + if got := r.URL.Query().Get("artifactId"); got != want { + t.Errorf("artifactId = %q, want %q", got, want) + } + if !strings.Contains(r.URL.Path, "/_apis/policy/evaluations") { + t.Errorf("path = %q, want the policy evaluations endpoint", r.URL.Path) + } + // Azure DevOps refuses this endpoint at a plain version: "The + // requested version "7.1" of the resource is under preview. The + // -preview flag must be supplied in the api-version for such + // requests." + if got := r.URL.Query().Get("api-version"); !strings.HasSuffix(got, "-preview.1") { + t.Errorf("api-version = %q, want the preview form the endpoint requires", got) + } + w.Write([]byte(gateBody)) + }) + + gates, err := c.PullRequestGateBuilds(PullRequest{ID: 512, ProjectID: "p-guid"}) + if err != nil { + t.Fatalf("PullRequestGateBuilds returned %v", err) + } + if len(gates) != 1 { + t.Fatalf("got %d gates, want only the one with a build", len(gates)) + } + got := gates[0] + if got.BuildID != 9001 { + t.Errorf("build id = %d, want 9001", got.BuildID) + } + if got.Policy != "CI gate" { + t.Errorf("policy = %q, want CI gate", got.Policy) + } + if got.Status != "rejected" { + t.Errorf("status = %q, want rejected", got.Status) + } +} + +func TestPullRequestGateBuildsNamesAnUnnamedPolicy(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"value":[ + {"configuration":{"type":{"id":"0609b952-1397-4640-95ec-e00a01b2c241","displayName":"Build"},"settings":{}}, + "status":"approved","context":{"buildId":42}}]}`)) + }) + + gates, err := c.PullRequestGateBuilds(PullRequest{ID: 7, ProjectID: "p-guid"}) + if err != nil { + t.Fatalf("PullRequestGateBuilds returned %v", err) + } + if len(gates) != 1 { + t.Fatalf("got %d gates, want 1", len(gates)) + } + if gates[0].Policy != "build validation" { + t.Errorf("policy = %q, want the fallback name", gates[0].Policy) + } +} + +func TestPullRequestGateBuildsWithoutAProjectIDSaysSo(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + t.Error("the evaluations endpoint was called without a project id") + }) + + if _, err := c.PullRequestGateBuilds(PullRequest{ID: 512}); err == nil { + t.Fatal("PullRequestGateBuilds returned no error for a pull request with no project id") + } +} diff --git a/internal/azdo/pullrequests.go b/internal/azdo/pullrequests.go index 558a15e..528288b 100644 --- a/internal/azdo/pullrequests.go +++ b/internal/azdo/pullrequests.go @@ -59,10 +59,15 @@ func (r Reviewer) VoteLabel() string { // PullRequest is one active pull request, in any repository in the project. type PullRequest struct { - ID int - Title string - Repo string - RepoID string + ID int + Title string + Repo string + RepoID string + // ProjectID is the GUID of the team project the repository belongs to. + // The project is known by name everywhere else in boardwalk, but the + // policy endpoint addresses a pull request by an artifact id built from + // the GUID, and the name will not do there. + ProjectID string Author string AuthorKey string // uniqueName, lowercased, compared against Client.Me IsDraft bool @@ -193,8 +198,11 @@ type pullRequestJSON struct { UniqueName string `json:"uniqueName"` } `json:"createdBy"` Repository struct { - ID string `json:"id"` - Name string `json:"name"` + ID string `json:"id"` + Name string `json:"name"` + Project struct { + ID string `json:"id"` + } `json:"project"` } `json:"repository"` LastMergeSourceCommit struct { CommitID string `json:"commitId"` @@ -217,6 +225,7 @@ func (v pullRequestJSON) pullRequest() PullRequest { Title: v.Title, Repo: v.Repository.Name, RepoID: v.Repository.ID, + ProjectID: v.Repository.Project.ID, Author: v.CreatedBy.DisplayName, AuthorKey: strings.ToLower(v.CreatedBy.UniqueName), IsDraft: v.IsDraft, diff --git a/internal/azdo/pullrequests_test.go b/internal/azdo/pullrequests_test.go index f94262a..7652487 100644 --- a/internal/azdo/pullrequests_test.go +++ b/internal/azdo/pullrequests_test.go @@ -602,3 +602,20 @@ func TestCreateThreadReportsTheServersRefusal(t *testing.T) { t.Errorf("error = %v, want the server's own message in it", err) } } + +func TestPullRequestParsesTheProjectItsRepositoryBelongsTo(t *testing.T) { + c := testClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"value":[ + {"pullRequestId":512,"title":"Retry webhooks","sourceRefName":"refs/heads/x","targetRefName":"refs/heads/main", + "creationDate":"2026-09-10T09:00:00Z","createdBy":{"displayName":"Dev","uniqueName":"dev@acme.test"}, + "repository":{"id":"r1","name":"platform-api","project":{"id":"p-guid","name":"Platform"}}}]}`)) + }) + + prs, err := c.PullRequests() + if err != nil { + t.Fatalf("PullRequests returned %v", err) + } + if prs[0].ProjectID != "p-guid" { + t.Errorf("project id = %q, want p-guid", prs[0].ProjectID) + } +} diff --git a/internal/ui/builds.go b/internal/ui/builds.go index 86e888d..ebc241b 100644 --- a/internal/ui/builds.go +++ b/internal/ui/builds.go @@ -79,6 +79,19 @@ type timelineMsg struct { Err error } +// gateRunsMsg carries the runs behind one pull request's build gates. It is +// its own message rather than a buildsMsg because both listings can be alive +// at once — the project's recent runs underneath, one pull request's gates on +// top — and Root broadcasts data to every view in the stack. It names the +// pull request for the same reason timelineMsg names its build, and carries +// Err rather than emitting ErrMsg, which has no owner to route on and would +// be read by the project list as its own fetch failing. +type gateRunsMsg struct { + PR int + Builds []azdo.Build + Err error +} + // buildPRMsg carries the outcome of looking up the pull request a build ran // for. It names the build for the same reason timelineMsg does: pressing p // again for a different build before the first lookup answers must not let @@ -116,6 +129,16 @@ type Builds struct { queueDefinition int queueLabel string + // prID and gateBuilds are set when this list was opened from a pull + // request's build gates rather than from the menu: the pull request it + // belongs to, and the runs its gates produced. gateBuilds is what Init + // fetches instead of the project's recent runs, and it is fixed for the + // life of the view — a gate that starts a fresh run after this list + // opened is picked up by reopening it from the pull request, which is + // where the policy evaluations are read. + prID int + gateBuilds []int + // linkBuild is the id of the build a pull request lookup is in flight // for, or 0 when none is. It is how a late answer knows whether it is // still the one asked about — see buildPRMsg. @@ -156,8 +179,23 @@ func NewBuilds(c *azdo.Client) *Builds { return m } -// Init fetches the project's recent runs. +// NewBuildsForPullRequest opens the same browser over one pull request's gate +// runs. The ids come from the policy evaluations the pull request view read, +// so this fetches each run by id rather than filtering the project's recent +// ones: a gate that ran days ago is still the gate on this pull request, and +// would have fallen off the end of a fixed-size listing. +func NewBuildsForPullRequest(c *azdo.Client, prID int, ids []int) *Builds { + m := NewBuilds(c) + m.prID, m.gateBuilds = prID, ids + return m +} + +// Init fetches the project's recent runs, or the gate runs this list was +// opened for. func (m *Builds) Init() tea.Cmd { + if len(m.gateBuilds) > 0 { + return tea.Batch(m.fetchGateBuilds(), m.work.begin(1)) + } client := m.client fetch := func() tea.Msg { builds, err := client.Builds(buildPageSize) @@ -169,6 +207,30 @@ func (m *Builds) Init() tea.Cmd { return tea.Batch(fetch, m.work.begin(1)) } +// fetchGateBuilds reads each gate run by id. They are fetched in one command +// rather than one each so the list appears whole: a handful of gates is a +// short serial walk, and rows arriving one at a time would reorder the list +// under the cursor as each answer landed. +func (m *Builds) fetchGateBuilds() tea.Cmd { + client, prID, ids := m.client, m.prID, m.gateBuilds + return func() tea.Msg { return gateRuns(prID, ids, client.BuildByID) } +} + +// gateRuns reads each gate run and folds the outcome into one answer. The +// fetch is a parameter rather than the client so this — the part that decides +// what the list is told — can be exercised without a server. +func gateRuns(prID int, ids []int, fetch func(int) (azdo.Build, error)) gateRunsMsg { + builds := make([]azdo.Build, 0, len(ids)) + for _, id := range ids { + b, err := fetch(id) + if err != nil { + return gateRunsMsg{PR: prID, Err: fmt.Errorf("could not fetch build %d: %w", id, err)} + } + builds = append(builds, b) + } + return gateRunsMsg{PR: prID, Builds: builds} +} + // applyRows rebuilds the browser's rows, carrying forward whatever timelines // have already loaded. func (m *Builds) applyRows() { @@ -268,7 +330,25 @@ func (m *Builds) Update(msg tea.Msg) (View, tea.Cmd) { return m, m.work.tick(msg) case buildsMsg: + if m.prID != 0 { + // The project's recent runs, broadcast to a list that is showing + // one pull request's gates. See gateRunsMsg. + return m, nil + } + m.work.done() + m.builds, m.loaded = msg.Builds, true + m.applyRows() + return m, m.fetchTimelines() + + case gateRunsMsg: + if m.prID == 0 || msg.PR != m.prID { + return m, nil + } m.work.done() + if msg.Err != nil { + m.status, m.failed = msg.Err.Error(), true + return m, nil + } m.builds, m.loaded = msg.Builds, true m.applyRows() return m, m.fetchTimelines() @@ -507,11 +587,19 @@ func (m *Builds) Body(width, height int) string { return placeholder("builds", m.status, m.failed, m.work.View()) } if m.browser.Len() == 0 { - return emptyState("no pipeline runs in this project", width, height) + return emptyState(m.emptyLine(), width, height) } return m.browser.View() } +// emptyLine says what is missing in the terms the list was opened in. +func (m *Builds) emptyLine() string { + if m.prID != 0 { + return fmt.Sprintf("no gate runs left to show for !%d", m.prID) + } + return "no pipeline runs in this project" +} + // refresh re-reads the list from scratch, dropping every timeline with it so // the rows rebuild against what the server says now. It is its own method // because three things ask for it — the refresh key and the two actions that @@ -566,8 +654,14 @@ func (m *Builds) handleArmedBuild(msg tea.KeyMsg) tea.Cmd { return armed.fire(m.client) } -// Title reports the run count and the project the builds belong to. +// Title reports the run count and the project the builds belong to, naming +// the pull request when the list is one pull request's gates rather than the +// project's recent runs. func (m *Builds) Title() string { + if m.prID != 0 { + return fmt.Sprintf("build gates for !%d (%d) · %s/%s", + m.prID, m.browser.Len(), m.client.Org, m.client.Project) + } return fmt.Sprintf("builds (%d) · %s/%s", m.browser.Len(), m.client.Org, m.client.Project) } diff --git a/internal/ui/builds_test.go b/internal/ui/builds_test.go index 5d7f8e7..85fd147 100644 --- a/internal/ui/builds_test.go +++ b/internal/ui/builds_test.go @@ -381,3 +381,113 @@ func TestBuildsEmptyStateSaysThereAreNoRuns(t *testing.T) { t.Errorf("empty view did not draw the gulls:\n%s", out) } } + +func TestBuildsForAPullRequestNamesItInTheTitle(t *testing.T) { + c, builds := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001, 9000}) + updated, _ := m.Update(gateRunsMsg{PR: 512, Builds: builds}) + m = updated.(*Builds) + + if !strings.Contains(m.Title(), "!512") { + t.Errorf("title = %q, want it to name the pull request", m.Title()) + } + if !strings.Contains(m.Title(), "(2)") { + t.Errorf("title = %q, want the run count", m.Title()) + } +} + +func TestBuildsForAPullRequestOpensOnTheGateRunsItWasGiven(t *testing.T) { + c, _ := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + + if got := m.gateBuilds; len(got) != 1 || got[0] != 9001 { + t.Errorf("gate builds = %v, want the ids it was opened with", got) + } +} + +func TestBuildsForAPullRequestSaysSoWhenEmpty(t *testing.T) { + c, _ := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ := m.Update(gateRunsMsg{PR: 512}) + m = updated.(*Builds) + + if body := m.Body(120, 20); !strings.Contains(body, "!512") { + t.Errorf("empty state = %q, want it to name the pull request", body) + } +} + +func TestBuildsForAPullRequestReportsItsOwnFetchFailing(t *testing.T) { + c, _ := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ := m.Update(gateRunsMsg{PR: 512, Err: errTest}) + m = updated.(*Builds) + + status, isErr := m.Status() + if !isErr || !strings.Contains(status, errTest.Error()) { + t.Errorf("status = %q, isErr = %v, want the failure reported", status, isErr) + } +} + +// The gate list and the project list can both be alive in one stack — menu, +// builds, p, then B from the pull request it opened — and Root broadcasts +// data to every view in it. Each listing must land in exactly one of them. +func TestBuildsForAPullRequestIgnoresTheProjectListing(t *testing.T) { + c, builds := buildFixture() + m := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ := m.Update(buildsMsg{Builds: builds}) + m = updated.(*Builds) + + if m.loaded { + t.Errorf("the gate list took the project's recent runs as its own:\n%s", m.Body(120, 20)) + } +} + +func TestBuildsIgnoresGateRunsThatAreNotItsOwn(t *testing.T) { + c, builds := buildFixture() + + project := newBuilds(t) + updated, _ := project.Update(gateRunsMsg{PR: 512, Builds: builds[:1]}) + project = updated.(*Builds) + if project.browser.Len() != 2 { + t.Errorf("the project list took a gate listing: %d rows, want its own 2", project.browser.Len()) + } + + gates := NewBuildsForPullRequest(c, 512, []int{9001}) + updated, _ = gates.Update(gateRunsMsg{PR: 999, Builds: builds}) + gates = updated.(*Builds) + if gates.loaded { + t.Error("the gate list took another pull request's gate runs") + } +} + +// The gate list is the one Builds instance Root does not build, so the +// property that makes a second instance safe is asserted here rather than +// assumed: every answer it reads back names the pull request it belongs to, +// including a failed one. An ErrMsg would name nobody and be read by the +// project list as its own fetch failing. +func TestGateRunsAnswerNamesThePullRequestEvenWhenTheFetchFails(t *testing.T) { + _, builds := buildFixture() + + got := gateRuns(512, []int{9001, 9000}, func(id int) (azdo.Build, error) { + for _, b := range builds { + if b.ID == id { + return b, nil + } + } + return azdo.Build{}, errTest + }) + if got.PR != 512 || got.Err != nil || len(got.Builds) != 2 { + t.Fatalf("gateRuns = %+v, want !512's two runs and no error", got) + } + + got = gateRuns(512, []int{7}, func(int) (azdo.Build, error) { return azdo.Build{}, errTest }) + if got.PR != 512 { + t.Errorf("a failed answer names !%d, want !512", got.PR) + } + if got.Err == nil { + t.Error("a failed fetch reported no error") + } + if got.Builds != nil { + t.Errorf("a failed fetch returned %d runs, want none", len(got.Builds)) + } +} diff --git a/internal/ui/keys.go b/internal/ui/keys.go index 717a422..2d07e71 100644 --- a/internal/ui/keys.go +++ b/internal/ui/keys.go @@ -28,11 +28,18 @@ var ( // keyState is capital because s is already "copy slack link"; a is kept // alongside it rather than folded in, since Active is the common case the // picker would otherwise make one keystroke slower. - keyState = key.NewBinding(key.WithKeys("S"), key.WithHelp("S", "set state")) - keyBranch = key.NewBinding(key.WithKeys("b"), key.WithHelp("b", "branch")) - keyDrafts = key.NewBinding(key.WithKeys("d"), key.WithHelp("d", "drafts")) - keyReview = key.NewBinding(key.WithKeys("v"), key.WithHelp("v", "needs my review")) - keyLinkedPR = key.NewBinding(key.WithKeys("p"), key.WithHelp("p", "linked pull request")) + keyState = key.NewBinding(key.WithKeys("S"), key.WithHelp("S", "set state")) + keyBranch = key.NewBinding(key.WithKeys("b"), key.WithHelp("b", "branch")) + keyDrafts = key.NewBinding(key.WithKeys("d"), key.WithHelp("d", "drafts")) + keyReview = key.NewBinding(key.WithKeys("v"), key.WithHelp("v", "needs my review")) + keyLinkedPR = key.NewBinding(key.WithKeys("p"), key.WithHelp("p", "linked pull request")) + // keyGateBuilds opens the runs a pull request's build validation policies + // produced — the reverse of keyLinkedPR, which finds the pull request a + // run built. It is capital because "b" is the branch key on the work item + // list, and that list reaches the pull request pane this binding lives on + // through keyLinkedPR: a key meaning one thing on a list and another on a + // pane that list opens is the trap keyDiff's comment names. + keyGateBuilds = key.NewBinding(key.WithKeys("B"), key.WithHelp("B", "build gates")) keyLinkedItem = key.NewBinding(key.WithKeys("w"), key.WithHelp("w", "linked work item")) keyLogs = key.NewBinding(key.WithKeys("enter"), key.WithHelp("enter", "logs")) keyItem = key.NewBinding(key.WithKeys("enter"), key.WithHelp("enter", "open item")) diff --git a/internal/ui/pullrequestdetail.go b/internal/ui/pullrequestdetail.go index 2bb2d02..d96d900 100644 --- a/internal/ui/pullrequestdetail.go +++ b/internal/ui/pullrequestdetail.go @@ -48,6 +48,15 @@ type threadResolvedMsg struct { Err error } +// gateBuildsMsg carries the runs this pull request's build validation +// policies produced. It names its pull request for the same reason +// threadsLoadedMsg does: Root broadcasts data to every view in the stack. +type gateBuildsMsg struct { + PR int + Gates []azdo.GateBuild + Err error +} + // voteCastMsg carries the outcome of casting a reviewer vote. type voteCastMsg struct { PR int @@ -133,6 +142,20 @@ type PullRequestDetail struct { // considered, so there is nothing left to arm a vote while it is open. armedVote *pendingVote + // gatesPending is true while a build gate lookup is in flight, so a + // second press does not start another, and a late answer knows whether + // it is still the one being waited on. + gatesPending bool + + // hidden is true from the moment this view pushes something on top of + // itself until it is back on top. Root broadcasts gateBuildsMsg to the + // whole stack, so a lookup that resolves while the diff pane is showing + // must not push the build list over it. A KeyMsg is proof of being back + // on top — Root routes keys to the top view alone — so the flag clears + // the moment one arrives. Same arrangement as the builds view; see its + // hidden field. + hidden bool + // armedDraft is the same arrangement for the draft toggle, and for the // same reason — see draft.go. No two of the three modal states can be // open at once: each of them swallows every key that is not its own @@ -185,6 +208,42 @@ func (m *PullRequestDetail) fetchThreads() tea.Cmd { // fetchLinked resolves the work items this pull request is attached to. The // endpoint returns bare ids, so the titles come from one batch fetch. +// fetchGateBuilds reads the runs this pull request's build validation +// policies produced. It is asked for on the keypress rather than fetched on +// entry, the same reasoning the builds view gives for its pull request link: +// most visits to this pane never ask, and the evaluations are a call of their +// own. +func (m *PullRequestDetail) fetchGateBuilds() tea.Cmd { + client, pr := m.client, m.pr + return func() tea.Msg { + gates, err := client.PullRequestGateBuilds(pr) + if err != nil { + return gateBuildsMsg{PR: pr.ID, Err: fmt.Errorf("could not load the build gates for !%d: %w", pr.ID, err)} + } + return gateBuildsMsg{PR: pr.ID, Gates: gates} + } +} + +// gateBuildIDs is the runs, in the order the policies came back in. +func gateBuildIDs(gates []azdo.GateBuild) []int { + ids := make([]int, 0, len(gates)) + for _, g := range gates { + ids = append(ids, g.BuildID) + } + return ids +} + +// gateSummary is the status line the build list is opened with. The policy +// verdict is worth carrying over because it is not the build's: a rejected +// gate whose run succeeded means the policy was evaluated against an earlier +// iteration, and the build list alone would show nothing but a green run. +func gateSummary(gates []azdo.GateBuild) string { + if len(gates) == 1 { + return fmt.Sprintf("%s: %s", gates[0].Policy, gates[0].Status) + } + return fmt.Sprintf("%d build gates", len(gates)) +} + func (m *PullRequestDetail) fetchLinked() tea.Cmd { client, repo, id := m.client, m.pr.RepoID, m.pr.ID return func() tea.Msg { @@ -445,11 +504,39 @@ func (m *PullRequestDetail) Update(msg tea.Msg) (View, tea.Cmd) { m.invalidate() return m, nil + case gateBuildsMsg: + m.work.done() + if msg.PR != m.pr.ID || !m.gatesPending { + // Either an answer for the other pull request this stack can have + // open, or one this view is no longer waiting on. + return m, nil + } + m.gatesPending = false + if msg.Err != nil { + m.status, m.failed = msg.Err.Error(), true + return m, nil + } + if len(msg.Gates) == 0 { + m.status, m.failed = fmt.Sprintf("no build policy has run for !%d", m.pr.ID), false + return m, nil + } + if m.hidden { + // The lookup was still in flight when the user opened something + // else — see the hidden field's doc. + return m, nil + } + m.status, m.failed = gateSummary(msg.Gates), false + builds := NewBuildsForPullRequest(m.client, m.pr.ID, gateBuildIDs(msg.Gates)) + return m, func() tea.Msg { return PushMsg{View: builds} } + case StatusMsg: m.status = msg.Text return m, nil case tea.KeyMsg: + // A key is proof this view is back on top: Root routes keys to the + // top view alone. + m.hidden = false // The reply prompt owns every key while it is open, including esc and // the letters that are otherwise actions — typing "o" into it must add // the letter, not open a browser. Mirrors the branch prompt in @@ -519,13 +606,22 @@ func (m *PullRequestDetail) Update(msg tea.Msg) (View, tea.Cmd) { // and the diff pane shows each comment against the line it was // written on rather than fetching the same threads again. diff := NewPullRequestDiff(m.client, m.pr, m.threads) + m.hidden = true return m, func() tea.Msg { return PushMsg{View: diff} } + case key.Matches(msg, keyGateBuilds): + if m.gatesPending { + return m, nil // already looking them up + } + m.gatesPending = true + m.status, m.failed = "looking for the build gates…", false + return m, tea.Batch(m.fetchGateBuilds(), m.work.begin(1)) case key.Matches(msg, keyLinkedItem): if len(m.linked) == 0 { m.status = "no work items are linked to this pull request" return m, nil } item := NewItem(m.client, m.linked[0], nil) + m.hidden = true return m, func() tea.Msg { return PushMsg{View: item} } case key.Matches(msg, keyReply): t, ok := m.selectedThread() @@ -740,13 +836,13 @@ func (m *PullRequestDetail) Title() string { // keys_test.go, which renders every view's footer through the real help // component rather than trusting a character count. func (m *PullRequestDetail) Keys() help.KeyMap { - short := []key.Binding{keyDiff, keyLinkedItem, keyReply, keyResolve} + short := []key.Binding{keyDiff, keyGateBuilds, keyReply, keyResolve} // The draft toggle is in the panel rather than on the footer, labelled for // the pull request it is looking at — "publish" on a draft, "mark draft" // on a published one — and absent altogether on a merged or abandoned one, // which this view reaches through a work item's or a build's link. Offering // a key that would only be refused is worse than not offering it. - full := []key.Binding{keyDiff, keyLinkedItem, keyReply, keyResolve} + full := []key.Binding{keyDiff, keyGateBuilds, keyLinkedItem, keyReply, keyResolve} if draftable(m.pr) { full = append(full, draftBinding(m.pr.IsDraft)) } diff --git a/internal/ui/pullrequestdetail_test.go b/internal/ui/pullrequestdetail_test.go index 65ced49..257e7bc 100644 --- a/internal/ui/pullrequestdetail_test.go +++ b/internal/ui/pullrequestdetail_test.go @@ -695,3 +695,71 @@ func TestVoteCastIgnoresAnotherPullRequests(t *testing.T) { t.Error("a vote belonging to another pull request was applied") } } + +func TestPullRequestDetailOpensTheGateBuilds(t *testing.T) { + m := newPRDetail(t, []azdo.Thread{}) + + updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + m = updated.(*PullRequestDetail) + if cmd == nil { + t.Fatal("B produced no command") + } + if status, _ := m.Status(); !strings.Contains(status, "build gate") { + t.Errorf("status = %q, want it to say the gates are being looked up", status) + } + + updated, cmd = m.Update(gateBuildsMsg{PR: 512, Gates: []azdo.GateBuild{{BuildID: 9001, Policy: "CI gate", Status: "rejected"}}}) + m = updated.(*PullRequestDetail) + if cmd == nil { + t.Fatal("the gate answer produced no command") + } + push, ok := cmd().(PushMsg) + if !ok { + t.Fatalf("the gate answer produced %T, want a PushMsg", cmd()) + } + if !strings.Contains(push.View.Title(), "!512") { + t.Errorf("pushed view = %q, want the pull request's build list", push.View.Title()) + } +} + +func TestPullRequestDetailSaysWhenNothingGatesIt(t *testing.T) { + m := newPRDetail(t, []azdo.Thread{}) + m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + + updated, cmd := m.Update(gateBuildsMsg{PR: 512}) + m = updated.(*PullRequestDetail) + if cmd != nil { + t.Errorf("a pull request with no gates pushed a view anyway") + } + if status, _ := m.Status(); !strings.Contains(status, "no build") { + t.Errorf("status = %q, want it to say there is nothing gating this pull request", status) + } +} + +func TestPullRequestDetailIgnoresAGateAnswerForAnotherPullRequest(t *testing.T) { + // Root broadcasts data to every view in the stack, and two of these can be + // open at once — see replySentMsg. + m := newPRDetail(t, []azdo.Thread{}) + m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + + _, cmd := m.Update(gateBuildsMsg{PR: 999, Gates: []azdo.GateBuild{{BuildID: 1}}}) + if cmd != nil { + t.Error("an answer for another pull request pushed a view") + } +} + +func TestPullRequestDetailDoesNotJumpToGatesFromUnderAnotherView(t *testing.T) { + m := newPRDetail(t, nil) + m.pr.SourceCommit, m.pr.TargetCommit = "aaa", "bbb" + + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'B'}}) + m = updated.(*PullRequestDetail) + // The diff pane goes on top while the lookup is still in flight. + updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'D'}}) + m = updated.(*PullRequestDetail) + + _, cmd := m.Update(gateBuildsMsg{PR: 512, Gates: []azdo.GateBuild{{BuildID: 9001}}}) + if cmd != nil { + t.Error("the gate answer pushed a view over the one the user had opened") + } +} diff --git a/internal/ui/root_test.go b/internal/ui/root_test.go index dfc248c..9fd8271 100644 --- a/internal/ui/root_test.go +++ b/internal/ui/root_test.go @@ -344,11 +344,18 @@ func TestRootBroadcastsErrMsgToTheViewItBelongsToEvenWhenNotOnTop(t *testing.T) // emit ErrMsg with no id naming which instance it belongs to, which is only // safe to broadcast because exactly one instance of each is ever alive in a // stack — every one is built once, by Root.build, from an empty stack (see -// NewRoot and the menu's enter handler in Root.key). A ninth feature that -// pushed a second instance of one of these three views — a work item view -// reachable from inside another view's drill-down, say — would make a -// broadcast ErrMsg ambiguous between two live views and resurrect the very -// bug the broadcast was written to fix, silently. +// NewRoot and the menu's enter handler in Root.key). A feature that pushed a +// second instance of one of these three views — a work item view reachable +// from inside another view's drill-down, say — would make a broadcast ErrMsg +// ambiguous between two live views and resurrect the very bug the broadcast +// was written to fix, silently. +// +// NewBuildsForPullRequest is the one exception, and it earns it by not +// relying on the invariant at all: a build list opened on a pull request's +// gates fetches through gateRunsMsg, which names its pull request, ignores +// the project listing, and reports its own failures rather than emitting an +// unowned ErrMsg — see the gate tests in builds_test.go. Its call to +// NewBuilds is skipped below for that reason, and nothing else may be. // // There is no runtime state that exercises this today — the risk is a future // call site, not a reachable program state — so this checks the source itself @@ -378,6 +385,9 @@ func TestListViewsAreConstructedOnlyByRootBuild(t *testing.T) { if strings.Contains(line, "func "+ctor) { continue // the constructor's own definition, not a call site. } + if name == "builds.go" && ctor == "NewBuilds(" && strings.Contains(line, "m := NewBuilds(c)") { + continue // NewBuildsForPullRequest; see this test's doc. + } if name != "root.go" { t.Errorf("%s:%d calls %s outside root.go — a second live instance of this view "+ "would make the ErrMsg broadcast ambiguous between it and the original",