Conversation
…o dev # Conflicts: # src/api/admin/admin.controller.ts # src/api/admin/admin.service.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| ); | ||
|
|
||
| if (result.error || !result.data) { |
There was a problem hiding this comment.
[maintainability]
Consider logging the error details before throwing the InternalServerErrorException. This can help with debugging and monitoring by providing more context about the failure.
|
|
||
| winnings.push(...result.data.winnings); | ||
|
|
||
| if (result.data.winnings.length < AdminController.EXPORT_BATCH_SIZE) { |
There was a problem hiding this comment.
[correctness]
The condition result.data.winnings.length < AdminController.EXPORT_BATCH_SIZE assumes that the API will always return a number of winnings equal to or less than the batch size. Ensure that this logic aligns with the API's behavior, especially if the API could return more winnings than requested due to pagination issues.
| */ | ||
| async searchWinnings( | ||
| searchProps: WinningRequestDto, | ||
| options: SearchWinningsOptions = {}, |
There was a problem hiding this comment.
[maintainability]
The options parameter is introduced without any documentation or type definition. Consider defining a TypeScript interface for SearchWinningsOptions to ensure type safety and improve maintainability.
| options: SearchWinningsOptions = {}, | ||
| ): Promise<ResponseDto<SearchWinningResult>> { | ||
| const result = new ResponseDto<SearchWinningResult>(); | ||
| const includeCount = options.includeCount ?? true; |
There was a problem hiding this comment.
[correctness]
The default value for includeCount is set to true. Ensure that this default behavior is intentional and documented, as it may affect the logic where this function is called.
| ): Promise<ResponseDto<SearchWinningResult>> { | ||
| const result = new ResponseDto<SearchWinningResult>(); | ||
| const includeCount = options.includeCount ?? true; | ||
| const includePayoutStatus = options.includePayoutStatus ?? true; |
There was a problem hiding this comment.
[correctness]
The default value for includePayoutStatus is set to true. Verify that this default aligns with the expected behavior of the function and is documented accordingly.
| ); | ||
|
|
||
| const winnings = await this.prisma.winnings.findMany({ | ||
| const limit = searchProps.limit ?? 10; |
There was a problem hiding this comment.
[correctness]
Consider validating searchProps.limit and searchProps.offset to ensure they are non-negative integers. This will prevent potential issues with pagination logic.
| }); | ||
|
|
||
| const count = await this.prisma.winnings.count({ where: queryWhere }); | ||
| const [winnings, count] = includeCount |
There was a problem hiding this comment.
[correctness]
The use of Promise.all here is appropriate for parallelizing the fetching of winnings and count. However, ensure that both operations are independent and that the count operation does not depend on any side effects of the winningsPromise. If there is any dependency, this could lead to incorrect results.
| const usersPayoutStatusMap = winnings?.length | ||
| ? await this.getUsersPayoutStatusForWinnings(winnings) | ||
| : ({} as { [key: string]: payment_status }); | ||
| const usersPayoutStatusMap: Record<string, unknown> = |
There was a problem hiding this comment.
[💡 maintainability]
The type Record<string, unknown> for usersPayoutStatusMap is quite generic. If possible, specify a more precise type to improve type safety and maintainability.
| paymentStatus: usersPayoutStatusMap[item.winner_id], | ||
| paymentStatus: usersPayoutStatusMap[ | ||
| item.winner_id | ||
| ] as WinningDto['paymentStatus'], |
There was a problem hiding this comment.
[correctness]
Casting item.winner_id to WinningDto['paymentStatus'] may hide potential type mismatches. Ensure that usersPayoutStatusMap always returns a value compatible with WinningDto['paymentStatus'] to avoid runtime errors.
| pageSize: searchProps.limit, | ||
| currentPage: Math.ceil(searchProps.offset / searchProps.limit) + 1, | ||
| totalItems, | ||
| totalPages: Math.ceil(totalItems / limit), |
There was a problem hiding this comment.
[❗❗ correctness]
Ensure that limit is never zero to prevent a division by zero error when calculating totalPages.
1 participant
No description provided.