Repository: mattermost/mattermost-plugin-github
Pull request #335 (2020-07-27 16:27:27)

[GH-257] Display labels and assignee on new issues and PRs
<!-- Thank you for contributing a pull request! Here are a few tips to help you:

1. If this is your first contribution, make sure you've read the Contribution Checklist https://developers.mattermost.com/contribute/getting-started/contribution-checklist/
2. Read our blog post about "Submitting Great PRs" https://developers.mattermost.com/blog/2019-01-24-submitting-great-prs
3. Take a look at other repository specific documentation at https://developers.mattermost.com/contribute
-->

#### Summary
Display labels and assignee when creating new Issue or PR.

#### Ticket Link
<!--
If this pull request addresses a Help Wanted ticket, please link the relevant GitHub issue, e.g.

  Fixes https://github.com/mattermost/mattermost-server/issues/XXXXX

Otherwise, link the JIRA ticket.
-->
Fixes https://github.com/mattermost/mattermost-plugin-github/issues/257



Szymongib: Hi @mickmister, 
I have a couple of follow up questions to finalize PR:
- Do you have any more suggestions regarding how labels and assignee should be displayed?
Currently, it looks like that:
![image](https://user-images.githubusercontent.com/19947566/88567432-c67bc400-d037-11ea-9a89-e8652f3f8a10.png)
- Do you think those should be displayed in case of any other event as well?

codecov-commenter: # [Codecov](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=h1) Report
> Merging [#335](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=desc) into [master](https://codecov.io/gh/mattermost/mattermost-plugin-github/commit/cc58a21bb35079f0cbb823371274e1bac15bacb2&el=desc) will **increase** coverage by `0.62%`.
> The diff coverage is `84.00%`.

[![Impacted file tree graph](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335/graphs/tree.svg?width=650&height=150&src=pr&token=0uHZTjTrgl)](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #335      +/-   ##
==========================================
+ Coverage   19.97%   20.59%   +0.62%     
==========================================
  Files          10       10              
  Lines        2553     2578      +25     
==========================================
+ Hits          510      531      +21     
- Misses       2008     2010       +2     
- Partials       35       37       +2     
```


| [Impacted Files](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [server/plugin/template.go](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335/diff?src=pr&el=tree#diff-c2VydmVyL3BsdWdpbi90ZW1wbGF0ZS5nbw==) | `94.80% <84.00%> (-1.32%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=footer). Last update [cc58a21...f837662](https://codecov.io/gh/mattermost/mattermost-plugin-github/pull/335?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

mm-cloud-bot: Creating a new SpinWick test server using Mattermost Cloud.
mm-cloud-bot: Mattermost test server created! :tada:

Access here: https://mattermost-plugin-github-pr-335.test.mattermost.cloud

| Account Type | Username | Password |
|---|---|---|
| Admin | sysadmin | Sys@dmin123 |
| User | user-1 | User-1@123 |
mickmister: @Szymongib A quick pass through the code looks good :+1:

> Do you have any more suggestions regarding how labels and assignee should be displayed?

Do you mind posting some screenshots of the entire post, one with lots of summary/description text, and with a small amount?

> Do you think those should be displayed in case of any other event as well?

I think the additions would make sense on new comment posts as well, that are sent to a subscribed channel. As opposed to a DM notification from a comment.
@asaadmahmood What do you think?

What do you think about showing the other fields that can be set on an issue/PR? Such as Projects, Milestones, and Linked issues/PRs?
mm-cloud-bot: Test server destroyed
mickmister: I'm thinking this has a lot of overlap with the plugin tooltip display, but this PR's functionality would show on mobile. Also, a post by the GitHub bot is more significant than a single link to an issue/PR, which is the scenario for the tooltip.

We may want to keep it simple with just labels and assignees. @Szymongib I'm wondering how it looks in the mobile app with more fields showing
asaadmahmood: @mickmister @Szymongib Let's get a full screenshot of the channel/center channel to see how it looks.
Also @mickmister please help me explain this a bit more. I'm assuming this information will be displayed as a message to the channel (if subscribed) or DM.

What would be the case where this appears as a comment?
mickmister: @asaadmahmood 
> Also @mickmister please help me explain this a bit more. I'm assuming this information will be displayed as a message to the channel (if subscribed) or DM.

Right now, it is showing for new PRs and new issues. If a channel is subscribed to pull request or issue creation, there will be posts in that channel and the labels will show up with this PRs functionality.

> What would be the case where this appears as a comment?

It's just a thought of having them show for comments as well. Channels can subscribe to GitHub comment events, and the labels could be shown with those posts. DM notifications could also have them, but I think that will be bloated for the GitHub DM feed.
asaadmahmood: @mickmister I think it makes sense for issue creation and Pull requests, but I think for every comment, it may not be the best for us, as we may get a duplication on that most of the time.

I feel like we can have a separate event (if we don't have that right now) for label updates or assignee updates that can be used if people want to record those activities as well.
mickmister: @asaadmahmood Currently there is no event for label or assignee changes. Oftentimes, labels are added by the reporter  after issue creation. So having an event for that would be nice. It may be within scope of task since it is directly related to this use case.
Szymongib: @mickmister I think showing additional information like Project or Milestone could be added later as an enhancement to first see feedback from adding labels and assignee.

Here are some screenshots:

![long issue](https://user-images.githubusercontent.com/19947566/88775415-f684ad80-d184-11ea-8054-dd170a0562e1.png)


![short issue](https://user-images.githubusercontent.com/19947566/88775625-377cc200-d185-11ea-9361-e4619646a4cc.png)


PR:

![PR](https://user-images.githubusercontent.com/19947566/88777086-1fa63d80-d187-11ea-8dd9-de0e10a6e5c3.png)


and from the mobile app (IOS):

![long issue](https://user-images.githubusercontent.com/19947566/88776756-ac9cc700-d186-11ea-9a13-4397ecfdad74.PNG)


![short issue](https://user-images.githubusercontent.com/19947566/88776775-b45c6b80-d186-11ea-880c-72bf2b890c9f.PNG)


![PR](https://user-images.githubusercontent.com/19947566/88777398-7449b880-d187-11ea-8c16-6b524bae79cf.PNG)



mickmister: @asaadmahmood Here are some screenshots ^

@Szymongib Placement of the new items looks good to me
mickmister: @Szymongib Maybe place commas between each label? Just a thought, not sure if it is right to do so
Szymongib: > Maybe place commas between each label? Just a thought, not sure if it is right to do so

Sure @mickmister, I think that makes sense, especially for mobile version.

Here are some screenshots for comparison:

![image](https://user-images.githubusercontent.com/19947566/88887306-6e61df00-d23c-11ea-9aa5-1c03886cd47d.png)

![mobile](https://user-images.githubusercontent.com/19947566/88887254-54280100-d23c-11ea-82ec-b7ced20d4a57.PNG)


Szymongib: @mickmister, @asaadmahmood
Regarding the events for label updated I am not sure how it would suppose to work as I believe the GitHub would emit an event for every label added so if the 3 labels are added to the issue it would result in 3 posted messages. I feel like it could get really spammy quite quickly.
What are your thoughts about it?
asaadmahmood: @Szymongib If they emit 3 events, then yes, we can post 3 events. But I think when a person is setting multiple labels together, it may just emit a single event.
So just something to look at.

The designs high level look good. On mobile, I think we have more spacing between the labels and the new issue text, but if not possible, its fine as is.
Szymongib: @asaadmahmood You are right if multiple labels are assigned at once only one event is emitted.

Sorry, I do not have enough knowledge about the plugin, so correct me if I am wrong. As I understand the functionality would post a message to the subscribed channel when the issue/PR is labeled or assigned?

If that is the case I am not totally convinced if that is a good move, as it may result in a lot of messages and I am not sure if those would bring much value. Maybe making it somehow optional would make more sense?

asaadmahmood: Most definitely, I'm fine if we can make that optional in the subscription.
Szymongib: I have found that there already is the functionality of posting messages when the issue is labeled. By subscribing to feature label:"[LABEL_NAME]" the message is posted to the channel when the issue/PR is labeled with `[LABEL_NAME]`.
I feel like the new functionality would kind of conflict/duplicate already existing feature.

I have reached out to @mickmister to align how to proceed with this PR.

Szymongib: We agreed, that the functionality with labels changes will be implemented as part of a different issue https://github.com/mattermost/mattermost-plugin-github/issues/340

Patch:
diff --git a/server/plugin/command.go b/server/plugin/command.go
index 72fca8e..e75ae76 100644
--- a/server/plugin/command.go
+++ b/server/plugin/command.go
@@ -155,7 +155,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 {
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/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) {