Skip to content

Fix a number of push notification issues#1000

Merged
astralbodies merged 4 commits intodevelopfrom
issue/961-push-wonkiness
Jan 7, 2014
Merged

Fix a number of push notification issues#1000
astralbodies merged 4 commits intodevelopfrom
issue/961-push-wonkiness

Conversation

@astralbodies
Copy link
Contributor

Fixes #961

Still can't reproduce the problem with credentials being lost on the device after a background push was received. Discovered a number of problems with push, however, and have resolved them in this PR.

  1. Background pushes should not be updating the UI (if possible) since we also refresh data when the UI is presented for notifications.
  2. Background pushes have the ability to launch the app in iOS 7 - with the side effect of that push being sent through the options dictionary. The UI is now not updated in that case.
  3. When we unregister a device with our backend push server we should really also be unregistering with Apple's push network.
  4. All of our launch work was being done in willFinishLaunchingWithOptions - UI work should really be done in didFinishLaunchingWithOptions. Moved the key window call and notification handling to the delegate method called after the app is running.
  5. WordPress.com API was not calling unregister for push when logging out the main WP.com account. Probably shouldn't be handled in the API since we're going to make it a Pod but it can't be eliminated from this release. Marked as a FIXME.

…dle notification if active

When a background push launches the app (which it can do in iOS 7) the notification was being handled twice.  Ensure that the app is in an active state when attempting to handle a notification sent through to the didFinishLaunching.
@pivotal-rebecca
Copy link

4 - it's being called on logout in 22-accounts I believe.

1 - the background pushes were updating the UI to use iOS7's multitasking features. When the completion handler is called, a screenshot is taken of the current state. Granted, you may not be on the notifications screen :) Perhaps if you are, we should update the UI?

@astralbodies
Copy link
Contributor Author

@xtreme-rebecca-putinski I reversed the change made to the WordPress.com API for unregistering notifications.

Yeah I think I'm going to put the Notifications screen UI changes into another ticket, because some cleanup is going to be needed. Hooking into the will/did/Appear/Disappear is hokey especially with the background updates.

Copy link
Contributor

Choose a reason for hiding this comment

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

@astralbodies - Did you mean to get rid of the mixpanel call as well?

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 removed it since this is fired off and has no user interaction (app didn't actually open)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh - can we move it to someplace where the user does have interaction - i.e. https://github.com/wordpress-mobile/WordPress-iOS/pull/1000/files#diff-12c33a973ac67adb445a57af0d26b92cR128?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sendhil It is being handled in this method - sorry it took me forever to verify 😄

+ (void)handleNotificationForApplicationLaunch:(NSDictionary *)launchOptions {

Copy link
Contributor

Choose a reason for hiding this comment

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

@astralbodies - awesome, thanks for that.

astralbodies added a commit that referenced this pull request Jan 7, 2014
@astralbodies astralbodies merged commit 0eaac16 into develop Jan 7, 2014
@astralbodies astralbodies deleted the issue/961-push-wonkiness branch January 7, 2014 18:54
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.

Notifications: Time out displayed when phone locked

4 participants