feat(providers): improve expo response management and multi device token #4508
Conversation
|
|
||
| if (!result) { | ||
| integrationsWithErrors++; | ||
| const target = (overrides as { deviceTokens?: string[] }).deviceTokens || deviceTokens; |
There was a problem hiding this comment.
Extracting the deviceTokens out of this.sendMessage allowed us to easily loop them reusing the new flow implemented in #4495.
| command, | ||
| command.payload, |
There was a problem hiding this comment.
Also taking advantage of cleaning the function arguments.
| subscriber: IPushOptions['subscriber'], | ||
| integration: IntegrationEntity, | ||
| target: string[], | ||
| deviceToken: string, |
There was a problem hiding this comment.
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.
| @@ -340,7 +315,7 @@ export class SendMessagePush extends SendMessageBase { | |||
| status: ExecutionDetailsStatusEnum.SUCCESS, | |||
| isTest: false, | |||
| isRetry: false, | |||
| raw: JSON.stringify(result), | |||
| raw: JSON.stringify({ result, deviceToken }), | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@scopsy is it good to proceed?
| @@ -17,7 +17,7 @@ export class PushFactory implements IPushFactory { | |||
| new PushWebhookHandler(), | |||
| ]; | |||
|
|
|||
| getHandler(integration: IntegrationEntity) { | |||
| getHandler(integration: IntegrationEntity): IPushHandler { | |||
There was a problem hiding this comment.
Type wasn't being infered by TypeScript. +1 for team explicit here.
| @@ -43,6 +44,8 @@ const LOG_CONTEXT = 'SendMessagePush'; | |||
| export class SendMessagePush extends SendMessageBase { | |||
| channelType = ChannelTypeEnum.PUSH; | |||
|
|
|||
| private pushFactory; | |||
There was a problem hiding this comment.
@p-fernandez due to how nestjs works, we cannot have this one here as a private method, as nestjs @Injectable classes are singletons, and this will be reused across multiple executions. Which we don't want We need here to not store it as a method but rather pass it.
p-fernandez
force-pushed
the
nv-3023-handle-multiple-device-tokens-in-expo
branch
from
612e10d to
e36dee3
Compare
What change does this PR introduce?
Improves the response management of the Expo Provider and finally handles error cases.
In SendMessagePush we improve the information provided for the responses showing the device token that has failed or succeeded for situations of subscribers with multiple device tokens in the channel integration.
Why was this change needed?
This work is part of the bug discovered by @jainpawan21. Execution Details wasn't showing clearly that some device tokens were failing when using multiple active providers and using multiple device tokens for the same subscriber.
Also the Expo provider implementation had a lot of room to improve and was part to blame of the situation we were having.
Other information (Screenshots)
Expo provider will still miss multi token management (inherited tech debt).
Screenshots:
