Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Notifier/OneSignal] Fix external player IDs#53248
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
norkunas commentedDec 28, 2023
OneSignal deprecated the old way, but it's not removed yet? It was implemented before it based on their older api |
fabpot commentedDec 28, 2023
That should be done in 7.1 (on lower branches, we cannot deprecate anything nor add new features). |
KDederichs commentedDec 28, 2023
yeah, but even the old way the parameter was called |
OskarStark commentedDec 28, 2023
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. |
norkunas commentedDec 28, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 tofd2b111CompareKDederichs commentedDec 28, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
norkunas commentedDec 28, 2023
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.. |
KDederichs commentedDec 28, 2023
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? |
norkunas commentedDec 28, 2023
Totally |
KDederichs commentedDec 28, 2023
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 | MITAs 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.