New topcoder reports and CSV support #26
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| WHERE w.type = 'PAYMENT' | ||
| AND p.installment_number = 1 | ||
| AND p.payment_status = 'PAID' | ||
| AND COALESCE(p.date_paid, p.created_at) >= (CURRENT_DATE - INTERVAL '3 months') |
There was a problem hiding this comment.
[correctness]
The use of CURRENT_DATE - INTERVAL '3 months' assumes that the system's time zone is correctly set and that the current date is accurate. Consider using a more explicit date range or verifying the system's time zone settings to avoid potential discrepancies.
| ON proj.id = c."projectId"::bigint | ||
| LEFT JOIN members.member mem | ||
| ON mem."userId"::text = rp.winner_id | ||
| ORDER BY payment_created_at DESC; No newline at end of file |
There was a problem hiding this comment.
[💡 style]
The query does not end with a newline character. While this does not affect SQL execution, it's a good practice to end files with a newline to avoid potential issues with some tools and version control systems.
| WHERE c."typeId" = '929bc408-9cf2-4b3e-ba71-adfbf693046c' | ||
| ), | ||
| submission_participants AS ( | ||
| SELECT DISTINCT ON (s."memberId", tc.id) |
There was a problem hiding this comment.
[correctness]
Using DISTINCT ON without a complete understanding of its behavior can lead to unexpected results. Ensure that the ordering in the ORDER BY clause is correct and aligns with the intended logic, as it determines which row is chosen for each distinct group.
| s.id DESC | ||
| ), | ||
| winner_participants AS ( | ||
| SELECT DISTINCT ON (cw."userId", tc.id) |
There was a problem hiding this comment.
[correctness]
Similar to the previous comment, using DISTINCT ON requires careful consideration of the ORDER BY clause. Verify that the ordering logic is correct to ensure the desired row is selected for each distinct group.
| ), | ||
| combined_participants AS ( | ||
| SELECT | ||
| COALESCE(wp.member_id, sp.member_id) AS member_id, |
There was a problem hiding this comment.
[❗❗ correctness]
The use of COALESCE to merge member_id, member_handle, and challenge_id from winner_participants and submission_participants assumes that one of the two will always be non-null. Ensure there are no cases where both could be null, as this would lead to incorrect data in the combined_participants CTE.
| LEFT JOIN lookups."Country" AS comp_code | ||
| ON UPPER(comp_code."countryCode") = UPPER(mem."competitionCountryCode") | ||
| LEFT JOIN lookups."Country" AS comp_id | ||
| ON UPPER(comp_id.id) = UPPER(mem."competitionCountryCode") |
There was a problem hiding this comment.
[❗❗ correctness]
The join condition UPPER(comp_id.id) = UPPER(mem."competitionCountryCode") seems incorrect. Typically, id would be a numeric or unique identifier rather than a country code. Verify if this is the intended logic.
| CASE | ||
| WHEN marathon_stats.challenges IS NULL | ||
| OR marathon_stats.challenges = 0 THEN NULL | ||
| ELSE marathon_stats.competitions::double precision |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that marathon_stats.competitions is never zero when marathon_stats.challenges is non-zero to avoid division by zero errors. Consider adding a safeguard if this is a potential issue.
| ON mmar."dataScienceStatsId" = mds.id | ||
| WHERE ms."userId" = mb."userId" | ||
| ORDER BY | ||
| CASE WHEN ms."isPrivate" THEN 1 ELSE 0 END, |
There was a problem hiding this comment.
[💡 readability]
The use of CASE WHEN ms."isPrivate" THEN 1 ELSE 0 END for ordering might not be intuitive. Consider using a boolean column directly for clarity if supported by the database.
| @Injectable() | ||
| export class PermissionsGuard implements CanActivate { | ||
| private static readonly adminRoles = new Set( | ||
| Object.values(UserRoles).map((role) => role.toLowerCase()), |
There was a problem hiding this comment.
[correctness]
Using toLowerCase() on roles assumes that all role comparisons are case-insensitive. Ensure that this aligns with the application's role management policy, as it might lead to unexpected behavior if roles are case-sensitive.
| return false; | ||
| } | ||
|
|
||
| const normalizedScopes = scopes.map((scope) => scope?.toLowerCase()); |
There was a problem hiding this comment.
[correctness]
The use of toLowerCase() on scopes assumes that scope comparisons are case-insensitive. Verify that this is consistent with how scopes are intended to be used throughout the application, as it might cause issues if scopes are case-sensitive.
| } | ||
|
|
||
| if (Array.isArray(value) || typeof value === "object") { | ||
| return JSON.stringify(value); |
There was a problem hiding this comment.
[correctness]
Serializing arrays or objects using JSON.stringify may not produce a CSV-friendly format. Consider handling nested structures more explicitly to ensure CSV compatibility.
| return ""; | ||
| } | ||
|
|
||
| if (!/[",\r\n]/.test(value)) { |
There was a problem hiding this comment.
[correctness]
The regex /[",\r\n]/ is used to determine if a CSV cell needs escaping. Ensure this covers all edge cases, such as leading/trailing spaces, which may also require escaping in some CSV parsers.
| return next.handle().pipe( | ||
| map((data) => { | ||
| const csv = this.csvSerializer.serialize(data); | ||
| response.setHeader("Content-Type", "text/csv; charset=utf-8"); |
There was a problem hiding this comment.
[💡 design]
Consider using response.setHeader('Content-Disposition', 'attachment; filename="data.csv"') to suggest a default filename when the CSV is downloaded. This can improve user experience when handling CSV responses.
| ); | ||
| } | ||
|
|
||
| private shouldReturnCsv(request: Request) { |
There was a problem hiding this comment.
[💡 maintainability]
The method shouldReturnCsv could be marked as private to explicitly indicate its intended scope and usage within the class.
| @@ -0,0 +1,353 @@ | |||
| export type ReportGroupKey = "challenges" | "sfdc" | "statistics" | "topcoder"; | |||
|
|
|||
| type HttpMethod = "GET"; | |||
There was a problem hiding this comment.
[💡 design]
Consider using a union type for HttpMethod that includes all possible HTTP methods (e.g., GET, POST, PUT, etc.) instead of just GET. This will make the code more flexible and future-proof if additional methods are needed.
| "Countries of all registrants for the specified challenge", | ||
| ), | ||
| report( | ||
| "Marathon Match Stats", |
There was a problem hiding this comment.
[❗❗ security]
The path /topcoder/mm-stats/:handle suggests a dynamic segment :handle. Ensure that the code handling this path properly parses and validates the handle parameter to prevent potential security issues such as path traversal or injection attacks.
| } from "./report-directory.data"; | ||
|
|
||
| @ApiTags("Reports") | ||
| @Controller() |
There was a problem hiding this comment.
[maintainability]
Consider specifying a base path for the @Controller() decorator to improve clarity and maintainability. This will help in organizing routes and making it clear which endpoints belong to this controller.
| @ApiTags("Topcoder Reports") | ||
| @ApiBearerAuth() | ||
| @UseGuards(TopcoderReportsGuard) | ||
| @UseInterceptors(CsvResponseInterceptor) |
There was a problem hiding this comment.
[correctness]
Ensure that the CsvResponseInterceptor correctly handles errors and edge cases when transforming responses to CSV format. This is important for maintaining data integrity and providing meaningful error messages to clients.
| return this.reports.getRegistrantCountries(challengeId); | ||
| } | ||
|
|
||
| @Get("/mm-stats/:handle") |
There was a problem hiding this comment.
[security]
Consider validating the handle parameter to ensure it meets expected formats or constraints. This can prevent potential issues with invalid data being processed.
No description provided.