feat(signature-collection): Admin remove signature collection list #16217
Conversation
WalkthroughThe changes introduce a new mutation 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: 0
🧹 Outside diff range and nitpick comments (4)
libs/clients/signature-collection/src/lib/apis.ts (1)
19-19: LGTM: New AdminApi class is well-structured.The new
AdminApiclass follows the established pattern of other API classes in the file. It correctly extends the importedAdminClientand is properly exported.For consistency with other class declarations, consider adding a blank line before and after this class declaration:
+ export class AdminApi extends AdminClient {} +libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.service.ts (1)
181-186: LGTM with suggestions for improvementThe implementation of the
removeListmethod looks good and adheres to the coding guidelines. It's properly typed and follows the existing pattern in the service.However, consider the following improvements:
- Add error handling to manage potential failures when calling
signatureCollectionClientService.removeList.- Include documentation (JSDoc) for the method to improve maintainability.
Here's an example of how you could improve the method:
/** * Removes a signature collection list. * @param listId - The ID of the list to remove. * @param user - The user performing the action. * @returns A promise that resolves to SignatureCollectionSuccess. * @throws An error if the list removal fails. */ async removeList( listId: string, user: User, ): Promise<SignatureCollectionSuccess> { try { return await this.signatureCollectionClientService.removeList(listId, user); } catch (error) { // Log the error or handle it as appropriate for your application throw new Error(`Failed to remove list: ${error.message}`); } }libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (1)
300-311: LGTM: NewremoveListmethod is well-implemented.The
removeListmethod is a good addition to the service. It follows TypeScript best practices, uses proper error handling, and maintains consistency with other methods in the class.Suggestion for improvement:
Consider adding error logging to capture more details about failures. This could help with debugging and monitoring in production.Example:
} catch (error) { console.error('Failed to remove list:', error); return { success: false, reasons: [ReasonKey.DeniedByService] } }libs/api/domains/signature-collection/src/lib/signatureCollectionAdmin.resolver.ts (1)
198-205: Consider adding explicit authorization check and improve error handling.The new mutation
signatureCollectionAdminRemoveListlooks good and follows the existing patterns in the class. However, consider the following improvements:
Authorization: Although the class is decorated with
@Scopes(AdminPortalScope.signatureCollectionProcess), it might be beneficial to add an explicit authorization check within the method, especially if removing a list requires higher privileges.Error Handling: While error handling might be implemented in the
signatureCollectionService.removeListmethod, consider adding a try-catch block here to handle any unexpected errors and provide more informative error messages to the client.Example implementation:
@Mutation(() => SignatureCollectionSuccess) @Audit() async signatureCollectionAdminRemoveList( @CurrentUser() user: User, @Args('input') { listId }: SignatureCollectionListIdInput, ): Promise<SignatureCollectionSuccess> { if (!user.scope.includes(AdminPortalScope.signatureCollectionManage)) { throw new UnauthorizedException('Insufficient permissions to remove list'); } try { return await this.signatureCollectionService.removeList(listId, user); } catch (error) { // Log the error console.error('Error removing signature collection list:', error); throw new Error('Failed to remove signature collection list'); } }This implementation adds an explicit authorization check and basic error handling, improving the robustness of the mutation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- 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/clientConfig.json (8 hunks)
- libs/clients/signature-collection/src/lib/apis.ts (3 hunks)
- libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (4 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
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/apis.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."
🔇 Additional comments (12)
libs/clients/signature-collection/src/lib/apis.ts (3)
8-8: LGTM: Import statement is consistent and well-structured.The addition of
AdminApi as AdminClientto the import statement is consistent with the existing code style and helps avoid naming conflicts. This change supports the introduction of the newAdminApiclass.
79-79: LGTM: AdminApi correctly added to exportedApis array.The addition of
AdminApito theexportedApisarray is correct and consistent with the existing structure. This ensures that the newAdminApiwill be properly provided and instantiated with the same configuration as other admin API classes.
Line range hint
1-124: Summary: New AdminApi successfully integrated into the signature collection client library.The changes in this file effectively introduce the new
AdminApiclass and integrate it into the existing API structure. The modifications are consistent with the established patterns in the codebase and adhere to the coding guidelines for reusability and TypeScript usage.Key points:
- The new
AdminApiclass is properly imported, declared, and exported.- The
exportedApisarray is updated to include theAdminApi, ensuring it's provided and instantiated correctly.- The changes maintain the existing configuration and injection setup, promoting consistency across admin API classes.
These changes align well with the PR objectives of enhancing administrative capabilities for the signature collection feature.
libs/clients/signature-collection/src/lib/signature-collection-admin.service.ts (3)
23-23: LGTM: Import and type definition updates are consistent with new functionality.The addition of
AdminApito the imports and theApitype definition aligns well with the newremoveListmethod. These changes maintain good TypeScript practices and support effective tree-shaking.Also applies to: 32-32
42-42: LGTM: Constructor update enhances service capabilities.The addition of
private adminApi: AdminApito the constructor is a good practice. It follows dependency injection principles, which enhances testability and maintainability of the service.
Line range hint
1-338: Overall assessment: Changes successfully implement new feature and maintain code quality.The modifications to this file effectively implement the new feature for removing signature collection lists. The changes adhere to the coding guidelines, maintaining reusability, proper TypeScript usage, and supporting effective tree-shaking. The new functionality is well-integrated with the existing codebase, following established patterns and practices.
libs/clients/signature-collection/src/clientConfig.json (6)
180-215: Well-structured new admin endpoint for toggling list processing status.The new PATCH endpoint
/Admin/Medmaelalisti/{ID}/ToggleListis a good addition to the API. It provides a clear way for admins to toggle the processing status of a list. The optionalshouldToggleparameter adds flexibility, and the response schemas are appropriate.
Line range hint
217-266: Well-defined new admin endpoint for locking recommendation lists.The new PATCH endpoint
/Admin/Medmaelalisti/{ID}/LockListis a valuable addition to the API. It provides a clear mechanism for admins to lock a recommendation list. The optionalshouldLockparameter adds flexibility, and the response schemas are appropriate.
371-408: Useful new admin endpoint for retrieving area-specific information.The new GET endpoint
/Admin/Medmaelasofnun/{ID}/SvaediInfo/{svaediID}is a valuable addition to the API. It allows admins to retrieve detailed information about a specific area within a recommendation collection. The endpoint is well-documented, and the response schema (SvaediExtendedDTO) is appropriate for the purpose.
2169-2169: Improved DTOs with additional properties for recommendation lists.The updates to MedmaelalistiBaseDTO and MedmaelalistiDTO, along with the introduction of MedmaelalistiExtendedDTO, enhance the API's capability to provide more detailed information about recommendation lists. The consistent addition of the
urvinnslaLokidproperty and the new extended DTO with additional fields for electronic and written recommendations improve the overall functionality of the API.Also applies to: 2186-2186, 2196-2219
2393-2394: Enhanced MedmaeliDTO with locking information.The addition of the
lockedproperty to the MedmaeliDTO schema is a good improvement. This change aligns with the new locking functionality introduced by the LockList endpoint and enhances the API's capability to represent the locked state of recommendations.
Line range hint
1-2471: Overall improvement in API functionality and admin capabilities.The changes introduced in this update significantly enhance the MedmaeliRestAPI. New admin endpoints for toggling list processing status, locking recommendation lists, and retrieving area-specific information provide more granular control and insight. The updates to various DTOs, including the addition of the
urvinnslaLokidandlockedproperties, as well as the introduction of the MedmaelalistiExtendedDTO, improve the API's ability to represent and manage recommendation lists and individual recommendations.These changes collectively contribute to a more robust and feature-rich API, particularly for administrative functions. The consistent documentation and appropriate use of response schemas maintain the API's overall quality and usability.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16217 +/- ##
==========================================
- Coverage 36.77% 36.66% -0.11%
==========================================
Files 6784 6781 -3
Lines 139833 139716 -117
Branches 39763 39728 -35
==========================================
- Hits 51429 51233 -196
- Misses 88404 88483 +79
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 33 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 7 Total Test Services: 0 Failed, 6 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
oddsson
added
automerge
kodiakhq
bot
deleted the
feat/admin-remove-signature-collection-list
branch
…16217) * admin remove list endpoint * add new admin api --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[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
AdminApiclass to enhance administrative functionalities.Bug Fixes
Documentation