From a8286383dcad17fe91785fa24ff3fa5346697938 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 10 Sep 2024 22:03:31 +0000 Subject: [PATCH 1/3] fix requestNotificationPermission() requestNotificationPermission() was returning undefined due to incorrectly converting a string to an enum. --- .../notifications/subscriptionmanager.test.ts | 32 +++++++++++++++++++ src/shared/managers/SubscriptionManager.ts | 2 +- 2 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 __test__/unit/notifications/subscriptionmanager.test.ts diff --git a/__test__/unit/notifications/subscriptionmanager.test.ts b/__test__/unit/notifications/subscriptionmanager.test.ts new file mode 100644 index 000000000..e9e587948 --- /dev/null +++ b/__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, + ); + }); + }); +}); diff --git a/src/shared/managers/SubscriptionManager.ts b/src/shared/managers/SubscriptionManager.ts index 1c8abb549..52c7bd4c8 100644 --- a/src/shared/managers/SubscriptionManager.ts +++ b/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 { From 9435030778f2cdc9600d1cfc62f72dfb1bed3ae9 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Tue, 10 Sep 2024 23:48:23 +0000 Subject: [PATCH 2/3] ensure sw is a OneSignal one before using it Created a new getOneSignalRegistration() function to guarantee the service worker is OneSignal's. This encapsulates this so consuming code doesn't have to account for this, and it was found out most code was not. --- src/shared/libraries/WorkerMessenger.ts | 2 +- src/shared/managers/ServiceWorkerManager.ts | 36 ++++++++++++++------- src/shared/managers/SubscriptionManager.ts | 15 +++------ src/sw/helpers/ServiceWorkerUtilHelper.ts | 4 +-- test/unit/managers/ServiceWorkerManager.ts | 15 ++++----- 5 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/shared/libraries/WorkerMessenger.ts b/src/shared/libraries/WorkerMessenger.ts index a14a474bb..191d7fbf1 100644 --- a/src/shared/libraries/WorkerMessenger.ts +++ b/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!', diff --git a/src/shared/managers/ServiceWorkerManager.ts b/src/shared/managers/ServiceWorkerManager.ts index 9299e4138..4582e5179 100644 --- a/src/shared/managers/ServiceWorkerManager.ts +++ b/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 { - 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 { // 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...'); diff --git a/src/shared/managers/SubscriptionManager.ts b/src/shared/managers/SubscriptionManager.ts index 52c7bd4c8..7b37ce256 100644 --- a/src/shared/managers/SubscriptionManager.ts +++ b/src/shared/managers/SubscriptionManager.ts @@ -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 { diff --git a/src/sw/helpers/ServiceWorkerUtilHelper.ts b/src/sw/helpers/ServiceWorkerUtilHelper.ts index 2222086ce..6df47ae46 100644 --- a/src/sw/helpers/ServiceWorkerUtilHelper.ts +++ b/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 { + ): Promise { try { // Adding location.origin to negate the effects of a possible 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; } } diff --git a/test/unit/managers/ServiceWorkerManager.ts b/test/unit/managers/ServiceWorkerManager.ts index 91cc2a08c..cf65d0957 100644 --- a/test/unit/managers/ServiceWorkerManager.ts +++ b/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); }); From f3515f76280157838e8766220f28bfa8e730bf71 Mon Sep 17 00:00:00 2001 From: Josh Kasten Date: Wed, 11 Sep 2024 00:03:07 +0000 Subject: [PATCH 3/3] rm ServiceWorkerActiveState.Indeterminate Remove this unused state that was only for the v15 SDK. --- src/shared/helpers/ServiceWorkerHelper.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/shared/helpers/ServiceWorkerHelper.ts b/src/shared/helpers/ServiceWorkerHelper.ts index fbc87b24c..ea06d38c6 100755 --- a/src/shared/helpers/ServiceWorkerHelper.ts +++ b/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 {