fix(application-system): Fixing error messages for certain types of machines #17486
Conversation
…sages for unavailable street registration types
WalkthroughThis pull request introduces enhancements to error handling and validation logic related to the street registration process for work machines. Key changes include the addition of a new asynchronous method Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
libs/application/templates/aosh/street-registration/src/lib/dataSchema.ts (1)
15-26: Consider enhancing the machine object validation.While checking for non-empty object is good, the validation could be more specific about which fields are required for a valid machine object.
Consider updating the refinement to check for specific required fields:
- .refine((obj) => Object.keys(obj).length > 0), + .refine( + (obj) => obj.id !== undefined && obj.type !== undefined, + { message: "Machine must have at least an ID and type" } + ),libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/prerequisitesSection.ts (1)
62-66: Consider adding descriptive title and subtitle for better user experience.The new data provider
GetAvailableRegistrationTypesis added with empty title and subtitle. Consider adding descriptive text to inform users about the purpose of this data provider.buildDataProviderItem({ provider: GetAvailableRegistrationTypes, - title: '', - subTitle: '', + title: externalData.availableRegistrationTypes.title, + subTitle: externalData.availableRegistrationTypes.subTitle, }),libs/application/templates/aosh/street-registration/src/utils/getSelectedMachine.ts (2)
52-72: Add null safety checks and consider extracting common logic.The function could benefit from additional null safety checks and extraction of shared logic.
+const extractRegType = (regNumber: string | null | undefined): string => + (regNumber || '').substring(0, 2) export const isInvalidRegistrationType = ( externalData: ExternalData, - regNumber: string, + regNumber: string | null | undefined, ) => { const validTypes = getValueViaPath<string[]>( externalData, 'availableRegistrationTypes.data', [], ) const inspectBeforeTypes = getValueViaPath<string[]>( externalData, 'typesMustInspectBeforeRegistration.data', [], ) - const regType = regNumber.substring(0, 2) + const regType = extractRegType(regNumber) return ( !validTypes?.includes(regType) && !inspectBeforeTypes?.includes(regType) ) }
74-88: Reuse extracted logic and add return type annotation.The function duplicates type extraction logic and would benefit from reusing the shared logic.
export const isMachineDisabled = ( externalData: ExternalData, - regNumber: string, + regNumber: string | null | undefined, ): boolean => { const validTypes = getValueViaPath<string[]>( externalData, 'availableRegistrationTypes.data', [], ) - const regType = regNumber.substring(0, 2) + const regType = extractRegType(regNumber) // Checks the machine type against the available registration types return !validTypes?.includes(regType) }libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.tsx (2)
270-273: Consider extracting status determination logic.The status key determination logic could be moved to a separate function to improve maintainability and reusability.
+const getMachineStatusKey = ( + externalData: unknown, + regNumber: string, +): 'inspectBeforeRegistration' | 'unavailableTypeForRegistration' | null => { + const mustInspect = mustInspectBeforeStreetRegistration(externalData, regNumber) + const isUnavailableType = isInvalidRegistrationType(externalData, regNumber) + return mustInspect + ? 'inspectBeforeRegistration' + : isUnavailableType + ? 'unavailableTypeForRegistration' + : null +} const setMachineValues = (machineDetails: MachineDetails) => { const machineDisabled = machineDetails.disabled if (application.typeId === 'StreetRegistration') { - const isUnavailableTypeForRegistration = isInvalidRegistrationType( - application?.externalData, - machineDetails.regNumber || '', - ) - const mustInspect = mustInspectBeforeStreetRegistration( - application?.externalData, - machineDetails.regNumber || '', - ) - const statusKey = mustInspect - ? 'inspectBeforeRegistration' - : isUnavailableTypeForRegistration - ? 'unavailableTypeForRegistration' - : null + const statusKey = getMachineStatusKey( + application?.externalData, + machineDetails.regNumber || '' + )Also applies to: 278-282
284-287: Consider using constants for status keys.Define constants for the status keys to avoid magic strings and improve maintainability.
+const MACHINE_STATUS = { + INSPECT_BEFORE_REGISTRATION: 'inspectBeforeRegistration', + UNAVAILABLE_TYPE: 'unavailableTypeForRegistration', +} as const if ( - statusKey === 'inspectBeforeRegistration' || - statusKey === 'unavailableTypeForRegistration' + statusKey === MACHINE_STATUS.INSPECT_BEFORE_REGISTRATION || + statusKey === MACHINE_STATUS.UNAVAILABLE_TYPE ) { machineDetails = { ...machineDetails, disabled: true, status: validationErrors && - formatText(validationErrors[statusKey], application, formatMessage), + formatText(validationErrors[statusKey as keyof typeof validationErrors], application, formatMessage), } }Also applies to: 293-293
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/application/template-api-modules/src/lib/modules/templates/aosh/street-registration/street-registration.service.ts(1 hunks)libs/application/templates/aosh/street-registration/src/dataProviders/index.ts(1 hunks)libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.ts(3 hunks)libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/prerequisitesSection.ts(2 hunks)libs/application/templates/aosh/street-registration/src/lib/StreetRegistrationTemplate.ts(2 hunks)libs/application/templates/aosh/street-registration/src/lib/dataSchema.ts(1 hunks)libs/application/templates/aosh/street-registration/src/lib/messages/applicationCheck.ts(1 hunks)libs/application/templates/aosh/street-registration/src/lib/messages/information.ts(1 hunks)libs/application/templates/aosh/street-registration/src/utils/getSelectedMachine.ts(2 hunks)libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.tsx(2 hunks)libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.ts(1 hunks)libs/clients/work-machines/src/lib/workMachines.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
libs/application/templates/aosh/street-registration/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/aosh/street-registration/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/aosh/street-registration/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/aosh/street-registration/src/lib/StreetRegistrationTemplate.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/work-machines/src/lib/workMachines.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/aosh/street-registration/src/forms/StreetRegistrationForm/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/template-api-modules/src/lib/modules/templates/aosh/street-registration/street-registration.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/aosh/street-registration/src/lib/dataSchema.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/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.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/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.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/aosh/street-registration/src/utils/getSelectedMachine.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/aosh/street-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.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 (1)
libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: prepare
🔇 Additional comments (6)
libs/application/templates/aosh/street-registration/src/dataProviders/index.ts (1)
19-22: LGTM! The new data provider is well-structured.The implementation follows the established pattern and maintains consistency with other data providers in the file.
Let's verify the usage of this new data provider:
✅ Verification successful
✓ Data provider is properly integrated and used
The
GetAvailableRegistrationTypesis correctly used within the street registration template's prerequisite section and main template file.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to GetAvailableRegistrationTypes rg "GetAvailableRegistrationTypes" --type tsLength of output: 750
libs/application/templates/aosh/street-registration/src/lib/messages/applicationCheck.ts (1)
20-24: LGTM! Clear and consistent error message.The new validation message follows the established pattern and provides clear feedback for invalid machine types.
libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.ts (1)
47-51: Type annotation improvement looks good.The explicit type annotation for getValueViaPath improves type safety.
libs/application/templates/aosh/street-registration/src/lib/StreetRegistrationTemplate.ts (1)
33-33: LGTM! The API integration for registration type validation is well-structured.The addition of
GetAvailableRegistrationTypesto the API array enhances the applicant role's capabilities by enabling validation of machine registration types.Also applies to: 115-115
libs/application/templates/aosh/street-registration/src/lib/messages/information.ts (1)
85-89: LGTM! Clear and descriptive error message.The new error message effectively communicates when a machine's registration type is invalid, improving the user experience.
libs/clients/work-machines/src/lib/workMachines.service.ts (1)
305-309: LGTM! Well-implemented service method.The new method follows the service's established patterns for auth middleware and API calls.
libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.ts
Outdated
Show resolved
Hide resolved
...et-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.ts
Outdated
Show resolved
Hide resolved
...pi-modules/src/lib/modules/templates/aosh/street-registration/street-registration.service.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 6 Total Test Services: 0 Failed, 5 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
libs/application/templates/aosh/register-new-machine/src/index.ts (1)
6-6: LGTM! Consider applying similar type exports for shared types.The change to use
export typeis a good practice as it:
- Makes the type nature of
NewMachineAnswersexplicit- Improves tree-shaking during compilation
Consider using explicit type exports for types in
shared/typesas well:-export * from './shared/types' +export type * from './shared/types'libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.ts (2)
49-59: Add input validation for robustness.While the TypeScript improvements are good, the function should validate its inputs.
Consider this improvement:
export const mustInspectBeforeStreetRegistration = ( externalData: ExternalData, regNumber: string, ) => { + if (!regNumber || regNumber.length < REGISTRATION_TYPE_LENGTH) { + return false + } const inspectBeforeTypes = getValueViaPath<string[]>( externalData, 'typesMustInspectBeforeRegistration.data', [], ) return ( inspectBeforeTypes?.includes( regNumber.substring(0, REGISTRATION_TYPE_LENGTH), ) || false ) }
61-83: Extract shared logic to reduce duplication.The function implementation is good, but there's duplicated logic for accessing external data that could be extracted.
Consider creating a shared utility:
const getRegistrationTypes = (externalData: ExternalData) => { const validTypes = getValueViaPath<string[]>( externalData, 'availableRegistrationTypes.data', [], ); const inspectBeforeTypes = getValueViaPath<string[]>( externalData, 'typesMustInspectBeforeRegistration.data', [], ); return { validTypes, inspectBeforeTypes }; };This would make both functions more maintainable and reduce duplication.
libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.ts (1)
22-38: Improve null handling in validation checks.The function properly centralizes tag label logic, but the null coalescing for regNumber could be moved to a single place to avoid repetition.
Consider this improvement:
const getTagLabel = ( externalData: ExternalData, machine: MachineDto, formatMessage: (message: MessageDescriptor) => string, ): string => { + const regNumber = machine?.regNumber || '' if ( - mustInspectBeforeStreetRegistration(externalData, machine?.regNumber || '') + mustInspectBeforeStreetRegistration(externalData, regNumber) ) { return formatMessage( information.labels.pickMachine.inspectBeforeRegistration, ) } - if (isInvalidRegistrationType(externalData, machine?.regNumber || '')) { + if (isInvalidRegistrationType(externalData, regNumber)) { return formatMessage(information.labels.pickMachine.invalidRegistrationType) } return machine?.status || '' }libs/application/templates/aosh/street-registration/src/lib/messages/error.ts (1)
46-50: Add a period for message consistency.The new error message looks good but should include a period at the end of the
defaultMessageto maintain consistency with other messages in the file (e.g., "nameByNationalId" message).errorGetFromAOSH: { id: 'aosh.sr.application:error.errorGetFromAOSH', - defaultMessage: 'Ekki tókst að sækja gögn frá Vinnueftirlitinu', + defaultMessage: 'Ekki tókst að sækja gögn frá Vinnueftirlitinu.', description: 'Failed to fetch data from AOSH', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
libs/application/template-api-modules/src/lib/modules/templates/aosh/street-registration/street-registration.service.ts(2 hunks)libs/application/templates/aosh/register-new-machine/src/index.ts(1 hunks)libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.ts(3 hunks)libs/application/templates/aosh/street-registration/src/index.ts(1 hunks)libs/application/templates/aosh/street-registration/src/lib/messages/error.ts(1 hunks)libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/template-api-modules/src/lib/modules/templates/aosh/street-registration/street-registration.service.ts
🧰 Additional context used
📓 Path-based instructions (5)
libs/application/templates/aosh/street-registration/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/aosh/register-new-machine/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/aosh/street-registration/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/aosh/street-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.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/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.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 (5)
libs/application/ui-fields/src/lib/FindVehicleFormField/FindVehicleFormField.util.ts (2)
11-11: LGTM! Good practice extracting the magic number.The constant improves code maintainability and readability.
Line range hint
1-83: Excellent adherence to library code guidelines.The implementation follows best practices for library code:
- Uses TypeScript effectively with proper type definitions
- Exports are named for better tree-shaking
- Functions are pure and reusable across different NextJS apps
libs/application/templates/aosh/street-registration/src/forms/StreetRegistrationForm/InformationSection/pickMachineSubSection.ts (2)
9-20: LGTM! Well-structured imports with proper TypeScript types.The new imports enhance type safety and properly separate concerns for machine validation.
75-87: Simplify duplicate disabled state check.The condition for tag visibility duplicates the disabled state check.
Consider extracting the disabled check to a variable:
+ const isDisabled = + machine?.disabled || + isMachineDisabled(externalData, machine?.regNumber || '') return { value: machine.id || '', label: machine?.regNumber || '', subLabel: `${machine.category}: ${machine.type} - ${machine.subType}`, - disabled: machine?.disabled - ? true - : isMachineDisabled(externalData, machine?.regNumber || '') || - false, + disabled: isDisabled, tag: - machine?.disabled || - isMachineDisabled(externalData, machine?.regNumber || '') + isDisabled ? { label: getTagLabel( externalData, machine, useLocale().formatMessage, ), variant: 'red', outlined: true, } : undefined,libs/application/templates/aosh/street-registration/src/index.ts (1)
12-12: LGTM! Verify usage of the new error messages.The new export follows good module practices and aligns with the PR's error handling objectives.
Let's verify the usage of these error messages across the codebase:
✅ Verification successful
✓ Error messages module is properly structured
The module follows the established pattern used across other application templates in the codebase, using react-intl for internationalization. The messages are well-defined with proper IDs, descriptions, and default values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of error messages from this module rg -l "import.*from.*street-registration.*messages.*error"Length of output: 60
Script:
#!/bin/bash # Check the content of the error messages file cat libs/application/templates/aosh/street-registration/src/lib/messages/error.ts # Search for any imports containing 'street-registration' and 'error' rg "from.*street-registration.*error" # Search for any imports with partial paths rg "from.*messages/error"Length of output: 3390
...
Attach a link to issue if relevant
What
Fixing error messages on certain types of machines.
Fixing issue that could cause user to advance if all their machines are 'disabled'
Adding more complete error catching for all types that require inspection before registration as well as those that should not be allowed to register at all.
Specify what you're trying to achieve
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
The updates provide more robust handling of machine registration processes, with improved validation and user guidance.