fix(inheritance-report): Fixes for go live #17295
Conversation
WalkthroughThis pull request introduces several modifications across multiple files in the inheritance report template. The changes primarily focus on enhancing the data structure and form handling for inheritance-related applications. Key modifications include adding an 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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17295 +/- ##
==========================================
- Coverage 35.70% 35.68% -0.02%
==========================================
Files 6924 6951 +27
Lines 148637 148660 +23
Branches 42471 42451 -20
==========================================
- Hits 53065 53047 -18
- Misses 95572 95613 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 80 files 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: 0
🧹 Nitpick comments (2)
libs/application/templates/inheritance-report/src/lib/dataSchema.ts (1)
58-58: LGTM: Schema updated for asset enablementThe addition of the
enabledboolean field to the asset schema aligns with the mapper changes. Consider adding a JSDoc comment to document the purpose of this field.+ /** Indicates whether the asset should be included in calculations */ enabled: z.boolean(),libs/application/templates/inheritance-report/src/lib/messages.ts (1)
711-725: Consider adding more descriptive message descriptions.The message definitions are well-structured and maintain consistency with the file's pattern. However, the description field could be more informative to help other developers understand the context and usage of these messages.
Consider adding more descriptive message descriptions:
stocksFaceValue: { id: 'ir.application:stocksFaceValue', defaultMessage: 'Nafnverð á dánardegi', - description: '', + description: 'Label for stock face value field at date of death', }, stocksFaceValuePrePaid: { id: 'ir.application:stocksFaceValuePrePaid', defaultMessage: 'Nafnverð', - description: '', + description: 'Label for stock face value field in prepaid inheritance context', }, stocksRateOfChange: { id: 'ir.application:stocksRateOfChange', defaultMessage: 'Gengi á dánardegi', - description: '', + description: 'Label for stock rate of change field at date of death', }, stocksRateOfChangePrePaid: { id: 'ir.application:stocksRateOfChangePrePaid', defaultMessage: 'Gengi', - description: '', + description: 'Label for stock rate of change field in prepaid inheritance context', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
libs/application/template-api-modules/src/lib/modules/templates/inheritance-report/utils/mappers.ts(3 hunks)libs/application/templates/inheritance-report/src/forms/prerequisites.ts(0 hunks)libs/application/templates/inheritance-report/src/forms/sections/assets.ts(3 hunks)libs/application/templates/inheritance-report/src/forms/sections/debtsAndFuneralCost.ts(1 hunks)libs/application/templates/inheritance-report/src/forms/sections/prepaidInheritance/applicant.ts(1 hunks)libs/application/templates/inheritance-report/src/forms/sections/prepaidInheritance/inheritanceExecutor.ts(2 hunks)libs/application/templates/inheritance-report/src/lib/dataSchema.ts(1 hunks)libs/application/templates/inheritance-report/src/lib/messages.ts(1 hunks)libs/application/templates/inheritance-report/src/types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- libs/application/templates/inheritance-report/src/forms/prerequisites.ts
🧰 Additional context used
📓 Path-based instructions (8)
libs/application/templates/inheritance-report/src/forms/sections/debtsAndFuneralCost.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/inheritance-report/src/forms/sections/prepaidInheritance/inheritanceExecutor.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/inheritance-report/utils/mappers.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/inheritance-report/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/inheritance-report/src/types.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/inheritance-report/src/forms/sections/prepaidInheritance/applicant.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/inheritance-report/src/forms/sections/assets.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/inheritance-report/src/lib/messages.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 (12)
libs/application/templates/inheritance-report/src/forms/sections/prepaidInheritance/applicant.ts (2)
Line range hint 1-57: Implementation looks good!
The code follows best practices:
- Proper use of TypeScript for type safety
- Reusable form building components
- Clear structure and organization
- Consistent use of message constants for internationalization
23-23: Verify accessibility impact of removing the field title
Removing the title from the national ID field might impact screen reader accessibility. Please ensure that:
- The field remains properly labeled for screen readers
- The design system accounts for this empty title
- The user can still understand the purpose of this field
libs/application/templates/inheritance-report/src/forms/sections/prepaidInheritance/inheritanceExecutor.ts (1)
29-29: Consider accessibility implications of empty titles
Removing titles from form fields might impact accessibility. Ensure that these fields have proper aria-labels or other accessibility features to maintain usability for screen readers.
Consider implementing a consistent pattern for field labeling across the application, possibly using:
- Floating labels
- Hidden labels with aria-label attributes
- Label positioning through CSS
Also applies to: 76-76
libs/application/templates/inheritance-report/src/forms/sections/debtsAndFuneralCost.ts (1)
90-92: Verify handling of new debt type
The addition of InsuranceInstitute as a debt type looks good, but ensure that:
- All downstream processing handles this new debt type correctly
- Validation logic accommodates this new type
✅ Verification successful
Let me gather more information about the validation and processing of debt types.
Let me try a different search pattern to find any validation or processing logic.
Based on the search results, I can now provide a definitive response about the debt type validation and processing:
The addition of InsuranceInstitute debt type is properly handled
The codebase shows that:
- The new
InsuranceInstitutedebt type is properly defined in the local enumDebtTypesintypes.ts - The debt type is correctly added to the form selections alongside other existing debt types
- No special validation is required as it follows the same pattern as other debt types in the form structure
The implementation is consistent with the existing debt types pattern and doesn't require additional validation logic since:
- Debt types are handled through a simple enum selection in the form
- The form component handles all debt types uniformly through the selections array
- No special processing logic exists for individual debt types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any validation logic related to debt types
rg -A 5 "DebtTypes\." $(fd -e ts -e tsx)
# Check for any processing of debt types
ast-grep --pattern 'function $_($$_) {
$$$
DebtTypes.$_
$$$
}'
Length of output: 91002
Script:
#!/bin/bash
# Check for validation logic specifically around debt types
ast-grep --pattern 'function $_($$_) {
$$$
DebtTypes.$_
$$$
validate
$$$
}'
# Check for any transformations or processing of debt types
ast-grep --pattern 'function $_($$_) {
$$$
DebtTypes.$_
$$$
transform
$$$
}'
# Look for any mappers or converters that handle debt types
ast-grep --pattern 'const $_ = {
$$$
DebtTypes.$_
$$$
}'
Length of output: 241
Script:
#!/bin/bash
# Search for files that import and use DebtTypes
rg -l "import.*DebtTypes" $(fd -e ts -e tsx)
# Look for any validation or processing code in those files
rg "DebtTypes" -A 10 -B 10 $(fd -e ts -e tsx)
Length of output: 110402
libs/application/templates/inheritance-report/src/types.ts (2)
308-308: LGTM: New debt type addition
The addition of InsuranceInstitute to DebtTypes is properly implemented.
Line range hint 266-271: Address TODO comment and verify RelationEnum changes
The RelationEnum has been significantly modified, changing from relationship types (PARENT, CHILD, etc.) to role types (REPRESENTATIVE, HEIR, etc.). This is a breaking change that requires careful verification.
The TODO comment "do these value labels make sense?" should be addressed before going live.
libs/application/template-api-modules/src/lib/modules/templates/inheritance-report/utils/mappers.ts (4)
160-160: LGTM: Consistent implementation of the enabled property
The addition of the enabled property with a default value of true is implemented consistently across guns, realEstate, and vehicles assets. This maintains backward compatibility while supporting the new feature.
Also applies to: 203-203, 232-232
Line range hint 497-500: LGTM: Improved context-specific labeling
The conditional title implementation for stocks amount field appropriately handles different inheritance contexts, enhancing user experience with context-specific labels.
Line range hint 506-509: LGTM: Consistent conditional labeling pattern
The exchange rate field follows the same conditional title pattern as the amount field, maintaining consistency in the UI.
Line range hint 348-348: Verify data migration for bank account field renaming
The field renaming from 'propertyValuation' to 'amount' is semantically better, but requires verification of existing data migration.
Also applies to: 379-379
libs/application/templates/inheritance-report/src/forms/sections/assets.ts (2)
Line range hint 348-379: LGTM: Consistent field renaming in form definition
The bank account field renaming is consistently applied in the form definition, including the sumField reference.
497-509: LGTM: Well-structured conditional field labels
The conditional titles for stocks fields are well-implemented, providing appropriate context-specific labels while maintaining consistency with the codebase patterns.
albinagu
added
automerge
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(inheritance-report): Fixes for go live (#17295) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(inheritance-report): bugfixing 20.12 (#17322) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
…#17342) * fix(inheritance-report): Fixes for go live (#17295) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(inheritance-report): bugfixing 20.12 (#17322) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(inheritance-report): Send uploaded files data to syslumenn (#17325) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: juni-haukur <[email protected]>
Checklist:
Summary by CodeRabbit
enabledproperty for asset types (guns, real estate, vehicles) and heirs in the inheritance report, defaulting totrue.DebtTypes.InsuranceInstitute.enabledproperty.