feat(j-s): Add received date and new table state tag for indictment prison cases #17279
Conversation
…Date is populated
…s/add-acceptance-date-for-indictment
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces a new field Changes
Sequence DiagramsequenceDiagram
participant PA as Prison Admin
participant IC as Indictment Case
participant DS as Defendant Service
participant EL as Event Log
PA->>IC: Access Indictment
IC->>DS: Check Defendant Status
DS->>EL: Create Event Log
EL-->>DS: Log Created
DS->>IC: Update Defendant Status
IC->>PA: Display Opened Date
Possibly related PRs
Suggested reviewers
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
|
...-system/backend/src/app/modules/case/interceptors/defendantIndictmentAccessed.interceptor.ts
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
...-system/backend/src/app/modules/case/interceptors/defendantIndictmentAccessed.interceptor.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts
Show resolved
Hide resolved
thorhildurt
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (1)
Line range hint
303-307: Add await to prevent potential race conditionsThe call to
createDefendantEventshould be awaited to ensure the event is created before proceeding with the rest of the function execution.Apply this diff:
- this.createDefendantEvent({ + await this.createDefendantEvent({ caseId: theCase.id, defendantId: defendant.id, eventType: DefendantEventType.SENT_TO_PRISON_ADMIN, })
🧹 Nitpick comments (6)
apps/judicial-system/backend/src/app/modules/case/interceptors/defendantIndictmentAccessed.interceptor.ts (2)
37-40: Consider adding test coverage
Currently, there is no direct unit test for this interceptor's constructor or its dependencies. Although the constructor is straightforward, adding test coverage ensures changes to dependency injection or initialization logic are caught in the future.Would you like me to suggest a new test file or approach for ensuring coverage?
52-60: Handle possible errors from event creation
“createDefendantEvent” might throw exceptions (e.g., database issues). Right now, the interceptor does not handle or bubble up these errors. Consider adding error handling or logging to avoid silent failures.apps/judicial-system/backend/src/app/modules/defendant/models/defendantEventLog.model.ts (1)
23-35: Propose an index for performance
Accessing the first matching event type in an unsorted or large array may have performance implications at scale. An index or grouping by eventType in the database can optimize queries and reduce overhead.apps/judicial-system/backend/src/app/modules/case/interceptors/case.interceptor.ts (1)
20-28: Consider refactoring to reduce code duplication.The date retrieval logic is repeated with similar structure. Consider extracting a helper function to handle both cases.
export const transformDefendants = (defendants?: Defendant[]) => { + const getEventDate = (defendant: Defendant, eventType: DefendantEventType) => + DefendantEventLog.getDefendantEventLogTypeDate({ + defendantEventLogs: defendant.eventLogs, + eventType, + }); + return defendants?.map((defendant) => ({ ...defendant.toJSON(), sentToPrisonAdminDate: defendant.isSentToPrisonAdmin - ? DefendantEventLog.getDefendantEventLogTypeDate({ - defendantEventLogs: defendant.eventLogs, - eventType: DefendantEventType.SENT_TO_PRISON_ADMIN, - }) + ? getEventDate(defendant, DefendantEventType.SENT_TO_PRISON_ADMIN) : undefined, - openedByPrisonAdminDate: DefendantEventLog.getDefendantEventLogTypeDate({ - defendantEventLogs: defendant.eventLogs, - eventType: DefendantEventType.OPENED_BY_PRISON_ADMIN, - }), + openedByPrisonAdminDate: getEventDate( + defendant, + DefendantEventType.OPENED_BY_PRISON_ADMIN + ), })) }apps/judicial-system/web/src/components/Tags/utils.ts (1)
123-137: LGTM! Consider adding error logging for unexpected statesThe
getPrisonCaseStateTagfunction correctly implements the new prison case state handling, following existing patterns. However, consider adding error logging in the default case to track unexpected states.Consider this enhancement:
export const getPrisonCaseStateTag = ( prisonCaseState: CaseState, ): { color: TagVariant text: { id: string; defaultMessage: string; description: string } } => { switch (prisonCaseState) { case CaseState.NEW: return { color: 'purple', text: strings.new } case CaseState.RECEIVED: return { color: 'blue', text: strings.received } default: + if (process.env.NODE_ENV !== 'production') { + console.warn(`Unexpected prison case state: ${prisonCaseState}`) + } return { color: 'darkerBlue', text: strings.complete } } }apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx (1)
222-231: Simplify the prison case state determination logicThe current implementation has nested optional chaining that could be simplified for better readability.
Consider this cleaner implementation:
- const prisonCaseState = - row.defendants && - row.defendants?.length > 0 && - row.defendants[0].openedByPrisonAdminDate - ? CaseState.RECEIVED - : CaseState.NEW + const firstDefendant = row.defendants?.[0] + const prisonCaseState = firstDefendant?.openedByPrisonAdminDate + ? CaseState.RECEIVED + : CaseState.NEW
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/interceptors/case.interceptor.ts(2 hunks)apps/judicial-system/backend/src/app/modules/case/interceptors/defendantIndictmentAccessed.interceptor.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts(5 hunks)apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts(2 hunks)apps/judicial-system/backend/src/app/modules/defendant/models/defendantEventLog.model.ts(1 hunks)apps/judicial-system/web/src/components/FormProvider/case.graphql(1 hunks)apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.graphql(1 hunks)apps/judicial-system/web/src/components/Tags/utils.ts(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx(1 hunks)apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx(3 hunks)apps/judicial-system/web/src/routes/Shared/Cases/cases.graphql(1 hunks)apps/judicial-system/web/src/routes/Shared/Cases/prisonCases.graphql(1 hunks)libs/judicial-system/types/src/lib/eventLog.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (16)
apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
Pattern 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."
libs/judicial-system/types/src/lib/eventLog.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
apps/judicial-system/web/src/routes/Shared/Cases/cases.graphql (1)
Pattern 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/web/src/routes/Shared/Cases/prisonCases.graphql (1)
Pattern 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/api/src/app/modules/defendant/models/defendant.model.ts (1)
Pattern 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/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
Pattern 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/web/src/components/FormProvider/limitedAccessCase.graphql (1)
Pattern 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/interceptors/defendantIndictmentAccessed.interceptor.ts (1)
Pattern 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/case.service.ts (1)
Pattern 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/interceptors/case.interceptor.ts (1)
Pattern 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/defendant/defendant.service.ts (1)
Pattern 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/web/src/routes/Shared/Cases/PrisonCases.tsx (1)
Pattern 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/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts (1)
Pattern 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/defendant/models/defendantEventLog.model.ts (1)
Pattern 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/limitedAccessCase.controller.ts (1)
Pattern 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/web/src/components/Tags/utils.ts (1)
Pattern 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."
📓 Learnings (6)
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (2)
Learnt from: thorhildurt
PR: island-is/island.is#17198
File: apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx:42-50
Timestamp: 2024-12-11T14:25:44.741Z
Learning: In `IndictmentOverview.tsx`, when updating the punishment type, update the UI state before making the API call to immediately reflect the change.
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/api/src/app/modules/case/models/defendantEventLog.model.ts:6-22
Timestamp: 2024-11-18T21:50:40.004Z
Learning: In `apps/judicial-system/api/src/app/modules`, new models should be created following the existing patterns used in the API to maintain consistency.
apps/judicial-system/backend/src/app/modules/case/interceptors/case.interceptor.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/api/src/app/modules/case/models/defendantEventLog.model.ts:6-22
Timestamp: 2024-11-18T21:50:40.004Z
Learning: In `apps/judicial-system/api/src/app/modules`, new models should be created following the existing patterns used in the API to maintain consistency.
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/api/src/app/modules/case/models/defendantEventLog.model.ts:6-22
Timestamp: 2024-11-18T21:50:40.004Z
Learning: In `apps/judicial-system/api/src/app/modules`, new models should be created following the existing patterns used in the API to maintain consistency.
apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx (3)
Learnt from: oddsson
PR: island-is/island.is#14673
File: apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx:79-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The implementation of `mapIndictmentCaseStateToTagVariant` in `TagCaseState.tsx`, which maps only `CaseState.ACCEPTED` to a specific tag and defaults others to an "unknown" status, is intentional as per the application's requirements.
Learnt from: gudjong
PR: island-is/island.is#16760
File: apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx:112-118
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In `apps/judicial-system/web/src/routes/Prosecutor/components/CasesAwaitingConfirmationTable/CasesAwaitingConfirmationTable.tsx`, `apps/judicial-system/web/src/routes/Court/components/CasesAwaitingAssignmentTable/CasesAwaitingAssignmentTable.tsx`, and one instance in `apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx`, it's correct to pass only the `type` prop to `ColumnCaseType` without the `decision` and `parentCaseId` props.
Learnt from: thorhildurt
PR: island-is/island.is#17198
File: apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx:42-50
Timestamp: 2024-12-11T14:25:44.741Z
Learning: In `IndictmentOverview.tsx`, when updating the punishment type, update the UI state before making the API call to immediately reflect the change.
apps/judicial-system/backend/src/app/modules/defendant/models/defendantEventLog.model.ts (1)
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/api/src/app/modules/case/models/defendantEventLog.model.ts:6-22
Timestamp: 2024-11-18T21:50:40.004Z
Learning: In `apps/judicial-system/api/src/app/modules`, new models should be created following the existing patterns used in the API to maintain consistency.
🔇 Additional comments (17)
apps/judicial-system/backend/src/app/modules/case/interceptors/defendantIndictmentAccessed.interceptor.ts (2)
18-35: Ensure date compares align with business requirements
The logic checks if both "SENT_TO_PRISON_ADMIN" and "OPENED_BY_PRISON_ADMIN" events exist and confirms the “opened” event date is greater than or equal to the “sent” date. Verify that this aligns with real-world scenarios (e.g., time zones or partial case state updates) so that a subsequent event always follows the initial event properly.
46-50: Validate filtering conditions carefully
In the filter callback, you check for “isSentToPrisonAdmin” and then confirm that the relevant event logs are missing or invalid (“!hasValidOpenByPrisonAdminEvent(eventLogs)”). Make sure these conditions precisely reflect your desired scenario (i.e., a defendant with mixed states may inadvertently skip logging).
libs/judicial-system/types/src/lib/eventLog.ts (1)
18-18: Approved addition of OPENED_BY_PRISON_ADMIN
The new enum value is clear and consistent with existing naming.
apps/judicial-system/web/src/routes/Shared/Cases/prisonCases.graphql (1)
29-29: Verify that openedByPrisonAdminDate is used downstream
Adding a new field may require updating UI components, type definitions, and tests. Confirm that all references to this field in the UI handle the date appropriately.
apps/judicial-system/web/src/routes/Shared/Cases/cases.graphql (1)
31-31: LGTM! Verify field usage in frontend components.
The addition of openedByPrisonAdminDate to the defendants query is well-placed and aligns with the PR objectives.
✅ Verification successful
Field is properly integrated in frontend components
The openedByPrisonAdminDate field is correctly used in:
- PrisonCases component to determine case state (NEW vs RECEIVED)
- IndictmentOverview component to display the reception date in a formatted manner
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify frontend components using the new field
# Look for components that render the openedByPrisonAdminDate
rg -t tsx -t jsx "openedByPrisonAdminDate" apps/judicial-system/web/src/
Length of output: 100
Script:
#!/bin/bash
# Search for the new field usage in all files under the web directory
rg "openedByPrisonAdminDate" apps/judicial-system/web/src/
Length of output: 877
Script:
#!/bin/bash
# Check the usage context in the components
rg -A 5 -B 5 "openedByPrisonAdminDate" apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx
Length of output: 3025
apps/judicial-system/backend/src/app/modules/case/interceptors/case.interceptor.ts (1)
10-11: LGTM! Import addition is appropriate.
The DefendantEventType import is correctly added to support the new event type handling.
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts (2)
17-18: LGTM! Description update clarifies usage.
The description update from "Titill" to "Undirtitill" better reflects its usage as a subtitle.
19-22: LGTM! New message follows i18n patterns.
The new indictmentReceivedTitle message is well-structured with proper ID, templated date parameter, and clear description.
apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts (1)
113-115: LGTM! Field addition follows model conventions
The new openedByPrisonAdminDate field is properly typed and decorated for GraphQL, maintaining consistency with other date fields in the model.
Let's verify the field usage across the codebase:
✅ Verification successful
Field is properly integrated across the codebase
The openedByPrisonAdminDate field is consistently used throughout the codebase:
- Included in multiple GraphQL queries in web application
- Used in prison case views and indictment overview components
- Properly handled in backend interceptors for defendant event logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of openedByPrisonAdminDate field
# Look for GraphQL queries/mutations using this field
rg "openedByPrisonAdminDate" -t graphql
# Look for TypeScript/JavaScript usage
rg "openedByPrisonAdminDate" -t ts -t tsx
Length of output: 141
Script:
#!/bin/bash
# Search for openedByPrisonAdminDate across all files
rg "openedByPrisonAdminDate"
# Find all GraphQL schema files and check their content
fd -e graphql -x cat {}
# Find TypeScript files that might contain the field
fd -e ts -x cat {} | rg -A 5 -B 5 "openedByPrisonAdminDate"
Length of output: 67630
apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.graphql (1)
50-50: LGTM! GraphQL query field addition is correct
The openedByPrisonAdminDate field is properly added to the defendants object in the query, maintaining consistency with the model changes.
apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
38-38: LGTM: GraphQL schema addition is consistent
The new field openedByPrisonAdminDate is correctly placed within the defendants object and follows GraphQL schema conventions.
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
84-90: LGTM: UI enhancement follows React best practices
The conditional rendering of the received date is implemented correctly:
- Proper null checking with optional chaining
- Consistent use of formatDate utility
- Follows the component's existing pattern for displaying dates
apps/judicial-system/web/src/routes/Shared/Cases/PrisonCases.tsx (1)
222-238: Verify handling of multiple defendants
The current implementation uses only the first defendant's state to determine the case state. This might not accurately represent cases with multiple defendants.
Let's verify if there are cases with multiple defendants where this could be an issue:
✅ Verification successful
Multiple defendants are supported and should be handled correctly
The codebase clearly shows extensive support for multiple defendants:
- The UI components handle plural forms for multiple defendants (e.g., suffixes 'ar' vs 'i')
- There are specific handlers for deleting defendants when there's more than one
- PDF formatters adjust text based on defendant count
- The code contains multiple checks for
defendants.length > 1
The current implementation in PrisonCases.tsx that uses only the first defendant's state is actually correct because:
- The prison case state is a binary state (NEW/RECEIVED)
- When any defendant is received (has
openedByPrisonAdminDate), the case as a whole should be marked as RECEIVED - Using the first defendant is sufficient since all defendants in a case would be processed together in the prison workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for cases with multiple defendants having different openedByPrisonAdminDate values
ast-grep --pattern 'defendants: [
$_,
$_
]'
Length of output: 49
Script:
#!/bin/bash
# Let's try a different approach to find references to defendants array
# First, let's check the type definition for defendants
rg -A 5 "interface.*Case"
# Then, let's search for any array operations or checks on defendants
rg "defendants\?.*(length|map|filter|forEach)"
Length of output: 32839
Script:
#!/bin/bash
# Let's search for the Case interface definition and check if it supports multiple defendants
ast-grep --pattern 'interface Case {
$$$
}'
# Also search for any code that specifically handles multiple defendants
rg -B 2 -A 2 "defendants.*length > 1"
Length of output: 12319
apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts (2)
63-63: LGTM!
The import follows the project's conventions and is correctly placed with other interceptor imports.
91-95: LGTM!
The interceptor is correctly configured and placed in the appropriate order within the interceptor chain. This aligns with the PR objective of tracking when prison admins access indictment cases.
apps/judicial-system/backend/src/app/modules/defendant/defendant.service.ts (1)
274-288: LGTM!
The method is well-implemented with proper TypeScript types and follows best practices:
- Uses object destructuring for parameters
- Has proper async/await handling
- Follows the existing patterns in the API as per the learnings
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
420-429: LGTM!
The Sequelize include configuration is well-structured with proper performance optimizations:
- Uses
separate: trueto avoid cartesian product issues - Sets
required: falseto include records without events - Orders by
created DESCto efficiently retrieve latest events - Correctly filters events by type
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (1)
82-82: Consider adding more assertions for defender role behaviorWhile the test case correctly sets up a defender user, it could be more comprehensive.
Consider adding assertions to verify:
expect(user.role).toBe(UserRole.DEFENDER) expect(then.result.openedByDefender).toBeDefined()Also applies to: 86-86
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (1)
Pattern 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."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
🔇 Additional comments (3)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (3)
3-3: LGTM: Import addition aligns with type usage
The addition of UserRole import is necessary for the new role-based test case.
24-24: Good refactor: Improved test setup flexibility
The changes improve the test setup by:
- Introducing defaultUser for better reusability
- Making the user parameter optional with a sensible default
- Maintaining backward compatibility with existing tests
Also applies to: 45-56
17-17: Verify test coverage for all user roles
While the type signature change to include an optional user parameter is good, we should ensure we have test coverage for prison admin roles since that's the main focus of the PR.
Consider adding test cases for:
- Prison admin role
- Cases with received status
- OpenedByPrisonAdmin date handling
✅ Verification successful
Test coverage exists for prison admin role access
The codebase already has comprehensive test coverage for prison admin access through the prisonSystemStaffRule. Specifically:
- The
getByIdendpoint is tested with prison admin role access ingetByIdRolesRules.spec.ts - The controller properly implements
@RolesRules(prisonSystemStaffRule, defenderRule)for the relevant endpoints - Test coverage exists for prison admin access to court records, custody notices and other case-related documents
The PR's changes to add an optional user parameter are well-covered by the existing test infrastructure that verifies prison admin role permissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for prison admin role tests
rg -l "UserRole.PRISON_ADMIN" "apps/judicial-system/backend/src/app/modules/case/test"
Length of output: 84
Script:
#!/bin/bash
# Let's first check if UserRole enum is imported differently or defined in a different way
rg -A 2 "enum.*Role" apps/judicial-system/backend/src
# Also check for any prison admin related tests with different patterns
rg -i "prison.?admin" apps/judicial-system/backend/src/app/modules/case/test
# Check the specific test file content to understand the test structure
cat "apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts"
Length of output: 4995
Script:
#!/bin/bash
# Let's check for the shared types to understand the UserRole enum
rg -A 5 "enum UserRole" --type ts
# Let's also check other test files in the same directory for test patterns
fd . apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/ -e ts --exec cat {}
Length of output: 58383
Script:
#!/bin/bash
# Let's check for prison admin related tests in the entire codebase
rg -i "prison.?admin.*test" apps/judicial-system/backend/src
# Let's also check for any prison admin role usage in the controller
rg "UserRole.PRISON" apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.controller.ts
# Let's check the roles rules implementation
rg -A 5 "prisonSystemStaffRule" apps/judicial-system/backend/src
Length of output: 14081
…s/add-acceptance-date-for-indictment
gudjong
added
the
deprecated:automerge
…s/add-acceptance-date-for-indictment
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (1)
90-94: Consider improving the test description to include the user role.The test case correctly validates the behavior for defender roles, but the description could be more specific.
Consider updating the describe block to:
- describe('case exists and has not been opened by defender before', () => { + describe('case exists and has not been opened by defender before when accessed by a defender', () => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (1)
Pattern 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."
📓 Learnings (1)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
🔇 Additional comments (4)
apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getById.spec.ts (4)
3-3: LGTM! Clean import statement.
The import statement follows TypeScript best practices by explicitly importing both the type and enum.
17-21: LGTM! Well-structured type definition.
The type definition follows TypeScript best practices:
- Optional parameter is correctly placed last
- Clear parameter naming
- Proper type annotations
28-28: LGTM! Clear variable naming.
The variable name defaultUser clearly indicates its purpose as a default test user.
49-53: LGTM! Well-implemented test helper function.
The implementation:
- Correctly handles the optional user parameter
- Maintains backward compatibility with existing tests
- Matches the type definition perfectly
What
Why
newforeverTechnical context
OPENED_BY_PRISON_ADMINevents indefendantEventLogperdefendantOPENED_BY_PRISON_ADMINto all defendants on the fetched case where the event hasn't been created given the latest event state{ type: SENT_TO_PRISON_ADMIN, created: 01.01.2024}, {type: OPENED_BY_PRISON_ADMIN, created: 02.01.2024}{ type: SENT_TO_PRISON_ADMIN, created: 01.01.2024}, {type: OPENED_BY_PRISON_ADMIN, created: 01.01.2024}{ type: SENT_TO_PRISON_ADMIN, created: 01.01.2024}{ type: SENT_TO_PRISON_ADMIN, created: 01.01.2024}, {type: OPENED_BY_PRISON_ADMIN, created: 02.01.2024}, { type: SENT_TO_PRISON_ADMIN, created: 01.02.2024}openedByPrisonAdminDateto the prison case list, added via thetransformDefendantscase list interceptor logic, and if that is populated we show thereceivedstate otherwisenewScreenshots / Gifs
Screen.Recording.2024-12-18.at.10.27.16.mov
Checklist:
Summary by CodeRabbit
New Features
openedByPrisonAdminDateto theDefendantmodel, enhancing data capture for defendants.Bug Fixes
Documentation
Style
Chores
OPENED_BY_PRISON_ADMINto track additional defendant events.