fix(service-portal): subpoena fixes #16261
Conversation
WalkthroughThe pull request introduces a new GraphQL query method 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: 3
🧹 Outside diff range and nitpick comments (7)
libs/service-portal/documents/src/screens/Overview/Overview.graphql (2)
Line range hint
60-77: Approved: Enhanced document inbox line query with additional fields and localization supportThe
GetDocumentInboxLineV3query has been successfully updated with new fieldsconfirmationandalert, providing more detailed information about document inbox lines. The addition of the$localeparameter suggests support for localization, which is a valuable enhancement.Consider adding documentation for the new
$localeparameter to clarify its usage and supported values.
108-113: Approved: New query for document confirmation actionsThe new
DocumentConfirmActionsquery has been successfully added, aligning with the PR objectives related to subpoena fixes. The query structure is consistent with other queries in the file and follows the coding guidelines.Consider adding a brief comment above the query to explain its specific purpose and usage context. This would enhance maintainability and clarity for other developers.
libs/service-portal/core/src/components/ConfirmationModal/ConfirmationModal.tsx (1)
Line range hint
15-23: LGTM! Consider using consistent naming for callback props.The
Propsinterface is well-defined and enhances the component's type safety. It includes all necessary properties with appropriate types.For consistency, consider renaming
onClosetoonCloseModalto match the prop name used in theModalcomponent:interface Props { onSubmit: () => void onCancel: () => void - onClose: () => void + onCloseModal: () => void loading: boolean modalTitle: string modalText: string redirectPath: string }libs/api/domains/documents/src/lib/documentV2.resolver.ts (1)
92-96: Consider renaming the method to follow existing naming convention.To maintain consistency with other methods in the class (e.g.,
documentV2,documentsV2), consider renaming the method todocumentV2ConfirmActions. This would align with the GraphQL query name specified in the@Querydecorator.Here's the suggested change:
@Query(() => DocumentConfirmActions, { nullable: true, name: 'documentV2ConfirmActions', }) async documentV2ConfirmActions( // ... rest of the method )libs/api/domains/documents/src/lib/documentV2.service.ts (2)
Line range hint
67-75: LGTM! Consider using array destructuring for better readability.The changes enhance the document retrieval process by adding new data points for confirmation modal and alert box, which aligns with the PR objectives. The code adheres to TypeScript usage and improves the overall functionality.
To further improve code readability, consider using array destructuring:
const [confirmation, alert, ...actions] = document.actions?.reduce( ([conf, alrt, acts], action) => { if (action.type === 'confirmation') return [action, alrt, acts]; if (action.type === 'alert') return [conf, action, acts]; return [conf, alrt, [...acts, action]]; }, [undefined, undefined, []] );This approach eliminates the need for multiple array operations and improves performance slightly.
381-381: LGTM! Consider adding error handling for missing document ID.The change correctly addresses the PR objective of fixing the ID associated with a download file. Using the document ID instead of 'x.data' ensures that the correct identifier is used for generating the download URL, improving the accuracy of the download URL generation.
To further improve robustness, consider adding error handling for cases where the document ID might be undefined:
data: id ? `${this.downloadServiceConfig.baseUrl}/download/v1/electronic-documents/${id}` : undefined,This will prevent potential runtime errors if the document ID is missing for any reason.
libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx (1)
131-133: Reconsider the use of 'no-cache' fetch policySetting
fetchPolicy: 'no-cache'forconfirmActiondisables Apollo Client's caching mechanism, which may lead to unnecessary network requests and impact performance. Unless there's a specific need to bypass the cache, consider using the default fetch policy to leverage caching benefits.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- libs/api/domains/documents/src/lib/documentV2.resolver.ts (2 hunks)
- libs/api/domains/documents/src/lib/documentV2.service.ts (2 hunks)
- libs/api/domains/documents/src/lib/models/v2/confirmActions.input.ts (1 hunks)
- libs/api/domains/documents/src/lib/models/v2/confirmActions.model.ts (1 hunks)
- libs/service-portal/core/src/components/ConfirmationModal/ConfirmationModal.tsx (1 hunks)
- libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx (4 hunks)
- libs/service-portal/documents/src/screens/Overview/Overview.graphql (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
libs/api/domains/documents/src/lib/documentV2.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/documents/src/lib/documentV2.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/api/domains/documents/src/lib/models/v2/confirmActions.input.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/documents/src/lib/models/v2/confirmActions.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/service-portal/core/src/components/ConfirmationModal/ConfirmationModal.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/service-portal/documents/src/components/DocumentLine/DocumentLineV3.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/service-portal/documents/src/screens/Overview/Overview.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."
🔇 Additional comments (17)
libs/api/domains/documents/src/lib/models/v2/confirmActions.model.ts (5)
1-2: LGTM: Imports are appropriate and specific.The imports from '@nestjs/graphql' and 'class-validator' are correctly used for defining the GraphQL object type and adding validation. This adheres to good practices by importing only what's necessary.
4-5: LGTM: Class definition is well-structured.The
DocumentConfirmActionsclass is properly defined as a GraphQL object type using the@ObjectTypedecorator. The naming convention follows PascalCase, which is standard for TypeScript classes.
6-8: LGTM: The 'id' field is well-defined and validated.The
idfield is correctly exposed in the GraphQL schema using@Field(), validated as a string with@IsString(), and marked asreadonlyand non-nullable. This ensures type safety and prevents unintended modifications.
10-11: LGTM: The 'confirmed' field is properly defined as an optional boolean.The
confirmedfield is correctly exposed in the GraphQL schema as a nullable Boolean. It's marked asreadonlyand optional, which aligns with good practices for immutable, optional properties.
1-12: Adherence to coding guidelines forlibs/**/*confirmed.This model adheres to the coding guidelines for the
libs/**/*pattern:
- It's a reusable component that can be used across different NextJS apps.
- TypeScript is effectively used for defining props and exporting types.
- The simple nature of this model supports effective tree-shaking and bundling practices.
libs/api/domains/documents/src/lib/models/v2/confirmActions.input.ts (5)
1-2: Imports and file structure look good.The imports are concise and appropriate for the functionality implemented in the file. The file structure follows TypeScript best practices.
4-5: Class definition and decorator usage are correct.The class name
DocumentConfirmActionsInputis descriptive and follows PascalCase convention. The@InputTypedecorator is correctly used with the class name as an argument, adhering to TypeScript best practices for GraphQL input types.
6-8: Theidfield is well-implemented.The
idfield is correctly defined as a required string with appropriate decorators. The use of@Field()for GraphQL and@IsString()for validation is correct. The readonly modifier ensures immutability, which is a good practice for input types.
10-11: Theconfirmedfield is correctly implemented.The
confirmedfield is properly defined as an optional boolean. The@Field()decorator is correctly used with an explicit type and nullable option. The optional modifier?aligns with the nullable nature of the field, and the readonly modifier ensures immutability.
1-12: Code adheres tolibs/**/*coding guidelines.This implementation follows the coding guidelines for the
libsdirectory:
- The
DocumentConfirmActionsInputclass is reusable across different NextJS apps.- TypeScript is effectively used for defining props and exporting types.
- The code structure supports effective tree-shaking and bundling practices.
libs/service-portal/documents/src/screens/Overview/Overview.graphql (1)
Line range hint
22-58: Approved: Enhanced document query with additional fieldsThe
DocumentsV3query has been successfully updated with new fieldsisUrgentandconfirmation. These additions provide more detailed information about documents, aligning with the PR objectives. The changes are consistent with the existing structure and maintain reusability across different NextJS apps.libs/service-portal/core/src/components/ConfirmationModal/ConfirmationModal.tsx (2)
Line range hint
25-33: LGTM! Component signature is well-defined.The updated component signature correctly uses the
Propsinterface andReact.PropsWithChildren. This enhances type safety and allows for potential future expansion of the component to include child elements.
Line range hint
35-92: LGTM! Consider adding an aria-label to the modal for improved accessibility.The component implementation is clean, well-structured, and follows React best practices. It makes good use of the project's UI components and localization hooks.
To improve accessibility, consider adding an
aria-labelto theModalcomponent:<Modal id="confirmation-modal-errand" onCloseModal={onClose} + aria-label={formatMessage(m.confirmationModalAriaLabel)}>Don't forget to add the corresponding message to your localization file.
Let's verify if the
aria-labelproperty is supported by theModalcomponent:libs/api/domains/documents/src/lib/documentV2.resolver.ts (3)
33-34: LGTM: New imports are consistent with existing code.The new imports for
DocumentConfirmActionsInputandDocumentConfirmActionsare correctly added and follow the existing import style in the file.
92-108: LGTM: NewconfirmActionsmethod is well-implemented.The new method is properly scoped, uses correct decorators, and implements audit logging as per the PR objectives. Good job on maintaining consistency with other methods in the class.
Line range hint
1-108: Confirm adherence to coding guidelines forlibs/**/*pattern.The code in this file adheres to the specified guidelines:
- Reusability: The
DocumentResolverV2class can be reused across different NextJS apps that require document-related functionality.- TypeScript usage: The code effectively uses TypeScript for defining types, interfaces, and method signatures.
- Tree-shaking and bundling: The use of named exports allows for effective tree-shaking in the bundling process.
libs/api/domains/documents/src/lib/documentV2.service.ts (1)
Line range hint
93-94: LGTM! Consistent with earlier changes.The addition of 'confirmation' and 'alert' properties to the returned object is consistent with the earlier modifications in the method. This improves the document structure by including confirmation and alert data, which aligns with the PR objectives of enhancing audit logs.
libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx
Show resolved
Hide resolved
libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16261 +/- ##
=======================================
Coverage 36.92% 36.93%
=======================================
Files 6781 6781
Lines 140030 139997 -33
Branches 39815 39802 -13
=======================================
- Hits 51705 51701 -4
+ Misses 88325 88296 -29
Flags with carried forward coverage won't be shown. Click here to find out more. see 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 3 Total Test Services: 0 Failed, 2 Passed Test Services
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx (3)
135-136: Create a task for query refactoringThe TODO comment correctly identifies the need to refactor queries and move them to a shared file. This aligns with our coding guidelines for reusability across different NextJS apps.
Consider creating a specific task or issue to track this refactoring work, ensuring it doesn't get overlooked in future development cycles.
378-388: LGTM with minor suggestion: Modal action handlersThe changes to the ConfirmationModal handlers (
onSubmit,onCancel, andonClose) correctly implement the confirmation action calling, which aligns with the PR objective of subpoena-related fixes. Each handler appropriately callsconfirmActionCallerwith the correct boolean value.Consider refactoring to reduce the duplication of
setModalVisible(false)inonSubmitandonCancel. You could extract this into a separate function that also callsconfirmActionCaller, like this:const handleModalAction = (confirmed: boolean | null) => { setModalVisible(false); confirmActionCaller(confirmed); }; // Then in the handlers: onSubmit={() => { handleModalAction(true); getDocument(); }} onCancel={() => handleModalAction(false)} onClose={() => handleModalAction(null)}This refactoring would make the code more DRY and easier to maintain.
Line range hint
1-402: Overall assessment: Well-implemented changes with minor improvements neededThe changes in this file effectively implement the PR objectives of adding more detailed audit logs and fixing subpoena-related issues. The new functionality for handling confirmation actions and urgent documents is well-implemented and improves the user experience.
The code adheres to TypeScript usage for defining props and exporting types, which is in line with our coding guidelines. There are no apparent issues with tree-shaking or bundling practices.
To fully align with our coding guidelines for reusability across different NextJS apps, please prioritize the TODO task of moving queries to a shared file. This will enhance the modularity and reusability of the component.
Overall, great job on implementing these important features while maintaining code quality!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/api/domains/documents/src/lib/documentV2.resolver.ts (2 hunks)
- libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx (5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/api/domains/documents/src/lib/documentV2.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/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx (1)
Pattern
libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
🔇 Additional comments (5)
libs/api/domains/documents/src/lib/documentV2.resolver.ts (2)
33-34: LGTM: New imports are correctly added.The new imports for
DocumentConfirmActionsInputandDocumentConfirmActionsare appropriately added and follow TypeScript conventions. These imports are related to the newly addedconfirmActionsmethod.
Line range hint
1-110: LGTM: Code complies with coding guidelines forlibs/**/*files.The code in this file adheres to the specified coding guidelines:
- It contributes to the reusability of components across different NextJS apps by implementing a GraphQL resolver.
- It uses TypeScript for defining props and exporting types, as seen in the method signatures and import statements.
- There are no apparent issues with tree-shaking or bundling practices.
The implementation follows good practices for a resolver in a TypeScript environment.
libs/service-portal/documents/src/components/DocumentLine/DocumentLineV3.tsx (3)
131-133: LGTM: Addition of confirmation action queryThe addition of
useDocumentConfirmActionsLazyQuerywith ano-cachefetch policy aligns well with the PR objective of enhancing audit logs. This ensures that fresh data is always fetched for confirmation actions, which is crucial for maintaining accurate audit trails.
Line range hint
188-224: LGTM: Enhanced document metadata handlingThe changes to
getDocumentMetadatafunction effectively implement the handling of confirmation actions for urgent documents. This improvement aligns well with the PR objective of implementing subpoena-related fixes. The logic for setting modal data based on the confirmation action is sound and enhances the user experience.
251-255: LGTM: Addition of confirmActionCaller functionThe new
confirmActionCallerfunction provides a centralized way to handle confirmation actions, which is excellent for maintainability and supports the PR objective of adding more detailed audit logs. This implementation ensures consistent handling of confirmation actions across different scenarios.
Subpoena fixes
What
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation