diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index a538b6e290..3319f0de61 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -362,21 +362,35 @@ 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.Issue.Labels) > 0 { + actions = append(actions, "labeled") + } + if len(issuePayload.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)) { - matchTimes++ + for _, action := range actions { + if glob.MustCompile(val, '/').Match(action) { + matchTimes++ + break + } + } + // Once a match is found for this value, we can move to the next one + if matchTimes > 0 { break } } diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go index c8e1e553fe..8864dbb9b2 100644 --- a/modules/actions/workflows_test.go +++ b/modules/actions/workflows_test.go @@ -136,3 +136,159 @@ 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{}, + }, + 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"}, + }, + }, + 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"}, + }, + }, + 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"}, + }, + }, + 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"}, + }, + }, + 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"}, + }, + }, + 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 aaa9fbc9d3..c74b20b72b 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -310,13 +310,14 @@ const ( // IssuePayload represents the payload information that is sent along with an issue event. type IssuePayload struct { - Action HookIssueAction `json:"action"` - Index int64 `json:"number"` - Changes *ChangesPayload `json:"changes,omitempty"` - Issue *Issue `json:"issue"` - Repository *Repository `json:"repository"` - Sender *User `json:"sender"` - CommitID string `json:"commit_id"` + Action HookIssueAction `json:"action"` + Index int64 `json:"number"` + Changes *ChangesPayload `json:"changes,omitempty"` + RemovedLabels []*Label `json:"removed_labels"` + Issue *Issue `json:"issue"` + Repository *Repository `json:"repository"` + Sender *User `json:"sender"` + CommitID string `json:"commit_id"` } // JSONPayload encodes the IssuePayload to JSON, with an indentation of two spaces. @@ -341,6 +342,7 @@ type PullRequestPayload struct { Action HookIssueAction `json:"action"` Index int64 `json:"number"` Changes *ChangesPayload `json:"changes,omitempty"` + RemovedLabels []*Label `json:"removed_labels"` PullRequest *PullRequest `json:"pull_request"` RequestedReviewer *User `json:"requested_reviewer"` Repository *Repository `json:"repository"` diff --git a/services/actions/notifier.go b/services/actions/notifier.go index 831cde3523..3582590c8c 100644 --- a/services/actions/notifier.go +++ b/services/actions/notifier.go @@ -26,6 +26,12 @@ type actionsNotifier struct { notify_service.NullNotifier } +type contextKey string + +const ( + removedLabelsKey contextKey = "GiteaRemovedLabels" +) + var _ notify_service.Notifier = &actionsNotifier{} // NewNotifier create a new actionsNotifier notifier @@ -189,7 +195,7 @@ func (n *actionsNotifier) IssueChangeMilestone(ctx context.Context, doer *user_m } 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") @@ -198,6 +204,22 @@ func (n *actionsNotifier) IssueChangeLabels(ctx context.Context, doer *user_mode hookEvent = webhook_module.HookEventPullRequestLabel } + // Track removedLabels for the webhook payload + var removedAPILabels []*api.Label + if len(removedLabels) > 0 { + if err := issue.LoadRepo(ctx); err != nil { + log.Error("LoadRepo: %v", err) + return + } + + removedAPILabels = make([]*api.Label, 0, len(removedLabels)) + for _, label := range removedLabels { + removedAPILabels = append(removedAPILabels, convert.ToLabel(label, issue.Repo, doer)) + } + + ctx = context.WithValue(ctx, removedLabelsKey, removedAPILabels) + } + notifyIssueChange(ctx, doer, issue, hookEvent, api.HookIssueLabelUpdated) } @@ -213,34 +235,50 @@ func notifyIssueChange(ctx context.Context, doer *user_model.User, issue *issues return } + // Get removed labels from context if present + var removedAPILabels []*api.Label + if removedLabelsValue := ctx.Value(removedLabelsKey); removedLabelsValue != nil { + if labels, ok := removedLabelsValue.([]*api.Label); ok { + removedAPILabels = labels + } + } + 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), + 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), + 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) }