PS-3406: Update username on keycloak #132
Conversation
WalkthroughThe pull request introduces comprehensive changes to user management across multiple files, focusing on enhancing user data structure and Keycloak integration. The modifications include replacing a single Changes
Sequence DiagramsequenceDiagram
participant Controller as User Controller
participant Service as Postgres User Service
participant Keycloak as Keycloak Adapter
Controller->>Service: updateUser(userDto)
Service->>Keycloak: updateUserInKeyCloak
alt Username already exists
Keycloak-->>Service: Return 'exists'
Service-->>Controller: Return username exists error
else Update successful
Keycloak-->>Service: Return true
Service->>Service: Update user details in database
Service-->>Controller: Return updated user
else Update failed
Keycloak-->>Service: Return false
Service-->>Controller: Return update error
end
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
src/adapters/postgres/user-adapter.ts (1)
Line range hint
413-426: [CRITICAL] Potential SQL Injection Vulnerability in Dynamic SQL Query ConstructionThe code constructs SQL queries by concatenating strings with user-provided input (
value). This practice can lead to SQL injection attacks. It is strongly recommended to use parameterized queries or query builders provided by TypeORM to prevent SQL injection.Consider refactoring this code to use parameterized queries. For example:
if (key === "firstName") { whereCondition += ` U."${key}" ILIKE :firstName`; queryParams.firstName = `%${value}%`; }And pass
queryParamsto your query execution method.
🧹 Nitpick comments (13)
src/adapters/postgres/user-adapter.ts (4)
805-805: Fix code formatting to adhere to style guidelinesThe construction of
keycloakReqBodylacks spacing and does not conform to the style guide. Adding spaces after commas improves readability.Apply this diff to correct the formatting:
-const keycloakReqBody = {username,firstName,lastName, userId}; +const keycloakReqBody = { username, firstName, lastName, userId };🧰 Tools
🪛 eslint
[error] 805-805: Replace
username,firstName,lastName,·userIdwith·username,·firstName,·lastName,·userId·(prettier/prettier)
926-926: Use 'const' instead of 'let' for variables that are not reassignedThe variable
updateResultis never reassigned after its initial assignment. Usingconstimproves code readability and enforces immutability where applicable.Apply this diff:
-let updateResult = await updateUserInKeyCloak(updateField, token) +const updateResult = await updateUserInKeyCloak(updateField, token);🧰 Tools
🪛 eslint
[error] 926-926: 'updateResult' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 926-926: Insert
;(prettier/prettier)
927-927: Simplify redundant ternary expressionThe expression
return updateResult ? true : false;is unnecessarily verbose. You can simplify it by returning the boolean value directly.Apply this diff:
-return updateResult ? true : false; +return Boolean(updateResult);🧰 Tools
🪛 Biome (1.9.4)
[error] 927-927: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
🪛 eslint
[error] 927-928: Delete
⏎(prettier/prettier)
913-913: Improve method signature formatting for better readabilityThe method signature lacks line breaks and proper indentation, making it less readable. Adjust the formatting to align with style guidelines.
Apply this diff:
-async updateUsernameInKeycloak(updateField: UpdateField): Promise<'exists' | false | true> { +async updateUsernameInKeycloak( + updateField: UpdateField +): Promise<'exists' | false | true> {🧰 Tools
🪛 eslint
[error] 913-913: Replace
updateField:·UpdateField):·Promise<'exists'with⏎····updateField:·UpdateField⏎··):·Promise<"exists"(prettier/prettier)
src/user/entities/user-entity.ts (2)
34-35: Use a TypeScript Enum for 'gender' field for stronger type safety and maintainabilityDefining an explicit TypeScript
enumfor thegenderfield enhances type safety and ensures consistent usage of gender values throughout the codebase.Define the enum:
export enum Gender { MALE = "male", FEMALE = "female", TRANSGENDER = "transgender", }Then update the
gendercolumn to use this enum:@Column({ type: "enum", enum: Gender, nullable: false }) gender: Gender;🧰 Tools
🪛 eslint
[error] 34-34: Replace
·type:·'enum',·enum:·['male',·'female',·'transgender'],·nullable:·falsewith⏎····type:·"enum",⏎····enum:·["male",·"female",·"transgender"],⏎····nullable:·false,⏎·(prettier/prettier)
25-35: Fix code formatting to adhere to style guidelinesThere are formatting inconsistencies in the new property definitions. Ensure that strings use consistent quotation marks and that the code is properly indented.
Apply the following diff:
-@Column({ type: 'varchar', length: 50, nullable: false }) +@Column({ type: "varchar", length: 50, nullable: false }) firstName: string; -@Column({ type: 'varchar', length: 50, nullable: true }) +@Column({ type: "varchar", length: 50, nullable: true }) middleName: string; -@Column({ type: 'varchar', length: 50, nullable: false }) +@Column({ type: "varchar", length: 50, nullable: false }) lastName: string; -@Column({ type: 'enum', enum: ['male', 'female', 'transgender'], nullable: false }) +@Column({ + type: "enum", + enum: ["male", "female", "transgender"], + nullable: false, +}) gender: string;🧰 Tools
🪛 eslint
[error] 25-25: Replace
'varchar'with"varchar"(prettier/prettier)
[error] 28-28: Replace
'varchar'with"varchar"(prettier/prettier)
[error] 31-31: Replace
'varchar'with"varchar"(prettier/prettier)
[error] 34-34: Replace
·type:·'enum',·enum:·['male',·'female',·'transgender'],·nullable:·falsewith⏎····type:·"enum",⏎····enum:·["male",·"female",·"transgender"],⏎····nullable:·false,⏎·(prettier/prettier)
src/user/dto/user-create.dto.ts (2)
94-102: Define and use a 'Gender' enum for validation and documentationInstead of hardcoding the gender options, define a
Genderenum and use it for both the@IsEnumvalidation and the@ApiPropertydecorator. This promotes code reusability and consistency.Define the enum:
export enum Gender { MALE = "male", FEMALE = "female", TRANSGENDER = "transgender", }Update the
genderproperty:@ApiProperty({ type: String, description: "Gender of the user", enum: Gender }) @Expose() @IsEnum(Gender) gender: Gender;🧰 Tools
🪛 eslint
[error] 94-94: Delete
·(prettier/prettier)
[error] 95-95: Delete
·(prettier/prettier)
[error] 96-96: Replace
'Gender·of·the·user',·with"Gender·of·the·user",(prettier/prettier)
[error] 97-97: Replace
'male',·'female',·'transgender']·with"male",·"female",·"transgender"],(prettier/prettier)
[error] 100-100: Replace
'male',·'female',·'transgender'with"male",·"female",·"transgender"(prettier/prettier)
[error] 101-102: Delete
⏎(prettier/prettier)
75-86: Adjust formatting for consistency and readabilityThe
@ApiPropertyand@ApiPropertyOptionaldecorators contain multiple properties. Formatting them across multiple lines improves readability.Apply the following diff:
@ApiProperty({ type: String, description: 'First name of the user', maxLength: 50 }) +@ApiProperty({ + type: String, + description: "First name of the user", + maxLength: 50, +}) @Expose() @IsString() @Length(1, 50) firstName: string; @ApiPropertyOptional({ type: String, description: 'Middle name of the user (optional)', maxLength: 50, required: false }) +@ApiPropertyOptional({ + type: String, + description: "Middle name of the user (optional)", + maxLength: 50, + required: false, +}) @Expose() @IsOptional() @IsString() @Length(0, 50) middleName?: string;🧰 Tools
🪛 eslint
[error] 75-75: Replace
·type:·String,·description:·'First·name·of·the·user',·maxLength:·50with⏎····type:·String,⏎····description:·"First·name·of·the·user",⏎····maxLength:·50,⏎·(prettier/prettier)
[error] 81-81: Replace
·type:·String,·description:·'Middle·name·of·the·user·(optional)',·maxLength:·50,·required:·falsewith⏎····type:·String,⏎····description:·"Middle·name·of·the·user·(optional)",⏎····maxLength:·50,⏎····required:·false,⏎·(prettier/prettier)
src/user/dto/user-update.dto.ts (1)
48-56: Consider using an enum type for gender values.While the implementation works, it would be better to define the gender values in an enum type for better type safety and maintainability.
+export enum Gender { + MALE = 'male', + FEMALE = 'female', + TRANSGENDER = 'transgender' +} @ApiProperty({ type: String, description: 'Gender of the user', - enum: ['male', 'female', 'transgender'] + enum: Gender }) @Expose() -@IsEnum(['male', 'female', 'transgender']) +@IsEnum(Gender) @IsOptional() -gender?: string; +gender?: Gender;🧰 Tools
🪛 eslint
[error] 48-48: Delete
·(prettier/prettier)
[error] 49-49: Delete
·(prettier/prettier)
[error] 50-50: Replace
'Gender·of·the·user',·with"Gender·of·the·user",(prettier/prettier)
[error] 51-51: Replace
'male',·'female',·'transgender']·with"male",·"female",·"transgender"],(prettier/prettier)
[error] 54-54: Replace
'male',·'female',·'transgender'with"male",·"female",·"transgender"(prettier/prettier)
src/common/utils/response.messages.ts (1)
98-99: Fix grammar in Keycloak username exists message.The message has incorrect grammar.
- USERNAME_EXISTS_KEYCLOAK: 'Username is already exists in keycloak', + USERNAME_EXISTS_KEYCLOAK: 'Username already exists in Keycloak',🧰 Tools
🪛 eslint
[error] 98-98: Replace
'Username·is·already·exists·in·keycloak'with"Username·is·already·exists·in·keycloak"(prettier/prettier)
[error] 99-99: Replace
'Failed·to·update·username·details·in·Keycloak.'with·"Failed·to·update·username·details·in·Keycloak."(prettier/prettier)
src/adapters/userservicelocator.ts (3)
19-19: Remove extra space after method name.The extra space after
updateUserviolates formatting guidelines.- updateUser( userDto?: any, response?: any); + updateUser(userDto?: any, response?: any);🧰 Tools
🪛 eslint
[error] 19-19: Delete
·(prettier/prettier)
19-19: Consider adding method documentation.Since this method is part of a public interface and has been modified to support Keycloak integration, it would be beneficial to add JSDoc documentation explaining the method's purpose, parameters, and return type.
+ /** + * Updates user information including Keycloak username. + * @param userDto - The user data transfer object containing updated user information + * @param response - The Express response object + * @returns Promise that resolves when the user is updated + */ updateUser(userDto?: any, response?: any);🧰 Tools
🪛 eslint
[error] 19-19: Delete
·(prettier/prettier)
Line range hint
9-18: Remove commented code.The commented
getUsermethod signature should be removed as it adds noise to the codebase.- // getUser( - // userId?:Record<string, string>, - // response?: any, - // tenantId?: string, - // id?: any, - // accessRole?: string, - // request?: any, - // );🧰 Tools
🪛 eslint
[error] 19-19: Delete
·(prettier/prettier)
[error] 20-20: Replace
request:·any,·userDto:·UserCreateDto,·academicYearId:·string,·response:·Responsewith⏎····request:·any,⏎····userDto:·UserCreateDto,⏎····academicYearId:·string,⏎····response:·Response⏎··(prettier/prettier)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/adapters/postgres/user-adapter.ts(11 hunks)src/adapters/userservicelocator.ts(1 hunks)src/common/utils/keycloak.adapter.util.ts(4 hunks)src/common/utils/response.messages.ts(1 hunks)src/user/dto/user-create.dto.ts(2 hunks)src/user/dto/user-update.dto.ts(3 hunks)src/user/entities/user-entity.ts(1 hunks)src/user/user.controller.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
src/common/utils/response.messages.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/common/utils/keycloak.adapter.util.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/user/entities/user-entity.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/user/dto/user-create.dto.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/user/dto/user-update.dto.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/adapters/postgres/user-adapter.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/user/user.controller.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
src/adapters/userservicelocator.ts (1)
Pattern **/*.ts: "Review the JavaScript code for conformity with the Google JavaScript style guide, highlighting any deviations. Ensure that:
- The code adheres to best practices associated with nodejs.
- The code adheres to best practices associated with nestjs framework.
- The code adheres to best practices recommended for performance.
- The code adheres to similar naming conventions for controllers, models, services, methods, variables."
🪛 eslint
src/common/utils/response.messages.ts
[error] 98-98: Replace 'Username·is·already·exists·in·keycloak' with "Username·is·already·exists·in·keycloak"
(prettier/prettier)
[error] 99-99: Replace 'Failed·to·update·username·details·in·Keycloak.' with ·"Failed·to·update·username·details·in·Keycloak."
(prettier/prettier)
src/common/utils/keycloak.adapter.util.ts
[error] 86-86: Delete ··
(prettier/prettier)
[error] 107-109: Replace ⏎async·function·updateUserInKeyCloak(query,·token)·{··⏎ with async·function·updateUserInKeyCloak(query,·token)·{
(prettier/prettier)
[error] 110-110: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 116-116: Delete ··
(prettier/prettier)
[error] 124-124: Delete ··
(prettier/prettier)
[error] 137-137: Insert ·
(prettier/prettier)
[error] 139-139: Replace else with ·else·
(prettier/prettier)
[error] 143-143: Replace ·error.response?.data?.errorMessage·||·"Failed·to·update·user·in·Keycloak";· with ⏎······error.response?.data?.errorMessage·||·"Failed·to·update·user·in·Keycloak";
(prettier/prettier)
[error] 148-148: Insert ;
(prettier/prettier)
src/user/entities/user-entity.ts
[error] 25-25: Replace 'varchar' with "varchar"
(prettier/prettier)
[error] 28-28: Replace 'varchar' with "varchar"
(prettier/prettier)
[error] 31-31: Replace 'varchar' with "varchar"
(prettier/prettier)
[error] 34-34: Replace ·type:·'enum',·enum:·['male',·'female',·'transgender'],·nullable:·false with ⏎····type:·"enum",⏎····enum:·["male",·"female",·"transgender"],⏎····nullable:·false,⏎·
(prettier/prettier)
src/user/dto/user-create.dto.ts
[error] 75-75: Replace ·type:·String,·description:·'First·name·of·the·user',·maxLength:·50 with ⏎····type:·String,⏎····description:·"First·name·of·the·user",⏎····maxLength:·50,⏎·
(prettier/prettier)
[error] 81-81: Replace ·type:·String,·description:·'Middle·name·of·the·user·(optional)',·maxLength:·50,·required:·false with ⏎····type:·String,⏎····description:·"Middle·name·of·the·user·(optional)",⏎····maxLength:·50,⏎····required:·false,⏎·
(prettier/prettier)
[error] 88-88: Replace ·type:·String,·description:·'Last·name·of·the·user',·maxLength:·50 with ⏎····type:·String,⏎····description:·"Last·name·of·the·user",⏎····maxLength:·50,⏎·
(prettier/prettier)
[error] 94-94: Delete ·
(prettier/prettier)
[error] 95-95: Delete ·
(prettier/prettier)
[error] 96-96: Replace 'Gender·of·the·user',· with "Gender·of·the·user",
(prettier/prettier)
[error] 97-97: Replace 'male',·'female',·'transgender']· with "male",·"female",·"transgender"],
(prettier/prettier)
[error] 100-100: Replace 'male',·'female',·'transgender' with "male",·"female",·"transgender"
(prettier/prettier)
[error] 101-102: Delete ⏎
(prettier/prettier)
src/user/dto/user-update.dto.ts
[error] 27-27: Replace ·type:·String,·description:·'First·name·of·the·user',·maxLength:·50 with ⏎····type:·String,⏎····description:·"First·name·of·the·user",⏎····maxLength:·50,⏎·
(prettier/prettier)
[error] 34-34: Replace ·type:·String,·description:·'Middle·name·of·the·user·(optional)',·maxLength:·50,·required:·false with ⏎····type:·String,⏎····description:·"Middle·name·of·the·user·(optional)",⏎····maxLength:·50,⏎····required:·false,⏎·
(prettier/prettier)
[error] 41-41: Replace ·type:·String,·description:·'Last·name·of·the·user',·maxLength:·50 with ⏎····type:·String,⏎····description:·"Last·name·of·the·user",⏎····maxLength:·50,⏎·
(prettier/prettier)
[error] 48-48: Delete ·
(prettier/prettier)
[error] 49-49: Delete ·
(prettier/prettier)
[error] 50-50: Replace 'Gender·of·the·user',· with "Gender·of·the·user",
(prettier/prettier)
[error] 51-51: Replace 'male',·'female',·'transgender']· with "male",·"female",·"transgender"],
(prettier/prettier)
[error] 54-54: Replace 'male',·'female',·'transgender' with "male",·"female",·"transgender"
(prettier/prettier)
[error] 63-63: Replace ·type:·()·=>·String,·format:·'date-time',·example:·'1990-01-01' with ⏎····type:·()·=>·String,⏎····format:·"date-time",⏎····example:·"1990-01-01",⏎·
(prettier/prettier)
[error] 94-94: Replace 'date-time' with "date-time"
(prettier/prettier)
[error] 100-100: Replace 'date-time' with "date-time"
(prettier/prettier)
src/adapters/postgres/user-adapter.ts
[error] 771-771: Delete ·
(prettier/prettier)
[error] 805-805: Replace username,firstName,lastName,·userId with ·username,·firstName,·lastName,·userId·
(prettier/prettier)
[error] 806-806: Delete ······
(prettier/prettier)
[error] 808-808: Replace (username) with ·(username)·
(prettier/prettier)
[error] 809-809: Replace keycloakReqBody); with ⏎··········keycloakReqBody
(prettier/prettier)
[error] 810-810: Insert );⏎
(prettier/prettier)
[error] 811-811: Replace (updateuserDataInKeycloak·===·'exists') with ·(updateuserDataInKeycloak·===·"exists")·
(prettier/prettier)
[error] 820-820: Delete ··
(prettier/prettier)
[error] 821-821: Replace (updateuserDataInKeycloak·===·false) with ·(updateuserDataInKeycloak·===·false)·
(prettier/prettier)
[error] 831-831: Delete ······
(prettier/prettier)
[error] 913-913: Replace updateField:·UpdateField):·Promise<'exists' with ⏎····updateField:·UpdateField⏎··):·Promise<"exists"
(prettier/prettier)
[error] 914-915: Delete ⏎
(prettier/prettier)
[error] 920-920: Replace updateField with ⏎········updateField⏎······
(prettier/prettier)
[error] 922-922: Replace 'exists' with "exists"
(prettier/prettier)
[error] 926-926: 'updateResult' is never reassigned. Use 'const' instead.
(prefer-const)
[error] 926-926: Insert ;
(prettier/prettier)
[error] 927-928: Delete ⏎
(prettier/prettier)
[error] 932-932: Delete ,
(prettier/prettier)
[error] 936-936: Delete ··
(prettier/prettier)
[error] 963-963: Replace userId:·string,·userData:·Partial<User> with ⏎····userId:·string,⏎····userData:·Partial<User>⏎··
(prettier/prettier)
[error] 977-977: Replace ·where:·{·userId·} with ⏎········where:·{·userId·},⏎·····
(prettier/prettier)
[error] 982-982: Replace 'An·error·occurred·while·updating·user·details' with "An·error·occurred·while·updating·user·details"
(prettier/prettier)
[error] 984-986: Delete ⏎⏎
(prettier/prettier)
[error] 1086-1087: Delete ⏎
(prettier/prettier)
src/user/user.controller.ts
[error] 165-165: Delete ·
(prettier/prettier)
src/adapters/userservicelocator.ts
[error] 19-19: Delete ·
(prettier/prettier)
🪛 Biome (1.9.4)
src/adapters/postgres/user-adapter.ts
[error] 927-927: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (6)
src/user/dto/user-update.dto.ts (4)
27-32: LGTM! firstName field implementation looks good.The validation constraints and API documentation are properly implemented.
🧰 Tools
🪛 eslint
[error] 27-27: Replace
·type:·String,·description:·'First·name·of·the·user',·maxLength:·50with⏎····type:·String,⏎····description:·"First·name·of·the·user",⏎····maxLength:·50,⏎·(prettier/prettier)
34-39: LGTM! middleName field implementation looks good.The validation constraints and API documentation are properly implemented.
🧰 Tools
🪛 eslint
[error] 34-34: Replace
·type:·String,·description:·'Middle·name·of·the·user·(optional)',·maxLength:·50,·required:·falsewith⏎····type:·String,⏎····description:·"Middle·name·of·the·user·(optional)",⏎····maxLength:·50,⏎····required:·false,⏎·(prettier/prettier)
41-46: LGTM! lastName field implementation looks good.The validation constraints and API documentation are properly implemented.
🧰 Tools
🪛 eslint
[error] 41-41: Replace
·type:·String,·description:·'Last·name·of·the·user',·maxLength:·50with⏎····type:·String,⏎····description:·"Last·name·of·the·user",⏎····maxLength:·50,⏎·(prettier/prettier)
63-67: Consider timezone handling for date fields.The date fields (dob, createdAt, updatedAt) are now using the Date type, which is good. However, ensure that timezone handling is properly implemented in the service layer to avoid any timezone-related issues.
Run this script to check for timezone handling in the service layer:
Also applies to: 94-98, 100-104
🧰 Tools
🪛 eslint
[error] 63-63: Replace
·type:·()·=>·String,·format:·'date-time',·example:·'1990-01-01'with⏎····type:·()·=>·String,⏎····format:·"date-time",⏎····example:·"1990-01-01",⏎·(prettier/prettier)
src/common/utils/keycloak.adapter.util.ts (1)
64-65: LGTM! Updated user creation with firstName and lastName.The changes align with the new user data structure.
src/user/user.controller.ts (1)
165-165: LGTM! Simplified updateUser method call.The removal of the request parameter and simplified method call improve the code clarity.
🧰 Tools
🪛 eslint
[error] 165-165: Delete
·(prettier/prettier)
| LoggerUtil.error( | ||
| `${API_RESPONSES.SERVER_ERROR}`, | ||
| `KeyCloak Error: ${error.message}`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle errors appropriately within async functions
In the catch block, the function logs the error but does not return a value or rethrow the error. This can lead to unexpected behavior if the function returns undefined. Ensure that the function returns a consistent value or rethrows the error.
Consider rethrowing the error or returning a specific error value. For example:
catch (error) {
LoggerUtil.error(
`${API_RESPONSES.SERVER_ERROR}`,
`KeyCloak Error: ${error.message}`,
apiId
);
return false; // or rethrow the error
}🧰 Tools
🪛 eslint
[error] 932-932: Delete ,
(prettier/prettier)
| (user.userId = userCreateDto?.userId), | ||
| (user.username = userCreateDto?.username), | ||
| (user.firstName = userCreateDto?.firstName), | ||
| (user.middleName = userCreateDto?.middleName), | ||
| (user.lastName = userCreateDto?.lastName), | ||
| (user.gender = userCreateDto?.gender), | ||
| (user.email = userCreateDto?.email), | ||
| (user.mobile = Number(userCreateDto?.mobile) || null), | ||
| (user.createdBy = userCreateDto?.createdBy || userCreateDto?.userId), | ||
| (user.updatedBy = userCreateDto?.updatedBy || userCreateDto?.userId), | ||
| (user.userId = userCreateDto?.userId), | ||
| (user.state = userCreateDto?.state), | ||
| (user.district = userCreateDto?.district), | ||
| (user.address = userCreateDto?.address), | ||
| (user.pincode = userCreateDto?.pincode); | ||
| (user.createdBy = userCreateDto?.createdBy || userCreateDto?.createdBy); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor property assignments for better readability
The current assignment of properties using the comma operator and parentheses is non-standard and reduces code readability. It is recommended to assign properties individually or use object spread for clarity.
Refactor the code as follows:
-(user.userId = userCreateDto?.userId),
- (user.username = userCreateDto?.username),
- (user.firstName = userCreateDto?.firstName),
- (user.middleName = userCreateDto?.middleName),
- (user.lastName = userCreateDto?.lastName),
- (user.gender = userCreateDto?.gender),
- (user.email = userCreateDto?.email),
- (user.mobile = Number(userCreateDto?.mobile) || null),
- (user.createdBy = userCreateDto?.createdBy || userCreateDto?.createdBy);
+user.userId = userCreateDto?.userId;
+user.username = userCreateDto?.username;
+user.firstName = userCreateDto?.firstName;
+user.middleName = userCreateDto?.middleName;
+user.lastName = userCreateDto?.lastName;
+user.gender = userCreateDto?.gender;
+user.email = userCreateDto?.email;
+user.mobile = Number(userCreateDto?.mobile) || null;
+user.createdBy = userCreateDto?.createdBy;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| (user.userId = userCreateDto?.userId), | |
| (user.username = userCreateDto?.username), | |
| (user.firstName = userCreateDto?.firstName), | |
| (user.middleName = userCreateDto?.middleName), | |
| (user.lastName = userCreateDto?.lastName), | |
| (user.gender = userCreateDto?.gender), | |
| (user.email = userCreateDto?.email), | |
| (user.mobile = Number(userCreateDto?.mobile) || null), | |
| (user.createdBy = userCreateDto?.createdBy || userCreateDto?.userId), | |
| (user.updatedBy = userCreateDto?.updatedBy || userCreateDto?.userId), | |
| (user.userId = userCreateDto?.userId), | |
| (user.state = userCreateDto?.state), | |
| (user.district = userCreateDto?.district), | |
| (user.address = userCreateDto?.address), | |
| (user.pincode = userCreateDto?.pincode); | |
| (user.createdBy = userCreateDto?.createdBy || userCreateDto?.createdBy); | |
| user.userId = userCreateDto?.userId; | |
| user.username = userCreateDto?.username; | |
| user.firstName = userCreateDto?.firstName; | |
| user.middleName = userCreateDto?.middleName; | |
| user.lastName = userCreateDto?.lastName; | |
| user.gender = userCreateDto?.gender; | |
| user.email = userCreateDto?.email; | |
| user.mobile = Number(userCreateDto?.mobile) || null; | |
| user.createdBy = userCreateDto?.createdBy; |
| async function updateUserInKeyCloak(query, token) { | ||
|
|
||
| const axios = require("axios"); | ||
|
|
||
| // Validate required parameters | ||
| if (!query.userId) { | ||
| return "User cannot be updated, userId missing"; | ||
| } | ||
|
|
||
| // Prepare data payload | ||
| const data = JSON.stringify({ | ||
| username: query.username, // New username | ||
| enabled: query.enabled !== undefined ? query.enabled : true, // Default to enabled if not specified | ||
| ...(query.firstName && { firstName: query.firstName }), | ||
| ...(query.lastName && { lastName: query.lastName }), | ||
| }); | ||
|
|
||
| const config = { | ||
| method: "put", | ||
| url: `${process.env.KEYCLOAK}${process.env.KEYCLOAK_ADMIN}/${query.userId}`, | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: "Bearer " + token, | ||
| }, | ||
| data: data, | ||
| }; | ||
|
|
||
| try { | ||
| const response = await axios(config); | ||
| if(response.status === 204) { | ||
| return true; | ||
| }else{ | ||
| return false; | ||
| } | ||
| } catch (error) { | ||
| const errorMessage = error.response?.data?.errorMessage || "Failed to update user in Keycloak"; | ||
|
|
||
| LoggerUtil.error( | ||
| `${API_RESPONSES.SERVER_ERROR}`, | ||
| `Error: ${error.message},` | ||
| ) | ||
| return errorMessage; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in the updateUserInKeyCloak function.
- Error handling could be more specific
- Code duplication in axios configuration
- Security consideration for the enabled flag
Consider these improvements:
async function updateUserInKeyCloak(query, token) {
const axios = require("axios");
if (!query.userId) {
- return "User cannot be updated, userId missing";
+ throw new Error("User cannot be updated: userId is required");
}
const data = JSON.stringify({
username: query.username,
- enabled: query.enabled !== undefined ? query.enabled : true,
+ enabled: query.enabled, // Remove default to ensure explicit enabled status
...(query.firstName && { firstName: query.firstName }),
...(query.lastName && { lastName: query.lastName }),
});
+ // Extract common axios config to avoid duplication
+ const baseConfig = {
+ headers: {
+ "Content-Type": "application/json",
+ Authorization: "Bearer " + token,
+ },
+ };
const config = {
+ ...baseConfig,
method: "put",
url: `${process.env.KEYCLOAK}${process.env.KEYCLOAK_ADMIN}/${query.userId}`,
- headers: {
- "Content-Type": "application/json",
- Authorization: "Bearer " + token,
- },
data: data,
};
try {
const response = await axios(config);
- if(response.status === 204) {
- return true;
- }else{
- return false;
- }
+ return response.status === 204;
} catch (error) {
- const errorMessage = error.response?.data?.errorMessage || "Failed to update user in Keycloak";
+ const errorMessage = error.response?.data?.errorMessage || API_RESPONSES.UPDATE_USER_KEYCLOAK_ERROR;
LoggerUtil.error(
`${API_RESPONSES.SERVER_ERROR}`,
- `Error: ${error.message},`
+ `Error updating user in Keycloak: ${error.message}`
)
return errorMessage;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function updateUserInKeyCloak(query, token) { | |
| const axios = require("axios"); | |
| // Validate required parameters | |
| if (!query.userId) { | |
| return "User cannot be updated, userId missing"; | |
| } | |
| // Prepare data payload | |
| const data = JSON.stringify({ | |
| username: query.username, // New username | |
| enabled: query.enabled !== undefined ? query.enabled : true, // Default to enabled if not specified | |
| ...(query.firstName && { firstName: query.firstName }), | |
| ...(query.lastName && { lastName: query.lastName }), | |
| }); | |
| const config = { | |
| method: "put", | |
| url: `${process.env.KEYCLOAK}${process.env.KEYCLOAK_ADMIN}/${query.userId}`, | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: "Bearer " + token, | |
| }, | |
| data: data, | |
| }; | |
| try { | |
| const response = await axios(config); | |
| if(response.status === 204) { | |
| return true; | |
| }else{ | |
| return false; | |
| } | |
| } catch (error) { | |
| const errorMessage = error.response?.data?.errorMessage || "Failed to update user in Keycloak"; | |
| LoggerUtil.error( | |
| `${API_RESPONSES.SERVER_ERROR}`, | |
| `Error: ${error.message},` | |
| ) | |
| return errorMessage; | |
| } | |
| async function updateUserInKeyCloak(query, token) { | |
| const axios = require("axios"); | |
| if (!query.userId) { | |
| throw new Error("User cannot be updated: userId is required"); | |
| } | |
| const data = JSON.stringify({ | |
| username: query.username, | |
| enabled: query.enabled, | |
| ...(query.firstName && { firstName: query.firstName }), | |
| ...(query.lastName && { lastName: query.lastName }), | |
| }); | |
| const baseConfig = { | |
| headers: { | |
| "Content-Type": "application/json", | |
| Authorization: "Bearer " + token, | |
| }, | |
| }; | |
| const config = { | |
| ...baseConfig, | |
| method: "put", | |
| url: `${process.env.KEYCLOAK}${process.env.KEYCLOAK_ADMIN}/${query.userId}`, | |
| data: data, | |
| }; | |
| try { | |
| const response = await axios(config); | |
| return response.status === 204; | |
| } catch (error) { | |
| const errorMessage = error.response?.data?.errorMessage || API_RESPONSES.UPDATE_USER_KEYCLOAK_ERROR; | |
| LoggerUtil.error( | |
| `${API_RESPONSES.SERVER_ERROR}`, | |
| `Error updating user in Keycloak: ${error.message}` | |
| ) | |
| return errorMessage; | |
| } | |
| } |
🧰 Tools
🪛 eslint
[error] 110-110: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 116-116: Delete ··
(prettier/prettier)
[error] 124-124: Delete ··
(prettier/prettier)
[error] 137-137: Insert ·
(prettier/prettier)
[error] 139-139: Replace else with ·else·
(prettier/prettier)
[error] 143-143: Replace ·error.response?.data?.errorMessage·||·"Failed·to·update·user·in·Keycloak";· with ⏎······error.response?.data?.errorMessage·||·"Failed·to·update·user·in·Keycloak";
(prettier/prettier)
[error] 148-148: Insert ;
(prettier/prettier)
| @@ -16,7 +16,7 @@ export interface IServicelocator { | |||
| // request?: any, | |||
| // ); | |||
| getUsersDetailsById(userData: UserData, response: any); | |||
| updateUser(userDto?: any, response?: any); | |||
| updateUser( userDto?: any, response?: any); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any types with proper TypeScript types.
Using any type reduces type safety. Based on the related changes mentioned in the summary, the interface should be updated to use proper types that match the new user data structure with firstName, middleName, lastName, and gender fields.
- updateUser(userDto?: any, response?: any);
+ updateUser(userDto?: UserUpdateDTO, response?: Response): Promise<void>;Don't forget to add the import:
import { UserUpdateDTO } from 'src/user/dto/user-update.dto';🧰 Tools
🪛 eslint
[error] 19-19: Delete ·
(prettier/prettier)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor