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

Initial implementation of android GCM push notifications #637

Merged
merged 5 commits into from Jun 16, 2017

Conversation

Projects
None yet
5 participants
@kunall17
Contributor

kunall17 commented May 25, 2017

Using https://github.com/wix/react-native-notifications

  • Registers the GCM over the server
  • Use the incoming messages to pop some notifications
  • Handle the notifications properly when notification is pressed
@smarx

This comment has been minimized.

smarx commented May 25, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented May 29, 2017

Great. That is a high-priority feature and we need to work on this and finish it next.

@zulipbot zulipbot added reviewed and removed needs review labels May 29, 2017

@borisyankov

Nice start.
Please do the changes and test to make sure it works.

@@ -32,6 +32,7 @@
android:name="io.fabric.ApiKey"
android:value="[YOUR API KEY]"
/>
<meta-data android:name="com.wix.reactnativenotifications.gcmSenderId" android:value="835904834568"/>

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

Make sure to register a new ID with Google next.

@@ -18,6 +20,15 @@ export default class ZulipMobile extends Component {
restore(() => {
this.setState({ rehydrated: true });
});
if (Platform.OS === 'android') {
unregisterGCM(this.props.auth.gcmToken);

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

Is unregister on 'mount' correct? Shouldn't it be the opposite?

componentWillUnmount() {
if (Platform.OS === 'android') {
registerGCM(this.props.auth.gcmToken);

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

Same for this: 'unmount' + register?

@@ -39,3 +40,8 @@ export const loginSuccess = (realm: string, email: string, apiKey: string) => ({
export const logout = () => ({
type: LOGOUT,
});
export const saveGCM = (gcmToken) => ({

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

Where do you intend to call this from?
The name should match more closely the action. So saveTokenGCM?

@@ -6,6 +6,7 @@ import {
ACCOUNT_REMOVE,
LOGIN_SUCCESS,
LOGOUT,
SAVE_GCM_TOKEN

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

You might want to call it SAVE_TOKEN_GCM as there will be another SAVE_TOKEN_APNS or similar.
In this naming, follow with the main action (save token) and then the details (GCM) though it is not how you would order it if you speak.

@@ -90,6 +91,12 @@ export default (state: AccountState = initialState, action: Action) => {
newState.splice(action.index, 1);
return newState;
}
case SAVE_GCM_TOKEN: {
return [
{ ...state[0], gcmToken: action.gcmToken },

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

Please, don't forget to write tests for that action.

case SAVE_GCM_TOKEN: {
return [
{ ...state[0], gcmToken: action.gcmToken },
state.splice(1, state.length - 1)

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

splice is incorrect. It mutates the list. You might mean slice.
If you had tests this would happen less frequently ;)

@@ -15,6 +15,7 @@ export const getAuth = (state: StateType) => {
apiKey: undefined,
email: undefined,
realm: undefined,
gcmToken: ''

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

This should not be part of the 'auth' structure.
We use it as a something the api needs to authenticate and nothing else will be added to it.

@@ -0,0 +1,16 @@
import { NotificationsAndroid } from 'react-native-notifications';

This comment has been minimized.

@borisyankov

borisyankov May 29, 2017

Contributor

This probably belongs to a /utils/notifications.js file?

@neerajwahi

This comment has been minimized.

Member

neerajwahi commented May 30, 2017

Give me a chance to review this PR before merging

@zulipbot zulipbot added needs review and removed reviewed labels Jun 7, 2017

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jun 11, 2017

Looks reasonable from code perspective, but does throw.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 11, 2017

Yeah, will give it a complete test currently waiting for the sender key,
@timabbott Does the dev env supports the push notifications or its only for the production?

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 13, 2017

Update: Got the notifications working, got the notifications coming on the phone with the old sender ID. I have no clue why this is working with the old sender id, but glad it works the notifications popups up, just need to handle this more correctly!

06-13 08:25:53.269 19886-19944/com.zulipmobile D/ReactNativeNotifs: New message from GCM: Bundle[{sender_full_name=Watson, google.sent_time=1497322553574, sender_avatar_url=https://secure.gravatar.com/avatar/7b82892ab28701cc6bfe7798b3498eb7?d=identicon&version=1, content_truncated=false, zulip_message_id=225696, recipient_type=private, time=1497322553, user=kunall.gupta17@gmail.com, alert=New private message from Watson, event=message, google.message_id=0:1497322553585267%46869b2df9fd7ecd, content=test, sender_email=watson43517@gmail.com}]

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 13, 2017

Done (most of the code taken from native java project) :)
Up for a review @borisyankov @neerajwahi
screenshot_20170613-091641

@kunall17 kunall17 changed the title from [WIP] Initial implementation of android GCM push notifications to Initial implementation of android GCM push notifications Jun 13, 2017

@borisyankov

Looks close to being done. Good.

import com.smixx.fabric.FabricPackage;
import com.crashlytics.android.Crashlytics;

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

Why remove these?

super.onCreate();
SoLoader.init(this, /* native exopackage */ false);
//Fabric.with(this, new Crashlytics());
}
}

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

The diff of the whole file is too random, while the actual changes seem to be few. Try not doing this.

}
return null;
}
}

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

Can we extract part of that logic to the JS side?

This comment has been minimized.

@kunall17

kunall17 Jun 14, 2017

Contributor

So the problem is, the library has a service running which receives the GCM notification and displays the notification it self, hence there is no js code manipulating the notification data!
Therefore I'm facing a problem with avatar as the server does not sends the full URL of the avatar if its uploaded (/user_uploads/....) and currently crashing.
So most probably I'll have to modify the server part.

Correct me if I'm wrong

This comment has been minimized.

@borisyankov

borisyankov Jun 14, 2017

Contributor

We can solve this in a subsequent PR though, no need to do it here.

Ideally:

  • the server will make it simpler for us to do that
  • the code between iOS and Android versions will be shared as much as possible
}
public String getRecipientType() {

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

NIT: random empty lines?

@@ -30,5 +31,5 @@ export default connect(
(state) => ({
locale: state.settings.locale,
theme: state.settings.theme,
}),
}), boundActions,

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

The changes to this file seem to not do anything?

if (needsInitialFetch) {
fetchEssentialInitialData(auth);
fetchRestOfInitialData(auth);
fetchRestOfInitialData(auth, realm);

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

You probably don't need the token retrieval in this function anyways.

auth: getAuth(state),
locale: state.settings.locale,
needsInitialFetch: state.app.needsInitialFetch,
accounts: state.accounts,
navigation: state.nav,
realm: state.realm

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

Retrieve the token itself

import MainScreen from './MainScreen';
class MainScreenContainer extends React.Component {
componentWillMount() {
const { auth, saveTokenGCM, doNarrow } = this.props;
if (Platform.OS === 'android') {

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

Extract that in a separate file, and call it *.android.js with an empty function for *.ios.js

}
PendingNotifications.getInitialNotification()
.then((notification) => {

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

Use await

}
}
})
.catch((err) => console.error('getInitialNotifiation() failed', err));

This comment has been minimized.

@borisyankov

borisyankov Jun 13, 2017

Contributor

When will this fail? Is this enough to error handle it?

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jun 14, 2017

The PR looks good to merge now. @neerajwahi ?
Please, just have a look at the lint and flow errors and fix them before that.

kunall17 added some commits Jun 14, 2017

Fix: Show the default Zulip icon, if the avatar is uploaded on the se…
…rver

Currently the server sends only path of the avatar URL (without the
server domain name) eg: /user_uploads/...

This causes a crash, and the server needs to send the full URL
@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 15, 2017

  • zulip/zulip#5398 merged and deployed on chat.zulip.org, therefore the avatar works now
  • Changed the stacking of notification and only one notification is displayed now (last one remains). Will be updated in further PR, displaying all the changes, currently they are being replaced
  • Now correctly narrows when opened by notification

@borisyankov borisyankov merged commit 4398f85 into zulip:master Jun 16, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
coverage/coveralls Coverage increased (+0.2%) to 59.922%
Details

@zulipbot zulipbot removed the needs review label Jun 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment