fix(j-s): Display correct verdict appeal info for defenders #16533
Conversation
…expiry calculation to API
WalkthroughThe pull request introduces changes to the judicial system API and GraphQL queries, primarily focusing on renaming the function 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: 3
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/api/src/app/modules/case/interceptors/limitedAccessCase.transformer.ts (1)
47-54: Consider adding explicit return type for improved type safetyThe implementation correctly handles the conditional exposure of case information based on defender permissions.
Consider adding an explicit return type for better type safety:
-const transformRequestCase = (theCase: Case): Case => { +const transformRequestCase = (theCase: Case): Readonly<Case> => {apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts (1)
75-77: Add JSDoc documentation for the new field.The implementation looks good and aligns with the PR objective of moving verdict appeal expiry calculation to the API. However, consider adding JSDoc documentation to explain the field's purpose and when it might be null.
+ /** + * Indicates whether the verdict appeal deadline has expired. + * Null when verdict hasn't been viewed or appeal deadline hasn't been set. + */ @Field(() => Boolean, { nullable: true }) readonly isVerdictAppealDeadlineExpired?: booleanapps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (1)
688-709: Consider adding test cases for edge scenarios.The test suite effectively covers the basic scenarios for verdict appeal deadline handling. However, consider adding test cases for:
- Invalid verdict view dates
- Empty defendants array
- Null/undefined defendants
- Edge cases around the deadline calculation (e.g., date rollovers)
Example test cases:
it('should handle invalid verdict view dates gracefully', () => { const defendants = [ { verdictViewDate: 'invalid-date' } as Defendant, ] const defendantsInfo = getIndictmentDefendantsInfo(defendants) expect(defendantsInfo[0].verdictAppealDeadline).toBeUndefined() }) it('should handle empty defendants array', () => { const defendants: Defendant[] = [] const defendantsInfo = getIndictmentDefendantsInfo(defendants) expect(defendantsInfo).toEqual([]) }) it('should handle null/undefined defendants', () => { const defendants = [null, undefined] as unknown as Defendant[] const defendantsInfo = getIndictmentDefendantsInfo(defendants) expect(defendantsInfo).toEqual([{}, {}]) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (2 hunks)
- apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (2 hunks)
- apps/judicial-system/api/src/app/modules/case/interceptors/limitedAccessCase.transformer.ts (2 hunks)
- apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts (1 hunks)
- apps/judicial-system/web/src/components/FormProvider/case.graphql (1 hunks)
- apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.graphql (2 hunks)
- apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.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/api/src/app/modules/case/interceptors/case.transformer.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/api/src/app/modules/case/interceptors/limitedAccessCase.transformer.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/api/src/app/modules/defendant/models/defendant.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/web/src/components/FormProvider/case.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."
apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.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."
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 (4)
Learnt from: unakb PR: island-is/island.is#14922 File: apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx:43-49 Timestamp: 2024-10-08T15:39:04.351Z Learning: The `getAppealExpirationInfo` function in `DefendantInfo.tsx` correctly uses `today < expiryDate` to determine if the current date is before the appeal expiration date.Learnt from: oddsson PR: island-is/island.is#15461 File: apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx:36-62 Timestamp: 2024-07-08T13:30:58.001Z Learning: The user prefers readability over optimization in the `getAppealExpirationInfo` function within `DefendantInfo.tsx`.Learnt from: oddsson PR: island-is/island.is#15461 File: apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx:36-62 Timestamp: 2024-10-08T15:39:04.351Z 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-05-27T10:53:40.687Z 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 (12)
apps/judicial-system/api/src/app/modules/case/interceptors/limitedAccessCase.transformer.ts (2)
4-4: LGTM: Import changes enhance type safety and support new functionalityThe addition of the type guard and indictment-related imports aligns well with the PR's objective of improving verdict appeal information handling.
Also applies to: 9-12
56-69: 🛠️ Refactor suggestionAdd validation for required properties
While the implementation correctly transforms indictment cases, it might be vulnerable to runtime errors if required properties are missing.
Consider adding validation:
const transformIndictmentCase = (theCase: Case): Case => { + if (!theCase.defendants) { + throw new Error('Defendants array is required for indictment case transformation') + } const { indictmentRulingDecision, rulingDate, defendants, eventLogs } = theCase return {Let's verify the usage of these properties across the codebase:
apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.graphql (2)
37-40: LGTM! New defendant fields are well-structured.The addition of
serviceRequirementandisVerdictAppealDeadlineExpiredfields to the defendants object is well-placed and aligns with the PR objective of moving verdict appeal calculations to the API side.
164-165: Verify the relationship between case-level and defendant-level appeal fields.The new case-level fields look good, but let's verify their implementation:
- How does
indictmentVerdictViewedByAllrelate to individual defendant'sverdictViewDate?- How does
indictmentVerdictAppealDeadlineExpiredrelate to individual defendant'sisVerdictAppealDeadlineExpired?Let's check for the resolver implementation:
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx (3)
41-41: LGTM! Well-structured parameter addition.The new parameter
isVerdictAppealDeadlineExpiredfollows TypeScript best practices and maintains backward compatibility.
54-56: Clean and readable implementation.The simplified logic using the boolean flag improves maintainability while preserving the existing message structure. This aligns well with the team's preference for readability, as noted in previous feedback.
73-73: Verify GraphQL schema includes the new property.The code assumes
isVerdictAppealDeadlineExpiredis available in theDefendanttype. Let's verify this assumption.✅ Verification successful
Let me try a different approach to locate the GraphQL schema and query definitions.
This should help us locate:
- GraphQL schema files if they exist
- TypeScript interface definition for Defendant
- Usage of the property in question
Based on the search results, I can now provide a conclusive response about the GraphQL schema compatibility.
Property is correctly defined and used in the GraphQL schema
The
isVerdictAppealDeadlineExpiredproperty is:
- Defined in the GraphQL schema as shown in both
case.graphqlandlimitedAccessCase.graphqlfiles- Properly modeled in the backend as a nullable boolean field in
defendant.model.ts- Correctly computed in the backend transformer (
case.transformer.ts)- Used consistently in the frontend components
The property exists in the GraphQL schema and is properly integrated throughout the codebase, making its usage in
DefendantInfo.tsxsafe and correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that isVerdictAppealDeadlineExpired is defined in the GraphQL schema # and that all queries fetching Defendant include this field # Check schema definition rg -A 5 "type Defendant" # Check GraphQL queries rg -A 10 "fragment.*Defendant|query.*Defendant"Length of output: 1505
Script:
#!/bin/bash # Search for GraphQL schema files echo "=== Searching for GraphQL schema files ===" fd "\.(graphql|gql)$" # Search for type definitions in TypeScript files echo -e "\n=== Searching for Defendant type definitions ===" ast-grep --pattern 'interface Defendant { $$$ }' # Search for isVerdictAppealDeadlineExpired in TypeScript files echo -e "\n=== Searching for isVerdictAppealDeadlineExpired usage ===" rg "isVerdictAppealDeadlineExpired" -B 2 -A 2Length of output: 27794
apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
31-31: LGTM! Field placement and naming follow GraphQL conventions.The new field
isVerdictAppealDeadlineExpiredis logically placed near other verdict-related fields and follows boolean field naming conventions.Please update the documentation to reflect this new field, as noted in the PR checklist.
Let's verify the frontend usage of this new field:
✅ Verification successful
Let me try a different search strategy to find the frontend usage.
Let me check the actual implementation in the DefendantInfo component to verify proper usage.
The new field is properly integrated and used in the frontend
The verification shows that:
- The field is correctly used in
DefendantInfo.tsxcomponent- It's properly typed as
boolean | null- It's used in the
getAppealExpirationInfofunction to determine the correct message to display- The field is also present in both GraphQL queries (
case.graphqlandlimitedAccessCase.graphql)The implementation aligns perfectly with the PR's objective of moving verdict appeal expiry calculation from frontend to API.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find components using the new isVerdictAppealDeadlineExpired field # Expected: DefendantInfo component should use this field rg "isVerdictAppealDeadlineExpired" apps/judicial-system/web/src/components/ -t tsxLength of output: 111
Script:
#!/bin/bash # Search for the field usage without file type restriction since the extension might be .ts or .tsx rg "isVerdictAppealDeadlineExpired" apps/judicial-system/web/src/components/ # Also search in the entire web directory to catch any usage rg "isVerdictAppealDeadlineExpired" apps/judicial-system/web/src/Length of output: 1358
Script:
#!/bin/bash # Get the full context of how the field is used in DefendantInfo cat apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsxLength of output: 4714
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts (3)
172-174: LGTM: Clear and type-safe function signature.The function rename to
getIndictmentDefendantsInfobetter reflects its specific purpose in handling indictment defendants, improving code clarity.
189-189: LGTM: Property addition matches PR requirements.The addition of
isVerdictAppealDeadlineExpiredto the defendant info aligns with the PR objective of providing correct verdict appeal information.
203-203: Verify all usages of the renamed function.The function usage here is correct, but let's verify that all other occurrences of the old function name have been updated.
✅ Verification successful
Let me try to search for both the old and new function names to get a better understanding.
All usages of the renamed function have been properly updated
The search results show that:
- No instances of the old function name
getDefendantsInfoexist in the codebase- The new function name
getIndictmentDefendantsInfois consistently used across all relevant files:
- Defined and exported in
case.transformer.ts- Used in
case.transformer.tsandlimitedAccessCase.transformer.ts- Properly tested in
case.transformer.spec.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old function name rg "getDefendantsInfo" --type tsLength of output: 32
Script:
#!/bin/bash # Search for both old and new function names echo "Searching for old function name (getDefendantsInfo):" rg "getDefendantsInfo" -A 2 --type ts echo -e "\nSearching for new function name (getIndictmentDefendantsInfo):" rg "getIndictmentDefendantsInfo" -A 2 --type ts # Also search for function declaration ast-grep --pattern 'function getIndictmentDefendantsInfo($_) { $$$ }'Length of output: 2875
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.spec.ts (1)
17-17: LGTM! Clear and descriptive function rename.The rename from
getDefendantsInfotogetIndictmentDefendantsInfobetter reflects the function's purpose and aligns with the PR's objective.
apps/judicial-system/api/src/app/modules/case/interceptors/limitedAccessCase.transformer.ts
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts
Show resolved
Hide resolved
apps/judicial-system/api/src/app/modules/case/interceptors/case.transformer.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16533 +/- ##
==========================================
- Coverage 36.77% 36.75% -0.03%
==========================================
Files 6853 6853
Lines 142181 142176 -5
Branches 40556 40556
==========================================
- Hits 52292 52252 -40
- Misses 89889 89924 +35 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.spec.ts (2)
67-73: Consider using dynamic dates in testsWhile the test logic is correct, using hardcoded future dates ('2024-08-05') might cause the tests to fail when these dates pass. Consider using dynamic date calculation relative to the mocked system time.
Example refactor:
- const verdictAppealDeadline = '2024-08-05' + const futureDate = new Date('2024-07-08') // Using mocked system time + futureDate.setDate(futureDate.getDate() + 28) // 28 days in future + const verdictAppealDeadline = futureDate.toISOString().split('T')[0]Also applies to: 84-90
Line range hint
1-133: Consider enhancing type safety in testsWhile the tests are well-structured, consider adding TypeScript type annotations for the test parameters and return values. This would make the tests more maintainable and catch potential type-related issues early.
Example:
interface AppealExpirationInfo { message: { id: string }; date?: string; } // Use in test cases: const dataSections: AppealExpirationInfo = getAppealExpirationInfo(...)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts (1 hunks)
- apps/judicial-system/web/src/components/FormProvider/case.graphql (1 hunks)
- apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.spec.ts (7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts
- apps/judicial-system/web/src/components/FormProvider/case.graphql
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.spec.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."
📓 Learnings (1)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.spec.ts (1)
Learnt from: unakb PR: island-is/island.is#14922 File: apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.tsx:43-49 Timestamp: 2024-10-08T15:39:04.351Z 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 (2)
apps/judicial-system/web/src/components/InfoCard/DefendantInfo/DefendantInfo.spec.ts (2)
18-24: LGTM! Test correctly handles undefined deadline scenarioThe test case properly sets
isVerdictAppealDeadlineExpiredto false when no deadline is provided, maintaining the expected behavior while adapting to the new API-driven expiration calculation.
101-107: Consider adding edge case test for same-day expirationThe tests cover past and future dates well, but there's no test case for when
verdictAppealDeadlineis exactly the same as the current date ('2024-07-08'). This edge case should be tested to ensure correct behavior.Also applies to: 118-124
Ef ekki þarf að birta dóm á að standa birting dóms ekki þörf á info cardi hjá verjanda.
What
Why
Checklist:
Summary by CodeRabbit
New Features
isVerdictAppealDeadlineExpiredto provide information on the status of verdict appeal deadlines for defendants.Bug Fixes
getAppealExpirationInfofunction to improve clarity and efficiency.Documentation