Sync emojis in comments #1

LokeshN · 2022-07-31T07:49:43Z

mickmister

Great work @LokeshN 🎉 I think this feature is going to provide lot of value

I have a few requests on how we can improve the code in regards to:

Handling emoji aliases and skin tones
De-duplicating some code
Supporting PRs and issues instead of just comments

mickmister · 2022-08-01T18:24:40Z

server/plugin/plugin.go

+	p.emojiMap = map[string]string{
+		"+1":       "+1",
+		"-1":       "-1",
+		"laughing": "laugh",
+		"confused": "confused",
+		"heart":    "heart",
+		"tada":     "hooray",
+		"rocket":   "rocket",
+		"eyes":     "eyes",
+	}


There are aliases such as thumbsup, and skin tone-specific versions like :-1_medium_light_skin_tone: that Mattermost users could have used for their reaction.

You can pre-populate a map for the skin tone-specific ones like this:

var baseGithubEmojiMap = map[string]string{ "+1": "+1", "thumbsup": "+1", } var githubEmojiMap = map[string]string{} for systemEmoji := range model.SystemEmojis { for mmBase, ghBase := range baseGithubEmojiMap { if strings.HasPrefix(systemEmoji, mmBase) { githubEmojiMap[systemEmoji] = ghBase } } }

Since emojiMap is a static piece of data, we can instead define it on the module level, rather than as a struct field. You can use this syntax on the module level:

var githubEmojiMap = map[string]string{}

0/5 leaving it in p. and initializing in OnActivate (or NewPlugin) is just as idiomatic, for plugins.

mickmister · 2022-08-01T18:30:10Z

server/plugin/plugin.go

+	if githubEmoji == "" {
+		return
+	}
+	post, error := p.client.Post.GetPost(reaction.PostId)


error is a an existing "type" in Go, so we shouldn't shadow it here

Suggested change

post, error := p.client.Post.GetPost(reaction.PostId)

post, err := p.client.Post.GetPost(reaction.PostId)

mickmister · 2022-08-01T18:35:40Z

server/plugin/plugin.go

+	}
+	post, error := p.client.Post.GetPost(reaction.PostId)
+	if error != nil {
+		p.API.LogError("Invalid post id. Addition of reaction failed.")


Suggested change

p.API.LogError("Invalid post id. Addition of reaction failed.")

p.API.LogError("Error fetching post for reaction add", "error", err.Error())

mickmister · 2022-08-01T18:43:25Z

server/plugin/plugin.go

+
+	repo := post.GetProp("gh_repo")
+	if repo == nil {
+		p.API.LogDebug("Required prop is not present. This post is not created by this plugin, hence ignoring")


Let's remove this log message. This will be the case for almost all reactions added on Mattermost, so it's not something we need to log here.

For further checks for legitimacy, we should also check to see if the post.UserId is the GitHub bot's user id

mickmister · 2022-08-01T18:55:03Z

server/plugin/plugin.go

+		return
+	}
+
+	orgRepo := strings.Split(repo.(string), "/")


I suggest we make the code more compact by doing nil checking and type assertion in the same line. The ok syntax is the "safer" way of doing a type assertion. Otherwise if repo is not a string, the program will panic. That is highly unlikely here but should always be treated as possible.

Suggested change

orgRepo := strings.Split(repo.(string), "/")

repo, ok := post.GetProp("gh_repo").(string)

if !ok || repo == "" {

return

}

orgRepo := strings.Split(repo, "/")

Suggested change

orgRepo := strings.Split(repo.(string), "/")

orgRepo := strings.Split(repo.(string), "/")

if len(orgRepo) != 2 {

// handle error

}

mickmister · 2022-08-01T19:10:49Z

server/plugin/plugin.go

+
+	reactions, _, error := ghClient.Reactions.ListIssueCommentReactions(context.Background(), orgRepo[0], orgRepo[1], int64(id), &github.ListOptions{})
+
+	if error == nil {


We should "return early" instead of having this indented block for the rest of the function

Suggested change

if error == nil {

if err != nil {

p.API.LogDebug(...)

return

mickmister · 2022-08-01T19:13:06Z

server/plugin/plugin.go

+	}
+	post, error := p.client.Post.GetPost(reaction.PostId)
+	if error != nil {
+		p.API.LogError("Invalid post id. Removal of reaction failed.")


Please apply the change requests for ReactionHasBeenAdded to this function as well. Also, there is a decent amount of code duplication among these two functions. Can you please try extracting some code out to make the functions a bit cleaner?

It may be better to keep it as-is, since there are different "error" and "harmless skip" cases here, and it could be complicated to handle the different "log error" versus "just return" cases between the caller and callee.

mickmister · 2022-08-01T19:23:29Z

server/plugin/plugin.go

+
+	if error == nil {
+		for _, reactionObj := range reactions {
+			if info.UserID == reaction.UserId && p.emojiMap[reaction.EmojiName] == reactionObj.GetContent() {


p.emojiMap[reaction.EmojiName] should ideally already be computed as a local variable to avoid verbosity on this long line

emoji := p.emojiMap[reaction.EmojiName]

mickmister · 2022-08-01T19:26:29Z

server/plugin/plugin.go

+				// delete this reaction
+				_, error = ghClient.Reactions.DeleteIssueCommentReaction(context.Background(), orgRepo[0], orgRepo[1], int64(id), reactionObj.GetID())
+				if error != nil {
+					p.API.LogError("Error found while removing issue comment reaction")


Suggested change

p.API.LogError("Error found while removing issue comment reaction")

p.API.LogDebug("Error removing issue comment reaction", "error", err.Error())

mickmister · 2022-08-01T19:28:10Z

server/plugin/webhook.go

+	repoName := repo.GetFullName()
+	repoName = strings.ToLower(repoName)
+	commentID := event.GetComment().GetID()
+
+	post.AddProp("gh_repo", repoName)
+	post.AddProp("gh_object_id", commentID)


This should be applied to any webhook event associated with a PR or issue, not just the "issue comment" events. For example, if a new pull request is created, I should be able to react to the post in Mattermost and have my reaction on the PR itself in GitHub. Probably best to try to de-duplicate this code addition as well.

I'm thinking we will need to add an gh_object_type or gh_object_name post prop in order know what github API call to make, e.g. DeleteIssueCommentReaction vs DeleteIssueReaction

mickmister · 2022-08-01T19:54:14Z

@LokeshN I have one more request to add. Can you please write unit tests for the ReactionHasBeenAdded and ReactionHasBeenRemoved functions? Thank you!

levb

Mostly 👍 -ing @mickmister's comments

levb · 2022-08-02T14:52:41Z

server/plugin/plugin.go

+	p.emojiMap = map[string]string{
+		"+1":       "+1",
+		"-1":       "-1",
+		"laughing": "laugh",
+		"confused": "confused",
+		"heart":    "heart",
+		"tada":     "hooray",
+		"rocket":   "rocket",
+		"eyes":     "eyes",
+	}


0/5 leaving it in p. and initializing in OnActivate (or NewPlugin) is just as idiomatic, for plugins.

levb · 2022-08-04T13:13:27Z

server/plugin/webhook.go

@@ -702,6 +702,13 @@ func (p *Plugin) postIssueCommentEvent(event *github.IssueCommentEvent) {
 		Type:   "custom_git_comment",
 	}

+	repoName := repo.GetFullName()


0/5 collapse into repoName := strings.ToLower(repo.GetFullName()) for readability.

levb · 2022-08-04T13:15:42Z

server/plugin/plugin.go

+	info, appErr := p.getGitHubUserInfo(reaction.UserId)
+	if appErr != nil {
+		if appErr.ID != apiErrorIDNotConnected {
+			p.API.LogError("Error in getting user info", "error", appErr.Message)
+		}
+	}


👍 important

levb · 2022-08-04T13:16:17Z

server/plugin/plugin.go

+	if appErr != nil {
+		if appErr.ID != apiErrorIDNotConnected {
+			p.API.LogError("Error in getting user info", "error", appErr.Message)
+		}


LokeshN · 2022-08-08T14:44:38Z

Thanks for the comments, I will take this up separately in a new PR.

LokeshN added 5 commits July 31, 2022 11:42

test

80678a1

Feat for adding reactions to comments

e4375c9

Fix checkstyle

cc46344

revert readme

0d3312a

revert

e0151c3

mickmister suggested changes Aug 1, 2022

View reviewed changes

levb suggested changes Aug 4, 2022

View reviewed changes

LokeshN closed this Aug 8, 2022

mickmister mentioned this pull request Nov 10, 2022

Feature: Synchronize emoji reactions on posts generated by the plugin mattermost/mattermost-plugin-github#500

Closed

Kshitij-Katiyar mentioned this pull request Jan 31, 2023

[GH-500]: Fixed github issue "Emoji synchronisation for PR/issue on github" mattermost/mattermost-plugin-github#632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync emojis in comments #1

Sync emojis in comments #1

LokeshN commented Jul 31, 2022 •

edited

Loading

mickmister left a comment

mickmister Aug 1, 2022

mickmister Aug 1, 2022

levb Aug 2, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister Aug 1, 2022

mickmister commented Aug 1, 2022

levb left a comment

levb Aug 2, 2022

levb Aug 4, 2022

levb Aug 4, 2022

levb Aug 4, 2022

LokeshN commented Aug 8, 2022

	post, error := p.client.Post.GetPost(reaction.PostId)
	post, err := p.client.Post.GetPost(reaction.PostId)

	p.API.LogError("Invalid post id. Addition of reaction failed.")
	p.API.LogError("Error fetching post for reaction add", "error", err.Error())

-	orgRepo := strings.Split(repo.(string), "/")
+	repo, ok := post.GetProp("gh_repo").(string)
+	if !ok || repo == "" {
+		return
+	}
+	orgRepo := strings.Split(repo, "/")


		reactions, _, error := ghClient.Reactions.ListIssueCommentReactions(context.Background(), orgRepo[0], orgRepo[1], int64(id), &github.ListOptions{})

		if error == nil {

-	if error == nil {
+	if err != nil {
+	    p.API.LogDebug(...)
+	    return

	p.API.LogError("Error found while removing issue comment reaction")
	p.API.LogDebug("Error removing issue comment reaction", "error", err.Error())

Sync emojis in comments #1

Sync emojis in comments #1

Conversation

LokeshN commented Jul 31, 2022 • edited Loading

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

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

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 commented Aug 1, 2022

levb 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

LokeshN commented Aug 8, 2022

LokeshN commented Jul 31, 2022 •

edited

Loading