Merge pull request #23 from ravocean/trunk

Finished refactoring (addCombined)

api/queries_pr.go

@@ -151,17 +151,27 @@ type PullRequestFile struct {
 type ReviewRequests struct {
 	Nodes []struct {
 		RequestedReviewer struct {
-			TypeName string `json:"__typename"`
-			Login    string `json:"login"`
-			Name     string `json:"name"`
+			TypeName     string `json:"__typename"`
+			Login        string `json:"login"`
+			Name         string `json:"name"`
+			Slug         string `json:"slug"`
+			Organization struct {
+				Login string `json:"login"`
+			}
 		}
 	}
 }
 
+const teamTypeName = "Team"
+
 func (r ReviewRequests) Logins() []string {
 	logins := make([]string, len(r.Nodes))
 	for i, a := range r.Nodes {
-		logins[i] = a.RequestedReviewer.Login
+		if a.RequestedReviewer.TypeName == teamTypeName {
+			logins[i] = fmt.Sprintf("%s/%s", a.RequestedReviewer.Organization.Login, a.RequestedReviewer.Slug)
+		} else {
+			logins[i] = a.RequestedReviewer.Login
+		}
 	}
 	return logins
 }
@@ -393,8 +403,8 @@ func PullRequestStatus(client *Client, repo ghrepo.Interface, options StatusOpti
 	queryPrefix := `
 	query PullRequestStatus($owner: String!, $repo: String!, $headRefName: String!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) {
 		repository(owner: $owner, name: $repo) {
-			defaultBranchRef { 
-				name 
+			defaultBranchRef {
+				name
 			}
 			pullRequests(headRefName: $headRefName, first: $per_page, orderBy: { field: CREATED_AT, direction: DESC }) {
 				totalCount
@@ -410,8 +420,8 @@ func PullRequestStatus(client *Client, repo ghrepo.Interface, options StatusOpti
 		queryPrefix = `
 		query PullRequestStatus($owner: String!, $repo: String!, $number: Int!, $viewerQuery: String!, $reviewerQuery: String!, $per_page: Int = 10) {
 			repository(owner: $owner, name: $repo) {
-				defaultBranchRef { 
-					name 
+				defaultBranchRef {
+					name
 				}
 				pullRequest(number: $number) {
 					...prWithReviews

api/queries_pr_test.go

@@ -1,12 +1,13 @@
 package api
 
 import (
-	"reflect"
+	"encoding/json"
 	"testing"
 
 	"github.com/MakeNowJust/heredoc"
 	"github.com/cli/cli/internal/ghrepo"
 	"github.com/cli/cli/pkg/httpmock"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestBranchDeleteRemote(t *testing.T) {
@@ -148,13 +149,94 @@ func Test_determinePullRequestFeatures(t *testing.T) {
 			}
 
 			gotPrFeatures, err := determinePullRequestFeatures(httpClient, tt.hostname)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("determinePullRequestFeatures() error = %v, wantErr %v", err, tt.wantErr)
+			if tt.wantErr {
+				assert.Error(t, err)
 				return
+			} else {
+				assert.NoError(t, err)
 			}
-			if !reflect.DeepEqual(gotPrFeatures, tt.wantPrFeatures) {
-				t.Errorf("determinePullRequestFeatures() = %v, want %v", gotPrFeatures, tt.wantPrFeatures)
-			}
+			assert.Equal(t, tt.wantPrFeatures, gotPrFeatures)
+		})
+	}
+}
+
+func Test_Logins(t *testing.T) {
+	rr := ReviewRequests{}
+	var tests = []struct {
+		name             string
+		requestedReviews string
+		want             []string
+	}{
+		{
+			name:             "no requested reviewers",
+			requestedReviews: `{"nodes": []}`,
+			want:             []string{},
+		},
+		{
+			name: "user",
+			requestedReviews: `{"nodes": [
+				{
+					"requestedreviewer": {
+						"__typename": "User", "login": "testuser"
+					}
+				}
+			]}`,
+			want: []string{"testuser"},
+		},
+		{
+			name: "team",
+			requestedReviews: `{"nodes": [
+				{
+					"requestedreviewer": {
+						"__typename": "Team",
+						"name": "Test Team",
+						"slug": "test-team",
+						"organization": {"login": "myorg"}
+					}
+				}
+			]}`,
+			want: []string{"myorg/test-team"},
+		},
+		{
+			name: "multiple users and teams",
+			requestedReviews: `{"nodes": [
+				{
+					"requestedreviewer": {
+						"__typename": "User", "login": "user1"
+					}
+				},
+				{
+					"requestedreviewer": {
+						"__typename": "User", "login": "user2"
+					}
+				},
+				{
+					"requestedreviewer": {
+						"__typename": "Team",
+						"name": "Test Team",
+						"slug": "test-team",
+						"organization": {"login": "myorg"}
+					}
+				},
+				{
+					"requestedreviewer": {
+						"__typename": "Team",
+						"name": "Dev Team",
+						"slug": "dev-team",
+						"organization": {"login": "myorg"}
+					}
+				}
+			]}`,
+			want: []string{"user1", "user2", "myorg/test-team", "myorg/dev-team"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := json.Unmarshal([]byte(tt.requestedReviews), &rr)
+			assert.NoError(t, err, "Failed to unmarshal json string as ReviewRequests")
+			logins := rr.Logins()
+			assert.Equal(t, tt.want, logins)
 		})
 	}
 }

api/query_builder.go

@@ -41,7 +41,11 @@ var prReviewRequests = shortenQuery(`
 			requestedReviewer {
 				__typename,
 				...on User{login},
-				...on Team{name}
+				...on Team{
+					organization{login}
+					name,
+					slug
+				}
 			}
 		}
 	}

pkg/cmd/api/api_test.go

@@ -43,7 +43,7 @@ func Test_NewCmdApi(t *testing.T) {
 				RequestInputFile:    "",
 				RawFields:           []string(nil),
 				MagicFields:         []string(nil),
-				RequestHeaders:      []string(nil),
+				RequestHeaders:      []string(nil),   
 				ShowResponseHeaders: false,
 				Paginate:            false,
 				Silent:              false,

pkg/cmd/browse/browse.go

@@ -3,8 +3,8 @@ package browse
 import (
 	"fmt"
 	"net/http"
-	"strings"
 	"strconv"
+	"strings"
 
 	"github.com/cli/cli/internal/ghrepo"
 	"github.com/cli/cli/pkg/cmdutil"
@@ -12,7 +12,6 @@ import (
 	"github.com/spf13/cobra"
 
 	"github.com/cli/cli/api"
-	"github.com/cli/cli/utils"
 )
 
 type browser interface {
@@ -107,7 +106,7 @@ func openInBrowser(cmd *cobra.Command, opts *BrowseOptions) {
 	repoUrl := ghrepo.GenerateRepoURL(baseRepo, "")
 
 	if !hasArg(opts) && hasFlag(cmd) {
-		response, repoUrl = addFlags(opts, repoUrl)
+		response, repoUrl = addFlag(opts, repoUrl)
 	} else if hasArg(opts) && !hasFlag(cmd) {
 		response, repoUrl = addArg(opts, repoUrl, branchName)
 	} else if hasArg(opts) && hasFlag(cmd) {
@@ -124,35 +123,39 @@ func openInBrowser(cmd *cobra.Command, opts *BrowseOptions) {
 
 func addCombined(opts *BrowseOptions, url string, branchName string) (exitCode, string) {
 
-	if(!opts.BranchFlag){ // gh browse --settings main.go
+	if !opts.BranchFlag { // gh browse --settings main.go
 		return exitError, "" // TODO still need to make error codes better!! If not branch say, "Sorry this flag doesnt work with args"
 	}
 
-
-	arr := parseFileArg(opts)
-	if(len(arr) > 1){
-		return exitSuccess, url + "/tree/" + branchName + "/" + arr[0] + "#L" + arr[1]
+	if opts.AdditionalArg == "" {
+		return exitSuccess, url + "/tree/" + opts.SelectorArg
 	}
 
-	return exitSuccess, url + "/tree/" + branchName + "/" + arr[0]
+	arr := parseFileArg(opts)
+	if len(arr) > 1 {
+		return exitSuccess, url + "/tree/" + opts.AdditionalArg + "/" + arr[0] + "#L" + arr[1]
+	}
+
+	return exitSuccess, url + "/tree/" + opts.AdditionalArg + "/" + arr[0]
 
 }
 
-func addFlags(opts *BrowseOptions, url string) (exitCode, string) {
+func addFlag(opts *BrowseOptions, url string) (exitCode, string) {
 	if opts.ProjectsFlag {
 		return exitSuccess, url + "/projects"
 	} else if opts.SettingsFlag {
 		return exitSuccess, url + "/settings"
 	} else if opts.WikiFlag {
 		return exitSuccess, url + "/wiki"
+	} else if opts.BranchFlag {
+		return exitError, "" // If you have a branch flag but no arg, this won't work
 	}
 	return exitError, "" //TODO need a new exit error later (possiblly)
 }
 
 func addArg(opts *BrowseOptions, url string, branchName string) (exitCode, string) {
 
-
-	if(opts.AdditionalArg != ""){
+	if opts.AdditionalArg != "" {
 		return exitError, "" // TODO refine the exit codes to make sense
 	}
 
@@ -162,14 +165,14 @@ func addArg(opts *BrowseOptions, url string, branchName string) (exitCode, strin
 	}
 
 	arr := parseFileArg(opts)
-	if(len(arr) > 1){
+	if len(arr) > 1 {
 		return exitSuccess, url + "/tree/" + branchName + "/" + arr[0] + "#L" + arr[1]
 	}
 
 	return exitSuccess, url + "/tree/" + branchName + "/" + arr[0]
 }
 
-func parseFileArg(opts *BrowseOptions) (string[]) {
+func parseFileArg(opts *BrowseOptions) []string {
 	arr := strings.Split(opts.SelectorArg, ":")
 	return arr
 }

pkg/cmd/browse/browse_test.go

@@ -9,4 +9,4 @@ func TestNewCmdBrowse(t *testing.T) {
 	//		instead of opening multiple browsers for each test,
 	//		we can test the http code sent back after calling a site
 
-}
+}

pkg/cmd/issue/list/list.go

@@ -112,9 +112,14 @@ func listRun(opts *ListOptions) error {
 		return err
 	}
 
+	issueState := strings.ToLower(opts.State)
+	if issueState == "open" && shared.QueryHasStateClause(opts.Search) {
+		issueState = ""
+	}
+
 	filterOptions := prShared.FilterOptions{
 		Entity:    "issue",
-		State:     strings.ToLower(opts.State),
+		State:     issueState,
 		Assignee:  opts.Assignee,
 		Labels:    opts.Labels,
 		Author:    opts.Author,

pkg/cmd/pr/list/list.go

@@ -116,9 +116,14 @@ func listRun(opts *ListOptions) error {
 		return err
 	}
 
+	prState := strings.ToLower(opts.State)
+	if prState == "open" && shared.QueryHasStateClause(opts.Search) {
+		prState = ""
+	}
+
 	filters := shared.FilterOptions{
 		Entity:     "pr",
-		State:      strings.ToLower(opts.State),
+		State:      prState,
 		Author:     opts.Author,
 		Assignee:   opts.Assignee,
 		Labels:     opts.Labels,

pkg/cmd/pr/shared/params.go

@@ -2,6 +2,7 @@ package shared
 
 import (
 	"fmt"
+	"github.com/google/shlex"
 	"net/url"
 	"strings"
 
@@ -243,6 +244,21 @@ func SearchQueryBuild(options FilterOptions) string {
 	return q.String()
 }
 
+func QueryHasStateClause(searchQuery string) bool {
+	argv, err := shlex.Split(searchQuery)
+	if err != nil {
+		return false
+	}
+
+	for _, arg := range argv {
+		if arg == "is:closed" || arg == "is:merged" || arg == "state:closed" || arg == "state:merged" || strings.HasPrefix(arg, "merged:") || strings.HasPrefix(arg, "closed:") {
+			return true
+		}
+	}
+
+	return false
+}
+
 // MeReplacer resolves usages of `@me` to the handle of the currently logged in user.
 type MeReplacer struct {
 	apiClient *api.Client

pkg/cmd/pr/shared/params_test.go

@@ -1,6 +1,7 @@
 package shared
 
 import (
+	"github.com/stretchr/testify/assert"
 	"net/http"
 	"reflect"
 	"testing"
@@ -151,3 +152,43 @@ func TestMeReplacer_Replace(t *testing.T) {
 		})
 	}
 }
+
+func Test_QueryHasStateClause(t *testing.T) {
+	tests := []struct {
+		searchQuery string
+		hasState    bool
+	}{
+		{
+			searchQuery: "is:closed is:merged",
+			hasState:    true,
+		},
+		{
+			searchQuery: "author:mislav",
+			hasState:    false,
+		},
+		{
+			searchQuery: "assignee:g14a mentions:vilmibm",
+			hasState:    false,
+		},
+		{
+			searchQuery: "merged:>2021-05-20",
+			hasState:    true,
+		},
+		{
+			searchQuery: "state:merged state:open",
+			hasState:    true,
+		},
+		{
+			searchQuery: "assignee:g14a is:closed",
+			hasState:    true,
+		},
+		{
+			searchQuery: "state:closed label:bug",
+			hasState:    true,
+		},
+	}
+	for _, tt := range tests {
+		gotState := QueryHasStateClause(tt.searchQuery)
+		assert.Equal(t, tt.hasState, gotState)
+	}
+}