chore(j-s): Rewrites Input Advocate #17471
Conversation
|
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 significant changes to the judicial system web application, focusing on refactoring and improving the handling of advocate information, particularly for defenders. The modifications span multiple components and utility files, streamlining the logic for updating case and advocate details, removing unused utility functions, and enhancing the internationalization approach for input fields related to legal professionals. Changes
Possibly related PRs
Suggested labels
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
|
![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
🧹 Nitpick comments (10)
apps/judicial-system/web/src/components/Inputs/InputAdvocate.strings.ts (1)
4-23: Consider optimizing duplicate message IDs.The same ID 'judicial.system.core:lawyer_input.defender' is used across different label types. Consider using unique IDs like 'lawyer_input.defender.name', 'lawyer_input.defender.email', etc., to prevent potential conflicts and improve maintainability.
export const nameLabelStrings = defineMessages({ defender: { - id: 'judicial.system.core:lawyer_input.defender', + id: 'judicial.system.core:lawyer_input.defender.name', defaultMessage: 'Nafn verjanda', description: 'Notaður sem titill á nafni fyrir verjanda.', }, // ... similar changes for other message IDs })Also applies to: 27-46, 50-69
apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx (2)
42-45: Enhance component documentation.While the current documentation provides a basic overview, it could be improved by documenting:
- The purpose of each prop
- Validation rules
- Error handling behavior
Line range hint
95-179: Consider extracting validation logic.The email and phone number validation logic is duplicated across handlers. Consider extracting it into a custom hook or utility function.
// useInputValidation.ts export const useInputValidation = (type: 'email' | 'phone') => { const [errorMessage, setErrorMessage] = useState<string>('') const handleChange = useCallback((value: string, onChange: (value: string | null) => void) => { const sanitized = replaceTabs(value) removeErrorMessageIfValid( [type === 'email' ? 'email-format' : 'phonenumber'], sanitized, errorMessage, setErrorMessage ) onChange(sanitized || null) }, [errorMessage]) const handleBlur = useCallback((value: string, onSave: (value: string | null) => void) => { const sanitized = replaceTabs(value) validateAndSetErrorMessage( [type === 'email' ? 'email-format' : 'phonenumber'], sanitized, setErrorMessage ) onSave(sanitized || null) }, []) return { errorMessage, handleChange, handleBlur } }apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (2)
37-59: Consider simplifying title logic.The nested if-else structure could be simplified using a lookup object.
const titleMessages = { restriction: { prosecution: defenderInfo.restrictionCases.sections.defender.heading, default: defenderInfo.restrictionCases.sections.defender.title, }, investigation: { prosecution: defenderInfo.investigationCases.sections.defender.heading, default: defenderInfo.investigationCases.sections.defender.title, }, } const getSectionTitle = () => { const caseType = isRestrictionCase(workingCase.type) ? 'restriction' : 'investigation' const userType = isProsecutionUser(user) ? 'prosecution' : 'default' const message = titleMessages[caseType][userType] return formatMessage(message, { defenderType: workingCase.sessionArrangements === SessionArrangements.ALL_PRESENT_SPOKESPERSON ? 'Talsmaður' : 'Verjandi', }) }
116-135: Consider memoizing callback props.The inline callback props for email and phone number changes could benefit from memoization to prevent unnecessary rerenders.
const handleEmailChange = useCallback( (defenderEmail: string | null) => setWorkingCase((prev) => ({ ...prev, defenderEmail })), [] ) const handleEmailSave = useCallback( (defenderEmail: string | null) => updateCase(workingCase.id, { defenderEmail }), [workingCase.id, updateCase] )apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectDefender.tsx (2)
55-80: Consider simplifying the defender waiver logic.While the implementation is thorough, the conditional assignments could be simplified using object spread and conditional operators.
Consider this more maintainable approach:
- const update: UpdateDefendant = { - defendantId: defendant.id, - defenderNationalId: defendantWaivesRightToCounsel - ? null - : defendant.defenderNationalId || null, - defenderName: defendantWaivesRightToCounsel - ? null - : defendant.defenderName, - defenderEmail: defendantWaivesRightToCounsel - ? null - : defendant.defenderEmail, - defenderPhoneNumber: defendantWaivesRightToCounsel - ? null - : defendant.defenderPhoneNumber, - defenderChoice: - defendantWaivesRightToCounsel === true - ? DefenderChoice.WAIVE - : DefenderChoice.DELAY, - } + const update: UpdateDefendant = { + defendantId: defendant.id, + defenderChoice: defendantWaivesRightToCounsel ? DefenderChoice.WAIVE : DefenderChoice.DELAY, + ...(defendantWaivesRightToCounsel + ? { + defenderNationalId: null, + defenderName: null, + defenderEmail: null, + defenderPhoneNumber: null, + } + : { + defenderNationalId: defendant.defenderNationalId || null, + defenderName: defendant.defenderName, + defenderEmail: defendant.defenderEmail, + defenderPhoneNumber: defendant.defenderPhoneNumber, + }), + }
82-109: Enhance readability of defender choice logic.The current implementation uses multiple boolean checks that could be made more explicit.
Consider extracting the choice determination logic into a separate function:
const determineDefenderChoice = ( defenderName: string | null, currentChoice: DefenderChoice | null ): DefenderChoice => { if (!defenderName && (!currentChoice || currentChoice === DefenderChoice.CHOOSE)) { return DefenderChoice.DELAY } if (defenderName && (!currentChoice || currentChoice === DefenderChoice.DELAY)) { return DefenderChoice.CHOOSE } return currentChoice as DefenderChoice }Then use it in the main function:
- const isDelaying = - !defenderName && - (!defenderChoice || defenderChoice === DefenderChoice.CHOOSE) - const isChoosing = - defenderName && - (!defenderChoice || defenderChoice === DefenderChoice.DELAY) - - const defenderChoiceUpdate = isDelaying - ? DefenderChoice.DELAY - : isChoosing - ? DefenderChoice.CHOOSE - : defenderChoice + const defenderChoiceUpdate = determineDefenderChoice(defenderName, defenderChoice)apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx (1)
44-65: Consider adding error handling.The handler functions are well-structured with clear responsibilities. However, consider adding error handling for server communication failures.
const handleUpdateCivilClaimant = (update: UpdateCivilClaimant) => { + try { updateCivilClaimant({ caseId: workingCase.id, civilClaimantId: civilClaimant.id, ...update, }) + } catch (error) { + // Handle error appropriately + console.error('Failed to update civil claimant:', error) + } }apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (2)
146-210: Consider optimizing removeAllCivilClaimants.While the implementation is correct, consider using Promise.all for parallel deletion of civil claimants:
const removeAllCivilClaimants = () => { if (!workingCase.civilClaimants) { return } - for (const civilClaimant of workingCase.civilClaimants) { - removeCivilClaimantById(civilClaimant.id) - } + return Promise.all( + workingCase.civilClaimants.map((civilClaimant) => + removeCivilClaimantById(civilClaimant.id) + ) + ) }
586-636: Consider extracting common advocate handling logic.The implementation is correct but duplicates patterns seen in SelectCivilClaimantAdvocate.tsx. Consider extracting these patterns into a custom hook for reusability.
Example:
// useAdvocateHandlers.ts export const useAdvocateHandlers = ( civilClaimantId: string, handleUpdate: (update: any) => void ) => { const onEmailChange = useCallback( (email: string | null) => handleUpdate({ civilClaimantId, spokespersonEmail: email }), [civilClaimantId, handleUpdate] ) // ... other handlers return { onEmailChange, ... } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx(7 hunks)apps/judicial-system/web/src/components/Inputs/Input.strings.ts(0 hunks)apps/judicial-system/web/src/components/Inputs/InputAdvocate.strings.ts(1 hunks)apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx(4 hunks)apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx(7 hunks)apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectDefender.tsx(3 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx(18 hunks)apps/judicial-system/web/src/utils/formatters.ts(0 hunks)apps/judicial-system/web/src/utils/utils.spec.tsx(0 hunks)libs/judicial-system/types/src/index.ts(1 hunks)libs/judicial-system/types/src/lib/lawyer.ts(0 hunks)
💤 Files with no reviewable changes (4)
- libs/judicial-system/types/src/lib/lawyer.ts
- apps/judicial-system/web/src/components/Inputs/Input.strings.ts
- apps/judicial-system/web/src/utils/utils.spec.tsx
- apps/judicial-system/web/src/utils/formatters.ts
🧰 Additional context used
📓 Path-based instructions (7)
libs/judicial-system/types/src/index.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/components/Inputs/InputAdvocate.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/web/src/routes/Court/Indictments/Advocates/SelectDefender.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/Prosecutor/Indictments/Processing/Processing.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/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.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/Inputs/InputAdvocate.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/DefenderInfo/DefenderInfo.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/routes/Court/Indictments/Advocates/SelectDefender.tsx (1)
Learnt from: unakb
PR: island-is/island.is#16393
File: apps/judicial-system/web/src/routes/Court/Indictments/Advocates/Advocates.tsx:62-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: When optional chaining is already used on `workingCase.defendants` in the `Advocates.tsx` component, avoid suggesting additional null checks as it's already handled.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1)
Learnt from: gudjong
PR: island-is/island.is#15421
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx:55-61
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The `updateCase` method in the `apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx` file has its own error handling, and additional error handling in the `initialize` function is not necessary.
🔇 Additional comments (16)
libs/judicial-system/types/src/index.ts (1)
123-123: LGTM! Export path updated correctly.The change from './lib/advocate' to './lib/lawyer' aligns with the refactoring of advocate-related types.
apps/judicial-system/web/src/components/Inputs/InputAdvocate.strings.ts (1)
3-24: LGTM! Well-structured i18n messages.The message definitions follow react-intl best practices with clear IDs, default messages, and descriptions.
Also applies to: 26-47, 49-70, 72-88
apps/judicial-system/web/src/components/Inputs/InputAdvocate.tsx (2)
24-38: LGTM! Well-typed props interface.The props interface clearly defines all required properties with appropriate TypeScript types.
Line range hint
183-234: LGTM! Clean render logic with proper i18n.The render logic is well-structured with appropriate use of internationalized labels and error handling.
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (2)
31-34: LGTM! Clean hook usage.The component properly destructures the required hooks and manages state effectively.
92-109: LGTM! Well-structured update handlers.The handlers are well-organized and properly handle the case updates.
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectDefender.tsx (3)
23-23: LGTM! Clean interface definition.The interface follows TypeScript best practices by extending and omitting specific fields.
40-53: Well-structured state management handlers!The functions follow a clear separation of concerns:
handleUpdateDefendantState: Updates local statehandleUpdateDefendant: Updates without state refreshhandleSetAndSendDefendantToServer: Updates with state refreshThis pattern aligns with React best practices for state management.
158-200: Well-structured advocate input handling!The InputAdvocate component implementation:
- Uses strongly-typed callbacks
- Separates concerns between immediate state updates and server synchronization
- Properly handles all advocate data aspects (name, email, phone)
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectCivilClaimantAdvocate.tsx (4)
30-31: LGTM! Clean interface definition.The interface properly extends the input type while omitting ID fields that are handled at a higher level.
36-40: LGTM! Clean hook usage.The hook destructuring is well-organized and follows React best practices.
85-87: LGTM! Clean radio button handlers.The handlers are consistent and properly manage the spokesperson type updates.
Also applies to: 101-103
113-145: LGTM! Well-structured input handling.The InputAdvocate component props are well-organized with clear separation between immediate updates and save operations.
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (3)
57-61: LGTM! Consistent interface definitions.The interfaces follow the same pattern as in SelectCivilClaimantAdvocate.tsx, maintaining consistency across the codebase.
Line range hint
228-279: LGTM! Robust national ID handling.The implementation properly handles various edge cases and integrates well with the person data lookup functionality.
Line range hint
324-359: LGTM! Consistent plea handling.The plea handlers follow a consistent pattern and properly update the defendant's state.
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 (5) |
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx
Outdated
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectDefender.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Court/Indictments/Advocates/SelectDefender.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (1)
169-173: Consider reducing repetition in radio button handlers.The force flag is repeated in multiple handlers. Consider extracting a helper function to reduce duplication.
+const handleRequestSharedWithDefender = (requestSharedWithDefender: RequestSharedWithDefender) => { + handleSetAndSendCaseToServer({ + requestSharedWithDefender, + force: true, + }) +} -onChange={() => { - handleSetAndSendCaseToServer({ - requestSharedWithDefender: RequestSharedWithDefender.READY_FOR_COURT, - force: true, - }) -}} +onChange={() => handleRequestSharedWithDefender(RequestSharedWithDefender.READY_FOR_COURT)}Also applies to: 196-200, 223-227
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.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."
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (4)
apps/judicial-system/web/src/components/DefenderInfo/DefenderInfo.tsx (4)
2-2: LGTM! Clean import and hook updates.The changes follow React and i18n best practices, with proper typing and hook usage.
Also applies to: 17-17, 31-31
39-62: Consider optimizing the conditional formatting logic.The defenderType formatting is only needed for non-restriction cases when the user is not a prosecution user. Consider extracting this logic for better readability.
} else { if (isProsecutionUser(user)) { return formatMessage( defenderInfo.investigationCases.sections.defender.heading, ) } else { + const message = defenderInfo.investigationCases.sections.defender.title + const isSpokesperson = workingCase.sessionArrangements === SessionArrangements.ALL_PRESENT_SPOKESPERSON + return formatMessage(message, { + defenderType: isSpokesperson ? 'Talsmaður' : 'Verjandi', + }) - return formatMessage( - defenderInfo.investigationCases.sections.defender.title, - { - defenderType: - workingCase.sessionArrangements === - SessionArrangements.ALL_PRESENT_SPOKESPERSON - ? 'Talsmaður' - : 'Verjandi', - }, - ) } }
97-114: LGTM! Well-structured handler functions.The handlers effectively centralize the update logic and properly handle the force flag for autofill scenarios.
121-140: LGTM! Well-structured component props.The InputAdvocate component now has:
- Clear separation between local state updates and server persistence
- Type-safe props with meaningful names
- Granular control over individual fields
gudjong
added
the
deprecated:automerge
oddsson
added
deprecated:automerge
Rewrites Input Advocate
Rewrite advocate input
label - lögmaður eða réttargæslumaður kemur ekki rétt út
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
The changes focus on improving the user experience for managing legal professional information in the judicial system application.