Task #259635 chore: Implement audit logs for Center Management Batch …#767
Open
kkbiswal wants to merge 11 commits into
Open
Task #259635 chore: Implement audit logs for Center Management Batch …#767kkbiswal wants to merge 11 commits into
kkbiswal wants to merge 11 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (7)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds the ChangesAudit Logging Integration
Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestContextMiddleware
participant requestContext
participant ServiceMethod
participant getAuditContext
participant AuditLoggerService
participant KafkaTopic
Client->>RequestContextMiddleware: HTTP request (with JWT/user headers)
RequestContextMiddleware->>requestContext: run(req, next)
requestContext->>ServiceMethod: executes within async context
ServiceMethod->>requestContext: getStore() → req
ServiceMethod->>getAuditContext: getAuditContext()
getAuditContext->>requestContext: getStore() → actorId, actorName, userRole, ip, userAgent
getAuditContext-->>ServiceMethod: auditCtx
ServiceMethod->>AuditLoggerService: emitAuditSafely({ entityType, eventAction, entityId, metadata, ...auditCtx })
AuditLoggerService->>KafkaTopic: publish to audit.events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates the @tekdi/audit-logger library and introduces an AsyncLocalStorage-based request context middleware to globally manage request data, allowing the removal of explicitly passed request parameters across various controllers and services. Audit log emissions have been added for key lifecycle events across cohorts, cohort members, fields, roles, permissions, users, and tenant mappings. The review feedback identifies several critical issues, including an undefined res variable in cohortMembers.service.ts that will cause runtime crashes, a missing early return in fields.service.ts leading to a headers-already-sent error, a duplicate and unsafe audit log emission in user.service.ts, a problematic comma-joined entityId in cohort.service.ts, and an unused request variable in user.service.ts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/fields/fields.service.ts (1)
897-926:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
returncauses audit emission on failure path.When
resultis falsy (line 897),APIResponse.error()is called but the function continues to execute the success path: callingAPIResponse.success()and emitting aCREATEDaudit event. This results in:
- Double response write (corrupted HTTP response or "headers already sent" error)
- False audit trail showing a
CREATEDevent for a failed operation🐛 Proposed fix: add return after error response
if (!result) { - APIResponse.error( + return APIResponse.error( res, apiId, `Fields not found or already exist`, `Fields not found or already exist`, HttpStatus.NOT_FOUND ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/fields/fields.service.ts` around lines 897 - 926, The error handling block for when result is falsy is missing a return statement after calling APIResponse.error(). This causes the function to continue executing the success path, resulting in both an error and success response being sent along with a false audit trail. Add a return statement immediately after the APIResponse.error() call to prevent execution from reaching the APIResponse.success() call and the subsequent audit logger emit that follows it.src/permissionRbac/rolePermissionMapping/role-permission-mapping.service.ts (1)
112-135:⚠️ Potential issue | 🟠 MajorGate audit emission on successful row changes to prevent false UPDATED/DELETED events.
Both
update()anddelete()emit audit logs unconditionally, even when no rows are affected (affected=0). This produces incorrect audit trails for no-op operations. Check theaffectedfield before emitting UPDATED/DELETED events:Example fix for update()
let result = await this.rolePermissionRepository.update( rolePermissionCreateDto.rolePermissionId, { roleTitle: rolePermissionCreateDto.roleTitle, apiPath: rolePermissionCreateDto.apiPath, requestType: rolePermissionCreateDto.requestType, module: rolePermissionCreateDto.module, } ); const apiRes = APIResponse.success( response, apiId, result, HttpStatus.OK, "Permission updated succesfully." ); - const auditCtx = getAuditContext(); - this.auditLoggerService.emit({ - entityType: "ROLE_PERMISSION", - entityId: rolePermissionCreateDto.rolePermissionId || "N/A", - eventAction: "UPDATED", - ...auditCtx - }); + if ((result.affected ?? 0) > 0) { + const auditCtx = getAuditContext(); + this.auditLoggerService.emit({ + entityType: "ROLE_PERMISSION", + entityId: rolePermissionCreateDto.rolePermissionId || "N/A", + eventAction: "UPDATED", + ...auditCtx + }); + }Apply the same pattern to the
deletePermission()method.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/permissionRbac/rolePermissionMapping/role-permission-mapping.service.ts` around lines 112 - 135, The audit log emission in the method handling role permission updates is occurring unconditionally, even when the repository update operation does not affect any rows. Check the affected field from the result returned by rolePermissionRepository.update() and only emit the audit event via auditLoggerService.emit() when affected is greater than zero to prevent false UPDATED events for no-op operations. Apply the same conditional logic pattern to the deletePermission() method to prevent false DELETED events.
🧹 Nitpick comments (8)
src/user/user.controller.ts (1)
164-165: 💤 Low valueUnused parameter:
@Req() requestis injected but never used.The
requestparameter was added to the method signature but is never referenced in the method body. The service now reads the request context internally viarequestContext.getStore(). This dead parameter should be removed.public async updateUser( - `@Req`() request: Request, `@Headers`() headers, `@Param`("userid") userId: string,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/user/user.controller.ts` around lines 164 - 165, The updateUser method in the user controller has an unused `@Req`() request parameter that was added to the method signature but is never referenced in the method body. Since the service now reads the request context internally via requestContext.getStore(), this dead parameter should be removed. Delete the `@Req`() request: Request parameter from the updateUser method signature entirely to clean up the unused import.src/user/user.service.ts (2)
3457-3459: 💤 Low valueUnused variable:
requestis obtained but never used.The request variable is assigned but never referenced in this method.
async suggestUsername(response: Response, suggestUserDto: SuggestUserDto) { - const request = requestContext.getStore() as any; - const apiId = APIID.USER_LIST;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/user/user.service.ts` around lines 3457 - 3459, Remove the unused variable assignment in the suggestUsername method in src/user/user.service.ts. The request variable is assigned from requestContext.getStore() but never referenced elsewhere in the method, so delete the line that declares and assigns this unused variable.
139-139: 💤 Low valueUnused
requestvariables across multiple methods after parameter removal refactoring.Several methods obtain
requestfromrequestContext.getStore()but never reference the variable. These are leftover assignments from the migration pattern where request parameters were removed. Consider removing these dead assignments for code clarity.
src/user/user.service.ts#L139: Remove unusedrequestinsendPasswordResetLinksrc/user/user.service.ts#L256: Remove unusedrequestinforgotPasswordsrc/user/user.service.ts#L424: Remove unusedrequestinsearchUserMultiTenantsrc/user/user.service.ts#L1275: Remove unusedrequestinupdateUsersrc/user/user.service.ts#L3458: Remove unusedrequestinsuggestUsername🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/user/user.service.ts` at line 139, Remove the unused request variable assignments that were left over from a prior refactoring where request parameters were removed from method signatures. In src/user/user.service.ts at line 139 (sendPasswordResetLink), line 256 (forgotPassword), line 424 (searchUserMultiTenant), line 1275 (updateUser), and line 3458 (suggestUsername), delete the line `const request = requestContext.getStore() as any;` from each method since the request variable is never referenced in any of these methods' implementations.src/rbac/assign-role/assign-role.service.ts (1)
163-163: ⚡ Quick winUnused variable
requestingetAssignedRole.The
requestvariable is fetched from context but never referenced in the method body. Remove the dead code.Proposed fix
public async getAssignedRole( userId: string, response: Response ) { - const request = requestContext.getStore() as any; const apiId = APIID.USERROLE_GET;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rbac/assign-role/assign-role.service.ts` at line 163, The variable `request` is assigned from `requestContext.getStore() as any` in the `getAssignedRole` method but is never used anywhere in the method body. Remove the unused variable assignment line entirely to clean up the dead code.src/rbac/assign-role/assign-role.controller.ts (1)
100-100: 💤 Low valueUnused
requestparameters indeleteRoleandbulkUpdateUserRoles.Both methods inject
@Req() requestbut never pass it to the service—AssignRoleServicereads fromrequestContextinternally. These are dead code unless intentionally kept for documentation purposes.Consider removing them for consistency with
createandgetRolewhich no longer injectrequest, or add a comment explaining the intent if they're kept for future use.Also applies to: 118-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rbac/assign-role/assign-role.controller.ts` at line 100, Remove the unused `@Req() request` parameter from both the `deleteRole` method (at line 100) and the `bulkUpdateUserRoles` method (at line 118) in the assign-role.controller.ts file. These parameters are never used within the methods and are inconsistent with the `create` and `getRole` methods which do not inject the request parameter. Since the service uses `requestContext` internally instead of the injected request, deleting these parameters will clean up the code and improve consistency across all controller methods.src/rbac/role/role.controller.ts (1)
127-132: 💤 Low valueUnused
@Req() requestparameters in role and assign-role controllers. All three endpoints inject the Express request but never pass it to the service layer, which reads fromrequestContext.getStore()internally. This creates dead code across multiple controllers.
src/rbac/role/role.controller.ts#L127-L132: Remove unusedrequestfromdeleteRoleor document why it's retained.src/rbac/assign-role/assign-role.controller.ts#L100-L106: Remove unusedrequestfromdeleteRoleor document why it's retained.src/rbac/assign-role/assign-role.controller.ts#L117-L135: Remove unusedrequestfrombulkUpdateUserRolesor document why it's retained.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rbac/role/role.controller.ts` around lines 127 - 132, The `@Req`() request parameter is injected but never used in three controller methods across two files; the service layer instead accesses request context via requestContext.getStore() internally. Remove the unused `@Req`() request: Request parameter from the following methods: deleteRole method in src/rbac/role/role.controller.ts at lines 127-132, deleteRole method in src/rbac/assign-role/assign-role.controller.ts at lines 100-106, and bulkUpdateUserRoles method in src/rbac/assign-role/assign-role.controller.ts at lines 117-135. This eliminates dead code while maintaining the same functionality since the services do not depend on the request parameter being passed.src/rbac/role/role.service.ts (2)
161-161: ⚡ Quick winUnused variable
requestinupdateRole.The
requestvariable is fetched from context on line 161 but never referenced—getAuditContext()internally reads from the same store. Remove the unused declaration to avoid dead code and potential reader confusion.Proposed fix
public async updateRole( roleId: string, roleDto: RoleDto, response: Response ) { - const request = requestContext.getStore() as any; const apiId = APIID.ROLE_UPDATE;Also applies to: 167-175
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rbac/role/role.service.ts` at line 161, The updateRole method declares a request variable by calling requestContext.getStore() on line 161 but never uses it, creating dead code. Remove this unused variable declaration since getAuditContext() internally reads from the same store and makes the explicit extraction redundant. Apply the same fix to the additional occurrences of this pattern mentioned at lines 167-175 within the same method.
335-336: ⚡ Quick winUnused variable
requestindeleteRole.Same issue:
requestis declared but never used becausegetAuditContext()reads from the store internally. Remove the unused variable.Proposed fix
public async deleteRole(roleId: string, res: Response) { - const request = requestContext.getStore() as any; - const apiId = APIID.ROLE_DELETE;Also applies to: 365-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/rbac/role/role.service.ts` around lines 335 - 336, The variable request declared via requestContext.getStore() in the deleteRole method is never used and should be removed. Since getAuditContext() reads from the store internally, this variable is redundant. Remove the unused request variable declaration from the deleteRole method. The same unused variable pattern exists at other locations in the file (lines 365-372), so remove the unused request variable from all affected methods where it appears but is not referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cohortMembers/cohortMembers.service.ts`:
- Around line 813-829: The auditLoggerService.emit call for COHORT_MEMBER update
events is breaking the established audit event contract by nesting request
metadata under a context object and renaming the userAgent field to platform.
Refactor the emit payload to use the shared/established audit context shape that
is consistent with other services in the codebase. Flatten the nested context
structure and use the correct field names (such as userAgent instead of
platform) that align with the standard audit event contract expected by
downstream consumers.
In `@src/permissionRbac/rolePermissionMapping/role-permission-mapping.service.ts`:
- Around line 86-92: The auditLoggerService.emit() call in the ROLE_PERMISSION
creation handler is not awaited and shares the same try/catch block as the
database operation, which means if emit() throws synchronously or returns a
rejected Promise, it will cause an error response to be sent to the client even
after successful database mutations. Decouple the audit emission from the CRUD
response flow by moving the auditLoggerService.emit() call outside the try/catch
block that wraps the database operation, and handle any errors from emit()
separately using a .catch() handler or similar pattern to ensure that audit
logging failures never interfere with returning successful API responses to the
client. Apply this same decoupling pattern to all other locations in the service
where auditLoggerService.emit() is called without awaiting.
In `@src/rbac/assign-role/assign-role.service.ts`:
- Line 42: The code is accessing request["user"].userId without null safety
after calling requestContext.getStore(), which can return undefined and cause a
null-pointer exception. Replace the unsafe access of request["user"].userId with
actorId retrieved from getAuditContext() instead, ensuring consistency with the
audit emit that follows and eliminating the null-safety risk when the request
context is unavailable.
- Around line 269-284: The audit log emit call in the deleteAssignedRole method
uses inconsistent field structure compared to createAssignRole—it manually
extracts user and request details inline and wraps context fields in a nested
object with platform, while createAssignRole uses getAuditContext() which
spreads top-level fields including userAgent. Refactor the audit emit in the
deleteAssignRoleDto roleId loop to call getAuditContext() (passing the request
object) and spread its returned object into the emit payload to match the
consistent schema used by createAssignRole, removing the manual inline field
extraction and the nested context wrapper.
- Around line 338-351: The audit payload in the bulkUpdateUserRoles method uses
inline extraction with a context wrapper instead of the getAuditContext() helper
function, and also places tenantId inside context which is inconsistent with
other methods like createAssignRole that use metadata. Refactor the
auditLoggerService.emit call to use the getAuditContext() method instead of
manually extracting actor and context details, ensuring tenantId placement is
consistent with other audit emits in the file (such as createAssignRole). After
refactoring, remove the now-unused request variable that was previously declared
on line 318.
In `@src/rbac/role/role.service.ts`:
- Line 40: In src/rbac/role/role.service.ts, the code at line 40 assigns the
result of requestContext.getStore() to request without handling the case where
it returns undefined, which causes a null-pointer exception at lines 88-90 when
attempting to access request.user.userId without optional chaining. Replace the
direct assignment and usage pattern at line 40 (the request variable
declaration) and lines 88-90 (where request.user.userId is accessed) by instead
using the existing getAuditContext() helper method which already provides proper
null-safety with optional chaining and fallbacks. After making this change,
remove the now-unused request variable declaration at line 40 since
getAuditContext() will handle the context retrieval safely.
In `@src/user/user.service.ts`:
- Around line 2864-2876: The deleteUserById method is emitting the DELETED audit
event twice, creating duplicate audit records. Remove one of the two audit log
emissions for the DELETED event - either the one using direct request property
access or the one using getAuditContext(). Keep only the emission that provides
complete and consistent audit context information to avoid duplication while
maintaining full audit trail coverage.
In `@src/userTenantMapping/user-tenant-mapping.service.ts`:
- Around line 162-171: The USER_TENANT_MAPPING audit events use inconsistent
entityId formats: the CREATED event (in the auditLoggerService.emit call around
the composite string format) uses a composite string pattern combining userId,
tenantId, and roleId, while the UPDATED event uses the database ID. This
inconsistency prevents reliable lifecycle correlation in audit consumers.
Standardize the entityId scheme across both the CREATED event and the UPDATED
event (also applies to lines 397-406) by selecting one format and applying it
consistently to all USER_TENANT_MAPPING audit events so that the same mapping
can be reliably tracked across all lifecycle actions.
---
Outside diff comments:
In `@src/fields/fields.service.ts`:
- Around line 897-926: The error handling block for when result is falsy is
missing a return statement after calling APIResponse.error(). This causes the
function to continue executing the success path, resulting in both an error and
success response being sent along with a false audit trail. Add a return
statement immediately after the APIResponse.error() call to prevent execution
from reaching the APIResponse.success() call and the subsequent audit logger
emit that follows it.
In `@src/permissionRbac/rolePermissionMapping/role-permission-mapping.service.ts`:
- Around line 112-135: The audit log emission in the method handling role
permission updates is occurring unconditionally, even when the repository update
operation does not affect any rows. Check the affected field from the result
returned by rolePermissionRepository.update() and only emit the audit event via
auditLoggerService.emit() when affected is greater than zero to prevent false
UPDATED events for no-op operations. Apply the same conditional logic pattern to
the deletePermission() method to prevent false DELETED events.
---
Nitpick comments:
In `@src/rbac/assign-role/assign-role.controller.ts`:
- Line 100: Remove the unused `@Req() request` parameter from both the
`deleteRole` method (at line 100) and the `bulkUpdateUserRoles` method (at line
118) in the assign-role.controller.ts file. These parameters are never used
within the methods and are inconsistent with the `create` and `getRole` methods
which do not inject the request parameter. Since the service uses
`requestContext` internally instead of the injected request, deleting these
parameters will clean up the code and improve consistency across all controller
methods.
In `@src/rbac/assign-role/assign-role.service.ts`:
- Line 163: The variable `request` is assigned from `requestContext.getStore()
as any` in the `getAssignedRole` method but is never used anywhere in the method
body. Remove the unused variable assignment line entirely to clean up the dead
code.
In `@src/rbac/role/role.controller.ts`:
- Around line 127-132: The `@Req`() request parameter is injected but never used
in three controller methods across two files; the service layer instead accesses
request context via requestContext.getStore() internally. Remove the unused
`@Req`() request: Request parameter from the following methods: deleteRole method
in src/rbac/role/role.controller.ts at lines 127-132, deleteRole method in
src/rbac/assign-role/assign-role.controller.ts at lines 100-106, and
bulkUpdateUserRoles method in src/rbac/assign-role/assign-role.controller.ts at
lines 117-135. This eliminates dead code while maintaining the same
functionality since the services do not depend on the request parameter being
passed.
In `@src/rbac/role/role.service.ts`:
- Line 161: The updateRole method declares a request variable by calling
requestContext.getStore() on line 161 but never uses it, creating dead code.
Remove this unused variable declaration since getAuditContext() internally reads
from the same store and makes the explicit extraction redundant. Apply the same
fix to the additional occurrences of this pattern mentioned at lines 167-175
within the same method.
- Around line 335-336: The variable request declared via
requestContext.getStore() in the deleteRole method is never used and should be
removed. Since getAuditContext() reads from the store internally, this variable
is redundant. Remove the unused request variable declaration from the deleteRole
method. The same unused variable pattern exists at other locations in the file
(lines 365-372), so remove the unused request variable from all affected methods
where it appears but is not referenced.
In `@src/user/user.controller.ts`:
- Around line 164-165: The updateUser method in the user controller has an
unused `@Req`() request parameter that was added to the method signature but is
never referenced in the method body. Since the service now reads the request
context internally via requestContext.getStore(), this dead parameter should be
removed. Delete the `@Req`() request: Request parameter from the updateUser method
signature entirely to clean up the unused import.
In `@src/user/user.service.ts`:
- Around line 3457-3459: Remove the unused variable assignment in the
suggestUsername method in src/user/user.service.ts. The request variable is
assigned from requestContext.getStore() but never referenced elsewhere in the
method, so delete the line that declares and assigns this unused variable.
- Line 139: Remove the unused request variable assignments that were left over
from a prior refactoring where request parameters were removed from method
signatures. In src/user/user.service.ts at line 139 (sendPasswordResetLink),
line 256 (forgotPassword), line 424 (searchUserMultiTenant), line 1275
(updateUser), and line 3458 (suggestUsername), delete the line `const request =
requestContext.getStore() as any;` from each method since the request variable
is never referenced in any of these methods' implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c3dac30-3d32-4440-9893-868a3e8773b8
📒 Files selected for processing (23)
package.jsonsrc/app.module.tssrc/cohort/cohort.controller.tssrc/cohort/cohort.service.tssrc/cohortMembers/cohortMembers.controller.tssrc/cohortMembers/cohortMembers.service.tssrc/common/middleware/request-context.middleware.tssrc/common/utils/audit-helper.tssrc/common/utils/request-context.tssrc/fields/fields.controller.tssrc/fields/fields.service.tssrc/permissionRbac/rolePermissionMapping/role-permission-mapping.controller.tssrc/permissionRbac/rolePermissionMapping/role-permission-mapping.service.tssrc/rbac/assign-role/assign-role.controller.tssrc/rbac/assign-role/assign-role.service.tssrc/rbac/role/role.controller.tssrc/rbac/role/role.service.tssrc/sso/sso.service.tssrc/user/user.controller.tssrc/user/user.service.tssrc/userTenantMapping/user-tenant-mapping.controller.tssrc/userTenantMapping/user-tenant-mapping.service.tstsconfig.json
💤 Files with no reviewable changes (1)
- src/sso/sso.service.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cohortMembers/cohortMembers.service.ts (2)
925-931:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the method
resin the catch path, not the imported Expressresponseprototype.Line 926 passes
responseinstead ofres; this can break error responses during exception handling.🐛 Minimal fix
- return APIResponse.error( - response, + return APIResponse.error( + res, apiId, API_RESPONSES.INTERNAL_SERVER_ERROR, `Error : ${error.message}`, HttpStatus.INTERNAL_SERVER_ERROR );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cohortMembers/cohortMembers.service.ts` around lines 925 - 931, In the catch block of the getCohortMembers or related error handling, the APIResponse.error call is passing the imported Express response prototype instead of the actual res parameter that was passed to the function. Replace the `response` argument with `res` in the APIResponse.error() method call to ensure error responses are properly sent through the correct response object.
785-797:⚠️ Potential issue | 🔴 CriticalThe validation check must use the object's
isValidproperty, not check the object itself.The
validateCustomFieldByContextmethod always returns an object ({ isValid: boolean, error?: string }). The conditionif (!customFieldValidate)will never be true because objects are truthy in JavaScript—invalid validation results will pass through this check uncaught.Change line 791 to:
if (!customFieldValidate.isValid) {This allows invalid custom field payloads to proceed past validation, creating a security and data integrity issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cohortMembers/cohortMembers.service.ts` around lines 785 - 797, The validation check for customFieldValidate is incorrectly testing the object itself with if (!customFieldValidate) rather than checking its isValid property. Since validateCustomFieldByContext returns an object (which is always truthy), this condition will never properly detect validation failures. Fix this by changing the condition to check customFieldValidate.isValid instead of the entire object. This change must be made in the conditional statement immediately following the validateCustomFieldByContext method call in the cohortMembers.service.ts file to ensure invalid custom field payloads are properly rejected.
🧹 Nitpick comments (1)
src/cohort/cohort.service.ts (1)
505-511: ⚡ Quick winRemove the unused
requestparameter fromupdateCohortStatuses.
getAuditContext()is already used for actor/context resolution, sorequestis dead input and forces unnecessary controller/service coupling.♻️ Suggested cleanup
- public async updateCohortStatuses( - request: any, - cohortIds: string[], + public async updateCohortStatuses( + cohortIds: string[], status: string, updatedBy: string, res ) {As per coding guidelines, this should adhere to NestJS best practices and maintain similar naming/parameter conventions across services.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cohort/cohort.service.ts` around lines 505 - 511, The `updateCohortStatuses` method has an unused `request` parameter that creates unnecessary coupling between the controller and service layers. Remove the `request` parameter from the method signature since context and actor information is already resolved via the `getAuditContext()` call within the method. Update any callers of this method to pass only the remaining parameters: cohortIds, status, updatedBy, and res.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/cohortMembers/cohortMembers.service.ts`:
- Around line 925-931: In the catch block of the getCohortMembers or related
error handling, the APIResponse.error call is passing the imported Express
response prototype instead of the actual res parameter that was passed to the
function. Replace the `response` argument with `res` in the APIResponse.error()
method call to ensure error responses are properly sent through the correct
response object.
- Around line 785-797: The validation check for customFieldValidate is
incorrectly testing the object itself with if (!customFieldValidate) rather than
checking its isValid property. Since validateCustomFieldByContext returns an
object (which is always truthy), this condition will never properly detect
validation failures. Fix this by changing the condition to check
customFieldValidate.isValid instead of the entire object. This change must be
made in the conditional statement immediately following the
validateCustomFieldByContext method call in the cohortMembers.service.ts file to
ensure invalid custom field payloads are properly rejected.
---
Nitpick comments:
In `@src/cohort/cohort.service.ts`:
- Around line 505-511: The `updateCohortStatuses` method has an unused `request`
parameter that creates unnecessary coupling between the controller and service
layers. Remove the `request` parameter from the method signature since context
and actor information is already resolved via the `getAuditContext()` call
within the method. Update any callers of this method to pass only the remaining
parameters: cohortIds, status, updatedBy, and res.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 902e720f-8fb6-4be1-ad64-f2a8939247d3
📒 Files selected for processing (10)
src/cohort/cohort.service.tssrc/cohortMembers/cohortMembers.service.tssrc/common/utils/request-context.tssrc/fields/fields.service.tssrc/permissionRbac/rolePermissionMapping/role-permission-mapping.service.tssrc/rbac/assign-role/assign-role.service.tssrc/rbac/role/role.service.tssrc/sso/sso.service.tssrc/user/user.service.tssrc/userTenantMapping/user-tenant-mapping.service.ts
💤 Files with no reviewable changes (1)
- src/sso/sso.service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/common/utils/request-context.ts
- src/userTenantMapping/user-tenant-mapping.service.ts
- src/user/user.service.ts
- src/fields/fields.service.ts
Shubham4026
reviewed
Jul 8, 2026
Shubham4026
reviewed
Jul 8, 2026
Shubham4026
reviewed
Jul 8, 2026
snehal0904
reviewed
Jul 13, 2026
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…Management
Summary by CodeRabbit
Release Notes
New Features
Chores