feat: extend group ratio functionality to support user-specific overr… #2085
Conversation
|
Warning Rate limit exceeded@zhaolion has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces default group-ratio retrieval with a user-group-aware variant. Adds GetGroupRatioExtendCopy(userGroup string) which merges default ratios with optional per-user-group overrides. Controller now passes the current user group when requesting ratios. Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as controller/group.go
participant RatioSetting as ratio_setting.GetGroupRatioExtendCopy(userGroup)
participant Defaults as DefaultGroupRatios
participant Overrides as GroupGroupRatio[userGroup]
participant Merge as groupMerge()
participant Result as FinalRatios
Controller->>RatioSetting: GetGroupRatioExtendCopy(userGroup)
RatioSetting->>Defaults: copy default ratios
Defaults-->>RatioSetting: defaultMap
RatioSetting->>Overrides: getGroupGroupRatioCopy(userGroup)
Overrides-->>RatioSetting: overrideMap (or none)
alt overrides exist
RatioSetting->>Merge: groupMerge(defaultMap, overrideMap)
Merge-->>Result: mergedMap (overrides precedence)
else no overrides
RatioSetting-->>Result: defaultMap
end
Result-->>Controller: final ratio map
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR extends the group ratio functionality to support user-specific overrides, allowing users in special groups to see different ratio configurations based on their group membership.
Key Changes:
- Added
GetGroupRatioExtendCopy()function to merge user-specific group ratio overrides with base ratios - Added helper functions
getGroupGroupRatioCopy()andgroupMerge()to support the override mechanism - Updated
GetUserGroups()controller to use the new extended ratio function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| setting/ratio_setting/group_ratio.go | Implements the core functionality for merging user-specific group ratio overrides with base ratios |
| controller/group.go | Updates the group retrieval logic to use the new extended ratio function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
setting/ratio_setting/group_ratio.go
Outdated
| } else { | ||
| return groupMerge(groupCopy, extendGroup) | ||
| } |
There was a problem hiding this comment.
The else block is unnecessary after a return statement. Remove the else and unindent the return statement to improve readability.
| } else { | |
| return groupMerge(groupCopy, extendGroup) | |
| } | |
| } | |
| return groupMerge(groupCopy, extendGroup) |
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
controller/group.go (2)
27-27: Optional: Remove unnecessary initialization.The
userGroupvariable is initialized to an empty string but immediately overwritten on line 29.Apply this diff to remove the initialization:
- userGroup := "" + var userGroup string
30-39: Move GetUserUsableGroups call outside the loop.
setting.GetUserUsableGroups(userGroup)is called on every iteration but returns the same value each time, causing unnecessary overhead.Apply this diff to improve performance:
for groupName, ratio := range ratio_setting.GetGroupRatioExtendCopy(userGroup) { - // UserUsableGroups contains the groups that the user can use - userUsableGroups := setting.GetUserUsableGroups(userGroup) + userUsableGroups := setting.GetUserUsableGroups(userGroup) + for groupName, ratio := range ratio_setting.GetGroupRatioExtendCopy(userGroup) { if desc, ok := userUsableGroups[groupName]; ok {Or more clearly:
+ userUsableGroups := setting.GetUserUsableGroups(userGroup) for groupName, ratio := range ratio_setting.GetGroupRatioExtendCopy(userGroup) { - // UserUsableGroups contains the groups that the user can use - userUsableGroups := setting.GetUserUsableGroups(userGroup) if desc, ok := userUsableGroups[groupName]; ok {setting/ratio_setting/group_ratio.go (2)
53-58: Optional: Simplify conditional logic.The
elseblock is unnecessary and can be removed for cleaner code.Apply this diff:
extendGroup, ok := getGroupGroupRatioCopy(userGroup) if !ok { return groupCopy - } else { - return groupMerge(groupCopy, extendGroup) } + return groupMerge(groupCopy, extendGroup) }
163-172: Optional: Document or avoid in-place mutation.The function mutates
groupAdirectly rather than creating a new map, which may be unexpected for a merge operation. While safe here (caller passes a copy), consider either:
- Documenting this behavior in the comment, or
- Creating a new result map for clearer semantics
Example with explicit new map:
-// GroupMerge merges two group ratio maps, with values from groupB overriding those in groupA. +// groupMerge creates a new map by merging groupA and groupB, with values from groupB overriding those in groupA. func groupMerge(groupA, groupB map[string]float64) map[string]float64 { + result := make(map[string]float64, len(groupA)) + for k, v := range groupA { + result[k] = v + } for k, v := range groupB { - if _, ok := groupA[k]; ok { - groupA[k] = v + if _, ok := result[k]; ok { + result[k] = v } } - return groupA + return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controller/group.go(1 hunks)setting/ratio_setting/group_ratio.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controller/group.go (1)
setting/ratio_setting/group_ratio.go (1)
GetGroupRatioExtendCopy(49-58)
🔇 Additional comments (3)
controller/group.go (1)
30-30: LGTM! User-specific ratio retrieval implemented correctly.The change from
GetGroupRatioCopy()toGetGroupRatioExtendCopy(userGroup)correctly passes the user's group to retrieve group ratios with user-specific overrides.setting/ratio_setting/group_ratio.go (2)
114-128: LGTM! Helper function correctly retrieves user-specific overrides.The function properly uses read locks and returns a copy to prevent concurrent modification issues.
164-172: Clarify merge behavior: only existing keys are updated.The function only updates keys that already exist in
groupA(line 166). Keys ingroupBthat don't exist ingroupAare silently ignored. While this appears intentional based on the documentation example, it limits the ability to add new group ratios viaGroupGroupRatio.Consider whether the intended behavior should allow adding new groups (e.g., VIP-exclusive groups not in the default ratio). If so, remove the existence check:
Current behavior (only updates existing keys):
for k, v := range groupB { if _, ok := groupA[k]; ok { // Only if key exists groupA[k] = v } }Alternative behavior (adds and updates keys):
for k, v := range groupB { groupA[k] = v // Adds new keys and updates existing ones }Please verify which behavior aligns with your requirements. The documentation comment should also clarify this behavior.
…ides, user will see extend system group when user.group hint GroupGroupRatios
1 participant
feature
支持用户分组命中 【分组特殊倍率】的分组时候，允许用户可见特殊配置的分组
case:1
case:2
Summary by CodeRabbit
New Features
Refactor