[PROD] - Payment Status UI fixes -> master #127
Conversation
…yment changes & payment reconciliation
PS-469 - add more logs
…liation PS-469 - update query for user payment reconciliation
Fix ba consume
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| this.logger.log( | ||
| `updateWinnings called by ${userId} for winningsId=${winningsId}`, | ||
| ); | ||
| this.logger.log(`updateWinnings payload: ${JSON.stringify(body)}`); |
There was a problem hiding this comment.
[❗❗ security]
Logging the entire body object with JSON.stringify could potentially expose sensitive information. Consider logging only non-sensitive fields or using a redaction mechanism.
| this.logger.warn( | ||
| `Attempt to update cancelled payment ${payment.payment_id} — rejecting`, | ||
| ); | ||
| throw new BadRequestException('cannot update cancelled winnings'); |
There was a problem hiding this comment.
[❗❗ correctness]
Throwing an exception inside a forEach loop may not behave as expected because forEach does not handle exceptions in a way that stops execution. Consider using a for...of loop or Array.prototype.some/Array.prototype.every for better control over flow.
| }), | ||
| ); | ||
| queuedActions.push( | ||
| `update description -> "${body.description}" (version ${version})`, |
There was a problem hiding this comment.
[❗❗ security]
The string interpolation used here assumes that body.description and version are safe to include directly in the log message. Ensure that these variables are sanitized or validated to prevent potential injection attacks or logging of sensitive information.
| tx, | ||
| ), | ||
| ); | ||
| queuedActions.push('add audit for description change'); |
There was a problem hiding this comment.
[correctness]
The string 'add audit for description change' is added to the queuedActions array. Ensure that this action is handled appropriately later in the code to avoid potential issues with unprocessed actions.
| payment.payment_status !== PaymentStatus.ON_HOLD_ADMIN && | ||
| payment.payment_status !== PaymentStatus.PAID | ||
| ) { | ||
| this.logger.warn( |
There was a problem hiding this comment.
[maintainability]
Consider using structured logging to include more context, such as user ID or request ID, to make it easier to trace this warning in logs.
|
|
||
| default: | ||
| this.logger.warn( | ||
| `Invalid payment status provided: ${body.paymentStatus}`, |
There was a problem hiding this comment.
[security]
Consider sanitizing or validating body.paymentStatus before logging it to prevent potential logging of sensitive or malformed data.
| payment.payment_status === PaymentStatus.PROCESSING | ||
| ) { | ||
| this.logger.warn( | ||
| `Rejected status change for ${payment.payment_id}: ${errMessage}`, |
There was a problem hiding this comment.
[maintainability]
Consider including more context in the log message, such as the user ID or request ID, to aid in debugging and traceability.
| ), | ||
| ); | ||
| queuedActions.push( | ||
| `update status ${payment.payment_status} -> ${body.paymentStatus}`, |
There was a problem hiding this comment.
[maintainability]
Consider using a structured logging approach instead of string interpolation for queuedActions.push. This can help avoid potential issues with special characters in payment.payment_status or body.paymentStatus and improve log parsing.
| tx, | ||
| ), | ||
| ); | ||
| queuedActions.push('add audit for status change'); |
There was a problem hiding this comment.
[maintainability]
Consider using a constant or an enum for the action string 'add audit for status change' to avoid potential typos and to improve maintainability.
| PaymentStatus.ON_HOLD_ADMIN, | ||
| ].includes(paymentStatus) | ||
| ) { | ||
| this.logger.warn( |
There was a problem hiding this comment.
[maintainability]
Consider using a more specific log level, such as error, instead of warn for this situation, as it involves throwing an exception. This can help in distinguishing between recoverable warnings and critical errors in the logs.
| ), | ||
| ); | ||
| queuedActions.push( | ||
| `update release_date ${payment.release_date?.toISOString()} -> ${newReleaseDate.toISOString()}`, |
There was a problem hiding this comment.
[❗❗ correctness]
Using optional chaining (?.) on payment.release_date might lead to a TypeError if newReleaseDate is undefined. Ensure newReleaseDate is always defined or handle the case where it might be undefined.
| tx, | ||
| ), | ||
| ); | ||
| queuedActions.push('add audit for release date change'); |
There was a problem hiding this comment.
[maintainability]
Consider using a constant or an enum for the action string 'add audit for release date change' to avoid potential typos and improve maintainability.
| ); | ||
|
|
||
| queuedActions.push( | ||
| `update amounts -> ${body.paymentAmount.toFixed(2)} (installment 1)`, |
There was a problem hiding this comment.
[correctness]
The use of toFixed(2) for formatting the payment amount might lead to rounding issues. Consider using a more precise method for handling currency values, such as a dedicated library for financial calculations, to avoid potential inaccuracies.
| for (const transaction of transactions) { | ||
| await transaction(tx); | ||
| for (let i = 0; i < transactions.length; i++) { | ||
| this.logger.log(`Executing transaction ${i + 1}/${transactions.length}`); |
There was a problem hiding this comment.
[💡 readability]
Consider using a forEach loop instead of a for loop for iterating over transactions. This can improve readability by eliminating the need for manual index management.
|
|
||
| if (winning?.winner_id) { | ||
| this.logger.log( | ||
| `Triggering payments reconciliation for user ${winning.winner_id}`, |
There was a problem hiding this comment.
[💡 maintainability]
Consider using structured logging to include winning.winner_id as a separate field rather than interpolating it into the message string. This can improve log searchability and analysis.
| ); | ||
| await this.paymentsService.reconcileUserPayments(winning.winner_id); | ||
| this.logger.log( | ||
| `Reconciliation triggered for user ${winning.winner_id}`, |
There was a problem hiding this comment.
[💡 maintainability]
Consider using structured logging to include winning.winner_id as a separate field rather than interpolating it into the message string. This can improve log searchability and analysis.
| ); | ||
| } else { | ||
| this.logger.warn( | ||
| `Needs reconciliation but no winner_id found for winningsId=${winningsId}`, |
There was a problem hiding this comment.
[💡 maintainability]
Consider using structured logging to include winningsId as a separate field rather than interpolating it into the message string. This can improve log searchability and analysis.
|
|
||
| result.data = 'Successfully updated winnings'; | ||
| this.logger.log( | ||
| `updateWinnings completed for winningsId=${winningsId}: ${result.data}`, |
There was a problem hiding this comment.
[💡 maintainability]
Consider using structured logging to include winningsId as a separate field rather than interpolating it into the message string. This can improve log searchability and analysis.
| error instanceof BadRequestException | ||
| ) { | ||
| this.logger.warn( | ||
| `updateWinnings validation error for winningsId=${winningsId}: ${error.message}`, |
There was a problem hiding this comment.
[💡 maintainability]
Consider using structured logging to include winningsId and error.message as separate fields rather than interpolating them into the message string. This can improve log searchability and analysis.
| this.logger.log(`Lock acquired for challenge ${challenge.id}`); | ||
| } catch (err: any) { | ||
| if (err.code === 'P2002') { | ||
| this.logger.log(`Challenge Lock already acquired for ${challenge.id}`); |
There was a problem hiding this comment.
[maintainability]
Consider using logger.error instead of logger.log for logging the error case where the challenge lock is already acquired. This will help in distinguishing error logs from informational logs.
| `Challenge Lock already acquired for ${challenge.id}`, | ||
| ); | ||
| } | ||
| this.logger.error( |
There was a problem hiding this comment.
[💡 maintainability]
Consider using this.logger.warn instead of this.logger.error for logging the failure to acquire a lock, as it might not be an error condition but rather a normal occurrence in concurrent environments.
| this.logger.log(`Payment creation completed for challenge ${challenge.id}`); | ||
| } catch (error) { | ||
| if (error.message.includes('Lock already acquired')) { | ||
| this.logger.error( |
There was a problem hiding this comment.
[❗❗ security]
Ensure that sensitive information is not logged in error.message. Consider sanitizing or filtering the message before logging to prevent potential exposure of sensitive data.
| await this.prisma.challenge_lock | ||
| .deleteMany({ | ||
| try { | ||
| const result = await this.prisma.challenge_lock.deleteMany({ |
There was a problem hiding this comment.
[correctness]
Consider checking if challenge.id is defined before using it in the where clause. This will prevent potential runtime errors if challenge.id is undefined or null.
| // swallow errors if lock was already released | ||
| }); | ||
| this.logger.log( | ||
| `Released lock for challenge ${challenge.id}. Rows deleted: ${result.count}`, |
There was a problem hiding this comment.
[security]
Ensure that this.logger.log is appropriately configured to handle potentially sensitive information. Logging challenge IDs might expose sensitive data if not handled correctly.
| // swallow errors if lock was already released but log for observability | ||
| this.logger.error( | ||
| `Failed to release lock for challenge ${challenge.id}`, | ||
| releaseErr.message ?? releaseErr, |
There was a problem hiding this comment.
[💡 maintainability]
Consider adding more context to the error message being logged. Including additional details such as the stack trace or error code could aid in debugging.
| recipient: trolley_recipient, | ||
| taxFormData: TaxFormStatusUpdatedEventData, | ||
| ) { | ||
| this.logger.log( |
There was a problem hiding this comment.
[maintainability]
Consider using structured logging instead of string interpolation for logging. This can improve log searchability and parsing in log management systems.
| this.logger.log( | ||
| `Tax form '${taxFormId}' marked Voided — removing association(s) for user '${recipient.user_id}'`, | ||
| ); | ||
| const result = await this.prisma.user_tax_form_associations.deleteMany({ |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the deleteMany operation. If this operation fails, it could leave the system in an inconsistent state, especially since it involves database modifications.
| this.logger.log( | ||
| `Creating tax form association for user '${recipient.user_id}' taxFormId '${taxFormId}'`, | ||
| ); | ||
| const created = await this.prisma.user_tax_form_associations.create({ |
There was a problem hiding this comment.
[❗❗ correctness]
Consider adding error handling for the create operation. Without proper error handling, failures in this database operation could lead to unexpected behavior or data inconsistencies.
| }, | ||
| }); | ||
| this.logger.log( | ||
| `Created association id='${created.id}' tax_form_status='${created.tax_form_status}'`, |
There was a problem hiding this comment.
[security]
Consider using parameterized logging to prevent potential log injection vulnerabilities. For example, use this.logger.log('Created association', { id: created.id, tax_form_status: created.tax_form_status });.
| return this.prisma.user_tax_form_associations.update({ | ||
| where: { id: existingFormAssociation?.id }, | ||
| this.logger.log( | ||
| `Updating association id='${existingFormAssociation.id}' for user '${recipient.user_id}'`, |
There was a problem hiding this comment.
[security]
Consider using parameterized logging to prevent potential log injection vulnerabilities. For example, use this.logger.log('Updating association', { id: existingFormAssociation.id, user_id: recipient.user_id });.
| }, | ||
| }); | ||
| this.logger.log( | ||
| `Updated association id='${updated.id}' tax_form_status='${updated.tax_form_status}'`, |
There was a problem hiding this comment.
[security]
Consider using parameterized logging to prevent potential log injection vulnerabilities. For example, use this.logger.log('Updated association', { id: updated.id, tax_form_status: updated.tax_form_status });.
|
|
||
| @Injectable() | ||
| export class PaymentsService { | ||
| private readonly logger = new Logger(PaymentsService.name); |
There was a problem hiding this comment.
[maintainability]
Consider ensuring that the Logger class is correctly configured to handle different logging levels and outputs. This can help in maintaining clarity and control over the logging behavior, especially in production environments.
| }[] = []; | ||
|
|
||
| if (userIds.length > 0) { | ||
| const ids = uniq(userIds); |
There was a problem hiding this comment.
[correctness]
The use of uniq to filter userIds is a good approach to ensure no duplicate IDs are processed. However, ensure that uniq is imported from the correct utility library (e.g., Lodash) and that it handles the input as expected.
| >` | ||
| >` | ||
| WITH u(user_id) AS ( | ||
| VALUES ${Prisma.join(ids.map((id) => Prisma.sql`(${id})`))} |
There was a problem hiding this comment.
[❗❗ security]
The use of Prisma.join and Prisma.sql for constructing the SQL query is correct, but ensure that ids.map((id) => Prisma.sql(${id})) properly sanitizes the input to prevent SQL injection. Double-check that Prisma.sql is used correctly for parameterized queries.
| LEFT JOIN user_tax_form_associations utx ON upm.user_id = utx.user_id AND utx.tax_form_status = 'ACTIVE' | ||
| LEFT JOIN user_identity_verification_associations uiv ON upm.user_id = uiv.user_id | ||
| WHERE upm.user_id IN (${Prisma.join(uniq(userIds))}) | ||
| FROM u |
There was a problem hiding this comment.
[❗❗ correctness]
The alias u for the table is not defined in the FROM clause. Ensure that the table u is correctly defined or replace it with the appropriate table name.
| )}`, | ||
| ); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
[maintainability]
Consider adding specific error handling for known error types to provide more detailed logging and potentially recover from certain errors without throwing. This can improve the robustness of the service.
| interface LockAmountDTO { | ||
| challengeId: string; | ||
| lockAmount: number; | ||
| amount: number; |
There was a problem hiding this comment.
[💡 readability]
Renaming lockAmount to amount could lead to confusion if the context of 'locking' is important for understanding the code. Consider whether this change might reduce clarity in the context where this DTO is used.
| challengeId: string; | ||
| consumeAmount: number; | ||
| markup?: number; | ||
| amount: number; |
There was a problem hiding this comment.
[💡 readability]
Renaming consumeAmount to amount may reduce clarity if the distinction between 'consuming' and other operations is significant. Ensure that this change does not obscure the purpose of this DTO in its usage context.
| { | ||
| method: 'PATCH', | ||
| body: JSON.stringify({ param: dto }), | ||
| body: JSON.stringify(dto), |
There was a problem hiding this comment.
[❗❗ correctness]
The change from JSON.stringify({ param: dto }) to JSON.stringify(dto) alters the structure of the request payload. Ensure that the receiving API endpoint is expecting the new payload structure. This change could potentially break the integration if the API expects the param wrapper.
| ); | ||
| throw new Error( | ||
| `Budget Error: Requested amount $${dto.lockAmount} exceeds available budget for Billing Account #${billingAccountId}. | ||
| `Budget Error: Requested amount $${dto.amount} exceeds available budget for Billing Account #${billingAccountId}. |
There was a problem hiding this comment.
[❗❗ correctness]
The variable dto.amount is used here, replacing dto.lockAmount. Ensure that dto.amount is correctly initialized and validated earlier in the code to prevent potential runtime errors or incorrect error messages.
| { | ||
| method: 'PATCH', | ||
| body: JSON.stringify({ param: dto }), | ||
| body: JSON.stringify(dto), |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the body from JSON.stringify({ param: dto }) to JSON.stringify(dto) alters the structure of the request payload. Ensure that the API endpoint is expecting the new format, as this change could lead to unexpected behavior if the API is not updated accordingly.
| await this.lockAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, | ||
| lockAmount: | ||
| amount: |
There was a problem hiding this comment.
[❗❗ correctness]
The parameter name change from lockAmount to amount should be verified to ensure that it aligns with the expected parameter names in the lockAmount function. If the function signature or the consuming code expects lockAmount, this change could lead to runtime errors.
| await this.consumeAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, | ||
| consumeAmount: | ||
| amount: |
There was a problem hiding this comment.
[❗❗ correctness]
The parameter name change from consumeAmount to amount should be verified across all usages of consumeAmount to ensure consistency and correctness throughout the codebase.
| await this.lockAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, | ||
| lockAmount: rollback ? prevAmount : 0, | ||
| amount: rollback ? prevAmount : 0, |
There was a problem hiding this comment.
[❗❗ correctness]
The parameter name change from lockAmount to amount should be verified against the lockAmount method's expected parameter names. Ensure that this change does not break the method's functionality or any related logic.
| method: (finalOptions.method ?? 'GET'), | ||
| status: response.status, | ||
| statusText: response.statusText, | ||
| requestBody: (finalOptions as any).body, |
There was a problem hiding this comment.
[maintainability]
Consider using a more specific type than any for finalOptions to improve type safety and maintainability. This will help catch potential errors at compile time.
| responseBody = text; | ||
| } | ||
| } catch (e) { | ||
| responseBody = `Failed to read response body: ${e?.message ?? e}`; |
There was a problem hiding this comment.
[correctness]
The error message construction using e?.message ?? e assumes e is an object with a message property, which might not always be the case. Consider using instanceof Error to check if e is an Error object before accessing e.message.
https://topcoder.atlassian.net/browse/PS-469