Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent slack Update policy from posting new messages #298

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Prevent slack Update policy from posting new messages #298

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
def4988
Prevent slack Update policy from posting new messages
AndreiPetrusMihai May 20, 2024
d93d8cc
Merge branch 'master' into master
AndreiPetrusMihai Jun 14, 2024
15a938c
Merge branch 'master' into master
AndreiPetrusMihai Jun 26, 2024
b20ac36
Merge branch 'master' into master
AndreiPetrusMihai Jan 31, 2025
21b4f68
Merge branch 'master' into master
AndreiPetrusMihai Mar 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions pkg/util/slack/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating is the first condition so it always takes priority. This way, when a message has a policy of PostAndUpdate, it always updates the existing message instead of posting a new one in a thread.

This might also fix a race condition in which PostAndUpdate would post a message in a thread instead of updating the initial message. I didn't encounter this behavior a lot so I'm not 100% sure. Either way, it makes sense for updates to take priority in this case.

_, _, 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why you removed ts == "" check?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, it's been a while, but I think this was the source of the problem.

Let's suppose that ts != ""

If the policy is Update or PostAndUpdate, the first if would get executed and it would update the message. This is fine

If the policy is Post, this if block would get executed anyway. This is fine since it would post a message to a thread

Let's suppose that ts == ""

If the policy is Post or PostAndUpdate, this if would run, just like before. It would post a new message.

If the policy is Update, this if would get skipped, which didn't happen before. This is fine since this is what I want to fix. We don't want to post a message if the policy is Update and there is no timestamp found for thegroupingKey.

I hope I didn't miss anything. Some time has passed since I worked on this

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
Expand Down
38 changes: 18 additions & 20 deletions pkg/util/slack/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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)
})
}
}
Loading