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.

Sign up for GitHub

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

Jump to bottom

[GH-262] Merge request approval subscription #307

Merged
merged 11 commits into from
Mar 22, 2023
Merged

[GH-262] Merge request approval subscription #307

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4337f2a
Notify for Merge Request approval/unapproval
MatthewDorner Jun 5, 2022
7d514a5
Added to commandHelp
MatthewDorner Jun 13, 2022
f9b4be1
Remove subscription feature
MatthewDorner Jun 15, 2022
c262803
Add channel messages
MatthewDorner Jun 24, 2022
4bdac0e
Update command.go
MatthewDorner Jun 24, 2022
27bb466
Update subscription.go
MatthewDorner Jun 24, 2022
286ec2f
Reword message templates
MatthewDorner Jul 9, 2022
9795c7b
Merge branch 'merge-request-approval-subscription' of https://github.…
MatthewDorner Jul 9, 2022
30c35d2
npm install --verbose
mickmister Jul 22, 2022
1371a64
Update Makefile
MatthewDorner Dec 2, 2022
6bc336c
Merge remote-tracking branch 'upstream/master' into merge-request-app…
MatthewDorner Mar 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion server/subscription/subscription.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var allFeatures = map[string]bool{
"pipeline": true,
"tag": true,
"pull_reviews": true,
// "label:": true,//particular case for label:XXX
// "label:": true,//particular case for label:XXX
}

type Subscription struct {
Expand Down
12 changes: 7 additions & 5 deletions server/webhook/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package webhook

const (
actionOpen = "open"
actionClose = "close"
actionMerge = "merge"
actionReopen = "reopen"
actionUpdate = "update"
actionOpen = "open"
actionClose = "close"
actionMerge = "merge"
actionReopen = "reopen"
actionUpdate = "update"
actionApproved = "approved"
actionUnapproved = "unapproved"

stateOpened = "opened"
stateClosed = "closed"
Expand Down
8 changes: 8 additions & 0 deletions server/webhook/merge_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ func (w *webhook) handleDMMergeRequest(event *gitlab.MergeEvent) ([]*HandleWebho
case actionUpdate:
// TODO not enough check (opened/update) to say assigned to you...
message = fmt.Sprintf("[%s](%s) assigned you to merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

2022_07_24_0lg_Kleki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
case stateClosed:
message = fmt.Sprintf("[%s](%s) closed your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
Expand Down Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

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

(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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

case actionUnapproved:
message = fmt.Sprintf("[%s] Merge request [!%v %s](%s) changes were requested by [%s](%s)", repo.PathWithNamespace, pr.IID, pr.Title, pr.URL, senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername))
}

if len(message) > 0 {
Expand Down
127 changes: 127 additions & 0 deletions server/webhook/merge_request_fixture_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,3 +833,130 @@ const MergeRequestMerged = `{
"avatar_url":"https://www.gravatar.com/avatar/c6b552a4cd47f7cf1701ea5b650cd2e3?s=80\\u0026d=identicon"
}
}`

const ApproveMergeRequest = `{
"object_kind":"merge_request",
"event_type":"merge_request",
"user":{
"name":"manland",
"username":"manland",
"avatar_url":"https://www.gravatar.com/avatar/c6b552a4cd47f7cf1701ea5b650cd2e3?s=80\\u0026d=identicon"
},
"project":{
"id":24,
"name":"webhook",
"description":"",
"web_url":"http://localhost:3000/manland/webhook",
"avatar_url":null,
"git_ssh_url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"git_http_url":"http://localhost:3000/manland/webhook.git",
"namespace":"manland",
"visibility_level":20,
"path_with_namespace":"manland/webhook",
"default_branch":"master",
"ci_config_path":null,
"homepage":"http://localhost:3000/manland/webhook",
"url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"ssh_url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"http_url":"http://localhost:3000/manland/webhook.git"
},
"object_attributes":{
"assignee_id":50,
"author_id":1,
"created_at":"2019-04-03 21:07:32 UTC",
"description":"test open merge request",
"head_pipeline_id":57,
"id":35,
"iid":4,
"last_edited_at":null,
"last_edited_by_id":null,
"merge_commit_sha":null,
"merge_error":null,
"merge_params":{
"force_remove_source_branch":null
},
"merge_status":"can_be_merged",
"merge_user_id":null,
"merge_when_pipeline_succeeds":false,
"milestone_id":null,
"source_branch":"master",
"source_project_id":25,
"state":"opened",
"target_branch":"master",
"target_project_id":24,
"time_estimate":0,
"title":"Master",
"updated_at":"2019-04-03 21:29:41 UTC",
"updated_by_id":null,
"url":"http://localhost:3000/manland/webhook/merge_requests/4",
"source":{
"id":25,
"name":"webhook",
"description":"",
"web_url":"http://localhost:3000/root/webhook",
"avatar_url":null,
"git_ssh_url":"ssh://rmaneschi@localhost:2222/root/webhook.git",
"git_http_url":"http://localhost:3000/root/webhook.git",
"namespace":"root",
"visibility_level":20,
"path_with_namespace":"root/webhook",
"default_branch":"master",
"ci_config_path":null,
"homepage":"http://localhost:3000/root/webhook",
"url":"ssh://rmaneschi@localhost:2222/root/webhook.git",
"ssh_url":"ssh://rmaneschi@localhost:2222/root/webhook.git",
"http_url":"http://localhost:3000/root/webhook.git"
},
"target":{
"id":24,
"name":"webhook",
"description":"",
"web_url":"http://localhost:3000/manland/webhook",
"avatar_url":null,
"git_ssh_url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"git_http_url":"http://localhost:3000/manland/webhook.git",
"namespace":"manland",
"visibility_level":20,
"path_with_namespace":"manland/webhook",
"default_branch":"master",
"ci_config_path":null,
"homepage":"http://localhost:3000/manland/webhook",
"url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"ssh_url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"http_url":"http://localhost:3000/manland/webhook.git"
},
"last_commit":{
"id":"1fd967c14f8265a6056525c343d984ce56472d5c",
"message":"Update README.md",
"timestamp":"2019-04-03T21:04:58Z",
"url":"http://localhost:3000/manland/webhook/commit/1fd967c14f8265a6056525c343d984ce56472d5c",
"author":{
"name":"Administrator",
"email":"admin@example.com"
}
},
"work_in_progress":false,
"total_time_spent":0,
"human_total_time_spent":null,
"human_time_estimate":null,
"action":"approved"
},
"labels":[],
"changes":{
"updated_at":{
"previous":"2019-04-03 21:07:32 UTC",
"current":"2019-04-03 21:29:41 UTC"
}
},
"repository":{
"name":"webhook",
"url":"ssh://rmaneschi@localhost:2222/manland/webhook.git",
"description":"",
"homepage":"http://localhost:3000/manland/webhook"
},
"assignee":{
"name":"manland",
"username":"manland",
"avatar_url":"https://www.gravatar.com/avatar/c6b552a4cd47f7cf1701ea5b650cd2e3?s=80\\u0026d=identicon"
}
}`
34 changes: 34 additions & 0 deletions server/webhook/merge_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,40 @@ var testDataMergeRequest = []testDataMergeRequestStr{
ToChannels: []string{"channel1"},
From: "manland",
}},
}, {
testTitle: "manland approve root merge-request and display in channel1",
fixture: ApproveMergeRequest,
gitlabRetreiver: newFakeWebhook([]*subscription.Subscription{
{ChannelID: "channel1", CreatorID: "1", Features: "merges", Repository: "manland/webhook"},
}),
res: []*HandleWebhook{{
Message: "[manland](http://my.gitlab.com/manland) approved your merge request [manland/webhook!4](http://localhost:3000/manland/webhook/merge_requests/4)",
ToUsers: []string{"root"},
ToChannels: []string{},
From: "manland",
}, {
Message: "[manland/webhook] Merge request [!4 Master](http://localhost:3000/manland/webhook/merge_requests/4) was approved by [manland](http://my.gitlab.com/manland)",
ToUsers: []string{},
ToChannels: []string{"channel1"},
From: "manland",
}},
}, {
testTitle: "manland unapprove root merge-request and display in channel1",
fixture: strings.ReplaceAll(ApproveMergeRequest, "approved", "unapproved"),
gitlabRetreiver: newFakeWebhook([]*subscription.Subscription{
{ChannelID: "channel1", CreatorID: "1", Features: "merges", Repository: "manland/webhook"},
}),
res: []*HandleWebhook{{
Message: "[manland](http://my.gitlab.com/manland) requested changes to your merge request [manland/webhook!4](http://localhost:3000/manland/webhook/merge_requests/4)",
ToUsers: []string{"root"},
ToChannels: []string{},
From: "manland",
}, {
Message: "[manland/webhook] Merge request [!4 Master](http://localhost:3000/manland/webhook/merge_requests/4) changes were requested by [manland](http://my.gitlab.com/manland)",
ToUsers: []string{},
ToChannels: []string{"channel1"},
From: "manland",
}},
}, {
testTitle: "root close its own MR without assignee and display in channel1",
fixture: CloseMergeRequestByCreator,
Expand Down
2 changes: 1 addition & 1 deletion server/webhook/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func cleanWebhookHandlers(handlers []*HandleWebhook) []*HandleWebhook {
func cleanWebhookHandlerTo(handler *HandleWebhook) *HandleWebhook {
users := map[string]bool{}
for _, v := range handler.ToUsers {
if handler.From != v && v != "" { // don't send message to author or unknown user
if handler.From != v && v != "" { // don't send message to webhook sender or unknown user
users[v] = true
}
}
Expand Down