[Bug]: Unable to push notifications to users within segment

iAmWillShepherd commented 2 years ago

What happened?

I built the following sample app to push notifications to all users within a segment, but unfortunately, I was unable to get it to work.

require 'dotenv/load'
require 'time'
require 'onesignal'

rest_api_key = ENV['ONESIGNAL_REST_API_KEY']
app_id = ENV['ONESIGNAL_APP_ID']

OneSignal.configure do |config|
  config.app_key = rest_api_key
end

api_instance = OneSignal::DefaultApi.new

begin
  notification = OneSignal::Notification.new({ app_id: app_id })
  notification.contents = OneSignal::StringMap.new({ "en": 'English Message' })
  notification.included_segments = ['Include_segment']
  p notification

  notification_response = api_instance.create_notification(notification)
  notification_id = notification_response.id
  p notification_response
rescue OneSignal::ApiError => e
  puts "Error when calling DefaultApi->create_notification: #{e}"
end

FWIW, @jmadler and I did a little debugging to confirm that my segment did indeed have users who were subscribed to push, but still got the same error. I also tried several different apps and other segments and got the same results.

Steps to reproduce?

1. Change the `rest_api_key` and `app_id` to an app that makes use of segments
2. Run the script I shared
3. Note that you receive `All included players are not subscribed` as an error message

What did you expect to happen?

I expected a push notification to be sent to my device.

Relevant log output

#<OneSignal::Notification:0x00000001072d9440 @is_ios=true, @app_id="8152db8f-1417-48a4-b8db-264a744b87a3", @contents=#<OneSignal::StringMap:0x00000001072d9170 @en="English Message">, @included_segments=["Subscribed Users"]>
#<OneSignal::InlineResponse200:0x0000000107430bb8 @id="", @recipients=0, @errors=["All included players are not subscribed"]>

Code of Conduct

[X] I agree to follow this project's Code of Conduct

kesheshyan commented 2 years ago

Hey @iAmWillShepherd, thanks for reporting this. It seems you also need to specify:

notification.is_chrome_web = true
notification.is_any_web= true

I'm not sure why but that is the way our backend works.

iAmWillShepherd commented 2 years ago

Hmm, that is interesting 🤔

I think we should keep this issue open with the action item to update the docs for this project to highlight these quirks. What do you think?

jkasten2 commented 2 years ago

I think this should be documented as well, since these are not considered required fields today by this SDK's defined API.

Also maybe we can enforce at least one is_ value is set to true and report that as an error before attempting to make the network call? Or maybe even better, the REST API itself can return a more helpful error when this happens.

Background on how the REST API works today: If you omit all is* properties today from the REST API it assumes all are true, however if you pass any is* flags all others are defaulted to false.

Lastly, in a 2.0.0 major release, we could make all these true by default (or omit them to give the same effected as noted above)

OneSignal / onesignal-ruby-api