Cleaned up is isNewSubscription logic

* Added more test coverage
* Checks for null / undefined applicationServerKey

src/managers/SubscriptionManager.ts

@@ -470,9 +470,6 @@ export class SubscriptionManager {
 
     const existingPushSubscription = await pushManager.getSubscription();
 
-    /* Record the subscription created at timestamp only if this is a new subscription */
-    let isNewSubscription = !existingPushSubscription;
-
     /* Depending on the subscription strategy, handle existing subscription in various ways */
     switch (subscriptionStrategy) {
       case SubscriptionStrategyKind.ResubscribeExisting:
@@ -502,28 +499,23 @@ export class SubscriptionManager {
           */
 
           /* We're unsubscribing, so we want to store the created at timestamp */
-          isNewSubscription = await SubscriptionManager.doPushUnsubscribe(existingPushSubscription);
+          await SubscriptionManager.doPushUnsubscribe(existingPushSubscription);
         }
         break;
       case SubscriptionStrategyKind.SubscribeNew:
         /* Since we want a new subscription every time with this strategy, just unsubscribe. */
         if (existingPushSubscription) {
-          isNewSubscription = await SubscriptionManager.doPushUnsubscribe(existingPushSubscription);
-          break;
+          await SubscriptionManager.doPushUnsubscribe(existingPushSubscription);
         }
-
-        // Always record the subscription if we're resubscribing
-        isNewSubscription = true;
         break;
     }
 
     // Actually subscribe the user to push
-    const [newPushSubscription, createdNew] =
+    const [newPushSubscription, isNewSubscription] =
       await SubscriptionManager.doPushSubscribe(pushManager, this.getVapidKeyForBrowser());
 
     // Update saved create and expired times
-    const didCreateNewSubscription = isNewSubscription || createdNew;
-    await SubscriptionManager.updateSubscriptionTime(didCreateNewSubscription, newPushSubscription.expirationTime);
+    await SubscriptionManager.updateSubscriptionTime(isNewSubscription, newPushSubscription.expirationTime);
 
     // Create our own custom object from the browser's native PushSubscription object
     const pushSubscriptionDetails = RawPushSubscription.setFromW3cSubscription(newPushSubscription);
@@ -559,13 +551,18 @@ export class SubscriptionManager {
     applicationServerKey: ArrayBuffer | undefined)
     :Promise<[PushSubscription, boolean]> {
 
+    if (!applicationServerKey) {
+      throw new Error("Missing required 'applicationServerKey' to subscribe for push notifications!");
+    }
+
     const subscriptionOptions: PushSubscriptionOptionsInit = {
       userVisibleOnly: true,
       applicationServerKey: applicationServerKey
     };
     Log.debug('[Subscription Manager] Subscribing to web push with these options:', subscriptionOptions);
     try {
-      return [await pushManager.subscribe(subscriptionOptions), false];
+      const existingSubscription = await pushManager.getSubscription();
+      return [await pushManager.subscribe(subscriptionOptions), !existingSubscription];
     } catch (e) {
       if (e.name == "InvalidStateError") {
         // This exception is thrown if the key for the existing applicationServerKey is different,

test/support/mocks/service-workers/models/PushManager.ts

@@ -52,6 +52,9 @@ export default class PushManager {
    * Only to be used internally from PushSubscription.unsubscribe().
    */
   public __unsubscribe() {
+    if (!this.subscription) {
+      throw new Error("No Existing subscription!");
+    }
     this.subscription = null;
   }
 }

test/unit/managers/SubscriptionManager.ts

@@ -233,6 +233,53 @@ test(
   }
 );
 
+test(
+  "resubscribe existing strategy does not unsubscribes if options are not null",
+  async t => {
+    const subsequentVapidKeys = generateVapidKeys();
+    let unsubscribeSpy: sinon.SinonSpy;
+
+    await testCase(
+      t,
+      BrowserUserAgent.ChromeMacSupported,
+      subsequentVapidKeys.uniquePublic,
+      subsequentVapidKeys.sharedPublic,
+      SubscriptionStrategyKind.ResubscribeExisting,
+      async (_pushManager, _subscriptionManager) => {
+        // And spy on PushManager.unsubscribe(), because we expect the existing subscription to be unsubscribed
+        unsubscribeSpy = sandbox.spy(PushSubscription.prototype, 'unsubscribe');
+      },
+      async (_pushManager, pushManagerSubscribeSpy) => {
+        // The subscription options used should be our subsequent subscription's options
+        const calledSubscriptionOptions = pushManagerSubscribeSpy.getCall(0).args[0];
+        const subsequentSubscriptionOptions: PushSubscriptionOptions = {
+          userVisibleOnly: true,
+          applicationServerKey: base64ToUint8Array(subsequentVapidKeys.uniquePublic).buffer,
+        };
+        t.deepEqual(calledSubscriptionOptions, subsequentSubscriptionOptions);
+
+        // Unsubscribe should NOT have been called
+        t.false(unsubscribeSpy.calledOnce);
+      }
+    );
+  }
+);
+
+test(
+  "null applicationServerKey throws when subscribing",
+  async t => {
+
+    const manager = new SubscriptionManager(OneSignal.context, {
+      safariWebId: undefined,
+      appId: Random.getRandomUuid(),
+      vapidPublicKey: <any>undefined, // Forcing vapidPublicKey to undefined to test throwing
+      onesignalVapidPublicKey: generateVapidKeys().sharedPublic
+    } as SubscriptionManagerConfig);
+
+    await t.throws(manager.subscribe(SubscriptionStrategyKind.SubscribeNew), Error);
+  }
+);
+
 test("registerSubscription with an existing subsription sends player update", async t => {
   TestEnvironment.mockInternalOneSignal();
 
@@ -396,18 +443,7 @@ test(
 test(
   "subscribe new strategy unsubscribes existing subscription to create new subscription",
   async t => {
-    const initialVapidKeys = generateVapidKeys();
     const subsequentVapidKeys = generateVapidKeys();
-
-    const initialSubscriptionOptions: PushSubscriptionOptions = {
-      userVisibleOnly: true,
-      applicationServerKey: base64ToUint8Array(initialVapidKeys.uniquePublic).buffer,
-    };
-    const subsequentSubscriptionOptions: PushSubscriptionOptions = {
-      userVisibleOnly: true,
-      applicationServerKey: base64ToUint8Array(subsequentVapidKeys.uniquePublic).buffer,
-    };
-
     let unsubscribeSpy: sinon.SinonSpy;
 
     await testCase(
@@ -416,15 +452,20 @@ test(
       subsequentVapidKeys.uniquePublic,
       subsequentVapidKeys.sharedPublic,
       SubscriptionStrategyKind.SubscribeNew,
-      async (pushManager, subscriptionManager) => {
+      async (pushManager, _subscriptionManager) => {
         // Create an initial subscription, so subsequent subscriptions attempt to re-use this initial
         // subscription's options
-        await pushManager.subscribe(initialSubscriptionOptions);
+        const initialVapidKeys = generateVapidKeys();
+        const subscriptionOptions: PushSubscriptionOptions = {
+          userVisibleOnly: true,
+          applicationServerKey: base64ToUint8Array(initialVapidKeys.uniquePublic).buffer,
+        };
+        await pushManager.subscribe(subscriptionOptions);
 
         // And spy on PushManager.unsubscribe(), because we expect the existing subscription to be unsubscribed
         unsubscribeSpy = sandbox.spy(PushSubscription.prototype, 'unsubscribe');
       },
-      async (pushManager, pushManagerSubscribeSpy) => {
+      async (_pushManager, _pushManagerSubscribeSpy) => {
         // Unsubscribe should have been called
         t.true(unsubscribeSpy.calledOnce);
       }