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

Implementation of ios push notifications #746

Merged
merged 2 commits into from Jul 2, 2017

Conversation

Projects
None yet
4 participants
@kunall17
Contributor

kunall17 commented Jun 26, 2017

  • Currently untested, due to some issues of apns certificate on chat.zulip.org (Will be resolved soon)
  • Not sure if Bundle ID should be the same as the certificate ID, but a provisioning team needs to be create to test this on a device
    screen shot 2017-06-26 at 6 05 16 am
  • The displaying content part of the notification is mostly related from the server (unlike GCM), hence a PR will be landing there.
@smarx

This comment has been minimized.

smarx commented Jun 26, 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 Jun 26, 2017

At a first glance the code looks ok. I'll do a more thorough review and merge if OK.
Make sure the code does not crash the app even if the server has no support for notifications, both for iOS and Android, because we might have to connect to older servers that don't have that.

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jun 27, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jun 28, 2017

Rebase and try to test and improve the code and prepare it for the server with a certificate. Likely we will have it very soon and then it will be highest priority to make it work.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 1, 2017

Make sure tests pass.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 2, 2017

Done

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 2, 2017

Nice. Getting it in.

@borisyankov borisyankov merged commit 20ef05a into zulip:master Jul 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 59.25%
Details

@zulipbot zulipbot removed the needs review label Jul 2, 2017

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