Conversation
…e-proofing just to make sure
…ng prize is not USD
…heckpoints PM-2688 payments for checkpoints
…-winnings Pm 3242 support points winnings
Pm 3259 feedback
Updates to allow Task managers to create payments -> dev
Return "points" in wallet response
PM-3260 - add winnings category & type filters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| export enum Role { | ||
| Administrator = 'Administrator', | ||
| PaymentAdmin = 'Payment Admin', | ||
| PaymentBaAdmin = 'Payment BA Admin', |
There was a problem hiding this comment.
[💡 readability]
Consider using consistent naming conventions for enum values. The value PaymentBaAdmin uses an abbreviation (Ba) which might not be immediately clear to all developers. Consider using PaymentBusinessAdmin for clarity.
| }; | ||
|
|
||
| Object.keys(auth0User).forEach((key) => { | ||
| if (key.match(/\/roles$/gi) || key === 'roles' || key === 'role') { |
There was a problem hiding this comment.
[correctness]
Using key.match(/\/roles$/gi) may lead to unexpected matches if there are keys with similar suffixes. Consider using a more precise check or a stricter pattern to avoid false positives.
|
|
||
| Object.keys(auth0User).forEach((key) => { | ||
| if (key.match(/\/roles$/gi) || key === 'roles' || key === 'role') { | ||
| appendRoles(auth0User[key]); |
There was a problem hiding this comment.
[💡 maintainability]
The appendRoles function is defined inside collectUserRoles and is called multiple times. Consider extracting it to a separate method to improve readability and maintainability.
| } | ||
| if (typeof value === 'string' && value.trim()) { | ||
| value | ||
| .split(',') |
There was a problem hiding this comment.
[❗❗ correctness]
The method collectUserRoles is introduced but not shown in the diff. Ensure that it correctly replicates the previous logic of extracting roles from auth0User keys ending with /roles. Any deviation might lead to incorrect role extraction.
| return roles; | ||
| }, []); | ||
| const userRoles = this.collectUserRoles(auth0User); | ||
| const normalizedUserRoles = new Set( |
There was a problem hiding this comment.
[performance]
Using a Set for normalizedUserRoles is a good choice for performance when checking membership, but ensure that normalizeRole is deterministic and idempotent to avoid unexpected behavior.
| const normalizedUserRoles = new Set( | ||
| userRoles.map((role) => this.normalizeRole(role)), | ||
| ); | ||
| const normalizedRequiredRoles = requiredRoles.map((role) => |
There was a problem hiding this comment.
[performance]
Consider using a Set for normalizedRequiredRoles as well, to maintain consistency and potentially improve performance when checking for role inclusion.
| id: userId, | ||
| handle: userHandle, | ||
| email: request.email, | ||
| roles: userRoles, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that userRoles is properly validated and sanitized before being assigned to roles. This is important to prevent potential security issues such as injection attacks if userRoles originates from an untrusted source.
| Active: 'ACTIVE', | ||
| Draft: 'DRAFT', | ||
| Approved: 'APPROVED', | ||
| Canceled: 'CANCELLED', |
There was a problem hiding this comment.
[❗❗ correctness]
The spelling of 'Canceled' has been changed to 'Cancelled'. Ensure that this change is intentional and consistent across the codebase, as it may affect any logic or database entries that rely on the previous spelling.
| @@ -1,5 +1,6 @@ | |||
| import { ApiProperty } from '@nestjs/swagger'; | |||
| import { IsNumber, Min, IsInt, IsString, IsNotEmpty } from 'class-validator'; | |||
| import { IsNumber, Min, IsInt, IsString, IsNotEmpty, isEnum, IsEnum } from 'class-validator'; | |||
There was a problem hiding this comment.
[💡 maintainability]
The isEnum import is unused. Consider removing it to keep the imports clean and maintainable.
| example: 'USD', | ||
| }) | ||
| @IsString() | ||
| @IsEnum(PrizeType) |
There was a problem hiding this comment.
[❗❗ correctness]
The @IsEnum(PrizeType) decorator suggests that currency should be an enum type, but it is still declared as a string. This mismatch can lead to runtime errors or validation issues. Consider changing the type of currency to match the enum or adjust the validation decorator accordingly.
| unit: 'currency', | ||
| }, | ||
| { | ||
| type: 'POINTS', |
There was a problem hiding this comment.
[💡 readability]
Consider using a consistent naming convention for type values. The existing currency is lowercase, while the new POINTS is uppercase. Consistency improves readability and reduces potential errors.
| @IsOptional() | ||
| @IsEnum(WinningsCategory) | ||
| type?: WinningsCategory; | ||
| category?: WinningsCategory; |
There was a problem hiding this comment.
[❗❗ correctness]
Renaming type to category may affect existing code that relies on the type property. Ensure that all references to type are updated accordingly throughout the codebase to prevent runtime errors.
| }) | ||
| @IsOptional() | ||
| @IsEnum(WinningsType) | ||
| type?: WinningsType; |
There was a problem hiding this comment.
[design]
The introduction of the type property as WinningsType is a breaking change if this DTO is used in existing APIs. Ensure that consumers of this DTO are aware of this change and that it is documented appropriately.
| }) | ||
| @IsOptional() | ||
| @IsArray() | ||
| @ArrayNotEmpty() |
There was a problem hiding this comment.
[correctness]
The @ArrayNotEmpty() decorator ensures that the array is not empty, but the @IsOptional() decorator above allows billingAccounts to be undefined. Consider whether this is the intended behavior, as it might lead to confusion. If billingAccounts should be either undefined or a non-empty array, this is correct. Otherwise, you may want to adjust the decorators to reflect the intended validation logic.
| if (!baPrismaClient) { | ||
| if (!ENV_CONFIG.BILLING_ACCOUNTS_DB_URL) { | ||
| throw new Error( | ||
| 'BILLING_ACCOUNTS_DB_URL must be set for challenges Prisma client', |
There was a problem hiding this comment.
[💡 readability]
The error message should specify that it is related to the billing accounts database URL to avoid confusion with other potential environment variables.
| 'BILLING_ACCOUNTS_DB_URL must be set for challenges Prisma client', | ||
| ); | ||
| } | ||
| baPrismaClient = new BaPrismaClient({ |
There was a problem hiding this comment.
[maintainability]
Consider adding error handling for the BaPrismaClient instantiation to catch and log any potential errors during client creation. This will help in diagnosing issues if the client fails to initialize.
| @@ -1 +1,2 @@ | |||
| export * from './logger'; | |||
| export * from './ba-prisma.client'; | |||
There was a problem hiding this comment.
[maintainability]
Exporting everything from ./ba-prisma.client can lead to potential namespace conflicts or unintended exports. Consider explicitly exporting only the necessary modules to improve maintainability and avoid accidental exposure of internal modules.
…nt payments that get created as On Hold (Admin)
| (sum, payment) => sum + payment.details[0].totalAmount, | ||
| // compute USD totals for BA validation/locking (POINT payments are persisted but not billed) | ||
| const totalUsdAmount = payments.reduce( | ||
| (sum, payment) => |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding a check to ensure payment.details is not empty before accessing payment.details[0]. This will prevent potential runtime errors if payment.details is unexpectedly empty.
| error.message ?? error, | ||
| ); | ||
| if ( | ||
| error && |
There was a problem hiding this comment.
[❗❗ correctness]
The check for error being truthy is removed. Ensure that error is always defined before accessing error.message to avoid potential runtime errors.
|
|
||
| const payrollPayment = (body.attributes || {})['payroll'] === true; | ||
| const isPointsAward = body.category === WinningsCategory.POINTS_AWARD; | ||
| const requestedStatus = body.status as payment_status | undefined; |
There was a problem hiding this comment.
[❗❗ correctness]
Casting body.status to payment_status without validation could lead to runtime errors if body.status is not a valid payment_status. Consider validating the value before casting to ensure type safety.
| ? PaymentStatus.OWED | ||
| : PaymentStatus.ON_HOLD; | ||
| let resolvedStatus: payment_status; | ||
| if (requestedStatus) { |
There was a problem hiding this comment.
[❗❗ correctness]
The variable resolvedStatus is declared with the type payment_status, which is not defined in the provided diff. Ensure that payment_status is a valid type or enum. If it's a typo, it should be corrected to PaymentStatus.
| ); | ||
| resolvedStatus = PaymentStatus.PAID; | ||
| } else if (body.category === WinningsCategory.POINTS_AWARD) { | ||
| resolvedStatus = payment_status.CREDITED; |
There was a problem hiding this comment.
[❗❗ correctness]
The use of payment_status.CREDITED suggests that payment_status might be an incorrect reference. Ensure that payment_status is correctly defined or replace it with PaymentStatus if that was the intended enum.
| IsInt, | ||
| IsString, | ||
| IsNotEmpty, | ||
| isEnum, |
There was a problem hiding this comment.
[💡 maintainability]
The isEnum import is not used anywhere in the current diff. Consider removing it to avoid unnecessary imports, which can improve maintainability.
| required: false, | ||
| }) | ||
| @IsOptional() | ||
| @IsEnum(PaymentStatus) |
There was a problem hiding this comment.
[correctness]
Consider adding a default value for status to ensure consistent behavior when the property is not provided. This can prevent potential issues where the absence of a status might lead to unexpected behavior in the application logic.
| setupComplete: boolean; | ||
| }[] | ||
| >` | ||
| WITH u(user_id) AS ( |
There was a problem hiding this comment.
[❗❗ security]
Using Prisma.join with Prisma.sql for constructing SQL queries can potentially lead to SQL injection vulnerabilities if not handled correctly. Ensure that ids are properly sanitized or validated before being used in this query. Consider using parameterized queries to mitigate this risk.
jmgasper
changed the title
…n-user PM-3575 - use m2m to BA api to fetch user's billing accounts
| private readonly prisma: PrismaService, | ||
| private readonly paymentsService: PaymentsService, | ||
| private readonly tcChallengesService: TopcoderChallengesService, | ||
| private readonly baService: BillingAccountsService, |
There was a problem hiding this comment.
[maintainability]
The baService dependency is introduced but not used in the current diff. Ensure that this service is necessary for the class, or remove it to avoid unused dependencies.
|
|
||
| return { | ||
| ...filters, | ||
| billingAccounts: await this.baService.getBillingAccountsForUser(userId), |
There was a problem hiding this comment.
[❗❗ correctness]
The change from this.getBillingAccountsForUser(userId) to this.baService.getBillingAccountsForUser(userId) introduces a dependency on baService. Ensure that baService is properly initialized and injected into the AdminService class to avoid potential runtime errors.
| return; | ||
| } | ||
|
|
||
| const allowedBAs = await this.baService.getBillingAccountsForUser(userId); |
There was a problem hiding this comment.
[❗❗ correctness]
The method getBillingAccountsForUser has been moved from AdminService to baService. Ensure that baService is correctly initialized and available in this context to prevent runtime errors.
| 'BA admin user does not have access to the billing account for this winnings', | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
[💡 style]
There is an extra semicolon at the end of the line. While this does not affect functionality, it is unnecessary and could be removed for cleaner code.
| `Failed to fetch billing accounts for user '${userId}'!`, | ||
| err, | ||
| ); | ||
| throw new Error('Failed to fetch billing acccounts!'); |
There was a problem hiding this comment.
[💡 correctness]
The error message contains a typo: 'acccounts' should be 'accounts'.
| .m2mFetch< | ||
| { tcBillingAccountId: string }[] | ||
| >(`${TOPCODER_API_V6_BASE_URL}/billing-accounts/users/${userId}`) | ||
| .then((r) => r.map((b) => `${b.tcBillingAccountId}`)); |
There was a problem hiding this comment.
[readability]
Consider using await instead of .then() for consistency with the async/await pattern used in the rest of the method. This improves readability and makes error handling more straightforward.
| .then((r) => r.map((b) => `${b.tcBillingAccountId}`)); | ||
| } catch (err: any) { | ||
| this.logger.error( | ||
| err.response?.data?.result?.content ?? |
There was a problem hiding this comment.
[maintainability]
The error logging could be improved by providing more context about the error. Consider including the userId in the log message to aid in debugging.
4 participants
No description provided.