Prevent slack Update policy from posting new messages

argoproj · May 20, 2024 · a09520e · a09520e
1 parent 1f05776
commit a09520e
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 35 deletions.
diff --git a/pkg/util/slack/client.go b/pkg/util/slack/client.go
@@ -114,37 +114,40 @@ func (c *threadedClient) SendMessage(ctx context.Context, recipient string, grou
 		options = append(options, sl.MsgOptionTS(ts))
 	}
 
-	if ts == "" || policy == Post || policy == PostAndUpdate {
-		newTs, channelID, err := SendMessageRateLimited(
+	// Updating an existing message
+	if ts != "" && (policy == Update || policy == PostAndUpdate) {
+		_, _, err := SendMessageRateLimited(
 			c.Client,
 			ctx,
 			c.Limiter,
-			recipient,
-			sl.MsgOptionPost(),
-			buildPostOptions(broadcast, options),
+			c.getChannelID(recipient),
+			sl.MsgOptionUpdate(ts),
+			sl.MsgOptionAsUser(true),
+			sl.MsgOptionCompose(options...),
 		)
 		if err != nil {
 			return err
 		}
-		if groupingKey != "" && ts == "" {
-			c.setThreadTimestamp(recipient, groupingKey, newTs)
-		}
-		c.ChannelIDs[recipient] = channelID
+		return nil
 	}
 
-	if ts != "" && (policy == Update || policy == PostAndUpdate) {
-		_, _, err := SendMessageRateLimited(
+	// Posting a new message
+	if policy == Post || policy == PostAndUpdate {
+		newTs, channelID, err := SendMessageRateLimited(
 			c.Client,
 			ctx,
 			c.Limiter,
-			c.getChannelID(recipient),
-			sl.MsgOptionUpdate(ts),
-			sl.MsgOptionAsUser(true),
-			sl.MsgOptionCompose(options...),
+			recipient,
+			sl.MsgOptionPost(),
+			buildPostOptions(broadcast, options),
 		)
 		if err != nil {
 			return err
 		}
+		if groupingKey != "" && ts == "" {
+			c.setThreadTimestamp(recipient, groupingKey, newTs)
+		}
+		c.ChannelIDs[recipient] = channelID
 	}
 
 	return nil

diff --git a/pkg/util/slack/client_test.go b/pkg/util/slack/client_test.go
@@ -103,58 +103,56 @@ func TestThreadedClient(t *testing.T) {
 		groupingKey   string
 		policy        DeliveryPolicy
 		wantPostType1 gomock.Matcher
-		wantPostType2 gomock.Matcher
-		wantthreadTSs timestampMap
+		wantThreadTSs timestampMap
 	}{
 		"Post, basic": {
 			threadTSs:     timestampMap{},
 			groupingKey:   "",
 			policy:        Post,
 			wantPostType1: EqChatPost(),
-			wantthreadTSs: timestampMap{},
+			wantThreadTSs: timestampMap{},
 		},
 		"Post, no parent, with grouping": {
 			threadTSs:     timestampMap{},
 			groupingKey:   groupingKey,
 			policy:        Post,
 			wantPostType1: EqChatPost(),
-			wantthreadTSs: timestampMap{channel: {groupingKey: ts1}},
+			wantThreadTSs: timestampMap{channel: {groupingKey: ts1}},
 		},
 		"Post, with parent, with grouping": {
 			threadTSs:     timestampMap{channel: {groupingKey: ts2}},
 			groupingKey:   groupingKey,
 			policy:        Post,
 			wantPostType1: EqChatPost(),
-			wantthreadTSs: timestampMap{channel: {groupingKey: ts2}},
+			wantThreadTSs: timestampMap{channel: {groupingKey: ts2}},
 		},
 		"PostAndUpdate, no parent. First post should not be updated": {
 			threadTSs:     timestampMap{},
 			groupingKey:   groupingKey,
 			policy:        PostAndUpdate,
 			wantPostType1: EqChatPost(),
-			wantthreadTSs: timestampMap{channel: {groupingKey: ts1}},
+			wantThreadTSs: timestampMap{channel: {groupingKey: ts1}},
 		},
 		"PostAndUpdate, with parent. First post should be updated": {
 			threadTSs:     timestampMap{channel: {groupingKey: ts2}},
 			groupingKey:   groupingKey,
 			policy:        PostAndUpdate,
-			wantPostType1: EqChatPost(),
-			wantPostType2: EqChatUpdate(),
-			wantthreadTSs: timestampMap{channel: {groupingKey: ts2}},
+			wantPostType1: EqChatUpdate(),
+			wantThreadTSs: timestampMap{channel: {groupingKey: ts2}},
 		},
-		"Update, no parent. Only call should be post": {
+		"Update, no parent. There should be no call, no new thread": {
 			threadTSs:     timestampMap{},
 			groupingKey:   groupingKey,
 			policy:        Update,
-			wantPostType1: EqChatPost(),
-			wantthreadTSs: timestampMap{channel: {groupingKey: ts1}},
+			wantPostType1: nil,
+			wantThreadTSs: timestampMap{},
 		},
 		"Update, with parent. Only call should be update": {
 			threadTSs:     timestampMap{channel: {groupingKey: ts2}},
 			groupingKey:   groupingKey,
 			policy:        Update,
 			wantPostType1: EqChatUpdate(),
-			wantthreadTSs: timestampMap{channel: {groupingKey: ts2}},
+			wantThreadTSs: timestampMap{channel: {groupingKey: ts2}},
 		},
 	}
 	for name, tc := range tests {
@@ -163,19 +161,19 @@ func TestThreadedClient(t *testing.T) {
 			defer ctrl.Finish()
 			m := mocks.NewMockSlackClient(ctrl)
 
-			m.EXPECT().
-				SendMessageContext(gomock.Any(), gomock.Eq(channel), tc.wantPostType1).
-				Return(channelID, ts1, "", nil)
+			expectedFunctionCall := m.EXPECT().
+				SendMessageContext(gomock.Any(), gomock.Eq(channel), tc.wantPostType1)
 
-			if tc.wantPostType2 != nil {
-				m.EXPECT().
-					SendMessageContext(gomock.Any(), gomock.Eq(channelID), tc.wantPostType2)
+			if tc.wantPostType1 != nil {
+				expectedFunctionCall.Return(channelID, ts1, "", nil)
+			} else {
+				expectedFunctionCall.MaxTimes(0)
 			}
 
 			client := NewThreadedClient(m, &state{rate.NewLimiter(rate.Inf, 1), tc.threadTSs, channelMap{}})
 			err := client.SendMessage(context.TODO(), channel, tc.groupingKey, false, tc.policy, []slack.MsgOption{})
 			assert.NoError(t, err)
-			assert.Equal(t, tc.wantthreadTSs, client.ThreadTSs)
+			assert.Equal(t, tc.wantThreadTSs, client.ThreadTSs)
 		})
 	}
 }