Merge pull request #1192 from OneSignal/fix/double-prompting-with-sw

Fix prompting again after closing notification permission prompt if site has its own ServiceWorker

__test__/unit/notifications/subscriptionmanager.test.ts

@@ -0,0 +1,32 @@
+import MockNotification from '../../support/mocks/MockNotification';
+import { SubscriptionManager } from '../../../src/shared/managers/SubscriptionManager';
+import { NotificationPermission } from '../../../src/shared/models/NotificationPermission';
+
+describe('SubscriptionManager', () => {
+  describe('requestNotificationPermission', () => {
+    beforeEach(() => {
+      window.Notification = MockNotification;
+    });
+
+    test('default', async () => {
+      MockNotification.permission = 'default';
+      expect(await SubscriptionManager.requestNotificationPermission()).toBe(
+        NotificationPermission.Default,
+      );
+    });
+
+    test('denied', async () => {
+      MockNotification.permission = 'denied';
+      expect(await SubscriptionManager.requestNotificationPermission()).toBe(
+        NotificationPermission.Denied,
+      );
+    });
+
+    test('granted', async () => {
+      MockNotification.permission = 'granted';
+      expect(await SubscriptionManager.requestNotificationPermission()).toBe(
+        NotificationPermission.Granted,
+      );
+    });
+  });
+});

src/shared/helpers/ServiceWorkerHelper.ts

@@ -279,12 +279,6 @@ export enum ServiceWorkerActiveState {
    * No service worker is installed.
    */
   None = 'None',
-  /**
-   * Service workers are not supported in this environment. This status is used
-   * on HTTP pages where it isn't possible to know whether a service worker is
-   * installed or not or in any of the other states.
-   */
-  Indeterminate = 'Indeterminate',
 }
 
 export interface ServiceWorkerManagerConfig {

src/shared/libraries/WorkerMessenger.ts

@@ -197,7 +197,7 @@ export class WorkerMessenger {
     );
 
     const workerRegistration =
-      await this.context?.serviceWorkerManager.getRegistration();
+      await this.context?.serviceWorkerManager.getOneSignalRegistration();
     if (!workerRegistration) {
       Log.error(
         '`[Worker Messenger] [Page -> SW] Could not get ServiceWorkerRegistration to postMessage!',

src/shared/managers/ServiceWorkerManager.ts

@@ -1,9 +1,7 @@
 import Environment from '../helpers/Environment';
 import { WorkerMessengerCommand } from '../libraries/WorkerMessenger';
 import Path from '../models/Path';
-import SdkEnvironment from './SdkEnvironment';
 import Database from '../services/Database';
-import { WindowEnvironmentKind } from '../models/WindowEnvironmentKind';
 import Log from '../libraries/Log';
 import OneSignalEvent from '../services/OneSignalEvent';
 import ServiceWorkerRegistrationError from '../errors/ServiceWorkerRegistrationError';
@@ -35,18 +33,35 @@ export class ServiceWorkerManager {
     this.config = config;
   }
 
-  // Gets details on the OneSignal service-worker (if any)
-  public async getRegistration(): Promise<
-    ServiceWorkerRegistration | null | undefined
+  /**
+   * Gets the current ServiceWorkerRegistration in the scoped configured
+   * in OneSignal.
+   * WARNING: This might be a non-OneSignal service worker, use
+   * getOneSignalRegistration() instead if you need this guarantee.
+   */
+  private async getRegistration(): Promise<
+    ServiceWorkerRegistration | undefined
   > {
-    return await ServiceWorkerUtilHelper.getRegistration(
+    return ServiceWorkerUtilHelper.getRegistration(
       this.config.registrationOptions.scope,
     );
   }
 
+  /**
+   *  Gets the OneSignal ServiceWorkerRegistration reference, if it was registered
+   */
+  public async getOneSignalRegistration(): Promise<
+    ServiceWorkerRegistration | undefined
+  > {
+    const state = await this.getActiveState();
+    if (state === ServiceWorkerActiveState.OneSignalWorker) {
+      return this.getRegistration();
+    }
+    return undefined;
+  }
+
   public async getActiveState(): Promise<ServiceWorkerActiveState> {
-    const workerRegistration =
-      await this.context.serviceWorkerManager.getRegistration();
+    const workerRegistration = await this.getRegistration();
     if (!workerRegistration) {
       return ServiceWorkerActiveState.None;
     }
@@ -163,8 +178,7 @@ export class ServiceWorkerManager {
 
   private async haveParamsChanged(): Promise<boolean> {
     // 1. No workerRegistration
-    const workerRegistration =
-      await this.context.serviceWorkerManager.getRegistration();
+    const workerRegistration = await this.getRegistration();
     if (!workerRegistration) {
       Log.info(
         '[changedServiceWorkerParams] workerRegistration not found at scope',
@@ -382,7 +396,7 @@ export class ServiceWorkerManager {
     ServiceWorkerRegistration | undefined | null
   > {
     if (!(await this.shouldInstallWorker())) {
-      return this.getRegistration();
+      return this.getOneSignalRegistration();
     }
 
     Log.info('Installing worker...');

src/shared/managers/SubscriptionManager.ts

@@ -333,7 +333,7 @@ export class SubscriptionManager {
     const results = await window.Notification.requestPermission();
     // TODO: Clean up our custom NotificationPermission enum
     //         in favor of TS union type NotificationPermission instead of converting
-    return NotificationPermission[results];
+    return results as NotificationPermission;
   }
 
   public async isAlreadyRegisteredWithOneSignal(): Promise<boolean> {
@@ -750,7 +750,7 @@ export class SubscriptionManager {
     }
 
     const serviceWorkerRegistration =
-      await this.context.serviceWorkerManager.getRegistration();
+      await this.context.serviceWorkerManager.getOneSignalRegistration();
     if (!serviceWorkerRegistration) return false;
 
     // It's possible to get here in Safari 11.1+ version
@@ -834,17 +834,12 @@ export class SubscriptionManager {
       };
     }
 
-    const workerState =
-      await this.context.serviceWorkerManager.getActiveState();
     const workerRegistration =
-      await this.context.serviceWorkerManager.getRegistration();
+      await this.context.serviceWorkerManager.getOneSignalRegistration();
     const notificationPermission =
       await this.context.permissionManager.getNotificationPermission(
         this.context.appConfig.safariWebId,
       );
-    const isWorkerActive =
-      workerState === ServiceWorkerActiveState.OneSignalWorker;
-
     if (!workerRegistration) {
       /* You can't be subscribed without a service worker registration */
       return {
@@ -861,16 +856,14 @@ export class SubscriptionManager {
      * const isPushEnabled = !!(
      *   pushSubscription &&
      *   deviceId &&
-     *   notificationPermission === NotificationPermission.Granted &&
-     *   isWorkerActive
+     *   notificationPermission === NotificationPermission.Granted
      * );
      */
 
     const isPushEnabled = !!(
       isValidPushSubscription &&
       subscriptionToken &&
-      notificationPermission === NotificationPermission.Granted &&
-      isWorkerActive
+      notificationPermission === NotificationPermission.Granted
     );
 
     return {

src/sw/helpers/ServiceWorkerUtilHelper.ts

@@ -5,7 +5,7 @@ export default class ServiceWorkerUtilHelper {
   // A  relative scope is required as a domain can have none to many service workers installed.
   static async getRegistration(
     scope: string,
-  ): Promise<ServiceWorkerRegistration | null | undefined> {
+  ): Promise<ServiceWorkerRegistration | undefined> {
     try {
       // Adding location.origin to negate the effects of a possible <base> html tag the page may have.
       const url = location.origin + scope;
@@ -17,7 +17,7 @@ export default class ServiceWorkerUtilHelper {
         scope,
         e,
       );
-      return null;
+      return undefined;
     }
   }
 

test/unit/managers/ServiceWorkerManager.ts

@@ -5,10 +5,7 @@ import sinon, { SinonSandbox, SinonStub } from 'sinon';
 import nock from 'nock';
 import { ServiceWorkerManager } from '../../../src/shared/managers/ServiceWorkerManager';
 import { ServiceWorkerActiveState } from '../../../src/shared/helpers/ServiceWorkerHelper';
-import {
-  TestEnvironment,
-  TestEnvironmentConfig,
-} from '../../support/sdk/TestEnvironment';
+import { TestEnvironment } from '../../support/sdk/TestEnvironment';
 import Context from '../../../src/page/models/Context';
 import SdkEnvironment from '../../../src/shared/managers/SdkEnvironment';
 import { WindowEnvironmentKind } from '../../../src/shared/models/WindowEnvironmentKind';
@@ -25,7 +22,6 @@ import { ServiceWorkerRegistrationError } from '../../../src/shared/errors/Servi
 import OneSignalUtils from '../../../src/shared/utils/OneSignalUtils';
 import { MockServiceWorkerRegistration } from '../../support/mocks/service-workers/models/MockServiceWorkerRegistration';
 import { MockServiceWorker } from '../../support/mocks/service-workers/models/MockServiceWorker';
-import { ConfigIntegrationKind } from '../../../src/shared/models/AppConfig';
 import Environment from '../../../src/shared/helpers/Environment';
 import { MockServiceWorkerContainerWithAPIBan } from '../../support/mocks/service-workers/models/MockServiceWorkerContainerWithAPIBan';
 import Path from '../../../src/shared/models/Path';
@@ -374,12 +370,14 @@ test('Service worker failed to install due to 404 on host page. Send notificatio
 
 test('ServiceWorkerManager.getRegistration() returns valid instance when sw is registered', async (t) => {
   await navigator.serviceWorker.register('/Worker.js');
-  const result = await OneSignal.context.serviceWorkerManager.getRegistration();
+  const result =
+    await OneSignal.context.serviceWorkerManager.getOneSignalRegistration();
   t.truthy(result);
 });
 
 test('ServiceWorkerManager.getRegistration() returns undefined when sw is not registered ', async (t) => {
-  const result = await OneSignal.context.serviceWorkerManager.getRegistration();
+  const result =
+    await OneSignal.context.serviceWorkerManager.getOneSignalRegistration();
   t.is(result, undefined);
 });
 
@@ -395,6 +393,7 @@ test('ServiceWorkerManager.getRegistration() handles throws by returning null',
       throw new Error('HTTP NOT SUPPORTED');
     }),
   );
-  const result = await OneSignal.context.serviceWorkerManager.getRegistration();
+  const result =
+    await OneSignal.context.serviceWorkerManager.getOneSignalRegistration();
   t.is(result, null);
 });