[PROD RELEASE] - Q2 #72
Conversation
kkartunov
commented
Jun 17, 2025
kkartunov
commented
- Trolley going LIVE to production
- when admin resets return payment, make sure datePaid is set to null - do not handle processed events after payment was marked as "Returned" - add errors to failed payments
…-handling Update returned payment handling
…tatus PM-1206 - reset payment status
…ith-legacy PM-1201 - sync payments with legacy
refactor logging
PM-1257 - handle wipro users payout
…yment-setup PM-1263 - notify users via email about payment setup required
Fix mail link for payout setup notification
…ndpoint PM-1277 - update wallet endpoint
…ection PM-1279 - paypal fee collection
…-fee-data PM-1111 - process & store the challenge fee & markup for each payment
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| TopcoderEmailService, | ||
| ], | ||
| exports: [ | ||
| TopcoderChallengesService, |
There was a problem hiding this comment.
medium
maintainability
Consider whether TopcoderChallengesService should be exported. If it's not used outside this module, exporting it might unnecessarily expose internal functionality.
…draw-amount PM-1278 - return minWithdrawAmount in wallet details
| ...(taxWithholdingDetails ?? {}), | ||
| minWithdrawAmount: ENV_CONFIG.TROLLEY_MINIMUM_PAYMENT_AMOUNT ?? 0, |
There was a problem hiding this comment.
medium
correctness
Using the nullish coalescing operator (??) with ENV_CONFIG.TROLLEY_MINIMUM_PAYMENT_AMOUNT assumes that a value of 0 is acceptable. If 0 is not a valid minWithdrawAmount, consider explicitly checking for undefined or null to avoid unintended behavior.
| primaryCurrency?: string | null; | ||
| estimatedFees?: string | null; | ||
| taxWithholdingPercentage?: string | null; | ||
| minWithdrawAmount: number; |
There was a problem hiding this comment.
medium
correctness
Consider making minWithdrawAmount optional or providing a default value to ensure backward compatibility and prevent potential runtime errors if the value is not provided.
…escription-update PM-1304 - allow admin to update payment description
| @@ -164,19 +164,24 @@ export class AdminController { | |||
| @Body() body: WinningUpdateRequestDto, | |||
| @User() user: UserInfo, | |||
| ): Promise<ResponseDto<string>> { | |||
| if (!body.paymentAmount && !body.releaseDate && !body.paymentStatus) { | |||
| if ( | |||
There was a problem hiding this comment.
medium
correctness
The error message in the BadRequestException should match the condition being checked. Ensure that the message accurately reflects all fields being validated to avoid confusion for the API consumers.
src/api/admin/admin.service.ts
Outdated
| }, | ||
| updated_at: new Date(), | ||
| updated_by: userId, | ||
| version: version++, |
There was a problem hiding this comment.
high
correctness
The version variable is being incremented with version++, but this operation is not atomic. If this code is executed in a concurrent environment, it could lead to race conditions where multiple transactions might end up with the same version number. Consider using a more robust mechanism to handle versioning, such as database-level versioning or a transaction-safe increment operation.
| }) | ||
| @IsOptional() | ||
| @IsString() | ||
| description: string; |
There was a problem hiding this comment.
medium
security
Consider adding a length constraint to the description field to prevent excessively long input, which could impact performance or lead to unexpected behavior.
…n-withdrawing PM-1374 - require otp when withdrawing
|
|
||
| */ | ||
| -- AlterTable | ||
| ALTER TABLE "otp" DROP COLUMN "transaction_id"; |
There was a problem hiding this comment.
high
correctness
Dropping the transaction_id column from the otp table will result in data loss. Ensure that this data is no longer needed or has been backed up before proceeding.
| ALTER TABLE "otp" DROP COLUMN "transaction_id"; | ||
|
|
||
| -- DropTable | ||
| DROP TABLE "transaction"; |
There was a problem hiding this comment.
high
correctness
Dropping the transaction table will result in complete data loss for that table. Confirm that this data is backed up or no longer required before executing this migration.
| example: '123456', | ||
| }) | ||
| @Matches(/^[0-9]{6}$/) | ||
| @IsOptional() |
There was a problem hiding this comment.
medium
correctness
The @Matches(/^[0-9]{6}$/) decorator ensures the OTP is a 6-digit number, but the @IsOptional() decorator allows otpCode to be undefined. Consider whether this is the intended behavior, as it might lead to situations where the OTP is expected but not provided.
| ); | ||
| result.status = ResponseStatusType.SUCCESS; | ||
| body.otpCode, | ||
| )) as any; |
There was a problem hiding this comment.
medium
maintainability
Casting the response to any can lead to runtime errors if the expected structure of the response changes. Consider defining a specific type for the response to ensure type safety.
| result.status = ResponseStatusType.SUCCESS; | ||
| body.otpCode, | ||
| )) as any; | ||
| result.status = response?.error |
There was a problem hiding this comment.
medium
correctness
The use of optional chaining (response?.error) assumes that response can be null or undefined. Ensure that withdraw method can indeed return such values, or handle the case where response is unexpectedly null or undefined.
| WithdrawUpdateData, | ||
| } from 'src/shared/topcoder/challenges.service'; | ||
| import { TopcoderMembersService } from 'src/shared/topcoder/members.service'; | ||
| import { BasicMemberInfo, BASIC_MEMBER_FIELDS } from 'src/shared/topcoder'; |
There was a problem hiding this comment.
low
maintainability
The import of BASIC_MEMBER_FIELDS suggests that it might be used for specific operations. Ensure that it is utilized in the code to avoid unnecessary imports, which can lead to confusion and maintenance overhead.
| import { TopcoderMembersService } from 'src/shared/topcoder/members.service'; | ||
| import { BasicMemberInfo, BASIC_MEMBER_FIELDS } from 'src/shared/topcoder'; | ||
| import { Logger } from 'src/shared/global'; | ||
| import { OtpService } from 'src/shared/global/otp.service'; |
There was a problem hiding this comment.
high
security
The addition of OtpService import indicates a new dependency. Verify that this service is correctly integrated and used in the code to ensure that it does not introduce any security vulnerabilities, especially if it handles sensitive operations like OTP generation or validation.
| @@ -156,8 +186,12 @@ export class WithdrawalService { | |||
| userHandle: string, | |||
| winningsIds: string[], | |||
| paymentMemo?: string, | |||
| otpCode?: string, | |||
There was a problem hiding this comment.
high
security
The addition of the otpCode parameter suggests a potential security feature for withdrawal requests. Ensure that this parameter is validated and used securely within the method to prevent unauthorized access.
| ); | ||
| } | ||
|
|
||
| let userInfo: BasicMemberInfo; |
There was a problem hiding this comment.
medium
correctness
The type assertion as unknown as BasicMemberInfo is potentially unsafe and could lead to runtime errors if the structure of the object does not match BasicMemberInfo. Consider using a type guard or validation to ensure the object conforms to the expected type.
| userInfo, | ||
| reference_type.WITHDRAW_PAYMENT, | ||
| ); | ||
| return { error: otpError }; |
There was a problem hiding this comment.
medium
maintainability
Returning { error: otpError } directly might not provide enough context about the error. Consider including additional information or using a standardized error response format to improve error handling.
| ); | ||
|
|
||
| if (!otpResponse || otpResponse.code !== 'success') { | ||
| return { error: otpResponse }; |
There was a problem hiding this comment.
medium
maintainability
Returning { error: otpResponse } directly might not provide enough context about the error. Consider including additional information or using a standardized error response format to improve error handling.
|
|
||
| @IsNumber() | ||
| @IsOptional() | ||
| OTP_CODE_VALIDITY_MINUTES: number = 5; |
There was a problem hiding this comment.
medium
correctness
Consider validating that OTP_CODE_VALIDITY_MINUTES is within an acceptable range (e.g., greater than zero and not excessively large) to prevent potential misuse or misconfiguration.
| OTP_CODE_VALIDITY_MINUTES: number = 5; | ||
|
|
||
| @IsString() | ||
| SENDGRID_TEMPLATE_ID_OTP_CODE: string = 'd-2d0ab9f6c9cc4efba50080668a9c35c1'; |
There was a problem hiding this comment.
medium
correctness
Ensure that SENDGRID_TEMPLATE_ID_OTP_CODE is validated to match the expected format for SendGrid template IDs to avoid runtime errors.
| @@ -1,12 +1,15 @@ | |||
| import { Global, Module } from '@nestjs/common'; | |||
| import { PrismaService } from './prisma.service'; | |||
| import { TrolleyService } from './trolley.service'; | |||
| import { OtpService } from './otp.service'; | |||
| import { TopcoderModule } from '../topcoder/topcoder.module'; | |||
There was a problem hiding this comment.
medium
maintainability
Consider verifying that TopcoderModule is necessary to be imported globally. If it's only used in specific modules, importing it globally could lead to unnecessary dependencies and potential circular dependencies.
| import { TopcoderEmailService } from '../topcoder/tc-email.service'; | ||
| import { BasicMemberInfo } from '../topcoder'; | ||
|
|
||
| const generateRandomOtp = (length: number): string => { |
There was a problem hiding this comment.
high
security
Consider using a cryptographically secure random number generator, such as crypto.randomInt, to generate OTPs. This will enhance the security of the OTP generation process.
|
|
||
| // Simulate sending an email (replace with actual email service logic) | ||
| await this.tcEmailService.sendEmail( | ||
| email, |
There was a problem hiding this comment.
medium
maintainability
Ensure that the email sending logic handles potential errors gracefully. Consider adding error handling to manage cases where the email service fails to send the OTP.
| ) { | ||
| const record = await this.prisma.otp.findFirst({ | ||
| where: { | ||
| otp_hash: hashOtp(otpCode), |
There was a problem hiding this comment.
high
correctness
The OTP verification process should also check if the expiration_time is greater than the current time before proceeding with further checks. This will prevent unnecessary database updates for expired OTPs.
| async createEvent(topic: string, payload: any): Promise<void> { | ||
| this.logger.debug(`Sending message to bus topic ${topic}`, { | ||
| ...payload, | ||
| data: {}, |
There was a problem hiding this comment.
high
correctness
The addition of data: {} to the payload might unintentionally overwrite existing data properties in the payload. Ensure that this is the intended behavior, or consider using a more descriptive key to avoid potential data loss.
| firstName: string; | ||
| lastName: string; | ||
| email: string; | ||
| addresses: any[]; |
There was a problem hiding this comment.
medium
maintainability
Consider using a more specific type than any[] for addresses. This will improve type safety and maintainability by ensuring that the structure of the address objects is consistent and expected.
prisma/schema.prisma
Outdated
| @@ -26,19 +26,18 @@ model origin { | |||
| } | |||
|
|
|||
| model otp { | |||
| id String @id @default(dbgenerated("uuid_generate_v4()")) @db.Uuid | |||
| id String @id @default(dbgenerated("public.uuid_generate_v4()")) @db.Uuid | |||
There was a problem hiding this comment.
medium
correctness
The change from uuid_generate_v4() to public.uuid_generate_v4() specifies the schema explicitly, which is generally a good practice for clarity and avoiding conflicts. However, ensure that the public schema is always available and that this change aligns with your database setup and deployment scripts.
prisma/schema.prisma
Outdated
| @@ -81,7 +81,7 @@ model payment_release_associations { | |||
| } | |||
|
|
|||
| model payment_releases { | |||
| payment_release_id String @id @default(dbgenerated("uuid_generate_v4()")) @db.Uuid | |||
| payment_release_id String @id @default(dbgenerated("public.uuid_generate_v4()")) @db.Uuid | |||
There was a problem hiding this comment.
high
correctness
The change from uuid_generate_v4() to public.uuid_generate_v4() assumes that the public schema is always available and contains the uuid_generate_v4() function. Ensure that this schema is consistently available across all environments, as missing or misconfigured schemas could lead to runtime errors.
fix payment update
| }, | ||
| updated_at: new Date(), | ||
| updated_by: userId, | ||
| version, |
There was a problem hiding this comment.
high
correctness
The increment operation on version has been removed. If the intention was to maintain the current version without incrementing, ensure that this change aligns with the business logic and requirements. If the version should still be incremented, consider using version + 1 or another appropriate method to achieve this.
