fix(auth-admin): Tweak modal to handle general mandate delegations #16174
Conversation
WalkthroughThe changes in this pull request involve modifications to 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/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncomingModal/DelegationIncomingModal.tsx (5)
13-13: Consider reordering imports for consistency.The new import for
formatfromdate-fnsis correctly added. However, to maintain a consistent and organized import structure, consider moving this import statement to group it with other third-party library imports at the top of the file.
Line range hint
30-37: Approve the safety check and suggest optional chaining.The addition of the safety check
delegation && delegation.domainis a good improvement to prevent potential errors. To further enhance this, consider using optional chaining as suggested by the static analysis tool.You can refactor the condition to use optional chaining like this:
if (delegation?.domain) { getAuthScopeTree({ variables: { input: { domain: delegation.domain.name, lang, }, }, }) }This change maintains the safety check while making the code more concise.
🧰 Tools
🪛 Biome
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
97-103: Approve the addition of GeneralMandate IdentityCard.The new IdentityCard component for general mandate delegations is a good addition that aligns with the PR objectives. The conditional rendering and use of localized messages are implemented correctly.
For consistency with other image sources in the component, consider using a more specific path for the image source:
imgSrc="/assets/images/skjaldarmerki.svg"This ensures that the image path is treated as an absolute path from the public directory, which is a common practice in Next.js applications.
105-110: Approve the addition of validity date IdentityCard.The new IdentityCard component for displaying the validity date of general mandate delegations is a valuable addition. The conditional rendering and date formatting are implemented correctly.
To improve the date formatting and make it more flexible for different locales, consider using the
formatDatefunction from the@island.is/localizationpackage if available. If not, you can enhance the current implementation by including the locale:import { format } from 'date-fns' import { enUS, is } from 'date-fns/locale' // Inside the component const dateLocale = lang === 'is' ? is : enUS; title={format(new Date(delegation.validTo), 'dd.MM.yyyy', { locale: dateLocale })}This change ensures that the date format respects the user's language setting.
112-119: Approve the conditional rendering of AccessListContainer.The addition of the condition to exclude the AccessListContainer for general mandate delegations is appropriate and aligns with the PR objectives. The use of optional chaining is a good practice for safe property access.
To improve code organization and readability, consider extracting the condition into a separate variable:
const isGeneralMandate = delegation?.type === 'GeneralMandate'; // Then use it in your JSX {!isGeneralMandate && ( <AccessListContainer delegation={delegation} scopes={delegation?.scopes} scopeTree={authScopeTree} loading={scopeTreeLoading} /> )}This change makes the code more self-documenting and easier to maintain.
libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1)
169-172: LGTM! Consider enhancing readability with a constant.The changes successfully implement the handling of general mandate delegations as per the PR objectives. The code adheres to TypeScript usage and maintains reusability across NextJS apps.
To improve readability, consider extracting the condition for custom and general mandate types into a constant:
const isCustomOrGeneralMandate = delegation.type === AuthDelegationType.Custom || delegation.type === AuthDelegationType.GeneralMandate; const showActions = canModify && (isOutgoing || isCustomOrGeneralMandate);This refactoring would make the condition more self-explanatory and easier to maintain in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/portals/shared-modules/delegations/src/components/access/AccessCard.tsx (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncomingModal/DelegationIncomingModal.graphql (1 hunks)
- libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncomingModal/DelegationIncomingModal.tsx (3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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/incoming/DelegationIncomingModal/DelegationIncomingModal.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/DelegationIncomingModal/DelegationIncomingModal.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/incoming/DelegationIncomingModal/DelegationIncomingModal.tsx
[error] 30-30: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncomingModal/DelegationIncomingModal.graphql (1)
24-24: LGTM! The addition ofvalidTofield enhances the delegation information.The inclusion of the
validTofield in theAuthDelegationsIncomingquery aligns well with the PR objective of tweaking the modal to handle general mandate delegations. This change provides crucial information about the validity period of the delegation, which is essential for proper handling and display in the UI.The modification adheres to the coding guidelines:
- Reusability: The query is defined in a shared module, promoting reuse across different NextJS apps.
- Tree-shaking: The use of fragments (
AuthCustomDelegation) allows for efficient tree-shaking and bundling.libs/portals/shared-modules/delegations/src/components/delegations/incoming/DelegationIncomingModal/DelegationIncomingModal.tsx (1)
Line range hint
1-121: Overall implementation reviewThe changes made to the
DelegationIncomingModalcomponent effectively address the PR objective of handling general mandate delegations. The implementation demonstrates good practices in terms of type safety, conditional rendering, and localization.Key points:
- The addition of safety checks and optional chaining improves the robustness of the code.
- New
IdentityCardcomponents for general mandates and validity dates enhance the user interface.- The conditional rendering of the
AccessListContainerappropriately handles different delegation types.The suggested minor improvements, if implemented, will further enhance code quality and maintainability. Great job on adapting the modal to accommodate general mandate delegations while maintaining code quality and adhering to the project's coding guidelines.
Datadog ReportAll test runs ✅ 68 Total Test Services: 0 Failed, 66 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (3) |
GunnlaugurG
added
the
deprecated:automerge
kodiakhq
bot
deleted the
fix/general-mandate-delegation-incomming-modal
branch
…16174) * tweak modal to handle general mandate delegations * 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>
What
Tweak incomnig modal to handle General mandate delegations
Why
To match design
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
validTofield to the authDelegations query for enhanced delegation information.Bug Fixes