Fix unresponsive notification toggles (#8549)

* Return consistently typed setting values from localStorage

* Watch notification toggles

* Test that it all works

Author: robintown

src/components/views/settings/Notifications.tsx

@@ -105,15 +105,36 @@ interface IState {
     };
     pushers?: IPusher[];
     threepids?: IThreepid[];
+
+    desktopNotifications: boolean;
+    desktopShowBody: boolean;
+    audioNotifications: boolean;
 }
 
 export default class Notifications extends React.PureComponent<IProps, IState> {
+    private settingWatchers: string[];
+
     public constructor(props: IProps) {
         super(props);
 
         this.state = {
             phase: Phase.Loading,
+            desktopNotifications: SettingsStore.getValue("notificationsEnabled"),
+            desktopShowBody: SettingsStore.getValue("notificationBodyEnabled"),
+            audioNotifications: SettingsStore.getValue("audioNotificationsEnabled"),
         };
+
+        this.settingWatchers = [
+            SettingsStore.watchSetting("notificationsEnabled", null, (...[,,,, value]) =>
+                this.setState({ desktopNotifications: value as boolean }),
+            ),
+            SettingsStore.watchSetting("notificationBodyEnabled", null, (...[,,,, value]) =>
+                this.setState({ desktopShowBody: value as boolean }),
+            ),
+            SettingsStore.watchSetting("audioNotificationsEnabled", null, (...[,,,, value]) =>
+                this.setState({ audioNotifications: value as boolean }),
+            ),
+        ];
     }
 
     private get isInhibited(): boolean {
@@ -129,6 +150,10 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
         this.refreshFromServer();
     }
 
+    public componentWillUnmount() {
+        this.settingWatchers.forEach(watcher => SettingsStore.unwatchSetting(watcher));
+    }
+
     private async refreshFromServer() {
         try {
             const newState = (await Promise.all([
@@ -137,7 +162,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
                 this.refreshThreepids(),
             ])).reduce((p, c) => Object.assign(c, p), {});
 
-            this.setState({
+            this.setState<keyof Omit<IState, "desktopNotifications" | "desktopShowBody" | "audioNotifications">>({
                 ...newState,
                 phase: Phase.Ready,
             });
@@ -308,17 +333,14 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
 
     private onDesktopNotificationsChanged = async (checked: boolean) => {
         await SettingsStore.setValue("notificationsEnabled", null, SettingLevel.DEVICE, checked);
-        this.forceUpdate(); // the toggle is controlled by SettingsStore#getValue()
     };
 
     private onDesktopShowBodyChanged = async (checked: boolean) => {
         await SettingsStore.setValue("notificationBodyEnabled", null, SettingLevel.DEVICE, checked);
-        this.forceUpdate(); // the toggle is controlled by SettingsStore#getValue()
     };
 
     private onAudioNotificationsChanged = async (checked: boolean) => {
         await SettingsStore.setValue("audioNotificationsEnabled", null, SettingLevel.DEVICE, checked);
-        this.forceUpdate(); // the toggle is controlled by SettingsStore#getValue()
     };
 
     private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState) => {
@@ -499,23 +521,23 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
 
             <LabelledToggleSwitch
                 data-test-id='notif-setting-notificationsEnabled'
-                value={SettingsStore.getValue("notificationsEnabled")}
+                value={this.state.desktopNotifications}
                 onChange={this.onDesktopNotificationsChanged}
                 label={_t('Enable desktop notifications for this session')}
                 disabled={this.state.phase === Phase.Persisting}
             />
 
             <LabelledToggleSwitch
                 data-test-id='notif-setting-notificationBodyEnabled'
-                value={SettingsStore.getValue("notificationBodyEnabled")}
+                value={this.state.desktopShowBody}
                 onChange={this.onDesktopShowBodyChanged}
                 label={_t('Show message in desktop notification')}
                 disabled={this.state.phase === Phase.Persisting}
             />
 
             <LabelledToggleSwitch
                 data-test-id='notif-setting-audioNotificationsEnabled'
-                value={SettingsStore.getValue("audioNotificationsEnabled")}
+                value={this.state.audioNotifications}
                 onChange={this.onAudioNotificationsChanged}
                 label={_t('Enable audible notifications for this session')}
                 disabled={this.state.phase === Phase.Persisting}

src/settings/handlers/AbstractLocalStorageSettingsHandler.ts

@@ -22,7 +22,7 @@ import SettingsHandler from "./SettingsHandler";
  */
 export default abstract class AbstractLocalStorageSettingsHandler extends SettingsHandler {
     // Shared cache between all subclass instances
-    private static itemCache = new Map<string, any>();
+    private static itemCache = new Map<string, string>();
     private static objectCache = new Map<string, object>();
     private static storageListenerBound = false;
 
@@ -51,7 +51,7 @@ export default abstract class AbstractLocalStorageSettingsHandler extends Settin
         }
     }
 
-    protected getItem(key: string): any {
+    protected getItem(key: string): string {
         if (!AbstractLocalStorageSettingsHandler.itemCache.has(key)) {
             const value = localStorage.getItem(key);
             AbstractLocalStorageSettingsHandler.itemCache.set(key, value);
@@ -61,6 +61,14 @@ export default abstract class AbstractLocalStorageSettingsHandler extends Settin
         return AbstractLocalStorageSettingsHandler.itemCache.get(key);
     }
 
+    protected getBoolean(key: string): boolean | null {
+        const item = this.getItem(key);
+        if (item === "true") return true;
+        if (item === "false") return false;
+        // Fall back to the next config level
+        return null;
+    }
+
     protected getObject<T extends object>(key: string): T | null {
         if (!AbstractLocalStorageSettingsHandler.objectCache.has(key)) {
             try {
@@ -76,11 +84,15 @@ export default abstract class AbstractLocalStorageSettingsHandler extends Settin
         return AbstractLocalStorageSettingsHandler.objectCache.get(key) as T;
     }
 
-    protected setItem(key: string, value: any): void {
+    protected setItem(key: string, value: string): void {
         AbstractLocalStorageSettingsHandler.itemCache.set(key, value);
         localStorage.setItem(key, value);
     }
 
+    protected setBoolean(key: string, value: boolean | null): void {
+        this.setItem(key, `${value}`);
+    }
+
     protected setObject(key: string, value: object): void {
         AbstractLocalStorageSettingsHandler.objectCache.set(key, value);
         localStorage.setItem(key, JSON.stringify(value));

src/settings/handlers/DeviceSettingsHandler.ts

@@ -43,17 +43,11 @@ export default class DeviceSettingsHandler extends AbstractLocalStorageSettingsH
 
         // Special case notifications
         if (settingName === "notificationsEnabled") {
-            const value = this.getItem("notifications_enabled");
-            if (typeof(value) === "string") return value === "true";
-            return null; // wrong type or otherwise not set
+            return this.getBoolean("notifications_enabled");
         } else if (settingName === "notificationBodyEnabled") {
-            const value = this.getItem("notifications_body_enabled");
-            if (typeof(value) === "string") return value === "true";
-            return null; // wrong type or otherwise not set
+            return this.getBoolean("notifications_body_enabled");
         } else if (settingName === "audioNotificationsEnabled") {
-            const value = this.getItem("audio_notifications_enabled");
-            if (typeof(value) === "string") return value === "true";
-            return null; // wrong type or otherwise not set
+            return this.getBoolean("audio_notifications_enabled");
         }
 
         const settings = this.getSettings() || {};
@@ -68,15 +62,15 @@ export default class DeviceSettingsHandler extends AbstractLocalStorageSettingsH
 
         // Special case notifications
         if (settingName === "notificationsEnabled") {
-            this.setItem("notifications_enabled", newValue);
+            this.setBoolean("notifications_enabled", newValue);
             this.watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
             return Promise.resolve();
         } else if (settingName === "notificationBodyEnabled") {
-            this.setItem("notifications_body_enabled", newValue);
+            this.setBoolean("notifications_body_enabled", newValue);
             this.watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
             return Promise.resolve();
         } else if (settingName === "audioNotificationsEnabled") {
-            this.setItem("audio_notifications_enabled", newValue);
+            this.setBoolean("audio_notifications_enabled", newValue);
             this.watchers.notifyUpdate(settingName, null, SettingLevel.DEVICE, newValue);
             return Promise.resolve();
         }
@@ -126,15 +120,11 @@ export default class DeviceSettingsHandler extends AbstractLocalStorageSettingsH
             return false;
         }
 
-        const value = this.getItem("mx_labs_feature_" + featureName);
-        if (value === "true") return true;
-        if (value === "false") return false;
-        // Try to read the next config level for the feature.
-        return null;
+        return this.getBoolean("mx_labs_feature_" + featureName);
     }
 
     private writeFeature(featureName: string, enabled: boolean | null) {
-        this.setItem("mx_labs_feature_" + featureName, `${enabled}`);
+        this.setBoolean("mx_labs_feature_" + featureName, enabled);
         this.watchers.notifyUpdate(featureName, null, SettingLevel.DEVICE, enabled);
     }
 }

test/components/views/settings/Notifications-test.tsx

@@ -13,7 +13,7 @@ limitations under the License.
 */
 
 import React from 'react';
-import { mount } from 'enzyme';
+import { mount, ReactWrapper } from 'enzyme';
 import { IPushRule, IPushRules, RuleId, IPusher } from 'matrix-js-sdk/src/matrix';
 import { IThreepid, ThreepidMedium } from 'matrix-js-sdk/src/@types/threepids';
 import { act } from 'react-dom/test-utils';
@@ -23,16 +23,11 @@ import SettingsStore from "../../../../src/settings/SettingsStore";
 import { StandardActions } from '../../../../src/notifications/StandardActions';
 import { getMockClientWithEventEmitter } from '../../../test-utils';
 
-jest.mock('../../../../src/settings/SettingsStore', () => ({
-    monitorSetting: jest.fn(),
-    getValue: jest.fn(),
-    setValue: jest.fn(),
-}));
-
 // don't pollute test output with error logs from mock rejections
 jest.mock("matrix-js-sdk/src/logger");
 
-jest.useRealTimers();
+// Avoid indirectly importing any eagerly created stores that would require extra setup
+jest.mock("../../../../src/Notifier");
 
 const masterRule = {
     actions: ["dont_notify"],
@@ -81,21 +76,13 @@ describe('<Notifications />', () => {
         mockClient.getPushers.mockClear().mockResolvedValue({ pushers: [] });
         mockClient.getThreePids.mockClear().mockResolvedValue({ threepids: [] });
         mockClient.setPusher.mockClear().mockResolvedValue({});
-
-        (SettingsStore.getValue as jest.Mock).mockClear().mockReturnValue(true);
-        (SettingsStore.setValue as jest.Mock).mockClear().mockResolvedValue(true);
     });
 
     it('renders spinner while loading', () => {
         const component = getComponent();
         expect(component.find('.mx_Spinner').length).toBeTruthy();
     });
 
-    it('renders error message when fetching push rules fails', async () => {
-        mockClient.getPushRules.mockRejectedValue({});
-        const component = await getComponentAndWait();
-        expect(findByTestId(component, 'error-message').length).toBeTruthy();
-    });
     it('renders error message when fetching push rules fails', async () => {
         mockClient.getPushRules.mockRejectedValue({});
         const component = await getComponentAndWait();
@@ -221,17 +208,24 @@ describe('<Notifications />', () => {
             });
         });
 
-        it('sets settings value on toggle click', async () => {
+        it('toggles and sets settings correctly', async () => {
             const component = await getComponentAndWait();
+            let audioNotifsToggle: ReactWrapper;
 
-            const audioNotifsToggle = findByTestId(component, 'notif-setting-audioNotificationsEnabled')
-                .find('div[role="switch"]');
+            const update = () => {
+                audioNotifsToggle = findByTestId(component, 'notif-setting-audioNotificationsEnabled')
+                    .find('div[role="switch"]');
+            };
+            update();
 
-            await act(async () => {
-                audioNotifsToggle.simulate('click');
-            });
+            expect(audioNotifsToggle.getDOMNode<HTMLElement>().getAttribute("aria-checked")).toEqual("true");
+            expect(SettingsStore.getValue("audioNotificationsEnabled")).toEqual(true);
+
+            act(() => { audioNotifsToggle.simulate('click'); });
+            update();
 
-            expect(SettingsStore.setValue).toHaveBeenCalledWith('audioNotificationsEnabled', null, "device", false);
+            expect(audioNotifsToggle.getDOMNode<HTMLElement>().getAttribute("aria-checked")).toEqual("false");
+            expect(SettingsStore.getValue("audioNotificationsEnabled")).toEqual(false);
         });
     });