fix(j-s): Double service not required #16934
Conversation
…e-announcement-defenders
…e-announcement-defenders
…e-announcement-defenders
…d-is/island.is into j-s/service-announcement-defenders
…e-announcement-defenders
…e-announcement-defenders
WalkthroughThe pull request introduces several modifications across multiple components in the judicial system application. Key changes include the removal of the 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16934 +/- ##
=======================================
Coverage 36.46% 36.46%
=======================================
Files 6903 6903
Lines 144583 144604 +21
Branches 41283 41289 +6
=======================================
+ Hits 52718 52726 +8
- Misses 91865 91878 +13 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ❌ 4 Failed (0 Known Flaky), 329 Passed, 0 Skipped, 56.05s Total Time ❌ Failed Tests (4)
🔻 Code Coverage Decreases vs Default Branch (1)
|
![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 and nitpick comments (1)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx (1)
135-141: Consider simplifying the conditional logic.While the current implementation correctly fixes the duplicate message issue, we can improve readability by combining the conditions.
Consider this refactor:
- {displayVerdictViewDate && - defendant.serviceRequirement && - defendant.serviceRequirement !== ServiceRequirement.NOT_REQUIRED && ( + {displayVerdictViewDate && defendant.serviceRequirement === ServiceRequirement.REQUIRED && ( <Text marginTop={1} fontWeight="semiBold"> {getVerdictViewDateText(formatMessage, defendant.verdictViewDate)} </Text> )}This change:
- Simplifies the conditions by using a direct equality check
- Makes the intent clearer - we only show the verdict view date when service is explicitly required
- Maintains the same functionality while being more concise
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.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/components/InfoCard/DefendantInfo/DefendantInfo.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#15461
File: apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx:36-62
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The user prefers readability over optimization in the `getAppealExpirationInfo` function within `DefendantInfo.tsx`.
Learnt from: unakb
PR: island-is/island.is#14922
File: apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx:43-49
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The `getAppealExpirationInfo` function in `DefendantInfo.tsx` correctly uses `today < expiryDate` to determine if the current date is before the appeal expiration date.
🔇 Additional comments (1)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx (1)
129-132: LGTM! Clean and readable implementation.
The simplified component structure improves readability while maintaining the same functionality.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/web/src/routes/Shared/Cases/ActiveCases.tsx (1)
60-73: LGTM! Consider enhancing type safety.The simplified arrow function with implicit return improves readability while maintaining the same functionality. The conditional menu items are handled correctly using the spread operator.
Consider adding explicit return type for better type safety:
- generateContextMenuItems={(row) => [ + generateContextMenuItems={(row): ContextMenuItem[] => [apps/judicial-system/web/src/components/Table/Table.tsx (2)
101-105: LGTM: Well-structured click handler with good separation of concernsConsider adding TypeScript return type annotation for better type safety:
- const handleCaseClick = (theCase: CaseListEntry) => { + const handleCaseClick = (theCase: CaseListEntry): void => {
125-146: Consider extracting date formatting logicThe court date formatting logic could be moved to a separate utility function to improve reusability and testing.
+ const formatCourtDateTime = (date: string): string => { + const parsedDate = parseISO(date) + return `${formatMessage(strings.hearing)} ${formatDate(parsedDate, 'd.M.y')} kl. ${formatDate(parsedDate, 'kk:mm')}` + } const renderPostponedOrCourtDateText = ( postponedIndefinitelyExplanation?: string | null, caseState?: CaseState | null, courtDate?: string | null, ) => { if (postponedIndefinitelyExplanation) { return <Text>{formatMessage(strings.postponed)}</Text> } if (!isCompletedCase(caseState) && courtDate) { return ( <Text fontWeight="medium" variant="small"> - {`${formatMessage(strings.hearing)} ${formatDate( - parseISO(courtDate), - 'd.M.y', - )} kl. ${formatDate(parseISO(courtDate), 'kk:mm')}`} + {formatCourtDateTime(courtDate)} </Text> ) } return null }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
apps/judicial-system/web/src/components/Table/PastCasesTable/MobilePastCase.tsx(0 hunks)apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.tsx(3 hunks)apps/judicial-system/web/src/components/Table/Table.css.ts(0 hunks)apps/judicial-system/web/src/components/Table/Table.tsx(5 hunks)apps/judicial-system/web/src/routes/Shared/Cases/ActiveCases.tsx(1 hunks)apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- apps/judicial-system/web/src/components/Table/PastCasesTable/MobilePastCase.tsx
- apps/judicial-system/web/src/components/Table/Table.css.ts
✅ Files skipped from review due to trivial changes (1)
- apps/judicial-system/web/src/routes/Shared/Cases/Cases.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/web/src/components/Table/PastCasesTable/PastCasesTable.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/Table/Table.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/Shared/Cases/ActiveCases.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/components/Table/PastCasesTable/PastCasesTable.tsx (1)
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.
🔇 Additional comments (5)
apps/judicial-system/web/src/components/Table/Table.tsx (5)
10-15: LGTM: Well-structured type imports
The new imports enhance type safety and provide clear domain-specific utilities for case type checking.
107-123: LGTM: Clear prosecutor text rendering logic
The function has good null handling and clear conditions.
148-169: LGTM: Well-structured duration date rendering
Good use of type guards and clear conditional logic.
204-220: LGTM: Consistent mobile implementation
Good use of the new rendering functions with proper prop passing.
260-260: LGTM: Consistent click handling in desktop view
The implementation maintains consistency with the mobile view.
| <ColumnCaseType | ||
| type={row.type} | ||
| decision={row.decision} | ||
| parentCaseId={row.parentCaseId} |
There was a problem hiding this comment.
Remove unnecessary props from ColumnCaseType component
According to previous implementations and retrieved learnings, the ColumnCaseType component should only receive the type prop. The decision and parentCaseId props are not required and can be removed to simplify the component interface.
Apply this diff to remove the unnecessary props:
-<ColumnCaseType
- type={row.type}
- decision={row.decision}
- parentCaseId={row.parentCaseId}
-/>
+<ColumnCaseType
+ type={row.type}
+/>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ColumnCaseType | |
| type={row.type} | |
| decision={row.decision} | |
| parentCaseId={row.parentCaseId} | |
| <ColumnCaseType | |
| type={row.type} |
Double service not required
Asana
What
When a service is not required, we'd show "Birting dóms ekki þörf" twice in some info cards. This has now been fixed.
Why
It's a bug
Screenshots / Gifs
Before
After
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes