fix(application-plc): Detect user delegation type #16429
Conversation
…amentary-list-creation-user-detection
…amentary-list-creation-user-detection
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16429 +/- ##
==========================================
- Coverage 36.72% 36.72% -0.01%
==========================================
Files 6809 6809
Lines 141165 141172 +7
Branches 40245 40247 +2
==========================================
+ Hits 51844 51846 +2
- Misses 89321 89326 +5 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 21 Total Test Services: 0 Failed, 20 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (1)
|
WalkthroughThe pull request introduces several modifications to the parliamentary list creation service and related components. Key changes include the update of existing messages and the addition of new messages for user interface elements, a new method in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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 (6)
libs/application/templates/signature-collection/parliamentary-list-creation/src/dataProviders/index.ts (1)
14-17: LGTM! Consider grouping related APIs together.The addition of
IsDelegatedToCompanyApiis consistent with the existing pattern and adheres to the coding guidelines for thelibsdirectory. It's properly exported for reusability, uses TypeScript, and allows for effective tree-shaking.Consider grouping related APIs together for better code organization. For example, you might want to place
IsDelegatedToCompanyApinext toParliamentaryIdentityApiif they are closely related in functionality.libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Done.ts (2)
63-75: LGTM! Consider enhancing type safety.The new
buildMessageWithLinkButtonFieldcomponent is well-implemented and follows good practices for reusability and internationalization. The condition for rendering is clear and uses TypeScript effectively.To further improve type safety, consider defining an interface for the
externalDatastructure:interface ExternalData { delegatedToCompany: { data: { delegatedToCompany: boolean; }; }; }Then, update the condition function:
condition: (_, externalData: ExternalData) => { return !externalData.delegatedToCompany.data.delegatedToCompany; },This change would make the code more robust and easier to maintain.
76-89: LGTM! Consider refactoring to reduce duplication.The second
buildMessageWithLinkButtonFieldcomponent is well-implemented and consistent with the first one. It correctly handles the company-specific scenario.To reduce duplication and improve maintainability, consider refactoring both components into a single, more flexible component:
const delegationStatusButton = buildMessageWithLinkButtonField({ condition: (_, externalData: ExternalData) => true, // Always render id: 'done.goToServicePortal', title: '', url: (externalData: ExternalData) => externalData.delegatedToCompany.data.delegatedToCompany ? '/minarsidur/fyrirtaeki/listar/althingis-medmaelasofnun' : '/minarsidur/min-gogn/listar/althingis-medmaelasofnun', buttonTitle: (externalData: ExternalData) => externalData.delegatedToCompany.data.delegatedToCompany ? m.linkFieldButtonCompanyTitle : m.linkFieldButtonTitle, message: (externalData: ExternalData) => externalData.delegatedToCompany.data.delegatedToCompany ? m.linkFieldCompanyMessage : m.linkFieldMessage, });This refactoring would make the code more DRY and easier to maintain, while still preserving the functionality.
libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Prerequisites.ts (1)
94-98: LGTM: New data provider correctly addedThe
IsDelegatedToCompanyApidata provider is correctly added to thedataProvidersarray, following the existing pattern. This addition enhances the functionality related to delegation in the signature collection process.Consider adding a comment explaining the purpose of this new data provider, especially since the
titleandsubTitleare empty strings. This would improve code readability and maintainability.libs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-creation/parliamentary-list-creation.service.ts (1)
91-98: LGTM: New method is well-structured, but consider adding error handling and logging.The
delegatedToCompanymethod is correctly implemented and follows TypeScript best practices. It effectively checks for theProcurationHolderdelegation type and returns a boolean result.Suggestions for improvement:
- Consider adding error handling to catch any unexpected issues when accessing
auth.delegationType.- Add logging statements to track method invocation and results, which can aid in debugging.
Example implementation with improvements:
async delegatedToCompany({ auth }: TemplateApiModuleActionProps) { try { this.logger.debug('Checking delegation type for user', { nationalId: auth.nationalId }); const delegatedToCompany = auth.delegationType?.includes(AuthDelegationType.ProcurationHolder) ?? false; this.logger.info('Delegation check completed', { delegatedToCompany, nationalId: auth.nationalId }); return { delegatedToCompany }; } catch (error) { this.logger.error('Error checking delegation type', { error, nationalId: auth.nationalId }); throw new TemplateApiError('Failed to check delegation type', 500); } }These changes would enhance the robustness and observability of the method.
libs/application/templates/signature-collection/parliamentary-list-creation/src/lib/messages.ts (1)
305-315: LGTM! Consider a minor naming improvement.The new messages
linkFieldButtonCompanyTitleandlinkFieldCompanyMessageare well-structured and consistent with the existing code. They adhere to the coding guidelines for library files, including TypeScript usage and reusability across NextJS apps.For improved consistency, consider renaming
linkFieldCompanyMessagetolinkFieldMessageCompanyto match the pattern oflinkFieldButtonCompanyTitle.linkFieldButtonCompanyTitle: { id: 'plc.application:linkFieldButtonCompanyTitle', defaultMessage: 'Mínar síður (fyrirtæki)', description: '', }, -linkFieldCompanyMessage: { +linkFieldMessageCompany: { id: 'plc.application:linkFieldCompanyMessage', defaultMessage: 'Á mínum síðum fyrirtækja sést hve mörgum meðmælum hefur verið safnað í hverjum landsfjórðungi.', description: '', },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- libs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-creation/parliamentary-list-creation.service.ts (2 hunks)
- libs/application/templates/signature-collection/parliamentary-list-creation/src/dataProviders/index.ts (1 hunks)
- libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Done.ts (1 hunks)
- libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Prerequisites.ts (2 hunks)
- libs/application/templates/signature-collection/parliamentary-list-creation/src/lib/createCollectionTemplate.ts (2 hunks)
- libs/application/templates/signature-collection/parliamentary-list-creation/src/lib/messages.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
libs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-creation/parliamentary-list-creation.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/signature-collection/parliamentary-list-creation/src/dataProviders/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/signature-collection/parliamentary-list-creation/src/forms/Done.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/signature-collection/parliamentary-list-creation/src/forms/Prerequisites.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/signature-collection/parliamentary-list-creation/src/lib/createCollectionTemplate.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/signature-collection/parliamentary-list-creation/src/lib/messages.ts (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (7)
libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Done.ts (1)
63-89: Overall, the changes look good and align with the project's standards.The new components enhance the functionality related to delegation in the signature collection process. They are reusable, use TypeScript effectively, and support internationalization. The conditional rendering based on delegation status is well-implemented.
To further improve the code:
- Consider enhancing type safety by defining interfaces for external data structures.
- Explore refactoring the two similar components into a single, more flexible component to reduce duplication.
These changes would make the code more robust, maintainable, and aligned with best practices for library code in NextJS applications.
libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Prerequisites.ts (2)
23-23: LGTM: Import statement is correctly addedThe new import for
IsDelegatedToCompanyApiis correctly added to the existing imports from the dataProviders. This maintains consistency with the current code structure and follows TypeScript best practices.
23-23: Adherence to coding guidelines confirmedThe changes in this file adhere to the coding guidelines for the
libs/directory:
- They maintain reusability across different NextJS apps by extending existing functionality without breaking the structure.
- TypeScript is used consistently for defining types and imports.
- The changes don't negatively impact tree-shaking or bundling practices.
Also applies to: 94-98
libs/application/templates/signature-collection/parliamentary-list-creation/src/lib/createCollectionTemplate.ts (2)
24-24: LGTM: New API import follows existing patternsThe import of
IsDelegatedToCompanyApifrom '../dataProviders' is consistent with the existing import structure and naming conventions. It adheres to the TypeScript usage guidelines for the library.
85-85: Approved: New API added to APPLICANT roleThe addition of
IsDelegatedToCompanyApito theapiarray for the APPLICANT role is consistent with the existing structure and enhances the role's functionality. This change adheres to the coding guidelines for library files and maintains the component's reusability across different NextJS apps.To ensure this addition is necessary for all use cases of this template, please run the following verification script:
This script will help identify where and how the new API is used, allowing you to verify its necessity across different scenarios.
✅ Verification successful
To proceed with verifying the usage of
IsDelegatedToCompanyApi, please run the following script:
Verified:
IsDelegatedToCompanyApiusage confirmedThe usage of
IsDelegatedToCompanyApihas been confirmed in the following files:
libs/application/templates/signature-collection/parliamentary-list-creation/src/forms/Prerequisites.tslibs/application/templates/signature-collection/parliamentary-list-creation/src/lib/createCollectionTemplate.tslibs/application/templates/signature-collection/parliamentary-list-creation/src/dataProviders/index.tsNo conditional imports or usage were found, indicating that the addition is consistently applied and necessary across these components. Therefore, the addition of
IsDelegatedToCompanyApito theapiarray is verified and approved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of IsDelegatedToCompanyApi in related components # Test: Search for references to IsDelegatedToCompanyApi echo "Searching for references to IsDelegatedToCompanyApi:" rg "IsDelegatedToCompanyApi" --type typescript # Test: Check if there are any conditional imports or usage of this API echo "Checking for conditional imports or usage:" rg "IsDelegatedToCompanyApi.*\?|if.*IsDelegatedToCompanyApi" --type typescriptLength of output: 407
Script:
#!/bin/bash # Description: Check for usage of IsDelegatedToCompanyApi in related components # Test: Search for references to IsDelegatedToCompanyApi without specifying type echo "Searching for references to IsDelegatedToCompanyApi in all files:" rg "IsDelegatedToCompanyApi" # Test: Check if there are any conditional imports or usage of this API echo "Checking for conditional imports or usage of IsDelegatedToCompanyApi:" rg "IsDelegatedToCompanyApi.*\?|if.*IsDelegatedToCompanyApi"Length of output: 1117
libs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-creation/parliamentary-list-creation.service.ts (2)
23-23: LGTM: Import statement is correctly placed and follows best practices.The import of
AuthDelegationTypefrom the shared types package is well-structured and supports reusability across different NextJS apps. It also facilitates effective tree-shaking by importing only the specific type needed.
23-23: Summary: New method for detecting user delegation type is well-implemented.The changes in this file effectively introduce a new capability to detect user delegation type, specifically for company delegation. The implementation is consistent with the existing codebase and follows TypeScript best practices.
Key points:
- New import for
AuthDelegationTypesupports the added functionality.- The
delegatedToCompanymethod is well-structured and focused.- The changes align with the PR objective of detecting user delegation type.
While the implementation is solid, consider the suggestions in the previous comment to further enhance robustness and observability.
Also applies to: 91-98
…ub.com:/island-is/island.is into fix/parliamentary-list-creation-user-detection
kksteini
added
the
deprecated:automerge
kksteini
removed
the
deprecated:automerge
…amentary-list-creation-user-detection
kksteini
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/core/src/lib/fieldBuilders.ts (1)
Line range hint
527-540: Consider enhancing thebuildMessageWithLinkButtonFieldfunctionThe new function looks good and follows the established patterns in the file. However, consider the following improvements:
- Add TypeScript type annotations for the function parameters and return type to enhance type safety and developer experience.
- Include JSDoc comments to describe the function's purpose and parameters.
- Consider handling optional properties more robustly, using default values or optional chaining where appropriate.
Here's an example of how you could refactor the function:
/** * Builds a MessageWithLinkButtonField object. * @param {Omit<MessageWithLinkButtonField, 'type' | 'component' | 'children'>} data - The field data. * @returns {MessageWithLinkButtonField} The constructed field object. */ export const buildMessageWithLinkButtonField = ( data: Omit<MessageWithLinkButtonField, 'type' | 'component' | 'children'> ): MessageWithLinkButtonField => { const { url, message, buttonTitle, marginBottom, marginTop } = data; return { ...extractCommonFields(data), children: undefined, url, message, buttonTitle, marginTop: marginTop ?? undefined, marginBottom: marginBottom ?? undefined, type: FieldTypes.MESSAGE_WITH_LINK_BUTTON_FIELD, component: FieldComponents.MESSAGE_WITH_LINK_BUTTON_FIELD, }; };This refactored version includes type annotations, JSDoc comments, and handles potentially optional
marginTopandmarginBottomproperties.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/application/core/src/lib/fieldBuilders.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-creation/parliamentary-list-creation.service.ts (2 hunks)
- libs/application/templates/signature-collection/parliamentary-list-creation/src/dataProviders/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-creation/parliamentary-list-creation.service.ts
- libs/application/templates/signature-collection/parliamentary-list-creation/src/dataProviders/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/core/src/lib/fieldBuilders.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."
kodiakhq
bot
deleted the
fix/parliamentary-list-creation-user-detection
branch
* wip * wip * Small fixes * Conditioned * Fixed messages * Fixed conditionals
* fix(application-plc): Detect user delegation type (#16429) * wip * wip * Small fixes * Conditioned * Fixed messages * Fixed conditionals * feat(signature-collection): user not found handling (#16431) * feat(signature=collection): user not found handling * fix: text update * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collection): refetch is owner 16.10 (#16432) * fix(signature-collection): refetch is owner after remove last list * tw * tweak * tw * tw --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: Alex Diljar Birkisbur Hellsing <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: albinagu <[email protected]>
Checklist:
Summary by CodeRabbit