Conversation
…tooltip-info PM-3717 - group challenge events by skill event type
fix(PM-3733): Reduce space between courses and certifications
…tooltip-info PM-3717 - Convert 2nd place & 3rd placements to wins
…tooltip-info PM-3717- Debug and fix db connection for engagements prisma client
feat(PM-3734): Topcoder stats implementation in activity section
… member as a resource (PS-526)
…t resource on when accessing emails, to avoid sending emails to users with copilot role that may be a submitter or reviewer on a challenge.
…profile sensitive data
…privacy PM-3825 make sure we properly restrict visibility over user profile sensitive data
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| */ | ||
| const ADMIN_ROLES = ['administrator', 'admin'] | ||
| const PROFILE_DOWNLOAD_ROLES = ['project manager', 'Talent Manager'] | ||
| const SENSITIVE_DATA_ROLES = [...ADMIN_ROLES, 'Talent Manager'] |
There was a problem hiding this comment.
[maintainability]
The change from PROFILE_DOWNLOAD_ROLES to SENSITIVE_DATA_ROLES suggests a broader scope for this constant. Ensure that all usages of PROFILE_DOWNLOAD_ROLES are reviewed to confirm they align with the new intent of handling sensitive data access.
| .map(field => field.trim()) | ||
| .filter(field => field.length > 0) | ||
|
|
||
| if (!communicationSecureFields.includes('email')) { |
There was a problem hiding this comment.
[performance]
The logic to ensure 'email' is included in communicationSecureFields is correct, but it might be more efficient to handle this within the initial array creation. Consider adding 'email' directly to the default array and then deduplicating after processing the environment variable, if necessary.
| return hasCopilotRole(currentUser) | ||
| } | ||
|
|
||
| function normalizeUserId (userId) { |
There was a problem hiding this comment.
[correctness]
The normalizeUserId function returns null for undefined or null inputs, which is fine. However, consider handling cases where userId is an empty string or a non-numeric string more explicitly, as these could lead to unexpected behavior if not intended.
| } | ||
|
|
||
| try { | ||
| const copilotResources = await resourcesPrisma.resource.findMany({ |
There was a problem hiding this comment.
[performance]
Consider adding a timeout or limit to the findMany query to prevent potential performance issues if the dataset is large. This can help mitigate risks of long-running queries.
| accessibleMemberIds.add(memberId) | ||
| } | ||
| }) | ||
| } catch (error) { |
There was a problem hiding this comment.
[maintainability]
The catch block logs an error but does not rethrow or handle it further. Consider whether the function should fail silently or if additional error handling is needed to inform the caller of the failure.
| } | ||
| return false | ||
|
|
||
| const allowedRolesLower = constants.SENSITIVE_DATA_ROLES.map(r => r.toLowerCase()) |
There was a problem hiding this comment.
[correctness]
The use of map and some for role checking is more efficient and readable than the previous nested loop approach. However, ensure that constants.SENSITIVE_DATA_ROLES is always defined and an array to prevent runtime errors.
| // PM can download | ||
| if (hasProfileDownloadableRole(currentUser)) { | ||
| // Admin or Talent Manager can download | ||
| if (hasSensitiveDataRole(currentUser)) { |
There was a problem hiding this comment.
[maintainability]
The comment mentions 'Admin or Talent Manager', but the function hasSensitiveDataRole checks against constants.SENSITIVE_DATA_ROLES. Ensure that constants.SENSITIVE_DATA_ROLES accurately reflects the roles intended by this comment to avoid potential misunderstandings.
| ], | ||
| }; | ||
|
|
||
| const createPgAdapter = (dbUrl) => { |
There was a problem hiding this comment.
[correctness]
The createPgAdapter function currently does not handle invalid URLs gracefully. Consider adding validation to ensure dbUrl is a valid URL before attempting to parse it. This will prevent potential runtime errors if an invalid URL is passed.
| engagementsClient = new EngagementsPrismaClient({ | ||
| ...clientOptions, | ||
| adapter, | ||
| adapter: createPgAdapter(engagementsDbUrl), |
There was a problem hiding this comment.
[correctness]
The createPgAdapter function is used to create the adapter for the EngagementsPrismaClient. Ensure that the extractSchemaFromUrl function correctly extracts the schema from the URL and that the schema exists in the database. If the schema does not exist, it could lead to runtime errors.
|
|
||
| ret.activity = _.mapValues(grouped, (v, k) => ({ | ||
| sources: _.uniqBy(v, 'sourceId').map(s => s.sourceId) | ||
| sources: v, |
There was a problem hiding this comment.
[❗❗ correctness]
The change from _.uniqBy(v, 'sourceId').map(s => s.sourceId) to v means that the sources array will now contain full event objects instead of just unique sourceIds. Ensure that this change is intentional and that the consuming code can handle the new structure.
| activityItem: { | ||
| fontSize: 10, | ||
| marginBottom: 5, | ||
| marginBottom: 2, |
There was a problem hiding this comment.
[💡 readability]
Reducing marginBottom from 5 to 2 for activityItem might affect the visual spacing and readability of the activity section. Ensure this change aligns with the overall design requirements.
| certificationItem: { | ||
| fontSize: 10, | ||
| marginBottom: 8, | ||
| marginBottom: 2, |
There was a problem hiding this comment.
[💡 readability]
Reducing marginBottom from 8 to 2 for certificationItem could impact the visual separation between items. Verify that this change maintains the intended layout and readability.
| marginBottom: 2, | ||
| lineHeight: 1.5, | ||
| color: '#000000' | ||
| }, |
There was a problem hiding this comment.
[💡 readability]
Removing marginTop from courseItem may affect the spacing between course items and preceding elements. Confirm that this change does not negatively impact the visual structure.
| const statsItems = topcoderActivity.statsByTrack.map((stat, index) => { | ||
| const isCompetitiveProgramming = stat.trackName === 'Competitive Programming' | ||
| const valueText = isCompetitiveProgramming | ||
| ? `${stat.rating ?? 0} rating, ${stat.wins ?? 0} wins, ${stat.competitions ?? 0} competitions` |
There was a problem hiding this comment.
[💡 correctness]
Using the nullish coalescing operator (??) is a good choice for handling potential null or undefined values in stat.rating, stat.wins, and stat.competitions. Ensure that 0 is the desired fallback value in all cases.
| } | ||
|
|
||
| // Remove availableForGigs if user doesn't have permission | ||
| if (!canManageMember && !hasSensitiveDataRole && res.availableForGigs !== undefined) { |
There was a problem hiding this comment.
[💡 maintainability]
The check res.availableForGigs !== undefined is redundant since _.omit will handle undefined values gracefully. Consider removing it for cleaner code.
|
|
||
| // Copilots can only see member email when they share at least one challenge resource. | ||
| if (response.email !== undefined) { | ||
| const canAccessMemberEmail = await copilotEmailAccess.canCopilotAccessMemberEmail( |
There was a problem hiding this comment.
[correctness]
The function canCopilotAccessMemberEmail is awaited but not wrapped in a try-catch block. Consider adding error handling to ensure that any exceptions do not cause the entire getMember function to fail.
| const trackMap = {} // track enum -> { wins, submissions, challenges } or { rating, wins, competitions } for DATA_SCIENCE | ||
|
|
||
| try { | ||
| const numUserId = typeof userId === 'bigint' ? helper.bigIntToNumber(userId) : userId |
There was a problem hiding this comment.
[💡 maintainability]
The conversion of userId to a number using helper.bigIntToNumber is repeated. Consider refactoring this logic to a separate function or performing it once at the beginning of the function to improve readability and maintainability.
| if (challengeSources.length > 0) { | ||
| const challengeIds = challengeSources | ||
|
|
||
| const winMap = {challenge_2nd_place: 'challenge_win', challenge_3rd_place: 'challenge_win'}; |
There was a problem hiding this comment.
[💡 performance]
The winMap object is defined within the function scope but could be moved outside if it is reused across multiple calls to avoid re-creation and improve performance.
| searchData.result = _.map(searchData.result, res => helper.truncateLastName(res)) | ||
| } | ||
|
|
||
| searchData.result = await applyCopilotEmailFiltering( |
There was a problem hiding this comment.
[maintainability]
The function applyCopilotEmailFiltering is called multiple times in different functions. Consider refactoring to reduce code duplication and improve maintainability.
| ) | ||
|
|
||
| if (shouldLimitCopilotEmailAccess && isEmailLookup) { | ||
| searchData.result = searchData.result.filter(result => ( |
There was a problem hiding this comment.
[correctness]
The filtering logic for searchData.result assumes that result.email is always defined and non-null. Consider handling cases where result.email might be undefined or null to prevent potential runtime errors.
| } | ||
| }) | ||
|
|
||
| await copilotEmailAccess.stripUnauthorizedCopilotEmails(currentUser, members) |
There was a problem hiding this comment.
[correctness]
The stripUnauthorizedCopilotEmails function is called without checking if members is an array or if it contains elements. Consider adding a check to ensure members is a valid array before calling this function.
| }) | ||
|
|
||
| it('get member should not expose email for regular users when communication fields are misconfigured', async () => { | ||
| const originalCommunicationSecureFields = [...config.COMMUNICATION_SECURE_FIELDS] |
There was a problem hiding this comment.
[maintainability]
Modifying global configuration config.COMMUNICATION_SECURE_FIELDS directly can lead to side effects in other tests or parts of the application. Consider using a mock or a more isolated approach to modify configuration settings for this test.
…privacy PM-3826 #time 30m exclude private personalization trait from api response
|
|
||
| const hasAutocompleteRole = helper.hasAutocompleteRole(currentUser) | ||
| const isAdminOrM2M = currentUser && (currentUser.isMachine || helper.hasAdminRole(currentUser)) | ||
| const hasSensitiveDataRole = helper.hasSensitiveDataRole(currentUser) |
There was a problem hiding this comment.
[❗❗ security]
The change from hasAutocompleteRole to hasSensitiveDataRole may affect the logic of determining access rights. Ensure that hasSensitiveDataRole is the intended check for accessing private personalization info, as this could impact security and correctness.
| const hasAutocompleteRole = helper.hasAutocompleteRole(currentUser) | ||
| const isAdminOrM2M = currentUser && (currentUser.isMachine || helper.hasAdminRole(currentUser)) | ||
| const hasSensitiveDataRole = helper.hasSensitiveDataRole(currentUser) | ||
| const isM2M = currentUser && currentUser.isMachine |
There was a problem hiding this comment.
[❗❗ security]
The removal of the isAdmin check in isAdminOrM2M to just isM2M might unintentionally restrict access for admin users. Verify that this change aligns with the intended access control policy.
4 participants
https://topcoder.atlassian.net/browse/PM-3825
https://topcoder.atlassian.net/browse/PS-526?search_id=61f7941b-b639-426f-8acb-a97253d71d8c
https://topcoder.atlassian.net/browse/PM-3733?search_id=d3e34168-a679-49e9-a5a6-bb1dcb8f210b
https://topcoder.atlassian.net/browse/PM-3717?search_id=d3e34168-a679-49e9-a5a6-bb1dcb8f210b