feat(auth-admin): View delegation and refactor access card #16243
Conversation
…tion in access control
WalkthroughThe changes in this pull request involve modifications across several components related to delegation management. A new Changes
Possibly related PRs
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
|
Datadog ReportAll test runs ✅ 15 Total Test Services: 0 Failed, 15 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16243 +/- ##
==========================================
- Coverage 36.78% 36.78% -0.01%
==========================================
Files 6805 6805
Lines 140832 140839 +7
Branches 40023 40025 +2
==========================================
+ Hits 51812 51814 +2
- Misses 89020 89025 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
magnearun
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (11)
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (3)
25-26: LGTM! Consider enhancing type safety for thevariantprop.The changes to the AccessCard props align well with the refactoring objectives. The removal of
canModifyand the change fromdirectiontovariantstreamline the component's API.To further improve type safety and maintain consistency with the
DelegationPropsinterface, consider using a type assertion or updating theAccessCardcomponent to accept the samedirectiontype:variant={direction as 'incoming' | 'outgoing'}This ensures that the
variantprop matches the expected values defined in theDelegationPropsinterface.
27-39: LGTM! Consider adding error handling to the delete operation.The new implementation of the
onDeleteprop is well-structured and aligns with the PR objectives. The conditional check fordelegation.referenceIdensures that the delete action is only available when appropriate.To further improve the robustness of the code, consider adding error handling:
onDelete={ !!delegation.referenceId && (async () => { try { const { data } = await deleteCustomDelegationAdminMutation({ variables: { id: delegation.id as string, }, }) if (data) { revalidate() } } catch (error) { console.error('Error deleting delegation:', error) // Consider adding user feedback here, e.g., showing an error toast } }) }This addition will ensure that any errors during the deletion process are properly caught and logged, improving the overall reliability of the application.
Line range hint
1-45: Enhance adherence to coding guidelines for library components.While the component generally follows the guidelines for
libs/**/*, consider the following improvements:
- Export the
DelegationPropsinterface to allow reuse in other components:export interface DelegationProps { direction: 'incoming' | 'outgoing' delegationsList: AuthCustomDelegation[] }
- To improve tree-shaking, consider exporting the component as a named export instead of default:
export const DelegationList = ({ delegationsList, direction }: DelegationProps) => { // ... component implementation ... }These changes will enhance the reusability of the component and its types across different NextJS apps while also improving tree-shaking capabilities.
libs/api/mocks/src/domains/auth/factories.ts (1)
47-51: LGTM! Consider making thetypefield configurable.The addition of the
createdByproperty aligns well with the PR objectives and follows the existing code style. Good job on maintaining consistency with the other properties in the factory.A minor suggestion for improvement:
Consider making the
typefield configurable instead of hardcoding it as 'Person'. This would allow for more flexibility in generating mock data. Here's a possible implementation:- createdBy: () => ({ + createdBy: (type = 'Person') => ({ nationalId: createNationalId('person'), name: faker.name.findName(), - type: 'Person', + type, }),This change would allow you to specify a different type when needed while keeping 'Person' as the default.
libs/api/domains/auth/src/lib/models/delegation.model.ts (1)
54-56: LGTM! Consider adding a comment for clarity.The addition of the
createdByfield to theDelegationabstract class is well-implemented and aligns with the PR objectives. The use of TypeScript for prop definition and the@Fielddecorator from@nestjs/graphqladheres to the coding guidelines.Consider adding a brief comment explaining the purpose of the
createdByfield for improved code documentation:@Field(() => Identity, { nullable: true }) +// Represents the identity of the user who created the delegation createdBy?: Identitylibs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.resolver.ts (1)
94-103: LGTM with a minor suggestion for improvementThe new
resolveCreatedByIdentitymethod is well-implemented and adheres to the coding guidelines. It uses TypeScript, leverages the reusableIdentityLoader, and is part of a tree-shakeable resolver class.To further improve type safety, consider explicitly typing the return value:
@ResolveField('createdBy', () => Identity, { nullable: true }) resolveCreatedByIdentity( @Loader(IdentityLoader) identityLoader: IdentityDataLoader, @Parent() customDelegation: DelegationDTO, ): Promise<Identity | null> { if (!customDelegation.createdByNationalId) { return Promise.resolve(null); } return identityLoader.load(customDelegation.createdByNationalId); }This change makes the return type explicit and ensures that the method always returns a Promise, maintaining consistency with other resolver methods.
libs/api/domains/auth/src/lib/resolvers/delegation.resolver.ts (1)
131-140: LGTM! Consider adding error handling.The new
resolveCreatedBymethod is well-implemented and aligns with the PR objectives. It properly uses TypeScript, follows the existing coding patterns, and handles the case wherecreatedByNationalIdmight not exist.Consider adding error handling for the
getIdentitycall:@ResolveField('createdBy', () => Identity, { nullable: true }) async resolveCreatedBy( @Parent() delegation: DelegationDTO, ): Promise<Identity | null> { if (!delegation.createdByNationalId) { return null; } try { return await this.identityService.getIdentity(delegation.createdByNationalId); } catch (error) { console.error(`Failed to resolve identity for createdByNationalId: ${delegation.createdByNationalId}`, error); return null; } }This change would ensure that any errors in fetching the identity are logged and don't cause the entire resolver to fail.
libs/portals/shared-modules/delegations/src/lib/messages.ts (2)
117-120: LGTM! Consider adding a description.The
createdBymessage is correctly structured and aligns with the PR objectives. It follows the existing patterns in the file and contributes to the reusability of components across different NextJS apps.Consider adding a description for this message to provide context for translators, similar to some other messages in this file. For example:
createdBy: { id: 'sp.access-control-delegations:created-by', defaultMessage: 'Skráð af', description: 'Label indicating who created the delegation', },
238-241: LGTM! Consider adding a description.The
referenceIdmessage is correctly structured and aligns with the PR objectives. It follows the existing patterns in the file and contributes to the reusability of components across different NextJS apps.Consider adding a description for this message to provide context for translators. For example:
referenceId: { id: 'sp.access-control-delegations:reference-id', defaultMessage: 'Númer máls í Zendesk', description: 'Label for the Zendesk reference ID', },libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1)
207-210: LGTM: Proper TypeScript usage with a suggestion for improvementThe component demonstrates good use of TypeScript for defining props and types, aligning with the coding guidelines. The type casting on line 207 ensures type safety when accessing properties of different delegation types.
However, consider using a type guard function to determine the delegation type instead of type casting. This could improve type inference and make the code more robust:
function isOutgoingDelegation(delegation: AuthCustomDelegation): delegation is AuthCustomDelegationOutgoing { return 'to' in delegation; } // Usage {isOutgoing ? isOutgoingDelegation(delegation) ? delegation.to.name : '' : 'from' in delegation ? delegation.from.name : ''}This approach would provide better type safety and make the code more self-documenting.
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
118-118: Ensure the image path inimgSrcis correct and accessibleThe image path
./assets/images/skjaldarmerki.svgmay not resolve correctly depending on the build configuration and directory structure. Consider importing the image directly or verifying that the relative path is correct in the context of the compiled code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (15)
- libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.resolver.ts (1 hunks)
- libs/api/domains/auth/src/lib/models/delegation.model.ts (1 hunks)
- libs/api/domains/auth/src/lib/resolvers/delegation.resolver.ts (1 hunks)
- libs/api/mocks/src/domains/auth/factories.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/dto/delegation.dto.ts (1 hunks)
- libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (1 hunks)
- libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (6 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (3 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncoming.graphql (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (4 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.graphql (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (5 hunks)
- libs/portals/shared-modules/delegations/src/lib/messages.ts (2 hunks)
- libs/portals/shared-modules/delegations/src/types/customDelegation.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (15)
libs/api/domains/auth-admin/src/lib/delegationAdmin/delegation-admin.resolver.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/api/domains/auth/src/lib/models/delegation.model.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/api/domains/auth/src/lib/resolvers/delegation.resolver.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/api/mocks/src/domains/auth/factories.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/auth-api-lib/src/lib/delegations/dto/delegation.dto.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/auth-api-lib/src/lib/delegations/models/delegation.model.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/portals/admin/delegation-admin/src/components/DelegationList.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncoming.graphql (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/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.graphql (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/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/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/portals/shared-modules/delegations/src/types/customDelegation.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 (19)
libs/portals/shared-modules/delegations/src/types/customDelegation.ts (2)
Line range hint
1-17: Well-structured types for reusability across NextJS apps.The file demonstrates good practices in TypeScript usage:
- Clear type definitions using GraphQL query results.
- Effective use of utility types like
Extract.- Export of well-defined types for reuse.
These practices align with the coding guidelines for files in the
libsdirectory, promoting reusability and type safety across different NextJS apps.
1-1: LGTM! Verify the new import path.The simplified import path aligns with good practices for module organization and potentially improves tree-shaking. This change appears to be part of a module restructuring.
Please run the following script to ensure the new import path is correct and the file exists:
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncoming.graphql (1)
25-37: Excellent additions to the AuthDelegationsIncoming query!The new fields
referenceId,to, andcreatedByalign perfectly with the PR objectives. These additions enhance the query's functionality by providing more detailed information about delegations, specifically:
- The
referenceIdfield allows for unique identification of general mandate delegations.- The
tofield provides clear information about the delegation recipient.- The
createdByfield indicates who created the general mandate delegations, as specified in the PR objectives.These changes improve the component's reusability across different NextJS apps and support the enhanced functionality of the AccessCard component mentioned in the PR summary.
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.graphql (1)
27-40: LGTM! Changes align well with PR objectives.The additions to the
AuthDelegationsOutgoingquery (validTo,referenceId,from, andcreatedByfields) are consistent with the PR objectives. These changes enhance the query's functionality by providing more detailed information about delegations, particularly thecreatedByfield for general mandate delegations.The modifications maintain the query's reusability across different NextJS apps and adhere to the coding guidelines for
libs/**/*files.libs/api/domains/auth/src/lib/models/delegation.model.ts (1)
Line range hint
1-124: Overall assessment: Code meets guidelines and PR objectives.The changes in this file adhere to the coding guidelines for
libs/**/*files:
- The code maintains reusability across different NextJS apps.
- TypeScript is used effectively for defining props and exporting types.
- The change supports effective tree-shaking and bundling practices.
The addition of the
createdByfield to theDelegationabstract class aligns with the PR objectives and is well-implemented.libs/auth-api-lib/src/lib/delegations/dto/delegation.dto.ts (2)
71-74: LGTM: New property aligns with PR objectives and follows best practices.The addition of the
createdByNationalIdproperty to theDelegationDTOclass is well-implemented:
- It uses TypeScript for type definition (
string | null).- The
@IsOptional()and@IsString()decorators ensure proper validation.- The
@ApiPropertyOptional()decorator provides correct API documentation.This change aligns with the PR objective to introduce a
createdByfield for Delegations, enhancing the data structure without altering existing properties.
Line range hint
1-138: Excellent: File adheres to coding guidelines forlibsdirectory.This file demonstrates good practices for code in the
libsdirectory:
- Reusability: The DTO classes defined here can be easily used across different NextJS apps for handling delegation data.
- TypeScript usage: Props are well-defined using TypeScript, and types are properly exported.
- Tree-shaking and bundling: The file contains only type definitions and decorators, which allows for effective tree-shaking and bundling.
The use of decorators from
class-validatorand@nestjs/swaggerenhances the reusability of these DTOs for both backend validation and API documentation.libs/auth-api-lib/src/lib/delegations/models/delegation.model.ts (1)
144-144: LGTM: Addition ofcreatedByNationalIdaligns with PR objectivesThe addition of
createdByNationalIdto thetoDTOmethod's return object is correct and aligns with the PR objective of introducing acreatedByfield to Delegations. This change enhances the DTO representation without affecting the overall structure or logic of the method.The modification adheres to the coding guidelines for
libs/**/*files:
- It maintains reusability across NextJS apps.
- It uses TypeScript consistently with the rest of the code.
- It doesn't introduce any issues for tree-shaking or bundling.
To ensure this change is reflected in the
DelegationDTOtype, let's verify its definition:libs/portals/shared-modules/delegations/src/lib/messages.ts (1)
Line range hint
1-241: Overall, the changes align well with coding guidelines and PR objectives.The additions to this file (
createdBy,noValidToDate, andreferenceIdmessages) contribute to the internationalization setup and support the new features described in the PR objectives. The file structure adheres to the coding guidelines forlibs/**/*:
- It supports reusability of components across different NextJS apps by providing centralized message definitions.
- It implicitly uses TypeScript through the
defineMessagesfunction fromreact-intl.- The structure of the file supports effective tree-shaking and bundling practices.
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (5)
51-54: LGTM: Improved flexibility in action handlingThe changes to the
AccessCardPropsinterface enhance the component's flexibility by making action callbacks optional and allowing them to be explicitly disabled. This aligns well with the PR objective of refactoring the AccessCard to display specific actions based on provided functions.
65-66: LGTM: Component props aligned with interface updatesThe destructuring of
onEditandonRenewprops in the AccessCard component correctly aligns with the updates made to theAccessCardPropsinterface. This change supports the new functionality for editing and renewing delegations as intended in the PR objectives.
162-162: LGTM: Simplified and improved action visibility logicThe
hasActionslogic has been streamlined to include the newonEditaction. This change ensures that the actions section is rendered when any of the action callbacks are provided, aligning with the PR objective of improving the logic governing the visibility of different actions.
263-270: LGTM: Enhanced action button rendering logicThe changes to the action button rendering logic significantly improve the component's flexibility and contextual appropriateness:
- The
onDeletebutton is now conditionally rendered with proper optional chaining.- The
onViewbutton rendering has been updated for consistency.- New conditional rendering for
onEditandonRenewbuttons has been added, taking into account theisExpiredstate.These improvements align well with the PR objective of ensuring that the actions displayed on the AccessCard are contextually appropriate and match the design specifications.
Also applies to: 277-285, 286-309
Line range hint
1-324: Overall assessment: Well-implemented refactoringThe changes to the AccessCard component successfully achieve the PR objectives:
- The refactoring enhances the component's flexibility in displaying specific actions.
- The logic for action visibility has been streamlined.
- The component now handles the new
createdByfield for general mandate delegations (although not directly visible in this file).The implementation adheres to the coding guidelines for reusability and TypeScript usage. The component is well-structured for potential use across different NextJS apps within the project.
Great job on this refactoring! The changes improve the component's functionality and maintainability.
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (3)
16-16: Ensure consistent import paths for generated hooksThe import path for
DelegationViewModalis updated correctly. However, please ensure that the import paths for all generated hooks (useAuthDelegationsIncomingQuery) are consistent and accurately reflect their locations after any refactoring.
114-119:⚠️ Potential issueEnsure
DelegationViewModalhandles multiple delegation typesThe
DelegationViewModalcomponent receivesdelegationcast asAuthCustomDelegationIncoming, but it may also receive aGeneralMandatedelegation. Ensure thatDelegationViewModalcorrectly handles all delegation types being passed to it.Review the
DelegationViewModalcomponent to confirm it supportsAuthGeneralMandateDelegationIncoming.Run the following script to verify the props of
DelegationViewModal:#!/bin/bash # Description: Check the `DelegationViewModal` component's `delegation` prop type rg 'DelegationViewModal' -A 10 | rg 'delegation'This will display where the component is defined and how the
delegationprop is typed.
16-17: Confirm effective tree-shaking with updated importsThe import statement for
DelegationViewModalhas been updated. Please ensure that this change does not impact tree-shaking and bundling practices, keeping the build efficient.Run the following script to analyze the bundle size impact:
I can help draft configurations or scripts for bundle analysis if needed.
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (2)
132-145:onEditandonRenewhandlers navigate to the same routeBoth the
onEditandonRenewhandlers navigate to the same path:navigate(`${DelegationPaths.Delegations}/${delegation.id}`)If the intent is for these actions to perform different functions, consider updating the navigation paths or handler logic to reflect their distinct purposes.
36-37:⚠️ Potential issuePotential type mismatch in
delegationViewstateThe
delegationViewstate is initialized with the typeAuthCustomDelegationIncoming | nullin an outgoing delegations component. Since this component handles outgoing delegations, using the incoming delegation type may be incorrect. Please verify that the correct delegation type is being used to prevent potential type errors.
...rtals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
Outdated
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
Outdated
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
Outdated
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Outdated
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx
Outdated
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx
Outdated
Show resolved
Hide resolved
…b.com:island-is/island.is into feat/view-delegation-and-refactor-access-card
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (3)
59-62: LGTM: Improved type checking, consider further enhancementThe introduction of
isCustomandisGeneralMandateconstants improves code readability and type safety. This aligns well with the PR objectives for refactoring the AccessCard component.However, consider implementing a type guard function as suggested in a previous review to further enhance type safety:
const isDelegationType = (delegation: AuthCustomDelegationIncoming, type: AuthDelegationType): boolean => { return delegation.type === type; }; const isCustom = isDelegationType(delegation, AuthDelegationType.Custom); const isGeneralMandate = isDelegationType(delegation, AuthDelegationType.GeneralMandate);This approach would provide better type inference and make the code more maintainable.
115-119: LGTM: Updated modal component, consider type-safe approachThe replacement of
DelegationIncomingModalwithDelegationViewModaland the addition of thedirectionprop align well with the PR objectives. This change enhances the flexibility of the modal component.However, the unsafe type casting to
AuthCustomDelegationIncomingpersists. Consider implementing a type-safe approach to handle different delegation types. For example:delegation={delegationView as AuthCustomDelegationIncoming | AuthGeneralMandateDelegationIncoming}This approach allows the modal to handle both custom and general mandate delegations safely.
Line range hint
1-122: Overall assessment: Good improvements with room for enhancementThe changes in this file align well with the PR objectives, particularly in refactoring the AccessCard component and updating the View modal. The introduction of type checks and conditional rendering enhances the component's flexibility and functionality.
However, there are several areas where the code could be further improved:
- Type safety: Implement type guards or use union types to avoid unsafe type castings throughout the component.
- Code simplification: Simplify the
onDeleteandonViewhandlers as suggested in the review comments.- Consistency: Ensure consistent handling of different delegation types across the component.
Addressing these points will significantly improve the code's robustness, maintainability, and adherence to TypeScript best practices.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1 hunks)
- libs/portals/admin/delegation-admin/src/lib/messages.ts (1 hunks)
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (7 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (3 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (4 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (5 hunks)
- libs/portals/shared-modules/delegations/src/lib/messages.ts (2 hunks)
- libs/portals/shared-modules/delegations/src/types/customDelegation.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- libs/portals/admin/delegation-admin/src/lib/messages.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/portals/admin/delegation-admin/src/components/DelegationList.tsx
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx
- libs/portals/shared-modules/delegations/src/lib/messages.ts
- libs/portals/shared-modules/delegations/src/types/customDelegation.ts
🧰 Additional context used
📓 Path-based instructions (3)
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.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 (4)
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (1)
15-16: LGTM: Import changes align with PR objectivesThe import changes reflect the refactoring mentioned in the PR objectives. The
DelegationViewModalimport and the updated path foruseAuthDelegationsIncomingQueryare consistent with the described changes.libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
20-22: Props definition aligns with TypeScript best practicesThe
DelegationViewModalPropstype correctly defines the component's props using TypeScript, enhancing type safety and maintainability across different NextJS apps.libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (2)
127-130: Avoid explicit type casting by refining delegation typesIn the
onDeletehandler,delegationis explicitly cast toAuthCustomDelegationOutgoing. This suggests that thedelegationvariable may not have a precise type. To improve type safety and maintainability, consider refining thedelegationtype or implementing type guards instead of using explicit casting.
152-155: Type castingdelegationtoAuthCustomDelegationIncomingmay be unsafeIn the
onViewhandler,delegationis cast toAuthCustomDelegationIncomingbefore settingdelegationView. Sincedelegationoriginates from outgoing delegations, casting it to an incoming type might lead to runtime errors or unexpected behavior. Consider checking the delegation type and using appropriate type guards to ensure type safety.
...rtals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
Outdated
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (2)
25-57: LGTM with a minor suggestion: Component declaration and initial logicThe component declaration and initial logic are well-structured. Good use of hooks and proper handling of props. The Modal 'id' now correctly reflects the delegation direction, addressing the previous comment.
A minor suggestion for improvement:
Consider using optional chaining in the useEffect hook's condition:
- if (delegation && delegation.domain) { + if (delegation?.domain) {This change would make the code more concise and consistent with modern JavaScript practices.
🧰 Tools
🪛 Biome
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
98-151: LGTM with a suggestion: GeneralMandate render logicThe render logic for GeneralMandate delegations is well-implemented with proper conditional rendering and localization. The responsive layout using Box components is a good approach.
However, there's room for improvement in the date validation:
While you've added a check for date validity using
isValid, it's better to handle potentialundefinedvalues explicitly. Consider updating the date formatting logic as follows:- delegation?.validTo && isValid(delegation.validTo) - ? format(new Date(delegation?.validTo), 'dd.MM.yyyy') - : formatMessage(m.noValidToDate) + delegation?.validTo && isValid(new Date(delegation.validTo)) + ? format(new Date(delegation.validTo), 'dd.MM.yyyy') + : formatMessage(m.noValidToDate)This change ensures that both
undefinedvalues and invalid dates are handled correctly.libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (1)
182-187: LGTM: DelegationViewModal added with minor suggestionThe addition of the DelegationViewModal component aligns well with the PR objectives for handling general mandate delegations. The component props are correctly typed and used.
Consider simplifying the
delegationprop:- delegation={delegationView || undefined} + delegation={delegationView}This change is possible because
nullis a valid value for an optional prop in React, and it simplifies the code slightly.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.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."
🪛 Biome
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (2)
1-23: LGTM: Imports and type definitions are well-structuredThe imports cover all necessary dependencies for the component's functionality. The
DelegationViewModalPropstype is correctly defined to handle both incoming and outgoing delegations. Good job addressing the previous comment about renaming the props type.
152-163: LGTM: AccessListContainer renderingThe conditional rendering of the AccessListContainer for non-GeneralMandate type delegations is well-implemented. All necessary props are correctly passed to the component. Good job on using the AuthDelegationType enum consistently for the type comparison, addressing the previous comment.
libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (4)
9-9: LGTM: New imports align with PR objectivesThe added imports for
AuthDelegationType,AuthCustomDelegationIncoming,AuthCustomDelegationOutgoing,useNavigate, andDelegationViewModalare consistent with the PR objectives. They support the new functionality for handling different delegation types and the introduction of the view modal.Also applies to: 18-21, 25-26
36-37: LGTM: New state variable for delegation viewThe addition of the
delegationViewstate variable of typeAuthCustomDelegationIncoming | nullis in line with the PR objectives. It provides the necessary state management for handling general mandate delegations and displaying them in the view modal.
39-39: LGTM: Added useNavigate hook for navigationThe addition of the
useNavigatehook from react-router-dom is appropriate for handling navigation to edit and renew delegations. This change aligns with React best practices for navigation in functional components.
Line range hint
1-190: Overall: Good refactor with room for minor improvementsThe changes in this file successfully meet the PR objectives of refactoring the AccessCard component and handling general mandate delegations. The code adheres to the coding guidelines, effectively using TypeScript for props and types, and maintaining a structure that supports reusability across different NextJS apps.
Key improvements:
- Proper handling of different delegation types, particularly general mandates.
- Introduction of the DelegationViewModal for viewing general mandate delegations.
- Conditional rendering of AccessCard props based on delegation type.
Suggestions for further improvement:
- Enhance type safety by using type guards instead of explicit type casting.
- Simplify the conditional logic for AccessCard props using a helper function.
- Minor simplification of the DelegationViewModal delegation prop.
These changes have significantly improved the component's functionality while maintaining good coding practices. Implementing the suggested improvements will further enhance the code's robustness and maintainability.
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Outdated
Show resolved
Hide resolved
...rtals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
libs/portals/shared-modules/delegations/src/index.ts (1)
Line range hint
1-15: Confirm compliance with coding guidelinesThe file structure and export statements comply with the coding guidelines for
libs/**/*files:
- Supports reusability across different NextJS apps
- Uses TypeScript for exporting (implicit in export statements)
- Enables effective tree-shaking through granular exports
For improved consistency and maintainability, consider grouping related exports together (e.g., all delegation-related components).
Here's a suggested reorganization of the exports:
// Core module export { delegationsModule } from './module' // Utilities export * from './lib/paths' export * from './lib/navigation' // Hooks export * from './hooks/useDynamicShadow' // Components export * from './components/access/AccessCard' export * from './components/IdentityCard/IdentityCard' // Delegation-specific components export * from './components/delegations/DelegationsEmptyState' export * from './components/delegations/DelegationsFormFooter' export * from './components/delegations/DelegationViewModal'libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (4)
15-21: LGTM: New state variables enhance functionalityThe introduction of
delegationToDeleteanddelegationViewstate variables aligns with the PR objectives of improving delegation management. The use of TypeScript for type safety is commendable.Consider using more descriptive names for the state variables, such as
selectedDelegationForDeletionandselectedDelegationForView, to enhance code readability.
23-33: LGTM: Effective deletion handler implementationThe
deleteHandlerfunction is well-implemented, aligning with the PR objectives of improving delegation management. It correctly uses the mutation and updates the state accordingly.Consider adding error handling to the
deleteHandlerfunction. For example:const deleteHandler = async (id: string) => { try { const { data } = await deleteCustomDelegationAdminMutation({ variables: { id }, }); if (data) { revalidate(); setDelegationToDelete(null); } } catch (error) { console.error('Error deleting delegation:', error); // Consider adding user feedback for the error case } };This will improve the robustness of the function and provide better feedback in case of errors.
57-68: LGTM: DelegationDeleteModal enhances user interactionThe introduction of the DelegationDeleteModal component aligns well with the PR objectives of improving delegation management. It provides a clear user interface for confirming deletion actions.
Consider extracting the
onDeletelogic into a separate function for better readability:const handleDelete = () => { if (delegationToDelete) { deleteHandler(delegationToDelete.id as string); } }; // Then in the component props: onDelete={handleDelete}This would make the component props more concise and easier to read.
Line range hint
1-82: Overall: Excellent refactoring and feature enhancementThe changes in this file significantly improve the delegation management functionality, aligning well with the PR objectives. The introduction of new components (DelegationDeleteModal and DelegationViewModal) and state variables enhances the user experience for viewing and deleting delegations.
Key improvements:
- Refactored AccessCard component for better flexibility and reusability.
- Added robust deletion handling with confirmation modal.
- Implemented viewing functionality for delegation details.
- Effective use of TypeScript for type safety.
These changes contribute to a more maintainable and user-friendly delegation management system in the admin portal.
Consider creating a custom hook (e.g.,
useDelegationManagement) to encapsulate the deletion and viewing logic. This would further improve the component's reusability and make it easier to test the business logic independently of the UI components.libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
20-24: Consider renaming the props type for consistencyThe props type
DelegationViewModalPropsis well-defined, but it could be more consistent with the component name. Consider renaming it toDelegationViewModalPropsto match the component name exactly.libs/portals/shared-modules/delegations/src/lib/messages.ts (4)
117-120: Consider adding a description for thecreatedBymessage.The
createdBymessage is correctly structured and aligns with the PR objectives. To enhance clarity for translators, consider adding a description. For example:createdBy: { id: 'sp.access-control-delegations:created-by', defaultMessage: 'Skráð af', description: 'Label indicating who created the delegation', },This addition will improve the context for translators and maintain consistency with other message definitions in the file.
234-237: LGTM! Consider adding a description.The
noValidToDatemessage is correctly structured and the typo has been fixed as per the previous review comment. To further improve clarity for translators, consider adding a description. For example:noValidToDate: { id: 'sp.access-control-delegations:no-valid-to-date', defaultMessage: 'Gildistími óendanlegur', description: 'Message indicating that the validity period is indefinite', },This addition will provide better context for translators and maintain consistency with other message definitions in the file.
238-241: Consider adding a description for thereferenceIdmessage.The
referenceIdmessage is correctly structured and aligns with the PR objectives. To enhance clarity for translators, consider adding a description. For example:referenceId: { id: 'sp.access-control-delegations:reference-id', defaultMessage: 'Númer máls í Zendesk', description: 'Label for the Zendesk case number associated with the delegation', },This addition will improve the context for translators and maintain consistency with other message definitions in the file.
246-249: Consider adding a description for theviewDelegationModalTitlemessage.The
viewDelegationModalTitlemessage is correctly structured and aligns with the PR objectives. To enhance clarity for translators, consider adding a description. For example:viewDelegationModalTitle: { id: 'sp.access-control-delegations:view-delegation-modal-title', defaultMessage: 'Upplýsingar um umboð', description: 'Title for the modal displaying delegation information', },This addition will provide better context for translators and maintain consistency with other message definitions in the file.
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (2)
52-55: LGTM: Improved flexibility with optional callbacksThe changes to
AccessCardPropsalign with the PR objectives and improve the component's flexibility. Consider using a consistent style for optional props:onDelete?: (delegation: AuthCustomDelegation) => void; onEdit?: (delegation: AuthCustomDelegation) => void; onView?: (delegation: AuthCustomDelegation) => void; onRenew?: (delegation: AuthCustomDelegation) => void;This style (with semicolons) is more common in TypeScript interfaces.
163-163: LGTM: Improved logic and type safetyThe changes improve the component's logic and type safety:
- The
hasActionscondition simplifies action visibility logic.- The access holder's name rendering is now more type-safe.
- The
marginTopadjustment ensures proper spacing.Consider using nullish coalescing for the access holder's name:
{isOutgoing ? (delegation as AuthCustomDelegationOutgoing)?.to?.name ?? 'Unknown' : (delegation as AuthCustomDelegationIncoming)?.from?.name ?? 'Unknown' }This would handle cases where the name might be undefined or null.
Also applies to: 208-210, 232-232
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1 hunks)
- libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (2 hunks)
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (8 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (4 hunks)
- libs/portals/shared-modules/delegations/src/index.ts (1 hunks)
- libs/portals/shared-modules/delegations/src/lib/messages.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
🧰 Additional context used
📓 Path-based instructions (6)
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (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/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/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/portals/shared-modules/delegations/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."
🪛 Biome
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (13)
libs/portals/shared-modules/delegations/src/index.ts (1)
15-15: LGTM: New export enhances module functionalityThe addition of the
DelegationViewModalexport aligns with the existing structure and enhances the module's capabilities. This change supports the PR's objectives by making the new component available for use across the application.To ensure the new component is properly integrated, please run the following script:
libs/portals/admin/delegation-admin/src/screens/DelegationAdminDetails/DelegationAdmin.graphql (3)
25-28: LGTM: Addition ofcreatedByfield enhances delegation information.The introduction of the
createdByfield withnameandnationalIdsubfields aligns well with the PR objectives. This addition provides valuable context about who created the delegations, enhancing the overall functionality of the delegation management system.
56-59: LGTM: Consistent implementation ofcreatedByfield in outgoing delegations.The addition of the
createdByfield to theoutgoingsection maintains consistency with theincomingsection. This ensures that creator information is available for both incoming and outgoing delegations, providing a comprehensive view of delegation data.
Line range hint
1-79: Overall assessment: Changes align with coding guidelines and enhance functionality.The modifications to the
getCustomDelegationsAdminquery, specifically the addition of thecreatedByfield to bothincomingandoutgoingsections, adhere to the coding guidelines for files in thelibsdirectory:
- These changes enhance the reusability of the query across different NextJS apps by providing more comprehensive delegation data.
- The structured nature of the GraphQL query supports strong typing when used with TypeScript in consuming components.
- The additions don't negatively impact tree-shaking or bundling practices.
The changes successfully implement the PR objectives by introducing the
createdByfield to Delegations, improving the overall functionality of the delegation management system.libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (3)
3-3: LGTM: New imports align with PR objectivesThe addition of
DelegationViewModal,useState, andDelegationDeleteModalimports aligns well with the PR objectives of enhancing the delegation management functionality. These changes adhere to the coding guidelines for TypeScript usage and component reusability.Also applies to: 6-7
41-52: LGTM: AccessCard refactor improves flexibilityThe refactoring of the AccessCard component aligns well with the PR objectives. The new props and conditional rendering of
onViewandonDeleteenhance the component's flexibility and reusability.As per the previous review comment by saevarma, the
DelegationViewModalis now being reused in the admin portal for viewing delegation details, which is a good implementation of the suggested improvement.
69-75: LGTM: DelegationViewModal improves user experienceThe addition of the DelegationViewModal component aligns perfectly with the PR objectives of enhancing the view functionality for delegations. It provides a clear interface for users to view delegation details, improving the overall user experience.
The component follows good practices for modal implementation and prop passing, enhancing the reusability and maintainability of the code.
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1)
1-164: Overall assessment: Well-structured and reusable componentThe
DelegationViewModalcomponent is well-implemented and adheres to the coding guidelines:
- Reusability: The component is designed to be reusable across different NextJS apps within the Island.is ecosystem.
- TypeScript usage: Props and types are well-defined using TypeScript, enhancing type safety.
- Effective tree-shaking: The component structure allows for efficient tree-shaking and bundling.
The component effectively uses the Island.is design system and follows React best practices. The suggested improvements in previous comments will further enhance its quality, particularly in terms of optimization, accessibility, and error handling.
🧰 Tools
🪛 Biome
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/portals/shared-modules/delegations/src/lib/messages.ts (1)
Line range hint
117-249: LGTM! The changes align with coding guidelines and PR objectives.The new message entries (
createdBy,noValidToDate,referenceId, andviewDelegationModalTitle) have been correctly implemented and contribute to the reusability of components across different NextJS apps. The changes adhere to TypeScript usage for defining props and align with the PR objectives, particularly in enhancing the delegation information display.To further improve the file:
- Consider adding descriptions to all new message entries for better context for translators.
- Ensure consistent formatting across all message entries (some have trailing commas, others don't).
These changes effectively support the internationalization setup and enhance the overall functionality of the delegation management system.
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (4)
27-27: LGTM: Import and type safety improvementsThe import of
AuthCustomDelegationand the changes to thegetTagsfunction improve type safety and align with TypeScript best practices.Also applies to: 39-41
66-67: LGTM: Destructuring new optional propsThe addition of
onEditandonRenewto the destructured props is consistent with the interface changes and allows the component to use these new optional callbacks.
278-286: LGTM: Improved view button renderingThe updated rendering logic for the view button aligns with the PR objectives and ensures the button is only displayed when the
onViewcallback is provided. This change improves the component's flexibility and adheres to the principle of conditional rendering based on prop availability.
Line range hint
1-321: Overall assessment: Successful refactoring with minor improvements neededThe refactoring of the AccessCard component has significantly improved its flexibility and functionality, aligning well with the PR objectives. Key improvements include:
- More flexible prop structure with optional callbacks.
- Enhanced type safety throughout the component.
- Improved conditional rendering of action buttons.
- Better handling of different delegation types.
These changes contribute to a more maintainable and reusable component. The suggested minor improvements, mainly removing unnecessary optional chaining, will further refine the code quality.
Great job on this refactoring! Once the minor issues are addressed, this PR will be ready for merge.
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx
Outdated
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (3)
39-41: LGTM: Improved type safety in tag name retrievalThe update enhances type safety by using optional chaining to handle potential undefined values. This change aligns with TypeScript best practices.
Consider using nullish coalescing for a more concise expression:
name: scope?.apiScope ? getTagName(scope.apiScope as AuthApiScope) : scope.displayName ?? '',This ensures a fallback to an empty string if both
apiScopeanddisplayNameare null or undefined.
Line range hint
163-232: LGTM: Improved action availability check and delegation holder renderingThe changes in this segment improve the component in two ways:
- The action availability check is now more concise and readable.
- The rendering of the delegation holder's name now correctly handles both incoming and outgoing delegations.
These updates align with the PR objectives to refactor the AccessCard component for better functionality.
Consider using optional chaining for accessing nested properties to further improve type safety:
{isOutgoing ? delegation.to?.name : delegation.from?.name}This change would handle cases where
toorfrommight be undefined.
Line range hint
256-310: LGTM: Updated rendering logic for action buttonsThe changes to the action button rendering logic align well with the PR objectives:
- Buttons are only rendered when their corresponding callback functions are provided, improving flexibility.
- The conditional rendering based on the
isExpiredstate enhances contextual appropriateness.These updates significantly improve the component's ability to display specific actions based on the provided functions, as intended in the PR objectives.
However, there are two instances of unnecessary optional chaining that should be removed:
- Line 271:
- onClick={() => onDelete?.(delegation)} + onClick={() => onDelete(delegation)}
- Line 305:
- onClick={() => onRenew?.(delegation)} + onClick={() => onRenew(delegation)}These changes maintain the intended functionality while removing redundant code, as
onDeleteandonReneware already checked in their respective conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/shared-modules/delegations/src/components/access/AccessCard.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 (4)
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (4)
27-27: LGTM: Import statement added for better type safetyThe addition of the
AuthCustomDelegationimport from@island.is/api/schemaenhances type safety and aligns with our TypeScript usage guidelines.
52-55: LGTM: AccessCardProps interface updated with new optional propertiesThe changes to the
AccessCardPropsinterface align well with the PR objectives:
onDeleteis now optional, allowing more flexible usage.- New optional properties
onEditandonRenewhave been added, enhancing the component's functionality.These updates improve the reusability of the component across different NextJS apps, as specified in the coding guidelines.
66-67: LGTM: New props added to AccessCard componentThe addition of
onEditandonRenewto the component's prop destructuring is consistent with the updates to theAccessCardPropsinterface. This change enables the component to utilize the new optional callback functions, enhancing its flexibility and functionality as intended in the PR objectives.
Line range hint
1-324: Overall assessment: Approved with minor suggestionsThe refactoring of the AccessCard component has been successfully implemented, aligning well with the PR objectives and coding guidelines. Key improvements include:
- Enhanced flexibility through new optional properties and conditional rendering.
- Improved type safety with better handling of potential undefined values.
- More concise and readable code for action availability checks.
- Better handling of different delegation types.
The changes contribute to the reusability of the component across different NextJS apps, as specified in the coding guidelines.
Minor suggestions have been made to further improve type safety and remove unnecessary optional chaining. Once these small issues are addressed, the code will be ready for merging.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (3)
26-36: LGTM! Deletion logic encapsulated in a separate function.The
deleteHandlerfunction effectively manages the deletion process, including state revalidation. This promotes cleaner code organization and aligns with the PR objectives.Consider adding error handling to manage potential failures in the deletion process:
const deleteHandler = async (id: string) => { - const { data } = await deleteCustomDelegationAdminMutation({ - variables: { - id, - }, - }) - if (data) { - revalidate() - setDelegationToDelete(null) - } + try { + const { data } = await deleteCustomDelegationAdminMutation({ + variables: { id }, + }) + if (data) { + revalidate() + setDelegationToDelete(null) + } + } catch (error) { + console.error('Failed to delete delegation:', error) + // Consider adding user feedback for the error case + } }
44-59: LGTM! AccessCard component refactored as per PR objectives.The changes to the
AccessCardcomponent, including the newvariantprop and conditional handlers, align well with the PR objectives. The refactoring improves the component's flexibility and type safety.Consider extracting the conditional logic for
onViewandonDeleteinto separate variables to improve readability:+ const canPerformActions = !!delegation.referenceId; + const viewHandler = canPerformActions ? (d) => setDelegationView(d) : undefined; + const deleteHandler = canPerformActions ? () => setDelegationToDelete(delegation) : undefined; <AccessCard key={delegation.id} delegation={delegation} isAdminView variant={direction} - onView={ - delegation.referenceId - ? (d) => setDelegationView(d) - : undefined - } - onDelete={ - delegation.referenceId - ? () => setDelegationToDelete(delegation) - : undefined - } + onView={viewHandler} + onDelete={deleteHandler} />
64-75: LGTM! DelegationDeleteModal added for improved user interaction.The addition of the
DelegationDeleteModalcomponent enhances the user interface for delegation actions, aligning with the PR objectives. The reuse of thedeleteHandlerfunction promotes code efficiency.Consider using a type assertion for
delegationToDeleteto avoid the use ofasand improve type safety:<DelegationDeleteModal id={`${direction}-delegation-delete-modal`} - delegation={delegationToDelete as AuthCustomDelegation} + delegation={delegationToDelete!} loading={loading} onClose={() => setDelegationToDelete(null)} onDelete={() => { if (delegationToDelete) { deleteHandler(delegationToDelete.id as string) } }} isVisible={!!delegationToDelete} />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1 hunks)
- libs/portals/admin/delegation-admin/src/lib/messages.ts (3 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/lib/messages.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portals/admin/delegation-admin/src/lib/messages.ts
- libs/portals/shared-modules/delegations/src/lib/messages.ts
🧰 Additional context used
📓 Path-based instructions (2)
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.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."
🪛 Biome
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
libs/portals/admin/delegation-admin/src/components/DelegationList.tsx (4)
3-6: LGTM! Imports updated to include shared components.The addition of
DelegationViewModalfrom the shared modules addresses the previous review comment and promotes reusability across different NextJS apps. This change aligns well with our coding guidelines for libs.
9-10: LGTM! Imports updated for new functionality.The explicit import of
useStateand the newDelegationDeleteModalcomponent align with the PR objectives and support effective tree-shaking. These changes contribute to better code organization and functionality.
18-24: LGTM! New state variables added with proper TypeScript typing.The introduction of
delegationToDeleteanddelegationViewstate variables supports the new view and delete functionalities mentioned in the PR objectives. The use of TypeScript for typing these variables (AuthCustomDelegation | null) aligns well with our coding guidelines for libs.
76-82: LGTM! DelegationViewModal added as suggested in the past review.The addition of the
DelegationViewModalcomponent addresses the past review comment and enhances the user interface for viewing delegation details. This change aligns well with the PR objectives and improves the overall functionality of the component.Great job on implementing the suggested improvement!
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (2)
1-165: LGTM with minor suggestions for improvementThe implementation of the
DelegationViewModalcomponent is well-structured and follows good practices. It makes appropriate use of island.is ecosystem components and hooks, and adheres to the coding guidelines for reusability and TypeScript usage. The component effectively handles both incoming and outgoing delegations, with conditional rendering based on delegation type and admin view.Consider implementing the suggested improvements to enhance maintainability, accessibility, and error handling. These changes will further improve the component's robustness and usability across different NextJS apps.
🧰 Tools
🪛 Biome
[error] 40-40: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
121-121: 🛠️ Refactor suggestionConsider importing the image asset to ensure correct path resolution in reusable components
In reusable components, referencing images via a relative path like
"./assets/images/skjaldarmerki.svg"can lead to issues when the component is used across different NextJS apps. Importing the image directly ensures correct path resolution and enhances the reusability of the component.Apply this diff to import the image asset:
+ import skjaldarmerki from '../../assets/images/skjaldarmerki.svg'; ... - imgSrc="./assets/images/skjaldarmerki.svg" + imgSrc={skjaldarmerki}Likely invalid or redundant comment.
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Outdated
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Outdated
Show resolved
Hide resolved
...ls/shared-modules/delegations/src/components/delegations/incoming/DelegationIncoming.graphql
Outdated
Show resolved
Hide resolved
...s/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.graphql
Outdated
Show resolved
Hide resolved
magnearun
added
the
deprecated:automerge
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
libs/portals/shared-modules/delegations/src/components/access/AccessList/AccessListContainer/AccessListContainer.tsx (1)
18-21: Improved type definition fordelegationpropThe changes to the
AccessListContainerPropstype enhance type safety and clarity. By explicitly defining the structure of thedelegationprop, you've improved the component's API and made it easier for consumers to understand what data is expected.This change aligns well with TypeScript best practices and may contribute to better tree-shaking by allowing unused properties to be removed more easily.
Consider extracting this type definition into a separate interface if it's used in multiple places throughout the codebase. This would further improve reusability and maintainability.
interface DelegationType { validTo?: string | null; scopes?: AuthDelegationScope[]; } type AccessListContainerProps = { delegation?: DelegationType; // ... other props }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- libs/portals/shared-modules/delegations/src/components/access/AccessList/AccessListContainer/AccessListContainer.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncoming.graphql (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx (4 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.graphql (2 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncoming.graphql
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationsIncoming.tsx
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.graphql
- libs/portals/shared-modules/delegations/src/components/delegations/outgoing/DelegationsOutgoing.tsx
🧰 Additional context used
📓 Path-based instructions (2)
libs/portals/shared-modules/delegations/src/components/access/AccessList/AccessListContainer/AccessListContainer.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.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."
🪛 Biome
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
[error] 47-47: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
libs/portals/shared-modules/delegations/src/components/access/AccessList/AccessListContainer/AccessListContainer.tsx (1)
Line range hint
1-67: Well-structured and reusable component implementationThe
AccessListContainercomponent is well-implemented and adheres to best practices for shared modules:
- It uses TypeScript effectively for prop definitions and type safety.
- The component logic is generic and doesn't contain app-specific code, making it reusable across different NextJS apps.
- The use of shared UI components from
@island.is/island-ui/corepromotes consistency.- The component structure allows for effective tree-shaking and bundling.
Great job on maintaining a clean and reusable component structure!
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.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."
🪛 Biome
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
libs/portals/shared-modules/delegations/src/components/delegations/DelegationViewModal.tsx
Show resolved
Hide resolved
saevarma
removed
the
deprecated:automerge
saevarma
added
the
deprecated:automerge
* refactor AccessCard component and make change to modal to view delegation in access control * remove unused createdBy * chore: nx format:write update dirty files * refactor * fix typo * chore: nx format:write update dirty files * fix type * small refactor * fix delete function * chore: nx format:write update dirty files * Adds delete modal * chore: nx format:write update dirty files * Update DelegationDeleteModal.tsx * adds loading to create modal * chore: nx format:write update dirty files * small fixes and use view delegation modal in admin portal * Update AccessCard.tsx * chore: nx format:write update dirty files * fix query and types * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* refactor AccessCard component and make change to modal to view delegation in access control * remove unused createdBy * chore: nx format:write update dirty files * refactor * fix typo * chore: nx format:write update dirty files * fix type * small refactor * fix delete function * chore: nx format:write update dirty files * Adds delete modal * chore: nx format:write update dirty files * Update DelegationDeleteModal.tsx * adds loading to create modal * chore: nx format:write update dirty files * small fixes and use view delegation modal in admin portal * Update AccessCard.tsx * chore: nx format:write update dirty files * fix query and types * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* feat(auth-admin): View delegation and refactor access card (#16243) * refactor AccessCard component and make change to modal to view delegation in access control * remove unused createdBy * chore: nx format:write update dirty files * refactor * fix typo * chore: nx format:write update dirty files * fix type * small refactor * fix delete function * chore: nx format:write update dirty files * Adds delete modal * chore: nx format:write update dirty files * Update DelegationDeleteModal.tsx * adds loading to create modal * chore: nx format:write update dirty files * small fixes and use view delegation modal in admin portal * Update AccessCard.tsx * chore: nx format:write update dirty files * fix query and types * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix:(auth-admin): Adds nationalId to IdentityCard for createdBy in DelegationViewModal (#16347) * adds nationalId to IdentityCard for createdBy in DelegationViewModal * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: Magnea Rún Vignisdóttir <magneavignisdottir@gmail.com> Co-authored-by: andes-it <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Refactor AccessCard, now it displays actions depending on what functions are provided, if onDelete is provided a delete button is displayed etc, moving the logic about when different actions are displayed out side of the card.
General mandate delegations should only have the View action in "From me" list í Access control
Adds createdBy field to Delegations, used to display who created the general mandate delegations
Updates the View modal to dislpay referenceId and createdby when viewing a general mandate delegation.
Why
Now making changes to different actions on the card you can be confident you are not messing up what actions show up on other lists elsewhere.
Match design and show correct actions in correct places
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization