Add new Member Payment Accrual report, and tweak Topgear hourly report based on their feedback #35
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,87 @@ | |||
| WITH provided_dates AS ( | |||
| SELECT | |||
| NULLIF($1, '')::timestamptz AS start_date, | |||
There was a problem hiding this comment.
[❗❗ correctness]
Casting user input directly to timestamptz without validation can lead to runtime errors if the input is not a valid timestamp. Consider adding input validation or error handling to ensure robustness.
| ON cb."challengeId" = c."id" | ||
| LEFT JOIN "billing-accounts"."BillingAccount" ba | ||
| ON ba."id" = COALESCE( | ||
| NULLIF(rp.billing_account, '')::int, |
There was a problem hiding this comment.
[correctness]
Using NULLIF and COALESCE to handle potential null or empty values is good, but ensure that the ::int cast will not fail if cb."billingAccountId" is not a valid integer. Consider adding validation or error handling for this case.
| LEFT JOIN finance.payment_method pm | ||
| ON pm.payment_method_id = rp.payment_method_id | ||
| LEFT JOIN members.member mem | ||
| ON mem."userId"::text = rp.winner_id |
There was a problem hiding this comment.
[correctness]
Casting mem."userId" to text for comparison with rp.winner_id assumes that rp.winner_id is always a valid string representation of a user ID. Ensure that this assumption holds or add validation to prevent potential mismatches.
| AND cb."billingAccountId" = '80000062' | ||
| ) ba ON TRUE | ||
| WHERE c."createdAt" >= now() - interval '4 months' | ||
| WHERE c."updatedAt" >= now() - interval '100 days' |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the interval from '4 months' to '100 days' alters the time window for selecting challenges. Ensure this change aligns with the business requirements, as it could affect the data included in the report.
| WHERE bc.billing_account_id = '80000062' | ||
| AND bc."createdAt" >= now() - interval '4 months' | ||
| ORDER BY bc."createdAt" DESC; | ||
| AND (pd.latest_actual_end_date >= now() - interval '100 days' |
There was a problem hiding this comment.
[correctness]
The condition (pd.latest_actual_end_date >= now() - interval '100 days' OR bc.status='ACTIVE') may include challenges that are not relevant if pd.latest_actual_end_date is NULL. Consider verifying if this logic meets the intended criteria for filtering challenges.
| ORDER BY bc."createdAt" DESC; | ||
| AND (pd.latest_actual_end_date >= now() - interval '100 days' | ||
| OR bc.status='ACTIVE') | ||
| ORDER BY bc."updatedAt" DESC; |
There was a problem hiding this comment.
[correctness]
The order by clause was changed from bc."createdAt" DESC to bc."updatedAt" DESC. This change will affect the sorting of the results. Ensure this change is intentional and aligns with the report's requirements.
|
|
||
| const logger = new Logger("AuthMiddleware"); | ||
|
|
||
| function resolveAuthorizationHeader(headers: Record<string, unknown>): string { |
There was a problem hiding this comment.
[correctness]
The function resolveAuthorizationHeader returns an empty string if no valid authorization header is found. Consider returning null instead to clearly indicate the absence of a valid header, which can help prevent potential misuse of an empty string as a valid token.
|
|
||
| use(req: any, res: Response, next: NextFunction) { | ||
| if (req.headers.authorization) { | ||
| const authorizationHeader = resolveAuthorizationHeader(req.headers ?? {}); |
There was a problem hiding this comment.
[💡 style]
The use of req.headers ?? {} is unnecessary because req.headers is always defined as an object in Express. Consider simplifying this to req.headers.
| "Member Payment Accrual", | ||
| "/topcoder/member-payment-accrual", | ||
| "Member payment accruals for the provided date range (defaults to last 3 months)", | ||
| [paymentsStartDateParam, paymentsEndDateParam], |
There was a problem hiding this comment.
[correctness]
Consider adding validation to ensure that paymentsStartDateParam and paymentsEndDateParam are provided in a valid date format and that the start date is not after the end date. This will prevent potential errors when querying the report.
| return REPORTS_DIRECTORY; | ||
| } | ||
|
|
||
| @Get("/directory") |
There was a problem hiding this comment.
[maintainability]
The new endpoint /directory is an alias for the existing / endpoint. Consider whether this duplication is necessary, as it could lead to maintenance challenges if the logic for these endpoints diverges in the future.
| } | ||
|
|
||
| @Get("/30-day-payments") | ||
| @Get("/member-payment-accrual") |
There was a problem hiding this comment.
[❗❗ design]
The endpoint path /member-payment-accrual is a breaking change from the previous /30-day-payments. Ensure that any clients consuming this API are updated accordingly, or consider maintaining backward compatibility if needed.
| }) | ||
| get30DayPayments() { | ||
| return this.reports.get30DayPayments(); | ||
| getMemberPaymentAccrual(@Query() query: MemberPaymentAccrualQueryDto) { |
There was a problem hiding this comment.
[correctness]
Consider adding validation for startDate and endDate in MemberPaymentAccrualQueryDto to ensure they are valid dates and startDate is not after endDate. This will prevent potential errors or unexpected behavior in the report generation.
| const rows = await this.db.query<ThirtyDayPaymentRow>(query); | ||
| async getMemberPaymentAccrual( | ||
| startDate?: string, | ||
| endDate?: string, |
There was a problem hiding this comment.
[correctness]
The endDate parameter is optional, but the SQL query might behave unexpectedly if null values are passed. Ensure the SQL handles null values appropriately or validate the input before querying.
| customerName: row.customer_name ?? null, | ||
| reportingAccountName: row.reporting_account_name ?? null, | ||
| memberId: row.member_id ?? null, | ||
| challengeCreatedAt: row.challenge_created_date ?? null, |
There was a problem hiding this comment.
[💡 maintainability]
Consider normalizing challenge_created_date using this.normalizeDate for consistency with other date fields.
https://topcoder.atlassian.net/browse/PS-481
https://topcoder.atlassian.net/browse/PS-475