System admin fixes #1317
System admin fixes #1317
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| challengeInfo, | ||
| }: useFetchChallengeProps = useFetchChallenge(challengeId) | ||
| const isMM = useMemo(() => checkIsMM(challengeInfo), [challengeInfo]) | ||
| const submissionReprocessTopic = useMemo( |
There was a problem hiding this comment.
[💡 performance]
Using useMemo for submissionReprocessTopic might not be necessary if getSubmissionReprocessTopic is not computationally expensive. Consider removing useMemo if performance is not a concern here.
| isLoadingBool: isReprocessingSubmissionBool, | ||
| doReprocessSubmission, | ||
| }: useManageSubmissionReprocessProps | ||
| = useManageSubmissionReprocess(submissionReprocessTopic) |
There was a problem hiding this comment.
[correctness]
Ensure that submissionReprocessTopic is always a valid topic string before passing it to useManageSubmissionReprocess. If getSubmissionReprocessTopic can return an invalid or empty value, it might cause unexpected behavior.
| 'mime-type': 'application/json', | ||
| originator: 'review-api-v6', | ||
| payload, | ||
| timestamp: new Date() |
There was a problem hiding this comment.
[💡 performance]
Consider using Date.now() for generating the timestamp to avoid creating a new Date object unnecessarily. This can slightly improve performance by reducing object creation overhead.
| isReprocessingSubmission={ | ||
| props.isReprocessingSubmission | ||
| } | ||
| doReprocessSubmission={function doReprocessSubmission() { |
There was a problem hiding this comment.
[performance]
Consider using an arrow function for doReprocessSubmission to maintain consistency with other function props and avoid unnecessary function creation on each render.
| AV Rescan | ||
| </Button> | ||
| )} | ||
| {props.canReprocessSubmission && ( |
There was a problem hiding this comment.
[correctness]
The canReprocessSubmission prop is optional, but its usage here assumes it is always defined. Consider providing a default value or handling the case where it might be undefined to avoid potential runtime errors.
| props.doReprocessSubmission() | ||
| }} | ||
| primary | ||
| disabled={props.isReprocessingSubmission[props.data.id]} |
There was a problem hiding this comment.
[correctness]
Ensure that props.data.id is always a valid key in isReprocessingSubmission. If props.data.id can be undefined or not present in isReprocessingSubmission, this could lead to unexpected behavior.
|
|
||
| const doReprocessSubmission = useCallback( | ||
| async (submission: Submission) => { | ||
| if (!topic) { |
There was a problem hiding this comment.
[design]
The topic parameter is used to determine if the reprocess action should proceed. Consider validating topic earlier or at the point of function call to prevent unnecessary function execution.
| toast.error( | ||
| 'Submission reprocess is only available for specific challenge types.', | ||
| { | ||
| toastId: 'Reprocess submission', |
There was a problem hiding this comment.
[💡 maintainability]
The toastId is hardcoded as 'Reprocess submission'. If multiple toasts are triggered simultaneously, consider using a unique identifier to prevent potential conflicts.
|
|
||
| try { | ||
| const payload | ||
| = await createSubmissionReprocessPayload(submission) |
There was a problem hiding this comment.
[❗❗ correctness]
The createSubmissionReprocessPayload function is awaited, which implies it returns a promise. Ensure this function handles errors internally or consider wrapping it in a try-catch block to handle potential rejections.
| export interface RequestBusAPIReprocess { | ||
| topic: string | ||
| originator: string | ||
| timestamp: string |
There was a problem hiding this comment.
[correctness]
Consider using a more specific type for timestamp such as Date or a validated date string format to ensure consistency and prevent potential parsing errors.
| topic: string | ||
| originator: string | ||
| timestamp: string | ||
| 'mime-type': string |
There was a problem hiding this comment.
[💡 style]
The property name 'mime-type' is using a string literal which could lead to potential issues with property access. Consider using camelCase mimeType for consistency with TypeScript conventions.
| submissionUrl: string | ||
| memberHandle: string | ||
| memberId: string | ||
| submittedDate: string |
There was a problem hiding this comment.
[correctness]
The submittedDate field is typed as string. Consider using a more specific type like Date or a validated date string format to ensure proper handling of date values.
| const challengeId = submissionInfo.challengeId | ||
| const submissionUrl = submissionInfo.url | ||
| const memberId = submissionInfo.memberId | ||
| const memberHandle = submissionInfo.submitterHandle ?? submissionInfo.createdBy |
There was a problem hiding this comment.
[correctness]
The use of submissionInfo.submitterHandle ?? submissionInfo.createdBy assumes that createdBy is a valid fallback for submitterHandle. Ensure that createdBy is always a valid and appropriate substitute, or consider handling cases where both might be undefined.
| throw new Error('Member handle is not valid') | ||
| } | ||
|
|
||
| const submittedDate = submittedDateValue |
There was a problem hiding this comment.
[correctness]
The conversion of submittedDateValue to a Date object and subsequent validation could fail silently if submittedDateValue is an invalid date string. Consider adding explicit error handling for invalid date formats to prevent unexpected behavior.
| } | ||
|
|
||
| return filter | ||
| return qs.stringify(params, { arrayFormat: 'brackets', skipNulls: true }) |
There was a problem hiding this comment.
[correctness]
The use of qs.stringify with arrayFormat: 'brackets' is appropriate for handling arrays in query parameters. However, ensure that the backend service correctly interprets this format, as it may expect a different array format (e.g., comma-separated values).
| export function getSubmissionReprocessTopic( | ||
| challenge?: Challenge, | ||
| ): string | undefined { | ||
| const normalizedType = challenge?.type?.name |
There was a problem hiding this comment.
[maintainability]
The normalizedType variable is being assigned an empty string if challenge?.type?.name is not defined. Consider returning undefined early if challenge or challenge.type is undefined to avoid unnecessary processing.
| return SUBMISSION_REPROCESS_TOPICS.TOPGEAR_TASK | ||
| } | ||
|
|
||
| return undefined |
There was a problem hiding this comment.
[correctness]
Returning undefined here might lead to unexpected behavior if the caller does not handle this case. Consider documenting this behavior or ensuring that the caller is prepared to handle an undefined return value.
Allow admins the ability to reprocess submissions