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
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}`);
Expand All @@ -497,27 +506,49 @@ export class ChallengesService {
`Challenge Lock already acquired for ${challenge.id}`,
);
}
this.logger.error(
`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(
`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({
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}`,
);
} 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,
);
}
}
}
}
18 changes: 8 additions & 10 deletions src/shared/topcoder/billing-accounts.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ const { TOPCODER_API_V6_BASE_URL, TGBillingAccounts } = ENV_CONFIG;

interface LockAmountDTO {
challengeId: string;
lockAmount: number;
amount: number;
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The renaming of lockAmount to amount could lead to confusion if the context of its usage is not clear. Ensure that the change is consistently applied throughout the codebase and that the new name accurately reflects its purpose in all contexts.

}
interface ConsumeAmountDTO {
challengeId: string;
consumeAmount: number;
markup?: number;
amount: number;
Copy link

Choose a reason for hiding this comment

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

[💡 readability]
The renaming of consumeAmount to amount might reduce clarity if the distinction between different types of amounts is important. Consider whether a more descriptive name would better convey the intended use in this context.

}

export interface BAValidation {
Expand All @@ -40,7 +39,7 @@ export class BillingAccountsService {
`${TOPCODER_API_V6_BASE_URL}/billing-accounts/${billingAccountId}/lock-amount`,
{
method: 'PATCH',
body: JSON.stringify({ param: dto }),
body: JSON.stringify(dto),
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
Changing JSON.stringify({ param: dto }) to JSON.stringify(dto) modifies the structure of the request body. Ensure that the API endpoint expects the new format, as this change could lead to unexpected behavior if the API expects the param wrapper.

},
);
} catch (err: any) {
Expand All @@ -49,7 +48,7 @@ export class BillingAccountsService {
'Failed to lock challenge amount',
);
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}.
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The variable dto.amount is used here, but it is not clear from the diff whether this change is intentional or if dto.lockAmount was the correct variable to use. Ensure that dto.amount is indeed the intended value for this error message, as using the wrong variable could lead to incorrect error reporting.

Please contact the Topcoder Project Manager for further assistance.`,
);
}
Expand All @@ -63,7 +62,7 @@ export class BillingAccountsService {
`${TOPCODER_API_V6_BASE_URL}/billing-accounts/${billingAccountId}/consume-amount`,
{
method: 'PATCH',
body: JSON.stringify({ param: dto }),
body: JSON.stringify(dto),
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The change from JSON.stringify({ param: dto }) to JSON.stringify(dto) alters the structure of the request payload. Ensure that the API endpoint expects the new payload format, as this could affect the correctness of the request.

},
);
} catch (err: any) {
Expand Down Expand Up @@ -111,7 +110,7 @@ export class BillingAccountsService {

await this.lockAmount(billingAccountId, {
challengeId: baValidation.challengeId!,
lockAmount:
amount:
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The property name change from lockAmount to amount should be verified for consistency across the codebase. Ensure that this change does not break any other parts of the application that rely on the previous property name.

(rollback ? prevAmount : currAmount) * (1 + baValidation.markup!),
});
} else if (status === ChallengeStatuses.Completed.toLowerCase()) {
Expand All @@ -125,9 +124,8 @@ export class BillingAccountsService {
if (currAmount !== prevAmount) {
await this.consumeAmount(billingAccountId, {
challengeId: baValidation.challengeId!,
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The property name change from consumeAmount to amount should be verified for consistency across the codebase. Ensure that any other references to consumeAmount are updated accordingly to prevent runtime errors.

consumeAmount:
amount:
(rollback ? prevAmount : currAmount) * (1 + baValidation.markup!),
markup: baValidation.markup,
});
}
} else if (
Expand Down Expand Up @@ -155,7 +153,7 @@ export class BillingAccountsService {
if (currAmount !== prevAmount) {
await this.lockAmount(billingAccountId, {
challengeId: baValidation.challengeId!,
lockAmount: rollback ? prevAmount : 0,
amount: rollback ? prevAmount : 0,
Copy link

Choose a reason for hiding this comment

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

[❗❗ correctness]
The parameter name change from lockAmount to amount should be verified against the lockAmount method's expected parameters. Ensure that this change does not affect the method's functionality or any dependent code. If lockAmount is a method from an external library or API, double-check the documentation to confirm that amount is the correct parameter name.

});
}
}
Expand Down
23 changes: 23 additions & 0 deletions src/shared/topcoder/topcoder-m2m.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,29 @@ export class TopcoderM2MService {
const response = await fetch(url, finalOptions);

if (!response.ok) {
let responseBody: unknown;
try {
const text = await response.text();
try {
responseBody = JSON.parse(text);
} catch {
responseBody = text;
}
} catch (e) {
responseBody = `Failed to read response body: ${e?.message ?? e}`;
}

this.logger.error(
'M2M fetch failed',
{
url: String(url),
method: (finalOptions.method ?? 'GET'),
status: response.status,
statusText: response.statusText,
requestBody: (finalOptions as any).body,
Copy link

Choose a reason for hiding this comment

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

[⚠️ maintainability]
Casting finalOptions to any to access body can lead to runtime errors if finalOptions does not have a body property. Consider using a more specific type or checking if body exists before accessing it.

responseBody,
},
);
// Optional: You could throw a custom error here
Copy link

Choose a reason for hiding this comment

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

[⚠️ design]
Consider throwing a custom error instead of a generic Error. This can provide more context and make error handling more robust and maintainable.

throw new Error(`HTTP error! Status: ${response.status}`);
}
Expand Down
Loading