feat: 新增普通充值自动升级分组规则配置 - #3409
Open
guoruqiang wants to merge 20 commits into
Open
Conversation
…guoruqiang/new-api into feature/topup-auto-switch-group
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a payment auto-switch-group feature: validates and persists auto-switch settings, computes USD-normalized successful top-ups, transactionally determines and applies user group switches based on thresholds, integrates into top-up completion and subscription expiry flows, and exposes a controller endpoint plus frontend settings UI. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Controller as Controller\n(topup.go)
participant Model as TopUpModel\n(model/topup.go)
participant DB as Database
participant OpSet as OperationSetting
participant Cache as UserCache
Client->>Controller: Top-up success webhook / complete
Controller->>DB: Begin transaction
Controller->>Model: Mark top-up success (record CompleteTime)
Model->>DB: GetUserSuccessfulTopupTotalUSDTx (sum per-user successful topups)
Model->>OpSet: Read PaymentSetting (AutoSwitch rules)
Model->>Model: Match highest eligible rule → targetGroup
Model->>DB: Check active subscription upgrade (getActiveSubscriptionUpgradeGroupTx)
alt Active upgrade exists
Model->>DB: Prefer upgrade group (no top-up switch)
else
Model->>DB: applyTopUpAutoSwitchGroupTx → update user.group
end
DB->>DB: Commit
Controller->>Cache: UpdateUserGroupCache (if changed)
Controller->>Client: Respond success
sequenceDiagram
participant Cron
participant SubModel as SubscriptionModel\n(model/subscription.go)
participant TopUpModel as TopUpModel\n(model/topup.go)
participant DB as Database
participant OpSet as OperationSetting
participant Cache as UserCache
Cron->>SubModel: ExpireDueSubscriptions(cutoff)
SubModel->>DB: Begin transaction
SubModel->>DB: Find expired subscriptions (lock user)
loop per expired subscription
SubModel->>SubModel: Resolve effective fallback group
SubModel->>TopUpModel: resolveUserEffectiveGroupTx (may call GetUserSuccessfulTopupTotalUSDTx)
TopUpModel->>OpSet: Read AutoSwitch rules
TopUpModel->>TopUpModel: Determine targetGroup via rules
alt Active upgrade exists
SubModel->>DB: Use active upgrade group (no switch)
else if targetGroup valid and != current
SubModel->>DB: updateUserGroupTx to targetGroup
end
end
DB->>DB: Commit
SubModel->>Cache: UpdateUserGroupCache for changed users
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
setting/operation_setting/payment_setting.go (1)
43-60: Extract the deep-copy logic into one helper.The clone path is duplicated in both accessors. If
PaymentSettinggains another slice/map field later, it is easy to update one copy site and miss the other, which would quietly reintroduce shared mutable state.♻️ Suggested refactor
+func clonePaymentSetting(src PaymentSetting) PaymentSetting { + cloned := src + if src.AmountOptions != nil { + cloned.AmountOptions = append([]int(nil), src.AmountOptions...) + } + if src.AmountDiscount != nil { + cloned.AmountDiscount = make(map[int]float64, len(src.AmountDiscount)) + for amount, discount := range src.AmountDiscount { + cloned.AmountDiscount[amount] = discount + } + } + if src.AutoSwitchGroupRules != nil { + cloned.AutoSwitchGroupRules = append([]PaymentAutoSwitchGroupRule(nil), src.AutoSwitchGroupRules...) + } + return cloned +} + func GetPaymentSetting() PaymentSetting { paymentSettingRWMutex.RLock() defer paymentSettingRWMutex.RUnlock() - - copiedSetting := paymentSetting - if paymentSetting.AmountOptions != nil { - copiedSetting.AmountOptions = append([]int(nil), paymentSetting.AmountOptions...) - } - if paymentSetting.AmountDiscount != nil { - copiedSetting.AmountDiscount = make(map[int]float64, len(paymentSetting.AmountDiscount)) - for amount, discount := range paymentSetting.AmountDiscount { - copiedSetting.AmountDiscount[amount] = discount - } - } - if paymentSetting.AutoSwitchGroupRules != nil { - copiedSetting.AutoSwitchGroupRules = append([]PaymentAutoSwitchGroupRule(nil), paymentSetting.AutoSwitchGroupRules...) - } - return copiedSetting + return clonePaymentSetting(paymentSetting) }Also applies to: 78-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/operation_setting/payment_setting.go` around lines 43 - 60, Extract the deep-copy code into a single helper function (e.g., ClonePaymentSetting or copyPaymentSetting) that accepts a PaymentSetting and returns a fully deep-copied PaymentSetting; move the existing slice/map copy logic (AmountOptions, AmountDiscount, AutoSwitchGroupRules and any future slice/map fields) into that helper, then update GetPaymentSetting to call the helper while retaining the paymentSettingRWMutex.RLock/RUnlock; do the same for the other accessor that duplicates this logic so both call the new helper to avoid divergent copy implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/option.go`:
- Around line 221-247: The current DB.Transaction block (DB.Transaction) commits
before in-memory validation and application (updateOptionMap and
applyPaymentSettingOptionValues), which can leave partial updates if validation
fails; move the validation/application logic inside the same transaction so the
whole multi-key update is atomic: perform updateOptionMap checks and
collect/validate paymentSettingOptionValues within the transaction callback
(alongside FirstOrCreate/Save for Option), and call
applyPaymentSettingOptionValues from inside that transaction so any error causes
tx rollback and is returned to the caller; ensure
applyPaymentSettingOptionValues returns/propagates errors so DB.Transaction can
roll back.
- Around line 260-272: The code stores the raw input for
auto_switch_group_base_group into common.OptionMap instead of the normalized
value produced via UpdatePaymentSetting; update the flow so the normalized
base-group is written back to common.OptionMap (and ideally normalized before
persisting to DB) by retrieving the normalized value from the PaymentSetting
after operation_setting.UpdatePaymentSetting (or from
paymentSettingConfigMap/UpdateConfigFromMap result) and assigning that
normalized string to common.OptionMap["auto_switch_group_base_group"] instead of
the original raw value; adjust code around
operation_setting.UpdatePaymentSetting, paymentSettingConfigMap,
UpdateConfigFromMap, and common.OptionMap to ensure storage, runtime and UI all
use the same normalized value.
In `@model/topup.go`:
- Around line 616-617: Replace the inconsistent logger call in the RecordLog
invocation: currently it uses logger.LogQuota(...) but other recharge paths use
logger.FormatQuota(...); update the RecordLog call that references topUp.UserId,
LogTypeTopup and topUp.Money to call logger.FormatQuota(quotaToAdd) instead of
logger.LogQuota(quotaToAdd) so the log formatting is consistent with the other
recharge flows.
- Around line 293-315: The transaction only locks the top-up row and can race
when multiple top-ups for the same user commit concurrently; before calling
completeTopUpTx(tx, &topUp, userUpdates) acquire a per-user lock to serialize
completions for that user (e.g. a DB advisory lock or SELECT ... FOR UPDATE on
the users row) using the user ID from topUp, hold the lock for the duration of
the transaction, then call completeTopUpTx and release the lock at transaction
end; update the code around the transaction block that references topUp, tx,
userUpdatesBuilder and completeTopUpTx to acquire and release this per-user lock
so recomputation of cumulative tiers and writes to users.group are serialized.
- Around line 294-295: The SELECT using a FOR UPDATE lock
(tx.Set("gorm:query_option", "FOR UPDATE").Where(...).First(&topUp)) is
incompatible with SQLite; guard it by checking common.UsingSQLite and only apply
the FOR UPDATE option when not using SQLite (i.e., if !common.UsingSQLite {
tx.Set("gorm:query_option", "FOR UPDATE")... } ), otherwise perform the plain
SELECT and rely on the existing manual rollback/serial approach used in
model/checkin.go; apply the same pattern to the similar usages in
model/subscription.go, model/user.go, and model/redemption.go to ensure
cross-database compatibility.
In `@setting/operation_setting/payment_setting.go`:
- Line 25: paymentSetting is subject to a data race because ExportAllConfigs →
ConfigToMap uses reflection to read struct fields without locking
paymentSettingRWMutex while UpdatePaymentSetting writes to AmountDiscount and
AutoSwitchGroupRules; also deep-copy logic is duplicated in GetPaymentSetting
and UpdatePaymentSetting. Fix by centralizing safe copying and ensuring
reflections read the protected copy: add a helper function (e.g.,
clonePaymentSetting or copyPaymentSetting) that takes the lock, makes a deep
copy of paymentSetting (including proper deep copies of AmountDiscount and
AutoSwitchGroupRules), and returns it; replace the duplicated deep-copy code in
GetPaymentSetting and UpdatePaymentSetting with calls to this helper; change
ExportAllConfigs/ConfigToMap to call GetPaymentSetting or clonePaymentSetting
(so reflection operates on an immutable copy) instead of directly reflecting
over paymentSetting.
In `@web/src/components/settings/PaymentSetting.jsx`:
- Around line 109-115: The JSON.parse of item.value that assigns to
newInputs['AutoSwitchGroupRules'] may return non-array values; after parsing
inside the try block (the code that sets newInputs['AutoSwitchGroupRules']),
validate the parsed result with Array.isArray and only assign it if true,
otherwise set newInputs['AutoSwitchGroupRules'] = []; keep the existing catch to
handle parse errors but add the Array.isArray check for the parsed value to
prevent non-array objects from being stored.
In `@web/src/i18n/locales/fr.json`:
- Around line 158-171: Add the missing French translation for the new error key
used in SettingsPaymentGateway.jsx: add an entry for the Chinese key
"分组加载失败，请刷新后重试" to web/src/i18n/locales/fr.json with an appropriate French
message (e.g., "Échec du chargement des groupes, veuillez actualiser et
réessayer") so t('分组加载失败，请刷新后重试') returns a French string instead of falling
back to Chinese.
In `@web/src/pages/Setting/Payment/SettingsPaymentGateway.jsx`:
- Around line 339-349: handleFormChange currently normalizes inputs into React
state only, leaving the Semi Form store stale; update the form store with the
same normalizedValues when a change is accepted. Inside handleFormChange (after
computing normalizedValues and confirming compareObjects(prev,
normalizedValues).length !== 0), call the Semi Form API to set the form values
(e.g., formApi.setValues or formRef.current.setValues) with normalizedValues so
the form store and React state stay in sync, taking care to only call the form
setter when values actually changed to avoid infinite loops.
---
Nitpick comments:
In `@setting/operation_setting/payment_setting.go`:
- Around line 43-60: Extract the deep-copy code into a single helper function
(e.g., ClonePaymentSetting or copyPaymentSetting) that accepts a PaymentSetting
and returns a fully deep-copied PaymentSetting; move the existing slice/map copy
logic (AmountOptions, AmountDiscount, AutoSwitchGroupRules and any future
slice/map fields) into that helper, then update GetPaymentSetting to call the
helper while retaining the paymentSettingRWMutex.RLock/RUnlock; do the same for
the other accessor that duplicates this logic so both call the new helper to
avoid divergent copy implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e558997a-a6bd-4544-ac15-047bb355905c
📒 Files selected for processing (17)
controller/option.gocontroller/topup.gomodel/option.gomodel/subscription.gomodel/topup.gomodel/topup_auto_switch_test.gorouter/api-router.gosetting/operation_setting/payment_setting.goweb/src/components/settings/PaymentSetting.jsxweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/pages/Setting/Payment/SettingsPaymentGateway.jsx
Comment on lines
+339
to
+349
| const handleFormChange = (values) => { | ||
| setInputs(values); | ||
| setInputs((prev) => { | ||
| const normalizedValues = normalizeAutoSwitchGroupInputs({ | ||
| ...prev, | ||
| ...values, | ||
| }); | ||
| if (compareObjects(prev, normalizedValues).length === 0) { | ||
| return prev; | ||
| } | ||
| return normalizedValues; | ||
| }); |
Contributor
There was a problem hiding this comment.
Sync normalized values back into the form store.
normalizeAutoSwitchGroupInputs() only updates React state. Semi Form still keeps the old AutoSwitchGroupOnlyNewTopups value, so turning auto-switch off and then back on can resurrect a stale true and save the wrong mode.
🩹 One way to keep the form store and React state aligned
const handleFormChange = (values) => {
- setInputs((prev) => {
- const normalizedValues = normalizeAutoSwitchGroupInputs({
- ...prev,
- ...values,
- });
- if (compareObjects(prev, normalizedValues).length === 0) {
- return prev;
- }
- return normalizedValues;
- });
+ const mergedValues = {
+ ...inputs,
+ ...values,
+ };
+ const normalizedValues = normalizeAutoSwitchGroupInputs(mergedValues);
+ if (compareObjects(mergedValues, normalizedValues).length !== 0) {
+ formApiRef.current?.setValues(normalizedValues);
+ }
+ setInputs((prev) =>
+ compareObjects(prev, normalizedValues).length === 0
+ ? prev
+ : normalizedValues,
+ );
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/pages/Setting/Payment/SettingsPaymentGateway.jsx` around lines 339 -
349, handleFormChange currently normalizes inputs into React state only, leaving
the Semi Form store stale; update the form store with the same normalizedValues
when a change is accepted. Inside handleFormChange (after computing
normalizedValues and confirming compareObjects(prev, normalizedValues).length
!== 0), call the Semi Form API to set the form values (e.g., formApi.setValues
or formRef.current.setValues) with normalizedValues so the form store and React
state stay in sync, taking care to only call the form setter when values
actually changed to avoid infinite loops.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
model/option.go (2)
281-292:⚠️ Potential issue | 🟠 MajorMirror normalized base-group into
OptionMap, not raw input.
UpdatePaymentSettingnormalizesauto_switch_group_base_group, butOptionMapstill stores the original raw value. This keeps API/UI-visible value inconsistent with runtime-enforced value.Focused fix
var paymentSettingErr error -operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) { +normalizedPaymentSetting := operation_setting.UpdatePaymentSetting(func(setting *operation_setting.PaymentSetting) { paymentSettingErr = config.UpdateConfigFromMap(setting, paymentSettingConfigMap) }) if paymentSettingErr != nil { return paymentSettingErr } common.OptionMapRWMutex.Lock() defer common.OptionMapRWMutex.Unlock() for key, value := range optionValues { + if key == "payment_setting.auto_switch_group_base_group" { + common.OptionMap[key] = normalizedPaymentSetting.AutoSwitchGroupBaseGroup + continue + } common.OptionMap[key] = value }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 281 - 292, Update OptionMap to store the normalized auto_switch_group_base_group value instead of the raw input: inside the UpdatePaymentSetting closure (the func(setting *operation_setting.PaymentSetting) passed to operation_setting.UpdatePaymentSetting), capture the normalized field (e.g., setting.AutoSwitchGroupBaseGroup) into a local variable; after the closure completes and before writing optionValues into common.OptionMap (the loop using common.OptionMapRWMutex), overwrite optionValues["auto_switch_group_base_group"] with that captured normalized value so the value written into common.OptionMap matches the runtime-normalized payment setting.
229-253:⚠️ Potential issue | 🟠 Major
UpdateOptionscan still fail after DB commit (partial success path).The transaction commits before
updateOptionMap/applyPaymentSettingOptionValuesrun. If either later step fails, the caller receives an error after persistence has already succeeded.Suggested direction
- if err := DB.Transaction(func(tx *gorm.DB) error { + return DB.Transaction(func(tx *gorm.DB) error { for _, key := range keys { option := Option{Key: key} if err := tx.FirstOrCreate(&option, Option{Key: key}).Error; err != nil { return err } option.Value = optionValues[key] if err := tx.Save(&option).Error; err != nil { return err } } - return nil - }); err != nil { - return err - } - - for _, key := range keys { - if strings.HasPrefix(key, "payment_setting.") { - continue - } - if err := updateOptionMap(key, optionValues[key]); err != nil { - return err - } - } - return applyPaymentSettingOptionValues(paymentSettingOptionValues) + for _, key := range keys { + if strings.HasPrefix(key, "payment_setting.") { + continue + } + if err := updateOptionMap(key, optionValues[key]); err != nil { + return err + } + } + return applyPaymentSettingOptionValues(paymentSettingOptionValues) + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 229 - 253, The current flow commits DB changes inside DB.Transaction then runs updateOptionMap and applyPaymentSettingOptionValues, causing possible partial success; to fix, perform the map update and payment-setting application inside the same DB.Transaction closure so any failure returns an error and triggers rollback: update the Transaction lambda that iterates keys (using Option, optionValues, paymentSettingOptionValues, keys) to call updateOptionMap(key, ...) for non-"payment_setting." keys and to call applyPaymentSettingOptionValues(...) before returning nil; if updateOptionMap or applyPaymentSettingOptionValues are non-DB operations, refactor them to return errors and make them safe to call inside the transaction (or add TX-aware variants) so failures propagate and the transaction can rollback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/option.go`:
- Around line 225-227: The current validatePaymentSettingOptionValues is too
permissive because it calls config.UpdateConfigFromMap which skips unknown
keys/parse errors; change validatePaymentSettingOptionValues to perform strict
decoding into the exact expected config struct (e.g., marshal the option map to
JSON/YAML and unmarshal with a decoder that DisallowUnknownFields or use a
strict decoder/validator) and return an error on any unknown keys or parse
failures instead of silently accepting them; replace the permissive
UpdateConfigFromMap usage with this strict decode+validate flow and apply the
same strict approach to the other similar validators referenced in the review.
---
Duplicate comments:
In `@model/option.go`:
- Around line 281-292: Update OptionMap to store the normalized
auto_switch_group_base_group value instead of the raw input: inside the
UpdatePaymentSetting closure (the func(setting
*operation_setting.PaymentSetting) passed to
operation_setting.UpdatePaymentSetting), capture the normalized field (e.g.,
setting.AutoSwitchGroupBaseGroup) into a local variable; after the closure
completes and before writing optionValues into common.OptionMap (the loop using
common.OptionMapRWMutex), overwrite optionValues["auto_switch_group_base_group"]
with that captured normalized value so the value written into common.OptionMap
matches the runtime-normalized payment setting.
- Around line 229-253: The current flow commits DB changes inside DB.Transaction
then runs updateOptionMap and applyPaymentSettingOptionValues, causing possible
partial success; to fix, perform the map update and payment-setting application
inside the same DB.Transaction closure so any failure returns an error and
triggers rollback: update the Transaction lambda that iterates keys (using
Option, optionValues, paymentSettingOptionValues, keys) to call
updateOptionMap(key, ...) for non-"payment_setting." keys and to call
applyPaymentSettingOptionValues(...) before returning nil; if updateOptionMap or
applyPaymentSettingOptionValues are non-DB operations, refactor them to return
errors and make them safe to call inside the transaction (or add TX-aware
variants) so failures propagate and the transaction can rollback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cc63373-16a9-4340-b0f4-0a00a4dcd81c
📒 Files selected for processing (2)
model/option.goweb/src/i18n/locales/fr.json
✅ Files skipped from review due to trivial changes (1)
- web/src/i18n/locales/fr.json
Comment on lines
+225
to
+227
| if err := validatePaymentSettingOptionValues(paymentSettingOptionValues); err != nil { | ||
| return err | ||
| } |
Contributor
There was a problem hiding this comment.
Current payment-setting “validation” is effectively non-strict.
validatePaymentSettingOptionValues relies on config.UpdateConfigFromMap, which is permissive (unknown keys / parse failures are skipped). Invalid values can pass validation and still be written to DB, causing persisted config to diverge from effective runtime config.
Suggested strict validation shape
+import "fmt"
...
func validatePaymentSettingOptionValues(optionValues map[string]string) error {
if len(optionValues) == 0 {
return nil
}
paymentSettingConfigMap := make(map[string]string, len(optionValues))
for key, value := range optionValues {
paymentSettingConfigMap[strings.TrimPrefix(key, "payment_setting.")] = value
}
- paymentSetting := operation_setting.GetPaymentSetting()
- return config.UpdateConfigFromMap(&paymentSetting, paymentSettingConfigMap)
+ for key, value := range paymentSettingConfigMap {
+ v := strings.TrimSpace(value)
+ switch key {
+ case "auto_switch_group_enabled", "auto_switch_group_only_new_topups":
+ if _, err := strconv.ParseBool(v); err != nil {
+ return fmt.Errorf("invalid %s: %w", key, err)
+ }
+ case "auto_switch_group_enabled_from":
+ if _, err := strconv.ParseInt(v, 10, 64); err != nil {
+ return fmt.Errorf("invalid %s: %w", key, err)
+ }
+ case "auto_switch_group_base_group":
+ // normalized later; accept empty/whitespace.
+ case "auto_switch_group_rules":
+ var rules []operation_setting.PaymentAutoSwitchGroupRule
+ if err := common.UnmarshalJsonStr(v, &rules); err != nil {
+ return fmt.Errorf("invalid %s: %w", key, err)
+ }
+ default:
+ return fmt.Errorf("unsupported payment setting key: %s", key)
+ }
+ }
+ return nil
}Also applies to: 256-268, 280-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/option.go` around lines 225 - 227, The current
validatePaymentSettingOptionValues is too permissive because it calls
config.UpdateConfigFromMap which skips unknown keys/parse errors; change
validatePaymentSettingOptionValues to perform strict decoding into the exact
expected config struct (e.g., marshal the option map to JSON/YAML and unmarshal
with a decoder that DisallowUnknownFields or use a strict decoder/validator) and
return an error on any unknown keys or parse failures instead of silently
accepting them; replace the permissive UpdateConfigFromMap usage with this
strict decode+validate flow and apply the same strict approach to the other
similar validators referenced in the review.
This comment was marked as spam.
This comment was marked as spam.
|
希望能合并 |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
setting/operation_setting/payment_setting.go (1)
11-17:⚠️ Potential issue | 🔴 CriticalProtect shared
PaymentSettingfrom concurrent mutation.Line 11-Line 17 introduce additional mutable fields (map/slice). Combined with global shared state, this amplifies race risk when settings are read while being updated. Please switch readers to a deep-copied snapshot (or guard access with RW locking) instead of exposing shared mutable internals.
Also applies to: 21-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/operation_setting/payment_setting.go` around lines 11 - 17, The PaymentSetting struct exposes mutable internals (AmountOptions, AmountDiscount, AutoSwitchGroupRules, etc.) that can race when shared globally; fix by either protecting the shared instance with a RWMutex around all readers/writers of PaymentSetting or by returning a deep-copied snapshot to callers: ensure any accessor (e.g., GetPaymentSetting or similar) produces a full copy that duplicates slices and maps (copy AmountOptions slice, deep-copy AmountDiscount map, clone AutoSwitchGroupRules slice/entries) so callers never mutate shared memory; apply the same treatment to the other mutable fields noted (lines 21–28).model/topup.go (1)
232-281:⚠️ Potential issue | 🔴 CriticalSerialize auto-switch recomputation per user to avoid threshold under-switch.
The cumulative-USD match +
users.groupupdate path is still vulnerable to concurrent completions for the same user. Two transactions can each compute totals without seeing the other uncommitted success and both persist a lower tier than the true post-commit cumulative tier. Lock the user row (or equivalent per-user mutex) for the full recompute-and-update section inside the transaction.Also applies to: 367-368, 605-606, 683-684, 749-750, 815-816
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/topup.go` around lines 232 - 281, The auto-switch path in applyTopUpAutoSwitchGroupTx is vulnerable to concurrent transactions because recompute (cumulative USD checks like getTopUpAutoSwitchTargetGroupTx / getActiveSubscriptionUpgradeGroupTx) and the subsequent updateUserGroupTx are not serialized; fix by acquiring a per-user row lock in the same tx before doing the recompute-and-update sequence (use SELECT ... FOR UPDATE on the users row or add a helper like getUserRowForUpdateTx / extend getUserGroupByIdTx with a forUpdate flag), perform all reads that determine activeUpgradeGroup/targetGroup while the lock is held, then call updateUserGroupTx and release the lock at transaction end; apply the same locking pattern to the other recompute-and-update locations that call getTopUpAutoSwitchTargetGroupTx/updateUserGroupTx to prevent under-switch races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@model/topup.go`:
- Around line 232-281: The auto-switch path in applyTopUpAutoSwitchGroupTx is
vulnerable to concurrent transactions because recompute (cumulative USD checks
like getTopUpAutoSwitchTargetGroupTx / getActiveSubscriptionUpgradeGroupTx) and
the subsequent updateUserGroupTx are not serialized; fix by acquiring a per-user
row lock in the same tx before doing the recompute-and-update sequence (use
SELECT ... FOR UPDATE on the users row or add a helper like
getUserRowForUpdateTx / extend getUserGroupByIdTx with a forUpdate flag),
perform all reads that determine activeUpgradeGroup/targetGroup while the lock
is held, then call updateUserGroupTx and release the lock at transaction end;
apply the same locking pattern to the other recompute-and-update locations that
call getTopUpAutoSwitchTargetGroupTx/updateUserGroupTx to prevent under-switch
races.
In `@setting/operation_setting/payment_setting.go`:
- Around line 11-17: The PaymentSetting struct exposes mutable internals
(AmountOptions, AmountDiscount, AutoSwitchGroupRules, etc.) that can race when
shared globally; fix by either protecting the shared instance with a RWMutex
around all readers/writers of PaymentSetting or by returning a deep-copied
snapshot to callers: ensure any accessor (e.g., GetPaymentSetting or similar)
produces a full copy that duplicates slices and maps (copy AmountOptions slice,
deep-copy AmountDiscount map, clone AutoSwitchGroupRules slice/entries) so
callers never mutate shared memory; apply the same treatment to the other
mutable fields noted (lines 21–28).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcae2f99-6ec9-411b-9af8-04377813eb89
📒 Files selected for processing (11)
controller/option.gocontroller/option_payment_auto_switch_test.gocontroller/topup.gomodel/option.gomodel/subscription.gomodel/topup.gomodel/topup_auto_switch_test.gorouter/api-router.gosetting/operation_setting/payment_setting.goweb/src/components/settings/PaymentSetting.jsxweb/src/pages/Setting/Payment/SettingsPaymentAutoSwitchGroup.jsx
✅ Files skipped from review due to trivial changes (1)
- router/api-router.go
🚧 Files skipped from review as they are similar to previous changes (2)
- controller/topup.go
- model/option.go
Return copied payment settings so mutable maps and slices are not shared across requests. Update payment settings through a locked path, and lock the user row before recomputing auto-switch or subscription-driven group changes.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
setting/operation_setting/payment_setting.go (1)
35-44:⚠️ Potential issue | 🟠 MajorReflection-based config readers still race against
UpdatePaymentSetting.
initat Line 38 registers&paymentSettingdirectly, so any path that goes throughconfig.GlobalConfig.Get("payment_setting")/ExportAllConfigs()(e.g.,model.InitOptionMapreflecting the struct viaConfigToMap) reads the live struct without holdingpaymentSettingRWMutex. Concurrent calls toUpdatePaymentSettingmutateAmountOptions,AmountDiscount, andAutoSwitchGroupRuleson the same instance, so reflection iteration of those slices/maps is a data race (and may panic on map iteration). Consider registering a getter/snapshot or routing reflective reads through the locked clone path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/operation_setting/payment_setting.go` around lines 35 - 44, The code registers the live variable paymentSetting in init (via config.GlobalConfig.Register("payment_setting", &paymentSetting)), allowing reflection-based readers to access the mutable struct without acquiring paymentSettingRWMutex and race with UpdatePaymentSetting; fix by changing the registration to expose a safe snapshot getter instead of the live pointer — i.e., have Register use a function or wrapper that calls GetPaymentSetting()/clonePaymentSettingLocked() to return a copy (or register the result of clonePaymentSettingLocked) so reflective reads always operate on an immutable clone; update any related code paths that expect the raw pointer to use the getter/register-snapshot approach and keep UpdatePaymentSetting and clonePaymentSettingLocked unchanged.model/option.go (2)
240-269:⚠️ Potential issue | 🟠 MajorAtomicity gap: DB is committed before in-memory updates are applied/validated.
UpdateOptionscommits the DB transaction at Line 254 and only then appliesupdateOptionMap/updatePaymentSettingOptionMap(Lines 256-268). If any of those fail (e.g., invalidauto_switch_group_rulesJSON), the DB row has already been persisted while the in-memorycommon.OptionMapandoperation_setting.paymentSettingare left untouched, so a subsequent process restart will load the bad value throughloadOptionsFromDatabaseand apply it without callers seeing a failure. Move the in-memory apply step inside the transaction (and roll back on error), or validate strictly before opening the transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 240 - 269, UpdateOptions currently commits the DB transaction before applying in-memory changes, causing an atomicity gap; move the in-memory updates and validation into the same DB.Transaction callback so any failure rolls back the DB. Specifically, inside the DB.Transaction(func(tx *gorm.DB) error { ... }) where you FirstOrCreate/Save the Option records, build paymentSettingOptionValues and call updateOptionMap(key, ...) and updatePaymentSettingOptionMap(paymentSettingOptionValues) there (or validate inputs beforehand) and return any error so the transaction is rolled back; keep references to Option, keys, optionValues, updateOptionMap, updatePaymentSettingOptionMap and DB.Transaction when making the change.
605-640:⚠️ Potential issue | 🟠 MajorRaw
auto_switch_group_base_groupvalue still leaks intocommon.OptionMap.
UpdatePaymentSettingnormalizesAutoSwitchGroupBaseGroup(e.g.,""/whitespace →"default"), but Line 637 writes back the originaloptionValues[key](raw input) intocommon.OptionMap. The DB row inUpdateOptions(Line 246) is also written with the raw value. As a result, runtimeoperation_setting.GetPaymentSetting()returns the normalized value while the OptionMap/DB and the admin UI continue to show whatever was submitted, leading to drift between displayed and effective configuration. AfterUpdatePaymentSettingreturns, mirror itsAutoSwitchGroupBaseGroup(and any other normalized fields) into bothOptionMapand the persisted row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 605 - 640, The code writes raw optionValues back into common.OptionMap and the DB, causing divergence from the normalized fields updated by operation_setting.UpdatePaymentSetting; fix updatePaymentSettingOptionMap by, after calling operation_setting.UpdatePaymentSetting (or inside the callback), reading the normalized PaymentSetting (e.g., via the setting pointer provided to the callback or operation_setting.GetPaymentSetting()) and using those normalized values (at minimum AutoSwitchGroupBaseGroup) to overwrite the corresponding entries in common.OptionMap and the persisted row (the UpdateOptions path) instead of the original raw optionValues; ensure you update both common.OptionMap and the persisted UpdateOptions write to use the normalized config keys/values produced by config.UpdateConfigFromMap/PaymentSetting normalization so UI, DB, and runtime stay consistent.
🧹 Nitpick comments (2)
setting/operation_setting/payment_setting.go (1)
84-90: Duplicate normalization helper across packages.
normalizePaymentAutoSwitchGroupBaseGrouphere is a verbatim duplicate of the same-named helper inmodel/topup.go(Lines 105-111). Consider exporting one canonical implementation (e.g.,operation_setting.NormalizePaymentAutoSwitchGroupBaseGroup) and havingmodel/topup.gocall it, so future tweaks (e.g., default group name) don't drift between the two locations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setting/operation_setting/payment_setting.go` around lines 84 - 90, The function normalizePaymentAutoSwitchGroupBaseGroup is duplicated; extract and export a single canonical implementation (e.g., NormalizePaymentAutoSwitchGroupBaseGroup) in the operation_setting package and remove the duplicate in model/topup.go; update callers in both packages to import operation_setting and call operation_setting.NormalizePaymentAutoSwitchGroupBaseGroup, and ensure imports are adjusted and tests/build updated so the default trimming/empty->"default" behavior remains identical.model/topup.go (1)
105-111: Duplicate of the helper insetting/operation_setting/payment_setting.go.This
normalizePaymentAutoSwitchGroupBaseGroupis identical to the one defined insetting/operation_setting/payment_setting.go(Lines 84-90). Re-using a single exported implementation prevents future drift if the default group name or trim rules change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/topup.go` around lines 105 - 111, Remove the duplicate normalizePaymentAutoSwitchGroupBaseGroup in model/topup.go and delegate to the single implementation in setting/operation_setting/payment_setting.go: import the payment_setting package, delete the local normalizePaymentAutoSwitchGroupBaseGroup definition, and replace all calls with payment_setting.NormalizePaymentAutoSwitchGroupBaseGroup(...); if the helper in payment_setting.go is not exported, rename it to an exported identifier (NormalizePaymentAutoSwitchGroupBaseGroup) so model/topup.go can reuse it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model/topup.go`:
- Around line 158-175: The current query filters with "amount > 0" which is
inconsistent with NormalizeTopUpValueUSD (which uses Money for Stripe); update
the query in the function that builds the top-up fetch (the query variable using
tx.Model(&TopUp{}) / Select(...).Where(...)) to align filtering with
normalization: either replace the "amount > 0" predicate with a predicate that
checks the monetary field used by NormalizeTopUpValueUSD (e.g., money > 0) or
add a composite condition that preserves non-Stripe logic but also includes rows
where payment_method = 'stripe' AND money > 0; ensure the filter references
TopUp.payment_method and TopUp.Money consistently so that NormalizeTopUpValueUSD
will not skip valid Stripe top-ups.
---
Duplicate comments:
In `@model/option.go`:
- Around line 240-269: UpdateOptions currently commits the DB transaction before
applying in-memory changes, causing an atomicity gap; move the in-memory updates
and validation into the same DB.Transaction callback so any failure rolls back
the DB. Specifically, inside the DB.Transaction(func(tx *gorm.DB) error { ... })
where you FirstOrCreate/Save the Option records, build
paymentSettingOptionValues and call updateOptionMap(key, ...) and
updatePaymentSettingOptionMap(paymentSettingOptionValues) there (or validate
inputs beforehand) and return any error so the transaction is rolled back; keep
references to Option, keys, optionValues, updateOptionMap,
updatePaymentSettingOptionMap and DB.Transaction when making the change.
- Around line 605-640: The code writes raw optionValues back into
common.OptionMap and the DB, causing divergence from the normalized fields
updated by operation_setting.UpdatePaymentSetting; fix
updatePaymentSettingOptionMap by, after calling
operation_setting.UpdatePaymentSetting (or inside the callback), reading the
normalized PaymentSetting (e.g., via the setting pointer provided to the
callback or operation_setting.GetPaymentSetting()) and using those normalized
values (at minimum AutoSwitchGroupBaseGroup) to overwrite the corresponding
entries in common.OptionMap and the persisted row (the UpdateOptions path)
instead of the original raw optionValues; ensure you update both
common.OptionMap and the persisted UpdateOptions write to use the normalized
config keys/values produced by config.UpdateConfigFromMap/PaymentSetting
normalization so UI, DB, and runtime stay consistent.
In `@setting/operation_setting/payment_setting.go`:
- Around line 35-44: The code registers the live variable paymentSetting in init
(via config.GlobalConfig.Register("payment_setting", &paymentSetting)), allowing
reflection-based readers to access the mutable struct without acquiring
paymentSettingRWMutex and race with UpdatePaymentSetting; fix by changing the
registration to expose a safe snapshot getter instead of the live pointer —
i.e., have Register use a function or wrapper that calls
GetPaymentSetting()/clonePaymentSettingLocked() to return a copy (or register
the result of clonePaymentSettingLocked) so reflective reads always operate on
an immutable clone; update any related code paths that expect the raw pointer to
use the getter/register-snapshot approach and keep UpdatePaymentSetting and
clonePaymentSettingLocked unchanged.
---
Nitpick comments:
In `@model/topup.go`:
- Around line 105-111: Remove the duplicate
normalizePaymentAutoSwitchGroupBaseGroup in model/topup.go and delegate to the
single implementation in setting/operation_setting/payment_setting.go: import
the payment_setting package, delete the local
normalizePaymentAutoSwitchGroupBaseGroup definition, and replace all calls with
payment_setting.NormalizePaymentAutoSwitchGroupBaseGroup(...); if the helper in
payment_setting.go is not exported, rename it to an exported identifier
(NormalizePaymentAutoSwitchGroupBaseGroup) so model/topup.go can reuse it.
In `@setting/operation_setting/payment_setting.go`:
- Around line 84-90: The function normalizePaymentAutoSwitchGroupBaseGroup is
duplicated; extract and export a single canonical implementation (e.g.,
NormalizePaymentAutoSwitchGroupBaseGroup) in the operation_setting package and
remove the duplicate in model/topup.go; update callers in both packages to
import operation_setting and call
operation_setting.NormalizePaymentAutoSwitchGroupBaseGroup, and ensure imports
are adjusted and tests/build updated so the default trimming/empty->"default"
behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 253867c5-0241-413f-be19-3a0e3565f8e5
📒 Files selected for processing (7)
controller/topup_waffo_pancake_test.gomodel/option.gomodel/subscription.gomodel/topup.gomodel/topup_auto_switch_test.gosetting/operation_setting/payment_setting.gosetting/operation_setting/payment_setting_test.go
Align Stripe top-up accumulation with money-based normalization, keep payment setting snapshots behind locked config access, and persist normalized payment option values consistently across DB, OptionMap, and runtime state.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model/subscription.go (1)
899-945:⚠️ Potential issue | 🟠 MajorDon't increment
expiredCountbefore the transaction succeeds.Line 909 updates
expiredCountbefore the later downgrade/group-switch work can fail. If this callback returns an error, the DB changes are rolled back but the outer counter stays incremented, so the function can report expired rows that never committed.🛠️ Suggested fix
for userId := range userIds { cacheGroup := "" + userExpiredCount := 0 err := DB.Transaction(func(tx *gorm.DB) error { res := tx.Model(&UserSubscription{}). Where("user_id = ? AND status = ? AND end_time > 0 AND end_time <= ?", userId, "active", now). Updates(map[string]interface{}{ "status": "expired", "updated_at": common.GetTimestamp(), }) if res.Error != nil { return res.Error } - expiredCount += int(res.RowsAffected) + userExpiredCount = int(res.RowsAffected) @@ return nil }) if err != nil { return expiredCount, err } + expiredCount += userExpiredCount if cacheGroup != "" { _ = UpdateUserGroupCache(userId, cacheGroup) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/subscription.go` around lines 899 - 945, The code increments the outer expiredCount inside the DB.Transaction callback (using expiredCount += int(res.RowsAffected)), which can be rolled back if the transaction later returns an error; change this by capturing the affected rows inside the transaction (e.g. local var rowsAffectedFromTx) but do NOT modify the outer expiredCount there, then after DB.Transaction returns successfully add rowsAffectedFromTx to expiredCount; locate DB.Transaction block, the res := tx.Model(&UserSubscription{})... Updates call and the use of expiredCount, and ensure functions like getUserGroupForUpdateTx, resolveUserEffectiveGroupTx and updateUserGroupTx remain inside the tx but the increment of expiredCount happens only on a successful transaction return.
♻️ Duplicate comments (3)
model/option.go (2)
233-255:⚠️ Potential issue | 🟠 MajorApply runtime state only after
DB.Transactionreturns successfully.Lines 251-255 still call
updateOptionMap/updatePaymentSettingOptionMapinside the transaction callback. Those mutatecommon.OptionMapand runtime globals immediately, so a later error or commit failure leaves memory on the new values while the DB rolls back.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 233 - 255, The transaction callback currently calls updateOptionMap and updatePaymentSettingOptionMap which mutate runtime globals while still inside DB.Transaction; instead, inside the transaction (in the DB.Transaction anonymous func) only perform DB operations and build/return the paymentSettingOptionValues map and a list/map of non-payment option key->value pairs (using keys and normalizedOptionValues), then after DB.Transaction returns successfully (err == nil) iterate over that collected non-payment map and call updateOptionMap(key, value) and call updatePaymentSettingOptionMap(paymentSettingOptionValues); reference DB.Transaction, updateOptionMap, updatePaymentSettingOptionMap, keys, normalizedOptionValues and ensure no global mutations happen inside the transaction callback.
625-640:⚠️ Potential issue | 🟠 MajorInvalid
payment_setting.*writes are still accepted silently.Both helpers rely on
config.UpdateConfigFromMap, which skips unknown keys and parse failures instead of returning an error. That meansUpdateOptionscan return success for malformed payment-setting updates while ignoring the requested change, which is especially risky becauseUpdateOptionsandUpdateOptionare exported entry points.Also applies to: 660-679
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/option.go` around lines 625 - 640, UpdateOptions currently calls config.UpdateConfigFromMap which silently skips unknown keys or parse failures; fix by validating the update succeeded for every incoming key: after calling config.UpdateConfigFromMap(paymentSetting, configMap) and converting back with config.ConfigToMap(paymentSetting), compare the original configMap keys (payment-setting keys passed into UpdateOptions/UpdateOption) against the keys present in paymentSettingMap (use paymentSettingConfigKey and paymentSettingOptionValues to map/normalize names) and return an explicit error if any requested key was not applied or failed to parse; alternatively replace the call with a strict updater that returns errors on unknown keys, ensuring UpdateOptions/UpdateOption surface failures instead of silently ignoring them (refer to operation_setting.GetPaymentSetting, config.UpdateConfigFromMap, config.ConfigToMap, paymentSettingConfigKey, paymentSettingOptionValues, normalizedOptionValues).model/topup.go (1)
92-101:⚠️ Potential issue | 🟠 MajorUse
PaymentProvideras the discriminator here.The provider-specific branches are keyed off
PaymentMethod, but the rest of this file treats gateway identity asPaymentProvider. If a Stripe/Creem top-up stores a channel inpayment_method, this code will normalize it with the wrong field or exclude it from the aggregate, so cumulative USD can be undercounted and the user never reaches the expected switch threshold.Suggested fix
- switch normalizeTopUpPaymentMethod(topUp.PaymentMethod) { + switch normalizeTopUpPaymentMethod(topUp.PaymentProvider) { case PaymentMethodStripe: return topUp.Money case PaymentMethodCreem: if common.QuotaPerUnit <= 0 { return 0 @@ - query := tx.Model(&TopUp{}). - Select("amount", "money", "payment_method"). + query := tx.Model(&TopUp{}). + Select("amount", "money", "payment_provider"). Where( - "user_id = ? AND status = ? AND ((LOWER(payment_method) = ? AND money > 0) OR (LOWER(payment_method) <> ? AND amount > 0))", + "user_id = ? AND status = ? AND ((LOWER(payment_provider) = ? AND money > 0) OR (LOWER(payment_provider) <> ? AND amount > 0))", userId, common.TopUpStatusSuccess, PaymentMethodStripe, PaymentMethodStripe, )Also applies to: 150-158
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/topup.go` around lines 92 - 101, The switch is using normalizeTopUpPaymentMethod(topUp.PaymentMethod) but the rest of the codebase uses PaymentProvider as the gateway discriminator; change the switch to use the provider field and its normalizer (e.g., normalizeTopUpPaymentProvider(topUp.PaymentProvider) or normalizeTopUpPaymentProvider(topUp.PaymentProvider.String())) and switch on PaymentProvider constants (PaymentProviderStripe, PaymentProviderCreem) instead of PaymentMethod; update the analogous switch at the other occurrence (around the 150-158 block) so both places consistently use topUp.PaymentProvider and the provider normalizer to compute the returned amounts.
🧹 Nitpick comments (1)
model/topup_auto_switch_test.go (1)
111-115: Run these assertions through a real transaction.These cases pass the root
DBhandle intoapplyTopUpAutoSwitchGroupTx, so they never exercise the same transaction/locking path used in production. That makes the tests too weak to catch MySQL/PostgreSQL regressions around theFOR UPDATE/locking behavior. Wrap the call inDB.Transaction(...)or callApplyTopUpAutoSwitchGroupinstead.Also applies to: 124-128, 147-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/topup_auto_switch_test.go` around lines 111 - 115, The test is calling applyTopUpAutoSwitchGroupTx with the root DB handle so it bypasses the transaction/locking path; change the test to execute the logic inside a real transaction by either wrapping the call in DB.Transaction(...) (invoking applyTopUpAutoSwitchGroupTx inside the transaction callback) or by calling the higher-level ApplyTopUpAutoSwitchGroup which opens a transaction itself; update the assertions around switchedGroup/getUserGroupForAutoSwitchTest to run after the transaction completes and apply the same change to the other occurrences mentioned (the blocks around the calls at the other lines).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model/subscription.go`:
- Around line 899-945: The code increments the outer expiredCount inside the
DB.Transaction callback (using expiredCount += int(res.RowsAffected)), which can
be rolled back if the transaction later returns an error; change this by
capturing the affected rows inside the transaction (e.g. local var
rowsAffectedFromTx) but do NOT modify the outer expiredCount there, then after
DB.Transaction returns successfully add rowsAffectedFromTx to expiredCount;
locate DB.Transaction block, the res := tx.Model(&UserSubscription{})... Updates
call and the use of expiredCount, and ensure functions like
getUserGroupForUpdateTx, resolveUserEffectiveGroupTx and updateUserGroupTx
remain inside the tx but the increment of expiredCount happens only on a
successful transaction return.
---
Duplicate comments:
In `@model/option.go`:
- Around line 233-255: The transaction callback currently calls updateOptionMap
and updatePaymentSettingOptionMap which mutate runtime globals while still
inside DB.Transaction; instead, inside the transaction (in the DB.Transaction
anonymous func) only perform DB operations and build/return the
paymentSettingOptionValues map and a list/map of non-payment option key->value
pairs (using keys and normalizedOptionValues), then after DB.Transaction returns
successfully (err == nil) iterate over that collected non-payment map and call
updateOptionMap(key, value) and call
updatePaymentSettingOptionMap(paymentSettingOptionValues); reference
DB.Transaction, updateOptionMap, updatePaymentSettingOptionMap, keys,
normalizedOptionValues and ensure no global mutations happen inside the
transaction callback.
- Around line 625-640: UpdateOptions currently calls config.UpdateConfigFromMap
which silently skips unknown keys or parse failures; fix by validating the
update succeeded for every incoming key: after calling
config.UpdateConfigFromMap(paymentSetting, configMap) and converting back with
config.ConfigToMap(paymentSetting), compare the original configMap keys
(payment-setting keys passed into UpdateOptions/UpdateOption) against the keys
present in paymentSettingMap (use paymentSettingConfigKey and
paymentSettingOptionValues to map/normalize names) and return an explicit error
if any requested key was not applied or failed to parse; alternatively replace
the call with a strict updater that returns errors on unknown keys, ensuring
UpdateOptions/UpdateOption surface failures instead of silently ignoring them
(refer to operation_setting.GetPaymentSetting, config.UpdateConfigFromMap,
config.ConfigToMap, paymentSettingConfigKey, paymentSettingOptionValues,
normalizedOptionValues).
In `@model/topup.go`:
- Around line 92-101: The switch is using
normalizeTopUpPaymentMethod(topUp.PaymentMethod) but the rest of the codebase
uses PaymentProvider as the gateway discriminator; change the switch to use the
provider field and its normalizer (e.g.,
normalizeTopUpPaymentProvider(topUp.PaymentProvider) or
normalizeTopUpPaymentProvider(topUp.PaymentProvider.String())) and switch on
PaymentProvider constants (PaymentProviderStripe, PaymentProviderCreem) instead
of PaymentMethod; update the analogous switch at the other occurrence (around
the 150-158 block) so both places consistently use topUp.PaymentProvider and the
provider normalizer to compute the returned amounts.
---
Nitpick comments:
In `@model/topup_auto_switch_test.go`:
- Around line 111-115: The test is calling applyTopUpAutoSwitchGroupTx with the
root DB handle so it bypasses the transaction/locking path; change the test to
execute the logic inside a real transaction by either wrapping the call in
DB.Transaction(...) (invoking applyTopUpAutoSwitchGroupTx inside the transaction
callback) or by calling the higher-level ApplyTopUpAutoSwitchGroup which opens a
transaction itself; update the assertions around
switchedGroup/getUserGroupForAutoSwitchTest to run after the transaction
completes and apply the same change to the other occurrences mentioned (the
blocks around the calls at the other lines).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1caf8d4d-d0f8-4dc9-8ca4-d8fa905d7a54
📒 Files selected for processing (11)
controller/topup_waffo_pancake_test.gomodel/option.gomodel/option_payment_setting_test.gomodel/subscription.gomodel/topup.gomodel/topup_auto_switch_test.gosetting/config/config.gosetting/config/config_test.gosetting/operation_setting/payment_setting.gosetting/operation_setting/payment_setting_test.goweb/src/components/settings/PaymentSetting.jsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
变更说明
payment_setting.auto_switch_group_enabled。payment_setting.auto_switch_group_rules。payment_setting.auto_switch_group_only_new_topups。payment_setting.auto_switch_group_base_group。upgrade_group逻辑。变更类型
安全性与兼容性
PaymentSetting对外读取改为快照副本，避免暴露全局共享的 map / slice。PaymentSetting更新路径统一加锁并复制可变字段，避免配置读写竞争。money计算，非 Stripe 普通充值按amount计算，和归一化逻辑保持一致。关联任务
验证摘要
当前测试规则
1 USD -> user110 USD -> vip100 USD -> svip已完成验证
default/user1/vip/svip初始分组、无历史充值、有历史充值、跨阈值累计、Stripe 累计、Creem + Epay 混合累计、重复补单幂等，以及 active subscription 用户再次普通充值。failed/expired历史订单不参与累计、订阅失效后的回退、active subscription 保护、关闭自动切换、单规则配置、单规则未达阈值、未命中任何阈值。1 / 10 / 100 USD精确命中、failed/expired订单不能补成成功、同一订单并发重复完成、active subscription 不会被普通充值覆盖、subscription 失效后会回退到正确分组、链外分组不会被自动切组覆盖。测试命令
go test ./setting/config ./setting/operation_setting ./model ./controller额外验证
结论
upgrade_group不会被普通充值覆盖。Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests