fix(application-system): Error messages, input validation, code cleanup #17571
Conversation
…pplication, Fixing time and date issue if accident date is today and time is into the future
…o contentful string
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. WalkthroughThis pull request introduces several modifications to the Work Accident Notification application, focusing on enhancing error handling, validation, and user experience. Changes span multiple files across the application, including service logic, form validation, message definitions, and API scopes. The modifications aim to improve data integrity, provide clearer user feedback, and refine the application's overall robustness. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/EmployeeSection/employee.ts (1)Pattern
🔇 Additional comments (1)
✨ 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: 2
🧹 Nitpick comments (7)
libs/application/templates/aosh/work-accident-notification/src/fields/Occupation/index.tsx (3)
4-4: Remove unused ErrorMessage import.The
ErrorMessagecomponent is imported but never used in the code. Remove it to maintain effective tree-shaking.-import { Box, ErrorMessage, Select } from '@island.is/island-ui/core' +import { Box, Select } from '@island.is/island-ui/core'
178-186: Extract duplicate validation logic into a reusable function.The validation logic is duplicated between
onChangeSubMajorGroupandonChangeMinorGroup. Extract it into a shared function to improve maintainability.+const updateOccupationIfValid = ( + selectedGroup: Options, + groupOptions: VictimsOccupationDto[], + idx: number, + setValue: Function, +) => { + if (selectedGroup?.value) { + const isValidToSelect = findGroupByCode( + groupOptions, + selectedGroup.value, + )?.validToSelect + if (isValidToSelect) { + setValue(`employee[${idx}].victimsOccupation`, selectedGroup) + } + } +} const onChangeSubMajorGroup = (value?: EventOption) => { if (!value || !value.value || !value.label) return const selectedGroup: Options = { value: value.value, label: value.label, } setSelectedSubMajorGroup(selectedGroup) - if (selectedGroup?.value) { - const isValidToSelect = findGroupByCode( - subMajorGroupOptions, - selectedGroup.value, - )?.validToSelect - if (isValidToSelect) { - setValue(`employee[${idx}].victimsOccupation`, selectedGroup) - } - } + updateOccupationIfValid(selectedGroup, subMajorGroupOptions, idx, setValue) // ... rest of the function } const onChangeMinorGroup = (value?: EventOption) => { if (!value || !value.value || !value.label) return const selectedGroup: Options = { value: value.value, label: value.label, } setSelectedMinorGroup(selectedGroup) - if (selectedGroup?.value) { - const isValidToSelect = findGroupByCode( - minorGroupOptions, - selectedGroup.value, - )?.validToSelect - if (isValidToSelect) { - setValue(`employee[${idx}].victimsOccupation`, selectedGroup) - } - } + updateOccupationIfValid(selectedGroup, minorGroupOptions, idx, setValue) // ... rest of the function }Also applies to: 207-215
Line range hint
1-494: Consider splitting the component for better reusability.The component is quite large and handles multiple responsibilities (search, validation, state management). Consider splitting it into smaller, focused components:
- OccupationSearch
- OccupationGroupSelect
- OccupationForm
This would improve maintainability and reusability across different NextJS apps.
libs/application/template-api-modules/src/lib/modules/templates/aosh/work-accident-notification/work-accident-notification.service.ts (1)
108-113: Consider enhancing error handling with specific error details.The current implementation throws a generic error message. Consider:
- Including the original error details in the thrown error
- Using a custom error class for better error handling upstream
- throw Error('Error submitting application to AOSH') + throw new TemplateApiError( + { + title: 'AOSH Submission Error', + summary: error.message || 'Error submitting application to AOSH', + }, + 500, + )libs/application/templates/aosh/work-accident-notification/src/lib/dataSchema.ts (2)
30-49: Add edge case handling for time validation.The time validation for the current date could have edge cases around midnight. Consider adding a small buffer time to prevent validation issues during form submission.
if (date.toDateString() === now.toDateString()) { const hours = parseInt(data.time.slice(0, 2)) const minutes = parseInt(data.time.slice(2, 4)) const inputTime = new Date() inputTime.setHours(hours, minutes, 0, 0) + // Add a 1-minute buffer for form submission + const bufferTime = new Date(now.getTime() + 60000) - if (inputTime > now) { + if (inputTime > bufferTime) { return false } }
82-90: Add input sanitization for employment rate.While the validation is correct, consider sanitizing the input by trimming whitespace and handling decimal values.
employmentRate: z.string().refine( (v) => { + const sanitized = v.trim() + if (!/^\d+\.?\d*$/.test(sanitized)) return false - const rateNum = parseInt(v, 10) + const rateNum = parseFloat(sanitized) return rateNum > 0 && rateNum <= 100 }, { message: '1-100%', }, ),libs/application/templates/aosh/work-accident-notification/src/lib/messages/externalData.ts (1)
Line range hint
27-41: Consider breaking down the long announcement message.The announcement message contains multiple important pieces of information mixed together. Consider:
- Breaking it into smaller, focused messages for better maintainability
- Using a structured format (like a list) for better readability
- Moving the business logic (user role validation) to a separate component
Example structure:
announcementWarning: { id: '...#markdown', defaultMessage: 'Vinsamlega athugaðu að þú ert innskráð/ur sem einstaklingur.', }, announcementEmployerDuty: { id: '...#markdown', defaultMessage: 'Atvinnurekanda ber skylda til að...', }, announcementLoginAdvice: { id: '...#markdown', defaultMessage: 'Ef þú ert að skrá þessa tilkynningu fyrir hönd fyrirtækis...', }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
libs/application/template-api-modules/src/lib/modules/templates/aosh/work-accident-notification/work-accident-notification.service.ts(2 hunks)libs/application/templates/aosh/work-accident-notification/src/fields/Occupation/index.tsx(7 hunks)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/AccidentSection/about.ts(3 hunks)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/AnnouncementSection/index.ts(2 hunks)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/ConclusionSection/index.ts(1 hunks)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/EmployeeSection/employee.ts(2 hunks)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/InformationSection/companySection.ts(2 hunks)libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/InformationSection/projectPurchase.ts(0 hunks)libs/application/templates/aosh/work-accident-notification/src/lib/WorkAccidentNotificationTemplate.ts(1 hunks)libs/application/templates/aosh/work-accident-notification/src/lib/dataSchema.ts(3 hunks)libs/application/templates/aosh/work-accident-notification/src/lib/messages/employee.ts(1 hunks)libs/application/templates/aosh/work-accident-notification/src/lib/messages/externalData.ts(1 hunks)libs/auth/scopes/src/lib/api.scope.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/InformationSection/projectPurchase.ts
🧰 Additional context used
📓 Path-based instructions (12)
libs/auth/scopes/src/lib/api.scope.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/work-accident-notification/src/forms/WorkAccidentNotificationForm/AccidentSection/about.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/work-accident-notification/src/forms/WorkAccidentNotificationForm/InformationSection/companySection.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/work-accident-notification/work-accident-notification.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/work-accident-notification/src/lib/messages/employee.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/work-accident-notification/src/forms/WorkAccidentNotificationForm/ConclusionSection/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/work-accident-notification/src/lib/WorkAccidentNotificationTemplate.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/work-accident-notification/src/forms/WorkAccidentNotificationForm/AnnouncementSection/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/work-accident-notification/src/lib/messages/externalData.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/work-accident-notification/src/forms/WorkAccidentNotificationForm/EmployeeSection/employee.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/work-accident-notification/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/templates/aosh/work-accident-notification/src/fields/Occupation/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 (13)
libs/auth/scopes/src/lib/api.scope.ts (1)
31-31: LGTM! Good separation of concerns for accident-related functionality.The new scope
vinnueftirlitidAccidentprovides more granular access control compared to the generalvinnueftirlitidscope, following the principle of least privilege.Let's verify if this new scope is used consistently:
✅ Verification successful
Scope implementation verified and consistent
The new
vinnueftirlitidAccidentscope is properly implemented and consistently used:
- Defined in the central scopes file
- Correctly used in the work accident notification template
- No instances of using the general scope for accident-related functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any usage of the old scope in accident-related files rg "vinnueftirlitid[^A]" -g "*accident*" # Search for usage of the new scope rg "vinnueftirlitidAccident" -g "*.ts"Length of output: 345
libs/application/templates/aosh/work-accident-notification/src/lib/WorkAccidentNotificationTemplate.ts (1)
48-48: LGTM! Proper implementation of the new granular scope.The template correctly uses the new accident-specific scope, maintaining consistency with the scope definition changes.
libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/ConclusionSection/index.ts (1)
14-14: LGTM! Improved section title.Adding a meaningful title enhances the user experience and maintains consistency with the tabTitle.
libs/application/template-api-modules/src/lib/modules/templates/aosh/work-accident-notification/work-accident-notification.service.ts (1)
57-57: LGTM! Added correlation ID for better traceability.Using application.id as correlation ID helps in tracking requests through the system.
libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/AccidentSection/about.ts (1)
132-132: Verify the reason for reducing maxLength to 498 characters.The maxLength has been reduced by 2 characters for three text fields. This seems like a specific constraint, possibly related to backend processing or database limitations.
Also applies to: 141-141, 150-150
libs/application/templates/aosh/work-accident-notification/src/lib/messages/employee.ts (1)
164-169: LGTM! Well-crafted alert message with legal context.The new alert message effectively:
- Warns about self-reporting restrictions
- References relevant legislation (law 46/1980, article 79)
- Provides clear guidance for both employees and self-employed individuals
libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/EmployeeSection/employee.ts (1)
24-25: LGTM! Well-typed imports from shared packages.The new imports follow TypeScript best practices and promote code reusability by utilizing shared type definitions.
libs/application/templates/aosh/work-accident-notification/src/lib/dataSchema.ts (2)
66-66: LGTM! Nullable postnumber field.Making postnumberOfBranch nullable aligns with the optional nature of branch information in the form.
12-14: Consider adding timezone handling to time validation.The time validation should explicitly handle timezone differences to ensure consistent behavior across different regions.
const TimeWithRefine = z .string() - .refine((x) => (x ? isValid24HFormatTime(x) : false), {}) + .refine((x) => (x ? isValid24HFormatTime(x) : false), { + message: 'Invalid time format. Please use 24-hour format (HHMM)', + })libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/InformationSection/companySection.ts (1)
Line range hint
177-201: LGTM! Improved parameter handling in conditions.The changes properly handle unused parameters using the underscore convention, which is a TypeScript best practice. The conditional logic remains correct while improving code clarity.
libs/application/templates/aosh/work-accident-notification/src/forms/WorkAccidentNotificationForm/AnnouncementSection/index.ts (2)
8-8: LGTM! Import changes improve tree-shaking.The specific import of
FormValueinstead of the entireApplicationtype improves tree-shaking capabilities.
23-25: Consider accessibility implications of empty title.An empty title in a description field might affect screen readers and accessibility. If the heading is intentionally removed, consider adding an aria-label or using an alternative accessible structure.
✅ Verification successful
Empty title is part of the established design pattern.
The empty title in this description field follows a common pattern used throughout the codebase for supplementary content and layout purposes. The accessibility is maintained through:
- Parent section headings providing context
- Description text for screen readers
- Proper heading hierarchy with titleVariant properties
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other empty titles in description fields ast-grep --pattern 'buildDescriptionField({ $$$ title: "", $$$ })'Length of output: 74
Script:
#!/bin/bash # Check for buildDescriptionField usage with broader pattern ast-grep --pattern 'buildDescriptionField({$$$})' # Also check the component implementation rg -l "buildDescriptionField" --type ts # Look for similar title patterns rg "title: ['\"]{2}" --type tsLength of output: 66858
libs/application/templates/aosh/work-accident-notification/src/lib/messages/externalData.ts (1)
27-27: Verify markdown support implementation.The addition of #markdown suffix suggests markdown formatting support. Ensure that:
- The markdown parser is properly implemented
- The content is safely sanitized
- The rendering component handles markdown correctly
✅ Verification successful
Markdown implementation is properly secured and well-implemented
The markdown implementation:
- Uses a reliable markdown parser (markdown-to-jsx)
- Implements proper sanitization by disabling raw HTML parsing
- Uses secure component overrides from the UI library
- Handles links safely with configurable behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for markdown handling implementation rg -l "markdown|sanitize|parse" --type tsLength of output: 53078
Script:
#!/bin/bash # Check for Markdown component usage in the template rg -A 5 "Markdown" "libs/application/templates/aosh/work-accident-notification/" # Check Markdown component implementation cat "libs/shared/components/src/Markdown/Markdown.tsx" # Check markdown options and sanitization cat "libs/shared/components/src/Markdown/markdownOptions.tsx"Length of output: 2122
libs/application/templates/aosh/work-accident-notification/src/fields/Occupation/index.tsx
Show resolved
Hide resolved
...ork-accident-notification/src/forms/WorkAccidentNotificationForm/EmployeeSection/employee.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 10 Total Test Services: 0 Failed, 10 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17571 +/- ##
=======================================
Coverage 35.57% 35.57%
=======================================
Files 7027 7027
Lines 150428 150429 +1
Branches 42940 42940
=======================================
+ Hits 53516 53518 +2
+ Misses 96912 96911 -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.
|
github-merge-queue
bot
removed this pull request from the merge queue due to an unknown Git error
...
Attach a link to issue if relevant
What
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
New Features
Bug Fixes
Documentation
Refactor
Style