[GH-593] Add feature flag for new pull requests (#712)

* [GH-593] Add feature flag for new pull requests
- Add new_pulls feature flag
- Add check for the flag in postPullRequestEvent
- Add check for conflicting flags and refactor the conflicting checks
to a helper function

* Add feature to README

* new_pulls -> pulls_new

* pulls_new -> pulls_merged

* Additional changes after code review in #612

* errMsg -> testFailureMessage

* Revert unnecessary changes

---------

Co-authored-by: Clint Decker <clint@tendosystems.com>

README.md

@@ -150,7 +150,7 @@ When you’ve tested the plugin and confirmed it’s working, notify your team s
    /github subscriptions add mattermost/mattermost-server --features issues,pulls,issue_comments,label:"Help Wanted"
    ```
   - The following flags are supported:
-     - `--features`: comma-delimited list of one or more of: issues, pulls, pulls_merged, pushes, creates, deletes, issue_creations, issue_comments, pull_reviews, label:"labelname". Defaults to pulls,issues,creates,deletes.
+     - `--features`: comma-delimited list of one or more of: issues, pulls, pulls_merged, pulls_created, pushes, creates, deletes, issue_creations, issue_comments, pull_reviews, label:"labelname". Defaults to pulls,issues,creates,deletes.
      - `--exclude-org-member`: events triggered by organization members will not be delivered. It will be locked to the organization provided in the plugin configuration and it will only work for users whose membership is public. Note that organization members and collaborators are not the same.
      - `--render-style`: notifications will be delivered in the specified style (for example, the body of a pull request will not be displayed). Supported
      values are `collapsed`, `skip-body` or `default` (same as omitting the flag).

server/plugin/command.go

@@ -18,6 +18,7 @@ const (
 	featureIssues        = "issues"
 	featurePulls         = "pulls"
 	featurePullsMerged   = "pulls_merged"
+	featurePullsCreated  = "pulls_created"
 	featurePushes        = "pushes"
 	featureCreates       = "creates"
 	featureDeletes       = "deletes"
@@ -35,6 +36,7 @@ var validFeatures = map[string]bool{
 	featureIssues:        true,
 	featurePulls:         true,
 	featurePullsMerged:   true,
+	featurePullsCreated:  true,
 	featurePushes:        true,
 	featureCreates:       true,
 	featureDeletes:       true,
@@ -86,6 +88,21 @@ func validateFeatures(features []string) (bool, []string) {
 	return valid, invalidFeatures
 }
 
+// checkFeatureConflict returns false when given features
+// cannot be added together along with a list of the conflicting features.
+func checkFeatureConflict(fs []string) (bool, []string) {
+	if SliceContainsString(fs, featureIssues) && SliceContainsString(fs, featureIssueCreation) {
+		return false, []string{featureIssues, featureIssueCreation}
+	}
+	if SliceContainsString(fs, featurePulls) && SliceContainsString(fs, featurePullsMerged) {
+		return false, []string{featurePulls, featurePullsMerged}
+	}
+	if SliceContainsString(fs, featurePulls) && SliceContainsString(fs, featurePullsCreated) {
+		return false, []string{featurePulls, featurePullsCreated}
+	}
+	return true, nil
+}
+
 func (p *Plugin) getCommand(config *Configuration) (*model.Command, error) {
 	iconData, err := command.GetIconData(&p.client.System, "assets/icon-bg.svg")
 	if err != nil {
@@ -383,12 +400,16 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs,
 		}
 
 		fs := subscriptionEvents.ToSlice()
-		if SliceContainsString(fs, featureIssues) && SliceContainsString(fs, featureIssueCreation) {
-			return "Feature list cannot contain both issue and issue_creations"
-		}
-		if SliceContainsString(fs, featurePulls) && SliceContainsString(fs, featurePullsMerged) {
-			return "Feature list cannot contain both pulls and pulls_merged"
+
+		ok, conflictingFs := checkFeatureConflict(fs)
+
+		if !ok {
+			if len(conflictingFs) == 2 {
+				return fmt.Sprintf("Feature list cannot contain both %s and %s", conflictingFs[0], conflictingFs[1])
+			}
+			return fmt.Sprintf("Conflicting feature(s) provided: %s", strings.Join(conflictingFs, ","))
 		}
+
 		ok, ifs := validateFeatures(fs)
 		if !ok {
 			msg := fmt.Sprintf("Invalid feature(s) provided: %s", strings.Join(ifs, ","))
@@ -868,7 +889,7 @@ func getAutocompleteData(config *Configuration) *model.AutocompleteData {
 
 	subscriptionsAdd := model.NewAutocompleteData("add", "[owner/repo] [features] [flags]", "Subscribe the current channel to receive notifications about opened pull requests and issues for an organization or repository. [features] and [flags] are optional arguments")
 	subscriptionsAdd.AddTextArgument("Owner/repo to subscribe to", "[owner/repo]", "")
-	subscriptionsAdd.AddNamedTextArgument("features", "Comma-delimited list of one or more of: issues, pulls, pulls_merged, pushes, creates, deletes, issue_creations, issue_comments, pull_reviews, label:\"<labelname>\". Defaults to pulls,issues,creates,deletes", "", `/[^,-\s]+(,[^,-\s]+)*/`, false)
+	subscriptionsAdd.AddNamedTextArgument("features", "Comma-delimited list of one or more of: issues, pulls, pulls_merged, pulls_created, pushes, creates, deletes, issue_creations, issue_comments, pull_reviews, label:\"<labelname>\". Defaults to pulls,issues,creates,deletes", "", `/[^,-\s]+(,[^,-\s]+)*/`, false)
 
 	if config.GitHubOrg != "" {
 		subscriptionsAdd.AddNamedStaticListArgument("exclude-org-member", "Events triggered by organization members will not be delivered (the organization config should be set, otherwise this flag has not effect)", false, []model.AutocompleteListItem{

server/plugin/command_test.go

@@ -78,8 +78,8 @@ func TestValidateFeatures(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			ok, fs := validateFeatures(tt.args)
 			got := output{ok, fs}
-			errMsg := fmt.Sprintf("validateFeatures() = %v, want %v", got, tt.want)
-			assert.EqualValues(t, tt.want, got, errMsg)
+			testFailureMessage := fmt.Sprintf("validateFeatures() = %v, want %v", got, tt.want)
+			assert.EqualValues(t, tt.want, got, testFailureMessage)
 		})
 	}
 }
@@ -192,8 +192,50 @@ func TestParseCommand(t *testing.T) {
 		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)
+			testFailureMessage := fmt.Sprintf("validateFeatures() = %v, want %v", got, tc.want)
+			assert.EqualValues(t, tc.want, got, testFailureMessage)
+		})
+	}
+}
+
+func TestCheckConflictingFeatures(t *testing.T) {
+	type output struct {
+		valid               bool
+		conflictingFeatures []string
+	}
+	tests := []struct {
+		name string
+		args []string
+		want output
+	}{
+		{
+			name: "no conflicts",
+			args: []string{"creates", "pushes", "issue_comments"},
+			want: output{true, nil},
+		},
+		{
+			name: "conflict with issue and issue creation",
+			args: []string{"pulls", "issues", "issue_creations"},
+			want: output{false, []string{"issues", "issue_creations"}},
+		},
+		{
+			name: "conflict with pulls and pulls created",
+			args: []string{"pulls", "issues", "pulls_created"},
+			want: output{false, []string{"pulls", "pulls_created"}},
+		},
+		{
+			name: "conflict with pulls and pulls merged",
+			args: []string{"pulls", "pushes", "pulls_merged"},
+			want: output{false, []string{"pulls", "pulls_merged"}},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ok, fs := checkFeatureConflict(tt.args)
+			got := output{ok, fs}
+			testFailureMessage := fmt.Sprintf("checkFeatureConflict() = %v, want %v", got, tt.want)
+			assert.EqualValues(t, tt.want, got, testFailureMessage)
 		})
 	}
 }

server/plugin/subscriptions.go

@@ -83,6 +83,10 @@ func (s *Subscription) Pulls() bool {
 	return strings.Contains(s.Features.String(), featurePulls)
 }
 
+func (s *Subscription) PullsCreated() bool {
+	return strings.Contains(s.Features.String(), featurePullsCreated)
+}
+
 func (s *Subscription) PullsMerged() bool {
 	return strings.Contains(s.Features.String(), "pulls_merged")
 }

server/plugin/template.go

@@ -387,6 +387,7 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use
 		"    	* `issues` - includes new and closed issues\n" +
 		"    	* `pulls` - includes new and closed pull requests\n" +
 		"    	* `pulls_merged` - includes merged pull requests only\n" +
+		"    	* `pulls_created` - includes new pull requests only\n" +
 		"    	* `pushes` - includes pushes\n" +
 		"    	* `creates` - includes branch and tag creations\n" +
 		"    	* `deletes` - includes branch and tag deletions\n" +

server/plugin/webhook.go

@@ -376,14 +376,18 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) {
 	}
 
 	for _, sub := range subs {
-		if !sub.Pulls() && !sub.PullsMerged() {
+		if !sub.Pulls() && !sub.PullsMerged() && !sub.PullsCreated() {
 			continue
 		}
 
 		if sub.PullsMerged() && action != actionClosed {
 			continue
 		}
 
+		if sub.PullsCreated() && action != actionOpened {
+			continue
+		}
+
 		if p.excludeConfigOrgMember(event.GetSender(), sub) {
 			continue
 		}