[PROD RELEASE] - Trolley #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}
| */ | ||
| -- 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.
| 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.
| 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
|
|
||
| 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
|
|
||
| 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.
PM-1304 - add audit trail when description gets updated
| : undefined, | ||
| }, | ||
| include: { | ||
| winnings: true, |
There was a problem hiding this comment.
high
security
Ensure that including winnings does not inadvertently expose sensitive information. Review the data model and access controls to confirm that this inclusion aligns with security and privacy requirements.
| }), | ||
| ); | ||
|
|
||
| if (payment.installment_number === 1) { |
There was a problem hiding this comment.
high
correctness
The condition payment.installment_number === 1 is used to determine when to add an audit entry. Ensure that this logic is correct and that it aligns with business requirements, as it may lead to missing audit entries for other installment numbers.
|
|
||
| if (payment.installment_number === 1) { | ||
| transactions.push((tx) => | ||
| this.addAudit( |
There was a problem hiding this comment.
medium
maintainability
Consider handling potential errors from this.addAudit to ensure that any issues during the audit logging do not affect the main transaction flow.
…on-event-handling Update revcipientverification event handling
| where: { trolley_id: payload.recipientId }, | ||
| }); | ||
|
|
||
| if (payload.type !== RecipientVerificationType.individual) { |
There was a problem hiding this comment.
high
correctness
The check for payload.type !== RecipientVerificationType.individual should be done after verifying that recipient exists. If recipient is null, the function should return early with an error log, as it currently does. This ensures that the function doesn't log misleading information about handling non-individual types when the recipient doesn't exist.
| individual = 'individual', | ||
| } | ||
|
|
||
| interface VerifiedIdentityData { |
There was a problem hiding this comment.
medium
correctness
Consider using Date type for dob to ensure date manipulation and validation are handled correctly.
| postalCodeMatch: boolean | null; | ||
| }; | ||
| ageWhenVerified: number; | ||
| documentValidFrom: string | null; |
There was a problem hiding this comment.
medium
correctness
Consider using Date type for documentValidFrom to ensure date manipulation and validation are handled correctly.
| }; | ||
| ageWhenVerified: number; | ||
| documentValidFrom: string | null; | ||
| documentValidUntil: string | null; |
There was a problem hiding this comment.
medium
correctness
Consider using Date type for documentValidUntil to ensure date manipulation and validation are handled correctly.
| } | ||
|
|
||
| interface VerifiedPhoneData { | ||
| phone: string; |
There was a problem hiding this comment.
high
correctness
Ensure that the phone field is validated for correct format and length, as incorrect phone numbers could lead to failed verifications.
| } | ||
|
|
||
| export interface RecipientVerificationStatusUpdateEventData { | ||
| type: RecipientVerificationType; |
There was a problem hiding this comment.
high
correctness
Changing type from string to RecipientVerificationType is a good improvement for type safety, but ensure that all existing usages of this field are updated accordingly to prevent runtime errors.
| decisionAt: string | null; | ||
| id: string; | ||
| reasonType: string | null; | ||
| verifiedData: VerifiedIdentityData | VerifiedPhoneData; |
There was a problem hiding this comment.
high
correctness
The change from a specific object structure to a union type (VerifiedIdentityData | VerifiedPhoneData) for verifiedData may introduce issues if these types are not mutually exclusive or if they do not cover all necessary fields. Ensure that both types are well-defined and that the consuming code can handle both types correctly.
3 participants
