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

mickmister · 2024-03-21T13:54:01Z

Summary

This PR fixes an issue in the pull request event webhook logic where we reuse the same post struct for multiple calls to CreatePost, resulting in this error:

CreatePost: You cannot update an existing Post.

This PR applies a fix to make it so we instantiate a Post struct in the subscriptions loop, and only when we are going to create a post for that given subscription for the most part.

Ticket Link

Fixes #754

codecov-commenter · 2024-03-21T13:57:38Z

Codecov Report

Attention: Patch coverage is 0% with 68 lines in your changes are missing coverage. Please review.

Project coverage is 16.16%. Comparing base (da4c4df) to head (e6c087f).
Report is 3 commits behind head on master.

❗ Current head e6c087f differs from pull request most recent head 35851e2

Please upload reports for the commit 35851e2 to get more accurate results.

Files	Patch %	Lines
server/plugin/webhook.go	0.00%	68 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #755   +/-   ##
=======================================
  Coverage   16.16%   16.16%           
=======================================
  Files          17       17           
  Lines        6021     6021           
=======================================
  Hits          973      973           
  Misses       5003     5003           
  Partials       45       45

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ayusht2810 · 2024-03-22T13:43:07Z

server/plugin/webhook.go

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


I think we can move this out of the loop

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

1/5 that getting the commentID and assigning it as a prop should stay together as they semantically belong together.

@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.

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

@mickmister Sure, that's fine with me.

server/plugin/webhook.go

hanzei

Should the code in postDeleteEvent also get updated? It seems to suffer from the same issue.

hanzei · 2024-03-25T10:52:43Z

server/plugin/webhook.go

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


Should we move the post creation into a separate method and reuse it?

hanzei · 2024-03-25T10:54:38Z

server/plugin/webhook.go

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


1/5 that getting the commentID and assigning it as a prop should stay together as they semantically belong together.

hanzei · 2024-03-25T10:55:04Z

server/plugin/webhook.go

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

@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

No, the tests are not a requirement.

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

hanzei

Should the code in postDeleteEvent also get updated? It seems to suffer from the same issue.

Kshitij-Katiyar · 2024-05-27T10:00:55Z

@mickmister Can we please fix the lint errors here ?

mickmister · 2024-05-30T16:15:34Z

@AayushChaudhary0001 Can you take a look at this PR when you have the chance? Essentially all webhook events need to be tested

mickmister added 2 commits March 21, 2024 09:34

avoid reusing the same post struct for multiple webhook posts

bf2274d

apply fix to other subscription loops

e6c087f

mickmister added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Mar 21, 2024

mickmister requested a review from ayusht2810 March 21, 2024 13:54

mickmister requested a review from hanzei as a code owner March 21, 2024 13:54

mickmister mentioned this pull request Mar 21, 2024

Added debug logs to check issue in subscription for PR updates #747

Closed

ayusht2810 reviewed Mar 22, 2024

View reviewed changes

hanzei requested changes Mar 25, 2024

View reviewed changes

ayusht2810 added this to the v2.3.0 milestone Apr 5, 2024

hanzei mentioned this pull request Apr 5, 2024

[MM-352] Add feature to publish release create and delete events #762

Merged

3 tasks

hanzei added the Awaiting Submitter Action Blocked on the author label Apr 30, 2024

hanzei mentioned this pull request Apr 30, 2024

[MM-356] Add feature to publish workflow failure events #765

Open

3 tasks

mickmister added 2 commits May 13, 2024 16:23

PR feedback

8d258fc

Merge branch 'master' into fix-create-update-post-issue

00c2fb9

mickmister removed the Awaiting Submitter Action Blocked on the author label May 13, 2024

mickmister requested review from AayushChaudhary0001, hanzei and ayusht2810 May 13, 2024 20:24

Kshitij-Katiyar approved these changes May 14, 2024

View reviewed changes

mickmister added 2 commits May 24, 2024 12:48

Merge branch 'master' into fix-create-update-post-issue

b5895e0

fix merge issue

c34b6c6

cpatulea mentioned this pull request May 29, 2024

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

Open

lint

35851e2

mickmister requested a review from Kshitij-Katiyar May 29, 2024 14:23

ayusht2810 approved these changes May 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mickmister commented Mar 21, 2024

codecov-commenter commented Mar 21, 2024 •

edited

ayusht2810 Mar 22, 2024

mickmister Mar 22, 2024 •

edited

hanzei Mar 25, 2024

mickmister Mar 26, 2024

mickmister May 13, 2024

ayusht2810 May 30, 2024

hanzei left a comment

hanzei Mar 25, 2024

hanzei Mar 25, 2024

hanzei Mar 25, 2024

mickmister Mar 25, 2024 •

edited

hanzei Mar 25, 2024

hanzei left a comment

Kshitij-Katiyar commented May 27, 2024

mickmister commented May 30, 2024

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

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

Are you sure you want to change the base?

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

Conversation

mickmister commented Mar 21, 2024

Summary

Ticket Link

codecov-commenter commented Mar 21, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

mickmister Mar 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hanzei left a comment

Choose a reason for hiding this comment

Kshitij-Katiyar commented May 27, 2024

mickmister commented May 30, 2024

codecov-commenter commented Mar 21, 2024 •

edited

mickmister Mar 22, 2024 •

edited

mickmister Mar 25, 2024 •

edited