Conversation
PM-3920 Exclude profile picture from completion metric
What was broken:\nImported member max-rating colors could default to a single red value, which produced incorrect handle colors in API data after migration/import when source color was missing or blank.\n\nRoot cause:\nThe migration script used a hardcoded DEFAULT_RATING_COLOR (#EF3A3A) whenever maxRating.ratingColor was absent, instead of deriving color from the member rating tiers.\n\nWhat was changed:\n- Added rating-band based fallback resolution in migrate-dynamo-data (resolveMaxRatingColor).\n- Preserved valid imported hex colors; otherwise derive color from rating.\n- Replaced all migration/import fallback sites to use the new resolver.\n\nAny added/updated tests:\n- Added unit test file test/unit/MigrateDynamoData.test.js covering:\n - preserving valid imported color\n - deriving color when color is missing\n - deriving color when color is blank
What was broken: - Talent search requests to /members/searchBySkills for broad skills (for example Java) could take too long and return gateway timeout errors. Root cause: - The skill-score path materialized and scored the full matched population using very large IN queries and app-side grouping, which became too expensive for high-volume skills. What was changed: - Reworked skill-id matching to run as a grouped SQL query in the skills DB (COUNT DISTINCT + HAVING) instead of loading all user_skill rows and grouping in memory. - Updated sortBy=skillScore member filling to process matched user IDs in batches (1000 per batch), compute scores per batch, and then paginate ranked results. - Avoided an unnecessary member.count call for the sortBy=skillScore flow; member.count remains for non-skillScore paths. Added/updated tests: - Added test/unit/SearchService.test.js to verify skill-score searches are chunked and return paged results without invoking member.count in that path.
What was broken:
member-api-v6 did not provide /v6/members/{handle}/sendgrid-emails, so admins/M2M could not retrieve recent SendGrid email activity for a member.
Root cause:
No route/controller/service implementation existed for SendGrid Email Activity retrieval, and SENDGRID_API_KEY was not wired in this service configuration.
What was changed:
- Added GET /members/:handle/sendgrid-emails route with JWT auth, admin-role access for user tokens, and existing member read scopes for M2M.
- Added MemberController.getMemberSendgridEmails and MemberService.getMemberSendgridEmails.
- Implemented service logic to enforce admin-or-M2M access, resolve member email by handle, query SendGrid messages API for the last 30 days, and follow pagination via _metadata.next to return all records.
- Added SENDGRID_API_KEY to config/default.js.
- Updated API/config documentation in docs/swagger.yaml and ReadMe.md.
Any added/updated tests:
- Updated test/unit/MemberService.test.js with new sendgrid email activity tests covering:
- forbidden access for non-admin JWT users
- successful paginated retrieval for admin callers
- successful access for M2M callers
- missing SENDGRID_API_KEY error handling
PM-3924 check for city and country in profile completeness
Initial stats refactoring
…onalization trait
Additional tweaks to new stats stuff, including option to use legacy or new tables when returning data, for testing
PM-3924 Allow TMs to view full address
fix(PM-3847): added extra logic to remove the nested links in personalization trait
PM 3893 Throw error when duplicate phone is added
PM 3893 Only block duplicate phone in payload
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| branches: | ||
| only: | ||
| - develop | ||
| - PM-3893 |
There was a problem hiding this comment.
[maintainability]
Adding specific feature branches like PM-3893 and pm-3847_1 to the build filters can be useful for testing, but ensure these branches are temporary and removed once the feature is merged to avoid clutter and potential confusion in the workflow configuration.
| AUTH_SECRET: process.env.AUTH_SECRET || 'mysecret', | ||
| VALID_ISSUERS: process.env.VALID_ISSUERS || '["https://api.topcoder-dev.com", "https://api.topcoder.com", "https://topcoder-dev.auth0.com/", "https://auth.topcoder-dev.com/"]', | ||
| IDENTITY_DB_URL: process.env.IDENTITY_DB_URL, | ||
| CHALLENGE_DB_URL: process.env.CHALLENGE_DB_URL, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that CHALLENGE_DB_URL is properly validated and sanitized if it is used to construct database connections, to prevent potential security vulnerabilities such as SQL injection.
| ? process.env.STATISTICS_SECURE_FIELDS.split(',') | ||
| : ['createdBy', 'updatedBy'], | ||
| // Select stats read source: 'unified' (new tables) or 'legacy' (pre-refactor tables) | ||
| STATS_READ_SOURCE: process.env.STATS_READ_SOURCE || 'unified', |
There was a problem hiding this comment.
[maintainability]
Consider documenting the possible values for STATS_READ_SOURCE and their implications elsewhere in the codebase or documentation, as this choice could significantly impact data handling and performance.
|
|
||
| // Finance database connection string | ||
| FINANCE_DATABASE_URL: process.env.FINANCE_DATABASE_URL, | ||
| SENDGRID_API_KEY: process.env.SENDGRID_API_KEY, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that SENDGRID_API_KEY is stored securely and not logged or exposed in any way, as it is a sensitive credential.
|
|
||
| datasource challengedb { | ||
| provider = "postgresql" | ||
| url = env("CHALLENGE_DB_URL") |
There was a problem hiding this comment.
[security]
Consider adding a validation or a fallback mechanism for CHALLENGE_DB_URL to ensure that the environment variable is set correctly and the application doesn't fail at runtime due to a missing or incorrect database URL.
| } | ||
|
|
||
| model Challenge { | ||
| id String @id |
There was a problem hiding this comment.
[design]
Consider adding a relation between Challenge and ChallengeTrack using the @relation attribute to enforce referential integrity and make it clear that trackId is a foreign key.
| model Challenge { | ||
| id String @id | ||
| trackId String | ||
| typeId String |
There was a problem hiding this comment.
[design]
Consider adding a relation between Challenge and ChallengeType using the @relation attribute to enforce referential integrity and make it clear that typeId is a foreign key.
| /* eslint-disable */ | ||
| module.exports = { ...require('.') } No newline at end of file | ||
| // biome-ignore-all lint: generated file | ||
| module.exports = { ...require('#main-entry-point') } No newline at end of file |
There was a problem hiding this comment.
[❗❗ correctness]
The change from require('.') to require('#main-entry-point') suggests a shift in the module being imported. Ensure that #main-entry-point is correctly configured and available in the environment where this code will run. This could impact the correctness if the module path is not properly resolved.
| handleLower: { | ||
| startsWith: term, | ||
| not: term | ||
| } |
There was a problem hiding this comment.
[correctness]
The use of not: term in the handleLower filter might not work as intended if term is an empty string. Consider adding a check to ensure term is not empty before applying this filter.
| records = await prisma.member.findMany({ | ||
| ...nonExactFilter, | ||
| select: selectFields, | ||
| skip: offset - 1, |
There was a problem hiding this comment.
[❗❗ correctness]
Subtracting 1 from offset when offset is greater than 0 could lead to incorrect pagination results. Ensure that offset is always non-negative to avoid potential errors.
| } | ||
| ) | ||
|
|
||
| should.equal(response.total, 3) |
There was a problem hiding this comment.
[maintainability]
The test assumes that service.autocomplete will always return a total of 3. Consider verifying that the mock setup aligns with the actual implementation logic to ensure the test remains valid if the implementation changes.
| } | ||
| ) | ||
|
|
||
| should.equal(response.total, 5) |
There was a problem hiding this comment.
[maintainability]
The test assumes that service.autocomplete will always return a total of 5. Consider verifying that the mock setup aligns with the actual implementation logic to ensure the test remains valid if the implementation changes.
4 participants
No description provided.