Conversation
… into PM-4158_completed-profiles-report
…les-report PM-4158 completed profiles report
…les-report PM-4158 - Update profiles report
… into PM-4158_completed-profiles-report
…les-report PM-4158fix user role for completed profile report
… into PM-4198_customer-portal-completed-profiles-report-updates
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| WITH challenge_context AS ( | ||
| SELECT | ||
| c.id, | ||
| (ct.name = 'Marathon Match') AS is_marathon_match |
There was a problem hiding this comment.
[💡 readability]
Using a boolean expression (ct.name = 'Marathon Match') directly in the SELECT clause can be unclear. Consider using a CASE statement for better readability and maintainability.
| SELECT rs."aggregateScore" | ||
| FROM reviews."reviewSummation" AS rs | ||
| WHERE rs."submissionId" = s.id | ||
| AND COALESCE(rs."isFinal", TRUE) = TRUE |
There was a problem hiding this comment.
[correctness]
The use of COALESCE(rs."isFinal", TRUE) = TRUE assumes that isFinal can be NULL and defaults it to TRUE. Ensure this logic is intentional and aligns with business rules.
| END AS "finalRank" | ||
| FROM submitter_members AS sm | ||
| LEFT JOIN identity."user" AS u | ||
| ON sm."memberId" ~ '^[0-9]+$' |
There was a problem hiding this comment.
[maintainability]
The regular expression sm."memberId" ~ '^[0-9]+$' is used multiple times. Consider extracting this logic into a common CTE or function to improve maintainability.
| ON sm."memberId" ~ '^[0-9]+$' | ||
| AND mem."userId" = sm."memberId"::bigint | ||
| LEFT JOIN lookups."Country" AS home_code | ||
| ON UPPER(home_code."countryCode") = UPPER(mem."homeCountryCode") |
There was a problem hiding this comment.
[performance]
Using UPPER() for case-insensitive comparison can lead to performance issues if the column is not indexed accordingly. Consider ensuring the column is indexed with a case-insensitive collation or using a more efficient method.
| final_review."aggregateScore" AS final_score_raw, | ||
| ( | ||
| passing_review.is_passing IS TRUE | ||
| OR COALESCE(s."finalScore"::double precision, 0) > 98 |
There was a problem hiding this comment.
[maintainability]
The condition COALESCE(s."finalScore"::double precision, 0) > 98 assumes a threshold of 98 for a valid submission. Ensure this threshold is appropriate and consistent with business rules, as hardcoding such values can lead to maintenance challenges if the criteria change.
| FROM submission_metrics | ||
| WHERE is_valid_submission = TRUE | ||
| ), | ||
| valid_submitter_members AS MATERIALIZED ( |
There was a problem hiding this comment.
[performance]
Consider whether the use of MATERIALIZED for the valid_submitter_members CTE is necessary. While it can improve performance by caching the result, it may also increase memory usage. Evaluate the trade-offs based on the expected size of the data.
| END AS "finalRank" | ||
| FROM valid_submitter_members AS vsm | ||
| LEFT JOIN identity."user" AS u | ||
| ON vsm."memberId" ~ '^[0-9]+$' |
There was a problem hiding this comment.
[maintainability]
The regular expression vsm."memberId" ~ '^[0-9]+$' is used multiple times to check if memberId is numeric. Consider extracting this logic into a separate CTE or function to avoid repetition and improve maintainability.
| LEFT JOIN mm_ranked_scores AS mrs | ||
| ON mrs."memberId" = vsm."memberId" | ||
| ORDER BY | ||
| CASE |
There was a problem hiding this comment.
[correctness]
The ORDER BY clause uses multiple CASE statements with NULLS LAST. Ensure that the ordering logic aligns with the intended business requirements, as complex ordering can lead to unexpected results if not thoroughly validated.
| WITH challenge_context AS ( | ||
| SELECT | ||
| c.id, | ||
| (ct.name = 'Marathon Match') AS is_marathon_match |
There was a problem hiding this comment.
[correctness]
The expression (ct.name = 'Marathon Match') AS is_marathon_match relies on a string comparison to determine the challenge type. Consider using a more robust method, such as a type ID or enum, to avoid potential issues with string mismatches or typos.
| JOIN reviews."submission" AS s | ||
| ON s."challengeId" = cc.id | ||
| AND s."memberId" IS NOT NULL | ||
| LEFT JOIN LATERAL ( |
There was a problem hiding this comment.
[performance]
Using LEFT JOIN LATERAL with ORDER BY COALESCE(rs."reviewedDate", rs."createdAt") DESC NULLS LAST, rs.id DESC could potentially lead to performance issues if the reviewSummation table is large and not properly indexed on these columns. Ensure that appropriate indexes are in place to optimize this query.
| END AS "finalRank" | ||
| FROM winner_members AS wm | ||
| LEFT JOIN identity."user" AS u | ||
| ON wm."memberId" ~ '^[0-9]+$' |
There was a problem hiding this comment.
[💡 maintainability]
The condition wm."memberId" ~ '^[0-9]+$' is used multiple times to check if memberId is numeric. Consider extracting this logic into a common expression or function to improve maintainability and reduce repetition.
| LEFT JOIN members."member" AS mem | ||
| ON wm."memberId" ~ '^[0-9]+$' | ||
| AND mem."userId" = wm."memberId"::bigint | ||
| LEFT JOIN lookups."Country" AS home_code |
There was a problem hiding this comment.
[performance]
The use of UPPER() for case-insensitive comparison in joins with the Country table could lead to performance degradation if the table is large. Ensure that the countryCode and id columns are indexed appropriately to support case-insensitive lookups.
| LEFT JOIN groups."User" gu | ||
| ON gu.id = gm."memberId" | ||
| JOIN identity."user" u | ||
| ON u.user_id::text = ( |
There was a problem hiding this comment.
[performance]
The use of u.user_id::text in the join condition could lead to performance issues due to type casting on every row. Consider ensuring that gm."memberId" and gu."universalUID" are stored as the same type as u.user_id to avoid casting.
| LEFT JOIN identity.email e | ||
| ON e.user_id = u.user_id | ||
| AND e.primary_ind = 1 | ||
| WHERE ($1::text IS NOT NULL OR $2::text IS NOT NULL) |
There was a problem hiding this comment.
[performance]
The condition ($1::text IS NOT NULL OR $2::text IS NOT NULL) in the WHERE clause could lead to a full table scan if both parameters are NULL. Consider adding an index or restructuring the query to handle cases where both parameters are NULL more efficiently.
| ) | ||
| AND ( | ||
| $2::text IS NULL | ||
| OR LOWER(g.name) = LOWER($2::text) |
There was a problem hiding this comment.
[performance]
Using LOWER() on both sides of the comparison in the WHERE clause can be inefficient, especially if the table is large. Consider creating a case-insensitive index on g.name and g.description to improve query performance.
| ) AS "country" | ||
| FROM input_handles AS ih | ||
| LEFT JOIN identity."user" AS u | ||
| ON LOWER(u.handle) = LOWER(ih.handle_input) |
There was a problem hiding this comment.
[performance]
The use of LOWER for case-insensitive comparison can lead to performance issues if the handle column is not indexed with a case-insensitive index. Consider adding an appropriate index to improve query performance.
| SELECT e.address | ||
| FROM identity.email AS e | ||
| WHERE e.user_id = u.user_id | ||
| AND NULLIF(BTRIM(e.address), '') IS NOT NULL |
There was a problem hiding this comment.
[correctness]
The condition NULLIF(BTRIM(e.address), '') IS NOT NULL is used to filter out empty email addresses. Ensure that this logic aligns with the business requirements, as it may exclude valid but empty email addresses.
| FROM identity.role r | ||
| JOIN identity.role_assignment ra | ||
| ON r.id = ra.role_id | ||
| JOIN identity."user" u |
There was a problem hiding this comment.
[💡 readability]
Using double quotes for the table name "user" is necessary due to it being a reserved keyword in SQL. However, consider renaming the table to avoid the need for quoting, which can improve readability and reduce potential for errors.
| LEFT JOIN identity.email e | ||
| ON e.user_id = u.user_id | ||
| AND e.primary_ind = 1 | ||
| WHERE ($1::int IS NOT NULL OR $2::text IS NOT NULL) |
There was a problem hiding this comment.
[performance]
The condition ($1::int IS NOT NULL OR $2::text IS NOT NULL) in the WHERE clause could lead to a full table scan if both parameters are NULL. Consider adding an index on the role_id and name columns to improve query performance.
| WHERE ($1::int IS NOT NULL OR $2::text IS NOT NULL) | ||
| AND ra.subject_type = 1 | ||
| AND ($1::int IS NULL OR r.id = $1::int) | ||
| AND ($2::text IS NULL OR LOWER(r.name) = LOWER($2::text)); |
There was a problem hiding this comment.
[performance]
Using LOWER(r.name) = LOWER($2::text) can prevent the use of an index on r.name if one exists. Consider using a case-insensitive collation or a functional index to optimize this condition.
| WITH challenge_context AS ( | ||
| SELECT | ||
| c.id, | ||
| (LOWER(ct.name) = LOWER('Marathon Match')) AS is_marathon_match |
There was a problem hiding this comment.
[performance]
Using LOWER(ct.name) = LOWER('Marathon Match') for case-insensitive comparison is functional but can be inefficient if the dataset is large. Consider using ILIKE for case-insensitive comparisons if supported by your SQL dialect.
| FROM challenges."Challenge" AS c | ||
| JOIN challenges."ChallengeType" AS ct | ||
| ON ct.id = c."typeId" | ||
| WHERE c.id = $1::text |
There was a problem hiding this comment.
[❗❗ security]
Ensure that $1::text is properly sanitized and validated to prevent SQL injection vulnerabilities, especially if this value comes from user input.
| SELECT | ||
| st.user_id, | ||
| COALESCE( | ||
| jsonb_agg( |
There was a problem hiding this comment.
[performance]
The use of jsonb_agg with a FILTER clause is correct, but be aware that this could lead to performance issues if the dataset is large. Consider evaluating the performance impact if this query is expected to handle a significant amount of data.
| SELECT | ||
| st.user_id, | ||
| COALESCE( | ||
| NULLIF(TRIM(mem.handle), ''), |
There was a problem hiding this comment.
[performance]
The use of NULLIF(TRIM(mem.handle), '') is a good practice to handle empty strings, but ensure that mem.handle is indexed if this query is expected to run on large datasets to avoid performance degradation.
| AND res."memberId" = st.user_id::text | ||
| ) AS handle_fallback ON TRUE | ||
| LEFT JOIN lookups."Country" AS home_code | ||
| ON UPPER(home_code."countryCode") = UPPER(mem."homeCountryCode") |
There was a problem hiding this comment.
[performance]
Using UPPER for case-insensitive joins can lead to performance issues if the countryCode fields are not indexed. Consider adding functional indexes if this query is expected to run frequently on large datasets.
| 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]
Using COALESCE with m."homeCountryCode" and m."competitionCountryCode" assumes that if homeCountryCode is NULL, competitionCountryCode will be used. Ensure this logic aligns with the intended behavior, as it might lead to unexpected results if both fields can be NULL.
| AND m."photoURL" <> '' | ||
| AND m."homeCountryCode" IS NOT NULL | ||
| AND ($1::text IS NULL OR COALESCE(m."homeCountryCode", m."competitionCountryCode") = $1) | ||
| AND ($3::uuid[] IS NULL OR ms.skill_ids @> $3::uuid[]) |
There was a problem hiding this comment.
[correctness]
The use of the @> operator assumes that ms.skill_ids contains all elements of $3::uuid[]. Ensure that this behavior is intended, as it might exclude members with additional skills not in the filter.
| AND mtp.key = 'openToWork' | ||
| AND mtp.value IS NOT NULL | ||
| AND ( | ||
| NOT (mtp.value::jsonb ? 'availability') |
There was a problem hiding this comment.
[correctness]
The condition NOT (mtp.value::jsonb ? 'availability') might lead to unexpected behavior if availability is a required field in some contexts. Double-check the logic to ensure it aligns with the business requirements.
| SELECT | ||
| mtp.value AS open_to_work_value, | ||
| ( | ||
| mtp.value::jsonb ? 'availability' |
There was a problem hiding this comment.
[correctness]
The use of btrim(mtp.value->>'availability') <> '' could be problematic if availability is not a string or is missing. Consider using a safer check to ensure availability is a string and exists before trimming.
| 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 will be a valid fallback. Ensure that this logic aligns with business requirements, as it may lead to unexpected results if both are null.
| -- Check engagement availability exists | ||
| AND ot.open_to_work_value IS NOT NULL | ||
| AND ( | ||
| NOT (ot.open_to_work_value::jsonb ? 'availability') |
There was a problem hiding this comment.
[❗❗ correctness]
The logic here checks for the presence of availability and preferredRoles in open_to_work_value. If open_to_work_value is not guaranteed to be a JSON object, this could lead to runtime errors. Consider adding a type check or validation step.
| ) AS max_rating ON TRUE | ||
| LEFT JOIN LATERAL ( | ||
| SELECT mmar.* | ||
| SELECT COUNT(*) AS competitions |
There was a problem hiding this comment.
[performance]
The use of COUNT(*) without a specific column can lead to performance issues if the table is large. Consider counting a specific indexed column instead, such as msh."userId", to potentially improve performance.
| AND ms."typeId" = 'MARATHON_MATCH' | ||
| ORDER BY | ||
| CASE WHEN ms."isPrivate" THEN 1 ELSE 0 END, | ||
| ms."isPrivate" ASC, |
There was a problem hiding this comment.
[correctness]
The change from CASE WHEN ms."isPrivate" THEN 1 ELSE 0 END to ms."isPrivate" ASC is correct but ensure that the column ms."isPrivate" is of a boolean type. If it's not, this could lead to unexpected ordering behavior.
| }; | ||
|
|
||
| export const UserRoles = { | ||
| export const AdminRoles = { |
There was a problem hiding this comment.
[maintainability]
Renaming UserRoles to AdminRoles could lead to confusion since UserRoles is still used elsewhere in the code. Consider ensuring that the naming is consistent and clear to avoid misunderstandings about the role definitions.
| UserRoles.TalentManager, | ||
| ] as const; | ||
|
|
||
| export const ScopeRoleAccess: Record<string, readonly string[]> = { |
There was a problem hiding this comment.
[correctness]
The ScopeRoleAccess object uses string keys derived from Scopes. If any scope key changes, it could lead to runtime errors. Consider using TypeScript enums or constants to ensure compile-time safety and reduce the risk of typos.
| if (this.hasRequiredScope(scopes, requiredScopes)) { | ||
| return true; | ||
| } | ||
| if (hasAccessToScopes(authUser, requiredScopes)) { |
There was a problem hiding this comment.
[correctness]
The refactoring to use hasAccessToScopes simplifies the logic, but ensure that this utility function correctly handles both machine and user roles, as the previous implementation had distinct logic for each.
| authUser, | ||
| }), | ||
| }), | ||
| } as unknown as ExecutionContext; |
There was a problem hiding this comment.
[maintainability]
Casting the return value to ExecutionContext using as unknown as ExecutionContext could hide potential type mismatches. Consider using a more explicit type assertion or ensuring the object structure matches ExecutionContext to improve type safety.
| SCOPES_KEY, | ||
| [context.getHandler(), context.getClass()], | ||
| ) ?? [Scopes.TopcoderReports, Scopes.AllReports]; | ||
|
|
There was a problem hiding this comment.
[security]
The default scopes [Scopes.TopcoderReports, Scopes.AllReports] are used if this.reflector.getAllAndOverride returns null or undefined. Ensure this default behavior is intended, as it might grant broader access than expected if the decorator is missing or misconfigured.
| private isAdmin(roles: string[]): boolean { | ||
| return roles.some((role) => | ||
| TopcoderReportsGuard.adminRoles.has(role?.toLowerCase()), | ||
| return getNormalizedRoles(authUser).some((role) => |
There was a problem hiding this comment.
[❗❗ correctness]
The method getNormalizedRoles is used to check roles against completedProfilesRoles. Ensure that getNormalizedRoles correctly normalizes roles to match the format used in completedProfilesRoles. Any mismatch could lead to incorrect access control.
|
|
||
| export type AuthUserLike = { | ||
| isMachine?: boolean; | ||
| roles?: string[] | string; |
There was a problem hiding this comment.
[design]
The roles and role properties in AuthUserLike are both optional and can be either a string or an array of strings. This could lead to confusion or errors if both are provided with different values. Consider consolidating these into a single property or ensuring consistent usage across the codebase.
| * Scope comparisons are case-insensitive. | ||
| */ | ||
| export function hasRequiredScope( | ||
| scopes: readonly string[] | string | undefined, |
There was a problem hiding this comment.
[correctness]
The hasRequiredScope function does not handle cases where scopes is undefined or an empty string effectively, as it will return false early. Consider adding a check to handle these cases explicitly to avoid unexpected behavior.
| * Returns true when the caller has one of the configured administrator roles. | ||
| */ | ||
| export function hasAdminRole( | ||
| roles: readonly string[] | string | undefined, |
There was a problem hiding this comment.
[correctness]
The hasAdminRole function assumes that roles will always be defined. Consider adding a check for undefined or empty roles to prevent potential runtime errors.
| * Returns true when any required scope is mapped to one of the caller's roles. | ||
| */ | ||
| export function hasRequiredRoleAccess( | ||
| roles: readonly string[] | string | undefined, |
There was a problem hiding this comment.
[correctness]
In hasRequiredRoleAccess, the roles parameter is not checked for undefined or empty values before normalization. This could lead to unexpected results if roles is not provided. Consider adding a guard clause to handle these cases.
| status: 200, | ||
| description: "Export successful.", | ||
| }) | ||
| async getRegisteredUsers(@Param() params: ChallengeUsersPathParamDto) { |
There was a problem hiding this comment.
[security]
Consider adding validation to ensure challengeId is a valid format before passing it to the service. This can prevent potential errors or security issues if invalid data is passed.
| status: 200, | ||
| description: "Export successful.", | ||
| }) | ||
| async getSubmitters(@Param() params: ChallengeUsersPathParamDto) { |
There was a problem hiding this comment.
[security]
Consider adding validation to ensure challengeId is a valid format before passing it to the service. This can prevent potential errors or security issues if invalid data is passed.
| status: 200, | ||
| description: "Export successful.", | ||
| }) | ||
| async getValidSubmitters(@Param() params: ChallengeUsersPathParamDto) { |
There was a problem hiding this comment.
[security]
Consider adding validation to ensure challengeId is a valid format before passing it to the service. This can prevent potential errors or security issues if invalid data is passed.
| status: 200, | ||
| description: "Export successful.", | ||
| }) | ||
| async getWinners(@Param() params: ChallengeUsersPathParamDto) { |
There was a problem hiding this comment.
[security]
Consider adding validation to ensure challengeId is a valid format before passing it to the service. This can prevent potential errors or security issues if invalid data is passed.
|
|
||
| return results; | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding more context to the error log message to help with debugging. Currently, only the error object is logged, which might not provide enough information about the context in which the error occurred.
|
|
||
| return this.formatChallengeUserReport(results); | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding more context to the error log message to help with debugging. Currently, only the error object is logged, which might not provide enough information about the context in which the error occurred.
|
|
||
| return this.formatChallengeUserReport(results); | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding more context to the error log message to help with debugging. Currently, only the error object is logged, which might not provide enough information about the context in which the error occurred.
|
|
||
| return this.formatChallengeUserReport(results); | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding more context to the error log message to help with debugging. Currently, only the error object is logged, which might not provide enough information about the context in which the error occurred.
| * latest submission and finalRank by current effective score, breaking ties by | ||
| * earlier submission time. | ||
| */ | ||
| export interface ChallengeUserRecordDto { |
There was a problem hiding this comment.
[💡 maintainability]
Consider using readonly for properties in ChallengeUserRecordDto if they are not meant to be modified after creation. This can help prevent accidental mutations and improve the immutability of the data structure.
| export class UsersByRoleQueryDto { | ||
| @ApiProperty({ required: false }) | ||
| @IsOptional() | ||
| @IsInt() |
There was a problem hiding this comment.
[correctness]
Consider adding a validation to ensure roleId is a positive integer if applicable, as negative IDs might not be valid in your domain.
| * Shared identity user payload returned by identity report endpoints. | ||
| */ | ||
| export interface IdentityUserDto { | ||
| userId: number | null; |
There was a problem hiding this comment.
[correctness]
The userId field is defined as number | null. Ensure that the rest of the application logic correctly handles the null case to avoid potential runtime errors.
| */ | ||
| export interface IdentityUserDto { | ||
| userId: number | null; | ||
| handle: string; |
There was a problem hiding this comment.
[correctness]
Consider adding validation to ensure that handle is not an empty string, as this might be an invalid state for a user handle.
| export interface IdentityUserDto { | ||
| userId: number | null; | ||
| handle: string; | ||
| email: string | null; |
There was a problem hiding this comment.
[correctness]
The email field is defined as string | null. Ensure that the rest of the application logic correctly handles the null case to avoid potential runtime errors.
| userId: number | null; | ||
| handle: string; | ||
| email: string | null; | ||
| country: string | null; |
There was a problem hiding this comment.
[correctness]
The country field is defined as string | null. Ensure that the rest of the application logic correctly handles the null case to avoid potential runtime errors.
| */ | ||
| @Post("/users-by-handles") | ||
| @UseGuards(PermissionsGuard) | ||
| @UseInterceptors(FileInterceptor("file")) |
There was a problem hiding this comment.
[❗❗ security]
Using FileInterceptor with @UploadedFile() suggests that file uploads are expected. Ensure that the file size and type are validated to prevent potential security risks such as large file uploads or malicious file types.
| @ApiResponse({ status: 200, description: "Export successful." }) | ||
| async getUsersByHandles( | ||
| @Body( | ||
| new ValidationPipe({ |
There was a problem hiding this comment.
[correctness]
The ValidationPipe is configured with skipMissingProperties: true, which means missing properties in the DTO will not trigger validation errors. Ensure this behavior is intended, as it might allow incomplete data to pass through.
|
|
||
| return results; | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider providing more context in the error logging to help with debugging. For example, include the filters used in the query.
|
|
||
| return results; | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider providing more context in the error logging to help with debugging. For example, include the filters used in the query.
| country: alpha3ToCountryName(row.country) ?? row.country, | ||
| })); | ||
| } catch (e) { | ||
| this.logger.error(e); |
There was a problem hiding this comment.
[💡 maintainability]
Consider providing more context in the error logging to help with debugging. For example, include the handles being queried.
| */ | ||
| private parseHandlesFromFile(file: UploadedHandlesFile): string[] { | ||
| const extension = extname(file.originalname ?? "").toLowerCase(); | ||
| if (!SUPPORTED_HANDLES_UPLOAD_EXTENSIONS.has(extension)) { |
There was a problem hiding this comment.
[correctness]
The extname function may return an empty string if the file has no extension. Consider handling this case explicitly to improve error messaging.
| }); | ||
|
|
||
| it("returns an empty directory when no JWT user is present", () => { | ||
| expect(getAccessibleReportsDirectory()).toEqual({}); |
There was a problem hiding this comment.
[correctness]
The test case for when no JWT user is present does not provide any input to getAccessibleReportsDirectory. If the function relies on certain properties being present, this could lead to unexpected behavior. Consider explicitly passing an empty object or a default parameter to ensure clarity and prevent potential issues.
| }; | ||
|
|
||
| export type ReportsDirectory = Record<ReportGroupKey, ReportGroup>; | ||
| export type ReportsDirectory = Partial<Record<ReportGroupKey, ReportGroup>>; |
There was a problem hiding this comment.
[correctness]
Changing ReportsDirectory to a Partial<Record<ReportGroupKey, ReportGroup>> means that some report groups might be missing. Ensure that this change is intentional and that the rest of the codebase can handle potentially missing groups.
|
|
||
| type RegisteredReportsDirectory = Record<ReportGroupKey, RegisteredReportGroup>; | ||
|
|
||
| const report = ( |
There was a problem hiding this comment.
[correctness]
The report function defaults the method to "GET". Ensure that this is the intended behavior for all reports using this function, as it might lead to incorrect HTTP method usage if not explicitly specified.
| requiredScopes, | ||
| }); | ||
|
|
||
| const postReport = ( |
There was a problem hiding this comment.
[correctness]
The postReport function defaults the method to "POST". Ensure that this is the intended behavior for all reports using this function, as it might lead to incorrect HTTP method usage if not explicitly specified.
| location: "query", | ||
| }; | ||
|
|
||
| const handlesBodyParam: ReportParameter = { |
There was a problem hiding this comment.
[correctness]
The handlesBodyParam is marked as required: true and location: "body". Ensure that the API endpoints using this parameter are correctly handling body parameters, as this differs from typical query or path parameters.
| location: "query", | ||
| }; | ||
|
|
||
| const REGISTERED_REPORTS_DIRECTORY: RegisteredReportsDirectory = { |
There was a problem hiding this comment.
[maintainability]
The REGISTERED_REPORTS_DIRECTORY now includes new report groups such as member and admin. Ensure that these additions are correctly integrated with the rest of the system, particularly in terms of access control and data handling.
| ), | ||
| ); | ||
|
|
||
| export const REPORTS_DIRECTORY: ReportsDirectory = Object.fromEntries( |
There was a problem hiding this comment.
[correctness]
The REPORTS_DIRECTORY is now constructed using Object.fromEntries. Ensure that all necessary report groups are included in REGISTERED_REPORTS_DIRECTORY, as missing entries will result in missing report groups in REPORTS_DIRECTORY.
| * Returns the subset of the report catalog that the authenticated caller can run. | ||
| * Empty groups are omitted from the response. | ||
| */ | ||
| export function getAccessibleReportsDirectory( |
There was a problem hiding this comment.
[correctness]
The getAccessibleReportsDirectory function uses flatMap to filter accessible reports. Ensure that this logic correctly handles cases where a user has partial access to reports within a group, as it may affect the returned structure.
| @Get() | ||
| @UseGuards(PermissionsGuard) | ||
| @Scopes(AppScopes.AllReports) | ||
| @Scopes(AppScopes.AllReports, ...REPORTS_DIRECTORY_REQUIRED_SCOPES) |
There was a problem hiding this comment.
[❗❗ security]
The addition of ...REPORTS_DIRECTORY_REQUIRED_SCOPES to the @Scopes decorator increases the complexity of the permissions required. Ensure that this change aligns with the intended access control policy and that all necessary scopes are correctly defined in REPORTS_DIRECTORY_REQUIRED_SCOPES.
| @Get("/directory") | ||
| @UseGuards(PermissionsGuard) | ||
| @Scopes(AppScopes.AllReports) | ||
| @Scopes(AppScopes.AllReports, ...REPORTS_DIRECTORY_REQUIRED_SCOPES) |
There was a problem hiding this comment.
[❗❗ security]
Similar to the change on line 18, verify that the inclusion of ...REPORTS_DIRECTORY_REQUIRED_SCOPES in the @Scopes decorator is consistent with the desired security model and that it does not inadvertently restrict or expand access beyond what is intended.
| example: true, | ||
| }) | ||
| @IsOptional() | ||
| @Transform(({ value }) => |
There was a problem hiding this comment.
[correctness]
The transformation logic for openToWork could be improved for clarity. Consider using value === 'true' directly, as value === true will not match if the input is a string, which is likely the case when dealing with query parameters.
| return undefined; | ||
| } | ||
| const values = Array.isArray(value) ? value : [value]; | ||
| return values.map((v) => String(v)).filter((v) => v.trim().length > 0); |
There was a problem hiding this comment.
[💡 performance]
The transformation logic for skillId could be optimized by using filter(Boolean) instead of filter((v) => v.trim().length > 0), which is more concise and achieves the same result.
| @UseGuards(TopcoderReportsGuard) | ||
| @UseInterceptors(CsvResponseInterceptor) | ||
| @Controller("/topcoder") | ||
| @RequiredScopes(AppScopes.AllReports, AppScopes.TopcoderReports) |
There was a problem hiding this comment.
[security]
The @RequiredScopes decorator is applied to the entire controller, which might be too broad if different endpoints require different scopes. Consider applying it to individual methods if scope requirements vary.
| } | ||
|
|
||
| @Get("/member-payment-accrual") | ||
| @Get("/admin/member-payment-accrual") |
There was a problem hiding this comment.
[design]
The route /admin/member-payment-accrual is inconsistent with the other routes prefixed with /topcoder. Ensure this is intentional and aligns with your API design.
| @ApiOperation({ | ||
| summary: "List of members with 100% completed profiles", | ||
| }) | ||
| getCompletedProfiles(@Query() query: CompletedProfilesQueryDto) { |
There was a problem hiding this comment.
[correctness]
The getCompletedProfiles method parses page and perPage parameters to ensure they are within a valid range. However, it does not handle cases where page or perPage might be non-numeric strings. Consider adding validation to ensure these parameters are numeric.
| email: row.email ?? null, | ||
| country: row.country ?? null, | ||
| place: this.toNullableNumber(row.place), | ||
| provisionalScores: this.toNullableNumberArray(row.provisionalScores), |
There was a problem hiding this comment.
[correctness]
The toNullableNumberArray function is used to convert provisionalScores to a number array. However, the type of provisionalScores is unknown, which could lead to unexpected behavior if the input is not a JSON string or an array. Consider adding validation or type-checking to ensure the input is in the expected format.
| skillIdsParam, | ||
| ], | ||
| ); | ||
| const total = Number(countRows?.[0]?.total ?? 0); |
There was a problem hiding this comment.
[correctness]
The total variable is derived from countRows?.[0]?.total. If countRows is empty or total is not a valid number, this could lead to incorrect pagination. Consider adding a check to ensure total is a valid number.
| ]); | ||
|
|
||
| const data = rows.map((row) => ({ | ||
| userId: row.userId ? Number(row.userId) : null, |
There was a problem hiding this comment.
[correctness]
The conversion of userId to a number assumes that row.userId is a valid number or a string representation of a number. If row.userId is not a valid number, this could lead to unexpected results. Consider adding validation to ensure userId is a valid number before conversion.
| countryCode: row.countryCode || undefined, | ||
| countryName: row.countryName || undefined, | ||
| city: row.city || undefined, | ||
| skillCount: |
There was a problem hiding this comment.
[correctness]
The conversion of skillCount to a number assumes that row.skillCount is a valid number or a string representation of a number. If row.skillCount is not a valid number, this could lead to unexpected results. Consider adding validation to ensure skillCount is a valid number before conversion.
| * @param fileName snapshot file name under `data/statistics/mm` | ||
| * @returns parsed JSON payload exposed by the statistics endpoints | ||
| */ | ||
| private loadJson<T = any>(fileName: string): T { |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for fs.readFileSync and JSON.parse to manage potential file read errors or JSON parsing errors. This will improve the robustness of the method.
| */ | ||
| private loadJson<T = any>(fileName: string): T { | ||
| const fullPath = path.join(this.baseDir, fileName); | ||
| const raw = fs.readFileSync(fullPath, "utf-8"); |
There was a problem hiding this comment.
[correctness]
Consider adding error handling for fs.readFileSync and JSON.parse to manage potential file read errors or JSON parsing errors gracefully. This will improve the robustness of the service.
4 participants
No description provided.