From cecac31f60b3701b1124a68a29291cc8c30712d5 Mon Sep 17 00:00:00 2001
From: Sumit Paul <sumit.18.paul@gmail.com>
Date: Tue, 29 Apr 2025 23:32:08 +0530
Subject: [PATCH] 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 fa248fa834..c2ca769d39 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 4062cdd6c2..a7eec1dee2 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")
 		})
 	}
 }