Fix unresponsive notification toggles (#8549) (#8552)

* Return consistently typed setting values from localStorage

* Watch notification toggles

* Test that it all works

(cherry picked from commit 0b0e414fd4)

Co-authored-by: Robin <robin@robin.town>
This commit is contained in:
Michael Telatynski 2022-05-10 14:35:47 +01:00 committed by GitHub
parent 873e9726fd
commit 47b39b3ef7
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
4 changed files with 69 additions and 51 deletions

View file

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

View file

@ -22,7 +22,7 @@ import SettingsHandler from "./SettingsHandler";
*/ */
export default abstract class AbstractLocalStorageSettingsHandler extends SettingsHandler { export default abstract class AbstractLocalStorageSettingsHandler extends SettingsHandler {
// Shared cache between all subclass instances // 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 objectCache = new Map<string, object>();
private static storageListenerBound = false; 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)) { if (!AbstractLocalStorageSettingsHandler.itemCache.has(key)) {
const value = localStorage.getItem(key); const value = localStorage.getItem(key);
AbstractLocalStorageSettingsHandler.itemCache.set(key, value); AbstractLocalStorageSettingsHandler.itemCache.set(key, value);
@ -61,6 +61,14 @@ export default abstract class AbstractLocalStorageSettingsHandler extends Settin
return AbstractLocalStorageSettingsHandler.itemCache.get(key); 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 { protected getObject<T extends object>(key: string): T | null {
if (!AbstractLocalStorageSettingsHandler.objectCache.has(key)) { if (!AbstractLocalStorageSettingsHandler.objectCache.has(key)) {
try { try {
@ -76,11 +84,15 @@ export default abstract class AbstractLocalStorageSettingsHandler extends Settin
return AbstractLocalStorageSettingsHandler.objectCache.get(key) as T; 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); AbstractLocalStorageSettingsHandler.itemCache.set(key, value);
localStorage.setItem(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 { protected setObject(key: string, value: object): void {
AbstractLocalStorageSettingsHandler.objectCache.set(key, value); AbstractLocalStorageSettingsHandler.objectCache.set(key, value);
localStorage.setItem(key, JSON.stringify(value)); localStorage.setItem(key, JSON.stringify(value));

View file

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

View file

@ -13,7 +13,7 @@ limitations under the License.
*/ */
import React from 'react'; 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 { IPushRule, IPushRules, RuleId, IPusher } from 'matrix-js-sdk/src/matrix';
import { IThreepid, ThreepidMedium } from 'matrix-js-sdk/src/@types/threepids'; import { IThreepid, ThreepidMedium } from 'matrix-js-sdk/src/@types/threepids';
import { act } from 'react-dom/test-utils'; import { act } from 'react-dom/test-utils';
@ -23,16 +23,11 @@ import SettingsStore from "../../../../src/settings/SettingsStore";
import { StandardActions } from '../../../../src/notifications/StandardActions'; import { StandardActions } from '../../../../src/notifications/StandardActions';
import { getMockClientWithEventEmitter } from '../../../test-utils'; 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 // don't pollute test output with error logs from mock rejections
jest.mock("matrix-js-sdk/src/logger"); 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 = { const masterRule = {
actions: ["dont_notify"], actions: ["dont_notify"],
@ -81,9 +76,6 @@ describe('<Notifications />', () => {
mockClient.getPushers.mockClear().mockResolvedValue({ pushers: [] }); mockClient.getPushers.mockClear().mockResolvedValue({ pushers: [] });
mockClient.getThreePids.mockClear().mockResolvedValue({ threepids: [] }); mockClient.getThreePids.mockClear().mockResolvedValue({ threepids: [] });
mockClient.setPusher.mockClear().mockResolvedValue({}); 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', () => { it('renders spinner while loading', () => {
@ -91,11 +83,6 @@ describe('<Notifications />', () => {
expect(component.find('.mx_Spinner').length).toBeTruthy(); 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 () => { it('renders error message when fetching push rules fails', async () => {
mockClient.getPushRules.mockRejectedValue({}); mockClient.getPushRules.mockRejectedValue({});
const component = await getComponentAndWait(); 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(); const component = await getComponentAndWait();
let audioNotifsToggle: ReactWrapper;
const audioNotifsToggle = findByTestId(component, 'notif-setting-audioNotificationsEnabled') const update = () => {
audioNotifsToggle = findByTestId(component, 'notif-setting-audioNotificationsEnabled')
.find('div[role="switch"]'); .find('div[role="switch"]');
};
update();
await act(async () => { expect(audioNotifsToggle.getDOMNode<HTMLElement>().getAttribute("aria-checked")).toEqual("true");
audioNotifsToggle.simulate('click'); expect(SettingsStore.getValue("audioNotificationsEnabled")).toEqual(true);
});
expect(SettingsStore.setValue).toHaveBeenCalledWith('audioNotificationsEnabled', null, "device", false); act(() => { audioNotifsToggle.simulate('click'); });
update();
expect(audioNotifsToggle.getDOMNode<HTMLElement>().getAttribute("aria-checked")).toEqual("false");
expect(SettingsStore.getValue("audioNotificationsEnabled")).toEqual(false);
}); });
}); });