fix: refactor login/logout flow and subscription handling (#1436)

Author: fadi-george

index.html

@@ -95,7 +95,6 @@
 
         // for testing, normally this is set on (temporary) in the database client
         OneSignal._isNewVisitor = true;
-        console.log('_isNewVisitor', OneSignal._isNewVisitor);
 
         OneSignal.init({
           appId,

package.json

@@ -81,7 +81,7 @@
     },
     {
       "path": "./build/releases/OneSignalSDK.page.es6.js",
-      "limit": "43.77 kB",
+      "limit": "43.75 kB",
       "gzip": true
     },
     {

src/core/CoreModuleDirector.ts

@@ -133,8 +133,10 @@ export class CoreModuleDirector {
     logMethodCall(
       'CoreModuleDirector.getPushSubscriptionModelByLastKnownToken',
     );
-    const lastKnownPushToken = await getPushToken();
-    if (lastKnownPushToken) {
+
+    // Checking '' in case we create a temp/fake subscription on logi
+    const lastKnownPushToken = (await getPushToken()) ?? '';
+    if (lastKnownPushToken !== null) {
       return this._getSubscriptionOfTypeWithToken(
         SubscriptionChannel._Push,
         lastKnownPushToken,
@@ -151,10 +153,8 @@ export class CoreModuleDirector {
     SubscriptionModel | undefined
   > {
     logMethodCall('CoreModuleDirector.getPushSubscriptionModel');
-    return (
-      (await this._getPushSubscriptionModelByCurrentToken()) ||
-      (await this._getPushSubscriptionModelByLastKnownToken())
-    );
+    const sub = await this._getPushSubscriptionModelByLastKnownToken();
+    return (await this._getPushSubscriptionModelByCurrentToken()) || sub;
   }
 
   public _getIdentityModel(): IdentityModel {

src/onesignal/OneSignal.test.ts

@@ -29,8 +29,10 @@ import {
   setGetUserResponse,
   setSendCustomEventResponse,
   setTransferSubscriptionResponse,
+  setUpdateSubscriptionResponse,
   setUpdateUserResponse,
   transferSubscriptionFn,
+  updateSubscriptionFn,
   updateUserFn,
 } from '__test__/support/helpers/requests';
 import {
@@ -713,7 +715,16 @@ describe('OneSignal - No Consent Required', () => {
               external_id: externalId,
             },
             ...BASE_IDENTITY,
-            subscriptions: [],
+            subscriptions: [
+              {
+                device_model: '',
+                device_os: DEVICE_OS,
+                enabled: false,
+                sdk: expect.any(String),
+                token: '',
+                type: 'ChromePush',
+              },
+            ],
           });
         });
 
@@ -747,7 +758,7 @@ describe('OneSignal - No Consent Required', () => {
           });
         });
 
-        test('login then accept web push permissions - it should make two user calls', async () => {
+        test('login then accept web push permissions - it should create user then update subscription', async () => {
           const { promise, resolve } = Promise.withResolvers();
           OneSignal._emitter.on(OneSignal.EVENTS.SUBSCRIPTION_CHANGED, resolve);
           setGetUserResponse();
@@ -760,6 +771,7 @@ describe('OneSignal - No Consent Required', () => {
               },
             ],
           });
+          setUpdateSubscriptionResponse({ subscriptionId: SUB_ID });
 
           OneSignal._coreDirector._subscriptionModelStore._replaceAll(
             [],
@@ -783,38 +795,36 @@ describe('OneSignal - No Consent Required', () => {
           };
           registerForPushNotifications();
 
-          // first call just sets the external id
+          // create user call includes the empty subscription
           await vi.waitUntil(() => createUserFn.mock.calls.length === 1, {
             interval: 0,
           });
-          expect(createUserFn).toHaveBeenCalledWith({
-            identity: {
-              external_id: externalId,
-            },
-            ...BASE_IDENTITY,
-            subscriptions: [],
-          });
-          await promise;
-
-          // second call creates the subscription
-          await vi.waitUntil(() => createUserFn.mock.calls.length === 2, {
-            interval: 1,
-          });
+          expect(createUserFn).toHaveBeenCalledTimes(1);
           expect(createUserFn).toHaveBeenCalledWith({
             identity: {
               external_id: externalId,
             },
             ...BASE_IDENTITY,
             subscriptions: [
               {
-                ...BASE_SUB,
-                token: PUSH_TOKEN,
+                device_model: '',
+                device_os: DEVICE_OS,
+                enabled: false,
+                sdk: expect.any(String),
+                token: '',
                 type: 'ChromePush',
-                web_auth: 'w3cAuth',
-                web_p256: 'w3cP256dh',
               },
             ],
           });
+          await promise;
+
+          // accepting permissions patches the existing subscription with the token
+          await vi.waitUntil(
+            () => updateSubscriptionFn.mock.calls.length >= 1,
+            {
+              interval: 1,
+            },
+          );
 
           let pushSub: SubscriptionSchema | undefined;
           await vi.waitUntil(

src/onesignal/userDirector.ts

@@ -59,7 +59,6 @@ export default class Message {
   }
 
   async _display(type: string, content: string, duration = 0) {
-    console.log('zzzzzdisplay', type, content, duration);
     Log._debug(`Calling display(${type}, ${content}, ${duration}).`);
     if (this._shown) await this._hide();
     this._content = decodeHtmlEntities(content);

src/page/bell/Message.ts

@@ -1,13 +1,11 @@
 import { TestEnvironment } from '__test__/support/environment/TestEnvironment';
 import { updateIdentityModel } from '__test__/support/helpers/setup';
 import { SubscriptionModel } from 'src/core/models/SubscriptionModel';
+import { BaseSubscriptionOperation } from 'src/core/operations/BaseSubscriptionOperation';
 import { db } from 'src/shared/database/client';
 import Log from 'src/shared/libraries/Log';
-import * as userDirector from '../../onesignal/userDirector';
 import LoginManager from './LoginManager';
 
-const createUserOnServerSpy = vi.spyOn(userDirector, 'createUserOnServer');
-
 describe('LoginManager', () => {
   beforeEach(() => {
     TestEnvironment.initialize();
@@ -52,6 +50,48 @@ describe('LoginManager', () => {
     expect(enqueueAndWaitSpy).toHaveBeenCalled();
   });
 
+  test('login: with existing push sub enqueues transfer operation', async () => {
+    const mockPushSub = { id: 'push-sub-id' } as SubscriptionModel;
+    vi.spyOn(
+      OneSignal._coreDirector,
+      '_getPushSubscriptionModel',
+    ).mockResolvedValue(mockPushSub);
+    const enqueueSpy = vi.spyOn(
+      OneSignal._coreDirector._operationRepo,
+      '_enqueue',
+    );
+    vi.spyOn(
+      OneSignal._coreDirector._operationRepo,
+      '_enqueueAndWait',
+    ).mockResolvedValue(undefined);
+
+    await LoginManager.login('new-id');
+
+    expect(enqueueSpy).toHaveBeenCalled();
+    const transferOp = enqueueSpy.mock.calls[0][0] as BaseSubscriptionOperation;
+    expect(transferOp._subscriptionId).toBe('push-sub-id');
+  });
+
+  test('login: without push sub creates new subscription model', async () => {
+    vi.spyOn(
+      OneSignal._coreDirector,
+      '_getPushSubscriptionModel',
+    ).mockResolvedValue(undefined);
+    vi.spyOn(
+      OneSignal._coreDirector._operationRepo,
+      '_enqueueAndWait',
+    ).mockResolvedValue(undefined);
+    const addSpy = vi.spyOn(
+      OneSignal._coreDirector._subscriptionModelStore,
+      '_add',
+    );
+
+    await LoginManager.login('new-id');
+
+    expect(addSpy).toHaveBeenCalled();
+    expect(addSpy.mock.calls[0][0].token).toBe('');
+  });
+
   test('logout: no external id logs debug and returns', async () => {
     const debugSpy = vi
       .spyOn(Log, '_debug')
@@ -63,21 +103,26 @@ describe('LoginManager', () => {
     );
   });
 
-  test('logout: with external id and no subscriptions resets models and skips user creation', async () => {
+  test('logout: with external id and push sub enqueues transfer and login operations', async () => {
     await updateIdentityModel('external_id', 'abc');
-    await LoginManager.logout();
-    expect(createUserOnServerSpy).not.toHaveBeenCalled();
-  });
-
-  test('logout: with external id and a subscription resets models and creates anonymous user', async () => {
-    await updateIdentityModel('external_id', 'abc');
-    const mockSub = { id: 'sub-id' } as SubscriptionModel;
+    const mockPushSub = { id: 'sub-id' } as SubscriptionModel;
     vi.spyOn(
-      OneSignal._coreDirector._subscriptionModelStore,
-      '_list',
-    ).mockReturnValue([mockSub]);
+      OneSignal._coreDirector,
+      '_getPushSubscriptionModel',
+    ).mockResolvedValue(mockPushSub);
+    const enqueueSpy = vi.spyOn(
+      OneSignal._coreDirector._operationRepo,
+      '_enqueue',
+    );
+    vi.spyOn(
+      OneSignal._coreDirector._operationRepo,
+      '_enqueueAndWait',
+    ).mockResolvedValue(undefined);
 
     await LoginManager.logout();
-    expect(createUserOnServerSpy).toHaveBeenCalled();
+
+    expect(enqueueSpy).toHaveBeenCalled();
+    const transferOp = enqueueSpy.mock.calls[0][0] as BaseSubscriptionOperation;
+    expect(transferOp._subscriptionId).toBe('sub-id');
   });
 });