[PROD RELEASE V6] #1288
[PROD RELEASE V6] #1288
Conversation
feat(PM-1722): Drag and drop implementation in created/edit scorecards
Update Review Management Page: Add Submission End Date Column
fix(PM-1805): category not updated when project type is changed in edit scorecard page
fix(PM-1805): Category not saved properly
Dev changes to v6
Feat/review into feat/v6
Update Submissions Endpoints and perPage
Feat/system admin
This reverts commit 6ad92b3.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| dateDescription: string | ||
| endDate?: Date | ||
| id: number | ||
| endYear?: string |
There was a problem hiding this comment.
[correctness]
Changing endDate from Date to endYear as string may lead to issues if date manipulation is needed later. Consider whether this change is appropriate for the intended use cases.
| id: number | ||
| endYear?: string | ||
| id: number, | ||
| traitId: string, |
There was a problem hiding this comment.
[maintainability]
Adding traitId as a required field in EducationInfo may affect existing code that uses this interface. Ensure all usages of EducationInfo are updated to handle this new field appropriately.
| language: boolean; | ||
| skills: boolean; | ||
| work: boolean; | ||
| profile_picture: boolean; |
There was a problem hiding this comment.
[💡 style]
Consider using camelCase for profile_picture to maintain consistency with JavaScript naming conventions and improve readability.
| @@ -1,5 +1,5 @@ | |||
| export default interface WorkInfo { | |||
| company?: string | |||
| companyName?: string | |||
There was a problem hiding this comment.
[❗❗ correctness]
The change from company to companyName in the WorkInfo interface should be reflected in the emptyWorkInfo function as well. Currently, emptyWorkInfo still uses company, which will lead to a mismatch and potential runtime errors when accessing companyName on objects created by emptyWorkInfo.
| import _ from 'lodash' | ||
| import classNames from 'classnames' | ||
| import moment from 'moment' | ||
|
|
There was a problem hiding this comment.
[correctness]
The removal of moment import suggests a change in date handling. Ensure that the new approach using endYear is consistent with the rest of the application and correctly handles all date-related logic.
| const endDate: Date | undefined = educationItem.endDate | ||
| const endDateString: string = endDate ? moment(endDate) | ||
| .format('YYYY') : '' | ||
| const endYear: string = educationItem.endYear as string |
There was a problem hiding this comment.
[correctness]
Casting educationItem.endYear to string assumes that endYear is always a valid string. Consider adding validation or handling cases where endYear might not be a string to prevent potential runtime errors.
| onClick={function onClick() { | ||
| onClick={async function onClick() { | ||
| if (!props.reduxOnboardingChecklist) { | ||
| await props.createOnboardingChecklist({ |
There was a problem hiding this comment.
[💡 readability]
The use of props.reduxOnboardingChecklist || {} in the createOnboardingChecklist call is redundant since props.reduxOnboardingChecklist is already checked for null or undefined in the preceding if condition. Consider removing the fallback to {} for clarity.
| work: !!props.reduxWorks?.length, | ||
| } as OnboardingChecklistInfo) | ||
| } else { | ||
| await props.updateMemberOnboardingChecklist({ |
There was a problem hiding this comment.
[💡 readability]
The use of props.reduxOnboardingChecklist || {} in the updateMemberOnboardingChecklist call is unnecessary because props.reduxOnboardingChecklist is guaranteed to be defined in this branch of the conditional. Consider removing the fallback to {} for clarity.
| ...(endDateString ? [endDateString] : []), | ||
| ].join('-'), | ||
| description: workItem.company, | ||
| description: workItem.companyName, |
There was a problem hiding this comment.
[correctness]
Ensure that workItem.companyName is correctly populated in all contexts where workItem is used. If companyName is not guaranteed to be present, consider adding validation or fallback logic to handle potential undefined or null values.
| await createMemberTraits(tokenInfo.handle || '', createOnboardingChecklistData(onboardingChecklist)) | ||
| dispatch(updateOnboardingChecklist(onboardingChecklist)) | ||
| } catch (error) { | ||
| } |
There was a problem hiding this comment.
[maintainability]
The catch block is empty, which means any errors during the async operation will be silently ignored. Consider logging the error or handling it appropriately to avoid potential debugging difficulties.
|
|
||
| await updateMemberTraits(tokenInfo.handle || '', createOnboardingChecklistData(onboardingChecklistInfo)) | ||
| dispatch(updateOnboardingChecklist(onboardingChecklistInfo)) | ||
| } catch (error) { |
There was a problem hiding this comment.
[maintainability]
The catch block is empty, which means any errors during the async operation will be silently ignored. Consider logging the error or handling it appropriately to avoid potential debugging difficulties.
|
|
||
| await createMemberTraits(tokenInfo.handle || '', createOnboardingChecklistData(onboardingChecklist)) | ||
| dispatch(updateOnboardingChecklist(onboardingChecklist)) | ||
| } catch (error) { |
There was a problem hiding this comment.
[correctness]
The createOnboardingChecklist function does not handle the case where getAsyncToken or createMemberTraits might fail. Consider adding error handling to ensure the function behaves predictably in case of failure.
| const tokenInfo: TokenModel = await getAsyncToken() | ||
|
|
||
| await updateMemberTraits(tokenInfo.handle || '', createOnboardingChecklistData(onboardingChecklistInfo)) | ||
| dispatch(updateOnboardingChecklist(onboardingChecklistInfo)) |
There was a problem hiding this comment.
[correctness]
The updateMemberOnboardingChecklist function does not handle the case where getAsyncToken or updateMemberTraits might fail. Consider adding error handling to ensure the function behaves predictably in case of failure.
| } | ||
|
|
||
| tcUniNav( | ||
| getTcUniNav()?.( |
There was a problem hiding this comment.
[correctness]
Using optional chaining (?.) with getTcUniNav() assumes that the function might return undefined. If getTcUniNav() is expected to always return a valid function, consider removing the optional chaining for clarity and to avoid potential silent failures.
| headerInit.current = true | ||
|
|
||
| tcUniNav( | ||
| getTcUniNav()?.( |
There was a problem hiding this comment.
[correctness]
The use of optional chaining with getTcUniNav()?.('init', ...) assumes that getTcUniNav might return undefined. Ensure that the rest of the code can handle the case where the function is not called due to undefined return value. Consider logging a warning or handling this case explicitly to avoid silent failures.
| useEffect(() => { | ||
|
|
||
| tcUniNav( | ||
| getTcUniNav()?.( |
There was a problem hiding this comment.
[correctness]
The use of optional chaining with getTcUniNav()?.('update', ...) assumes that getTcUniNav might return undefined. Ensure that the rest of the code can handle the case where the function is not called due to undefined return value. Consider logging a warning or handling this case explicitly to avoid silent failures.
|
|
||
| tcUniNav( | ||
| getTcUniNav()?.( | ||
| 'update', |
There was a problem hiding this comment.
[correctness]
The use of optional chaining with getTcUniNav()?.('update', ...) assumes that getTcUniNav might return undefined. Ensure that the rest of the code can handle the case where the function is not called due to undefined return value. Consider logging a warning or handling this case explicitly to avoid silent failures.
| @@ -0,0 +1 @@ | |||
| export * from './other' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports and makes it harder to track what is being exported from this module. Consider explicitly exporting only the necessary components to improve maintainability and clarity.
| @@ -0,0 +1,15 @@ | |||
| import { TcUniNavFn } from 'universal-navigation' | |||
|
|
|||
| declare let tcUniNav: TcUniNavFn | |||
There was a problem hiding this comment.
[❗❗ correctness]
The use of declare let tcUniNav: TcUniNavFn implies that tcUniNav is expected to be defined globally elsewhere. Consider verifying that this assumption is valid and that tcUniNav is indeed available in all environments where this code will run. If not, it could lead to runtime errors.
| * @returns tcUniNav | ||
| */ | ||
| export function getTcUniNav(): TcUniNavFn | undefined { | ||
| if (typeof tcUniNav === 'undefined') { |
There was a problem hiding this comment.
[correctness]
Checking typeof tcUniNav === 'undefined' is a valid approach, but ensure that this check is sufficient for all cases where tcUniNav might not be initialized. Consider whether additional checks or initialization logic might be necessary.
| @@ -1,4 +1,4 @@ | |||
| import { EnvironmentConfig } from '~/config' | |||
| // import { EnvironmentConfig } from '~/config' | |||
There was a problem hiding this comment.
[maintainability]
The commented-out import statement suggests that EnvironmentConfig is no longer used in this file. If this is intentional and the import is not needed elsewhere, consider removing it entirely to avoid confusion and maintain cleaner code.
| } | ||
|
|
||
| export const CES_SURVEY_ID = EnvironmentConfig.USERFLOW_SURVEYS.PROFILES | ||
| // (removed) CES Survey/Userflow integrations |
There was a problem hiding this comment.
[❗❗ correctness]
The comment (removed) CES Survey/Userflow integrations indicates that the constant CES_SURVEY_ID was removed. Ensure that this removal does not affect any other parts of the codebase that might rely on this constant, as it could lead to runtime errors or unexpected behavior.
| if (userProfile) { | ||
| notifyUniNavi(userProfile) | ||
| triggerSurvey() | ||
| } |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of triggerSurvey() may impact functionality if surveys are intended to be triggered upon profile refresh. Ensure that this removal is intentional and does not affect any required business logic.
| memberEducation?.map((education: UserTrait) => ( | ||
| <EducationCard | ||
| key={`${education.schoolCollegeName}-${education.major}`} | ||
| key={`${education.collegeName}-${education.degree}`} |
There was a problem hiding this comment.
[❗❗ correctness]
The change from education.schoolCollegeName to education.collegeName and from education.major to education.degree in the key generation logic could potentially lead to duplicate keys if collegeName and degree are not unique identifiers. Ensure that these fields are unique or consider using a combination of fields that guarantee uniqueness.
| import { FC } from 'react' | ||
| import classNames from 'classnames' | ||
| import moment from 'moment' | ||
|
|
There was a problem hiding this comment.
[❗❗ correctness]
The import of moment has been removed, but it is not clear if this is intentional or if the functionality it provided is no longer needed. Ensure that the removal of moment does not affect any date formatting or calculations elsewhere in the code.
| </div> | ||
| { | ||
| props.education.timePeriodFrom || props.education.timePeriodTo ? ( | ||
| props.education.endYear ? ( |
There was a problem hiding this comment.
[❗❗ correctness]
The logic for displaying the education time period has been changed from checking timePeriodFrom or timePeriodTo to only checking endYear. Ensure that this change aligns with the intended functionality and that no important information is lost.
| traitId: UserTraitIds.education, | ||
| traits: { | ||
| data: memberEducation || [], | ||
| traitId: UserTraitIds.education, |
There was a problem hiding this comment.
[💡 maintainability]
The traitId property is being set twice in the traits object. This is redundant and could lead to confusion. Consider removing the duplicate assignment.
| break | ||
| } | ||
|
|
||
| const value = key === 'endYear' |
There was a problem hiding this comment.
[correctness]
The handleFormValueChange function now directly assigns Number(event.target.value) for the endYear key. Ensure that event.target.value is always a valid number or handle potential NaN cases to prevent unexpected behavior.
| }, 1000) | ||
| } | ||
|
|
||
| const locationDisplay: string = useMemo(() => { |
There was a problem hiding this comment.
[performance]
The use of useMemo here is appropriate for optimizing performance by memoizing the locationDisplay value. However, ensure that the dependencies [city, memberCountry] are correctly set to avoid unnecessary recomputation.
| ] | ||
| = useState<UserTrait[] | undefined>(props.workExpirence) | ||
|
|
||
| const industryOptions: any = sortBy(INDUSTRIES_OPTIONS) |
There was a problem hiding this comment.
[maintainability]
The use of any for industryOptions can lead to runtime errors and makes the code harder to maintain. Consider using a more specific type to ensure type safety.
| traitId: UserTraitIds.work, | ||
| traits: { | ||
| data: workExpirence || [], | ||
| traitId: UserTraitIds.work, |
There was a problem hiding this comment.
[💡 maintainability]
Duplicating traitId inside traits is unnecessary since it is already specified in the outer object. Consider removing the redundant traitId from traits.
| /> | ||
| workExpirence?.map((work: UserTrait, indx: number) => { | ||
| const companyName: string | undefined = work.company || work.companyName | ||
| const uniqueKey: string = [ |
There was a problem hiding this comment.
[correctness]
The uniqueKey generation logic could lead to duplicate keys if companyName, industry, position, and timePeriodFrom are not unique. Consider including a unique identifier like indx to ensure keys are truly unique.
| /> | ||
| )) | ||
| ? workExpirence?.map((work: UserTrait, index: number) => { | ||
| const companyName: string | undefined = work.company || work.companyName |
There was a problem hiding this comment.
[correctness]
Using work.company || work.companyName to determine companyName could lead to unexpected results if both properties exist but work.company is falsy. Consider explicitly checking for undefined or using a more deterministic approach.
|
|
||
| return ( | ||
| <WorkExpirenceCard | ||
| key={uniqueKey || `${work.position || 'experience'}-${index}`} |
There was a problem hiding this comment.
[❗❗ correctness]
The fallback key ${work.position || 'experience'}-${index} could lead to duplicate keys if work.position is falsy for multiple items. Consider ensuring uniqueness by incorporating more attributes or using a UUID.
| import moment from 'moment' | ||
|
|
||
| import { UserTrait } from '~/libs/core' | ||
| import { getIndustryOptionLabel } from '~/libs/shared' |
There was a problem hiding this comment.
[correctness]
Ensure that getIndustryOptionLabel handles all possible values of industry gracefully, especially if industry can be undefined or null. Consider adding validation or default handling if necessary.
| props.work.timePeriodFrom || props.work.timePeriodTo || props.work.working ? ( | ||
| <div className={styles.workExpirenceCardHeaderRight}> | ||
| const formatDateRange = (work: UserTrait): string | undefined => { | ||
| const periodFrom: string | Date | undefined = work.timePeriodFrom || work.startDate |
There was a problem hiding this comment.
[correctness]
The use of work.startDate and work.endDate as fallbacks for work.timePeriodFrom and work.timePeriodTo should be verified to ensure that these fields are always available and correctly formatted. If these fields are not guaranteed to be present, consider adding validation or default values.
| return formattedFrom || formattedTo || undefined | ||
| } | ||
|
|
||
| const WorkExpirenceCard: FC<WorkExpirenceCardProps> = (props: WorkExpirenceCardProps) => { |
There was a problem hiding this comment.
[💡 maintainability]
Consider renaming WorkExpirenceCard to WorkExperienceCard to correct the spelling mistake in the component name. This will improve readability and maintainability.
| @@ -0,0 +1 @@ | |||
| export * from './src' | |||
There was a problem hiding this comment.
[maintainability]
Using export * can lead to unintended exports if new modules are added to the src directory. Consider explicitly exporting only the necessary modules to improve maintainability and prevent accidental exposure of internal modules.
|
|
||
| const ReviewApp: FC = () => { | ||
| const { getChildRoutes }: RouterContextData = useContext(routerContext) | ||
| const childRoutes = useMemo(() => getChildRoutes(toolTitle), [getChildRoutes]) |
There was a problem hiding this comment.
[💡 performance]
Using useMemo here might be unnecessary unless getChildRoutes is computationally expensive or the component re-renders frequently. Consider removing useMemo if performance is not a concern, as it adds complexity.
| <SWRConfigProvider> | ||
| <Layout> | ||
| <Outlet /> | ||
| <Routes>{childRoutes}</Routes> |
There was a problem hiding this comment.
[❗❗ correctness]
The Routes component should typically be used with Route components as children. Ensure that childRoutes returns valid Route elements to avoid runtime errors.
| } | ||
| export const TABLE_DATE_FORMAT = 'MMM DD YYYY, HH:mm A' | ||
| export const TABLE_PAGINATION_ITEM_PER_PAGE = 100 | ||
| export const THRESHOLD_SHORT_TIME = 2 * 60 * 60 * 1000 // in miliseconds |
There was a problem hiding this comment.
[💡 style]
There is a typo in the comment: 'miliseconds' should be 'milliseconds'.
| export const TABLE_PAGINATION_ITEM_PER_PAGE = 100 | ||
| export const THRESHOLD_SHORT_TIME = 2 * 60 * 60 * 1000 // in miliseconds | ||
|
|
||
| export const ORDINAL_SUFFIX = new Map([[1, '1st'], [2, '2nd'], [3, '3rd']]) |
There was a problem hiding this comment.
[correctness]
The ORDINAL_SUFFIX map only includes mappings for 1, 2, and 3. Consider adding mappings for other numbers or clarifying its intended use to avoid potential misuse.
| */ | ||
| import { AppSubdomain, EnvironmentConfig } from '~/config' | ||
|
|
||
| export const rootRoute: string |
There was a problem hiding this comment.
[💡 readability]
Consider using a more descriptive variable name for rootRoute to clarify its purpose, such as reviewRootRoute. This can improve readability and maintainability by making the code's intent clearer.
|
|
||
| const onSubmit = useCallback( | ||
| (data: FormAddDefaultReviewer) => { | ||
| const requestBody = _.omitBy( |
There was a problem hiding this comment.
[correctness]
Using _.omitBy to filter out undefined, null, and empty string values is a change from _.pickBy with _.identity. This change is technically correct but could potentially alter the behavior if the previous implementation relied on truthy values other than these. Ensure that this change aligns with the intended logic and that no unintended data is being omitted.
| ...rest | ||
| }: BasicSelectProps = props | ||
|
|
||
| const normalizedValue = value === null || value === undefined ? '' : String(value) |
There was a problem hiding this comment.
[correctness]
Converting value to a string using String(value) may lead to unexpected results if value is an object or a symbol. Consider ensuring value is always a primitive type before this conversion.
| </option> | ||
| {options.map(option => ( | ||
| <option | ||
| key={String(option.value ?? option.label)} |
There was a problem hiding this comment.
[❗❗ correctness]
Using String(option.value ?? option.label) for the key prop can lead to duplicate keys if option.value and option.label are not unique across options. Consider ensuring uniqueness of keys to avoid rendering issues.
| mapValue: (value: string | number | boolean | '') => { | ||
| const stringValue = typeof value === 'string' | ||
| ? value | ||
| : String(value ?? '') |
There was a problem hiding this comment.
[💡 performance]
The use of String(value ?? '') ensures that non-string values are converted to strings, but it might be more efficient to handle null and undefined values explicitly before conversion. Consider checking for these cases separately to avoid unnecessary string conversion.
| const stringValue = typeof value === 'string' | ||
| ? value | ||
| : String(value ?? '') | ||
| return `${stringValue.toLowerCase()}${ |
There was a problem hiding this comment.
[correctness]
The use of toLowerCase() on stringValue assumes that stringValue is always a string. Ensure that value is validated or sanitized before this operation to avoid potential runtime errors.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?