New work app to replace work-manager, and v6 projects API usage in copilots app#1487
New work app to replace work-manager, and v6 projects API usage in copilots app#1487
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,5 @@ | |||
| REACT_APP_GROUPS_API_URL=https://api.topcoder.com/v6/groups | |||
| REACT_APP_TERMS_API_URL=https://api.topcoder.com/v5/terms | |||
There was a problem hiding this comment.
[❗❗ correctness]
The API version for REACT_APP_TERMS_API_URL is v5, while others are v6. Ensure this is intentional and that the correct API version is being used.
| height: 400, | ||
| menubar: false, | ||
| plugins: ['table', 'link', 'textcolor', 'contextmenu'], | ||
| plugins: ['table', 'link'], |
There was a problem hiding this comment.
[correctness]
The removal of the textcolor plugin from the TinyMCE editor configuration might impact users who rely on text color customization. If this change is intentional, ensure that there are no requirements for text color editing. Otherwise, consider re-adding the plugin to maintain existing functionality.
| height: 400, | ||
| menubar: false, | ||
| plugins: ['table', 'link', 'textcolor', 'contextmenu'], | ||
| plugins: ['table', 'link'], |
There was a problem hiding this comment.
[correctness]
The removal of the contextmenu plugin could affect the user experience by disabling right-click context menus in the editor. Verify if this change aligns with the desired functionality and user requirements.
| const viewRequestDetails = useMemo(() => ( | ||
| routeParams.requestId && find(requests, { id: +routeParams.requestId }) as CopilotRequest | ||
| routeParams.requestId | ||
| ? find(requests, request => `${request.id}` === routeParams.requestId) as CopilotRequest |
There was a problem hiding this comment.
[💡 performance]
The use of string interpolation to compare request.id with routeParams.requestId could be avoided if routeParams.requestId is already a string. Consider ensuring request.id is a string when stored or retrieved, or use a more explicit conversion method if necessary.
| import { CopilotRequest } from '../models/CopilotRequest' | ||
|
|
||
| const baseUrl = `${EnvironmentConfig.API.V5}/projects` | ||
| const baseUrl = `${EnvironmentConfig.API.V6}/projects` |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that all endpoints and related logic are compatible with the new API version V6. This change might require additional updates elsewhere in the codebase to handle any differences in API responses or behavior.
| createdAt: new Date(data.createdAt), | ||
| data: undefined, | ||
| opportunity: data.copilotOpportunity?.[0], | ||
| projectId: String(data?.projectId ?? data?.data?.projectId ?? ''), |
There was a problem hiding this comment.
[correctness]
The use of String(data?.projectId ?? data?.data?.projectId ?? '') could lead to unexpected results if projectId is 0 or false. Consider explicitly checking for undefined or null to avoid coercing falsy values.
|
|
||
| export type ProjectsResponse = SWRResponse<Project[], Project[]> | ||
|
|
||
| const sleep = (ms: number): Promise<()=> void> => new Promise(resolve => { setTimeout(resolve, ms) }) |
There was a problem hiding this comment.
[correctness]
The sleep function returns a Promise<() => void>, which is misleading because the resolved value is actually undefined. Consider changing the return type to Promise<void> for clarity.
| const sleep = (ms: number): Promise<()=> void> => new Promise(resolve => { setTimeout(resolve, ms) }) | ||
| const normalizeProject = (project: Project): Project => ({ | ||
| ...project, | ||
| id: String(project.id), |
There was a problem hiding this comment.
[correctness]
The normalizeProject function converts project.id to a string. Ensure that this conversion is necessary and that all parts of the application expect id to be a string. If id is used as a number elsewhere, this could lead to inconsistencies.
| export const PageWrapper: FC<PropsWithChildren<Props>> = props => ( | ||
| <div className={classNames(styles.container, props.className)}> | ||
| <BreadCrumb list={props.breadCrumb} /> | ||
| {props.breadCrumb.length > 0 && ( |
There was a problem hiding this comment.
[correctness]
The check props.breadCrumb.length > 0 is a good optimization to avoid rendering an empty BreadCrumb component. However, ensure that props.breadCrumb is always initialized as an array to prevent potential runtime errors.
| {props.pageTitle} | ||
| </h3> | ||
| </PageHeader> | ||
| {props.titleAction |
There was a problem hiding this comment.
[💡 readability]
The conditional rendering of props.titleAction using a ternary operator is correct, but the use of undefined as the fallback is unnecessary. Consider using a logical AND (&&) operator for cleaner code: {props.titleAction && <div className={styles.blockTitleAction}>{props.titleAction}</div>}.
|
|
||
| const WorkApp: FC = () => { | ||
| const { getChildRoutes }: RouterContextData = useContext(routerContext) | ||
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) |
There was a problem hiding this comment.
[💡 performance]
Using useMemo here is unnecessary unless getChildRoutes is computationally expensive or has side effects. Consider removing useMemo if it's not needed for performance optimization.
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) | ||
|
|
||
| useEffect(() => { | ||
| document.body.classList.add(WORK_APP_BODY_CLASS) |
There was a problem hiding this comment.
[maintainability]
Directly manipulating the document.body.classList can lead to unexpected side effects, especially if multiple components are modifying the class list. Consider using a state management solution or a library like classnames to manage body classes more predictably.
| export const TABLE_DATE_FORMAT = 'MMM DD YYYY, HH:mm A' | ||
|
|
||
| export const PAGINATION_PER_PAGE_OPTIONS = [ | ||
| { label: '5', value: '5' }, |
There was a problem hiding this comment.
[correctness]
The value in PAGINATION_PER_PAGE_OPTIONS is a string, but it might be more appropriate to use a number type for consistency and to avoid potential type-related issues when using these values for pagination logic.
| @@ -0,0 +1,28 @@ | |||
| import { AppSubdomain, EnvironmentConfig } from '~/config' | |||
|
|
|||
| export const rootRoute: string | |||
There was a problem hiding this comment.
[💡 maintainability]
Consider using a function to determine rootRoute instead of a ternary operator. This could improve readability and maintainability, especially if the logic becomes more complex in the future.
| const defaultDate = new Date() | ||
| defaultDate.setHours(0, 0, 0, 0) | ||
|
|
||
| const daysUntilSaturday = (SATURDAY_DAY_INDEX - defaultDate.getDay() + 7) % 7 |
There was a problem hiding this comment.
[❗❗ correctness]
The calculation for daysUntilSaturday could be simplified by using (SATURDAY_DAY_INDEX - defaultDate.getDay() + 7) % 7. This ensures that the result is always non-negative, which is crucial for correctly setting the date.
| const [remarks, setRemarks] = useState<string>('') | ||
| const [weekEndingDate, setWeekEndingDate] = useState<Date | null>(() => getDefaultWeekEndingDate()) | ||
|
|
||
| const paymentTitle = useMemo( |
There was a problem hiding this comment.
[correctness]
The useMemo hook for paymentTitle depends on weekEndingDate, props.projectName, and props.engagementName. Ensure that these dependencies are correctly updated to prevent stale values from being used.
| }, []) | ||
|
|
||
| useEffect(() => { | ||
| if (!props.open) { |
There was a problem hiding this comment.
[design]
The useEffect hook resets the state whenever props.open changes. Consider whether this is the desired behavior, as it might reset the form unexpectedly if open is toggled.
| customInput={<WeekEndingInput />} | ||
| dateFormat='MM/dd/yyyy' | ||
| disabled={isSubmitting} | ||
| filterDate={isSaturday} |
There was a problem hiding this comment.
[💡 usability]
The filterDate function isSaturday ensures that only Saturdays can be selected. This is a good constraint, but ensure that users are aware of this restriction, perhaps through UI hints or documentation.
| <ul className={styles.list}> | ||
| {paymentsResult.payments.map((payment, index) => { | ||
| const paymentStatus = getPaymentStatus(payment) | ||
| const normalizedPaymentStatus = paymentStatus |
There was a problem hiding this comment.
[maintainability]
The trim() and toLowerCase() operations on paymentStatus could be moved into getPaymentStatus if this normalization is always required. This would improve maintainability by centralizing the logic.
| import { xhrGetAsync } from '~/libs/core' | ||
|
|
||
| export interface BillingAccount { | ||
| active?: boolean |
There was a problem hiding this comment.
[correctness]
The active property is optional, but its type is not specified. Consider specifying a boolean type for clarity and to avoid potential type-related issues.
|
|
||
| export interface BillingAccount { | ||
| active?: boolean | ||
| endDate?: string |
There was a problem hiding this comment.
[correctness]
The endDate property is optional and typed as a string. Consider specifying the expected date format or using a Date object to ensure consistent handling of date values.
| endDate?: string | ||
| id: number | string | ||
| name: string | ||
| startDate?: string |
There was a problem hiding this comment.
[correctness]
The startDate property is optional and typed as a string. Consider specifying the expected date format or using a Date object to ensure consistent handling of date values.
| id: number | string | ||
| name: string | ||
| startDate?: string | ||
| status?: string |
There was a problem hiding this comment.
[maintainability]
The status property is optional and typed as a string. Consider defining an enumeration for possible status values to improve type safety and maintainability.
|
|
||
| await Promise.all( | ||
| projectIds.map(async projectId => { | ||
| if (projectNameByProjectId.has(projectId)) { |
There was a problem hiding this comment.
[correctness]
Consider checking if projectId is a valid string before using it in fetchProjectById. This could prevent unnecessary API calls with invalid IDs.
| projectNameByProjectId.set(projectId, projectName) | ||
| } | ||
| } catch { | ||
| // Project lookup failures should not break engagement loading. |
There was a problem hiding this comment.
[maintainability]
Swallowing all errors in the catch block without logging or handling them might make debugging difficult. Consider logging the error or handling specific error cases.
| } | ||
|
|
||
| const projectId = getEngagementProjectId(engagement) | ||
| const projectName = projectNameByProjectId.get(projectId) |
There was a problem hiding this comment.
[correctness]
Accessing projectNameByProjectId without checking if projectId is a valid string could lead to unexpected behavior. Ensure projectId is valid before using it as a key.
| } | ||
| } | ||
|
|
||
| if (typedError?.response?.status !== 403) { |
There was a problem hiding this comment.
[correctness]
The check for typedError?.response?.status !== 403 should be more explicit. Consider using typedError?.response?.status !== 403 to ensure the status is exactly 403, as other 4xx errors might also indicate authorization issues.
| } | ||
|
|
||
| if ( | ||
| normalizedBillingAccount.active === undefined |
There was a problem hiding this comment.
[💡 maintainability]
The condition in the if statement checks for multiple properties being undefined or falsy. Consider using a utility function to improve readability and maintainability.
|
|
||
| return extractProjectTypes(response) | ||
| } catch (error) { | ||
| if (hasAuthorizationMetadataError(error)) { |
There was a problem hiding this comment.
[correctness]
The fallback mechanism for fetching project types when an authorization error occurs is a good approach. However, ensure that the fallback URL (PROJECT_METADATA_API_URL) is correct and that it provides the expected data structure.
| size='sm' | ||
| /> | ||
| </Link> | ||
| {assignedStatus |
There was a problem hiding this comment.
[maintainability]
The conditional rendering of the assignedStatus block is duplicated for both leftActions and rightActions. Consider refactoring this logic to avoid repetition and improve maintainability.
|
|
||
| <PaymentFormModal | ||
| billingAccountId={projectResult.project?.billingAccountId} | ||
| engagementName={engagementResult.engagement?.title} |
There was a problem hiding this comment.
[💡 correctness]
Ensure that engagementResult.engagement?.title is always defined before passing it to PaymentFormModal. If there's a chance it could be undefined, consider providing a default value or handling the case where it is not available.
| onCancel={() => setPaymentMember(undefined)} | ||
| onConfirm={handlePaymentSubmit} | ||
| open={!!paymentMember} | ||
| projectName={projectResult.project?.name} |
There was a problem hiding this comment.
[💡 correctness]
Ensure that projectResult.project?.name is always defined before passing it to PaymentFormModal. If there's a chance it could be undefined, consider providing a default value or handling the case where it is not available.
| return ( | ||
| <PageWrapper | ||
| backUrl={backUrl} | ||
| breadCrumb={[]} |
There was a problem hiding this comment.
[maintainability]
The breadCrumb prop is now set to an empty array, which might lead to a loss of navigation context for users. If the breadcrumb functionality is intended to be removed, ensure that this change is communicated to the team and any dependent components or tests are updated accordingly.
| } | ||
| } | ||
|
|
||
| const currentBillingAccount = params.billingAccounts.find( |
There was a problem hiding this comment.
[correctness]
Consider using Array.prototype.find with a type guard or a more specific type assertion to ensure currentBillingAccount is of type BillingAccount. This will help prevent runtime errors if billingAccounts contains unexpected types.
| const currentBillingAccount = params.billingAccounts.find( | ||
| billingAccount => String(billingAccount.id) === params.currentBillingAccountId, | ||
| ) | ||
| const billingAccountWithDetails: ProjectBillingAccount | BillingAccount | undefined |
There was a problem hiding this comment.
[design]
The variable billingAccountWithDetails is assigned using a fallback pattern. Ensure that params.projectBillingAccount and currentBillingAccount are mutually exclusive or handle cases where both might be defined to avoid unexpected behavior.
| } | ||
|
|
||
| return { | ||
| endDate: getBillingAccountDate(billingAccountWithDetails, 'endDate'), |
There was a problem hiding this comment.
[correctness]
The function getBillingAccountDate is called with billingAccountWithDetails which can be either ProjectBillingAccount or BillingAccount. Ensure that both types have the expected properties (endDate and startDate) to avoid potential runtime errors.
| return { | ||
| endDate: getBillingAccountDate(billingAccountWithDetails, 'endDate'), | ||
| id: params.currentBillingAccountId, | ||
| name: resolvedBillingAccountName || getBillingAccountName(billingAccountWithDetails), |
There was a problem hiding this comment.
[💡 maintainability]
The resolvedBillingAccountName is computed using multiple fallbacks. Consider logging or handling cases where all fallbacks result in undefined to improve debugging and ensure data integrity.
| const savedChallenge = await createChallenge({ | ||
| name: formData.name, | ||
| projectId: createProjectId, | ||
| status: CHALLENGE_STATUS.NEW, |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of roundType from the createChallenge payload may impact the challenge creation logic if roundType is required or used elsewhere in the application. Ensure that this change is intentional and that roundType is not needed for the challenge creation process.
| } | ||
|
|
||
| downloadProfileAsync(application.handle) | ||
| .catch(() => undefined) |
There was a problem hiding this comment.
[maintainability]
Catching and ignoring all errors with .catch(() => undefined) can make debugging difficult. Consider logging the error or handling specific error cases.
| </div> | ||
| <div> | ||
| <span className={styles.label}>Experience (years)</span> | ||
| <span className={styles.value}>{application.yearsOfExperience ?? '-'}</span> |
There was a problem hiding this comment.
[correctness]
Using the nullish coalescing operator (??) is appropriate here, but ensure that application.yearsOfExperience is not intended to be 0, as 0 will be replaced by '-'. If 0 is a valid value, consider using a different check.
| const parsedDuration = Number(duration) | ||
| const maxDuration = PHASE_DURATION_MAX_HOURS * 60 | ||
|
|
||
| if (!Number.isFinite(parsedDuration)) { |
There was a problem hiding this comment.
[correctness]
The normalizeDuration function uses Number.isFinite to check if parsedDuration is a valid number. However, Number.isFinite will return false for NaN, Infinity, and -Infinity. If the intention is to only check for NaN, consider using Number.isNaN instead, or clarify the behavior if Infinity values are expected.
| const predecessorEndDate = toDate(predecessorPhase?.scheduledEndDate) | ||
|
|
||
| if (predecessorEndDate) { | ||
| phaseStartDate = isIterativeReviewPhase(phase.name) |
There was a problem hiding this comment.
[correctness]
In the recalculatePhases function, when determining phaseStartDate for an iterative review phase, the logic defaults to predecessorStartDate || predecessorEndDate. This could lead to unexpected behavior if predecessorStartDate is undefined but predecessorEndDate is valid. Consider verifying if this logic aligns with the intended scheduling behavior.
| ? predecessorStartDate || predecessorEndDate | ||
| : predecessorEndDate | ||
| } else if (shouldScheduleDates && !calculationError) { | ||
| const phaseName = phase.name || phase.phaseId || `${index + 1}` |
There was a problem hiding this comment.
[maintainability]
The error message for an invalid predecessor configuration is set but not immediately returned or logged. Ensure that the error is handled appropriately, either by returning it immediately or by logging it for debugging purposes.
| domain: AppSubdomain.work, | ||
| element: <WorkApp />, | ||
| id: toolTitle, | ||
| rolesRequired: WORK_MANAGER_ALLOWED_ROLES, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that WORK_MANAGER_ALLOWED_ROLES is correctly defined and includes all necessary roles for accessing this route. If this list is incomplete or incorrect, it could lead to unauthorized access or prevent authorized users from accessing the route.
| expect(getChallengeLinkId({ | ||
| id: '', | ||
| legacyId: 654_321, | ||
| })) |
There was a problem hiding this comment.
[correctness]
Consider using String(legacyId) to ensure consistent string conversion, as toBe('654321') expects a string.
| id: '', | ||
| legacy: { id: 123_456 }, | ||
| })) | ||
| .toBe('123456') |
There was a problem hiding this comment.
[correctness]
Consider using String(legacy.id) to ensure consistent string conversion, as toBe('123456') expects a string.
| && props.challenge.projectId !== undefined | ||
| const hasLegacyId | ||
| = 'legacyId' in props.challenge && props.challenge.legacyId !== undefined | ||
| = typeof props.challenge.projectId === 'number' |
There was a problem hiding this comment.
[💡 readability]
The check typeof props.challenge.projectId === 'number' is redundant if props.challenge.projectId > 0 is already being checked, as the greater-than comparison will implicitly ensure it's a number. Consider removing the typeof check for simplicity.
| `${EnvironmentConfig.ADMIN.REVIEW_UI_URL}/=${props.challenge.id}` /* eslint-disable-line max-len */ | ||
| } | ||
| href={getReviewUiChallengeUrl( | ||
| EnvironmentConfig.ADMIN.REVIEW_UI_URL, |
There was a problem hiding this comment.
[correctness]
Passing an empty string as a fallback for challengeLinkId in getReviewUiChallengeUrl could lead to unexpected behavior if challengeLinkId is null or undefined. Ensure that getChallengeLinkId always returns a valid string or handle the fallback case more explicitly.
| * Returns whether the Review UI link can be opened for a challenge. | ||
| */ | ||
| export function canOpenReviewUi(challengeId?: string): boolean { | ||
| return Boolean(challengeId?.trim()) |
There was a problem hiding this comment.
[💡 performance]
The use of challengeId?.trim() in Boolean(challengeId?.trim()) is redundant because Boolean() will already handle null or undefined values. Consider simplifying to Boolean(challengeId && challengeId.trim()) to ensure trim() is only called when challengeId is not null or undefined.
| COMPLETED_REVIEW_STATUSES.has((value ?? '').toUpperCase()) | ||
| ) | ||
|
|
||
| const resolveScreeningReviewDetails = (entry: Screening): ScreeningReviewDetail[] => { |
There was a problem hiding this comment.
[correctness]
The function resolveScreeningReviewDetails returns an array of ScreeningReviewDetail objects. However, if entry.screeningReviews is not defined, it constructs a new array with a single object. This could lead to inconsistent behavior if screeningReviews is expected to always be an array. Consider ensuring screeningReviews is always an array to simplify logic and avoid potential issues.
| hasMultipleScreeners, | ||
| maxScreenerCount, | ||
| }: ScreeningColumnConfig): TableColumn<Screening>[] => { | ||
| const screenerColumnCount = Math.max(1, maxScreenerCount) |
There was a problem hiding this comment.
[correctness]
The screenerColumnCount is calculated using Math.max(1, maxScreenerCount). If maxScreenerCount is less than 1, this will default to 1, which might not be the intended behavior. Ensure that maxScreenerCount is validated or calculated correctly to avoid unexpected UI behavior.
| return <span>--</span> | ||
| } | ||
|
|
||
| const maskScore = shouldMaskScore(data, detail) |
There was a problem hiding this comment.
[❗❗ correctness]
The shouldMaskScore function is called with both data and detail arguments, but the logic inside shouldMaskScore may not account for detail being undefined. Ensure that shouldMaskScore handles cases where detail is not provided to avoid runtime errors.
| label: 'Screening Score', | ||
| propertyName: 'screening-aggregate-score', | ||
| renderer: (data: Screening) => { | ||
| if (hasIncompleteScreeningReviews(data)) { |
There was a problem hiding this comment.
[💡 readability]
In the renderer function for the 'Screening Score' column, the condition hasIncompleteScreeningReviews(data) is checked. If this returns true, a placeholder -- is shown. Consider providing more context to the user about why the score is not available, as this could improve user experience.
|
|
||
| const isSubmissionNotViewable = (submission: Screening): boolean => ( | ||
| !canViewSubmissions && String(submission.memberId) !== String(loginUserInfo?.userId) | ||
| const isSubmissionNotViewable = useCallback( |
There was a problem hiding this comment.
[💡 performance]
The isSubmissionNotViewable function is wrapped in useCallback, but it does not seem to have any dependencies that would change. Consider removing useCallback if memoization is not necessary, as it adds unnecessary complexity.
| ? candidateReviews | ||
| : (matchedReview ? [matchedReview] : []) | ||
|
|
||
| const screeningReviewsWithScore = reviewEntries.map(reviewEntry => { |
There was a problem hiding this comment.
[maintainability]
The logic for determining reviewNumericScore is duplicated from the removed lines. Consider refactoring this logic into a separate function to improve maintainability and reduce code duplication.
| const hasPassingResult = screeningReviews.some(reviewDetail => ( | ||
| (reviewDetail.result ?? '').toUpperCase() === 'PASS' | ||
| )) | ||
| const aggregatedScoreFallback = hasMultipleScreeners |
There was a problem hiding this comment.
[maintainability]
The aggregatedScoreFallback logic is repeated multiple times in different forms. Consider extracting this logic into a helper function to improve maintainability and readability.
| ? '--' | ||
| : scoreToDisplay(averageNumericScore, aggregatedScoreFallback) | ||
|
|
||
| let aggregatedResult: Screening['result'] = base.result |
There was a problem hiding this comment.
[readability]
The logic for determining aggregatedResult is complex and involves multiple conditions. Consider simplifying or breaking it down into smaller functions to enhance readability and maintainability.
| reviewStatus?: string | ||
| screenerId?: string | ||
| screener?: BackendResource | ||
| score: string |
There was a problem hiding this comment.
[correctness]
The score field is defined as a string, but it might be more appropriate to use a number type if it represents a numerical score. This would prevent potential issues with numerical operations and comparisons.
| updated?: string | ||
| createdBy?: string | ||
| copilot?: string | ||
| reviewer?: string |
There was a problem hiding this comment.
[design]
The new reviewer field is a string, but there is already a reviewers field of type Reviewer[]. Ensure that the reviewer field is not redundant or conflicting with the reviewers array. Consider whether this should be a single Reviewer object or if it should be integrated with the existing reviewers array.
| prizeSets?: PrizeSet[] | ||
| privateDescription?: string | ||
| reviewers?: Reviewer[] | ||
| reviewer?: string |
There was a problem hiding this comment.
[maintainability]
The addition of the reviewer field alongside reviewers could lead to confusion. Consider clarifying the distinction between these fields or consolidating them if they serve similar purposes.
| tags: string[] | ||
| terms?: string[] | ||
| legacy?: { | ||
| isTask?: boolean |
There was a problem hiding this comment.
[maintainability]
The isTask field is added to the legacy object. Ensure that this field is necessary and that its usage is well-documented elsewhere in the codebase to avoid confusion, as legacy fields can often lead to technical debt.
| challengeBasicInfoSchema, | ||
| } from './challenge-editor.schema' | ||
|
|
||
| jest.mock('~/config', () => ({ |
There was a problem hiding this comment.
[maintainability]
Using jest.mock with a Proxy can lead to unexpected behavior if the get trap is not carefully managed. Consider explicitly handling all expected properties to avoid potential issues with unexpected properties returning the default URL.
| prizes?: unknown[] | ||
| } | undefined | ||
|
|
||
| return Array.isArray(placementPrizeSet?.prizes) |
There was a problem hiding this comment.
[💡 readability]
The condition (placementPrizeSet?.prizes?.length || 0) > 0 could be simplified to placementPrizeSet?.prizes?.length > 0 for better readability.
| isTask: boolean | undefined, | ||
| reviewType: string | undefined, | ||
| ): boolean => ( | ||
| isTask === true |
There was a problem hiding this comment.
[💡 readability]
The condition String(reviewType || '').trim().toUpperCase() === REVIEW_TYPES.INTERNAL could be simplified to reviewType?.trim().toUpperCase() === REVIEW_TYPES.INTERNAL for better readability and to avoid unnecessary string operations.
| const status = normalizeOptionalString(formData.status) | ||
| ?.toUpperCase() | ||
| const billingAccountId = normalizeOptionalId(formData.billing?.billingAccountId) | ||
| const prizeSets = formData.funChallenge === true |
There was a problem hiding this comment.
[correctness]
The logic for setting prizeSets based on formData.funChallenge could lead to unexpected behavior if formData.funChallenge is not explicitly set to true or false. Consider explicitly checking for true to avoid potential issues with falsy values.
| ]) | ||
|
|
||
| useEffect(() => { | ||
| if (isChallengeCreated || isMarathonMatchChallengeSelected || values.funChallenge !== true) { |
There was a problem hiding this comment.
[💡 maintainability]
The condition values.funChallenge !== true is checked twice in the useEffect dependencies and the if statement. Consider simplifying the logic to avoid redundancy.
| <ChallengeNameField /> | ||
| <ChallengeTrackField disabled={isChallengeCreated} /> | ||
| <ChallengeTypeField disabled={isChallengeCreated} /> | ||
| {showFunChallengeField |
There was a problem hiding this comment.
[💡 style]
The conditional rendering of FunChallengeField using undefined can be simplified by using null instead, which is more idiomatic in React for rendering nothing.
| </div> | ||
| </section> | ||
|
|
||
| {showPrizesAndBillingSection |
There was a problem hiding this comment.
[💡 style]
The conditional rendering of the Prizes & Billing section using undefined can be simplified by using null instead, which is more idiomatic in React for rendering nothing.
| ? <AssignedMemberField /> | ||
| : undefined} | ||
| <CopilotField projectId={fallbackProjectId} /> | ||
| {isTaskChallengeSelected |
There was a problem hiding this comment.
[💡 style]
The conditional rendering of ReviewTypeField using undefined can be simplified by using null instead, which is more idiomatic in React for rendering nothing.
| ) | ||
| const isReviewerSelectDisabled = useMemo( | ||
| (): boolean => { | ||
| if (!props.projectId || areProjectMembersLoading) { |
There was a problem hiding this comment.
[correctness]
The condition !props.projectId || areProjectMembersLoading is used to disable the reviewer select field. Consider adding a check for projectMembers.length === 0 to ensure the field is disabled when there are no members, even if loading is complete.
| ) | ||
|
|
||
| useEffect(() => { | ||
| if (!props.isTaskChallenge) { |
There was a problem hiding this comment.
[correctness]
The useEffect hook that sets the review type to INTERNAL for task challenges does not handle the case where setValue might not be available or might fail. Consider adding error handling to ensure robustness.
| } | ||
|
|
||
| return ( | ||
| <div className={styles.container}> |
There was a problem hiding this comment.
[💡 design]
The FormRadioGroup is disabled unconditionally. If this is intentional, ensure that it aligns with the intended UX. Otherwise, consider making this behavior conditional based on props or state.
| return false | ||
| } | ||
|
|
||
| const normalizedStatus = toOptionalString(invite.status) |
There was a problem hiding this comment.
[correctness]
The toOptionalString function is used to normalize the invite.status before checking its presence in OPEN_INVITE_STATUSES. Ensure that toOptionalString correctly handles all possible input types for invite.status to avoid unexpected behavior.
| return `At least one member reviewer with a scorecard is required for phase "${uncoveredPhase.name}".` | ||
| } | ||
|
|
||
| interface ReviewerValidationOptions { |
There was a problem hiding this comment.
[maintainability]
The renaming of ReviewerLaunchValidationOptions to ReviewerValidationOptions is appropriate given the removal of the launch-specific logic. However, ensure that all references to this interface outside of this file are updated accordingly to prevent any potential type errors.
| name: options.challengeTypeName, | ||
| }) | ||
|
|
||
| if (!isMarathonMatch && reviewers.length === 0 && requiredPhases.length > 0) { |
There was a problem hiding this comment.
[correctness]
Replacing the hardcoded error message with options.requiredReviewersErrorMessage improves flexibility. Ensure that this message is appropriately set in all contexts where getReviewerValidationError is called to avoid missing error messages.
| }: SaveStatusMetadata = getSaveStatusMetadata(formData.status, {}) | ||
|
|
||
| if (isSaveAsDraft) { | ||
| const reviewerValidationError = getReviewerValidationError(formData, { |
There was a problem hiding this comment.
[💡 maintainability]
The conditional check for isSaveAsDraft and subsequent validation logic is correct. However, consider extracting this logic into a separate function to improve readability and maintainability, especially if similar validation is needed elsewhere.
|
|
||
| import { aggregateSubmissionReviews } from './aggregateSubmissionReviews' | ||
|
|
||
| jest.mock('~/libs/core', () => ({ |
There was a problem hiding this comment.
[correctness]
The mock for getRatingColor returns a hardcoded color #2a2a2a. If the color is expected to vary based on input, consider adding test cases to verify different scenarios.
| ] | ||
|
|
||
| const rows = aggregateSubmissionReviews({ | ||
| mappingReviewAppeal: {} as MappingReviewAppeal, |
There was a problem hiding this comment.
[correctness]
The mappingReviewAppeal is cast to MappingReviewAppeal with an empty object. Ensure that this casting does not hide potential issues with the expected structure of MappingReviewAppeal. Consider initializing it with a more realistic mock or verifying its structure in the test.
| .toHaveLength(2) | ||
|
|
||
| const firstRowScores = new Map( | ||
| rows[0].reviews.map(review => [review.reviewerHandle, review.finalScore]), |
There was a problem hiding this comment.
[correctness]
The use of Map to store scores by reviewerHandle is appropriate here, but ensure that reviewerHandle is always unique within the context of rows[0].reviews. If there is any chance of duplication, consider handling it explicitly.
|
|
||
| const normalizedHandle = normalizeStringValue(reviewerHandle) | ||
| if (normalizedHandle) { | ||
| return `handle:${normalizedHandle.toLowerCase()}` |
There was a problem hiding this comment.
[correctness]
The use of toLowerCase() on normalizedHandle assumes that all reviewer handles should be case-insensitive. Ensure this aligns with the business logic, as it might lead to unexpected behavior if handles are case-sensitive.
| // Prefer the explicit reviewers list; otherwise fall back to discovered resourceIds. | ||
| const orderedResourceIds: string[] = (() => { | ||
| const compareReviewerIds = (a: string, b: string): number => ( | ||
| (reviewerHandleByResourceId[a] || a) |
There was a problem hiding this comment.
[correctness]
The compareReviewerIds function uses localeCompare with sensitivity: 'base', which ignores case differences. Confirm that this is the intended behavior, as it may affect the ordering of reviewer IDs if case sensitivity is required.
| }) | ||
| if (reviewerKey) { | ||
| r.reviewerKey = reviewerKey | ||
| if (!byReviewerKey[reviewerKey]) { |
There was a problem hiding this comment.
[design]
The logic for assigning reviewerKey to byReviewerKey assumes that the first occurrence of a reviewer key is the correct one to store. Consider if there are scenarios where multiple reviews could have the same reviewerKey but different details, and if so, how those should be handled.
| perPage: String(userIdChunk.length), | ||
| }) | ||
| userIdChunk.forEach(userId => { | ||
| query.append('userIds[]', userId) |
There was a problem hiding this comment.
[performance]
Appending userIds[] in a loop can lead to a large query string if userIdChunk is large. Consider checking the maximum URL length supported by the server to avoid potential issues.
| perPage: String(chunkUserIds.length), | ||
| }) | ||
| chunkUserIds.forEach(userId => { | ||
| searchQuery.append('userIds[]', userId) |
There was a problem hiding this comment.
[correctness]
The use of searchQuery.append('userIds[]', userId) is correct, but ensure that the backend API correctly interprets this format for array parameters. If the API expects a different format, this could lead to unexpected results.
| ) | ||
| const isChallengeCreated = !!currentChallengeId | ||
| const isFunChallengeSelected = values.funChallenge === true | ||
| const showFunChallengeField = isMarathonMatchChallengeSelected |
There was a problem hiding this comment.
[correctness]
The condition for showFunChallengeField has been changed to always show the field for Marathon Match challenges, even if the challenge is already created. Ensure this change is intentional, as it alters the previous logic where the field was not shown for created challenges.
| } | ||
|
|
||
| const savedChallenge = await createChallenge({ | ||
| funChallenge: formData.funChallenge === true, |
There was a problem hiding this comment.
[design]
Ensure that the funChallenge property is correctly handled by the createChallenge function. If this is a new addition, verify that the backend supports this field and that it is necessary for the challenge creation process.
| ) | ||
|
|
||
| const hasChallengeDetailsAccess = canOpenReviewUi(props.challenge.id) | ||
| const hasProjectId |
There was a problem hiding this comment.
[performance]
The function canOpenReviewUi is called twice with the same argument props.challenge.id on lines 187 and 192. Consider storing the result in a variable to avoid redundant function calls and improve performance.
| resourceId, | ||
| reviewerHandle, | ||
| }: ReviewerIdentityArgs): string | undefined => { | ||
| const normalizedHandle = normalizeStringValue(reviewerHandle) |
There was a problem hiding this comment.
[correctness]
The order of normalization checks has been changed, prioritizing reviewerHandle over memberId. Ensure this change aligns with the intended logic, as it may affect how reviewer identities are resolved.
| return false | ||
| } | ||
|
|
||
| if (r.reviewerKey && orderedReviewerKeys.has(r.reviewerKey)) { |
There was a problem hiding this comment.
[💡 maintainability]
The condition r.reviewerKey && orderedReviewerKeys.has(r.reviewerKey) is redundant because orderedReviewerKeys is derived from ordered, which already includes all reviewerKey values from ordered. Consider removing this condition to simplify the logic.
| } | ||
|
|
||
| if (typeof value === 'number' && Number.isFinite(value)) { | ||
| return String(value) |
There was a problem hiding this comment.
[correctness]
Converting a number to a string without trimming could lead to unexpected results if the number is very large or has decimal places. Consider if this behavior is intended.
| ] | ||
| .slice(0, reviewerSlots) | ||
| .map(memberId => toNormalizedText(memberId)) | ||
| const missingSlotIndex = normalizedAssignedMemberSlots.findIndex(memberId => !memberId) |
There was a problem hiding this comment.
[correctness]
Using findIndex to check for missing slots is efficient, but ensure that toNormalizedText correctly handles all potential falsy values that could be present in normalizedAssignedMemberSlots.
1 participant
https://work.topcoder-dev.com