From d3dac41cdabede9b207e7642f93017d8767cdadd Mon Sep 17 00:00:00 2001 From: Harrison Healey Date: Mon, 15 Jul 2024 12:50:25 -0400 Subject: [PATCH] MM-59416 Don't request notification permissions when we already have them (#27629) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * MM-59416 Only request notification permissions from the browser once per session * MM-59416 Don't request notification permissions when we already have them * Address feedback Co-authored-by: Daniel Espino García * Fix bad merge --------- Co-authored-by: Daniel Espino García --- .../channels/src/utils/notifications.test.ts | 203 ++++++++++++++---- webapp/channels/src/utils/notifications.ts | 34 +-- 2 files changed, 175 insertions(+), 62 deletions(-) diff --git a/webapp/channels/src/utils/notifications.test.ts b/webapp/channels/src/utils/notifications.test.ts index 3b10c7113f..306232b095 100644 --- a/webapp/channels/src/utils/notifications.test.ts +++ b/webapp/channels/src/utils/notifications.test.ts @@ -21,6 +21,15 @@ describe('Notifications.showNotification', () => { let store: ReturnType; beforeEach(() => { + // Re-initialize window.Notification so that tests can modify it as needed. By default, everything exists, + // we've never requested permissions before, and any request for permissions will be denied. + window.Notification = jest.fn(() => ({ + close: jest.fn(), + })); + window.Notification.requestPermission = jest.fn(() => Promise.resolve('denied')); + window.Notification.permission = 'default'; + + // Reset and re-import utils/notifications for every test to reset requestedNotificationPermission jest.resetModules(); Notifications = require('utils/notifications'); @@ -28,45 +37,47 @@ describe('Notifications.showNotification', () => { }); it('should throw an exception if Notification is not defined on window', async () => { + delete window.Notification; + await expect(store.dispatch(Notifications.showNotification())).rejects.toThrow('Notification not supported'); }); it('should throw an exception if Notification.requestPermission is not defined', async () => { - window.Notification = {}; + window.Notification = jest.fn(); + await expect(store.dispatch(Notifications.showNotification())).rejects.toThrow('Notification.requestPermission not supported'); }); it('should throw an exception if Notification.requestPermission is not a function', async () => { - window.Notification = { - requestPermission: true, - }; - await expect(store.dispatch(Notifications.showNotification())).rejects.toThrow('Notification.requestPermission not supported'); - }); - - it('should request permissions, promise style, if not previously requested, do nothing', async () => { - window.Notification = { - requestPermission: () => Promise.resolve('denied'), - permission: 'denied', - }; - await expect(store.dispatch(Notifications.showNotification())).resolves.toBeTruthy(); - }); - - it('should request permissions, callback style, if not previously requested, do nothing', async () => { - window.Notification = { - requestPermission: (callback: NotificationPermissionCallback) => { - if (callback) { - callback('denied'); - } - }, - permission: 'denied', - }; - await expect(store.dispatch(Notifications.showNotification())).resolves.toBeTruthy(); - }); - - it('should request permissions, promise style, if not previously requested, handling success', async () => { window.Notification = jest.fn(); - window.Notification.requestPermission = () => Promise.resolve('granted'); - window.Notification.permission = 'denied'; + window.Notification.requestPermission = true; + + await expect(store.dispatch(Notifications.showNotification())).rejects.toThrow('Notification.requestPermission not supported'); + expect(window.Notification).not.toHaveBeenCalled(); + }); + + it('should request permissions, promise style, if not previously requested and not show a notification when permission is denied', async () => { + window.Notification.requestPermission.mockResolvedValue('denied'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_denied', + }); + expect(window.Notification).not.toHaveBeenCalled(); + }); + + it('should request permissions, callback style, if not previously requested and not show a notification when permission is denied', async () => { + window.Notification.requestPermission = (callback: NotificationPermissionCallback) => callback?.('denied'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_denied', + }); + expect(window.Notification).not.toHaveBeenCalled(); + }); + + it('should request permissions, promise style, if not previously requested and show notification when permission is granted', async () => { + window.Notification.requestPermission.mockResolvedValue('granted'); const n = {}; window.Notification.mockReturnValueOnce(n); @@ -76,8 +87,10 @@ describe('Notifications.showNotification', () => { requireInteraction: true, silent: false, title: '', - }))).resolves.toBeTruthy(); - await expect(window.Notification.mock.calls.length).toBe(1); + }))).resolves.toMatchObject({ + status: 'success', + }); + expect(window.Notification.mock.calls.length).toBe(1); const call = window.Notification.mock.calls[0]; expect(call[1]).toEqual({ body: 'body', @@ -88,25 +101,22 @@ describe('Notifications.showNotification', () => { }); }); - it('should request permissions, callback style, if not previously requested, handling success', async () => { - window.Notification = jest.fn(); + it('should request permissions, callback style, if not previously requested and show notification when permission is granted', async () => { window.Notification.requestPermission = (callback: NotificationPermissionCallback) => { if (callback) { callback('granted'); } }; - window.Notification.permission = 'denied'; - - const n = {}; - window.Notification.mockReturnValueOnce(n); await expect(store.dispatch(Notifications.showNotification({ body: 'body', requireInteraction: true, silent: false, title: '', - }))).resolves.toBeTruthy(); - await expect(window.Notification.mock.calls.length).toBe(1); + }))).resolves.toMatchObject({ + status: 'success', + }); + expect(window.Notification.mock.calls.length).toBe(1); const call = window.Notification.mock.calls[0]; expect(call[1]).toEqual({ body: 'body', @@ -118,15 +128,116 @@ describe('Notifications.showNotification', () => { }); it('should do nothing if permissions previously requested but not granted', async () => { - window.Notification = { - requestPermission: () => Promise.resolve('denied'), - permission: 'denied', - }; + window.Notification.requestPermission.mockResolvedValue('denied'); // Call one to deny and mark as already requested, do nothing, throw nothing - await expect(store.dispatch(Notifications.showNotification())).resolves.toBeTruthy(); + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_denied', + }); // Try again - await expect(store.dispatch(Notifications.showNotification())).resolves.toBeTruthy(); + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_previously_denied', + }); + }); + + it('should only request permissions once if permissions are granted in response to that request', async () => { + window.Notification.requestPermission.mockResolvedValue('granted'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'success', + }); + expect(window.Notification).toHaveBeenCalledTimes(1); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(1); + + window.Notification.permission = 'granted'; + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'success', + }); + expect(window.Notification).toHaveBeenCalledTimes(2); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(1); + }); + + it('should only request permissions once if request gets denied', async () => { + window.Notification.requestPermission.mockResolvedValue('denied'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_denied', + }); + expect(window.Notification).toHaveBeenCalledTimes(0); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(1); + + window.Notification.permission = 'denied'; + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_previously_denied', + }); + expect(window.Notification).toHaveBeenCalledTimes(0); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(1); + }); + + it('should only request permissions once if request gets ignored', async () => { + window.Notification.requestPermission.mockResolvedValue('default'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_denied', + }); + expect(window.Notification).toHaveBeenCalledTimes(0); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(1); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_previously_denied', + }); + expect(window.Notification).toHaveBeenCalledTimes(0); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(1); + }); + + it('should not request permission if it was granted during a previous session', async () => { + window.Notification.permission = 'granted'; + + // Reload utils/notifications to set requestedNotificationPermission to true based on Notification.permission + jest.resetModules(); + Notifications = require('utils/notifications'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'success', + }); + expect(window.Notification).toHaveBeenCalledTimes(1); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(0); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'success', + }); + expect(window.Notification).toHaveBeenCalledTimes(2); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(0); + }); + + it('should not request permission if it was denied during a previous session', async () => { + window.Notification.permission = 'denied'; + + // Reload utils/notifications to set requestedNotificationPermission to true based on Notification.permission + jest.resetModules(); + Notifications = require('utils/notifications'); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_previously_denied', + }); + expect(window.Notification).toHaveBeenCalledTimes(0); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(0); + + await expect(store.dispatch(Notifications.showNotification())).resolves.toMatchObject({ + status: 'not_sent', + reason: 'notifications_permission_previously_denied', + }); + expect(window.Notification).toHaveBeenCalledTimes(0); + expect(window.Notification.requestPermission).toHaveBeenCalledTimes(0); }); }); diff --git a/webapp/channels/src/utils/notifications.ts b/webapp/channels/src/utils/notifications.ts index 6e4e80ebbc..8f3c63ef83 100644 --- a/webapp/channels/src/utils/notifications.ts +++ b/webapp/channels/src/utils/notifications.ts @@ -14,7 +14,7 @@ export type NotificationResult = { data?: string; } -let requestedNotificationPermission = false; +let requestedNotificationPermission = Boolean('Notification' in window && Notification.permission !== 'default'); // showNotification displays a platform notification with the configured parameters. // @@ -60,24 +60,26 @@ export function showNotification( throw new Error('Notification.requestPermission not supported'); } - if (Notification.permission !== 'granted' && requestedNotificationPermission) { - // User didn't allow notifications - return {status: 'not_sent', reason: 'notifications_permission_previously_denied', data: Notification.permission, callback: () => {}}; - } + if (Notification.permission !== 'granted') { + if (requestedNotificationPermission) { + // User didn't allow notifications + return {status: 'not_sent', reason: 'notifications_permission_previously_denied', data: Notification.permission, callback: () => {}}; + } - requestedNotificationPermission = true; + requestedNotificationPermission = true; - let permission = await Notification.requestPermission(); - if (typeof permission === 'undefined') { - // Handle browsers that don't support the promise-based syntax. - permission = await new Promise((resolve) => { - Notification.requestPermission(resolve); - }); - } + let permission = await Notification.requestPermission(); + if (typeof permission === 'undefined') { + // Handle browsers that don't support the promise-based syntax. + permission = await new Promise((resolve) => { + Notification.requestPermission(resolve); + }); + } - if (permission !== 'granted') { - // User has denied notification for the site - return {status: 'not_sent', reason: 'notifications_permission_denied', data: permission, callback: () => {}}; + if (permission !== 'granted') { + // User has denied notification for the site + return {status: 'not_sent', reason: 'notifications_permission_denied', data: permission, callback: () => {}}; + } } const notification = new Notification(title, {