chore(j-s): Set current user as prosecutor in new indictments #18005
Conversation
WalkthroughThis pull request introduces an optional Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`apps/**/*`: "Confirm that the code adheres to the following...
🔇 Additional comments (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
|
oddsson
changed the title
|
View your CI Pipeline Execution ↗ for commit 6aebb66.
☁️ Nx Cloud last updated this comment at |
…utor-section-refactor
![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 (3)
apps/judicial-system/web/src/routes/Prosecutor/components/ProsecutorSection/ProsecutorSection.tsx (1)
16-29: Consider memoizing the setProsecutor function.The
setProsecutorfunction is recreated on every render. Consider usinguseCallbackto memoize it:-const setProsecutor = async (prosecutorId: string) => { +const setProsecutor = useCallback(async (prosecutorId: string) => { if (workingCase) { const updatedCase = await updateCase(workingCase.id, { prosecutorId: prosecutorId, }) const prosecutor = updatedCase?.prosecutor setWorkingCase((prevWorkingCase) => ({ ...prevWorkingCase, prosecutor, })) } -} +}, [workingCase, updateCase, setWorkingCase])apps/judicial-system/web/src/components/ProsecutorSelection/ProsecutorSelection.tsx (2)
77-91: Simplify the onChange handler logic.The current implementation can be simplified by extracting the prosecutor update logic:
-onChange={(value) => { - const id = value?.value - - if (id && typeof id === 'string') { - if (!workingCase.id) { - const prosecutor = data?.users?.find((p) => p.id === id) - - setWorkingCase((prevWorkingCase) => ({ - ...prevWorkingCase, - prosecutor, - })) - } else { - onChange(id) - } - } -}} +onChange={(value) => { + const id = value?.value + if (!id || typeof id !== 'string') return + + if (!workingCase.id) { + const prosecutor = data?.users?.find((p) => p.id === id) + setWorkingCase((prev) => ({ ...prev, prosecutor })) + return + } + + onChange(id) +}}
41-64: Optimize eligibleProsecutors memo dependencies.The
eligibleProsecutorsmemo depends on individual properties fromworkingCase. Consider creating a separate memo for the institution ID to reduce unnecessary recalculations:+const institutionId = useMemo( + () => (workingCase.id ? workingCase.prosecutorsOffice?.id : currentUser?.institution?.id), + [workingCase.id, workingCase.prosecutorsOffice?.id, currentUser?.institution?.id] +) const eligibleProsecutors: Option<string>[] = useMemo(() => { if (!data?.users) { return [] } return data.users .filter( (user) => user.role === UserRole.PROSECUTOR && - user.institution?.id === (workingCase.id - ? workingCase.prosecutorsOffice?.id - : currentUser?.institution?.id), + user.institution?.id === institutionId, ) .map(({ id, name }) => ({ label: name ?? '', value: id, })) -}, [currentUser?.institution?.id, data?.users, workingCase.id, workingCase.prosecutorsOffice?.id]) +}, [data?.users, institutionId])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/judicial-system/api/src/app/modules/case/dto/createCase.input.ts(2 hunks)apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts(2 hunks)apps/judicial-system/web/src/components/ProsecutorSelection/ProsecutorSelection.tsx(2 hunks)apps/judicial-system/web/src/routes/Court/Indictments/Summary/Summary.tsx(0 hunks)apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx(2 hunks)apps/judicial-system/web/src/routes/Prosecutor/components/ProsecutorSection/ProsecutorSection.tsx(2 hunks)apps/judicial-system/web/src/utils/hooks/useCase/index.ts(1 hunks)apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.ts(0 hunks)
💤 Files with no reviewable changes (2)
- apps/judicial-system/web/src/routes/Court/Indictments/Summary/Summary.tsx
- apps/system-e2e/src/tests/judicial-system/regression/indictment-tests.spec.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/web/src/utils/hooks/useCase/index.tsapps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.tsapps/judicial-system/web/src/routes/Prosecutor/components/ProsecutorSection/ProsecutorSection.tsxapps/judicial-system/api/src/app/modules/case/dto/createCase.input.tsapps/judicial-system/web/src/components/ProsecutorSelection/ProsecutorSelection.tsxapps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsxapps/judicial-system/backend/src/app/modules/case/case.service.ts
🧠 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#17921
File: apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx:457-459
Timestamp: 2025-02-12T08:46:49.953Z
Learning: In the judicial system web app, if a case has been saved (has workingCase.id), it is guaranteed to have a prosecutor set and they cannot be removed. Therefore, prosecutor selection validation is only needed for new cases.
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (8)
apps/judicial-system/api/src/app/modules/case/dto/createCase.input.ts (1)
79-82: LGTM! Well-structured field addition.The new
prosecutorIdfield is properly decorated with GraphQL and validation decorators, following NestJS best practices.apps/judicial-system/backend/src/app/modules/case/dto/createCase.dto.ts (1)
95-98: LGTM! Proper validation setup.The new
prosecutorIdfield is correctly decorated with validation and Swagger decorators, ensuring proper UUID validation and API documentation.apps/judicial-system/web/src/routes/Prosecutor/components/ProsecutorSection/ProsecutorSection.tsx (1)
12-12: LGTM! Clean component simplification.The removal of unnecessary props and interface simplifies the component's API, making it more maintainable.
apps/judicial-system/web/src/components/ProsecutorSelection/ProsecutorSelection.tsx (1)
24-34: LGTM! Smart fallback to current user.The
selectedProsecutorlogic intelligently falls back to the current user when no prosecutor is selected, aligning with the PR's objective.apps/judicial-system/web/src/utils/hooks/useCase/index.ts (1)
195-195: LGTM!The addition of
prosecutorIdto the case creation input is correct and aligns with the PR objectives.apps/judicial-system/web/src/routes/Prosecutor/Indictments/Defendant/Defendant.tsx (2)
461-461: LGTM!The simplification of step validation to only check defendant information is correct, as prosecutor selection is now handled automatically.
476-478: LGTM!The simplified rendering of
ProsecutorSectionwithout props is correct, as the prosecutor selection is now controlled internally.apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
742-746: LGTM!The conditional assignment of
prosecutorIdis well-implemented:
- For indictment cases: Uses the provided
prosecutorId- For other cases: Uses the user's id if they are a prosecutor
- Otherwise: Remains undefined
This change correctly implements the automatic prosecutor selection for new indictments while maintaining the existing behavior for other case types.
apps/judicial-system/web/src/components/ProsecutorSelection/ProsecutorSelection.tsx
Show resolved
Hide resolved
…utor-section-refactor
…utor-section-refactor
…s/island.is into j-s/prosecutor-section-refactor
…utor-section-refactor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
759-763: LGTM! The prosecutor assignment logic is correct.The conditional assignment of
prosecutorIdcorrectly implements the PR objective:
- For indictment cases, it uses the provided
prosecutorId- For non-indictment cases, it maintains the existing behavior
Consider improving readability by extracting the condition into a separate variable:
- prosecutorId: isIndictmentCase(caseToCreate.type) - ? caseToCreate.prosecutorId - : user.role === UserRole.PROSECUTOR - ? user.id - : undefined, + prosecutorId: isIndictmentCase(caseToCreate.type) + ? caseToCreate.prosecutorId + : isProsecutor(user) + ? user.id + : undefined,And add a helper function at the top of the file:
const isProsecutor = (user: TUser): boolean => user.role === UserRole.PROSECUTOR;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/test/caseController/create.spec.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/test/caseController/create.spec.tsapps/judicial-system/backend/src/app/modules/case/case.service.ts
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/case/test/caseController/create.spec.ts (1)
71-71: LGTM! Test coverage looks good.The test correctly verifies that the
prosecutorIdis passed to the case creation, aligning with the PR objective of setting the current user as prosecutor in new indictments.
…s/island.is into j-s/prosecutor-section-refactor
Set current user as prosecutor in new indictments
Asana
What
In #17921 we moved the prosecutor selection to the defendant screen. There were a few issues with that PR that this PR fixes. The main things are
This makes the changes done to the e2e tests, where a prosecutor was selected manually, redundant.
A note on code quality
There is a change in this PR that is not ideal in terms of code quality.
My issue with this is that the
ProsecutorSelectioncomponent is now being controlled in two places, in the component itself and via theonChangeprop. The reason for this is that the prosectors are fetched inside the component and if an indictment is being created, we need to select from that list, but if the case exists, then we don't use that list. I have a few ideas on how to potentially fix this, but all of them are somewhat complex in implementation and I feel like it's not worth the effort. If you strongly disagree, then let's talk about it 😉Why
Better UX
Screenshots / Gifs
Screen.Recording.2025-02-18.at.10.54.31.mov
Checklist:
Summary by CodeRabbit
New Features
createCasemutation response now includes atypefield, providing additional information about the created case.Refactor
Bug Fixes