feat(j-s): User National Id #17976
feat(j-s): User National Id #17976
Conversation
… users from the backend
|
View your CI Pipeline Execution ↗ for commit f042d2f.
☁️ Nx Cloud last updated this comment at |
WalkthroughThis pull request updates multiple modules to support handling user queries that return arrays rather than a single user object. In the authentication, case, and user modules, methods and tests have been refactored to retrieve and process user data as arrays. Additionally, the database migration adjusts the unique constraint on national IDs by including the institution ID. These changes improve the clarity of error handling and data processing without altering the core business logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Auth as AuthService
participant API as External API
Auth->>API: Call for user data (findUser)
API-->>Auth: Return JSON array of users
Auth->>Auth: Select first user from array
Auth-->>Caller: Return selected user
sequenceDiagram
participant C as UserController
participant S as UserService
participant DB as Database
C->>S: getByNationalId(nationalId)
S->>DB: findAll({nationalId})
DB-->>S: Return array of users
S-->>C: Return user array
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🔭 Outside diff range comments (2)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/create.spec.ts (1)
161-175: 🛠️ Refactor suggestionAdd test case for empty array scenario.
The "user not found" test currently checks for
undefined, but with the new array-based implementation, we should also verify the behavior when an empty array is returned.Apply this diff to add the test case:
describe('creating user not found', () => { + describe('when undefined is returned', () => { let then: Then beforeEach(async () => { const mockFindByNationalId = mockUserService.findByNationalId as jest.Mock mockFindByNationalId.mockResolvedValueOnce(undefined) then = await givenWhenThen(caseToCreate) }) it('should throw BadRequestException', () => { expect(then.error).toBeInstanceOf(BadRequestException) expect(then.error.message).toBe('Creating user not found') }) + }) + + describe('when empty array is returned', () => { + let then: Then + + beforeEach(async () => { + const mockFindByNationalId = mockUserService.findByNationalId as jest.Mock + mockFindByNationalId.mockResolvedValueOnce([]) + + then = await givenWhenThen(caseToCreate) + }) + + it('should throw BadRequestException', () => { + expect(then.error).toBeInstanceOf(BadRequestException) + expect(then.error.message).toBe('Creating user not found') + }) + }) })apps/judicial-system/backend/src/app/modules/user/test/getByNationalId.spec.ts (1)
14-17: 🛠️ Refactor suggestionUpdate interface to reflect array return type.
The
Theninterface should be updated to expect an array of users.interface Then { - result: User + result: User[] error: Error }
🧹 Nitpick comments (9)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/create.spec.ts (3)
116-116: Consider adding test coverage for multiple users.While the mock now correctly returns an array of users, the test only verifies the behavior with a single user in the array. Given that the system now supports multiple users with the same national ID, consider adding test cases that verify the behavior when multiple users are returned.
177-202: Update test description for clarity.The test description "creating user not from prosecution office" could be more specific about which user from the array is being validated.
Apply this diff to improve the test description:
-describe('creating user not from prosecution office', () => { +describe('first user not from prosecution office', () => {
288-291: Consider consolidating error test cases.The error test cases (case creation, defendant creation, and case lookup failures) follow the same pattern of mocking the user service to return an array. Consider using a shared setup to reduce code duplication.
Apply this diff to consolidate the setup:
+const createUserMock = (user: User) => { + const mockFindByNationalId = mockUserService.findByNationalId as jest.Mock + mockFindByNationalId.mockResolvedValueOnce([user]) + return mockFindByNationalId +} describe('case creation fails', () => { // ... user definition ... let then: Then beforeEach(async () => { - const mockFindByNationalId = mockUserService.findByNationalId as jest.Mock - mockFindByNationalId.mockResolvedValueOnce([user]) + createUserMock(user) then = await givenWhenThen(caseToCreate) }) // ... rest of the test ... })Also applies to: 341-344, 367-372, 395-403
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
1-1: Remove redundant 'use strict' directive.JavaScript modules are automatically in strict mode, making this directive unnecessary.
-'use strict'🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
apps/judicial-system/backend/src/app/modules/user/user.model.ts (1)
41-42: Update API documentation to reflect array return type.The
@ApiPropertydecorator should be updated to indicate that multiple users can share the same national ID.- @ApiProperty({ type: String }) + @ApiProperty({ type: String, description: 'National ID (can be shared by multiple users)' })apps/judicial-system/backend/src/app/modules/user/user.controller.ts (1)
95-98: Update API documentation for array return type.The API documentation needs to be updated to reflect that the endpoint now returns an array of users.
@ApiOkResponse({ type: User, + isArray: true, - description: 'Gets an existing user by national id', + description: 'Gets all users with the specified national id', })apps/judicial-system/backend/src/app/modules/user/test/getByNationalId.spec.ts (1)
72-91: Add test cases for edge scenarios.Consider adding test cases for:
- Empty array when no users found
- Array with more than two users
- Error handling for invalid national IDs
apps/judicial-system/backend/src/app/modules/user/user.service.ts (1)
57-95: LGTM! Method signature change aligns with PR objectives.The changes correctly implement the return of multiple users while maintaining backward compatibility by wrapping single users in arrays.
Consider improving error handling and admin user logic.
The error message could be more specific about handling multiple users, and the admin user logic could be more explicit about returning a single-element array.
Apply this diff to improve clarity:
async findByNationalId(nationalId: string): Promise<User[]> { // First check if the user is an admin try { const admin = this.config.adminUsers.find( (user: { nationalId: string }) => user.nationalId === nationalId, ) if (admin) { + // Return a single-element array for admin users + // This maintains consistency with the multiple users support return [ { created: nowFactory(), modified: nowFactory(), mobileNumber: '', email: '', role: UserRole.ADMIN, active: true, canConfirmIndictment: false, ...admin, } as User, ] } } catch (error) { // Tolerate failure, but log error this.logger.error('Failed to parse admin users', { error }) } const users = await this.userModel.findAll({ where: { nationalId, active: true }, include: [{ model: Institution, as: 'institution' }], }) if (!users || users.length === 0) { - throw new NotFoundException('User does not exist') + throw new NotFoundException('No active users found with the provided national ID') } return users }apps/judicial-system/api/src/app/modules/auth/auth.service.ts (1)
54-107:❓ Verification inconclusive
Consider aligning with multiple users approach.
Since the system is being updated to support multiple users per national ID, this method might need similar changes to handle multiple defenders.
Consider:
- Updating the method to handle an array of defenders.
- Adding selection criteria for defenders based on institution.
- Updating the return type to
Promise<User[]>.Let's verify if the defender endpoint has been updated to return multiple users:
🏁 Script executed:
#!/bin/bash # Search for defender endpoint implementation rg -A 10 "limitedAccess/defender.*nationalId" --type tsLength of output: 1122
Action Required: Update
findDefenderto Support Multiple DefendersThe current implementation in
apps/judicial-system/api/src/app/modules/auth/auth.service.tsstill treats the defender as a single user. Since the system is evolving to support multiple users per national ID, please consider refactoring this method as follows:
- Return Type: Change the return type to
Promise<User[]>instead of a singleUser.- Response Handling: Process the JSON response as an array of defenders.
- Selection Criteria: Implement appropriate selection or filtering (e.g., based on institution) to determine which defender(s) to use when multiple are available.
Our verification (via the search for the defender endpoint) indicates that the endpoint continues to return a single result. Further modifications may be necessary once the backend supports multiple defenders.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/judicial-system/api/src/app/modules/auth/auth.service.ts(2 hunks)apps/judicial-system/backend/migrations/20250214111730-update-user.js(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/pdf.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/create.spec.ts(7 hunks)apps/judicial-system/backend/src/app/modules/user/test/getByNationalId.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/user/user.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/user/user.model.ts(1 hunks)apps/judicial-system/backend/src/app/modules/user/user.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/migrations/20250214111730-update-user.jsapps/judicial-system/backend/src/app/modules/user/user.model.tsapps/judicial-system/backend/src/app/modules/case/internalCase.service.tsapps/judicial-system/backend/src/app/modules/user/user.controller.tsapps/judicial-system/backend/src/app/modules/user/test/getByNationalId.spec.tsapps/judicial-system/api/src/app/modules/auth/auth.service.tsapps/judicial-system/backend/src/app/modules/user/user.service.tsapps/judicial-system/backend/src/app/modules/case/test/internalCaseController/create.spec.tsapps/judicial-system/backend/src/app/modules/case/pdf.service.ts
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20250214111730-update-user.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (5)
apps/judicial-system/backend/src/app/modules/case/test/internalCaseController/create.spec.ts (1)
247-253: LGTM!The test for prosecutor representative creating indictment case correctly handles the array-based user response and verifies the appropriate case creation.
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
4-6: LGTM! Migration correctly removes the unique constraint.The
upmethod properly removes the unique constraint on the national_id column, aligning with the PR objective to support multiple users with the same national ID.apps/judicial-system/backend/src/app/modules/user/user.model.ts (1)
40-42: LGTM! Model changes align with database migration.The removal of the unique constraint from the nationalId column correctly implements the database schema change.
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
334-344: LGTM! Well-structured handling of multiple users.The changes correctly handle the array response with appropriate error checking and include a clear TODO about future improvements.
apps/judicial-system/api/src/app/modules/auth/auth.service.ts (1)
68-70: LGTM! Improved error handling structure.The explicit else block enhances code readability and maintainability.
apps/judicial-system/backend/src/app/modules/case/pdf.service.ts
Outdated
Show resolved
Hide resolved
…oational-id-not-unique
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
342-342: Track the TODO comment in the issue tracker.The TODO comment indicates a dependency on synchronization with LÖKE for user selection.
Would you like me to create an issue to track this TODO and the implementation of the user selection strategy? The issue can include:
- Requirement to sync with LÖKE
- Implementation of user selection criteria
- Testing scenarios for multiple users
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests (judicial-system-backend)
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
1-1: Remove redundant 'use strict' directive.JavaScript modules are automatically in strict mode, making this directive unnecessary.
-'use strict'🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/backend/migrations/20250214111730-update-user.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/migrations/20250214111730-update-user.js
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20250214111730-update-user.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (1)
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
4-13:❓ Verification inconclusive
LGTM! Migration aligns with PR objectives.
The migration correctly implements the required changes to support multiple users with the same national ID while maintaining uniqueness per institution.
Let's verify the constraint changes:
🏁 Script executed:
#!/bin/bash # Description: Verify constraint changes in the database schema # Search for any remaining unique constraints on national_id rg -A 5 "national_id.*unique" apps/judicial-system/backend/src/Length of output: 65
LGTM! Migration aligns with PR objectives.
The migration correctly removes the existing unique constraint on "national_id" and adds a composite unique constraint ("unique_national_id_per_institution") to ensure uniqueness per institution. Although the earlier grep in the
apps/judicial-system/backend/src/directory showed no trace of a unique constraint on "national_id", this check did not cover the entire codebase. Please manually verify that:
- The legacy constraint
"user_national_id_key"does not appear anywhere else in the repository.- The new composite constraint
"unique_national_id_per_institution"is properly applied without conflict.
apps/judicial-system/backend/migrations/20250214111730-update-user.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
15-27:⚠️ Potential issueFix redundant constraints in down migration.
The down migration has redundant constraints:
- It attempts to add a unique constraint on 'national_id' while also having a composite unique constraint
- Having both constraints is redundant and potentially conflicting
Apply this diff:
down(queryInterface, Sequelize) { return Promise.all([ queryInterface.changeColumn('user', 'national_id', { type: Sequelize.STRING, unique: true, allowNull: false, }), - queryInterface.removeConstraint( - 'user', - 'unique_national_id_per_institution', - ), ]) },apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (1)
338-339: 🛠️ Refactor suggestionConsider implementing a more robust user selection strategy.
The current implementation arbitrarily selects the first prosecution user without specific criteria. This could lead to incorrect prosecutor selection when multiple users are associated with the same national ID.
Consider implementing a selection strategy based on specific criteria, such as:
- User's institution/office
- User's role or seniority
- Active status
- Case load
- // TODO: Sync with LÖKE so we can select the correct user - const creator = users?.find((user) => isProsecutionUser(user)) + const creator = await this.selectAppropriateUser(users, caseToCreate)Add a new method to handle user selection:
private async selectAppropriateUser(users: User[], caseToCreate: InternalCreateCaseDto): Promise<User> { // Implement selection logic based on business requirements // Example: Select user by institution const appropriateUser = users.find(user => user.institution?.id === caseToCreate.prosecutorsOfficeId ); return appropriateUser ?? users[0]; // Fallback to first user if no match }
🧹 Nitpick comments (1)
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
1-2: Remove redundant 'use strict' directive.The 'use strict' directive is redundant as JavaScript modules are automatically in strict mode.
Apply this diff:
-'use strict' -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/judicial-system/api/src/app/modules/auth/auth.service.ts(2 hunks)apps/judicial-system/backend/migrations/20250214111730-update-user.js(1 hunks)apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/api/src/app/modules/auth/auth.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
`apps/**/*`: "Confirm that the code adheres to the following...
apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/case/internalCase.service.tsapps/judicial-system/backend/migrations/20250214111730-update-user.js
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20250214111730-update-user.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: tests (judicial-system-backend)
🔇 Additional comments (3)
apps/judicial-system/backend/migrations/20250214111730-update-user.js (1)
4-13: LGTM! The up migration correctly implements the national ID constraint changes.The migration properly removes the unique constraint on 'national_id' and adds a composite unique constraint on 'national_id' and 'institution_id', which aligns with the PR objective to support multiple users with the same national ID.
apps/judicial-system/backend/src/app/modules/case/internalCase.service.ts (2)
334-336: LGTM! User retrieval now supports multiple users.The code correctly handles the new array return type from
findByNationalId, aligning with the PR objective to support multiple users with the same national ID.
341-345: LGTM! Error handling is clear and accurate.The error message accurately reflects the validation requirements for creating a case, checking both user existence and prosecution role.
User National Id
Taka unique constraint af kennitölu og skila lista af notendum úr bakenda
What
Remaining work
Why
Checklist:
Summary by CodeRabbit
New Features
Refactor