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

feature/refactor #29

Closed lsmolic closed 4 years ago

lsmolic commented 5 years ago

This draft PR is a suggested refactor. I have addressed some of the concerns that the community has expressed, most important being the ability to see and review ALL errors returned from the Expo API. To do this in a non-destructive, backwards compatible way, I have marked publish for deprecation and added a new method send_messages.

Here are the major changes to logic:

1) I decided to raise an exception for all 400 and 500 level api response errors. This way we know nothing has sent and we should handle that accordingly in our code. This allows us to treat errors at the message level more individually. send_messages actually returns the ResponseHandler so we can access any number of properties on it. I believe this satisfies #21

2) Additionally, I've added the ability to call for receipts. verify_deliveries which accepts a list of receipt_ids (This is actually a requirement listed in the docs: DeviceNotRegistered: the device cannot receive push notifications anymore and you should stop sending messages to the corresponding Expo push token. -- we should all be addressing these.

3) Any invalid push_tokens that are flagged on the initial call are thrown into its own array so they can be handled. Not sure how often this happens, but I have actually experienced it with changing the name of an active app. Really don't recommend that, but there are cases that we would want to identify failures and pull these tokens out of rotation.

4) I tried to catch all the edge cases for bad return values from Expo's servers. I was personally getting a lot of JSON parsing errors and it would seriously impact my apps. I believe this satisfies #14

5) Added an example folder to better help newbies get up and going faster

6) Addressed a core issue in using the gzip: true attribute. Apparently the Typhoeus webclient doesn't have you pass that as a header, it's just a regular attribute on the Request.post() call. So, this is fixed and I'm not getting 100% proper gzip responses.

7) Added Rubocop to ensure we have similar formatting going forward.

Looking forward to your feedback.

lsmolic commented 5 years ago

quick note that I didn't make very clear earlier: This doesn't change existing behavior. It just marks the previous methods as deprecated and throws a warn on the publish method.

jonsgreen commented 5 years ago

@lsmolic Thank you for your efforts here. I may not be able to look at this in the near future but hope that my client will want to support me trying it out at some point. I just wanted to at least express my gratitude in the meantime.

lsmolic commented 5 years ago

Your kind words are heartfelt. I hope it helps make your tasks easier if you have the opportunity. Cheers!

On Tue, May 28, 2019 at 11:01 AM Jonathan notifications@github.com wrote:

@lsmolic https://github.com/lsmolic Thank you for your efforts here. I may not be able to look at this in the near future but hope that my client will want to support me trying it out at some point. I just wanted to at least express my gratitude in the meantime.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/expo/expo-server-sdk-ruby/pull/29?email_source=notifications&email_token=AAEFV3FOFJXE26EUHPXR3L3PXVXP7A5CNFSM4HQAIFY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWM6JKA#issuecomment-496624808, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEFV3AQDM3HVBEPMKSCQ5DPXVXP7ANCNFSM4HQAIFYQ .

courtsimas commented 4 years ago

Any progress on this?

lsmolic commented 4 years ago

Sadly no. That’s my bad. I can close it and reopen when I have more time to commit

On Jan 14, 2020, at 08:06, Court Simas notifications@github.com wrote:

 Any progress on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

lsmolic commented 4 years ago

@audiolion @courtsimas @jonsgreen Okay fam. I addressed the concerns and then some.

I'm not sure the best option with handling >100 messages. For now, we'll throw an exception. My main concern with doing some sort of bulk processing in the library is that the responses will each have their own status, body, and errors. I don't think we'd want to obfuscate that much, and until we have a better plan, I'd like to move forward and merge this in.

Anyone want to give it a try locally?

gem 'exponent-server-sdk', git: 'https://github.com/lsmolic/expo-server-sdk-ruby.git', branch: 'feature/refactor'

audiolion commented 4 years ago

I unfortunately no longer am at the same company where I was working on this project to test it out, sorry 😞

lsmolic commented 4 years ago

@pablonahuelgomez do you have rights to review and merge?

pablonahuelgomez commented 4 years ago

@lsmolic sure man, thanks for this.

lsmolic commented 4 years ago

@pablonahuelgomez Thank you for the merge. What is your process for publishing the new version https://guides.rubygems.org/publishing/#publishing-to-rubygemsorg ? I'm assuming one of the maintainers does the publish.

pablonahuelgomez commented 4 years ago

@lsmolic the gem has been published. Thanks.