Feat/system admin #1108
Feat/system admin #1108
Conversation
Permission, Billing account, Review update
Topcoder Admin App - Misc Update 0505
Fix unapprove challenge id
Topcoder Admin App - Misc Update 0518
…6878 Topcoder Admin App - Marathon Match Functionality
Topcoder Admin App - Misc Update 0601
Admin billing accounts: always set payment_terms_id = 1 when send update
Topcoder Admin App - Misc Update 0610
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -136,14 +132,9 @@ export const adminRoutes: ReadonlyArray<PlatformRoute> = [ | |||
| route: ':challengeId/manage-user', | |||
| }, | |||
| { | |||
| element: <ManageResourcePage />, | |||
| element: <ManageSubmissionPage />, | |||
There was a problem hiding this comment.
The id for this route is still 'manage-resource', but the element has been changed to ManageSubmissionPage. Consider updating the id to reflect the new functionality, such as 'manage-submission'.
| element: <AddResourcePage />, | ||
| id: 'add-resource', | ||
| route: ':challengeId/manage-resource/add', | ||
| route: ':challengeId/manage-submission', |
There was a problem hiding this comment.
The route for this entry has been updated to ':challengeId/manage-submission', but the id is still 'manage-resource'. Ensure that the id accurately describes the route's purpose, possibly changing it to 'manage-submission'.
| @@ -14,6 +14,7 @@ import _ from 'lodash' | |||
| import classNames from 'classnames' | |||
| import moment from 'moment' | |||
|
|
|||
| import { EnvironmentConfig } from '~/config' | |||
There was a problem hiding this comment.
The import statement for EnvironmentConfig is added, but it is not used in the current code. Consider removing it if it's not needed to avoid unnecessary imports.
| @@ -55,8 +57,8 @@ export const BillingAccountNewPage: FC<Props> = (props: Props) => { | |||
| billingAccount, | |||
| }: useManageAddBillingAccountProps = useManageAddBillingAccount(accountId) | |||
| const pageTitle = useMemo( | |||
| () => (accountId ? 'Edit Billing Account' : 'New Billing Account'), | |||
| [accountId], | |||
| () => (isEdit ? 'Edit Billing Account' : 'New Billing Account'), | |||
There was a problem hiding this comment.
The variable isEdit is used here, but it is not clear from the diff if it is defined or how it is being set. Ensure that isEdit is properly defined and initialized in the component to avoid potential runtime errors.
| @@ -81,7 +83,9 @@ export const BillingAccountNewPage: FC<Props> = (props: Props) => { | |||
| subscriptionNumber: '' as any, | |||
| }, | |||
| mode: 'all', | |||
| resolver: yupResolver(formEditBillingAccountSchema), | |||
| resolver: yupResolver( | |||
| isEdit ? formEditBillingAccountSchema : formAddBillingAccountSchema, | |||
There was a problem hiding this comment.
The use of isEdit to determine the schema could be clarified by ensuring isEdit is well-defined and initialized before this point in the code. Verify that isEdit is correctly set based on the intended logic to avoid potential runtime errors.
| @@ -80,7 +81,7 @@ export const fetchGroups = async (params: { | |||
| const result = await xhrGetAsync<UserGroup[]>( | |||
| `${EnvironmentConfig.API.V5}/groups?${qs.stringify(params)}`, | |||
| ) | |||
| return result.map(adjustUserGroupResponse) | |||
| return _.orderBy(result.map(adjustUserGroupResponse), ['createdAt'], ['desc']) | |||
There was a problem hiding this comment.
Consider checking if result is not null or undefined before calling map on it to avoid potential runtime errors.
| @@ -94,6 +95,16 @@ export const createGroup = async (data: FormAddGroup): Promise<UserGroup> => { | |||
| `${EnvironmentConfig.API.V5}/groups`, | |||
| data, | |||
| ) | |||
|
|
|||
| if (!result.updatedAt) { | |||
There was a problem hiding this comment.
Consider checking if result.createdAt is defined before assigning it to result.updatedAt to avoid potential errors if createdAt is undefined.
| result.updatedAt = result.createdAt | ||
| } | ||
|
|
||
| if (!result.updatedBy) { |
There was a problem hiding this comment.
The hardcoded value '00000000' for updatedBy might not be appropriate for all cases. Consider using a more descriptive default value or handling this scenario differently.
|
|
||
| if (!result.updatedBy) { | ||
| result.updatedBy = '00000000' | ||
| result.updatedByHandle = '00000000 (not found)' |
There was a problem hiding this comment.
The hardcoded value '00000000 (not found)' for updatedByHandle might not be appropriate for all cases. Consider using a more descriptive default value or handling this scenario differently.
| ): Promise<ApiV5ResponseSuccess> => { | ||
| for (const reviewSummationId of reviewSummationIds) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await removeReviewSummation(reviewSummationId) |
There was a problem hiding this comment.
Using await inside a loop can lead to performance issues because it waits for each promise to resolve before proceeding to the next iteration. Consider using Promise.all to handle multiple asynchronous operations concurrently.
| export const fetchSubmissionsOfChallenge = async ( | ||
| challengeId: string, | ||
| ): Promise<MemberSubmission[]> => { | ||
| if (!challengeId) { |
There was a problem hiding this comment.
Consider adding a type guard or validation for challengeId to ensure it is a valid string before proceeding with the API call.
| */ | ||
| export const removeSubmission = async ( | ||
| submissionId: string, | ||
| ): Promise<ApiV5ResponseSuccess> => { |
There was a problem hiding this comment.
Consider adding a check to ensure submissionId is valid before making the API call to prevent potential errors.
| submissions: MemberSubmission[] | ||
| } { | ||
| let maxFinalScore | ||
| = _.get(submissions[0], 'submissions[0]', { |
There was a problem hiding this comment.
The use of _.get(submissions[0], 'submissions[0]', { finalScore: 0 }).finalScore ?? 0 is a bit confusing. It seems like you are trying to get the finalScore of the first submission, but the use of submissions[0] twice is unclear. Consider simplifying or clarifying this logic.
| if (pA === pB) { | ||
| const timeA = _.get(a, 'submissions[0].submittedDate') | ||
| const timeB = _.get(b, 'submissions[0].submittedDate') | ||
| return timeA.getTime() - timeB.getTime() |
There was a problem hiding this comment.
Ensure that timeA and timeB are valid Date objects before calling getTime() on them to avoid potential runtime errors.
| if (pA === pB) { | ||
| const timeA = _.get(a, 'submissions[0].submittedDate') | ||
| const timeB = _.get(b, 'submissions[0].submittedDate') | ||
| return timeA.getTime() - timeB.getTime() |
There was a problem hiding this comment.
Ensure that timeA and timeB are valid Date objects before calling getTime() on them to avoid potential runtime errors.
| let pB = _.get(b, 'submissions[0]', { finalScore: 0 }).finalScore | ||
| if (pA === undefined) pA = 0 | ||
| if (pB === undefined) pB = 0 | ||
| if (pA > 0) maxFinalScore = pA |
There was a problem hiding this comment.
The logic for updating maxFinalScore seems incorrect. It should only update maxFinalScore if the current score is greater than the existing maxFinalScore.
| * @param decimal number of unit to fix | ||
| * @returns number | ||
| */ | ||
| function toAcurateFixed(num: number, decimal: number): string | undefined { |
There was a problem hiding this comment.
The function name toAcurateFixed seems to have a typo. Consider renaming it to toAccurateFixed for clarity.
| return num | ||
| } | ||
|
|
||
| if (Number.isNaN(Number(num))) return num as number |
There was a problem hiding this comment.
The return statement return num as number might not be appropriate if num is a string that cannot be converted to a number. Consider returning undefined or handling this case differently to avoid unexpected behavior.
| * @returns true if object is number | ||
| */ | ||
| export function checkIsNumberObject(numberObject: any): boolean { | ||
| return typeof numberObject === 'number' |
There was a problem hiding this comment.
The function checkIsNumberObject checks if the input is of type number, but the name suggests it checks for a number object. Consider renaming the function to checkIsNumber for clarity.
| export function checkIsStringNumeric(str: string): boolean { | ||
| if (typeof str !== 'string') return false // we only process strings! | ||
| return ( | ||
| !Number.isNaN(str as any) |
There was a problem hiding this comment.
The use of str as any is unnecessary and can be avoided. Consider using Number.isNaN(Number(str)) instead for clarity.
| if (typeof str !== 'string') return false // we only process strings! | ||
| return ( | ||
| !Number.isNaN(str as any) | ||
| && !Number.isNaN(parseFloat(str)) // use type coercion to parse the |
There was a problem hiding this comment.
The comment about parseFloat should be moved to a more appropriate location or rephrased to clarify its relevance to the code. It currently interrupts the logic flow.
| @@ -134,7 +106,7 @@ export function isValidNumber(value: number | undefined): boolean { | |||
| /** | |||
| * validation schema for form new billing account | |||
| */ | |||
| export const formEditBillingAccountSchema: Yup.ObjectSchema<FormEditBillingAccount> | |||
| export const formAddBillingAccountSchema: Yup.ObjectSchema<FormEditBillingAccount> | |||
There was a problem hiding this comment.
The schema name formAddBillingAccountSchema suggests it is for adding a billing account, but it is typed as FormEditBillingAccount. Ensure that the schema name and type are consistent with their intended use.
| /** | ||
| * validation schema for form new billing account | ||
| */ | ||
| export const formEditBillingAccountSchema: Yup.ObjectSchema<FormEditBillingAccount> |
There was a problem hiding this comment.
The formEditBillingAccountSchema is defined twice in the code. Consider renaming the second instance to avoid confusion and potential errors.
| @@ -152,7 +152,7 @@ export const PermissionGroupMembersPage: FC<Props> = (props: Props) => { | |||
| {(groupMembers[memberType] || []) | |||
| .length === 0 ? ( | |||
| <p className={styles.noRecordFound}> | |||
| No members | |||
| No groups | |||
There was a problem hiding this comment.
The message 'No groups' may not accurately reflect the context if this section is intended to display members. Consider revising the message to better match the context of what is being displayed.
| @@ -32,6 +32,7 @@ import { | |||
| } from '../../lib/models' | |||
| import { | |||
| approveApplication, | |||
| getChallengeByLegacyId, | |||
There was a problem hiding this comment.
The function getChallengeByLegacyId is imported but not used in this file. Consider removing it if it's not needed to avoid unnecessary imports.
| @@ -139,6 +141,15 @@ export const ManageReviewerPage: FC = () => { | |||
| search() | |||
| }, [challengeId]) // eslint-disable-line react-hooks/exhaustive-deps -- missing dependency: search | |||
|
|
|||
| // Gets the challenge details by legacyId | |||
| useEffect(() => { | |||
| getChallengeByLegacyId(+challengeId) | |||
There was a problem hiding this comment.
Consider adding error handling for the promise returned by getChallengeByLegacyId. This will help manage any potential errors that might occur during the API call.
| useEffect(() => { | ||
| getChallengeByLegacyId(+challengeId) | ||
| .then(challenge => { | ||
| setChallengeUuid(challenge.id) |
There was a problem hiding this comment.
Ensure that challenge.id is always defined before calling setChallengeUuid. You might want to add a check to handle cases where challenge might be undefined or challenge.id is not present.
| primary | ||
| onClick={handleRejectPendingConfirmDialog} | ||
| size='lg' | ||
| to={`${rootRoute}/challenge-management/${challengeUuid}/manage-user`} |
There was a problem hiding this comment.
The to prop in the LinkButton component should be checked to ensure that the URL is correctly constructed and does not lead to any broken links. Consider validating the challengeUuid variable to ensure it is defined and correctly formatted.
| CONNECT_URL: 'https://connect.topcoder-dev.com', | ||
| DEFAULT_PAYMENT_TERMS: 1, |
There was a problem hiding this comment.
Consider clarifying the purpose of DEFAULT_PAYMENT_TERMS. If it represents a duration, specifying the unit (e.g., days, weeks) could improve readability and prevent potential misinterpretation.
| @@ -45,8 +45,11 @@ export interface GlobalConfig { | |||
| }, | |||
| ADMIN: { | |||
| CONNECT_URL: string | |||
| DEFAULT_PAYMENT_TERMS: number | |||
There was a problem hiding this comment.
Consider providing a more descriptive name for DEFAULT_PAYMENT_TERMS to clarify what the number represents, such as DEFAULT_PAYMENT_TERMS_DAYS if it refers to days.
| DIRECT_URL: string | ||
| WORK_MANAGER_URL: string | ||
| ONLINE_REVIEW_URL: string | ||
| CHALLENGE_URL: string | ||
| AV_SCAN_SCORER_REVIEW_TYPE_ID: string |
There was a problem hiding this comment.
Ensure that CHALLENGE_URL is consistent with other URL naming conventions in the codebase, such as using CHALLENGE_MANAGER_URL if it aligns better with existing patterns.
| DIRECT_URL: string | ||
| WORK_MANAGER_URL: string | ||
| ONLINE_REVIEW_URL: string | ||
| CHALLENGE_URL: string | ||
| AV_SCAN_SCORER_REVIEW_TYPE_ID: string | ||
| } |
There was a problem hiding this comment.
The AV_SCAN_SCORER_REVIEW_TYPE_ID could benefit from a more descriptive name to indicate what the ID represents or is used for, especially if it is a specific type of review ID.
| CONNECT_URL: 'https://connect.topcoder.com', | ||
| DEFAULT_PAYMENT_TERMS: 1, |
There was a problem hiding this comment.
The DEFAULT_PAYMENT_TERMS value is set to 1, which might not be self-explanatory. Ensure that this value is correct and consider if it should be configurable or documented elsewhere.
| @@ -30,6 +34,9 @@ interface TableProps<T> { | |||
| readonly onRowClick?: (data: T) => void | |||
| readonly onToggleSort?: (sort: Sort) => void | |||
| readonly removeDefaultSort?: boolean | |||
| readonly colWidth?: colWidthType | undefined, | |||
There was a problem hiding this comment.
Consider renaming colWidth to columnWidth for clarity and consistency with naming conventions.
| @@ -30,6 +34,9 @@ interface TableProps<T> { | |||
| readonly onRowClick?: (data: T) => void | |||
| readonly onToggleSort?: (sort: Sort) => void | |||
| readonly removeDefaultSort?: boolean | |||
| readonly colWidth?: colWidthType | undefined, | |||
| readonly setColWidth?: Dispatch<SetStateAction<colWidthType>> | undefined | |||
There was a problem hiding this comment.
The setColWidth prop could be renamed to setColumnWidth to match the suggested change for colWidth and improve readability.
| @@ -39,6 +46,11 @@ interface DefaultSortDirectionMap { | |||
|
|
|||
| const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) => JSX.Element | |||
| = <T extends { [propertyName: string]: any }>(props: TableProps<T>) => { | |||
| const { width: screenWidth }: WindowSize = useWindowSize() | |||
There was a problem hiding this comment.
Consider destructuring useWindowSize() directly in the function parameters if screenWidth is the only property used, to improve readability and reduce unnecessary variable declarations.
| const { width: screenWidth }: WindowSize = useWindowSize() | ||
| const tableRef = useRef<HTMLTableElement>(null) | ||
| const screenWidthBkRef = useRef<number>(0) | ||
| const colWidthInput = props.colWidth |
There was a problem hiding this comment.
Ensure colWidthInput is used appropriately in the component. If it's not used, consider removing it to avoid unused variable declarations.
| const tableRef = useRef<HTMLTableElement>(null) | ||
| const screenWidthBkRef = useRef<number>(0) | ||
| const colWidthInput = props.colWidth | ||
| const setColWidth = props.setColWidth |
There was a problem hiding this comment.
Ensure setColWidth is used appropriately in the component. If it's not used, consider removing it to avoid unused variable declarations.
| @@ -62,6 +74,52 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| const [sortedData, setSortedData]: [ReadonlyArray<T>, Dispatch<SetStateAction<ReadonlyArray<T>>>] | |||
| = useState<ReadonlyArray<T>>(props.data) | |||
|
|
|||
| useEffect(() => { | |||
| if (_.isEmpty(colWidthInput) && colWidthInput) { | |||
There was a problem hiding this comment.
The condition _.isEmpty(colWidthInput) && colWidthInput seems contradictory. If colWidthInput is empty, the second part of the condition will always be false. Consider revising the logic to ensure it behaves as intended.
| @@ -62,6 +74,52 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| const [sortedData, setSortedData]: [ReadonlyArray<T>, Dispatch<SetStateAction<ReadonlyArray<T>>>] | |||
| = useState<ReadonlyArray<T>>(props.data) | |||
|
|
|||
| useEffect(() => { | |||
| if (_.isEmpty(colWidthInput) && colWidthInput) { | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
Using setTimeout with a fixed delay of 10 milliseconds may lead to unpredictable behavior depending on the execution environment. Consider using requestAnimationFrame or another method to ensure the timing is appropriate.
| } | ||
| }, 10) | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
The use of // eslint-disable-next-line react-hooks/exhaustive-deps indicates that the dependencies of the useEffect hook might be incomplete. Consider reviewing the dependencies to ensure all necessary variables are included.
| return // do not reset col width on init | ||
| } | ||
|
|
||
| if (!_.isEmpty(colWidthInput) && screenWidthBkRef.current !== screenWidth) { |
There was a problem hiding this comment.
The condition !_.isEmpty(colWidthInput) is used here, but it seems inconsistent with the previous useEffect where the opposite condition is checked. Ensure the logic aligns with the intended behavior.
| } | ||
|
|
||
| screenWidthBkRef.current = screenWidth | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
The use of // eslint-disable-next-line react-hooks/exhaustive-deps suggests potential issues with the dependencies of the useEffect hook. Review the dependencies to ensure all necessary variables are accounted for.
| return ( | ||
| <th | ||
| className={styles.th} | ||
| className={classNames(styles.th, columnId)} |
There was a problem hiding this comment.
The columnId variable is being used directly as a class name. Ensure that columnId does not contain any special characters that could interfere with CSS class naming conventions.
| key={getKey(index)} | ||
| > | ||
| <div | ||
| className={ | ||
| classNames(styles['header-container'], styles[col.type], colorClass, sortableClass) | ||
| } | ||
| style={colWidth ? { width: `${colWidth}px` } : {}} |
There was a problem hiding this comment.
The inline style for width is being applied conditionally. Consider verifying that colWidth is a valid number before using it to set the width, to prevent potential rendering issues.
| @@ -182,14 +243,15 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| columns={props.columns} | |||
| index={index} | |||
| showExpand={props.showExpand} | |||
| colWidth={colWidthInput} | |||
There was a problem hiding this comment.
Consider verifying if colWidthInput is correctly defined and passed to the Row component, as it seems to be a new addition. Ensure it aligns with the expected data type and usage within the component.
| preventDefault={props.preventDefault} | ||
| /> | ||
| )) | ||
|
|
||
| return ( | ||
| /* TODO: sticky header */ | ||
| <div className={styles['table-wrap']}> | ||
| <table className={styles.table}> | ||
| <div className={classNames(styles['table-wrap'], props.className)}> |
There was a problem hiding this comment.
The addition of props.className using classNames is a good approach for dynamic styling. However, ensure that props.className is validated and does not conflict with existing styles in styles['table-wrap'].
| <div className={styles['table-wrap']}> | ||
| <table className={styles.table}> | ||
| <div className={classNames(styles['table-wrap'], props.className)}> | ||
| <table ref={tableRef} className={styles.table}> |
There was a problem hiding this comment.
The tableRef is introduced here. Ensure that it is properly initialized and used within the component logic, especially if it is intended for DOM manipulation or other operations.
| @@ -93,6 +94,7 @@ const TableCell: <T extends { [propertyName: string]: any }>( | |||
| 'TableCell_blockCell', | |||
| )} | |||
| onClick={onClick} | |||
| style={props.style} | |||
There was a problem hiding this comment.
Consider verifying if props.style is always defined or if there should be a default value to prevent potential rendering issues when props.style is undefined.
| /> | ||
| )) | ||
| const cells: Array<JSX.Element> = displayColumns.map((col, colIndex) => { | ||
| const columnId = `column-id-${col.columnId}-` |
There was a problem hiding this comment.
Consider checking if col.columnId is defined before using it to construct columnId. This will prevent potential errors if col.columnId is undefined or null.
| )) | ||
| const cells: Array<JSX.Element> = displayColumns.map((col, colIndex) => { | ||
| const columnId = `column-id-${col.columnId}-` | ||
| const colWidth = props.colWidth?.[columnId] |
There was a problem hiding this comment.
Ensure that props.colWidth is properly defined and contains valid values for columnId. If props.colWidth is undefined or does not contain the expected keys, this could lead to unexpected behavior.
| col.className, | ||
| 'TableCell_blockExpandValue', | ||
| )} | ||
| style={colWidth ? { width: `${colWidth}px` } : {}} |
There was a problem hiding this comment.
Consider verifying if colWidth is a valid number before using it in the style attribute to prevent potential rendering issues. You might want to ensure that colWidth is not NaN or negative.
| @@ -34,7 +36,7 @@ const TableSort: FC<TableSortProps> = (props: TableSortProps) => { | |||
|
|
|||
| return ( | |||
| <Button | |||
| className={classNames(props.iconClass, 'TableSort')} | |||
| className={classNames(props.iconClass, 'TableSort', styles.btnSort)} | |||
There was a problem hiding this comment.
Ensure that styles.btnSort is defined and imported correctly. If it's not defined, it could lead to runtime errors.
| } | ||
|
|
||
| export function useWindowSize(): WindowSize { | ||
| const [windowSize, setWindowSize]: [WindowSize, Dispatch<SetStateAction<WindowSize>>] = useState({ |
There was a problem hiding this comment.
Consider initializing windowSize with the actual current window dimensions instead of zeros to avoid an unnecessary initial resize event.
|
|
||
| // Remove event listener on cleanup | ||
| return () => window.removeEventListener('resize', handleResize) | ||
| }, [handleResize]) // Empty array ensures that effect is only run on mount |
There was a problem hiding this comment.
The comment 'Empty array ensures that effect is only run on mount' is misleading because the dependency array is not empty; it contains handleResize. Consider rephrasing or removing the comment.
| @@ -17439,7 +17439,7 @@ stringify-object@^3.3.0: | |||
| is-obj "^1.0.1" | |||
| is-regexp "^1.0.0" | |||
|
|
|||
| "strip-ansi-cjs@npm:strip-ansi@^6.0.1": | |||
| "strip-ansi-cjs@npm:strip-ansi@^6.0.1", strip-ansi@^6.0.0, strip-ansi@^6.0.1: | |||
There was a problem hiding this comment.
The addition of multiple aliases for strip-ansi seems unnecessary unless there is a specific reason for supporting multiple versions. Consider consolidating to a single version if possible to avoid potential conflicts or confusion.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-440
https://topcoder.atlassian.net/issues/PM-1076
What's in this PR?
Initial system admin app implementation