From 855d848a3b0e6b8ab1d0750e6f258877299ed7de Mon Sep 17 00:00:00 2001 From: Pooya Raki Date: Fri, 8 Nov 2024 12:54:42 +0100 Subject: [PATCH] Check whether the notification feature flag is enabled before calling notification v2 --- .../v1/notifications.controller.spec.ts | 88 +++++++++++++------ .../v1/notifications.controller.ts | 66 ++++++++------ 2 files changed, 103 insertions(+), 51 deletions(-) diff --git a/src/routes/notifications/v1/notifications.controller.spec.ts b/src/routes/notifications/v1/notifications.controller.spec.ts index 3bc721309c..638624fde1 100644 --- a/src/routes/notifications/v1/notifications.controller.spec.ts +++ b/src/routes/notifications/v1/notifications.controller.spec.ts @@ -41,6 +41,7 @@ describe('Notifications Controller (Unit)', () => { let safeConfigUrl: string; let networkService: jest.MockedObjectDeep; let notificationServiceV2: jest.MockedObjectDeep; + let isNotificationsV2Enabled: boolean; beforeEach(async () => { jest.resetAllMocks(); @@ -71,6 +72,9 @@ describe('Notifications Controller (Unit)', () => { ); notificationServiceV2 = moduleFixture.get(NotificationsServiceV2); safeConfigUrl = configurationService.getOrThrow('safeConfig.baseUri'); + isNotificationsV2Enabled = configurationService.getOrThrow( + 'features.pushNotifications', + ); networkService = moduleFixture.get(NetworkService); app = await new TestAppProvider().provide(moduleFixture); @@ -149,18 +153,20 @@ describe('Notifications Controller (Unit)', () => { (safeRegistration) => safeRegistration.safes.length > 0, ); - expect(notificationServiceV2.upsertSubscriptions).toHaveBeenCalledTimes( - safeRegistrationsWithSafe.length, - ); - - for (const [ - index, - upsertSubscriptionsV2, - ] of upsertSubscriptionsV2Dto.entries()) { - const nthCall = index + 1; // Convert zero-based index to a one-based call number + if (isNotificationsV2Enabled) { expect( notificationServiceV2.upsertSubscriptions, - ).toHaveBeenNthCalledWith(nthCall, upsertSubscriptionsV2); + ).toHaveBeenCalledTimes(safeRegistrationsWithSafe.length); + + for (const [ + index, + upsertSubscriptionsV2, + ] of upsertSubscriptionsV2Dto.entries()) { + const nthCall = index + 1; // Convert zero-based index to a one-based call number + expect( + notificationServiceV2.upsertSubscriptions, + ).toHaveBeenNthCalledWith(nthCall, upsertSubscriptionsV2); + } } }, ); @@ -202,7 +208,11 @@ describe('Notifications Controller (Unit)', () => { }), ); - expect(notificationServiceV2.upsertSubscriptions).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect( + notificationServiceV2.upsertSubscriptions, + ).not.toHaveBeenCalled(); + } }); it('Server errors returned from provider', async () => { @@ -240,7 +250,11 @@ describe('Notifications Controller (Unit)', () => { error: 'Internal Server Error', }); - expect(notificationServiceV2.upsertSubscriptions).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect( + notificationServiceV2.upsertSubscriptions, + ).not.toHaveBeenCalled(); + } }); it('Both client and server errors returned from provider', async () => { @@ -293,7 +307,11 @@ describe('Notifications Controller (Unit)', () => { error: 'Internal Server Error', }); - expect(notificationServiceV2.upsertSubscriptions).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect( + notificationServiceV2.upsertSubscriptions, + ).not.toHaveBeenCalled(); + } }); it('No status code errors returned from provider', async () => { @@ -329,7 +347,11 @@ describe('Notifications Controller (Unit)', () => { error: 'Internal Server Error', }); - expect(notificationServiceV2.upsertSubscriptions).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect( + notificationServiceV2.upsertSubscriptions, + ).not.toHaveBeenCalled(); + } }); }); @@ -358,8 +380,10 @@ describe('Notifications Controller (Unit)', () => { url: expectedProviderURL, }); - expect(notificationServiceV2.deleteDevice).toHaveBeenCalledTimes(1); - expect(notificationServiceV2.deleteDevice).toHaveBeenCalledWith(uuid); + if (isNotificationsV2Enabled) { + expect(notificationServiceV2.deleteDevice).toHaveBeenCalledTimes(1); + expect(notificationServiceV2.deleteDevice).toHaveBeenCalledWith(uuid); + } }); it('Failure: Config API fails', async () => { @@ -376,7 +400,9 @@ describe('Notifications Controller (Unit)', () => { .expect(503); expect(networkService.delete).toHaveBeenCalledTimes(0); - expect(notificationServiceV2.deleteDevice).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect(notificationServiceV2.deleteDevice).not.toHaveBeenCalled(); + } }); it('Failure: Transaction API fails', async () => { @@ -399,7 +425,9 @@ describe('Notifications Controller (Unit)', () => { .expect(503); expect(networkService.delete).toHaveBeenCalledTimes(1); - expect(notificationServiceV2.deleteDevice).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect(notificationServiceV2.deleteDevice).not.toHaveBeenCalled(); + } }); }); @@ -432,12 +460,16 @@ describe('Notifications Controller (Unit)', () => { url: expectedProviderURL, }); - expect(notificationServiceV2.deleteSubscription).toHaveBeenCalledTimes(1); - expect(notificationServiceV2.deleteSubscription).toHaveBeenCalledWith({ - deviceUuid: uuid, - chainId: chain.chainId, - safeAddress: getAddress(safeAddress), - }); + if (isNotificationsV2Enabled) { + expect(notificationServiceV2.deleteSubscription).toHaveBeenCalledTimes( + 1, + ); + expect(notificationServiceV2.deleteSubscription).toHaveBeenCalledWith({ + deviceUuid: uuid, + chainId: chain.chainId, + safeAddress: getAddress(safeAddress), + }); + } }); it('Failure: Config API fails', async () => { @@ -457,7 +489,9 @@ describe('Notifications Controller (Unit)', () => { .expect(503); expect(networkService.delete).toHaveBeenCalledTimes(0); - expect(notificationServiceV2.deleteSubscription).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect(notificationServiceV2.deleteSubscription).not.toHaveBeenCalled(); + } }); it('Failure: Transaction API fails', async () => { @@ -483,7 +517,9 @@ describe('Notifications Controller (Unit)', () => { .expect(503); expect(networkService.delete).toHaveBeenCalledTimes(1); - expect(notificationServiceV2.deleteSubscription).not.toHaveBeenCalled(); + if (isNotificationsV2Enabled) { + expect(notificationServiceV2.deleteSubscription).not.toHaveBeenCalled(); + } }); }); }); diff --git a/src/routes/notifications/v1/notifications.controller.ts b/src/routes/notifications/v1/notifications.controller.ts index 337fa65830..427f83822e 100644 --- a/src/routes/notifications/v1/notifications.controller.ts +++ b/src/routes/notifications/v1/notifications.controller.ts @@ -22,17 +22,27 @@ import type { UUID } from 'crypto'; import { NotificationsServiceV2 } from '@/routes/notifications/v2/notifications.service'; import { recoverMessageAddress } from 'viem'; import { UuidSchema } from '@/validation/entities/schemas/uuid.schema'; +import { IConfigurationService } from '@/config/configuration.service.interface'; @ApiTags('notifications') @Controller({ path: '', version: '1' }) export class NotificationsController { + private isPushNotificationV2Enabled = false; constructor( // Adding NotificationServiceV2 to ensure compatibility with V1. // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. @Inject(NotificationsServiceV2) private readonly notificationServiceV2: NotificationsServiceV2, private readonly notificationsService: NotificationsService, - ) {} + + @Inject(IConfigurationService) + private readonly configurationService: IConfigurationService, + ) { + this.isPushNotificationV2Enabled = + this.configurationService.getOrThrow( + 'features.pushNotifications', + ); + } @ApiOkResponse() @Post('register/notifications') @@ -42,21 +52,23 @@ export class NotificationsController { ): Promise { await this.notificationsService.registerDevice(registerDeviceDto); - // Compatibility with V2 - // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. - const compatibleV2Requests = - await this.createV2RegisterDto(registerDeviceDto); - - const v2Requests = []; - for (const compatibleV2Request of compatibleV2Requests) { - v2Requests.push( - await this.notificationServiceV2.upsertSubscriptions( - compatibleV2Request, - ), - ); - } + if (this.isPushNotificationV2Enabled) { + // Compatibility with V2 + // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. + const compatibleV2Requests = + await this.createV2RegisterDto(registerDeviceDto); + + const v2Requests = []; + for (const compatibleV2Request of compatibleV2Requests) { + v2Requests.push( + await this.notificationServiceV2.upsertSubscriptions( + compatibleV2Request, + ), + ); + } - await Promise.all(v2Requests); + await Promise.all(v2Requests); + } } private async createV2RegisterDto(args: RegisterDeviceDto): Promise< @@ -131,9 +143,11 @@ export class NotificationsController { ): Promise { await this.notificationsService.unregisterDevice({ chainId, uuid }); - // Compatibility with V2 - // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. - await this.notificationServiceV2.deleteDevice(uuid); + if (this.isPushNotificationV2Enabled) { + // Compatibility with V2 + // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. + await this.notificationServiceV2.deleteDevice(uuid); + } } @Delete('chains/:chainId/notifications/devices/:uuid/safes/:safeAddress') @@ -149,12 +163,14 @@ export class NotificationsController { safeAddress, }); - // Compatibility with V2 - // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. - await this.notificationServiceV2.deleteSubscription({ - deviceUuid: uuid, - chainId: chainId, - safeAddress: safeAddress, - }); + if (this.isPushNotificationV2Enabled) { + // Compatibility with V2 + // @TODO Remove NotificationModuleV2 after all clients have migrated and compatibility is no longer needed. + await this.notificationServiceV2.deleteSubscription({ + deviceUuid: uuid, + chainId: chainId, + safeAddress: safeAddress, + }); + } } }