Log out: Ensure that unregistering push notifications completes (#16386)

Author: itsmeichigo

RELEASE-NOTES.txt

@@ -6,6 +6,7 @@
 - [*] Better support site credential login for sites with captcha plugins [https://github.com/woocommerce/woocommerce-ios/pull/16372]
 - [*] Crash fix attempt to resolve the race condition in request authenticator swapping [https://github.com/woocommerce/woocommerce-ios/pull/16370]
 - [*] Update products on the Top Performers card when there are changes made to displayed products [https://github.com/woocommerce/woocommerce-ios/pull/16380]
+- [*] Fixed issue unregistering device from push notifications [https://github.com/woocommerce/woocommerce-ios/pull/16386]
 - [Internal] Fix broken navigation to a variable product selector [https://github.com/woocommerce/woocommerce-ios/pull/16363]
 
 23.7

WooCommerce/Classes/Notifications/PushNotificationsManager.swift

@@ -167,18 +167,19 @@ extension PushNotificationsManager {
 
     /// Unregisters the Application from WordPress.com Push Notifications Service.
     ///
-    func unregisterForRemoteNotifications() {
+    func unregisterForRemoteNotifications(onCompletion: @escaping () -> Void) {
         DDLogInfo("📱 Unregistering For Remote Notifications...")
 
         unregisterDotcomDeviceIfPossible() { error in
             if let error = error {
                 DDLogError("⛔️ Unable to unregister from WordPress.com Push Notifications: \(error)")
-                return
+            } else {
+                DDLogInfo("📱 Successfully unregistered from WordPress.com Push Notifications!")
+                self.deviceID = nil
+                self.deviceToken = nil
             }
-
-            DDLogInfo("📱 Successfully unregistered from WordPress.com Push Notifications!")
-            self.deviceID = nil
-            self.deviceToken = nil
+            // Always call completion, even on error
+            onCompletion()
         }
     }
 
@@ -249,7 +250,7 @@ extension PushNotificationsManager {
     ///
     func registrationDidFail(with error: Error) {
         DDLogError("⛔️ Push Notifications Registration Failure: \(error)")
-        unregisterForRemoteNotifications()
+        unregisterForRemoteNotifications {}
     }
 
     /// Handles a notification while the app is in foreground

WooCommerce/Classes/ServiceLocator/PushNotesManager.swift

@@ -50,8 +50,9 @@ protocol PushNotesManager {
     func registerForRemoteNotifications()
 
     /// Unregisters the Application from WordPress.com Push Notifications Service.
+    /// - Parameter onCompletion: Closure to be executed on completion.
     ///
-    func unregisterForRemoteNotifications()
+    func unregisterForRemoteNotifications(onCompletion: @escaping () -> Void)
 
     /// Requests Authorization to receive Push Notifications, *only* when the current Status is not determined.
     ///

WooCommerce/Classes/Yosemite/AuthenticatedState.swift

@@ -237,11 +237,6 @@ class AuthenticatedState: StoresManagerState {
     /// Executed before the current state is deactivated.
     ///
     func willLeave() {
-        let pushNotesManager = ServiceLocator.pushNotesManager
-
-        pushNotesManager.unregisterForRemoteNotifications()
-        pushNotesManager.resetBadgeCountForAllStores(onCompletion: {})
-
         resetServices()
     }
 

WooCommerce/Classes/Yosemite/DefaultStoresManager.swift

@@ -270,7 +270,7 @@ class DefaultStoresManager: StoresManager {
         sessionManager.deleteApplicationPassword(locally: true)
         ServiceLocator.analytics.refreshUserData()
         ZendeskProvider.shared.reset()
-        ServiceLocator.pushNotesManager.unregisterForRemoteNotifications()
+        ServiceLocator.pushNotesManager.unregisterForRemoteNotifications {}
     }
 
     /// Fully deauthenticates the user, if needed.
@@ -290,6 +290,20 @@ class DefaultStoresManager: StoresManager {
     ///
     @discardableResult
     func deauthenticate() -> StoresManager {
+        let pushNotesManager = ServiceLocator.pushNotesManager
+        pushNotesManager.resetBadgeCountForAllStores(onCompletion: {})
+
+        // Keep a strong reference to the current state to prevent it from being deallocated
+        // until the unregister completes
+        let currentState = state
+
+        // Unregister from remote notifications asynchronously
+        pushNotesManager.unregisterForRemoteNotifications {
+            DDLogInfo("📱 Push notification unregistration completed after logout")
+            // Keep `currentState` until the end of the closure
+            _ = currentState
+        }
+
         applicationPasswordGenerationFailureObserver = nil
         invalidWPCOMTokenNotificationObserver = nil
         stopObservingNetworkNotifications()

WooCommerce/WooCommerceTests/Mocks/MockPushNotificationsManager.swift

@@ -78,7 +78,7 @@ final class MockPushNotificationsManager: PushNotesManager {
 
     }
 
-    func unregisterForRemoteNotifications() {
+    func unregisterForRemoteNotifications(onCompletion: @escaping () -> Void) {
 
     }
 

WooCommerce/WooCommerceTests/Notifications/PushNotificationsManagerTests.swift

@@ -134,7 +134,7 @@ final class PushNotificationsManagerTests: XCTestCase {
     ///
     func testUnregisterForRemoteNotificationsDoesNothingWhenThereIsNoDeviceIdStored() {
         XCTAssert(storesManager.receivedActions.isEmpty)
-        manager.unregisterForRemoteNotifications()
+        manager.unregisterForRemoteNotifications {}
         XCTAssert(storesManager.receivedActions.isEmpty)
     }
 
@@ -143,7 +143,7 @@ final class PushNotificationsManagerTests: XCTestCase {
     ///
     func testUnregisterForRemoteNotificationsEffectivelyDispatchesUnregisterDeviceAction() {
         defaults.set(Sample.deviceID, forKey: .deviceID)
-        manager.unregisterForRemoteNotifications()
+        manager.unregisterForRemoteNotifications {}
 
         guard case let .unregisterDevice(deviceID, _) = storesManager.receivedActions.first as! NotificationAction else {
             XCTFail()
@@ -161,7 +161,7 @@ final class PushNotificationsManagerTests: XCTestCase {
         defaults.set(Sample.deviceID, forKey: .deviceID)
         defaults.set(Sample.deviceToken, forKey: .deviceToken)
 
-        manager.unregisterForRemoteNotifications()
+        manager.unregisterForRemoteNotifications {}
 
         guard case let .unregisterDevice(_, onCompletion) = storesManager.receivedActions.first as! NotificationAction else {
             XCTFail()