[Notifier/OneSignal] Fix external player IDs #53248
Conversation
|
OneSignal deprecated the old way, but it's not removed yet? It was implemented before it based on their older api |
|
That should be done in 7.1 (on lower branches, we cannot deprecate anything nor add new features). |
yeah, but even the old way the parameter was called |
|
We need to split this PR, 5.4 should fix the used key, while 7.1 should introduce the new isExternal method and deprecate the old method. |
I see now, so external ids were not implemented initially,only old player ids, trully not sure why that method is there Edit: I found this in v9 docs: |
…nclude_subscription_ids
4458ce5 to
fd2b111
Compare
|
Those docs are a strange mess... at the top it says this: So... both work? Edit: Oh seems like it's something different, seems like the |
|
I think that this option is not for an user id,but instead custom id for deduplication,so it should not be removed 🤔 yeah hard to understans their docs.. would be easier if they would have multiple endpoints for notifications based on needed filters.. |
|
So, since it seems to be a new feature after all I'd assume the right course of action would be to close this and re-open a PR against 7.1 right? |
|
Totally |
|
Ok, closing this then and preparing one for 7.1 |
…rnal user ids (KDederichs) This PR was merged into the 7.1 branch. Discussion ---------- [Notifier] [OneSignal] Add support for sending to external user ids | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | yes <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | Fix #50779 | License | MIT As discussed in #53248, here's the feature PR against 7.1. This introduces a new `isExternalUserId()` option to indicate that the receiver is an external user id. At the same time it also replaces the deprecated `include_player_ids` option. Commits ------- 533a831 Add support for sending to external user ids
After digging into it I found that the original implementation seems to plan for external players but the implementation seems like it never would have worked, since the parameter was called
include_external_user_idsand notexternal_id.Looking at their docs, you can also see they recently changed their API parameters, so I took the chance to just go ahead and update to the new parameters.
Since the new one is a bit more complicated to set external IDs I ended up marking the old method as deprecated and adding a new option.
I think this should be better since external ids and subscription ids should be mutually exclusive.
See: https://documentation.onesignal.com/reference/create-notification
I didn't have time to test it yet so it's a draft for now, so please someone feel free to check it out, otherwise I'll try to real world test it in the coming days.