feat(j-s): Display service status #16177
Conversation
…ment-case-arraignment-date
…ment-case-arraignment-date
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
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
|
Datadog ReportAll test runs ✅ 9 Total Test Services: 0 Failed, 9 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (3) |
…ment-case-new-subpoena
…ment-case-new-subpoena
…d.is into j-s/show-subpoena-status
![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 (2)
apps/judicial-system/web/src/utils/hooks/useSubpoena/getSubpoenaStatus.graphql (1)
3-8: LGTM: Comprehensive field selection for subpoena status.The selected fields (
serviceStatus,servedBy,comment,serviceDate,defenderNationalId) provide a comprehensive view of the subpoena status, aligning well with the PR objectives.Consider adding a
__typenamefield to the query. This can be helpful for client-side type checking and debugging, especially if you're using Apollo Client or a similar GraphQL client that supports automatic cache updates.You could add the
__typenamefield like this:query SubpoenaStatus($input: SubpoenaStatusQueryInput!) { subpoenaStatus(input: $input) { + __typename serviceStatus servedBy comment serviceDate defenderNationalId } }apps/judicial-system/api/src/app/modules/police/models/subpoenaStatus.model.ts (1)
10-20: LGTM with suggestions: Optional fields are well-defined.The servedBy, comment, serviceDate, and defenderNationalId fields are correctly decorated for GraphQL, set as readonly and optional, which is consistent with their nullable status in the GraphQL schema.
Consider using more specific types for improved type safety:
- For serviceDate, you might want to use a Date type instead of string.
- For defenderNationalId, you could create a custom type (e.g., NationalId) to enforce a specific format.
Example:
@Field(() => GraphQLISODateTime, { nullable: true }) readonly serviceDate?: Date @Field(() => NationalId, { nullable: true }) readonly defenderNationalId?: NationalIdThis would require defining GraphQLISODateTime and NationalId types, but it would provide stronger type checking and potentially better GraphQL schema documentation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/judicial-system/api/src/app/modules/police/models/subpoenaStatus.model.ts (1 hunks)
- apps/judicial-system/backend/src/app/modules/police/police.service.ts (4 hunks)
- apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx (1 hunks)
- apps/judicial-system/web/src/utils/hooks/useSubpoena/getSubpoenaStatus.graphql (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/judicial-system/api/src/app/modules/police/models/subpoenaStatus.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/police/police.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/web/src/components/ServiceAnnouncement/ServiceAnnouncement.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/utils/hooks/useSubpoena/getSubpoenaStatus.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."
🔇 Additional comments (11)
apps/judicial-system/web/src/utils/hooks/useSubpoena/getSubpoenaStatus.graphql (1)
1-2: LGTM: Query structure and input are well-defined.The
SubpoenaStatusquery is correctly structured with a non-nullable input of typeSubpoenaStatusQueryInput. This ensures that the necessary data is always provided when querying for subpoena status.apps/judicial-system/api/src/app/modules/police/models/subpoenaStatus.model.ts (4)
1-3: LGTM: Imports and file structure are correct.The imports are properly structured, and the file is correctly placed within the NextJS application structure. The use of @nestjs/graphql demonstrates proper integration with GraphQL in a NestJS application.
5-6: LGTM: Class definition and decorator are correct.The SubpoenaStatus class is properly defined with the @ObjectType() decorator, indicating a GraphQL object type. The class is correctly exported and follows the PascalCase naming convention, adhering to TypeScript best practices.
7-8: LGTM: serviceStatus field is well-defined.The serviceStatus field is correctly decorated for GraphQL, set as readonly for immutability, and uses a custom type for improved type safety. The non-nullable definition is appropriate for a required field.
1-21: Overall, excellent implementation of the SubpoenaStatus model.This file successfully defines the SubpoenaStatus GraphQL object type, adhering to NextJS and TypeScript best practices. It demonstrates good use of decorators, type safety, and immutability. The structure and naming conventions are appropriate, and the code is clean and readable.
The suggestions for more specific types for serviceDate and defenderNationalId are minor improvements that could enhance type safety and schema documentation, but they're not critical issues.
This implementation aligns well with the PR objectives of displaying service status in the RVG system.
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx (2)
1-74: LGTM: Well-structured imports and helper functionsThe imports, helper functions, and error handling are well-organized and follow TypeScript and React best practices. The code is clean and easy to understand.
76-80: LGTM: Well-defined props interfaceThe
ServiceAnnouncementPropsinterface is correctly defined and follows the suggested naming convention. The props are appropriately typed.apps/judicial-system/backend/src/app/modules/police/police.service.ts (4)
26-30: LGTM: New imports for subpoena functionality.The new imports are correctly placed and follow TypeScript best practices. They appropriately introduce the necessary types and services for the new subpoena functionality.
Also applies to: 37-38
131-142: Consider refining thesubpoenaStructuredefinition.The
subpoenaStructureis well-defined using Zod for runtime type checking. However, consider the following improvements:
- Some fields might benefit from being non-nullable if they are always expected to have a value.
- The property name
prosecutedConfirmedSubpoenaThroughIslandisis unusually long. Consider shortening it or breaking it into more manageable parts.
151-152: LGTM: Proper injection of SubpoenaService.The
SubpoenaServiceis correctly injected into the constructor using@InjectandforwardRef. This follows the established pattern in the class and adheres to dependency injection best practices.
339-416: 🛠️ Refactor suggestionImprove readability and error handling in
getSubpoenaStatus.The
getSubpoenaStatusmethod is well-structured and follows the established error handling pattern. However, consider the following improvements:
Refactor the complex nested ternary operator for
serviceStatus(lines 361-370) into a more readable structure, such as a switch statement or a separate function.Update the error message in the
postErrorEventcall (line 401) from 'Failed to get subpoena' to 'Failed to get subpoena status' for clarity.Apply this refactored code for the
serviceStatusdetermination:- serviceStatus: response.deliveredToLawyer - ? ServiceStatus.DEFENDER - : response.prosecutedConfirmedSubpoenaThroughIslandis - ? ServiceStatus.ELECTRONICALLY - : response.deliveredOnPaper || response.delivered === true - ? ServiceStatus.IN_PERSON - : response.acknowledged === false - ? ServiceStatus.FAILED - : // TODO: handle expired - undefined, + serviceStatus: (() => { + if (response.deliveredToLawyer) return ServiceStatus.DEFENDER; + if (response.prosecutedConfirmedSubpoenaThroughIslandis) return ServiceStatus.ELECTRONICALLY; + if (response.deliveredOnPaper || response.delivered === true) return ServiceStatus.IN_PERSON; + if (response.acknowledged === false) return ServiceStatus.FAILED; + // TODO: handle expired + return undefined; + })(),And update the error message:
- 'Failed to get subpoena', + 'Failed to get subpoena status',
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx
Show resolved
Hide resolved
apps/judicial-system/web/src/components/ServiceAnnouncement/ServiceAnnouncement.tsx
Show resolved
Hide resolved
…is/island.is into j-s/show-subpoena-status
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/xrd-api/src/app/dto/subpoena.dto.ts (2)
23-26: LGTM! Consider adding date-time format validation.The addition of the
servedAtproperty is well-implemented with proper decorators for optional status, string validation, and API documentation.To enhance data integrity, consider adding a date-time format validation. You can use the
@IsISO8601()decorator from class-validator:@IsOptional() @IsString() +@IsISO8601() @ApiProperty({ type: String, required: false }) servedAt?: stringThis ensures that the
servedAtstring adheres to the ISO 8601 date-time format.
Line range hint
1-55: Great overall code quality. Minor suggestion for import organization.The code demonstrates excellent adherence to TypeScript best practices, effective use of decorators for validation and API documentation, and proper implementation of the DTO pattern. This aligns well with NextJS best practices and ensures type safety and clear API structure.
For improved readability and consistency, consider organizing the import statements. Group them by external libraries, then internal modules, separated by a blank line:
import { IsBoolean, IsEnum, IsOptional, IsString } from 'class-validator' import { ApiProperty } from '@nestjs/swagger' import { DefenderChoice } from '@island.is/judicial-system/types'This minor change enhances code organization and follows common TypeScript/JavaScript import conventions.
apps/judicial-system/xrd-api/src/app/app.service.ts (1)
146-156: Ensure correct inclusion of defender informationWhen constructing
updateToSend, the use ofdefenderInfo.defenderName || undefinedensures that only defined values are sent. If you implement the earlier suggestion to initializedefenderInfoproperties asundefined, the|| undefinedchecks may become unnecessary.Apply this diff if you have initialized
defenderInfoproperties asundefined:defenderName: defenderInfo.defenderName || undefined, defenderEmail: defenderInfo.defenderEmail || undefined, defenderPhoneNumber: defenderInfo.defenderPhoneNumber || undefined,Can be simplified to:
defenderName: defenderInfo.defenderName, defenderEmail: defenderInfo.defenderEmail, defenderPhoneNumber: defenderInfo.defenderPhoneNumber,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- apps/judicial-system/xrd-api/src/app/app.service.ts (3 hunks)
- apps/judicial-system/xrd-api/src/app/dto/subpoena.dto.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/xrd-api/src/app/app.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/xrd-api/src/app/dto/subpoena.dto.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/xrd-api/src/app/dto/subpoena.dto.ts (1)
Learnt from: oddsson PR: island-is/island.is#16177 File: apps/judicial-system/backend/src/app/modules/subpoena/dto/updateSubpoena.dto.ts:16-16 Timestamp: 2024-10-04T15:11:19.318Z Learning: In the TypeScript file `apps/judicial-system/xrd-api/src/app/dto/subpoena.dto.ts`, the property is `servedBy`, not `registeredBy`. It is correctly named, and no renaming is necessary.
🔇 Additional comments (4)
apps/judicial-system/xrd-api/src/app/dto/subpoena.dto.ts (1)
Line range hint
1-55: Verify the impact of removingdeliveryInvalidated.The
deliveryInvalidatedproperty has been removed from theUpdateSubpoenaDtoclass. While this aligns with the PR objective of updating the subpoena service status display, we need to ensure this removal doesn't negatively impact existing functionality.Please run the following script to check for any remaining usage of
deliveryInvalidated:If any usages are found, please update those occurrences accordingly or provide a rationale for keeping the property removed.
✅ Verification successful
Action Required: Enhance the search for
deliveryInvalidatedusage.Please run the following script to thoroughly check for any remaining usage of
deliveryInvalidatedacross the codebase:
Action Required: Perform a comprehensive search for
deliveryInvalidatedusage across all files.Please run the following script to thoroughly check for any remaining usage of
deliveryInvalidatedin the codebase:
Verified: Removal of
deliveryInvalidateddoes not impact the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of deliveryInvalidated in the codebase # Search for deliveryInvalidated in TypeScript and JavaScript files echo "Searching for deliveryInvalidated usage:" rg --type-add 'script:*.{ts,js}' -t script 'deliveryInvalidated' # Search for any TODO or FIXME comments related to deliveryInvalidated echo "\nSearching for TODO or FIXME comments related to deliveryInvalidated:" rg --type-add 'script:*.{ts,js}' -t script -e 'TODO.*deliveryInvalidated' -e 'FIXME.*deliveryInvalidated'Length of output: 414
Script:
# #!/bin/bash # Description: Comprehensive search for any remaining usage of deliveryInvalidated in the codebase echo "Performing a case-insensitive search for deliveryInvalidated usage in TypeScript, JavaScript, and related files:" rg -i --type-add 'scripts:*.{ts,js,tsx,jsx}' -t scripts 'deliveryInvalidated' echo -e "\nSearching for TODO or FIXME comments related to deliveryInvalidated, case-insensitive:" rg -i --type-add 'scripts:*.{ts,js,tsx,jsx}' -t scripts -e 'TODO.*deliveryInvalidated' -e 'FIXME.*deliveryInvalidated'Length of output: 622
Script:
# #!/bin/bash # Description: Comprehensive search for any remaining usage of deliveryInvalidated in the codebase without file type restrictions echo "Performing a case-insensitive search for 'deliveryInvalidated' across all files:" rg -i 'deliveryInvalidated' echo -e "\nSearching for TODO or FIXME comments related to 'deliveryInvalidated', case-insensitive:" rg -i -e 'TODO.*deliveryInvalidated' -e 'FIXME.*deliveryInvalidated'Length of output: 474
apps/judicial-system/xrd-api/src/app/app.service.ts (3)
19-19: Import statement is appropriate and necessaryThe addition of
ServiceStatusto the import statement is correct, as it is used later in the code for defining theserviceStatusvariable.
106-107: Validation logic is correctThe condition correctly ensures that when
defenderChoiceisDefenderChoice.CHOOSE, adefenderNationalIdmust be provided. This validation will prevent incomplete data from being processed.
167-167: Request body is correctly constructedThe
updateToSendobject is properly serialized and included in the request body. This ensures that all necessary data is sent in the PATCH request.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/xrd-api/src/app/app.service.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/xrd-api/src/app/app.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."
🔇 Additional comments (2)
apps/judicial-system/xrd-api/src/app/app.service.ts (2)
19-19: Import ofServiceStatusis appropriateThe addition of
ServiceStatusto the imports from@island.is/judicial-system/typesis necessary for determining the service status in the updated logic.
110-117: Validation logic is correctly implementedThe validation ensures that when
defenderChoiceisCHOOSE, adefenderNationalIdmust be provided. Throwing aBadRequestExceptionin this scenario is appropriate and ensures data integrity.
This reverts commit a11885c.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/police/test/createTestingPoliceModule.ts (2)
24-24: LGTM: SubpoenaService correctly mockedThe SubpoenaService is properly mocked using jest.mock, which is consistent with the mocking of other services in this file. This ensures that SubpoenaService will be mocked during tests.
For consistency, consider moving this mock call to be grouped with the other service mocks (e.g., after line 21).
Line range hint
15-41: Overall: Good enhancement of the testing moduleThe changes to include SubpoenaService in the testing module are well-implemented and consistent with the existing pattern for other services. This enhancement allows for more comprehensive testing of the police module's functionality, which is crucial for maintaining code quality.
To further improve the module's organization, consider grouping all service-related imports, mocks, and provider entries together. This could enhance readability and make future additions more straightforward.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/judicial-system/backend/src/app/modules/police/test/createTestingPoliceModule.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/police/test/createTestingPoliceModule.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."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/police/test/createTestingPoliceModule.ts (2)
15-15: LGTM: SubpoenaService import added correctlyThe import statement for SubpoenaService is correctly added and follows TypeScript best practices. This import is necessary for including SubpoenaService in the testing module.
41-41: LGTM: SubpoenaService added to providersSubpoenaService is correctly added to the providers array, allowing it to be injected into other components during testing. This is a necessary step for including the service in the testing module.
To ensure this addition is utilized, let's verify if SubpoenaService is being used in the PoliceService or PoliceController. Run the following script:
✅ Verification successful
Verified: SubpoenaService is utilized correctly
SubpoenaService is properly added to the providers array and is being used in PoliceService as intended. No issues were found regarding its usage in PoliceController.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if SubpoenaService is used in PoliceService or PoliceController # Search for SubpoenaService usage in PoliceService echo "Checking PoliceService for SubpoenaService usage:" rg "SubpoenaService" apps/judicial-system/backend/src/app/modules/police/police.service.ts # Search for SubpoenaService usage in PoliceController echo "Checking PoliceController for SubpoenaService usage:" rg "SubpoenaService" apps/judicial-system/backend/src/app/modules/police/police.controller.tsLength of output: 566
Display service status
Asana
What
We need to show the status of a subpoena service in RVG. This PR implements that. This info comes from RLS and we fetch it from them but we also save a copy in our db to display, just in case the connection ever goes down.
Why
Judges and prosecutors need to see the status of the service.
Screenshots / Gifs
Failed
Expired
Success - In person
Success - To defender
Success - island.is
Checklist:
Summary by CodeRabbit
Release Notes
New Features
serviceStatusproperty to better reflect the status of subpoenas.SubpoenaModulefor improved accessibility in the application.Enhancements
SubpoenaModuleinto thePoliceModulefor enhanced functionality.Bug Fixes
Documentation