Topcoder Admin App - Add Non-MM Submission Management #1191
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| isLoading: isLoadingChallenge, | ||
| challengeInfo, | ||
| }: useFetchChallengeProps = useFetchChallenge(challengeId) | ||
| const isMM = useMemo(() => checkIsMM(challengeInfo), [challengeInfo]) |
There was a problem hiding this comment.
useMemo is used here to memoize the result of checkIsMM(challengeInfo). Ensure that checkIsMM is a pure function, as impure functions can lead to unexpected behavior when used with useMemo.
| @@ -48,6 +61,19 @@ export const ManageSubmissionPage: FC<Props> = (props: Props) => { | |||
| }: useManageChallengeSubmissionsProps | |||
| = useManageChallengeSubmissions(challengeId) | |||
|
|
|||
| const { | |||
| isLoading: isDownloadingSubmission, | |||
| isLoadingBool: isDownloadingSubmissionBool, | |||
There was a problem hiding this comment.
The variable isLoadingBool seems redundant if isLoading already represents the loading state. Consider removing isLoadingBool unless it serves a distinct purpose.
| }: useDownloadSubmissionProps = useDownloadSubmission() | ||
| const { | ||
| isLoading: isDoingAvScan, | ||
| isLoadingBool: isDoingAvScanBool, |
There was a problem hiding this comment.
Similar to the previous comment, isLoadingBool appears redundant if isLoading is sufficient to represent the loading state. Evaluate if both are necessary.
| doPostBusEvent: doPostBusEventAvScan, | ||
| }: useManageAVScanProps = useManageAVScan() | ||
|
|
||
| const isLoading = isLoadingSubmission || isLoadingChallenge |
There was a problem hiding this comment.
The isLoading variable combines isLoadingSubmission and isLoadingChallenge. Ensure this logic correctly represents the intended loading state for the page, especially if additional loading states are introduced.
| @@ -80,9 +110,12 @@ export const ManageSubmissionPage: FC<Props> = (props: Props) => { | |||
| doPostBusEvent={doPostBusEvent} | |||
| showSubmissionHistory={showSubmissionHistory} | |||
| setShowSubmissionHistory={setShowSubmissionHistory} | |||
| isMM={isMM} | |||
There was a problem hiding this comment.
The isMM prop is added to the component, but it's not clear from the diff if this prop is being used within the component. Ensure that isMM is utilized appropriately within the component to avoid unnecessary prop passing.
| export const CREATE_BUS_EVENT_AV_RESCAN = ( | ||
| payload: RequestBusAPIAVScanPayload, | ||
| ): RequestBusAPIAVScan => ({ | ||
| 'mime-type': 'application/json', |
There was a problem hiding this comment.
Consider using consistent casing for keys. The key 'mime-type' uses kebab-case, while other keys use camelCase. It might be beneficial to standardize the casing for consistency.
|
|
||
| export const ShowHistoryButton: FC<Props> = (props: Props) => ( | ||
| <Button | ||
| onClick={function onClick() { |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with modern React practices and improve readability.
| @@ -13,6 +13,10 @@ | |||
| .rowActions { | |||
| display: flex; | |||
| align-items: center; | |||
|
|
|||
| @include ltelg { | |||
There was a problem hiding this comment.
The mixin ltelg is used here, but it is not clear from the diff whether it is defined or imported correctly in the file. Ensure that the mixin is properly defined or imported to avoid potential errors.
| return screenWidth <= 1479 | ||
| } | ||
|
|
||
| return screenWidth <= 900 |
There was a problem hiding this comment.
Consider extracting the screen width threshold values (1479 and 900) into constants with descriptive names to improve readability and maintainability.
| ...prev, | ||
| [data.id]: !prev[data.id], | ||
| })) | ||
| () => (props.isMM |
There was a problem hiding this comment.
Consider using a more descriptive variable name than isMM to improve code readability. It might not be immediately clear to other developers what isMM stands for.
| label: '', | ||
| renderer: (data: Submission) => ( | ||
| <div className={styles.rowActions}> | ||
| <SubmissionTableActionsNonMM |
There was a problem hiding this comment.
The function doPostBusEventAvScan is defined inline. Consider defining this function outside of the JSX to avoid unnecessary re-creations on each render, which can improve performance.
| }, | ||
| ], | ||
| isDownloading={props.isDownloading} | ||
| downloadSubmission={function downloadSubmission() { |
There was a problem hiding this comment.
The function downloadSubmission is defined inline. Consider defining this function outside of the JSX to avoid unnecessary re-creations on each render, which can improve performance.
| <div className={styles.container}> | ||
| <Button | ||
| onClick={function onClick() { | ||
| props.downloadSubmission() |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with modern React practices. Example: onClick={() => props.downloadSubmission()}
| {props.data.isTheLatestSubmission && ( | ||
| <Button | ||
| onClick={function onClick() { | ||
| props.doPostBusEventAvScan() |
There was a problem hiding this comment.
Consider using an arrow function for the onClick handler to maintain consistency with modern React practices. Example: onClick={() => props.doPostBusEventAvScan()}
| props.downloadSubmission() | ||
| }} | ||
| primary | ||
| disabled={props.isDownloading[props.data.id]} |
There was a problem hiding this comment.
Ensure that props.isDownloading is properly typed and initialized to prevent potential runtime errors when accessing props.isDownloading[props.data.id].
| props.doPostBusEventAvScan() | ||
| }} | ||
| primary | ||
| disabled={props.isDoingAvScan[props.data.id]} |
There was a problem hiding this comment.
Ensure that props.isDoingAvScan is properly typed and initialized to prevent potential runtime errors when accessing props.isDoingAvScan[props.data.id].
| const [challengeInfo, setChallengeInfo] = useState<Challenge>() | ||
|
|
||
| useEffect(() => { | ||
| if (challengeId && !isLoadingRef.current) { |
There was a problem hiding this comment.
Consider adding a check to ensure challengeId is a valid string before proceeding with the fetch operation. This can prevent unnecessary API calls if challengeId is invalid or empty.
| setIsLoading(isLoadingRef.current) | ||
| setChallengeInfo(data) | ||
| }) | ||
| .catch(e => { |
There was a problem hiding this comment.
Consider providing more context or information in the error handling logic to improve debugging and error tracking. For example, logging the challengeId along with the error message could be helpful.
| /** | ||
| * Manage bus event | ||
| */ | ||
| export function useManageAVScan(): useManageAVScanProps { |
There was a problem hiding this comment.
The function useManageAVScan is documented as 'Manage bus event', which is not very descriptive. Consider providing a more specific description that reflects the function's purpose and behavior.
|
|
||
| createAvScanSubmissionPayload(submission) | ||
| .then(payload => { | ||
| const data = CREATE_BUS_EVENT_AV_RESCAN(payload) |
There was a problem hiding this comment.
The variable data is assigned the result of CREATE_BUS_EVENT_AV_RESCAN(payload). Ensure that CREATE_BUS_EVENT_AV_RESCAN is correctly handling the payload and returning the expected format for reqToBusAPI.
| [submission.id]: false, | ||
| })) | ||
| toast.success( | ||
| 'Sending request to av rescan successfully', |
There was a problem hiding this comment.
The success message 'Sending request to av rescan successfully' could be rephrased for clarity. Consider 'AV rescan request sent successfully.'
| * Request to av scan bus api | ||
| */ | ||
| export interface RequestBusAPIAVScanPayload { | ||
| submissionId: string |
There was a problem hiding this comment.
Consider adding validation or constraints for the submissionId to ensure it meets expected formats or requirements.
| */ | ||
| export interface RequestBusAPIAVScanPayload { | ||
| submissionId: string | ||
| url: string |
There was a problem hiding this comment.
Ensure the url is validated to be a properly formatted URL to prevent potential issues with malformed URLs.
| submissionId: string | ||
| url: string | ||
| fileName?: string | ||
| moveFile: boolean |
There was a problem hiding this comment.
The moveFile boolean should have a clear definition of its behavior. Consider documenting its intended effect or ensuring its usage is consistent across the application.
| url: string | ||
| fileName?: string | ||
| moveFile: boolean | ||
| cleanDestinationBucket: string |
There was a problem hiding this comment.
Consider validating cleanDestinationBucket to ensure it is a valid bucket name and exists in the storage system.
| fileName?: string | ||
| moveFile: boolean | ||
| cleanDestinationBucket: string | ||
| quarantineDestinationBucket: string |
There was a problem hiding this comment.
Similarly, validate quarantineDestinationBucket to ensure it is a valid and existing bucket name.
| moveFile: boolean | ||
| cleanDestinationBucket: string | ||
| quarantineDestinationBucket: string | ||
| callbackOption: string |
There was a problem hiding this comment.
The callbackOption should have defined acceptable values. Consider using an enum or similar construct to enforce valid options.
| cleanDestinationBucket: string | ||
| quarantineDestinationBucket: string | ||
| callbackOption: string | ||
| callbackKafkaTopic: string |
There was a problem hiding this comment.
Ensure callbackKafkaTopic is validated to be a valid Kafka topic name and exists within the Kafka setup.
| export interface RequestBusAPIAVScan { | ||
| topic: string | ||
| originator: string | ||
| timestamp: string |
There was a problem hiding this comment.
The timestamp should be in a consistent format, such as ISO 8601. Consider adding validation to ensure this.
| topic: string | ||
| originator: string | ||
| timestamp: string | ||
| 'mime-type': string |
There was a problem hiding this comment.
The mime-type field should be validated to ensure it contains a valid MIME type string.
| @@ -4,15 +4,15 @@ | |||
| import { EnvironmentConfig } from '~/config' | |||
| import { xhrPostAsync } from '~/libs/core' | |||
|
|
|||
| import { RequestBusAPI } from '../models' | |||
| import { CommonRequestBusAPI } from '../models' | |||
There was a problem hiding this comment.
The import statement has been changed from RequestBusAPI to CommonRequestBusAPI. Ensure that CommonRequestBusAPI is the correct type and that it is properly defined in the ../models module.
|
|
||
| /** | ||
| * Send post event to bus api | ||
| * @param data bus event data | ||
| * @returns resolve to empty string if success | ||
| */ | ||
| export const reqToBusAPI = async (data: RequestBusAPI): Promise<string> => { | ||
| const resultData = await xhrPostAsync<RequestBusAPI, string>( | ||
| export const reqToBusAPI = async (data: CommonRequestBusAPI): Promise<string> => { |
There was a problem hiding this comment.
The function parameter type has been updated from RequestBusAPI to CommonRequestBusAPI. Verify that this change aligns with the intended functionality and that CommonRequestBusAPI includes all necessary properties for the reqToBusAPI function.
| const url = submissionInfo.url | ||
| const { isValid, key: fileName }: ValidateS3URIResult = validateS3URI(url) | ||
| if (!isValid) { | ||
| throw new Error('Submission url is not a valid') |
There was a problem hiding this comment.
The error message 'Submission url is not a valid' seems to have a grammatical error. Consider changing it to 'Submission URL is not valid'.
| @@ -11,7 +11,7 @@ import { Challenge, MemberSubmission } from '../models' | |||
| * @param challenge challenge info | |||
| * @returns true if challenge is mm | |||
| */ | |||
| export function checkIsMM(challenge: Challenge): boolean { | |||
| export function checkIsMM(challenge?: Challenge): boolean { | |||
There was a problem hiding this comment.
The function checkIsMM now accepts an optional challenge parameter, but the logic inside the function does not handle the case where challenge is undefined before accessing its properties. Consider adding a check to handle undefined challenge to prevent potential runtime errors.
| @@ -1,6 +1,11 @@ | |||
| /** | |||
| * Util for other check | |||
| */ | |||
| import AmazonS3URI from 'amazon-s3-uri' | |||
There was a problem hiding this comment.
Consider adding error handling for the import AmazonS3URI from 'amazon-s3-uri' statement to manage cases where the module might not be available or fails to load.
| @@ -1,6 +1,11 @@ | |||
| /** | |||
| * Util for other check | |||
| */ | |||
| import AmazonS3URI from 'amazon-s3-uri' | |||
|
|
|||
| import { EnvironmentConfig } from '~/config' | |||
There was a problem hiding this comment.
Ensure that EnvironmentConfig is correctly configured and used within the application. If it's not used in this file, consider removing the import to keep the code clean.
|
|
||
| import { EnvironmentConfig } from '~/config' | ||
|
|
||
| import { ValidateS3URIResult } from '../models' |
There was a problem hiding this comment.
Verify that ValidateS3URIResult is used in this file. If it's not utilized, consider removing the import to avoid unnecessary code.
| isValid: true, | ||
| key: key ?? undefined, | ||
| } | ||
| } catch (error) {} |
There was a problem hiding this comment.
The catch block is empty, which can lead to silent failures and make debugging difficult. Consider logging the error or handling it appropriately.
| @@ -80,10 +80,16 @@ export const ADMIN = { | |||
| AGREE_ELECTRONICALLY: '5b2798b2-ae82-4210-9b4d-5d6428125ccb', | |||
| AGREE_FOR_DOCUSIGN_TEMPLATE: '999a26ad-b334-453c-8425-165d4cf496d7', | |||
| AV_SCAN_SCORER_REVIEW_TYPE_ID: '68c5a381-c8ab-48af-92a7-7a869a4ee6c3', | |||
| AVSCAN_TOPIC: 'avscan.action.scan', | |||
| AWS_CLEAN_BUCKET: '', | |||
There was a problem hiding this comment.
The AWS_CLEAN_BUCKET variable is currently an empty string. Ensure that this is intentional and that the correct bucket name will be provided in the appropriate environment configuration.
| AVSCAN_TOPIC: 'avscan.action.scan', | ||
| AWS_CLEAN_BUCKET: '', | ||
| AWS_DMZ_BUCKET: 'topcoder-dev-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', |
There was a problem hiding this comment.
The AWS_QUARANTINE_BUCKET variable is currently an empty string. Ensure that this is intentional and that the correct bucket name will be provided in the appropriate environment configuration.
| @@ -10,10 +10,16 @@ export const ADMIN = { | |||
| AGREE_ELECTRONICALLY: '2db6c920-4089-4755-9cd1-99b0df0af961', | |||
| AGREE_FOR_DOCUSIGN_TEMPLATE: '1363a7ab-fd3e-4d7c-abbb-2f7440b8b355', | |||
| AV_SCAN_SCORER_REVIEW_TYPE_ID: '55bbb17d-aac2-45a6-89c3-a8d102863d05', | |||
| AVSCAN_TOPIC: 'avscan.action.scan', | |||
| AWS_CLEAN_BUCKET: '', | |||
There was a problem hiding this comment.
Consider providing a default value or explanation for AWS_CLEAN_BUCKET to avoid potential issues with empty configurations.
| AVSCAN_TOPIC: 'avscan.action.scan', | ||
| AWS_CLEAN_BUCKET: '', | ||
| AWS_DMZ_BUCKET: 'topcoder-submissions', | ||
| AWS_QUARANTINE_BUCKET: '', |
There was a problem hiding this comment.
Consider providing a default value or explanation for AWS_QUARANTINE_BUCKET to avoid potential issues with empty configurations.
challenge https://www.topcoder.com/challenges/0ac19268-4459-46e0-8285-916072ccae0e