From 60d22b7d6d6f6dfaa2e23d73fe9a5e02db3d28ae Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Fri, 2 Apr 2021 21:26:21 -0300 Subject: [PATCH 1/4] Fix detecting PR status when passing branch as arg --- api/queries_pr.go | 1 + 1 file changed, 1 insertion(+) diff --git a/api/queries_pr.go b/api/queries_pr.go index 9b4661e37..2816f5676 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -672,6 +672,7 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea number title state + closed body mergeable additions From be5dab8166948b3a11898861834acefa8e9077a6 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Mon, 5 Apr 2021 18:57:14 -0300 Subject: [PATCH 2/4] Add test case --- pkg/cmd/pr/reopen/reopen_test.go | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/pkg/cmd/pr/reopen/reopen_test.go b/pkg/cmd/pr/reopen/reopen_test.go index 19c4a1e6a..69ba4efb5 100644 --- a/pkg/cmd/pr/reopen/reopen_test.go +++ b/pkg/cmd/pr/reopen/reopen_test.go @@ -86,6 +86,39 @@ func TestPRReopen(t *testing.T) { } } +func TestPRReopen_BranchArg(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + http.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(` + { "data": { "repository": { "pullRequests": { + "nodes": [ + { "id": "THE-ID", "number": 666, "title": "The title of the PR", "headRefName": "fix-bug", "closed": true } + ] + } } } }`), + ) + http.Register( + httpmock.GraphQL(`mutation PullRequestReopen\b`), + httpmock.GraphQLMutation(`{"id": "THE-ID"}`, + func(inputs map[string]interface{}) { + assert.Equal(t, inputs["pullRequestId"], "THE-ID") + }), + ) + + output, err := runCommand(http, true, "fix-bug") + if err != nil { + t.Fatalf("error running command `pr reopen`: %v", err) + } + + r := regexp.MustCompile(`Reopened pull request #666 \(The title of the PR\)`) + + if !r.MatchString(output.Stderr()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + func TestPRReopen_alreadyOpen(t *testing.T) { http := &httpmock.Registry{} defer http.Verify(t) From 9c471fe7a5f7cdf1995995a7b75abfd0b45cadc5 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Thu, 8 Apr 2021 00:23:22 -0300 Subject: [PATCH 3/4] Improve PR status detection --- api/queries_pr.go | 6 ++++-- pkg/cmd/pr/close/close.go | 2 +- pkg/cmd/pr/ready/ready.go | 2 +- pkg/cmd/pr/reopen/reopen.go | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 2816f5676..49b114ed4 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -33,7 +33,6 @@ type PullRequest struct { Number int Title string State string - Closed bool URL string BaseRefName string HeadRefName string @@ -142,6 +141,10 @@ func (pr PullRequest) Identifier() string { return pr.ID } +func (pr PullRequest) IsOpen() bool { + return pr.State == "OPEN" +} + type PullRequestReviewStatus struct { ChangesRequested bool Approved bool @@ -672,7 +675,6 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea number title state - closed body mergeable additions diff --git a/pkg/cmd/pr/close/close.go b/pkg/cmd/pr/close/close.go index f4cc442c2..850bd7631 100644 --- a/pkg/cmd/pr/close/close.go +++ b/pkg/cmd/pr/close/close.go @@ -76,7 +76,7 @@ func closeRun(opts *CloseOptions) error { if pr.State == "MERGED" { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) can't be closed because it was already merged", cs.Red("!"), pr.Number, pr.Title) return cmdutil.SilentError - } else if pr.Closed { + } else if !pr.IsOpen() { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already closed\n", cs.Yellow("!"), pr.Number, pr.Title) return nil } diff --git a/pkg/cmd/pr/ready/ready.go b/pkg/cmd/pr/ready/ready.go index 003902963..a33248f9a 100644 --- a/pkg/cmd/pr/ready/ready.go +++ b/pkg/cmd/pr/ready/ready.go @@ -75,7 +75,7 @@ func readyRun(opts *ReadyOptions) error { return err } - if pr.Closed { + if !pr.IsOpen() { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is closed. Only draft pull requests can be marked as \"ready for review\"", cs.Red("!"), pr.Number) return cmdutil.SilentError } else if !pr.IsDraft { diff --git a/pkg/cmd/pr/reopen/reopen.go b/pkg/cmd/pr/reopen/reopen.go index 135e341fa..f22b8bfd2 100644 --- a/pkg/cmd/pr/reopen/reopen.go +++ b/pkg/cmd/pr/reopen/reopen.go @@ -70,7 +70,7 @@ func reopenRun(opts *ReopenOptions) error { return cmdutil.SilentError } - if !pr.Closed { + if pr.IsOpen() { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) is already open\n", cs.Yellow("!"), pr.Number, pr.Title) return nil } From a8921162b15ff3465b8c49d0b08f7711ca9f283a Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Thu, 8 Apr 2021 00:27:52 -0300 Subject: [PATCH 4/4] Update tests --- pkg/cmd/pr/close/close_test.go | 13 +++++++++---- pkg/cmd/pr/ready/ready_test.go | 6 +++--- pkg/cmd/pr/reopen/reopen_test.go | 8 ++++---- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/pr/close/close_test.go b/pkg/cmd/pr/close/close_test.go index 09d8e521b..4aa239384 100644 --- a/pkg/cmd/pr/close/close_test.go +++ b/pkg/cmd/pr/close/close_test.go @@ -67,7 +67,7 @@ func TestPrClose(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 96, "title": "The title of the PR" } + "pullRequest": { "id": "THE-ID", "number": 96, "title": "The title of the PR", "state": "OPEN" } } } }`), ) http.Register( @@ -98,7 +98,7 @@ func TestPrClose_alreadyClosed(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "number": 101, "title": "The title of the PR", "closed": true } + "pullRequest": { "number": 101, "title": "The title of the PR", "state": "CLOSED" } } } }`), ) @@ -121,8 +121,13 @@ func TestPrClose_deleteBranch(t *testing.T) { http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 96, "title": "The title of the PR", "headRefName":"blueberries", "headRepositoryOwner": {"login": "OWNER"}} + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 96, + "title": "The title of the PR", + "headRefName":"blueberries", + "headRepositoryOwner": {"login": "OWNER"}, + "state": "OPEN" } } } }`), ) http.Register( diff --git a/pkg/cmd/pr/ready/ready_test.go b/pkg/cmd/pr/ready/ready_test.go index caf2c7e5f..a53a15d24 100644 --- a/pkg/cmd/pr/ready/ready_test.go +++ b/pkg/cmd/pr/ready/ready_test.go @@ -147,7 +147,7 @@ func TestPRReady(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 444, "closed": false, "isDraft": true} + "pullRequest": { "id": "THE-ID", "number": 444, "state": "OPEN", "isDraft": true} } } }`), ) http.Register( @@ -178,7 +178,7 @@ func TestPRReady_alreadyReady(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "number": 445, "closed": false, "isDraft": false} + "pullRequest": { "number": 445, "state": "OPEN", "isDraft": false} } } }`), ) @@ -202,7 +202,7 @@ func TestPRReady_closed(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "number": 446, "closed": true, "isDraft": true} + "pullRequest": { "number": 446, "state": "CLOSED", "isDraft": true} } } }`), ) diff --git a/pkg/cmd/pr/reopen/reopen_test.go b/pkg/cmd/pr/reopen/reopen_test.go index 69ba4efb5..9c94f11b8 100644 --- a/pkg/cmd/pr/reopen/reopen_test.go +++ b/pkg/cmd/pr/reopen/reopen_test.go @@ -63,7 +63,7 @@ func TestPRReopen(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "id": "THE-ID", "number": 666, "title": "The title of the PR", "closed": true} + "pullRequest": { "id": "THE-ID", "number": 666, "title": "The title of the PR", "state": "CLOSED" } } } }`), ) http.Register( @@ -95,7 +95,7 @@ func TestPRReopen_BranchArg(t *testing.T) { httpmock.StringResponse(` { "data": { "repository": { "pullRequests": { "nodes": [ - { "id": "THE-ID", "number": 666, "title": "The title of the PR", "headRefName": "fix-bug", "closed": true } + { "id": "THE-ID", "number": 666, "title": "The title of the PR", "headRefName": "fix-bug", "state": "CLOSED" } ] } } } }`), ) @@ -127,7 +127,7 @@ func TestPRReopen_alreadyOpen(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "closed": false} + "pullRequest": { "number": 666, "title": "The title of the PR", "state": "OPEN" } } } }`), ) @@ -151,7 +151,7 @@ func TestPRReopen_alreadyMerged(t *testing.T) { httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { - "pullRequest": { "number": 666, "title": "The title of the PR", "closed": true, "state": "MERGED"} + "pullRequest": { "number": 666, "title": "The title of the PR", "state": "MERGED"} } } }`), )