Skip to content

Adds Pusher beams and code that enables push notifications handling #68

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

Merged
merged 6 commits into from
Apr 23, 2020

Conversation

fdocr
Copy link
Contributor

@fdocr fdocr commented Mar 25, 2020

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Documentation Update

Description

Includes Pusher Beams support and subscribes to channels in order to receive Push Notifications. This is my first Kotlin PR (ever) so any comments/suggestions for improvement are greatly appreciated 😄

🚨🚨IMPORTANT NOTES🚨🚨

  1. We need to replace app/google-services.json with one using the DEV Firebase production account to make sure the PN are sent on Production.
  2. A PR in the main web repo is necessary too (submitting soon) in order to send PN (only Apple PN are configured at the moment)

Related Tickets & Documents

Push Notification issue

[optional] What gif best describes this PR or how it makes you feel?

Neo i know kung fu

(After diving in Kotlin and getting Push Notifications to work locally)

Copy link

@joshpuetz joshpuetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions inline, but I'm not sure how blocking they are

{
"project_info": {
"project_number": "731685995596",
"firebase_url": "https://dev-personal-a792e.firebaseio.com",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is exposing this information ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not 😳 luckily that's my personal (free) account for now and I revoked the keys.

I had a sync with Mac and talked about this earlier today. So far the best solution I can think of would be to add this file to .gitignore and add a walkthrough on the README so any contributor can set these up on their own.

Definitely sorting this out before merging 👍

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fdoxyz One thing I did in the past was checking this file in encrypted and sharing the decryption key out of band. The way it worked was like this:

  1. A rake task that used the encryption key from the environment to create an encrypted version of the credentials so they can be checked in:
namespace :google_credentials do
  desc 'Creates encrypted version of Google service account JSON file'
  task encrypt: :environment do
    key = ENV.fetch('GOOGLE_CREDENTIALS_KEY')
    infile = File.join(Rails.root, 'config', 'google_credentials.json')
    outfile = File.join(Rails.root, 'config', 'google_credentials.json.enc')

    json = JSON.dump(JSON.parse(File.read(infile))) # remove newlines etc.
    crypt = ActiveSupport::MessageEncryptor.new(key)
    File.open(outfile, 'w') { |f| f.write(crypt.encrypt_and_sign(json)) }
  end
end
  1. A method in the app that returned the encrypted key as a StringIO object:
    def json_key_io
      encrypted = File.read(CREDENTIALS_FILE)
      key = ENV.fetch('GOOGLE_CREDENTIALS_KEY')
      crypt = ActiveSupport::MessageEncryptor.new(key)
      StringIO.new(crypt.decrypt_and_verify(encrypted))
    end

This worked really well for us and did make development quite a bit easier. I know this is Kotlin not Ruby, but maybe a similar approach could help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's definitely a clever and interesting idea @citizen428, I really appreciate the input! I don't think it may work as easily though, because every contributor will need it in order to compile the project locally (the library fails if it can't parse the JSON values).

So far the idea I had in place was for each contributor to create their own Firebase account (at least it's free) and download their own google-services.json, it doesn't have to be hooked up to any Pusher instance to compile. I added some instructions in the README to cover this and it might even be a good learning experience for anyone that has never seen that process before.

An alternative that came to mind was to share a public google-services.json with credentials to a "dev" Firebase app, just so everyone can work in their local environments (again no need to have it hooked up to a working Pusher instance). Adding a release flavor and having that release/google-services.json file in .gitignore would make it "easier"/"safer" for the production credentials file. The problem with this would be that anyone working with Push Notifications in development would still have to override the development version of this file, and it's pretty easy for any of them to share their version of the credentials with an eventual PR.

Still unsure if this alternative is better than the current or if any other options are available. I'll try to see if I can make the release flavors work since it's a bit easier for new contributors to start working on the project. Aside from this credential setup decision and a git conflict that I'll sort out soon I think the PR might be ready 🙂

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it may work as easily though, because every contributor will need it in order to compile the project locally

My bad, I didn't realize the mobile apps are fully open to external contributors as well, my approach was for a small-ish in-house dev team.

@@ -28,6 +29,15 @@ class MainActivity : BaseActivity<ActivityMainBinding>(), CustomWebChromeClient.
setWebViewSettings()
savedInstanceState?.let { restoreState(it) } ?: navigateToHome()
handleIntent(intent)
PushNotifications.start(getApplicationContext(), "cdaf9857-fad0-4bfb-b360-64c1b2693ef3");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be extracted to an environment variable or something like it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to look into how to do it but it's a good idea

@fdocr fdocr changed the title Adds Pusher beams and code that enables push notifications handling [WIP] Adds Pusher beams and code that enables push notifications handling Mar 26, 2020
@fdocr
Copy link
Contributor Author

fdocr commented Mar 26, 2020

Thanks for the review @joshpuetz! Added WIP so we know it still needs some work

@fdocr fdocr changed the title [WIP] Adds Pusher beams and code that enables push notifications handling Adds Pusher beams and code that enables push notifications handling Apr 20, 2020
@@ -1,6 +1,7 @@
## dev.to configurations
chrome.baseUrl=https://dev.to/
chrome.userAgent=DEV-Native-android
pusher.instanceId=698717d2-30ff-4cf7-aac8-7d6efbd5f030
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this should we switched to the same one we are using in iOS

Copy link
Contributor

@maestromac maestromac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@maestromac maestromac merged commit ccf4aa2 into master Apr 23, 2020
@maestromac maestromac deleted the pusher-beams-support branch April 23, 2020 15:33
@pr-triage pr-triage bot added the PR: merged label Apr 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants