feat(providers): Add possibility to Onesignal choose version api #6976
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This solution use new API. This allows you to use the new and recommended Onesignal API. The old API (with player model) may be marked as obsolete or unsupported over time. |
Absolutely, I closed mine |
| @@ -606,6 +606,18 @@ export const oneSignalConfig: IConfigCredentials[] = [ | |||
| type: 'text', | |||
| required: true, | |||
| }, | |||
| { | |||
| key: CredentialsKeyEnum.Region, | |||
There was a problem hiding this comment.
@suplere do we need to use CredentialsKeyEnum.Domain here as we use this.apiVersion = config.domain; in the provider?
There was a problem hiding this comment.
Yes, I have updated this to be apiVersion and passed the proper info from the handler
| }, | ||
| } | ||
| : { include_player_ids: options.target }; | ||
|
|
||
| const notification = this.transform(bridgeProviderData, { |
There was a problem hiding this comment.
@L-U-C-K-Y, do you know by chance if the API body signature is similar to both modes and API endpoints?
There was a problem hiding this comment.
I'm just wondering if we even need to change the BASE URL at all
There was a problem hiding this comment.
@scopsy yes that would still be required if we want to keep "include_player_ids" in the older model.
Your change looks all good for me 😄
v9
See here the API Documentation for v9 (old):
https://documentation.onesignal.com/v9.0/reference/create-notification
V9 does not have the key include_aliases (was named: include_external_user_ids and still had the concept of player ids and therefore had include_player_ids.
v11.6
And here 11.6 (new):
https://documentation.onesignal.com/reference/push-notification
V11.6 has include_aliases where we can send the external ids, but no include_player_ids.
In the new versions, player ids are not used anymore and the recommended way is the external id:
https://documentation.onesignal.com/docs/users
It is always recommended to use the External ID to identify users because this is the only way to track a user across all their subscriptions.
| ? { | ||
| include_aliases: { | ||
| external_id: options.target, | ||
| }, | ||
| } |
There was a problem hiding this comment.
| ? { | |
| include_aliases: { | |
| external_id: options.target, | |
| }, | |
| } | |
| ? { | |
| include_aliases: { | |
| external_id: options.target, | |
| }, | |
| target_channel: 'push', | |
| } |
I just noticed in the docs that we would need to include the target_channel for the new API endpoint:
There was a problem hiding this comment.
Yes this si good addition target channell pu#h
| include_aliases: { | ||
| external_id: ['userId'], | ||
| }, |
There was a problem hiding this comment.
| include_aliases: { | |
| external_id: ['userId'], | |
| }, | |
| include_aliases: { | |
| external_id: ['userId'], | |
| }, | |
| target_channel: 'push', |
I just noticed in the docs that we would need to include the target_channel for the new API endpoint:
|
Is there some problem ? |
|
@suplere could you take a look on @L-U-C-K-Y suggestions? |
| ? { | ||
| include_aliases: { | ||
| external_id: options.target, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Yes this si good addition target channell pu#h
fix: add onesignal target_channel push
|
@scopsy changes are merged, LGTM! 😄 |
What changed? Why was the change needed?
This PR add possibility to Onesignal provider choose between:
a) player model
b) user model
User model documentation
Expand for optional sections
Special notes for your reviewer