Conversation
…ed-skills PM-3717 - enhance the data returned for user skill
PM-3785 - safe check for challenge task
Member skill: return "Winner" position for earned skill
fix(PM-3733): Line spacing issues in Profile PDF
fix(PM-3732): description number list line spacing
Account for multiple roles in member skill
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| if (first.skill && first.skill.skillEvents) { | ||
| if (first.skill && first.skill.skillEvents?.length) { |
There was a problem hiding this comment.
[💡 maintainability]
The use of optional chaining (?.) in first.skill.skillEvents?.length is a good improvement for safety, but the subsequent _.orderBy(first.skill.skillEvents || [], 'createdAt', 'desc') still uses a fallback to an empty array. Consider removing the fallback since the optional chaining already ensures skillEvents is either defined or undefined. This will make the code cleaner and avoid unnecessary operations.
| member.statusBarText | ||
| ) | ||
| ) | ||
| : React.createElement(View, { style: styles.statusBarSeparator }) |
There was a problem hiding this comment.
[design]
The addition of a statusBarSeparator when member.statusBarText is not present changes the layout logic. Ensure this behavior is intended, as it introduces a visual element that was not previously displayed.
| select: { id: true, name: true } | ||
| }).then(dbChallenges => { | ||
| Promise.all([ | ||
| resourcesPrisma.resource.findMany({ |
There was a problem hiding this comment.
[maintainability]
Using Promise.all here is appropriate for parallel execution, but be aware that if any promise rejects, the entire operation will fail. Consider adding error handling to manage individual promise failures gracefully.
| // For each role: sort by endDate desc, keep last 3, include total count | ||
| skill.activity.challenge = Object.fromEntries( | ||
| Object.entries(groups).map(([role, challenges]) => { | ||
| const sorted = challenges.sort((a, b) => |
There was a problem hiding this comment.
[correctness]
Sorting by endDate assumes that all challenges have a valid endDate. If any challenge lacks an endDate, this could lead to unexpected behavior. Consider handling cases where endDate might be null or undefined.
| new Date(b.endDate || 0) - new Date(a.endDate || 0) | ||
| ) | ||
| return [role, { | ||
| count: sorted.length, |
There was a problem hiding this comment.
[💡 design]
The use of slice(0, 3) limits the results to the last 3 challenges. Ensure that this behavior is intentional and documented, as it might not be obvious to all developers why only 3 are kept.
| // industry is optional; treat blank values as omitted | ||
| omitBlankStringField(t, 'industry') | ||
| omitBlankStringField(t, 'otherIndustry') | ||
| if (t.industry !== 'Other') { |
There was a problem hiding this comment.
[correctness]
The logic here assumes that t.industry is always defined. If t.industry is undefined, this condition will not delete t.otherIndustry even if it should be. Consider explicitly checking for undefined or using a more robust condition.
| should.equal(traits[0].traitId, 'work') | ||
| should.equal(traits[0].traits.data.length, 1) | ||
| should.equal(traits[0].traits.data[0].companyName, 'JP Morgan 3') | ||
| should.not.equal(traits[0].traits.data[0].industry, '') |
There was a problem hiding this comment.
[correctness]
The assertion should.not.equal(traits[0].traits.data[0].industry, '') checks that the industry field is not an empty string, but it doesn't verify if the field is properly handled when it contains only whitespace. Consider adding a check to ensure that the industry field is either trimmed or validated to prevent storing invalid data.
4 participants
https://topcoder.atlassian.net/browse/PM-3717
https://topcoder.atlassian.net/browse/PM-2651