Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 91 additions & 2 deletions src/api/admin/admin.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,34 +79,58 @@ export class AdminService {

let needsReconciliation = false;
const winningsId = body.winningsId;
this.logger.log(
`updateWinnings called by ${userId} for winningsId=${winningsId}`,
);
this.logger.log(`updateWinnings payload: ${JSON.stringify(body)}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.


try {
const payments = await this.getPaymentsByWinningsId(
winningsId,
body.paymentId,
);

this.logger.log(
`Found ${payments.length} payment(s) for winningsId=${winningsId}`,
);
if (payments.length === 0) {
this.logger.warn(
`No payments found for winningsId=${winningsId}, paymentId=${body.paymentId}`,
);
throw new NotFoundException('failed to get current payments');
}

let releaseDate;
if (body.paymentStatus) {
releaseDate = await this.getPaymentReleaseDateByWinningsId(winningsId);
this.logger.log(
`Payment release date for winningsId=${winningsId}: ${releaseDate}`,
);
}

const transactions: ((
tx: Prisma.TransactionClient,
) => Promise<unknown>)[] = [];
const now = new Date().getTime();

// iterate payments and build transaction list
payments.forEach((payment) => {
this.logger.log(
`Processing payment ${payment.payment_id} (installment ${payment.installment_number}) with current status=${payment.payment_status}`,
);

if (
payment.payment_status &&
payment.payment_status === PaymentStatus.CANCELLED
) {
this.logger.warn(
`Attempt to update cancelled payment ${payment.payment_id} — rejecting`,
);
throw new BadRequestException('cannot update cancelled winnings');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.

}

let version = payment.version ?? 1;
const queuedActions: string[] = [];

if (body.description) {
transactions.push((tx) =>
Expand All @@ -129,6 +153,9 @@ export class AdminService {
},
}),
);
queuedActions.push(
`update description -> "${body.description}" (version ${version})`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.

);

if (payment.installment_number === 1) {
transactions.push((tx) =>
Expand All @@ -140,6 +167,7 @@ export class AdminService {
tx,
),
);
queuedActions.push('add audit for description change');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

}
}

Expand Down Expand Up @@ -171,6 +199,9 @@ export class AdminService {
payment.payment_status !== PaymentStatus.ON_HOLD_ADMIN &&
payment.payment_status !== PaymentStatus.PAID
) {
this.logger.warn(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

`Invalid attempt to set OWED for payment ${payment.payment_id} when not on hold admin or paid`,
);
throw new BadRequestException(
"cannot put a payment back to owed unless it is on hold by an admin, or it's been paid",
);
Expand All @@ -180,13 +211,19 @@ export class AdminService {
break;

default:
this.logger.warn(
`Invalid payment status provided: ${body.paymentStatus}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ security]
Consider sanitizing or validating body.paymentStatus before logging it to prevent potential logging of sensitive or malformed data.

);
throw new BadRequestException('invalid payment status provided');
}

if (
errMessage &&
payment.payment_status === PaymentStatus.PROCESSING
) {
this.logger.warn(
`Rejected status change for ${payment.payment_id}: ${errMessage}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ maintainability]
Consider including more context in the log message, such as the user ID or request ID, to aid in debugging and traceability.

);
throw new BadRequestException(errMessage);
}

Expand All @@ -201,11 +238,17 @@ export class AdminService {
tx,
),
);
queuedActions.push(
`update status ${payment.payment_status} -> ${body.paymentStatus}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

);

paymentStatus = body.paymentStatus as PaymentStatus;

if (body.paymentStatus === PaymentStatus.OWED) {
needsReconciliation = true;
this.logger.log(
`Payment ${payment.payment_id} marked OWED; will trigger reconciliation later`,
);
}

if (payment.installment_number === 1) {
Expand All @@ -218,6 +261,7 @@ export class AdminService {
tx,
),
);
queuedActions.push('add audit for status change');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

}
}

Expand All @@ -232,6 +276,9 @@ export class AdminService {
PaymentStatus.ON_HOLD_ADMIN,
].includes(paymentStatus)
) {
this.logger.warn(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

`Cannot update release date for payment ${payment.payment_id} in status ${paymentStatus}`,
);
throw new BadRequestException(
`Cannot update release date for payment unless it's in one of the states: ${[
PaymentStatus.OWED,
Expand All @@ -251,6 +298,9 @@ export class AdminService {
tx,
),
);
queuedActions.push(
`update release_date ${payment.release_date?.toISOString()} -> ${newReleaseDate.toISOString()}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.

);

if (payment.installment_number === 1) {
transactions.push((tx) =>
Expand All @@ -262,6 +312,7 @@ export class AdminService {
tx,
),
);
queuedActions.push('add audit for release date change');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

}
}

Expand Down Expand Up @@ -297,6 +348,10 @@ export class AdminService {
tx,
),
);

queuedActions.push(
`update amounts -> ${body.paymentAmount.toFixed(2)} (installment 1)`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

);
} else {
transactions.push((tx) =>
this.updatePaymentAmount(
Expand All @@ -310,17 +365,35 @@ export class AdminService {
tx,
),
);
queuedActions.push(
`update amounts -> total ${body.paymentAmount.toFixed(2)} (installment ${payment.installment_number})`,
);
}
}

this.logger.log(
`Queued ${queuedActions.length} action(s) for payment ${payment.payment_id}: ${queuedActions.join(
' ; ',
)}`,
);
});

this.logger.log(
`Executing ${transactions.length} transaction step(s) for winningsId=${winningsId}`,
);

// Run all transaction tasks in a single prisma transaction
await this.prisma.$transaction(async (tx) => {
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}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

await transactions[i](tx);
}
});

this.logger.log(
`Successfully executed transactions for winningsId=${winningsId}`,
);

if (needsReconciliation) {
const winning = await this.prisma.winnings.findFirst({
select: {
Expand All @@ -332,16 +405,32 @@ export class AdminService {
});

if (winning?.winner_id) {
this.logger.log(
`Triggering payments reconciliation for user ${winning.winner_id}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

);
} catch (error) {
if (
error instanceof NotFoundException ||
error instanceof BadRequestException
) {
this.logger.warn(
`updateWinnings validation error for winningsId=${winningsId}: ${error.message}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

);
throw error;
}
this.logger.error('Updating winnings failed', error);
Expand Down
43 changes: 37 additions & 6 deletions src/api/challenges/challenges.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -470,25 +470,34 @@ export class ChallengesService {

async generateChallengePayments(challengeId: string, userId: string) {
const challenge = await this.getChallenge(challengeId);
this.logger.log(`Fetched challenge ${challengeId}`);

if (!challenge) {
this.logger.error(`Challenge not found: ${challengeId}`);
throw new Error('Challenge not found!');
}

this.logger.log(`Challenge ${challenge.id} - "${challenge.name}" with status "${challenge.status}" retrieved`);

const allowedStatuses = [
ChallengeStatuses.Completed.toLowerCase(),
ChallengeStatuses.CancelledFailedReview.toLowerCase(),
];

if (!allowedStatuses.includes(challenge.status.toLowerCase())) {
this.logger.error(
`Challenge ${challenge.id} isn't in a payable status: ${challenge.status}`,
);
throw new Error("Challenge isn't in a payable status!");
}

// need to read for update (LOCK the rows)
this.logger.log(`Attempting to acquire lock for challenge ${challenge.id}`);
try {
await this.prisma.challenge_lock.create({
data: { external_id: challenge.id },
});
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}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

Expand All @@ -497,27 +506,49 @@ export class ChallengesService {
`Challenge Lock already acquired for ${challenge.id}`,
);
}
this.logger.error(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

`Failed to acquire lock for challenge ${challenge.id}`,
err.message ?? err,
);
throw err;
}

try {
this.logger.log(`Starting payment creation for challenge ${challenge.id}`);
await this.createPayments(challenge, userId);
this.logger.log(`Payment creation completed for challenge ${challenge.id}`);
} catch (error) {
if (error.message.includes('Lock already acquired')) {
this.logger.error(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[❗❗ 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.

`Error while creating payments for challenge ${challenge.id}`,
error.message ?? error,
);
if (
error &&
(typeof error.message === 'string') &&
error.message.includes('Lock already acquired')
) {
this.logger.log(`Conflict detected while creating payments for ${challenge.id}`);
throw new ConflictException(
'Another payment operation is in progress.',
);
} else {
throw error;
}
} finally {
await this.prisma.challenge_lock
.deleteMany({
try {
const result = await this.prisma.challenge_lock.deleteMany({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

where: { external_id: challenge.id },
})
.catch(() => {
// swallow errors if lock was already released
});
this.logger.log(
`Released lock for challenge ${challenge.id}. Rows deleted: ${result.count}`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[⚠️ 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.

);
} catch (releaseErr) {
// 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,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[💡 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.

);
}
}
}
}
Loading
Loading