From 5125875100075239452a741f9ba42d94ebed39bd Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 5 Oct 2022 12:46:18 +0200 Subject: [PATCH 1/3] eagerly save m.local_notification_settings events --- src/components/structures/MatrixChat.tsx | 13 ++++++++++ src/utils/notifications.ts | 28 +++++++++++++++++++- test/utils/notifications-test.ts | 33 ++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 1 deletion(-) diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 1d24e1da087..539350324e7 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -137,6 +137,7 @@ import { TimelineRenderingType } from "../../contexts/RoomContext"; import { UseCaseSelection } from '../views/elements/UseCaseSelection'; import { ValidatedServerConfig } from '../../utils/ValidatedServerConfig'; import { isLocalRoom } from '../../utils/localRoom/isLocalRoom'; +import { createLocalNotificationSettingsIfNeeded } from '../../utils/notifications'; // legacy export export { default as Views } from "../../Views"; @@ -352,6 +353,8 @@ export default class MatrixChat extends React.PureComponent { initSentry(SdkConfig.get("sentry")); } + private get cli(): MatrixClient { return MatrixClientPeg.get(); } + private async postLoginSetup() { const cli = MatrixClientPeg.get(); const cryptoEnabled = cli.isCryptoEnabled(); @@ -1257,6 +1260,8 @@ export default class MatrixChat extends React.PureComponent { this.themeWatcher.recheck(); StorageManager.tryPersistStorage(); + this.cli.on(ClientEvent.Sync, this.onInitialSync); + if ( MatrixClientPeg.currentUserIsJustRegistered() && SettingsStore.getValue("FTUE.useCaseSelection") === null @@ -1283,6 +1288,14 @@ export default class MatrixChat extends React.PureComponent { } } + private onInitialSync = (): void => { + if (this.cli.isInitialSyncComplete()) { + this.cli.off(ClientEvent.Sync, this.onInitialSync); + } + + createLocalNotificationSettingsIfNeeded(this.cli); + }; + private async onShowPostLoginScreen(useCase?: UseCase) { if (useCase) { PosthogAnalytics.instance.setProperty("ftueUseCaseSelection", useCase); diff --git a/src/utils/notifications.ts b/src/utils/notifications.ts index f41edd24bb7..7e8aff6d0bd 100644 --- a/src/utils/notifications.ts +++ b/src/utils/notifications.ts @@ -14,14 +14,40 @@ See the License for the specific language governing permissions and limitations under the License. */ +import { MatrixClient } from "matrix-js-sdk/src/client"; import { LOCAL_NOTIFICATION_SETTINGS_PREFIX } from "matrix-js-sdk/src/@types/event"; import { LocalNotificationSettings } from "matrix-js-sdk/src/@types/local_notifications"; -import { MatrixClient } from "matrix-js-sdk/src/client"; + +import SettingsStore from "../settings/SettingsStore"; + +export const deviceNotificationSettingsKeys = [ + "notificationsEnabled", + "notificationBodyEnabled", + "audioNotificationsEnabled", +]; export function getLocalNotificationAccountDataEventType(deviceId: string): string { return `${LOCAL_NOTIFICATION_SETTINGS_PREFIX.name}.${deviceId}`; } +export async function createLocalNotificationSettingsIfNeeded(cli: MatrixClient): Promise { + const eventType = getLocalNotificationAccountDataEventType(cli.deviceId); + const event = cli.getAccountData(eventType); + // New sessions will create an account data event to signify they support + // remote toggling of push notifications on this device. Default `is_silenced=true` + // For backwards compat purposes, older sessions will need to check settings value + // to determine what the state of `is_silenced` + if (!event) { + // If any of the above is true, we fall in the "backwards compat" case, + // and `is_silenced` will be set to `false` + const isSilenced = !deviceNotificationSettingsKeys.some(key => SettingsStore.getValue(key)); + + await cli.setAccountData(eventType, { + is_silenced: isSilenced, + }); + } +} + export function localNotificationsAreSilenced(cli: MatrixClient): boolean { const eventType = getLocalNotificationAccountDataEventType(cli.deviceId); const event = cli.getAccountData(eventType); diff --git a/test/utils/notifications-test.ts b/test/utils/notifications-test.ts index b27a660ebff..f1e10164912 100644 --- a/test/utils/notifications-test.ts +++ b/test/utils/notifications-test.ts @@ -20,6 +20,7 @@ import { mocked } from "jest-mock"; import { localNotificationsAreSilenced, getLocalNotificationAccountDataEventType, + createLocalNotificationSettingsIfNeeded, } from "../../src/utils/notifications"; import SettingsStore from "../../src/settings/SettingsStore"; import { getMockClientWithEventEmitter } from "../test-utils/client"; @@ -46,6 +47,38 @@ describe('notifications', () => { mocked(SettingsStore).getValue.mockReturnValue(false); }); + describe('createLocalNotification', () => { + it('creates account data event', async () => { + await createLocalNotificationSettingsIfNeeded(mockClient); + const event = mockClient.getAccountData(accountDataEventKey); + expect(event?.getContent().is_silenced).toBe(true); + }); + + // Can't figure out why the mock does not override the value here + /*.each(deviceNotificationSettingsKeys) instead of skip */ + it.skip("unsilenced for existing sessions", async (/*settingKey*/) => { + mocked(SettingsStore) + .getValue + .mockImplementation((key) => { + // return key === settingKey; + }); + + await createLocalNotificationSettingsIfNeeded(mockClient); + const event = mockClient.getAccountData(accountDataEventKey); + expect(event?.getContent().is_silenced).toBe(false); + }); + + it("does not override an existing account event data", async () => { + mockClient.setAccountData(accountDataEventKey, { + is_silenced: false, + }); + + await createLocalNotificationSettingsIfNeeded(mockClient); + const event = mockClient.getAccountData(accountDataEventKey); + expect(event?.getContent().is_silenced).toBe(false); + }); + }); + describe('localNotificationsAreSilenced', () => { it('defaults to true when no setting exists', () => { expect(localNotificationsAreSilenced(mockClient)).toBeTruthy(); From fbeb7e131fc38cd57221f65187625bd3dcb7e2c2 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 5 Oct 2022 12:56:26 +0200 Subject: [PATCH 2/3] unskip test --- test/utils/notifications-test.ts | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/test/utils/notifications-test.ts b/test/utils/notifications-test.ts index f1e10164912..ba134c14808 100644 --- a/test/utils/notifications-test.ts +++ b/test/utils/notifications-test.ts @@ -21,6 +21,7 @@ import { localNotificationsAreSilenced, getLocalNotificationAccountDataEventType, createLocalNotificationSettingsIfNeeded, + deviceNotificationSettingsKeys, } from "../../src/utils/notifications"; import SettingsStore from "../../src/settings/SettingsStore"; import { getMockClientWithEventEmitter } from "../test-utils/client"; @@ -54,19 +55,19 @@ describe('notifications', () => { expect(event?.getContent().is_silenced).toBe(true); }); - // Can't figure out why the mock does not override the value here - /*.each(deviceNotificationSettingsKeys) instead of skip */ - it.skip("unsilenced for existing sessions", async (/*settingKey*/) => { - mocked(SettingsStore) - .getValue - .mockImplementation((key) => { - // return key === settingKey; - }); - - await createLocalNotificationSettingsIfNeeded(mockClient); - const event = mockClient.getAccountData(accountDataEventKey); - expect(event?.getContent().is_silenced).toBe(false); - }); + it.each(deviceNotificationSettingsKeys)( + 'unsilenced for existing sessions when %s setting is truthy', + async (settingKey) => { + mocked(SettingsStore) + .getValue + .mockImplementation((key) => { + return key === settingKey; + }); + + await createLocalNotificationSettingsIfNeeded(mockClient); + const event = mockClient.getAccountData(accountDataEventKey); + expect(event?.getContent().is_silenced).toBe(false); + }); it("does not override an existing account event data", async () => { mockClient.setAccountData(accountDataEventKey, { From 9688fefc32a3527fe0fda677340f546dea8a2eef Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 10 Oct 2022 15:48:14 +0200 Subject: [PATCH 3/3] create local notification settings after first non-cached sync --- src/Notifier.ts | 16 +++++-- src/components/structures/MatrixChat.tsx | 13 ----- test/Notifier-test.ts | 60 ++++++++++++++++++++++-- 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/src/Notifier.ts b/src/Notifier.ts index 875402d982d..64f4a6547f6 100644 --- a/src/Notifier.ts +++ b/src/Notifier.ts @@ -26,6 +26,7 @@ import { M_LOCATION } from "matrix-js-sdk/src/@types/location"; import { PermissionChanged as PermissionChangedEvent, } from "@matrix-org/analytics-events/types/typescript/PermissionChanged"; +import { ISyncStateData, SyncState } from "matrix-js-sdk/src/sync"; import { MatrixClientPeg } from './MatrixClientPeg'; import { PosthogAnalytics } from "./PosthogAnalytics"; @@ -50,6 +51,7 @@ import { localNotificationsAreSilenced } from "./utils/notifications"; import { getIncomingCallToastKey, IncomingCallToast } from "./toasts/IncomingCallToast"; import ToastStore from "./stores/ToastStore"; import { ElementCall } from "./models/Call"; +import { createLocalNotificationSettingsIfNeeded } from './utils/notifications'; /* * Dispatches: @@ -351,12 +353,20 @@ export const Notifier = { return this.toolbarHidden; }, - onSyncStateChange: function(state: string) { - if (state === "SYNCING") { + onSyncStateChange: function(state: SyncState, prevState?: SyncState, data?: ISyncStateData) { + if (state === SyncState.Syncing) { this.isSyncing = true; - } else if (state === "STOPPED" || state === "ERROR") { + } else if (state === SyncState.Stopped || state === SyncState.Error) { this.isSyncing = false; } + + // wait for first non-cached sync to complete + if ( + ![SyncState.Stopped, SyncState.Error].includes(state) && + !data?.fromCache + ) { + createLocalNotificationSettingsIfNeeded(MatrixClientPeg.get()); + } }, onEvent: function(ev: MatrixEvent) { diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 1f8708b3587..6dd2820aa17 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -137,7 +137,6 @@ import { TimelineRenderingType } from "../../contexts/RoomContext"; import { UseCaseSelection } from '../views/elements/UseCaseSelection'; import { ValidatedServerConfig } from '../../utils/ValidatedServerConfig'; import { isLocalRoom } from '../../utils/localRoom/isLocalRoom'; -import { createLocalNotificationSettingsIfNeeded } from '../../utils/notifications'; // legacy export export { default as Views } from "../../Views"; @@ -351,8 +350,6 @@ export default class MatrixChat extends React.PureComponent { initSentry(SdkConfig.get("sentry")); } - private get cli(): MatrixClient { return MatrixClientPeg.get(); } - private async postLoginSetup() { const cli = MatrixClientPeg.get(); const cryptoEnabled = cli.isCryptoEnabled(); @@ -1245,8 +1242,6 @@ export default class MatrixChat extends React.PureComponent { this.themeWatcher.recheck(); StorageManager.tryPersistStorage(); - this.cli.on(ClientEvent.Sync, this.onInitialSync); - if ( MatrixClientPeg.currentUserIsJustRegistered() && SettingsStore.getValue("FTUE.useCaseSelection") === null @@ -1273,14 +1268,6 @@ export default class MatrixChat extends React.PureComponent { } } - private onInitialSync = (): void => { - if (this.cli.isInitialSyncComplete()) { - this.cli.off(ClientEvent.Sync, this.onInitialSync); - } - - createLocalNotificationSettingsIfNeeded(this.cli); - }; - private async onShowPostLoginScreen(useCase?: UseCase) { if (useCase) { PosthogAnalytics.instance.setProperty("ftueUseCaseSelection", useCase); diff --git a/test/Notifier-test.ts b/test/Notifier-test.ts index 4bac0a54233..eb5d5c7fced 100644 --- a/test/Notifier-test.ts +++ b/test/Notifier-test.ts @@ -14,20 +14,30 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { MockedObject } from "jest-mock"; -import { MatrixClient } from "matrix-js-sdk/src/client"; +import { mocked, MockedObject } from "jest-mock"; +import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client"; import { Room } from "matrix-js-sdk/src/models/room"; import { MatrixEvent } from "matrix-js-sdk/src/models/event"; +import { SyncState } from "matrix-js-sdk/src/sync"; import BasePlatform from "../src/BasePlatform"; import { ElementCall } from "../src/models/Call"; import Notifier from "../src/Notifier"; import SettingsStore from "../src/settings/SettingsStore"; import ToastStore from "../src/stores/ToastStore"; -import { getLocalNotificationAccountDataEventType } from "../src/utils/notifications"; +import { + createLocalNotificationSettingsIfNeeded, + getLocalNotificationAccountDataEventType, +} from "../src/utils/notifications"; import { getMockClientWithEventEmitter, mkEvent, mkRoom, mockPlatformPeg } from "./test-utils"; import { IncomingCallToast } from "../src/toasts/IncomingCallToast"; +jest.mock("../src/utils/notifications", () => ({ + // @ts-ignore + ...jest.requireActual("../src/utils/notifications"), + createLocalNotificationSettingsIfNeeded: jest.fn(), +})); + describe("Notifier", () => { const roomId = "!room1:server"; const testEvent = mkEvent({ @@ -111,7 +121,7 @@ describe("Notifier", () => { tweaks: {}, }); - Notifier.onSyncStateChange("SYNCING"); + Notifier.onSyncStateChange(SyncState.Syncing); }); afterEach(() => { @@ -169,4 +179,46 @@ describe("Notifier", () => { expect(ToastStore.sharedInstance().addOrReplaceToast).not.toHaveBeenCalled(); }); }); + + describe('local notification settings', () => { + const createLocalNotificationSettingsIfNeededMock = mocked(createLocalNotificationSettingsIfNeeded); + let hasStartedNotiferBefore = false; + beforeEach(() => { + // notifier defines some listener functions in start + // and references them in stop + // so blows up if stopped before it was started + if (hasStartedNotiferBefore) { + Notifier.stop(); + } + Notifier.start(); + hasStartedNotiferBefore = true; + createLocalNotificationSettingsIfNeededMock.mockClear(); + }); + + afterAll(() => { + Notifier.stop(); + }); + + it('does not create local notifications event after a sync error', () => { + mockClient.emit(ClientEvent.Sync, SyncState.Error, SyncState.Syncing); + expect(createLocalNotificationSettingsIfNeededMock).not.toHaveBeenCalled(); + }); + + it('does not create local notifications event after sync stops', () => { + mockClient.emit(ClientEvent.Sync, SyncState.Stopped, SyncState.Syncing); + expect(createLocalNotificationSettingsIfNeededMock).not.toHaveBeenCalled(); + }); + + it('does not create local notifications event after a cached sync', () => { + mockClient.emit(ClientEvent.Sync, SyncState.Syncing, SyncState.Syncing, { + fromCache: true, + }); + expect(createLocalNotificationSettingsIfNeededMock).not.toHaveBeenCalled(); + }); + + it('creates local notifications event after a non-cached sync', () => { + mockClient.emit(ClientEvent.Sync, SyncState.Syncing, SyncState.Syncing, {}); + expect(createLocalNotificationSettingsIfNeededMock).toHaveBeenCalled(); + }); + }); });