Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(api): Use UpdatePreference use-case for all Subscriber Preference updates #6889

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -128,51 +128,7 @@ describe('UpdatePreferences', () => {
}
});

it('should create user preference if absent', async () => {
const command = {
environmentId: 'env-1',
organizationId: 'org-1',
subscriberId: 'test-mockSubscriber',
level: PreferenceLevelEnum.GLOBAL,
chat: true,
};

subscriberRepositoryMock.findBySubscriberId.resolves(mockedSubscriber);
subscriberPreferenceRepositoryMock.findOne.resolves(undefined);
getSubscriberGlobalPreferenceMock.execute.resolves(mockedGlobalPreference);

const result = await updatePreferences.execute(command);

expect(getSubscriberGlobalPreferenceMock.execute.called).to.be.true;
expect(getSubscriberGlobalPreferenceMock.execute.lastCall.args).to.deep.equal([
GetSubscriberGlobalPreferenceCommand.create({
environmentId: command.environmentId,
organizationId: command.organizationId,
subscriberId: mockedSubscriber.subscriberId,
}),
]);

expect(analyticsServiceMock.mixpanelTrack.firstCall.args).to.deep.equal([
AnalyticsEventsEnum.CREATE_PREFERENCES,
'',
{
_organization: command.organizationId,
_subscriber: mockedSubscriber._id,
level: command.level,
_workflowId: undefined,
channels: {
chat: true,
},
},
]);

expect(result).to.deep.equal({
level: command.level,
...mockedGlobalPreference.preference,
});
});

it('should update user preference if preference exists', async () => {
it('should update subscriber preference', async () => {
const command = {
environmentId: 'env-1',
organizationId: 'org-1',
Expand Down Expand Up @@ -216,7 +172,7 @@ describe('UpdatePreferences', () => {
});
});

it('should update user preference if preference exists and level is template', async () => {
it('should update subscriber preference if preference exists and level is template', async () => {
const command = {
environmentId: 'env-1',
organizationId: 'org-1',
Expand Down
Copy link
Collaborator Author

@rifont rifont Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow-up PR (to keep this diff clean), I will rename this usecase from UpdatePreferences to UpsertSubscriberPreferences as it functions as such now.

Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ import {
UpsertSubscriberWorkflowPreferencesCommand,
UpsertSubscriberGlobalPreferencesCommand,
InstrumentUsecase,
Instrument,
} from '@novu/application-generic';
import {
NotificationTemplateEntity,
NotificationTemplateRepository,
PreferenceLevelEnum,
SubscriberEntity,
SubscriberPreferenceEntity,
SubscriberPreferenceRepository,
SubscriberRepository,
} from '@novu/dal';
Expand Down Expand Up @@ -55,46 +55,16 @@ export class UpdatePreferences {
}
}

const userPreference: SubscriberPreferenceEntity | null = await this.subscriberPreferenceRepository.findOne(
this.commonQuery(command, subscriber)
);
if (!userPreference) {
await this.createUserPreference(command, subscriber);
} else {
await this.updateUserPreference(command, subscriber);
}
await this.updateSubscriberPreference(command, subscriber);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As said above, we treat this as an upsert now. GetSubscriberPreferences always returns a valid preference set now (falling back to default when nothing has been persisted), so there's no need to perform an extra lookup now before persisting the data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner!


return await this.findPreference(command, subscriber);
}

private async createUserPreference(command: UpdatePreferencesCommand, subscriber: SubscriberEntity): Promise<void> {
const channelPreferences: IPreferenceChannels = this.buildPreferenceChannels(command);

await this.storePreferencesV2({
channels: channelPreferences,
organizationId: command.organizationId,
environmentId: command.environmentId,
_subscriberId: subscriber._id,
templateId: command.workflowId,
});

this.analyticsService.mixpanelTrack(AnalyticsEventsEnum.CREATE_PREFERENCES, '', {
_organization: command.organizationId,
_subscriber: subscriber._id,
_workflowId: command.workflowId,
level: command.level,
channels: channelPreferences,
});

const query = this.commonQuery(command, subscriber);
await this.subscriberPreferenceRepository.create({
...query,
enabled: true,
channels: channelPreferences,
});
}

private async updateUserPreference(command: UpdatePreferencesCommand, subscriber: SubscriberEntity): Promise<void> {
@Instrument()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding some more instrumentation to the update preference flow.

private async updateSubscriberPreference(
command: UpdatePreferencesCommand,
subscriber: SubscriberEntity
): Promise<void> {
const channelPreferences: IPreferenceChannels = this.buildPreferenceChannels(command);

await this.storePreferencesV2({
Expand Down Expand Up @@ -136,6 +106,7 @@ export class UpdatePreferences {
};
}

@Instrument()
private async findPreference(
command: UpdatePreferencesCommand,
subscriber: SubscriberEntity
Expand Down Expand Up @@ -198,6 +169,7 @@ export class UpdatePreferences {
/**
* Strangler pattern to migrate to V2 preferences.
*/
@Instrument()
private async storePreferencesV2(item: {
channels: IPreferenceChannels;
organizationId: string;
Expand Down
1 change: 0 additions & 1 deletion apps/api/src/app/inbox/utils/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,4 @@ export enum AnalyticsEventsEnum {
UPDATE_ALL_NOTIFICATIONS = 'Update All Notifications - [Inbox]',
FETCH_PREFERENCES = 'Fetch Preferences - [Inbox]',
UPDATE_PREFERENCES = 'Update Preferences - [Inbox]',
CREATE_PREFERENCES = 'Create Preferences - [Inbox]',
}
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,14 @@ describe('Update Subscribers global preferences - /subscribers/:subscriberId/pre
expect(response.data.data.preference.channels).to.eql({});
});

it('should update user global preference and disable the flag for the future channels update', async function () {
it('should unset all preferences when the preferences object is empty', async function () {
const response = await updateGlobalPreferences({}, session);

expect(response.data.data.preference.channels).to.eql({});
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case asserts that empty objects can be supplied when updating global preferences in relation to this UpsertPreferences change


// `enabled` flag is not used anymore. The presence of a preference object means that the subscriber has enabled notifications.
it.skip('should update user global preference and disable the flag for the future channels update', async function () {
const disablePreferenceData = {
enabled: false,
};
Expand Down
8 changes: 5 additions & 3 deletions apps/api/src/app/subscribers/e2e/update-preference.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ describe('Update Subscribers preferences - /subscribers/:subscriberId/preference
expect(response.status).to.eql(404);
expect(response.data).to.have.include({
statusCode: 404,
message: 'Template with id 63cc6e0b561e0a609f223e27 is not found',
message: 'Workflow with id: 63cc6e0b561e0a609f223e27 is not found',
error: 'Not Found',
});
}
Expand Down Expand Up @@ -148,7 +148,8 @@ describe('Update Subscribers preferences - /subscribers/:subscriberId/preference
});
});

it('should update user preference and disable the flag for the future general notification template preference', async function () {
// `enabled` flag is not used anymore. The presence of a preference object means that the subscriber has enabled notifications.
it.skip('should update user preference and disable the flag for the future general notification template preference', async function () {
const initialPreferences = (await getPreference(session)).data.data[0];
expect(initialPreferences.preference.enabled).to.eql(true);
expect(initialPreferences.preference.channels).to.eql({
Expand Down Expand Up @@ -186,7 +187,8 @@ describe('Update Subscribers preferences - /subscribers/:subscriberId/preference
});
});

it('should update user preference and enable the flag for the future general notification template preference', async function () {
// `enabled` flag is not used anymore. The presence of a preference object means that the subscriber has enabled notifications.
it.skip('should update user preference and enable the flag for the future general notification template preference', async function () {
const initialPreferences = (await getPreference(session)).data.data[0];
expect(initialPreferences.preference.enabled).to.eql(true);
expect(initialPreferences.preference.channels).to.eql({
Expand Down
84 changes: 58 additions & 26 deletions apps/api/src/app/subscribers/subscribers.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
Get,
HttpCode,
HttpStatus,
NotFoundException,
Param,
Patch,
Post,
Expand All @@ -29,6 +30,8 @@ import {
ApiRateLimitCostEnum,
ButtonTypeEnum,
ChatProviderIdEnum,
IPreferenceChannels,
TriggerTypeEnum,
UserSessionData,
} from '@novu/shared';
import { MessageEntity, PreferenceLevelEnum } from '@novu/dal';
Expand All @@ -50,8 +53,6 @@ import { GetSubscribers, GetSubscribersCommand } from './usecases/get-subscriber
import { GetSubscriber, GetSubscriberCommand } from './usecases/get-subscriber';
import { GetPreferencesByLevelCommand } from './usecases/get-preferences-by-level/get-preferences-by-level.command';
import { GetPreferencesByLevel } from './usecases/get-preferences-by-level/get-preferences-by-level.usecase';
import { UpdatePreference } from './usecases/update-preference/update-preference.usecase';
import { UpdateSubscriberPreferenceCommand } from './usecases/update-subscriber-preference';
import { UpdateSubscriberPreferenceResponseDto } from '../widgets/dtos/update-subscriber-preference-response.dto';
import { UpdateSubscriberPreferenceRequestDto } from '../widgets/dtos/update-subscriber-preference-request.dto';
import { MessageResponseDto } from '../widgets/dtos/message-response.dto';
Expand Down Expand Up @@ -90,10 +91,6 @@ import { MarkAllMessagesAs } from '../widgets/usecases/mark-all-messages-as/mark
import { MarkAllMessageAsRequestDto } from './dtos/mark-all-messages-as-request.dto';
import { BulkCreateSubscribers } from './usecases/bulk-create-subscribers/bulk-create-subscribers.usecase';
import { BulkCreateSubscribersCommand } from './usecases/bulk-create-subscribers';
import {
UpdateSubscriberGlobalPreferences,
UpdateSubscriberGlobalPreferencesCommand,
} from './usecases/update-subscriber-global-preferences';
import { GetSubscriberPreferencesByLevelParams } from './params';
import { ThrottlerCategory, ThrottlerCost } from '../rate-limiting/guards';
import { MessageMarkAsRequestDto } from '../widgets/dtos/mark-as-request.dto';
Expand All @@ -102,6 +99,8 @@ import { MarkMessageAsByMark } from '../widgets/usecases/mark-message-as-by-mark
import { FeedResponseDto } from '../widgets/dtos/feeds-response.dto';
import { UserAuthentication } from '../shared/framework/swagger/api.key.security';
import { SdkGroupName, SdkMethodName, SdkUsePagination } from '../shared/framework/swagger/sdk.decorators';
import { UpdatePreferences } from '../inbox/usecases/update-preferences/update-preferences.usecase';
import { UpdatePreferencesCommand } from '../inbox/usecases/update-preferences/update-preferences.command';

@ThrottlerCategory(ApiRateLimitCategoryEnum.CONFIGURATION)
@ApiCommonResponses()
Expand All @@ -117,8 +116,7 @@ export class SubscribersController {
private getSubscriberUseCase: GetSubscriber,
private getSubscribersUsecase: GetSubscribers,
private getPreferenceUsecase: GetPreferencesByLevel,
private updatePreferenceUsecase: UpdatePreference,
private updateGlobalPreferenceUsecase: UpdateSubscriberGlobalPreferences,
private updatePreferencesUsecase: UpdatePreferences,
private getNotificationsFeedUsecase: GetNotificationsFeed,
private getFeedCountUsecase: GetFeedCount,
private markMessageAsUsecase: MarkMessageAs,
Expand Down Expand Up @@ -465,16 +463,37 @@ export class SubscribersController {
@Param('parameter') templateId: string,
@Body() body: UpdateSubscriberPreferenceRequestDto
): Promise<UpdateSubscriberPreferenceResponseDto> {
const command = UpdateSubscriberPreferenceCommand.create({
organizationId: user.organizationId,
subscriberId,
environmentId: user.environmentId,
templateId,
...(typeof body.enabled === 'boolean' && { enabled: body.enabled }),
...(body.channel && { channel: body.channel }),
});
const result = await this.updatePreferencesUsecase.execute(
UpdatePreferencesCommand.create({
environmentId: user.environmentId,
organizationId: user.organizationId,
subscriberId,
workflowId: templateId,
level: PreferenceLevelEnum.TEMPLATE,
...(body.channel && { [body.channel.type]: body.channel.enabled }),
})
);

return await this.updatePreferenceUsecase.execute(command);
if (!result.workflow) throw new NotFoundException('Workflow not found');

return {
preference: {
channels: result.channels,
enabled: result.enabled,
},
template: {
_id: result.workflow.id,
name: result.workflow.name,
critical: result.workflow.critical,
triggers: [
{
identifier: result.workflow.identifier,
type: TriggerTypeEnum.EVENT,
variables: [],
},
],
},
};
}

@Patch('/:subscriberId/preferences')
Expand All @@ -490,16 +509,29 @@ export class SubscribersController {
@UserSession() user: UserSessionData,
@Param('subscriberId') subscriberId: string,
@Body() body: UpdateSubscriberGlobalPreferencesRequestDto
) {
const command = UpdateSubscriberGlobalPreferencesCommand.create({
organizationId: user.organizationId,
subscriberId,
environmentId: user.environmentId,
enabled: body.enabled,
preferences: body.preferences,
});
): Promise<Omit<UpdateSubscriberPreferenceResponseDto, 'template'>> {
const channels = body.preferences?.reduce((acc, curr) => {
acc[curr.type] = curr.enabled;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this enabled flag the same as the one we talked about in skipped tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is the channel-level flag. There are 2 enabled flags:

  • enabled at top level of V1 Preferences, which controls whether to use the subscriber preference during preference merging
  • enabled flag for each channel, which controls delivery for the channel


return acc;
}, {} as IPreferenceChannels);

const result = await this.updatePreferencesUsecase.execute(
UpdatePreferencesCommand.create({
environmentId: user.environmentId,
organizationId: user.organizationId,
subscriberId,
level: PreferenceLevelEnum.GLOBAL,
...channels,
})
);

return await this.updateGlobalPreferenceUsecase.execute(command);
return {
preference: {
channels: result.channels,
enabled: result.enabled,
},
};
}

@ExternalApiAccessible()
Expand Down
2 changes: 0 additions & 2 deletions apps/api/src/app/subscribers/subscribers.module.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import { forwardRef, Module } from '@nestjs/common';
import { TerminusModule } from '@nestjs/terminus';
import { GetPreferences, UpsertPreferences } from '@novu/application-generic';
import { PreferencesRepository } from '@novu/dal';
import { AuthModule } from '../auth/auth.module';
import { SharedModule } from '../shared/shared.module';
import { WidgetsModule } from '../widgets/widgets.module';
Expand Down
8 changes: 2 additions & 6 deletions apps/api/src/app/subscribers/usecases/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,17 +12,15 @@ import { GetSubscriber } from './get-subscriber';
import { GetPreferencesByLevel } from './get-preferences-by-level/get-preferences-by-level.usecase';
import { RemoveSubscriber } from './remove-subscriber';
import { SearchByExternalSubscriberIds } from './search-by-external-subscriber-ids';
import { UpdatePreference } from './update-preference/update-preference.usecase';
import { UpdateSubscriberPreference } from './update-subscriber-preference';
import { UpdateSubscriberOnlineFlag } from './update-subscriber-online-flag';
import { ChatOauth } from './chat-oauth/chat-oauth.usecase';
import { ChatOauthCallback } from './chat-oauth-callback/chat-oauth-callback.usecase';
import { DeleteSubscriberCredentials } from './delete-subscriber-credentials/delete-subscriber-credentials.usecase';
import { BulkCreateSubscribers } from './bulk-create-subscribers/bulk-create-subscribers.usecase';
import { UpdateSubscriberGlobalPreferences } from './update-subscriber-global-preferences';
import { CreateIntegration } from '../../integrations/usecases/create-integration/create-integration.usecase';
import { CheckIntegration } from '../../integrations/usecases/check-integration/check-integration.usecase';
import { CheckIntegrationEMail } from '../../integrations/usecases/check-integration/check-integration-email.usecase';
import { UpdatePreferences } from '../../inbox/usecases/update-preferences/update-preferences.usecase';

export {
SearchByExternalSubscriberIds,
Expand All @@ -38,18 +36,16 @@ export const USE_CASES = [
GetPreferencesByLevel,
RemoveSubscriber,
SearchByExternalSubscriberIds,
UpdatePreference,
UpdateSubscriber,
UpdateSubscriberChannel,
UpdateSubscriberPreference,
UpdateSubscriberOnlineFlag,
ChatOauthCallback,
ChatOauth,
DeleteSubscriberCredentials,
BulkCreateSubscribers,
UpdateSubscriberGlobalPreferences,
GetSubscriberGlobalPreference,
CreateIntegration,
CheckIntegration,
CheckIntegrationEMail,
UpdatePreferences,
];
Loading
Loading