feat(handler): Add background handler for /mention-each command#95
feat(handler): Add background handler for /mention-each command#95Devashish08 wants to merge 19 commits intoRealDevSquad:developfrom
Conversation
* Implemented MentionEach command to mention all users with a specific role. * Updated command names and constants to include MentionEach. * Added utility functions for handling user and role mentions.
…d refactor getUserWithRole to use models.SessionInterface
Summary by CodeRabbit
WalkthroughThis update introduces the "MentionEach" command to a Discord bot codebase. It adds the command definition, handler, and service logic to process requests for mentioning all users with a specified role, either collectively or individually, with optional custom messages and feature flags. Supporting utilities for member retrieval and mention formatting are implemented, alongside comprehensive unit tests. Several method signature corrections and mock implementations are included to facilitate testing and ensure interface consistency. Minor code cleanups, such as import reordering and typo fixes, are also present. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Discord
participant Service
participant Handler
participant Utils
User->>Discord: Issues /mention-each command
Discord->>Service: Sends interaction request
Service->>Service: Validate input & feature flag
Service->>Service: Prepare metadata & DataPacket
Service->>Handler: Enqueue DataPacket for processing
Handler->>Handler: Extract parameters from metadata
Handler->>Utils: Fetch users with role
Utils-->>Handler: Return members or error
Handler->>Discord: Send mention(s) or error message
Assessment against linked issues
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
commands/main/resgister_test.go (1)
98-104:⚠️ Potential issueRemove duplicate test case.
This test case is identical to the one at lines 91-97. Remove the duplicate to maintain clean test code.
- t.Run("should panic when openSession.ApplicationCommandCreate() returns an error", func(t *testing.T) { - mockSess := &mockSession{openError: nil, commandError: assert.AnError} - - assert.Panics(t, func() { - RegisterCommands(mockSess) - }, "RegisterCommands should panic when ApplicationCommandCreate returns an error") - })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
commands/handlers/main.go(1 hunks)commands/handlers/mentionEachHandler.go(1 hunks)commands/main.go(1 hunks)commands/main/register.go(1 hunks)commands/main/resgister_test.go(2 hunks)commands/register/register.go(1 hunks)commands/register/register_test.go(2 hunks)config/config.go(1 hunks)dtos/commands.go(1 hunks)go.mod(1 hunks)models/discord.go(1 hunks)models/discord_test.go(2 hunks)service/main.go(1 hunks)service/mentionEachService.go(1 hunks)tests/mocks/discordSessionMock.go(1 hunks)utils/constants.go(1 hunks)utils/membersUtils.go(1 hunks)utils/membersUtils_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
commands/register/register_test.go (1)
Learnt from: Devashish08
PR: Real-Dev-Squad/discord-service#80
File: utils/members_utils_test.go:28-37
Timestamp: 2025-04-11T16:51:24.677Z
Learning: The `ChannelMessageSend` mock implementation in `utils/members_utils_test.go` is necessary because it's part of the `DiscordSessionInterface` interface in the Discord service. In Go, all methods of an interface must be implemented for a mock to properly satisfy the interface, even if some methods aren't directly used in the current tests.
commands/main/resgister_test.go (1)
Learnt from: Devashish08
PR: Real-Dev-Squad/discord-service#80
File: utils/members_utils_test.go:28-37
Timestamp: 2025-04-11T16:51:24.677Z
Learning: The `ChannelMessageSend` mock implementation in `utils/members_utils_test.go` is necessary because it's part of the `DiscordSessionInterface` interface in the Discord service. In Go, all methods of an interface must be implemented for a mock to properly satisfy the interface, even if some methods aren't directly used in the current tests.
tests/mocks/discordSessionMock.go (1)
Learnt from: Devashish08
PR: Real-Dev-Squad/discord-service#80
File: utils/members_utils_test.go:28-37
Timestamp: 2025-04-11T16:51:24.677Z
Learning: The `ChannelMessageSend` mock implementation in `utils/members_utils_test.go` is necessary because it's part of the `DiscordSessionInterface` interface in the Discord service. In Go, all methods of an interface must be implemented for a mock to properly satisfy the interface, even if some methods aren't directly used in the current tests.
🧬 Code Graph Analysis (7)
commands/handlers/main.go (2)
utils/constants.go (1)
CommandNames(11-16)service/main.go (1)
CS(14-14)
service/main.go (2)
utils/constants.go (1)
CommandNames(11-16)commands/handlers/main.go (1)
CS(18-18)
utils/constants.go (1)
dtos/commands.go (1)
CommandNameTypes(3-8)
commands/handlers/mentionEachHandler.go (3)
models/discord.go (2)
SessionInterface(33-40)SessionWrapper(5-7)utils/membersUtils.go (4)
GetUsersWithRole(12-55)FormatUserListResponse(79-92)FormatMentionResponse(69-77)FormatUserMentions(57-67)commands/handlers/main.go (2)
CommandHandler(14-16)CreateSession(44-57)
utils/membersUtils.go (2)
models/discord.go (1)
SessionInterface(33-40)utils/constants.go (1)
DISCORD_GUILD_MEMBER_API_LIMIT(8-8)
utils/membersUtils_test.go (3)
tests/mocks/discordSessionMock.go (1)
DiscordSession(12-14)utils/constants.go (1)
DISCORD_GUILD_MEMBER_API_LIMIT(8-8)utils/membersUtils.go (4)
GetUsersWithRole(12-55)FormatUserMentions(57-67)FormatMentionResponse(69-77)FormatUserListResponse(79-92)
commands/main.go (1)
utils/constants.go (1)
CommandNames(11-16)
🔇 Additional comments (24)
commands/main/register.go (1)
33-33: Good fix for the method name typo.Correcting
GetUerId()toGetUserId()ensures proper user ID retrieval during application command creation, which is essential for Discord API integration. This correction maintains consistency with corresponding method signature fixes in other files.commands/register/register.go (1)
35-35: Typo fix: method name corrected from GetUerId to GetUserIdThe method name has been correctly updated to match the standardized interface method name across the codebase. This ensures consistency and proper function calls.
service/main.go (1)
25-26: Properly integrated MentionEach command into the service routerThe MentionEach command case has been added to the switch statement following the existing pattern in the codebase. This properly integrates the new command functionality into the service layer.
commands/handlers/main.go (1)
31-32: Properly integrated MentionEach command into the handler routerThe MentionEach command case has been correctly added to the MainHandler function, following the established pattern for routing commands to their specific handlers. This implementation properly integrates with the asynchronous message processing system.
models/discord_test.go (3)
10-11: Good practice: replaced external dependency with a local constantUsing a local constant rather than an external dependency reduces coupling and makes the test more maintainable.
13-13: Updated command name reference to use local constantThis change correctly utilizes the newly defined local constant.
44-54: Added test coverage for new session interface methodsTest cases for the newly added GuildMembers and ChannelMessageSend methods have been properly implemented, ensuring the interface remains fully tested. The implementation follows the same pattern as the existing tests, asserting that unimplemented methods panic when called on the dummy session wrapper.
utils/constants.go (2)
5-9: Appropriate constant grouping and new API limit constant added.The constant grouping provides good organization, and the new
DISCORD_GUILD_MEMBER_API_LIMITconstant with value 1000 properly captures Discord's API pagination limit for guild member requests.
15-15: New command registration looks good.The "mention-each" command is correctly added to the CommandNames struct, consistent with the PR objective of implementing the /mention-each command handler.
commands/register/register_test.go (3)
40-43: Test mock fields properly track the new methods.These fields appropriately track calls to the newly added methods and allow simulating errors for comprehensive test coverage.
62-65: Method name correctly fixed.Fixed the typo in method name from
GetUerIdtoGetUserId, maintaining consistency with the interface update in models/discord.go.
67-74: New mock methods properly implemented.The mock implementations for
GuildMembersandChannelMessageSendcorrectly set tracking flags and return configured errors, providing the necessary support for testing the new mention-each command functionality.commands/main/resgister_test.go (3)
40-43: Test mock fields properly track the new methods.These fields appropriately track calls to the newly added methods and allow simulating errors for comprehensive test coverage.
62-65: Method name correctly fixed.Fixed the typo in method name from
GetUerIdtoGetUserId, maintaining consistency with the interface update in models/discord.go.
67-75: New mock methods properly implemented.The mock implementations for
GuildMembersandChannelMessageSendcorrectly set tracking flags and return configured errors, supporting the testing of the new mention-each command functionality.models/discord.go (3)
21-23: Method name corrected for consistency.Fixed the typo in method name from
GetUerIdtoGetUserIdfor consistency across the codebase.
25-31: New session wrapper methods properly implemented.The methods
GuildMembersandChannelMessageSendproperly delegate to the underlying discordgo Session, providing the functionality needed for the mention-each command to fetch guild members and send messages.
37-39: Session interface appropriately extended.The interface now includes the corrected
GetUserIdmethod and the newly addedGuildMembersandChannelMessageSendmethods, ensuring all implementations will provide these required functions.commands/handlers/mentionEachHandler.go (1)
120-178: Consider guarding against Discord’s 2000-character limitBatching at 5 users is safe, but individual messages (
params.Message + mention) or the final summary may still exceed 2000 characters ifparams.Messageis lengthy. A quick length check beforeChannelMessageSendwould prevent runtime Discord errors.Would you like a helper that chunks overly long messages automatically?
utils/membersUtils.go (1)
57-66: 🧹 Nitpick (assertive)Tiny nit: pre-allocate string slice with exact capacity
You already know the exact length (
len(members)), so pre-allocating is great.
For readability you can inline the literal:-mentions := make([]string, 0, len(members)) +mentions := make([]string, len(members)) for i, member := range members { - if member.User != nil { - mentions = append(mentions, fmt.Sprintf("<@%s>", member.User.ID)) + if member.User != nil { + mentions[i] = fmt.Sprintf("<@%s>", member.User.ID) } else { ...This avoids the bounds checks on each
append.
Not critical but an easy micro-optimisation.Likely an incorrect or invalid review comment.
commands/main.go (1)
47-60: 🧹 Nitpick (assertive)Slash-command option names: avoid underscores for better UX
Discord’s CLI autocompleter treats dashes (
-) more naturally than underscores (_).
Consider renamingdev_title→dev-title(and the corresponding handler lookup) for consistency with common slash-command conventions.No code change needed for functionality, but improves discoverability.
⛔ Skipped due to learnings
Learnt from: Devashish08 PR: Real-Dev-Squad/discord-service#80 File: commands/main.go:56-57 Timestamp: 2025-04-10T14:08:41.301Z Learning: In the discord-service codebase, command option names consistently use lowercase for single words and snake_case for multi-word names (e.g., `dev_title`), not camelCase.config/config.go (1)
7-7: Approve import reorder.The import of
"os"has been moved within the block, but this only affects formatting and has no impact on functionality.go.mod (1)
19-19: Verify necessity of new indirect dependency.An indirect dependency on
github.com/stretchr/objx v0.5.2was added. Please confirm that this package is actually referenced in the codebase (e.g., in tests or mock utilities), and if not, remove it by runninggo mod tidy.dtos/commands.go (1)
7-7: EnsureMentionEachis initialized.The new
MentionEachfield inCommandNameTypesdefaults to an empty string. Verify that it’s assigned the"mention-each"command name in your configuration or initializer; otherwise, downstream handlers relying on this value may fail.
| t.Run("SessionWrapper should always implement getUerId() method", func(t *testing.T) { | ||
| assert.Panics(t, func() { | ||
| sessionWrapper.GetUerId() | ||
| sessionWrapper.GetUserId() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Method name corrected from getUerId to GetUserId
The method name has been properly updated to match the standardized interface method name used throughout the codebase.
Consider also updating the error message on line 42 from "getUerId" to "GetUserId" for complete consistency:
- }, "should panic when getUerId() is called")
+ }, "should panic when GetUserId() is called")| type DiscordSession struct { | ||
| mock.Mock | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add compile-time interface assertion to catch drift early
If the SessionInterface ever changes, we’ll discover it at compile time instead of at runtime when the tests start failing with an opaque mock error.
type DiscordSession struct {
mock.Mock
}
+
+// Assert implementation at compile-time
+var _ models.SessionInterface = (*DiscordSession)(nil)-import (
- "fmt"
- "github.com/bwmarrin/discordgo"
- "github.com/stretchr/testify/mock"
-)
+import (
+ "fmt"
+ "github.com/Real-Dev-Squad/discord-service/models"
+ "github.com/bwmarrin/discordgo"
+ "github.com/stretchr/testify/mock"
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type DiscordSession struct { | |
| mock.Mock | |
| } | |
| import ( | |
| "fmt" | |
| "github.com/Real-Dev-Squad/discord-service/models" | |
| "github.com/bwmarrin/discordgo" | |
| "github.com/stretchr/testify/mock" | |
| ) | |
| type DiscordSession struct { | |
| mock.Mock | |
| } | |
| // Assert implementation at compile-time | |
| var _ models.SessionInterface = (*DiscordSession)(nil) |
| import ( | ||
| // Import the actual interface definition | ||
|
|
||
| "fmt" | ||
|
|
||
| "github.com/bwmarrin/discordgo" | ||
| "github.com/stretchr/testify/mock" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prune placeholder comment & let goimports manage spacing
The “Import the actual interface definition” comment and the blank lines inside the import block are stale and break the canonical goimports layout. Keeping the imports tightly grouped prevents accidental merge conflicts and lets the formatter do its job.
-import (
- // Import the actual interface definition
-
- "fmt"
-
- "github.com/bwmarrin/discordgo"
- "github.com/stretchr/testify/mock"
-)
+import (
+ "fmt"
+
+ "github.com/bwmarrin/discordgo"
+ "github.com/stretchr/testify/mock"
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ( | |
| // Import the actual interface definition | |
| "fmt" | |
| "github.com/bwmarrin/discordgo" | |
| "github.com/stretchr/testify/mock" | |
| ) | |
| import ( | |
| "fmt" | |
| "github.com/bwmarrin/discordgo" | |
| "github.com/stretchr/testify/mock" | |
| ) |
| package utils | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/Real-Dev-Squad/discord-service/tests/mocks" | ||
| "github.com/bwmarrin/discordgo" | ||
| "github.com/stretchr/testify/assert" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Run tests in parallel for faster feedback
None of the sub-tests share state, and each creates its own mock. Marking the top-level test functions with t.Parallel() speeds up CI without additional risk.
-func TestGetUsersWithRole(t *testing.T) {
+func TestGetUsersWithRole(t *testing.T) {
+ t.Parallel()Repeat the pattern for the other top-level test functions (TestFormatUserMentions, TestFormatMentionResponse, TestFormatUserListResponse).
| userMessage := metaData["message"] | ||
| if len(userMessage) > MaxUserMessageLength { | ||
| truncatedSuffix := "..." | ||
| logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength) | ||
| userMessage = userMessage[:MaxUserMessageLength] + truncatedSuffix | ||
| } | ||
| params.Message = userMessage |
There was a problem hiding this comment.
Truncation can overshoot the advertised limit
You slice to MaxUserMessageLength and then append "...", yielding a final length of MaxUserMessageLength + 3. This defeats the purpose of the guard.
- if len(userMessage) > MaxUserMessageLength {
- truncatedSuffix := "..."
- logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength)
- userMessage = userMessage[:MaxUserMessageLength] + truncatedSuffix
+ if len(userMessage) > MaxUserMessageLength {
+ const truncatedSuffix = "..."
+ cutoff := MaxUserMessageLength - len(truncatedSuffix)
+ if cutoff < 0 {
+ cutoff = 0
+ }
+ logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength)
+ userMessage = userMessage[:cutoff] + truncatedSuffix
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| userMessage := metaData["message"] | |
| if len(userMessage) > MaxUserMessageLength { | |
| truncatedSuffix := "..." | |
| logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength) | |
| userMessage = userMessage[:MaxUserMessageLength] + truncatedSuffix | |
| } | |
| params.Message = userMessage | |
| userMessage := metaData["message"] | |
| if len(userMessage) > MaxUserMessageLength { | |
| const truncatedSuffix = "..." | |
| cutoff := MaxUserMessageLength - len(truncatedSuffix) | |
| if cutoff < 0 { | |
| cutoff = 0 | |
| } | |
| logrus.Warnf("User provided message length (%d) exceeds limit (%d). Truncating.", len(userMessage), MaxUserMessageLength) | |
| userMessage = userMessage[:cutoff] + truncatedSuffix | |
| } | |
| params.Message = userMessage |
| func GetUsersWithRole(session models.SessionInterface, guildID string, roleID string) ([]discordgo.Member, error) { | ||
| var membersWithRole []discordgo.Member | ||
| lastMemberID := "" | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer pointer slices to avoid unnecessary memory copies
membersWithRole is declared as []discordgo.Member, causing every matched member object to be copied into the slice.
Because discordgo.Member contains nested structs and pointers, the copy can be relatively expensive for large guilds.
-var membersWithRole []discordgo.Member
+var membersWithRole []*discordgo.MemberYou would then append member directly (without the * dereference) and adjust the return type.
Down-stream helpers (FormatUserMentions, FormatUserListResponse) would need a tiny signature tweak but will benefit from reduced allocations.
Committable suggestion skipped: line range outside the PR's diff.
| foundInChunk := 0 | ||
| lastIdInChunk := "" | ||
| for _, member := range membersChunk { | ||
| if member.User == nil { | ||
| logrus.Warnf("Guild %s: Member object or User data is nil, skipping member: %+v", guildID, member) | ||
| continue | ||
| } | ||
| for _, r := range member.Roles { | ||
| if r == roleID { | ||
| membersWithRole = append(membersWithRole, *member) | ||
| foundInChunk++ | ||
| break | ||
| } | ||
| } | ||
|
|
||
| lastIdInChunk = member.User.ID | ||
| } | ||
| lastMemberID = lastIdInChunk | ||
| logrus.Debugf("Found %d members with role %s in this chunk (Guild %s)", foundInChunk, roleID, guildID) |
There was a problem hiding this comment.
Potential infinite-loop when entire chunk has nil users
lastIdInChunk is updated only inside the loop and after the nil-user check.
If a chunk contains only entries with member.User == nil, lastIdInChunk remains the previous value, lastMemberID is unchanged, and the next GuildMembers call retrieves the very same page → infinite loop.
- for _, member := range membersChunk {
+ for _, member := range membersChunk {
if member.User == nil {
...
continue
}
...
lastIdInChunk = member.User.ID
}
- lastMemberID = lastIdInChunk
+ if lastIdInChunk == "" {
+ // no progress possible – bail out to avoid infinite loop
+ break
+ }
+ lastMemberID = lastIdInChunkConsider early-breaking as shown or always advancing lastMemberID with the last raw member to guarantee progress.
| roleID, ok := roleOption.Value.(string) | ||
| if !ok { | ||
| errorMsg := "Invalid role format(not a String)" | ||
| logrus.Errorf("%s: Expected string, got %T", errorMsg, roleOption.Value) | ||
| sendErrorResponse(response, errorMsg) | ||
| return | ||
| } |
There was a problem hiding this comment.
Bug: roleID is shadowed, producing an empty value downstream
Using := redeclares roleID inside this block, leaving the outer roleID (used later for logs, metadata, and response content) unchanged and still empty.
Consequently the function will hit the “empty ID” check and return an error.
- roleID, ok := roleOption.Value.(string)
+ var ok bool
+ roleID, ok = roleOption.Value.(string)Or, clearer:
roleIDStr, ok := roleOption.Value.(string)
if !ok { ... }
roleID = roleIDStrMake sure accompanying unit tests cover this path.
| logrus.Infof("Mention-each options: role=%s, message=%s, dev=%v, devTitle=%v", roleID, message, dev, devTitle) | ||
|
|
||
| var responseContent string | ||
| if devTitle { | ||
| responseContent = fmt.Sprintf("Fetching users with the <@&%s>", roleID) | ||
| } else if dev { | ||
| responseContent = fmt.Sprintf("Sending individual mentions to users with the <@&%s>", roleID) | ||
| } else { | ||
| responseContent = fmt.Sprintf("Mentioning all users with the <@&%s>", roleID) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider re-phrasing user feedback for clarity
Responses like
"Mentioning all users with the <@&123456>"
lack the word “role” and may confuse end-users. A tiny wording tweak adds clarity:
- responseContent = fmt.Sprintf("Mentioning all users with the <@&%s>", roleID)
+ responseContent = fmt.Sprintf("Mentioning all users with the <@&%s> role", roleID)Same for the other branches.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logrus.Infof("Mention-each options: role=%s, message=%s, dev=%v, devTitle=%v", roleID, message, dev, devTitle) | |
| var responseContent string | |
| if devTitle { | |
| responseContent = fmt.Sprintf("Fetching users with the <@&%s>", roleID) | |
| } else if dev { | |
| responseContent = fmt.Sprintf("Sending individual mentions to users with the <@&%s>", roleID) | |
| } else { | |
| responseContent = fmt.Sprintf("Mentioning all users with the <@&%s>", roleID) | |
| } | |
| logrus.Infof("Mention-each options: role=%s, message=%s, dev=%v, devTitle=%v", roleID, message, dev, devTitle) | |
| var responseContent string | |
| if devTitle { | |
| responseContent = fmt.Sprintf("Fetching users with the <@&%s>", roleID) | |
| } else if dev { | |
| responseContent = fmt.Sprintf("Sending individual mentions to users with the <@&%s>", roleID) | |
| } else { | |
| - responseContent = fmt.Sprintf("Mentioning all users with the <@&%s>", roleID) | |
| + responseContent = fmt.Sprintf("Mentioning all users with the <@&%s> role", roleID) | |
| } |
| metaData := map[string]string{ | ||
| "role_id": roleID, | ||
| "message": message, | ||
| "dev": fmt.Sprintf("%v", dev), | ||
| "dev_title": fmt.Sprintf("%v", devTitle), | ||
| "guild_id": s.discordMessage.Data.GuildId, | ||
| "channel_id": s.discordMessage.ChannelId, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Store booleans as booleans in metadata
metaData converts every value to string.
If downstream consumers ever need to read dev / dev_title as booleans they will have to parse the string again.
- "dev": fmt.Sprintf("%v", dev),
- "dev_title": fmt.Sprintf("%v", devTitle),
+ "dev": strconv.FormatBool(dev),
+ "dev_title": strconv.FormatBool(devTitle),(Standard strconv is already in fmt’s transitive deps.)
Not a blocker, but preserves type information.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| metaData := map[string]string{ | |
| "role_id": roleID, | |
| "message": message, | |
| "dev": fmt.Sprintf("%v", dev), | |
| "dev_title": fmt.Sprintf("%v", devTitle), | |
| "guild_id": s.discordMessage.Data.GuildId, | |
| "channel_id": s.discordMessage.ChannelId, | |
| } | |
| metaData := map[string]string{ | |
| "role_id": roleID, | |
| "message": message, | |
| "dev": strconv.FormatBool(dev), | |
| "dev_title": strconv.FormatBool(devTitle), | |
| "guild_id": s.discordMessage.Data.GuildId, | |
| "channel_id": s.discordMessage.ChannelId, | |
| } |
| type CommandParams struct { | ||
| RoleID string | ||
| ChannelID string | ||
| GuildID string | ||
| Message string | ||
| Dev bool | ||
| DevTitle bool | ||
| } |
There was a problem hiding this comment.
This is particular to this command only?
There was a problem hiding this comment.
yes commandParams defined here hold the specific parameters needed only for the mentionEach command
| const ( | ||
| BatchSize = 5 | ||
| BatchDelay = 1 * time.Second | ||
| MaxUserMessageLength = 1000 | ||
| ) | ||
|
|
There was a problem hiding this comment.
Can this be reused for other commands?
There was a problem hiding this comment.
dont know that for now have to see usecases for other command for that
| ) | ||
|
|
||
| func (s *CommandHandler) mentionEachHandler() error { | ||
| logrus.Info("Processing mention-each command") |
There was a problem hiding this comment.
this is not required but put there for easy debugging and tracing
| } | ||
|
|
||
| var ( | ||
| extractCommandParamsFunc = func(metaData map[string]string) (CommandParams, error) { |
There was a problem hiding this comment.
Put better name for this function.
Also this is something which is common across other commands. we should create this as util function.
2 participants
Date: 29/04/2025
Developer Name: @Devashish08
Issue Ticket Number
Closes #70
Description
Notes for Reviewers:
commands/handlers/directory:commands/handlers/mentionEachHandler.go(New file / main logic)commands/handlers/main.go(Changes to theswitchstatement inMainHandler)utils/,service/,dtos/, etc., originate from previous PRs.This PR implements the background handler logic for the
/mention-eachcommand within thecommands/handlerspackage. This code executes asynchronously after being dequeued from RabbitMQ, performing the core tasks of fetching members and sending the appropriate mention/list messages back to Discord.Builds Upon: #80
Related PR: #93
Testing:
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Test Coverage
Additional Notes