V6 -> dev #106
V6 -> dev #106
Conversation
[skip ci]
… PM-1112_apply-challenge-payments
…-payments PM-1112 - apply challenge payments at end of challenge
… PM-2087_use-v6-apis
…s by adding the payments for reviewers, since they still get paid.
Fix challenge payments calculations
Update reviewer model: use coefficients & fixed amount
…-place PM-1112 fix payment for place
PM-2091 cleanup tables
… PM-2087_use-v6-apis
PM-2088 - use Topcoder v6 APIs
… PM-1112_fix-payment-for-place
…-place Add challenge lock, update logic for reviewer, copilot payments
round up reviewers payments
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @IsOptional() | ||
| @IsEnum(WinningsCategory) | ||
| type: WinningsCategory; | ||
| type?: WinningsCategory; |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the type property to be optional (type?: WinningsCategory) may lead to potential issues if the rest of the codebase assumes this property is always present. Ensure that all usages of this property handle the undefined case appropriately to avoid runtime errors.
| @IsOptional() | ||
| @IsEnum(PaymentStatus) | ||
| status: PaymentStatus; | ||
| status?: PaymentStatus; |
There was a problem hiding this comment.
[❗❗ correctness]
Changing status from a required property to an optional one (status?: PaymentStatus) can have implications on how this DTO is used throughout the codebase. Ensure that all usages of this DTO handle the status property being undefined to prevent potential runtime errors.
| @IsOptional() | ||
| @IsEnum(DateFilterType) | ||
| date: DateFilterType; | ||
| date?: DateFilterType; |
There was a problem hiding this comment.
[❗❗ correctness]
Changing the date property to be optional (date?: DateFilterType) can lead to potential issues if the rest of the codebase assumes this property is always present. Ensure that all usages of this property handle the undefined case appropriately to avoid runtime errors.
| import { WithdrawalModule } from './api/withdrawal/withdrawal.module'; | ||
| import { Logger } from 'src/shared/global'; | ||
|
|
||
| const API_DOCS_URL = `${ENV_CONFIG.API_BASE}/api-docs`; |
There was a problem hiding this comment.
[correctness]
Consider validating ENV_CONFIG.API_BASE to ensure it is a valid URL before using it to construct API_DOCS_URL. This can prevent potential runtime errors if ENV_CONFIG.API_BASE is undefined or malformed.
| }); | ||
| SwaggerModule.setup('/v5/finance/api-docs', app, document); | ||
| const document = SwaggerModule.createDocument(app, config); | ||
| SwaggerModule.setup(API_DOCS_URL, app, document); |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of the include option in SwaggerModule.createDocument could lead to missing API documentation for specific modules. Ensure that all necessary modules are included elsewhere or verify that their documentation is not needed.
| }); | ||
| SwaggerModule.setup('/v5/finance/api-docs', app, document); | ||
| const document = SwaggerModule.createDocument(app, config); | ||
| SwaggerModule.setup(API_DOCS_URL, app, document); |
There was a problem hiding this comment.
[❗❗ correctness]
The API_DOCS_URL variable should be verified to ensure it is correctly defined and points to the intended endpoint. If this variable is not defined or incorrectly set, it could lead to incorrect API documentation setup.
|
|
||
| await app.listen(ENV_CONFIG.PORT ?? 3000); | ||
| logger.log(`Application is running on: ${await app.getUrl()}`); | ||
| logger.log(`Swagger docs available at: ${await app.getUrl()}${API_DOCS_URL}`); |
There was a problem hiding this comment.
[performance]
Consider storing the result of await app.getUrl() in a variable to avoid calling it twice. This will improve performance slightly and ensure consistency if the URL could change between calls.
| challengeId: baValidation.challengeId!, | ||
| lockAmount: | ||
| (rollback ? prevAmount : currAmount) * (1 + baValidation.markup!), | ||
| }); |
There was a problem hiding this comment.
[❗❗ correctness]
The expression (rollback ? prevAmount : currAmount) * (1 + baValidation.markup!) assumes that baValidation.markup is always defined. Consider adding a check or default value to avoid potential runtime errors.
| await this.consumeAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, | ||
| consumeAmount: | ||
| (rollback ? prevAmount : currAmount) * (1 + baValidation.markup!), |
There was a problem hiding this comment.
[❗❗ correctness]
The expression (rollback ? prevAmount : currAmount) * (1 + baValidation.markup!) assumes that baValidation.markup is always defined. Consider adding a check or default value to avoid potential runtime errors.
| const prevAmount = (baValidation.prevTotalPrizesInCents ?? 0) / 100; | ||
|
|
||
| await this.lockAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, |
There was a problem hiding this comment.
[❗❗ correctness]
The non-null assertion operator ! is used on baValidation.challengeId. Ensure that challengeId is always defined when this method is called, or add a check to prevent potential runtime errors.
|
|
||
| if (currAmount !== prevAmount) { | ||
| await this.consumeAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, |
There was a problem hiding this comment.
[❗❗ correctness]
The non-null assertion operator ! is used on baValidation.challengeId. Ensure that challengeId is always defined when this method is called, or add a check to prevent potential runtime errors.
|
|
||
| if (currAmount !== prevAmount) { | ||
| await this.lockAmount(billingAccountId, { | ||
| challengeId: baValidation.challengeId!, |
There was a problem hiding this comment.
[❗❗ correctness]
The non-null assertion operator ! is used on baValidation.challengeId. Ensure that challengeId is always defined when this method is called, or add a check to prevent potential runtime errors.
| try { | ||
| const headers = await this.getHeaders(); | ||
| const response = await fetch(`${TOPCODER_API_BASE_URL}/bus/events`, { | ||
| const response = await fetch(`${TC_API_BASE}/bus/events`, { |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that TC_API_BASE is correctly defined and initialized elsewhere in the codebase. If it is not, this change could lead to runtime errors due to an undefined variable.
| // Split the unique user IDs into chunks of 100 to comply with API request limits | ||
| const requests = chunk(uniqUserIds, 30).map((chunk) => { | ||
| const requestUrl = `${TOPCODER_API_BASE_URL}/members?${chunk.map((id) => `userIds[]=${id}`).join('&')}&fields=handle,userId`; | ||
| const requestUrl = `${TC_API_BASE}/members?${chunk.map((id) => `userIds[]=${id}`).join('&')}&fields=handle,userId`; |
There was a problem hiding this comment.
[❗❗ correctness]
The variable TC_API_BASE should be verified to ensure it is correctly defined and initialized elsewhere in the codebase. If TC_API_BASE is not properly set, it could lead to runtime errors or incorrect API requests.
| ); | ||
| } | ||
| const requestUrl = `${TOPCODER_API_BASE_URL}/members/${handle}${fields ? `?fields=${fields.join(',')}` : ''}`; | ||
| const requestUrl = `${TC_API_BASE}/members/${handle}${fields ? `?fields=${fields.join(',')}` : ''}`; |
There was a problem hiding this comment.
[❗❗ correctness]
The change from TOPCODER_API_BASE_URL to TC_API_BASE should be verified to ensure that TC_API_BASE is correctly defined and initialized elsewhere in the codebase. If TC_API_BASE is not properly set, it could lead to incorrect API requests and potential failures in fetching member data.
| } | ||
|
|
||
| if (!m2mToken) { | ||
| throw new Error('Failed to fetch m2m token for m2m call!') |
There was a problem hiding this comment.
[maintainability]
Consider using a custom error class instead of a generic Error to provide more context about the failure. This can improve error handling and debugging.
| const response = await fetch(url, finalOptions); | ||
|
|
||
| if (!response.ok) { | ||
| // Optional: You could throw a custom error here |
There was a problem hiding this comment.
[maintainability]
Consider using a custom error class here as well to provide more context about the HTTP error. This can make error handling more robust and informative.
| } | ||
|
|
||
| // If not JSON, return text | ||
| return response.text() as unknown as T; |
There was a problem hiding this comment.
[correctness]
Casting the result of response.text() as unknown and then as T could lead to runtime errors if T is not compatible with a string. Consider ensuring type safety or handling this conversion more explicitly.
…ners PM-1105 - Checkpoint winners
| } | ||
| } | ||
|
|
||
| generateWinnersPayments( |
There was a problem hiding this comment.
[❗❗ correctness]
The function signature has been changed to accept winners and prizes as separate parameters instead of extracting them from the challenge object. Ensure that this change is reflected in all places where generateWinnersPayments is called, as it might break existing functionality if those calls are not updated accordingly.
| challenge: Challenge, | ||
| winners: Winner[], | ||
| prizes: Prize[], | ||
| type?: WinningsCategory, |
There was a problem hiding this comment.
[correctness]
The optional parameter type is added to the function signature. Verify that this parameter is used correctly within the function, and consider how its optional nature might affect the logic. If type is crucial for determining payment logic, ensure there is a default behavior when it is not provided.
| 'desc', | ||
| ); | ||
|
|
||
| if ((checkpointPrizes?.length ?? 0) < (checkpointWinners?.length ?? 0)) { |
There was a problem hiding this comment.
[💡 maintainability]
The error message could be more informative by including the actual numbers of checkpoint winners and prizes. This would aid in debugging by providing more context about the mismatch.
| ); | ||
| } | ||
|
|
||
| return this.generateWinnersPayments(challenge, winners, placementPrizes); |
There was a problem hiding this comment.
[maintainability]
The refactoring to use generateWinnersPayments improves modularity and maintainability by encapsulating the logic for generating winner payments. Ensure that generateWinnersPayments is thoroughly tested to verify it handles all edge cases correctly, as this change centralizes critical logic.
| throw new Error('Missing challenge resources!'); | ||
| } | ||
|
|
||
| const winnersPayments = this.generatePlacementWinnersPayments(challenge); |
There was a problem hiding this comment.
[❗❗ correctness]
The method generatePlacementWinnersPayments is introduced here, replacing generateWinnersPayments. Ensure that this new method correctly implements the intended logic and that all necessary tests are updated to reflect this change.
|
|
||
| const winnersPayments = this.generatePlacementWinnersPayments(challenge); | ||
| const checkpointPayments = | ||
| this.generateCheckpointWinnersPayments(challenge); |
There was a problem hiding this comment.
[❗❗ correctness]
The introduction of generateCheckpointWinnersPayments suggests additional logic for handling checkpoint payments. Verify that this method is correctly implemented and integrated with the rest of the payment generation logic. Ensure that edge cases are considered and tested.
|
|
||
| const payments: PaymentPayload[] = [ | ||
| ...winnersPayments, | ||
| ...checkpointPayments, |
There was a problem hiding this comment.
[❗❗ security]
Ensure that checkpointPayments is properly validated and sanitized before being spread into the payments array. This is important to prevent potential security vulnerabilities or data integrity issues if checkpointPayments contains unexpected or malicious data.
| updated: string; | ||
| overview: PrizeOverview; | ||
| winners: Winner[]; | ||
| checkpointWinners: Winner[]; |
There was a problem hiding this comment.
[correctness]
Consider whether checkpointWinners should be initialized with a default value (e.g., an empty array) to prevent potential undefined errors when accessing this property.
…for-reward-challenges PM-2561 skip payments for reward challenges
…tion PM-2596 - Improved description for copilot & checkpoint winner payments
…ailed-challenge PM-2595 - handle challenge canceled due to failed review
| Canceled: 'Canceled', | ||
| Completed: 'Completed', | ||
| Deleted: 'Deleted', | ||
| CancelledFailedReview: 'CANCELLED_FAILED_REVIEW', |
There was a problem hiding this comment.
[readability]
Consider using consistent casing for status values. The change from 'Cancelled - Failed Review' to 'CANCELLED_FAILED_REVIEW' introduces a different casing style compared to other status values like 'New' or 'Active'. This inconsistency might lead to confusion or errors when these values are used in comparisons or displayed to users.
No description provided.