korbit-ai[bot] commented on code in PR #33631: URL: https://github.com/apache/superset/pull/33631#discussion_r2116191622
########## superset-frontend/src/pages/UserRegistrations/index.tsx: ########## @@ -0,0 +1,238 @@ +/** + * 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 { useMemo, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { + ListViewFilters, + ListViewFilterOperator, + ListView, + DeleteModal, +} from 'src/components'; +import { ActionProps, ActionsBar } from 'src/components/ListView/ActionsBar'; +import SubMenu from 'src/features/home/SubMenu'; + +const PAGE_SIZE = 25; + +export type UserRegistration = { + id: number; + username: string; + first_name: string; + last_name: string; + email: string; + registration_date: string; + registration_hash: string; +}; Review Comment: ### Missing documentation for registration_hash field <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The UserRegistration type interface lacks documentation explaining the purpose and significance of the registration_hash field. ###### Why this matters Without understanding the purpose of registration_hash, developers might misuse it or remove it during refactoring, potentially breaking the registration verification flow. ###### Suggested change ∙ *Feature Preview* export type UserRegistration = { id: number; username: string; first_name: string; last_name: string; email: string; registration_date: string; /** Unique hash used to verify and track the registration process */ registration_hash: string; }; ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/061e228c-f22e-4871-8133-7de3ec1124f8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/061e228c-f22e-4871-8133-7de3ec1124f8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/061e228c-f22e-4871-8133-7de3ec1124f8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/061e228c-f22e-4871-8133-7de3ec1124f8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/061e228c-f22e-4871-8133-7de3ec1124f8) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:31edded1-fcd9-407c-8d7b-1258e674c6c8 --> [](31edded1-fcd9-407c-8d7b-1258e674c6c8) ########## superset-frontend/src/pages/UserRegistrations/index.tsx: ########## @@ -0,0 +1,238 @@ +/** + * 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 { useMemo, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { + ListViewFilters, + ListViewFilterOperator, + ListView, + DeleteModal, +} from 'src/components'; +import { ActionProps, ActionsBar } from 'src/components/ListView/ActionsBar'; +import SubMenu from 'src/features/home/SubMenu'; + +const PAGE_SIZE = 25; + +export type UserRegistration = { + id: number; + username: string; + first_name: string; + last_name: string; + email: string; + registration_date: string; + registration_hash: string; +}; + +export default function UserRegistrations() { + const { addSuccessToast, addDangerToast } = useToasts(); + const [ + userRegistrationCurrentlyDeleting, + setUserRegistrationCurrentlyDeleting, + ] = useState<UserRegistration | null>(null); + + const { + state: { + loading, + resourceCount: registrationsCount, + resourceCollection: registrations, + }, + refreshData, + fetchData, + } = useListViewResource<UserRegistration>( + 'user_registrations', + t('User Registrations'), + addDangerToast, + ); + + const handleUserRegistrationDelete = async ({ + id, + username, + }: UserRegistration) => { + try { + await SupersetClient.delete({ + endpoint: `/api/v1/user_registrations/${id}`, + }); + refreshData(); + setUserRegistrationCurrentlyDeleting(null); + addSuccessToast(t('Deleted user registration for user: %s', username)); + } catch (error) { + addDangerToast( + t('There was an issue deleting registration for user: %s', username), + ); + } + }; + + const initialSort = [{ id: 'registration_date', desc: true }]; + + const columns = useMemo( + () => [ + { + accessor: 'username', + id: 'username', + Header: t('Username'), + Cell: ({ row: { original } }: any) => original.username, + }, Review Comment: ### Untyped Table Cell Components <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The columns array is using type 'any' for the Cell property's row parameter, which bypasses TypeScript's performance optimizations and type checking. ###### Why this matters Using 'any' type defeats TypeScript's ability to perform compile-time optimizations and type inference, potentially leading to runtime performance issues and type-related bugs. ###### Suggested change ∙ *Feature Preview* Define proper types for the row parameter in Cell components. Replace 'any' with the correct type definition: ```typescript Cell: ({ row: { original } }: { row: { original: UserRegistration } }) => original.username ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/610d640c-7f6a-4f9c-a7c1-82ebc2496a0f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/610d640c-7f6a-4f9c-a7c1-82ebc2496a0f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/610d640c-7f6a-4f9c-a7c1-82ebc2496a0f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/610d640c-7f6a-4f9c-a7c1-82ebc2496a0f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/610d640c-7f6a-4f9c-a7c1-82ebc2496a0f) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:5bf4d61f-f6e5-4d00-a96f-8a32c6692e7f --> [](5bf4d61f-f6e5-4d00-a96f-8a32c6692e7f) ########## superset/security/api.py: ########## @@ -333,3 +337,22 @@ def get_list(self, **kwargs: Any) -> Response: return self.response_403(message=str(e)) except Exception as e: return self.response_500(message=str(e)) + + +class UserRegistrationsRestAPI(BaseSupersetModelRestApi): + """ + APIs for listing user registrations (Admin only) + """ + + resource_name = "user_registrations" + datamodel = SQLAInterface(RegisterUser) + allow_browser_login = True + list_columns = [ + "id", + "username", + "email", + "first_name", + "last_name", + "registration_date", + "registration_hash", + ] Review Comment: ### Sensitive Registration Hash Exposure <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The registration_hash is exposed in the API response, which is a sensitive value that should not be visible. ###### Why this matters Exposing the registration hash could potentially allow malicious actors to hijack the registration process or impersonate users during registration. ###### Suggested change ∙ *Feature Preview* Remove the registration_hash from the list_columns: ```python list_columns = [ "id", "username", "email", "first_name", "last_name", "registration_date" ] ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb0e0574-39fc-4ccf-920a-3c0767aa7e74/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb0e0574-39fc-4ccf-920a-3c0767aa7e74?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb0e0574-39fc-4ccf-920a-3c0767aa7e74?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb0e0574-39fc-4ccf-920a-3c0767aa7e74?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/eb0e0574-39fc-4ccf-920a-3c0767aa7e74) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:18f15349-923f-4a03-b108-d05fdc8bb112 --> [](18f15349-923f-4a03-b108-d05fdc8bb112) ########## superset-frontend/src/pages/UserRegistrations/index.tsx: ########## @@ -0,0 +1,238 @@ +/** + * 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 { useMemo, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { + ListViewFilters, + ListViewFilterOperator, + ListView, + DeleteModal, +} from 'src/components'; +import { ActionProps, ActionsBar } from 'src/components/ListView/ActionsBar'; +import SubMenu from 'src/features/home/SubMenu'; + +const PAGE_SIZE = 25; + +export type UserRegistration = { + id: number; + username: string; + first_name: string; + last_name: string; + email: string; + registration_date: string; + registration_hash: string; +}; + +export default function UserRegistrations() { + const { addSuccessToast, addDangerToast } = useToasts(); + const [ + userRegistrationCurrentlyDeleting, + setUserRegistrationCurrentlyDeleting, + ] = useState<UserRegistration | null>(null); + + const { + state: { + loading, + resourceCount: registrationsCount, + resourceCollection: registrations, + }, + refreshData, + fetchData, + } = useListViewResource<UserRegistration>( + 'user_registrations', + t('User Registrations'), + addDangerToast, + ); + + const handleUserRegistrationDelete = async ({ + id, + username, + }: UserRegistration) => { + try { + await SupersetClient.delete({ + endpoint: `/api/v1/user_registrations/${id}`, + }); + refreshData(); + setUserRegistrationCurrentlyDeleting(null); + addSuccessToast(t('Deleted user registration for user: %s', username)); + } catch (error) { + addDangerToast( + t('There was an issue deleting registration for user: %s', username), + ); + } + }; + + const initialSort = [{ id: 'registration_date', desc: true }]; + + const columns = useMemo( + () => [ + { + accessor: 'username', + id: 'username', + Header: t('Username'), + Cell: ({ row: { original } }: any) => original.username, + }, + { + accessor: 'first_name', + id: 'first_name', + Header: t('First name'), + Cell: ({ row: { original } }: any) => original.first_name, + }, + { + accessor: 'last_name', + id: 'last_name', + Header: t('Last name'), + Cell: ({ row: { original } }: any) => original.last_name, + }, + { + accessor: 'email', + id: 'email', + Header: t('Email'), + Cell: ({ row: { original } }: any) => original.email, + }, + { + accessor: 'registration_hash', + id: 'registration_hash', + Header: t('Registration hash'), + Cell: ({ row: { original } }: any) => original.registration_hash, + }, + { + accessor: 'registration_date', + id: 'registration_date', + Header: t('Registration date'), + Cell: ({ row: { original } }: any) => original.registration_date, Review Comment: ### Unformatted Date Display <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The registration_date is displayed without any date formatting, potentially showing raw ISO strings to users. ###### Why this matters Raw date strings are not user-friendly and may be inconsistent with the application's date display conventions. ###### Suggested change ∙ *Feature Preview* Add date formatting to the registration_date cell: ```typescript import { DateTime } from '@superset-ui/core'; // In the columns definition: { accessor: 'registration_date', id: 'registration_date', Header: t('Registration date'), Cell: ({ row: { original } }: any) => DateTime.utc(original.registration_date).format('YYYY-MM-DD HH:mm:ss'), } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0fe65b2-9217-4199-b333-000b6341d74b/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0fe65b2-9217-4199-b333-000b6341d74b?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0fe65b2-9217-4199-b333-000b6341d74b?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0fe65b2-9217-4199-b333-000b6341d74b?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e0fe65b2-9217-4199-b333-000b6341d74b) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ace62be3-d428-41c6-86d7-49968dbc1d37 --> [](ace62be3-d428-41c6-86d7-49968dbc1d37) ########## superset-frontend/src/pages/UserRegistrations/index.tsx: ########## @@ -0,0 +1,238 @@ +/** + * 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 { useMemo, useState } from 'react'; +import { SupersetClient, t } from '@superset-ui/core'; +import { useListViewResource } from 'src/views/CRUD/hooks'; +import { useToasts } from 'src/components/MessageToasts/withToasts'; +import { + ListViewFilters, + ListViewFilterOperator, + ListView, + DeleteModal, +} from 'src/components'; +import { ActionProps, ActionsBar } from 'src/components/ListView/ActionsBar'; +import SubMenu from 'src/features/home/SubMenu'; + +const PAGE_SIZE = 25; + +export type UserRegistration = { + id: number; + username: string; + first_name: string; + last_name: string; + email: string; + registration_date: string; + registration_hash: string; +}; + +export default function UserRegistrations() { + const { addSuccessToast, addDangerToast } = useToasts(); + const [ + userRegistrationCurrentlyDeleting, + setUserRegistrationCurrentlyDeleting, + ] = useState<UserRegistration | null>(null); + + const { + state: { + loading, + resourceCount: registrationsCount, + resourceCollection: registrations, + }, + refreshData, + fetchData, + } = useListViewResource<UserRegistration>( + 'user_registrations', + t('User Registrations'), + addDangerToast, + ); + + const handleUserRegistrationDelete = async ({ + id, + username, + }: UserRegistration) => { + try { + await SupersetClient.delete({ + endpoint: `/api/v1/user_registrations/${id}`, + }); + refreshData(); + setUserRegistrationCurrentlyDeleting(null); + addSuccessToast(t('Deleted user registration for user: %s', username)); + } catch (error) { + addDangerToast( + t('There was an issue deleting registration for user: %s', username), + ); + } + }; Review Comment: ### Delete Modal Closes on Error <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The delete operation's error handling doesn't rethrow the error after showing the toast, allowing the modal to close even when deletion fails. ###### Why this matters Users might be misled into thinking the deletion was successful since the modal closes regardless of the operation's outcome. ###### Suggested change ∙ *Feature Preview* Modify the error handling to maintain the modal state on failure: ```typescript const handleUserRegistrationDelete = async ({ id, username, }: UserRegistration) => { try { await SupersetClient.delete({ endpoint: `/api/v1/user_registrations/${id}`, }); refreshData(); setUserRegistrationCurrentlyDeleting(null); addSuccessToast(t('Deleted user registration for user: %s', username)); } catch (error) { addDangerToast( t('There was an issue deleting registration for user: %s', username), ); throw error; } }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/599468f4-0bd4-4e73-895f-df5a6bff3518/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/599468f4-0bd4-4e73-895f-df5a6bff3518?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/599468f4-0bd4-4e73-895f-df5a6bff3518?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/599468f4-0bd4-4e73-895f-df5a6bff3518?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/599468f4-0bd4-4e73-895f-df5a6bff3518) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:286a8039-290c-4e9f-b94a-cb56230cf2db --> [](286a8039-290c-4e9f-b94a-cb56230cf2db) -- 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]
