Skip to content

fix: portal auth setting #7762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 9, 2026
Merged

fix: portal auth setting #7762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ui/src/views/system/chat-management/portal-setting/components/ButtonPortalAuthSetting.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@
import { computed, reactive, ref, useTemplateRef } from 'vue'
import type { FormInstance, FormRules } from 'element-plus'
import { LOGIN_METHOD } from '@/api/enums'
import type { PortalAuthConfig, PortalSetting, PortalSettingPayload } from '@/api/types'
import { LOGIN_METHOD_LABELS } from '@/constants'
import type { LoginMethod, PortalAuthConfig, PortalSetting, PortalSettingPayload } from '@/api/types'

const props = defineProps<{ disabled: boolean; setting: PortalSetting; saving: boolean; save: (payload: PortalSettingPayload) => Promise<void> }>()

Expand Down Expand Up @@ -115,7 +116,7 @@ defineExpose({ open })
<el-form-item label="登录方式" prop="login_value">
<el-checkbox-group v-model="authForm.login_value" @change="handleLoginMethodsChange" class="flex-wrap">
<el-checkbox v-for="loginMethodOption in loginMethodOptions" :key="loginMethodOption.value" :value="loginMethodOption.value" class="w-32">
{{ loginMethodOption.label }}
{{ LOGIN_METHOD_LABELS[loginMethodOption.value as LoginMethod] || loginMethodOption.label }}
</el-checkbox>
</el-checkbox-group>
</el-form-item>
Expand All @@ -125,7 +126,7 @@ defineExpose({ open })
v-for="loginMethodOption in defaultLoginMethodOptions"
:key="loginMethodOption.value"
:value="loginMethodOption.value"
:label="loginMethodOption.label"
:label="LOGIN_METHOD_LABELS[loginMethodOption.value as LoginMethod] || loginMethodOption.label"
/>
</el-select>
</el-form-item>

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my review of the diff:

Issues & Concerns

  1. Redundant fallback logic — LOGIN_METHOD_LABELS[...] || loginMethodOption.label suggests uncertainty about whether loginMethodOption.value is a valid LoginMethod. If it isn't, using it as an index is unsafe; if it is, the || fallback pollutes intent. Pick one source of truth.

  2. Unsafe type casting (as LoginMethod) — repeated inline casts in the template obscure the real type of loginMethodOption.value. Better to type loginMethodOptions / defaultLoginMethodOptions correctly at their source (e.g., Array<{ value: LoginMethod; label: string }>), then drop the casts.

  3. Inconsistent data sources — if LOGIN_METHOD_LABELS is now the canonical label map, why keep label on the option objects at all? Maintaining labels in two places invites drift. Either:

    • Remove label from the options and rely solely on LOGIN_METHOD_LABELS, or
    • Generate the options from LOGIN_METHOD_LABELS (single iteration over Object.entries).

  4. Missing-key behavior — LOGIN_METHOD_LABELS[k] || label silently falls back when a key is missing. That may hide real bugs (e.g., a new LOGIN_METHOD added without a label). Consider ?? (nullish) instead of || — but note that if a label can legitimately be '', even ?? won't help; in that case use explicit in checks or TypeScript exhaustiveness.

  5. Computed for label lookup (optimization) — if the checkbox/select lists are stable, precompute a mapped list once with computed() rather than doing the dictionary lookup per-item per-render:

    const labeledLoginMethodOptions = computed(() =>
  loginMethodOptions.map(o => ({ ...o, label: LOGIN_METHOD_LABELS[o.value] ?? o.label }))
)
const labeledDefaultLoginMethodOptions = computed(() =>
  defaultLoginMethodOptions.map(o => ({ ...o, label: LOGIN_METHOD_LABELS[o.value] ?? o.label }))
)

    Then iterate those in the template with no casts.

  6. Logic duplication — the same "resolve label" logic appears twice. Extract into a helper:

    const resolveLabel = (opt: { value: LoginMethod; label: string }) =>
  LOGIN_METHOD_LABELS[opt.value] ?? opt.label

  7. Ensure LoginMethod and LOGIN_METHOD_LABELS stay in sync — define LOGIN_METHOD_LABELS as Record<LoginMethod, string> so TypeScript enforces exhaustiveness. This removes the need for the cast entirely.

  8. Import location consistency — LOGIN_METHOD comes from @/api/enums but LOGIN_METHOD_LABELS comes from @/constants. Consider co-locating them (both in enums, or both in constants) to avoid future divergence.

  9. Minor: blank line / diff noise — the diff looks clean otherwise; no unrelated changes detected.

Suggested Rewrite (sketch)

// types
export const LOGIN_METHOD_LABELS: Record<LoginMethod, string> = { /* ... */ }

// component
const labeledOptions = computed(() =>
  loginMethodOptions.map(o => ({ ...o, label: LOGIN_METHOD_LABELS[o.value] }))
)
<el-checkbox v-for="o in labeledOptions" :key="o.value" :value="o.value" class="w-32">
  {{ o.label }}
</el-checkbox>

This removes casts, removes duplication, enforces label coverage at compile time, and avoids per-render dictionary lookups.

Expand Down
7 changes: 4 additions & 3 deletions ui/src/views/system/settings/authentication/components/LoginSetting.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import { pick } from 'lodash'
import CurrentUserApi from '@/api/admin/auth/current-user'
import AuthSettingApi from '@/api/admin/system/settings/auth-setting'
import UserGroupsApi from '@/api/admin/system/user-groups'
import type { ListItem, LoginAuthSettingPayload, SystemUserGroup } from '@/api/types'
import type { ListItem, LoginAuthSettingPayload, LoginMethod, SystemUserGroup } from '@/api/types'
import type { FormInstance, FormRules } from 'element-plus'
import { LOGIN_METHOD, ROLE_TYPE } from '@/api/enums'
import { LOGIN_METHOD_LABELS } from '@/constants'
import { MsgSuccess } from '@/utils/message'
import { useStore } from '@/stores'
import perm from '@/permission'
Expand Down Expand Up @@ -171,7 +172,7 @@ onActivated(() => {
<el-form-item label="登录方式" prop="login_methods">
<el-checkbox-group v-model="form.login_methods" @change="handleLoginMethodsChange" class="flex-wrap">
<el-checkbox v-for="loginMethodOption in loginMethodOptions" :key="loginMethodOption.value" :value="loginMethodOption.value" class="w-40">
{{ loginMethodOption.label }}
{{ LOGIN_METHOD_LABELS[loginMethodOption.value as LoginMethod] || loginMethodOption.label }}
</el-checkbox>
</el-checkbox-group>
</el-form-item>
Expand All @@ -182,7 +183,7 @@ onActivated(() => {
v-for="loginMethodOption in defaultLoginMethodOptions"
:key="loginMethodOption.value"
:value="loginMethodOption.value"
:label="loginMethodOption.label"
:label="LOGIN_METHOD_LABELS[loginMethodOption.value as LoginMethod] || loginMethodOption.label"
/>
</el-select>
</el-form-item>

@shaohuzhang1 shaohuzhang1 Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Potential Issues

  1. Runtime lookup with type assertion (as LoginMethod)

    LOGIN_METHOD_LABELS[loginMethodOption.value as LoginMethod] || loginMethodOption.label
    • loginMethodOption.value is likely typed as string/number, so the as LoginMethod cast is unsafe — it bypasses type checking rather than verifying membership.
    • If loginMethodOption.value isn't actually a key of LOGIN_METHOD_LABELS, you get undefined and fall back silently. That hides misconfigurations instead of surfacing them.
    • Better: type loginMethodOptions / defaultLoginMethodOptions as arrays keyed by LoginMethod (e.g., Array<{ value: LoginMethod; label: string }>). Then no cast is needed and the compiler guarantees the key exists.

  2. Repeated lookup logic in the template
    The same expression appears twice. Templates re-evaluate on every render, so LOGIN_METHOD_LABELS[...] runs per item per render. This is minor but duplicated and noisy.

  3. Two sources of truth for labels
    You now have loginMethodOption.label and LOGIN_METHOD_LABELS. The || fallback means the constant wins when present, but both must be kept in sync. Consider removing label from the option objects entirely and deriving display text solely from LOGIN_METHOD_LABELS.

  4. Untyped import addition
    LoginMethod was added to the type import — confirm it's actually exported from @/api/types (the diff assumes so; a typo here breaks the build).

Optimization Suggestions

  1. Centralize resolution in a helper/computed

    const getLoginMethodLabel = (method: LoginMethod) =>
  LOGIN_METHOD_LABELS[method] ?? method

    Then use {{ getLoginMethodLabel(option.value) }} — avoids casts, single place to change, easier to test.

  2. Precompute options with labels
    Build loginMethodOptions as a computed that already maps values → labels:

    const loginMethodOptions = computed(() =>
  rawOptions.map(o => ({ ...o, label: LOGIN_METHOD_LABELS[o.value] ?? o.label }))
)

    Removes template logic and dedupes both call sites.

  3. Use ?? instead of ||
    If a label could legitimately be an empty string, || would incorrectly fall back. Prefer nullish coalescing:

    LOGIN_METHOD_LABELS[value] ?? fallbackLabel

  4. Explicitly type the record
    Ensure LOGIN_METHOD_LABELS is declared as Record<LoginMethod, string>. Then indexing with a properly typed value is checked at compile time, and missing keys become a type error — eliminating the need for runtime fallbacks.

Minor

  • <el-checkbox :value=...> — verify this is the correct prop for your Element Plus version (older versions used label for the bound value; newer ones use value). Mixed usage across the file (checkbox uses :value, select uses :value + label) is fine for current versions but worth double-checking against your element-plus version.
  • No functional/regression concerns otherwise; changes are backward-compatible thanks to the || fallback.

Expand Down
Loading