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.

Sign up for GitHub

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

Jump to bottom

Fix PR subscription error "You cannot update an existing Post" #755

Merged
merged 7 commits into from
Jun 4, 2024
Merged

Fix PR subscription error "You cannot update an existing Post" #755

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bf2274d
avoid reusing the same post struct for multiple webhook posts
mickmister Mar 21, 2024
e6c087f
apply fix to other subscription loops
mickmister Mar 21, 2024
8d258fc
PR feedback
mickmister May 13, 2024
00c2fb9
Merge branch 'master' into fix-create-update-post-issue
mickmister May 13, 2024
b5895e0
Merge branch 'master' into fix-create-update-post-issue
mickmister May 24, 2024
c34b6c6
fix merge issue
mickmister May 24, 2024
35851e2
lint
mickmister May 29, 2024
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
126 changes: 45 additions & 81 deletions server/plugin/webhook.go
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

I would love to see tests for these changes. One can only dream...

Copy link
Member Author

@mickmister mickmister Mar 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@hanzei Sure I think I can do that. May take a bit to get to this. This is currently affecting community, and maybe other servers without them realizing it. @ayusht2810 Are you able to look into this (writing unit tests)?

@hanzei Is this a requirement to merge? Also are you thinking this could replace the manual QA for this? We also need to itemize the changes so QA knows what to look into

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the tests are not a requirement.

The lack of tests is one of the reasons that heavy manual QA testing is required.

Original file line number Diff line number Diff line change
Expand Up @@ -383,11 +383,6 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) {
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_pr",
}

for _, sub := range subs {
if !sub.Pulls() && !sub.PullsMerged() && !sub.PullsCreated() {
continue
Expand All @@ -414,17 +409,19 @@ func (p *Plugin) postPullRequestEvent(event *github.PullRequestEvent) {
}
}

if !contained && label != "" {
continue
}

repoName := strings.ToLower(repo.GetFullName())
prNumber := event.GetPullRequest().Number

post := p.makeBotPost("", "custom_git_pr")

post.AddProp(postPropGithubRepo, repoName)
post.AddProp(postPropGithubObjectID, prNumber)
post.AddProp(postPropGithubObjectType, githubObjectTypeIssue)

if !contained && label != "" {
continue
}

if action == actionLabeled {
if label != "" && label == eventLabel {
pullRequestLabelledMessage, err := renderTemplate("pullRequestLabelled", event)
Expand Down Expand Up @@ -509,12 +506,6 @@ func (p *Plugin) handlePRDescriptionMentionNotification(event *github.PullReques
return
}

post := &model.Post{
UserId: p.BotUserID,
Message: message,
Type: "custom_git_mention",
}

for _, username := range mentionedUsernames {
// Don't notify user of their own comment
if username == event.GetSender().GetLogin() {
Expand All @@ -540,6 +531,7 @@ func (p *Plugin) handlePRDescriptionMentionNotification(event *github.PullReques
continue
}

post := p.makeBotPost(message, "custom_git_mention")
post.ChannelId = channel.Id

if err = p.client.Post.CreatePost(post); err != nil {
Expand Down Expand Up @@ -610,11 +602,7 @@ func (p *Plugin) postIssueEvent(event *github.IssuesEvent) {
}
renderedMessage = p.sanitizeDescription(renderedMessage)

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_issue",
Message: renderedMessage,
}
post := p.makeBotPost(renderedMessage, "custom_git_issue")

repoName := strings.ToLower(repo.GetFullName())
issueNumber := issue.Number
Expand Down Expand Up @@ -670,12 +658,6 @@ func (p *Plugin) postPushEvent(event *github.PushEvent) {
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_push",
Message: pushedCommitsMessage,
}

for _, sub := range subs {
if !sub.Pushes() {
continue
Expand All @@ -685,6 +667,8 @@ func (p *Plugin) postPushEvent(event *github.PushEvent) {
continue
}

post := p.makeBotPost(pushedCommitsMessage, "custom_git_push")

post.ChannelId = sub.ChannelID
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
Expand All @@ -711,12 +695,6 @@ func (p *Plugin) postCreateEvent(event *github.CreateEvent) {
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_create",
Message: newCreateMessage,
}

for _, sub := range subs {
if !sub.Creates() {
continue
Expand All @@ -726,6 +704,8 @@ func (p *Plugin) postCreateEvent(event *github.CreateEvent) {
continue
}

post := p.makeBotPost(newCreateMessage, "custom_git_create")

post.ChannelId = sub.ChannelID
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
Expand Down Expand Up @@ -754,12 +734,6 @@ func (p *Plugin) postDeleteEvent(event *github.DeleteEvent) {
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_delete",
Message: newDeleteMessage,
}

for _, sub := range subs {
if !sub.Deletes() {
continue
Expand All @@ -769,6 +743,7 @@ func (p *Plugin) postDeleteEvent(event *github.DeleteEvent) {
continue
}

post := p.makeBotPost(newDeleteMessage, "custom_git_delete")
post.ChannelId = sub.ChannelID
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
Expand All @@ -795,18 +770,6 @@ func (p *Plugin) postIssueCommentEvent(event *github.IssueCommentEvent) {
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_comment",
}

repoName := strings.ToLower(repo.GetFullName())
commentID := event.GetComment().GetID()

post.AddProp(postPropGithubRepo, repoName)
post.AddProp(postPropGithubObjectID, commentID)
post.AddProp(postPropGithubObjectType, githubObjectTypeIssueComment)

labels := make([]string, len(event.GetIssue().Labels))
for i, v := range event.GetIssue().Labels {
labels[i] = v.GetName()
Expand Down Expand Up @@ -834,6 +797,15 @@ func (p *Plugin) postIssueCommentEvent(event *github.IssueCommentEvent) {
continue
}

post := p.makeBotPost("", "custom_git_comment")

repoName := strings.ToLower(repo.GetFullName())
commentID := event.GetComment().GetID()
Comment on lines +802 to +803
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can move this out of the loop

Copy link
Member Author

@mickmister mickmister Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure 👍 I was mainly keeping it close to the post for readability, but you're right it doesn't need to be in the loop

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 that getting the commentID and assigning it as a prop should stay together as they semantically belong together.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ayusht2810 Note that these methods being called are not performing network requests. They are essentially helper methods that are accessing fields on the struct for us. @hanzei Sure we can keep it as is. strings.ToLower is happening each time but that's the only concern I could potentially have. The repo name is always going to be short.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ayusht2810 I chose to keep this code as is. Let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

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

@mickmister Sure, that's fine with me.


post.AddProp(postPropGithubRepo, repoName)
post.AddProp(postPropGithubObjectID, commentID)
post.AddProp(postPropGithubObjectType, githubObjectTypeIssueComment)

if event.GetAction() == actionCreated {
post.Message = message
}
Expand Down Expand Up @@ -886,12 +858,6 @@ func (p *Plugin) postPullRequestReviewEvent(event *github.PullRequestReviewEvent
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_pull_review",
Message: newReviewMessage,
}

labels := make([]string, len(event.GetPullRequest().Labels))
for i, v := range event.GetPullRequest().Labels {
labels[i] = v.GetName()
Expand Down Expand Up @@ -919,6 +885,8 @@ func (p *Plugin) postPullRequestReviewEvent(event *github.PullRequestReviewEvent
continue
}

post := p.makeBotPost(newReviewMessage, "custom_git_pull_review")

post.ChannelId = sub.ChannelID
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
Expand All @@ -940,19 +908,6 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_pull_review_comment",
Message: newReviewMessage,
}

repoName := strings.ToLower(repo.GetFullName())
commentID := event.GetComment().GetID()

post.AddProp(postPropGithubRepo, repoName)
post.AddProp(postPropGithubObjectID, commentID)
post.AddProp(postPropGithubObjectType, githubObjectTypePRReviewComment)

labels := make([]string, len(event.GetPullRequest().Labels))
for i, v := range event.GetPullRequest().Labels {
labels[i] = v.GetName()
Expand Down Expand Up @@ -980,6 +935,15 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi
continue
}

post := p.makeBotPost(newReviewMessage, "custom_git_pull_review_comment")

repoName := strings.ToLower(repo.GetFullName())
commentID := event.GetComment().GetID()
ayusht2810 marked this conversation as resolved.
Show resolved Hide resolved

post.AddProp(postPropGithubRepo, repoName)
post.AddProp(postPropGithubObjectID, commentID)
post.AddProp(postPropGithubObjectType, githubObjectTypePRReviewComment)

post.ChannelId = sub.ChannelID
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
Expand Down Expand Up @@ -1008,12 +972,6 @@ func (p *Plugin) handleCommentMentionNotification(event *github.IssueCommentEven
return
}

post := &model.Post{
UserId: p.BotUserID,
Message: message,
Type: "custom_git_mention",
}

assignees := event.GetIssue().Assignees

for _, username := range mentionedUsernames {
Expand Down Expand Up @@ -1054,6 +1012,8 @@ func (p *Plugin) handleCommentMentionNotification(event *github.IssueCommentEven
continue
}

post := p.makeBotPost(message, "custom_git_mention")

post.ChannelId = channel.Id
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error creating mention post", "error", err.Error())
Expand Down Expand Up @@ -1361,12 +1321,6 @@ func (p *Plugin) postStarEvent(event *github.StarEvent) {
return
}

post := &model.Post{
UserId: p.BotUserID,
Type: "custom_git_star",
Message: newStarMessage,
}

for _, sub := range subs {
if !sub.Stars() {
continue
Expand All @@ -1376,13 +1330,23 @@ func (p *Plugin) postStarEvent(event *github.StarEvent) {
continue
}

post := p.makeBotPost(newStarMessage, "custom_git_star")

post.ChannelId = sub.ChannelID
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
}
}
}

func (p *Plugin) makeBotPost(message, postType string) *model.Post {
return &model.Post{
UserId: p.BotUserID,
Type: postType,
Message: message,
}
}

func (p *Plugin) postReleaseEvent(event *github.ReleaseEvent) {
if event.GetAction() != actionCreated && event.GetAction() != actionDeleted {
return
Expand Down
Loading