From def4988bc55d9f481ed5b8dadc92e4136df719d3 Mon Sep 17 00:00:00 2001 From: Andrei Petrus Date: Mon, 20 May 2024 11:27:23 +0300 Subject: [PATCH] Prevent slack Update policy from posting new messages Signed-off-by: Andrei Petrus --- pkg/util/slack/client.go | 33 ++++++++++++++++-------------- pkg/util/slack/client_test.go | 38 +++++++++++++++++------------------ 2 files changed, 36 insertions(+), 35 deletions(-) diff --git a/pkg/util/slack/client.go b/pkg/util/slack/client.go index 50ed8d95..158e1ab5 100644 --- 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 index 31276312..f934694a 100644 --- 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) }) } }