From 8cf783fa049dd60d1f1cd30aceb77e63d48f4052 Mon Sep 17 00:00:00 2001 From: kshitij katiyar <90389917+Kshitij-Katiyar@users.noreply.github.com> Date: Tue, 28 Nov 2023 17:02:13 +0530 Subject: [PATCH 1/3] [GH-512,513] Fixed issue #512 and #513 on github plugin 'Updated subscription success messages' (#661) * [MI-2904]:Fixed issue #512 and #513 on github plugin (#25) * [MI-2904]:Fixed issue #512 and #513 on github plugin * [MI-2904]:Fixed review comments * [MI-2904]:Fixed review comments * [MI-2928]:Fixed review comments for issue 512 and 513 on github plugin (#26) * [MM-512,513]:Fixed review comments given by mm team * [MM-512,513]:Updated user error msg * [MM-512,513]:Fixed lint errors * [MM-512,513]: reverted package-lock.json changes * Fixed lint. --------- Co-authored-by: Abhishek Verma --- server/plugin/command.go | 151 ++++++++++++++++++++++++++++----- server/plugin/subscriptions.go | 42 ++++----- 2 files changed, 148 insertions(+), 45 deletions(-) diff --git a/server/plugin/command.go b/server/plugin/command.go index 7bcfff52f..e95a6d5d0 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -43,6 +43,20 @@ var validFeatures = map[string]bool{ featureStars: true, } +type Features string + +func (features Features) String() string { + return string(features) +} + +func (features Features) FormattedString() string { + return "`" + strings.Join(strings.Split(features.String(), ","), "`, `") + "`" +} + +func (features Features) ToSlice() []string { + return strings.Split(string(features), ",") +} + // validateFeatures returns false when 1 or more given features // are invalid along with a list of the invalid features. func validateFeatures(features []string) (bool, []string) { @@ -269,7 +283,7 @@ func (p *Plugin) handleSubscriptionsList(_ *plugin.Context, args *model.CommandA } for _, sub := range subs { subFlags := sub.Flags.String() - txt += fmt.Sprintf("* `%s` - %s", strings.Trim(sub.Repository, "/"), sub.Features) + txt += fmt.Sprintf("* `%s` - %s", strings.Trim(sub.Repository, "/"), sub.Features.String()) if subFlags != "" { txt += fmt.Sprintf(" %s", subFlags) } @@ -279,6 +293,21 @@ func (p *Plugin) handleSubscriptionsList(_ *plugin.Context, args *model.CommandA return txt } +func (p *Plugin) createPost(channelID, userID, message string) error { + post := &model.Post{ + ChannelId: channelID, + UserId: userID, + Message: message, + } + + if _, appErr := p.API.CreatePost(post); appErr != nil { + p.API.LogWarn("Error while creating post", "Post", post, "Error", appErr.Error()) + return appErr + } + + return nil +} + func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClient *github.Client, repo, owner string) (bool, error) { found := false opt := &github.ListOptions{ @@ -321,15 +350,14 @@ func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClien func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string { const errorNoWebhookFound = "\nNo webhook was found for this repository or organization. To create one, enter the following slash command `/github setup webhook`" const errorWebhookToUser = "\nNot able to get the list of webhooks. This feature is not available for subscription to a user." + subscriptionEvents := Features("pulls,issues,creates,deletes") if len(parameters) == 0 { return "Please specify a repository." } - config := p.getConfiguration() + baseURL := config.getBaseURL() - features := "pulls,issues,creates,deletes" flags := SubscriptionFlags{} - if len(parameters) > 1 { flagParams := parameters[1:] @@ -346,7 +374,7 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parsedFlag := parseFlag(flag) if parsedFlag == flagFeatures { - features = value + subscriptionEvents = Features(value) continue } if err := flags.AddFlag(parsedFlag, value); err != nil { @@ -354,7 +382,7 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, } } - fs := strings.Split(features, ",") + fs := subscriptionEvents.ToSlice() if SliceContainsString(fs, featureIssues) && SliceContainsString(fs, featureIssueCreation) { return "Feature list cannot contain both issue and issue_creations" } @@ -373,21 +401,45 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, ctx := context.Background() githubClient := p.githubConnectUser(ctx, userInfo) + user, appErr := p.API.GetUser(args.UserId) + if appErr != nil { + return errors.Wrap(appErr, "failed to get the user").Error() + } + + owner, repo := parseOwnerAndRepo(parameters[0], baseURL) + previousSubscribedEvents, err := p.getSubscribedFeatures(args.ChannelId, owner, repo) + if err != nil { + return errors.Wrap(err, "failed to get the subscribed events").Error() + } + + var previousSubscribedEventMessage string + if previousSubscribedEvents != "" { + previousSubscribedEventMessage = fmt.Sprintf("\nThe previous subscription with: %s was overwritten.\n", previousSubscribedEvents.FormattedString()) + } - owner, repo := parseOwnerAndRepo(parameters[0], config.getBaseURL()) if repo == "" { - if err := p.SubscribeOrg(ctx, githubClient, args.UserId, owner, args.ChannelId, features, flags); err != nil { - return err.Error() + if err = p.SubscribeOrg(ctx, githubClient, args.UserId, owner, args.ChannelId, subscriptionEvents, flags); err != nil { + return errors.Wrap(err, "failed to get the subscribed org").Error() + } + orgLink := baseURL + owner + subscriptionSuccess := fmt.Sprintf("@%v subscribed this channel to [%s](%s) with the following events: %s.", user.Username, owner, orgLink, subscriptionEvents.FormattedString()) + + if previousSubscribedEvents != "" { + subscriptionSuccess += previousSubscribedEventMessage + } + + if err = p.createPost(args.ChannelId, p.BotUserID, subscriptionSuccess); err != nil { + return fmt.Sprintf("%s error creating the public post: %s", subscriptionSuccess, err.Error()) } subOrgMsg := fmt.Sprintf("Successfully subscribed to organization %s.", owner) - found, err := p.checkIfConfiguredWebhookExists(ctx, githubClient, repo, owner) - if err != nil { - if strings.Contains(err.Error(), "404 Not Found") { + found, foundErr := p.checkIfConfiguredWebhookExists(ctx, githubClient, repo, owner) + if foundErr != nil { + if strings.Contains(foundErr.Error(), "404 Not Found") { return errorWebhookToUser } - return errors.Wrap(err, "failed to get the list of webhooks").Error() + return errors.Wrap(foundErr, "failed to get the list of webhooks").Error() } if !found { @@ -400,12 +452,15 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, return "Exclude repository feature is only available to subscriptions of an organization." } - if err := p.Subscribe(ctx, githubClient, args.UserId, owner, repo, args.ChannelId, features, flags); err != nil { - return err.Error() + if err = p.Subscribe(ctx, githubClient, args.UserId, owner, repo, args.ChannelId, subscriptionEvents, flags); err != nil { + return errors.Wrap(err, "failed to create a subscription").Error() } repoLink := config.getBaseURL() + owner + "/" + repo - msg := fmt.Sprintf("Successfully subscribed to [%s](%s).", repo, repoLink) + msg := fmt.Sprintf("@%v subscribed this channel to [%s/%s](%s) with the following events: %s", user.Username, owner, repo, repoLink, subscriptionEvents.FormattedString()) + if previousSubscribedEvents != "" { + msg += previousSubscribedEventMessage + } ghRepo, _, err := githubClient.Repositories.Get(ctx, owner, repo) if err != nil { @@ -414,6 +469,10 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, 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." } + if err = p.createPost(args.ChannelId, p.BotUserID, msg); err != nil { + return fmt.Sprintf("%s\nError creating the public post: %s", msg, appErr.Error()) + } + found, err := p.checkIfConfiguredWebhookExists(ctx, githubClient, repo, owner) if err != nil { if strings.Contains(err.Error(), "404 Not Found") { @@ -429,19 +488,73 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, return msg } +func (p *Plugin) getSubscribedFeatures(channelID, owner, repo string) (Features, error) { + var previousFeatures Features + subs, err := p.GetSubscriptionsByChannel(channelID) + if err != nil { + return previousFeatures, err + } + + for _, sub := range subs { + fullRepoName := repo + if owner != "" { + fullRepoName = owner + "/" + repo + } + + if sub.Repository == fullRepoName { + previousFeatures = sub.Features + return previousFeatures, nil + } + } + + return previousFeatures, nil +} func (p *Plugin) handleUnsubscribe(_ *plugin.Context, args *model.CommandArgs, parameters []string, _ *GitHubUserInfo) string { if len(parameters) == 0 { return "Please specify a repository." } repo := parameters[0] + config := p.getConfiguration() + owner, repo := parseOwnerAndRepo(repo, config.getBaseURL()) + if owner == "" && repo == "" { + return "invalid repository" + } - if err := p.Unsubscribe(args.ChannelId, repo); err != nil { - p.client.Log.Warn("Failed to unsubscribe", "repo", repo, "error", err.Error()) + owner = strings.ToLower(owner) + repo = strings.ToLower(repo) + if err := p.Unsubscribe(args.ChannelId, repo, owner); err != nil { + p.API.LogWarn("Failed to unsubscribe", "repo", repo, "error", err.Error()) return "Encountered an error trying to unsubscribe. Please try again." } - return fmt.Sprintf("Successfully unsubscribed from %s.", repo) + baseURL := config.getBaseURL() + user, appErr := p.API.GetUser(args.UserId) + if appErr != nil { + p.API.LogWarn("Error while fetching user details", "Error", appErr.Error()) + return fmt.Sprintf("error while fetching user details: %s", appErr.Error()) + } + + unsubscribeMessage := "" + if repo == "" { + orgLink := baseURL + owner + unsubscribeMessage = fmt.Sprintf("@%v unsubscribed this channel from [%s](%s)", user.Username, owner, orgLink) + + if err := p.createPost(args.ChannelId, p.BotUserID, unsubscribeMessage); err != nil { + return fmt.Sprintf("%s error creating the public post: %s", unsubscribeMessage, err.Error()) + } + + return "" + } + + repoLink := baseURL + owner + "/" + repo + unsubscribeMessage = fmt.Sprintf("@%v unsubscribed this channel from [%s/%s](%s)", user.Username, owner, repo, repoLink) + + if err := p.createPost(args.ChannelId, p.BotUserID, unsubscribeMessage); err != nil { + return fmt.Sprintf("%s error creating the public post: %s", unsubscribeMessage, err.Error()) + } + + return "" } func (p *Plugin) handleDisconnect(_ *plugin.Context, args *model.CommandArgs, _ []string, _ *GitHubUserInfo) string { diff --git a/server/plugin/subscriptions.go b/server/plugin/subscriptions.go index 2f76d4a28..2ceccb23c 100644 --- a/server/plugin/subscriptions.go +++ b/server/plugin/subscriptions.go @@ -70,7 +70,7 @@ func (s SubscriptionFlags) String() string { type Subscription struct { ChannelID string CreatorID string - Features string + Features Features Flags SubscriptionFlags Repository string } @@ -80,51 +80,51 @@ type Subscriptions struct { } func (s *Subscription) Pulls() bool { - return strings.Contains(s.Features, featurePulls) + return strings.Contains(s.Features.String(), featurePulls) } func (s *Subscription) PullsMerged() bool { - return strings.Contains(s.Features, "pulls_merged") + return strings.Contains(s.Features.String(), "pulls_merged") } func (s *Subscription) IssueCreations() bool { - return strings.Contains(s.Features, "issue_creations") + return strings.Contains(s.Features.String(), "issue_creations") } func (s *Subscription) Issues() bool { - return strings.Contains(s.Features, featureIssues) + return strings.Contains(s.Features.String(), featureIssues) } func (s *Subscription) Pushes() bool { - return strings.Contains(s.Features, "pushes") + return strings.Contains(s.Features.String(), "pushes") } func (s *Subscription) Creates() bool { - return strings.Contains(s.Features, "creates") + return strings.Contains(s.Features.String(), "creates") } func (s *Subscription) Deletes() bool { - return strings.Contains(s.Features, "deletes") + return strings.Contains(s.Features.String(), "deletes") } func (s *Subscription) IssueComments() bool { - return strings.Contains(s.Features, "issue_comments") + return strings.Contains(s.Features.String(), "issue_comments") } func (s *Subscription) PullReviews() bool { - return strings.Contains(s.Features, "pull_reviews") + return strings.Contains(s.Features.String(), "pull_reviews") } func (s *Subscription) Stars() bool { - return strings.Contains(s.Features, featureStars) + return strings.Contains(s.Features.String(), featureStars) } func (s *Subscription) Label() string { - if !strings.Contains(s.Features, "label:") { + if !strings.Contains(s.Features.String(), "label:") { return "" } - labelSplit := strings.Split(s.Features, "\"") + labelSplit := strings.Split(s.Features.String(), "\"") if len(labelSplit) < 3 { return "" } @@ -149,7 +149,7 @@ func (s *Subscription) excludedRepoForSub(repo *github.Repository) bool { return false } -func (p *Plugin) Subscribe(ctx context.Context, githubClient *github.Client, userID, owner, repo, channelID, features string, flags SubscriptionFlags) error { +func (p *Plugin) Subscribe(ctx context.Context, githubClient *github.Client, userID, owner, repo, channelID string, features Features, flags SubscriptionFlags) error { if owner == "" { return errors.Errorf("invalid repository") } @@ -206,7 +206,7 @@ func (p *Plugin) Subscribe(ctx context.Context, githubClient *github.Client, use return nil } -func (p *Plugin) SubscribeOrg(ctx context.Context, githubClient *github.Client, userID, org, channelID, features string, flags SubscriptionFlags) error { +func (p *Plugin) SubscribeOrg(ctx context.Context, githubClient *github.Client, userID, org, channelID string, features Features, flags SubscriptionFlags) error { if org == "" { return errors.New("invalid organization") } @@ -338,17 +338,7 @@ func (p *Plugin) GetSubscribedChannelsForRepository(repo *github.Repository) []* return subsToReturn } -func (p *Plugin) Unsubscribe(channelID string, repo string) error { - config := p.getConfiguration() - - owner, repo := parseOwnerAndRepo(repo, config.getBaseURL()) - if owner == "" && repo == "" { - return errors.New("invalid repository") - } - - owner = strings.ToLower(owner) - repo = strings.ToLower(repo) - +func (p *Plugin) Unsubscribe(channelID, repo, owner string) error { repoWithOwner := fmt.Sprintf("%s/%s", owner, repo) subs, err := p.GetSubscriptions() From f5d22916f51040dba43440ba5fb6750eb65444dd Mon Sep 17 00:00:00 2001 From: Alexander Ziborov <1420883+San4es@users.noreply.github.com> Date: Mon, 4 Dec 2023 15:19:25 +0300 Subject: [PATCH 2/3] [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 --- README.md | 2 +- server/plugin/command.go | 33 ++++++++++++++++++---- server/plugin/command_test.go | 50 +++++++++++++++++++++++++++++++--- server/plugin/subscriptions.go | 4 +++ server/plugin/template.go | 1 + server/plugin/webhook.go | 6 +++- 6 files changed, 84 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 2cd310bdf..031af8d33 100644 --- a/README.md +++ b/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). diff --git a/server/plugin/command.go b/server/plugin/command.go index e95a6d5d0..c4027de29 100644 --- a/server/plugin/command.go +++ b/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:\"\". 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:\"\". 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{ diff --git a/server/plugin/command_test.go b/server/plugin/command_test.go index 8130833a2..5b9970190 100644 --- a/server/plugin/command_test.go +++ b/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) }) } } diff --git a/server/plugin/subscriptions.go b/server/plugin/subscriptions.go index 2ceccb23c..deb1daf26 100644 --- a/server/plugin/subscriptions.go +++ b/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") } diff --git a/server/plugin/template.go b/server/plugin/template.go index cc48216e8..f08edabaa 100644 --- a/server/plugin/template.go +++ b/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" + diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index d9044cbee..ce728259c 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -376,7 +376,7 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) { } for _, sub := range subs { - if !sub.Pulls() && !sub.PullsMerged() { + if !sub.Pulls() && !sub.PullsMerged() && !sub.PullsCreated() { continue } @@ -384,6 +384,10 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) { continue } + if sub.PullsCreated() && action != actionOpened { + continue + } + if p.excludeConfigOrgMember(event.GetSender(), sub) { continue } From d8fb0014f6012dc35814bbd6f5252bc8fa855b64 Mon Sep 17 00:00:00 2001 From: Raghav Aggarwal Date: Tue, 5 Dec 2023 17:52:15 +0530 Subject: [PATCH 3/3] [GH-717] Implement subscription notification for pull request "reopened" event (#718) * [MI-3808] Fix issue: Subscription notification not working for pull request "reopened" event. * [MI-3808] Added test case for new template * [MI-3808] Review fix --- server/plugin/template.go | 4 ++++ server/plugin/template_test.go | 16 ++++++++++++++++ server/plugin/webhook.go | 19 ++++++++++++++++++- 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/server/plugin/template.go b/server/plugin/template.go index f08edabaa..46769ba0f 100644 --- a/server/plugin/template.go +++ b/server/plugin/template.go @@ -224,6 +224,10 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use {{- if .GetPullRequest.GetMerged }} merged {{- else }} closed {{- end }} by {{template "user" .GetSender}}. +`)) + + template.Must(masterTemplate.New("reopenedPR").Funcs(funcMap).Parse(` +{{template "repo" .GetRepo}} Pull request {{template "pullRequest" .GetPullRequest}} was reopened by {{template "user" .GetSender}}. `)) template.Must(masterTemplate.New("pullRequestLabelled").Funcs(funcMap).Parse(` diff --git a/server/plugin/template_test.go b/server/plugin/template_test.go index 6b31a508d..5545c3140 100644 --- a/server/plugin/template_test.go +++ b/server/plugin/template_test.go @@ -393,6 +393,22 @@ func TestClosedPRMessageTemplate(t *testing.T) { }) } +func TestReopenedPRMessageTemplate(t *testing.T) { + t.Run("reopened", func(t *testing.T) { + expected := ` +[\[mattermost-plugin-github\]](https://github.com/mattermost/mattermost-plugin-github) Pull request [#42 Leverage git-get-head](https://github.com/mattermost/mattermost-plugin-github/pull/42) was reopened by [panda](https://github.com/panda). +` + + actual, err := renderTemplate("reopenedPR", &github.PullRequestEvent{ + Repo: &repo, + PullRequest: &pullRequest, + Sender: &user, + }) + require.NoError(t, err) + require.Equal(t, expected, actual) + }) +} + func TestPullRequestLabelledTemplate(t *testing.T) { expected := ` #### Leverage git-get-head diff --git a/server/plugin/webhook.go b/server/plugin/webhook.go index ce728259c..c2225e89b 100644 --- a/server/plugin/webhook.go +++ b/server/plugin/webhook.go @@ -353,9 +353,16 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) { } action := event.GetAction() - if action != actionOpened && action != actionMarkedReadyForReview && action != actionLabeled && action != actionClosed { + switch action { + case actionOpened, + actionReopened, + actionMarkedReadyForReview, + actionLabeled, + actionClosed: + default: return } + pr := event.GetPullRequest() isPRInDraftState := pr.GetDraft() eventLabel := event.GetLabel().GetName() @@ -440,6 +447,16 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) { post.Message = p.sanitizeDescription(newPRMessage) } + if action == actionReopened { + reopenedPRMessage, err := renderTemplate("reopenedPR", event) + if err != nil { + p.client.Log.Warn("Failed to render template", "error", err.Error()) + return + } + + post.Message = p.sanitizeDescription(reopenedPRMessage) + } + if action == actionMarkedReadyForReview { markedReadyToReviewPRMessage, err := renderTemplate("markedReadyToReviewPR", GetEventWithRenderConfig(event, sub)) if err != nil {