Topcoder Admin App - Add SSOLogin to User Management #1121
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -67,7 +67,7 @@ export const DialogEditUserEmail: FC<Props> = (props: Props) => { | |||
| className={classNames(styles.container, props.className)} | |||
| onSubmit={handleSubmit(onSubmit)} | |||
| > | |||
| <div> | |||
| <div className={styles.blockForm}> | |||
There was a problem hiding this comment.
Consider verifying that styles.blockForm is defined and correctly imported to ensure it applies the intended styles.
| @@ -0,0 +1,71 @@ | |||
| .modal { | |||
| width: 800px !important; | |||
There was a problem hiding this comment.
Avoid using !important unless absolutely necessary, as it can make the CSS harder to maintain and override.
| align-items: flex-start; | ||
|
|
||
| th:first-child { | ||
| padding-left: 16px !important; |
There was a problem hiding this comment.
Consider removing !important from padding-left to maintain CSS specificity and avoid potential conflicts.
| } | ||
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
The use of !important for white-space can lead to specificity issues. Try to refactor the CSS to avoid using !important.
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; | ||
| text-align: left !important; |
There was a problem hiding this comment.
Avoid using !important for text-align. Consider restructuring the CSS to achieve the desired effect without it.
| width: 100%; | ||
| } | ||
|
|
||
| .btnActions { |
There was a problem hiding this comment.
The .btnActions class appears to have the same styles as .actionButtons. Consider consolidating these classes to reduce redundancy.
| props.setOpen(false) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isLoading]) |
There was a problem hiding this comment.
The useCallback dependency array is missing props.setOpen. Consider adding it to ensure the callback updates correctly when props.setOpen changes.
| variant='danger' | ||
| label='Remove' | ||
| onClick={function onClick() { | ||
| doRemoveSSOUserLogin(data) |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with other handlers in the code.
| <Button | ||
| primary | ||
| label='Edit' | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with other handlers in the code.
| <Button | ||
| primary | ||
| type='submit' | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with other handlers in the code.
| userId: props.editingData.userId, | ||
| }) | ||
| } | ||
| }, [props.editingData]) |
There was a problem hiding this comment.
Consider adding reset to the dependency array to ensure that the function is stable and doesn't change between renders.
| onChange={controlProps.field.onChange} | ||
| classNameWrapper={styles.inputField} | ||
| disabled={props.isAdding || isEditing} | ||
| dirty |
There was a problem hiding this comment.
The disabled prop for the InputSelectReact component is set to props.isAdding || isEditing. This means the provider cannot be changed when editing. If this is not the intended behavior, consider revising the condition.
| import { Pagination } from '../common/Pagination' | ||
| import { ReactComponent as RectangleListRegularIcon } from '../../assets/i/rectangle-list-regular-icon.svg' | ||
| import { fetchSSOLoginProviders } from '../../services' |
There was a problem hiding this comment.
Consider adding error handling for the fetchSSOLoginProviders function to ensure that any potential issues during the fetch process are managed appropriately.
| import { DialogEditUserTerms } from '../DialogEditUserTerms' | ||
| import { DialogEditUserStatus } from '../DialogEditUserStatus' | ||
| import { DialogUserStatusHistory } from '../DialogUserStatusHistory' | ||
| import { DropdownMenuButton } from '../common/DropdownMenuButton' | ||
| import { useTableFilterLocal, useTableFilterLocalProps } from '../../hooks' | ||
| import { useOnComponentDidMount, useTableFilterLocal, useTableFilterLocalProps } from '../../hooks' |
There was a problem hiding this comment.
The addition of useOnComponentDidMount suggests new functionality. Ensure that this hook is used correctly within the component to avoid any side effects or performance issues.
| @@ -359,6 +370,16 @@ export const UsersTable: FC<Props> = props => { | |||
| [isTablet, isMobile], | |||
| ) | |||
|
|
|||
| useOnComponentDidMount(() => { | |||
There was a problem hiding this comment.
Consider adding a dependency array to useOnComponentDidMount to ensure it behaves as expected. Without it, the effect might run more than once if the component re-renders.
| setSsoLoginProviders(result) | ||
| }) | ||
| .catch(e => { | ||
| handleError(e) |
There was a problem hiding this comment.
Ensure that handleError is defined and properly handles the error scenario. It might be helpful to log the error or provide user feedback.
| {showDialogEditUserSSOLogin && ( | ||
| <DialogEditUserSSOLogin | ||
| open | ||
| setOpen={function setOpen() { |
There was a problem hiding this comment.
Consider using an arrow function for setOpen to maintain consistency with modern React practices: setOpen={() => setShowDialogEditSSOLogin(undefined)}.
| case SSOUserLoginsActionType.UPDATE_SSO_USER_LOGIN_INIT: { | ||
| return { | ||
| ...previousState, | ||
| isAdding: true, |
There was a problem hiding this comment.
The isAdding state is being used for both adding and updating SSO user logins. Consider using a separate state for updating to avoid potential confusion or issues with UI feedback.
| ...previousState, | ||
| isAdding: false, | ||
| ssoUserLogins: previousState.ssoUserLogins.map( | ||
| item => (item.provider === action.payload.provider |
There was a problem hiding this comment.
The comparison item.provider === action.payload.provider assumes that provider is a unique identifier. Ensure that this is the case, or consider using a more robust method to identify the correct item to update.
| } | ||
|
|
||
| case SSOUserLoginsActionType.REMOVE_SSO_USER_LOGIN_DONE: { | ||
| const ssoUserLogins = _.filter( |
There was a problem hiding this comment.
Using _.filter from lodash here is fine, but if lodash is not heavily used elsewhere, consider using native JavaScript methods like Array.prototype.filter to reduce dependency on external libraries.
|
|
||
| /** | ||
| * Fetch list of sso login provider. | ||
| * @returns resolves to sso user logins |
There was a problem hiding this comment.
The return type in the JSDoc comment should be updated to reflect that it resolves to SSO login providers, not user logins. Consider changing @returns resolves to sso user logins to @returns resolves to sso login providers.
| * @param userId user id. | ||
| * @param userLogin user login info. | ||
| * @returns resolves to sso user login | ||
| */ |
There was a problem hiding this comment.
The return type in the JSDoc comment should specify that it resolves to the created SSO user login. Consider changing @returns resolves to sso user login to @returns resolves to the created sso user login.
| * Update sso user login. | ||
| * @param userId user id. | ||
| * @param userLogin user login info. | ||
| * @returns resolves to sso user login |
There was a problem hiding this comment.
The return type in the JSDoc comment should specify that it resolves to the updated SSO user login. Consider changing @returns resolves to sso user login to @returns resolves to the updated sso user login.
| * Delete sso user login. | ||
| * @param userId user id. | ||
| * @param provider login provider. | ||
| * @returns resolves to sso user login |
There was a problem hiding this comment.
The return type in the JSDoc comment should specify that it resolves to the deleted SSO user login. Consider changing @returns resolves to sso user login to @returns resolves to the deleted sso user login.
Challenge https://www.topcoder.com/challenges/adb6dd4a-998d-4e2e-b3bc-155f089b0581?tab=details