fix(appliaction-estate): Catch if preregistered heirs are foreign citizens #16362
Conversation
WalkthroughThe pull request involves significant modifications across various files in the estate template module. Key changes include the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
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
|
juni-haukur
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (26)
libs/application/templates/estate/src/forms/Sections/approveSubmission.ts (1)
Line range hint
9-41: Consider enhancing type safety with explicit type annotationsThe code structure looks good and follows our guidelines for TypeScript usage and reusability. To further improve type safety, consider adding explicit type annotations to the
buildSectionandbuildMultiFieldfunction calls. This would make the code more robust and self-documenting.For example:
export const approvePrivateDivisionSubmission = buildSection<YourFormType>({ // ... existing code })Replace
YourFormTypewith the appropriate type for your form structure.libs/application/templates/estate/src/forms/Sections/estateWithoutAssets.ts (3)
7-8: Approve changes in import statements with a minor suggestion.The changes improve consistency by using core constants for boolean values and local constants for language-specific terms. This aligns well with effective tree-shaking and bundling practices.
Consider combining the imports from
@island.is/application/coreinto a single line for better readability:import { buildDescriptionField, buildSection, buildMultiField, buildRadioField, getValueViaPath, YES, NO } from '@island.is/application/core'Also applies to: 11-11
Line range hint
28-31: Approve changes in radio field options with a suggestion for improved type safety.The changes improve type safety and internationalization support by using consistent boolean constants for values and language-specific constants for labels. This aligns well with TypeScript usage guidelines for defining props.
To further enhance type safety, consider defining a type for the radio options:
type RadioOption = { label: string; value: typeof YES | typeof NO; }; const options: RadioOption[] = [ { label: JA, value: YES }, { label: NEI, value: NO }, ];Then use this type in the
buildRadioFieldfunction:buildRadioField<RadioOption>({ // ... other props options: options, });This will ensure that all radio fields consistently use the correct option structure.
Also applies to: 39-42
Line range hint
1-68: Overall assessment: Improvements in consistency and type safety.The changes in this file align well with the coding guidelines for the
libs/**/*path pattern. The modifications enhance reusability, improve type safety, and support effective tree-shaking. The use of core constants and local language-specific constants improves consistency and internationalization support.To further improve the module's architecture:
- Consider creating a dedicated types file for commonly used types like radio options.
- Evaluate if other parts of the estate template could benefit from similar consistency improvements.
- Document the usage of core constants vs. local constants to guide future development.
libs/application/templates/estate/src/forms/Sections/spouseOfTheDeceased.ts (1)
Line range hint
38-39: Ensure consistency between labels and values in radio optionsThe values for the radio options have been updated to use the new
YESandNOconstants, which is good. However, the labels still useJAandNEI. This inconsistency might lead to confusion.Consider updating the labels to match the values for better consistency:
options: [ { label: YES, value: YES }, { label: NO, value: NO }, ],If
JAandNEIare specific Icelandic terms that need to be retained, please add a comment explaining this choice for clarity.libs/application/templates/estate/src/types.ts (1)
53-53: LGTM: Enhanced type safety and consistencyThe update to use
typeof YES | typeof NOfor theknowledgeOfOtherWillsproperty improves type safety and consistency. This change aligns well with our TypeScript usage guidelines.Consider using a more descriptive type alias for better readability:
type YesNo = typeof YES | typeof NO; // Then use it in the Answers type knowledgeOfOtherWills: YesNo;This would improve code readability and make it easier to reuse this type combination elsewhere if needed.
libs/application/templates/estate/src/forms/OverviewSections/confirmAction.ts (2)
1-7: Approved: Import change improves consistencyThe change to import
YESfrom@island.is/application/coreinstead of local constants is a good improvement. It enhances consistency and aligns with the guideline of reusability across different NextJS apps.Consider moving
EstateTypesto@island.is/application/coreas well if it's used across multiple components or applications. This would further improve consistency and reusability.
Line range hint
9-89: LGTM: Consistent usage of YES constantThe usage of the
YESconstant throughout the file is consistent and maintains the existing functionality. The code structure allows for effective tree-shaking and bundling, which is good for performance.To improve readability, consider extracting the complex conditions in the
buildCheckboxFieldcalls into separate functions. For example:const isEstateWithoutAssets = (answers) => getValueViaPath(answers, 'selectedEstate') === EstateTypes.estateWithoutAssets; const hasAssetsAndDebt = (answers) => { const hasAssets = getValueViaPath(answers, 'estateWithoutAssets.estateAssetsExist') === YES; const hasDebt = getValueViaPath(answers, 'estateWithoutAssets.estateDebtsExist') === YES; return hasAssets && hasDebt; }; // Use these functions in the condition fields condition: isEstateWithoutAssets, // ... condition: (answers) => isEstateWithoutAssets(answers) && hasAssetsAndDebt(answers),This would make the code more readable and easier to maintain.
libs/application/template-api-modules/src/lib/modules/templates/estate/types/index.ts (1)
40-40: Approved: Type modification enhances type safety.The update to
noContactInfoproperty type improves type safety and ensures consistency with the core module's definitions. This change aligns well with TypeScript best practices.Consider adding a comment explaining the purpose of the
noContactInfoproperty for better code documentation. For example:/** Indicates whether contact information is available for the estate member */ noContactInfo?: (typeof YES | typeof NO)[]libs/application/templates/estate/src/forms/OverviewSections/commonFields.ts (1)
7-7: Approve import changes with a suggestion for improvementThe import changes improve consistency by using the
YESconstant from the core module. This aligns with the coding guidelines for libs/** files, promoting reusability across different NextJS apps.However, the addition of
JAandNEIimports might be redundant if they're used to replace the previousYESconstant. Consider using theYESandNOconstants from '@island.is/application/core' throughout the file for better consistency.Consider replacing
JAandNEIwithYESandNOfrom '@island.is/application/core' respectively:-import { JA, NEI } from '../../lib/constants' +import { YES, NO } from '@island.is/application/core'Then update their usage throughout the file.
Also applies to: 15-15
libs/application/templates/estate/src/forms/Sections/announcerInfo.ts (2)
10-13: Improve consistency in constant usageThe changes to import
YESandNOfrom@island.is/application/coreimprove reusability and consistency across the application. This aligns well with our coding guidelines for libs.However, we're still using local constants
JAandNEI. Consider replacing these with the importedYESandNOfor better consistency and maintainability.Consider updating the usage of
JAandNEIthroughout the file to use the importedYESandNOconstants. This will further improve consistency and make the code more maintainable.
Line range hint
134-139: Approve changes tobuildRadioFieldwith minor suggestionThe use of imported
YESandNOconstants for values in thebuildRadioFieldcomponent improves consistency with the core application. This change aligns well with our coding guidelines for reusability and type safety.Consider adding a comment explaining why
JAandNEIare used for labels whileYESandNOare used for values. This will help future maintainers understand the reasoning behind using different constants for labels and values.libs/application/templates/estate/src/forms/Overviews.ts (1)
8-9: Improved import consistency, consider further optimization.The changes to import
YESandNOfrom@island.is/application/coreimprove consistency and align with the guideline for reusability across different NextJS apps. This is a positive change.Consider evaluating if
EstateTypescould also be moved to a shared location, potentially improving reusability further. IfEstateTypesis specific to this module, please add a comment explaining why it's kept local.Also applies to: 17-17
libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts (1)
Line range hint
34-48: LGTM: Improved type safety and handling of foreign citizenship.The changes in the
estateMemberMapperfunction enhance type safety and consistency:
- Using
[NO]instead of['No']fornoContactInfoimproves type checking.- The new
foreignCitizenshipproperty handles cases wherenationalIdis missing or ends with '0000'.These updates align with the coding guidelines for TypeScript usage and type safety.
Consider extracting the
foreignCitizenshiplogic into a separate function for better readability:const determineForeignCitizenship = (nationalId: string | undefined): (typeof YES | typeof NO)[] => !nationalId || nationalId.endsWith('0000') ? [YES] : [NO]; // Usage in estateMemberMapper foreignCitizenship: element.foreignCitizenship ?? determineForeignCitizenship(element.nationalId),libs/clients/syslumenn/src/lib/syslumennClient.types.ts (3)
226-232: LGTM! Consider using a shared type for consistency.The changes to
EstateMembertype look good. ThenoContactInfoproperty now uses lowercase values, which improves consistency. The addition offoreignCitizenshipaligns with the PR objective of handling foreign citizens.To further improve consistency and maintainability, consider creating a shared type for the
('yes' | 'no')[]pattern:type YesNo = 'yes' | 'no'; export type EstateMember = { // ... other properties noContactInfo?: YesNo[]; foreignCitizenship?: YesNo[]; }This approach would make it easier to update the type if needed in the future and ensure consistency across the codebase.
282-282: LGTM! Consider using the shared type for consistency.The change to the
knowledgeOfOtherWillsproperty in theEstateCommoninterface looks good. It now uses lowercase values, which improves consistency with theEstateMembertype.To maintain consistency with the earlier suggestion, consider using the shared
YesNotype here as well:type YesNo = 'yes' | 'no'; interface EstateCommon { // ... other properties knowledgeOfOtherWills: YesNo; }This approach ensures consistency across the entire file and makes future updates easier.
Line range hint
1-450: Overall, the changes improve consistency and align with PR objectives.The modifications to
EstateMemberandEstateCommontypes enhance consistency by using lowercase values for boolean-like string literals. The addition of theforeignCitizenshipproperty addresses the PR objective of handling foreign citizens.These changes adhere to the coding guidelines for
libs/**/*files:
- They define reusable types that can be used across different NextJS apps.
- They use TypeScript for defining and exporting types.
- The changes don't negatively impact tree-shaking or bundling practices.
To further improve the type definitions:
- Consider creating a shared
YesNotype for the'yes' | 'no'pattern.- Evaluate if other properties in this file could benefit from similar consistency improvements.
- Document the purpose of the new
foreignCitizenshipproperty to provide context for its addition.These enhancements will contribute to better maintainability and clarity of the codebase.
libs/application/templates/estate/src/forms/Sections/estateAssets.ts (1)
9-12: Approved: Improved import consistency, with a suggestion for further enhancement.The change to import
YESfrom@island.is/application/coreimproves consistency and aligns with the coding guideline for reusability across different NextJS apps. This is a positive change.Consider also moving
EstateTypesto the core library if it's used across multiple templates or applications. This would further enhance reusability and consistency.libs/application/templates/estate/src/fields/EstateMembersRepeater/AdditionalEstateMember.tsx (3)
Line range hint
65-68: Improved type safety in useWatch hookThe use of
hasYes(field.foreignCitizenship)in theuseWatchhook enhances type safety and aligns with our TypeScript usage guidelines. This change improves the handling of theforeignCitizenshipfield.For even better clarity, consider using a constant for the empty string default value:
const DEFAULT_FOREIGN_CITIZENSHIP = ''; // ... defaultValue: hasYes(field.foreignCitizenship) ? [YES] : DEFAULT_FOREIGN_CITIZENSHIP,This would make the intent clearer and easier to maintain.
Line range hint
125-157: Improved conditional rendering and form validationThe changes in the conditional rendering logic and the addition of the
requiredprop to theDatePickerControllerare good improvements:
- The explicit check
foreignCitizenship?.[0] === YESenhances code clarity and type safety.- Making the date of birth field required for foreign citizens is a logical improvement.
To maintain consistency across the component, consider extracting the condition into a constant:
const isForeignCitizen = foreignCitizenship?.[0] === YES; // Then use it in the conditional rendering {isForeignCitizen ? ( // ... existing code for foreign citizens ) : ( // ... existing code for non-foreign citizens )}This would make the code more readable and easier to maintain, especially if this condition is used in multiple places within the component.
Line range hint
289-301: Consistent use of core constants in CheckboxControllerThe use of the
YESconstant from the core package in theCheckboxControllerfor foreign citizenship is a good change. It aligns with the earlier import modifications and improves consistency across the application.To further improve code organization and reusability, consider extracting the checkbox option into a constant:
const FOREIGN_CITIZENSHIP_OPTION = { label: formatMessage(m.inheritanceForeignCitizenshipLabel), value: YES, }; // Then use it in the CheckboxController <CheckboxController // ... other props options={[FOREIGN_CITIZENSHIP_OPTION]} // ... rest of the component />This approach would enhance maintainability and make it easier to reuse this option if needed elsewhere in the component or in other components.
libs/application/templates/estate/src/lib/dataSchema.ts (1)
Line range hint
124-130: LGTM: Improved handling of foreign citizenship fornationalIdvalidation.The refinement now correctly checks if
foreignCitizenship[0] === NObefore validating thenationalId, which aligns with the PR objective to handle preregistered heirs who are foreign citizens.Consider adding a comment explaining the logic behind this condition for better maintainability.
.refine( ({ foreignCitizenship, nationalId, enabled }) => { + // Only validate nationalId for non-foreign citizens return enabled && foreignCitizenship?.[0] === NO ? nationalId && kennitala.isValid(nationalId) : true }, { path: ['nationalId'], }, )libs/application/templates/inheritance-report/src/fields/HeirsRepeater/index.tsx (3)
Line range hint
65-76: Improved age validation logic, but consider refactoring for readabilityThe updated logic for checking if an estate member is under 18 now correctly handles both foreign and domestic citizens. The use of
intervalToDurationfor age calculation of foreign citizens is more accurate. However, the complexity of this logic might make it harder to maintain in the future.Consider refactoring this part into a separate function for improved readability and maintainability. For example:
const isUnder18 = (member: EstateMember): boolean => { const hasForeignCitizenship = member?.foreignCitizenship?.[0] === YES; const birthDate = member?.dateOfBirth; const memberAge = hasForeignCitizenship && birthDate ? intervalToDuration({ start: new Date(birthDate), end: new Date() })?.years : info(member.nationalId)?.age; return (memberAge ?? 0) < 18 && (member?.nationalId || birthDate) && member.enabled; };Then you can use this function in your component:
const hasEstateMemberUnder18 = values.estate?.estateMembers?.some(isUnder18);
Line range hint
210-275: Improved flexibility for handling different application types, but function complexity is highThe
updateValuesfunction has been successfully modified to handle both regular and pre-paid inheritance applications. The logic for calculating inheritance values and taxes has been updated accordingly, which improves the overall functionality of the component.However, the function has become quite long and complex. To improve maintainability and testability, consider splitting this function into smaller, more focused functions. For example:
- A function to determine if the heir is a spouse
- A function to calculate inheritance values
- A function to calculate taxes
This refactoring would make the code easier to understand, maintain, and test. It would also align better with the SOLID principles, particularly the Single Responsibility Principle.
Line range hint
441-495: Improved custom field rendering, but error handling could be optimizedThe updated rendering logic for custom fields enhances the component's flexibility, allowing it to handle different types of custom fields appropriately. The structure of the code, using early returns for different field types, improves readability compared to nested if-else statements.
However, the error handling logic is repeated across different field types. Consider extracting this logic into a separate function to reduce code duplication and improve maintainability. For example:
const getFieldError = (error: any, mainIndex: number, fieldId: string) => error && error[mainIndex] ? error[mainIndex][fieldId] : undefined; // Usage error={getFieldError(error, mainIndex, customField.id)}This change would align with the DRY (Don't Repeat Yourself) principle and make the code more maintainable.
libs/clients/syslumenn/src/lib/syslumennClient.utils.ts (1)
405-409: LGTM! Consider a minor improvement for clarity.The addition of the
foreignCitizenshipproperty effectively addresses the PR objective to catch if preregistered heirs are foreign citizens. The logic is sound, assuming that a missing national ID or one ending with '0000' indicates foreign citizenship.For improved clarity, consider using a boolean value instead of an array with 'yes' or 'no'. This would make the property more intuitive to use:
- foreignCitizenship: - !estateRaw.kennitala || estateRaw.kennitala.endsWith('0000') - ? ['yes'] - : ['no'], + isForeignCitizen: + !estateRaw.kennitala || estateRaw.kennitala.endsWith('0000'),This change would make the property easier to use in conditional statements without needing to access an array element.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
- libs/application/template-api-modules/src/lib/modules/templates/estate/consts.ts (0 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/types/index.ts (2 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/utils/fakeData.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts (3 hunks)
- libs/application/templates/estate/src/fields/EstateMembersRepeater/AdditionalEstateMember.tsx (3 hunks)
- libs/application/templates/estate/src/fields/EstateMembersRepeater/index.tsx (3 hunks)
- libs/application/templates/estate/src/forms/OverviewSections/commonFields.ts (1 hunks)
- libs/application/templates/estate/src/forms/OverviewSections/confirmAction.ts (1 hunks)
- libs/application/templates/estate/src/forms/Overviews.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/announcerInfo.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/approveSubmission.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/estateAssets.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/estateDebts.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/estateWithoutAssets.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/spouseOfTheDeceased.ts (1 hunks)
- libs/application/templates/estate/src/forms/Sections/testamentInfo.ts (1 hunks)
- libs/application/templates/estate/src/lib/constants.ts (0 hunks)
- libs/application/templates/estate/src/lib/dataSchema.ts (5 hunks)
- libs/application/templates/estate/src/types.ts (3 hunks)
- libs/application/templates/inheritance-report/src/fields/HeirsRepeater/index.tsx (2 hunks)
- libs/clients/syslumenn/src/lib/syslumennClient.types.ts (2 hunks)
- libs/clients/syslumenn/src/lib/syslumennClient.utils.ts (3 hunks)
💤 Files with no reviewable changes (2)
- libs/application/template-api-modules/src/lib/modules/templates/estate/consts.ts
- libs/application/templates/estate/src/lib/constants.ts
✅ Files skipped from review due to trivial changes (1)
- libs/application/template-api-modules/src/lib/modules/templates/estate/utils/fakeData.ts
🧰 Additional context used
📓 Path-based instructions (19)
libs/application/template-api-modules/src/lib/modules/templates/estate/types/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/template-api-modules/src/lib/modules/templates/estate/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/estate/src/fields/EstateMembersRepeater/AdditionalEstateMember.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/estate/src/fields/EstateMembersRepeater/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/estate/src/forms/OverviewSections/commonFields.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/estate/src/forms/OverviewSections/confirmAction.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/estate/src/forms/Overviews.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/estate/src/forms/Sections/announcerInfo.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/estate/src/forms/Sections/approveSubmission.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/estate/src/forms/Sections/estateAssets.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/estate/src/forms/Sections/estateDebts.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/estate/src/forms/Sections/estateWithoutAssets.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/estate/src/forms/Sections/spouseOfTheDeceased.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/estate/src/forms/Sections/testamentInfo.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/estate/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/estate/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/fields/HeirsRepeater/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/clients/syslumenn/src/lib/syslumennClient.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/clients/syslumenn/src/lib/syslumennClient.utils.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🪛 Biome
libs/application/templates/estate/src/fields/EstateMembersRepeater/index.tsx
[error] 245-245: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 262-262: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (23)
libs/application/templates/estate/src/forms/Sections/approveSubmission.ts (1)
Line range hint
1-7: Approved: Improved import for better reusability and consistencyThe change to import
YESfrom@island.is/application/coreinstead of a local constants file is a good improvement. This aligns with our coding guidelines for better reusability across different NextJS apps and may enhance tree-shaking and bundling practices.libs/application/templates/estate/src/forms/Sections/spouseOfTheDeceased.ts (2)
Line range hint
52-56: Approve updates to condition checksThe condition checks have been correctly updated to use the new
YESconstant. This change maintains consistency with the imported constants while preserving the original logic.Also applies to: 63-67
Line range hint
1-70: Overall improvements with minor adjustments neededThe changes in this file contribute to better reusability and consistency by using shared constants from the core module. The TypeScript usage for defining props and exporting types is maintained, adhering to our coding guidelines.
However, there are a few minor issues to address:
- The missing import for the
JAconstant.- The inconsistency between labels and values in the radio options.
Once these are resolved, the changes will significantly improve the code quality and maintainability.
To ensure all usages of
YES,NO,JA, andNEIare consistent throughout the codebase, please run the following script:This will help identify any inconsistencies in constant usage across the estate template module.
libs/application/templates/estate/src/types.ts (1)
1-1: LGTM: Improved reusability and consistencyThe addition of the import statement for
YESandNOfrom@island.is/application/coreenhances reusability across different NextJS apps and promotes consistency by using shared constants. This change also facilitates effective tree-shaking.libs/application/templates/estate/src/forms/Sections/testamentInfo.ts (1)
8-9: Approve: Improved import management and constant usageThe changes to the import statements enhance code maintainability and consistency:
- Importing
YESandNOfrom@island.is/application/coreinstead of a local constants file promotes reusability across different NextJS apps.- This change ensures consistent usage of these constants throughout the application.
- The modification doesn't affect functionality, as the constants are used in the same way in the
buildRadioFieldoptions.These changes align well with our coding guidelines for libs, particularly in terms of reusability and effective bundling practices.
Also applies to: 12-12
libs/application/templates/estate/src/forms/Sections/estateDebts.ts (3)
Line range hint
1-9: LGTM: Import changes improve reusabilityThe modification to import
YESfrom@island.is/application/coreinstead of local constants aligns with the coding guideline for reusability across different NextJS apps. This change promotes consistency and makes it easier to maintain shared constants.
Line range hint
11-26: Correct usage of imported YES constantThe
YESconstant, now imported from@island.is/application/core, is correctly used in theconditionfunction. The logic remains unchanged, ensuring that the refactoring of the import doesn't affect the functionality of the code.
Line range hint
1-85: Code adheres to guidelines and best practicesThe code in this file adheres to the coding guidelines for the
libs/**/*pattern. It effectively uses TypeScript for defining props and exporting types, and the structure promotes good tree-shaking and bundling practices. The changes made improve reusability while maintaining the existing functionality.libs/application/template-api-modules/src/lib/modules/templates/estate/types/index.ts (2)
1-1: LGTM: Import statement enhances reusability and consistency.The import of
NOandYESconstants from the core module aligns with best practices for maintaining consistency across the application and enhances reusability.
Line range hint
1-40: Adherence to coding guidelines forlibs/**/*filesThe file successfully meets the following guidelines:
- Reusability: The type definitions can be easily reused across different NextJS apps.
- TypeScript usage: Props and types are effectively defined using TypeScript.
The guideline regarding effective tree-shaking and bundling practices is not directly applicable to this file as it only contains type definitions.
libs/application/templates/estate/src/forms/OverviewSections/commonFields.ts (1)
Line range hint
86-89: Improve consistency in constant usage and verify changesThe usage of
YESfor condition checking is correct. However, there's an inconsistency between usingYESfor checking andJA/NEIfor display values. This mix of languages (English and Icelandic) could lead to confusion.Consider using
YESandNOconsistently for both checking and display:-value: ({ answers }) => - getValueViaPath(answers, 'estate.testament.wills') === YES ? JA : NEI, +value: ({ answers }) => + getValueViaPath(answers, 'estate.testament.wills') === YES ? YES : NO,Apply this change to both
buildKeyValueFieldcomponents in this segment.Let's verify the impact of these changes across the codebase:
This script will help us understand the current usage patterns and ensure that our suggested changes are applied consistently across the estate template module.
Also applies to: 92-95
✅ Verification successful
</run_scripts>
Consistency Verified
The usage of
YESfor condition checks andJA/NEIfor display values is consistent and appropriate within the codebase.JAandNEIserve as localized labels, whileYESandNOhandle conditional logic effectively.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of JA, NEI, YES, and NO constants # Test 1: Check for JA and NEI usage echo "Checking for JA and NEI usage:" rg --type typescript "JA|NEI" libs/application/templates/estate/ # Test 2: Check for YES and NO usage from @island.is/application/core echo "Checking for YES and NO usage from @island.is/application/core:" rg --type typescript "import.*YES.*from '@island.is/application/core'" libs/application/templates/estate/ rg --type typescript "import.*NO.*from '@island.is/application/core'" libs/application/templates/estate/ # Test 3: Check for local constant definitions of YES and NO echo "Checking for local constant definitions of YES and NO:" rg --type typescript "const.*YES.*=|const.*NO.*=" libs/application/templates/estate/Length of output: 847
Script:
#!/bin/bash # Description: Check for usage of JA, NEI, YES, and NO constants in TypeScript files # Test 1: Check for JA and NEI usage echo "Checking for JA and NEI usage:" rg "JA|NEI" libs/application/templates/estate/ -g '*.ts' -g '*.tsx' # Test 2: Check for YES and NO usage from @island.is/application/core echo "Checking for YES and NO usage from @island.is/application/core:" rg "import.*YES.*from '@island.is/application/core'" libs/application/templates/estate/ -g '*.ts' -g '*.tsx' rg "import.*NO.*from '@island.is/application/core'" libs/application/templates/estate/ -g '*.ts' -g '*.tsx' # Test 3: Check for local constant definitions of YES and NO echo "Checking for local constant definitions of YES and NO:" rg "const\s+YES\s*=|const\s+NO\s*=" libs/application/templates/estate/ -g '*.ts' -g '*.tsx'Length of output: 4085
libs/application/templates/estate/src/forms/Overviews.ts (1)
Line range hint
1-217: Changes align with coding guidelines and maintain functionality.The modifications to the imports and subsequent usage of
YESandNOconstants throughout the file maintain the existing functionality while improving code consistency. The changes adhere to the following guidelines:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices.
The overall structure of the file, including the use of
buildSection,buildMultiField, and other utility functions, remains intact and continues to support these practices.libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts (2)
12-12: LGTM: Import of shared constants enhances consistency.The import of
NOandYESconstants from@island.is/application/corealigns with the coding guidelines for reusability across different NextJS apps. This change promotes consistency in the codebase.
Line range hint
1-248: Verify: Overall impact of changes is positive and localized.The modifications in this file are well-contained within the
estateMemberMapperfunction and do not introduce breaking changes to the public API. The updates improve type safety and enhance the handling of foreign citizenship cases.To ensure the changes don't have unintended consequences, please run the following verification script:
This script will help ensure that:
- The
NOandYESconstants are properly imported and used.- There are no remaining hardcoded 'No' or 'Yes' strings that should be replaced with the constants.
- The
estateMemberMapperfunction is used consistently across the codebase.✅ Verification successful
Verified: Changes are correctly implemented and localized.
The
NOandYESconstants are properly imported and utilized withinmappers.ts. Additionally, there are no remaining hardcoded 'No' or 'Yes' strings. TheestateMemberMapperfunction is consistently used across the codebase, ensuring uniform behavior.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of NO and YES constants, and check for any remaining hardcoded 'No' or 'Yes' strings. # Test 1: Check for proper import and usage of NO and YES constants echo "Test 1: Verifying import and usage of NO and YES constants" rg --type typescript "import.*\{.*(?:NO|YES).*\}.*from '@island.is/application/core'" libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts rg --type typescript "\b(NO|YES)\b" libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts # Test 2: Check for any remaining hardcoded 'No' or 'Yes' strings echo "Test 2: Checking for hardcoded 'No' or 'Yes' strings" rg --type typescript "'(?:No|Yes)'" libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts # Test 3: Verify usage of estateMemberMapper function echo "Test 3: Verifying usage of estateMemberMapper function" rg --type typescript "estateMemberMapper" libsLength of output: 999
Script:
#!/bin/bash # Description: Verify the usage of NO and YES constants, and check for any remaining hardcoded 'No' or 'Yes' strings. # Test 1: Check for proper import and usage of NO and YES constants echo "Test 1: Verifying import and usage of NO and YES constants" rg "import.*\{.*(?:NO|YES).*\}.*from '@island.is/application/core'" --glob '*.ts' libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts rg "\b(NO|YES)\b" --glob '*.ts' libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts # Test 2: Check for any remaining hardcoded 'No' or 'Yes' strings echo "Test 2: Checking for hardcoded 'No' or 'Yes' strings" rg "'(?:No|Yes)'" --glob '*.ts' libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts # Test 3: Verify usage of estateMemberMapper function echo "Test 3: Verifying usage of estateMemberMapper function" rg "estateMemberMapper" --glob '*.ts' libsLength of output: 2040
libs/application/templates/estate/src/forms/Sections/estateAssets.ts (2)
Line range hint
18-23: LGTM: Correct usage of imported YES constant.The
YESconstant is correctly used in the condition for theestateAssetssection. The logic remains unchanged, maintaining the existing behavior while benefiting from the improved import consistency.
Line range hint
1-359: Approved: Excellent adherence to coding guidelines.The code demonstrates strong adherence to the coding guidelines for
libs/**/*:
- It uses TypeScript for defining props and exporting types.
- The components and hooks are designed for reusability across different NextJS apps.
- The modular structure supports effective tree-shaking and bundling.
The use of utility functions like
buildSection,buildSubSection, etc., promotes consistency and reusability throughout the codebase.libs/application/templates/estate/src/fields/EstateMembersRepeater/AdditionalEstateMember.tsx (2)
19-19: Improved imports for better reusability and tree-shakingThe changes in the import statements, particularly the addition of
hasYesandYESfrom@island.is/application/core, align well with our coding guidelines. This modification enhances reusability across different NextJS apps and supports effective tree-shaking and bundling practices.Also applies to: 23-23
Line range hint
1-324: Overall assessment: Improved type safety and consistencyThe changes in this file demonstrate a commendable effort to enhance type safety, consistency, and alignment with core package usage. The modifications adhere well to the coding guidelines for
libs/**/*files, particularly in terms of:
- Improving reusability across different NextJS apps
- Enhancing TypeScript usage for better type definitions
- Supporting effective tree-shaking and bundling practices
The refactoring of imports, the use of core constants, and the improvements in conditional logic all contribute to a more maintainable and robust component. The suggested minor improvements in the previous comments, if implemented, would further enhance code clarity and organization.
Great job on these improvements! The changes will contribute to a more consistent and maintainable codebase.
libs/application/templates/estate/src/lib/dataSchema.ts (2)
3-3: LGTM: Import changes improve consistency and follow best practices.The change to import
NOandYESfrom '@island.is/application/core' aligns with the coding guidelines for effective tree-shaking and bundling practices. It also enhances consistency by using shared constants across the application.Also applies to: 12-12
105-106: LGTM: Improved type safety fornoContactInfofield.The change to use
z.union([z.literal(YES), z.literal(NO)])for thenoContactInfofield improves type safety and ensures consistency with the rest of the application by using the imported constants.libs/application/templates/inheritance-report/src/fields/HeirsRepeater/index.tsx (1)
19-19: Improved code maintainability with constant importThe addition of the
YESconstant import from '@island.is/application/core' enhances code maintainability and consistency. This change aligns well with the coding guidelines for reusability across different NextJS apps.libs/clients/syslumenn/src/lib/syslumennClient.utils.ts (2)
542-542: LGTM! Consistent with previous update.The change to the
knowledgeOfOtherWillsproperty in themapEstateInfofunction is consistent with the update in themapEstateRegistrantfunction. This consistency is good for maintaining a uniform approach across the codebase.
444-444: LGTM! Verify consistency across the codebase.The update to the
knowledgeOfOtherWillsproperty, converting the boolean value to 'yes' or 'no' strings, looks good. This change likely improves consistency or meets specific API requirements.To ensure this change is consistent with other parts of the codebase, please run the following script:
This will help identify if similar conversions are used elsewhere in the codebase, ensuring a uniform approach.
✅ Verification successful
Please update the search command to use the correct file type and rerun the verification.
To fix the previous error, use
tsas the file type identifier. Please execute the following script to verify consistency across the codebase:
Consistency Verified
All instances of
knowledgeOfOtherWillsconsistently use 'yes' or 'no' strings across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of 'knowledgeOfOtherWills' to verify consistency rg --type typescript 'knowledgeOfOtherWills.*[?:].*yes.*:.*no' -g '!*.test.ts'Length of output: 115
Script:
#!/bin/bash # Search for other occurrences of 'knowledgeOfOtherWills' to verify consistency rg --type ts 'knowledgeOfOtherWills.*[?:].*yes.*:.*no' -g '!*.test.ts'Length of output: 333
libs/application/templates/estate/src/forms/Sections/approveSubmission.ts
Show resolved
Hide resolved
libs/application/templates/estate/src/forms/Sections/spouseOfTheDeceased.ts
Show resolved
Hide resolved
libs/application/templates/estate/src/fields/EstateMembersRepeater/index.tsx
Show resolved
Hide resolved
libs/application/templates/estate/src/fields/EstateMembersRepeater/index.tsx
Show resolved
Hide resolved
libs/application/templates/estate/src/fields/EstateMembersRepeater/index.tsx
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 18 Total Test Services: 0 Failed, 18 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
libs/application/template-api-modules/src/lib/modules/templates/estate/utils/fakeData.ts
Outdated
Show resolved
Hide resolved
libs/application/template-api-modules/src/lib/modules/templates/estate/utils/mappers.ts
Outdated
Show resolved
Hide resolved
libs/application/templates/estate/src/fields/EstateMembersRepeater/index.tsx
Show resolved
Hide resolved
libs/application/templates/inheritance-report/src/fields/HeirsRepeater/index.tsx
Show resolved
Hide resolved
|
Affected services are: api,application-system-api,services-auth-admin-api,services-auth-ids-api,services-auth-delegation-api,services-auth-personal-representative,services-auth-personal-representative-public,services-auth-public-api,services-user-notification,services-user-profile,application-system-form, Deployed services: application-system-api,application-system-form,service-portal-api,api,application-system-api-worker,consultation-portal,web. |
...
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
noContactInfo,foreignCitizenship, andknowledgeOfOtherWills.foreignCitizenshipto theEstateMembertype.