fix(signature-collection): Fix paper signatures for candidacy #16411
Conversation
WalkthroughThe pull request introduces modifications to several services and a component related to signature collection. Key changes include an update 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: 1
🧹 Outside diff range and nitpick comments (3)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/Signees/PaperSignees.tsx (2)
76-76: Approved: Improved success condition checkThe change correctly updates the success condition to check
res.signatureCollectionUploadPaperSignature?.success. This is an improvement as it more accurately accesses the success status from the mutation response.Consider adding an explicit error message for cases where the
signatureCollectionUploadPaperSignatureproperty is undefined:if (res.signatureCollectionUploadPaperSignature?.success) { toast.success(formatMessage(m.paperSigneeSuccess)) refetchSignees() } else if (res.signatureCollectionUploadPaperSignature === undefined) { toast.error(formatMessage(m.unexpectedResponseError)) } else { toast.error(formatMessage(m.paperSigneeError)) }This would provide more specific error handling and potentially help with debugging.
Line range hint
1-208: Suggestions for code improvementsWhile the component functions correctly, there are a few areas where it could be improved:
Consider refactoring the national ID validation and name retrieval logic into a custom hook. This would improve readability and reusability.
The error handling could be more centralized. Consider creating a custom error handling hook or utility function.
Replace hardcoded error messages with localized messages using the
formatMessagefunction consistently throughout the component.The
onClearFormfunction could be memoized usinguseCallbackto optimize performance, especially if it's passed as a prop to child components.Here's an example of how you could refactor the national ID validation into a custom hook:
function useNationalIdValidation(nationalId: string, listId: string) { const [name, setName] = useState('') const [nationalIdTypo, setNationalIdTypo] = useState(false) const { data, loading } = useIdentityQuery({ variables: { input: { nationalId } }, skip: nationalId.length !== 10 || !nationalId.isValid(nationalId), onCompleted: (data) => setName(data.identity?.name || ''), }) const { canSign, loadingCanSign } = useGetCanSign( nationalId, listId, nationalId.isValid(nationalId), ) useEffect(() => { if (nationalId.length === 10) { setNationalIdTypo( !nationalId.isValid(nationalId) || (!loading && !data?.identity?.name), ) } else { setName('') setNationalIdTypo(false) } }, [nationalId, loading, data]) return { name, nationalIdTypo, canSign, loading: loading || loadingCanSign } }This would simplify the main component and make the logic more reusable.
libs/clients/signature-collection/src/lib/signature-collection.service.ts (1)
280-302: Improved error handling, but consider logging errorsThe refactoring of
candidacyUploadPaperSignaturemethod improves error handling by using a try-catch block. This change makes the method more robust against unexpected API responses or network issues.Consider adding error logging in the catch block to facilitate debugging:
} catch { + console.error('Error uploading paper signature:', error); return { success: false, } }This will help in identifying and resolving issues in the future without changing the method's behavior.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (1 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-shared.service.ts (1 hunks)
- libs/clients/signature-collection/src/lib/signature-collection.service.ts (1 hunks)
- libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/Signees/PaperSignees.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
libs/api/domains/signature-collection/src/lib/signatureCollection.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-shared.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.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/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/Signees/PaperSignees.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 (2)
libs/service-portal/signature-collection/src/screens/Parliamentary/OwnerView/ViewList/Signees/PaperSignees.tsx (1)
Line range hint
1-208: Adherence to coding guidelines forlibsdirectoryThe
PaperSigneescomponent adheres well to the coding guidelines for thelibsdirectory:
- It's designed for reusability across different NextJS apps.
- TypeScript is used for defining props and implicit type exports.
- The component uses hooks that can be shared across apps.
- There are no apparent issues with tree-shaking or bundling practices.
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (1)
179-180: Approve with suggestions: Clarify business logic and consider additional loggingThe changes to the
canSignFromPapermethod look good from a technical perspective. The addition ofReasonKey.noInvalidSignatureas an acceptable condition for paper signing broadens the scenarios where this is allowed.However, I have a few suggestions to improve clarity and maintainability:
Please add a comment explaining the business logic behind allowing
ReasonKey.noInvalidSignaturefor paper signatures. This will help future developers understand the reasoning.Consider adding logging for these new cases to track how often they occur. This could be valuable for monitoring and future optimizations.
It might be beneficial to extract this condition into a separate method for better readability, e.g.,
canSignFromPaperBasedOnReasons(signatureSignee).To ensure this change doesn't have unintended consequences, let's verify its usage:
This will help us understand if this change affects other parts of the codebase and if similar changes are needed elsewhere.
✅ Verification successful
Re-executing shell scripts with correct TypeScript type identifiers
The previous shell script failed to recognize the
typescriptfile type. Let's rerun the searches using the correct type identifiertsto ensure accurate results.
Verification Successful:
canSignFromPaperMethod Update is Consistent Across the CodebaseThe addition of
ReasonKey.noInvalidSignaturein thecanSignFromPapermethod has been successfully verified. This reason key is consistently utilized across the following files:
libs/clients/signature-collection/src/lib/types/success.dto.tslibs/application/template-api-modules/src/lib/modules/templates/signature-collection/signature-list-signing/signature-list-signing.service.tslibs/application/template-api-modules/src/lib/modules/templates/signature-collection/parliamentary-list-signing/parliamentary-list-signing.service.tslibs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.tsAdditionally, the
canSignFromPapermethod is appropriately referenced in multiple resolver and service files:
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.tslibs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.tslibs/api/domains/signature-collection/src/lib/dto/index.tslibs/api/domains/signature-collection/src/lib/signatureCollection.service.tsAll usages align with the intended logic, ensuring that the change does not introduce inconsistencies or unintended behaviors.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of ReasonKey.noInvalidSignature # and canSignFromPaper method usage echo "Searching for other occurrences of ReasonKey.noInvalidSignature:" rg "ReasonKey.noInvalidSignature" --type typescript echo "\nSearching for usages of canSignFromPaper method:" rg "canSignFromPaper" --type typescriptLength of output: 411
Script:
#!/bin/bash # Description: Check for other occurrences of ReasonKey.noInvalidSignature # and canSignFromPaper method usage with correct TypeScript type identifier echo "Searching for other occurrences of ReasonKey.noInvalidSignature:" rg "ReasonKey.noInvalidSignature" --type ts echo "\nSearching for usages of canSignFromPaper method:" rg "canSignFromPaper" --type tsLength of output: 1850
libs/clients/signature-collection/src/lib/signature-collection-shared.service.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16411 +/- ##
=======================================
Coverage 36.73% 36.73%
=======================================
Files 6809 6809
Lines 141087 141080 -7
Branches 40225 40224 -1
=======================================
Hits 51825 51825
+ Misses 89262 89255 -7
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.
|
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
|
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
* fix(service-portal): company route signature collections (#16382) * draft * chore: nx format:write update dirty files * fix: company path * tweaks * p * view list company path * chore: nx format:write update dirty files * nav tweak --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: Ásdís Erna Guðmundsdóttir <[email protected]> * fix(service-portal): sp list submitted (#16383) * fix(service-portal): sp list submitted * tweak * chore: nx format:write update dirty files --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: andes-it <[email protected]> * fix(portals-admin): adding report date (#16384) * fix(portals-admin): adding report date * chore: nx format:write update dirty files * tweak * tweak supervisors info * chore: nx format:write update dirty files * tweak * message - company card * tweak * t * t * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collection): tweaks 15.10 (#16402) * tweaka * chore: nx format:write update dirty files --------- Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collection): Tweaks for parliamentary collection (#16407) * Tweaks for parliamentary collection * revertt' * Update libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * managers should see other managers --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collection): Fix paper signatures for candidacy (#16411) Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> * fix(signature-collections): ongoing updates (#16409) * tweaks * tweak - create list * copylink * 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> --------- Co-authored-by: albinagu <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: Ásdís Erna Guðmundsdóttir <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: juni-haukur <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
...
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
Documentation