feat(new-primary-school): Data implementation and remove not used pages #16096
Conversation
…ation (#15319) * [TS-806] Implement Allergies and intolerances page - Data implementation * Updated function name
* refactor: updated apitags * fix? clean * chore: nx format:write update dirty files --------- Co-authored-by: Alex Diljar <alex@juni.is> Co-authored-by: andes-it <builders@andes.is>
* Move child page to prerequisites * Update clientConfig * Updated Api module action name
* Relatives - Data implementation * Fixed after review
* feat(new-primary-school): Pronoun Select Field Added a select field for selecting pronoun https://dit-iceland.atlassian.net/browse/TS-811 * Make pronoun full width and change place with preferred name * Use gender data from Júní * Review comment fixes * use defaultValue for pronouns --------- Co-authored-by: hfhelgason <hfhelgason@deloitte.is>
* [TS-816] Implement no children found page * Remove comments
* [TS-883] Remove 'Má sækja barn' - Relatives page * [TS-884] Remove gender - Child info page * [TS-885] Remove use of footage page * Update Child in Review
Co-authored-by: bkristmundsson <bikkibo@gmail.com>
* New school - Data implementation * testing build problems * testing build problems * build problem testing * build problem testing * Rollback build test --------- Co-authored-by: bkristmundsson <bikkibo@gmail.com>
…/github.com/island-is/island.is into feat/new-primary-school-data-implementation
![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
🧹 Outside diff range and nitpick comments (3)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (3)
143-143: Approve changes, but consider adding null checksThe enhancements to
getApplicationExternalDataimprove child-related information management. However, consider adding null checks when accessing nested properties to prevent potential runtime errors. For example:const childGradeLevel = getValueViaPath( externalData, 'childInformation.data.gradeLevel', '', // Provide a default value ) as string const childMemberships = getValueViaPath( externalData, 'childInformation.data.memberships', [], // Provide a default empty array ) as Membership[]This approach ensures type safety and prevents accessing properties of undefined objects.
Also applies to: 170-190, 198-201
319-337: Approve changes, but suggest handling exceptions for 11th, 12th, and 13th gradesThe
formatGradefunction now correctly handles English ordinal suffixes for most grades. However, it doesn't account for the exceptions of 11th, 12th, and 13th grades. Consider modifying the function to handle these cases:export const formatGrade = (gradeLevel: string, lang: Locale) => { let grade = gradeLevel.startsWith('0') ? gradeLevel.substring(1) : gradeLevel if (lang !== 'en') return grade const num = parseInt(grade, 10) if (num >= 11 && num <= 13) return `${grade}th` const lastDigit = num % 10 switch (lastDigit) { case 1: return `${grade}st` case 2: return `${grade}nd` case 3: return `${grade}rd` default: return `${grade}th` } }This modification ensures correct handling of all grade levels in English.
340-348: Approve new function, but suggest adding null checksThe
getCurrentSchoolNamefunction is a useful addition for retrieving the current school name. However, to improve robustness, consider adding null checks:export const getCurrentSchoolName = (application: Application) => { const { primaryOrgId, childMemberships } = getApplicationExternalData( application.externalData ) if (!primaryOrgId || !childMemberships) return undefined return childMemberships .map((membership) => membership.organization) .find((organization) => organization?.id === primaryOrgId)?.name }This modification prevents potential runtime errors when dealing with undefined values.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (5 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.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 (4)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (4)
8-8: LGTM: New imports added correctlyThe new imports (Locale, FriggChildInformation, Membership, SelectOption) are correctly added and likely support the new functionality in the file.
Also applies to: 12-13, 17-17
110-113: Approve changes, but clarification needed on newSchoolHiddenInputThe removal of allergy-related variables aligns with the PR objectives. However, could you please clarify the purpose of the newly added
newSchoolHiddenInputvariable? Understanding its role will help ensure it's being used correctly.Also applies to: 136-136
308-316: LGTM: Well-implemented utility functionThe
getSelectedOptionLabelfunction is a well-implemented utility that enhances reusability across different NextJS apps. It follows TypeScript best practices and handles the case when the key is undefined.
Line range hint
1-348: Overall improvements to newPrimarySchoolUtils.tsThe changes made to this file align well with the PR objectives and coding guidelines. Key improvements include:
- Removal of allergy-related functionality, streamlining the application process.
- Enhanced child and school data handling with new utility functions.
- Improved reusability with the addition of
getSelectedOptionLabelandgetCurrentSchoolNamefunctions.- Better handling of grade formatting for different locales.
These changes contribute to a more maintainable and focused codebase. The suggestions for null checks and handling edge cases in grade formatting will further improve the robustness of the code.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/templates/new-primary-school/src/lib/messages.ts (1)
682-693: LGTM: Informative error messages for no eligible childrenThe addition of
noChildrenFoundTitleandnoChildrenFoundMessageprovides clear guidance for users without registered children of primary school age. The use of markdown and inclusion of external resources is helpful.Consider using markdown link syntax for better readability in the code:
- 'Eingöngu sá sem er með lögheimilisforsjá hefur heimild til að sækja um fyrir barn. \n\nÞjóðskrá skráir hver eða hverjir teljast foreldrar barns og hver fari með forsjárskyldur þess. Upplýsingar um skráningu forsjár og lögheimilisforeldris má nálgast hér: [Foreldrar og forsjá | Þjóðskrá (skra.is)](https://www.skra.is/folk/skraning-barns/foreldrar-og-forsja/)\n\nUpplýsingum um tengsl á milli barna og foreldra auk forsjáraðila eru einnig aðgengilegar á [Mínum síðum á Ísland.is](https://island.is/minarsidur)', + 'Eingöngu sá sem er með lögheimilisforsjá hefur heimild til að sækja um fyrir barn. \n\nÞjóðskrá skráir hver eða hverjir teljast foreldrar barns og hver fari með forsjárskyldur þess. Upplýsingar um skráningu forsjár og lögheimilisforeldris má nálgast hér: [Foreldrar og forsjá | Þjóðskrá (skra.is)][1]\n\nUpplýsingum um tengsl á milli barna og foreldra auk forsjáraðila eru einnig aðgengilegar á [Mínum síðum á Ísland.is][2]\n\n[1]: https://www.skra.is/folk/skraning-barns/foreldrar-og-forsja/\n[2]: https://island.is/minarsidur',This change improves code readability while maintaining the same output in the UI.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/application/templates/new-primary-school/src/lib/messages.ts (7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/lib/messages.ts (2)
Learnt from: birkirkristmunds PR: island-is/island.is#16096 File: libs/application/templates/new-primary-school/src/lib/messages.ts:168-168 Timestamp: 2024-10-03T15:29:46.932Z Learning: Including a '#' character in message IDs is necessary for the markdown to be displayed correctly in the UI.Learnt from: veronikasif PR: island-is/island.is#16096 File: libs/application/templates/new-primary-school/src/lib/messages.ts:204-211 Timestamp: 2024-10-04T11:50:03.315Z Learning: In `libs/application/templates/new-primary-school/src/lib/messages.ts`, the translation of `'Fornafn'` as `'Pronoun'` is acceptable and does not need correction.
🔇 Additional comments (8)
libs/application/templates/new-primary-school/src/lib/messages.ts (8)
59-59: LGTM: Consistent message ID formatThe update to the
idfield for the
86-86: LGTM: Improved description grammarThe update to the
descriptionfield for thephoneNumbermessage improves its grammatical correctness.
113-119: LGTM: New messages for prerequisitesThe addition of
externalDataSectionandexternalDataSubSectionmessages provides necessary information for the application process. The message IDs and descriptions are consistent with the existing style.
163-171: LGTM: Updated children-related messagesThe changes to
childrenSubSectionTitleandchildrenDescriptionmessages align with the new structure of the application. The addition of the#markdownidentifier in thechildrenDescriptionmessage ID is consistent with the UI display requirements. The updated content provides more detailed information about the child selection process.
198-202: LGTM: More inclusive terminology for child's nameThe update from "chosen name" to "preferred name" in the message ID and description improves clarity and aligns with more inclusive practices. This change enhances the overall user experience and sensitivity of the application.
203-211: LGTM: Addition of pronoun-related messagesThe introduction of
childInfoPronounsandchildInfoPronounsPlaceholdermessages demonstrates a commendable move towards more inclusive language in the application. The translation of "Fornafn" as "Pronoun" is correct and consistent with the provided learning.
588-610: LGTM: Enhanced overview sectionThe addition of new messages for school information and grade details in the overview section provides more comprehensive information about the child's schooling. These additions will improve the user's understanding of the application summary.
Line range hint
1-693: LGTM: Comprehensive updates improving clarity and inclusivityThe changes in this file demonstrate a commendable effort to enhance the user experience, improve clarity, and promote inclusivity. Key improvements include:
- Consistent use of message ID formats
- Addition of prerequisite information
- More inclusive language (e.g., preferred name, pronouns)
- Enhanced overview section with detailed school information
- Informative error messages for edge cases
These updates align well with the provided learnings and coding guidelines, particularly in the use of markdown identifiers for UI display. The changes will contribute to a more user-friendly and inclusive application process.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/templates/new-primary-school/src/types.ts (1)
37-38: LGTM! Consider adding documentation for new fields.The addition of
preferredNameandpronounsfields enhances inclusivity. The use ofstring[]for pronouns is appropriate given that options are server-provided. Consider adding comments or documentation to explain the purpose and expected format of these new fields.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts (1 hunks)
- libs/application/templates/new-primary-school/src/types.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/template-api-modules/src/lib/modules/templates/new-primary-school/new-primary-school.service.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/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."
📓 Learnings (1)
libs/application/templates/new-primary-school/src/types.ts (2)
Learnt from: birkirkristmunds PR: island-is/island.is#16096 File: libs/application/templates/new-primary-school/src/types.ts:37-38 Timestamp: 2024-10-03T16:09:47.250Z Learning: The `pronouns` options are received from the server, so we cannot create an enum or union type for them in the client code.Learnt from: birkirkristmunds PR: island-is/island.is#16096 File: libs/application/templates/new-primary-school/src/types.ts:11-11 Timestamp: 2024-10-03T16:09:14.670Z Learning: The `relation` property in `RelativesRow` should remain as `string` because the possible relation options are received from the server at runtime, so defining an enum locally is not feasible.
🔇 Additional comments (7)
libs/application/templates/new-primary-school/src/types.ts (7)
11-11: Acknowledged:relationtype changed tostringThe change from
RelationOptionstostringfor therelationproperty aligns with the fact that relation options are received from the server. This increases flexibility but might reduce type safety. Consider adding runtime validation for this field if not already implemented.
64-67: LGTM! Verify usage of newSelectOptiontype.The new
SelectOptiontype is a good addition for representing select input options. It follows a common pattern withlabelandvalueproperties. Ensure that this type is used consistently across the application where appropriate.#!/bin/bash # Verify usage of new SelectOption type rg --type typescript 'SelectOption' libs/application/templates/new-primary-school/src/
69-76: LGTM! Verify usage of newAgenttype.The new
Agenttype is a comprehensive representation of an agent or representative in the system. It includes all necessary fields for identification and contact. Ensure that this type is used consistently where agent information is required in the application.#!/bin/bash # Verify usage of new Agent type rg --type typescript 'Agent' libs/application/templates/new-primary-school/src/
78-91: LGTM! Verify usage of newMembershipandMembershipOrganizationtypes.The new
MembershipandMembershipOrganizationtypes provide a structured way to represent membership information. They make good use of the importedMembershipRoleandMembershipOrganizationType. Ensure these types are used consistently where membership data is handled in the application.#!/bin/bash # Verify usage of new Membership and MembershipOrganization types rg --type typescript 'Membership|MembershipOrganization' libs/application/templates/new-primary-school/src/
93-114:⚠️ Potential issueLGTM! Verify usage and address potential data inconsistencies.
The new
AddressModelandFriggChildInformationtypes provide comprehensive structures for address and child information. They make good use of previously defined types. However, the comments inAddressModelsuggest potential data format inconsistencies with MMS data.
- Ensure these types are used consistently where address and child data are handled in the application.
- Consider addressing the potential data format inconsistencies with MMS data for
municipalityandcountryfields inAddressModel. You might need to implement a data transformation layer or update the MMS data format.#!/bin/bash # Verify usage of new AddressModel and FriggChildInformation types rg --type typescript 'AddressModel|FriggChildInformation' libs/application/templates/new-primary-school/src/ # Check for potential data transformation for municipality and country rg --type typescript 'municipality.*object|country.*object' libs/application/templates/new-primary-school/src/
53-53: LGTM! Verify usage of newstreetNameproperty.The addition of the optional
streetNameproperty to the address object provides more detailed address information while maintaining backwards compatibility. Ensure that this new property is properly handled in any code that processes address data.✅ Verification successful
Verified! The
streetNameproperty is properly utilized across the codebase, ensuring detailed address information while maintaining backward compatibility.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of new streetName property rg --type typescript 'streetName' libs/application/templates/new-primary-school/src/Length of output: 293
Script:
#!/bin/bash # Verify usage of new streetName property in TypeScript files fd --extension ts,tsx --exclude node_modules --exclude dist | xargs rg 'streetName'Length of output: 8045
2-3: LGTM! Verify usage of new imports.The import changes reflect the modifications in the types used. Ensure that
MembershipOrganizationTypeandMembershipRoleare used appropriately throughout the file.✅ Verification successful
Verified: Usage of new imports confirmed.
The
MembershipOrganizationTypeandMembershipRoleimports are appropriately used withintypes.tsand are correctly defined inconstants.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of new imports rg --type typescript 'MembershipOrganizationType|MembershipRole' libs/application/templates/new-primary-school/src/Length of output: 152
Script:
#!/bin/bash # Verify usage of new imports without specifying file type rg 'MembershipOrganizationType|MembershipRole' libs/application/templates/new-primary-school/src/Length of output: 645
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/School.tsx (2)
36-43: LGTM with suggestion: GraphQL query implementation is solidThe use of Apollo Client's
useQueryhook and the memoization ofselectedSchoolNamewithuseMemoare excellent practices. This implementation supports effective tree-shaking and optimizes performance.However, consider adding error handling:
const { data, loading, error } = useQuery<FriggSchoolsByMunicipalityQuery>( friggSchoolsByMunicipalityQuery ); // Add error handling if (error) { // Handle the error appropriately }This will improve the robustness of the component.
Line range hint
1-105: LGTM with minor suggestion: Excellent component implementationOverall, this is a well-implemented React component that aligns perfectly with the coding guidelines for libs/**/* files. It demonstrates:
- Reusability across different NextJS apps
- Effective use of TypeScript for props and exported types
- Support for tree-shaking and efficient bundling
The component effectively uses hooks, GraphQL queries, and reusable UI components. It handles loading states and localization well.
To further improve the component, consider adding error handling for the GraphQL query:
if (error) { // Handle the error, e.g., display an error message return <ErrorComponent message={error.message} />; }This addition would make the component more robust and provide a better user experience in case of data fetching errors.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/application/templates/new-primary-school/src/fields/Review/review-groups/School.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/School.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 (5)
libs/application/templates/new-primary-school/src/fields/Review/review-groups/School.tsx (5)
1-19: LGTM: Import statements are well-organized and support tree-shakingThe new import statements are logically grouped and follow TypeScript conventions. They support the component's new functionality while enabling effective tree-shaking, which aligns with the coding guidelines for libs/**/* files.
28-34: LGTM: Effective use of hooks and utility functionsThe component makes good use of the
useLocalehook and utility functions likegetApplicationAnswersandgetApplicationExternalData. This approach promotes code reusability and adheres to the guideline of creating reusable components and hooks across different NextJS apps.
59-61: LGTM: Effective loading state handlingThe use of a SkeletonLoader during the loading state is an excellent practice. It provides visual feedback to the user while data is being fetched, enhancing the overall user experience.
62-100: LGTM: Well-structured and localized data displayThe data display implementation is excellent. It uses reusable components (DataValue) for consistent styling, handles localization properly with formatting functions, and presents information in a clear, structured manner. This approach enhances maintainability and supports the guideline of creating reusable components across different NextJS apps.
88-88: LGTM: Effective use of formatGrade functionThe use of the
formatGradefunction withchildGradeLevelandlangparameters is an excellent practice. It supports localization and provides a consistent way to format grade levels across the application. This aligns well with the coding guidelines for creating reusable and maintainable code.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (3)
110-113: LGTM: Updated getApplicationAnswers functionThe function has been correctly updated to remove food allergy-related variables and add the newSchoolHiddenInput variable. This aligns with the PR objectives.
Consider adding a comment explaining the purpose of the newSchoolHiddenInput variable for better code readability.
Also applies to: 136-136
327-345: Improved formatGrade function, but consider handling special casesThe formatGrade function has been updated to handle different locales and remove leading zeros. This is a good improvement.
Consider handling special cases for 11th, 12th, and 13th grades in English. Here's a suggested improvement:
export const formatGrade = (gradeLevel: string, lang: Locale) => { let grade = gradeLevel.replace(/^0+/, '') if (lang === 'en') { const num = parseInt(grade, 10) if (num >= 11 && num <= 13) { return `${grade}th` } const lastDigit = num % 10 switch (lastDigit) { case 1: return `${grade}st` case 2: return `${grade}nd` case 3: return `${grade}rd` default: return `${grade}th` } } return grade }This implementation correctly handles all English ordinal suffixes, including the special cases.
Line range hint
1-356: Overall LGTM: Improved utility functions with minor suggestions for enhancementThe changes in this file align well with the PR objectives. Unused code related to food allergies has been removed, and new reusable functions have been added to enhance functionality. The modifications adhere to TypeScript best practices and promote code reusability.
Some minor improvements have been suggested for better error handling and edge case management, particularly in the
getApplicationExternalData,formatGrade, andgetCurrentSchoolNamefunctions.Consider creating a separate file for locale-specific utilities (like
formatGrade) if more such functions are added in the future. This would improve modularity and make it easier to manage locale-specific logic.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.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 (2)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (2)
8-8: LGTM: New imports added for enhanced functionalityThe new imports (Locale, FriggChildInformation, Membership, SelectOption) are correctly added and used in the new or modified functions. This adheres to TypeScript best practices for type definitions.
Also applies to: 12-13, 17-17
316-324: LGTM: Added reusable getSelectedOptionLabel functionThe new getSelectedOptionLabel function is a well-implemented, generic utility that can be used across different parts of the application. It adheres to TypeScript best practices and promotes code reusability.
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts
Show resolved
Hide resolved
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (6)
110-113: LGTM: Updated getApplicationAnswers functionThe removal of food allergy-related variables and the addition of
newSchoolHiddenInputalign with the PR objectives. This change streamlines the function and focuses on the new primary school application process.Consider adding a comment explaining the purpose of
newSchoolHiddenInputfor better code documentation.Also applies to: 136-136
143-143: LGTM: Enhanced getApplicationExternalData functionThe changes improve the function's robustness by adding default values for applicant-related data and introducing new variables for child information. This aligns with the coding guidelines for TypeScript usage and error handling.
Consider using the nullish coalescing operator (
??) instead of the logical OR (||) for default values to handle falsy values more accurately. For example:const applicantName = getValueViaPath( externalData, 'nationalRegistry.data.name', ) ?? '' as stringAlso applies to: 148-148, 154-154, 160-160, 166-166, 172-172, 175-197, 205-208
315-323: LGTM: New getSelectedOptionLabel functionThis utility function enhances reusability and follows TypeScript best practices. It's a good addition to the codebase.
Consider using optional chaining for a more concise implementation:
export const getSelectedOptionLabel = ( options: SelectOption[], key?: string, ) => options.find((option) => option.value === key)?.labelThis change reduces the function to a single line while maintaining the same functionality.
326-344: LGTM: Improved formatGrade functionThe updated
formatGradefunction now handles different languages and correctly formats English ordinals. This improvement aligns with the coding guidelines for internationalization.Consider adding support for 11th, 12th, and 13th grades, which are exceptions to the general rule:
if (lang === 'en') { if (['11', '12', '13'].includes(grade)) { return `${grade}th` } // ... rest of the switch statement }This change would make the function more robust for all grade levels.
347-359: LGTM: New getCurrentSchoolName functionThe
getCurrentSchoolNamefunction is a well-implemented addition that retrieves the current school name based on the primary organization ID and child memberships. The null checks enhance error handling, and the use of functional programming concepts aligns with modern JavaScript practices.Consider adding a type annotation for the return value to improve type safety:
export const getCurrentSchoolName = (application: Application): string | undefined => { // ... rest of the function }This change would make the function's return type explicit, enhancing code readability and type safety.
Line range hint
1-359: Overall: Significant improvements to newPrimarySchoolUtils.tsThe changes in this file align well with the PR objectives, removing food allergy-related code and focusing on primary school application processes. The new utility functions (
getSelectedOptionLabel,formatGrade, andgetCurrentSchoolName) enhance the module's functionality and reusability.The updates follow TypeScript best practices, improve error handling, and support internationalization. These changes contribute to a more robust and maintainable codebase.
Consider adding comprehensive unit tests for the new and modified functions to ensure their correctness and prevent future regressions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/application/templates/new-primary-school/src/fields/RelativesTableRepeater/index.tsx (1 hunks)
- libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/application/templates/new-primary-school/src/fields/RelativesTableRepeater/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.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 (1)
libs/application/templates/new-primary-school/src/lib/newPrimarySchoolUtils.ts (1)
8-8: LGTM: New imports addedThe new imports (Locale, FriggChildInformation, Membership, SelectOption) are appropriate for the changes made in the file. They enhance type safety and align with the TypeScript usage guidelines.
Also applies to: 12-13, 17-17
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- charts/islandis/values.dev.yaml (2 hunks)
- charts/islandis/values.prod.yaml (2 hunks)
- charts/islandis/values.staging.yaml (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/islandis/values.staging.yaml
🔇 Additional comments (4)
charts/islandis/values.prod.yaml (3)
642-642: Approve consistent change across servicesThe
XROAD_MMS_FRIGG_PATHhas been updated to 'IS/GOV/10066/MMS-Protected/frigg-form-service' in the application-system-api configuration as well. This change is consistent with the update in the api section, ensuring that both services are using the same updated endpoint.
Line range hint
1-3803: Summary of changes and potential impactThe main change in this configuration file is the update of the
XROAD_MMS_FRIGG_PATHenvironment variable in both theapiandapplication-system-apisections. This change reflects an update in the MMS Frigg service endpoint from 'frigg-api' to 'frigg-form-service'.Potential impacts and considerations:
- Ensure that all services depending on this endpoint are updated accordingly.
- Verify that the new endpoint is functional and provides the expected services.
- Update any documentation or internal references to reflect this change.
- Consider if any rollback procedures need to be in place in case of issues with the new endpoint.
Overall, the change appears to be straightforward and consistently applied where needed.
362-362: Approve change and verify new endpointThe
XROAD_MMS_FRIGG_PATHhas been updated from 'frigg-api' to 'frigg-form-service'. This change looks good, as it likely reflects an update in the service endpoint.Please run the following script to verify that the new endpoint is correctly configured and accessible:
charts/islandis/values.dev.yaml (1)
652-652: Consistent update of X-Road path for MMS Frigg service in application-system-apiThe
XROAD_MMS_FRIGG_PATHenvironment variable has been consistently updated in the application-system-api section, mirroring the change observed earlier.This consistent change across different components is good practice. To ensure a smooth transition:
- Verify that the application-system-api is compatible with the new 'frigg-form-service' endpoint.
- Test the integration between the application-system-api and the MMS Frigg service to confirm functionality.
- Update any API documentation specific to the application-system-api that might reference the old X-Road path.
...
Attach a link to issue if relevant
What
Why
Specify why you need to achieve this
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes