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-500]: Fixed github issue "Emoji synchronisation for PR/issue on github" #632

Merged
merged 4 commits into from
Feb 23, 2023

Conversation

Kshitij-Katiyar
Copy link
Contributor

@Kshitij-Katiyar Kshitij-Katiyar commented Jan 17, 2023

Summary

  • When the GitHub bot creates a post, it's always related to some specific issue/PR or comment. This PR has added the functionality that if a user reacts to the post in Mattermost with one of the 8 emojis supported for reactions on GitHub, the plugin will proxy this reaction to the appropriate PR/comment on GitHub. Likewise, if the user removes their emoji reaction in Mattermost, it will remove it in GitHub. The supported event notifications are:-

  • Issue/PR creations

  • Issue/PR comments

  • PR review comments

  • This PR is an extension of PR #575

  • Issue #500

…#17)

* test

* Feat for adding reactions to comments

* Fix checkstyle

* revert readme

* revert

* [MI-2597]:Fixed github issue mattermost#500 for emoji synchronisation

* [MI-2597]:Fixed review comments

* [MI-2597]:Fixed review comments

* [MI-2597]:Fixed review comments

Co-authored-by: LokeshN <[email protected]>
@mattermost-build
Copy link
Contributor

Hello @Kshitij-Katiyar,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@Kshitij-Katiyar Kshitij-Katiyar changed the title [GH-500]: Fixed github issue mattermost#500 for emoji synchronisation [GH-500]: Fixed github issue "Emoji synchronisation for PR/issue on github" Jan 17, 2023
@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 27, 2023
server/plugin/plugin.go Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Show resolved Hide resolved
server/plugin/plugin.go Show resolved Hide resolved
@javaguirre
Copy link

Thank you for the work @Kshitij-Katiyar ! Let me know what you think.

Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Nice work 👍

server/plugin/plugin.go Outdated Show resolved Hide resolved
server/plugin/plugin.go Outdated Show resolved Hide resolved
Comment on lines 278 to 282
post, err := p.client.Post.GetPost(reaction.PostId)
if err != nil {
p.API.LogDebug("Error fetching post for reaction", "error", err.Error())
return orgRepo, id, objectType, false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is costly. For most of the reactions posted, an RPC request is made to fetch the post. It's a bummer ReactionHasBeenAdded doesn't provide the post.

Anyway, I don't see a way to get around the RPC call without changing the hook.

server/plugin/plugin.go Outdated Show resolved Hide resolved
…n github plugin (#19)

* [MI-2688]:Fixed review comments by mm team for issue mattermost#500 on github plugin

* [MI-2688]:Fixed review comments

* [MI-2688]:Fixed review comments
@hanzei hanzei removed the 2: Dev Review Requires review by a core committer label Feb 6, 2023
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@hanzei
Copy link
Contributor

hanzei commented Feb 20, 2023

@DHaussermann Gentle reminder to review the PR

@hanzei hanzei added this to the v2.2.0 milestone Feb 20, 2023
@hanzei
Copy link
Contributor

hanzei commented Feb 21, 2023

/update-branch

@mattermost-build
Copy link
Contributor

We don't have permissions to update this PR, please contact the submitter to apply the update.

@Kshitij-Katiyar
Copy link
Contributor Author

@hanzei synced with master

@DHaussermann
Copy link

DHaussermann commented Feb 23, 2023

Thanks @Kshitij-Katiyar
For the supported reactions:

  • User reactions in MM will add in GitHub
  • User removing from MM will remove the reaction
  • Users not mapped to a GitHub account have no affect (as expected as there would be no user to attribute the reaction to)
  • User reacting in GitHub has no effect on MM post (as expected)
  • Tested on Issue/PR creations and Issue/PR comments

Note I'm having an issue making the pull_reviews feature work but it's just something that's not clear about the subscriptions in general. I'm fine confirming this post merge.

LGTM!

@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester label Feb 23, 2023
@DHaussermann DHaussermann added the 4: Reviews Complete All reviewers have approved the pull request label Feb 23, 2023
@codecov-commenter
Copy link

Codecov Report

Base: 15.63% // Head: 15.53% // Decreases project coverage by -0.11% ⚠️

Coverage data is based on head (f40c7bb) compared to base (9f37670).
Patch coverage: 12.28% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #632      +/-   ##
==========================================
- Coverage   15.63%   15.53%   -0.11%     
==========================================
  Files          15       15              
  Lines        5243     5414     +171     
==========================================
+ Hits          820      841      +21     
- Misses       4380     4530     +150     
  Partials       43       43              
Impacted Files Coverage Δ
server/plugin/webhook.go 0.83% <0.00%> (-0.03%) ⬇️
server/plugin/plugin.go 5.98% <14.68%> (+2.29%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hanzei hanzei merged commit 8d28b2e into mattermost:master Feb 23, 2023
@mickmister mickmister mentioned this pull request Mar 20, 2023
@mickmister
Copy link
Contributor

It looks like we don't take into account skin-tone emojis here. Maybe a good HW ticket to support this. More info here LokeshN#1 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Synchronize emoji reactions on posts generated by the plugin
7 participants