feat(service): Add service layer for /mention-each command#93
feat(service): Add service layer for /mention-each command#93Devashish08 wants to merge 17 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 a new Discord slash command, "MentionEach", which allows mentioning all users with a specified role, with several options for message customization and feature toggling. The implementation includes the addition of the command declaration, its service handler, and supporting utilities for querying and formatting user mentions. The Discord session interface and its mock/test implementations are extended to support the necessary API calls. Comprehensive unit tests and a new mock for Discord sessions are also included. Minor refactoring, typo corrections, and import order adjustments are made for consistency and correctness. Changes
Sequence Diagram(s)sequenceDiagram
participant DiscordUser
participant DiscordAPI
participant MainService
participant CommandService
participant Utils
participant Queue
DiscordUser->>DiscordAPI: Issues /mention-each command
DiscordAPI->>MainService: Forwards interaction event
MainService->>CommandService: Calls MentionEachService
CommandService->>CommandService: Validates and parses options
CommandService->>Utils: Finds users with specified role
Utils-->>CommandService: Returns user list / mentions
CommandService->>Queue: Enqueues DataPacket for async processing
Queue-->>CommandService: Confirms enqueue
CommandService->>DiscordAPI: Responds with confirmation or error
DiscordAPI-->>DiscordUser: Shows result message
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: 7
🔭 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. Both have the same name and are testing the same condition.
- 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 (16)
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/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.
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.
🧬 Code Graph Analysis (5)
service/main.go (2)
utils/constants.go (1)
CommandNames(11-16)commands/handlers/main.go (1)
CS(18-18)
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)
utils/membersUtils.go (2)
models/discord.go (1)
SessionInterface(33-40)utils/constants.go (1)
DISCORD_GUILD_MEMBER_API_LIMIT(8-8)
utils/constants.go (1)
dtos/commands.go (1)
CommandNameTypes(3-8)
commands/main.go (1)
utils/constants.go (1)
CommandNames(11-16)
🔇 Additional comments (30)
config/config.go (1)
4-8: Import reorder is fine.The
ospackage import has been moved within the block without any functional impact.dtos/commands.go (1)
7-7: Looking good - addition of MentionEach command constant.The new
MentionEachfield in the CommandNameTypes struct aligns with the PR objective of adding support for the/mention-eachcommand. This field will be used to reference the command across the codebase consistently.go.mod (1)
19-19: Addition of stretchr/objx dependency looks appropriate.The indirect dependency on
github.com/stretchr/objx v0.5.2is likely pulled in to support mock objects for testing the new functionality. This is a common dependency used with the testify framework for creating mocks.commands/register/register.go (1)
35-35: Good catch - fixed typo in method name.Corrected the method name from
GetUerId()toGetUserId(), ensuring consistent method naming throughout the codebase. This improves code quality and avoids potential runtime errors.commands/main/register.go (1)
33-33: Good catch - fixed method name here as well.Corrected the method name from
GetUerId()toGetUserId()for consistency with the rest of the codebase, matching the fix made incommands/register/register.go. This ensures proper functioning of command registration.service/main.go (1)
25-26: Addition of MentionEach handler looks correct!The integration of the new
/mention-eachcommand follows the established pattern for command routing. This cleanly connects the command defined incommands/main.goto its handler implementation.utils/constants.go (2)
5-9: Good constant organizationConsolidating related constants into a single block improves code organization and readability. The
DISCORD_GUILD_MEMBER_API_LIMITof 1000 appears to align with Discord's API pagination limits.
11-16: Command name addition looks correctThe addition of the
MentionEachfield to theCommandNamesmap properly aligns with the command structure defined incommands/main.goand matches the DTO structure indtos/commands.go.models/discord_test.go (3)
10-11: Good refactoring to local constantReplacing the external package dependency with a local constant reduces coupling and makes the test more self-contained.
13-14: Correctly updated command referenceUpdated the command reference to use the local constant.
44-54: Good test coverage for new methodsThe tests for
GuildMembers()andChannelMessageSend()follow the established pattern and ensure that these methods panic when called on the dummy session wrapper, maintaining consistency with the other method tests.commands/main.go (1)
29-68: Well-structured command definition with clear documentationThe new
MentionEachcommand is well-defined with appropriate options and descriptive comments. The inclusion of a feature flag (ff_enabled) is a good practice for controlling access to new features.A few observations:
- The role option is properly set as required
- The message option is correctly set as optional string
- The dev flags have clear descriptions
- The feature flag implementation allows for controlled rollout
This command structure aligns well with the PR objective of implementing the service layer for the
/mention-eachcommand.commands/register/register_test.go (3)
40-43: LGTM: Additional mock tracking fields added for new session methods.The new fields added to the
mockSessionstruct properly track calls to theChannelMessageSendandGuildMembersmethods and their error returns. This aligns with the other tracking fields in the mock.
62-65: Typo correction in method name.The method was renamed from
GetUerIdtoGetUserId, fixing a typo in the method name. This improves consistency across the codebase.
67-74: LGTM: Mock implementations for new session interface methods.These new methods properly implement the
GuildMembersandChannelMessageSendmethods required by theSessionInterface. The implementation follows the existing pattern of setting the tracking flag and returning the configured error.commands/main/resgister_test.go (3)
40-43: LGTM: Additional mock tracking fields added.The new fields properly track calls to
ChannelMessageSendandGuildMembersmethods and their error returns, consistent with other tracking fields in the mock.
62-65: Typo correction in method name.The method was renamed from
GetUerIdtoGetUserId, fixing a typo and improving consistency across the codebase.
67-75: LGTM: Mock implementations for new session interface methods.These implementations properly satisfy the
SessionInterfacerequirements, allowing for testing of code that depends on these methods.models/discord.go (3)
21-23: Typo correction in method name.The method was renamed from
GetUerIdtoGetUserId, fixing a typo and improving consistency across the codebase.
25-31: LGTM: New session wrapper methods.These methods properly wrap the underlying Discord session methods for retrieving guild members and sending channel messages, which are needed for the new
/mention-eachcommand functionality.
37-39: LGTM: Updated interface with new method signatures.The
SessionInterfacehas been updated with the corrected method name and new methods required for the/mention-eachcommand functionality.tests/mocks/discordSessionMock.go (1)
1-70: LGTM: Comprehensive Discord session mock implementation.This new mock implementation using the
testify/mockpackage provides a more sophisticated approach to mocking the Discord session for testing. Key strengths:
- Uses the established mock pattern from testify
- Properly records method calls and arguments
- Includes type assertions with descriptive error messages
- Implements all required methods of the
SessionInterfaceThis will make testing Discord session interactions more robust, especially for the new
/mention-eachcommand functionality.service/mentionEachService.go (2)
145-154: LGTM! Error response handling is well-implemented.The error response function correctly uses ephemeral messages to show errors only to the user who triggered the command.
52-58:⚠️ Potential issueFix undeclared variable 'ok'
There's a reference to an undeclared variable "ok" on line 52. You need to declare this variable before using it.
- roleID, ok := roleOption.Value.(string) + var ok bool + roleID, ok = roleOption.Value.(string)Likely an incorrect or invalid review comment.
utils/membersUtils_test.go (3)
106-144: Excellent pagination testingThe pagination test is comprehensive and well-structured, covering multiple pages of members and ensuring the correct behavior when reaching the end of available members.
164-167: LGTM! Good edge case handlingTesting with nil member lists is important as it could happen in production and lead to bugs if not handled properly.
168-177: LGTM! Thorough null checkingThe test for handling members with nil User is important as it verifies the robustness of the utility function.
utils/membersUtils.go (3)
57-67: LGTM! Robust handling of nil User referencesThe function correctly checks for nil User references and provides appropriate logging, which is important for robustness in production.
79-92: LGTM! Clean implementation with good user experienceThe function elegantly handles different cases with specific, user-friendly messages for each scenario.
32-48:⚠️ Potential issuePotential nil pointer in lastIdInChunk assignment
There's a potential nil pointer dereference when assigning
lastIdInChunk. If a member with a nil User reference is the last member in the chunk, line 47 will panic.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 + // We've already checked member.User != nil above + lastIdInChunk = member.User.ID }To fix this issue, make sure the code correctly handles nil User references throughout the chunk processing.
Likely an incorrect or invalid review comment.
| sessionWrapper.GetUserId() | ||
| }, "should panic when getUerId() is called") |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Method name typo fixed
The method name has been corrected from GetUerId() to GetUserId(), though the test description in line 39 and error message in line 42 still refer to "getUerId". Consider updating these for consistency.
- t.Run("SessionWrapper should always implement getUerId() method", func(t *testing.T) {
+ t.Run("SessionWrapper should always implement GetUserId() method", func(t *testing.T) {
assert.Panics(t, func() {
sessionWrapper.GetUserId()
- }, "should panic when getUerId() is called")
+ }, "should panic when GetUserId() is called")📝 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.
| sessionWrapper.GetUserId() | |
| }, "should panic when getUerId() is called") | |
| t.Run("SessionWrapper should always implement GetUserId() method", func(t *testing.T) { | |
| assert.Panics(t, func() { | |
| sessionWrapper.GetUserId() | |
| }, "should panic when GetUserId() is called") |
| if err := queue.SendMessage(bytePacket); err != nil { | ||
| errorMsg := "Failed to process your request" | ||
| logrus.Errorf("Failed to enqueue message: %v", err) | ||
| sendErrorResponse(response, errorMsg) | ||
| return | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing error message for queue failure
The current error message for queue failure is generic. Consider adding more context to help users understand what happened.
if err := queue.SendMessage(bytePacket); err != nil {
- errorMsg := "Failed to process your request"
+ errorMsg := "Failed to queue your request for processing. Please try again later."
logrus.Errorf("Failed to enqueue message: %v", err)
sendErrorResponse(response, errorMsg)
return
}| func (s *CommandService) MentionEachService(response http.ResponseWriter, request *http.Request) { | ||
| logrus.Info("Mention-each command received") | ||
|
|
||
| if s.discordMessage == nil || s.discordMessage.Data == nil || | ||
| s.discordMessage.Member == nil || s.discordMessage.Member.User == nil { | ||
| errorMsg := "Invalid request data" | ||
| logrus.Error(errorMsg) | ||
| sendErrorResponse(response, errorMsg) | ||
| return | ||
| } | ||
| var featureEnabled bool | ||
| ffOption := findOption(s.discordMessage.Data.Options, "ff_enabled") | ||
| if ffOption != nil && ffOption.Value != nil { | ||
| var ok bool | ||
| featureEnabled, ok = ffOption.Value.(bool) | ||
| if !ok { | ||
| logrus.Warnf("Invalid non-boolean value provided for 'ff_enabled' flag: Type=%T, Value=%v. Treating as disabled.", ffOption.Value, ffOption.Value) | ||
| featureEnabled = false | ||
| } | ||
| } | ||
|
|
||
| if !featureEnabled { | ||
| logrus.Warnf("Mention-each command run without 'ff_enabled:true'. User: %s", s.discordMessage.Member.User.ID) | ||
| sendErrorResponse(response, "Sorry, the /mention-each command requires the `ff_enabled:True` option to run currently.") | ||
| return | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding panic recovery
As a critical service function handling Discord interactions, it would be beneficial to add panic recovery to prevent unexpected crashes.
func (s *CommandService) MentionEachService(response http.ResponseWriter, request *http.Request) {
+ defer func() {
+ if r := recover(); r != nil {
+ logrus.Errorf("Recovered from panic in MentionEachService: %v", r)
+ sendErrorResponse(response, "An unexpected error occurred")
+ }
+ }()
+
logrus.Info("Mention-each command received")📝 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.
| func (s *CommandService) MentionEachService(response http.ResponseWriter, request *http.Request) { | |
| logrus.Info("Mention-each command received") | |
| if s.discordMessage == nil || s.discordMessage.Data == nil || | |
| s.discordMessage.Member == nil || s.discordMessage.Member.User == nil { | |
| errorMsg := "Invalid request data" | |
| logrus.Error(errorMsg) | |
| sendErrorResponse(response, errorMsg) | |
| return | |
| } | |
| var featureEnabled bool | |
| ffOption := findOption(s.discordMessage.Data.Options, "ff_enabled") | |
| if ffOption != nil && ffOption.Value != nil { | |
| var ok bool | |
| featureEnabled, ok = ffOption.Value.(bool) | |
| if !ok { | |
| logrus.Warnf("Invalid non-boolean value provided for 'ff_enabled' flag: Type=%T, Value=%v. Treating as disabled.", ffOption.Value, ffOption.Value) | |
| featureEnabled = false | |
| } | |
| } | |
| if !featureEnabled { | |
| logrus.Warnf("Mention-each command run without 'ff_enabled:true'. User: %s", s.discordMessage.Member.User.ID) | |
| sendErrorResponse(response, "Sorry, the /mention-each command requires the `ff_enabled:True` option to run currently.") | |
| return | |
| } | |
| func (s *CommandService) MentionEachService(response http.ResponseWriter, request *http.Request) { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| logrus.Errorf("Recovered from panic in MentionEachService: %v", r) | |
| sendErrorResponse(response, "An unexpected error occurred") | |
| } | |
| }() | |
| logrus.Info("Mention-each command received") | |
| if s.discordMessage == nil || s.discordMessage.Data == nil || | |
| s.discordMessage.Member == nil || s.discordMessage.Member.User == nil { | |
| errorMsg := "Invalid request data" | |
| logrus.Error(errorMsg) | |
| sendErrorResponse(response, errorMsg) | |
| return | |
| } | |
| var featureEnabled bool | |
| ffOption := findOption(s.discordMessage.Data.Options, "ff_enabled") | |
| if ffOption != nil && ffOption.Value != nil { | |
| var ok bool | |
| featureEnabled, ok = ffOption.Value.(bool) | |
| if !ok { | |
| logrus.Warnf("Invalid non-boolean value provided for 'ff_enabled' flag: Type=%T, Value=%v. Treating as disabled.", ffOption.Value, ffOption.Value) | |
| featureEnabled = false | |
| } | |
| } | |
| if !featureEnabled { | |
| logrus.Warnf("Mention-each command run without 'ff_enabled:true'. User: %s", s.discordMessage.Member.User.ID) | |
| sendErrorResponse(response, "Sorry, the /mention-each command requires the `ff_enabled:True` option to run currently.") | |
| return | |
| } | |
| // ... rest of implementation ... | |
| } |
| 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)
Extract response content creation into a separate function
The response content creation logic could be extracted to improve readability and maintainability.
+func constructResponseContent(roleID string, devTitle bool, dev bool) string {
+ if devTitle {
+ return fmt.Sprintf("Fetching users with the <@&%s>", roleID)
+ } else if dev {
+ return fmt.Sprintf("Sending individual mentions to users with the <@&%s>", roleID)
+ } else {
+ return fmt.Sprintf("Mentioning all users with the <@&%s>", roleID)
+ }
+}
+
func (s *CommandService) MentionEachService(response http.ResponseWriter, request *http.Request) {
// ... existing code ...
- 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 := constructResponseContent(roleID, devTitle, dev)📝 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.
| 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) | |
| } | |
| // service/mentionEachService.go | |
| // extract the response‐content logic into its own function | |
| func constructResponseContent(roleID string, devTitle bool, dev bool) string { | |
| if devTitle { | |
| return fmt.Sprintf("Fetching users with the <@&%s>", roleID) | |
| } else if dev { | |
| return fmt.Sprintf("Sending individual mentions to users with the <@&%s>", roleID) | |
| } else { | |
| return fmt.Sprintf("Mentioning all users with the <@&%s>", roleID) | |
| } | |
| } | |
| func (s *CommandService) MentionEachService(response http.ResponseWriter, request *http.Request) { | |
| // ... existing code ... | |
| // replace the inline if/else block with a call into our new helper | |
| responseContent := constructResponseContent(roleID, devTitle, dev) | |
| // ... remainder of the method ... | |
| } |
|
|
||
| t.Run("handles nil mentions", func(t *testing.T) { | ||
| response := FormatUserListResponse(nil, roleID) | ||
| expected := fmt.Sprintf("Found 0 users with the %s role", roleMention) | ||
| assert.Equal(t, expected, response) | ||
| }) | ||
|
|
||
| t.Run("handles empty role ID", func(t *testing.T) { | ||
| mentions := []string{"<@123>"} | ||
| emptyRoleMention := "<@&>" | ||
| response := FormatUserListResponse([]string{"<@123>"}, "") | ||
| expected := fmt.Sprintf("Found 1 user with the %s role: %s", emptyRoleMention, mentions[0]) | ||
| assert.Equal(t, expected, response) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding tests for large member lists
The current tests cover basic cases well, but consider adding tests for large member lists to ensure performance and formatting remain correct with many users.
t.Run("formats response with many users (performance test)", func(t *testing.T) {
// Create a slice with many mentions (e.g., 100+)
mentions := make([]string, 150)
for i := range mentions {
mentions[i] = fmt.Sprintf("<@user%d>", i)
}
response := FormatUserListResponse(mentions, roleID)
// Check that the first part of the response is correct
expected := fmt.Sprintf("Found %d users with the %s role:", len(mentions), roleMention)
assert.Contains(t, response, expected)
// Check that the response contains the first and last mention
assert.Contains(t, response, mentions[0])
assert.Contains(t, response, mentions[len(mentions)-1])
})| func FormatMentionResponse(mentions []string, message string) string { | ||
| mentionStrings := strings.Join(mentions, " ") | ||
| if message == "" { | ||
| return mentionStrings | ||
| } | ||
|
|
||
| return fmt.Sprintf("%s %s", message, mentionStrings) | ||
|
|
||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider handling case when mentions array is empty
The current implementation doesn't specifically handle the case when the mentions array is empty. While it works correctly (returning an empty string or just the message), explicit handling would improve readability.
func FormatMentionResponse(mentions []string, message string) string {
+ if len(mentions) == 0 {
+ return message
+ }
+
mentionStrings := strings.Join(mentions, " ")
if message == "" {
return mentionStrings
}
return fmt.Sprintf("%s %s", message, mentionStrings)
}📝 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.
| func FormatMentionResponse(mentions []string, message string) string { | |
| mentionStrings := strings.Join(mentions, " ") | |
| if message == "" { | |
| return mentionStrings | |
| } | |
| return fmt.Sprintf("%s %s", message, mentionStrings) | |
| } | |
| func FormatMentionResponse(mentions []string, message string) string { | |
| if len(mentions) == 0 { | |
| return message | |
| } | |
| mentionStrings := strings.Join(mentions, " ") | |
| if message == "" { | |
| return mentionStrings | |
| } | |
| return fmt.Sprintf("%s %s", message, mentionStrings) | |
| } |
| lastMemberID = lastIdInChunk | ||
| logrus.Debugf("Found %d members with role %s in this chunk (Guild %s)", foundInChunk, roleID, guildID) | ||
| } | ||
|
|
||
| logrus.Infof("Finished fetching. Found %d total members with role %s in guild %s", len(membersWithRole), roleID, guildID) | ||
| return membersWithRole, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add safety check for empty lastIdInChunk
If all members in a chunk have nil User fields, lastIdInChunk would remain empty, which could lead to fetching the same members repeatedly.
- lastMemberID = lastIdInChunk
+ if lastIdInChunk == "" {
+ logrus.Warnf("No valid user IDs found in chunk for guild %s, breaking pagination loop", guildID)
+ break
+ }
+ lastMemberID = lastIdInChunk📝 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.
| lastMemberID = lastIdInChunk | |
| logrus.Debugf("Found %d members with role %s in this chunk (Guild %s)", foundInChunk, roleID, guildID) | |
| } | |
| logrus.Infof("Finished fetching. Found %d total members with role %s in guild %s", len(membersWithRole), roleID, guildID) | |
| return membersWithRole, nil | |
| } | |
| if lastIdInChunk == "" { | |
| logrus.Warnf("No valid user IDs found in chunk for guild %s, breaking pagination loop", guildID) | |
| break | |
| } | |
| lastMemberID = lastIdInChunk | |
| logrus.Debugf("Found %d members with role %s in this chunk (Guild %s)", foundInChunk, roleID, guildID) | |
| } | |
| logrus.Infof("Finished fetching. Found %d total members with role %s in guild %s", len(membersWithRole), roleID, guildID) | |
| return membersWithRole, nil | |
| } |
1 participant
Date: 29 Feb 2025
Developer Name: @Devashish08
Issue Ticket Number
#70
Description
Notes for Reviewers:
developbranch.service/directory:service/mentionEachService.go(New file)service/main.go(Changes to theswitchstatement)command/main.go(add feature flag option)utils/,dtos/, orcommands/constants/originate from PR feat: Mention Each command registration and add members utils #80.This Pull Request implements the service layer for the
/mention-eachslash command. It handles the initial Discord interaction, performs validation, manages the feature flag check, prepares data for background processing, sends the job to the queue, and returns an immediate public acknowledgement to Discord.This is the second PR in a stack for the mention-each feature:
Testing:
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes
/mention-eachfeature flag as a command option (ff_enabled) as discussed.