Conversation
Further wording tweaks requested
…eteness PM-3357 - profile completeness messaging
…al-score For approvers show final score if available
…eteness PM-3357 - make sure to filter out profileCompleteness categories
PM-3397 Add email to profiles UI
feat(PM-3398): added description and associated skills field in Add/Update Work experience modal, work card in Profile Page
feat(PM-3319): added download button in profile page
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| LeaveStatus.LEAVE, | ||
| LeaveStatus.HOLIDAY, | ||
| LeaveStatus.WIPRO_HOLIDAY, | ||
| LeaveStatus.WEEKEND, |
There was a problem hiding this comment.
[❗❗ correctness]
Removing LeaveStatus.WIPRO_HOLIDAY from legendStatusOrder may affect any functionality that relies on the order of statuses. Ensure that this change is intentional and does not break any dependent logic.
| } | ||
|
|
||
| // Check if user has admin roles | ||
| if (authProfile.roles?.some(role => ADMIN_ROLES.includes(role.toLowerCase() as UserRole))) { |
There was a problem hiding this comment.
[correctness]
The use of toLowerCase() on roles might lead to unexpected behavior if ADMIN_ROLES contains roles with mixed casing. Consider ensuring ADMIN_ROLES is consistently cased or adjust the logic to handle casing more robustly.
| const allowedRoles = ['Project Manager', 'Talent Manager'] | ||
| if (authProfile | ||
| .roles?.some( | ||
| role => allowedRoles.some(allowed => role.toLowerCase() === allowed.toLowerCase()), |
There was a problem hiding this comment.
[💡 performance]
The allowedRoles array is defined within the function and used only once. Consider moving it outside the function to avoid redefining it on each function call, which can slightly improve performance.
| import classNames from 'classnames' | ||
|
|
||
| import { NamesAndHandleAppearance, useMemberTraits, UserProfile, UserTraitIds, UserTraits } from '~/libs/core' | ||
| import { CopyButton } from '~/apps/admin/src/lib/components/CopyButton' |
There was a problem hiding this comment.
[maintainability]
The CopyButton component is imported from the admin app's library. Ensure that this component is intended to be shared across different apps and that there are no unintended dependencies or side effects. Consider moving shared components to a common library if they are used across multiple apps.
| ) | ||
| } | ||
|
|
||
| {props.profile?.email |
There was a problem hiding this comment.
[❗❗ security]
Displaying the user's email address directly in the UI may have privacy implications. Ensure that displaying the email is compliant with privacy policies and consider adding user consent or a privacy notice if necessary.
| .downloadButtonWrap { | ||
| position: absolute; | ||
| top: $sp-4; | ||
| right: calc((100% - #{$xxl-min}) / 2); |
There was a problem hiding this comment.
[correctness]
The calculation for right using calc((100% - #{$xxl-min}) / 2) may lead to unexpected layout issues if $xxl-min is larger than the container width. Consider verifying the value of $xxl-min or using a different approach to ensure consistent alignment.
| right: $sp-4; | ||
| } | ||
|
|
||
| @include ltelg { |
There was a problem hiding this comment.
[💡 maintainability]
The @include ltelg block redefines several properties that are already set outside of this block. This could lead to redundancy and potential maintenance issues. Consider consolidating these styles to avoid duplication.
| setIsDownloading(true) | ||
| try { | ||
| await downloadProfileAsync(props.profile.handle) | ||
| } catch (error) {} finally { |
There was a problem hiding this comment.
[maintainability]
Consider logging the error in the catch block to aid in debugging and monitoring potential issues during the profile download process.
|
|
||
| setIsDownloading(true) | ||
| try { | ||
| await downloadProfileAsync(props.profile.handle) |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that downloadProfileAsync handles all potential errors, such as network failures or invalid responses, to prevent unhandled promise rejections.
|
|
||
| useEffect(() => { completeness?.mutate() }, [props.profile]) | ||
|
|
||
| const [count, incompleteEntries] = useMemo(() => { |
There was a problem hiding this comment.
[💡 performance]
Using useMemo here is unnecessary unless the computation of fields is expensive or the component re-renders frequently. Consider removing useMemo if performance is not a concern, as it adds complexity without clear benefit.
| useEffect(() => { completeness?.mutate() }, [props.profile]) | ||
|
|
||
| const [count, incompleteEntries] = useMemo(() => { | ||
| const fields = Object.entries(completeness.entries) |
There was a problem hiding this comment.
[💡 maintainability]
The use of Object.entries followed by filtering and mapping could be simplified if completeness.entries is guaranteed to be an object with known keys. Consider using a more direct approach if possible.
| const [formValues, setFormValues]: [ | ||
| { [key: string]: string | boolean | Date | undefined }, | ||
| Dispatch<SetStateAction<{ [key: string]: string | boolean | Date | undefined }>> | ||
| { [key: string]: string | boolean | Date | any[] | undefined }, |
There was a problem hiding this comment.
[maintainability]
Using any[] in the type definition for formValues can lead to potential type safety issues. Consider defining a more specific type for the array elements to improve type safety and maintainability.
| }) | ||
| } | ||
|
|
||
| function handleSkillsChange(event: ChangeEvent<HTMLInputElement>): void { |
There was a problem hiding this comment.
[correctness]
The handleSkillsChange function uses any for the event target value. Consider using a more specific type to improve type safety and prevent potential runtime errors.
|
|
||
| const updatedWorkExpirence: UserTrait = { | ||
| cityTown: formValues.city, | ||
| associatedSkills: (formValues.associatedSkills as any[])?.map((s: any) => s.id || s) || [], |
There was a problem hiding this comment.
[maintainability]
The use of any[] for associatedSkills in updatedWorkExpirence could lead to type safety issues. Consider defining a more specific type for skill objects to ensure consistency and prevent errors.
|
|
||
| setEditedItemIndex(indx) | ||
|
|
||
| let associatedSkills: any[] = [] |
There was a problem hiding this comment.
[maintainability]
The associatedSkills array is typed as any[]. Consider defining a specific type for skill objects to improve type safety and maintainability.
| = useMemo(() => memberWorkExpirenceTraits?.[0]?.traits?.data, [memberWorkExpirenceTraits]) | ||
|
|
||
| // Collect all unique skill IDs from work experience entries | ||
| const allSkillIds = useMemo(() => { |
There was a problem hiding this comment.
[💡 maintainability]
The use of useMemo here is appropriate for memoizing the list of skill IDs. However, consider adding a type annotation to the returned array for better type safety and clarity.
| return Array.from(skillIdsSet) | ||
| }, [workExpirence]) | ||
|
|
||
| const { data: fetchedSkills, error: skillsError }: SWRResponse<UserSkill[], Error> = useSkillsByIds( |
There was a problem hiding this comment.
[correctness]
The useSkillsByIds hook is called with undefined if allSkillIds is empty. Ensure that useSkillsByIds can handle undefined gracefully without causing errors.
| return map | ||
| }, [fetchedSkills, allSkillIds]) | ||
|
|
||
| const areSkillsLoaded = useCallback((work: UserTrait): boolean => { |
There was a problem hiding this comment.
[💡 performance]
The useCallback hook is used to memoize the areSkillsLoaded function. Ensure that this function is not being re-created unnecessarily by verifying that skillNamesMap is stable and does not change frequently.
| <WorkExpirenceCard | ||
| key={uniqueKey || `${work.position || 'experience'}-${index}`} | ||
| work={work} | ||
| skillNamesMap={skillNamesMap} |
There was a problem hiding this comment.
[performance]
Passing skillNamesMap as a prop to WorkExpirenceCard could lead to unnecessary re-renders if skillNamesMap changes frequently. Ensure that skillNamesMap is memoized properly to prevent performance issues.
| align-items: flex-end; | ||
| } | ||
|
|
||
| .workExpirenceCardDescription { |
There was a problem hiding this comment.
[maintainability]
The class name .workExpirenceCardDescription contains a typo. It should be .workExperienceCardDescription to maintain consistency and avoid confusion.
| } | ||
| } | ||
|
|
||
| .workExpirenceCardSkills { |
There was a problem hiding this comment.
[maintainability]
The class name .workExpirenceCardSkills contains a typo. It should be .workExperienceCardSkills to maintain consistency and avoid confusion.
| @@ -1,3 +1,5 @@ | |||
| /* eslint-disable complexity */ | |||
There was a problem hiding this comment.
[maintainability]
Disabling complexity checks can hide potential issues in the code. Consider refactoring the component to reduce complexity instead of disabling the rule.
| <div className={styles.workExpirenceCardSkills}> | ||
| <p className='body-main-small-bold'>Skills:</p> | ||
| <div className={styles.skillsList}> | ||
| {props.work.associatedSkills.map((skillId: string) => { |
There was a problem hiding this comment.
[❗❗ correctness]
The skillNamesMap is accessed without checking if it is defined, which could lead to runtime errors if skillNamesMap is undefined. Consider using optional chaining or a default value to ensure safety.
| } | ||
|
|
||
| return <span>{rawScoreDisplay}</span> | ||
| return <span>{rawScoreDisplay || aggregateScore}</span> |
There was a problem hiding this comment.
[correctness]
Using rawScoreDisplay || aggregateScore could lead to unexpected behavior if rawScoreDisplay is a falsy value like 0. Consider explicitly checking for undefined or null to ensure the correct value is displayed.
|
|
||
| const percentComplete = data?.data?.percentComplete | ||
| return { | ||
| entries: omit(data?.data ?? {}, 'percentComplete'), |
There was a problem hiding this comment.
[performance]
The use of omit from lodash is generally fine, but consider the potential performance impact if data?.data contains a large number of keys. If performance becomes an issue, you might want to explore more efficient ways to handle this.
| link.click() | ||
| link.parentNode?.removeChild(link) | ||
| } catch (error) { | ||
| console.error('Failed to download profile:', error) |
There was a problem hiding this comment.
[maintainability]
Consider providing more detailed error handling or logging for the downloadProfileAsync function. Currently, the catch block only logs a generic error message, which might not be sufficient for troubleshooting specific issues.
| link.setAttribute('download', `profile-${handle}.pdf`) | ||
| document.body.appendChild(link) | ||
| link.click() | ||
| link.parentNode?.removeChild(link) |
There was a problem hiding this comment.
[correctness]
Ensure that the link element is always removed from the DOM, even if an error occurs during the link.click() operation. Consider moving the link.parentNode?.removeChild(link) line into a finally block to guarantee cleanup.
| } | ||
|
|
||
| try { | ||
| const skillPromises = skillIds.map(skillId => xhrGetAsync<UserSkill>(`${baseUrl}/skills/${skillId}`) |
There was a problem hiding this comment.
[maintainability]
Consider handling specific errors instead of catching all errors with a generic catch. This will help in diagnosing issues more effectively and maintaining the code.
|
|
||
| return useSWR<UserSkill[], Error>( | ||
| swrKey, | ||
| () => fetchSkillsByIdsFetcher(skillIds!), |
There was a problem hiding this comment.
[❗❗ correctness]
Using the non-null assertion operator (!) on skillIds assumes that swrKey will never be undefined. Ensure that this assumption is valid or handle the case where skillIds might be undefined to avoid potential runtime errors.
5 participants
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/
What's in this PR?