Copilot commented on code in PR #36764:
URL: https://github.com/apache/superset/pull/36764#discussion_r2673540418
##########
superset-frontend/src/pages/UsersList/index.tsx:
##########
@@ -564,6 +578,22 @@ function UsersList({ user }: UsersListProps) {
title={t('Delete User?')}
/>
)}
+
+ {modalState.password_change && currentUser && (
+ <UserListPasswordChangeModal
+ user={currentUser}
+ show={modalState.password_change}
+ onHide={() => closeModal(ModalType.PASSWORD_CHANGE)}
+ onSave={() => {
+ refreshData();
+ closeModal(ModalType.PASSWORD_CHANGE);
+ }}
+ roles={roles}
+ groups={groups}
Review Comment:
The roles and groups props are passed to UserListPasswordChangeModal but are
not needed for password changes. Since showAssociationFields is false when
isPasswordChange is true, these props serve no purpose in this context and
should be removed to avoid confusion.
```suggestion
```
##########
superset-frontend/src/pages/UsersList/index.tsx:
##########
@@ -352,6 +359,13 @@ function UsersList({ user }: UsersListProps) {
icon: 'DeleteOutlined',
onClick: handleDelete,
},
+ {
+ label: 'user-list-password-action',
+ tooltip: t('Change password'),
+ placement: 'bottom',
+ icon: 'KeyOutlined',
+ onClick: handlePasswordChange,
+ }
Review Comment:
The new password change functionality lacks test coverage. Similar to the
existing tests for "opens add modal when Add User button is clicked" and "open
edit modal when edit button is clicked", there should be a test to verify that
clicking the password change button opens the password change modal correctly.
##########
superset-frontend/src/pages/UsersList/index.tsx:
##########
@@ -564,6 +578,22 @@ function UsersList({ user }: UsersListProps) {
title={t('Delete User?')}
/>
)}
+
+ {modalState.password_change && currentUser && (
+ <UserListPasswordChangeModal
+ user={currentUser}
+ show={modalState.password_change}
+ onHide={() => closeModal(ModalType.PASSWORD_CHANGE)}
+ onSave={() => {
+ refreshData();
+ closeModal(ModalType.PASSWORD_CHANGE);
+ }}
+ roles={roles}
+ groups={groups}
+ />
+ )}
+
+
Review Comment:
Extra blank line should be removed. There are two consecutive blank lines
here, which is inconsistent with the rest of the file's formatting.
```suggestion
```
##########
superset-frontend/src/pages/UsersList/index.tsx:
##########
@@ -352,6 +359,13 @@ function UsersList({ user }: UsersListProps) {
icon: 'DeleteOutlined',
onClick: handleDelete,
},
+ {
+ label: 'user-list-password-action',
+ tooltip: t('Change password'),
+ placement: 'bottom',
+ icon: 'KeyOutlined',
+ onClick: handlePasswordChange,
+ }
Review Comment:
Missing comma after the action object. This should be added for consistency
with the other action objects in the array and to prevent potential syntax
errors.
```suggestion
},
```
##########
superset-frontend/src/pages/UsersList/index.tsx:
##########
@@ -336,6 +339,10 @@ function UsersList({ user }: UsersListProps) {
openModal(ModalType.EDIT);
};
const handleDelete = () => setUserCurrentlyDeleting(original);
+ const handlePasswordChange = () => {
+ setCurrentUser(original);
+ openModal(ModalType.PASSWORD_CHANGE);
+ }
Review Comment:
Missing semicolon at the end of the function declaration.
JavaScript/TypeScript best practices recommend consistent use of semicolons.
```suggestion
};
```
--
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]
