Skip to content
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

Push Notification - iOS & Android Updates! (Now includes FCM - Fuse 1.10+) #1220

Merged
merged 10 commits into from
Jul 30, 2019

Conversation

AndrewEQ
Copy link
Contributor

@AndrewEQ AndrewEQ commented Sep 27, 2018

19 Jun 2019 - iOS

Fixed push registration for iOS 8-9 devices

16 Nov - Android: Updated from GCM to FCM! :)

  • updated android GCM components with FCM components
  • added function to update token if refreshed
  • updated play services detection
  • updated MapView dependency (its too old!)
  • updated color & lightcolor to allow for a color with or without #
  • NB! This commit of updates is only compatible with the latest Fuselibs and Uno (1.10)
  • requires google-services.json from firebase project to be included in root of project
  • requires fuse copyfile command which can be done by making and including this file in project root:

Android.uxl

<Extensions Backend="CPlusPlus" Condition="Android">
    <CopyFile Condition="Android" Name="google-services.json" TargetName="app/google-services.json" />
</Extensions>

27 Sep - iOS & Android: Push Notification Fixes + Features

Android

Fixes

  • Added default Notification Channel, so won't have to compile for API 25 to get push notifications working, so you basically don't need to define anything to get it working but its a good practice to define specific channels for a user.
    • Can configure the default channel name via unoconfig

Features

  • Added setting of icon color / app name
    • Can set color via unoconfig & notification payload
  • Added the ability to hide the title via unoconfig

iOS

Fixes

  • Explicitly ask for permission so notifications won't be silent

Feature

  • Added function to get whether or not the user has push notifications turned on or off

28 Sep - Added Push Notification Features

Android

  • Notification Category
  • Notification Color
  • Notification Lockscreen Visibility
  • Notification Priority
  • Notification Sound
  • No Title Style

Android 8+

  • Notification Channel
  • Nofitication Channel Group
  • Notification Channel Importance
  • Notification Channel Lockscreen Visibility
  • Notification Channel Light Color
  • Notification Channel Sound
  • Notification Channel Show Badge
  • Notification Channel Vibration
  • Notification Badge Number
  • Notification Badge Icon Type

This PR contains:

  • Changelog
  • Documentation
  • Tests

@AndrewEQ AndrewEQ changed the title iOS & Android: Fixes + Features iOS & Android: Push Notification Fixes + Features Sep 27, 2018
Copy link

@ekkotech ekkotech left a comment

Choose a reason for hiding this comment

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

In iOS/Impl.uno, the code will work today but will fail when the deprecated method registerUserNotificationSettings is removed from iOS. At that time, the if (..respondsToSelector..) clause will not execute and the else clause will crash the app due to the no longer existing method call registerForRemoteNotificationTypes.
I would recommend that you test first for the existence of the new API (iOS10+) with:
UNUserNotificationCenter* center = [UNUserNotificationCenter currentNotificationCenter];
if ([center respondsToSelector:@selector(requestAuthorizationWithOptions:)]) {
// Now in iOS10+ land
[center requestAuthorizationWithOptions: ... same code ....]
else {
// Now in pre-iOS10 land
[application registerUserNotificationSettings: ....
}

Also, in the completion callback, if you add a test for granted then registration with APNS will not occur if the user declines to give permission:
[center requestAuthorizationWithOptions: (alert | sound | badge)
completionHandler: ^(BOOL granted, NSError * _Nullable error) {
if (granted) {
[application registerForRemoteNotifications];
}];

Copy link

@ekkotech ekkotech left a comment

Choose a reason for hiding this comment

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

For the Android changes for Oreo+ push notifications, there is an existing PR (#1197) waiting to be merged that addresses both the channel id and background task issue (JobIntentService).

How does this PR tally with #1197? Is it incremental to #1197?

@AndrewEQ
Copy link
Contributor Author

hey @ekkotech, thanks for the feedback, yeah, the goal of this pr was to enhance the existing code to get it functional as it really isn't working as it stands.

iOS Issue1: I like your fallback logic, I didn't write the original but I'll enhance it with yours.
iOS Issue2: I didn't do the check for granted because I still wanted to register the user (as in get a token and register with my server side) because then if they later turned it on via their general settings, push would work straight away in the app. The other way is for the user to turn on notifications in general settings, then "turn it on" in your app, which means you would need to call register again, then actually register the token with your server side, which I found clunky in terms of user experience.

My issue with #1197 : I couldn't get it to work. I really tried, maybe there's something different between our setups, ionno, and the PR didn't seem to be moving.
Android Issue1: background notification - I haven't had an issue yet with receiving background notifications with the current build, so I didn't touch that or am I misunderstanding the issue?
Android Issue2: Push not working in Android 8(Oreo)+ - So I made the necessary changes at first (adding a channel because its mandatory). I then added a ton of missing android push notification features (see list above).
Android Issue3: We need to move from GCM to FCM by April 2019; this still needs to be done, all the android push features I added can still be used whenever that happens, they could even be added to #1197 if that goes through.

So I would say the main differences are that I broke my PR's into separate bite-sized issue resolutions; for instance the map one is a critical breaking fix, so I don't see why that needs to be held up.

Ok, so the summary break down:

iOS fix - push notifications are silent right now because we don't explicitly ask for permission
iOS feature - you can get whether or not the users push is enabled (test twitter app to see this in use)

Android fix - Android 8+ compatibility
Android features - all the new and old missing push notification features

@AndrewEQ
Copy link
Contributor Author

@ekkotech I just updated the iOS check for versions older than 10

27 Sep - iOS & Android: Push Notification Fixes + Features
Android
Fixes

- Added default Notification Channel, so won't have to compile for API 25 to get push notifications working.
- Can configure the default channel name via unoconfig
Features

- Added setting of icon color / app name
- Can set color via unoconfig & notification payload
- Added the ability to hide the title via unoconfig

iOS
Fixes

- Explicitly ask for permission so notifications won't be silent
- Updated version check

Feature

- Added function to get whether or not the user has push notifications turned on or off

28 Sep - Added Push Notification Features

Android
- Notification Category
- Notification Color
- Notification Lockscreen Visibility
- Notification Priority
- Notification Sound
- No Title Style

Android 8+
- Notification Channel
- Nofitication Channel Group
- Notification Channel Importance
- Notification Channel Lockscreen Visibility
- Notification Channel Light Color
- Notification Channel Sound
- Notification Channel Show Badge
- Notification Channel Vibration
- Notification Badge Number
- Notification Badge Icon Type

1 Oct - Added documentation for how to implement the new android push notification features in fuse
@kusma
Copy link
Contributor

kusma commented Oct 1, 2018

Why was this closed?

@kusma kusma mentioned this pull request Oct 1, 2018
3 tasks
@kusma kusma reopened this Oct 2, 2018
@ekkotech
Copy link

ekkotech commented Oct 2, 2018

@AndrewEQ re. iOS issue2: I see where you are coming from and, from a technical standpoint it has merit, but if you look at it from a data security standpoint, I have two concerns. 1) You are leaking a user's data out to APNS when the user's expectation is privacy, 2) this is library code; applications built on top of this will get a response back from APNS and go ahead and push that out to their own application server. Now there is nothing stopping the user from getting push notifications when they said they didn't want them.

@AndrewEQ
Copy link
Contributor Author

AndrewEQ commented Oct 2, 2018

@ekkotech 1) I know what you're thinking but I've tested it, the notifications are turned off for the user when they say no in the dialog. 2) It was doing this, and still is before my updates. That's where I got the initial idea from. Difference is that the current implementation is silent even when the user says "yes".

From my tests and according to the Apple docs:

Scenario 1 + 1.1 - current code

Scenario 1:

  1. App requests registration
  2. System prompts user with dialog
  3. User selects yes
  4. Token is received, notifications are received but silently (no sound even if turned on)

Scenario 1.1:

  1. App requests registration
  2. System prompts user with dialog
  3. User selects no
  4. Token is received, notifications can be received but are not as the notification settings for the app are turned off

Scenario 2 + 2.1 - new code

Scenario 2:

  1. App requests permission
  2. System prompts user with dialog
  3. User selects yes
  4. App requests registration
  5. Token is received, notifications are received with sound

Scenario 2.1:

  1. App requests permission
  2. System prompts user with dialog
  3. User selects no
  4. App requests registration
  5. Token is received, notifications can be received but are not received as the notification settings for the app are turned off

@ekkotech
Copy link

ekkotech commented Oct 3, 2018

@AndrewEQ Question for you: when you were testing your code changes, what key did you use when sending to GCM? For example, here is the script that I use for pre-API26 tests:

`#!/bin/bash

curl -X POST
-H "Authorization: key=AAAACny-blah-blah-BQWtGsIl-1"
-H "Content-Type: application/json"
-d '{
"to": "APA9blah-blah-Xhr7e8",
"data": {
"notification": {
"channel": "some_channel_id" <<<<---- Is 'channel' correct? Should it be 'channelID'?
"alert": {
"title": "Alert title 4.0",
"body": "Alert body 4.0"
}
},
"title": "Test title 4.1",
"body": "Test body 4.1"
}
}'
https://android.googleapis.com/gcm/send
`

@AndrewEQ
Copy link
Contributor Author

AndrewEQ commented Oct 3, 2018

hey @ekkotech, this was the payload structure that I was testing with: https://github.com/fuse-open/fuselibs/pull/1220/files#diff-0aae1bdd92b0184bb9df83ff47aa3973R230

@AndrewEQ
Copy link
Contributor Author

AndrewEQ commented Oct 3, 2018

@ekkotech were you able to get your notification?

@ekkotech
Copy link

ekkotech commented Oct 3, 2018

@AndrewEQ - I didn't get to test your stuff today. I already had #1197 changes integrated so I tested that. I have another test slot coming up on Friday so I hope to test out your stuff then
BTW - my feedback for #1197 is up there so take a look...

@AndrewEQ
Copy link
Contributor Author

AndrewEQ commented Oct 3, 2018

cool @ekkotech, I think I shall retest sometime and see about combining the differences of #1197 with my update in a separate PR.

- updated android gcm components with fcm components
- added function to update token if refreshed
- updated play services detection
- updated MapView dependency (its too old)
- updated color & lightcolor to allow for with or without #
@AndrewEQ AndrewEQ changed the title iOS & Android: Push Notification Fixes + Features iOS & Android: Updates! Nov 16, 2018
@AndrewEQ
Copy link
Contributor Author

Updated with FCM support! 🎉

@AndrewEQ AndrewEQ changed the title iOS & Android: Updates! iOS & Android Push Notification Updates! Nov 16, 2018
@AndrewEQ AndrewEQ changed the title iOS & Android Push Notification Updates! Push Notification - iOS & Android Updates! Nov 16, 2018
@AndrewEQ AndrewEQ changed the title Push Notification - iOS & Android Updates! Push Notification - iOS & Android Updates! (Includes FCM - Fuse 1.10+) Nov 16, 2018
@AndrewEQ AndrewEQ changed the title Push Notification - iOS & Android Updates! (Includes FCM - Fuse 1.10+) Push Notification - iOS & Android Updates! (Now includes FCM - Fuse 1.10+) Nov 16, 2018
API Compatibility and auto included file for android to find.
@AndrewEQ AndrewEQ requested a review from ckarmy July 22, 2019 22:52
Copy link
Member

@ichan-mb ichan-mb left a comment

Choose a reason for hiding this comment

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

I think you should create another PR for map fix because that fix is nothing to do on what PR title said

@AndrewEQ
Copy link
Contributor Author

I think you should create another PR for map fix because that fix is nothing to do on what PR title said

I guess I needed it as well at the time cos maps was needed too, else the compilation broke without it.

@ichan-mb
Copy link
Member

I think you should create another PR for map fix because that fix is nothing to do on what PR title said

I guess I needed it as well at the time cos maps was needed too, else the compilation broke without it.

Ah, I see. But since there is an issue posted on forums about MapView is crashing when targeting Android 9, and the fix for it is on this PR by this commit. But this PR is not yet merged by reviewers. So my recommendation is to create another PR just to fix the MapView Issue. Since the fix is so simple, I think that PR will merge immediately by @kusma

@AndrewEQ
Copy link
Contributor Author

@ichan-mb @kusma sure thing man: #1284

Copy link
Contributor

@ckarmy ckarmy left a comment

Choose a reason for hiding this comment

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

Tested on Android and iOS

@ckarmy ckarmy merged commit ae566ac into fuse-open:master Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants