feat(PM-3628): implement work experience changes in onboarding #1440
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| export const emptyWorkInfo: () => WorkInfo = () => ({ | ||
| company: '', | ||
| companyName: '', |
There was a problem hiding this comment.
[💡 maintainability]
The property companyName is initialized as an empty string, which is inconsistent with other optional properties initialized as undefined. Consider initializing it as undefined for consistency and to clearly indicate the absence of a value.
| position: '', | ||
| startDate: undefined, | ||
| city: undefined, | ||
| associatedSkills: undefined, |
There was a problem hiding this comment.
[correctness]
The property associatedSkills is initialized as undefined, which may lead to issues if the code expects an array. Consider initializing it as an empty array [] to avoid potential runtime errors when iterating over it.
| associatedSkills: work.associatedSkills, | ||
| cityName: work.city, | ||
| company: work.companyName, | ||
| companyName: work.companyName, |
There was a problem hiding this comment.
[💡 maintainability]
The companyName field is being set twice in the workInfoToUserTrait function. This is redundant and could lead to confusion. Consider removing one of the assignments.
| function userTraitToWorkInfo(trait: UserTrait, id: number): WorkInfo { | ||
| return { | ||
| id, | ||
| associatedSkills: Array.isArray(trait.associatedSkills) ? trait.associatedSkills : undefined, |
There was a problem hiding this comment.
[correctness]
The associatedSkills field is being set to undefined if it's not an array. This could lead to loss of data if trait.associatedSkills is mistakenly not an array. Consider logging a warning or handling this case more explicitly.
| return { | ||
| id, | ||
| associatedSkills: Array.isArray(trait.associatedSkills) ? trait.associatedSkills : undefined, | ||
| city: trait.cityName || trait.cityTown || trait.city, |
There was a problem hiding this comment.
[correctness]
The city field is being set using a fallback chain of trait.cityName || trait.cityTown || trait.city. Ensure that this fallback logic aligns with the intended data model and that it doesn't mask potential data issues.
| companyName: trait.company || trait.companyName, | ||
| currentlyWorking: trait.working, | ||
| description: trait.description, | ||
| endDate: trait.timePeriodTo ? new Date(trait.timePeriodTo) : (trait.endDate ? new Date(trait.endDate) : undefined), |
There was a problem hiding this comment.
[correctness]
The endDate is being parsed using new Date(), which can lead to inconsistencies if the input string is not in a standard format. Consider using a library like date-fns or moment for more reliable date parsing.
| industry: trait.industry, | ||
| otherIndustry: trait.otherIndustry, | ||
| position: trait.position, | ||
| startDate: trait.timePeriodFrom ? new Date(trait.timePeriodFrom) : (trait.startDate ? new Date(trait.startDate) : undefined), |
There was a problem hiding this comment.
[correctness]
The startDate is being parsed using new Date(), similar to endDate. Ensure that the date format is consistent and consider using a library for parsing to avoid potential issues.
| return Array.from(ids) | ||
| }, [works]) | ||
|
|
||
| const { data: fetchedSkills } = useSkillsByIds(allSkillIds.length > 0 ? allSkillIds : undefined) |
There was a problem hiding this comment.
[💡 performance]
The useSkillsByIds hook is being called with undefined if allSkillIds is empty. Ensure that the hook handles this case correctly and doesn't result in unnecessary API calls.
| const works: WorkInfo[] = workExpValue.map((j: any, index: number) => { | ||
| const startDate: Date | undefined = dateTimeToDate(j.startDate) | ||
| const endDate: Date | undefined = dateTimeToDate(j.endDate) | ||
| const startDate: Date | undefined = dateTimeToDate(j.startDate || j.timePeriodFrom) |
There was a problem hiding this comment.
[correctness]
The fallback to j.timePeriodFrom and j.timePeriodTo for startDate and endDate is a good addition. However, ensure that dateTimeToDate can handle both formats correctly. If these fields can be in different formats, consider adding validation or logging to catch any unexpected values.
| const endDate: Date | undefined = dateTimeToDate(j.endDate || j.timePeriodTo) | ||
| return ({ | ||
| companyName: j.companyName, | ||
| associatedSkills: Array.isArray(j.associatedSkills) ? j.associatedSkills : undefined, |
There was a problem hiding this comment.
[💡 correctness]
The use of Array.isArray(j.associatedSkills) is a good check. However, if associatedSkills can be an empty array, consider whether undefined is the desired fallback. If not, you might want to default to an empty array instead.
| return ({ | ||
| companyName: j.companyName, | ||
| associatedSkills: Array.isArray(j.associatedSkills) ? j.associatedSkills : undefined, | ||
| city: j.cityName || j.cityTown || j.city, |
There was a problem hiding this comment.
[💡 maintainability]
The logic for determining the city value is potentially fragile if j.cityName, j.cityTown, and j.city can all be present. Consider documenting or ensuring the precedence order is correct and consistent with business logic.
| companyName: company || '', | ||
| associatedSkills: Array.isArray(associatedSkills) ? associatedSkills : undefined, | ||
| cityName: city, | ||
| companyName: companyName || '', |
There was a problem hiding this comment.
[correctness]
Ensure that cityName is the correct field to use when creating the payload. If there are multiple city fields, confirm that this is the intended one for consistency.
| associatedSkills: Array.isArray(associatedSkills) ? associatedSkills : undefined, | ||
| cityName: city, | ||
| companyName: companyName || '', | ||
| description: description || undefined, |
There was a problem hiding this comment.
[💡 correctness]
The fallback to undefined for description is acceptable if the API expects it. However, if the API can handle an empty string or another default value more gracefully, consider using that instead.
| import { WorkExpirenceCard } from '../WorkExpirenceCard' | ||
| import { fetchSkillsByIds } from '~/libs/shared/lib/services/standard-skills' | ||
|
|
||
| import styles from './ModifyWorkExpirenceModal.module.scss' |
There was a problem hiding this comment.
[❗❗ correctness]
The import path for styles is incorrect due to the typo in ModifyWorkExpirenceModal.module.scss. It should be ModifyWorkExperienceModal.module.scss to match the corrected spelling of 'Experience'.
| @@ -37,20 +35,6 @@ const ModifyWorkExpirenceModal: FC<ModifyWorkExpirenceModalProps> = (props: Modi | |||
| const [addingNewItem, setAddingNewItem]: [boolean, Dispatch<SetStateAction<boolean>>] | |||
There was a problem hiding this comment.
[💡 style]
The variable addingNewItem is initialized with a condition that includes || false, which is redundant. The expression props.workExpirence?.length === 0 already evaluates to a boolean.
| const industryOptions: any = getIndustryOptionsWithOthersLast(INDUSTRIES_OPTIONS) | ||
|
|
||
| function handleModifyWorkExpirenceSave(): void { | ||
| if (addingNewItem || editedItemIndex !== undefined) { |
There was a problem hiding this comment.
[maintainability]
The handleModifyWorkExpirenceSave function calls formRef.current?.submit() without checking if formRef.current is non-null. While the optional chaining operator prevents runtime errors, consider adding a more explicit check or error handling to ensure the form reference is correctly initialized.
| className={styles.ctaBtn} | ||
| icon={IconOutline.PencilIcon} | ||
| onClick={bind(handleWorkExpirenceEdit, this, indx)} | ||
| onClick={bind(handleWorkExpirenceEdit, null, indx)} |
There was a problem hiding this comment.
[💡 readability]
The bind function is used with null as the context for handleWorkExpirenceEdit. If the function does not rely on this, consider using an arrow function instead for clarity and consistency.
| className={styles.ctaBtn} | ||
| icon={IconOutline.TrashIcon} | ||
| onClick={bind(handleWorkExpirenceDelete, this, indx)} | ||
| onClick={bind(handleWorkExpirenceDelete, null, indx)} |
There was a problem hiding this comment.
[💡 readability]
The bind function is used with null as the context for handleWorkExpirenceDelete. If the function does not rely on this, consider using an arrow function instead for clarity and consistency.
| import { SWRResponse } from 'swr' | ||
|
|
||
| import { MemberTraitsAPI, useMemberTraits, UserProfile, UserSkill, UserTrait, UserTraitIds } from '~/libs/core' | ||
| import { WorkExperienceCard } from '~/libs/shared' |
There was a problem hiding this comment.
[maintainability]
The import path for WorkExperienceCard has been changed from a local path to a shared library path. Ensure that the shared component is compatible with this module's requirements and that any necessary adjustments have been made to accommodate the shared component's API.
| @@ -0,0 +1,31 @@ | |||
| @import '@libs/ui/styles/includes'; | |||
There was a problem hiding this comment.
[performance]
Consider using a more specific import path if possible to avoid potential issues with importing unnecessary styles, which can impact performance and maintainability.
| align-items: center; | ||
| flex-wrap: wrap; | ||
|
|
||
| :global(.input-wrapper) { |
There was a problem hiding this comment.
[maintainability]
Using :global can lead to unintended side effects by affecting styles outside of this component. Ensure this is necessary and consider alternatives if possible.
| submit: () => void | ||
| } | ||
|
|
||
| const industryOptions: any = getIndustryOptionsWithOthersLast(INDUSTRIES_OPTIONS) |
There was a problem hiding this comment.
[maintainability]
Avoid using any for industryOptions. Consider defining a more specific type to improve type safety and maintainability.
|
|
||
| const AddEditWorkExperienceFormInner: ForwardRefRenderFunction<AddEditWorkExperienceFormRef, AddEditWorkExperienceFormProps> = (props, ref) => { | ||
| const [formValues, setFormValues] = useState<{ | ||
| [key: string]: string | boolean | Date | any[] | undefined |
There was a problem hiding this comment.
[maintainability]
The use of any in the type definition for formValues can lead to potential type safety issues. Consider defining a more specific type for the form values.
| : undefined | ||
|
|
||
| const work: UserTrait = { | ||
| associatedSkills: (formValues.associatedSkills as any[])?.map((s: any) => s.id || s) || [], |
There was a problem hiding this comment.
[maintainability]
The use of any in associatedSkills can lead to potential type safety issues. Consider defining a more specific type for skills.
| setFormValues(prev => ({ ...prev, description: value })) | ||
| } | ||
|
|
||
| function handleSkillsChange(event: ChangeEvent<HTMLInputElement>): void { |
There was a problem hiding this comment.
[maintainability]
The handleSkillsChange function uses any for selectedSkills. Consider defining a more specific type for skills to improve type safety.
| })) | ||
| } | ||
|
|
||
| function doSubmit(): void { |
There was a problem hiding this comment.
[correctness]
Consider adding validation for associatedSkills to ensure that the skills have both id and name properties. This will prevent potential issues when saving or displaying skills.
| const title = props.initialWork ? 'Edit Experience' : 'Add Experience' | ||
|
|
||
| function handleSaveClick(): void { | ||
| formRef.current?.submit() |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for the submit method on formRef.current. If the form submission fails, it might be useful to provide feedback to the user or log the error for debugging purposes.
| initialWork={props.initialWork} | ||
| onSave={work => { | ||
| props.onSave(work) | ||
| props.onClose() |
There was a problem hiding this comment.
[correctness]
The onClose function is called immediately after onSave. If onSave involves asynchronous operations, consider awaiting its completion before closing the modal to ensure that any potential errors or state updates are handled before the modal is dismissed.
| const WorkExperienceCard: FC<WorkExperienceCardProps> = (props: WorkExperienceCardProps) => { | ||
| const companyName: string | undefined = props.work.company || props.work.companyName | ||
| const city: string | undefined = props.work.cityTown || props.work.city | ||
| const city: string | undefined = props.work.cityName || props.work.cityTown || props.work.city |
There was a problem hiding this comment.
[correctness]
The change from props.work.cityTown || props.work.city to props.work.cityName || props.work.cityTown || props.work.city introduces a new property cityName. Ensure that cityName is consistently available in the UserTrait type and that it does not conflict with existing data structure assumptions.
|
|
||
| export const emptyWorkInfo: () => WorkInfo = () => ({ | ||
| company: '', | ||
| associatedSkills: undefined, |
There was a problem hiding this comment.
[correctness]
The associatedSkills and city properties were moved to the top of the emptyWorkInfo function. Ensure this change is intentional and does not affect any logic that depends on the order of properties, especially if serialization or deserialization is involved.
| companyName: trait.company || trait.companyName, | ||
| currentlyWorking: trait.working, | ||
| description: trait.description, | ||
| endDate: trait.timePeriodTo |
There was a problem hiding this comment.
[💡 readability]
The logic for determining endDate could be simplified by using a single conditional statement to check for trait.timePeriodTo and trait.endDate. This would improve readability and reduce potential errors.
| industry: trait.industry, | ||
| otherIndustry: trait.otherIndustry, | ||
| position: trait.position, | ||
| startDate: trait.timePeriodFrom |
There was a problem hiding this comment.
[💡 readability]
The logic for determining startDate could be simplified by using a single conditional statement to check for trait.timePeriodFrom and trait.startDate. This would improve readability and reduce potential errors.
| return Array.from(ids) | ||
| }, [works]) | ||
|
|
||
| const { data: fetchedSkills }: SWRResponse<UserSkill[], Error> = useSkillsByIds( |
There was a problem hiding this comment.
[💡 style]
The SWRResponse type annotation is redundant here as TypeScript can infer the type from useSkillsByIds. Consider removing it to simplify the code.
| className={styles.ctaBtn} | ||
| icon={IconOutline.PencilIcon} | ||
| onClick={bind(handleWorkExpirenceEdit, this, indx)} | ||
| onClick={bind(handleWorkExpirenceEdit, undefined, indx)} |
There was a problem hiding this comment.
[correctness]
The change from null to undefined in the bind function call is appropriate if the function expects undefined for optional parameters. Ensure that the handleWorkExpirenceEdit function correctly handles undefined values.
| className={styles.ctaBtn} | ||
| icon={IconOutline.TrashIcon} | ||
| onClick={bind(handleWorkExpirenceDelete, this, indx)} | ||
| onClick={bind(handleWorkExpirenceDelete, undefined, indx)} |
There was a problem hiding this comment.
[correctness]
Similarly, ensure that the handleWorkExpirenceDelete function correctly handles undefined values as the change from null to undefined might affect the logic if not properly managed.
| submit: () => void | ||
| } | ||
|
|
||
| const industryOptions: any = getIndustryOptionsWithOthersLast(INDUSTRIES_OPTIONS) |
There was a problem hiding this comment.
[maintainability]
Using any for industryOptions can lead to runtime errors and makes the code harder to maintain. Consider defining a specific type for industry options to improve type safety.
| if (props.initialWork) { | ||
| const work = props.initialWork | ||
| const baseValues = { | ||
| associatedSkills: [] as any[], |
There was a problem hiding this comment.
[maintainability]
The type any[] for associatedSkills is too generic and can lead to runtime errors. Consider using a more specific type to improve type safety and maintainability.
| if (value) { | ||
| oldFormValues.endDate = undefined | ||
| } | ||
|
|
There was a problem hiding this comment.
[💡 style]
Unnecessary blank line. Consider removing it to keep the code concise and improve readability.
| if (value !== 'Other') { | ||
| oldFormValues.otherIndustry = undefined | ||
| } | ||
|
|
There was a problem hiding this comment.
[💡 style]
Unnecessary blank line. Consider removing it to keep the code concise and improve readability.
| setFormErrors(prev => ({ ...prev, startDate: 'Start date is required when end date is given' })) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
[💡 style]
Unnecessary blank line. Consider removing it to keep the code concise and improve readability.
| <form | ||
| ref={formElRef} | ||
| className={styles.formWrap} | ||
| onSubmit={function (event: React.FormEvent<HTMLFormElement>): void { |
There was a problem hiding this comment.
[💡 style]
The change from an arrow function to a named function for onSubmit is unnecessary unless there is a specific reason for this change. Consider reverting to maintain consistency with other event handlers.
| <AddEditWorkExperienceForm | ||
| ref={formRef} | ||
| initialWork={props.initialWork} | ||
| onSave={function (work: UserTrait): void { |
There was a problem hiding this comment.
[💡 style]
The onSave function is defined inline with a function keyword. Consider using an arrow function for consistency with modern React patterns and to avoid potential issues with this binding.
| onSave={function (work: UserTrait): void { | ||
| props.onSave(work) | ||
| props.onClose() | ||
| }} |
There was a problem hiding this comment.
[correctness]
The isSaving prop is removed from AddEditWorkExperienceForm, but it is still used to disable the 'Save' button. Ensure that this change does not affect the form's behavior, especially if the form needs to reflect the saving state.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3628
What's in this PR?