feat: add a delete notifications endpoint #2598
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new endpoint to delete multiple safe push notification subscriptions for a given device.
- Adds a new service method (deleteSubscriptions) to handle multiple subscription deletions.
- Enhances the controller with a DELETE endpoint and updates Swagger documentation.
- Provides corresponding tests and DTO updates to support the new functionality.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/routes/notifications/v2/notifications.service.ts | Introduces a new service method to delete subscriptions for multiple safes. |
| src/routes/notifications/v2/notifications.controller.ts | Adds a new DELETE route and API documentation for bulk deletion. |
| src/routes/notifications/v2/notifications.controller.spec.ts | Covers testing for the new bulk deletion endpoint, handling both success and error cases. |
| src/routes/notifications/v2/entities/tests/delete-subscriptions.dto.builder.ts | Implements a DTO builder to generate test data for delete subscriptions. |
| src/domain/notifications/v2/entities/delete-subscriptions.dto.entity.ts | Creates DTO definitions and validation schemas for the delete subscriptions endpoint. |
| src/domain/notifications/v2/entities/tests/delete-subscriptions-dto.entity.spec.ts | Provides tests for validating the delete subscriptions DTO and its constraints. |
| for (const safe of args.safes) { | ||
| await this.notificationsRepository.deleteSubscription({ | ||
| deviceUuid: args.deviceUuid, | ||
| chainId: safe.chainId, | ||
| safeAddress: safe.safeAddress, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Consider using Promise.all to concurrently delete subscriptions for improved performance if the deletion operations are independent.
| for (const safe of args.safes) { | |
| await this.notificationsRepository.deleteSubscription({ | |
| deviceUuid: args.deviceUuid, | |
| chainId: safe.chainId, | |
| safeAddress: safe.safeAddress, | |
| }); | |
| } | |
| await Promise.all( | |
| args.safes.map(safe => | |
| this.notificationsRepository.deleteSubscription({ | |
| deviceUuid: args.deviceUuid, | |
| chainId: safe.chainId, | |
| safeAddress: safe.safeAddress, | |
| }), | |
| ), | |
| ); |
Copilot uses AI. Check for mistakes.
The endpoint allows the deletion of multiple safe push notification subscriptions at once similar to the register endpoint where one can provide multiple safes to subscribe to.
| ): Promise<void> { | ||
| return this.notificationsService.deleteSubscriptions({ | ||
| deviceUuid, | ||
| safes: deleteSubscriptionsDto.safes.map((safe) => ({ |
There was a problem hiding this comment.
Nit: For more readability we can extract this map to a const
| safes: Array<{ chainId: string; safeAddress: `0x${string}` }>; | ||
| }): Promise<void> { | ||
| for (const safe of args.safes) { | ||
| await this.notificationsRepository.deleteSubscription({ |
There was a problem hiding this comment.
I suggest we avoid calling deleteSubscription in a loop and instead create a new repository method with a custom database query. Currently, passing 10 subscriptions results in at least 10 queries—possibly 20, depending on how we aak the ORM to load the device relation. By writing a dedicated method, we can fetch all related devices in a single query and remove all safes for those devices in another, reducing it to just two queries total.
|
I'll close the PR for now. Please feel free to reopen it with a fix. |
Summary
The endpoint allows the deletion of multiple safe push notification subscriptions at once, similar to the register endpoint where one can provide multiple safes to subscribe to.
fixes #2597
Changes