Topcoder Admin App - Roles Management Update #1056
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -9,20 +9,22 @@ import { | |||
| useMemo, | |||
| useRef, | |||
| } from 'react' | |||
| import ReactSelect, { components, SingleValue } from 'react-select' | |||
| import { components, SingleValue } from 'react-select' | |||
There was a problem hiding this comment.
The import statement for ReactSelect has been split into two separate imports. Consider consolidating them into a single import statement for better readability and to reduce redundancy.
| import classNames from 'classnames' | ||
|
|
||
| import { IconOutline, InputWrapper, LoadingSpinner } from '~/libs/ui' | ||
|
|
||
| import { SelectOption } from '../../models' | ||
| import ReactSelect from '../common/ReactSelectExport' |
There was a problem hiding this comment.
The import path for ReactSelect has been changed to a relative path. Ensure that this path is correct and that the ReactSelectExport module is available at the specified location. If this is intentional, verify that it aligns with the project's structure and conventions.
| @@ -60,6 +66,10 @@ export const FieldSingleSelect: FC<Props> = (props: Props) => { | |||
| [], | |||
| ) | |||
|
|
|||
| const Input = useMemo(() => ( | |||
| props.creatable ? CreatableReactSelect : ReactSelect | |||
There was a problem hiding this comment.
Consider renaming the Input variable to something more descriptive, such as SelectComponent, to better convey its purpose and improve code readability.
| @@ -72,7 +82,7 @@ export const FieldSingleSelect: FC<Props> = (props: Props) => { | |||
| hideInlineErrors={props.hideInlineErrors} | |||
| ref={wrapRef as MutableRefObject<HTMLDivElement>} | |||
| > | |||
| <ReactSelect | |||
| <Input | |||
There was a problem hiding this comment.
The component ReactSelect has been replaced with Input. Ensure that Input supports all the necessary props and functionality that ReactSelect provided, such as custom components, styling, and placeholder handling. Verify that asyncSelectComponents is compatible with Input.
| @@ -88,9 +98,13 @@ export const FieldSingleSelect: FC<Props> = (props: Props) => { | |||
| } | |||
| }} | |||
| value={props.value} | |||
| defaultValue={props.value} | |||
There was a problem hiding this comment.
The defaultValue prop is being set to props.value. Ensure that this is the intended behavior, as defaultValue is typically used for uncontrolled components, while value is for controlled components. Using both can lead to unexpected behavior.
| isDisabled={props.disabled || props.isLoading} | ||
| onBlur={props.onBlur} | ||
| options={props.options} | ||
| onInputChange={props.onSearchChange} |
There was a problem hiding this comment.
The onInputChange prop is being set to props.onSearchChange. Verify that props.onSearchChange is correctly handling the input change event and that it is intended to replace any existing input change logic.
| isDisabled={props.disabled || props.isLoading} | ||
| onBlur={props.onBlur} | ||
| options={props.options} | ||
| onInputChange={props.onSearchChange} | ||
| createOptionPosition='first' |
There was a problem hiding this comment.
The createOptionPosition prop is set to 'first'. Ensure this is the desired behavior, as it will affect where new options are added in the list.
| isDisabled={props.disabled || props.isLoading} | ||
| onBlur={props.onBlur} | ||
| options={props.options} | ||
| onInputChange={props.onSearchChange} | ||
| createOptionPosition='first' | ||
| formatCreateLabel={props.createLabel} |
There was a problem hiding this comment.
The formatCreateLabel prop is set to props.createLabel. Ensure that props.createLabel is a function that returns a string or React element, as expected by the formatCreateLabel prop.
| label='Email' | ||
| placeholder='Enter' | ||
| tabIndex={0} | ||
| onChange={_.noop} |
There was a problem hiding this comment.
The onChange handler is set to _.noop, which means it does nothing. If the email input field is intended to be functional, consider implementing an appropriate handler to manage changes to the email input value.
| @@ -1,16 +1,15 @@ | |||
| /** | |||
| * Role members table. | |||
| */ | |||
| import { FC, useContext, useEffect, useMemo } from 'react' | |||
| import { FC, useMemo } from 'react' | |||
There was a problem hiding this comment.
The useContext and useEffect hooks have been removed from the imports. Ensure that their removal does not affect any functionality that depends on context or side effects.
| @@ -108,20 +84,12 @@ export const RoleMembersTable: FC<Props> = (props: Props) => { | |||
| { | |||
| label: 'Handle', | |||
There was a problem hiding this comment.
The propertyName for the 'Handle' column is set to 'handle', but the renderer logic has been removed. Ensure that usersMapping is correctly handled elsewhere to display the handle, or consider re-implementing the renderer if necessary.
| type: 'text', | ||
| }, | ||
| { | ||
| label: 'Email', |
There was a problem hiding this comment.
The addition of the 'Email' column with propertyName: 'email' assumes that the data source includes an 'email' field. Verify that the data source provides this field to avoid potential runtime errors.
| { | ||
| ...columns[3], | ||
| className: '', | ||
| label: `${columns[3].label as string} label`, |
There was a problem hiding this comment.
The label concatenation with 'label' seems redundant. Consider revising the label assignment to avoid potential confusion.
| type: 'element', | ||
| }, | ||
| { | ||
| ...columns[3], |
There was a problem hiding this comment.
The use of ...columns[3] is repeated here. Ensure that this is intentional and that the properties from columns[3] are correctly applied to this new column.
| ], | ||
| [ | ||
| { | ||
| ...columns[3], | ||
| ...columns[4], |
There was a problem hiding this comment.
The spread operator is changed from columns[3] to columns[4]. Verify that this change is correct and that the intended column is being referenced.
| @@ -1,15 +1,33 @@ | |||
| /** | |||
| * Roles filter ui. | |||
| */ | |||
| import { FC, useCallback, useEffect } from 'react' | |||
| import { useForm, UseFormReturn } from 'react-hook-form' | |||
| import { | |||
There was a problem hiding this comment.
The import statement for FocusEvent is added but not used in the code. Consider removing it if it's not needed.
| ControllerRenderProps, | ||
| useForm, | ||
| UseFormReturn, | ||
| } from 'react-hook-form' | ||
| import _ from 'lodash' |
There was a problem hiding this comment.
The lodash import _ is present but not used in the code. Consider removing it if it's not necessary.
| import { yupResolver } from '@hookform/resolvers/yup' | ||
|
|
||
| import { FormRolesFilter, TableRolesFilter } from '../../models' | ||
| import { FieldSingleSelect } from '../FieldSingleSelect' |
There was a problem hiding this comment.
The FieldSingleSelect component is imported but not used in the code. Consider removing the import if it's not needed.
There was a problem hiding this comment.
@suppermancool - Can we fix this please?
There was a problem hiding this comment.
@jmgasper this is wrong the FieldSingleSelect is used in this file at line 123
| import { FieldSingleSelect } from '../FieldSingleSelect' | ||
| import { | ||
| FormRolesFilter, | ||
| SelectOption, |
There was a problem hiding this comment.
The SelectOption and UserRole imports are added but not used in the code. Consider removing them if they are not necessary.
There was a problem hiding this comment.
@suppermancool - Another one to fix, thanks.
There was a problem hiding this comment.
@jmgasper, this is wrong the SelectOption and UserRole are used in this file
|
|
||
| return ( | ||
| <form | ||
| className={classNames(styles.container, props.className)} | ||
| onSubmit={handleSubmit(onSubmit)} | ||
| onSubmit={_.noop} |
There was a problem hiding this comment.
The onSubmit handler is currently set to _.noop, which means the form will not perform any action on submission. If this is intentional, consider adding a comment explaining why the form submission is intentionally disabled. Otherwise, ensure that the form submission logic is correctly implemented.
There was a problem hiding this comment.
@suppermancool - A comment would be good here, thanks.
There was a problem hiding this comment.
@jmgasper done
| placeholder='Select' | ||
| value={controlProps.field.value} | ||
| onChange={function onChange(newValue: SelectOption) { | ||
| if (newValue.label === newValue.value) { |
There was a problem hiding this comment.
The condition if (newValue.label === newValue.value) assumes that the label and value are always the same for new options. Ensure this logic is correct for all use cases, as it might not handle cases where label and value differ.
| controlProps.field.onChange(newValue) | ||
| } | ||
| }} | ||
| onBlur={function onBlur(event: FocusEvent<HTMLInputElement, Element>) { |
There was a problem hiding this comment.
The onBlur event handler creates a new option if the input value is not empty. Consider whether this behavior is intended, as it might lead to unexpected creation of options when the user simply navigates away from the input field.
| disabled={!isValid || props.isLoading || props.isAdding} | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
The onClick handler is replacing the type='submit' attribute. Ensure that the form submission behavior is not affected by this change, as the button will no longer trigger a form submit event by default.
| variant='danger' | ||
| onClick={function onClick() { | ||
| reset(defaultValues) | ||
| resetAllValue() |
There was a problem hiding this comment.
The function resetAllValue is used here, but it is not clear if it is defined or imported correctly. Ensure that resetAllValue is defined and imported properly to avoid runtime errors.
| @@ -10,7 +10,7 @@ import { | |||
| RoleMemberInfo, | |||
| UserRole, | |||
| } from '../models' | |||
| import { fetchRole, searchUsers, unassignRole } from '../services' | |||
| import { fetchRole, unassignRole } from '../services' | |||
There was a problem hiding this comment.
The searchUsers import has been removed. Ensure that this function is no longer needed in the codebase. If it is still required elsewhere, consider re-importing it in the appropriate file.
| memberId => ({ id: memberId }), | ||
| const allRoleMembers = _.filter( | ||
| roleInfo.subjects || [], | ||
| roleMember => (!!roleMember.handle || !!roleMember.email), |
There was a problem hiding this comment.
Consider using a more descriptive variable name instead of roleMember to clarify the context, such as member or subject.
| .map(roleMember => ({ | ||
| email: roleMember.email, | ||
| handle: roleMember.handle, | ||
| id: roleMember.userId, |
There was a problem hiding this comment.
The property userId is being used here, but it is not clear from the context if roleMember always contains this property. Ensure that roleMember.userId is always defined or handle the case where it might be undefined.
| @@ -225,56 +231,29 @@ export function useManagePermissionRoleMembers( | |||
| (filterData: FormRoleMembersFilters) => { | |||
There was a problem hiding this comment.
The removal of the server request logic for filtering by user handle may impact the ability to filter accurately when user handles are not available locally. Consider whether this change aligns with the intended functionality and whether it might require additional handling or documentation elsewhere in the application.
| payload: filteredMembers, | ||
| type: RolesActionType.FILTER_ROLE_MEMBERS_DONE, | ||
| if (filterData.userHandle) { | ||
| filteredMembers = _.filter(filteredMembers, { |
There was a problem hiding this comment.
Filtering by userHandle now directly uses local data without server requests. Ensure that the local data is comprehensive enough to support this filtering method effectively, as it might miss users not present in the local dataset.
| @@ -2,5 +2,8 @@ | |||
| * Model for roles filter form | |||
| */ | |||
| export type FormRolesFilter = { | |||
| roleName: string | |||
| roleName: { | |||
There was a problem hiding this comment.
The change from roleName: string to an object with label and value properties is a significant modification. Ensure that all parts of the application that use FormRolesFilter are updated to handle this new structure, as it may affect data binding and form handling logic.
| @@ -3,5 +3,6 @@ | |||
| */ | |||
| export interface RoleMemberInfo { | |||
| id: string | |||
| handle?: string | |||
| handle: string | null | |||
There was a problem hiding this comment.
Changing the type of handle from string? to string | null may affect how existing code handles optional values. Ensure that all usages of RoleMemberInfo are updated to handle null values appropriately.
| @@ -3,5 +3,6 @@ | |||
| */ | |||
| export interface RoleMemberInfo { | |||
| id: string | |||
| handle?: string | |||
| handle: string | null | |||
| email: string | null | |||
There was a problem hiding this comment.
Adding a new email field to the RoleMemberInfo interface requires updating all instances where this interface is used. Ensure that the new field is handled correctly in all relevant parts of the application.
| @@ -16,7 +16,11 @@ export interface UserRole { | |||
| modifiedAt: Date | |||
| modifiedAtString?: string | |||
| modifiedByHandle?: string | |||
| subjects?: string[] | |||
| subjects?: { | |||
There was a problem hiding this comment.
The userId field in the subjects array should be optional to maintain consistency with the other fields (email and handle) which are nullable. Consider changing userId: string to userId: string | null.
| roleName: Yup.object() | ||
| .shape({ | ||
| label: Yup.string() | ||
| .required('label id is required.'), |
There was a problem hiding this comment.
Consider changing the error message to 'Label is required.' for consistency with other error messages.
There was a problem hiding this comment.
@suppermancool - Can you make this change please?
There was a problem hiding this comment.
@jmgasper done
| label: Yup.string() | ||
| .required('label id is required.'), | ||
| value: Yup.string() | ||
| .required('value is required.'), |
There was a problem hiding this comment.
Consider changing the error message to 'Value is required.' for consistency with other error messages.
There was a problem hiding this comment.
@suppermancool - Another one, thanks.
There was a problem hiding this comment.
@jmgasper done
Challenge https://www.topcoder.com/challenges/90b9dcd4-24b0-48a5-aae1-12b6158c2cd6