fix(samgongustofa): Validate when buyer fields are empty in TransferOfVehicleOwnership #16999
Conversation
This reverts commit 719277e.
WalkthroughThe pull request introduces several modifications primarily focused on enhancing error handling and validation logic in various components related to vehicle ownership management. Key updates include the addition of new state variables for tracking error conditions in the Changes
Possibly related PRs
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 title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16999 +/- ##
==========================================
- Coverage 35.59% 35.59% -0.01%
==========================================
Files 6920 6920
Lines 146139 146139
Branches 41512 41654 +142
==========================================
- Hits 52021 52020 -1
- Misses 94118 94119 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
![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 (13)
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getApproveAnswers.ts (2)
Line range hint
20-30: Inconsistent naming:ownerCoOwnerkey returns an arrayThe property name
ownerCoOwnersuggests a single co-owner, but it returns an array of all co-owners. This could be confusing for consumers of this function.Consider either:
- Keeping the original
ownerCoOwnersname to accurately reflect the array return type, or- Restructuring to return only the specific co-owner's data:
- ownerCoOwner: ownerCoOwners.map((ownerCoOwner) => { + ownerCoOwners: ownerCoOwners.map((ownerCoOwner) => {
Line range hint
5-8: Add explicit return type for better type safetyThe function lacks an explicit return type, which could make it harder to catch type-related issues during refactoring.
Consider adding an interface and return type:
interface ApprovalResponse { ownerCoOwners?: Array<{ nationalId: string; name: string; email: string; phone: string; approved: boolean; }>; operators?: Array<{ nationalId: string; name: string; email: string; phone: string; wasRemoved: boolean; approved: boolean; }>; } export const getApproveAnswers = ( reviewerNationalId: string, answers: FormValue ): ApprovalResponse => {libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx (3)
1-10: Consider exporting UserInformation type from a dedicated types fileFor better reusability across different components, consider moving the UserInformation type to a dedicated types file within the shared directory.
-import { UserInformation } from '../../shared' +import { UserInformation } from '../../shared/types'
11-13: Consider using a more maintainable email validation approachThe current email regex might be too restrictive and hard to maintain. Consider:
- Moving it to a shared constants file for reuse
- Using a well-tested email validation library or the HTML5 email input validation
-const emailRegex = - /^[\w!#$%&'*+/=?`{|}~^-]+(?:\.[\w!#$%&'*+/=?`{|}~^-]+)*@(?:[A-Z0-9-]+\.)+[A-Z]{2,6}$/i +import { EMAIL_REGEX } from '../../constants'
58-63: Be explicit about props passed to BuyerItemInstead of spreading all props, consider being explicit about which props BuyerItem needs. This improves type safety and prevents passing unnecessary props.
- <BuyerItem id={id} buyer={buyer} setBuyer={setBuyer} {...props} /> + <BuyerItem + id={id} + buyer={buyer} + setBuyer={setBuyer} + application={application} + field={field} + />libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/BuyerCoOwnerAndOperatorRepeaterItem.tsx (1)
27-27: Consider adding JSDoc documentation.Adding documentation would help other developers understand:
- The purpose of this component
- The relationship between buyers, co-owners, and operators
- The validation rules being applied
Example:
+/** + * A form component that handles the input fields for a buyer, co-owner, or operator + * in the vehicle ownership transfer process. + * + * @param props.id - Unique identifier for the form field + * @param props.index - Index in the repeater array + * @param props.rowLocation - Visual position in the form + * @param props.repeaterField - Field configuration and state + * @param props.handleRemove - Callback to remove this item + * @param props.addNationalIdToCoOwners - Callback to update co-owners list + */ export const BuyerCoOwnerAndOperatorRepeaterItem: FC<libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/CoOwnerRepeater/index.tsx (1)
153-175: Enhance type safety and error message management.Two improvements could make this code more maintainable:
- Use typed error constants instead of hardcoded strings
- Add type safety for the validation return tuple
Consider these improvements:
+ type ValidationResult = [boolean, string | null]; + const VALIDATION_ERRORS = { + IDENTICAL_IDS: 'Identical nationalIds', + NO_CHANGES: 'No co-owner has been added/removed' + } as const; setBeforeSubmitCallback?.(async () => { setIdenticalError(false) setNoCoOwnerChangesError(false) const hasDuplicate = await checkDuplicate() if (hasDuplicate) { setIdenticalError(true) - return [false, 'Identical nationalIds'] + return [false, VALIDATION_ERRORS.IDENTICAL_IDS] as ValidationResult } const coOwnerWasAdded = coOwners?.filter(({ wasRemoved }) => wasRemoved !== 'true').length > 0 const coOwnerWasRemoved = !!ownerCoOwners?.find( (x) => x.wasRemoved === 'true', ) const noCoOwnerChanges = !coOwnerWasAdded && !coOwnerWasRemoved if (noCoOwnerChanges) { setNoCoOwnerChangesError(true) - return [false, 'No co-owner has been added/removed'] + return [false, VALIDATION_ERRORS.NO_CHANGES] as ValidationResult } - return [true, null] + return [true, null] as ValidationResult })libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/messages/information.ts (1)
178-182: LGTM with a minor suggestion for the description field.The new error message is well-structured and aligns with the existing message patterns. The message key and content appropriately support the validation requirements mentioned in the PR objectives.
Consider making the description more specific to better indicate its usage context:
noChangesError: { id: 'ta.ccov.application:information.labels.coOwner.noChangesError', defaultMessage: 'Ekki er búið að gera neinar breytingar á meðeigendum', - description: 'coOwner no changes error', + description: 'Error shown when attempting to proceed without any changes to co-owners', },libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/OperatorRepeater/index.tsx (2)
218-240: Consider refactoring the validation logic for better maintainabilityThe validation logic in the beforeSubmit callback could be improved by:
- Moving validation logic into separate, testable functions
- Using typed constants for error messages
- Splitting the different validation checks into smaller, focused functions
Consider refactoring like this:
+ const ERROR_MESSAGES = { + IDENTICAL_IDS: 'Identical nationalIds', + NO_CHANGES: 'No operator has been added/removed' + } as const; + + const validateDuplicateIds = async ( + checkDuplicate: () => Promise<boolean>, + setError: (value: boolean) => void + ): Promise<[boolean, string | null]> => { + const hasDuplicate = await checkDuplicate(); + if (hasDuplicate) { + setError(true); + return [false, ERROR_MESSAGES.IDENTICAL_IDS]; + } + return [true, null]; + }; + + const validateOperatorChanges = ( + operators: OperatorInformation[], + oldOperators: OldOperatorInformation[], + setError: (value: boolean) => void + ): [boolean, string | null] => { + const operatorWasAdded = operators?.filter( + ({ wasRemoved }) => wasRemoved !== 'true' + ).length > 0; + const operatorWasRemoved = !!oldOperators?.find( + (x) => x.wasRemoved === 'true' + ); + if (!operatorWasAdded && !operatorWasRemoved) { + setError(true); + return [false, ERROR_MESSAGES.NO_CHANGES]; + } + return [true, null]; + }; setBeforeSubmitCallback?.(async () => { setIdenticalError(false); setNoOperatorChangesError(false); - const hasDuplicate = await checkDuplicate() - if (hasDuplicate) { - setIdenticalError(true) - return [false, 'Identical nationalIds'] - } - - const operatorWasAdded = - operators?.filter(({ wasRemoved }) => wasRemoved !== 'true').length > 0 - const operatorWasRemoved = !!oldOperators?.find( - (x) => x.wasRemoved === 'true', - ) - const noOperatorChanges = !operatorWasAdded && !operatorWasRemoved - if (noOperatorChanges) { - setNoOperatorChangesError(true) - return [false, 'No operator has been added/removed'] - } + const [isValidIds, idError] = await validateDuplicateIds( + checkDuplicate, + setIdenticalError + ); + if (!isValidIds) return [false, idError]; + + const [hasChanges, changeError] = validateOperatorChanges( + operators, + oldOperators, + setNoOperatorChangesError + ); + if (!hasChanges) return [false, changeError]; return [true, null]; });
Line range hint
1-318: Verify compliance with coding guidelinesThe component follows the required guidelines for
libs/**/*:
- ✓ Component is reusable across NextJS apps
- ✓ Uses TypeScript for props and type definitions
- ✓ Follows proper import practices for effective tree-shaking
However, consider extracting the validation logic into a separate hook to improve reusability across different forms.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/messages/information.ts (1)
220-224: LGTM! Consider enhancing message reusability.The new error message is well-structured and aligns with the PR's objective to validate scenarios where no operator changes are made. The implementation follows the established i18n patterns and maintains consistency with existing messages.
For better reusability across different NextJS apps, consider extracting common vehicle-related error messages to a shared messages file.
Create a new shared messages file (e.g.,
libs/shared/messages/vehicle-errors.ts) and move common error messages there:// libs/shared/messages/vehicle-errors.ts import { defineMessages } from 'react-intl' export const vehicleErrors = defineMessages({ noOperatorChanges: { id: 'shared.errors.vehicle.noOperatorChanges', defaultMessage: 'Ekki er búið að gera neinar breytingar á umráðamönnum', description: 'Error shown when no changes are made to operators', }, // Add other common vehicle-related error messages here })libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx (2)
22-27: Consider providing user feedback on data fetching errorsCurrently, the
onErrorhandler in youruseLazyQueryonly logs errors to the console. Consider displaying an error message to inform the user of the issue, enhancing the user experience.
122-126: Use boolean value forwasRemovedto enhance type safetyThe
wasRemovedproperty is being set to the string'true'. Using a boolean value improves type safety and prevents potential bugs.Apply this diff to change
wasRemovedto a boolean:- return { ...coOwnerOrOperator, wasRemoved: 'true' } + return { ...coOwnerOrOperator, wasRemoved: true }Update the filtering condition accordingly:
- const filteredCoOwnersAndOperators = buyerCoOwnerAndOperator.filter( - ({ wasRemoved }) => wasRemoved !== 'true', - ) + const filteredCoOwnersAndOperators = buyerCoOwnerAndOperator.filter( + ({ wasRemoved }) => !wasRemoved, + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/CoOwnerRepeater/index.tsx(3 hunks)libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/lib/messages/information.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/OperatorRepeater/index.tsx(3 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/lib/messages/information.ts(1 hunks)libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getApproveAnswers.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/BuyerCoOwnerAndOperatorRepeaterItem.tsx(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx(3 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/index.ts(1 hunks)libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/buyerSubSection.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/CoOwnerRepeater/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/lib/messages/information.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/OperatorRepeater/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/lib/messages/information.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/getApproveAnswers.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/Buyer/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/BuyerCoOwnerAndOperatorRepeater/BuyerCoOwnerAndOperatorRepeaterItem.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/BuyerCoOwnerAndOperatorRepeater/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/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/forms/TransferOfVehicleOwnershipForm/InformationSection/buyerSubSection.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 (13)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/index.ts (2)
6-7: Verify TypeScript types in new components.
Ensure proper type definitions exist for the new components.
Let's check the TypeScript types in the new components:
✅ Verification successful
TypeScript types are properly defined in the new components
Both components have well-defined TypeScript types:
-
The Buyer component:
- Uses
FieldBasePropsfrom '@island.is/application/types' - Has proper type definitions for the
UserInformationinterface - Includes proper typing for all state and props
- Uses
-
The BuyerCoOwnerAndOperatorRepeater component:
- Uses
FieldBasePropsfrom '@island.is/application/types' - Has proper type definitions for
CoOwnerAndOperatorarray - Includes proper typing for all handlers and state
- Uses
All supporting components (BuyerItem, etc.) also have proper interface definitions and type imports.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TypeScript types in new components
# Check Buyer component types
ast-grep --pattern 'interface $name {
$$$
}' ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx
# Check BuyerCoOwnerAndOperatorRepeater component types
ast-grep --pattern 'interface $name {
$$$
}' ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx
Length of output: 335
Script:
#!/bin/bash
# Let's try a different approach to check the TypeScript types
# Check content of Buyer component
cat ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.ts
# Check content of BuyerCoOwnerAndOperatorRepeater component
cat ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.ts
# Also search for type definitions in the entire codebase
rg "type|interface" --type ts ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/
Length of output: 16007
Script:
#!/bin/bash
# Let's check the actual content of the Buyer and BuyerCoOwnerAndOperatorRepeater components
# Note: Previous search showed they might be .tsx files instead of .ts
# Check Buyer component
cat ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx
# Check BuyerCoOwnerAndOperatorRepeater component
cat ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx
# Also check their related files
cat ./libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/BuyerItem.tsx
Length of output: 10736
6-7: LGTM! Verify removed export references.
The new exports align with the PR objectives and follow consistent naming patterns.
Let's verify if there are any remaining references to the removed export:
✅ Verification successful
Removal of CoOwnerAndOperatorRepeater export is safe
The removed export is not directly imported from the barrel file. The codebase uses ReviewCoOwnerAndOperatorRepeater and BuyerCoOwnerAndOperatorRepeater components instead, which are imported from their respective directories. The changes align with the PR objectives and maintain code integrity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to CoOwnerAndOperatorRepeater
# that might cause runtime errors
rg -l "CoOwnerAndOperatorRepeater" \
--type ts \
--type tsx \
--glob '!**/fields/index.ts'
Length of output: 112
Script:
#!/bin/bash
# Let's try with ripgrep without file type filters, excluding the index.ts file
rg -l "CoOwnerAndOperatorRepeater" --glob '!**/fields/index.ts'
# Also search for potential imports of this component
rg -l "from ['\"].*CoOwnerAndOperatorRepeater['\"]" --glob '!**/fields/index.ts'
# Check if the component file still exists
fd "CoOwnerAndOperatorRepeater.ts" -t f
Length of output: 1139
Script:
#!/bin/bash
# Let's check the content of Review/index.tsx to see how it imports CoOwnerAndOperatorRepeater
rg "CoOwnerAndOperatorRepeater" -A 2 -B 2 libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Review/index.tsx
# Check if there's a direct import from fields/index.ts
rg "from ['\"]../index['\"]" libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Review/index.tsx
Length of output: 764
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/forms/TransferOfVehicleOwnershipForm/InformationSection/buyerSubSection.ts (3)
Line range hint 1-30: Verify TypeScript types for reusability.
Since this is in the libs/ directory, we should ensure proper TypeScript usage for component props and exported types to maintain reusability across NextJS apps.
Let's check for TypeScript type definitions:
✅ Verification successful
TypeScript types are properly used in this reusable library component
The code follows good TypeScript practices by:
- Using the strongly-typed
buildCustomField,buildMultiField, andbuildSubSectionfunctions from the core library - Properly importing types from
@island.is/application/core - Implementing a clean, type-safe component structure for reuse
The components Buyer and BuyerCoOwnerAndOperatorRepeater are referenced as string literals in the custom field definitions, which is the expected pattern for this codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for TypeScript types related to the Buyer components
rg -t typescript "type.*Props" -g "**/*buyer*.{ts,tsx}"
rg -t typescript "interface.*Props" -g "**/*buyer*.{ts,tsx}"
Length of output: 190
Script:
#!/bin/bash
# Search for type definitions and interfaces in buyer-related files
rg "type.*Props|interface.*Props" --type ts --type tsx
# Search for the Buyer and BuyerCoOwnerAndOperatorRepeater components
fd -e ts -e tsx "(Buyer|BuyerCoOwnerAndOperatorRepeater)\.tsx?"
# Check the content of any found component files
fd -e ts -e tsx "(Buyer|BuyerCoOwnerAndOperatorRepeater)\.tsx?" --exec cat {}
Length of output: 232
Script:
#!/bin/bash
# Search for files using ripgrep without file type restrictions
rg -g "*.ts" -g "*.tsx" "type.*Props|interface.*Props"
# Find and show content of Buyer components using ripgrep
rg -g "*.ts" -g "*.tsx" -l "Buyer|BuyerCoOwnerAndOperatorRepeater" | xargs cat
# Check imports and type definitions in the application core
rg -g "*.ts" -g "*.tsx" "buildCustomField|buildMultiField|buildSubSection" -A 5 -B 5
Length of output: 104129
9-9: LGTM! The ID changes align with validation requirements.
The changes correctly address the validation issues mentioned in the PR objectives by:
- Using a more descriptive section ID
- Adding a dedicated Buyer component with the correct field ID that matches the data schema
Let's verify the Buyer component implementation:
Also applies to: 17-21
✅ Verification successful
The ID change from 'buyer' to 'buyerSubSection' is safe and correctly implemented
The codebase search confirms that:
- The ID change is localized to the form section structure and doesn't affect data schema or validation
- The actual buyer data is accessed via 'buyer' path in the codebase (e.g. 'buyer.nationalId', 'buyer.approved')
- The component has been properly renamed from 'CoOwnerAndOperatorRepeater' to 'BuyerCoOwnerAndOperatorRepeater' with matching file and export names
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Buyer component exists and is properly exported
ast-grep --pattern 'export const Buyer = '
Length of output: 44
Script:
#!/bin/bash
# Let's try a broader search for Buyer-related components and exports
rg -i "buyer" --type ts -A 3 -B 3
Length of output: 106487
24-24: LGTM! Component rename improves clarity.
The rename to 'BuyerCoOwnerAndOperatorRepeater' provides better context and aligns with the buyer-specific functionality.
Let's verify the component exists:
✅ Verification successful
Component exists and is properly integrated
The component BuyerCoOwnerAndOperatorRepeater is correctly implemented and integrated in the codebase:
- Exported from
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx - Has associated files for styling (
.css.ts) and item rendering (RepeaterItem.tsx) - Properly exported through the fields barrel file
- Correctly referenced in the form configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the BuyerCoOwnerAndOperatorRepeater component exists
ast-grep --pattern 'export const BuyerCoOwnerAndOperatorRepeater = '
Length of output: 70
Script:
#!/bin/bash
# Let's try a broader search for the component name using ripgrep
rg "BuyerCoOwnerAndOperatorRepeater" -l
# Also search for any file that might contain this component
fd "BuyerCoOwnerAndOperatorRepeater"
# Let's also check for potential React/TypeScript component definitions with a more flexible pattern
ast-grep --pattern 'BuyerCoOwnerAndOperatorRepeater'
Length of output: 1478
libs/application/templates/transport-authority/change-operator-of-vehicle/src/utils/getApproveAnswers.ts (1)
Line range hint 20-30: Verify consumers of this function
The change in return property name from ownerCoOwners to ownerCoOwner could break components that consume this function.
Let's verify all usages of this function:
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx (1)
14-63: Verify fix for buyer field validation
The implementation looks correct for handling buyer field validation, but let's verify that it resolves the reported Datadog errors.
✅ Verification successful
Let me verify if the buyer field validation is properly implemented by checking for any related validation components and error handling.
Let me try a different search strategy to verify the buyer field validation implementation.
Let me check the complete implementation of the Buyer component to verify the validation logic.
Buyer field validation implementation is correct
The implementation properly validates the buyer fields with the following checks:
- Name must not be empty (length > 0)
- National ID must be exactly 10 characters
- Phone number must be at least 7 characters
- Email must match the email regex pattern
- Field ID is correctly passed to BuyerItem component
The validation triggers the updateApplication mutation only when all validation criteria are met, which should resolve the reported Datadog errors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related validation components to ensure consistent ID usage
ast-grep --pattern 'const { id } = field'
# Search for any remaining TODO comments or validation-related comments
rg -i "todo.*validation|fix.*validation"
Length of output: 136128
Script:
#!/bin/bash
# Search for buyer validation related code
rg -A 5 "buyer.*validation|validateBuyer|buyer.*error" libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
# Search for any Datadog error logging related to buyer fields
rg -i "datadog.*buyer|error.*buyer"
# Check for any test files related to buyer validation
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts buyer
Length of output: 233
Script:
#!/bin/bash
# Search for the BuyerItem component implementation
ast-grep --pattern 'const BuyerItem = $$$' libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
# Look for email validation regex pattern
rg "emailRegex|email.*test" libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
# Check for any validation-related imports
rg "import.*validation|from.*validation" libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
Length of output: 625
Script:
#!/bin/bash
# Get the full implementation of the Buyer component
rg -U "export const Buyer.*?\}" -A 50 libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx
# Look for the BuyerItem component definition
rg -U "BuyerItem.*?\}" -A 50 libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/
# Check for any validation-related constants or types
rg -A 3 "interface.*Buyer|type.*Buyer|const.*Buyer" libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/
Length of output: 16845
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/BuyerCoOwnerAndOperatorRepeaterItem.tsx (2)
Line range hint 27-37: LGTM! Component renamed and properly typed.
The component has been correctly renamed to align with its buyer-focused functionality, and the TypeScript types are properly defined using FC with PropsWithChildren.
Line range hint 41-47: Verify field ID construction matches schema.
The field paths are constructed using the provided id prop, which should resolve the validation issues mentioned in the PR objectives. Let's verify this:
✅ Verification successful
Based on the search results, I can now provide a final response:
Field path construction is consistent with schema validation
The field paths are properly constructed using the provided id prop and match the expected schema structure. The code shows:
-
Buyer co-owner fields are consistently mapped in both frontend and backend:
- Frontend uses proper field path construction (
${fieldIndex}.email,${fieldIndex}.phone, etc.) - Backend correctly maps these fields when processing data (
coOwner.nationalId,coOwner.email)
- Frontend uses proper field path construction (
-
The field naming convention is maintained throughout the codebase:
- Buyer co-owners are consistently referenced as
buyerCoOwnerAndOperator - Field paths follow the pattern
[id][index].fieldName
- Buyer co-owners are consistently referenced as
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for schema definitions to verify field IDs
rg -A 5 "schema.*buyer|schema.*owner|schema.*operator" --type typescript
# Search for field validation references
ast-grep --pattern 'const $field = `${$_}.${$_}`'
Length of output: 161
Script:
#!/bin/bash
# Search for schema definitions with .ts extension
rg -A 5 "schema.*buyer|schema.*owner|schema.*operator" -t ts
# Search for field validation patterns in TypeScript files
rg -t ts "const.*\= \`\$\{.*\}\..*\`" -A 2
# Look for validation schema files
fd "schema.ts|validation.ts" --exec cat {}
Length of output: 112537
Script:
#!/bin/bash
# Search for field validation patterns in TypeScript files
rg -t ts "const field.*=.*\`\$\{.*\}\..*\`" -A 2
# Search for schema definitions related to buyer/co-owner fields
rg -t ts "buyer.*coOwner|coOwner.*buyer" -A 5
# Look for field path construction patterns
ast-grep --pattern 'const $field = `${$_}.$_`'
Length of output: 6401
libs/application/templates/transport-authority/change-co-owner-of-vehicle/src/fields/CoOwnerRepeater/index.tsx (2)
38-40: LGTM: Clear and well-typed state management.
The new state variable follows React best practices and has a clear, descriptive name that indicates its purpose.
230-237: LGTM: Consistent error message implementation.
The new error message display follows the existing pattern and correctly uses the localization system.
libs/application/templates/transport-authority/change-operator-of-vehicle/src/fields/OperatorRepeater/index.tsx (2)
41-44: LGTM: Well-typed state declarations
The new state variables are properly typed and follow React conventions.
309-316: LGTM: Error message UI follows design system patterns
The error message implementation:
- Uses the correct design system components
- Maintains consistent spacing with other error messages
- Properly handles internationalization
...ation/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx
Show resolved
Hide resolved
...ation/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/Buyer/index.tsx
Show resolved
Hide resolved
...emplates/transport-authority/change-co-owner-of-vehicle/src/fields/CoOwnerRepeater/index.tsx
Show resolved
Hide resolved
...authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx (4)
Line range hint
1-19: LGTM! Component name change better reflects its responsibility.The TypeScript types and imports are well-structured. The component name change from
CoOwnerAndOperatortoBuyerCoOwnerAndOperatorbetter reflects its combined responsibility for managing buyer, co-owner, and operator information.Consider grouping related imports together (e.g., all UI components, all hooks, etc.) for better organization.
Line range hint
87-103: Optimize duplicate checking logic.The duplicate checking logic could be simplified and made more efficient using a Set.
Consider this optimization:
const checkDuplicate = async () => { const existingCoOwnersAndOperators = filteredCoOwnersAndOperators.map( ({ nationalId }) => nationalId ) const buyerNationalId = await getBuyerNationalId() const jointOperators = [...existingCoOwnersAndOperators, buyerNationalId] - return !!jointOperators.some((nationalId, index) => { - return ( - nationalId && - nationalId.length > 0 && - jointOperators.indexOf(nationalId) !== index - ) - }) + const validIds = jointOperators.filter(id => id && id.length > 0) + return new Set(validIds).size !== validIds.length }
132-139: Improve error message in setBeforeSubmitCallback.The error message "Identical nationalIds" is not descriptive enough for users to understand the issue.
Consider providing a more descriptive error message:
if (hasDuplicate) { - return [false, 'Identical nationalIds'] + return [false, 'Each person must have a unique national ID. Please check the buyer, co-owner, and operator IDs for duplicates.'] }
Line range hint
141-190: Enhance error message accessibility.The error message display could benefit from improved accessibility features.
Consider these accessibility improvements:
{identicalError && ( <Box marginTop={4}> <AlertMessage + role="alert" + aria-live="polite" type="error" title={formatMessage(information.labels.operator.identicalError)} /> </Box> )}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/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."
🔇 Additional comments (2)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx (2)
63-85:
Handle potential undefined application data.
The getBuyerNationalId function doesn't handle the case where updatedApplication is undefined, which could lead to runtime errors.
The previous review comment about handling undefined updatedApplication is still valid. Please implement the suggested fix.
Line range hint 1-190: Verify integration with related components.
The changes look good but let's verify the integration with related components.
✅ Verification successful
Let me generate additional verification scripts to check for related component usage and potential breaking changes.
Let me try one more verification with modified commands:
The changes are localized and properly integrated
The verification shows that:
- The
BuyerCoOwnerAndOperatorRepeatercomponent is only used within its own module in the transfer-of-vehicle-ownership template - The
buyer.nationalIdpath is consistently used across related templates and follows the same pattern - The implementation of
setBeforeSubmitCallbackaligns with other similar components in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related component usage and potential breaking changes
# Check for usage of the renamed component
rg -l "CoOwnerAndOperatorRepeater" --type typescript
# Check for other components using the buyer.nationalId path
rg -l "buyer\.nationalId" --type typescript
# Check for components implementing setBeforeSubmitCallback
ast-grep --pattern 'setBeforeSubmitCallback?.($$$)'
Length of output: 50037
Script:
#!/bin/bash
# Check for files importing or using BuyerCoOwnerAndOperatorRepeater
rg -l "BuyerCoOwnerAndOperatorRepeater" --type=ts --type=tsx
# Check for files using the shared CoOwnerAndOperator type
rg -l "CoOwnerAndOperator" --type=ts --type=tsx
# Check for files using buyer.nationalId path in getValueViaPath
ast-grep --pattern 'getValueViaPath($$$, "buyer.nationalId", $$$)'
Length of output: 233
Script:
#!/bin/bash
# Search for component usage without file type restrictions
rg -l "BuyerCoOwnerAndOperatorRepeater"
# Search for shared type usage
rg -l "CoOwnerAndOperator"
# Search for files using buyer.nationalId in any context
rg "buyer\.nationalId"
Length of output: 11138
...authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx
Outdated
Show resolved
Hide resolved
...authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx
Outdated
Show resolved
Hide resolved
johannaagma
added
automerge
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx (3)
15-21: Consider using a more specific error state type.While the state management improvements are good, the
identicalErrorboolean state could be enhanced to provide more detailed error information.Consider this improvement:
- const [identicalError, setIdenticalError] = useState<boolean>(false) + type ValidationError = { + hasError: boolean; + message?: string; + } + const [identicalError, setIdenticalError] = useState<ValidationError>({ + hasError: false + })
Line range hint
56-72: Enhance duplicate checking with additional validation.The current implementation could be improved by adding validation for empty or invalid national IDs.
Consider this enhancement:
const checkDuplicate = async () => { + const isValidNationalId = (id: string) => { + return id && id.length === 10 && /^\d+$/.test(id) + } + const existingCoOwnersAndOperators = filteredCoOwnersAndOperators.map( ({ nationalId }) => { - return nationalId + return nationalId.trim() }, ) const buyerNationalId = watch('buyer.nationalId') + if (!isValidNationalId(buyerNationalId)) { + return false + } const jointOperators = [...existingCoOwnersAndOperators, buyerNationalId] return !!jointOperators.some((nationalId, index) => { return ( - nationalId && - nationalId.length > 0 && + isValidNationalId(nationalId) && jointOperators.indexOf(nationalId) !== index ) }) }
Line range hint
118-127: Consider explicit prop passing instead of spreading.Using
{...props}can lead to prop pollution and make it harder to track which props are actually needed by the child component.Consider explicitly passing only the required props:
<BuyerCoOwnerAndOperatorRepeaterItem id={id} repeaterField={field} index={index} rowLocation={rowLocation + 1} key={`buyerCoOwnerAndOperator-${index}`} handleRemove={handleRemove} addNationalIdToCoOwners={addNationalIdToCoOwners} - {...props} + application={application} + field={field} + setBeforeSubmitCallback={setBeforeSubmitCallback} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/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."
🔇 Additional comments (1)
libs/application/templates/transport-authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx (1)
5-10: LGTM! Component naming and imports are well-structured.
The renaming to BuyerCoOwnerAndOperatorRepeater better reflects the component's purpose and responsibility. The addition of useFormContext aligns with modern form management practices.
Also applies to: 12-14
...authority/transfer-of-vehicle-ownership/src/fields/BuyerCoOwnerAndOperatorRepeater/index.tsx
Show resolved
Hide resolved
…fVehicleOwnership (#16999) * Validate buyer + buyerCoOwnerAndOperator in TransferOfVehicleOwnership * Validate added co-owners i ChangeCoOwnerOfVehicle * Fix getApproveAnswers for changeOperatorOfVehicle * Add error message if no changes in ChangeOperatorOfVehicle * Revert "Validate added co-owners i ChangeCoOwnerOfVehicle" This reverts commit 719277e. * Add error message if no changes in ChangeCoOwnerOfVehicle * Cleanup * Use watch instead of getApplicationInfo --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Since the customComponent didnt have the same ID as the field in dataSchema, the field buyer was not validated, and causing unecessary error logging in datadog when calling
/personcheckAlso fix ChangeOperatorOfVehicle where co-owner approval was never saved in application (so operator change was not possible if there was any co-owner)
Also added extra validation if there are no changes in operators (ChangeOperatorOfVehicle) or coOwners (ChangeCoOwnerOfVehicle) - otherwise you get stuck on paymentOverview page for a few seconds, because you cant continue since there are no items to pay for, but you still have to finish waiting for validation to finish to go back to the pervious page
Checklist:
Summary by CodeRabbit
New Features
Buyercomponent for managing buyer information.Bug Fixes
Refactor
CoOwnerAndOperatorRepeatertoBuyerCoOwnerAndOperatorRepeater.Documentation