[MM-405]: Fixed the pull review request events not working #768

Kshitij-Katiyar · 2024-04-15T11:54:52Z

Summary

Fixed the issue that the pull request review event not working. Removed the GetComment.GetDiffHunk from the template as it can be very long and not that very useful. Also changed the name for the type of the post.

Ticket Link

Fixes #405

hanzei

Glad to see this bug getting fixed 🚀

hanzei · 2024-04-30T11:03:00Z

server/plugin/webhook.go

@@ -937,7 +937,7 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi

 	post := &model.Post{
 		UserId:  p.BotUserID,
-		Type:    "custom_git_pull_review_comment",
+		Type:    "custom_git_pr_comment",


While we are not using the custom type currently, I think there is value in keeping the type the same for each post.

What is the reason behind changing it?

I kept getting the error that the length of the type should be less than 2, that's why I have reduced it.
@hanzei

hanzei · 2024-04-30T11:03:21Z

server/plugin/template.go

-{{- if eq .GetReview.GetState "APPROVED"}} approved
-{{- else if eq .GetReview.GetState "COMMENTED"}} commented on
-{{- else if eq .GetReview.GetState "CHANGES_REQUESTED"}} requested changes on


Did GitHub document the changes anywhere?

@hanzei I was getting the state in small case and in the code as well they have been changed to lower case in the using toLower. Let me try to find the GitHub documentation for this.

@hanzei Can't find a documentation but here is a screenshot of the webhook payload

mickmister

@Kshitij-Katiyar There's a PR that was submitted that solves this same issue #778. Can you please take a look there and compare any changes that may need to be added here?

mickmister · 2024-05-13T21:26:47Z

server/plugin/template.go

@@ -314,7 +314,6 @@ Assignees: {{range $i, $el := .Assignees -}} {{- if $i}}, {{end}}{{template "use
 	template.Must(masterTemplate.New("newReviewComment").Funcs(funcMap).Parse(`
 {{template "repo" .GetRepo}} New review comment by {{template "user" .GetSender}} on {{template "pullRequest" .GetPullRequest}}:

-{{.GetComment.GetDiffHunk}}


Can you provide a screenshot for what this looks like?

Old UI

New UI

Kshitij-Katiyar · 2024-06-21T07:06:41Z

@Kshitij-Katiyar There's a PR that was submitted that solves this same issue #778. Can you please take a look there and compare any changes that may need to be added here?

Thanks for the PR, added some useful changes from the PR

mickmister · 2024-06-21T14:29:26Z

webapp/package.json

@@ -58,7 +58,7 @@
    "sass": "1.60.0",
    "sass-loader": "10.4.1",
    "style-loader": "1.2.1",
-    "typescript": "4.0.3",
+    "typescript": "4.2.3",


This is adding a lot to the diff of the PR

@mickmister Without these changes the ci-lint will fail. Let me know if you want us to find any other way around to fix the failing lint test other than this.

webapp/src/components/sidebar_right/github_items.tsx

server/plugin/webhook.go

mickmister · 2024-06-21T19:45:15Z

server/plugin/webhook.go

-		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 {


Referencing https://github.com/mattermost/mattermost-plugin-github/pull/768/files#r1649049447 the p.client.Post.CreatePost(post) here mutates the post instead of returning the created post, which has had side effects in this plugin. I think we should instead use p.API.CreatePost(post) to avoid the risk of introducing the issue on unrelated code changes cc @hanzei

Kshitij-Katiyar · 2024-06-25T11:53:22Z

@mickmister Fixed the comments, please look into it.

hanzei · 2024-06-27T11:03:37Z

server/plugin/webhook.go

-		if err = p.client.Post.CreatePost(post); err != nil {
-			p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
+		if _, appErr := p.API.CreatePost(post); appErr != nil {
+			p.client.Log.Warn("Error webhook post", "post", post, "error", appErr.Error())


p.API should not be used. The returned model.AppError makes things way too hard. p.client is they way to go.

@hanzei Please refer to this comment: #768 (comment)

In this case, the p.client.Post method has essentially a bug in it, that has caused issues in this plugin because it mutates the post that is passed in. Is it really a "bug"? Maybe not, but I think we should avoid using it since it has caused issues in production that we then need to fix

2/5 The benefits of using error over AppError in this case are not worth it imo. I think keeping it as p.client.Post will make potential issues come back, just as they did in this PR. This can be changed in a different PR though

@Kshitij-Katiyar I'm thinking we should keep the changes in this PR to a minimum so let's keep this as-is

webapp/src/components/sidebar_right/github_items.tsx

mickmister · 2024-06-27T22:07:35Z

server/plugin/webhook.go

-		if err = p.client.Post.CreatePost(post); err != nil {
-			p.client.Log.Warn("Error webhook post", "post", post, "error", err.Error())
+		if _, appErr := p.API.CreatePost(post); appErr != nil {
+			p.client.Log.Warn("Error webhook post", "post", post, "error", appErr.Error())


@Kshitij-Katiyar I'm thinking we should keep the changes in this PR to a minimum so let's keep this as-is

Kshitij-Katiyar · 2024-06-28T08:12:22Z

@mickmister Fixed the comments, please have a look

AayushChaudhary0001

The above PR has been tested for the following scenarios:

Checked the pull review request events
Tested the notification for pull review request events

The PR was working fine for all the conditions, LGTM. Approved

[MM-405]: Fixed the pull review request events not working

2ac75ae

Kshitij-Katiyar requested review from hanzei and mickmister as code owners April 15, 2024 11:54

Kshitij-Katiyar added 2 commits April 15, 2024 18:03

[MM-405]: Fixed the CI test cases

81c88e3

[MM-405]: Fixed lint errors

f0dd395

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Apr 18, 2024

hanzei requested changes Apr 30, 2024

View reviewed changes

mickmister requested a review from hanzei May 13, 2024 21:25

mickmister mentioned this pull request May 13, 2024

fix message parse error on pull_reviews #778

Closed

mickmister reviewed May 13, 2024

View reviewed changes

[MM-405]: fixed comment and sync with base

5fae2f3

raghavaggarwal2308 requested a review from mickmister June 21, 2024 10:12

mickmister reviewed Jun 21, 2024

View reviewed changes

raghavaggarwal2308 self-requested a review June 23, 2024 17:13

[MM-405]: review fixes related to webhook post

e01f4e1

Kshitij-Katiyar requested a review from mickmister June 25, 2024 11:51

hanzei requested changes Jun 27, 2024

View reviewed changes

raghavaggarwal2308 requested a review from hanzei June 27, 2024 12:03

mickmister reviewed Jun 27, 2024

View reviewed changes

[MM-405]: review fixes

f086e6d

Kshitij-Katiyar requested a review from mickmister June 28, 2024 08:12

mickmister approved these changes Jun 28, 2024

View reviewed changes

raghavaggarwal2308 requested a review from AayushChaudhary0001 June 30, 2024 09:02

hanzei approved these changes Jul 4, 2024

View reviewed changes

AayushChaudhary0001 approved these changes Jul 4, 2024

View reviewed changes

raghavaggarwal2308 merged commit 2402ae6 into master Jul 4, 2024
9 checks passed

raghavaggarwal2308 deleted the MM-405 branch July 4, 2024 12:28

hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MM-405]: Fixed the pull review request events not working #768

[MM-405]: Fixed the pull review request events not working #768

Kshitij-Katiyar commented Apr 15, 2024 •

edited by hanzei

Loading

hanzei left a comment

hanzei Apr 30, 2024

Kshitij-Katiyar May 9, 2024

hanzei Apr 30, 2024

Kshitij-Katiyar May 9, 2024

Kshitij-Katiyar Jun 21, 2024

mickmister left a comment

mickmister May 13, 2024

Kshitij-Katiyar Jun 21, 2024

Kshitij-Katiyar commented Jun 21, 2024

mickmister Jun 21, 2024

Kshitij-Katiyar Jun 25, 2024

mickmister Jun 21, 2024

Kshitij-Katiyar commented Jun 25, 2024

hanzei Jun 27, 2024

raghavaggarwal2308 Jun 27, 2024

mickmister Jun 27, 2024 •

edited

Loading

mickmister Jun 27, 2024

mickmister Jun 27, 2024

Kshitij-Katiyar commented Jun 28, 2024

AayushChaudhary0001 left a comment

[MM-405]: Fixed the pull review request events not working #768

[MM-405]: Fixed the pull review request events not working #768

Conversation

Kshitij-Katiyar commented Apr 15, 2024 • edited by hanzei Loading

Summary

Ticket Link

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kshitij-Katiyar commented Jun 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kshitij-Katiyar commented Jun 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kshitij-Katiyar commented Jun 28, 2024

AayushChaudhary0001 left a comment

Choose a reason for hiding this comment

Kshitij-Katiyar commented Apr 15, 2024 •

edited by hanzei

Loading

mickmister Jun 27, 2024 •

edited

Loading