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.

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

[Bug]: Can't create notifications targeting a list of external ids #66

Closed
1 task done
remiremi opened this issue Aug 15, 2023 · 29 comments
Closed
1 task done

[Bug]: Can't create notifications targeting a list of external ids #66

remiremi opened this issue Aug 15, 2023 · 29 comments
Labels
bug Something isn't working

Comments

@remiremi
Copy link

remiremi commented Aug 15, 2023

What happened?

The REST API makes it possible to send notifications to a list of users given their external ids: https://documentation.onesignal.com/reference/create-notification#platform-to-deliver-to

However the sdk doesn't seem to make it possible, because the field include_aliases expects an object of type PlayerNotificationTargetIncludeAliases, which definition doesn't make it possible to define the external_id field.

This is what the serialized JSON for include_aliases should look like according to REST API documentation

  "include_aliases": {
    "external_id": ["external_user_id_on_push_email_sms"]
  },

Steps to reproduce?

Install latest version of sdk.

To try and make the call with the SDK, run:

    const notification = new OneSignal.Notification()
    notification.app_id = this.appId
    notification.include_aliases = new OneSignal.PlayerNotificationTargetIncludeAliases();
    notification.include_aliases.external_id = ["my-id-1", "my-id-2"]

Typescript will complain that

"Property 'external_id' does not exist on type 'PlayerNotificationTargetIncludeAliases'.ts(2339)"

What did you expect to happen?

I expect the SDK to have the correct type definition of PlayerNotificationTargetIncludeAliases so that the API call can be made with the correct parameters.

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@remiremi remiremi added the bug Something isn't working label Aug 15, 2023
@remiremi remiremi changed the title [Bug]: Can't create notifications targeted to a list of external ids [Bug]: Can't create notifications targeting a list of external ids Aug 15, 2023
@alexrabin
Copy link

alexrabin commented Aug 17, 2023

I'm getting this error as well. The typescript types are wrong

@iAmWillShepherd
Copy link

@remiremi @alexrabin could you share which version of the SDK you're using?

@alexrabin
Copy link

@remiremi @alexrabin could you share which version of the SDK you're using?

I am using 2.0.1-beta2

@iAmWillShepherd
Copy link

@remiremi

I am using 2.0.1-beta2

I suspected that, but I just wanted to confirm. Thanks for the prompt reply 🙏🏽

@iAmWillShepherd
Copy link

I'm working on reproducing this now in my sample project. Once I reproduce there, I can start to build additional context to understand what's going wrong and how we can fix our typings.

@alexrabin
Copy link

@iAmWillShepherd thanks for looking into this!

@iAmWillShepherd
Copy link

iAmWillShepherd commented Aug 31, 2023

I reproduce this issue, and the team is aware of the problem. However, I have little insight into when a fix will be available.

╭─iamwill@kronos ~/code/@onesignalDevelopers/OneSignal-Node-Sample ‹main●› 
╰─$ ./bin/dev notification create -A 202d4f61-1ca9-42df-9d36-bb17d8613abf --name "Include EUID test" -- "Did it work"
Sending push notification... done
{
  "id": "",
  "errors": {}
}

@remiremi

notification.include_aliases.external_id = ["my-id-1", "my-id-2"]

You're 100 percent correct that this isn't possible. The only property you can access is alias_label.

Did you try including EIDs via the Notification.include_external_user_ids property?

const notification = new OneSignal.Notification()
notification.include_external_user_ids = ['id1', 'id2', 'id3']

@alexrabin
Copy link

alexrabin commented Aug 31, 2023

@iAmWillShepherd Yes I am however the documentation says the attribute is deprecated:

DEPRECATED: Use include_aliases with target_channel instead.

https://documentation.onesignal.com/reference/create-notification#send-to-specific-devices

@iAmWillShepherd
Copy link

@iAmWillShepherd Yes I am however the documentation says the attribute is deprecated:

DEPRECATED: Use include_aliases with target_channel instead.

https://documentation.onesignal.com/reference/create-notification#send-to-specific-devices

They are indeed being deprecated, however, they should still be available to use until we ship an updated SDK.

@alexrabin
Copy link

@iAmWillShepherd Yes the attribute does currently work but I'd rather not use an attribute that is deprecated.

@iAmWillShepherd
Copy link

@iAmWillShepherd Yes the attribute does currently work but I'd rather not use an attribute that is deprecated.

I understand. However, the property is not deprecated yet. It's marked as Deprecating, so it's a warning that it will be deprecated in the future and subsequently not supported some time after the official deprecation takes place.

If you're OK with waiting indefinitely, we will ship an SDK that enables you to avoid using the deprecated property. My suggestion was meant to unblock you today. A fix will likely not be released at any point this week.

@alexrabin
Copy link

@iAmWillShepherd I understand. Thank you

@kpturner
Copy link

I am stuck on the same problem, but I want to include the onesignal_id alias rather than external_id.
What is the work around for that please?

@jpignata
Copy link

jpignata commented Oct 14, 2023

They are indeed being deprecated, however, they should still be available to use until we ship an updated SDK.

@iAmWillShepherd Available, yes, but not workable in production. The invalid_external_user_ids has been removed from errors in the response and moved to warnings according to the docs. This appears to have changed on 6/2 from our logs.

Additionally, invalid_aliases doesn't exist on Notification200Errors. As I mentioned, invalid_external_user_ids does but your API doesn't even seem to return this anymore from my testing and your docs. It's now in warnings which doesn't appear to be accessible through the SDK.

What's the ETA for this being wired up properly?

Thanks!

@manueltwistag
Copy link

This is still an issue, We've tried to do the migration today but faced the type issues described in the comments. Any ETA on this?

@emawby
Copy link
Contributor

emawby commented Oct 19, 2023

This SDK is undergoing another overhaul with more clear types and method names. The development for it is in progress and active and this issue is a priority

@abeisleem
Copy link

I see the development in progress and commenting to add more visibility as I too am blocked by this issue. Thanks!

@jpignata
Copy link

Heya - any progress here to report? I'd like to resume being able to report on failed push notifications.

@jpignata
Copy link

jpignata commented Dec 1, 2023

@emawby Sorry to be a pain. Do you have an expected release date, so I can fix the bug on my end that this change caused?

@padejar
Copy link

padejar commented Dec 27, 2023

Will it be released anytime soon?

@michalstruck
Copy link

@iAmWillShepherd @emawby any update? The situation is very unclear

@deepadoshi
Copy link

I am also blocked on this. Using include_aliases with external_id does nothing and include_external_user_ids gives a Bad Request HTTP error.

@kpturner
Copy link

It's beyond belief that an issue that's 5 months old has had no progress. I'm inclined to believe this repo isn't maintained, which is understandable for some open source projects but this is supposed to be OneSignal's official node SDK 🤷‍♀️

@jfishman1
Copy link

These are still the most up-to-date comments of this post:

#66 (comment)

#66 (comment)

These backend SDKs are going to take more time so they can be implemented correctly.

For now, if you want to target External IDs, continue using the Notification.include_external_user_ids property.

@MatheusLima7
Copy link

I received this generic error when I try to create a notification:

CreateNotificationSuccessResponse { id: '', errors: Notification200Errors {} }

I read that it is necessary to create a segment. It is possible to insert an array of ids in segment and after creating a notification it is necessary to use the id of segment how reference, but my account is free and it is impossible to create segment in free account. If someone to found the solution, please, talk me.

@kpturner
Copy link

kpturner commented Mar 6, 2024

I've never had to create a segment to send notifications. There is a default segment for everything anyway AFAIK

@MatheusLima7
Copy link

I've never had to create a segment to send notifications. There is a default segment for everything anyway AFAIK

I am trying a method of resolve the problem, because include_external_user_ids not works for me.

@EinfachHans
Copy link

This should work with 5.0.0-alpha.1 now.

The important change is this:
86801be#diff-2541f67df3dacc0f2f4754190bdf4b5ab7670412a7c8fb9d6994220a077106feR63-R66

But worth to mention about this new alpha version:

Documentation on API changes is lacking and under progress.
The README is outdated for this release.

@nan-li
Copy link
Contributor

nan-li commented May 9, 2024

Hi everyone, thanks again for reporting and for your patience.

As @EinfachHans mentioned, we recently released an alpha version of our latest user model API: Release 5.0.0-alpha-01 that addresses this and other issues.

Please read the release description for how to get started, and we appreciate any early feedback on this release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests