fix(transport-authority): Handle all warnSever (E, L, W) + dummy mileage in vehicle validation #16662
Conversation
Stop using mileageRequired from answers in getSelectedVehicle and always look at the value from externalData (it should not change)
…leswithmileageandinsp (not basicVehicleInformation) Cleanup make+vehcom (basicVehicleInformation) vs make (currentvehicles)
Display always E, L and W messages (but E and L first)
WalkthroughThe changes in this pull request involve the integration of new modules and functionalities related to vehicle management, specifically focusing on mileage data and vehicle ownership processes. Key modifications include updates to service classes, the addition of new GraphQL queries and input types, and enhancements to error handling mechanisms. The introduction of mockable APIs allows for improved testing capabilities. Overall, these changes aim to streamline the vehicle operator and owner change processes while enhancing the robustness of the application. 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
|
johannaagma
changed the base branch from
main
to
fix/transport-authority-require-mileage
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
libs/clients/transport-authority/vehicle-owner-change/src/index.ts (1)
6-10: Consider grouping related exports together.For better code organization and tree-shaking, consider grouping the error-related exports with the types export.
Here's a suggested reorganization:
export * from './lib/vehicleOwnerChangeClient.service' export * from './lib/vehicleOwnerChangeClient.module' export * from './lib/vehicleOwnerChangeClient.types' +export { + ErrorMessage, + getCleanErrorMessagesFromTryCatch, +} from './lib/vehicleOwnerChangeClient.utils' export { VehicleOwnerChangeClientConfig } from './lib/vehicleOwnerChangeClient.config' - -export { - ErrorMessage, - getCleanErrorMessagesFromTryCatch, -} from './lib/vehicleOwnerChangeClient.utils'libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts (1)
15-15: Consider these improvements for better type safety and maintainability.
- Add stronger typing for the input parameter instead of
any- Consider using an enum for warning severity types
Example implementation:
enum WarnSeverity { Error = 'E', Lock = 'L', Warning = 'W', } export const getCleanErrorMessagesFromTryCatch = ( e: { body?: { Errors?: ReturnTypeMessage[] } | ReturnTypeMessage[] } ): ErrorMessage[] => { // ... rest of the implementation }Also applies to: 31-33
libs/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts (1)
63-64: Consider improving API design for error responsesThe comment indicates that the API returns 4xx status codes with error messages in the body, instead of a 200 status with structured error messages. This makes error handling more complex than necessary.
Consider updating the API to:
- Return 200 status with a structured response containing both success and error states
- Reserve 4xx status codes for actual HTTP-level errors
This would simplify client-side error handling and better align with REST principles.libs/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.ts (1)
Line range hint
51-63: Consider extracting constants and centralizing API versionFor improved maintainability, consider:
- Moving the dummy values to named constants
- Centralizing the API version string
+const API_VERSION = '1.0'; +const DEFAULT_DELIVERY_OPTIONS = { + deliveryStationType: SGS_DELIVERY_STATION_TYPE, + deliveryStationCode: SGS_DELIVERY_STATION_CODE, + expressOrder: false, +} as const; await this.plateOrderingApiWithAuth(auth).orderplatesPost({ - apiVersion: '1.0', - apiVersion2: '1.0', + apiVersion: API_VERSION, + apiVersion2: API_VERSION, postOrderPlatesModel: { permno: permno, frontType: frontType, rearType: rearType, - stationToDeliverTo: deliveryStationCode, - stationType: deliveryStationType, - expressOrder: expressOrder, + stationToDeliverTo: DEFAULT_DELIVERY_OPTIONS.deliveryStationCode, + stationType: DEFAULT_DELIVERY_OPTIONS.deliveryStationType, + expressOrder: DEFAULT_DELIVERY_OPTIONS.expressOrder, checkOnly: true, }, })libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (4)
68-68: Document the new 'mileage' parameterThe
mileageparameter added tovalidateAllForOperatorChangeenhances the function's capabilities. Please ensure this parameter is documented properly to inform future developers of its purpose and usage.
103-103: Simplify the 'hasError' conditionSince
errorMessagesis an array, you can simplify thehasErrorassignment for better readability:Apply this diff:
- hasError: !!errorMessages?.length, + hasError: errorMessages.length > 0,
11-11: Organize import statements for clarityConsider grouping external and internal imports separately to enhance code readability and maintainability.
97-99: Reuse error handling utilities across clientsThe logic for extracting clean error messages using
getCleanErrorMessagesFromTryCatchis valuable. If other clients or services require similar error handling, consider abstracting this into a shared utility to promote reusability.Would you like assistance in creating a shared utility function?
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts (3)
18-21: Consider refactoring to reduce code duplicationBoth
ownerchangeApiWithAuthandmileageReadingApiWithAuthmethods perform similar operations of addingAuthMiddlewareto an API client. Consider creating a generic method that accepts an API client as a parameter to apply the middleware, improving maintainability and reducing duplication.
27-30: Reduce duplication in API client methodsThe
mileageReadingApiWithAuthmethod duplicates the functionality ofownerchangeApiWithAuth. Refactoring these methods into a single, reusable function would enhance code clarity and maintainability.
62-62: Clarify the increment ofcurrentMileageby 1Incrementing
currentMileageby 1 may not be immediately clear to future maintainers. Consider adding a comment to explain the rationale behind this operation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.module.ts(1 hunks)libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts(3 hunks)libs/clients/transport-authority/vehicle-owner-change/src/index.ts(1 hunks)libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.module.ts(1 hunks)libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts(5 hunks)libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts(1 hunks)libs/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.ts(3 hunks)libs/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.module.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/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.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/clients/transport-authority/vehicle-owner-change/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."
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.module.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/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.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/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.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/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.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/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.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."
🪛 Biome
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts
[error] 6-6: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (15)
libs/clients/transport-authority/vehicle-owner-change/src/index.ts (1)
7-10: LGTM! The new exports align with PR objectives.
The addition of error handling utilities (ErrorMessage and getCleanErrorMessagesFromTryCatch) supports the PR's goal of improving SGS warning severity handling.
Let's verify the TypeScript types are properly exported and the utilities are reusable:
✅ Verification successful
Exports are properly typed and implementation is correct
The verification shows that:
ErrorMessageis properly defined as a TypeScript interface with expected properties (errorNoanddefaultMessage)getCleanErrorMessagesFromTryCatchis a well-implemented utility that handles different error scenarios from the transport authority API- While there are no direct imports of these utilities yet (as they are newly added), they follow the same pattern as other error handling implementations in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TypeScript types and utility usage
# Test 1: Check if ErrorMessage is a TypeScript type/interface
ast-grep --pattern 'export (type|interface) ErrorMessage'
# Test 2: Check for usage of these utilities across different apps
rg -l "getCleanErrorMessagesFromTryCatch" "apps/"
Length of output: 107
Script:
#!/bin/bash
# Let's try a different approach to verify the types and usage
# Check the implementation file for ErrorMessage type/interface
fd vehicleOwnerChangeClient.utils.ts -x cat {}
# Check for any imports of these utilities across the codebase
rg "import.*\{.*(?:ErrorMessage|getCleanErrorMessagesFromTryCatch).*\}" --type ts
Length of output: 16204
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.module.ts (2)
4-4: LGTM! Clean import statement.
The import follows TypeScript best practices and supports effective tree-shaking.
7-7: LGTM! Proper module integration.
The VehiclesMileageClientModule is correctly integrated while maintaining the existing module structure. This change aligns with the PR objective of supporting mileage data in vehicle validation.
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.module.ts (1)
4-4: LGTM! Clean integration of VehiclesMileageClientModule.
The addition of VehiclesMileageClientModule aligns with the PR objective of handling dummy mileage in vehicle validation. The implementation follows good practices for dependency injection and module composition.
Let's verify the consistency of this integration across related modules:
Also applies to: 7-7
✅ Verification successful
The shell script results show that VehiclesMileageClientModule is consistently used across related vehicle modules in the codebase, particularly in:
-
Transport authority modules:
- Vehicle owner change
- Vehicle operators
- Change co-owner of vehicle
- Change operator of vehicle
- Transfer of vehicle ownership
-
API domains:
- Vehicles module
- Transport authority module
Integration of VehiclesMileageClientModule is consistent and well-structured
The module is properly integrated across all vehicle-related functionalities, following a consistent pattern of importing and using it in the relevant modules. This confirms that the changes in the review file align with the broader codebase architecture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of VehiclesMileageClientModule across vehicle-related modules
# Expected: Similar imports in related vehicle modules
# Test 1: Find all vehicle-related module files
echo "=== Vehicle-related module files ==="
fd -e ts module
# Test 2: Check for VehiclesMileageClientModule usage
echo -e "\n=== VehiclesMileageClientModule usage ==="
rg -A 2 "VehiclesMileageClientModule"
Length of output: 48516
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts (2)
10-13: LGTM! Well-structured interface definition.
The ErrorMessage interface is clear, properly typed, and aligns well with the PR objectives for handling SGS system messages.
15-69: LGTM! Well-implemented error handling logic.
The function successfully implements the PR requirements:
- Prioritizes Error (E) and Lock (L) messages over Warnings (W)
- Handles special cases for lock warnings
- Properly processes both input and data validation errors
libs/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts (2)
9-12: LGTM: Good use of shared utilities
The imports demonstrate good code reuse by leveraging shared error handling utilities across different transport authority client modules.
50-50: LGTM: Improved error handling implementation
The changes standardize error handling using shared utilities and provide type safety with the ErrorMessage type. The comments clearly explain the rationale behind the try-catch usage.
Also applies to: 63-65, 69-70
libs/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.ts (2)
11-14: LGTM: Good use of shared utilities
The addition of error handling utilities from the vehicle-owner-change client promotes code reuse and consistency across the transport authority modules.
47-47: LGTM: Improved error handling implementation
The new error handling approach using getCleanErrorMessagesFromTryCatch aligns well with the PR objectives and provides a more consistent way to handle warning severity messages across the transport authority modules.
Also applies to: 70-72, 76-77
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (1)
60-61: Clarify the increment of mileage by one
Increments the current mileage by one before passing it to validateAllForOperatorChange. Could you provide clarification on why the mileage is incremented? This could lead to inaccurate mileage reporting if not intended.
Would you like assistance in verifying the correct mileage handling?
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts (4)
9-14: Necessary imports have been added correctly
The added imports for ErrorMessage, getCleanErrorMessagesFromTryCatch, getDateAtTimestamp, and MileageReadingApi are appropriate and enhance the functionality of the service.
72-72: Initialization of errorMessages is appropriate
The declaration of errorMessages with the type ErrorMessage[] | undefined correctly reflects its potential states.
115-117: Effective use of getCleanErrorMessagesFromTryCatch
Utilizing getCleanErrorMessagesFromTryCatch in the catch block streamlines error handling and ensures consistent error message formatting.
121-122: Return statement accurately represents error state
Returning hasError and errorMessages appropriately conveys the result of the validation process to the caller.
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts
Show resolved
Hide resolved
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts
Show resolved
Hide resolved
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts
Outdated
Show resolved
Hide resolved
...clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts
Outdated
Show resolved
Hide resolved
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts
Outdated
Show resolved
Hide resolved
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts
Show resolved
Hide resolved
.../clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.module.ts
Show resolved
Hide resolved
...clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (1)
66-71: Document the mileage increment workaround.The
currentMileage + 1is used as a workaround for electric vehicles to prevent Warning errors. This should be documented in the code comments for future maintenance.Apply this diff:
return await this.validateAllForOperatorChange( auth, permno, null, + // Adding 1 to current mileage to prevent Warning errors for electric vehicles + // when mileage hasn't been populated yet currentMileage + 1, )libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts (2)
41-54: Consider documenting the mileage increment workaround.The implementation adds 1 to the current mileage reading. While this addresses the warning error for electric vehicles mentioned in the PR objectives, it would be helpful to document why this increment is necessary.
Add a comment explaining the workaround:
// Get current mileage reading +// Note: Adding 1 to the current mileage to avoid warning errors for electric vehicles let currentMileage = 0
Line range hint
82-100: Consider consolidating workaround documentation.The code contains multiple workarounds (dummy insurance company code, timestamp handling) scattered across different comments. Consider consolidating these into a more structured documentation block at the start of the method.
Add a structured comment block:
+/** + * Known workarounds in this method: + * 1. Insurance Company: Uses dummy code (6090 - VÍS) when not supplied + * 2. Timestamp: Avoids 00:00:00 due to API limitations + * 3. Return Type: Endpoint modified to void due to OpenAPI generator issues + */ let errorMessages: ErrorMessage[] | undefined
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts(3 hunks)libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.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/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.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."
🔇 Additional comments (9)
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (6)
8-13: LGTM: Clean dependency injection and imports.
The new imports and constructor injection for MileageReadingApi are well-structured and follow dependency injection patterns.
Also applies to: 17-20
26-29: LGTM: Consistent authentication pattern.
The new mileageReadingApiWithAuth method follows the same authentication pattern as the existing operatorsApiWithAuth method.
51-64: LGTM: Proper error handling for mileage reading.
The implementation includes proper error logging and error message handling for the mileage reading API call.
107-109: LGTM: Improved error handling.
The use of getCleanErrorMessagesFromTryCatch aligns with the PR objective of consolidating error message processing logic.
80-80: Initialize errorMessages as mentioned in previous review.
The previous review comment about initializing errorMessages to an empty array is still valid and should be addressed.
113-114: LGTM: Clean error response structure.
The error response structure is clean and consistent, properly deriving the hasError flag from the error messages array.
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts (3)
9-14: LGTM: Clean dependency injection and imports.
The addition of MileageReadingApi and error handling utilities follows good practices and aligns with the PR objectives.
Also applies to: 19-22
28-30: LGTM: Clean auth middleware implementation.
The private method follows the same pattern as ownerchangeApiWithAuth, maintaining consistency.
125-127: LGTM: Improved error handling implementation.
The use of getCleanErrorMessagesFromTryCatch aligns with the PR objectives to handle warning severity messages (E, L, W) more effectively.
Also applies to: 131-132
Allow all users to retry submit application if all approvals are finished
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (19)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts (2)
Line range hint
33-39: Consider immutable state updates.The current implementation mutates the vehicle object directly when updating the mileage reading. Consider using immutable updates to prevent potential side effects.
- if (vehicle && !Object.isFrozen(vehicle)) { - currentVehicleList.vehicles[index] = { - ...vehicle, - mileageReading: mileageReading, - } - } - - return currentVehicleList?.vehicles?.[index] + if (!vehicle) { + return undefined + } + + return Object.isFrozen(vehicle) + ? vehicle + : { + ...vehicle, + mileageReading: mileageReading, + }
Line range hint
29-29: Improve type safety of parseInt.The
parseIntcall should include a radix parameter to ensure consistent behavior across different environments.- const index = parseInt(vehicleIndex, 10) + const index = Number.parseInt(vehicleIndex, 10)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts (2)
6-52: Consider refactoring for improved maintainability.The function correctly handles all approval scenarios, but could be improved for maintainability:
export const canReviewerApprove = ( reviewerNationalId: string, answers: FormValue, ): boolean => { - // Check if reviewer is buyer and has not approved - const buyer = getValueViaPath(answers, 'buyer') as UserInformation - if (buyer?.nationalId === reviewerNationalId && !buyer.approved) { - return true - } - - // Check if reviewer is buyer's co-owner or operator and has not approved - const buyerCoOwnersAndOperators = ( - getValueViaPath( - answers, - 'buyerCoOwnerAndOperator', - [], - ) as CoOwnerAndOperator[] - ).filter(({ wasRemoved }) => wasRemoved !== 'true') - - if ( - buyerCoOwnersAndOperators.some( - ({ nationalId, approved }) => - nationalId === reviewerNationalId && !approved, - ) - ) { - return true - } - - // Check if reviewer is seller's co-owner and has not approved - const sellerCoOwners = getValueViaPath( - answers, - 'sellerCoOwner', - [], - ) as CoOwnerAndOperator[] - - if ( - sellerCoOwners.some( - ({ nationalId, approved }) => - nationalId === reviewerNationalId && !approved, - ) - ) { - return true - } - - return false + type Approver = { nationalId: string; approved?: boolean } + + const buyer = getValueViaPath(answers, 'buyer') as UserInformation + const buyerCoOwnersAndOperators = ( + getValueViaPath(answers, 'buyerCoOwnerAndOperator', []) as CoOwnerAndOperator[] + ).filter(({ wasRemoved }) => wasRemoved !== 'true') + const sellerCoOwners = getValueViaPath(answers, 'sellerCoOwner', []) as CoOwnerAndOperator[] + + const allApprovers: Approver[] = [ + buyer, + ...buyerCoOwnersAndOperators, + ...sellerCoOwners, + ] + + return allApprovers.some( + ({ nationalId, approved }) => nationalId === reviewerNationalId && !approved + ) }This refactoring:
- Reduces code duplication
- Makes the approval logic more maintainable
- Introduces a type-safe
Approverinterface
54-95: Enhance type safety and simplify boolean logic.The function is well-documented and handles the special case appropriately. However, there are opportunities for improvement:
export const canReviewerReApprove = ( reviewerNationalId: string, answers: FormValue, ): boolean => { + type Reviewer = { nationalId: string } + const sellerNationalId = getValueViaPath( answers, 'seller.nationalId', '', ) as string const buyerNationalId = getValueViaPath( answers, 'buyer.nationalId', '', ) as string const buyerCoOwnersAndOperators = ( getValueViaPath( answers, 'buyerCoOwnerAndOperator', [], ) as CoOwnerAndOperator[] ).filter(({ wasRemoved }) => wasRemoved !== 'true') const sellerCoOwners = getValueViaPath( answers, 'sellerCoOwner', [], ) as CoOwnerAndOperator[] - const isReviewerAuthorized = [ - sellerNationalId === reviewerNationalId, - buyerNationalId === reviewerNationalId, - buyerCoOwnersAndOperators.some( - ({ nationalId }) => nationalId === reviewerNationalId, - ), - sellerCoOwners.some(({ nationalId }) => nationalId === reviewerNationalId), - ].some(Boolean) + const allReviewers: Reviewer[] = [ + { nationalId: sellerNationalId }, + { nationalId: buyerNationalId }, + ...buyerCoOwnersAndOperators, + ...sellerCoOwners, + ] + + const isReviewerAuthorized = allReviewers.some( + ({ nationalId }) => nationalId === reviewerNationalId + ) return isReviewerAuthorized && !hasPendingApproval(answers) }This refactoring:
- Introduces a type-safe
Reviewerinterface- Simplifies the authorization check by consolidating all reviewers into a single array
- Makes the code more maintainable and easier to extend
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts (2)
86-86: Consider adding type validation for mileage reading.The null safety improvement is good, but consider adding runtime type validation to ensure the mileage reading is always a valid number when present.
- return vehicle?.mileageReading || '' + const reading = vehicle?.mileageReading + return (reading && !isNaN(Number(reading))) ? reading : ''
106-108: Consider extracting the placeholder text to messages.The null safety handling is good, but consider moving the hardcoded Icelandic text "Síðasta skráning ${vehicle.mileageReading} Km" to the messages file for consistency with other labels.
- return vehicle?.mileageReading - ? `Síðasta skráning ${vehicle.mileageReading} Km` - : '' + return vehicle?.mileageReading + ? information.labels.vehicle.lastMileageReading.replace( + '{value}', + vehicle.mileageReading + ) + : ''libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx (1)
Line range hint
85-109: Consider refactoring error message rendering logic.The error message rendering logic could be improved in several ways:
- Extract the message resolution logic into a separate function
- Use template literals instead of string concatenation for the fallback message
- Consider splitting the validation query logic into a custom hook
Example refactor:
const resolveErrorMessage = (error: OwnerChangeValidationMessage) => { const message = formatMessage( getValueViaPath(applicationCheck.validation, error?.errorNo || '') ); const defaultMessage = error.defaultMessage; const fallbackMessage = `${formatMessage( applicationCheck.validation.fallbackErrorMessage )} - ${error?.errorNo}`; return message || defaultMessage || fallbackMessage; }; // Usage in JSX <BulletList> {data.vehicleOwnerChangeValidation.errorMessages.map((error) => ( <Bullet key={error.errorNo}> {resolveErrorMessage(error)} </Bullet> ))} </BulletList>🧰 Tools
🪛 Biome
[error] 103-103: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts (2)
28-54: Consider enhancing error logging.While the error handling is present, the error logging could be more detailed to help with debugging.
Consider adding more context to the error log:
- logger.error(e) + logger.error('Failed to get mileage reading', { + error: e, + permno, + nationalId: auth.nationalId + })
72-72: Document the mileage increment logic.The increment of mileage by 1 seems arbitrary. Please add a comment explaining why this increment is necessary.
- mileage: currentMileage + 1, + // Increment mileage by 1 to avoid warning errors for electric vehicles + // when the mileage field hasn't been populated yet + mileage: currentMileage + 1,libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx (5)
82-82: Consider adding type annotation for buttonLoading state.While the change improves state management clarity, consider adding an explicit type annotation:
- const [buttonLoading, setButtonLoading] = useState(false) + const [buttonLoading, setButtonLoading] = useState<boolean>(false)
171-177: Consider implications of no-cache fetch policy.While using
fetchPolicy: 'no-cache'ensures fresh data, it might impact performance by bypassing Apollo's cache mechanism. Consider ifnetwork-onlywould be more appropriate for your use case, as it updates the cache while still fetching fresh data.Also applies to: 248-248
Line range hint
306-330: Add key prop to mapped Bullet components.React requires a unique key prop for elements in an array to optimize rendering and maintain component state correctly.
- <Bullet> + <Bullet key={error.errorNo || error.defaultMessage}> {message || defaultMessage || fallbackMessage} </Bullet>🧰 Tools
🪛 Biome
[error] 324-324: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
149-154: Improve optional chaining usage.Consider using optional chaining for more concise code:
- setStep && setStep('conclusion') + setStep?.('conclusion')🧰 Tools
🪛 Biome
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 154-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
366-377: Document re-approve functionality.Consider adding a comment explaining when and why a reviewer might need to re-approve. This would help future maintainers understand the business logic behind this feature.
+ // Re-approve button is shown when a reviewer needs to retry their approval + // after fixing validation issues or when their previous approval was invalidated {canReviewerReApprove(reviewerNationalId, application.answers) && application.state !== States.COMPLETED && (libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/TransferOfVehicleOwnershipTemplate.ts (2)
Line range hint
66-76: Consider enhancing function documentation and naming.While the logic change correctly implements the new approval check, the function could benefit from:
- JSDoc documentation explaining the different message states
- More descriptive parameter names (e.g.,
reviewerNationalIdinstead ofnationalId)- Type annotations for the parameters
+ /** + * Determines the pending action message based on reviewer's approval status + * @param application - The current application + * @param role - The role of the user + * @param reviewerNationalId - The national ID of the reviewer + * @returns PendingAction with appropriate message and status + */ - const reviewStatePendingAction = ( - application: Application, - role: string, - nationalId: string, + const reviewStatePendingAction = ( + application: Application, + role: string, + reviewerNationalId: string,
Line range hint
89-89: Enhance type definitions for better type safety.The template configuration uses TypeScript effectively, but consider:
- Adding a specific type for the template configuration object instead of using inline types
- Creating dedicated interfaces for the state metadata configurations
- Using more specific types for form loader return values
interface TransferOfVehicleOwnershipState { name: string; status: 'draft' | 'inprogress' | 'rejected' | 'completed'; // ... other state metadata properties } interface TransferOfVehicleOwnershipTemplate extends ApplicationTemplate< ApplicationContext, ApplicationStateSchema<Events>, Events > { // ... template specific properties }Also applies to: 90-91, 92-94
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (1)
589-589: Consider adding validation for negative mileage values.The mileage conversion should include validation to prevent negative values, which could cause issues in the vehicle registration system.
- mileage: mileage ? Number(mileage) || 0 : null, + mileage: mileage ? Math.max(Number(mileage) || 0, 0) : null,libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts (2)
82-87: Ensure accurate detection of added co-owners/operatorsThe condition
newBuyerCoOwnerAndOperator.length > oldBuyerCoOwnersAndOperators.lengthassumes that any increase in the array length indicates new co-owners/operators have been added. However, this may not account for cases where co-owners/operators have been both added and removed, resulting in the same length but different members.Consider comparing the arrays' contents to accurately detect any additions or removals. This can be achieved using a utility function to find differences between the arrays.
Apply this diff as a suggestion:
-if ( - newBuyerCoOwnerAndOperator.length > oldBuyerCoOwnersAndOperators.length -) { - return false +const coOwnersChanged = !isEqual( + newBuyerCoOwnerAndOperator, + oldBuyerCoOwnersAndOperators +) +if (coOwnersChanged) { + return false }
89-96: Simplify and clarify new reviewer detection logicThe logic to detect new reviewers can be streamlined for better readability. Using a set or map to track
nationalIds can make the comparison more efficient and the code more maintainable.Consider refactoring this section:
-const newReviewerAdded = newBuyerCoOwnerAndOperator.some( - ({ nationalId }) => - !oldBuyerCoOwnersAndOperators.some( - (oldReviewer) => oldReviewer.nationalId === nationalId, - ), -) -return !newReviewerAdded +const oldNationalIds = new Set(oldBuyerCoOwnersAndOperators.map(({ nationalId }) => nationalId)) +const newNationalIds = new Set(newBuyerCoOwnerAndOperator.map(({ nationalId }) => nationalId)) + +const reviewersAdded = [...newNationalIds].some((id) => !oldNationalIds.has(id)) +return !reviewersAdded
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ApplicationStatus/index.tsx(2 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx(12 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/BuyerSection.tsx(2 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/InsuranceSection.tsx(2 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx(3 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts(5 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/TransferOfVehicleOwnershipTemplate.ts(2 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/messages/review.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/hasReviewerApproved.ts(0 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/index.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts(1 hunks)libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts(7 hunks)
💤 Files with no reviewable changes (1)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/hasReviewerApproved.ts
🧰 Additional context used
📓 Path-based instructions (14)
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ApplicationStatus/index.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/BuyerSection.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/InsuranceSection.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/TransferOfVehicleOwnershipTemplate.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/messages/review.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/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."
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.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/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.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."
🪛 Biome
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 154-154: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 324-324: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx
[error] 103-103: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (27)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/getSelectedVehicle.ts (1)
8-8: LGTM: Return type change improves type safety.
The addition of undefined to the return type makes the function more type-safe by explicitly indicating that it may not return a vehicle, forcing consumers to handle this case properly.
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/index.ts (3)
16-16: LGTM! Clean export statement following consistent patterns.
The new exports maintain the established barrel pattern and follow clear naming conventions.
Line range hint 1-35: Excellent compliance with coding guidelines.
The module demonstrates:
- ✓ Reusable utility functions that can be shared across NextJS apps
- ✓ Proper TypeScript usage with type imports
- ✓ Effective tree-shaking support through specific exports
16-16: Verify the new approval functions implementation.
Let's verify the implementation of the new approval functions to ensure they handle all approval scenarios correctly.
✅ Verification successful
Let me gather more information about the implementation details of these approval functions.
Approval functions implementation is well-structured and handles all necessary scenarios
The implementation of both approval functions is thorough and covers all required approval scenarios:
-
canReviewerApprove:- Checks if the reviewer is the buyer who hasn't approved yet
- Verifies if the reviewer is a buyer's co-owner/operator pending approval
- Validates if the reviewer is a seller's co-owner pending approval
-
canReviewerReApprove:- Handles the special case of completing ownership change
- Verifies reviewer authorization (seller, buyer, or their co-owners)
- Ensures all required approvals are completed before allowing re-approval
The functions are properly used across the codebase in relevant components for approval flows and UI state management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of new approval functions
# Expected: Find the implementation and usage of these functions
# Find the implementation
echo "=== Implementation of approval functions ==="
ast-grep --pattern 'export function can$_($$_) {
$$$
}'
# Find usage in components
echo "=== Usage in components ==="
rg -A 3 "canReviewerApprove|canReviewerReApprove"
Length of output: 9345
Script:
#!/bin/bash
# Get the actual implementation of the approval functions
cat libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts
Length of output: 2932
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/BuyerSection.tsx (2)
33-33: LGTM: Logic update maintains correct validation flow.
The replacement of hasReviewerApproved with canReviewerApprove in the conditional check is appropriate and maintains the intended validation flow for displaying the add co-owner/operator button.
Line range hint 14-17: LGTM: Component structure follows best practices.
The component demonstrates good practices:
- Proper TypeScript typing for props
- Clear separation of concerns
- Modular and reusable structure
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/InsuranceSection.tsx (2)
12-12: LGTM! Function rename reflects improved semantic meaning.
The change from hasReviewerApproved to canReviewerApprove better represents the intent of checking approval eligibility rather than approval status.
Also applies to: 55-55
Line range hint 1-85: Component structure follows library code guidelines.
The component implementation adheres to the required guidelines for library code:
- Properly exports TypeScript types for props
- Uses modular imports enabling effective tree-shaking
- Component is reusable across different NextJS applications
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ApplicationStatus/index.tsx (2)
30-33: Verify the approval workflow behavior change.
The change in review button visibility logic (canReviewerApprove) might affect the approval workflow. Please ensure this aligns with the intended behavior for handling warning severity messages (E, L, W).
Consider adding a comment explaining the conditions under which canReviewerApprove returns true, as this is a critical part of the approval workflow.
Line range hint 1-93: Well-structured TypeScript implementation!
The component demonstrates good practices:
- Proper TypeScript usage with well-defined props and types
- Modular structure enabling reuse across different NextJS apps
- Clear separation of concerns between display logic and business logic
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts (1)
1-4: LGTM! Well-structured imports with proper type definitions.
The code follows TypeScript best practices by importing types from shared libraries and utilizing local type definitions.
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts (3)
33-33: LGTM: Null safety improvement for vehicle plate number.
The optional chaining operator improves type safety when accessing the vehicle's permit number.
47-47: LGTM: Consistent null safety handling for vehicle make.
The change maintains consistency with other null safety improvements in the file.
76-76: Verify the impact of defaulting requireMileage to false.
While the null safety improvement is good, please verify that defaulting to false aligns with the business requirements for mileage validation, especially for electric vehicles mentioned in the PR objectives.
✅ Verification successful
Let me check the actual usage of requireMileage in the relevant files to understand its handling.
Let me check the validation logic to understand how requireMileage affects the form behavior.
Defaulting requireMileage to false is safe and consistent with the codebase
The default value of false for requireMileage is appropriate because:
- The field is used in form validation logic where
requireMileage: trueenforces mileage value validation - When
requireMileageis false or undefined, the validation is skipped, making it a safe default - The value is properly propagated from the vehicle data through form fields (VehicleSelectField, VehicleRadioField)
- The schema validation (dataSchema.ts) correctly handles both true/false cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of requireMileage to ensure consistent handling
rg -l 'requireMileage' --type ts
Length of output: 3110
Script:
#!/bin/bash
# Check how requireMileage is used in the relevant template files
rg 'requireMileage' -B 2 -A 2 libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
Length of output: 8782
Script:
#!/bin/bash
# Check validation logic related to mileage requirement
rg -B 5 -A 5 'if.*requireMileage' libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
Length of output: 1562
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx (2)
5-11: LGTM! Import changes align with shared component guidelines.
The addition of Bullet and BulletList components from the shared UI library follows best practices for component reusability across NextJS apps.
Line range hint 85-109: Verify handling of different warning severity levels.
The PR objectives mention handling different warning severity levels (E, L, W), but the current implementation doesn't seem to differentiate between these levels when displaying messages.
Let's verify the error message structure:
🧰 Tools
🪛 Biome
[error] 103-103: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/messages/review.ts (2)
160-160: LGTM: String formatting standardization.
The change standardizes the string formatting by using single quotes, maintaining consistency with other messages in the file.
163-167: LGTM: Well-structured tryAgain button messages.
The new tryAgain button messages follow the established pattern and best practices:
- Unique ID following the namespace convention
- Clear default message in Icelandic
- Helpful description for translators
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.service.ts (3)
9-14: LGTM: Clean dependency injection and import organization.
The new imports and constructor injection for MileageReadingApi are well-structured and follow Angular's dependency injection pattern.
Also applies to: 19-22
125-127: LGTM: Well-documented error handling approach.
The error handling implementation using getCleanErrorMessagesFromTryCatch is clean and well-documented. The comments clearly explain the reasoning behind the 4xx error handling approach.
Also applies to: 131-132
163-163: LGTM: Consistent error handling implementation.
The saveOwnerChange method changes maintain consistency with the rest of the service:
- Return type properly reflects error handling
- Error handling follows the established pattern
- Request payload correctly includes mileage
Also applies to: 173-209
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx (1)
10-11: LGTM: Import changes align with new functionality.
The addition of UI components for structured error display and the updated approval utilities improve the code organization.
Also applies to: 32-33, 46-46
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/TransferOfVehicleOwnershipTemplate.ts (1)
37-37: LGTM: Import statement updated correctly.
The replacement of hasReviewerApproved with canReviewerApprove aligns with the PR's objective to refactor the approval logic.
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (2)
574-604: LGTM: Comprehensive data mapping with proper null handling.
The implementation correctly handles:
- Proper mapping of all required fields from answers
- Null safety for mileage using conditional operator
- Clear separation of concerns between different entity types (seller, buyer, co-owners, operators)
574-604: Verify dummy mileage handling in validation endpoints.
The PR objectives mention adding dummy mileage for vehicle validation, but we should verify this is consistently applied across all validation endpoint calls.
✅ Verification successful
Dummy mileage handling is properly implemented in validation endpoints
The implementation correctly handles dummy mileage in validation endpoints:
- In
validateVehicleForOwnerChange, it fetches current mileage and adds 1 to it for validation - The mileage field is properly typed as nullable and has validation constraints (min: 0, max: 9999999)
- The mileage handling is consistent across validation and submission endpoints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all validation endpoint calls to ensure consistent mileage handling
ast-grep --pattern 'validateVehicleForOwnerChange($$$)'
ast-grep --pattern 'validateAllForOwnerChange($$$)'
Length of output: 111
Script:
#!/bin/bash
# Search for validation-related method calls and implementations
rg -A 5 "validate.*ForOwnerChange|validateVehicle" --type ts
# Search for mileage-related validation logic
rg -A 3 "mileage.*validate|validate.*mileage" --type ts
# Search for client method definitions related to vehicle validation
ast-grep --pattern 'class $_ {
$$$
validate$_($$$) {
$$$
}
$$$
}'
Length of output: 15492
Script:
#!/bin/bash
# Let's check the implementation of validateVehicleForOwnerChange to see mileage handling
rg -B 5 -A 10 "validateVehicleForOwnerChange.*Promise" --type ts
# Also check for any mileage-related code in the owner change client
rg -A 5 "mileage" libs/clients/transport-authority/vehicle-owner-change/
Length of output: 13449
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts (2)
5-54: Code adheres to TypeScript best practices and is reusable
The hasPendingApproval function is well-defined with proper TypeScript types and can be reused across different applications. It improves code maintainability by consolidating the approval checks into a single utility function.
56-99: Function isLastReviewer correctly determines the last reviewer
The logic in the isLastReviewer function is appropriate, checking if the current reviewer is the last one needed for approval based on pending approvals and any changes in the co-owners/operators list.
...nsport-authority/transfer-of-vehicle-ownership/src/fields/Overview/sections/BuyerSection.tsx
Show resolved
Hide resolved
...tes/transport-authority/transfer-of-vehicle-ownership/src/fields/ApplicationStatus/index.tsx
Show resolved
Hide resolved
...ansport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx
Outdated
Show resolved
Hide resolved
...s/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts
Show resolved
Hide resolved
...tion/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts
Show resolved
Hide resolved
Display ValidationErrorMessages always in overview, no matter who is reviewing
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (49)
libs/application/templates/transport-authority/license-plate-renewal/src/utils/getSelectedVehicle.ts (1)
Line range hint
9-13: Add input validation to prevent runtime errors.The current implementation could throw runtime errors or return unexpected results in edge cases. Consider adding validation for array index and parsing errors.
Here's a suggested implementation with better error handling:
export const getSelectedVehicle = ( externalData: ExternalData, answers: FormValue, ): PlateOwnership | undefined => { const currentVehicleList = (externalData?.['myPlateOwnershipList']?.data as PlateOwnership[]) || [] const vehicleValue = getValueViaPath(answers, 'pickPlate.value', '') as string - return currentVehicleList[parseInt(vehicleValue, 10)] + const index = parseInt(vehicleValue, 10) + if (isNaN(index) || index < 0 || index >= currentVehicleList.length) { + return undefined + } + return currentVehicleList[index] }libs/application/templates/transport-authority/order-vehicle-license-plate/src/utils/getSelectedVehicle.ts (1)
Line range hint
10-15: Consider improving type safety of vehicle retrieval.The type assertion
as VehiclesCurrentVehiclecould be unsafe if the data doesn't match the expected shape.Consider this safer approach:
- const vehicle = getValueViaPath( - answers, - 'pickVehicle', - ) as VehiclesCurrentVehicle + const vehicle = getValueViaPath<VehiclesCurrentVehicle>( + answers, + 'pickVehicle' + )libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/utils/getSelectedVehicle.ts (1)
Line range hint
1-27: Consider adding error handling for debuggingThe function silently handles invalid states (missing data, invalid indices) which could make debugging difficult. Consider adding error logging or throwing specific errors in development.
Example enhancement:
interface GetSelectedVehicleError extends Error { code: 'INVALID_INDEX' | 'MISSING_VEHICLE_LIST' | 'INVALID_VEHICLE_DATA'; context: Record<string, unknown>; } export const getSelectedVehicle = ( externalData: ExternalData, answers: FormValue, ): VehiclesCurrentVehicle | undefined => { try { if (answers.findVehicle) { const vehicle = getValueViaPath( answers, 'pickVehicle', ) as VehiclesCurrentVehicle if (!vehicle?.permno) { // or other required field throw new Error('Invalid vehicle data structure') as GetSelectedVehicleError; } return vehicle; } const currentVehicleList = getValueViaPath( externalData, 'currentVehicleList.data', ) as CurrentVehiclesAndRecords; if (!currentVehicleList?.vehicles) { throw new Error('Missing vehicle list') as GetSelectedVehicleError; } const vehicleIndex = getValueViaPath( answers, 'pickVehicle.vehicle', '', ) as string; const index = parseInt(vehicleIndex, 10); if (isNaN(index) || index < 0 || index >= currentVehicleList.vehicles.length) { throw new Error('Invalid vehicle index') as GetSelectedVehicleError; } return currentVehicleList.vehicles[index]; } catch (error) { // In development, throw the error if (process.env.NODE_ENV === 'development') { throw error; } // In production, log and return undefined console.error('Failed to get selected vehicle:', error); return undefined; } }libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts (1)
Line range hint
1-47: LGTM! The implementation follows best practicesThe function:
- Uses proper null checks and optional chaining
- Follows TypeScript best practices
- Is reusable across different NextJS apps
- Exports types correctly
- Is tree-shakeable
Consider adding JSDoc comments to document:
- The function's purpose
- Parameter descriptions
- Return value scenarios
- Example usage
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts (1)
Line range hint
1-43: Consider refactoring for improved maintainability and type safetyThe function could benefit from several improvements:
- Split into smaller, focused functions
- Replace type assertions with type guards
- Avoid direct mutation of external state
Here's a suggested refactor:
import { getValueViaPath } from '@island.is/application/core' import { ExternalData, FormValue } from '@island.is/application/types' import { CurrentVehiclesAndRecords, VehiclesCurrentVehicle } from '../shared' +const getVehicleFromAnswers = ( + answers: FormValue, +): VehiclesCurrentVehicle | undefined => { + if (!answers.findVehicle) return undefined + const vehicle = getValueViaPath(answers, 'pickVehicle') + return isVehicle(vehicle) ? vehicle : undefined +} + +const isVehicle = (value: unknown): value is VehiclesCurrentVehicle => { + return value !== null && typeof value === 'object' + // Add more specific type checks based on VehiclesCurrentVehicle structure +} + +const updateVehicleMileage = ( + vehicle: VehiclesCurrentVehicle, + mileageReading: string, +): VehiclesCurrentVehicle => ({ + ...vehicle, + mileageReading, +}) + export const getSelectedVehicle = ( externalData: ExternalData, answers: FormValue, ): VehiclesCurrentVehicle | undefined => { - if (answers.findVehicle) { - const vehicle = getValueViaPath( - answers, - 'pickVehicle', - ) as VehiclesCurrentVehicle - return vehicle - } + const vehicleFromAnswers = getVehicleFromAnswers(answers) + if (vehicleFromAnswers) return vehicleFromAnswers const currentVehicleList = - (externalData?.currentVehicleList?.data as CurrentVehiclesAndRecords) ?? - undefined + externalData?.currentVehicleList?.data as CurrentVehiclesAndRecords | undefined const vehicleIndex = getValueViaPath( answers, 'pickVehicle.vehicle', - '', - ) as string + '' + ) const mileageReading = getValueViaPath( answers, 'vehicleMileage.mileageReading', - '', - ) as string + '' + ) - const index = parseInt(vehicleIndex, 10) + if (typeof vehicleIndex !== 'string') return undefined + const index = parseInt(vehicleIndex, 10) const vehicle = currentVehicleList?.vehicles?.[index] - if (vehicle && !Object.isFrozen(vehicle)) { - currentVehicleList.vehicles[index] = { - ...vehicle, - mileageReading: mileageReading, - } - } + if (!vehicle || Object.isFrozen(vehicle)) return vehicle - return currentVehicleList?.vehicles?.[index] + return updateVehicleMileage(vehicle, mileageReading) }libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts (2)
5-39: Consider refactoring to reduce code duplication and improve type safety.The function has similar logic repeated for co-owners and operators. Consider extracting the common approval check logic into a reusable helper function.
Here's a suggested refactor:
+type Approver = { + nationalId: string; + approved: boolean; +}; + +const hasUnapprovedMembers = ( + members: Approver[], + excludeNationalId?: string, +): boolean => + members.some( + ({ nationalId, approved }) => + (!excludeNationalId || nationalId !== excludeNationalId) && !approved, + ); + export const hasPendingApproval = ( answers: FormValue, excludeNationalId?: string, ): boolean => { const coOwners = getValueViaPath( answers, 'ownerCoOwner', [], ) as UserInformation[] - if ( - coOwners.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { - return true - } + if (hasUnapprovedMembers(coOwners, excludeNationalId)) return true; const newOperators = ( getValueViaPath(answers, 'operators', []) as OperatorInformation[] ).filter(({ wasRemoved }) => wasRemoved !== 'true') - if ( - newOperators.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { - return true - } + if (hasUnapprovedMembers(newOperators, excludeNationalId)) return true; return false }
41-51: LGTM with a minor type safety suggestion!The function is well-documented and has a clear, focused responsibility. Consider adding a type guard for the
answersparameter to ensure type safety at runtime.+type ReviewAnswers = { + ownerCoOwner?: UserInformation[]; + operators?: OperatorInformation[]; +}; + export const isLastReviewer = ( reviewerNationalId: string, - answers: FormValue, + answers: FormValue & ReviewAnswers, ): boolean => {libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/externalDataSection.ts (1)
45-48: Improve mock provider implementationThe mock provider implementation could be enhanced in the following ways:
- Add a descriptive title to maintain consistency with other providers
- Include a subTitle for better UI/UX
- Add documentation explaining the purpose of this mock provider, especially its relation to dummy mileage handling mentioned in the PR objectives
buildDataProviderItem({ provider: MockableSamgongustofaPaymentCatalogApi, - title: '', + title: externalData.payment.mockTitle, + subTitle: externalData.payment.mockSubTitle, }),libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts (2)
5-39: LGTM with suggestions for improved reusability.The function effectively checks for pending approvals while safely handling property access. However, consider these improvements:
- Make the function more reusable by accepting custom paths:
export const hasPendingApproval = ( answers: FormValue, excludeNationalId?: string, + ownerCoOwnersPath: string = 'ownerCoOwners', + coOwnersPath: string = 'coOwners', ): boolean => { const oldCoOwners = getValueViaPath( answers, - 'ownerCoOwners', + ownerCoOwnersPath, [], ) as OwnerCoOwnersInformation[] // ... const newCoOwners = ( getValueViaPath( answers, - 'coOwners', + coOwnersPath, [] ) as CoOwnersInformation[] )
- Consider using boolean comparison instead of string:
- ).filter(({ wasRemoved }) => wasRemoved !== 'true') + ).filter(({ wasRemoved }) => wasRemoved !== true)
41-51: Consider a more descriptive function name.The function is well-implemented and documented. However, the name
isLastReviewermight be slightly misleading as it actually checks if the reviewer is the only one with a pending approval.Consider renaming to better reflect its behavior:
-export const isLastReviewer = ( +export const isOnlyPendingReviewer = ( reviewerNationalId: string, answers: FormValue, ): boolean => {libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts (1)
6-40: Consider enhancing type safety and readabilityThe function logic is sound, but could benefit from some improvements:
-// Function to check if the reviewer is authorized to approve and hasn't done that yet +/** + * Determines if a reviewer can approve based on their national ID. + * @param reviewerNationalId - The national ID of the reviewer + * @param answers - The form answers containing approval state + * @returns true if the reviewer can approve, false otherwise + */ export const canReviewerApprove = ( reviewerNationalId: string, answers: FormValue, ): boolean => { const coOwners = getValueViaPath( answers, 'ownerCoOwner', [], ) as UserInformation[] + // Early return if co-owner can approve + if (coOwners.some( + ({ nationalId, approved }) => + nationalId === reviewerNationalId && !approved, + )) { + return true + } const newOperators = ( getValueViaPath(answers, 'operators', []) as OperatorInformation[] - ).filter(({ wasRemoved }) => wasRemoved !== 'true') + ).filter(({ wasRemoved }) => wasRemoved !== 'true' && wasRemoved !== true) - if ( - newOperators.some( - ({ nationalId, approved }) => - nationalId === reviewerNationalId && !approved, - ) - ) { - return true - } - - return false + return newOperators.some( + ({ nationalId, approved }) => + nationalId === reviewerNationalId && !approved, + ) }libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts (2)
7-40: Consider improving reusability and type safety.While the function works correctly, there are opportunities for improvement:
- Extract hardcoded paths as parameters to make the function more reusable across different templates.
- Replace type assertions with type guards for better type safety.
- Reduce code duplication in co-owner checking logic.
Consider this refactoring:
+ type CoOwnerPaths = { + oldCoOwnersPath: string + newCoOwnersPath: string + } + + const isCoOwnerInformation = (data: unknown): data is CoOwnersInformation[] => { + return Array.isArray(data) && data.every(item => + 'nationalId' in item && 'approved' in item + ) + } + export const canReviewerApprove = ( reviewerNationalId: string, answers: FormValue, + paths: CoOwnerPaths = { + oldCoOwnersPath: 'ownerCoOwners', + newCoOwnersPath: 'coOwners' + } ): boolean => { + const checkCoOwnerApproval = ( + coOwners: CoOwnersInformation[], + nationalId: string + ) => coOwners.some( + ({ nationalId: ownerId, approved }) => + ownerId === nationalId && !approved + ) + const oldCoOwners = getValueViaPath( answers, - 'ownerCoOwners', + paths.oldCoOwnersPath, [], - ) as OwnerCoOwnersInformation[] + ) + if (!isCoOwnerInformation(oldCoOwners)) return false if ( - oldCoOwners.some( - ({ nationalId, approved }) => - nationalId === reviewerNationalId && !approved, - ) + checkCoOwnerApproval(oldCoOwners, reviewerNationalId) ) { return true } const newCoOwners = ( getValueViaPath(answers, - 'coOwners', + paths.newCoOwnersPath, []) - ) as CoOwnersInformation[] + ) + if (!isCoOwnerInformation(newCoOwners)) return false const activeNewCoOwners = newCoOwners.filter( ({ wasRemoved }) => wasRemoved !== 'true' ) - if ( - activeNewCoOwners.some( - ({ nationalId, approved }) => - nationalId === reviewerNationalId && !approved, - ) - ) { - return true - } - return false + return checkCoOwnerApproval(activeNewCoOwners, reviewerNationalId) }
42-65: Well-documented special case handler, but could be improved.The function's purpose is well-documented, explaining the special case handling. However, there are opportunities for improvement:
- Similar reusability improvements as suggested for
canReviewerApprove.- The Boolean array with
.some()could be simplified.Consider this simplification:
export const canReviewerReApprove = ( reviewerNationalId: string, answers: FormValue, + paths: CoOwnerPaths = { + oldCoOwnersPath: 'ownerCoOwners', + newCoOwnersPath: 'coOwners' + } ): boolean => { // ... reuse the same type guards and path parameters as above ... - const isReviewerAuthorized = [ - oldCoOwners.some(({ nationalId }) => nationalId === reviewerNationalId), - newCoOwners.some(({ nationalId }) => nationalId === reviewerNationalId), - ].some(Boolean) + const isReviewerAuthorized = + oldCoOwners.some(({ nationalId }) => nationalId === reviewerNationalId) || + newCoOwners.some(({ nationalId }) => nationalId === reviewerNationalId) return isReviewerAuthorized && !hasPendingApproval(answers) }libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.ts (3)
53-55: Consider documenting the fallback behavior.While the implementation is correct, adding a comment explaining why
falseis used as the fallback value would improve maintainability, especially given the PR's focus on mileage validation handling.return vehicle?.requireMileage || false +// Default to false when vehicle is not found or requireMileage is undefined +// to prevent validation errors during vehicle submission
73-74: Consider extracting the condition logic for reusability.The condition logic could be reused across different vehicle-related forms. Consider extracting it into a named function in a shared utility file.
-condition: (answers, externalData) => { - const vehicle = getSelectedVehicle(externalData, answers) - return vehicle?.requireMileage || false -}, +condition: shouldShowMileageInput,Add to a shared utility file:
export const shouldShowMileageInput = (answers: Answers, externalData: ExternalData): boolean => { const vehicle = getSelectedVehicle(externalData, answers) return vehicle?.requireMileage || false }
80-83: Use the message system for the placeholder text.The placeholder text is hardcoded in Icelandic. For consistency and maintainability, consider using the message system like other labels in the form.
-return vehicle?.mileageReading - ? `Síðasta skráning ${vehicle.mileageReading} Km` - : '' +return vehicle?.mileageReading + ? information.labels.vehicle.lastMileageReading(vehicle.mileageReading) + : ''libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ApplicationStatus/index.tsx (2)
24-27: LGTM! Consider adding type safety improvementsThe logic change correctly implements the new review button visibility requirements. The code is clean and maintainable.
Consider adding explicit type annotations for better type safety:
- const showReviewButton = canReviewerApprove( + const showReviewButton: boolean = canReviewerApprove( reviewerNationalId, application.answers, )
Line range hint
1-93: Well-structured implementation that follows best practicesThe component implementation:
- Maintains reusability across NextJS apps
- Uses TypeScript effectively for type safety
- Follows clean code principles with good separation of concerns
- Successfully implements the PR objectives regarding review button visibility
Consider extracting the review button logic into a separate component if it grows more complex in the future, to maintain the component's focused responsibility.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.ts (1)
74-74: Consider moving the hardcoded text to messages.While the implementation is type-safe and provides good UX, the hardcoded Icelandic text
"Síðasta skráning ${vehicle.mileageReading} Km"should be moved to the messages file for better reusability and maintainability.Consider updating the placeholder to use a message from the messages file:
- return vehicle?.mileageReading - ? `Síðasta skráning ${vehicle.mileageReading} Km` - : '' + return vehicle?.mileageReading + ? information.labels.vehicle.lastMileageReading(vehicle.mileageReading) + : ''Also applies to: 81-84
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts (3)
5-9: Add JSDoc documentation for better type information and usage guidance.While the function is well-typed, adding JSDoc documentation would improve developer experience by providing more context about parameters and return values.
+/** + * Checks if there are any pending approvals in the vehicle transfer process + * @param answers - Form values containing approval states + * @param excludeNationalId - Optional national ID to exclude from approval checks + * @returns boolean indicating if any approvals are pending + */ export const hasPendingApproval = (
10-16: Add null safety checks for buyer object.The current implementation assumes buyer.nationalId and buyer.approved will always exist. Add null checks to handle edge cases.
-const buyer = getValueViaPath(answers, 'buyer', {}) as UserInformation +const buyer = getValueViaPath(answers, 'buyer', { + nationalId: '', + approved: false, +}) as UserInformation
19-33: Optimize array operations by combining filter and some.The current implementation chains filter and some operations. These can be combined for better performance.
-const buyerCoOwnersAndOperators = ( - getValueViaPath( - answers, - 'buyerCoOwnerAndOperator', - [], - ) as CoOwnerAndOperator[] -).filter(({ wasRemoved }) => wasRemoved !== 'true') -if ( - buyerCoOwnersAndOperators.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) -) +const buyerCoOwnersAndOperators = getValueViaPath( + answers, + 'buyerCoOwnerAndOperator', + [], +) as CoOwnerAndOperator[] +if ( + buyerCoOwnersAndOperators.some( + ({ nationalId, approved, wasRemoved }) => + wasRemoved !== 'true' && + (!excludeNationalId || nationalId !== excludeNationalId) && + !approved, + ) +)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/fields/MainOwner.tsx (2)
49-49: Consider memoizing setFieldLoadingState to prevent unnecessary re-renders.While adding
setFieldLoadingStateto the dependency array follows React's exhaustive dependencies rule, it could cause unnecessary re-renders if the function reference changes. Consider memoizing this callback in the parent component.Example in the parent component:
const memoizedSetFieldLoadingState = useCallback((loading: boolean) => { setFieldLoadingState?.(loading); }, []);
Line range hint
1-116: Component follows library code guidelines.The component adheres to the coding guidelines for
libs/**/*:
- ✅ Reusable across NextJS apps
- ✅ Uses TypeScript for props and type definitions
- ✅ Follows effective tree-shaking practices with named exports
Consider extracting the GraphQL query to a separate file to improve code organization and reusability.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ValidationErrorMessages/index.tsx (1)
61-66: Consider enhancing error handling with error callback.While the onCompleted callback correctly handles successful responses, consider adding an onError callback to handle query failures:
{ onCompleted: (data) => { if (data?.vehicleOperatorChangeValidation?.hasError) { setValidationErrorFound?.(true) } }, + onError: (error) => { + setValidationErrorFound?.(true) + // Consider logging the error or showing a generic error message + }, fetchPolicy: 'no-cache', }Also applies to: 72-72
libs/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/InformationSection/informationSubSection.ts (1)
68-72: Remove redundant optional chainingThe optional chaining operator (
?.) onvehicle?.endDateis unnecessary since we've already checked ifvehicleexists in the ternary condition.- const dateTo = vehicle ? new Date(vehicle?.endDate) : new Date() + const dateTo = vehicle ? new Date(vehicle.endDate) : new Date()libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/CoOwner/index.tsx (1)
28-28: Consider strengthening type safety for API inputs.While the code is functionally correct, we could enhance type safety for the GraphQL query variables.
Consider defining an interface for the query input:
interface VehicleInformationInput { permno: string | undefined; regno: string; vin: string; }Then use it in the variables:
variables: { - input: { + input: { permno: vehicle?.permno, regno: '', vin: '', - }, + } as VehicleInformationInput, },Also applies to: 37-40
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx (3)
75-80: Consider adding error boundary for validation state handling.While the onCompleted callback works, it might benefit from additional error handling to ensure robustness.
Consider wrapping the validation state update in a try-catch:
onCompleted: (data) => { - if (data?.vehicleOwnerChangeValidation?.hasError) { - setValidationErrorFound?.(true) - } + try { + if (data?.vehicleOwnerChangeValidation?.hasError) { + setValidationErrorFound?.(true) + } + } catch (error) { + console.error('Failed to update validation state:', error) + } },
Line range hint
125-139: Consider simplifying the conditional rendering logic.The current nested conditional structure could be made more readable.
Consider restructuring the conditions:
- return data?.vehicleOwnerChangeValidation?.hasError && - data.vehicleOwnerChangeValidation.errorMessages.length > 0 ? ( - <Box> - <AlertMessage type="error" .../> - </Box> - ) : !showErrorOnly ? ( - <Box> - <AlertMessage type="info" .../> - </Box> - ) : null + const hasErrors = data?.vehicleOwnerChangeValidation?.hasError && + data.vehicleOwnerChangeValidation.errorMessages.length > 0 + + if (hasErrors) { + return <Box><AlertMessage type="error" .../></Box> + } + + if (!showErrorOnly) { + return <Box><AlertMessage type="info" .../></Box> + } + + return null
Line range hint
1-139: Excellent adherence to library code guidelines.The component demonstrates:
- Strong TypeScript usage with proper type definitions
- Reusability across different NextJS apps through clear props interface
- Effective tree-shaking support with specific imports
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/InformationSection/vehicleSubSection.ts (1)
Line range hint
1-130: Enhance reusability by extracting common vehicle-related componentsWhile the code follows a modular structure, there's an opportunity to improve reusability across different NextJS apps:
- The vehicle information display logic could be extracted into a reusable component
- The condition functions for owner/co-owner checks could be moved to a shared utility
Consider extracting these patterns into shared components:
// libs/shared/components/src/vehicle/VehicleInformation.tsx export const VehicleInformation = ({ vehicle, owner, showAddress = true }: VehicleInformationProps) => { // Reusable component for displaying vehicle and owner information } // libs/shared/utils/src/vehicle/ownership.ts export const isVehicleOwner = (vehicle?: VehiclesCurrentVehicle) => vehicle?.role === 'Eigandi'This would improve reusability across different vehicle-related forms in the application.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx (3)
38-38: Consider adding error logging for undefined vehicle.While optional chaining prevents runtime errors, adding error logging would help debug cases where vehicle is unexpectedly undefined.
- permno: vehicle?.permno, + permno: vehicle?.permno ?? (() => { + console.error('Vehicle undefined when querying vehicle information'); + return ''; + })(),
57-59: Consider memoizing plateTypeFrontList filter result.The filtering logic is correct but could benefit from memoization to prevent unnecessary recalculations on re-renders.
+ const plateTypeFrontList = useMemo( + () => plateTypeList?.filter((x) => x.code === currentPlateTypeFront) || [], + [plateTypeList, currentPlateTypeFront] + ); - const plateTypeFrontList = - plateTypeList?.filter((x) => x.code === currentPlateTypeFront) || []
Line range hint
140-145: Simplify error message selection logic.The nested ternary operator makes the code harder to read. Consider extracting the error message selection into a separate function.
+ const getErrorMessage = () => { + if (plateTypeFrontListEmptyError) { + return information.labels.plateSize.noPlateMatchError; + } + if (error) { + return information.labels.plateSize.error; + } + return information.labels.plateSize.errorPlateTypeFront; + }; + <AlertMessage type="error" - title={formatMessage( - plateTypeFrontListEmptyError - ? information.labels.plateSize.noPlateMatchError - : error - ? information.labels.plateSize.error - : information.labels.plateSize.errorPlateTypeFront, - )} + title={formatMessage(getErrorMessage())} />libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (4)
51-64: Enhance error handling with specific error typesWhile the error handling is good, consider being more specific about the type of error that occurred:
} catch (e) { logger.error(e) + const errorMessage = e.response?.status === 404 + ? 'Unable to retrieve mileage reading: Vehicle not found' + : `Unable to retrieve mileage reading: ${e.message}`; return { hasError: true, - errorMessages: [{ defaultMessage: e.message }], + errorMessages: [{ defaultMessage: errorMessage }], } }
70-71: Document the mileage increment workaroundThe
currentMileage + 1appears to be a workaround for electric vehicles validation. Consider adding a code comment explaining this behavior.- currentMileage + 1, + // Adding 1 to current mileage to prevent Warning errors for electric vehicles + // during validation before the actual mileage input step + currentMileage + 1,
78-78: Consider making mileage parameter requiredSince mileage is now a critical part of the validation process, especially for electric vehicles, consider making it a required parameter by removing the
nulltype.- mileage: number | null, + mileage: number,
126-163: Reduce code duplication in error handlingConsider extracting the common API call pattern into a private method to reduce duplication:
private async executeOperatorAction( auth: Auth, action: () => Promise<void> ): Promise<ErrorMessage[] | undefined> { try { await action(); return undefined; } catch (e) { return getCleanErrorMessagesFromTryCatch(e); } }Then use it in both cases:
if (operators.length === 0) { - try { - await this.operatorsApiWithAuth(auth).closeWithoutcontractPost({ + errorMessages = await this.executeOperatorAction(auth, async () => { + await this.operatorsApiWithAuth(auth).closeWithoutcontractPost({ // ... existing params ... }) - } catch (e) { - errorMessages = getCleanErrorMessagesFromTryCatch(e) - } + }); } else { - try { - await this.operatorsApiWithAuth(auth).withoutcontractPost({ + errorMessages = await this.executeOperatorAction(auth, async () => { + await this.operatorsApiWithAuth(auth).withoutcontractPost({ // ... existing params ... }) - } catch (e) { - errorMessages = getCleanErrorMessagesFromTryCatch(e) - } + }); }libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/Overview/index.tsx (2)
53-55: Consider using a reducer for complex state managementThe component now manages multiple related states for loading and validation. Consider using useReducer to centralize this logic and make state transitions more predictable.
Example implementation:
type State = { buttonLoading: boolean; shouldLoadValidation: boolean; validationErrorFound: boolean; }; type Action = | { type: 'START_LOADING' } | { type: 'STOP_LOADING' } | { type: 'SET_VALIDATION_STATUS'; payload: boolean } | { type: 'RESET' }; const reducer = (state: State, action: Action): State => { switch (action.type) { case 'START_LOADING': return { ...state, buttonLoading: true, shouldLoadValidation: true }; case 'STOP_LOADING': return { ...state, buttonLoading: false }; case 'SET_VALIDATION_STATUS': return { ...state, validationErrorFound: action.payload }; case 'RESET': return initialState; default: return state; } };
Line range hint
173-206: Consider extracting button rendering logicThe button rendering logic could be simplified by extracting it into separate components to reduce duplication and improve maintainability.
type ActionButtonProps = { loading: boolean; onClick: () => void; icon: string; label: string; }; const ActionButton: FC<ActionButtonProps> = ({ loading, onClick, icon, label }) => ( <Box marginLeft={3}> <Button icon={icon} loading={loading} onClick={onClick} > {label} </Button> </Box> ); // Usage: {canReviewerApprove(reviewerNationalId, application.answers) && ( <Box display="flex" justifyContent="flexEnd" flexWrap="wrap"> <ActionButton icon="close" loading={buttonLoading} onClick={onRejectButtonClick} label={formatMessage(review.buttons.reject)} /> <ActionButton icon="checkmark" loading={buttonLoading} onClick={onApproveButtonClick} label={formatMessage(review.buttons.approve)} /> </Box> )}libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/Overview/index.tsx (3)
58-60: Consider adding TypeScript types for state variables.While the granular state management is good, consider defining explicit types for better type safety and documentation.
- const [buttonLoading, setButtonLoading] = useState(false) - const [shouldLoadValidation, setShouldLoadValidation] = useState(false) - const [validationErrorFound, setValidationErrorFound] = useState(false) + const [buttonLoading, setButtonLoading] = useState<boolean>(false) + const [shouldLoadValidation, setShouldLoadValidation] = useState<boolean>(false) + const [validationErrorFound, setValidationErrorFound] = useState<boolean>(false)
160-165: Consider memoizing ValidationErrorMessages component.Since the component depends on multiple props and state variables, consider using React.memo to prevent unnecessary re-renders.
+ const MemoizedValidationErrorMessages = React.memo(ValidationErrorMessages) {!buttonLoading && shouldLoadValidation && ( - <ValidationErrorMessages + <MemoizedValidationErrorMessages {...props} setValidationErrorFound={setValidationErrorFound} /> )}
201-212: LGTM! Consider using optional chaining.The "Try again" button implementation aligns well with the PR objectives. However, the condition could be simplified using optional chaining.
- {canReviewerReApprove(reviewerNationalId, application.answers) && - application.state !== States.COMPLETED && ( + {canReviewerReApprove(reviewerNationalId, application?.answers) && + application?.state !== States.COMPLETED && (libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx (4)
70-72: Consider initializing validationErrorFound based on existing errors.The state initialization looks good, but consider setting the initial value of
validationErrorFoundbased on any existing validation errors in the application state to prevent flickering of error messages.- const [validationErrorFound, setValidationErrorFound] = useState(false) + const [validationErrorFound, setValidationErrorFound] = useState( + Boolean(error || application.validationErrors?.length) + )
204-210: Optimize validation error messages rendering.Consider memoizing the ValidationErrorMessages component to prevent unnecessary re-renders:
+ const memoizedValidationErrors = useMemo( + () => ( + <ValidationErrorMessages + {...props} + showErrorOnly={true} + setValidationErrorFound={setValidationErrorFound} + /> + ), + [props, setValidationErrorFound] + ); {!buttonLoading && shouldLoadValidation && ( - <ValidationErrorMessages - {...props} - showErrorOnly={true} - setValidationErrorFound={setValidationErrorFound} - /> + {memoizedValidationErrors} )}
247-258: Enhance button accessibility.The re-approve button implementation looks good, but consider adding ARIA labels for better accessibility:
<Button icon="reload" loading={buttonLoading} onClick={onApproveButtonClick} + aria-label={formatMessage(review.buttons.tryAgain)} > {formatMessage(review.buttons.tryAgain)} </Button>
Line range hint
39-47: Consider extracting reusable components.As per coding guidelines for
libs/**/*, consider extracting the approval buttons into a separate reusable component to improve maintainability and reuse across different NextJS apps.libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/ChangeOperatorOfVehicleTemplate.ts (1)
145-145: Consider conditional inclusion of mock APIWhile the mock API addition is correct, consider conditionally including it based on the environment (development/testing vs production) to prevent potential misuse in production.
Example approach:
api: [ IdentityApi, UserProfileApi, SamgongustofaPaymentCatalogApi, ...(process.env.NODE_ENV !== 'production' ? [MockableSamgongustofaPaymentCatalogApi] : []), CurrentVehiclesApi, ]libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (1)
490-502: Enhance error message handlingThe error handling implementation correctly captures and throws SGS system errors. However, it could be improved by:
- Using optional chaining for cleaner null checks
- Preserving the error type information in the summary
Consider this enhancement:
- if ( - submitResult.hasError && - submitResult.errorMessages && - submitResult.errorMessages.length > 0 - ) { + if (submitResult.hasError && submitResult.errorMessages?.length > 0) { throw new TemplateApiError( { title: applicationCheck.validation.alertTitle, - summary: submitResult.errorMessages, + summary: submitResult.errorMessages.map(msg => `${msg.type}: ${msg.message}`), }, 400, ) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (46)
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts(0 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts(1 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts(1 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/dataProviders/index.ts(2 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ApplicationStatus/index.tsx(2 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/Overview/index.tsx(7 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ValidationErrorMessages/index.tsx(4 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.ts(5 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/externalDataSection.ts(2 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/ChangeCoOwnerOfVehicleTemplate.ts(3 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/messages/review.ts(1 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts(1 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/hasReviewerApproved.ts(0 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/index.ts(1 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/dataProviders/index.ts(2 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ApplicationStatus/index.tsx(2 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/Overview/index.tsx(7 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ValidationErrorMessages/index.tsx(4 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.ts(5 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/externalDataSection.ts(2 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/ChangeOperatorOfVehicleTemplate.ts(3 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/messages/review.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/hasReviewerApproved.ts(0 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/index.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/InformationSection/informationSubSection.ts(3 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/utils/getSelectedVehicle.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx(7 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/InformationSection/plateSizeSubSection.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/utils/getSelectedVehicle.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/fields/MainOwner.tsx(3 hunks)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/InformationSection/vehicleSubSection.ts(7 hunks)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/utils/getSelectedVehicle.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/CoOwner/index.tsx(3 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx(7 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx(5 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts(5 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/TransferOfVehicleOwnershipTemplate.ts(3 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/messages/steps.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts(1 hunks)libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts(4 hunks)
💤 Files with no reviewable changes (3)
- libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/hasReviewerApproved.ts
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/hasReviewerApproved.ts
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/messages/steps.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/vehicleSubSection.ts
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/TransferOfVehicleOwnershipTemplate.ts
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts
🧰 Additional context used
📓 Path-based instructions (39)
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.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/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/dataProviders/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."
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ApplicationStatus/index.tsx (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/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/Overview/index.tsx (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/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ValidationErrorMessages/index.tsx (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/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/externalDataSection.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/ChangeCoOwnerOfVehicleTemplate.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/messages/review.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.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/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/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."
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.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/application/templates/transport-authority/change-operator-of-vehicle/src/dataProviders/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."
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ApplicationStatus/index.tsx (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/application/templates/transport-authority/change-operator-of-vehicle/src/fields/Overview/index.tsx (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/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ValidationErrorMessages/index.tsx (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/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.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/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/externalDataSection.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/application/templates/transport-authority/change-operator-of-vehicle/src/lib/ChangeOperatorOfVehicleTemplate.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/application/templates/transport-authority/change-operator-of-vehicle/src/lib/messages/review.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/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.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/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.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/application/templates/transport-authority/change-operator-of-vehicle/src/utils/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."
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.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/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/InformationSection/informationSubSection.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/application/templates/transport-authority/license-plate-renewal/src/utils/getSelectedVehicle.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/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx (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/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/InformationSection/plateSizeSubSection.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/application/templates/transport-authority/order-vehicle-license-plate/src/utils/getSelectedVehicle.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/application/templates/transport-authority/order-vehicle-registration-certificate/src/fields/MainOwner.tsx (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/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/InformationSection/vehicleSubSection.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/application/templates/transport-authority/order-vehicle-registration-certificate/src/utils/getSelectedVehicle.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/CoOwner/index.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx (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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.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/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
📓 Learnings (3)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/dataProviders/index.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-10T17:05:28.408Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/externalDataSection.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-10T17:05:28.408Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/externalDataSection.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-10T17:05:28.408Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
🪛 Biome
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/Overview/index.tsx
[error] 117-117: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/Overview/index.tsx
[error] 122-122: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx
[error] 140-140: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (72)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/index.ts (2)
3-3: Ensure proper TypeScript type exports.
Since this is a library module (libs/**/*), consider exporting TypeScript types for these utility functions to maintain type safety across different NextJS applications using this module.
Let's check for type definitions:
#!/bin/bash
# Description: Check for TypeScript type exports
# Expected: Type definitions for the utility functions
# Look for type definitions
rg -g '*.d.ts' -g '*.ts' '(type|interface).*Review' --type typescript
# Check if types are being exported
rg "export.*(type|interface)" ./libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/3-3: Verify the implementation of new approval functions.
The new exports canReviewerApprove and canReviewerReApprove have more specific and descriptive names compared to the removed hasReviewerApproved. This aligns with the PR objective of improving the review process.
Let's verify the implementation of these new functions:
✅ Verification successful
Let me generate another script to find the implementation of these functions since the previous search pattern wasn't correct.
The new approval functions are well-implemented and correctly handle reviewer permissions
The implementation shows that:
canReviewerApproveproperly checks if the reviewer is either an old or new co-owner who hasn't approved yetcanReviewerReApprovehandles the special case of re-triggering approval when all approvals are complete but the application is stuck- Both functions use proper type safety with TypeScript and follow the repository's patterns
The changes align with the PR objectives and maintain code quality standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of new approval functions
# Expected: Functions should handle reviewer permissions and approval states
# Check the implementation file
rg -A 10 "export (function |const )(canReviewerApprove|canReviewerReApprove)" --type typescript
# Look for tests
fd "canReviewerApprove" -e test.ts -e spec.ts --exec cat
Length of output: 2570
Script:
#!/bin/bash
# Search for the implementation of canReviewerApprove functions
rg -A 10 "export (const|function) (canReviewerApprove|canReviewerReApprove)"
# Also check the actual file mentioned in the import
cat ./libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts
Length of output: 11243
libs/application/templates/transport-authority/license-plate-renewal/src/utils/getSelectedVehicle.ts (1)
8-8: LGTM! Explicit return type improves type safety.
The addition of explicit return type PlateOwnership | undefined enhances type safety and makes the function's contract clearer.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/index.ts (2)
3-3: LGTM! Clean and maintainable exports structure.
The new exports follow good practices for modular code organization and tree-shaking optimization.
3-3: Verify removal of hasReviewerApproved
Let's ensure all usages of the removed hasReviewerApproved function have been updated to use the new functions.
✅ Verification successful
Function replacement has been properly implemented
The verification shows that:
- There are no remaining usages of
hasReviewerApprovedin the change-operator-of-vehicle template - The new functions
canReviewerApproveandcanReviewerReApproveare properly implemented and exported - The importing files have been updated to use the new functions, particularly in:
- ChangeOperatorOfVehicleTemplate.ts
- ApplicationStatus/index.tsx
The old function has been successfully replaced with more specific implementations that handle both initial approval and re-approval scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining usage of hasReviewerApproved that might have been missed
# Search for any remaining direct usage of hasReviewerApproved
echo "Checking for remaining usage of hasReviewerApproved..."
rg "hasReviewerApproved" -A 3
# Search for imports from this utils file to verify updates
echo "Checking files that import from this utils file..."
rg "from ['\"].*transport-authority/change-operator-of-vehicle/src/utils['\"]" -A 3
Length of output: 7777
Script:
#!/bin/bash
# Let's check for any usage of hasReviewerApproved specifically in the transport-authority/change-operator-of-vehicle directory
echo "Checking for hasReviewerApproved in change-operator-of-vehicle..."
fd . --full-path "*transport-authority/change-operator-of-vehicle*" --type f --exec rg "hasReviewerApproved" {} -l
# Let's also check what files are importing from this utils file
echo "Checking imports from change-operator-of-vehicle utils..."
fd . --full-path "*transport-authority/change-operator-of-vehicle*" --type f --exec rg "from ['\"].*utils['\"]" {} -l
# Let's look at the content of the canReviewerApprove file to understand the changes
echo "Content of canReviewerApprove file..."
fd "canReviewerApprove.ts" --full-path "*transport-authority/change-operator-of-vehicle*" --type f --exec cat {}
Length of output: 1116
Script:
#!/bin/bash
# Let's try a different approach using relative paths and simpler commands
# Check for hasReviewerApproved in the specific template
echo "Checking for hasReviewerApproved in the template..."
rg "hasReviewerApproved" libs/application/templates/transport-authority/change-operator-of-vehicle/
# Check files importing from utils
echo "Checking imports from utils..."
rg "from ['\"].*utils['\"]" libs/application/templates/transport-authority/change-operator-of-vehicle/
# Look at the canReviewerApprove implementation
echo "Content of canReviewerApprove file..."
cat libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts
Length of output: 4873
libs/application/templates/transport-authority/order-vehicle-license-plate/src/utils/getSelectedVehicle.ts (1)
8-8: LGTM! Improved type safety with explicit undefined handling.
The change to return VehiclesCurrentVehicle | undefined makes the function more type-safe by forcing consumers to handle cases where no vehicle is found.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/dataProviders/index.ts (2)
4-4: LGTM: Clean import addition
The import of MockablePaymentCatalogApi is properly added alongside the existing imports.
17-23: LGTM: Well-structured mock API configuration
The mock API configuration follows the established pattern of maintaining both real and mock implementations, as previously learned from similar implementations. The configuration correctly mirrors the real API's structure while maintaining type safety.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/dataProviders/index.ts (2)
4-4: LGTM! Import follows tree-shaking best practices.
The named import of MockablePaymentCatalogApi allows for effective tree-shaking, ensuring only the necessary code is included in the final bundle.
17-23: LGTM! API configuration follows established patterns.
The implementation:
- Maintains consistency with the existing
SamgongustofaPaymentCatalogApi - Reuses the same configuration structure for better maintainability
- Properly utilizes TypeScript through the imported types and configurations
Let's verify the usage of this new API across the codebase:
✅ Verification successful
MockableSamgongustofaPaymentCatalogApi follows established patterns and is correctly implemented
The verification confirms that:
- The API configuration follows the consistent pattern used across the codebase for mockable payment catalog APIs
- It's properly derived from the base
MockablePaymentCatalogApi - The implementation matches other transport authority templates that use similar configuration
- The usage is appropriate in both test and non-test contexts, with proper separation between mockable and non-mockable versions
- The configuration parameters (organizationId and externalDataId) align with other Samgongustofa payment integrations
The implementation maintains consistency and follows the established patterns for payment catalog APIs in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the MockableSamgongustofaPaymentCatalogApi is properly used in test scenarios
# and not in production code
# Search for usage in test files
echo "Usage in test files:"
rg "MockableSamgongustofaPaymentCatalogApi" -g "*.test.*" -g "*.spec.*"
# Search for usage in non-test files (excluding the current file)
echo -e "\nUsage in non-test files:"
rg "MockableSamgongustofaPaymentCatalogApi" -g "!*.test.*" -g "!*.spec.*" -g "!dataProviders/index.ts"
Length of output: 2892
Script:
#!/bin/bash
# Let's verify if this is a mockable API pattern used consistently across templates
# and if it's properly derived from MockablePaymentCatalogApi
# Check the pattern of MockablePaymentCatalogApi configuration
echo "Checking MockablePaymentCatalogApi usage pattern:"
ast-grep --pattern 'MockablePaymentCatalogApi.configure({
$$$
})'
# Check if there are any direct uses of PaymentCatalogApi (non-mockable version)
echo -e "\nChecking PaymentCatalogApi direct usage:"
rg "PaymentCatalogApi" -g "!*.test.*" -g "!*.spec.*" | grep -v "Mockable"
Length of output: 29541
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/utils/getSelectedVehicle.ts (1)
Line range hint 1-27: Verify the impact of returning undefined
Since this is a breaking change in the return type, we should verify all consumers of this function are prepared to handle undefined values.
✅ Verification successful
Let me analyze the findings from the shell script results:
- The
getSelectedVehiclefunction is used across multiple templates in the transport-authority and energy-funds applications - All usages of the function already handle the undefined case by:
- Using optional chaining (
?.) when accessing properties - Providing fallback values with
||operator - Using type guards in conditional statements
- Using optional chaining (
For example:
vehicle?.permnoin vehicle subsectionsvehicle?.makein vehicle detailsvehicle?.role !== 'Eigandi'with conditional checksvehicle?.mileageReading || ''with fallback valuesvehicle?.requireMileage || falsewith boolean fallbacks
The return type change is safe as all consumers already handle undefined values
The codebase consistently uses null-safe operations when accessing properties of the returned vehicle object, indicating that undefined handling was already part of the design. The change in return type signature is merely making explicit what was already handled in practice.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of getSelectedVehicle to verify they handle undefined
rg -l "getSelectedVehicle" | while read -r file; do
echo "=== $file ==="
rg -A 5 "getSelectedVehicle" "$file"
done
Length of output: 21920
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts (1)
8-8:
Breaking change: Function can now return undefined
The return type change improves type safety but requires consumers to handle the undefined case. Ensure all consuming components are updated to handle this case appropriately.
Let's verify the impact on consuming components:
✅ Verification successful
Based on the search results, I can see that all usages of getSelectedVehicle already handle the undefined case through optional chaining (?.) or fallback values (|| false, || ''). Here are some examples:
return vehicle?.permnoreturn vehicle?.makereturn vehicle?.requireMileage || falsereturn vehicle?.mileageReading || ''const dateTo = vehicle ? new Date(vehicle?.endDate) : new Date()
Since all consumers are already handling the undefined case safely, this change is actually making the type signature more accurate to match the existing behavior.
Return type change is safe and matches existing usage patterns
All consuming components already handle the undefined case through optional chaining or fallback values, making this type signature change a safe improvement that better reflects the actual behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of getSelectedVehicle to ensure they handle undefined
# Test: Search for direct usage of getSelectedVehicle without null checks
rg -A 5 "getSelectedVehicle\("
# Test: Look for potential TypeScript errors in components using this function
ast-grep --pattern 'const $vehicle = getSelectedVehicle($_, $_)'
Length of output: 38796
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getSelectedVehicle.ts (2)
8-8: LGTM: Return type change improves type safety
The explicit undefined in the return type is a good practice as it forces consumers to handle cases where no vehicle is found.
Line range hint 1-43: Verify the impact of mileage updates
Since this function is part of handling dummy mileage in vehicle validation (per PR objectives), we should verify how this utility is used in the validation flow.
✅ Verification successful
Let me gather more specific information about how the mileage updates are handled in validation.
Mileage updates are properly handled in the validation flow
The verification shows that:
- The
getSelectedVehicleutility is used in the form's vehicle subsection to display and manage mileage information - Mileage validation is properly integrated in the service layer (
change-operator-of-vehicle.service.ts) where:- Mileage readings are fetched from the
MileageReadingApi - Mileage is validated during operator changes via
validateAllForOperatorChange - Mileage updates are included in the final submission through
saveOperators
- Mileage readings are fetched from the
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to getSelectedVehicle in validation-related files
rg -l "getSelectedVehicle" | rg "valid|validation"
# Search for files that might handle vehicle validation
rg -l "validateVehicle|vehicleValidation"
Length of output: 908
Script:
#!/bin/bash
# Search for validation logic related to mileage in vehicle operator changes
rg -A 5 "mileage" libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/
# Check for any validation rules or schemas that might involve mileage
ast-grep --pattern 'const $_ = z.object({
$$$
mileage$_: $_,
$$$
})'
# Look for validation functions that might use getSelectedVehicle
rg -A 10 "getSelectedVehicle" libs/application/templates/transport-authority/change-operator-of-vehicle/
Length of output: 27810
libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/externalDataSection.ts (2)
12-12: LGTM! Import statement follows best practices.
The import follows TypeScript practices and maintains a clear naming convention.
44-47: LGTM! Data provider addition follows established patterns.
The mock provider is correctly added alongside the real provider, which aligns with our learnings from PR #16471 where both implementations should coexist. This supports testing scenarios while maintaining production functionality.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/externalDataSection.ts (1)
12-12: LGTM!
The import statement follows TypeScript conventions and maintains consistent ordering with existing imports.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts (1)
Line range hint 1-51: Compliant with coding guidelines for libs directory.
The code meets all requirements:
- ✓ Functions are reusable across different NextJS apps
- ✓ TypeScript is used effectively for type definitions
- ✓ Code is tree-shakeable with named exports
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/InformationSection/plateSizeSubSection.ts (3)
36-37: LGTM! Improved type safety with optional chaining.
The removal of explicit type assertion and use of optional chaining (?.) provides better type safety and null checking.
50-51: LGTM! Consistent type handling approach.
The changes maintain consistency with the previous improvement, using optional chaining for safe property access.
Line range hint 1-62: Verify component reusability across NextJS apps.
The component structure follows good practices for reusability:
- Uses shared form building blocks from
@island.is/application/core - Centralizes messages in external files
- Maintains clear separation of concerns
However, let's verify its usage across different NextJS applications.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts (2)
1-5: LGTM! Well-structured imports with proper type definitions
The imports are well-organized, utilizing TypeScript types and utility functions from the application core. This follows the coding guidelines for reusability across NextJS apps.
1-65: Verify the integration with existing approval flows
Since these are new utility functions replacing hasReviewerApproved, we should verify their integration.
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Utility functions are properly integrated and consistently used
The new approval utility functions (canReviewerApprove and canReviewerReApprove) are consistently implemented across three transport authority templates:
- change-operator-of-vehicle
- change-co-owner-of-vehicle
- transfer-of-vehicle-ownership
Each template follows the same pattern for approval checks and the functions are used in the same key locations (Template, Overview, and ApplicationStatus components). The old hasReviewerApproved function exists in other unrelated templates (id-card, aosh) and doesn't conflict with these changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of the new approval functions and removed function
# Check for removed function usage that might need updating
echo "Checking for any remaining usage of old hasReviewerApproved function:"
rg "hasReviewerApproved"
# Check where the new functions are being used
echo "Checking usage of new approval functions:"
rg "canReviewerApprove|canReviewerReApprove"
# Look for potential similar implementations that could be consolidated
echo "Checking for similar approval check patterns:"
ast-grep --pattern 'nationalId === $_ && !approved'
Length of output: 9678
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts (1)
1-4: LGTM! Well-structured imports with proper TypeScript usage.
The imports are properly organized, utilizing specific type imports which enables effective tree-shaking.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/forms/ChangeOperatorOfVehicleForm/InformationSection/vehicleSubSection.ts (3)
29-31: LGTM! Type-safe implementation using optional chaining.
The removal of type assertions in favor of optional chaining improves type safety while maintaining the same functionality.
43-45: LGTM! Consistent implementation with the plate field.
The implementation maintains consistency with the plate field, using optional chaining for type safety.
63-65: Verify empty string handling for mileage validation.
Given that the PR aims to handle Warning errors for electric vehicles when mileage is not populated, we should verify that returning an empty string here doesn't trigger those warnings.
✅ Verification successful
Empty string mileage handling is properly validated
The empty string fallback for mileage reading is safe because:
- The schema validation shows that mileage fields are optional
- The validation logic checks
requireMileageflag before enforcing any constraints - When
requireMileageis false, empty values are explicitly allowed - The validation ensures proper comparison when values are provided
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for mileage validation logic to verify empty string handling
rg -A 5 "mileageReading|validateVehicle" --type typescript
Length of output: 95
Script:
#!/bin/bash
# Search for mileage validation logic to verify empty string handling
rg -A 5 "mileageReading|validateVehicle"
Length of output: 76015
Script:
#!/bin/bash
# Search for mileage validation logic in dataSchema files
rg -A 10 "vehicleMileage.*refine" libs/application/templates/transport-authority/*/src/lib/dataSchema.ts
Length of output: 1025
Script:
#!/bin/bash
# Search for mileage validation logic in dataSchema files with a broader pattern
rg -A 10 "vehicleMileage" libs/application/templates/transport-authority/*/src/lib/dataSchema.ts
Length of output: 5125
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ApplicationStatus/index.tsx (3)
11-11: LGTM! Import statement updated correctly.
The change from hasReviewerApproved to canReviewerApprove aligns with the PR objectives to improve reviewer access control.
Line range hint 16-89: Implementation follows library coding guidelines.
The component implementation adheres to the coding guidelines for library code:
- ✓ Reusable component with well-defined props
- ✓ Proper TypeScript usage for props and types
- ✓ Specific imports enabling effective tree-shaking
24-27: Verify the reviewer access control logic.
The implementation looks correct, but let's verify that the new canReviewerApprove function properly handles all reviewer scenarios as intended.
✅ Verification successful
Let me check the implementation of canReviewerApprove function to ensure it properly handles all reviewer scenarios.
Access control logic is properly implemented and consistent
The canReviewerApprove function correctly handles all reviewer scenarios for the change-operator-of-vehicle template:
- Verifies if the reviewer is a co-owner who hasn't approved yet
- Checks if the reviewer is a new operator who hasn't approved yet
- Includes proper filtering of removed operators
- Maintains consistency with other transport authority templates' implementations
- Has a companion
canReviewerReApprovefunction for handling edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of canReviewerApprove function
# and its usage across the codebase
# Check the implementation of canReviewerApprove
echo "Checking canReviewerApprove implementation:"
ast-grep --pattern 'export const canReviewerApprove = ($params) => {
$$$
}'
# Check for any remaining usage of old hasReviewerApproved
echo "Checking for any remaining usage of hasReviewerApproved:"
rg "hasReviewerApproved"
# Check for consistent usage of canReviewerApprove
echo "Checking usage of canReviewerApprove:"
rg "canReviewerApprove"
Length of output: 7276
Script:
#!/bin/bash
# Get the implementation of canReviewerApprove from all transport authority templates
echo "Implementation of canReviewerApprove in change-operator-of-vehicle:"
cat libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts
echo -e "\nImplementation of canReviewerApprove in transfer-of-vehicle-ownership:"
cat libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts
echo -e "\nImplementation of canReviewerApprove in change-co-owner-of-vehicle:"
cat libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts
Length of output: 7849
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ApplicationStatus/index.tsx (1)
11-11: Verify the implementation of canReviewerApprove utility
The import change from hasReviewerApproved to canReviewerApprove aligns with the PR objectives to improve review button visibility.
Let's verify the implementation of the new utility function:
✅ Verification successful
Import change is correct and implementation is properly aligned with requirements
The canReviewerApprove utility function is correctly implemented to:
- Check if the reviewer is an old co-owner who hasn't approved yet
- Check if the reviewer is a new co-owner who hasn't approved yet
- Return true only if the reviewer is authorized and hasn't approved
Additionally, the companion function canReviewerReApprove handles the special case when all approvals are complete but the application is stuck in REVIEW state, which aligns with the PR objective of improving the "Try again" button visibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of canReviewerApprove utility function
# Expected: Function should check if reviewer can approve when all approvals are in but application is in REVIEW state
ast-grep --pattern 'export function canReviewerApprove($_) {
$$$
}'
Length of output: 71
Script:
#!/bin/bash
# Let's try to find the file containing the utility functions first
fd "utils" --type f --extension ts --extension tsx libs/application/templates/transport-authority/change-co-owner-of-vehicle/src
# Then let's search for both old and new function implementations
echo "=== Searching for canReviewerApprove and hasReviewerApproved functions ==="
rg "function (canReviewerApprove|hasReviewerApproved)" -A 10 libs/application/templates/transport-authority/change-co-owner-of-vehicle/src
Length of output: 428
Script:
#!/bin/bash
# Let's try a broader search for the function names without the "function" keyword
echo "=== Searching for canReviewerApprove ==="
rg "canReviewerApprove" -A 5 libs/application/templates/transport-authority/change-co-owner-of-vehicle/src
echo "=== Searching for hasReviewerApproved ==="
rg "hasReviewerApproved" -A 5 libs/application/templates/transport-authority/change-co-owner-of-vehicle/src
# Also search for the export statement
echo "=== Searching for export statements ==="
rg "export.*canReviewerApprove|export.*hasReviewerApproved" libs/application/templates/transport-authority/change-co-owner-of-vehicle/src
Length of output: 8113
Script:
#!/bin/bash
# Let's check the implementation details of canReviewerApprove
echo "=== Checking canReviewerApprove implementation ==="
cat libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts
Length of output: 2364
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/forms/ChangeCoOwnerOfVehicleForm/InformationSection/vehicleSubSection.ts (3)
30-31: LGTM! Type-safe property access implemented.
The use of optional chaining improves type safety by gracefully handling cases where the vehicle might be undefined.
44-45: LGTM! Consistent type-safe implementation.
The implementation maintains consistency with other fields and ensures type safety through optional chaining.
54-55: LGTM! Mileage fields properly handle undefined cases.
The implementation supports the PR's objective of handling dummy mileage values with appropriate fallbacks.
Let's verify the mileage validation implementation mentioned in the PR objectives:
Also applies to: 64-65
✅ Verification successful
Mileage implementation is properly integrated with validation and type system
The implementation is well-integrated into the codebase:
- Proper validation schema in
dataSchema.tshandles mileage requirements and validation logic - Type definitions in
types.tscorrectly define optional mileage fields getSelectedVehicleutility properly manages mileage reading updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related mileage validation logic
rg -t typescript -l "mileage.*validation|validateVehicle.*mileage"
Length of output: 103
Script:
#!/bin/bash
# Search for mileage validation patterns with ripgrep
rg -l "mileage.*validation|validateVehicle.*mileage|requireMileage|mileageReading"
# Also search for validation-related files
fd "validation|validator" -e ts -e js
Length of output: 5924
Script:
#!/bin/bash
# Let's examine the mileage-related code in the relevant files
rg -A 5 "requireMileage|mileageReading" libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/dataSchema.ts
# Also check the type definitions
rg -A 5 "requireMileage|mileageReading" libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/shared/types.ts
# And check the vehicle selection logic
cat libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/getSelectedVehicle.ts
Length of output: 2597
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts (1)
74-77: Fix array comparison issue (duplicate comment)
The array comparison using === is incorrect as it checks reference equality instead of content equality.
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/fields/MainOwner.tsx (2)
28-28: LGTM! Improved type safety by removing type assertion.
The removal of the type assertion allows TypeScript to properly handle potentially undefined values, making the code more robust.
37-37: LGTM! Safe property access with optional chaining.
The use of optional chaining operator prevents runtime errors and properly handles the case when vehicle is undefined.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/ValidationErrorMessages/index.tsx (3)
5-19: LGTM! Well-structured imports and type definitions.
The changes follow TypeScript best practices and enable proper tree-shaking through specific imports.
22-23: LGTM! Clean component signature update.
The TypeScript type composition is well-structured, combining the new Props with existing FieldBaseProps.
69-70: LGTM! Proper useEffect dependency handling.
The dependency array correctly includes all dependencies used within the effect.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/ValidationErrorMessages/index.tsx (3)
5-10: LGTM! Imports and interface follow best practices.
The specific imports support effective tree-shaking, and the TypeScript interface is well-defined. The optional prop makes the component more reusable across different contexts.
Also applies to: 17-19
21-23: LGTM! Component signature is properly typed.
The component signature correctly merges the new Props interface with FieldBaseProps while maintaining support for children props.
82-82: LGTM! UI changes improve consistency.
The switch to BulletList/Bullet components from the design system improves UI consistency while maintaining the robust error message fallback logic.
Also applies to: 100-102, 106-106
libs/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/InformationSection/informationSubSection.ts (2)
33-37: LGTM! Type-safe vehicle data access
The changes properly handle the case where getSelectedVehicle may return undefined, using optional chaining for safe property access.
53-58: LGTM! Robust date handling with fallback
The implementation correctly handles both the presence and absence of vehicle data, providing a sensible fallback to the current date when needed.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/messages/review.ts (2)
113-113: LGTM: Consistent string formatting
The update to use single quotes for the approve button's defaultMessage maintains consistency with other string literals in the file.
116-120: LGTM: Well-structured tryAgain button messages
The new tryAgain button messages are properly implemented with:
- Consistent naming convention for the message ID
- Clear description for translators
- Proper integration with react-intl for i18n support
This aligns with the PR objective to improve the visibility of the "Try again" button for resubmitting applications.
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/CoOwner/index.tsx (2)
37-37: LGTM! Improved null safety with optional chaining.
The use of optional chaining for vehicle?.permno enhances code robustness by safely handling cases where vehicle might be undefined.
47-47: LGTM! Fixed useEffect dependencies.
Adding setFieldLoadingState to the dependency array ensures proper re-execution of the effect when the callback changes.
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/ValidationErrorMessages/index.tsx (4)
5-21: LGTM! Well-structured TypeScript interface and imports.
The Props interface is well-defined with proper TypeScript types, and the imports are organized efficiently.
23-25: LGTM! Type-safe component signature.
The component signature properly combines custom Props with FieldBaseProps while maintaining type safety.
84-86: LGTM! Proper useEffect dependency handling.
The useEffect hook correctly includes all dependencies and follows React hooks best practices.
Line range hint 96-120: LGTM! Clean UI component implementation.
The BulletList/Bullet implementation is clean and follows React best practices with proper key usage.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx (3)
14-14: LGTM! Import changes align with TypeScript best practices.
The removal of explicit type casting improves type flexibility while maintaining type safety.
69-69: LGTM! Correct useEffect dependencies.
The addition of setFieldLoadingState to the dependencies array prevents potential stale closure issues.
29-29: Verify type safety after removing VehiclesCurrentVehicle casting.
The removal of type casting requires verification to ensure type safety is maintained throughout the component's usage.
✅ Verification successful
Let me generate additional scripts to check the type definition and usage pattern.
Type safety is maintained after removing VehiclesCurrentVehicle casting
The removal of type casting is safe because:
- The codebase consistently uses optional chaining (
?.) when accessing vehicle properties, showing proper null-safety handling - In PickPlateSize.tsx, only the
permnoproperty is accessed with optional chaining, indicating proper type-safety awareness - The pattern is consistent across multiple vehicle-related components in the transport authority templates
- Only the energy funds template still uses type casting, which is isolated and doesn't affect this component
The changes align with TypeScript's type safety best practices by relying on the compiler's type inference and optional chaining instead of force-casting.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of getSelectedVehicle to ensure consistent typing
ast-grep --pattern 'getSelectedVehicle($$$)'
# Check for any type assertions or type-related errors
ast-grep --pattern 'as VehiclesCurrentVehicle'
Length of output: 22336
Script:
#!/bin/bash
# Check the type definition of getSelectedVehicle
ast-grep --pattern 'function getSelectedVehicle($$$)'
# Check how the vehicle object is used in PickPlateSize.tsx
rg -A 5 "const vehicle = getSelectedVehicle" libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx
Length of output: 349
Script:
#!/bin/bash
# Check how the vehicle object is used in the component
rg -B 5 -A 10 "vehicle\." libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx
# Find the import of getSelectedVehicle to check its type definition
rg "import.*getSelectedVehicle" libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/PickPlateSize.tsx
Length of output: 315
Script:
#!/bin/bash
# Check the implementation of getSelectedVehicle in utils
rg -A 10 "export.*getSelectedVehicle" libs/application/templates/transport-authority/order-vehicle-license-plate/src/utils.ts
# Check if vehicle is used with optional chaining
ast-grep --pattern 'vehicle?.$_'
Length of output: 7777
libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/messages/review.ts (2)
126-126: LGTM! Consistent string format
The change from template literal to string literal improves consistency with other message definitions in the file.
129-133: LGTM! Well-structured message definitions
The new tryAgain button messages are well-defined and follow best practices:
- Clear namespace convention in the ID
- Helpful description for translators
- Follows the module's structure for tree-shaking optimization
libs/clients/transport-authority/vehicle-operators/src/lib/vehicleOperatorsClient.service.ts (2)
8-12: LGTM: Clean dependency injection and imports
The new imports and constructor injection for MileageReadingApi are well-structured and follow dependency injection patterns.
Also applies to: 17-20
26-28: LGTM: Consistent auth middleware implementation
The new mileageReadingApiWithAuth method follows the same pattern as the existing operatorsApiWithAuth, maintaining consistency in the codebase.
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/Overview/index.tsx (2)
16-18: LGTM: Import changes and utility functions
The changes follow best practices for TypeScript usage and maintain tree-shakeable imports. The new utility functions improve the clarity of approval logic.
Also applies to: 26-27
154-159: LGTM: Validation error handling implementation
The validation logic effectively handles the display of error messages and properly manages the loading state. This aligns with the PR objective of improving warning severity message handling.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/Overview/index.tsx (1)
21-23: LGTM! Clear and descriptive function naming.
The renaming of hasReviewerApproved to canReviewerApprove and addition of canReviewerReApprove improve code clarity and align with the PR objective of enhancing resubmission functionality.
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx (1)
12-12: LGTM: Import changes align with new functionality.
The updated imports properly support the new validation and approval functionality while maintaining type safety.
Also applies to: 24-25, 33-33, 36-36
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/ChangeCoOwnerOfVehicleTemplate.ts (4)
69-69: LGTM! Improved warning display logic
The updated condition using canReviewerApprove better aligns with the PR's objective of handling warning severity messages. The change makes the warning display more proactive by showing it when a reviewer can approve rather than after approval.
33-33: Verify consistent usage of mock API across tests
The addition of MockableSamgongustofaPaymentCatalogApi is good for testing. Let's verify its usage pattern across the codebase.
#!/bin/bash
# Description: Check usage pattern of MockableSamgongustofaPaymentCatalogApi
# Find all files using the mock API
rg -l "MockableSamgongustofaPaymentCatalogApi"
# Check if it's properly used in test files
rg -l "MockableSamgongustofaPaymentCatalogApi" -g "*test*"
rg -l "MockableSamgongustofaPaymentCatalogApi" -g "*spec*"39-39: Verify all usages of replaced hasReviewerApproved function
The replacement of hasReviewerApproved with canReviewerApprove indicates a shift in approval logic. Let's ensure all usages are updated.
#!/bin/bash
# Description: Check for any remaining usages of hasReviewerApproved
# Search for any remaining instances of the old function
rg "hasReviewerApproved"
# Check the implementation of the new function
ast-grep --pattern 'function canReviewerApprove($_) {
$$$
}'149-149: Verify mock API configuration across roles
The mock API is added to the APPLICANT role. Let's verify if other roles need similar access for testing purposes.
✅ Verification successful
Based on the search results, I can see that both SamgongustofaPaymentCatalogApi and MockableSamgongustofaPaymentCatalogApi are consistently used together in templates that require payment functionality, specifically in:
- TransferOfVehicleOwnershipTemplate
- ChangeOperatorOfVehicleTemplate
- ChangeCoOwnerOfVehicleTemplate
While other templates only use SamgongustofaPaymentCatalogApi:
- OrderVehicleRegistrationCertificateTemplate
- OrderVehicleLicensePlateTemplate
- LicensePlateRenewalTemplate
- DigitalTachographWorkshopCardTemplate
- DigitalTachographCompanyCardTemplate
- DigitalTachographDriversCardTemplate
Mock API configuration is correctly applied
The mock API is only added to templates that require testing of complex ownership transfer scenarios, which is consistent with the codebase's pattern. Other templates using only the real API don't need the mock version for testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check role configurations for payment catalog API usage
# Search for role configurations using payment catalog APIs
rg -B 5 -A 5 "SamgongustofaPaymentCatalogApi" -g "*Template.ts"
Length of output: 32179
libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/ChangeOperatorOfVehicleTemplate.ts (3)
29-29: LGTM: Mock API import follows established patterns
The addition of MockableSamgongustofaPaymentCatalogApi follows TypeScript best practices and aligns with the testing requirements mentioned in the PR objectives.
Line range hint 65-77: LGTM: Improved reviewer action visibility
The change from checking if a reviewer has approved to checking if they can approve aligns with the PR objectives to improve the visibility of actions for reviewers. The implementation maintains proper type safety and provides appropriate warning/info status messages.
34-34: Verify the implementation of utility functions
The imported utility functions enhance code reusability. However, we should verify their implementation to ensure they handle all edge cases correctly.
✅ Verification successful
Let me try a different approach to locate these utility functions.
Let me try to read the content of these specific utility files directly.
Utility functions are well-implemented with proper edge case handling
The utility functions have been reviewed and found to be properly implemented:
canReviewerApprove: Correctly handles authorization checks for both co-owners and new operators, ensuring they can only approve once.canReviewerReApprove: Provides a safety mechanism for stuck applications with proper authorization checks.isRemovingOperatorOnly: Properly validates the removal-only scenario by checking co-owners, new operators, and removal status.
All functions include appropriate type checking, null safety, and edge case handling through array filtering and conditional logic.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of utility functions
# Find and display the implementation of canReviewerApprove
echo "Checking canReviewerApprove implementation:"
ast-grep --pattern 'export function canReviewerApprove($_) {
$$$
}'
# Find and display the implementation of isRemovingOperatorOnly
echo "Checking isRemovingOperatorOnly implementation:"
ast-grep --pattern 'export function isRemovingOperatorOnly($_) {
$$$
}'
Length of output: 349
Script:
#!/bin/bash
# Search for the utility functions in the utils directory relative to the file
cd libs/application/templates/transport-authority/change-operator-of-vehicle/src
echo "Searching for utility functions in utils directory:"
find . -type f -name "*.ts" -exec grep -l "canReviewerApprove\|isRemovingOperatorOnly" {} \;
echo "\nChecking implementation of canReviewerApprove:"
rg "export.*function.*canReviewerApprove" -A 10
echo "\nChecking implementation of isRemovingOperatorOnly:"
rg "export.*function.*isRemovingOperatorOnly" -A 10
Length of output: 741
Script:
#!/bin/bash
echo "Content of canReviewerApprove.ts:"
cat libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts
echo -e "\nContent of isRemovingOperatorOnly.ts:"
cat libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isRemovingOperatorOnly.ts
Length of output: 3566
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts (1)
458-488: LGTM: Vehicle owner change submission
The implementation correctly handles:
- Owner and co-owner information
- Date formatting
- Operator mappings with appropriate comments about email handling
- Insurance company code preservation
...on/templates/transport-authority/order-vehicle-license-plate/src/utils/getSelectedVehicle.ts
Show resolved
Hide resolved
...s/transport-authority/order-vehicle-registration-certificate/src/utils/getSelectedVehicle.ts
Show resolved
Hide resolved
...ion/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts
Outdated
Show resolved
Hide resolved
...tion/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts
Show resolved
Hide resolved
...tion/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts
Show resolved
Hide resolved
...ation/templates/transport-authority/change-co-owner-of-vehicle/src/fields/Overview/index.tsx
Show resolved
Hide resolved
...ation/templates/transport-authority/change-operator-of-vehicle/src/fields/Overview/index.tsx
Show resolved
Hide resolved
...on/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Overview/index.tsx
Show resolved
Hide resolved
...mplates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts
Show resolved
Hide resolved
...mplates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
Show resolved
Hide resolved
.../templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts
Show resolved
Hide resolved
.../templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts
Show resolved
Hide resolved
...ication/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts
Outdated
Show resolved
Hide resolved
...cation/templates/transport-authority/transfer-of-vehicle-ownership/src/lib/messages/steps.ts
Show resolved
Hide resolved
...tion/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts
Show resolved
Hide resolved
Add validation per plate if user has more than 5
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (11)
libs/application/templates/transport-authority/license-plate-renewal/src/utils/checkCanRenew.ts (1)
3-6: Consider adding JSDoc documentation.Since this is a utility function in a library that could be reused across different parts of the application, adding comprehensive documentation would improve its usability.
Add JSDoc documentation:
+/** + * Checks if a license plate can be renewed based on its end date. + * A plate can be renewed if its end date is within the next 3 months. + * + * @param plate - The plate ownership information containing the end date + * @returns true if the plate can be renewed, false otherwise + */ export const checkCanRenew = (plate?: PlateOwnership | null): boolean => {libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyQuery.ts (2)
18-21: Add cache configuration optionsThe query execution could benefit from configurable cache policies to optimize performance and data freshness.
client.query<TData, TVariables>({ query, variables, + fetchPolicy?: 'cache-first' | 'network-only' | 'cache-and-network', })
8-13: Enhance type safety for query result shapeConsider adding runtime type validation for the query result to ensure it matches the expected TypeScript type.
You could integrate a schema validation library like Zod or io-ts to validate the query results at runtime. This would provide an additional layer of type safety beyond compile-time checks.
libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyPlateDetails.ts (1)
6-6: Add JSDoc documentation for better developer experience.Consider adding JSDoc documentation to describe the hook's purpose, parameters, and return type. This would improve IDE support and make the hook more maintainable.
+/** + * Hook for lazy loading plate ownership details by registration number. + * @returns {Object} Query function and result for plate ownership checks + */ export const useLazyPlateDetails = () => {libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/error.ts (1)
1-8: Consider adding explicit TypeScript types for better maintainability.While the current implementation works well with react-intl's implicit typing, adding explicit types would improve maintainability and provide better IDE support.
Consider adding types like this:
import { defineMessages } from 'react-intl' type ErrorMessageKey = 'requiredValidPlate' | 'errorDataProvider' | 'validationAlertTitle' | 'plateOwnershipEmptyList' export const error = defineMessages<ErrorMessageKey>({ requiredValidPlate: { id: 'ta.lpr.application:error.requiredValidPlate', defaultMessage: 'Einkamerki þarf að vera gilt', description: 'Error message if the plate chosen is invalid or not chosen', }, // ... other messages })libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx (2)
42-44: Improve variable naming for clarity.The
disabledvariable combines two different checks but its name doesn't clearly indicate both conditions. Consider a more descriptive name.-const canRenew = checkCanRenew(plate) -const disabled = !!plate?.validationErrorMessages?.length || !canRenew +const canRenew = checkCanRenew(plate) +const isDisabledDueToValidationOrRenewal = !!plate?.validationErrorMessages?.length || !canRenew
Line range hint
45-89: Consider extracting plate option rendering logic to a separate component.The plate option rendering logic is complex and could be extracted into a reusable component to improve maintainability and align with the reusability guidelines for
libs/**/*files.Consider creating a new component like this:
interface PlateOptionProps { plate: PlateOwnership; canRenew: boolean; isDisabled: boolean; formatMessage: (message: any) => string; formatDateFns: (date: Date, format: string) => string; } const PlateOption: FC<PlateOptionProps> = ({ plate, canRenew, isDisabled, formatMessage, formatDateFns, }) => ( <Box display="flex" flexDirection="column"> {/* Move the existing JSX here */} </Box> );This would make the code more maintainable and reusable across different NextJS apps.
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts (3)
30-31: Optimize the totalRecords calculation using optional chainingThe current calculation can be simplified for better readability.
-const totalRecords = (result && result.length) || 0 +const totalRecords = result?.length ?? 0🧰 Tools
🪛 Biome
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
60-82: Add JSDoc documentation for better maintainabilityThe method is well-implemented, but would benefit from documentation explaining its parameters and return value.
+/** + * Maps a plate ownership to a structured object with optional validation + * @param auth - The authenticated user + * @param plateOwnership - The plate ownership to map + * @param fetchExtraData - Whether to perform additional validation + * @returns Mapped ownership with optional validation messages + */ private async mapPlateOwnership( auth: User, plateOwnership: PlateOwnership, fetchExtraData: boolean, )
Line range hint
85-144: Consider adding error message logging for debuggingPer the PR objectives, error messages should be saved in the externalData field for debugging purposes. Consider adding this functionality to the submitApplication method.
async submitApplication({ application, auth, currentUserLocale, }: TemplateApiModuleActionProps): Promise<void> { const age = info(auth.nationalId).age if (age < 65 && application.state === 'draft') { return } await this.handlePayment({ application, auth, currentUserLocale, }) const answers = application.answers as LicensePlateRenewalAnswers const regno = answers?.pickPlate?.regno - // Submit the application - await this.vehiclePlateRenewalClient.renewPlateOwnership(auth, regno) + try { + // Submit the application + await this.vehiclePlateRenewalClient.renewPlateOwnership(auth, regno) + } catch (error) { + // Save error message for debugging + application.externalData = { + ...application.externalData, + submissionError: error.message + } + throw error + } }libs/application/template-api-modules/src/lib/modules/templates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts (1)
Line range hint
61-115: Define and export TypeScript interfaces for return typesTo improve type safety and reusability, consider defining and exporting TypeScript interfaces for the return types of
getCurrentVehiclesWithPlateOrderChecksandmapVehicle. This will enhance clarity and make the codebase more maintainable.Apply this diff to define interfaces and specify return types:
+export interface VehicleInfo { + permno?: string; + make?: string; + color?: string; + role?: string; + validationErrorMessages: string[] | null; +} + +export interface VehiclesResponse { + totalRecords: number; + vehicles: VehicleInfo[]; +} + export class OrderVehicleLicensePlateService extends BaseTemplateApiService { // existing code... async getCurrentVehiclesWithPlateOrderChecks({ auth, }: TemplateApiModuleActionProps): Promise<VehiclesResponse> { // method body... } private async mapVehicle( auth: User, vehicle: CurrentVehiclesWithMilageAndNextInspDto, fetchExtraData: boolean, ): Promise<VehicleInfo> { // method body... } }Also applies to: 117-149
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts(3 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts(3 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts(3 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts(3 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts(3 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx(2 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx(4 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/graphql/queries.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyPlateDetails.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyQuery.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/lib/LicensePlateRenewalTemplate.ts(0 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/error.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/shared/constants.ts(0 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/utils/checkCanRenew.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/utils/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- libs/application/templates/transport-authority/license-plate-renewal/src/lib/LicensePlateRenewalTemplate.ts
- libs/application/templates/transport-authority/license-plate-renewal/src/shared/constants.ts
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/transport-authority/license-plate-renewal/src/graphql/queries.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-co-owner-of-vehicle/change-co-owner-of-vehicle.service.ts
- libs/application/template-api-modules/src/lib/modules/templates/transport-authority/change-operator-of-vehicle/change-operator-of-vehicle.service.ts
🧰 Additional context used
📓 Path-based instructions (10)
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.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/application/template-api-modules/src/lib/modules/templates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.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/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.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/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx (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/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx (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/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyPlateDetails.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/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyQuery.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/application/templates/transport-authority/license-plate-renewal/src/lib/messages/error.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/application/templates/transport-authority/license-plate-renewal/src/utils/checkCanRenew.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/application/templates/transport-authority/license-plate-renewal/src/utils/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."
🪛 Biome
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (10)
libs/application/templates/transport-authority/license-plate-renewal/src/utils/checkCanRenew.ts (1)
1-2: LGTM!
The TypeScript type import is correctly defined and used.
libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyPlateDetails.ts (2)
1-4: LGTM! Imports are well-structured for optimal tree-shaking.
The imports are properly organized with named imports, which enables effective tree-shaking during bundling.
6-19: LGTM! Well-typed and reusable hook implementation.
The hook is properly typed with GraphQL schema types and follows a clean, reusable pattern that can be used across different NextJS apps.
libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/error.ts (1)
4-8: LGTM! Well-structured error message definition.
The new error message follows the established pattern and includes all required fields with clear descriptions.
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx (1)
17-17: LGTM! Clean utility function import.
The import of the extracted checkCanRenew utility function follows TypeScript conventions and improves code organization.
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts (2)
15-15: LGTM: Clean import addition
The User type import is appropriately added and will be used for type safety in the new method parameters.
43-58: LGTM: Smart optimization for large datasets
The implementation intelligently handles plate ownership validation by:
- Deferring validation for large lists (> 5 items) until selection
- Immediate validation for smaller lists
- Using Promise.all for concurrent processing
This approach optimizes performance while maintaining a good user experience.
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts (2)
Line range hint 106-127: LGTM! Clear and well-documented logic for vehicle count cases.
The comments effectively explain the UI behavior for each vehicle count scenario:
- count > 20: Display search box
- 20 >= count > 5: Display dropdown
- count <= 5: Display radio buttons
606-618: Previous review comment is still valid.
The error handling needs to be enhanced to differentiate between severity levels (E, L, W) as mentioned in the PR objectives.
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx (1)
130-131: Verify the logic for 'variant' property in 'ActionCard'
The variant prop is set to 'red' when canRenew is true. Typically, 'red' indicates an error or warning state. Please verify that this reflects the intended design. If canRenew means the plate can be renewed, you might want to invert the logic.
libs/application/templates/transport-authority/license-plate-renewal/src/utils/checkCanRenew.ts
Show resolved
Hide resolved
libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyQuery.ts
Show resolved
Hide resolved
libs/application/templates/transport-authority/license-plate-renewal/src/hooks/useLazyQuery.ts
Show resolved
Hide resolved
libs/application/templates/transport-authority/license-plate-renewal/src/utils/index.ts
Show resolved
Hide resolved
...s/transport-authority/transfer-of-vehicle-ownership/transfer-of-vehicle-ownership.service.ts
Show resolved
Hide resolved
...mplates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx
Show resolved
Hide resolved
...mplates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx
Outdated
Show resolved
Hide resolved
...mplates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx
Outdated
Show resolved
Hide resolved
...lates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts
Outdated
Show resolved
Hide resolved
...lates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts (1)
5-39: LGTM! The implementation is robust and well-structured.The function correctly handles both co-owners and operators approval checks with proper type safety and error handling.
Consider extracting the duplicate approval check logic into a reusable helper function:
+const hasUnapprovedUsers = ( + users: { nationalId: string; approved?: boolean }[], + excludeNationalId?: string, +): boolean => + users.some( + ({ nationalId, approved }) => + (!excludeNationalId || nationalId !== excludeNationalId) && !approved, + ); export const applicationHasPendingApproval = ( answers: FormValue, excludeNationalId?: string, ): boolean => { const coOwners = getValueViaPath( answers, 'ownerCoOwner', [], ) as UserInformation[] - if ( - coOwners.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { + if (hasUnapprovedUsers(coOwners, excludeNationalId)) { return true } const newOperators = ( getValueViaPath(answers, 'operators', []) as OperatorInformation[] ).filter(({ wasRemoved }) => wasRemoved !== 'true') - if ( - newOperators.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { + if (hasUnapprovedUsers(newOperators, excludeNationalId)) { return true } return false }libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts (2)
5-39: Consider refactoring duplicate approval checks and enhancing TypeScript usage.The function logic is clear and well-structured, but could be improved for maintainability and type safety.
Consider these improvements:
+type CoOwner = { nationalId: string; approved: boolean; wasRemoved?: string } + +const hasUnapprovedCoOwner = ( + coOwners: CoOwner[], + excludeNationalId?: string +): boolean => + coOwners.some( + ({ nationalId, approved }) => + (!excludeNationalId || nationalId !== excludeNationalId) && !approved + ) + export const applicationHasPendingApproval = ( answers: FormValue, - excludeNationalId?: string, + excludeNationalId?: string ): boolean => { const oldCoOwners = getValueViaPath( answers, 'ownerCoOwners', [], ) as OwnerCoOwnersInformation[] - if ( - oldCoOwners.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { + if (hasUnapprovedCoOwner(oldCoOwners, excludeNationalId)) { return true } const newCoOwners = ( getValueViaPath(answers, 'coOwners', []) as CoOwnersInformation[] ).filter(({ wasRemoved }) => wasRemoved !== 'true') - if ( - newCoOwners.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { + if (hasUnapprovedCoOwner(newCoOwners, excludeNationalId)) { return true } return false }
41-50: LGTM with a minor TypeScript enhancement.The function is well-documented and follows single responsibility principle.
Consider adding an explicit return type:
export const isLastReviewer = ( reviewerNationalId: string, answers: FormValue, -): boolean => { +): boolean => {libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx (3)
49-51: Consider consolidating duplicate state managementThe
platestate appears to duplicate information already available inselectedPlate. Consider deriving the plate value fromselectedPlateinstead of maintaining a separate state.- const [plate, setPlate] = useState<string>( - getValueViaPath(application.answers, 'pickPlate.regno', '') as string, - ) + const plate = selectedPlate?.regno || ''
130-131: Address TODO comment in tag configurationThere's a TODO comment regarding the disabled property in the tag configuration. This should be addressed or removed if no longer relevant.
160-162: Improve type safety in error handlingThe type casting of errors could be replaced with proper TypeScript types.
+interface FormErrors { + pickPlate?: { + message: string; + }; +} -{!isLoading && plate.length === 0 && (errors as any)?.pickPlate && ( +{!isLoading && plate.length === 0 && (errors as FormErrors)?.pickPlate && (
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx(4 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/canReviewerApprove.ts
- libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/canReviewerApprove.ts
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/canReviewerApprove.ts
- libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.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/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.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/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx (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."
🔇 Additional comments (2)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts (2)
41-49: LGTM! Clear and concise implementation.
The function provides a simple and intuitive way to check if the current reviewer is the last one needed for approval. The implementation is well-documented and follows the single responsibility principle.
Line range hint 1-49: Excellent adherence to coding guidelines.
The implementation follows all required guidelines for libs/**/*:
- ✓ Functions are reusable across different NextJS apps
- ✓ TypeScript types are properly defined and imported
- ✓ Code structure supports effective tree-shaking
...mplates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (32)
libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/index.ts (1)
7-7: Consider tree-shaking impactSince this is in the libs directory, ensure that the applicationCheck module follows tree-shaking best practices by avoiding side effects and using named exports.
Consider these recommendations:
- Use named exports instead of default exports in applicationCheck.ts
- Avoid any side effects in the module
- Consider splitting large message objects if they're not always used together
libs/application/templates/transport-authority/order-vehicle-license-plate/src/index.ts (1)
Line range hint
1-13: LGTM! File structure follows best practicesThe barrel file structure:
- Facilitates effective tree-shaking
- Maintains clean module boundaries
- Provides clear TypeScript exports
Consider grouping related message exports under a single messages namespace for better organization as the number of message modules grows.
libs/api/domains/transport-authority/src/lib/graphql/models/plateOrderValidation.model.ts (2)
1-10: LGTM! Consider adding JSDoc comments.The
PlateOrderValidationMessageclass is well-structured with proper TypeScript types and GraphQL decorators. The nullable fields are correctly defined.Consider adding JSDoc comments to document the purpose of each field:
@ObjectType() export class PlateOrderValidationMessage { + /** Error code from the validation system */ @Field(() => String, { nullable: true }) errorNo?: string | null + /** Human-readable error message */ @Field(() => String, { nullable: true }) defaultMessage?: string | null }
12-19: LGTM! Consider type safety enhancement.The
PlateOrderValidationclass is well-structured with proper field definitions. The non-nullablehasErrorfield ensures validation state is always available.Consider making the array non-nullable but empty when there are no messages:
@ObjectType() export class PlateOrderValidation { @Field(() => Boolean) hasError!: boolean - @Field(() => [PlateOrderValidationMessage], { nullable: true }) - errorMessages?: PlateOrderValidationMessage[] | null + @Field(() => [PlateOrderValidationMessage]) + errorMessages: PlateOrderValidationMessage[] = [] }This change would:
- Improve type safety by eliminating null checks
- Simplify consuming code by guaranteeing an array
- Follow the principle of "null object pattern"
libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/applicationCheck.ts (2)
1-2: Add type imports from react-intlTo improve type safety and follow the coding guidelines for TypeScript usage, consider importing the message definition types.
-import { defineMessages } from 'react-intl' +import { defineMessages, MessageDescriptor } from 'react-intl'
3-16: Consider enhancing type safety and reusabilityWhile the message structure is good, we can improve type safety and reusability across NextJS apps by:
- Adding explicit TypeScript types
- Exporting message keys as a type for consumers
+type ApplicationCheckMessages = { + validation: { + alertTitle: MessageDescriptor + fallbackErrorMessage: MessageDescriptor + } +} +export type ApplicationCheckMessageKey = keyof ApplicationCheckMessages['validation'] + -export const applicationCheck = { +export const applicationCheck: ApplicationCheckMessages = { validation: defineMessages({ alertTitle: { id: 'ta.lpr.application:applicationCheck.validation.alertTitle', defaultMessage: 'Það kom upp villa', description: 'Application check validation alert title', }, fallbackErrorMessage: { id: 'ta.lpr.application:applicationCheck.validation.fallbackErrorMessage', defaultMessage: 'Það kom upp villa við að sannreyna gögn', description: 'Fallback error message for validation', }, }), }This change:
- Ensures type safety when accessing messages
- Makes it easier for other components to reference valid message keys
- Improves maintainability through explicit typing
- Facilitates better tree-shaking through explicit exports
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/index.ts (1)
Line range hint
1-24: Confirm component reusability and type safety.The implementation follows the library's coding guidelines:
- Uses TypeScript for type safety
- Maintains reusable form structure using core building blocks
- Follows proper module organization for effective tree-shaking
Consider documenting the form structure in a README.md file to help other teams reuse this template effectively.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/dataProviders/index.ts (1)
15-21: Consider adding JSDoc documentationThe implementation of
MockableSamgongustofaPaymentCatalogApiis well-structured and mirrors the configuration of the real API, which is great for testing. However, adding JSDoc documentation would help other developers understand when to use this mock version versus the real API.+/** + * Mockable version of SamgongustofaPaymentCatalogApi for testing purposes. + * Configured with the same parameters as the real API. + */ export const MockableSamgongustofaPaymentCatalogApi = MockablePaymentCatalogApi.configure({ params: {libs/application/templates/transport-authority/order-vehicle-license-plate/src/graphql/queries.ts (1)
36-46: Consider adding TypeScript type definitions for the query response.The query structure looks good and aligns well with the PR objectives for handling warning severity messages. However, to improve type safety and developer experience, consider adding TypeScript type definitions for the response structure.
Add these TypeScript interfaces:
interface ValidationErrorMessage { errorNo: string; defaultMessage: string; } interface VehiclePlateOrderValidationResponse { hasError: boolean; errorMessages: ValidationErrorMessage[]; } interface ValidateVehiclePlateOrderResponse { vehiclePlateOrderValidation: VehiclePlateOrderValidationResponse; }libs/api/domains/transport-authority/src/lib/graphql/dto/plateOrderAnswers.input.ts (1)
19-28: Maintain consistent naming convention across related DTOsThe class name
OperatorChangeAnswersPlateDeliverybreaks the naming pattern established by other DTOs. Consider renaming it toPlateOrderAnswersDeliveryfor consistency.-export class OperatorChangeAnswersPlateDelivery { +export class PlateOrderAnswersDelivery {libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/prerequisitesSection.ts (1)
38-41: Consider adding a descriptive title for the mock data provider.While the implementation correctly follows the pattern from the retrieved learning by including both the real and mock APIs, having an empty title for both providers might make it harder to distinguish between them during testing and debugging.
Consider applying this change:
buildDataProviderItem({ provider: MockableSamgongustofaPaymentCatalogApi, - title: '', + title: 'Mock Payment Catalog', }),libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/payment.ts (1)
10-14: Improve message descriptions for clarityThe descriptions could be more specific to help translators:
pageTitle: "Title of for payment page" has a typo and could be clearerforPayment: "For payment label" is too generictotal: "Total amount label" could provide more context about what it totalsConsider this improvement:
pageTitle: { id: 'ta.ovlp.application:payment.general.pageTitle', defaultMessage: 'Greiðsla', - description: 'Title of for payment page', + description: 'Title displayed at the top of the vehicle license plate payment page', }, forPayment: { id: 'ta.ovlp.application:payment.paymentChargeOverview.forPayment', defaultMessage: 'Til greiðslu', - description: 'For payment label', + description: 'Label indicating the amount due for payment in the charge overview', }, total: { id: 'ta.ovlp.application:payment.paymentChargeOverview.total', defaultMessage: 'Samtals', - description: 'Total amount label', + description: 'Label for the total sum of all license plate charges', },Also applies to: 22-26, 27-31
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/paymentSection.ts (2)
1-15: Add TypeScript type annotations for builder functionsConsider adding explicit TypeScript types for the imported builder functions to improve type safety and documentation.
+import type { BuilderFunction, Section, Field } from '@island.is/application/core' import { buildSection, buildCustomField, buildMultiField, buildSubmitField, buildPaymentChargeOverviewField, } from '@island.is/application/core'
20-36: Document the ValidationErrorMessages componentThe custom ValidationErrorMessages component's purpose and behavior should be documented for better maintainability.
buildCustomField({ id: 'ValidationErrorMessages', title: '', component: 'ValidationErrorMessages', + // Component that displays validation error messages from form submission + // Used to show SGS system warnings (E, L, W) as mentioned in PR objectives }),libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts (2)
15-15: Consider improving type safetyThe
anytype for the error parameter reduces type safety. Consider creating a specific error type or using a more specific type union.interface SGSError { body?: { Errors?: ReturnTypeMessage[]; } | ReturnTypeMessage[]; } export const getCleanErrorMessagesFromTryCatch = (e: SGSError): ErrorMessage[] => {
31-33: Consider extracting warning severity constantsThe warning severity strings could be moved to named constants at the module level for better maintainability and reusability.
export const WARNING_SEVERITY = { ERROR: 'E', LOCK: 'L', WARNING: 'W', } as const;libs/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts (2)
74-97: Consider extracting common plate ownership logic.The try-catch and error handling logic is duplicated between
validatePlateOwnershipandrenewPlateOwnership. Consider extracting this into a private method.Here's a suggested refactor:
+ private async processPlateOwnership( + auth: User, + regno: string, + isValidationOnly: boolean + ): Promise<PlateOwnershipValidation> { + let errorMessages: ErrorMessage[] | undefined + + try { + await this.plateOwnershipApiWithAuth(auth).renewplateownershipPost({ + apiVersion: '1.0', + apiVersion2: '1.0', + postRenewPlateOwnershipModel: { + regno: regno, + persidno: auth.nationalId, + check: isValidationOnly, + }, + }) + } catch (e) { + errorMessages = getCleanErrorMessagesFromTryCatch(e) + } + + return { + hasError: !!errorMessages?.length, + errorMessages: errorMessages, + } + } public async validatePlateOwnership( auth: User, regno: string, ): Promise<PlateOwnershipValidation> { - let errorMessages: ErrorMessage[] | undefined - - try { - await this.plateOwnershipApiWithAuth(auth).renewplateownershipPost({ - apiVersion: '1.0', - apiVersion2: '1.0', - postRenewPlateOwnershipModel: { - regno: regno, - persidno: auth.nationalId, - check: true, - }, - }) - } catch (e) { - errorMessages = getCleanErrorMessagesFromTryCatch(e) - } - - return { - hasError: !!errorMessages?.length, - errorMessages: errorMessages, - } + return this.processPlateOwnership(auth, regno, true) } public async renewPlateOwnership( auth: User, regno: string, ): Promise<PlateOwnershipValidation> { - let errorMessages: ErrorMessage[] | undefined - - try { - await this.plateOwnershipApiWithAuth(auth).renewplateownershipPost({ - apiVersion: '1.0', - apiVersion2: '1.0', - postRenewPlateOwnershipModel: { - regno: regno, - persidno: auth.nationalId, - check: false, - }, - }) - } catch (e) { - errorMessages = getCleanErrorMessagesFromTryCatch(e) - } - - return { - hasError: !!errorMessages?.length, - errorMessages: errorMessages, - } + return this.processPlateOwnership(auth, regno, false) }
Severity Handling Not Fully Implemented.
The current implementation does not define or handle different warning severities (ERROR, LOCK, WARNING) as required. Please implement an enum for severity levels and adjust the error handling to display the first Error/Lock message or a Warning if none exist.
🔗 Analysis chain
Line range hint
50-70: Verify warning severity handling requirements.Based on the PR objectives, this method should handle different warning severities (E, L, W) and display:
- First Error (E) or Lock (L) message if present
- Warning (W) message only if no E/L messages exist
The current implementation might not fully satisfy these requirements.
Let's verify the error handling implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for warning severity handling in the error handling utility rg -A 10 "getCleanErrorMessagesFromTryCatch" # Look for severity-related types or enums ast-grep --pattern 'enum $_ { $$$ ERROR $$$ LOCK $$$ WARNING $$$ $$$ }'Length of output: 19258
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx (1)
Line range hint
19-45: Consider extracting validation logic for better reusabilityWhile the component is well-structured, the validation logic could be more reusable across different NextJS applications within the transport authority domain.
Consider:
- Moving the validation logic to a shared hook (e.g.,
useVehicleValidation)- Creating a generic plate validation interface that can be implemented by different vehicle types
- Extracting the error display logic into a reusable component
This would align better with the coding guidelines for libs/** files regarding reusability across different NextJS apps.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/ValidationErrorMessages/index.tsx (2)
17-19: Add JSDoc documentation for better type clarity.Consider adding JSDoc documentation to describe the purpose of the Props interface and its properties, especially explaining when
setValidationErrorFoundis called and what it does.+/** + * Props for ValidationErrorMessages component + * @property setValidationErrorFound - Callback to update parent component when validation errors are found + */ interface Props { setValidationErrorFound?: Dispatch<SetStateAction<boolean>> }
30-60: Consider extracting query variables and improving error handling.The query setup could be improved for better maintainability and error handling:
+ const queryVariables = { + answers: { + pickVehicle: { + plate: answers?.pickVehicle?.plate, + }, + plateSize: { + frontPlateSize: answers?.plateSize?.frontPlateSize, + rearPlateSize: answers?.plateSize?.rearPlateSize, + }, + plateDelivery: { + deliveryMethodIsDeliveryStation: + answers?.plateDelivery?.deliveryMethodIsDeliveryStation, + deliveryStationTypeCode: + answers?.plateDelivery?.deliveryStationTypeCode, + includeRushFee: answers?.plateDelivery?.includeRushFee, + }, + }, + } const { data, loading } = useQuery( gql` ${VALIDATE_VEHICLE_PLATE_ORDER} `, { - variables: { - answers: { - // ... current structure - }, - }, + variables: queryVariables, onCompleted: (data) => { if (data?.vehiclePlateOrderValidation?.hasError) { setValidationErrorFound?.(true) } }, + onError: (error) => { + console.error('Validation query failed:', error) + setValidationErrorFound?.(true) + }, fetchPolicy: 'no-cache', }, )libs/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.ts (3)
41-62: Consider extracting dummy values as named constants.While the dummy values are well-documented with comments, consider extracting them as named constants at the class or module level for better maintainability and reusability.
+ private static readonly DEFAULT_EXPRESS_ORDER = false; public async validateVehicleForPlateOrder( auth: User, permno: string, frontType: string, rearType: string, ): Promise<PlateOrderValidation> { // Note: option "Pick up at Samgöngustofa" which is always valid const deliveryStationType = SGS_DELIVERY_STATION_TYPE; const deliveryStationCode = SGS_DELIVERY_STATION_CODE; - const expressOrder = false; + const expressOrder = VehiclePlateOrderingClient.DEFAULT_EXPRESS_ORDER;
64-99: Consider enabling strict null checks for better type safety.The null coalescing operators (
||) for optional parameters could be replaced with TypeScript's nullish coalescing operator (??) for better type safety.- frontType: frontType || null, - rearType: rearType || null, - stationToDeliverTo: deliveryStationCode || '', - stationType: deliveryStationType || '', + frontType: frontType ?? null, + rearType: rearType ?? null, + stationToDeliverTo: deliveryStationCode ?? '', + stationType: deliveryStationType ?? '',
101-128: Maintain consistent error handling pattern.The error handling implementation is consistent with validateAllForPlateOrder, which is good. However, the same type safety improvements suggested above should be applied here as well.
- frontType: plateOrder.frontType || null, - rearType: plateOrder.rearType || null, - stationToDeliverTo: plateOrder.deliveryStationCode || '', - stationType: plateOrder.deliveryStationType || '', + frontType: plateOrder.frontType ?? null, + rearType: plateOrder.rearType ?? null, + stationToDeliverTo: plateOrder.deliveryStationCode ?? '', + stationType: plateOrder.deliveryStationType ?? '',libs/api/domains/transport-authority/src/lib/graphql/main.resolver.ts (1)
128-138: Documentation update needed for the new query.Please ensure that the new
vehiclePlateOrderValidationquery is properly documented in the API specifications.Would you like me to help create a documentation template for this new query endpoint?
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts (1)
33-34: Simplify array length check using optional chaining.The current implementation can be more concise.
-const totalRecords = (result && result.length) || 0 +const totalRecords = result?.length ?? 0🧰 Tools
🪛 Biome
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx (1)
162-164: Improve type safety for error handlingUsing type assertions (
as any) should be avoided when possible. Consider defining proper types for the errors object.Consider defining an interface for the errors object:
interface FormErrors { pickPlate?: { message: string; type: string; }; }Then update the implementation:
- {!isLoading && plate.length === 0 && (errors as any)?.pickPlate && ( + {!isLoading && plate.length === 0 && (errors as FormErrors)?.pickPlate && (libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/VehiclesField/VehicleSelectField.tsx (2)
104-106: Consider enhancing the loading state management.While the implementation is functional, consider these improvements:
- Add a cleanup function to handle component unmounting during loading
- Extract this logic into a custom hook for reusability
useEffect(() => { setFieldLoadingState?.(isLoading) + return () => { + setFieldLoadingState?.(false) + } }, [isLoading])Alternative approach using a custom hook:
function useLoadingState( isLoading: boolean, onLoadingChange?: (loading: boolean) => void ) { useEffect(() => { onLoadingChange?.(isLoading) return () => { onLoadingChange?.(false) } }, [isLoading, onLoadingChange]) }
Line range hint
1-172: Consider architectural improvements for better reusability.As this component is in the
libsdirectory, consider these architectural improvements:
- Extract vehicle validation logic into a separate service
- Create a dedicated type for validation error messages
- Consider using a reducer pattern for complex state management
This would improve:
- Reusability across different NextJS apps
- Testing capabilities
- Maintenance of validation logic
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts (2)
64-74: Extract magic numbers into named constantsThe numbers 5 and 20 are used as thresholds for different UI behaviors. Consider extracting these into named constants to improve code maintainability and make the logic more self-documenting.
+const VEHICLE_LIST_DROPDOWN_THRESHOLD = 5; +const VEHICLE_LIST_SEARCH_THRESHOLD = 20; async getCurrentVehiclesWithPlateOrderChecks({ auth, }: TemplateApiModuleActionProps) { const result = await this.vehiclesApiWithAuth( auth, ).currentvehicleswithmileageandinspGet({ showOwned: true, showCoowned: false, showOperated: false, page: 1, - pageSize: 20, + pageSize: VEHICLE_LIST_SEARCH_THRESHOLD, }) // ... - if (totalRecords > 20) { + if (totalRecords > VEHICLE_LIST_SEARCH_THRESHOLD) { return { totalRecords: totalRecords, vehicles: [], } } // ... - if (totalRecords > 5) { + if (totalRecords > VEHICLE_LIST_DROPDOWN_THRESHOLD) { return this.mapVehicle(auth, vehicle, false) }Also applies to: 91-96, 104-106
204-212: Enhance error message handlingCurrently, the error messages from validation are not being used in the error response. Consider including the specific error messages in the response summary for better user feedback.
if (result.hasError && result.errorMessages?.length) { throw new TemplateApiError( { title: applicationCheck.validation.alertTitle, - summary: applicationCheck.validation.alertTitle, + summary: result.errorMessages.join('. '), }, 400, ) }libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (1)
118-118: Consider consolidating duplicate commentsThe same comment about subModel appears in multiple places. Consider moving this documentation to the
BasicVehicleInformationtype definition or creating a shared constant with the comment.Also applies to: 283-283
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (37)
libs/api/domains/transport-authority/src/lib/graphql/dto/index.ts(1 hunks)libs/api/domains/transport-authority/src/lib/graphql/dto/plateOrderAnswers.input.ts(1 hunks)libs/api/domains/transport-authority/src/lib/graphql/main.resolver.ts(3 hunks)libs/api/domains/transport-authority/src/lib/graphql/models/index.ts(1 hunks)libs/api/domains/transport-authority/src/lib/graphql/models/plateOrderValidation.model.ts(1 hunks)libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts(6 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts(5 hunks)libs/application/template-api-modules/src/lib/modules/templates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts(4 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/dataProviders/index.ts(2 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx(4 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx(5 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/prerequisitesSection.ts(2 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/index.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/lib/LicensePlateRenewalTemplate.ts(2 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/applicationCheck.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/index.ts(1 hunks)libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/information.ts(0 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/dataProviders/index.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/ValidationErrorMessages/index.tsx(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/VehiclesField/VehicleSelectField.tsx(3 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/index.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/index.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/paymentSection.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/prerequisitesSection.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/graphql/queries.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/index.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/OrderVehicleLicensePlateTemplate.ts(3 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/applicationCheck.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/index.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/payment.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-license-plate/src/shared/constants.ts(1 hunks)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/dataProviders/index.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/prerequisitesSection.ts(2 hunks)libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/lib/OrderVehicleRegistrationCertificateTemplate.ts(2 hunks)libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts(1 hunks)libs/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.ts(2 hunks)libs/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts(3 hunks)
💤 Files with no reviewable changes (1)
- libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/information.ts
✅ Files skipped from review due to trivial changes (1)
- libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/applicationCheck.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/templates/transport-authority/license-plate-renewal/src/lib/LicensePlateRenewalTemplate.ts
🧰 Additional context used
📓 Path-based instructions (34)
libs/api/domains/transport-authority/src/lib/graphql/dto/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."
libs/api/domains/transport-authority/src/lib/graphql/dto/plateOrderAnswers.input.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/api/domains/transport-authority/src/lib/graphql/main.resolver.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/api/domains/transport-authority/src/lib/graphql/models/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."
libs/api/domains/transport-authority/src/lib/graphql/models/plateOrderValidation.model.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/api/domains/transport-authority/src/lib/transportAuthority.service.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/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.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/application/template-api-modules/src/lib/modules/templates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.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/application/templates/transport-authority/license-plate-renewal/src/dataProviders/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."
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx (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/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx (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/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/prerequisitesSection.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/application/templates/transport-authority/license-plate-renewal/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."
libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/applicationCheck.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/application/templates/transport-authority/license-plate-renewal/src/lib/messages/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."
libs/application/templates/transport-authority/order-vehicle-license-plate/src/dataProviders/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."
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/ValidationErrorMessages/index.tsx (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/application/templates/transport-authority/order-vehicle-license-plate/src/fields/VehiclesField/VehicleSelectField.tsx (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/application/templates/transport-authority/order-vehicle-license-plate/src/fields/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."
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/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."
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/paymentSection.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/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/prerequisitesSection.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/application/templates/transport-authority/order-vehicle-license-plate/src/graphql/queries.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/application/templates/transport-authority/order-vehicle-license-plate/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."
libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/OrderVehicleLicensePlateTemplate.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/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/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."
libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/payment.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/application/templates/transport-authority/order-vehicle-license-plate/src/shared/constants.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/application/templates/transport-authority/order-vehicle-registration-certificate/src/dataProviders/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."
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/prerequisitesSection.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/application/templates/transport-authority/order-vehicle-registration-certificate/src/lib/OrderVehicleRegistrationCertificateTemplate.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/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.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/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.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/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
📓 Learnings (5)
libs/application/templates/transport-authority/license-plate-renewal/src/dataProviders/index.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
libs/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/prerequisitesSection.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/prerequisitesSection.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/dataProviders/index.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/prerequisitesSection.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
🪛 Biome
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts
[error] 33-33: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx
[error] 77-77: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx
[error] 151-151: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts
[error] 6-6: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (46)
libs/application/templates/transport-authority/order-vehicle-license-plate/src/shared/constants.ts (2)
2-2: LGTM! The new API action aligns with validation requirements.
The addition of validateApplication to the ApiActions enum follows TypeScript best practices and maintains consistency with existing patterns.
2-2: Verify consistent usage of the new API action.
Let's ensure the new validateApplication enum value is used consistently across the codebase.
libs/application/templates/transport-authority/license-plate-renewal/src/lib/messages/index.ts (1)
7-7: LGTM! Verify types in applicationCheck module.
The barrel file pattern is correctly used for organizing and re-exporting modules. This promotes clean code organization and module accessibility.
Let's verify the types in the applicationCheck module:
libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/index.ts (1)
7-7: Verify the applicationCheck module integration
The export statement follows the established pattern and aligns with the PR's objective of enhancing warning severity handling.
Let's verify the module existence and its usage:
✅ Verification successful
Verify the applicationCheck module integration
The applicationCheck module exists and is properly integrated with the codebase. The export statement follows the established pattern and aligns with the PR's objective of enhancing warning severity handling.
Consider tree-shaking optimizations
Ensure that the added export does not interfere with tree-shaking optimizations to maintain optimal bundle sizes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify applicationCheck module existence and usage
# Check if the module exists
echo "Checking for applicationCheck.ts module..."
fd --type f "applicationCheck.ts" libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/
# Check for usage of applicationCheck exports
echo "Checking for usage of applicationCheck exports..."
rg -l "applicationCheck" libs/application/templates/transport-authority/order-vehicle-license-plate/src
Length of output: 967
libs/api/domains/transport-authority/src/lib/graphql/dto/index.ts (1)
Line range hint 1-5: Consider explicit exports for better tree-shaking.
While the barrel file pattern promotes reusability, using wildcard exports (export *) might hinder effective tree-shaking. Consider explicitly exporting the required types:
- export * from './ownerChangeAnswers.input'
- export * from './operatorChangeAnswers.input'
- export * from './checkTachoNet.input'
- export * from './plateAvailability.input'
- export * from './plateOrderAnswers.input'
+ export type { OwnerChangeAnswers } from './ownerChangeAnswers.input'
+ export type { OperatorChangeAnswers } from './operatorChangeAnswers.input'
+ export type { CheckTachoNet } from './checkTachoNet.input'
+ export type { PlateAvailability } from './plateAvailability.input'
+ export type { PlateOrderAnswers } from './plateOrderAnswers.input'This approach:
- Maintains the benefits of centralized exports
- Improves tree-shaking by making dependencies explicit
- Provides better visibility of available types
Let's verify the types being exported:
libs/api/domains/transport-authority/src/lib/graphql/models/index.ts (1)
6-6: LGTM! Well-structured export following established patterns.
The new export follows good practices:
- Maintains consistent naming convention with other validation models
- Properly organized in a domain-specific structure
- Supports effective tree-shaking through individual file exports
libs/application/templates/transport-authority/order-vehicle-license-plate/src/index.ts (1)
11-11: Verify the existence and usage of the applicationCheck messages module
The new export aligns with the PR objectives for handling warning severity messages. However, let's verify the module's existence and its integration.
✅ Verification successful
Exported module exists and is properly used
The applicationCheck messages module is correctly exported and utilized across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and usage of applicationCheck messages
# Check if the file exists
echo "Checking for the applicationCheck messages file..."
fd --type f "applicationCheck.ts" "libs/application/templates/transport-authority"
# Find usages of applicationCheck messages
echo "Checking for usages of applicationCheck messages..."
rg -l "applicationCheck" "libs/application/templates/transport-authority"
Length of output: 3962
libs/application/templates/transport-authority/license-plate-renewal/src/dataProviders/index.ts (2)
4-4: LGTM: Import addition is well-placed
The MockablePaymentCatalogApi import is correctly added alongside related imports from @island.is/application/types.
17-23: LGTM: Mockable API configuration matches production API
The MockableSamgongustofaPaymentCatalogApi is correctly configured with the same parameters as the production API, maintaining consistency. This follows the established pattern from previous implementations (ref: PR #16471) where both real and mockable providers should be included.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/dataProviders/index.ts (1)
4-4: LGTM! Clean import addition
The import of MockablePaymentCatalogApi is properly structured and maintains consistency with the existing imports.
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/dataProviders/index.ts (2)
4-4: LGTM! Import follows shared module pattern
The import is correctly added from the shared types package, maintaining reusability across different NextJS applications.
17-23: LGTM! Mockable API correctly configured alongside real API
The implementation follows best practices by:
- Maintaining both real and mockable APIs
- Using identical configuration parameters for consistent behavior
- Properly utilizing TypeScript for type safety
Let's verify that both APIs are used consistently across the codebase:
✅ Verification successful
Verification Successful! Both real and mockable APIs are used consistently across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of both real and mockable APIs
# Expected: Both APIs should be used in test files and provider configurations
# Check for usage of both APIs
echo "Checking usage patterns of both APIs:"
rg -A 2 "SamgongustofaPaymentCatalogApi|MockableSamgongustofaPaymentCatalogApi" \
--type ts \
--glob '!node_modules'
Length of output: 33100
libs/api/domains/transport-authority/src/lib/graphql/dto/plateOrderAnswers.input.ts (2)
1-40: LGTM on TypeScript usage and exports
The code follows TypeScript best practices:
- Proper use of decorators
- Clear type definitions
- Explicit nullability annotations
- Proper export of types
31-40: Review required/optional field consistency in composite type
The composite PlateOrderAnswers marks all fields as required (nullable: false) even though they contain optional fields. This could lead to runtime errors when optional fields are undefined. Consider:
- Making the composite fields nullable to match their content, or
- Making all nested fields required if the business logic requires it
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/forms/OrderVehicleRegistrationCertificateForm/prerequisitesSection.ts (2)
11-11: LGTM! Import statement follows best practices.
The import is properly structured for effective tree-shaking, maintaining consistency with other imports from the same module.
Line range hint 1-46: Verify mock API implementation matches the real API contract.
To ensure reusability across different NextJS apps, we should verify that the mock API implements the same interface as the real API.
libs/application/templates/transport-authority/license-plate-renewal/src/forms/LicensePlateRenewalForm/prerequisitesSection.ts (2)
11-11: LGTM: Clean import addition
The import follows the established pattern and maintains consistency with the module structure.
38-41: Consider adding descriptive titles and verify mock implementation
While the addition of the mock provider aligns with our practices, consider:
- Adding descriptive titles to distinguish between the real and mock providers in the UI
- Ensuring the mock provider implements the same interface as the real one
Let's verify the mock provider implementation:
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/prerequisitesSection.ts (1)
12-12: LGTM: Import statement follows established patterns
The import statement is correctly structured as a named import, maintaining consistency with other data provider imports and supporting effective tree-shaking.
libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/messages/payment.ts (1)
Line range hint 1-50: LGTM on TypeScript and reusability aspects
The implementation:
- ✅ Uses TypeScript effectively with
defineMessages - ✅ Follows reusable component patterns
- ✅ Maintains consistent message structure
- ✅ Supports tree-shaking through proper exports
libs/application/templates/transport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/paymentSection.ts (1)
16-19: LGTM! Well-structured section configuration
The section configuration follows best practices with proper internationalization and clear identification.
libs/clients/transport-authority/vehicle-owner-change/src/lib/vehicleOwnerChangeClient.utils.ts (3)
4-8: Previously identified issue with isNaN usage remains valid
🧰 Tools
🪛 Biome
[error] 6-6: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
10-13: LGTM! Well-structured error message interface
The interface is properly typed and aligns well with the PR objectives for handling warning severity messages.
19-69: LGTM! Robust error handling implementation
The implementation successfully meets the PR objectives:
- Properly prioritizes Error (E) and Lock (L) messages over Warnings (W)
- Includes comprehensive error handling with proper fallbacks
- Well-documented with clear comments explaining the logic
libs/clients/transport-authority/vehicle-plate-renewal/src/lib/vehiclePlateRenewalClient.service.ts (1)
9-12: LGTM! Good use of shared utilities.
The imports demonstrate good code reuse by leveraging shared error handling utilities from the vehicle-owner-change client.
libs/application/templates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateRadioField.tsx (1)
17-17: Verify the implementation of checkCanRenew utility
The extraction of renewal logic into a utility function improves maintainability. However, we should verify its implementation to ensure it handles all edge cases correctly.
Also applies to: 42-44
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/ValidationErrorMessages/index.tsx (4)
1-16: LGTM! Well-organized imports with proper TypeScript types.
The imports are logically grouped and all necessary dependencies are properly imported with their types.
21-25: LGTM! Well-typed component declaration.
The component is properly typed with FC and PropsWithChildren, ensuring type safety for both props and children.
62-64: LGTM! Proper useEffect implementation.
The effect correctly manages the loading state with appropriate dependencies.
1-104: Verify compliance with libs/ coding guidelines.**
The component follows most guidelines but could benefit from:
- Better TypeScript type exports for reusability
- Enhanced tree-shaking through proper export structure
libs/clients/transport-authority/vehicle-plate-ordering/src/lib/vehiclePlateOrderingClient.service.ts (2)
11-14: LGTM! Good reuse of error handling utilities.
The imports demonstrate good code reuse by leveraging shared error handling utilities across different services.
90-92: Verify error message handling in getCleanErrorMessagesFromTryCatch.
The comment explains the error handling approach, but let's verify the implementation of getCleanErrorMessagesFromTryCatch to ensure it properly handles all warning severity messages (E, L, W) as mentioned in the PR objectives.
libs/api/domains/transport-authority/src/lib/graphql/main.resolver.ts (1)
18-18: LGTM! Import changes follow TypeScript best practices.
The new imports are properly scoped and follow the existing pattern in the file.
Also applies to: 29-29
libs/application/template-api-modules/src/lib/modules/templates/transport-authority/license-plate-renewal/license-plate-renewal.service.ts (3)
6-9: LGTM: Import changes align with PR objectives.
The new imports support the enhanced error handling functionality described in the PR objectives.
46-61: LGTM: Well-structured conditional validation logic.
The implementation efficiently handles different scenarios:
- For >5 plates: Validates only when selected in dropdown
- For ≤5 plates: Validates all immediately
This approach optimizes performance by avoiding unnecessary validations.
63-85: LGTM: Well-designed private method with clear responsibility.
The method effectively:
- Encapsulates plate ownership mapping logic
- Uses conditional validation for optimization
- Properly structures the return data
- Handles validation errors gracefully
libs/application/templates/transport-authority/order-vehicle-registration-certificate/src/lib/OrderVehicleRegistrationCertificateTemplate.ts (3)
27-27: LGTM! Import follows established patterns.
The addition of MockableSamgongustofaPaymentCatalogApi is well-placed within the related API imports group.
Line range hint 1-182: LGTM! Template structure follows best practices.
The template implementation:
- Uses TypeScript effectively with proper type definitions
- Maintains reusability through modular structure
- Follows proper export patterns for effective tree-shaking
111-111: Verify the inclusion of mock API in production code.
Including both mock and real versions of SamgongustofaPaymentCatalogApi in the production bundle might impact the final bundle size. Consider:
- Moving mock APIs to a separate test file
- Using environment-based conditional imports
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/VehiclesField/VehicleSelectField.tsx (2)
3-3: LGTM: Import changes are correct and well-organized.
The addition of useEffect is properly grouped with other React hooks.
30-30: Document the new prop and verify its type definition.
The setFieldLoadingState prop has been added correctly, but it would be beneficial to:
- Add JSDoc documentation for this prop
- Ensure the type is properly defined in
FieldBaseProps
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts (2)
6-10: LGTM: Clean import structure
The imports are well-organized and follow proper naming conventions.
359-359: LGTM: Clean implementation of vehicle make formatting
The getVehicleSubModel utility method is well-implemented and properly handles null values through the filter method.
Also applies to: 365-367
libs/application/templates/transport-authority/order-vehicle-license-plate/src/lib/OrderVehicleLicensePlateTemplate.ts (3)
28-28: Addition of MockableSamgongustofaPaymentCatalogApi import
The import of MockableSamgongustofaPaymentCatalogApi enhances testing capabilities by allowing mockable interactions with the payment catalog API.
110-110: Including MockableSamgongustofaPaymentCatalogApi in API array
Adding MockableSamgongustofaPaymentCatalogApi to the api array ensures that the applicant role has access to a mockable payment catalog during the DRAFT state, which enhances flexibility and testability.
88-90: Ensure ApiActions.validateApplication is properly defined
Adding the onExit handler with ApiActions.validateApplication is a good practice to validate the application upon exiting the DRAFT state. Verify that ApiActions.validateApplication is defined and correctly implemented.
Run the following script to confirm that ApiActions.validateApplication is defined:
✅ Verification successful
ApiActions.validateApplication is properly defined in the reviewed file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that `validateApplication` exists in the `ApiActions` enum.
rg -g '*.ts' 'export enum ApiActions' -A 10 | rg 'validateApplication'
Length of output: 900
libs/application/templates/transport-authority/order-vehicle-license-plate/src/fields/index.ts
Show resolved
Hide resolved
libs/application/templates/transport-authority/license-plate-renewal/src/index.ts
Show resolved
Hide resolved
...nsport-authority/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/index.ts
Show resolved
Hide resolved
libs/api/domains/transport-authority/src/lib/graphql/dto/plateOrderAnswers.input.ts
Show resolved
Hide resolved
...y/order-vehicle-license-plate/src/forms/OrderVehicleLicensePlateForm/prerequisitesSection.ts
Show resolved
Hide resolved
...mplates/transport-authority/license-plate-renewal/src/fields/PlateField/PlateSelectField.tsx
Show resolved
Hide resolved
...lates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts
Show resolved
Hide resolved
...lates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts
Show resolved
Hide resolved
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts
Show resolved
Hide resolved
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts (3)
6-9: Add TypeScript return type annotation for better type safety.Consider adding explicit return type annotation to improve type safety and documentation.
export const applicationHasPendingApproval = ( answers: FormValue, excludeNationalId?: string, -): boolean => { +): boolean => {
11-28: Extract path strings as constants for better maintainability.The path strings 'ownerCoOwner' and 'operators' are used directly in the code. Consider extracting these as named constants at the file level for better maintainability and reusability.
const PATHS = { OWNER_CO_OWNER: 'ownerCoOwner', OPERATORS: 'operators', } as const;
Line range hint
1-51: Consider enhancing error handling and testing capabilities.Since this is a library function used across different NextJS apps, consider these architectural improvements:
- Add error handling for invalid paths in
getValueViaPathcalls- Consider adding input validation for the
answersparameter- Add JSDoc comments with examples for better documentation
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts (2)
5-39: LGTM! Consider enhancing the function documentation.The implementation correctly handles both old and new co-owners, with proper filtering of removed co-owners. The function name change improves clarity as discussed in the previous review.
Consider adding JSDoc documentation to better describe the parameters and return value:
+/** + * Checks if there are any pending approvals for the application + * @param answers - The form values containing co-owner information + * @param excludeNationalId - Optional national ID to exclude from the check + * @returns true if there are pending approvals, false otherwise + */ export const applicationHasPendingApproval = (
41-49: LGTM! Consider adding JSDoc for consistency.The implementation provides a clear abstraction for checking if a reviewer is the last one pending. The logic is simple and effective.
For consistency with the previous function, consider adding JSDoc documentation:
+/** + * Determines if the current reviewer is the last one who needs to approve + * @param reviewerNationalId - The national ID of the current reviewer + * @param answers - The form values containing co-owner information + * @returns true if the reviewer is the last one pending, false otherwise + */ export const isLastReviewer = (libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts (1)
5-52: Extract common approval check logic to improve maintainability.The approval checks for buyer co-owners and seller co-owners follow the same pattern. Consider extracting this into a type-safe utility function.
+type ApprovalEntity = { + nationalId: string; + approved?: boolean; + wasRemoved?: string; +}; + +const hasUnapprovedEntities = <T extends ApprovalEntity>( + entities: T[], + excludeNationalId?: string, +): boolean => + entities + .filter(({ wasRemoved }) => wasRemoved !== 'true') + .some( + ({ nationalId, approved }) => + (!excludeNationalId || nationalId !== excludeNationalId) && !approved, + ); export const applicationHasPendingApproval = ( answers: FormValue, excludeNationalId?: string, ): boolean => { const buyer = getValueViaPath(answers, 'buyer', {}) as UserInformation if ( (!excludeNationalId || buyer.nationalId !== excludeNationalId) && !buyer.approved ) { return true } const buyerCoOwnersAndOperators = getValueViaPath( answers, 'buyerCoOwnerAndOperator', [], ) as CoOwnerAndOperator[] - if ( - buyerCoOwnersAndOperators.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { + if (hasUnapprovedEntities(buyerCoOwnersAndOperators, excludeNationalId)) { return true } const sellerCoOwners = getValueViaPath( answers, 'sellerCoOwner', [], ) as CoOwnerAndOperator[] - if ( - sellerCoOwners.some( - ({ nationalId, approved }) => - (!excludeNationalId || nationalId !== excludeNationalId) && !approved, - ) - ) { + if (hasUnapprovedEntities(sellerCoOwners, excludeNationalId)) { return true } return false }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/utils/isLastReviewer.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/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.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/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.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."
🔇 Additional comments (3)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/isLastReviewer.ts (1)
41-51: LGTM! Well-structured and clear implementation.
The function is well-documented, follows single responsibility principle, and has clear logic for determining if a reviewer is the last one pending approval.
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/utils/isLastReviewer.ts (2)
Line range hint 1-4: LGTM! Code follows the library guidelines.
The implementation:
- Uses TypeScript effectively with proper type annotations
- Exports pure, reusable functions
- Uses named exports enabling tree-shaking
79-84: Verify length comparison edge case.
The length comparison alone might not be sufficient to determine if new co-owners were added. It's possible that the same number of co-owners exists but with different national IDs (some removed, some added).
libs/api/domains/transport-authority/src/lib/transportAuthority.service.ts
Show resolved
Hide resolved
...lates/transport-authority/order-vehicle-license-plate/order-vehicle-license-plate.service.ts
Show resolved
Hide resolved
johannaagma
added
the
deprecated:automerge
kodiakhq
bot
deleted the
fix/transport-authority-error-handling-warnsever
branch
…age in vehicle validation (#16662) * Fix typo * Fix mileageReading vs isRequired Stop using mileageRequired from answers in getSelectedVehicle and always look at the value from externalData (it should not change) * Make sure to always use requiresMileageRegistration from currentvehicleswithmileageandinsp (not basicVehicleInformation) Cleanup make+vehcom (basicVehicleInformation) vs make (currentvehicles) * Cleanup * cleanup * cleanup * cleanup * Use shared function to extract messages from error body Display always E, L and W messages (but E and L first) * Use dummy mileage value when validating per vehicle (owner/operator change) * Catch error from mileage api * Fix the way validation errors are displayed in SGS ownerchange Allow all users to retry submit application if all approvals are finished * Apply same change to change co-owner + change operator Display ValidationErrorMessages always in overview, no matter who is reviewing * Cleanup * Cleanup in LicensePlateRenewal + OrderVehicleLicensePlate Add validation per plate if user has more than 5 * Fix the way vehicle subModel is displayed * Fixes after review * Fix the way errors are displayed for RenewLicensePlate Add MocablePayment * Add validation for OrderVehicleLicensePlate * Cleanup * Fix comment --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Improving the way validation errors are displayed. When final reviewer (usually buyer) is submitting the application, they will not get any information about what happened.
Before:
After:
Also changed so this validation is run everytime an reviewer approves (not only buyer/seller for ownerchange) for all 3 approval SGS applications (eigendaskipti, umráðamaður, meðeigandi)
Also added so the error from submit are saved in the externalData field: submitApplication, in case we need it to debug
Also added, so that the "Try again" button (to try and re-submit application) is visible to all reviewer (not just seller) if all approvals are in answers but application still in REVIEW state (before only seller could see this button)
Added validation to OrderVehicleLicensePlate (validated before payment)
Add missing validation for LicensePlateRenewal if user has more that 5 plates (no validation in Select field). Also remove unecessary validation before payment, since the validation function only takes in regno and no changeable answers)
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation
These changes aim to enhance user experience and improve the overall functionality of the vehicle registration and plate ordering processes.