diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index 27bcafa6497b9..4ae9c820ce0df 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -377,20 +377,28 @@ func matchIssuesEvent(issuePayload *api.IssuePayload, evt *jobparser.Event) bool // Actions with the same name: // opened, edited, closed, reopened, assigned, unassigned, milestoned, demilestoned // Actions need to be converted: - // label_updated -> labeled + // label_updated -> labeled (when adding) or unlabeled (when removing) // label_cleared -> unlabeled // Unsupported activity types: // deleted, transferred, pinned, unpinned, locked, unlocked - action := issuePayload.Action - switch action { + actions := []string{} + switch issuePayload.Action { case api.HookIssueLabelUpdated: - action = "labeled" + if len(issuePayload.Changes.AddedLabels) > 0 { + actions = append(actions, "labeled") + } + if len(issuePayload.Changes.RemovedLabels) > 0 { + actions = append(actions, "unlabeled") + } case api.HookIssueLabelCleared: - action = "unlabeled" + actions = append(actions, "unlabeled") + default: + actions = append(actions, string(issuePayload.Action)) } + for _, val := range vals { - if glob.MustCompile(val, '/').Match(string(action)) { + if slices.ContainsFunc(actions, glob.MustCompile(val, '/').Match) { matchTimes++ break } diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go index e23431651da3b..89620fb698861 100644 --- a/modules/actions/workflows_test.go +++ b/modules/actions/workflows_test.go @@ -154,3 +154,184 @@ func TestDetectMatched(t *testing.T) { }) } } + +func TestMatchIssuesEvent(t *testing.T) { + testCases := []struct { + desc string + payload *api.IssuePayload + yamlOn string + expected bool + eventType string + }{ + { + desc: "Label deletion should trigger unlabeled event", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelUpdated, + Issue: &api.Issue{ + Labels: []*api.Label{}, + }, + Changes: &api.ChangesPayload{ + RemovedLabels: []*api.Label{ + {ID: 123, Name: "deleted-label"}, + }, + }, + }, + yamlOn: "on:\n issues:\n types: [unlabeled]", + expected: true, + eventType: "unlabeled", + }, + { + desc: "Label deletion with existing labels should trigger unlabeled event", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelUpdated, + Issue: &api.Issue{ + Labels: []*api.Label{ + {ID: 456, Name: "existing-label"}, + }, + }, + Changes: &api.ChangesPayload{ + AddedLabels: nil, + RemovedLabels: []*api.Label{ + {ID: 123, Name: "deleted-label"}, + }, + }, + }, + yamlOn: "on:\n issues:\n types: [unlabeled]", + expected: true, + eventType: "unlabeled", + }, + { + desc: "Label addition should trigger labeled event", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelUpdated, + Issue: &api.Issue{ + Labels: []*api.Label{ + {ID: 123, Name: "new-label"}, + }, + }, + Changes: &api.ChangesPayload{ + AddedLabels: []*api.Label{ + {ID: 123, Name: "new-label"}, + }, + RemovedLabels: []*api.Label{}, // Empty array, no labels removed + }, + }, + yamlOn: "on:\n issues:\n types: [labeled]", + expected: true, + eventType: "labeled", + }, + { + desc: "Label clear should trigger unlabeled event", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelCleared, + Issue: &api.Issue{ + Labels: []*api.Label{}, + }, + }, + yamlOn: "on:\n issues:\n types: [unlabeled]", + expected: true, + eventType: "unlabeled", + }, + { + desc: "Both adding and removing labels should trigger labeled event", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelUpdated, + Issue: &api.Issue{ + Labels: []*api.Label{ + {ID: 789, Name: "new-label"}, + }, + }, + Changes: &api.ChangesPayload{ + AddedLabels: []*api.Label{ + {ID: 789, Name: "new-label"}, + }, + RemovedLabels: []*api.Label{ + {ID: 123, Name: "deleted-label"}, + }, + }, + }, + yamlOn: "on:\n issues:\n types: [labeled]", + expected: true, + eventType: "labeled", + }, + { + desc: "Both adding and removing labels should trigger unlabeled event", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelUpdated, + Issue: &api.Issue{ + Labels: []*api.Label{ + {ID: 789, Name: "new-label"}, + }, + }, + Changes: &api.ChangesPayload{ + AddedLabels: []*api.Label{ + {ID: 789, Name: "new-label"}, + }, + RemovedLabels: []*api.Label{ + {ID: 123, Name: "deleted-label"}, + }, + }, + }, + yamlOn: "on:\n issues:\n types: [unlabeled]", + expected: true, + eventType: "unlabeled", + }, + { + desc: "Both adding and removing labels should trigger both events", + payload: &api.IssuePayload{ + Action: api.HookIssueLabelUpdated, + Issue: &api.Issue{ + Labels: []*api.Label{ + {ID: 789, Name: "new-label"}, + }, + }, + Changes: &api.ChangesPayload{ + AddedLabels: []*api.Label{ + {ID: 789, Name: "new-label"}, + }, + RemovedLabels: []*api.Label{ + {ID: 123, Name: "deleted-label"}, + }, + }, + }, + yamlOn: "on:\n issues:\n types: [labeled, unlabeled]", + expected: true, + eventType: "multiple", + }, + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + evts, err := GetEventsFromContent([]byte(tc.yamlOn)) + assert.NoError(t, err) + assert.Len(t, evts, 1) + + // Test if the event matches as expected + assert.Equal(t, tc.expected, matchIssuesEvent(tc.payload, evts[0])) + + // For extra validation, check that action mapping works correctly + if tc.eventType == "multiple" { + // Skip direct action mapping validation for multiple events case + // as one action can map to multiple event types + return + } + + // Determine expected action for single event case + var expectedAction string + switch tc.payload.Action { + case api.HookIssueLabelUpdated: + if tc.eventType == "labeled" { + expectedAction = "labeled" + } else if tc.eventType == "unlabeled" { + expectedAction = "unlabeled" + } + case api.HookIssueLabelCleared: + expectedAction = "unlabeled" + default: + expectedAction = string(tc.payload.Action) + } + + assert.Equal(t, expectedAction, tc.eventType, "Event type should match expected") + }) + } +} diff --git a/modules/structs/hook.go b/modules/structs/hook.go index 776e44ccec54d..57af38464a2f3 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -418,6 +418,10 @@ type ChangesPayload struct { Body *ChangesFromPayload `json:"body,omitempty"` // Changes made to the reference Ref *ChangesFromPayload `json:"ref,omitempty"` + // Changes made to the labels added + AddedLabels []*Label `json:"added_labels"` + // Changes made to the labels removed + RemovedLabels []*Label `json:"removed_labels"` } // PullRequestPayload represents a payload information of pull request event. diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 110e330f68d59..c4bfe5c11b3da 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -169,7 +169,7 @@ func (n *actionsNotifier) IssueChangeAssignee(ctx context.Context, doer *user_mo hookEvent = webhook_module.HookEventPullRequestAssign } - notifyIssueChange(ctx, doer, issue, hookEvent, action) + notifyIssueChange(ctx, doer, issue, hookEvent, action, nil, nil) } // IssueChangeMilestone notifies assignee to notifiers @@ -188,11 +188,11 @@ func (n *actionsNotifier) IssueChangeMilestone(ctx context.Context, doer *user_m hookEvent = webhook_module.HookEventPullRequestMilestone } - notifyIssueChange(ctx, doer, issue, hookEvent, action) + notifyIssueChange(ctx, doer, issue, hookEvent, action, nil, nil) } func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, - _, _ []*issues_model.Label, + addedLabels, removedLabels []*issues_model.Label, ) { ctx = withMethod(ctx, "IssueChangeLabels") @@ -201,10 +201,10 @@ func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_mode hookEvent = webhook_module.HookEventPullRequestLabel } - notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated) + notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated, addedLabels, removedLabels) } -func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, event webhook_module.HookEventType, action api.HookIssueAction) { +func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues_model.Issue, event webhook_module.HookEventType, action api.HookIssueAction, addedLabels, removedLabels []*issues_model.Label) { var err error if err = issue.LoadRepo(ctx); err != nil { log.Error("LoadRepo: %v", err) @@ -216,34 +216,65 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues return } + var addedAPILabels []*api.Label + if addedLabels != nil { + addedAPILabels = make([]*api.Label, 0, len(addedLabels)) + for _, label := range addedLabels { + addedAPILabels = append(addedAPILabels, convert.ToLabel(label, issue.Repo, doer)) + } + } + + // Get removed labels from context if present + var removedAPILabels []*api.Label + if removedLabels != nil { + removedAPILabels = make([]*api.Label, 0, len(removedLabels)) + for _, label := range removedLabels { + removedAPILabels = append(removedAPILabels, convert.ToLabel(label, issue.Repo, doer)) + } + } + if issue.IsPull { if err = issue.LoadPullRequest(ctx); err != nil { log.Error("loadPullRequest: %v", err) return } + + payload := &api.PullRequestPayload{ + Action: action, + Index: issue.Index, + PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, nil), + Repository: convert.ToRepo(ctx, issue.Repo, access_model.Permission{AccessMode: perm_model.AccessModeNone}), + Sender: convert.ToUser(ctx, doer, nil), + Changes: &api.ChangesPayload{ + AddedLabels: addedAPILabels, + RemovedLabels: removedAPILabels, + }, + } + newNotifyInputFromIssue(issue, event). WithDoer(doer). - WithPayload(&api.PullRequestPayload{ - Action: action, - Index: issue.Index, - PullRequest: convert.ToAPIPullRequest(ctx, issue.PullRequest, nil), - Repository: convert.ToRepo(ctx, issue.Repo, access_model.Permission{AccessMode: perm_model.AccessModeNone}), - Sender: convert.ToUser(ctx, doer, nil), - }). + WithPayload(payload). WithPullRequest(issue.PullRequest). Notify(ctx) return } + permission, _ := access_model.GetUserRepoPermission(ctx, issue.Repo, issue.Poster) + payload := &api.IssuePayload{ + Action: action, + Index: issue.Index, + Issue: convert.ToAPIIssue(ctx, doer, issue), + Repository: convert.ToRepo(ctx, issue.Repo, permission), + Sender: convert.ToUser(ctx, doer, nil), + Changes: &api.ChangesPayload{ + AddedLabels: addedAPILabels, + RemovedLabels: removedAPILabels, + }, + } + newNotifyInputFromIssue(issue, event). WithDoer(doer). - WithPayload(&api.IssuePayload{ - Action: action, - Index: issue.Index, - Issue: convert.ToAPIIssue(ctx, doer, issue), - Repository: convert.ToRepo(ctx, issue.Repo, permission), - Sender: convert.ToUser(ctx, doer, nil), - }). + WithPayload(payload). Notify(ctx) }