Conversation
… into PM-4198_customer-portal-completed-profiles-report-updates
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| AND m."photoURL" IS NOT NULL | ||
| AND m."photoURL" <> '' | ||
| AND m."homeCountryCode" IS NOT NULL | ||
| AND ($1::text IS NULL OR COALESCE(m."homeCountryCode", m."competitionCountryCode") = $1) |
There was a problem hiding this comment.
[correctness]
The use of COALESCE(m."homeCountryCode", m."competitionCountryCode") assumes that if homeCountryCode is NULL, competitionCountryCode should be used. Ensure this logic aligns with business requirements, as it may lead to unexpected results if both fields are populated differently.
| AND mtp.key = 'openToWork' | ||
| AND mtp.value IS NOT NULL | ||
| AND ( | ||
| NOT (mtp.value::jsonb ? 'availability') |
There was a problem hiding this comment.
[maintainability]
The condition NOT (mtp.value::jsonb ? 'availability') followed by an OR clause could lead to complex logic that might be hard to maintain. Consider simplifying or documenting the intent of this logic to ensure future maintainability.
| ms.skill_count AS "skillCount" | ||
| FROM members.member m | ||
| INNER JOIN member_skills ms ON ms.user_id = m."userId" | ||
| LEFT JOIN LATERAL ( |
There was a problem hiding this comment.
[correctness]
The use of LEFT JOIN LATERAL with ORDER BY ma.id ASC LIMIT 1 is technically correct, but ensure that ma.id is the appropriate column to determine the 'first' address. If ma.id does not guarantee the desired order, consider using a more relevant column for ordering.
| ) | ||
| ORDER BY m.handle; | ||
| ORDER BY m.handle | ||
| LIMIT $2::int |
There was a problem hiding this comment.
[❗❗ security]
The addition of LIMIT $2::int OFFSET $3::int assumes that these parameters are properly validated and sanitized before being passed to the query. Ensure that these inputs are controlled to prevent SQL injection or unexpected behavior.
| example: "1", | ||
| }) | ||
| @IsOptional() | ||
| @IsNumberString() |
There was a problem hiding this comment.
[correctness]
Consider using @IsInt() instead of @IsNumberString() for page to ensure the value is a valid integer. This will prevent issues with non-numeric strings that could pass the current validation.
| example: "50", | ||
| }) | ||
| @IsOptional() | ||
| @IsNumberString() |
There was a problem hiding this comment.
[correctness]
Consider using @IsInt() instead of @IsNumberString() for perPage to ensure the value is a valid integer. This will prevent issues with non-numeric strings that could pass the current validation.
| const { countryCode } = query; | ||
| return this.reports.getCompletedProfiles(countryCode); | ||
| const { countryCode, page, perPage } = query; | ||
| const parsedPage = Math.max(Number(page || 1), 1); |
There was a problem hiding this comment.
[maintainability]
Consider using a utility function to parse and validate pagination parameters (page and perPage) to avoid code duplication and improve maintainability. This can also help ensure consistent behavior across different parts of the application.
|
|
||
| async getCompletedProfiles(countryCode?: string) { | ||
| async getCompletedProfiles(countryCode?: string, page = 1, perPage = 50) { | ||
| const safePage = Number.isFinite(page) ? Math.max(Math.floor(page), 1) : 1; |
There was a problem hiding this comment.
[correctness]
The safePage calculation uses Math.floor(page), which is correct for ensuring the page is an integer. However, the check Number.isFinite(page) should ensure page is a number before this calculation. Consider adding a type check or validation for page to ensure it is a number before proceeding with the calculation.
| async getCompletedProfiles(countryCode?: string) { | ||
| async getCompletedProfiles(countryCode?: string, page = 1, perPage = 50) { | ||
| const safePage = Number.isFinite(page) ? Math.max(Math.floor(page), 1) : 1; | ||
| const safePerPage = Number.isFinite(perPage) |
There was a problem hiding this comment.
[correctness]
The safePerPage calculation uses Math.floor(perPage), which is correct for ensuring the perPage is an integer. However, the check Number.isFinite(perPage) should ensure perPage is a number before this calculation. Consider adding a type check or validation for perPage to ensure it is a number before proceeding with the calculation.
| countQuery, | ||
| [countryCode || null], | ||
| ); | ||
| const total = Number(countRows?.[0]?.total ?? 0); |
There was a problem hiding this comment.
[correctness]
The conversion of total to a number using Number(countRows?.[0]?.total ?? 0) could lead to unexpected results if total is not a valid number string. Consider adding validation to ensure total is a valid number before conversion.
| countryName: row.countryName || undefined, | ||
| city: row.city || undefined, | ||
| skillCount: | ||
| row.skillCount !== null && row.skillCount !== undefined |
There was a problem hiding this comment.
[correctness]
The conversion of skillCount to a number using Number(row.skillCount) could lead to unexpected results if skillCount is not a valid number string. Consider adding validation to ensure skillCount is a valid number before conversion.
vas3a
deleted the
PM-4198_customer-portal-completed-profiles-report-updates
branch
1 participant
No description provided.