Engagements #137
Engagements #137
Conversation
…t to return all payments that have a given external ID. Useful for pulling engagement assignee payments by the assignment ID
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const result = new ResponseDto<WinningDto[]>(); | ||
|
|
||
| try { | ||
| const queryWhere = this.getWinningsQueryFilters( |
There was a problem hiding this comment.
[maintainability]
The method getWinningsQueryFilters is called with several undefined parameters. Consider refactoring the method to handle optional parameters more explicitly, which can improve readability and maintainability.
| ], | ||
| }); | ||
|
|
||
| const usersPayoutStatusMap = winnings?.length |
There was a problem hiding this comment.
[💡 performance]
The use of ?.length to check for array length is unnecessary. winnings.length is sufficient and more performant since winnings is already defined.
| billingAccount: paymentItem.billing_account, | ||
| })), | ||
| createdAt: item.created_at as Date, | ||
| updatedAt: (item.payment?.[0].date_paid ?? |
There was a problem hiding this comment.
[correctness]
The use of optional chaining ?. with array indexing payment?.[0] can lead to unexpected results if payment is not an array. Ensure that payment is always an array or handle the case where it might not be.
| paymentStatus: usersPayoutStatusMap[item.winner_id], | ||
| })); | ||
| } catch (error) { | ||
| this.logger.error('Getting winnings by external ID failed', error); |
There was a problem hiding this comment.
[💡 readability]
Consider using error.message instead of concatenating the entire error object to the message string. This will provide a cleaner and more informative error message.
| ): Promise<ResponseDto<WinningDto[]>> { | ||
| const result = await this.winningsRepo.getWinningsByExternalId(externalId); | ||
|
|
||
| result.status = ResponseStatusType.SUCCESS; |
There was a problem hiding this comment.
[❗❗ correctness]
The result.status is set to SUCCESS before checking for an error. This could lead to incorrect status reporting if an error exists. Consider setting the status to SUCCESS only if no error is present.
|
|
||
| return { | ||
| ...result, | ||
| data: result.data, |
There was a problem hiding this comment.
[correctness]
The data field is directly assigned from result.data. Ensure that result.data is always defined and correctly formatted as WinningDto[] to prevent potential runtime errors.
| @@ -1,8 +1,11 @@ | |||
| export enum Role { | |||
| Administrator = 'Administrator', | |||
There was a problem hiding this comment.
[💡 readability]
Consider using a consistent naming convention for enum values. For example, PaymentAdmin uses camel case while Administrator uses title case. Consistency improves readability and maintainability.
| @@ -1,8 +1,11 @@ | |||
| export enum Role { | |||
| Administrator = 'Administrator', | |||
| PaymentAdmin = 'Payment Admin', | |||
There was a problem hiding this comment.
[💡 readability]
Consider using a consistent naming convention for enum values. For example, Payment Admin uses a space while Administrator does not. Consistency improves readability and maintainability.
| }; | ||
|
|
||
| Object.keys(auth0User).forEach((key) => { | ||
| if (key.match(/\/roles$/gi) || key === 'roles' || key === 'role') { |
There was a problem hiding this comment.
[correctness]
The regular expression /\/roles$/gi is used to match keys ending with '/roles'. Consider whether the global flag g is necessary here, as it might lead to unexpected behavior if used in other contexts. Removing it could improve clarity and prevent potential issues.
|
|
||
| Object.keys(auth0User).forEach((key) => { | ||
| if (key.match(/\/roles$/gi) || key === 'roles' || key === 'role') { | ||
| appendRoles(auth0User[key]); |
There was a problem hiding this comment.
[maintainability]
The appendRoles function is defined within collectUserRoles and captures the roles array from its outer scope. Consider extracting appendRoles as a separate method to improve maintainability and testability.
| } | ||
| if (typeof value === 'string' && value.trim()) { | ||
| value | ||
| .split(',') |
There was a problem hiding this comment.
[❗❗ correctness]
The method collectUserRoles is introduced but not shown in the diff. Ensure that it correctly implements the logic to extract roles from auth0User, as this is critical for the guard's functionality.
| return roles; | ||
| }, []); | ||
| const userRoles = this.collectUserRoles(auth0User); | ||
| const normalizedUserRoles = new Set( |
There was a problem hiding this comment.
[performance]
Using Set for normalizedUserRoles is a good choice for performance when checking membership with has(). Ensure that normalizeRole is a pure function and handles edge cases correctly.
| const normalizedUserRoles = new Set( | ||
| userRoles.map((role) => this.normalizeRole(role)), | ||
| ); | ||
| const normalizedRequiredRoles = requiredRoles.map((role) => |
There was a problem hiding this comment.
[💡 performance]
Consider checking if requiredRoles is empty before mapping and normalizing, as this would avoid unnecessary computation when no roles are required.
No description provided.