You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "EnxDev (via GitHub)" <gi...@apache.org> on 2025/03/27 12:45:36 UTC
[PR] feat(List Users): Migrate List Users FAB to React [superset]
EnxDev opened a new pull request, #32882:
URL: https://github.com/apache/superset/pull/32882
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR migrates the List Users page from server-side rendering (SSR) to fully client-side rendering using React.
**Changes**
- Replaced server-side rendering logic with a React-based frontend implementation.
- Introduced API calls to fetch user data dynamically.
- Improved performance by reducing backend dependency.
- Ensured UI/UX consistency with the existing design.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
- Before users list page
- After users list page
- Before edit user page
- After edit user page
- Before add user page
- After add user page
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1) Add in your superset_config.py the following line:
`FAB_ADD_SECURITY_API = True`
2) Run `superset init`
3) Go to the settings on the menu and click `Users`
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(List Users): Migrate List Users FAB to React [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32882:
URL: https://github.com/apache/superset/pull/32882#discussion_r2042127834
##########
superset-frontend/src/views/routes.tsx:
##########
@@ -129,6 +134,10 @@
() => import(/* webpackChunkName: "RolesList" */ 'src/pages/RolesList'),
);
+const UsersList: LazyExoticComponent<any> = lazy(
Review Comment:
### Unsafe UsersList Component Type <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The UsersList component is typed with 'any', which bypasses TypeScript's type checking for the component's props and return type.
###### Why this matters
Using 'any' could hide potential runtime errors if the UsersList component's interface changes, leading to potential crashes or unexpected behavior in production.
###### Suggested change ∙ *Feature Preview*
Define a proper interface for the UsersList component and use it in the type declaration:
```typescript
interface UsersListProps {
// define required props here
}
const UsersList: LazyExoticComponent<ComponentType<UsersListProps>> = lazy(
() => import(/* webpackChunkName: "UsersList" */ 'src/pages/UsersList'),
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edc31ade-4c84-4ce6-9b71-844cc7636875/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edc31ade-4c84-4ce6-9b71-844cc7636875?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edc31ade-4c84-4ce6-9b71-844cc7636875?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edc31ade-4c84-4ce6-9b71-844cc7636875?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/edc31ade-4c84-4ce6-9b71-844cc7636875)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:72de2a3d-5e1d-4313-923a-79779854dfec -->
[](72de2a3d-5e1d-4313-923a-79779854dfec)
##########
superset-frontend/src/components/ListView/types.ts:
##########
@@ -74,7 +78,8 @@ export type InnerFilterValue =
| undefined
| string[]
| number[]
- | { label: string; value: string | number };
+ | { label: string; value: string | number }
+ | [number | null, number | null];
Review Comment:
### Ambiguous Numerical Range Type <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The numerical_range filter type was added but its InnerFilterValue type uses an array tuple without clear semantics for minimum and maximum values.
###### Why this matters
Using a generic tuple type makes the code less self-documenting and increases the chance of mixing up the min/max order in the application code, potentially leading to incorrect filter behavior.
###### Suggested change ∙ *Feature Preview*
Create a dedicated type for numerical range values to make the code more maintainable and self-documenting:
```typescript
interface NumericalRange {
min: number | null;
max: number | null;
}
export type InnerFilterValue =
| string
| boolean
| number
| null
| undefined
| string[]
| number[]
| { label: string; value: string | number }
| NumericalRange;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58f406a1-d4b2-4136-bac2-d884ee6a8582/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58f406a1-d4b2-4136-bac2-d884ee6a8582?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58f406a1-d4b2-4136-bac2-d884ee6a8582?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58f406a1-d4b2-4136-bac2-d884ee6a8582?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58f406a1-d4b2-4136-bac2-d884ee6a8582)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f4ef2f9a-c697-4ef4-9899-75c20471119a -->
[](f4ef2f9a-c697-4ef4-9899-75c20471119a)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
+ await SupersetClient.post({
+ endpoint: '/api/v1/security/users/',
+ jsonPayload: { ...payload },
+ });
+};
+
+export const updateUser = async (user_Id: number, values: FormValues) => {
+ await SupersetClient.put({
+ endpoint: `/api/v1/security/users/${user_Id}`,
+ jsonPayload: { ...values },
+ });
Review Comment:
### Missing Error Propagation in User Update <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The updateUser function doesn't handle potential API errors which could occur during the update operation.
###### Why this matters
Silent failures in user updates can lead to data inconsistency and poor user experience.
###### Suggested change ∙ *Feature Preview*
```typescript
try {
await SupersetClient.put({
endpoint: `/api/v1/security/users/${user_Id}`,
jsonPayload: { ...values },
});
} catch (error) {
throw new Error(`Failed to update user ${user_Id}: ${error.message || error}`);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e5168472-6f75-4b46-b489-67142357f893/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e5168472-6f75-4b46-b489-67142357f893?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e5168472-6f75-4b46-b489-67142357f893?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e5168472-6f75-4b46-b489-67142357f893?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e5168472-6f75-4b46-b489-67142357f893)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c6a57fc0-9d15-4458-bac8-10fb211a2dcf -->
[](c6a57fc0-9d15-4458-bac8-10fb211a2dcf)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
+ await SupersetClient.post({
+ endpoint: '/api/v1/security/users/',
+ jsonPayload: { ...payload },
+ });
Review Comment:
### Missing Error Propagation in User Creation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The createUser function catches no errors from the API call which could fail due to network issues, validation errors, or server problems.
###### Why this matters
When API calls fail silently, debugging becomes difficult and the UI can't properly inform users about failures.
###### Suggested change ∙ *Feature Preview*
```typescript
try {
await SupersetClient.post({
endpoint: '/api/v1/security/users/',
jsonPayload: { ...payload },
});
} catch (error) {
throw new Error(`Failed to create user: ${error.message || error}`);
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b16ffe3-65ef-4a83-bbdb-b70f66dbbe71/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b16ffe3-65ef-4a83-bbdb-b70f66dbbe71?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b16ffe3-65ef-4a83-bbdb-b70f66dbbe71?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b16ffe3-65ef-4a83-bbdb-b70f66dbbe71?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b16ffe3-65ef-4a83-bbdb-b70f66dbbe71)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:153b6a03-a5b9-4342-b646-c7249effa2a2 -->
[](153b6a03-a5b9-4342-b646-c7249effa2a2)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
+ await SupersetClient.post({
+ endpoint: '/api/v1/security/users/',
+ jsonPayload: { ...payload },
+ });
+};
+
+export const updateUser = async (user_Id: number, values: FormValues) => {
+ await SupersetClient.put({
+ endpoint: `/api/v1/security/users/${user_Id}`,
+ jsonPayload: { ...values },
Review Comment:
### Unnecessary Object Spread in updateUser <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary object spread operator used in updateUser when passing values object directly would suffice.
###### Why this matters
Creating a new object through spread when the original object could be used directly adds unnecessary object creation overhead.
###### Suggested change ∙ *Feature Preview*
Remove the spread operator and pass values directly:
```typescript
jsonPayload: values,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/48294880-1e8b-42c4-bead-fc998941d6c8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/48294880-1e8b-42c4-bead-fc998941d6c8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/48294880-1e8b-42c4-bead-fc998941d6c8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/48294880-1e8b-42c4-bead-fc998941d6c8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/48294880-1e8b-42c4-bead-fc998941d6c8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:aedf1c18-2be8-476e-a2c5-97e26ec13ee6 -->
[](aedf1c18-2be8-476e-a2c5-97e26ec13ee6)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
+ await SupersetClient.post({
+ endpoint: '/api/v1/security/users/',
+ jsonPayload: { ...payload },
Review Comment:
### Unnecessary Object Spread <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Unnecessary object spread operator used when passing the payload object directly would suffice.
###### Why this matters
Creating a new object through spread when the original object could be used directly adds unnecessary object creation overhead.
###### Suggested change ∙ *Feature Preview*
Remove the spread operator and pass the payload directly:
```typescript
jsonPayload: payload,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/66fa3585-7176-4433-be47-3359ac8e3841/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/66fa3585-7176-4433-be47-3359ac8e3841?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/66fa3585-7176-4433-be47-3359ac8e3841?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/66fa3585-7176-4433-be47-3359ac8e3841?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/66fa3585-7176-4433-be47-3359ac8e3841)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0af09862-5efc-4720-a4a7-7715f31a8ada -->
[](0af09862-5efc-4720-a4a7-7715f31a8ada)
##########
superset-frontend/src/views/routes.tsx:
##########
@@ -129,6 +134,10 @@ const RolesList = lazy(
() => import(/* webpackChunkName: "RolesList" */ 'src/pages/RolesList'),
);
+const UsersList: LazyExoticComponent<any> = lazy(
+ () => import(/* webpackChunkName: "UsersList" */ 'src/pages/UsersList'),
+);
Review Comment:
### Unsafe Type Annotation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using <any> type annotation in LazyExoticComponent defeats TypeScript's type checking benefits and could mask potential runtime errors.
###### Why this matters
Type safety issues may not be caught during development, potentially leading to runtime errors and decreased code reliability.
###### Suggested change ∙ *Feature Preview*
Define proper type interface for the UsersList component and use it in the lazy loading:
```typescript
interface UsersListProps {
// define required props here
}
const UsersList: LazyExoticComponent<ComponentType<UsersListProps>> = lazy(
() => import('src/pages/UsersList')
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/386ed16f-26f5-4dbd-b3b3-f6a78540a394/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/386ed16f-26f5-4dbd-b3b3-f6a78540a394?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/386ed16f-26f5-4dbd-b3b3-f6a78540a394?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/386ed16f-26f5-4dbd-b3b3-f6a78540a394?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/386ed16f-26f5-4dbd-b3b3-f6a78540a394)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:cc310807-42b1-48cf-8cf8-e9256c4e518c -->
[](cc310807-42b1-48cf-8cf8-e9256c4e518c)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
+ await SupersetClient.post({
+ endpoint: '/api/v1/security/users/',
+ jsonPayload: { ...payload },
+ });
+};
+
+export const updateUser = async (user_Id: number, values: FormValues) => {
Review Comment:
### Inconsistent Parameter Naming Convention <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The parameter 'user_Id' uses inconsistent naming convention with snake_case instead of camelCase, which is the TypeScript standard.
###### Why this matters
Inconsistent naming conventions make the code harder to read and violate TypeScript style guidelines, potentially confusing developers about parameter naming patterns.
###### Suggested change ∙ *Feature Preview*
```typescript
export const updateUser = async (userId: number, values: FormValues)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38c17d59-c1c3-4888-a7e6-fa03003ae232/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38c17d59-c1c3-4888-a7e6-fa03003ae232?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38c17d59-c1c3-4888-a7e6-fa03003ae232?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38c17d59-c1c3-4888-a7e6-fa03003ae232?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/38c17d59-c1c3-4888-a7e6-fa03003ae232)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9936a61b-67cc-43b9-8ca9-221c905f09db -->
[](9936a61b-67cc-43b9-8ca9-221c905f09db)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
+ await SupersetClient.post({
+ endpoint: '/api/v1/security/users/',
+ jsonPayload: { ...payload },
+ });
+};
+
+export const updateUser = async (user_Id: number, values: FormValues) => {
+ await SupersetClient.put({
+ endpoint: `/api/v1/security/users/${user_Id}`,
+ jsonPayload: { ...values },
+ });
+};
Review Comment:
### Unnecessary Password Confirmation in Update Payload <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The updateUser function sends the entire form values including the confirmPassword field to the API, which is unnecessary and potentially problematic.
###### Why this matters
Sending the confirmPassword field to the API is redundant and could cause issues if the API is not properly sanitizing the input. This also unnecessarily increases the payload size.
###### Suggested change ∙ *Feature Preview*
Remove confirmPassword from the update payload similar to the createUser function:
```typescript
export const updateUser = async (user_Id: number, values: FormValues) => {
const { confirmPassword, ...payload } = values;
await SupersetClient.put({
endpoint: `/api/v1/security/users/${user_Id}`,
jsonPayload: payload,
});
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72b42265-894c-4406-aec1-7852f0d7ee84/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72b42265-894c-4406-aec1-7852f0d7ee84?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72b42265-894c-4406-aec1-7852f0d7ee84?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72b42265-894c-4406-aec1-7852f0d7ee84?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/72b42265-894c-4406-aec1-7852f0d7ee84)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5a53e2e0-9548-4236-8ed3-41ad907611bd -->
[](5a53e2e0-9548-4236-8ed3-41ad907611bd)
##########
superset-frontend/src/features/users/utils.ts:
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { SupersetClient } from '@superset-ui/core';
+import { FormValues } from './types';
+
+export const createUser = async (values: FormValues) => {
+ const { confirmPassword, ...payload } = values;
+ if (payload.active == null) {
+ payload.active = false;
+ }
Review Comment:
### Loose Equality Check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The null check uses loose equality (==) instead of strict equality (===), which is less explicit and readable.
###### Why this matters
Using loose equality makes the code's intent less clear as it doesn't explicitly show which values are being checked for, potentially leading to unexpected behavior.
###### Suggested change ∙ *Feature Preview*
```typescript
if (payload.active === null || payload.active === undefined) {
payload.active = false;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d775dcd-413c-455f-84ae-6a493dc283a8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d775dcd-413c-455f-84ae-6a493dc283a8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d775dcd-413c-455f-84ae-6a493dc283a8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d775dcd-413c-455f-84ae-6a493dc283a8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8d775dcd-413c-455f-84ae-6a493dc283a8)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:da63c1cd-3e1d-4e66-8ede-45db7239b609 -->
[](da63c1cd-3e1d-4e66-8ede-45db7239b609)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(List Users): Migrate List Users FAB to React [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #32882:
URL: https://github.com/apache/superset/pull/32882#issuecomment-2842560753
> I am merging this but we are missing feature parity with the addition of users in groups. There is a follow-up PR for the List Groups FAB view migration, we can re-implement that there. Currently, the same functionality can be achieved through the old List Groups FAB view in master by editing a group and adding a user through the multi-select list.
- Should be done here #33301 #2339
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(List Users): Migrate List Users FAB to React [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32882:
URL: https://github.com/apache/superset/pull/32882#issuecomment-2757913094
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](7688a0f4-e91b-4960-bbbe-d5c92a6a127d)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(List Users): Migrate List Users FAB to React [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido merged PR #32882:
URL: https://github.com/apache/superset/pull/32882
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org