[GH-262] Merge request approval subscription #307

MatthewDorner · 2022-06-24T18:17:25Z

Summary

Add DM and Channel notifications for Merge Request approved and unapproved webhook actions.

Ticket Link

Fixes #282

mattermod · 2022-06-24T18:17:30Z

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.

Removed incorrect commandHelp text.

Clean up comment formatting.

mattermod · 2022-07-06T01:01:28Z

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!

spirosoik

a few changes

spirosoik · 2022-07-06T13:16:24Z

server/webhook/merge_request.go

+		case actionApproved:
+			message = fmt.Sprintf("[%s](%s) approved your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
+		case actionUnapproved:
+			message = fmt.Sprintf("[%s](%s) unapproved your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)


I would suggest a bit better wording like asked for changes. Something like that

spirosoik · 2022-07-06T13:16:38Z

server/webhook/merge_request.go

+	case actionApproved:
+		message = fmt.Sprintf("[%s] Merge request [!%v %s](%s) was approved by [%s](%s)", repo.PathWithNamespace, pr.IID, pr.Title, pr.URL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername))
+	case actionUnapproved:
+		message = fmt.Sprintf("[%s] Merge request [!%v %s](%s) was unapproved by [%s](%s)", repo.PathWithNamespace, pr.IID, pr.Title, pr.URL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername))


ditto. something like needs changes

…com/matthewdorner/mattermost-plugin-gitlab into merge-request-approval-subscription

MatthewDorner · 2022-07-09T15:28:21Z

Not sure why the CircleCI checks are timing out.

mattermod · 2022-07-20T01:01:27Z

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!

MatthewDorner · 2022-07-22T18:27:07Z

Changed the messages, anything else to do?

mickmister · 2022-07-22T21:13:38Z

server/webhook/merge_request.go

+		case actionApproved:
+			message = fmt.Sprintf("[%s](%s) approved your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
+		case actionUnapproved:
+			message = fmt.Sprintf("[%s](%s) requested changes to your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)


GitHub's convention is to use # here, not sure about GitLab. I see other references to ! here though

Suggested change

message = fmt.Sprintf("[%s](%s) requested changes to your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)

message = fmt.Sprintf("[%s](%s) requested changes to your merge request [%s#%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)

GitLab does use ! (https://docs.gitlab.com/ee/user/markdown.html) but the MR references in the Channel messages are formatted differently from the DM ones. Looks like the DM ones are doing it the correct way.

I can quickly fix it here.

In this screenshot, the second set of messages would be after my proposed changes where you can see the DM and Channel messages now format the MR reference in the same way. But I'd go even further and suggest that the sentence structure should also be the same instead of one being "X approved MR" and the other "MR was approved by". There's a bunch of little stuff like that I'm seeing but probably should be separate PRs.

@mickmister should I also mention the reviewer in replies to review threads like this one? It's been a while since I used GitHub...

Anyway I don't think I should commit the screenshotted change as part of this PR. I'd rather review all the different types of webhook message templates and propose fixes in a later PR, but I do think there are multiple things that should be fixed.

But the use of ! itself is correct for GitLab MRs.

mickmister · 2022-07-22T21:15:01Z

server/webhook/merge_request.go

@@ -83,6 +87,10 @@ func (w *webhook) handleChannelMergeRequest(ctx context.Context, event *gitlab.M
 		message = fmt.Sprintf("[%s] Merge request [!%v %s](%s) was closed by [%s](%s)", repo.PathWithNamespace, pr.IID, pr.Title, pr.URL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername))
 	case actionReopen:
 		message = fmt.Sprintf("[%s] Merge request [!%v %s](%s) was reopened by [%s](%s)", repo.PathWithNamespace, pr.IID, pr.Title, pr.URL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername))
+	case actionApproved:
+		message = fmt.Sprintf("[%s] Merge request [!%v %s](%s) was approved by [%s](%s)", repo.PathWithNamespace, pr.IID, pr.Title, pr.URL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername))


(Not part of this PR), but ideally we use the sender's connected Mattermost account to display their Mattermost username instead of their GitLab username

Couldn't the sender be somebody who hasn't connected their account?

Yes, but we should do our best to use the connected account. If they aren't connected, then we should fall back to the default functionality

mickmister · 2022-07-22T21:32:46Z

Changed the messages, anything else to do?

@MatthewDorner Make sure you either mention someone when asking a question or re-request review from the person if that's the context of the question

codecov · 2022-07-22T21:48:46Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.15 🎉

Comparison is base (21beec5) 32.27% compared to head (6bc336c) 32.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
+ Coverage   32.27%   32.43%   +0.15%     
==========================================
  Files          21       21              
  Lines        3433     3441       +8     
==========================================
+ Hits         1108     1116       +8     
  Misses       2215     2215              
  Partials      110      110

Impacted Files	Coverage Δ
server/subscription/subscription.go	`95.45% <ø> (ø)`
server/webhook/merge_request.go	`87.75% <100.00%> (+1.08%)`	⬆️
server/webhook/webhook.go	`53.26% <100.00%> (ø)`

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

spirosoik

thanks for the contribution

mattermod · 2022-09-05T01:01:31Z

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!

Makefile

Revert change to remove --verbose flag in Makefile

hanzei · 2023-02-22T21:24:58Z

/update-branch

hanzei · 2023-02-22T21:27:07Z

@DHaussermann Gentle reminder to review this PR

mattermost-build · 2023-03-07T01:02:00Z

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!

DHaussermann

Tested and passed

Created a subscription that includes merge_request_comments
Confirmed that both Approval and Revoke Approval now send webhook events for this feature.
A DM is sent to the user

LGTM!

Thanks @MatthewDorner for implementing this! 🎉
Sorry for the delay in getting this approved.

hanzei · 2023-03-10T20:15:48Z

@MatthewDorner Would you please merge master into your branch?

…roval-subscription - Fixed merge conflicts

MatthewDorner · 2023-03-10T21:05:58Z

@hanzei done

mattermost-build · 2023-03-22T01:01:34Z

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 · 2023-03-22T06:10:56Z

Thank you for the contribution @MatthewDorner 👍

MatthewDorner added 4 commits June 4, 2022 21:24

Notify for Merge Request approval/unapproval

4337f2a

Added to commandHelp

7d514a5

Remove subscription feature

f9b4be1

Add channel messages

c262803

MatthewDorner requested a review from mickmister as a code owner June 24, 2022 18:17

MatthewDorner added 2 commits June 24, 2022 13:18

Update command.go

4bdac0e

Removed incorrect commandHelp text.

Update subscription.go

27bb466

Clean up comment formatting.

mattermod added the Lifecycle/1:stale label Jul 6, 2022

spirosoik requested changes Jul 6, 2022

View reviewed changes

spirosoik added 2: Dev Review Requires review by a core committer and removed Lifecycle/1:stale labels Jul 6, 2022

MatthewDorner added 2 commits July 9, 2022 09:56

Reword message templates

286ec2f

Merge branch 'merge-request-approval-subscription' of https://github.…

9795c7b

…com/matthewdorner/mattermost-plugin-gitlab into merge-request-approval-subscription

mattermod added the Lifecycle/1:stale label Jul 20, 2022

mickmister approved these changes Jul 22, 2022

View reviewed changes

mickmister requested a review from spirosoik July 22, 2022 21:16

npm install --verbose

30c35d2

spirosoik approved these changes Aug 24, 2022

View reviewed changes

mickmister added 3: QA Review Requires review by a QA tester and removed 2: Dev Review Requires review by a core committer Lifecycle/1:stale labels Aug 25, 2022

mickmister requested a review from DHaussermann August 25, 2022 15:08

mattermod added the Lifecycle/1:stale label Sep 5, 2022

hanzei added this to the v1.6.0 milestone Sep 19, 2022

hanzei reviewed Nov 29, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

hanzei modified the milestones: v1.6.0, v1.7.0 Nov 29, 2022

Update Makefile

1371a64

Revert change to remove --verbose flag in Makefile

hanzei removed the Lifecycle/1:stale label Feb 22, 2023

mattermost-build added the Lifecycle/1:stale label Mar 7, 2023

DHaussermann approved these changes Mar 8, 2023

View reviewed changes

DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed Lifecycle/1:stale 3: QA Review Requires review by a QA tester labels Mar 8, 2023

hanzei added the Awaiting Submitter Action Blocked on the author label Mar 10, 2023

Merge remote-tracking branch 'upstream/master' into merge-request-app…

6bc336c

…roval-subscription - Fixed merge conflicts

hanzei removed the Awaiting Submitter Action Blocked on the author label Mar 11, 2023

mattermost-build added the Lifecycle/1:stale label Mar 22, 2023

hanzei removed the Lifecycle/1:stale label Mar 22, 2023

hanzei merged commit ad1d267 into mattermost:master Mar 22, 2023

mickmister mentioned this pull request May 25, 2023

Release 1.7.0 #380

Merged

mickmister mentioned this pull request Aug 22, 2023

Add v1.7.0 of mattermost-plugin-gitlab to the Marketplace mattermost/mattermost-marketplace#380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GH-262] Merge request approval subscription #307

[GH-262] Merge request approval subscription #307

MatthewDorner commented Jun 24, 2022

mattermod commented Jun 24, 2022

mattermod commented Jul 6, 2022

spirosoik left a comment

spirosoik Jul 6, 2022

spirosoik Jul 6, 2022

MatthewDorner commented Jul 9, 2022

mattermod commented Jul 20, 2022

MatthewDorner commented Jul 22, 2022

mickmister Jul 22, 2022

MatthewDorner Jul 24, 2022

MatthewDorner Jul 24, 2022

MatthewDorner Jul 25, 2022

mickmister Jul 22, 2022

MatthewDorner Jul 24, 2022

mickmister Aug 25, 2022

mickmister commented Jul 22, 2022

codecov bot commented Jul 22, 2022 •

edited

spirosoik left a comment

mattermod commented Sep 5, 2022

hanzei commented Feb 22, 2023

hanzei commented Feb 22, 2023

mattermost-build commented Mar 7, 2023

DHaussermann left a comment •

edited

hanzei commented Mar 10, 2023

MatthewDorner commented Mar 10, 2023

mattermost-build commented Mar 22, 2023

hanzei commented Mar 22, 2023

	message = fmt.Sprintf("[%s](%s) requested changes to your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
	message = fmt.Sprintf("[%s](%s) requested changes to your merge request [%s#%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)

[GH-262] Merge request approval subscription #307

[GH-262] Merge request approval subscription #307

Conversation

MatthewDorner commented Jun 24, 2022

Summary

Ticket Link

mattermod commented Jun 24, 2022

mattermod commented Jul 6, 2022

spirosoik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatthewDorner commented Jul 9, 2022

mattermod commented Jul 20, 2022

MatthewDorner commented Jul 22, 2022

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 Jul 22, 2022

codecov bot commented Jul 22, 2022 • edited

Codecov Report

spirosoik left a comment

Choose a reason for hiding this comment

mattermod commented Sep 5, 2022

hanzei commented Feb 22, 2023

hanzei commented Feb 22, 2023

mattermost-build commented Mar 7, 2023

DHaussermann left a comment • edited

Choose a reason for hiding this comment

hanzei commented Mar 10, 2023

MatthewDorner commented Mar 10, 2023

mattermost-build commented Mar 22, 2023

hanzei commented Mar 22, 2023

codecov bot commented Jul 22, 2022 •

edited

DHaussermann left a comment •

edited