fix(j-s): Civil claimants national id #16780
Conversation
WalkthroughThe pull request introduces changes to three main files: 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: 1
🧹 Outside diff range and nitpick comments (2)
libs/island-ui/core/src/lib/Toast/Toast.tsx (1)
36-45: Consider making the icon size configurable.The icon size is hardcoded to "large". For better reusability across different contexts, consider making it a prop with "large" as the default.
-const RenderMessage = ({ - message, - type, -}: { - message: string - type: 'error' | 'success' | 'warning' | 'info' -}) => { +interface RenderMessageProps { + message: string + type: 'error' | 'success' | 'warning' | 'info' + iconSize?: 'small' | 'medium' | 'large' +} + +const RenderMessage = ({ + message, + type, + iconSize = 'large', +}: RenderMessageProps) => { // ... - <Icon icon={icons[type]} color={colors[type]} size="large" /> + <Icon icon={icons[type]} color={colors[type]} size={iconSize} />apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1)
267-267: Consider adding a loading state for national registry lookups.The
nationalIdNotFoundstate is set immediately when no items are found, but there's no indication to the user that a lookup is in progress. This could lead to a confusing user experience when there's network latency.Consider adding a loading state:
+ const [isLookingUpNationalId, setIsLookingUpNationalId] = useState<boolean>(false) useEffect(() => { if (!civilClaimantNationalIdUpdate) { return } + setIsLookingUpNationalId(true) const items = personData?.items || [] const person = items[0] setNationalIdNotFound(items.length === 0) + setIsLookingUpNationalId(false) // ... rest of the code }, [personData])Then update the UI to show a loading indicator:
{civilClaimant.nationalId?.length === 11 && nationalIdNotFound && ( <Text color="red600" variant="eyebrow" marginTop={1}> {formatMessage(core.nationalIdNotFoundInNationalRegistry)} </Text> )} + {civilClaimant.nationalId?.length === 11 && isLookingUpNationalId && ( + <Text variant="eyebrow" marginTop={1}> + {formatMessage(core.lookingUpNationalRegistry)} + </Text> + )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx(1 hunks)libs/island-ui/core/src/lib/Toast/Toast.css.ts(3 hunks)libs/island-ui/core/src/lib/Toast/Toast.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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."
libs/island-ui/core/src/lib/Toast/Toast.css.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."
libs/island-ui/core/src/lib/Toast/Toast.tsx (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."
📓 Learnings (1)
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (2)
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-07-03T15:43:13.884Z
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.
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-10-08T15:39:04.351Z
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 (5)
libs/island-ui/core/src/lib/Toast/Toast.tsx (1)
54-54: LGTM! Type declaration simplified correctly.
The removal of React.PropsWithChildren is appropriate since the component doesn't utilize children props. This change maintains type safety while improving code clarity.
libs/island-ui/core/src/lib/Toast/Toast.css.ts (3)
17-17: LGTM: Toast container width adjustment
The width increase to 432px provides more space for content while maintaining mobile responsiveness.
163-163: LGTM: Improved close button alignment
The close button's vertical centering provides better visual balance within the toast notification.
100-100: Verify design specs alignment
The padding and height adjustments follow responsive design principles and use theme tokens correctly. However, please verify that these specific values align with the updated design specifications:
- Desktop: padding increased to theme.spacing[2]
- Mobile: padding theme.spacing[1] and reduced height of 48px
Also applies to: 111-112
✅ Verification successful
Let me gather more information about the theme spacing values to better understand the consistency of these changes.
Based on the extensive search results, I can now provide a definitive response about the Toast padding changes:
Toast padding adjustments follow design system spacing tokens correctly
The changes to the Toast component's padding (from theme.spacing[1] to theme.spacing[2] for desktop and back to theme.spacing[1] for mobile with 48px height) are consistent with how spacing tokens are used throughout the codebase. The codebase shows a clear pattern of using these specific theme spacing values for component padding across different breakpoints.
The evidence shows:
- Consistent usage of
theme.spacing[1]andtheme.spacing[2]for padding across many components - Similar responsive padding patterns in other components like Buttons, Cards, and Modals
- These specific spacing values being used as standard increments in the design system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other toast-related styling files to ensure consistent spacing
fd -e css -e scss -e ts -e tsx --full-path '.*[tT]oast.*' | grep -v 'Toast.css.ts' | xargs rg 'spacing|height|padding'
Length of output: 274
Script:
#!/bin/bash
# Search for theme spacing definitions and usage
ast-grep --pattern 'spacing[$_]'
# Also check for any toast-related style constants or theme configurations
rg -g '!*.{test,spec}.*' -A 3 'theme.*spacing|spacing.*theme'
Length of output: 324452
apps/judicial-system/web/src/routes/Prosecutor/Indictments/Processing/Processing.tsx (1)
Line range hint 260-277: Verify the handling of undefined person data.
The current implementation has a potential issue where person?.name could be undefined when no person is found, but we still update the civil claimant with a potentially undefined name while preserving their national ID.
Consider explicitly handling the undefined case:
const update = {
caseId: workingCase.id,
civilClaimantId: civilClaimantNationalIdUpdate?.civilClaimantId || '',
- name: person?.name,
+ name: person?.name || workingCase.civilClaimants?.find(
+ (c) => c.id === civilClaimantNationalIdUpdate?.civilClaimantId
+ )?.name,
nationalId: civilClaimantNationalIdUpdate?.nationalId,
}Let's verify the current behavior when person data is not found:
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16780 +/- ##
==========================================
- Coverage 36.45% 36.37% -0.08%
==========================================
Files 6903 6898 -5
Lines 144565 145188 +623
Branches 41278 41572 +294
==========================================
+ Hits 52704 52817 +113
- Misses 91861 92371 +510 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 57 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 36 Total Test Services: 0 Failed, 35 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
thorkellmani
added
automerge
|
Cycled |
…claimant-national-id-fix
* Save civil claimants national id even though its not found in the national registry * Remove debug code * Remove debug code * Remove debug code * Fix validation * Add React back --------- Co-authored-by: Guðjón Guðjónsson <gudjon@kolibri.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Save civil claimants national id even though its not found in the national registry
Asana
What
We are not saving a civil claimant's national id to database when the national id is not found in the national registry. This PR fixes that. It also refactors the
Toastcomponent in Island UI. The refactoring includesIconcomponent instead of the old deprecated one.Why
If the national registry is down, we still want to keep the national id set by the prosecutor.
Screenshots / Gifs
Screen.Recording.2024-11-08.at.12.09.54.mov
Checklist:
Summary by CodeRabbit
New Features
Style
Refactor