Topcoder Admin App - Marathon Match Functionality #1092
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -145,6 +149,11 @@ export const adminRoutes: ReadonlyArray<PlatformRoute> = [ | |||
| id: 'add-resource', | |||
| route: ':challengeId/manage-resource/add', | |||
| }, | |||
| { | |||
| element: <ManageSubmissionPage />, | |||
There was a problem hiding this comment.
The component <ManageSubmissionPage /> is added here, but ensure that it is imported correctly at the top of the file. If it's not imported, it will cause a runtime error.
There was a problem hiding this comment.
@suppermancool - Can you double check t his please?
There was a problem hiding this comment.
@jmgasper this code is correct
| } | ||
|
|
||
| export const ManageSubmissionPage: FC<Props> = (props: Props) => { | ||
| const { challengeId = '' }: { challengeId?: string } = useParams<{ |
There was a problem hiding this comment.
The destructuring of useParams could be simplified by directly using useParams<{ challengeId: string }>() without the need for an intermediate type annotation.
There was a problem hiding this comment.
@suppermancool - Can you double check this please?
There was a problem hiding this comment.
@jmgasper, the intermediate type annotation is required by eslint without it the 'yarn lint' will fail
| pageTitle='Submission Management' | ||
| className={classNames(styles.container, props.className)} | ||
| headerActions={( | ||
| <LinkButton primary light to='./../..' size='lg'> |
There was a problem hiding this comment.
Consider using a more descriptive path for the LinkButton to improve readability and maintainability. The current path './../..' is not immediately clear and could be replaced with a named route or a clearer path string.
| ) : ( | ||
| <div className={styles.blockTableContainer}> | ||
| <SubmissionTable | ||
| datas={submissions} |
There was a problem hiding this comment.
The prop datas should be renamed to data to follow standard naming conventions and improve readability.
There was a problem hiding this comment.
@suppermancool - Can you fix please?
There was a problem hiding this comment.
@jmgasper done in the latest commit
| testType: string, | ||
| ): RequestBusAPI => ({ | ||
| 'mime-type': 'application/json', | ||
| originator: 'MMFinalScoreProcessor', |
There was a problem hiding this comment.
Consider using a constant or configuration setting for the originator value 'MMFinalScoreProcessor' to avoid hardcoding it directly in the function. This can improve maintainability and make it easier to update if the originator changes in the future.
| }, | ||
| timestamp: new Date() | ||
| .toISOString(), | ||
| topic: 'submission.notification.score', |
There was a problem hiding this comment.
Consider using a constant or configuration setting for the topic value 'submission.notification.score' to avoid hardcoding it directly in the function. This can improve maintainability and make it easier to update if the topic changes in the future.
| @@ -134,6 +135,7 @@ const Actions: FC<{ | |||
| challenge: Challenge | |||
| currentFilters: ChallengeFilterCriteria | |||
| }> = props => { | |||
| const isMM = useMemo(() => checkIsMM(props.challenge), [props.challenge]) | |||
There was a problem hiding this comment.
Consider renaming the isMM variable to something more descriptive, such as isMarathonMatch, to improve code readability.
| import styles from './SubmissionTable.module.scss' | ||
|
|
||
| interface Props { | ||
| className?: string |
There was a problem hiding this comment.
Consider renaming datas to submissions for better clarity and to follow common naming conventions.
| ...column, | ||
| className: '', | ||
| label: `${column.label as string} label`, | ||
| mobileType: 'label', |
There was a problem hiding this comment.
The label ${column.label as string} label seems redundant. Consider simplifying it to just ${column.label as string}.
| columns={columns} | ||
| data={props.datas} | ||
| removeDefaultSort | ||
| onToggleSort={_.noop} |
There was a problem hiding this comment.
The removeDefaultSort prop is used here but not defined in the Table component props. Ensure that this prop is handled correctly in the Table component.
| open: boolean | ||
| setOpen: Dispatch<SetStateAction<boolean>> | ||
| }) => { | ||
| const createToggle = () => (): void => triggerProps.setOpen(!triggerProps.open) |
There was a problem hiding this comment.
Consider renaming createToggle to something more descriptive like toggleDropdown to better convey its purpose.
| }) => { | ||
| const createToggle = () => (): void => triggerProps.setOpen(!triggerProps.open) | ||
| return ( | ||
| <Button primary onClick={createToggle()}> |
There was a problem hiding this comment.
The Button component could benefit from an aria-label for better accessibility, especially since it triggers a dropdown menu.
| shouldIgnoreWhenClickMenu | ||
| > | ||
| <ul> | ||
| {props.data.isTheLatestSubmission && ( |
There was a problem hiding this comment.
Consider extracting the list items into separate components or functions for better readability and maintainability, especially since they have similar structures.
| <li | ||
| className={classNames({ | ||
| disabled: | ||
| props.isRemovingReviewSummations[props.data.id] |
There was a problem hiding this comment.
The condition !(props.data.reviewSummation ?? []).length could be simplified to !props.data.reviewSummation?.length for better readability.
There was a problem hiding this comment.
@suppermancool - Can you fix please?
There was a problem hiding this comment.
@jmgasper done in the latest commit
| import { toast } from 'react-toastify' | ||
| import _ from 'lodash' | ||
|
|
||
| import { CREATE_BUS_EVENT_DATA_SUBMISSION_MARATHON_MATCH } from '../../config/busEvent.config' |
There was a problem hiding this comment.
Consider renaming CREATE_BUS_EVENT_DATA_SUBMISSION_MARATHON_MATCH to a more concise name if possible, as it is quite long and may affect readability.
| */ | ||
| export function useManageBusEvent(): useManageBusEventProps { | ||
| const [isRunningTest, setIsRunningTest] = useState<IsRemovingType>({}) | ||
| const isRunningTestBool = useMemo( |
There was a problem hiding this comment.
The use of lodash's some function is fine, but if performance is a concern, consider using native JavaScript methods like Object.values(isRunningTest).some(...).
| })) | ||
| toast.success(`Run ${testType} test successfully`, { | ||
| toastId: 'Run test', | ||
| }) |
There was a problem hiding this comment.
The toastId value 'Run test' is hardcoded. If there are multiple types of tests, consider making this dynamic to avoid potential conflicts with other toasts.
| handleError(e) | ||
| }) | ||
| }, | ||
| [], |
There was a problem hiding this comment.
The dependency array for useCallback is empty. Ensure that this is intentional and that doPostBusEvent does not rely on any external variables that might change.
| isLoadingRef.current = false | ||
| setIsLoading(isLoadingRef.current) | ||
| handleError(e) | ||
| fail() |
There was a problem hiding this comment.
The function fail() is called here, but it is not defined or imported in this file. Ensure that fail() is defined or imported correctly, or remove this call if it is not needed.
| }) | ||
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [], |
There was a problem hiding this comment.
The dependency array for useCallback is empty, but the function doRemoveSubmission uses setIsRemovingSubmission, setMemberSubmissions, and handleError. Consider adding these dependencies to the array to ensure the callback updates correctly when these values change.
| }) | ||
| }, | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| [], |
There was a problem hiding this comment.
The dependency array for useCallback is empty, but the function doRemoveReviewSummations uses setIsRemovingReviewSummations, setMemberSubmissions, and handleError. Consider adding these dependencies to the array to ensure the callback updates correctly when these values change.
| @@ -0,0 +1,15 @@ | |||
| /** | |||
| * Request to bust api | |||
There was a problem hiding this comment.
The comment on line 2 seems to have a typo. It should likely be 'Request to bus API' instead of 'Request to bust api'.
There was a problem hiding this comment.
@suppermancool - Can you fix please?
There was a problem hiding this comment.
@jmgasper done in the latest commit
| export interface RequestBusAPI { | ||
| topic: string | ||
| originator: string | ||
| timestamp: string |
There was a problem hiding this comment.
Consider using a more specific type for the 'timestamp' field, such as 'Date' or a more precise string format, to ensure consistency and prevent errors.
| topic: string | ||
| originator: string | ||
| timestamp: string | ||
| 'mime-type': string |
There was a problem hiding this comment.
The 'mime-type' field name uses a hyphen, which is unconventional in TypeScript interfaces. Consider using camelCase ('mimeType') for consistency with TypeScript naming conventions.
| results = [...results, ...finalSubmission.submissions] | ||
| }) | ||
|
|
||
| return finalSubmissions |
There was a problem hiding this comment.
The function recalculateSubmissionRank returns finalSubmissions, which is of type MemberSubmission[], but the function signature suggests it should return Submission[]. Consider updating the return type in the function signature to match the actual return type.
| _.each(data, (value, key) => { | ||
| result.push({ | ||
| finalRank: undefined, | ||
| memberId: key as any, |
There was a problem hiding this comment.
The memberId is being cast to any. Consider using a more specific type instead of any to ensure type safety.
| * @returns updated data | ||
| */ | ||
| export function adjustSubmissionResponse(data: Submission): Submission { | ||
| const validReviews = _.reject(data.review, [ |
There was a problem hiding this comment.
The _.reject function is used to filter out reviews with a specific typeId. Ensure that EnvironmentConfig.ADMIN.AV_SCAN_SCORER_REVIEW_TYPE_ID is correctly defined and available in the environment configuration to avoid runtime errors.
| data: SubmissionReview, | ||
| ): SubmissionReview { | ||
| const created = data.created ? new Date(data.created) : data.created | ||
| const reviewedDate = data.created |
There was a problem hiding this comment.
The condition data.created is checked for created, reviewedDate, and updated. If data.created is falsy, the original value is returned, which may not be the intended behavior for reviewedDate and updated. Consider checking the respective fields instead.
| */ | ||
| export const reqToBusAPI = async (data: RequestBusAPI): Promise<string> => { | ||
| const resultData = await xhrPostAsync<RequestBusAPI, string>( | ||
| `${EnvironmentConfig.API.V5}/bus/events`, |
There was a problem hiding this comment.
Consider adding error handling for the xhrPostAsync call to manage potential failures or exceptions that might occur during the API request.
| ): 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 as it waits for each promise to resolve before continuing to the next iteration. Consider using Promise.all to execute all promises concurrently if the order of execution is not important.
| export const fetchSubmissionsOfChallenge = async ( | ||
| challengeId: string, | ||
| ): Promise<MemberSubmission[]> => { | ||
| if (!challengeId) { |
There was a problem hiding this comment.
Consider throwing an error or logging a warning if challengeId is not provided, as returning an empty array might hide potential issues with the function usage.
| export const removeSubmission = async ( | ||
| submissionId: string, | ||
| ): Promise<ApiV5ResponseSuccess> => { | ||
| const result = await xhrDeleteAsync<ApiV5ResponseSuccess>( |
There was a problem hiding this comment.
Consider adding error handling for the API call to xhrDeleteAsync to manage potential failures or network issues gracefully.
| maxFinalScore: number | ||
| submissions: MemberSubmission[] | ||
| } { | ||
| let maxFinalScore |
There was a problem hiding this comment.
The initialization of maxFinalScore is a bit convoluted. Consider simplifying it by directly assigning a default value if the submissions array is empty or if the finalScore is undefined.
| 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 not undefined before calling getTime() 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 not undefined before calling getTime() 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 is repeated. Consider extracting this logic into a separate function to avoid duplication and improve readability.
| */ | ||
| function removeDecimal(num: number): string | undefined { | ||
| // eslint-disable-next-line prefer-regex-literals | ||
| const re = new RegExp('^-?\\d+') |
There was a problem hiding this comment.
Consider using a literal regex /^-?\d+/ instead of new RegExp('^-?\\d+') for better readability and performance.
| * @returns number | ||
| */ | ||
| function toAcurateFixed(num: number, decimal: number): string | undefined { | ||
| const re = new RegExp(`^-?\\d+(?:.\\d{0,${decimal}})?`) |
There was a problem hiding this comment.
Consider using a literal regex /^-?\d+(?:\.\d{0,${decimal}})?/ instead of new RegExp(^-?\d+(?:.\d{0,${decimal}})?) for better readability and performance. Note the escaped dot \. to match a literal period.
| * @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 (_.isNaN(parseFloat(num as string))) return num as number |
There was a problem hiding this comment.
The use of _.isNaN(parseFloat(num as string)) can be replaced with isNaN(Number(num)) for simplicity and to avoid unnecessary lodash dependency.
There was a problem hiding this comment.
@suppermancool - Can you fix please?
There was a problem hiding this comment.
@jmgasper done in the latest commit
| if (_.isNaN(parseFloat(num as string))) return num as number | ||
| const numFloat = parseFloat(num as string) | ||
|
|
||
| const result = _.toFinite(toAcurateFixed(numFloat, decimal)) |
There was a problem hiding this comment.
The use of _.toFinite may not be necessary here. Consider directly using parseFloat or Number to convert the string to a number, as _.toFinite will convert NaN to 0, which might not be the intended behavior.
| const numFloat = parseFloat(num as string) | ||
|
|
||
| const result = _.toFinite(toAcurateFixed(numFloat, decimal)) | ||
| const integerResult = _.toFinite(removeDecimal(numFloat)) |
There was a problem hiding this comment.
The use of _.toFinite may not be necessary here. Consider directly using parseFloat or Number to convert the string to a number, as _.toFinite will convert NaN to 0, which might not be the intended behavior.
| * @returns true if the object is a string, false if otherwise | ||
| */ | ||
| export function isStringObject(str: any): boolean { | ||
| if (typeof str === 'string' || str instanceof String) { |
There was a problem hiding this comment.
Consider using strict equality checks (===) for type comparison to avoid potential issues with type coercion.
| */ | ||
| export function isStringObject(str: any): boolean { | ||
| if (typeof str === 'string' || str instanceof String) { | ||
| return true |
There was a problem hiding this comment.
Add a semicolon at the end of the statement to maintain consistency with the rest of the codebase.
There was a problem hiding this comment.
@suppermancool - Can you fix please?
There was a problem hiding this comment.
@jmgasper the ESLint requires removing the semicolon at the end of the statement
| return true | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
Add a semicolon at the end of the statement to maintain consistency with the rest of the codebase.
There was a problem hiding this comment.
@suppermancool - Can you fix please?
There was a problem hiding this comment.
@jmgasper the ESLint requires removing the semicolon at the end of the statement
| ) : ( | ||
| <div className={styles.blockTableContainer}> | ||
| <SubmissionTable | ||
| data={submissions} |
There was a problem hiding this comment.
The prop name datas has been changed to data. Ensure that this change is consistent throughout the codebase where this component is used to prevent any potential issues.
|
|
||
| interface Props { | ||
| className?: string | ||
| data: Submission[] |
There was a problem hiding this comment.
The variable name datas has been corrected to data, which is more appropriate and follows standard naming conventions. Ensure that all references to this variable in the code are updated accordingly to prevent any runtime errors.
| return ( | ||
| <TableWrapper className={classNames(styles.container, props.className)}> | ||
| {isTablet ? ( | ||
| <TableMobile columns={columnsMobile} data={props.data} /> |
There was a problem hiding this comment.
The prop data is being used instead of datas. Ensure that props.data is correctly defined and used throughout the component to avoid potential runtime errors.
| ) : ( | ||
| <Table | ||
| columns={columns} | ||
| data={props.data} |
There was a problem hiding this comment.
The prop data is being used instead of datas. Ensure that props.data is correctly defined and used throughout the component to avoid potential runtime errors.
Challenge https://www.topcoder.com/challenges/8ec2741c-7ee4-4083-9806-1a02fe361ea6