feat: Mention Each command registration and add members utils #80
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Summary by CodeRabbit
WalkthroughThis pull request introduces a new command, MentionEach, to the Discord bot, enabling users to mention all members of a specified role. The implementation involves updates to the command definitions, the addition of a new command name type, and the introduction of utility functions for managing user mentions and formatting responses. Additionally, comprehensive unit tests have been created to validate the functionality of these new features. An indirect dependency has also been added to the project. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CH as Command Handler
participant DU as Discord Utils
participant API as Discord API
U->>CH: Sends "MentionEach" command with options
CH->>DU: Invoke GetUsersWithRole(guildID, roleID)
DU->>API: Request guild members for the given role
API-->>DU: Return list of members
DU->>CH: Return formatted mentions (via FormatUserMentions/FormatMentionResponse)
CH->>API: Send message(s) with role mentions
API-->>U: Display response message(s)
Poem
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: 5
📜 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 (6)
commands/main.go(1 hunks)dtos/commands.go(1 hunks)go.mod(1 hunks)utils/constants.go(1 hunks)utils/members_utils.go(1 hunks)utils/members_utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
commands/main.go (1)
utils/constants.go (1)
CommandNames(8-13)
utils/members_utils_test.go (1)
utils/members_utils.go (6)
GetUsersWithRole(14-29)FormatUserMentions(31-40)FormatRoleMention(42-44)JoinMentions(46-57)FormatMentionResponse(60-70)FormatDevTitleResponse(72-85)
🪛 golangci-lint (1.64.8)
utils/members_utils.go
47-47: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (1.22.4)
🔇 Additional comments (7)
dtos/commands.go (1)
7-7: LGTM: Addition of MentionEach fieldThe new
MentionEachfield is correctly added to theCommandNameTypesstruct and follows the existing naming conventions. This addition properly supports the new command functionality.go.mod (1)
19-19:Details
✅ Verification successful
Verify if the new indirect dependency is necessary
A new indirect dependency
github.com/stretchr/objx v0.5.2has been added to the go.mod file. Since it's marked as indirect, it's likely pulled in by another dependency (possibly testify for the new tests), but let's verify it's actually needed.
🏁 Script executed:
#!/bin/bash # Check if this dependency is actually used by anything in the project # First check for direct imports echo "Checking for direct imports of github.com/stretchr/objx:" rg "github.com/stretchr/objx" --type go # Check go.sum to see if it was added deliberately or automatically echo -e "\nChecking go.sum for the dependency:" grep "github.com/stretchr/objx" go.sum # Run go mod why to see which package(s) require this dependency echo -e "\nChecking dependency tree:" go mod why github.com/stretchr/objxLength of output: 1632
Update: The indirect dependency is required as intended.
After running the verification steps, we confirmed the following:
- No direct imports of
github.com/stretchr/objxwere found in the codebase.- The
go.sumentries reflect both v0.1.0 and v0.5.2, indicating its presence as a dependency.- The
go mod whyoutput shows the dependency is part of the transitive chain (pulled in viagithub.com/stretchr/testify/mockused in our tests).Given these observations, the indirect dependency is necessary and correctly marked as indirect. No changes are required here.
utils/constants.go (1)
12-12: LGTM: Addition of MentionEach command name constantThe new
MentionEachentry in theCommandNamesvariable is properly formatted using kebab-case consistent with other command names. This aligns well with the new field added to theCommandNameTypesstruct.commands/main.go (2)
29-62: LGTM: Well-structured MentionEach command implementationThe new MentionEach command is well-structured with clear options and descriptions. The implementation follows the established patterns for command definitions in the project.
29-62:Details
❓ Verification inconclusive
Consider adding permission checks for role mention functionality
The MentionEach command allows mentioning all users with a specific role, which could potentially be misused (especially in large servers). Consider adding permission checks to restrict who can use this command.
🏁 Script executed:
#!/bin/bash # Check if there are any permission checks in the command handler for this new command # Look for the command handler implementation echo "Searching for MentionEach command handler implementation:" rg -A 10 "MentionEach" --type go | grep -v "commands/main.go" # Look for permission check patterns in command handlers echo -e "\nSearching for permission check patterns in other command handlers:" rg "HasPermission|PermissionLevel|Admin|Administrator|Manage" --type goLength of output: 539
Permission Checks Needed for the MentionEach Command
The current implementation of the MentionEach command does not show any explicit permission checks. This could potentially allow any user to invoke the command—which, in large servers, might be misused. Please verify whether a centralized permission mechanism is already handling this command. If not, add explicit permission checks (e.g., verifying user roles or admin status) either directly in the command handler or as part of a centralized middleware.
- Confirm if a centralized permissions system exists for command handling.
- If one isn’t in place, implement explicit permission checks for MentionEach to restrict its usage.
utils/members_utils.go (1)
1-85: Overall implementation looks goodThe utility functions provide a well-structured and comprehensive set of tools for Discord member management and mention formatting. The code follows good practices:
- Appropriate interface design for testability
- Consistent error handling
- Comprehensive edge case handling
- Clean, readable implementation
🧰 Tools
🪛 golangci-lint (1.64.8)
47-47: S1009: should omit nil check; len() for nil slices is defined as zero
(gosimple)
utils/members_utils_test.go (1)
1-192: Excellent test coverageThe test file provides comprehensive coverage for all utility functions, including edge cases:
- Empty inputs
- Nil inputs
- Single element cases
- Multiple element cases
- Error handling
This ensures the utility functions are robust and behave as expected in all scenarios.
* 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.
Devashish08
force-pushed
the
mention-each-pr1
branch
from
2c07732 to
72c3fc0
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
utils/members_utils.go (1)
15-30: 🧹 Nitpick (assertive)Consider implementing pagination for large guilds.
The function currently fetches up to 1000 guild members in a single call, which might not be sufficient for large Discord servers. This approach could miss members in servers that exceed this limit.
You could enhance the function to handle pagination for large guild databases:
-func GetUsersWithRole(session DiscordSessionInterface, guildID string, roleID string) ([]*discordgo.Member, error) { - members, err := session.GuildMembers(guildID, "", 1000) - - if err != nil { - return nil, fmt.Errorf("failed to fetch guild members: %w", err) - } +func GetUsersWithRole(session DiscordSessionInterface, guildID string, roleID string) ([]*discordgo.Member, error) { + var allMembers []*discordgo.Member + var lastMemberID string + + for { + members, err := session.GuildMembers(guildID, lastMemberID, 1000) + if err != nil { + return nil, fmt.Errorf("failed to fetch guild members: %w", err) + } + + if len(members) == 0 { + break + } + + allMembers = append(allMembers, members...) + + if len(members) < 1000 { + break + } + + lastMemberID = members[len(members)-1].User.ID + }
📜 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 (6)
commands/main.go(1 hunks)dtos/commands.go(1 hunks)go.mod(1 hunks)utils/constants.go(1 hunks)utils/members_utils.go(1 hunks)utils/members_utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
commands/main.go (1)
utils/constants.go (1)
CommandNames(8-13)
utils/members_utils_test.go (1)
utils/members_utils.go (6)
GetUsersWithRole(15-30)FormatUserMentions(32-41)FormatRoleMention(43-45)JoinMentions(47-52)FormatMentionResponse(54-64)FormatDevTitleResponse(66-79)
🔇 Additional comments (18)
dtos/commands.go (1)
7-7: Addition of MentionEach looks good.The new field is properly aligned with the existing pattern in the CommandNameTypes struct.
go.mod (1)
19-19: New dependency looks appropriate.The addition of
github.com/stretchr/objx v0.5.2as an indirect dependency is fine. This package is commonly used with testify for mocking in tests.utils/members_utils.go (6)
10-13: Good interface design for testability.The DiscordSessionInterface allows for easy mocking in tests, which is a best practice for testing code that interacts with external services like Discord.
32-41: Good defensive programming with nil check.The nil check in
FormatUserMentionsensures robustness by returning an empty slice instead of panicking when given a nil input.
43-45: Function is short and focused.The
FormatRoleMentionfunction has a clear purpose and follows the single responsibility principle.
47-52: Simplified nil check looks good.The function correctly handles empty slices and follows Go's idiomatic approach of using
len(mentions) == 0without an explicit nil check.
54-64: Well-structured response formatting.The
FormatMentionResponsefunction handles different cases appropriately and formats responses clearly.
66-79: Comprehensive case handling in FormatDevTitleResponse.The function uses a switch statement effectively to handle different scenarios (0, 1, or multiple users) with appropriate grammar for each case.
utils/constants.go (1)
12-12: Command name follows existing naming pattern.The added MentionEach command name uses kebab-case format, consistent with the other command names in the constants.
commands/main.go (2)
29-62: Well-structured command implementation for MentionEachThe MentionEach command is well-structured with clear descriptions and appropriate option types for each parameter. The command allows mentioning all users with a specific role, with additional options for customization.
I like that you've:
- Added clear comments explaining each option's purpose
- Made only the role option required, keeping the other options optional
- Included developer-oriented options that enhance functionality
56-57: Naming follows established conventionsThe
dev_titleoption correctly follows the established naming convention in the codebase, where multi-word option names use snake_case.utils/members_utils_test.go (7)
13-37: Comprehensive mock implementation for Discord sessionThe MockDiscordSession implementation provides proper mocking of the Discord API methods needed for testing. The implementation includes detailed error handling and type checking, which will help catch issues during testing.
42-108: Thorough test coverage for GetUsersWithRole functionThe tests for GetUsersWithRole are comprehensive, covering all important scenarios:
- Success case with a single user having the role
- Success case with multiple users having the role
- Error handling when the API call fails
- Empty result when no users have the specified role
The assertions are appropriate and verify both the absence of errors and the expected content of the results.
110-132: Complete test cases for FormatUserMentionsGood job testing all cases for the FormatUserMentions function, including empty and nil member lists. The function returns correctly formatted mention strings in all scenarios.
134-146: Good test coverage for FormatRoleMentionThe tests for FormatRoleMention cover both normal operation and the edge case of an empty role ID. This ensures the function operates correctly in all scenarios.
148-178: Comprehensive test cases for JoinMentionsExcellent test coverage for the JoinMentions function, including various separators and edge cases like single mention, empty list, and nil list.
180-205: Well-structured tests for FormatMentionResponseThe tests for FormatMentionResponse cover all relevant scenarios, including responses with and without custom messages, as well as handling empty and nil mention lists.
206-243: Complete test coverage for FormatDevTitleResponseThe tests for FormatDevTitleResponse thoroughly cover all the required scenarios:
- No users found
- Single user found
- Multiple users found
- Handling nil mentions
- Handling empty role ID
The tests verify that the response messages are appropriately formatted based on the number of users found.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
utils/members_utils_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
utils/members_utils_test.go (1)
utils/members_utils.go (6)
GetUsersWithRole(15-30)FormatUserMentions(32-41)FormatRoleMention(43-45)JoinMentions(47-52)FormatMentionResponse(54-64)FormatDevTitleResponse(66-79)
🔇 Additional comments (5)
utils/members_utils_test.go (5)
13-37: Mock implementation looks good.The mock implementation for the Discord session interface is well-structured and includes proper error handling. This will allow for effective testing of the utility functions without requiring actual Discord API calls.
108-128: Tests for FormatUserMentions are well-structured.The tests effectively verify all aspects of the function, including proper formatting of mentions and correct handling of edge cases (empty and nil member lists).
130-142: Tests for FormatRoleMention are thorough.The test cases properly validate the formatting of role mentions and handle the edge case of an empty role ID.
144-174: JoinMentions tests cover all scenarios.The tests thoroughly validate the function's behavior with different separators and handle edge cases appropriately (single mention, empty mentions, nil mentions).
176-201: FormatMentionResponse tests are comprehensive.All test cases validate the expected behavior, including proper formatting with and without custom messages, and handling of empty/nil mentions.
AnujChhikara
left a comment
AnujChhikara
left a comment
There was a problem hiding this comment.
Please follow the correct formatting
iamitprakash
left a comment
iamitprakash
left a comment
There was a problem hiding this comment.
work proof SS is missing
|
why is it not under feature flag? |
why this should be under feature flag ? this is bot command. |
Are you sure the bot command will work correctly and without any issues? |
added video of command working and also added feature flag for mention each command |
.env.example
Outdated
| QUEUE_URL = "<QUEUE_URL>" # Default :amqp://localhost | ||
| ENV = "development" # Other: production, staging | ||
| QUEUE_NAME = "<QUEUE_NAME>" #Default: "DISCORD_QUEUE" | ||
| MENTION_EACH_ENABLED = "false" # Default: false |
There was a problem hiding this comment.
why this is false by default and is in form of string
There was a problem hiding this comment.
false is default because this command is in currently under development and this PR is only first PR after all the PR will be merged then we can remove false and set it true
and is in form of string because all the other env were also string so i keep it that way
There was a problem hiding this comment.
sir then should i set it true if not that then what should be here if i remove it then how will we decide about feature flag
There was a problem hiding this comment.
when command will be available we will put back, as of now this is not needed here and creating confusion
There was a problem hiding this comment.
sir so should i remove all the feature flag code , Please clear this doubt that should this command be under feature flag or not if it should be what is alternative to like how we can control feature flag for this command
There was a problem hiding this comment.
remove all the code related this and now this command is not under feature flag
| case 1: | ||
| return fmt.Sprintf("Found 1 user with the %s role: %s", roleMention, mentions[0]) | ||
| default: | ||
| userList := strings.Join(mentions, ", ") |
There was a problem hiding this comment.
If mentions contains empty strings (e.g., because of earlier bad formatting), strings.Join(mentions, ", ") will still output commas for them.
There was a problem hiding this comment.
Sir FormatUserMentions will return only valid data so mentions slice will not contain an empty strings
commands/register/register_test.go
Outdated
| }) | ||
|
|
||
| createRegisterMockSession := func(openErr, cmdErr error) *mockSession { | ||
| sess := &mockSession{ |
There was a problem hiding this comment.
remove the code
config/config.go
Outdated
| return value | ||
| } | ||
|
|
||
| func loadBoolEnv(key string, defaultValue bool) bool { |
There was a problem hiding this comment.
fix hardcoded error values
There was a problem hiding this comment.
removed this code as feature flag is removed then this code here is not needed
utils/membersUtils_test.go
Outdated
| mockSession.AssertExpectations(t) | ||
| }) | ||
|
|
||
| t.Run("skips members with nil User or nil Roles safely", func(t *testing.T) { |
There was a problem hiding this comment.
fix this please as discussed
There was a problem hiding this comment.
rename the test
utils/membersUtils.go
Outdated
|
|
||
| } | ||
|
|
||
| func FormatDevTitleResponse(mentions []string, roleID string) string { |
There was a problem hiding this comment.
please change this function name
There was a problem hiding this comment.
sir i have change this function name to FormatUserListResponse is this okay name ?
utils/membersUtils.go
Outdated
| } | ||
|
|
||
| for _, r := range member.Roles { | ||
| if r == roleID { |
There was a problem hiding this comment.
please optimize this for loop
There was a problem hiding this comment.
remove hasRole function and move it in to GetUserWithRole function. please see once.
joyguptaa
left a comment
joyguptaa
left a comment
There was a problem hiding this comment.
Although we are registering the mention-each command, there is no handler associated with it yet. Please make sure to add a handler for this command before your code is merged.
the handler isn't in this PR. this PR focuses only on adding the command definition, constants and utility functions. The service layer routing (mentionEachService) and handler (mentionEachHandler) will be raised in separate PRs for refrence you can see PR #75 also i have refactor code since then but will clear overall how command working. |
If this is the case, the attached video is misleading. |
…d refactor getUserWithRole to use models.SessionInterface
removed the video |
6 participants
Date: 09/04/2025
Developer Name: @Devashish08
Issue Ticket Number
#70
Description
This is first PR for mention each migration command in this PR i have added constants for mention each command and members utils function to get members from discord
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Test Coverage
Screenshot 1
Additional Notes