fix(signature-collection): Tweaks for parliamentary collection #16407
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16407 +/- ##
==========================================
+ Coverage 36.73% 36.76% +0.02%
==========================================
Files 6808 6807 -1
Lines 141061 140972 -89
Branches 40217 40140 -77
==========================================
+ Hits 51817 51826 +9
+ Misses 89244 89146 -98
Flags with carried forward coverage won't be shown. Click here to find out more. see 17 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportBranch report: ✅ 0 Failed, 4 Passed, 0 Skipped, 3.22s Total Time |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (1)
57-70: Consider adding error handling and refactoring for better separation of concerns.While the overall logic improvement is good, there are a few suggestions to enhance the code:
Add error handling for the API calls to
getSigneeandlist. These could potentially fail and throw exceptions.Consider refactoring this method to adhere better to the Single Responsibility Principle. It's currently fetching data and determining eligibility. These could potentially be separated.
Here's a suggested refactoring:
async getCanSignInfo( auth: User, nationalId: string, listId: string, ): Promise<SignatureCollectionSuccess> { try { const [signatureSignee, list] = await Promise.all([ this.signatureCollectionBasicService.getSignee(auth, nationalId), this.list(listId, auth) ]); const canSign = this.determineCanSign(signatureSignee); const success = canSign && list.area.id === signatureSignee.area?.id; return { success, reasons: success ? [] : signatureSignee.canSignInfo, }; } catch (error) { // Log the error and return a failure response console.error('Error in getCanSignInfo:', error); return { success: false, reasons: [ReasonKey.UnknownError] }; } } private determineCanSign(signatureSignee: any): boolean { return signatureSignee.canSign || (signatureSignee.canSignInfo?.length === 1 && (signatureSignee.canSignInfo[0] === ReasonKey.AlreadySigned || signatureSignee.canSignInfo[0] === ReasonKey.noInvalidSignature)); }This refactoring:
- Adds error handling
- Uses
Promise.allfor concurrent API calls- Extracts the
canSignlogic into a separate method- Improves readability and maintainability
Despite these suggestions, the overall logic improvement in determining signing eligibility is commendable. The new implementation provides a more comprehensive check, including area validation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (3 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (4 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (1 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (1 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-shared.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/api/domains/signature-collection/src/lib/signatureCollection.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/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/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-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."
🔇 Additional comments (3)
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (1)
50-60: LGTM! Simplified method implementation.The changes to the
signatureCollectionAdminCanSignInfomethod look good. The simplification improves readability and maintainability. The return type change fromCanSignInfotoSignatureCollectionSuccessaligns with the updated implementation.To ensure these changes don't have unintended consequences, please verify:
- The
SignatureCollectionAdminService.getCanSignInfomethod signature matches this update.- All callers of
signatureCollectionAdminCanSignInfoare updated to handle the new return type.Run the following script to check for potential issues:
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (2)
Line range hint
48-54: MethodsignatureCollectionIsOwnercorrectly returns ownership statusThe
signatureCollectionIsOwnermethod appropriately checks if the current signee is the owner and returns the result as aSignatureCollectionSuccessobject. The use of decorators@AllowManager(),@Scopes(ApiScope.signatureCollection), and@Audit()is appropriate and enhances access control and auditing.
101-104: Addition ofsigneeparameter tosignatureCollectionListmethodIncluding
@CurrentSignee() signee: SignatureCollectionSigneein the method signature ensures that the signee context is properly passed to the service layer. Updating the service call tothis.signatureCollectionService.list(input.listId, user, signee)correctly incorporates the signee parameter.
libs/clients/signature-collection/src/lib/signature-collection-shared.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts
Show resolved
Hide resolved
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (1)
167-181: Clarify variable naming to avoid confusionThe variables
signeeandsignatureSigneehave similar names, which might cause confusion. Consider renamingsignatureSigneeto improve readability.Suggested change:
- Rename
signatureSigneetotargetSigneeorpaperSigneeto differentiate it fromsignee.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (3 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (4 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (1 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (1 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-shared.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/api/domains/signature-collection/src/lib/signatureCollection.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/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/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-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."
🔇 Additional comments (7)
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (2)
Line range hint
1-310: LGTM: Well-structured resolver adhering to coding guidelinesThe
SignatureCollectionAdminResolverclass is well-structured and follows good practices:
- Proper use of TypeScript for type definitions.
- Effective use of decorators for authentication, authorization, and auditing.
- Clear separation of concerns between different query and mutation methods.
- Reusable design suitable for use across different NextJS apps.
The code structure supports effective tree-shaking and bundling. Keep up the good work in maintaining clean and reusable code!
50-60:⚠️ Potential issueVerify the impact of the return type change and update related components.
The
signatureCollectionAdminCanSignInfomethod has been updated to returnPromise<SignatureCollectionSuccess>instead ofPromise<CanSignInfo>. This change simplifies the method and aligns it with the underlying service method. However, it may impact clients expecting the oldCanSignInfostructure.
- Ensure that all clients using this method are updated to handle the new return type.
- Update any documentation or API specifications to reflect this change.
- Consider adding a comment explaining the reason for this change, especially if it's part of a larger refactoring effort.
To verify the impact of this change, run the following script:
This script will help identify any potential areas that need to be updated due to the change in return type.
✅ Verification successful
Impact of Return Type Change Verified Successfully.
The
signatureCollectionAdminCanSignInfomethod now returnsPromise<SignatureCollectionSuccess>, and based on the analysis:
Internal Usage:
- No other internal components depend on this method beyond the resolver itself.
External Clients:
- Ensure that any external clients or API consumers are updated to handle the new
SignatureCollectionSuccessreturn type.Documentation:
- Update all relevant documentation and API specifications to reflect the change in return type.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usages of CanSignInfo type and signatureCollectionAdminCanSignInfo method # Search for CanSignInfo type usage echo "Searching for CanSignInfo type usage:" rg --type typescript "CanSignInfo" # Search for signatureCollectionAdminCanSignInfo method calls echo "Searching for signatureCollectionAdminCanSignInfo method calls:" rg --type typescript "signatureCollectionAdminCanSignInfo"Length of output: 385
Script:
#!/bin/bash # Description: Check for usages of CanSignInfo type and signatureCollectionAdminCanSignInfo method # Search for CanSignInfo type usage in .ts and .tsx files echo "Searching for CanSignInfo type usage in TypeScript files:" fd --type f --extension ts --extension tsx | xargs rg "CanSignInfo" # Search for signatureCollectionAdminCanSignInfo method calls in .ts and .tsx files echo "Searching for signatureCollectionAdminCanSignInfo method calls in TypeScript files:" fd --type f --extension ts --extension tsx | xargs rg "signatureCollectionAdminCanSignInfo"Length of output: 1255
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (2)
1-5: Imports added appropriatelyThe necessary exceptions
NotFoundExceptionandUnauthorizedExceptionhave been imported from@nestjs/commonfor use in the service.
95-99: Methodlistupdated to includesigneeparameterThe method signature now includes
signee: SignatureCollectionSigneeto enforce access control by checking list ownership withcheckListAccess.libs/api/domains/signature-collection/src/lib/signatureCollection.resolver.ts (3)
48-48: Appropriate use of@AllowManager()decoratorAdding the
@AllowManager()decorator tosignatureCollectionIsOwnerallows managers to access this method, aligning with the PR objective to allow delegations and enhance access control.
101-104: Enhancement: Added@CurrentSignee()parameter tosignatureCollectionListIncluding the
@CurrentSignee() signeeparameter in thesignatureCollectionListmethod and passing it to the service ensures that the signee's context is correctly incorporated, enhancing security by preventing users from accessing lists they do not own. This change supports the PR objective to restrict access to lists.
145-151: Update: Added@CurrentSignee()parameter tosignatureCollectionCanSignFromPaperBy adding the
@CurrentSignee() signeeparameter and passing it tocanSignFromPaper, the method now accurately reflects the signee's context, ensuring proper permission checks for signing from paper. This addresses the PR objective to fix the "cansign" feature for paper signatures.
libs/clients/signature-collection/src/lib/signature-collection-shared.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts
Outdated
Show resolved
Hide resolved
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts
Show resolved
Hide resolved
…ion.service.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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."
🔇 Additional comments (3)
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts (3)
1-5: LGTM: New imports are consistent with the changes.The addition of
NotFoundExceptionandUnauthorizedExceptionimports aligns well with the new error handling in thecheckListAccessmethod.
95-101: LGTM: Enhanced access control in thelistmethod.The updated
listmethod now includes proper access control by:
- Adding the
signeeparameter to the method signature.- Utilizing the
checkListAccessmethod to verify ownership before retrieving the list.These changes effectively implement the principle of least privilege and improve the overall security of the application.
35-39: 🛠️ Refactor suggestionLGTM: New
checkListAccessmethod enhances access control.The new method effectively verifies list ownership before allowing access. Consider a minor improvement:
Remove the redundant
|| falsefrom the condition:- if (!signee.ownedLists?.some((list) => list.id === listId) || false) { + if (!signee.ownedLists?.some((list) => list.id === listId)) {This simplifies the condition without changing its behavior.
Likely invalid or redundant comment.
libs/api/domains/signature-collection/src/lib/signatureCollection.service.ts
Show resolved
Hide resolved
…b.com:island-is/island.is into fix/signature-collection-parliamentary-tweaks
kodiakhq
bot
deleted the
fix/signature-collection-parliamentary-tweaks
branch
* 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(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 <builders@andes.is> Co-authored-by: Ásdís Erna Guðmundsdóttir <disa.erna@gmail.com> * 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 <builders@andes.is> * 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 <builders@andes.is> 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 <builders@andes.is> 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 <builders@andes.is> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> --------- Co-authored-by: albinagu <47886428+albinagu@users.noreply.github.com> Co-authored-by: andes-it <builders@andes.is> Co-authored-by: Ásdís Erna Guðmundsdóttir <disa.erna@gmail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: juni-haukur <158475136+juni-haukur@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
...
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