feat(slack): add support for thread replies and message updates (#187) #410
Conversation
willianpaixao
force-pushed
the
willianpaixao-issue-187
branch
from
27d9989 to
92e3a9f
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for explicit thread replies and message updates in the Slack notification service. The implementation extends the SlackNotification struct with threadTs and updateTs fields that support template expressions for dynamic values.
Key changes:
- Added
ThreadTSandUpdateTSfields toSlackNotificationstruct with template support - Modified
SendMessagemethod to handle explicit thread/update timestamps with early-return logic - Added comprehensive test coverage for new features and backward compatibility
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/services/slack.go | Added ThreadTS and UpdateTS fields to SlackNotification struct, implemented template parsing for both fields, and updated Send method call |
| pkg/util/slack/client.go | Extended SendMessage signature with threadTS and updateTS parameters, implemented early-return logic for explicit timestamps |
| pkg/util/slack/client_test.go | Added new test functions for explicit threadTS and updateTS, refactored existing tests into backward compatibility suite, updated all SendMessage calls with new parameters |
| go.mod | Updated slack-go/slack from v0.16.0 to v0.17.3, updated gorilla/websocket from v1.5.0 to v1.5.3 |
| go.sum | Updated checksums for updated dependencies |
| docs/services/slack.md | Added documentation section for Thread Replies and Message Updates with examples and important notes |
Comments suppressed due to low confidence (1)
pkg/util/slack/client_test.go:272
- The field name 'wantthreadTSs' has inconsistent capitalization. It should be 'wantThreadTSs' to follow Go naming conventions (camelCase with proper acronym capitalization).
wantthreadTSs timestampMap
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
Great improvement that allows a lot of use cases that currently must be done with webhooks and the slack API. Thank you!
|
Hey @pasha-codefresh @alexmt @blakepettersson can I get a review please? |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
pkg/util/slack/client_test.go:272
- The field name
wantthreadTSsuses inconsistent capitalization. It should bewantThreadTSsto follow Go naming conventions (camelCase with proper capitalization of acronyms).
wantthreadTSs timestampMap
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…proj#187) Changes: - Add threadTs field to post messages as replies in specific threads - Add updateTs field to update specific existing messages - Both fields support template expressions for dynamic values - When set, these fields override the default groupingKey/deliveryPolicy behavior - Maintain full backward compatibility with existing functionality Implementation details: - Extended SlackNotification struct with ThreadTS and UpdateTS fields - Modified SendMessage to handle explicit thread/update timestamps - Added comprehensive tests for new features and backward compatibility - Updated documentation with usage examples and important notes Signed-off-by: Willian Paixao <willian@ufpa.br>
willianpaixao
force-pushed
the
willianpaixao-issue-187
branch
3 times, most recently
from
36978d3 to
1397cce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestThreadedClient_ExplicitUpdateTS(t *testing.T) { | ||
| const ( | ||
| channel string = "channel" | ||
| channelID string = "channel-ID" | ||
| updateTS string = "1234567890.123456" | ||
| ) | ||
|
|
||
| t.Run("Explicit updateTS should update specific message", func(t *testing.T) { | ||
| ctrl := gomock.NewController(t) | ||
| defer ctrl.Finish() | ||
| m := mocks.NewMockSlackClient(ctrl) | ||
|
|
||
| // Expect an update call with channelID | ||
| m.EXPECT(). | ||
| SendMessageContext(gomock.Any(), gomock.Eq(channelID), EqChatUpdate()). | ||
| Return("", "", "", nil) | ||
|
|
||
| client := NewThreadedClient( | ||
| m, | ||
| &state{ | ||
| Limiter: rate.NewLimiter(rate.Inf, 1), | ||
| ThreadTSs: timestampMap{}, | ||
| ChannelIDs: channelMap{channel: channelID}, | ||
| }, | ||
| ) | ||
|
|
||
| // Send message with explicit updateTS | ||
| err := client.SendMessage(context.TODO(), channel, "", false, Post, "", updateTS, []slack.MsgOption{}) | ||
| assert.NoError(t, err) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the mutual exclusivity validation between threadTS and updateTS. The implementation at client.go:126-128 includes validation that returns an error when both fields are set, but there is no test case verifying this error condition is triggered correctly.
Signed-off-by: Willian Paixao <willian@ufpa.br>
willianpaixao
force-pushed
the
willianpaixao-issue-187
branch
from
1397cce to
65974d8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #410 +/- ##
==========================================
+ Coverage 55.41% 57.87% +2.45%
==========================================
Files 46 47 +1
Lines 4125 4529 +404
==========================================
+ Hits 2286 2621 +335
- Misses 1511 1558 +47
- Partials 328 350 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Changes:
Implementation details: