feat(j-s): Add punishment type to indictment overview #17198
Conversation
WalkthroughThis pull request introduces multiple enhancements to the judicial system's backend, focusing on limited access functionality. Key additions include new methods in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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
|
thorhildurt
changed the title
|
|
||
| return this.auditTrailService.audit( | ||
| user.id, | ||
| AuditedAction.UPDATE_DEFENDANT, |
There was a problem hiding this comment.
Using the same value for the general update endpoint
| ) {} | ||
|
|
||
| @UseGuards(CaseExistsGuard, DefendantExistsGuard) | ||
| @RolesRules(prisonSystemStaffUpdateRule) |
There was a problem hiding this comment.
This will ensure that prison staff can only pass in punishmentType and no other fields of the UpdateDefendantDto
There was a problem hiding this comment.
I only added tests that check the Roles rules config but not a test that enforces the logic it applies.
| defaultMessage: 'Sekt', | ||
| description: 'Notaður sem texti í sekt valmöguleika í Refsitegund', | ||
| }, | ||
| indictmentRulingDecisionfine: { |
There was a problem hiding this comment.
Saw this being used for "Viðurlagaákvörðun" in another place (apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts)
| setPunishmentType(updatedPunishmentType) | ||
| } | ||
|
|
||
| const hasSetPunishmentType = (punishmentType: PunishmentType) => |
There was a problem hiding this comment.
When a punishmentType value is already stored in the db, by default the radio button selection is empty
| @@ -78,6 +84,26 @@ const useDefendants = () => { | |||
| [formatMessage, updateDefendantMutation], | |||
| ) | |||
|
|
|||
| // TODO: This is maybe not ideal? | |||
| const limitedAccessUpdateDefendant = useCallback( | |||
There was a problem hiding this comment.
This is a bit redundant of updateDefendant above. In cases we handle these the mutations calls within the same function by checking the user...but maybe this is ok for now?
Datadog ReportAll test runs ✅ 98 Total Test Services: 0 Failed, 90 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (4) |
https://github.com/island-is/island.is into j-s/update-punishment-type-prison-indictment-overview
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
apps/judicial-system/backend/src/app/modules/defendant/guards/rolesRules.ts (1)
5-5: EnsurelimitedAccessFieldsaccurately reflects permissible fieldsCurrently,
limitedAccessFieldsincludes only['punishmentType']. Verify that this array includes all fields thatPRISON_SYSTEM_STAFFshould be allowed to update, and exclude any that they should not.apps/judicial-system/backend/migrations/20241211095654-update-defendant.js (1)
1-2: Remove redundant 'use strict' directiveThe 'use strict' directive is redundant in ES modules as they are automatically in strict mode.
-'use strict' -🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
apps/judicial-system/api/src/app/modules/defendant/limitedAccessDefendant.resolver.ts (1)
30-53: Consider adding error handling for the backend service call.While the implementation is generally good, the backend service call could benefit from explicit error handling.
Consider wrapping the backend service call in a try-catch:
@Mutation(() => Defendant, { nullable: true }) async limitedAccessUpdateDefendant( @Args('input', { type: () => UpdateDefendantInput }) input: UpdateDefendantInput, @CurrentGraphQlUser() user: User, @Context('dataSources') { backendService }: { backendService: BackendService }, ): Promise<Defendant> { const { caseId, defendantId, ...updateDefendant } = input this.logger.debug( `Updating limitedAccess defendant ${defendantId} for case ${caseId}`, ) + try { return this.auditTrailService.audit( user.id, AuditedAction.UPDATE_DEFENDANT, backendService.limitedAccessUpdateDefendant( caseId, defendantId, updateDefendant, ), defendantId, ) + } catch (error) { + this.logger.error( + `Failed to update defendant ${defendantId}`, + error, + ) + throw error + } }apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts (1)
51-55: Consider reusing existing translation for "Viðurlagaákvörðun".Based on the previous review comment, this string already exists in
BlueBoxWithDate.strings.ts.Consider importing and reusing the existing translation to maintain consistency.
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
100-190: Consider extracting radio buttons to a separate component.The current implementation has repetitive radio button code. Consider extracting it to a reusable component to improve maintainability.
Example implementation:
interface PunishmentTypeRadioProps { id: string; type: PunishmentType; selectedType: PunishmentType | null; onChange: (type: PunishmentType) => void; label: string; defendant?: Defendant; } const PunishmentTypeRadio: React.FC<PunishmentTypeRadioProps> = ({ id, type, selectedType, onChange, label, defendant, }) => ( <Box marginBottom={2}> <RadioButton id={id} name={id} checked={selectedType === type || (selectedType === null && defendant?.punishmentType === type)} onChange={() => onChange(type)} large backgroundColor="white" label={label} /> </Box> );apps/judicial-system/api/src/app/modules/backend/backend.service.ts (1)
Line range hint
479-483: Fix typo in URL paths.There's a typo in the URLs where
$is used instead of/.- `case/${id}/limitedAccess$/defendant/${defendantId}/file`, + `case/${id}/limitedAccess/defendant/${defendantId}/file`,- `case/${id}/limitedAccess$/civilClaimant/${civilClaimantId}/file`, + `case/${id}/limitedAccess/civilClaimant/${civilClaimantId}/file`,Also applies to: 491-495
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
apps/judicial-system/api/src/app/modules/backend/backend.service.ts(1 hunks)apps/judicial-system/api/src/app/modules/defendant/defendant.module.ts(1 hunks)apps/judicial-system/api/src/app/modules/defendant/dto/updateDefendant.input.ts(2 hunks)apps/judicial-system/api/src/app/modules/defendant/limitedAccessDefendant.resolver.ts(1 hunks)apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts(3 hunks)apps/judicial-system/backend/migrations/20241211095654-update-defendant.js(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/defendant.module.ts(2 hunks)apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts(2 hunks)apps/judicial-system/backend/src/app/modules/defendant/guards/rolesRules.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/models/defendant.model.ts(2 hunks)apps/judicial-system/web/src/components/FormProvider/case.graphql(1 hunks)apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.graphql(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx(4 hunks)apps/judicial-system/web/src/utils/hooks/useDefendants/index.ts(3 hunks)apps/judicial-system/web/src/utils/hooks/useDefendants/limitedAccessUpdateDefendant.graphql(1 hunks)libs/judicial-system/types/src/index.ts(1 hunks)libs/judicial-system/types/src/lib/defendant.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (19)
apps/judicial-system/web/src/utils/hooks/useDefendants/limitedAccessUpdateDefendant.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/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/backend/src/app/modules/defendant/defendant.module.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/migrations/20241211095654-update-defendant.js (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/defendant/guards/rolesRules.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/dto/updateDefendant.input.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/utils/hooks/useDefendants/index.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."
libs/judicial-system/types/src/lib/defendant.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/judicial-system/types/src/index.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."
apps/judicial-system/api/src/app/modules/defendant/defendant.module.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/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/api/src/app/modules/defendant/limitedAccessDefendant.resolver.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/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/backend/src/app/modules/defendant/dto/updateDefendant.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."
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/backend/src/app/modules/defendant/limitedAccessDefendant.controller.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/routes/Prison/IndictmentOverview/IndictmentOverview.strings.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/routes/Prison/IndictmentOverview/IndictmentOverview.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/api/src/app/modules/backend/backend.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."
📓 Learnings (3)
apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
Learnt from: gudjong
PR: island-is/island.is#16863
File: apps/judicial-system/web/src/components/FormProvider/case.graphql:47-47
Timestamp: 2024-11-27T14:33:19.485Z
Learning: In the `Case` GraphQL query at `apps/judicial-system/web/src/components/FormProvider/case.graphql`, the `defendantId` field is not needed in the `subpoenas` object within the `mergedCases.defendants` array.
apps/judicial-system/backend/migrations/20241211095654-update-defendant.js (3)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/backend/migrations/20241022142128-update-defendant.js:1-2
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the 'judicial-system' application, it is a convention to include `'use strict'` directives at the top of migration files in `apps/judicial-system/backend/migrations/*.js`.
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/backend/migrations/20241022142128-update-defendant.js:18-24
Timestamp: 2024-11-12T15:15:20.157Z
Learning: When rolling back migrations, if the associated code that uses the fields is also rolled back, data preservation concerns may not be an issue.
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/backend/migrations/20241114123528-create-defendant-event-log.js:0-0
Timestamp: 2024-11-18T21:57:03.051Z
Learning: In the `apps/judicial-system/backend` codebase, validations are not typically included in Sequelize migration files.
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts (2)
Learnt from: oddsson
PR: island-is/island.is#16495
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts:3-3
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.strings.ts`, prefer not to add explicit TypeScript type definitions for the messages object.
Learnt from: oddsson
PR: island-is/island.is#16831
File: apps/judicial-system/web/src/components/BlueBoxWithIcon/BlueBoxWithDate.tsx:204-337
Timestamp: 2024-11-19T09:28:36.448Z
Learning: Buttons in this project are considered accessible as is, and adding additional ARIA attributes like `aria-label` and `aria-disabled` is not required unless specifically needed.
🪛 Biome (1.9.4)
apps/judicial-system/backend/migrations/20241211095654-update-defendant.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (22)
apps/judicial-system/web/src/utils/hooks/useDefendants/limitedAccessUpdateDefendant.graphql (1)
1-5: Mutation is well-defined and follows GraphQL best practices
The LimitedAccessUpdateDefendant mutation is correctly structured and aligns with GraphQL conventions. It efficiently updates the defendant and returns the necessary fields.
apps/judicial-system/api/src/app/modules/defendant/defendant.module.ts (1)
5-12: LimitedAccessDefendantResolver added appropriately to the module
The new resolver is properly imported and included in the providers array of the DefendantModule. This ensures that limited access functionalities are correctly integrated.
apps/judicial-system/backend/migrations/20241211095654-update-defendant.js (1)
7-15: LGTM! Column definition aligns with requirements
The column definition is properly structured with:
- Appropriate nullable string type to store enum values
- Transaction usage for data integrity
- Follows existing migration patterns
apps/judicial-system/backend/src/app/modules/defendant/defendant.module.ts (1)
16-16: LGTM! Controller integration follows NestJS patterns
The LimitedAccessDefendantController is properly integrated into the module following NestJS best practices.
Also applies to: 29-29
libs/judicial-system/types/src/lib/defendant.ts (1)
39-45: LGTM! PunishmentType enum properly defined
The enum follows TypeScript best practices and includes all required punishment types.
Let's verify the enum values are consistently used across the codebase:
✅ Verification successful
PunishmentType enum values are consistently used across the codebase
The verification shows that:
- All enum values are properly referenced using the
PunishmentType.notation in the IndictmentOverview component - String literals match exactly with the enum definitions
- There's one additional 'FINE' string literal in case.ts, but this appears to be a different enum as it's defined in a separate types file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PunishmentType enum usage consistency
# Search for PunishmentType enum usage
rg "PunishmentType\." --type ts
# Search for string literals matching enum values to catch inconsistencies
rg "'(IMPRISONMENT|PROBATION|FINE|INDICTMENT_RULING_DECISION_FINE|SIGNED_FINE_INVITATION)'" --type ts
Length of output: 2904
apps/judicial-system/api/src/app/modules/defendant/limitedAccessDefendant.resolver.ts (1)
21-28: LGTM! Well-structured resolver class with proper dependency injection.
The class follows NestJS best practices with:
- Appropriate use of guards for authentication
- Clean dependency injection for audit trail and logging services
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts (1)
29-33: LGTM! Clear and consistent title definition.
The punishment type title follows the established pattern and includes proper description.
apps/judicial-system/api/src/app/modules/defendant/dto/updateDefendant.input.ts (1)
118-122: LGTM! Well-structured field definition.
The new punishmentType field follows the established pattern with:
- Proper validation decorators (@Allow, @IsOptional)
- Correct GraphQL field definition
- Consistent with other optional fields in the class
libs/judicial-system/types/src/index.ts (1)
10-10: LGTM! Export follows established patterns
The addition of PunishmentType export is properly grouped with other defendant-related types.
apps/judicial-system/api/src/app/modules/defendant/models/defendant.model.ts (3)
7-7: LGTM! Import follows established patterns
The PunishmentType import is correctly grouped with other defendant-related types.
19-19: LGTM! Enum registration follows established patterns
The PunishmentType enum is properly registered for GraphQL usage.
112-114: Verify impact of the new nullable field
The punishmentType field is correctly implemented as an optional field. However, since this is a new field in an existing GraphQL type, we should verify that existing queries won't break.
✅ Verification successful
Let me analyze the results of these commands to better understand the GraphQL usage patterns and potential impact of the nullable field.
Let me analyze the results to understand the GraphQL schema and usage patterns for the Defendant type.
Let me analyze these results to understand the GraphQL schema and usage patterns.
Optional field addition is safe to proceed
Based on the codebase analysis, the punishmentType field is being added as an optional (nullable) field to the Defendant type. Since it's marked as nullable with @Field(() => PunishmentType, { nullable: true }), existing queries will continue to work without breaking changes, as GraphQL clients are not required to request this field, and when they do, they can handle null values.
The search results show that the Defendant type is primarily used within the judicial system's backend services, and there's no evidence of strict non-null requirements for this field in existing queries. Adding an optional field is a backward-compatible change in GraphQL schemas.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing GraphQL queries that fetch Defendant type
# to ensure they won't break with the new field
rg -l "type Defendant" | xargs rg "query.*{.*defendant.*{" -A 10
Length of output: 179
Script:
#!/bin/bash
# Search for GraphQL queries related to Defendant type using a more reliable pattern
rg "type Defendant" --type ts
# Search for GraphQL query files that might reference defendant
fd ".graphql$"
# Search for defendant-related queries in TypeScript files
rg "query.*defendant" --type ts
# Look for specific GraphQL operations involving defendant
rg "defendant.*{" --type graphql
Length of output: 21017
Script:
#!/bin/bash
# Search for GraphQL queries/mutations involving defendant type in the judicial system app
rg -t ts -t graphql "type Defendant" -A 10
# Look for GraphQL operations using defendant fields
rg -t graphql "defendant\s*[{:]" --multiline
# Search for GraphQL schema definitions related to Defendant
ast-grep --pattern 'type Defendant {
$$$
}'
Length of output: 203
Script:
#!/bin/bash
# Search for Defendant type definition and its usages
rg "type Defendant" -A 10
# Look for GraphQL operations with defendant
rg "defendant.*{" --type ts
# Search for files that might contain GraphQL operations using defendant
rg -l "defendant" --type ts | grep -i "graphql\|query\|mutation"
Length of output: 1877
apps/judicial-system/backend/src/app/modules/defendant/dto/updateDefendant.dto.ts (2)
18-18: LGTM! Import follows established patterns
The PunishmentType import is correctly grouped with other type imports.
154-157: Verify role-based access control for the new field
The field is properly validated and documented. However, we should verify that appropriate role-based access controls are in place for updating this field.
✅ Verification successful
Role-based access controls are properly implemented for the punishmentType field
The punishmentType field is adequately protected by role-based access controls:
-
In the
LimitedAccessDefendantController, updates to punishmentType are restricted to prison system staff through:@UseGuards(JwtAuthGuard, RolesGuard)@RolesRules(prisonSystemStaffUpdateRule)- A specific
LimitedAccessUpdateDefendantinterface that only allows punishmentType updates
-
In the main
DefendantController, updates are protected by:@UseGuards(JwtAuthGuard, RolesGuard, CaseExistsGuard, CaseWriteGuard, DefendantExistsGuard)@RolesRuleslimiting access to prosecutors, court staff, and public prosecutor staff
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for role-based access control rules related to punishment type updates
# Look for guards or middleware
rg -l "PunishmentType" | xargs rg "Guard|Middleware|@Roles|UserRole" -A 5
# Look for role-based rules
rg -l "defendant.*controller" | xargs rg "roles.*Rules" -A 10
Length of output: 503
Script:
#!/bin/bash
# Let's search more specifically for RBAC patterns in the defendant-related files
# Search for controller and service files related to defendant
fd "defendant.*\.(controller|service)\.(ts|js)" --exec rg -B 5 -A 10 "@(Roles|UseGuards|RolesGuard|AuthGuard)"
# Search for any route decorators or guards specifically around punishmentType updates
fd "defendant.*\.(controller|service)\.(ts|js)" --exec rg -B 5 -A 10 "updateDefendant|punishmentType"
# Look for auth guards in the main app module or auth modules
fd "(app\.module|auth\.module)\.ts" --exec rg -B 2 -A 5 "Guard"
Length of output: 9490
apps/judicial-system/web/src/components/FormProvider/limitedAccessCase.graphql (1)
50-50: LGTM! The punishmentType field is correctly placed.
The addition of the punishmentType field to the defendants object follows GraphQL schema conventions and is logically placed within the query structure.
apps/judicial-system/web/src/utils/hooks/useDefendants/index.ts (2)
16-16: LGTM! Hook import and initialization is correct.
The mutation hook is properly imported and initialized following React hooks best practices.
Also applies to: 29-31
87-103: 🛠️ Refactor suggestion
Consider reducing code duplication with updateDefendant.
The limitedAccessUpdateDefendant function is nearly identical to updateDefendant. Consider refactoring to share the common logic:
-const limitedAccessUpdateDefendant = useCallback(
- async (updateDefendant: UpdateDefendantInput) => {
- try {
- const { data } = await limitedAccessUpdateDefendantMutation({
- variables: {
- input: updateDefendant,
- },
- })
-
- return Boolean(data)
- } catch (error) {
- toast.error(formatMessage(errors.updateDefendant))
- return false
- }
- },
- [formatMessage, limitedAccessUpdateDefendantMutation],
-)
+const createDefendantUpdater = (mutation: any) =>
+ useCallback(
+ async (updateDefendant: UpdateDefendantInput) => {
+ try {
+ const { data } = await mutation({
+ variables: {
+ input: updateDefendant,
+ },
+ })
+
+ return Boolean(data)
+ } catch (error) {
+ toast.error(formatMessage(errors.updateDefendant))
+ return false
+ }
+ },
+ [formatMessage, mutation],
+ )
+
+const updateDefendant = createDefendantUpdater(updateDefendantMutation)
+const limitedAccessUpdateDefendant = createDefendantUpdater(limitedAccessUpdateDefendantMutation)Likely invalid or redundant comment.
apps/judicial-system/backend/src/app/modules/defendant/models/defendant.model.ts (1)
205-212: LGTM! The punishmentType column is properly defined.
The implementation follows Sequelize best practices:
- Correct use of ENUM type with proper validation
- Consistent with other enum columns in the model
- Properly documented with Swagger decorators
- Nullable as required for optional fields
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (2)
56-57:
Fix the radio button selection issue.
The current implementation doesn't handle the initial state correctly when a punishment type is already stored in the database.
-const hasSetPunishmentType = (punishmentType: PunishmentType) =>
- !selectedPunishmentType && defendant?.punishmentType === punishmentType
+const hasSetPunishmentType = (punishmentType: PunishmentType) =>
+ selectedPunishmentType === null && defendant?.punishmentType === punishmentTypeLikely invalid or redundant comment.
44-44:
Add error handling for state updates.
The selectedPunishmentType state should be updated only after the API call succeeds.
-const [selectedPunishmentType, setPunishmentType] = useState<PunishmentType>()
+const [selectedPunishmentType, setPunishmentType] = useState<PunishmentType | null>(null)Likely invalid or redundant comment.
apps/judicial-system/api/src/app/modules/backend/backend.service.ts (1)
358-367: LGTM! The implementation follows the established patterns.
The method correctly implements limited access functionality for updating defendant information, following the same patterns as other limited access methods in the service.
apps/judicial-system/web/src/components/FormProvider/case.graphql (1)
38-38: LGTM! The punishmentType field is correctly positioned.
The field is appropriately added to the defendants object, following the existing field patterns and GraphQL conventions.
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/defendant/guards/rolesRules.ts
Show resolved
Hide resolved
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (3)
1-26: Sort imports for better organizationThe static analysis tool suggests sorting imports for better code organization.
🧰 Tools
🪛 eslint
[error] 1-26: Run autofix to sort these imports!
(simple-import-sort/imports)
55-57: Enhance debug logging with more contextConsider including more details in the debug log, such as the update DTO contents and the user performing the action.
- this.logger.debug( - `Updating limitedAccess defendant ${defendantId} of case ${caseId}`, - ) + this.logger.debug( + `Updating limitedAccess defendant ${defendantId} of case ${caseId} by user ${user.id}`, + { updateDto, userId: user.id } + )
43-46: Enhance API documentationThe current API documentation could be more descriptive. Consider adding details about the expected request body, possible responses, and error scenarios.
@ApiOkResponse({ type: Defendant, - description: 'Updates a defendant', + description: 'Updates a defendant\'s punishment type. Only accessible by prison system staff.', }) + @ApiBody({ type: LimitedAccessUpdateDefendant }) + @ApiUnauthorizedResponse({ description: 'Unauthorized access' }) + @ApiForbiddenResponse({ description: 'Insufficient permissions' })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.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."
🪛 eslint
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts
[error] 1-26: Run autofix to sort these imports!
(simple-import-sort/imports)
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (2)
28-29:
Convert interface to class with validation decorators
To ensure proper validation of incoming requests and prevent potential security issues, consider converting the interface to a class with validation decorators.
-interface LimitedAccessUpdateDefendant
- extends Pick<UpdateDefendantDto, 'punishmentType'> {}
+import { IsOptional, IsEnum } from 'class-validator'
+import { PunishmentType } from './models/punishment-type.enum'
+
+export class LimitedAccessUpdateDefendant {
+ @IsOptional()
+ @IsEnum(PunishmentType)
+ punishmentType?: PunishmentType
+}31-42: Verify role-based access control implementation
The controller uses multiple guards and role rules. Let's verify the implementation of prisonSystemStaffUpdateRule to ensure proper access control.
✅ Verification successful
Let me gather more information about the role-based access control implementation.
Let me gather more information about the RolesGuard implementation and the specific rule.
Based on the gathered information, I can now provide a final response about the role-based access control implementation.
Role-based access control is properly implemented
The prisonSystemStaffUpdateRule is correctly defined and implemented with:
- Proper role restriction to
PRISON_SYSTEM_STAFF - Field-based access control using
RulesType.FIELD - Limited access to only the
punishmentTypefield throughlimitedAccessFields
The implementation is consistent with the system's role-based access control patterns, as evidenced by:
- Clear role definition in the
UserRoleenum - Proper use of the
RolesRulesdecorator - Field-level access control through
RulesType.FIELD - Multiple guards working together (
JwtAuthGuard,RolesGuard,CaseExistsGuard,DefendantExistsGuard)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of prisonSystemStaffUpdateRule
# Search for the rule definition and its usage
rg -A 5 "prisonSystemStaffUpdateRule"
# Search for related role-based access tests
ast-grep --pattern 'describe($_, $_) {
$$$
it($_, $_) {
$$$
prisonSystemStaffUpdateRule
$$$
}
$$$
}'
Length of output: 2177
Script:
#!/bin/bash
# Search for the definition of limitedAccessFields and RolesRule
rg -A 5 "limitedAccessFields"
# Search for the RolesRules decorator implementation
ast-grep --pattern 'export class RolesGuard {
$$$
}'
# Search for related test files
fd "roles.*\.spec\.ts" --exec cat {}
Length of output: 50959
Script:
#!/bin/bash
# Search for RolesGuard implementation
ast-grep --pattern 'export class RolesGuard {
$$$
canActivate($_) {
$$$
}
$$$
}'
# Search for RolesRule type definition
rg -A 5 "interface RolesRule"
# Search for RulesType enum
rg -A 5 "enum RulesType"
# Search for UserRole enum
rg -A 10 "enum UserRole"
Length of output: 3223
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (1)
42-67: Consider enhancing error handlingWhile the implementation is solid, consider adding explicit error handling for cases where:
- The update operation fails
- The punishment type is invalid for the case state
- Concurrent updates occur
Example enhancement:
async updateDefendant( @Param('caseId') caseId: string, @Param('defendantId') defendantId: string, @CurrentHttpUser() user: User, @CurrentCase() theCase: Case, @CurrentDefendant() defendant: Defendant, @Body() updateDto: LimitedAccessUpdateDefendant, ): Promise<Defendant> { this.logger.debug( `Updating limitedAccess defendant ${defendantId} of case ${caseId}`, ) + try { return this.defendantService.updateRequestCaseDefendant( theCase, defendant, updateDto, user, ) + } catch (error) { + this.logger.error( + `Failed to update defendant ${defendantId}: ${error.message}`, + ) + throw new HttpException( + 'Failed to update defendant', + HttpStatus.INTERNAL_SERVER_ERROR, + ) + } }apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (3)
Line range hint
1-26: Consider grouping related imports togetherFor better maintainability, consider organizing imports into these groups:
- React and react-related
- UI components
- Constants and types
- Local components
- Utils and hooks
import { useContext, useState } from 'react' import { useIntl } from 'react-intl' + import { Box, RadioButton, Text } from '@island.is/island-ui/core' +import { BlueBox } from '@island.is/judicial-system-web/src/components' + import * as constants from '@island.is/judicial-system/consts' -import { formatDate } from '@island.is/judicial-system/formatters' import { PunishmentType } from '@island.is/judicial-system/types' + +import { formatDate } from '@island.is/judicial-system/formatters' import { core } from '@island.is/judicial-system-web/messages' -import { - BlueBox, +import { FormContentContainer, FormContext, // ... other component imports } from '@island.is/judicial-system-web/src/components'
39-42: Simplify defendant extraction with optional chainingThe current implementation can be simplified using optional chaining and nullish coalescing.
- const { defendants } = workingCase - const defendant = - defendants && defendants?.length > 0 ? defendants[0] : undefined + const defendant = workingCase.defendants?.[0] ?? undefined
98-188: Refactor radio buttons to reduce duplicationConsider extracting the repeated radio button logic into a reusable component and moving the punishment types to a configuration object.
type PunishmentOption = { type: PunishmentType; id: string; labelKey: string; } const PUNISHMENT_OPTIONS: PunishmentOption[] = [ { type: PunishmentType.IMPRISONMENT, id: 'punishment-type-imprisonment', labelKey: 'imprisonment' }, // ... other options ] const PunishmentTypeRadio: React.FC<{ option: PunishmentOption; selected?: PunishmentType; onChange: (type: PunishmentType) => void; defaultValue?: PunishmentType; }> = ({ option, selected, onChange, defaultValue }) => ( <Box marginBottom={2}> <RadioButton id={option.id} name={option.id} checked={selected === option.type || (!selected && defaultValue === option.type)} onChange={() => onChange(option.type)} large backgroundColor="white" label={formatMessage(strings[option.labelKey])} /> </Box> )Then use it like:
{PUNISHMENT_OPTIONS.map(option => ( <PunishmentTypeRadio key={option.id} option={option} selected={selectedPunishmentType} onChange={onChange} defaultValue={defendant?.punishmentType} /> ))}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
apps/judicial-system/backend/src/app/modules/defendant/guards/rolesRules.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/judicial-system/backend/src/app/modules/defendant/guards/rolesRules.ts
- apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.strings.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.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/routes/Prison/IndictmentOverview/IndictmentOverview.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/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
🔇 Additional comments (4)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (3)
1-29: LGTM! Well-organized imports
The imports are logically grouped and all necessary dependencies are properly imported.
33-40: LGTM! Well-structured controller setup
The controller follows NestJS best practices with:
- Proper RESTful path structure
- Appropriate security guards
- Clean dependency injection
30-31:
Convert interface to class with validation decorators
The current interface approach doesn't provide runtime validation for the punishmentType field. Converting to a class would allow the use of validation decorators to ensure type safety and prevent unwanted properties.
-interface LimitedAccessUpdateDefendant
- extends Pick<UpdateDefendantDto, 'punishmentType'> {}
+import { IsEnum, IsOptional } from 'class-validator'
+import { PunishmentType } from './models/defendant.model'
+
+export class LimitedAccessUpdateDefendant {
+ @IsOptional()
+ @IsEnum(PunishmentType)
+ punishmentType?: PunishmentType
+}apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
44-52:
Add error handling and prevent race conditions
The current implementation updates the UI state before confirming the API call's success, which could lead to inconsistent state.
const onChange = (updatedPunishmentType: PunishmentType) => {
defendant &&
- limitedAccessUpdateDefendant({
- caseId: workingCase.id,
- defendantId: defendant.id,
- punishmentType: updatedPunishmentType,
- })
- setPunishmentType(updatedPunishmentType)
+ limitedAccessUpdateDefendant({
+ caseId: workingCase.id,
+ defendantId: defendant.id,
+ punishmentType: updatedPunishmentType,
+ })
+ .then(() => setPunishmentType(updatedPunishmentType))
+ .catch((error) => {
+ console.error('Failed to update punishment type:', error)
+ // Add user notification for error
+ })
}
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
101-184: Reduce code duplication in radio button implementationsThe radio button implementations contain significant duplication. Consider extracting the configuration and generating the radio buttons dynamically.
const PUNISHMENT_TYPES = [ { id: 'imprisonment', type: PunishmentType.IMPRISONMENT, messageKey: 'imprisonment' }, { id: 'probation', type: PunishmentType.PROBATION, messageKey: 'probation' }, // ... other types ] // In the render method: {PUNISHMENT_TYPES.map(({ id, type, messageKey }) => ( <Box key={id} marginBottom={2}> <RadioButton id={`punishment-type-${id}`} name={`punishmentType${id}`} checked={selectedPunishmentType === type || hasSetPunishmentType(type)} onChange={() => onChange(type)} large backgroundColor="white" label={formatMessage(strings[messageKey])} disabled={isUpdating} /> </Box> ))}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts(1 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.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 (2)
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (2)
42-50:
Add error handling and loading state to the onChange handler
The current implementation could lead to inconsistent UI state if the API call fails. Consider adding loading state and error handling.
+ const [isUpdating, setIsUpdating] = useState(false)
+
const onChange = (updatedPunishmentType: PunishmentType) => {
+ setIsUpdating(true)
defendant &&
limitedAccessUpdateDefendant({
caseId: workingCase.id,
defendantId: defendant.id,
punishmentType: updatedPunishmentType,
})
- setPunishmentType(updatedPunishmentType)
+ .then(() => {
+ setPunishmentType(updatedPunishmentType)
+ })
+ .catch((error) => {
+ console.error('Failed to update punishment type:', error)
+ // TODO: Add error notification
+ })
+ .finally(() => {
+ setIsUpdating(false)
+ })
}96-186: 🛠️ Refactor suggestion
Improve accessibility for radio button group
The radio button group should be wrapped in a fieldset with a descriptive legend for better screen reader support.
<Box marginBottom={10}>
- <Text variant="h4" as="h4" marginBottom={2}>
- {formatMessage(strings.punishmentTypeTitle)}
- </Text>
<BlueBox>
+ <fieldset>
+ <legend>
+ <Text variant="h4" as="h4">
+ {formatMessage(strings.punishmentTypeTitle)}
+ </Text>
+ </legend>
{/* Radio buttons */}
+ </fieldset>
</BlueBox>
</Box>
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx
Show resolved
Hide resolved
… of https://github.com/island-is/island.is into j-s/update-punishment-type-prison-indictment-overview
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts (2)
42-73: Refactor the promise chain to improve readability and avoid assignments in expressions.The test setup is well-structured, but the promise chain in
givenWhenThencan be improved.Consider this refactor to make the code more maintainable:
givenWhenThen = async ( defendantUpdate: UpdateDefendantDto, type: CaseType, courtCaseNumber?: string, ) => { - const then = {} as Then + const then = {} as Then; + try { + then.result = await limitedAccessDefendantController + .updateDefendant( + caseId, + defendantId, + user, + { id: caseId, courtCaseNumber, type } as Case, + defendant, + defendantUpdate, + ); + } catch (error) { + then.error = error; + } - await limitedAccessDefendantController - .updateDefendant( - caseId, - defendantId, - user, - { id: caseId, courtCaseNumber, type } as Case, - defendant, - defendantUpdate, - ) - .then((result) => (then.result = result)) - .catch((error) => (then.error = error)) return then; }🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 69-69: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
75-115: Consider adding more test cases for comprehensive coverage.While the current test cases cover the basic scenarios, consider adding tests for:
- Multiple punishment types to ensure all enum values work
- Edge cases like empty/null values
- Permission-based scenarios mentioned in PR objectives
Would you like me to help generate additional test cases?
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (1)
52-54: Consider memoizing the helper functionThe
hasSetPunishmentTypefunction could benefit from memoization since it's used multiple times in the render cycle.- const hasSetPunishmentType = (punishmentType: PunishmentType) => - !selectedPunishmentType && defendant?.punishmentType === punishmentType + const hasSetPunishmentType = useMemo( + () => (punishmentType: PunishmentType) => + !selectedPunishmentType && defendant?.punishmentType === punishmentType, + [selectedPunishmentType, defendant?.punishmentType] + )apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendantRolesRules.spec.ts (2)
6-7: Consider using a more specific type instead of any[]The eslint disable comment could be avoided by using a more specific type for the rules array, improving type safety.
- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let rules: any[] + let rules: Array<(user: User, theCase: Case) => boolean>
16-19: Consider adding negative test casesThe test suite only verifies the positive case. Consider adding tests to verify that other roles cannot access this endpoint.
it('should deny permission to other roles', () => { const mockUser = { roles: ['unauthorized_role'] } const mockCase = {} expect(rules[0](mockUser, mockCase)).toBeFalsy() })apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (1)
54-56: Enhance logging with additional contextThe debug log could include more context about the update operation, such as the user performing the update and the fields being modified.
- this.logger.debug( - `Updating limitedAccess defendant ${defendantId} of case ${caseId}`, - ) + this.logger.debug( + `User ${user.id} updating limitedAccess defendant ${defendantId} of case ${caseId} with fields: ${Object.keys(updateDto).join(', ')}`, + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.ts(4 hunks)apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendantRolesRules.spec.ts(1 hunks)apps/judicial-system/web/src/components/FormProvider/case.graphql(2 hunks)apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx(4 hunks)apps/judicial-system/web/src/utils/hooks/useDefendants/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/judicial-system/web/src/utils/hooks/useDefendants/index.ts
- apps/judicial-system/web/src/components/FormProvider/case.graphql
🧰 Additional context used
📓 Path-based instructions (5)
apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.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/defendant/test/limitedAccessDefendantController/updateDefendantRolesRules.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/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.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/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.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/backend/src/app/modules/defendant/limitedAccessDefendant.controller.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 (2)
apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendantRolesRules.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (2)
Learnt from: oddsson
PR: island-is/island.is#16731
File: apps/judicial-system/web/src/routes/Shared/IndictmentOverview/IndictmentOverview.tsx:172-186
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `IndictmentOverview.tsx`, since the defendants data does not change, using `useMemo` to memoize the filtering logic is unnecessary.
Learnt from: thorhildurt
PR: island-is/island.is#17198
File: apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx:42-50
Timestamp: 2024-12-11T14:25:44.741Z
Learning: In `IndictmentOverview.tsx`, when updating the punishment type, update the UI state before making the API call to immediately reflect the change.
🪛 Biome (1.9.4)
apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts
[error] 68-68: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 69-69: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (7)
apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts (1)
1-26: LGTM! Well-structured test setup with proper typing.
The imports are well-organized, and the type definitions follow good testing practices with proper TypeScript typing.
apps/judicial-system/web/src/routes/Prison/IndictmentOverview/IndictmentOverview.tsx (4)
1-1: LGTM! Import organization is clean and well-structured.
All imports are properly organized and utilized within the component.
Also applies to: 4-4, 7-7, 10-10, 21-24
31-36: LGTM! Hooks are properly integrated.
The limitedAccessUpdateDefendant mutation is correctly extracted from the useDefendants hook.
37-39: 🛠️ Refactor suggestion
Improve defendant selection robustness
The current implementation could be more defensive in handling defendant selection.
- const defendant =
- defendants && defendants?.length > 0 ? defendants[0] : undefined
+ const defendant = defendants?.length
+ ? defendants[0]
+ : null
+
+ if (!defendant) {
+ console.warn('No defendant found for case:', workingCase.id)
+ }96-186: 🛠️ Refactor suggestion
Refactor repetitive RadioButton components
The RadioButton components share similar structure and props. Consider extracting them into a reusable component.
+ interface PunishmentTypeRadioProps {
+ type: PunishmentType;
+ label: string;
+ }
+
+ const PunishmentTypeRadio: React.FC<PunishmentTypeRadioProps> = ({ type, label }) => (
+ <Box marginBottom={2}>
+ <RadioButton
+ id={`punishment-type-${type.toLowerCase()}`}
+ name={`punishmentType${type}`}
+ checked={selectedPunishmentType === type || hasSetPunishmentType(type)}
+ onChange={() => onChange(type)}
+ large
+ backgroundColor="white"
+ label={label}
+ />
+ </Box>
+ );
<BlueBox>
- {/* Current repetitive RadioButton components */}
+ {Object.values(PunishmentType).map((type) => (
+ <PunishmentTypeRadio
+ key={type}
+ type={type}
+ label={formatMessage(strings[type.toLowerCase()])}
+ />
+ ))}
</BlueBox>Improve accessibility for radio button group
The radio button group should be wrapped in a fieldset with a descriptive legend.
<Box marginBottom={10}>
- <Text variant="h4" as="h4" marginBottom={2}>
- {formatMessage(strings.punishmentTypeTitle)}
- </Text>
<BlueBox>
+ <fieldset>
+ <legend>
+ <Text variant="h4" as="span">
+ {formatMessage(strings.punishmentTypeTitle)}
+ </Text>
+ </legend>
{/* RadioButton components */}
+ </fieldset>
</BlueBox>
</Box>apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (1)
52-52:
Use class-based DTO with validation decorators
Using Pick with UpdateDefendantDto doesn't enforce runtime validation. Consider creating a dedicated DTO class with validation decorators.
export class LimitedAccessUpdateDefendantDto {
@IsOptional()
@IsEnum(PunishmentType)
punishmentType?: PunishmentType;
}apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.ts (1)
36-36: LGTM! Controller integration follows existing patterns
The integration of LimitedAccessDefendantController into the testing module is clean and consistent with the existing implementation.
Also applies to: 113-114, 138-138
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (1)
30-37: LGTM! Consider enhancing API documentation.The controller setup follows NestJS best practices with proper security guards and dependency injection. The limited access path aligns well with the PR objectives.
Consider adding
@ApiParamdecorators to document the path parameters:@ApiParam({ name: 'caseId', description: 'The ID of the case' }) @Controller('api/case/:caseId/limitedAccess/defendant')apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts (3)
16-19: Consider using discriminated union for Then interfaceThe current interface allows both
resultanderrorto be present simultaneously, which shouldn't be possible in practice. Consider using a discriminated union type for better type safety:type Then = | { success: true; result: Defendant; error?: never } | { success: false; error: Error; result?: never }
49-50: Remove unnecessary default error mockThe default error mock is immediately overridden in the test case. Consider removing it to avoid confusion.
- const mockUpdate = mockDefendantModel.update as jest.Mock - mockUpdate.mockRejectedValue(new Error('Some error'))
59-70: Refactor promise chain to use async/awaitThe current promise chain with assignments in expressions has been flagged by static analysis. Consider using async/await for better readability and to avoid assignments in expressions.
try { then.result = await limitedAccessDefendantController.updateDefendant( caseId, defendantId, user, { id: caseId, courtCaseNumber, type } as Case, defendant, defendantUpdate, ) } catch (error) { then.error = error }🧰 Tools
🪛 Biome (1.9.4)
[error] 68-68: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 69-69: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.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/defendant/test/limitedAccessDefendantController/updateDefendant.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/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts (2)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-11-12T15:15:20.157Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
🪛 Biome (1.9.4)
apps/judicial-system/backend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts
[error] 68-68: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 69-69: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (1)
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts (1)
57-62:
Add error handling for the service call.
The controller should handle potential errors from the service call and return appropriate HTTP responses.
- return this.defendantService.updateRequestCaseDefendant(
- theCase,
- defendant,
- updateDto,
- user,
- )
+ try {
+ return await this.defendantService.updateRequestCaseDefendant(
+ theCase,
+ defendant,
+ updateDto,
+ user,
+ )
+ } catch (error) {
+ this.logger.error('Failed to update defendant', error)
+ throw new InternalServerErrorException('Failed to update defendant')
+ }
apps/judicial-system/backend/src/app/modules/defendant/limitedAccessDefendant.controller.ts
Show resolved
Hide resolved
...kend/src/app/modules/defendant/test/limitedAccessDefendantController/updateDefendant.spec.ts
Show resolved
Hide resolved
References
Fmst - Stöður hjá fangelsismálastofnun, málalisti og yfirlit máls
What
punishment_typecase,subpoenaWhy
Screen
Screen.Recording.2024-12-11.at.11.00.05.mov
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores