codeant-ai-for-open-source[bot] commented on code in PR #36764:
URL: https://github.com/apache/superset/pull/36764#discussion_r2634389703
##########
superset-frontend/src/features/users/UserListModal.tsx:
##########
@@ -135,99 +177,110 @@ function UserListModal({
>
{(form: FormInstance) => (
<>
- <FormItem
- name="first_name"
- label={t('First name')}
- rules={[{ required: true, message: t('First name is required') }]}
- >
- <Input
- name="first_name"
- placeholder={t("Enter the user's first name")}
- />
- </FormItem>
- <FormItem
- name="last_name"
- label={t('Last name')}
- rules={[{ required: true, message: t('Last name is required') }]}
- >
- <Input
- name="last_name"
- placeholder={t("Enter the user's last name")}
- />
- </FormItem>
- <FormItem
- name="username"
- label={t('Username')}
- rules={[{ required: true, message: t('Username is required') }]}
- >
- <Input
- name="username"
- placeholder={t("Enter the user's username")}
- />
- </FormItem>
- <FormItem
- name="active"
- label={t('Is active?')}
- valuePropName="checked"
- >
- <Checkbox
- onChange={checked => {
- form.setFieldsValue({ isActive: checked });
- }}
- />
- </FormItem>
- <FormItem
- name="email"
- label={t('Email')}
- rules={[
- { required: true, message: t('Email is required') },
- {
- type: 'email',
- message: t('Please enter a valid email address'),
- },
- ]}
- >
- <Input name="email" placeholder={t("Enter the user's email")} />
- </FormItem>
- <FormItem
- name="roles"
- label={t('Roles')}
- dependencies={['groups']}
- rules={[atLeastOneRoleOrGroup('groups')]}
- >
- <Select
- name="roles"
- mode="multiple"
- placeholder={t('Select roles')}
- options={roles.map(role => ({
- value: role.id,
- label: role.name,
- }))}
- getPopupContainer={trigger =>
- trigger.closest('.ant-modal-content')
- }
- />
- </FormItem>
- <FormItem
- name="groups"
- label={t('Groups')}
- dependencies={['roles']}
- rules={[atLeastOneRoleOrGroup('roles')]}
- >
- <Select
- name="groups"
- mode="multiple"
- placeholder={t('Select groups')}
- options={groups.map(group => ({
- value: group.id,
- label: group.name,
- }))}
- getPopupContainer={trigger =>
- trigger.closest('.ant-modal-content')
- }
- />
- </FormItem>
- {!isEditMode && (
+ {showProfileFields && (
+ <>
+ <FormItem
+ name="first_name"
+ label={t('First name')}
+ rules={[{ required: true, message: t('First name is required')
}]}
+ >
+ <Input
+ name="first_name"
+ placeholder={t("Enter the user's first name")}
+ />
+ </FormItem>
+ <FormItem
+ name="last_name"
+ label={t('Last name')}
+ rules={[{ required: true, message: t('Last name is required')
}]}
+ >
+ <Input
+ name="last_name"
+ placeholder={t("Enter the user's last name")}
+ />
+ </FormItem>
+ <FormItem
+ name="username"
+ label={t('Username')}
+ rules={[{ required: true, message: t('Username is required')
}]}
+ >
+ <Input
+ name="username"
+ placeholder={t("Enter the user's username")}
+ />
+ </FormItem>
+ <FormItem
+ name="active"
+ label={t('Is active?')}
+ valuePropName="checked"
+ >
+ <Checkbox
+ onChange={checked => {
+ form.setFieldsValue({ isActive: checked });
Review Comment:
**Suggestion:** The "Is active?" checkbox updates a form field named
`isActive`, which is not the field bound to the form item (`active`) and is not
used elsewhere in this component; this means the actual active flag may not be
updated as intended and an unexpected `isActive` value may be sent to the API.
Update the handler to set the `active` field instead so the checkbox correctly
controls the active status. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
form.setFieldsValue({ active: checked });
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The FormItem is bound to the "active" field but the Checkbox onChange sets
"isActive". That mismatch means the form model won't receive updates for the
"active" key when the checkbox changes, so the submitted values (or visible
checkbox state) may be incorrect. Changing the setter to form.setFieldsValue({
active: checked }) fixes a real logic bug and aligns the controlled input with
the form field.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/users/UserListModal.tsx
**Line:** 219:219
**Comment:**
*Logic Error: The "Is active?" checkbox updates a form field named
`isActive`, which is not the field bound to the form item (`active`) and is not
used elsewhere in this component; this means the actual active flag may not be
updated as intended and an unexpected `isActive` value may be sent to the API.
Update the handler to set the `active` field instead so the checkbox correctly
controls the active status.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/pages/UsersList/index.tsx:
##########
@@ -38,6 +38,7 @@ import { isUserAdmin } from
'src/dashboard/util/permissionUtils';
import {
UserListAddModal,
UserListEditModal,
+ UserListPasswordChangeModal,
} from 'src/features/users/UserListModal';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { deleteUser } from 'src/features/users/utils';
Review Comment:
**Suggestion:** The component `UserListPasswordChangeModal` is imported from
`src/features/users/UserListModal`, but that module does not export it anywhere
in the codebase, so this import and its usage will cause a
build-time/module-resolution error; you can fix this by importing only the
existing `UserListEditModal` and defining a local alias
`UserListPasswordChangeModal` that reuses it. [type error]
**Severity Level:** Minor ⚠️
```suggestion
} from 'src/features/users/UserListModal';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { deleteUser } from 'src/features/users/utils';
import { fetchPaginatedData } from 'src/utils/fetchOptions';
import type { UsersListProps, Group, Role, UserObject } from './types';
const UserListPasswordChangeModal = UserListEditModal;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
I verified that there is no exported UserListPasswordChangeModal in the
codebase: the module exports UserListAddModal and UserListEditModal (see
src/features/users/UserListModal.tsx). The current PR imports and uses
UserListPasswordChangeModal, which will cause a build-time module resolution
error. Replacing the import with only the exported components and providing a
local alias (or otherwise ensuring a proper export) is a practical way to
unblock the build. This change directly fixes a real bug rather than being a
purely cosmetic preference.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/UsersList/index.tsx
**Line:** 40:44
**Comment:**
*Type Error: The component `UserListPasswordChangeModal` is imported
from `src/features/users/UserListModal`, but that module does not export it
anywhere in the codebase, so this import and its usage will cause a
build-time/module-resolution error; you can fix this by importing only the
existing `UserListEditModal` and defining a local alias
`UserListPasswordChangeModal` that reuses it.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
