From 53d0d35b0305aea4b6b6ef1229ab1a33c0d75b7e Mon Sep 17 00:00:00 2001 From: Richard Fontein <32132657+rifont@users.noreply.github.com> Date: Mon, 19 Feb 2024 19:15:40 +0000 Subject: [PATCH] feat(api): Add idempotent subscriber credential update operation (#5211) * feat(api): Add idempotent subscriber credential update operation * fix(api): Patch operation wording for subscriber credential update * test(api): Fix broken delete subscriber test --- .../app/subscribers/subscribers.controller.ts | 29 +++++ .../chat-oauth-callback.usecase.ts | 1 + .../delete-subscriber-credentials.spec.ts | 2 + .../update-subscriber-channel.command.ts | 5 +- .../update-subscriber-channel.spec.ts | 104 +++++++++++++++--- .../update-subscriber-channel.usecase.ts | 18 ++- 6 files changed, 138 insertions(+), 21 deletions(-) diff --git a/apps/api/src/app/subscribers/subscribers.controller.ts b/apps/api/src/app/subscribers/subscribers.controller.ts index 7bd20ae00da..1ffd357da11 100644 --- a/apps/api/src/app/subscribers/subscribers.controller.ts +++ b/apps/api/src/app/subscribers/subscribers.controller.ts @@ -271,6 +271,35 @@ export class SubscribersController { credentials: body.credentials, integrationIdentifier: body.integrationIdentifier, oauthHandler: OAuthHandlerEnum.EXTERNAL, + isIdempotentOperation: true, + }) + ); + } + + @Patch('/:subscriberId/credentials') + @ExternalApiAccessible() + @UseGuards(UserAuthGuard) + @ApiResponse(SubscriberResponseDto) + @ApiOperation({ + summary: 'Modify subscriber credentials', + description: `Subscriber credentials associated to the delivery methods such as slack and push tokens. + This endpoint appends provided credentials and deviceTokens to the existing ones.`, + }) + async modifySubscriberChannel( + @UserSession() user: IJwtPayload, + @Param('subscriberId') subscriberId: string, + @Body() body: UpdateSubscriberChannelRequestDto + ): Promise { + return await this.updateSubscriberChannelUsecase.execute( + UpdateSubscriberChannelCommand.create({ + environmentId: user.environmentId, + organizationId: user.organizationId, + subscriberId, + providerId: body.providerId, + credentials: body.credentials, + integrationIdentifier: body.integrationIdentifier, + oauthHandler: OAuthHandlerEnum.EXTERNAL, + isIdempotentOperation: false, }) ); } diff --git a/apps/api/src/app/subscribers/usecases/chat-oauth-callback/chat-oauth-callback.usecase.ts b/apps/api/src/app/subscribers/usecases/chat-oauth-callback/chat-oauth-callback.usecase.ts index 5eb27f99e63..6f3fa5caf53 100644 --- a/apps/api/src/app/subscribers/usecases/chat-oauth-callback/chat-oauth-callback.usecase.ts +++ b/apps/api/src/app/subscribers/usecases/chat-oauth-callback/chat-oauth-callback.usecase.ts @@ -79,6 +79,7 @@ export class ChatOauthCallback { integrationIdentifier: command.integrationIdentifier, credentials: subscriberCredentials, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: false, }) ); } diff --git a/apps/api/src/app/subscribers/usecases/delete-subscriber-credentials/delete-subscriber-credentials.spec.ts b/apps/api/src/app/subscribers/usecases/delete-subscriber-credentials/delete-subscriber-credentials.spec.ts index 3e58fe3b16a..ef808b462b7 100644 --- a/apps/api/src/app/subscribers/usecases/delete-subscriber-credentials/delete-subscriber-credentials.spec.ts +++ b/apps/api/src/app/subscribers/usecases/delete-subscriber-credentials/delete-subscriber-credentials.spec.ts @@ -41,6 +41,7 @@ describe('Delete subscriber provider credentials', function () { providerId: ChatProviderIdEnum.Discord, credentials: { webhookUrl: 'newWebhookUrl' }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: false, }) ); @@ -52,6 +53,7 @@ describe('Delete subscriber provider credentials', function () { providerId: PushProviderIdEnum.FCM, credentials: { deviceTokens: fcmTokens }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: false, }) ); diff --git a/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.command.ts b/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.command.ts index c5fe2aeaa17..73dbb2ed04f 100644 --- a/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.command.ts +++ b/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.command.ts @@ -1,4 +1,4 @@ -import { IsNotEmpty, IsOptional, IsString, ValidateNested } from 'class-validator'; +import { IsBoolean, IsNotEmpty, IsOptional, IsString, ValidateNested } from 'class-validator'; import { EnvironmentCommand } from '../../../shared/commands/project.command'; import { ChatProviderIdEnum, PushProviderIdEnum } from '@novu/shared'; import { ChannelCredentials, SubscriberChannel } from '../../../shared/dtos/subscriber-channel'; @@ -33,4 +33,7 @@ export class UpdateSubscriberChannelCommand extends EnvironmentCommand implement @IsOptional() @IsString() integrationIdentifier?: string; + + @IsBoolean() + isIdempotentOperation: boolean; } diff --git a/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.spec.ts b/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.spec.ts index 0ed4904f0b0..73bd7604e2c 100644 --- a/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.spec.ts +++ b/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.spec.ts @@ -43,6 +43,7 @@ describe('Update Subscriber channel credentials', function () { providerId: subscriberChannel.providerId, credentials: subscriberChannel.credentials, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -70,6 +71,7 @@ describe('Update Subscriber channel credentials', function () { providerId: ChatProviderIdEnum.Discord, credentials: { webhookUrl: 'webhookUrl' }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -85,6 +87,7 @@ describe('Update Subscriber channel credentials', function () { providerId: newSlackSubscribersChannel.providerId, credentials: newSlackSubscribersChannel.credentials, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -122,6 +125,7 @@ describe('Update Subscriber channel credentials', function () { providerId: newSlackCredentials.providerId, credentials: newSlackCredentials.credentials, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -163,6 +167,7 @@ describe('Update Subscriber channel credentials', function () { providerId: ChatProviderIdEnum.Slack, credentials: { webhookUrl }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -178,7 +183,7 @@ describe('Update Subscriber channel credentials', function () { expect(updatedChannel?.credentials.webhookUrl).to.equal(webhookUrl); }); - it('should not add duplicated token ', async function () { + it('should not add duplicated token when the operation IS idempotent', async function () { const subscriberService = new SubscribersService(session.organization._id, session.environment._id); const subscriber = await subscriberService.createSubscriber(); @@ -195,6 +200,42 @@ describe('Update Subscriber channel credentials', function () { providerId: fcmCredentials.providerId, credentials: fcmCredentials.credentials, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, + }) + ); + + let updatedSubscriber = await subscriberRepository.findOne({ + _id: subscriber._id, + _environmentId: subscriber._environmentId, + }); + + const addedFcmToken = updatedSubscriber?.channels?.find( + (channel) => channel.providerId === fcmCredentials.providerId + ); + + expect(addedFcmToken?.providerId).to.equal(PushProviderIdEnum.FCM); + expect(addedFcmToken?.credentials?.deviceTokens?.length).to.equal(1); + expect(addedFcmToken?.credentials?.deviceTokens).to.deep.equal(['token_1']); + }); + + it('should not add duplicated token when the operation IS NOT idempotent', async function () { + const subscriberService = new SubscribersService(session.organization._id, session.environment._id); + const subscriber = await subscriberService.createSubscriber(); + + const fcmCredentials = { + providerId: PushProviderIdEnum.FCM, + credentials: { deviceTokens: ['token_1', 'token_1'] }, + }; + + await updateSubscriberChannelUsecase.execute( + UpdateSubscriberChannelCommand.create({ + organizationId: subscriber._organizationId, + subscriberId: subscriber.subscriberId, + environmentId: session.environment._id, + providerId: fcmCredentials.providerId, + credentials: fcmCredentials.credentials, + oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: false, }) ); @@ -212,7 +253,7 @@ describe('Update Subscriber channel credentials', function () { expect(addedFcmToken?.credentials?.deviceTokens).to.deep.equal(['identifier', 'token_1']); }); - it('should update deviceTokens with empty array', async function () { + it('should append to existing device token array when the operation IS NOT idempotent', async function () { const subscriberService = new SubscribersService(session.organization._id, session.environment._id); const subscriber = await subscriberService.createSubscriber(); @@ -229,6 +270,7 @@ describe('Update Subscriber channel credentials', function () { providerId: fcmCredentials.providerId, credentials: fcmCredentials.credentials, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: false, }) ); @@ -241,8 +283,43 @@ describe('Update Subscriber channel credentials', function () { (channel) => channel.providerId === fcmCredentials.providerId ); + expect(addedFcmToken?.providerId).to.equal(PushProviderIdEnum.FCM); expect(addedFcmToken?.credentials?.deviceTokens?.length).to.equal(2); expect(addedFcmToken?.credentials?.deviceTokens).to.deep.equal(['identifier', 'token_1']); + }); + + it('should update deviceTokens with empty array', async function () { + const subscriberService = new SubscribersService(session.organization._id, session.environment._id); + const subscriber = await subscriberService.createSubscriber(); + + const fcmCredentials = { + providerId: PushProviderIdEnum.FCM, + credentials: { deviceTokens: ['token_1'] }, + }; + + await updateSubscriberChannelUsecase.execute( + UpdateSubscriberChannelCommand.create({ + organizationId: subscriber._organizationId, + subscriberId: subscriber.subscriberId, + environmentId: session.environment._id, + providerId: fcmCredentials.providerId, + credentials: fcmCredentials.credentials, + oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, + }) + ); + + let updatedSubscriber = await subscriberRepository.findOne({ + _id: subscriber._id, + _environmentId: subscriber._environmentId, + }); + + const addedFcmToken = updatedSubscriber?.channels?.find( + (channel) => channel.providerId === fcmCredentials.providerId + ); + + expect(addedFcmToken?.credentials?.deviceTokens?.length).to.equal(1); + expect(addedFcmToken?.credentials?.deviceTokens).to.deep.equal(['token_1']); await updateSubscriberChannelUsecase.execute( UpdateSubscriberChannelCommand.create({ @@ -252,6 +329,7 @@ describe('Update Subscriber channel credentials', function () { providerId: fcmCredentials.providerId, credentials: { deviceTokens: [] }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -280,6 +358,7 @@ describe('Update Subscriber channel credentials', function () { providerId: PushProviderIdEnum.FCM, credentials: { deviceTokens: ['token_1'] }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -290,8 +369,8 @@ describe('Update Subscriber channel credentials', function () { let updateToken = updatedSubscriber?.channels?.find((channel) => channel.providerId === PushProviderIdEnum.FCM); - expect(updateToken?.credentials?.deviceTokens?.length).to.equal(2); - expect(updateToken?.credentials?.deviceTokens).to.deep.equal(['identifier', 'token_1']); + expect(updateToken?.credentials?.deviceTokens?.length).to.equal(1); + expect(updateToken?.credentials?.deviceTokens).to.deep.equal(['token_1']); await updateSubscriberChannelUsecase.execute( UpdateSubscriberChannelCommand.create({ @@ -301,6 +380,7 @@ describe('Update Subscriber channel credentials', function () { providerId: PushProviderIdEnum.FCM, credentials: { deviceTokens: ['token_1', 'token_2', 'token_2', 'token_3'] }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -311,8 +391,8 @@ describe('Update Subscriber channel credentials', function () { updateToken = updatedSubscriber?.channels?.find((channel) => channel.providerId === PushProviderIdEnum.FCM); - expect(updateToken?.credentials?.deviceTokens?.length).to.equal(4); - expect(updateToken?.credentials?.deviceTokens).to.deep.equal(['identifier', 'token_1', 'token_2', 'token_3']); + expect(updateToken?.credentials?.deviceTokens?.length).to.equal(3); + expect(updateToken?.credentials?.deviceTokens).to.deep.equal(['token_1', 'token_2', 'token_3']); await updateSubscriberChannelUsecase.execute( UpdateSubscriberChannelCommand.create({ @@ -322,6 +402,7 @@ describe('Update Subscriber channel credentials', function () { providerId: PushProviderIdEnum.FCM, credentials: { deviceTokens: ['token_555'] }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); @@ -332,14 +413,8 @@ describe('Update Subscriber channel credentials', function () { updateToken = updatedSubscriber?.channels?.find((channel) => channel.providerId === PushProviderIdEnum.FCM); - expect(updateToken?.credentials?.deviceTokens?.length).to.equal(5); - expect(updateToken?.credentials?.deviceTokens).to.deep.equal([ - 'identifier', - 'token_1', - 'token_2', - 'token_3', - 'token_555', - ]); + expect(updateToken?.credentials?.deviceTokens?.length).to.equal(1); + expect(updateToken?.credentials?.deviceTokens).to.deep.equal(['token_555']); }); it('should update deviceTokens without duplication on channel creation (addChannelToSubscriber)', async function () { @@ -362,6 +437,7 @@ describe('Update Subscriber channel credentials', function () { providerId: PushProviderIdEnum.FCM, credentials: { deviceTokens: ['token_1', 'token_1', 'token_1'] }, oauthHandler: OAuthHandlerEnum.NOVU, + isIdempotentOperation: true, }) ); diff --git a/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.usecase.ts b/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.usecase.ts index dea6de4e3f7..d94fcfb42c4 100644 --- a/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.usecase.ts +++ b/apps/api/src/app/subscribers/usecases/update-subscriber-channel/update-subscriber-channel.usecase.ts @@ -61,7 +61,8 @@ export class UpdateSubscriberChannel { command.environmentId, existingChannel, updatePayload, - foundSubscriber + foundSubscriber, + command.isIdempotentOperation ); } else { await this.addChannelToSubscriber(updatePayload, foundIntegration, command, foundSubscriber); @@ -110,7 +111,8 @@ export class UpdateSubscriberChannel { environmentId: string, existingChannel: IChannelSettings, updatePayload: Partial, - foundSubscriber: SubscriberEntity + foundSubscriber: SubscriberEntity, + isIdempotentOperation: boolean ) { const equal = isEqual(existingChannel.credentials, updatePayload.credentials); @@ -121,10 +123,14 @@ export class UpdateSubscriberChannel { let deviceTokens: string[] = []; if (updatePayload.credentials?.deviceTokens) { - deviceTokens = this.unionDeviceTokens( - existingChannel.credentials.deviceTokens ?? [], - updatePayload.credentials.deviceTokens - ); + if (isIdempotentOperation) { + deviceTokens = this.unionDeviceTokens([], updatePayload.credentials.deviceTokens); + } else { + deviceTokens = this.unionDeviceTokens( + existingChannel.credentials.deviceTokens ?? [], + updatePayload.credentials.deviceTokens + ); + } } await this.invalidateCache.invalidateByKey({