feat(j-s): Allow multiple subtypes in indictments #17192
Conversation
…island.is into j-s/select-indictment-subtype
…island.is into j-s/select-indictment-subtype
...ial-system/web/src/routes/Prosecutor/Indictments/Defendant/PoliceCaseInfo/PoliceCaseInfo.tsx
Outdated
Show resolved
Hide resolved
...ial-system/web/src/routes/Prosecutor/Indictments/Defendant/PoliceCaseInfo/PoliceCaseInfo.tsx
Show resolved
Hide resolved
…into j-s/multi-subtype-2
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/judicial-system/web/src/utils/validate.ts (1)
311-327: Consider renaming validation functions to follow isValid pattern.Following the suggestion from @thorhildurt, consider renaming these functions to follow the
isValidpattern since they return booleans:
validateTrafficViolation→isValidTrafficViolationvalidateNonTrafficViolation→isValidNonTrafficViolation
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/PoliceCaseInfo/PoliceCaseInfo.tsx(8 hunks)apps/judicial-system/web/src/utils/validate.spec.ts(2 hunks)apps/judicial-system/web/src/utils/validate.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/PoliceCaseInfo/PoliceCaseInfo.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/web/src/utils/validate.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."
apps/judicial-system/web/src/utils/validate.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/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
🔇 Additional comments (7)
apps/judicial-system/web/src/utils/validate.spec.ts (5)
308-317: LGTM! Basic validation test cases are comprehensive.
The test cases for basic validation (demands, civil claims) cover the essential edge cases and error conditions.
331-342: LGTM! Indictment subtypes validation is well tested.
The test case properly verifies that the function returns false when there are no indictment subtypes in a case.
344-378: LGTM! Pure traffic violations test case is thorough.
The test case for pure traffic violations is comprehensive, covering:
- Multiple indictment counts
- Required fields for traffic violations
- Proper subtype assignment
380-415: LGTM! Invalid traffic violation test case is well structured.
The test case properly verifies that a single invalid indictment count (empty offenses array) causes the validation to fail.
417-453: LGTM! Mixed case test scenario is comprehensive.
The test case properly verifies that traffic violation validation works correctly when combined with other subtypes.
apps/judicial-system/web/src/utils/validate.ts (2)
302-309: LGTM! Basic validation checks are clear and concise.
The hasValidDemands check properly validates both regular demands and civil demands when applicable.
328-333: LGTM! Pure traffic violation case handling is clear.
The code properly handles the case where all indictments are traffic violations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/web/src/utils/validate.ts (1)
311-318: Consider adding JSDoc comments for validation rulesThe validation logic for traffic violations is clear, but documenting the business rules would improve maintainability.
+/** + * Validates a traffic violation indictment count. + * Required fields: + * - policeCaseNumber + * - offenses (non-empty array) + * - vehicleRegistrationNumber + * - lawsBroken + * - incidentDescription + * - legalArguments + */ const isValidTrafficViolation = (indictmentCount: IndictmentCount) =>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/utils/validate.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/utils/validate.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/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
🔇 Additional comments (5)
apps/judicial-system/web/src/utils/validate.ts (5)
302-306: LGTM: Demands validation logic is clear and concise
The validation correctly checks for both general demands and civil demands when civil claims are present.
307-310: LGTM: Early validation checks prevent unnecessary processing
Good practice to fail fast by checking required conditions early.
319-322: LGTM: Non-traffic violation validation is appropriately less strict
The validation correctly requires only the essential fields for non-traffic violations.
323-327: LGTM: Clear type checking logic
The function effectively determines if an indictment count is a traffic violation.
328-341: Verify test coverage for mixed indictment scenarios
The logic for handling both traffic and non-traffic violations in the same case looks correct. However, ensure comprehensive test coverage for:
- Cases with only traffic violations
- Cases with mixed violation types
- Edge cases with empty indictment counts
✅ Verification successful
Test coverage for mixed indictment scenarios is adequate
The test suite includes comprehensive coverage for all required scenarios:
- Pure traffic violation cases (both valid and invalid)
- Mixed violation types ("should return true if one indictment count has selected traffic violation and another has not")
- Edge cases with empty or null indictment counts ("should return false if there are no demands in a case")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for mixed indictment scenarios
# Look for test cases covering mixed scenarios
rg -A 5 "describe.*isTrafficViolationStepValidIndictments" apps/judicial-system/web/src/utils/validate.spec.ts
# Check for specific test cases
rg "it.*mixed.*indictment" apps/judicial-system/web/src/utils/validate.spec.ts
Length of output: 412
Script:
#!/bin/bash
# Let's check the entire test file content for traffic violation validation tests
rg -B 2 -A 10 "isTrafficViolationStepValidIndictments|isTrafficViolation|isValidTrafficViolation|isValidNonTrafficViolation" apps/judicial-system/web/src/utils/validate.spec.ts
# Also check for any test cases with multiple indictment counts
rg -B 2 -A 5 "indictmentCounts.*\[" apps/judicial-system/web/src/utils/validate.spec.ts
Length of output: 3872
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
292-358: Consider extracting description generation logic into separate functions.The function has grown complex with multiple nested conditions and different description formats. Consider breaking it down into smaller, focused functions for better maintainability:
generateTrafficViolationDescriptiongenerateSingleSubtypeDescriptiongenerateMultiSubtypeDescriptionexport const getIncidentDescription = ( indictmentCount: TIndictmentCount, formatMessage: IntlShape['formatMessage'], crimeScene?: CrimeScene, subtypesRecord?: Record<string, IndictmentSubtype[]>, ) => { const { offenses, substances, vehicleRegistrationNumber, indictmentCountSubtypes, policeCaseNumber, } = indictmentCount const incidentLocation = crimeScene?.place || '[Vettvangur]' const incidentDate = crimeScene?.date ? formatDate(crimeScene.date, 'PPPP')?.replace('dagur,', 'daginn') || '' : '[Dagsetning]' const vehicleRegistration = vehicleRegistrationNumber || '[Skráningarnúmer ökutækis]' const subtypes = (subtypesRecord && policeCaseNumber && subtypesRecord[policeCaseNumber]) || [] if ( subtypes.length > 1 && (!indictmentCountSubtypes?.length || indictmentCountSubtypes.length === 0) ) { return '' } + const generateTrafficViolationDescription = () => { + if (!offenses || offenses.length === 0) { + return '' + } + + const reason = getIncidentDescriptionReason( + offenses, + substances || {}, + formatMessage, + ) + + return formatMessage(strings.incidentDescriptionAutofill, { + incidentDate, + vehicleRegistrationNumber: vehicleRegistration, + reason, + incidentLocation, + }) + } + + const generateSingleSubtypeDescription = () => { + return formatMessage(strings.indictmentDescriptionSubtypesAutofill, { + subtypes: indictmentSubtypes[subtypes[0]], + date: incidentDate, + }) + } + + const generateMultiSubtypeDescription = () => { + const allSubtypes = indictmentCountSubtypes + ?.map((subtype) => indictmentSubtypes[subtype]) + .join(', ') + + return formatMessage(strings.indictmentDescriptionSubtypesAutofill, { + subtypes: allSubtypes, + date: incidentDate, + }) + } if ( isTrafficViolationIndictmentCount(policeCaseNumber, subtypesRecord) || (indictmentCountSubtypes?.length === 1 && indictmentCountSubtypes[0] === IndictmentSubtype.TRAFFIC_VIOLATION) ) { - if (!offenses || offenses.length === 0) { - return '' - } - - const reason = getIncidentDescriptionReason( - offenses, - substances || {}, - formatMessage, - ) - - return formatMessage(strings.incidentDescriptionAutofill, { - incidentDate, - vehicleRegistrationNumber: vehicleRegistration, - reason, - incidentLocation, - }) + return generateTrafficViolationDescription() } if (subtypes.length === 1) { - return formatMessage(strings.indictmentDescriptionSubtypesAutofill, { - subtypes: indictmentSubtypes[subtypes[0]], - date: incidentDate, - }) + return generateSingleSubtypeDescription() } - const allSubtypes = indictmentCountSubtypes - ?.map((subtype) => indictmentSubtypes[subtype]) - .join(', ') - - return formatMessage(strings.indictmentDescriptionSubtypesAutofill, { - subtypes: allSubtypes, - date: incidentDate, - }) + return generateMultiSubtypeDescription() }
470-495: Consider simplifying the visibility check logic.The
shouldShowTrafficViolationFieldsfunction could be simplified by combining the conditions.const shouldShowTrafficViolationFields = () => { - if (isTrafficViolationCase(workingCase)) { - return true - } - - const policeCaseNumber = indictmentCount.policeCaseNumber - - if ( - isTrafficViolationIndictmentCount( - policeCaseNumber, - workingCase.indictmentSubtypes, - ) - ) { - return true - } - - if ( - indictmentCount?.indictmentCountSubtypes?.includes( - IndictmentSubtype.TRAFFIC_VIOLATION, - ) - ) { - return true - } - - return false + return ( + isTrafficViolationCase(workingCase) || + isTrafficViolationIndictmentCount( + indictmentCount.policeCaseNumber, + workingCase.indictmentSubtypes, + ) || + indictmentCount?.indictmentCountSubtypes?.includes( + IndictmentSubtype.TRAFFIC_VIOLATION, + ) + ) }apps/judicial-system/web/src/utils/validate.ts (1)
304-320: Add JSDoc documentation for helper functions.The helper functions are well-structured but would benefit from JSDoc documentation explaining their purpose and validation rules.
+/** + * Validates if an indictment count meets traffic violation requirements + * @param indictmentCount The indictment count to validate + * @returns true if all required fields are present + */ const isValidTrafficViolation = (indictmentCount: IndictmentCount) => +/** + * Validates if an indictment count meets non-traffic violation requirements + * @param indictmentCount The indictment count to validate + * @returns true if all required fields are present + */ const isValidNonTrafficViolation = (indictmentCount: IndictmentCount) => +/** + * Checks if an indictment count is classified as a traffic violation + * @param indictmentCount The indictment count to check + * @returns true if the count includes TRAFFIC_VIOLATION subtype + */ const isTrafficViolation = (indictmentCount: IndictmentCount) =>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/judicial-system/web/src/routes/Court/components/ReceptionAndAssignment/ReceptionAndAssignment.tsx(1 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.spec.tsx(2 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(9 hunks)apps/judicial-system/web/src/utils/validate.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.spec.tsx
- apps/judicial-system/web/src/routes/Court/components/ReceptionAndAssignment/ReceptionAndAssignment.tsx
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/web/src/utils/validate.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/Prosecutor/Indictments/Indictment/IndictmentCount.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."
📓 Learnings (2)
apps/judicial-system/web/src/utils/validate.ts (1)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (5)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:700-758
Timestamp: 2024-12-17T14:33:50.960Z
Learning: In the file `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx`, the user prefers not to add range validation for the blood alcohol content input.
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.
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: unakb
PR: island-is/island.is#15378
File: apps/judicial-system/web/src/routes/Court/Indictments/Summary/Summary.tsx:86-100
Timestamp: 2024-11-12T15:15:11.835Z
Learning: User unakb prefers explicit case handling in switch statements for key functionalities like `getRulingDecisionTagColor` to ensure clarity and avoid assumptions that a case was overlooked.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: tests (judicial-system-backend)
- GitHub Check: build (web)
🔇 Additional comments (8)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (5)
22-22: LGTM: New utility imports for traffic violation handling.The addition of
isTrafficViolationCaseandisTrafficViolationIndictmentCountutilities aligns with the PR's objective of supporting multiple subtypes in indictments.Also applies to: 38-38
383-386: LGTM: Clean subtype state initialization.The subtype state initialization is well-handled with proper null checks and default values.
462-466: LGTM: Proper cleanup of related fields.Good practice: When traffic violation subtype is removed, all related fields (offenses, substances, vehicle registration) are properly cleaned up.
Line range hint
511-543: LGTM: Well-structured police case number selection.The police case number selection UI is well-implemented with:
- Clear section heading
- Proper label and placeholder
- Required field validation
- Proper value handling
544-587: LGTM: Clean implementation of subtype selection.The subtype selection UI is well-structured with:
- Conditional rendering based on multiple subtypes
- Clear section heading
- Proper checkbox implementation
- Good TypeScript usage
apps/judicial-system/web/src/utils/validate.ts (3)
Line range hint
3-22: LGTM! Clean import organization.The new imports and types are well-organized and align with the feature requirements for handling multiple subtypes in indictments.
295-299: LGTM! Clear validation logic for demands.The validation logic for demands and civil claims is clear and concise.
321-334: Verify mixed case validation logic.The implementation for mixed cases looks correct, but let's verify the behavior when:
- Some counts are traffic violations and others are not
- Empty indictment counts array
oddsson
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (3)
292-292: Add type safety forsubtypesRecordparameter.Consider adding a more specific type for
subtypesRecordinstead of usingRecord<string, IndictmentSubtype[]>.-subtypesRecord?: Record<string, IndictmentSubtype[]> +type IndictmentSubtypesRecord = Record<string, IndictmentSubtype[]> +subtypesRecord?: IndictmentSubtypesRecord
314-357: Consider breaking down the complex conditional logic.The function has multiple nested conditions and different formatting paths. Consider extracting these into separate helper functions for better maintainability:
formatTrafficViolationDescriptionformatSingleSubtypeDescriptionformatMultipleSubtypesDescription
Line range hint
382-386: Consider memoizing the subtypes calculation.The subtypes calculation could be memoized using
useMemoto prevent unnecessary recalculations on re-renders.- const subtypes: IndictmentSubtype[] = indictmentCount.policeCaseNumber - ? workingCase.indictmentSubtypes[indictmentCount.policeCaseNumber] - : [] + const subtypes = useMemo(() => + indictmentCount.policeCaseNumber + ? workingCase.indictmentSubtypes[indictmentCount.policeCaseNumber] + : [], + [indictmentCount.policeCaseNumber, workingCase.indictmentSubtypes] + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (5)
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:579-587
Timestamp: 2024-12-17T20:28:47.549Z
Learning: In the `IndictmentCount.tsx` file of the `apps/judicial-system/web` project, constraints on the vehicle registration number are planned to be removed. Avoid suggesting changes related to vehicle registration number validation for now.
Learnt from: oddsson
PR: island-is/island.is#17192
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx:700-758
Timestamp: 2024-12-17T14:33:50.960Z
Learning: In the file `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx`, the user prefers not to add range validation for the blood alcohol content input.
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.
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: unakb
PR: island-is/island.is#15378
File: apps/judicial-system/web/src/routes/Court/Indictments/Summary/Summary.tsx:86-100
Timestamp: 2024-11-12T15:15:11.835Z
Learning: User unakb prefers explicit case handling in switch statements for key functionalities like `getRulingDecisionTagColor` to ensure clarity and avoid assumptions that a case was overlooked.
🔇 Additional comments (2)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Indictment/IndictmentCount.tsx (2)
22-22: LGTM! New utility imports for traffic violation checks.The addition of
isTrafficViolationCaseandisTrafficViolationIndictmentCountutilities aligns with the PR's objective to support multiple subtypes in indictments.Also applies to: 38-38
469-494: LGTM! Well-structured utility function.The
shouldShowTrafficViolationFieldsfunction is well-organized and follows a clear logical flow for determining when to show traffic violation fields.
…into j-s/multi-subtype-2
Allow multiple subtypes in indictments
Asana
What
The changes in this PR enable users to create indictments with multiple subtypes. The main complexity with this change is
requestDriversLicenseSuspensionand demands should be correctly autofilled based on whether or not there are traffic violation indictment counts.There are also other, smaller changes implemented
stepHelperfile inutilstoutils.stepHelperis a bit outdated andutilsis a bit clearer IMO. This change is responsible for most of the files changed in this PR.Why
This is crucial for us to be able to offer combo indictments.
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
Refactoring
stepHelpertoutilsmodule across multiple components.hasSentNotification,isBusiness, andshouldUseAppealWithdrawnRoutes.Localization
Indictment Handling
Testing
Minor Improvements