From 15e707c0c8c755bc4b4116e5e10dfb36c166b0b2 Mon Sep 17 00:00:00 2001 From: Sumit Paul Date: Tue, 29 Apr 2025 22:40:06 +0530 Subject: [PATCH 1/5] Enhance label event handling in matchIssuesEvent function and add tests for label addition/removal scenarios --- modules/actions/workflows.go | 9 ++- modules/actions/workflows_test.go | 95 +++++++++++++++++++++++++++++++ modules/structs/hook.go | 15 ++--- 3 files changed, 110 insertions(+), 9 deletions(-) diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index a538b6e290bec..fa248fa8349da 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -362,7 +362,7 @@ 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 @@ -370,7 +370,12 @@ func matchIssuesEvent(issuePayload *api.IssuePayload, evt *jobparser.Event) bool action := issuePayload.Action switch action { case api.HookIssueLabelUpdated: - action = "labeled" + // Check if any labels were removed to determine if this should be "labeled" or "unlabeled" + if len(issuePayload.RemovedLabels) > 0 { + action = "unlabeled" + } else { + action = "labeled" + } case api.HookIssueLabelCleared: action = "unlabeled" } diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go index c8e1e553fe94b..4062cdd6c2209 100644 --- a/modules/actions/workflows_test.go +++ b/modules/actions/workflows_test.go @@ -136,3 +136,98 @@ 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", + }, + } + + 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, use a direct call to test the actual mapping + action := tc.payload.Action + switch action { + case api.HookIssueLabelUpdated: + if len(tc.payload.RemovedLabels) > 0 { + action = "unlabeled" + } else { + action = "labeled" + } + case api.HookIssueLabelCleared: + action = "unlabeled" + } + assert.Equal(t, tc.eventType, string(action), "Event type should match expected") + }) + } +} diff --git a/modules/structs/hook.go b/modules/structs/hook.go index aaa9fbc9d364d..890a99c8fe522 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. From cecac31f60b3701b1124a68a29291cc8c30712d5 Mon Sep 17 00:00:00 2001 From: Sumit Paul Date: Tue, 29 Apr 2025 23:32:08 +0530 Subject: [PATCH 2/5] Enhance matchIssuesEvent function to support multiple label actions and add corresponding tests --- modules/actions/workflows.go | 32 +++++++++---- modules/actions/workflows_test.go | 79 +++++++++++++++++++++++++++---- 2 files changed, 93 insertions(+), 18 deletions(-) diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index fa248fa8349da..c2ca769d39d5e 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -367,21 +367,35 @@ func matchIssuesEvent(issuePayload *api.IssuePayload, evt *jobparser.Event) bool // Unsupported activity types: // deleted, transferred, pinned, unpinned, locked, unlocked - action := issuePayload.Action - switch action { + actions := []string{} + switch issuePayload.Action { case api.HookIssueLabelUpdated: - // Check if any labels were removed to determine if this should be "labeled" or "unlabeled" - if len(issuePayload.RemovedLabels) > 0 { - action = "unlabeled" + // Check if both labels were added and removed to determine events to fire + if len(issuePayload.Issue.Labels) > 0 && len(issuePayload.RemovedLabels) > 0 { + // Both labeled and unlabeled events should be triggered + actions = append(actions, "labeled", "unlabeled") + } else if len(issuePayload.RemovedLabels) > 0 { + // Only labels were removed + actions = append(actions, "unlabeled") } else { - action = "labeled" + // Only labels were added + actions = append(actions, "labeled") } 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 4062cdd6c2209..a7eec1dee2902 100644 --- a/modules/actions/workflows_test.go +++ b/modules/actions/workflows_test.go @@ -204,6 +204,57 @@ func TestMatchIssuesEvent(t *testing.T) { 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 { @@ -215,19 +266,29 @@ func TestMatchIssuesEvent(t *testing.T) { // Test if the event matches as expected assert.Equal(t, tc.expected, matchIssuesEvent(tc.payload, evts[0])) - // For extra validation, use a direct call to test the actual mapping - action := tc.payload.Action - switch action { + // 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 len(tc.payload.RemovedLabels) > 0 { - action = "unlabeled" - } else { - action = "labeled" + if tc.eventType == "labeled" { + expectedAction = "labeled" + } else if tc.eventType == "unlabeled" { + expectedAction = "unlabeled" } case api.HookIssueLabelCleared: - action = "unlabeled" + expectedAction = "unlabeled" + default: + expectedAction = string(tc.payload.Action) } - assert.Equal(t, tc.eventType, string(action), "Event type should match expected") + + assert.Equal(t, tc.eventType, expectedAction, "Event type should match expected") }) } } From 666411692b96a96075f3b0d05b6c3d0065b9b712 Mon Sep 17 00:00:00 2001 From: Sumit Paul Date: Tue, 29 Apr 2025 23:41:34 +0530 Subject: [PATCH 3/5] fix assert order --- modules/actions/workflows_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/actions/workflows_test.go b/modules/actions/workflows_test.go index a7eec1dee2902..8864dbb9b2387 100644 --- a/modules/actions/workflows_test.go +++ b/modules/actions/workflows_test.go @@ -288,7 +288,7 @@ func TestMatchIssuesEvent(t *testing.T) { expectedAction = string(tc.payload.Action) } - assert.Equal(t, tc.eventType, expectedAction, "Event type should match expected") + assert.Equal(t, expectedAction, tc.eventType, "Event type should match expected") }) } } From 56314f605ac5fbd5226f67ec99e23e6692e1b98f Mon Sep 17 00:00:00 2001 From: Sumit Date: Tue, 29 Apr 2025 23:51:49 +0530 Subject: [PATCH 4/5] Update modules/actions/workflows.go Co-authored-by: Lunny Xiao --- modules/actions/workflows.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/modules/actions/workflows.go b/modules/actions/workflows.go index c2ca769d39d5e..3319f0de61b87 100644 --- a/modules/actions/workflows.go +++ b/modules/actions/workflows.go @@ -370,17 +370,12 @@ func matchIssuesEvent(issuePayload *api.IssuePayload, evt *jobparser.Event) bool actions := []string{} switch issuePayload.Action { case api.HookIssueLabelUpdated: - // Check if both labels were added and removed to determine events to fire - if len(issuePayload.Issue.Labels) > 0 && len(issuePayload.RemovedLabels) > 0 { - // Both labeled and unlabeled events should be triggered - actions = append(actions, "labeled", "unlabeled") - } else if len(issuePayload.RemovedLabels) > 0 { - // Only labels were removed - actions = append(actions, "unlabeled") - } else { - // Only labels were added + if len(issuePayload.Issue.Labels) > 0 { actions = append(actions, "labeled") } + if len(issuePayload.RemovedLabels) > 0 { + actions = append(actions, "unlabeled") + } case api.HookIssueLabelCleared: actions = append(actions, "unlabeled") default: From 84b684eba4b993a4a85f764032131c7a14995eb1 Mon Sep 17 00:00:00 2001 From: Sumit Paul Date: Wed, 30 Apr 2025 00:51:15 +0530 Subject: [PATCH 5/5] Add support for removed labels in issue and pull request notifications --- modules/structs/hook.go | 1 + services/actions/notifier.go | 68 ++++++++++++++++++++++++++++-------- 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/modules/structs/hook.go b/modules/structs/hook.go index 890a99c8fe522..c74b20b72b614 100644 --- a/modules/structs/hook.go +++ b/modules/structs/hook.go @@ -342,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 831cde3523f73..3582590c8ca75 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) }