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.

Sign up for GitHub

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

feat(providers): improve expo response management and multi device token #4508

Merged
merged 1 commit into from
Oct 13, 2023
Merged

feat(providers): improve expo response management and multi device token #4508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
139 changes: 82 additions & 57 deletions apps/worker/src/app/workflow/usecases/send-message/send-message-push.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
SelectIntegration,
CompileTemplate,
CompileTemplateCommand,
IPushHandler,
PushFactory,
GetNovuProviderCredentials,
} from '@novu/application-generic';
Expand Down Expand Up @@ -148,21 +149,26 @@ export class SendMessagePush extends SendMessageBase {
await this.sendSelectedIntegrationExecution(command.job, integration);

const overrides = command.overrides[integration.providerId] || {};

const result = await this.sendMessage(
subscriber,
integration,
deviceTokens,
title,
content,
command,
command.payload,
Comment on lines -158 to -159
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also taking advantage of cleaning the function arguments.

overrides,
stepData
);

if (!result) {
integrationsWithErrors++;
const target = (overrides as { deviceTokens?: string[] }).deviceTokens || deviceTokens;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extracting the deviceTokens out of this.sendMessage allowed us to easily loop them reusing the new flow implemented in #4495.


const message = await this.createMessage(command, integration, title, content, target, overrides);

for (const deviceToken of target) {
const result = await this.sendMessage(
command,
message,
subscriber,
integration,
deviceToken,
title,
content,
overrides,
stepData
);

if (!result) {
integrationsWithErrors++;
}
}
}

Expand Down Expand Up @@ -273,59 +279,24 @@ export class SendMessagePush extends SendMessageBase {
}

private async sendMessage(
command: SendMessageCommand,
message: MessageEntity,
subscriber: IPushOptions['subscriber'],
integration: IntegrationEntity,
target: string[],
deviceToken: string,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we will miss the functionalities of sending multiple tokens at the same time in exchange of better execution details without doing breaking changes in the providers.
We need to implement auxiliary methods in the providers to handle the multiple token and to handle properly their responses.

title: string,
content: string,
command: SendMessageCommand,
payload: object,
overrides: object,
step: IPushOptions['step']
): Promise<boolean> {
const message: MessageEntity = await this.messageRepository.create({
_notificationId: command.notificationId,
_environmentId: command.environmentId,
_organizationId: command.organizationId,
_subscriberId: command._subscriberId,
_templateId: command._templateId,
_messageTemplateId: command.step?.template?._id,
channel: ChannelTypeEnum.PUSH,
transactionId: command.transactionId,
deviceTokens: target,
content: this.storeContent() ? content : null,
title,
payload: payload as never,
overrides: overrides as never,
providerId: integration.providerId,
_jobId: command.jobId,
});

await this.createExecutionDetails.execute(
CreateExecutionDetailsCommand.create({
...CreateExecutionDetailsCommand.getDetailsFromJob(command.job),
detail: `${DetailEnum.MESSAGE_CREATED}: ${integration.providerId}`,
source: ExecutionDetailsSourceEnum.INTERNAL,
status: ExecutionDetailsStatusEnum.PENDING,
messageId: message._id,
isTest: false,
isRetry: false,
raw: this.storeContent() ? JSON.stringify(content) : null,
})
);

try {
const pushFactory = new PushFactory();
const pushHandler = pushFactory.getHandler(integration);
if (!pushHandler) {
throw new PlatformException(`Push handler for provider ${integration.providerId} is not found`);
}
const pushHandler = this.getIntegrationHandler(integration);

const result = await pushHandler.send({
target: (overrides as { deviceTokens?: string[] }).deviceTokens || target,
target: [deviceToken],
title,
content,
payload,
payload: command.payload,
overrides,
subscriber,
step,
Expand All @@ -340,7 +311,7 @@ export class SendMessagePush extends SendMessageBase {
status: ExecutionDetailsStatusEnum.SUCCESS,
isTest: false,
isRetry: false,
raw: JSON.stringify(result),
raw: JSON.stringify({ result, deviceToken }),
Copy link
Contributor Author

@p-fernandez p-fernandez Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We add the device token in the execution detail raw information to help users to identify which ones succeeded or failed, though it can be arguable if we shouldn't as identifies the device of a user. We need to be careful with them.

Some providers treat the device token as not PII, like for example OneSignal:
https://documentation.onesignal.com/docs/handling-personal-data#push-tokens, but we should analyse our own risks.
CC: @scopsy @Cliftonz @djabarovgeorge @jainpawan21

Copy link
Member

@jainpawan21 jainpawan21 Oct 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@scopsy is it good to proceed?

})
);

Expand All @@ -363,4 +334,58 @@ export class SendMessagePush extends SendMessageBase {
return false;
}
}

private async createMessage(
command: SendMessageCommand,
integration: IntegrationEntity,
title: string,
content: string,
deviceTokens: string[],
overrides: object
): Promise<MessageEntity> {
const message = await this.messageRepository.create({
_notificationId: command.notificationId,
_environmentId: command.environmentId,
_organizationId: command.organizationId,
_subscriberId: command._subscriberId,
_templateId: command._templateId,
_messageTemplateId: command.step?.template?._id,
channel: ChannelTypeEnum.PUSH,
transactionId: command.transactionId,
deviceTokens,
content: this.storeContent() ? content : null,
title,
payload: command.payload as never,
overrides: overrides as never,
providerId: integration.providerId,
_jobId: command.jobId,
});

await this.createExecutionDetails.execute(
CreateExecutionDetailsCommand.create({
...CreateExecutionDetailsCommand.getDetailsFromJob(command.job),
detail: `${DetailEnum.MESSAGE_CREATED}: ${integration.providerId}`,
source: ExecutionDetailsSourceEnum.INTERNAL,
status: ExecutionDetailsStatusEnum.PENDING,
messageId: message._id,
isTest: false,
isRetry: false,
raw: this.storeContent() ? JSON.stringify(content) : null,
})
);

return message;
}

private getIntegrationHandler(integration): IPushHandler {
const pushFactory = new PushFactory();
const pushHandler = pushFactory.getHandler(integration);

if (!pushHandler) {
const message = `Push handler for provider ${integration.providerId} is not found`;
throw new PlatformException(message);
}

return pushHandler;
}
}
1 change: 1 addition & 0 deletions packages/application-generic/src/factories/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@ export * from './chat/chat.factory';
export * from './sms/sms.factory';
export * from './mail/mail.factory';
export * from './push/push.factory';
export * from './push/interfaces/push.handler.interface';
export * from './sms/interfaces';
export * from './mail/interfaces/send.handler.interface';
2 changes: 1 addition & 1 deletion packages/application-generic/src/factories/push/push.factory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export class PushFactory implements IPushFactory {
new PushWebhookHandler(),
];

getHandler(integration: IntegrationEntity) {
getHandler(integration: IntegrationEntity): IPushHandler {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type wasn't being infered by TypeScript. +1 for team explicit here.

const handler =
this.handlers.find((handlerItem) =>
handlerItem.canHandle(integration.providerId, integration.channel)
Expand Down
Loading
Loading