search

expo-community / expo-server-sdk-ruby

A Ruby library for sending push notifications with Expo's notification service
MIT License
95 stars 53 forks source link

Bugfix datatype where hash is treated as an array #23

Closed PrimeTimeTran closed 5 years ago

PrimeTimeTran commented 5 years ago

This PR resolves a datatyping error where the response is a Hash instead of an Array.

When the response object is an array, the current implementation works. However, there is a case in which the response is a Hash, in which case the method

ResponseHandler.extract_data

grabs only the first key => value pair from the object in conjunction with turning it into an array, i.e.

["id", "5ff3829e-48b2-40c3-8559-7240697ee912"]

This results in an error being thrown in this method.

ResponseHandler.validate_status
TypeError (no implicit conversion of String into Integer):

Screen shot of error:

screen shot 2019-01-08 at 11 03 59 am

Working implementation after update:

screen shot 2019-01-08 at 11 04 30 am

Please let me know if there's anything else you'd need to get this PR merged.

Thanks!

lsmolic commented 5 years ago

tests?

PrimeTimeTran commented 5 years ago

tests?

I can add them if they're required for a merge. I didn't feel confident writing them yet though because I'm not aware of all the edgecases.

lsmolic commented 5 years ago

Well, even a few that test positive and negative expectations. It’s not really for you, but whoever tries to use this function. Helps to clarify that it can take both an array and a single object. Usually the accepted practice with OSS in my experience.

Cheers!

On Jan 9, 2019, at 00:31, Loi Tran notifications@github.com wrote:

tests?

I can add them if they're required for a merge. I didn't feel confident writing them yet though because I'm not aware of all the edgecases.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

ide commented 5 years ago

Looks better than before but I think in the future the API needs a breaking change so that it doesn't throw if just one notif failed. Also there's no support for the async receipts API. The docs https://docs.expo.io/versions/latest/guides/push-notifications#http2-api and the JS reference are probably the best place to look https://github.com/expo/expo-server-sdk-node/blob/master/src/ExpoClient.ts