fix(signature-collection): Fix admin paper signature upload #16500
Conversation
WalkthroughThe pull request introduces a new mutation method 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
|
![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/clients/signature-collection/src/lib/types/bulkUpload.dto.ts (1)
53-76: LGTM: Well-implemented function with good type safety and reusability.The
getReasonKeyForPaperSignatureUploadfunction is well-structured, type-safe, and potentially reusable across different NextJS apps dealing with signature collection. It effectively maps the bulk response to reason keys.Minor suggestion for improved type safety:
Consider using a type assertion for
listMappingto ensure all keys are accounted for:const listMapping: { [K in keyof MedmaeliBulkResponseDTO]?: ReasonKey } = { // ... (rest of the mapping) } as const;This change would make the type system catch any missing or extra keys in the mapping.
libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1)
72-77: Improved error handling, consider destructuring for clarity.The changes enhance error handling by providing specific feedback based on the mutation's success status. This improvement aligns well with best practices for user experience.
Consider destructuring the response for improved readability:
onCompleted: ({ signatureCollectionAdminUploadPaperSignature }) => { if (signatureCollectionAdminUploadPaperSignature?.success) { toast.success(formatMessage(m.paperSigneeSuccess)) } else { toast.error(formatMessage(m.paperSigneeError)) } // ... rest of the code },This change would make the code slightly more concise and easier to read.
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (2)
70-77: LGTM: Improved logic for determining signing eligibility.The changes enhance the clarity and specificity of the
getCanSignInfomethod. The introduction of theinAreavariable and its incorporation into thesuccessandreasonsproperties provide more accurate feedback.Consider adding a comment explaining the logic behind the
canSigncondition, as it's quite complex:// A user can sign if they meet the general criteria or if their only restriction // is a previous signature (which doesn't prevent paper signatures) const canSign = signatureSignee.canSign || (signatureSignee.canSignInfo?.length === 1 && (signatureSignee.canSignInfo[0] === ReasonKey.AlreadySigned || signatureSignee.canSignInfo[0] === ReasonKey.noInvalidSignature))
255-263: LGTM: New method for uploading paper signatures.The
uploadPaperSignaturemethod is a well-structured addition to the service, consistent with the existing patterns and purpose of the class.For consistency with other methods in the class, consider reordering the parameters to have
userfirst:async uploadPaperSignature( user: User, input: SignatureCollectionUploadPaperSignatureInput ): Promise<SignatureCollectionSuccess> { return await this.signatureCollectionClientService.uploadPaperSignature( user, input ) }libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (1)
292-299: LGTM: New mutation method is well-implemented.The
signatureCollectionAdminUploadPaperSignaturemethod is correctly implemented and follows the existing patterns in the class. It's properly decorated and uses dependency injection as expected.For consistency with other methods in this class, consider adding the
@Scopes()decorator to specify which admin scopes are allowed to use this mutation.libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (1)
407-448: LGTM: New methoduploadPaperSignatureis well-implemented.The method follows the established patterns in the class, uses proper authentication, and handles errors appropriately. It adheres to TypeScript usage for defining props and returns a well-structured
Successobject.Consider enhancing the error handling by logging the error or providing more specific error information:
- } catch { + } catch (error) { + console.error('Error uploading paper signature:', error); return { success: false, - reasons: [ReasonKey.DeniedByService], + reasons: [ReasonKey.DeniedByService, error.message], } }This change would provide more context for debugging while maintaining the existing error response structure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (2 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (3 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (2 hunks)
- libs/clients/signature-collection/src/lib/types/bulkUpload.dto.ts (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (1 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/uploadPaperSignee.graphql (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.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/signature-collection/src/lib/signatureCollectionAdmin.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/clients/signature-collection/src/lib/signature-collection-admin.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/clients/signature-collection/src/lib/types/bulkUpload.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/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.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/signature-collection/src/screens-parliamentary/List/paperSignees/uploadPaperSignee.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 (8)
libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/uploadPaperSignee.graphql (1)
4-4: Approved: Mutation name change improves consistencyThe change from
signatureCollectionUploadPaperSignaturetosignatureCollectionAdminUploadPaperSignaturealigns with the new method introduced in theSignatureCollectionAdminResolverclass. This improves consistency across the codebase and clearly indicates that this mutation is for admin use.The mutation structure remains reusable and compatible with TypeScript type generation, adhering to the coding guidelines for files in the
libsdirectory.libs/clients/signature-collection/src/lib/types/bulkUpload.dto.ts (1)
2-2: LGTM: Import statement is correct and necessary.The import of
ReasonKeyis appropriate for the new function and follows TypeScript best practices.libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (2)
28-31: LGTM: Import changes are appropriate.The addition of
SignatureCollectionUploadPaperSignatureInputto the existing import statement is consistent with the newuploadPaperSignaturemethod and follows the established import structure.
Line range hint
1-263: Overall: Well-implemented changes for paper signature upload functionality.The modifications to this file effectively introduce the capability to upload paper signatures while maintaining consistency with the existing codebase. The changes adhere to the coding guidelines for
libs/**/*files:
- The new functionality is reusable across different NextJS apps.
- TypeScript is used appropriately for defining props and exporting types.
- The changes maintain the existing structure, promoting effective tree-shaking and bundling practices.
The enhancements to the
getCanSignInfomethod and the addition of theuploadPaperSignaturemethod improve the overall functionality of the signature collection admin service.libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (2)
39-39: LGTM: Import statement is correctly added.The new import statement for
SignatureCollectionUploadPaperSignatureInputis properly placed and follows TypeScript naming conventions. It's necessary for the new mutation method.
39-39: Overall, the changes look good and add new functionality.The addition of the
SignatureCollectionUploadPaperSignatureInputimport and thesignatureCollectionAdminUploadPaperSignaturemutation method effectively implements the new functionality for uploading paper signatures. The changes are minimal, focused, and consistent with the existing code patterns and structure.These modifications enhance the signature collection process by allowing admin users to upload paper signatures, which aligns with the PR objectives of fixing the admin paper signature upload.
Also applies to: 292-299
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (2)
19-23: LGTM: Import changes are appropriate.The addition of
getReasonKeyForPaperSignatureUploadfrom./types/bulkUpload.dtois relevant to the new functionality and follows good practices by importing only what's needed.
Line range hint
1-448: Overall, the changes are well-implemented and adhere to coding guidelines.The additions to this file enhance the functionality of the
SignatureCollectionAdminClientServicewhile maintaining consistency with the existing code structure. The newuploadPaperSignaturemethod is implemented in a way that ensures reusability across different NextJS apps. The use of TypeScript for defining props and the overall structure of the service class contribute to effective tree-shaking and bundling practices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (1)
431-472: LGTM: TheuploadPaperSignaturemethod is well-implemented.The new method follows the existing coding style, uses proper error handling, and is consistent with the class's purpose. Great job!
A minor suggestion for improved type safety:
Consider using a type alias for the input object to make the method signature more readable and maintainable:
type UploadPaperSignatureInput = { listId: string; nationalId: string; pageNumber: number; }; async uploadPaperSignature( auth: Auth, { listId, nationalId, pageNumber }: UploadPaperSignatureInput ): Promise<Success> { // ... existing implementation }This change would make it easier to reuse the input type if needed elsewhere and improve code readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx (2 hunks)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/uploadPaperSignee.graphql (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/index.tsx
- libs/portals/admin/signature-collection/src/screens-parliamentary/List/paperSignees/uploadPaperSignee.graphql
🧰 Additional context used
📓 Path-based instructions (1)
libs/clients/signature-collection/src/lib/signature-collection-admin.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."
🔇 Additional comments (2)
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (2)
19-23: LGTM: New imports are consistent with the added functionality.The new imports from './types/bulkUpload.dto' are appropriate for the new
uploadPaperSignaturemethod and follow the existing import style in the file.
Line range hint
1-472: Overall, the changes in this file are well-implemented and enhance the functionality.The new
uploadPaperSignaturemethod is a valuable addition to theSignatureCollectionAdminClientServiceclass. It follows the existing coding patterns, uses proper error handling, and is consistent with the file's purpose. The changes adhere to the coding guidelines for reusability and TypeScript usage.To ensure the new method is properly integrated, please run the following verification script:
This script will help identify any places where the new method might need to be integrated or where it's already being used.
✅ Verification successful
uploadPaperSignaturemethod is properly integrated and utilized across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the new uploadPaperSignature method # Test: Check if the new method is exported and used in other parts of the codebase rg -p "uploadPaperSignature" --type tsLength of output: 1285
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16500 +/- ##
==========================================
- Coverage 36.77% 36.77% -0.01%
==========================================
Files 6847 6847
Lines 141850 141871 +21
Branches 40417 40422 +5
==========================================
+ Hits 52165 52167 +2
- Misses 89685 89704 +19
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
...
Attach a link to issue if relevant
What
Specify what you're trying to achieve
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
New Features
Bug Fixes