Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GH-378]:Fixed issue #378 'Check for webhook while subscribing' #664

Merged
merged 6 commits into from
Nov 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 70 additions & 1 deletion server/plugin/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"strings"
"unicode"

"github.com/google/go-github/v41/github"
"github.com/mattermost/mattermost-plugin-api/experimental/command"
"github.com/mattermost/mattermost-server/v6/model"
"github.com/mattermost/mattermost-server/v6/plugin"
Expand All @@ -25,6 +26,11 @@ const (
featureStars = "stars"
)

const (
ErrorNoWebhookFound = "\nNo webhook was found for this repository or organization. To create one, enter the following slash command `/github setup webhook`"
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
GithubListOptionsPerPageValue = 50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a const for perPage anywhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no there is no constant for this in the code

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1/5 let's use a generic perPage here and in the rest of the code.

)

var validFeatures = map[string]bool{
featureIssueCreation: true,
featureIssues: true,
Expand Down Expand Up @@ -265,6 +271,50 @@ func (p *Plugin) handleSubscriptionsList(_ *plugin.Context, args *model.CommandA
return txt
}

func (p *Plugin) getWebhookListForRepoOrOrg(githubClient *github.Client, repo, owner string, ctx context.Context) (bool, error) {
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
isWebhook := false
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
opt := &github.ListOptions{
PerPage: GithubListOptionsPerPageValue,
}

for {
var githubHooks []*github.Hook
var githubResponse *github.Response
var err error

if repo == "" {
githubHooks, githubResponse, err = githubClient.Organizations.ListHooks(ctx, owner, opt)
} else {
githubHooks, githubResponse, err = githubClient.Repositories.ListHooks(ctx, owner, repo, opt)
}

if err != nil {
p.API.LogWarn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "Error", err.Error())
// Breaking from the loop if the repo or org is not found
if strings.Contains(err.Error(), "404 Not Found") {
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved
isWebhook = true
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking here de-facto means returning, right? Why is the 404 case special?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

404 case is special because we are checking whether there is an org or repo present with the name we put during subscribing if not we don't need any additional messages.

} else {
return isWebhook, err
}
}

for _, hook := range githubHooks {
if strings.Contains(hook.Config["url"].(string), p.getSiteURL()) {
isWebhook = true
break
}
}

if githubResponse.NextPage == 0 {
break
}
opt.Page = githubResponse.NextPage
}

return isWebhook, nil
}

func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string {
if len(parameters) == 0 {
return "Please specify a repository."
Expand Down Expand Up @@ -325,7 +375,17 @@ func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs,
return err.Error()
}

return fmt.Sprintf("Successfully subscribed to organization %s.", owner)
subOrgMsg := fmt.Sprintf("Successfully subscribed to organization %s.", owner)

isWebhook, err := p.getWebhookListForRepoOrOrg(githubClient, repo, owner, ctx)
if err != nil {
return errors.Wrap(err, "failed to get the list of webhooks").Error()
}

if !isWebhook {
subOrgMsg += ErrorNoWebhookFound
}
return subOrgMsg
}

if err := p.Subscribe(ctx, githubClient, args.UserId, owner, repo, args.ChannelId, features, flags); err != nil {
Expand All @@ -342,6 +402,15 @@ 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."
}

isWebhook, err := p.getWebhookListForRepoOrOrg(githubClient, repo, owner, ctx)
if err != nil {
return errors.Wrap(err, "failed to get the list of webhooks").Error()
}

if !isWebhook {
msg += ErrorNoWebhookFound
}

return msg
}

Expand Down
7 changes: 7 additions & 0 deletions server/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -990,3 +990,10 @@ func (p *Plugin) getUsername(mmUserID string) (string, error) {

return "@" + info.GitHubUsername, nil
}

func (p *Plugin) getSiteURL() string {
if p.API.GetConfig().ServiceSettings.SiteURL != nil {
return *p.API.GetConfig().ServiceSettings.SiteURL
}
return ""
}
Kshitij-Katiyar marked this conversation as resolved.
Show resolved Hide resolved