feat(j-s): Show district court abbreviation on mobile #17075
Conversation
WalkthroughThe pull request introduces changes across multiple files in the judicial system application. A new 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 (9)
apps/judicial-system/backend/src/app/modules/case/interceptors/caseList.interceptor.ts (2)
67-67: Add type annotation for better type safetyConsider explicitly typing the returned object to ensure type safety. This helps catch potential type mismatches during compilation.
- court: theCase.court, + court: theCase.court as Institution | null,
67-67: Update API documentationThe PR objectives mention that documentation updates are pending. Please ensure the API documentation is updated to reflect this new field in the response.
Would you like me to help generate the documentation for this new field?
apps/judicial-system/api/src/app/modules/case-list/models/caseList.model.ts (1)
80-82: Consider grouping related institution fields together.For better code organization and maintainability, consider moving the
courtfield next to other institution-related fields likeprosecutorsOffice. This would make it easier to maintain related fields in the future.@Field(() => User, { nullable: true }) readonly registrar?: User @Field(() => ID, { nullable: true }) readonly parentCaseId?: string + @Field(() => Institution, { nullable: true }) + readonly court?: Institution + @Field(() => CaseAppealState, { nullable: true }) readonly appealState?: CaseAppealState @Field(() => String, { nullable: true }) readonly appealedDate?: string @Field(() => String, { nullable: true }) readonly appealCaseNumber?: string @Field(() => CaseAppealRulingDecision, { nullable: true }) readonly appealRulingDecision?: CaseAppealRulingDecision @Field(() => Institution, { nullable: true }) readonly prosecutorsOffice?: Institution - @Field(() => Institution, { nullable: true }) - readonly court?: Institutionapps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx (1)
78-92: Consider extracting the template string for better readability.The implementation correctly handles the district court abbreviation display and null cases. However, the template string could be made more readable.
Consider this minor readability improvement:
cell: (row) => { const courtAbbreviation = districtCourtAbbreviation( row.court?.name, ) + const formattedCaseNumber = `${ + courtAbbreviation ? `${courtAbbreviation}: ` : '' + }${row.courtCaseNumber ?? ''}` return ( <CourtCaseNumber - courtCaseNumber={`${ - courtAbbreviation ? `${courtAbbreviation}: ` : '' - }${row.courtCaseNumber ?? ''}`} + courtCaseNumber={formattedCaseNumber} policeCaseNumbers={row.policeCaseNumbers ?? []} appealCaseNumber={row.appealCaseNumber ?? ''} /> ) },apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (1)
116-130: Consider memoizing the court abbreviation.The implementation correctly displays the district court abbreviation with the case number. However, for better performance, consider memoizing the abbreviation computation to prevent unnecessary recalculations during re-renders.
cell: (row) => { + const courtAbbreviation = useMemo( + () => districtCourtAbbreviation(row.court?.name), + [row.court?.name] + ) - const courtAbbreviation = districtCourtAbbreviation( - row.court?.name, - ) return ( <CourtCaseNumber courtCaseNumber={`${ courtAbbreviation ? `${courtAbbreviation}: ` : '' }${row.courtCaseNumber ?? ''}`} policeCaseNumbers={row.policeCaseNumbers ?? []} appealCaseNumber={row.appealCaseNumber ?? ''} /> ) },apps/judicial-system/web/src/components/Table/Table.tsx (2)
174-190: Consider enhancing type safety and error handlingThe
getColumnValuefunction is well-structured but could benefit from additional type safety and error handling.Consider these improvements:
const getColumnValue = ( entry: CaseListEntry, column: keyof CaseListEntry, -) => { +): string => { + if (!entry) { + return ''; + } + const courtAbbreviation = districtCourtAbbreviation(entry.court?.name) switch (column) { case 'defendants': - return entry.defendants?.[0]?.name ?? '' + return entry.defendants && entry.defendants.length > 0 + ? entry.defendants[0].name ?? '' + : '' case 'courtCaseNumber': return courtAbbreviation ? `${courtAbbreviation}: ${entry.courtCaseNumber}` : entry.courtCaseNumber ?? '' default: - return entry[column]?.toString() ?? '' + const value = entry[column] + return value !== null && value !== undefined ? value.toString() : '' } }These changes:
- Add explicit return type
- Add null check for entry parameter
- Improve array bounds checking for defendants
- Enhance null/undefined handling in default case
195-197: Consider memoizing sort callback for better performanceWhile the sorting implementation is correct, consider extracting and memoizing the comparison callback to prevent unnecessary recreations.
Here's a suggested improvement:
const sortCallback = useCallback( (a: CaseListEntry, b: CaseListEntry) => { const compareResult = compareLocaleIS( getColumnValue(a, sortConfig.column), getColumnValue(b, sortConfig.column), ) return sortConfig.direction === 'ascending' ? compareResult : -compareResult }, [sortConfig] ) useMemo(() => { if (sortConfig) { data.sort(sortCallback) } }, [data, sortCallback])libs/judicial-system/formatters/src/lib/formatters.ts (1)
221-242: LGTM! Consider TypeScript improvements for better type safety.The implementation is clean and handles all district courts in Iceland. Consider these TypeScript improvements:
+type DistrictCourt = + | 'Héraðsdómur Reykjavíkur' + | 'Héraðsdómur Reykjaness' + | 'Héraðsdómur Vesturlands' + | 'Héraðsdómur Suðurlands' + | 'Héraðsdómur Norðurlands eystra' + | 'Héraðsdómur Norðurlands vestra' + | 'Héraðsdómur Austurlands' + | 'Héraðsdómur Vestfjarða'; +const DISTRICT_COURTS: { [key in DistrictCourt]: string } = { + 'Héraðsdómur Reykjavíkur': 'HDR', + 'Héraðsdómur Reykjaness': 'HDRN', + 'Héraðsdómur Vesturlands': 'HDV', + 'Héraðsdómur Suðurlands': 'HDS', + 'Héraðsdómur Norðurlands eystra': 'HDNE', + 'Héraðsdómur Norðurlands vestra': 'HDNV', + 'Héraðsdómur Austurlands': 'HDA', + 'Héraðsdómur Vestfjarða': 'HDVF' +}; -export const districtCourtAbbreviation = (courtName?: string | null) => { +export const districtCourtAbbreviation = (courtName?: DistrictCourt | null): string => { - switch (courtName) { - case 'Héraðsdómur Reykjavíkur': - return 'HDR' - case 'Héraðsdómur Reykjaness': - return 'HDRN' - case 'Héraðsdómur Vesturlands': - return 'HDV' - case 'Héraðsdómur Suðurlands': - return 'HDS' - case 'Héraðsdómur Norðurlands eystra': - return 'HDNE' - case 'Héraðsdómur Norðurlands vestra': - return 'HDNV' - case 'Héraðsdómur Austurlands': - return 'HDA' - case 'Héraðsdómur Vestfjarða': - return 'HDVF' - default: - return '' - } + return courtName ? DISTRICT_COURTS[courtName] ?? '' : ''; }This refactor:
- Adds type safety with a union type for court names
- Moves court abbreviations to a constant object
- Simplifies the implementation
- Makes it easier to maintain and update court mappings
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
411-411: LGTM! Consider enhancing type safety.The addition of court institution to the case list include is correct and aligns with the feature requirements.
Consider defining a type-safe constant for the relationship alias to prevent typos and improve maintainability:
const INSTITUTION_RELATIONS = { COURT: 'court', PROSECUTORS_OFFICE: 'prosecutorsOffice', } as const; // Then use it in the include array { model: Institution, as: INSTITUTION_RELATIONS.COURT }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
apps/judicial-system/api/src/app/modules/case-list/models/caseList.model.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/case.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/case/interceptors/caseList.interceptor.ts(1 hunks)apps/judicial-system/web/src/components/Table/Table.tsx(2 hunks)apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx(3 hunks)apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx(3 hunks)apps/judicial-system/web/src/routes/Shared/Cases/cases.graphql(1 hunks)libs/judicial-system/formatters/src/lib/formatters.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
apps/judicial-system/api/src/app/modules/case-list/models/caseList.model.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/backend/src/app/modules/case/case.service.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/backend/src/app/modules/case/interceptors/caseList.interceptor.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/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/PublicProsecutor/Tables/CasesForReview.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/PublicProsecutor/Tables/CasesReviewed.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/cases.graphql (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/judicial-system/formatters/src/lib/formatters.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."
📓 Learnings (2)
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#14673
File: apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx:79-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The implementation of `mapIndictmentCaseStateToTagVariant` in `TagCaseState.tsx`, which maps only `CaseState.ACCEPTED` to a specific tag and defaults others to an "unknown" status, is intentional as per the application's requirements.
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.
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#14673
File: apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx:79-79
Timestamp: 2024-11-12T15:15:11.835Z
Learning: The implementation of `mapIndictmentCaseStateToTagVariant` in `TagCaseState.tsx`, which maps only `CaseState.ACCEPTED` to a specific tag and defaults others to an "unknown" status, is intentional as per the application's requirements.
🔇 Additional comments (11)
apps/judicial-system/web/src/routes/Shared/Cases/cases.graphql (2)
37-40: Verify court fields for abbreviation feature.
Please confirm that id and name are sufficient for generating the district court abbreviation. Are there any additional fields needed for the abbreviation logic?
✅ Verification successful
Let me search for the implementation of the districtCourtAbbreviation function to verify the required fields.
The court fields are sufficient for the abbreviation feature
The districtCourtAbbreviation function only requires the court's name field to generate the abbreviation. The id field is not used in the abbreviation logic, but it's still useful to keep for other purposes. No additional fields are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for the abbreviation utility function to verify required fields
# Find files containing district court abbreviation logic
rg -A 5 "districtCourtAbbreviation" apps/judicial-system/web/src/
Length of output: 4126
Script:
#!/bin/bash
# Search for the implementation of districtCourtAbbreviation
rg -l "export.*districtCourtAbbreviation" libs/
Length of output: 103
Script:
#!/bin/bash
# Get the implementation of districtCourtAbbreviation function
rg -A 10 "export.*districtCourtAbbreviation" libs/judicial-system/formatters/src/lib/formatters.ts
Length of output: 459
37-40: Consider query optimization to prevent over-fetching.
The addition of the court field looks good. However, since this query is used in the shared cases route, we should verify that we're not over-fetching court data in components that don't need it.
apps/judicial-system/backend/src/app/modules/case/interceptors/caseList.interceptor.ts (1)
67-67: Verify security implications of exposing court information
Given the warning comment about sensitive information, we should verify that exposing the court field is safe for all user roles accessing this endpoint.
✅ Verification successful
Court information is safe to expose in the case list endpoint
Based on the evidence found:
- The court field is already exposed in multiple test cases and endpoints, indicating it's considered public information
- The interceptor's warning comment specifically mentions to be careful with "sensitive information", but the PR objectives indicate this is for showing district court abbreviation to public prosecutors
- The CaseListInterceptor is used with a public API endpoint (@controller('api')) and already exposes several non-sensitive case fields
- The court information appears to be basic institutional data (name/abbreviation) rather than sensitive case details
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any access control or authorization checks related to court information
rg -l "court.*sensitive" apps/judicial-system
rg -l "Institution.*sensitive" apps/judicial-system
# Search for related authorization decorators or guards
ast-grep --pattern 'class $_ implements $_ {
$$$
@Roles($_)
$$$
}'
Length of output: 176
Script:
#!/bin/bash
# Let's try a broader search for authorization and court-related code
# Search for role-based access control
rg -A 5 "@Roles\(" apps/judicial-system/backend
# Search for court-related models and DTOs to understand the data structure
rg -l "class.*Court" apps/judicial-system/backend
fd Court.ts apps/judicial-system/backend
# Search for the Institution type definition
fd Institution.ts apps/judicial-system/backend
rg -A 5 "interface Institution" apps/judicial-system/backend
# Check the interceptor's usage context
rg -A 5 "CaseListInterceptor" apps/judicial-system/backend
Length of output: 2778
Script:
#!/bin/bash
# Let's examine the Case model and its usage
rg -A 10 "class Case" apps/judicial-system/backend
rg -A 10 "interface Case" apps/judicial-system/backend
# Check if court information is exposed in other public endpoints
rg -A 5 "@Public\(" apps/judicial-system/backend
# Look for existing court-related properties in responses
rg "court:" apps/judicial-system/backend/src/app/modules/case
# Check the controller's role decorators
rg -A 5 "@Controller\(" apps/judicial-system/backend/src/app/modules/case/case.controller.ts
Length of output: 26680
apps/judicial-system/api/src/app/modules/case-list/models/caseList.model.ts (1)
80-82: LGTM! The new court field is well-implemented.
The field is properly typed as Institution and correctly marked as nullable using GraphQL decorators, which aligns with the PR objectives of displaying district court information.
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesForReview.tsx (2)
6-10: LGTM! Import changes are well-structured.
The addition of districtCourtAbbreviation to the existing formatter imports maintains good code organization.
48-51: LGTM! Sortable configuration enhances table usability.
The addition of sorting capability for the court case number column follows the established pattern and improves user experience for public prosecutors managing multiple cases.
apps/judicial-system/web/src/routes/PublicProsecutor/Tables/CasesReviewed.tsx (2)
6-9: LGTM! Clean import organization.
The addition of districtCourtAbbreviation is well-organized within the existing formatter imports.
95-98: LGTM! Sorting enhancement improves usability.
The addition of sorting capability for court case numbers will help prosecutors better organize their case list.
apps/judicial-system/web/src/components/Table/Table.tsx (1)
9-12: LGTM: Clean import organization
The addition of districtCourtAbbreviation is well-organized, grouped appropriately with related formatting utilities.
libs/judicial-system/formatters/src/lib/formatters.ts (1)
221-242: Implementation aligns perfectly with PR objectives.
The districtCourtAbbreviation function effectively supports the PR's goal of helping public prosecutor staff identify district courts by providing clear, consistent abbreviations. The implementation in the formatters library ensures it can be reused across different parts of the application.
apps/judicial-system/backend/src/app/modules/case/case.service.ts (1)
411-411: Well-integrated change with existing functionality.
The addition of court information to case lists is cleanly implemented and maintains consistency with the existing pattern of eager loading relationships. The change effectively supports the requirement to display district court information while preserving the service's existing behavior.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17075 +/- ##
==========================================
- Coverage 35.73% 35.70% -0.04%
==========================================
Files 6926 6924 -2
Lines 147691 148391 +700
Branches 42073 42371 +298
==========================================
+ Hits 52775 52979 +204
- Misses 94916 95412 +496 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 49 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 23 Total Test Services: 0 Failed, 22 Passed Test ServicesThis report shows up to 10 services
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/web/src/routes/Shared/Cases/MobileCase.tsx (1)
73-73: Add type safety for the court propertyWhile the optional chaining handles undefined cases well, consider adding proper TypeScript types for the court property in the
CaseListEntryinterface to ensure type safety.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/web/src/routes/Shared/Cases/MobileCase.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Shared/Cases/MobileCase.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."
🔇 Additional comments (1)
apps/judicial-system/web/src/routes/Shared/Cases/MobileCase.tsx (1)
10-10: LGTM: Import addition is well-placed
The new import is correctly grouped with other formatter utilities.
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
oddsson
changed the title
oddsson
changed the title
* Add court abbr to getAllCases call * Refactoring * Refactoring * Refactoring * Refactoring * Add abbr to mobile as well * >merge --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Show district court abbreviation for public prosecutor
Asana
What
In the case list for public prosecutor, show a abbreviation of the district court's name with the court case number.
Why
The staff at the public prosecutors need to see which district court sent a case
Screenshots / Gifs
Checklist:
Summary by CodeRabbit
Release Notes
New Features
courtproperty to case entries, enhancing data related to court associations.districtCourtAbbreviationfunction for improved display of court case numbers.Improvements