feat(signature-collection): New endpoint to update signature page number #16163
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughA new feature has been implemented to allow the updating of the page number for signatures in the signature collection. This includes the introduction of a new input type class Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip You can make CodeRabbit's review more strict using the `assertive` profile, if that's what you prefer.Change the 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 (6)
libs/api/domains/signature-collection/src/lib/dto/signatureUpdate.input.ts (1)
7-9: Field declaration is correct, with a minor suggestion for improvement.The
pageNumberfield is properly declared with appropriate decorators for GraphQL schema integration (@Field()) and validation (@IsNumber()). The use of TypeScript's non-null assertion operator ensures type safety. This implementation aligns well with the PR objective of updating the signature page number.For improved clarity and to avoid potential runtime errors, consider using the
@IsInt()decorator instead of@IsNumber()if the page number is expected to be an integer. Additionally, you might want to add a@Min(1)decorator to ensure the page number is positive. Here's a suggested improvement:import { IsInt, Min } from 'class-validator' // ... @Field() @IsInt() @Min(1) pageNumber!: numberThis change would provide more precise validation for the
pageNumberfield.libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (1)
181-190: LGTM: New method added correctly. Consider adding error handling.The
updateSignaturePageNumbermethod is well-implemented and consistent with the existing code style and patterns. It correctly uses the new input type and delegates to the client service.Consider adding error handling to catch and properly handle any exceptions that might be thrown by the client service. For example:
async updateSignaturePageNumber( user: User, input: SignatureCollectionSignatureUpdateInput, ): Promise<SignatureCollectionSuccess> { try { return await this.signatureCollectionClientService.updateSignaturePageNumber( user, input.signatureId, input.pageNumber, ); } catch (error) { // Log the error console.error('Error updating signature page number:', error); // Rethrow or handle as appropriate for your use case throw new Error('Failed to update signature page number'); } }This would improve the robustness of the method and provide better debugging information in case of failures.
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (1)
297-314: LGTM with a suggestion for improved error handlingThe new
updateSignaturePageNumbermethod is well-implemented and follows good practices for TypeScript usage and error handling. It adheres to the coding guidelines for files in thelibs/**/*path pattern.To further improve the method, consider enhancing the error case to provide more information about the failure reason. This could help with debugging and provide more context to the caller.
Consider modifying the catch block to include more information:
} catch (error) { console.error('Failed to update signature page number:', error); return { success: false, reason: 'Failed to update signature page number' }; }libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (1)
246-256: LGTM: New mutation method is well-structured and consistent with existing patterns.The new
signatureCollectionAdminUpdatePaperSignaturePageNumbermethod is correctly implemented as a GraphQL mutation. It follows the existing patterns in the class, uses TypeScript for type definitions, and maintains a clear separation of concerns by delegating the actual work to the service layer.For consistency with other methods in this class, consider adding the
@Scopes()decorator to explicitly define the required scopes for this mutation.@Mutation(() => SignatureCollectionSuccess) @Audit() @Scopes(AdminPortalScope.signatureCollectionProcess) // Add this line async signatureCollectionAdminUpdatePaperSignaturePageNumber( // ... rest of the method )libs/clients/signature-collection/src/clientConfig.json (2)
77-114: LGTM! Consider adding English translations for descriptions.The new admin endpoint for updating the page number is well-structured and follows RESTful principles. The use of PATCH is appropriate for partial updates.
Consider adding English translations alongside the Icelandic descriptions to improve accessibility for non-Icelandic speaking developers who might work on this API in the future.
"patch": { "tags": ["Admin"], - "summary": "Uppfærir blaðsíðunúmer skriflegs meðmælis", - "description": "Aðeins m0gulegt fyrir skrifleg meðmæli", + "summary": "Uppfærir blaðsíðunúmer skriflegs meðmælis (Updates the page number of a written recommendation)", + "description": "Aðeins m0gulegt fyrir skrifleg meðmæli (Only possible for written recommendations)", ... }
1763-1800: LGTM! Maintain consistency with admin endpoint improvements.The new user endpoint for updating the page number is well-structured and consistent with the admin endpoint. This consistency is good for maintainability and understanding of the API.
For consistency with the suggested improvement for the admin endpoint, consider adding English translations alongside the Icelandic descriptions:
"patch": { "tags": ["Medmaeli"], - "summary": "Uppfærir blaðsíðunúmer skriflegs meðmælis", - "description": "Aðeins m0gulegt fyrir skrifleg meðmæli", + "summary": "Uppfærir blaðsíðunúmer skriflegs meðmælis (Updates the page number of a written recommendation)", + "description": "Aðeins m0gulegt fyrir skrifleg meðmæli (Only possible for written recommendations)", ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- libs/api/domains/signature-collection/src/lib/dto/signatureUpdate.input.ts (1 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (2 hunks)
- libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (2 hunks)
- libs/clients/signature-collection/src/clientConfig.json (3 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
libs/api/domains/signature-collection/src/lib/dto/signatureUpdate.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/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/clientConfig.json (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."
🔇 Additional comments (8)
libs/api/domains/signature-collection/src/lib/dto/signatureUpdate.input.ts (3)
1-3: Imports look good and follow guidelines.The imports are correctly specified and necessary for the functionality of the class. They adhere to the TypeScript usage guidelines mentioned in the coding guidelines for the
libsdirectory.
5-6: Class definition adheres to guidelines and promotes reusability.The
SignatureCollectionSignatureUpdateInputclass is well-defined using the@InputType()decorator from NestJS. ExtendingSignatureCollectionSignatureIdInputpromotes code reusability, which aligns with the coding guidelines for thelibsdirectory. The naming convention is clear and descriptive, enhancing maintainability.
1-10: Overall, excellent implementation that meets PR objectives and follows guidelines.This new input type class for updating signature page numbers is well-implemented. It adheres to NestJS and TypeScript conventions, promotes code reusability, and aligns perfectly with the PR objectives. The code structure and use of decorators ensure proper GraphQL schema integration and basic validation.
The implementation follows the coding guidelines for the
libsdirectory, particularly in terms of reusability and TypeScript usage. It's a valuable addition to the signature collection feature, allowing for the updating of signature page numbers as intended.Consider the minor suggestion for improving field validation in the previous comment to further enhance the robustness of this input type.
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (2)
24-24: LGTM: New import added correctly.The new import for
SignatureCollectionSignatureUpdateInputis correctly added and follows the existing conventions in the file.
Line range hint
1-190: Overall assessment: Changes are well-implemented and consistent with best practices.The new code adheres to the coding guidelines for
libs/**/*:
- It maintains reusability across different NextJS apps.
- TypeScript is used effectively for type definitions.
- The changes don't negatively impact tree-shaking or bundling practices.
The new method and import are consistent with the existing code style and patterns in the file. The only suggestion for improvement is to consider adding error handling to the new method.
libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (2)
37-37: LGTM: Import statement is well-structured and follows TypeScript conventions.The import statement is correctly using TypeScript for defining types and follows proper naming conventions. It's imported from a local file, which is good for modularity.
37-37: Compliance with coding guidelines forlibs/**/*files confirmed.The changes in this file adhere to the specified coding guidelines:
- The new method enhances the reusability of the resolver across different NextJS apps.
- TypeScript is correctly used for defining types in both the import statement and the new method.
- The local import and class-based structure support effective tree-shaking and bundling.
Also applies to: 246-256
libs/clients/signature-collection/src/clientConfig.json (1)
2281-2286:⚠️ Potential issueBreaking change detected. Clarification needed on new 'nr' field.
The
SvaediDTOschema has been updated to include a new required propertynrof type string. This is a breaking change that might affect existing clients.
- As this is a breaking change, ensure that all clients are updated to handle the new required field.
- The purpose of the
nrfield is not immediately clear. Consider adding a description to clarify its meaning and usage.- If
nris intended to be a numeric identifier, consider using type "integer" instead of "string", unless there's a specific reason for using a string (e.g., leading zeros or non-numeric characters).Please provide more context on the purpose and expected format of the
nrfield.To check for potential impacts of this change, let's search for usages of the
SvaediDTOschema:✅ Verification successful
Breaking change detected within signature-collection library.
The
SvaediDTOschema has been updated to include a new required propertynrof type string. This change affects thelibs/clients/signature-collection/src/clientConfig.jsonandlibs/clients/signature-collection/src/lib/types/area.dto.ts.
- Ensure all internal components using
SvaediDTOare updated to handle the new requirednrfield.- Consider adding a description for the
nrfield in the schema to clarify its purpose and usage.- Verify that the change does not inadvertently affect other parts of the application relying on
signature-collection.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for files referencing SvaediDTO echo "Files referencing SvaediDTO:" rg --type typescript --type javascript "SvaediDTO" # Search for API calls that might be affected echo "\nPotential API calls affected:" rg --type typescript --type javascript "api.*Svaedi"Length of output: 315
Script:
#!/bin/bash # Search for files referencing SvaediDTO without specifying file types echo "Files referencing SvaediDTO:" rg "SvaediDTO" # Search for API calls that might be affected without specifying file types echo "\nPotential API calls affected:" rg "api.*Svaedi"Length of output: 1085
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts
Show resolved
Hide resolved
juni-haukur
added
the
deprecated:automerge
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16163 +/- ##
==========================================
- Coverage 36.69% 36.67% -0.02%
==========================================
Files 6776 6769 -7
Lines 139581 139286 -295
Branches 39680 39573 -107
==========================================
- Hits 51222 51086 -136
+ Misses 88359 88200 -159
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 52 files 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
🔻 Code Coverage Decreases vs Default Branch (4) |
…ber (#16163) * New endpoint to update signature page number * Fix tests after client update --------- Co-authored-by: albinagu <[email protected]> Co-authored-by: kksteini <[email protected]>
...
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