chore(inao-pp-cem): dataverse check and template-api-module adjustments #16958
Conversation
WalkthroughThe pull request includes modifications to the ownership structure in the Changes
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
|
jonnigs
changed the title
...c/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16958 +/- ##
==========================================
+ Coverage 35.62% 35.89% +0.27%
==========================================
Files 6915 6913 -2
Lines 146037 145298 -739
Branches 41457 41456 -1
==========================================
+ Hits 52021 52155 +134
+ Misses 94016 93143 -873
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 86 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
...templates/financial-statement-political-party/financial-statement-political-party.service.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 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: 9
🧹 Outside diff range and nitpick comments (18)
libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/index.ts (2)
5-10: Consider adding explicit type definitions for form configuration.While the code is functionally correct, we should enhance type safety by explicitly defining the form configuration type.
+import { Form } from '@island.is/application/core' -export const notAllowedForm = buildForm({ +export const notAllowedForm: Form = buildForm({ id: 'notAllowedForm', title: '', logo: Logo, children: [notAllowedSection], })
1-10: Overall structure looks good and follows template guidelines.The form implementation is clean, modular, and follows the reusability guidelines for template components.
Consider documenting the purpose of this form in a comment block, especially since it's part of a not-allowed flow, to help other developers understand its usage context.
libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/index.ts (1)
1-3: Consider enhancing type safety and import structure.To improve type safety and maintainability:
- Consider importing and explicitly typing the form configuration interface
- Consider using absolute imports instead of relative paths for better maintainability
-import { buildForm } from '@island.is/application/core' +import { buildForm, FormBuilder } from '@island.is/application/core' -import { notAllowedSection } from './notAllowedSection' -import Logo from '../../components/Logo' +import { notAllowedSection } from '@island.is/application/templates/inao/financial-statement-political-party/forms/notAllowedForm/notAllowedSection' +import { Logo } from '@island.is/application/templates/inao/financial-statement-political-party/components/Logo'libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/notAllowedSection.ts (1)
1-6: LGTM! Consider exporting types for better reusabilityThe imports are well-organized and properly destructured. Since this is in the
libsdirectory, consider exporting any relevant types for better TypeScript integration across different apps.+import type { Section } from '@island.is/application/core' import { buildDescriptionField, buildSection, } from '@island.is/application/core' import { m } from '../../lib/messages' + +export type NotAllowedSectionType = Sectionlibs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/notAllowedSection.ts (2)
7-17: Consider adding a descriptive title for better accessibility.The empty title string might affect screen readers and accessibility. Consider adding a meaningful title or documenting why it's intentionally left empty.
export const notAllowedSection = buildSection({ id: 'notAllowedSection', - title: '', + title: m.notAllowedTitle, children: [
7-17: Consider making the section more reusable.To improve reusability across different NextJS apps (as per coding guidelines), consider:
- Making the messages configurable through props
- Adding documentation for reuse scenarios
+interface NotAllowedSectionProps { + titleMessage: string; + descriptionMessage: string; +} + +export const createNotAllowedSection = ({ + titleMessage, + descriptionMessage, +}: NotAllowedSectionProps): Section => buildSection({ id: 'notAllowedSection', title: '', children: [ buildDescriptionField({ id: 'notAllowedDescription', - title: m.notAllowedTitle, - description: m.notAllowedDescription, + title: titleMessage, + description: descriptionMessage, }), ], })libs/application/templates/inao/financial-statement-political-party/src/types/types.ts (1)
14-14: Maintain consistent naming convention in Roles enumThe value 'notAllowed' uses camelCase while other enum values use lowercase. Consider maintaining consistency with the existing pattern:
- NOTALLOWED = 'notAllowed', + NOTALLOWED = 'notallowed',libs/application/templates/inao/financial-statement-cemetery/src/types/types.ts (2)
37-37: Add JSDoc comment to document the role's purpose.The new role aligns well with the requirement to handle users not registered correctly with Ríkisendurskoðun. Consider adding documentation to explain its purpose.
+ /** Role assigned to users whose national ID is not properly registered with Ríkisendurskoðun */ NOTALLOWED = 'notAllowed',
37-37: Follow consistent enum naming convention.The enum value uses camelCase ('notAllowed') while other values in the enum use UPPERCASE. Consider maintaining consistency:
- NOTALLOWED = 'notAllowed', + NOTALLOWED = 'NOTALLOWED',libs/application/templates/inao/financial-statement-cemetery/src/utils/constants.ts (1)
17-17: Document the meaning of the user type valueThe magic number
150000002should be documented to explain its significance and usage. Consider adding a comment explaining what this user type represents and why this specific value is used.+// Represents a cemetery user type in the system, used for role-based access control export const CEMETERY_USER_TYPE = 150000002libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (1)
88-91: Enhance type safety with explicit return type.The method could benefit from an explicit return type to ensure type safety across the codebase.
- async getUserType({ auth }: TemplateApiModuleActionProps) { + async getUserType({ auth }: TemplateApiModuleActionProps): Promise<number | null> {libs/application/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts (1)
155-158: Enhance type safety for external data accessWhile the code uses TypeScript, the type safety of external data access could be improved.
Consider defining an interface for the external data structure:
interface UserTypeData { getUserType: { data: { value: number; }; }; } // Then use it in getValueViaPath const userType = getValueViaPath<number, UserTypeData>( application.externalData as UserTypeData, 'getUserType.data.value', )libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts (1)
Line range hint
115-127: Standardize error handling approach.The error handling in
submitApplicationshows mixed patterns:
- Some errors are thrown directly
- Others return an error object
- There's a commented-out throw statement
Consider standardizing the error handling approach across the service for consistency.
Suggested approach:
} catch (error) { this.logger.error('Error submitting application', error) - return { - success: false, - message: error.message, - } - // throw new Error('Application submission failed') + throw new Error(`Application submission failed: ${error.message}`) }libs/application/templates/inao/financial-statement-political-party/src/lib/messages.ts (2)
561-572: Consider making the phone number configurableThe implementation looks good and aligns with the PR objectives. However, consider extracting the phone number into a separate message to make it configurable independently.
notAllowedDescription: { id: 'fspp.application:notAllowedDescription', defaultMessage: - 'Ef þú telur að þessi kennitala ætti að vera skráð sem stjórnmálasamtök þá bendum við þér á að hafa samband við Ríkisendurskoðun í síma 448 8800', + 'Ef þú telur að þessi kennitala ætti að vera skráð sem stjórnmálasamtök þá bendum við þér á að hafa samband við Ríkisendurskoðun í síma {phoneNumber}', description: 'Descriptionwhen user is not allowed to apply', }, +inaoPhoneNumber: { + id: 'fspp.application:inaoPhoneNumber', + defaultMessage: '448 8800', + description: 'Phone number for INAO (Ríkisendurskoðun)', +},
571-571: Fix typo in the description fieldThere's a typo in the description field: "Descriptionwhen" should be "Description when".
- description: 'Descriptionwhen user is not allowed to apply', + description: 'Description when user is not allowed to apply',libs/application/templates/inao/financial-statement-cemetery/src/lib/messages.ts (1)
679-684: Consider extracting the phone number into a separate message.The phone number (448 8800) is currently hard-coded in the message. To improve maintainability, consider extracting it to a separate message.
+ inaoPhoneNumber: { + id: 'fsc.application:inaoPhoneNumber', + defaultMessage: '448 8800', + description: 'INAO contact phone number', + }, notAllowedDescription: { id: 'fsc.application:notAllowedDescription', defaultMessage: - 'Ef þú telur að þessi kennitala ætti að vera skráð sem kirkjugarður þá bendum við þér á að hafa samband við Ríkisendurskoðun í síma 448 8800', + 'Ef þú telur að þessi kennitala ætti að vera skráð sem kirkjugarður þá bendum við þér á að hafa samband við Ríkisendurskoðun í síma {phoneNumber}', - description: 'Descriptionwhen user is not allowed to apply', + description: 'Description when user is not allowed to apply', },libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts (2)
159-159: Consider removing unused parameter_nationalIdSince
_nationalIdis not utilized within themapUserToRolemethod, consider removing it if it is not required by the interface. This will clean up the code and prevent potential confusion.
162-165: Ensure safe retrieval ofuserTypefrom external dataTo prevent potential runtime errors, consider adding a default value or validation when retrieving
userTypeusinggetValueViaPath, in case the path does not exist or returnsundefined.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (22)
.github/CODEOWNERS(1 hunks)libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts(3 hunks)libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts(2 hunks)libs/application/templates/inao/financial-statement-cemetery/src/fields/DelegationCheck/index.tsx(0 hunks)libs/application/templates/inao/financial-statement-cemetery/src/fields/index.ts(0 hunks)libs/application/templates/inao/financial-statement-cemetery/src/forms/applicationForm/clientInfoSection/index.ts(0 hunks)libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/index.ts(1 hunks)libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/notAllowedSection.ts(1 hunks)libs/application/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts(5 hunks)libs/application/templates/inao/financial-statement-cemetery/src/lib/messages.ts(1 hunks)libs/application/templates/inao/financial-statement-cemetery/src/types/types.ts(1 hunks)libs/application/templates/inao/financial-statement-cemetery/src/utils/constants.ts(1 hunks)libs/application/templates/inao/financial-statement-political-party/src/fields/DelegationCheck/DelegationCheck.tsx(0 hunks)libs/application/templates/inao/financial-statement-political-party/src/fields/index.ts(0 hunks)libs/application/templates/inao/financial-statement-political-party/src/forms/applicationForm/clientInfoSection/index.ts(0 hunks)libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/index.ts(1 hunks)libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/notAllowedSection.ts(1 hunks)libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts(5 hunks)libs/application/templates/inao/financial-statement-political-party/src/lib/messages.ts(1 hunks)libs/application/templates/inao/financial-statement-political-party/src/types/types.ts(1 hunks)libs/application/templates/inao/financial-statement-political-party/src/utils/constants.ts(1 hunks)libs/clients/financial-statements-inao/src/lib/financialStatementsInao.config.ts(1 hunks)
💤 Files with no reviewable changes (6)
- libs/application/templates/inao/financial-statement-cemetery/src/fields/DelegationCheck/index.tsx
- libs/application/templates/inao/financial-statement-cemetery/src/fields/index.ts
- libs/application/templates/inao/financial-statement-cemetery/src/forms/applicationForm/clientInfoSection/index.ts
- libs/application/templates/inao/financial-statement-political-party/src/fields/DelegationCheck/DelegationCheck.tsx
- libs/application/templates/inao/financial-statement-political-party/src/fields/index.ts
- libs/application/templates/inao/financial-statement-political-party/src/forms/applicationForm/clientInfoSection/index.ts
✅ Files skipped from review due to trivial changes (1)
- libs/clients/financial-statements-inao/src/lib/financialStatementsInao.config.ts
🧰 Additional context used
📓 Path-based instructions (14)
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/notAllowedSection.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/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.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/inao/financial-statement-cemetery/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."
libs/application/templates/inao/financial-statement-cemetery/src/types/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/inao/financial-statement-cemetery/src/utils/constants.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/inao/financial-statement-political-party/src/forms/notAllowedForm/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/notAllowedSection.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/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.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/inao/financial-statement-political-party/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."
libs/application/templates/inao/financial-statement-political-party/src/types/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/inao/financial-statement-political-party/src/utils/constants.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 (20)
libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/index.ts (2)
1-4: LGTM! Clean imports following best practices.
The imports are well-organized and follow tree-shaking practices by importing specific components.
7-7: Verify if empty title is intentional.
The form title is set to an empty string. Please confirm if this is intentional or if it should display a meaningful title for better user experience.
✅ Verification successful
Empty title is intentional for this not-allowed form
Based on the context, the empty title is intentional as this is a special "not allowed" form that displays a message to users who aren't authorized to submit financial statements. The form's content is managed through the messages file where notAllowedTitle and notAllowedDescription define the actual content shown to users, explaining why they can't proceed and providing guidance on next steps.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other forms in the codebase typically have titles
# Look for buildForm usage with title property
ast-grep --pattern 'buildForm({
$$$
title: $title,
$$$
})'
Length of output: 66
Script:
#!/bin/bash
# Let's try a different approach to find form title patterns
# Search for files containing 'title' in forms directories
fd "index.ts" -p "forms" --exec rg -A 2 -B 2 "title: ['\"](.*?)['\"]" {}
# Also check the specific file content
cat libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/index.ts
Length of output: 549
Script:
#!/bin/bash
# Let's check the notAllowedSection to understand the context better
cat libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/notAllowedSection.ts
# Also search for other similar not allowed forms
rg -B 2 -A 2 "notAllowed.*Form" --type ts
Length of output: 3656
Script:
#!/bin/bash
# Let's check the messages file to see if there's a pattern with titles being defined there
cat libs/application/templates/inao/financial-statement-cemetery/src/lib/messages.ts
Length of output: 23403
libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/index.ts (1)
5-10: Verify handling of political party user role.
Please ensure that the notAllowedSection properly handles the new userRole for political parties as mentioned in the PR objectives.
✅ Verification successful
Let me gather more information about the notAllowedSection implementation.
Let me check the implementation of notAllowedSection to verify the handling of political party roles.
Let me check the complete implementation of the template to understand how political party roles are handled.
Political party role handling is properly implemented
The template correctly handles the political party user role through the following mechanisms:
- The
mapUserToRolefunction checks if the user is a political party (POLITICALPARTY_USER_TYPE) and assigns the appropriate role (APPLICANT or NOTALLOWED) - The
notAllowedSectionis shown to users with NOTALLOWED role, displaying appropriate messages defined in the template - The messages inform users that their ID is not registered as a political party and provide guidance to contact the relevant authority
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if notAllowedSection handles the new userRole
# Search for userRole handling in notAllowedSection
ast-grep --pattern 'export const notAllowedSection = {
$$$
userRole $$$
$$$
}'
# Check for role-related messages
rg -A 2 "notAllowedTitle|notAllowedDescription" --type ts
Length of output: 3668
Script:
#!/bin/bash
# Check the complete implementation of notAllowedSection
ast-grep --pattern 'export const notAllowedSection = {
$$$
}'
# Check for any role-based checks or conditions
rg -A 5 "userRole|isValidPoliticalParty" --type ts
Length of output: 42521
Script:
#!/bin/bash
# Get the implementation of notAllowedSection
cat libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/notAllowedSection.ts
# Check for any role-based logic in the template
rg -A 5 "mapUserToRole|getRoleInState" libs/application/templates/inao/financial-statement-political-party/src/
Length of output: 1522
Script:
#!/bin/bash
# Get the complete template implementation
cat libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts
# Check for role-related constants or types
rg -A 3 "enum.*Role|type.*Role|interface.*Role" libs/application/templates/inao/financial-statement-political-party/src/
Length of output: 5596
libs/application/templates/inao/financial-statement-cemetery/src/forms/notAllowed/notAllowedSection.ts (1)
7-17: Verify the empty title and consider adding documentation
The section implementation looks good with proper localization. However, there are two points to consider:
- Verify if the empty title on line 9 is intentional
- Consider adding JSDoc comments to document the purpose of this section
+/**
+ * Builds a section to display when a user is not allowed to access the application.
+ * This section contains a description explaining why access is denied.
+ */
export const notAllowedSection = buildSection({
id: 'notAllowedSection',
title: '',
children: [Let's verify the empty title pattern usage:
✅ Verification successful
Based on the search results, I can see that empty titles are commonly used throughout the codebase in various forms and sections. The pattern of having an empty title (title: '') appears to be an established practice, particularly in:
- Notification/status sections (e.g. in InReview forms)
- Conclusion sections
- Prerequisite sections
- Sections where the visual hierarchy doesn't require a title
The notAllowedSection in question follows this pattern appropriately since it's a notification-type section displaying access denial information.
Empty title is an established pattern, and JSDoc documentation would improve clarity
The empty title is consistent with the codebase patterns. Adding JSDoc documentation as suggested would indeed improve code clarity by explaining the section's purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if empty titles are used in other sections for consistency
# Search for buildSection with empty titles
rg -A 5 "buildSection\({[\s\n]*id:.*title:\s*['\"]'," libs/
Length of output: 206
Script:
#!/bin/bash
# Search for buildSection with empty titles using a simpler pattern
rg "title: ''" libs/
# Also search for buildSection pattern to understand the general usage
rg "buildSection\(" -A 3 libs/
Length of output: 552846
libs/application/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/notAllowedSection.ts (2)
1-5: LGTM! Well-structured imports following best practices.
The imports are properly organized, using named imports which enables effective tree-shaking.
13-14: Verify the message keys exist in the messages file.
Ensure that notAllowedTitle and notAllowedDescription are properly defined in the messages file.
✅ Verification successful
Message keys are properly defined in the messages file
Both notAllowedTitle and notAllowedDescription keys are correctly defined in the messages file at libs/application/templates/inao/financial-statement-political-party/src/lib/messages.ts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the required message keys exist in the messages file
rg "notAllowedTitle|notAllowedDescription" "libs/application/templates/inao/financial-statement-political-party/src/lib/messages.ts"
Length of output: 274
libs/application/templates/inao/financial-statement-political-party/src/types/types.ts (1)
9-9: LGTM! New state added correctly
The REJECTED state is properly added to handle applications that don't meet the registration requirements.
libs/application/templates/inao/financial-statement-cemetery/src/utils/constants.ts (1)
17-17: Consider explicit typing and potential relocation
A few suggestions to improve type safety and reusability:
- Add explicit type annotation
- If this user type is used across different templates, consider moving it to a shared constants file
-export const CEMETERY_USER_TYPE = 150000002
+export const CEMETERY_USER_TYPE: number = 150000002Let's check if this constant is used in other templates:
✅ Verification successful
No need to relocate the constant, but type annotation would improve safety
The constant CEMETERY_USER_TYPE is only used within the financial-statement-cemetery template (in financialStatementCemeteryTemplate.ts). Since it's not shared across other templates, it should remain in its current location. However, adding the explicit type annotation would indeed improve type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CEMETERY_USER_TYPE usage across the codebase
rg "CEMETERY_USER_TYPE" --type ts
Length of output: 474
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (1)
139-142: LGTM! Good separation of concerns.
The new private method properly encapsulates the value preparation logic with appropriate TypeScript types.
libs/application/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts (2)
4-4: LGTM: Import changes are well-structured
The new imports are properly organized and support tree-shaking by importing specific utilities.
Also applies to: 28-28
53-56: LGTM: Template API configuration is properly structured
The onEntry configuration correctly sets up the user type check and persists the data, which is essential for the subsequent role mapping.
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts (2)
36-37: Document the purpose of fake user types.
The fakeUserType appears to be used for testing purposes. Consider adding a comment explaining its purpose and when it should be used.
Let's verify the test usage:
✅ Verification successful
The fakeUserType pattern is consistently used across financial statement templates
The code follows an established pattern used across multiple financial statement templates (cemetery, individual election, political party, etc.) where fakeUserType is used to override the actual user type during testing. This is a standard testing mechanism in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how fake user types are used in tests
# Expected: Find test files using fakeUserType for political party template
rg -A 5 "fakeUserType|fakeData.options" --type ts
Length of output: 12988
92-92: Document and verify the null return behavior.
The change to return null for person IDs is a significant behavioral change that could affect downstream code. This appears to be part of implementing the new user role system for directing users to the "not registered correctly" screen.
Let's verify the handling of null returns:
✅ Verification successful
Null return behavior is properly handled
The code change to return null for person IDs is safe and aligns with the application's role-based access control pattern. From the codebase analysis:
- The template checks
getUserType.data.valueand maps it to appropriate roles (APPLICANT or NOTALLOWED) - The DelegationCheck component explicitly handles null responses from getUserType
- Other similar services (Cemetery, Individual Election) follow the same pattern of returning null for invalid user types
- The null response is used consistently across the application to trigger the "not registered correctly" screen
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how null returns from getUserType are handled
# Expected: Find components that handle the null case appropriately
# Check for getUserType usage
ast-grep --pattern 'getUserType($$$).then($$$)'
# Check for null handling
rg -A 5 "getUserType.*\b(null|undefined)\b" --type ts
Length of output: 105
Script:
#!/bin/bash
# Let's try a broader search for getUserType usage patterns
rg -A 10 "getUserType" --type ts
# Also search for any references to this service
rg -A 10 "FinancialStatementPoliticalPartyService" --type ts
# Look for components that might handle user type responses
ast-grep --pattern 'if ($_ === null) { $$$ }'
Length of output: 60947
libs/application/templates/inao/financial-statement-cemetery/src/lib/messages.ts (1)
673-684: LGTM! The new messages are well-structured and provide clear user guidance.
The messages follow the established pattern and provide appropriate feedback for users whose national ID is not registered as a cemetery.
.github/CODEOWNERS (2)
237-237: LGTM! The new entry follows the file's conventions.
The entry is properly formatted and positioned with other norda team assignments.
237-237: Consider consolidating financial statements ownership.
The client library is now owned by @island-is/norda, while related components (API domain, templates, template API modules) are owned by @island-is/programm. This split ownership might require additional coordination between teams.
Let's verify all related financial statements components:
✅ Verification successful
Let me gather more information about the ownership patterns to verify this concern.
Split ownership of financial statements components confirmed
The verification confirms the split ownership pattern:
- Client library (
/libs/clients/financial-statements-inao) is owned by@island-is/norda - All other components are owned by
@island-is/programm:- API domain (
/libs/api/domains/financial-statements-inao/) - Application template (
/libs/application/templates/financial-statements-inao/) - Template API module (
/libs/application/template-api-modules/src/lib/modules/templates/financial-statements-inao)
- API domain (
This split ownership structure could indeed lead to coordination overhead, especially since the client library is tightly coupled with other financial statements components as evidenced by the import statements found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List all financial statements related paths and their owners
# to help assess the impact of split ownership
echo "Financial statements related paths and their owners:"
rg "financial-statements-inao" --no-filename | sort -u
Length of output: 3322
Script:
#!/bin/bash
# Check CODEOWNERS entries for all financial statements related paths
echo "CODEOWNERS entries for financial statements components:"
rg "/financial-statements" .github/CODEOWNERS
# List all unique paths containing financial-statements-inao
echo -e "\nUnique paths containing financial-statements-inao:"
fd "financial-statements-inao"
Length of output: 1392
libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts (4)
17-17: Approved import of getValueViaPath utility function
The inclusion of getValueViaPath from @island.is/application/core correctly supports the retrieval of nested values from application.externalData.
28-28: Approved import of POLITICALPARTY_USER_TYPE constant
Importing POLITICALPARTY_USER_TYPE ensures consistency and maintainability when comparing user types throughout the application.
89-97: Approved addition of Roles.NOTALLOWED with appropriate form loader
The introduction of the Roles.NOTALLOWED role and the associated notAllowedForm provides clear guidance to users who are not permitted to proceed, improving user experience.
166-169: Approved logic for role assignment in mapUserToRole
The updated logic correctly assigns Roles.APPLICANT or Roles.NOTALLOWED based on the userType, enhancing access control within the application.
...ication/templates/inao/financial-statement-political-party/src/forms/notAllowedForm/index.ts
Show resolved
Hide resolved
...lates/inao/financial-statement-political-party/src/forms/notAllowedForm/notAllowedSection.ts
Show resolved
Hide resolved
libs/application/templates/inao/financial-statement-political-party/src/utils/constants.ts
Outdated
Show resolved
Hide resolved
...c/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts
Show resolved
Hide resolved
...c/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts
Outdated
Show resolved
Hide resolved
...on/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts
Outdated
Show resolved
Hide resolved
...on/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts
Show resolved
Hide resolved
...inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (2)
86-86: Enhance type safety and error handling.The method should explicitly define its return type and handle potential errors from
isPersonvalidation.Consider applying these changes:
- async getUserType({ auth }: TemplateApiModuleActionProps) { + async getUserType({ auth }: TemplateApiModuleActionProps): Promise<number | null> { const { nationalId } = auth + if (!nationalId) { + throw new Error('National ID is required') + } if (kennitala.isPerson(nationalId)) { return null } else { return this.financialStatementClientService.getUserClientType(nationalId) } }
111-133: Improve error handling and logging.While the error handling structure is good, there are opportunities for improvement:
- The empty string parameter on line 118 needs explanation or removal
- Error messages could be more specific
- Logging could include more context
Consider applying these changes:
try { const result = await this.financialStatementClientService.postFinancialStatementForCemetery( client, contacts, digitalSignee, year, - '', + '', // TODO: Document why this parameter is empty or remove if not needed values, fileName, ) if (!result) { - throw new Error('Application submission failed') + throw new Error('Cemetery financial statement submission failed: No result returned') } return { success: true } } catch (error) { - this.logger.error('Error submitting application', error) + this.logger.error( + `Failed to submit cemetery financial statement for client ${client.nationalId}`, + { error, year, hasAttachment: !!fileName } + ) return { success: false, - message: error.message, + message: `Failed to submit financial statement: ${error.message}`, } }libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts (2)
89-96: Enhance type safety for the notAllowedForm importConsider adding type safety to the form import to ensure the imported component matches the expected interface.
- formLoader: () => - import('../forms/notAllowedForm').then((val) => - Promise.resolve(val.notAllowedForm), - ), + formLoader: () => + import('../forms/notAllowedForm').then( + (val): Promise<typeof val.notAllowedForm> => + Promise.resolve(val.notAllowedForm), + ),
Line range hint
36-40: Enhance TypeScript usage with explicit typesConsider adding explicit type definitions for the configuration object and template interface to improve type safety and documentation.
-const configuration = - ApplicationConfigurations[ - ApplicationTypes.FINANCIAL_STATEMENT_POLITICAL_PARTY - ] +const configuration: typeof ApplicationConfigurations[ApplicationTypes.FINANCIAL_STATEMENT_POLITICAL_PARTY] = + ApplicationConfigurations[ + ApplicationTypes.FINANCIAL_STATEMENT_POLITICAL_PARTY + ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts(3 hunks)libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts(2 hunks)libs/application/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts(5 hunks)libs/application/templates/inao/financial-statement-cemetery/src/utils/constants.ts(1 hunks)libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts(5 hunks)libs/application/templates/inao/financial-statement-political-party/src/utils/constants.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/application/template-api-modules/src/lib/modules/templates/financial-statement-political-party/financial-statement-political-party.service.ts
- libs/application/templates/inao/financial-statement-cemetery/src/lib/financialStatementCemeteryTemplate.ts
- libs/application/templates/inao/financial-statement-cemetery/src/utils/constants.ts
- libs/application/templates/inao/financial-statement-political-party/src/utils/constants.ts
🧰 Additional context used
📓 Path-based instructions (2)
libs/application/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.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/template-api-modules/src/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts (1)
137-140: LGTM! Well-structured method.
The new private method follows good practices with proper TypeScript usage and clear single responsibility.
libs/application/templates/inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts (1)
55-58: Add error handling for the getUserType API action
The API action is correctly defined, but there's no visible error handling strategy. Consider adding error handling to gracefully handle API failures.
...c/lib/modules/templates/financial-statement-cemetery/financial-statement-cemetery.service.ts
Show resolved
Hide resolved
...inao/financial-statement-political-party/src/lib/financialStatementPoliticalPartyTemplate.ts
Show resolved
Hide resolved
…ts (#16958) * feat: add in the dataverse check before you enter the application * feat: adjust cemetery client * chore: remove console.dir * chore: remove unused imports * chore: remove console.log * chore: remove unused imports * chore: change codeowners of inao client * fix: remove delegation check once you are in the application * chore: remove fakeUser, not used any more * chore: add documentation * chore: edit not allowed permissions * feat: change financial-statements codeowner to Norda by Programm request --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Added a new
userRoleto political party and cemetery that sends you right away to a screen that says that the national Id is not registered correctly with RíkisendurskoðunAdjusted the template-api-modules so that they won't do the same user check again.
Made the template-api-modules behave the same way.
Why
Requested behaviour
Checklist:
Summary by CodeRabbit
Release Notes
New Features
notAllowedFormandnotAllowedSectionto handle restricted access scenarios.Improvements
NOTALLOWEDandREJECTED) and refined user type validation.Removals
DelegationCheckcomponent from both financial statement templates to simplify user input validation.