Default challenge reviewer editing in System-Admin app / PM-2635 #1279
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const onSubmit = useCallback( | ||
| (data: FormAddDefaultReviewer) => { | ||
| const requestBody = _.pickBy(data, _.identity) |
There was a problem hiding this comment.
[❗❗ correctness]
Using _.pickBy(data, _.identity) to filter out falsy values might unintentionally remove valid data if any field has a falsy value that is still meaningful (e.g., 0 or false). Consider specifying which fields to include explicitly or using a different method to filter out only undefined or null values.
| <InputCheckbox | ||
| name='isMemberReview' | ||
| label='Is Member Review' | ||
| onChange={function onChange(event: Event) { |
There was a problem hiding this comment.
[correctness]
The onChange handler for InputCheckbox uses event.target without checking if event is defined. Although unlikely, if event is undefined, this could cause a runtime error. Consider adding a check to ensure event and event.target are defined.
| <InputCheckbox | ||
| name='shouldOpenOpportunity' | ||
| label='Should Open Opportunity' | ||
| onChange={function onChange(event: Event) { |
There was a problem hiding this comment.
[correctness]
The onChange handler for InputCheckbox uses event.target without checking if event is defined. Although unlikely, if event is undefined, this could cause a runtime error. Consider adding a check to ensure event and event.target are defined.
|
|
||
| .fields { | ||
| display: flex; | ||
| gap: 15px; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a variable for the gap value instead of hardcoding 15px. This will improve maintainability and consistency across the styles.
|
|
||
| .blockBottom { | ||
| display: flex; | ||
| gap: 10px; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a variable for the gap value instead of hardcoding 10px. This will improve maintainability and consistency across the styles.
| .blockBottom { | ||
| display: flex; | ||
| gap: 10px; | ||
| margin-top: 3px; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a variable for the margin-top value instead of hardcoding 3px. This will improve maintainability and consistency across the styles.
| label='Phase Name' | ||
| placeholder='Enter phase name' | ||
| tabIndex={0} | ||
| onChange={_.noop} |
There was a problem hiding this comment.
[maintainability]
Using _.noop for onChange might be misleading as it suggests that the input field does not handle changes. Consider removing onChange if it's not needed, or implementing a meaningful handler if necessary.
| const handleReset = useCallback(() => { | ||
| reset(defaultValues) | ||
| setTimeout(() => { | ||
| onSubmit(defaultValues) |
There was a problem hiding this comment.
[correctness]
Using setTimeout with a delay of 0 to call onSubmit after reset might lead to unexpected behavior if the component re-renders before the timeout executes. Consider using a callback or a more reliable method to ensure onSubmit is called after reset completes.
| } | ||
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make it difficult to override styles in the future and should be avoided unless absolutely necessary. Consider refactoring to avoid its use if possible.
|
|
||
| .tableCell { | ||
| white-space: break-spaces !important; | ||
| text-align: left !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make it difficult to override styles in the future and should be avoided unless absolutely necessary. Consider refactoring to avoid its use if possible.
| await deleteDefaultReviewer(recordToDelete.id) | ||
| toast.success('Default Reviewer deleted successfully') | ||
| if (props.page > 1 && props.datas.length === 1) { | ||
| props.setPage(props.page - 1) |
There was a problem hiding this comment.
[correctness]
The condition props.page > 1 && props.datas.length === 1 assumes that the last item on the current page is being deleted. Consider checking if the current page is empty after deletion to ensure the page decrement is necessary.
| size='sm' | ||
| label='Delete' | ||
| onClick={handleDeleteClick} | ||
| disabled={deletingId !== undefined} |
There was a problem hiding this comment.
[💡 usability]
The Delete button is disabled when deletingId is not undefined, which prevents any delete action while another is in progress. This is good for preventing multiple deletions but could be improved by providing user feedback or disabling only the relevant row's delete button.
| columns={columns} | ||
| data={props.datas} | ||
| removeDefaultSort | ||
| onToggleSort={_.noop} |
There was a problem hiding this comment.
[💡 maintainability]
Using _.noop for onToggleSort is a placeholder that might lead to confusion. Consider explicitly stating the reason for disabling sorting or removing the prop if it's not needed.
| const doUpdateDefaultReviewer = useCallback( | ||
| (data: Partial<FormAddDefaultReviewer>, callBack: () => void) => { | ||
| setIsAdding(true) | ||
| updateDefaultReviewer(defaultReviewerId ?? '', data) |
There was a problem hiding this comment.
[correctness]
Using defaultReviewerId ?? '' in updateDefaultReviewer could lead to unintended behavior if defaultReviewerId is undefined. Consider explicitly handling the case where defaultReviewerId is not provided, as passing an empty string might not be the desired behavior.
| const doRemoveDefaultReviewer = useCallback( | ||
| (callBack: () => void) => { | ||
| setIsRemoving(true) | ||
| deleteDefaultReviewer(defaultReviewerId ?? '') |
There was a problem hiding this comment.
[correctness]
Using defaultReviewerId ?? '' in deleteDefaultReviewer could lead to unintended behavior if defaultReviewerId is undefined. Consider explicitly handling the case where defaultReviewerId is not provided, as passing an empty string might not be the desired behavior.
| type: DefaultReviewersActionType.FETCH_DEFAULT_REVIEWERS_INIT, | ||
| }) | ||
|
|
||
| const encodeFilterValue = (value: string): string => encodeURIComponent(value) |
There was a problem hiding this comment.
[💡 performance]
Consider moving the encodeFilterValue function outside of the useTableFilterBackend callback to avoid redefining it on every render. This can improve performance slightly by reducing unnecessary function creation.
| } | ||
| } | ||
|
|
||
| loadData() |
There was a problem hiding this comment.
[maintainability]
The .catch(() => undefined) pattern suppresses all errors silently. Consider logging the error or handling it in a way that provides more insight into potential issues during the loadData execution.
| opportunityType?: string; | ||
| isAIReviewer: boolean; | ||
| shouldOpenOpportunity: boolean; | ||
| createdAt: string; |
There was a problem hiding this comment.
[correctness]
Consider using a more precise type for createdAt and updatedAt fields, such as Date, to ensure proper date handling and avoid potential issues with string manipulation.
| isAIReviewer: boolean; | ||
| shouldOpenOpportunity: boolean; | ||
| createdAt: string; | ||
| createdBy: string; |
There was a problem hiding this comment.
[correctness]
The createdBy and updatedBy fields are typed as string. Ensure that these fields are validated to contain valid user identifiers to prevent potential data integrity issues.
| description?: string; | ||
| isOpen: boolean; | ||
| duration: number; | ||
| createdAt: string; |
There was a problem hiding this comment.
[correctness]
Consider using a more specific type for createdAt and updatedAt fields, such as Date, to ensure proper date handling and avoid potential issues with string manipulation.
| duration: number; | ||
| createdAt: string; | ||
| createdBy: string; | ||
| updatedAt: string; |
There was a problem hiding this comment.
[correctness]
Consider using a more specific type for createdAt and updatedAt fields, such as Date, to ensure proper date handling and avoid potential issues with string manipulation.
| version: string; | ||
| minScore: number; | ||
| maxScore: number; | ||
| minimumPassingScore: number; |
There was a problem hiding this comment.
[correctness]
Consider adding validation to ensure minimumPassingScore is between minScore and maxScore to prevent logical errors.
| name: string; | ||
| description?: string; | ||
| isActive: boolean; | ||
| createdAt: string; |
There was a problem hiding this comment.
[correctness]
Consider using a more specific type for createdAt and updatedAt to ensure date validation and manipulation are handled correctly. Using Date instead of string can help prevent potential errors related to date formatting and parsing.
| description?: string; | ||
| isActive: boolean; | ||
| createdAt: string; | ||
| createdBy: string; |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more specific type for createdBy and updatedBy if these fields are meant to reference user IDs or specific entities. This can help maintain consistency and clarity in the data model.
| filter: string, | ||
| ): Promise<PaginatedResponse<DefaultReviewersResponsePayload>> => xhrGetPaginatedAsync< | ||
| DefaultReviewersResponsePayload | ||
| >(`${EnvironmentConfig.API.V6}/default-challenge-reviewers?${filter}`) |
There was a problem hiding this comment.
[❗❗ security]
Consider validating the filter parameter to ensure it does not contain any malicious content or unintended query strings that could lead to security vulnerabilities such as injection attacks.
| id: string, | ||
| ): Promise<DefaultChallengeReviewer> => { | ||
| const result = await xhrGetAsync<DefaultChallengeReviewer>( | ||
| `${EnvironmentConfig.API.V6}/default-challenge-reviewers/${id}`, |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the xhrGetAsync call to manage potential network or server errors gracefully.
| Partial<FormAddDefaultReviewer>, | ||
| DefaultChallengeReviewer | ||
| >(`${EnvironmentConfig.API.V6}/default-challenge-reviewers`, data) | ||
| return result |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the xhrPostAsync call to manage potential network or server errors gracefully.
| Partial<FormAddDefaultReviewer>, | ||
| DefaultChallengeReviewer | ||
| >(`${EnvironmentConfig.API.V6}/default-challenge-reviewers/${id}`, data) | ||
| return result |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the xhrPutAsync call to manage potential network or server errors gracefully.
| const result = await xhrDeleteAsync<ApiV5ResponseSuccess>( | ||
| `${EnvironmentConfig.API.V6}/default-challenge-reviewers/${id}`, | ||
| ) | ||
| return result |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the xhrDeleteAsync call to manage potential network or server errors gracefully.
|
|
||
| export const getPhases = async (): Promise<Phase[]> => { | ||
| const result = await xhrGetAsync<Phase[]>( | ||
| `${EnvironmentConfig.API.V6}/challenge-phases`, |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the xhrGetAsync call to manage potential network or API errors gracefully.
| export const getScorecards = async ( | ||
| options: GetScorecardsOptions = {}, | ||
| ): Promise<Scorecard[]> => { | ||
| const fetchAll = options.fetchAll ?? true |
There was a problem hiding this comment.
[performance]
The default value for fetchAll is set to true, which means that by default, all pages will be fetched. This could lead to performance issues if the dataset is large. Consider setting the default to false or ensuring that the caller is aware of this behavior.
| params.set('perPage', String(perPage)) | ||
|
|
||
| const response = await xhrGetPaginatedAsync<Scorecard[] | ScorecardPagePayload>( | ||
| `${EnvironmentConfig.API.V6}/scorecards?${params.toString()}`, |
There was a problem hiding this comment.
[❗❗ security]
The URL for the API endpoint is constructed using string interpolation. Ensure that EnvironmentConfig.API.V6 is properly sanitized to prevent any potential injection attacks.
|
|
||
| const totalPagesFromHeader = Number(response.totalPages) | ||
| const totalPagesFromPayload = getTotalPagesFromPayload(response.data) | ||
| const totalPages = totalPagesFromHeader || totalPagesFromPayload |
There was a problem hiding this comment.
[correctness]
The logic for determining totalPages prioritizes totalPagesFromHeader over totalPagesFromPayload. Ensure that this is the intended behavior and that the header value is always reliable.
| const totalPages = totalPagesFromHeader || totalPagesFromPayload | ||
|
|
||
| const hasMoreByHeader = totalPages > 0 && pageNumber < totalPages | ||
| const hasMoreByCount = totalPages <= 0 && batch.length === perPage |
There was a problem hiding this comment.
[correctness]
The condition totalPages <= 0 && batch.length === perPage assumes that if totalPages is not available, the presence of a full batch indicates more pages. This might not always be accurate if the API changes its behavior. Consider adding a safeguard or logging to handle unexpected cases.
|
|
||
| export const getTimelineTemplates = async (): Promise<TimelineTemplate[]> => { | ||
| const result = await xhrGetAsync<TimelineTemplate[]>( | ||
| `${EnvironmentConfig.API.V6}/timeline-templates`, |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the xhrGetAsync call to manage potential network or API errors gracefully.
| .optional() | ||
| .when('isMemberReview', { | ||
| is: true, | ||
| otherwise: schema => schema.optional(), |
There was a problem hiding this comment.
[💡 readability]
Consider using schema => schema.notRequired() instead of schema => schema.optional() for clarity. notRequired() is more explicit in indicating that the field is not required when the condition is not met.
| } | ||
|
|
||
| export const DefaultReviewersAddPage: FC<Props> = props => { | ||
| useAutoScrollTopWhenInit() |
There was a problem hiding this comment.
[maintainability]
The useAutoScrollTopWhenInit hook is called but its effect or necessity is not clear from the context provided. Ensure that this hook is necessary for the component's functionality and does not introduce side effects that could impact the user experience.
| pageTitle='Edit Default Reviewer' | ||
| className={classNames(styles.container, props.className)} | ||
| > | ||
| <DefaultReviewersAddForm /> |
There was a problem hiding this comment.
[maintainability]
Consider renaming DefaultReviewersAddForm to something more indicative of its purpose in this context, such as DefaultReviewersEditForm, to improve clarity and maintainability. The current name suggests an 'add' operation, which may be misleading in an 'edit' page.
| const [colWidth, setColWidth] = useState<colWidthType>({}) | ||
| const { | ||
| isLoading, | ||
| datas, |
There was a problem hiding this comment.
[💡 readability]
Consider renaming datas to data for clarity and to adhere to common naming conventions. The term data is typically used as both singular and plural in programming contexts.
| type: 'element', | ||
| }), | ||
| [], | ||
| [isApprovalColumn], |
There was a problem hiding this comment.
[performance]
The dependency array for useMemo now includes isApprovalColumn. Ensure that isApprovalColumn is a stable value or memoized itself to prevent unnecessary recalculations of reviewDateColumn.
| if (!isCompleted) { | ||
| return <span>--</span> | ||
| } | ||
|
|
There was a problem hiding this comment.
[💡 maintainability]
The logic for returning <span>--</span> when !isCompleted is duplicated. Consider refactoring to reduce repetition and improve maintainability.
|
|
||
| const allowColumn = isReviewPhase(challengeInfo) || (isFirst2Finish && hasCompletedIterativeReviews) | ||
| const allowColumn = isReviewPhase(challengeInfo) | ||
| || hasMyIterativeReviewAssignments |
There was a problem hiding this comment.
[correctness]
The condition hasMyIterativeReviewAssignments is added to allowColumn. Ensure that this condition is necessary and correctly implemented to avoid unintended behavior changes.
| 'post-mortem': normalizedRoleName => normalizedRoleName.includes('postmortem'), | ||
| review: normalizedRoleName => normalizedRoleName === 'reviewer', | ||
| review: normalizedRoleName => ( | ||
| normalizedRoleName.includes('reviewer') |
There was a problem hiding this comment.
[correctness]
The updated condition for the review phase role matcher now excludes roles containing 'checkpoint' and 'postmortem'. Ensure that this change aligns with the intended logic and does not inadvertently exclude valid roles that should be considered as 'reviewer'.
| normalizedRoleName: string, | ||
| ): boolean => { | ||
| if (!reviewPhaseType || currentPhaseReviewType !== reviewPhaseType) { | ||
| if (!reviewPhaseType) { |
There was a problem hiding this comment.
[correctness]
The refactored condition in canRoleEditPhase now separates the checks for reviewPhaseType and currentPhaseReviewType. Verify that this change does not alter the intended logic, especially in scenarios where currentPhaseReviewType is undefined.
https://topcoder.atlassian.net/browse/PM-2635