Repository: mattermost/mattermost-plugin-github (go)

Issue:
Labels with spaces don't work when saving a subscription
When trying to set a subscription dependent on a label that contains a space, creating the subscription will fail. The user will receive a response that only 1 feature can be included.  
The same subscription command works if the GitHub label does not contain white space. 
This is not a regression from the previous release. 

Creating a subscription as follows works as expected 
`/github subscribe DHaussermann/hello-world issues,label:"Help"`
 
Creating a subscription as follows will return an error about the number of features.  
`/github subscribe DHaussermann/hello-world issues,label:"Help Wanted"`


Pull request:
[GH-283] Fix parsing multi-word labels in subscription command
#### Summary
This PR will update the command parser such that it will skip whitespaces in two double-quotes. This will fix the related bug by protecting the integrity of multi-word labels such as "Help Wanted".

#### Ticket Link
Fixes #283 



Patch:
diff --git a/server/plugin/command.go b/server/plugin/command.go
index 72fca8e..5d1852f 100644
--- a/server/plugin/command.go
+++ b/server/plugin/command.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"unicode"
 
 	"github.com/google/go-github/v31/github"
 	"github.com/mattermost/mattermost-server/v5/mlog"
@@ -155,7 +156,16 @@ func (p *Plugin) handleSubscribe(_ *plugin.Context, args *model.CommandArgs, par
 		return err.Error()
 	}
 
-	return fmt.Sprintf("Successfully subscribed to %s.", repo)
+	msg := fmt.Sprintf("Successfully subscribed to %s.", repo)
+
+	ghRepo, _, err := githubClient.Repositories.Get(ctx, owner, repo)
+	if err != nil {
+		p.API.LogWarn("Failed to fetch repository", "error", err.Error())
+	} else if ghRepo != nil && ghRepo.GetPrivate() {
+		msg += "\n\n**Warning:** You subscribed to a private repository. Anyone with access to this channel will be able to read the events getting posted here."
+	}
+
+	return msg
 }
 
 func (p *Plugin) handleUnsubscribe(_ *plugin.Context, args *model.CommandArgs, parameters []string, _ *GitHubUserInfo) string {
@@ -258,16 +268,7 @@ func (p *Plugin) handleSettings(_ *plugin.Context, _ *model.CommandArgs, paramet
 type CommandHandleFunc func(c *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string
 
 func (p *Plugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*model.CommandResponse, *model.AppError) {
-	split := strings.Fields(args.Command)
-	command := split[0]
-	var parameters []string
-	action := ""
-	if len(split) > 1 {
-		action = split[1]
-	}
-	if len(split) > 2 {
-		parameters = split[2:]
-	}
+	command, action, parameters := parseCommand(args.Command)
 
 	if command != "/github" {
 		return &model.CommandResponse{}, nil
@@ -323,3 +324,54 @@ func (p *Plugin) ExecuteCommand(c *plugin.Context, args *model.CommandArgs) (*mo
 	p.postCommandResponse(args, fmt.Sprintf("Unknown action %v", action))
 	return &model.CommandResponse{}, nil
 }
+
+// parseCommand parses the entire command input string and retrieves the command, action and parameters
+func parseCommand(input string) (command, action string, parameters []string) {
+	split := make([]string, 0)
+	current := ""
+	inQuotes := false
+
+	for _, char := range input {
+		if unicode.IsSpace(char) {
+			// keep whitespaces that are inside double qoutes
+			if inQuotes {
+				current += " "
+				continue
+			}
+
+			// ignore successive whitespaces that are outside of double quotes
+			if len(current) == 0 && !inQuotes {
+				continue
+			}
+
+			// append the current word to the list & move on to the next word/expression
+			split = append(split, current)
+			current = ""
+			continue
+		}
+
+		// append the current character to the current word
+		current += string(char)
+
+		if char == '"' {
+			inQuotes = !inQuotes
+		}
+	}
+
+	// append the last word/expression to the list
+	if len(current) > 0 {
+		split = append(split, current)
+	}
+
+	command = split[0]
+
+	if len(split) > 1 {
+		action = split[1]
+	}
+
+	if len(split) > 2 {
+		parameters = split[2:]
+	}
+
+	return command, action, parameters
+}
diff --git a/server/plugin/template.go b/server/plugin/template.go
index ddb549c..0cef875 100644
--- a/server/plugin/template.go
+++ b/server/plugin/template.go
@@ -2,6 +2,7 @@ package plugin
 
 import (
 	"bytes"
+	"net/url"
 	"regexp"
 	"strings"
 	"text/template"
@@ -81,6 +82,25 @@ func init() {
 		return ">" + strings.ReplaceAll(body, "\n", "\n>")
 	}
 
+	// Escape characters not allowed in URL path
+	funcMap["pathEscape"] = url.PathEscape
+
+	// Transform multiple variables to dictionary
+	funcMap["dict"] = func(values ...interface{}) (map[string]interface{}, error) {
+		if len(values)%2 != 0 {
+			return nil, errors.New("invalid dict call, exactly one value is required for every key")
+		}
+		dict := make(map[string]interface{}, len(values)/2)
+		for i := 0; i < len(values); i += 2 {
+			key, ok := values[i].(string)
+			if !ok {
+				return nil, errors.New("dict keys must be strings")
+			}
+			dict[key] = values[i+1]
+		}
+		return dict, nil
+	}
+
 	masterTemplate = template.Must(template.New("master").Funcs(funcMap).Parse(""))
 
 	// The user template links to the corresponding GitHub user. If the GitHub user is a known
@@ -149,10 +169,24 @@ func init() {
 		`{{template "eventRepoIssueFullLink" .}} - {{.GetIssue.GetTitle}}`,
 	))
 
+	template.Must(masterTemplate.New("labels").Funcs(funcMap).Parse(`
+{{- if .Labels }}
+Labels: {{range $i, $el := .Labels -}}` + "{{- if $i}}, {{end}}[`{{ $el.Name }}`]({{ $.RepositoryURL }}/labels/{{ $el.Name | pathEscape }})" + `{{end -}}
+{{ end -}}
+`))
+
+	template.Must(masterTemplate.New("assignee").Funcs(funcMap).Parse(`
+{{- if .Assignees }}
+Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "user" $el}}{{end -}}
+{{- end -}}
+`))
+
 	template.Must(masterTemplate.New("newPR").Funcs(funcMap).Parse(`
 #### {{.GetPullRequest.GetTitle}}
 ##### {{template "eventRepoPullRequest" .}}
 #new-pull-request by {{template "user" .GetSender}}
+{{- template "labels" dict "Labels" .GetPullRequest.Labels "RepositoryURL" .GetRepo.GetHTMLURL  }}
+{{- template "assignee" .GetPullRequest }}
 
 {{.GetPullRequest.GetBody | removeComments | replaceAllGitHubUsernames}}
 `))
@@ -178,6 +212,8 @@ func init() {
 #### {{.GetIssue.GetTitle}}
 ##### {{template "eventRepoIssue" .}}
 #new-issue by {{template "user" .GetSender}}
+{{- template "labels" dict "Labels" .GetIssue.Labels "RepositoryURL" .GetRepo.GetHTMLURL  }}
+{{- template "assignee" .GetIssue }}
 
 {{.GetIssue.GetBody | removeComments | replaceAllGitHubUsernames}}
 `))


Test patch:
diff --git a/server/plugin/command_test.go b/server/plugin/command_test.go
index d4791e7..8130833 100644
--- a/server/plugin/command_test.go
+++ b/server/plugin/command_test.go
@@ -44,32 +44,32 @@ func TestValidateFeatures(t *testing.T) {
 		},
 		{
 			name: "all features valid with label but issues and pulls missing",
-			args: []string{"pushes", "label:\"ruby\""},
+			args: []string{"pushes", `label:"ruby"`},
 			want: output{false, []string{}},
 		},
 		{
 			name: "all features valid with label and issues in features",
-			args: []string{"issues", "label:\"ruby\""},
+			args: []string{"issues", `label:"ruby"`},
 			want: output{true, []string{}},
 		},
 		{
 			name: "all features valid with label and pulls in features",
-			args: []string{"pulls", "label:\"ruby\""},
+			args: []string{"pulls", `label:"ruby"`},
 			want: output{true, []string{}},
 		},
 		{
 			name: "multiple features invalid with label but issues and pulls missing",
-			args: []string{"issue", "push", "label:\"ruby\""},
+			args: []string{"issue", "push", `label:"ruby"`},
 			want: output{false, []string{"issue", "push"}},
 		},
 		{
 			name: "multiple features invalid with label and issues in features",
-			args: []string{"issues", "push", "create", "label:\"ruby\""},
+			args: []string{"issues", "push", "create", `label:"ruby"`},
 			want: output{false, []string{"push", "create"}},
 		},
 		{
 			name: "multiple features invalid with label and pulls in features",
-			args: []string{"pulls", "push", "create", "label:\"ruby\""},
+			args: []string{"pulls", "push", "create", `label:"ruby"`},
 			want: output{false, []string{"push", "create"}},
 		},
 	}
@@ -83,3 +83,117 @@ func TestValidateFeatures(t *testing.T) {
 		})
 	}
 }
+
+func TestParseCommand(t *testing.T) {
+	type output struct {
+		command    string
+		action     string
+		parameters []string
+	}
+
+	tt := []struct {
+		name  string
+		input string
+		want  output
+	}{
+		{
+			name:  "no parameters",
+			input: "/github subscribe",
+			want: output{
+				"/github",
+				"subscribe",
+				[]string(nil),
+			},
+		},
+		{
+			name:  "no action and no parameters",
+			input: "/github",
+			want: output{
+				"/github",
+				"",
+				[]string(nil),
+			},
+		},
+		{
+			name:  "simple one-word label",
+			input: `/github subscribe DHaussermann/hello-world issues,label:"Help"`,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Help"`},
+			},
+		},
+		{
+			name:  "two-word label",
+			input: `/github subscribe DHaussermann/hello-world issues,label:"Help Wanted"`,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Help Wanted"`},
+			},
+		},
+		{
+			name:  "multi-word label",
+			input: `/github subscribe DHaussermann/hello-world issues,label:"Good First Issue"`,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Good First Issue"`},
+			},
+		},
+		{
+			name:  "multiple spaces inside double-quotes",
+			input: `/github subscribe DHaussermann/hello-world issues,label:"Help    Wanted"`,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Help    Wanted"`},
+			},
+		},
+		{
+			name:  "multiple spaces outside of double-quotes",
+			input: `  /github    subscribe     DHaussermann/hello-world issues,label:"Help Wanted"`,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Help Wanted"`},
+			},
+		},
+		{
+			name:  "trailing whitespaces",
+			input: `/github subscribe DHaussermann/hello-world issues,label:"Help Wanted" `,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Help Wanted"`},
+			},
+		},
+		{
+			name:  "non-ASCII characters",
+			input: `/github subscribe طماطم issues,label:"日本語"`,
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"طماطم", `issues,label:"日本語"`},
+			},
+		},
+		{
+			name:  "line breaks",
+			input: "/github \nsubscribe\nDHaussermann/hello-world\nissues,label:\"Good First Issue\"",
+			want: output{
+				"/github",
+				"subscribe",
+				[]string{"DHaussermann/hello-world", `issues,label:"Good First Issue"`},
+			},
+		},
+	}
+
+	for _, tc := range tt {
+		t.Run(tc.name, func(t *testing.T) {
+			command, action, parameters := parseCommand(tc.input)
+			got := output{command, action, parameters}
+			errMsg := fmt.Sprintf("validateFeatures() = %v, want %v", got, tc.want)
+			assert.EqualValues(t, tc.want, got, errMsg)
+		})
+	}
+}
diff --git a/server/plugin/template_test.go b/server/plugin/template_test.go
index bdc9b5a..c08de39 100644
--- a/server/plugin/template_test.go
+++ b/server/plugin/template_test.go
@@ -19,6 +19,21 @@ var pushEventRepository = github.PushEventRepository{
 	HTMLURL:  sToP("https://github.com/mattermost/mattermost-plugin-github"),
 }
 
+var singleLabel = []*github.Label{
+	{
+		Name: sToP("Help Wanted"),
+	},
+}
+
+var labels = []*github.Label{
+	{
+		Name: sToP("Help Wanted"),
+	},
+	{
+		Name: sToP("Tech/Go"),
+	},
+}
+
 var pullRequest = github.PullRequest{
 	Number:    iToP(42),
 	HTMLURL:   sToP("https://github.com/mattermost/mattermost-plugin-github/pull/42"),
@@ -46,6 +61,36 @@ var pullRequestWithMentions = github.PullRequest{
 -->`),
 }
 
+var pullRequestWithLabelAndAssignee = github.PullRequest{
+	Number:    iToP(42),
+	HTMLURL:   sToP("https://github.com/mattermost/mattermost-plugin-github/pull/42"),
+	Title:     sToP("Leverage git-get-head"),
+	CreatedAt: tToP(time.Date(2019, 04, 01, 02, 03, 04, 0, time.UTC)),
+	UpdatedAt: tToP(time.Date(2019, 05, 01, 02, 03, 04, 0, time.UTC)),
+	Body: sToP(`<!-- Thank you for opening this pull request-->git-get-head gets the non-sent upstream heads inside the stashed non-cleaned applied areas, and after pruning bases to many archives, you can initialize the origin of the bases.
+<!-- Please make sure you have done the following :
+- Added tests
+- Removed console logs
+-->`),
+	Labels:    singleLabel,
+	Assignees: []*github.User{&user},
+}
+
+var pullRequestWithMultipleLabelsAndAssignees = github.PullRequest{
+	Number:    iToP(42),
+	HTMLURL:   sToP("https://github.com/mattermost/mattermost-plugin-github/pull/42"),
+	Title:     sToP("Leverage git-get-head"),
+	CreatedAt: tToP(time.Date(2019, 04, 01, 02, 03, 04, 0, time.UTC)),
+	UpdatedAt: tToP(time.Date(2019, 05, 01, 02, 03, 04, 0, time.UTC)),
+	Body: sToP(`<!-- Thank you for opening this pull request-->git-get-head gets the non-sent upstream heads inside the stashed non-cleaned applied areas, and after pruning bases to many archives, you can initialize the origin of the bases.
+<!-- Please make sure you have done the following :
+- Added tests
+- Removed console logs
+-->`),
+	Labels:    labels,
+	Assignees: []*github.User{&user, &user},
+}
+
 var mergedPullRequest = github.PullRequest{
 	Number:    iToP(42),
 	HTMLURL:   sToP("https://github.com/mattermost/mattermost-plugin-github/pull/42"),
@@ -79,6 +124,29 @@ var issueWithMentions = github.Issue{
 ` + gitHubMentions),
 }
 
+var issueWithLabelAndAssignee = github.Issue{
+	Number:    iToP(1),
+	HTMLURL:   sToP("https://github.com/mattermost/mattermost-plugin-github/issues/1"),
+	Title:     sToP("Implement git-get-head"),
+	CreatedAt: tToP(time.Date(2019, 04, 01, 02, 03, 04, 0, time.UTC)),
+	UpdatedAt: tToP(time.Date(2019, 05, 01, 02, 03, 04, 0, time.UTC)),
+	Body:      sToP(`<!-- Thank you for opening this issue-->git-get-head sounds like a great feature we should support`),
+	Labels:    singleLabel,
+	Assignee:  &user,
+	Assignees: []*github.User{&user},
+}
+
+var issueWithMultipleLabelsAndAssignee = github.Issue{
+	Number:    iToP(1),
+	HTMLURL:   sToP("https://github.com/mattermost/mattermost-plugin-github/issues/1"),
+	Title:     sToP("Implement git-get-head"),
+	CreatedAt: tToP(time.Date(2019, 04, 01, 02, 03, 04, 0, time.UTC)),
+	UpdatedAt: tToP(time.Date(2019, 05, 01, 02, 03, 04, 0, time.UTC)),
+	Body:      sToP(`<!-- Thank you for opening this issue-->git-get-head sounds like a great feature we should support`),
+	Labels:    labels,
+	Assignees: []*github.User{&user, &user},
+}
+
 var user = github.User{
 	Login:   sToP("panda"),
 	HTMLURL: sToP("https://github.com/panda"),
@@ -198,6 +266,48 @@ git-get-head gets the non-sent upstream heads inside the stashed non-cleaned app
 		require.NoError(t, err)
 		require.Equal(t, expected, actual)
 	}))
+
+	t.Run("with single label and assignee", func(t *testing.T) {
+		expected := `
+#### Leverage git-get-head
+##### [mattermost-plugin-github#42](https://github.com/mattermost/mattermost-plugin-github/pull/42)
+#new-pull-request by [panda](https://github.com/panda)
+Labels: ` + "[`Help Wanted`](https://github.com/mattermost/mattermost-plugin-github/labels/Help%20Wanted)" + `
+Assignees: [panda](https://github.com/panda)
+
+git-get-head gets the non-sent upstream heads inside the stashed non-cleaned applied areas, and after pruning bases to many archives, you can initialize the origin of the bases.
+
+`
+
+		actual, err := renderTemplate("newPR", &github.PullRequestEvent{
+			Repo:        &repo,
+			PullRequest: &pullRequestWithLabelAndAssignee,
+			Sender:      &user,
+		})
+		require.NoError(t, err)
+		require.Equal(t, expected, actual)
+	})
+
+	t.Run("with multiple labels and assignees", func(t *testing.T) {
+		expected := `
+#### Leverage git-get-head
+##### [mattermost-plugin-github#42](https://github.com/mattermost/mattermost-plugin-github/pull/42)
+#new-pull-request by [panda](https://github.com/panda)
+Labels: ` + "[`Help Wanted`](https://github.com/mattermost/mattermost-plugin-github/labels/Help%20Wanted), [`Tech/Go`](https://github.com/mattermost/mattermost-plugin-github/labels/Tech%2FGo)" + `
+Assignees: [panda](https://github.com/panda), [panda](https://github.com/panda)
+
+git-get-head gets the non-sent upstream heads inside the stashed non-cleaned applied areas, and after pruning bases to many archives, you can initialize the origin of the bases.
+
+`
+
+		actual, err := renderTemplate("newPR", &github.PullRequestEvent{
+			Repo:        &repo,
+			PullRequest: &pullRequestWithMultipleLabelsAndAssignees,
+			Sender:      &user,
+		})
+		require.NoError(t, err)
+		require.Equal(t, expected, actual)
+	})
 }
 
 func TestClosedPRMessageTemplate(t *testing.T) {
@@ -286,6 +396,46 @@ git-get-head sounds like a great feature we should support
 		require.NoError(t, err)
 		require.Equal(t, expected, actual)
 	}))
+
+	t.Run("with single label and assignee", func(t *testing.T) {
+		expected := `
+#### Implement git-get-head
+##### [mattermost-plugin-github#1](https://github.com/mattermost/mattermost-plugin-github/issues/1)
+#new-issue by [panda](https://github.com/panda)
+Labels: ` + "[`Help Wanted`](https://github.com/mattermost/mattermost-plugin-github/labels/Help%20Wanted)" + `
+Assignees: [panda](https://github.com/panda)
+
+git-get-head sounds like a great feature we should support
+`
+
+		actual, err := renderTemplate("newIssue", &github.IssuesEvent{
+			Repo:   &repo,
+			Issue:  &issueWithLabelAndAssignee,
+			Sender: &user,
+		})
+		require.NoError(t, err)
+		require.Equal(t, expected, actual)
+	})
+
+	t.Run("with multiple labels and assignees", func(t *testing.T) {
+		expected := `
+#### Implement git-get-head
+##### [mattermost-plugin-github#1](https://github.com/mattermost/mattermost-plugin-github/issues/1)
+#new-issue by [panda](https://github.com/panda)
+Labels: ` + "[`Help Wanted`](https://github.com/mattermost/mattermost-plugin-github/labels/Help%20Wanted), [`Tech/Go`](https://github.com/mattermost/mattermost-plugin-github/labels/Tech%2FGo)" + `
+Assignees: [panda](https://github.com/panda), [panda](https://github.com/panda)
+
+git-get-head sounds like a great feature we should support
+`
+
+		actual, err := renderTemplate("newIssue", &github.IssuesEvent{
+			Repo:   &repo,
+			Issue:  &issueWithMultipleLabelsAndAssignee,
+			Sender: &user,
+		})
+		require.NoError(t, err)
+		require.Equal(t, expected, actual)
+	})
 }
 
 func TestClosedIssueTemplate(t *testing.T) {