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

[MM-340] Fix issue of receiving incorrect notification for various events in DM #471

Merged
merged 7 commits into from
Jul 10, 2024
Merged

[MM-340] Fix issue of receiving incorrect notification for various events in DM #471

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9fda6e1
[MM-340] Fix issue of receiving incorrect notification for various ev…
ayusht2810 Apr 3, 2024
f4d5e9c
[MM-340] Fix flaky tests
ayusht2810 Apr 3, 2024
16bbb47
[MM-340] Update list of users to notify during multiple events update
ayusht2810 Apr 3, 2024
b0334c0
[MM-340] Update function name
ayusht2810 Jul 1, 2024
baa6c3d
[MM-340] Remove additional events notification received from gitlab
ayusht2810 Jul 8, 2024
a9ed1fb
[MM-340] Break comment in multiple lines
ayusht2810 Jul 8, 2024
38cc4fc
[MM-340] Update multi line comment to single line comment
ayusht2810 Jul 10, 2024
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
14 changes: 7 additions & 7 deletions build/pluginctl/logs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,17 +154,17 @@ func TestFilterLogEntries(t *testing.T) {
},
"filter out old entries": {
logs: []string{
fmt.Sprintf(`{"message":"old2", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Add(-2*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"old1", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Add(-1*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"now", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new1", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Add(1*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new2", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Add(2*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"old2", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Add(-2*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"old1", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Add(-1*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"now", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new1", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Add(1*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new2", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Add(2*time.Second).Format(timeStampFormat)),
},
pluginID: "some.plugin.id",
since: now,
expectedLogs: []string{
fmt.Sprintf(`{"message":"new1", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Add(1*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new2", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, time.Now().Add(2*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new1", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Add(1*time.Second).Format(timeStampFormat)),
fmt.Sprintf(`{"message":"new2", "plugin_id": "some.plugin.id", "timestamp": "%s"}`, now.Add(2*time.Second).Format(timeStampFormat)),
},
expectedErr: false,
},
Expand Down
194 changes: 173 additions & 21 deletions server/webhook/merge_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,57 +29,140 @@
}

message := ""

handlers := []*HandleWebhook{}
switch event.ObjectAttributes.State {
case stateOpened:
switch event.ObjectAttributes.Action {
case actionOpen:
message = fmt.Sprintf("[%s](%s) requested your review on [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
case actionReopen:
message = fmt.Sprintf("[%s](%s) reopened your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
case actionUpdate:
toUsers = []string{authorGitlabUsername}

// Not going to show notification in case of commit push.
if event.ObjectAttributes.OldRev != "" {
break
}

for _, currentAssigneeID := range event.ObjectAttributes.AssigneeIDs {
assignedInPrevious := false
for _, previousAssignee := range event.Changes.Assignees.Previous {
if previousAssignee.ID == currentAssigneeID {
assignedInPrevious = true
break
}
// Handle change in assignees
if event.Changes.Assignees.Current != nil || event.Changes.Assignees.Previous != nil {
isAuthorPresent, updatedPreviousUsers, updatedCurrentUsers := w.handleDMNotificationUsers(event.ObjectAttributes.AuthorID, event.Changes.Assignees.Previous, event.Changes.Assignees.Current)

// Check if the MR author is present in the assignee changes
if !isAuthorPresent {
message = fmt.Sprintf("[%s](%s) updated the list of assignees for the merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = append(handlers, &HandleWebhook{
Message: message,
ToUsers: []string{authorGitlabUsername},
From: senderGitlabUsername,
})
}
if !assignedInPrevious {
toUsers = append(toUsers, w.gitlabRetreiver.GetUsernameByID(currentAssigneeID))

if len(updatedPreviousUsers) != 0 {
message = fmt.Sprintf("[%s](%s) removed you as an assignee from the merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = append(handlers, &HandleWebhook{
Message: message,
ToUsers: updatedPreviousUsers,
From: senderGitlabUsername,
})
}

if len(updatedCurrentUsers) != 0 {
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)
handlers = append(handlers, &HandleWebhook{
Message: message,
ToUsers: updatedCurrentUsers,
From: senderGitlabUsername,
})
}
}

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)
// Handle change in reviewers
if event.Changes.Reviewers.Current != nil || event.Changes.Reviewers.Previous != nil {
isAuthorPresent, updatedPreviousUsers, updatedCurrentUsers := w.handleDMNotificationUsers(event.ObjectAttributes.AuthorID, event.Changes.Reviewers.Previous, event.Changes.Reviewers.Current)

// Check if the MR author is present in the assignee changes
if !isAuthorPresent {
message = fmt.Sprintf("[%s](%s) updated the list of reviewers for the merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
ayusht2810 marked this conversation as resolved.
Show resolved Hide resolved
handlers = append(handlers, &HandleWebhook{
Message: message,
ToUsers: []string{authorGitlabUsername},
From: senderGitlabUsername,
})
}

if len(updatedPreviousUsers) != 0 {
message = fmt.Sprintf("[%s](%s) removed you as a reviewer from the merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
ayusht2810 marked this conversation as resolved.
Show resolved Hide resolved
handlers = append(handlers, &HandleWebhook{
Message: message,
ToUsers: updatedPreviousUsers,
From: senderGitlabUsername,
})
}

if len(updatedCurrentUsers) != 0 {
message = fmt.Sprintf("[%s](%s) assigned you as a reviewer to merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = append(handlers, &HandleWebhook{
Message: message,
ToUsers: updatedCurrentUsers,
From: senderGitlabUsername,
})
}
}

// Check if multiple events happened in single webhook event
if w.checkForMultipleEvents(event) {
message = fmt.Sprintf("[%s](%s) updated the merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
mickmister marked this conversation as resolved.
Show resolved Hide resolved
}

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)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
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)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
}
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)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
case stateMerged:
if event.ObjectAttributes.Action == actionMerge {
message = fmt.Sprintf("[%s](%s) merged your merge request [%s!%v](%s)", senderGitlabUsername, w.gitlabRetreiver.GetUserURL(senderGitlabUsername), event.ObjectAttributes.Target.PathWithNamespace, event.ObjectAttributes.IID, event.ObjectAttributes.URL)
handlers = []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
From: senderGitlabUsername,
}}
}
}

if len(message) > 0 {
handlers := []*HandleWebhook{{
Message: message,
ToUsers: toUsers,
ToChannels: []string{},
From: senderGitlabUsername,
}}

if len(handlers) > 0 {
if mention := w.handleMention(mentionDetails{
senderUsername: senderGitlabUsername,
pathWithNamespace: event.Project.PathWithNamespace,
Expand Down Expand Up @@ -148,3 +231,72 @@

return res, nil
}

func (w *webhook) handleDMNotificationUsers(authorID int, previousUsers, currentUsers []*gitlab.EventUser) (bool, []string, []string) {
mickmister marked this conversation as resolved.
Show resolved Hide resolved
mapPreviousUsers := map[int]*gitlab.EventUser{}
mapCurrentUsers := map[int]*gitlab.EventUser{}
updatedPreviousUsers := []string{}
updatedCurrentUsers := []string{}
isAuthorPresent := false
for _, previousUser := range previousUsers {
mapPreviousUsers[previousUser.ID] = previousUser
}

for _, currentUser := range currentUsers {
mapCurrentUsers[currentUser.ID] = currentUser
if _, exist := mapPreviousUsers[currentUser.ID]; !exist {
if currentUser.ID == authorID {
isAuthorPresent = true
}

Check warning on line 250 in server/webhook/merge_request.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/merge_request.go#L249-L250 

Added lines #L249 - L250 were not covered by tests
updatedCurrentUsers = append(updatedCurrentUsers, w.gitlabRetreiver.GetUsernameByID(currentUser.ID))
}
}

for _, previousUser := range previousUsers {
if _, exist := mapCurrentUsers[previousUser.ID]; !exist {
if previousUser.ID == authorID {
isAuthorPresent = true
}
updatedPreviousUsers = append(updatedPreviousUsers, w.gitlabRetreiver.GetUsernameByID(previousUser.ID))
}
}

return isAuthorPresent, updatedPreviousUsers, updatedCurrentUsers
}

func (w *webhook) checkForMultipleEvents(event *gitlab.MergeEvent) bool {
eventCount := 0
if event.Changes.Labels.Current != nil || event.Changes.Labels.Previous != nil {
eventCount++
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can the code simply return with return true in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the above function in the updated logic as there was no need for it.

}

if event.Changes.Description.Current != "" || event.Changes.Description.Previous != "" {
eventCount++
}

if event.Changes.Title.Current != "" || event.Changes.Title.Previous != "" {
eventCount++
}

if event.Changes.MilestoneID.Current != 0 || event.Changes.MilestoneID.Previous != 0 {
eventCount++
}

Check warning on line 283 in server/webhook/merge_request.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/merge_request.go#L282-L283 

Added lines #L282 - L283 were not covered by tests

if event.Changes.Draft.Current || event.Changes.Draft.Previous {
eventCount++
}

Check warning on line 287 in server/webhook/merge_request.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/merge_request.go#L286-L287 

Added lines #L286 - L287 were not covered by tests

if event.Changes.TargetBranch.Current != "" || event.Changes.TargetBranch.Previous != "" {
eventCount++
}

Check warning on line 291 in server/webhook/merge_request.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/merge_request.go#L290-L291 

Added lines #L290 - L291 were not covered by tests

if event.Changes.SourceBranch.Current != "" || event.Changes.SourceBranch.Previous != "" {
eventCount++
}

Check warning on line 295 in server/webhook/merge_request.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/merge_request.go#L294-L295 

Added lines #L294 - L295 were not covered by tests

if (event.Changes.Assignees.Previous != nil || event.Changes.Assignees.Current != nil) && (event.Changes.Reviewers.Previous != nil || event.Changes.Reviewers.Current != nil) {
eventCount++
}

Check warning on line 299 in server/webhook/merge_request.go

View check run for this annotation

Codecov / codecov/patch

server/webhook/merge_request.go#L298-L299 

Added lines #L298 - L299 were not covered by tests

return eventCount > 0
}
Loading
Loading