urbanairship / ruby-library

A Ruby wrapper for the Urban Airship API.
Other
200 stars 117 forks source link

Fix ScheduledPush.from_url #107

Closed alecspopa closed 6 years ago

alecspopa commented 6 years ago

When trying to create a ScheduledPush using the schedule_url of a notification the library was failing because the response was changed in the API.


Steps to reproduce:

notification_schedule_url = "https://go.urbanairship.com/api/schedules/XXXXXXXX-XXXX-..."
client = Urbanairship::Client.new(key: ENV['UA_APP_KEY'], secret: ENV['UA_APP_MASTER_SECRET'])
Urbanairship::Push::ScheduledPush.from_url(client: client, url: notification_schedule_url)
pdxmele commented 6 years ago

Thanks! We'll take a look at this soon.

danielvidal commented 6 years ago

@pdxmele Could you take a look at this PR, pls?

pdxmele commented 6 years ago

We are looking at this now.

pdxmele commented 6 years ago

Hm, the lib throws a test failure after I merge in your changes. Try running "rake":

Failures:

  1) Urbanairship::Push Urbanairship::Push::ScheduledPush#from_url loads an existing scheduled push from its URL
     Failure/Error: p.audience = payload['body']['push']['audience']

     NoMethodError:
       undefined method `[]' for nil:NilClass
     # ./lib/urbanairship/push/push.rb:109:in `from_url'
     # ./spec/lib/urbanairship/push/push_spec.rb:246:in `block (4 levels) in <top (required)>'

Finished in 0.09869 seconds (files took 0.8328 seconds to load)
198 examples, 1 failure

Failed examples:

rspec ./spec/lib/urbanairship/push/push_spec.rb:222 # Urbanairship::Push Urbanairship::Push::ScheduledPush#from_url loads an existing scheduled push from its URL

Looks like the test needs to be updated as well. But this is all a bit odd to me since I don't think this response has changed at all in API v3.

alecspopa commented 6 years ago

Hi @pdxmele, The response is mocked so it either needs to be changed or I was wrong and the API response does not respond with a 'body' in the payload. I'll try to test this a bit more but I don't have access to an UA account so I cannot schedule notifications anymore. Do you have some test client key and secrets that you can share?

alecspopa commented 6 years ago

I think I managed to see why the response contains the 'body'. client.send_request (lib/urbanairship/push/push.rb:101) is a Urbanairship::Client.build_response and has everything in it (code, headers, body), however the old code tried to read it as it was the body of the response only payload = JSON.load(response_body) which is not true.

pdxmele commented 6 years ago

Thanks! I managed to get the test fixed. I'll get that + your changes merged into the private version of the repo and release the fix soon, hopefully early next week.

alecspopa commented 6 years ago

Thanks @pdxmele. I didn't want to fix the tests because I was not sure if this was indeed wrong.