From 9f6608248683dfc734962563697cfeee550ae5ab Mon Sep 17 00:00:00 2001 From: Kerry Date: Tue, 14 Mar 2023 10:59:04 +1300 Subject: [PATCH] Notifications: inline error message on notifications saving error (#10288) * basic sync setup * formatting * get loudest value for synced rules * more types * test synced rules in notifications settings * type fixes * noimplicitany fixes * remove debug * tidying * extract updatePushRuleActions fn to utils * extract update synced rules * just synchronise in one place? * monitor account data changes AND trigger changes sync in notifications form * lint * setup LoggedInView test with enough mocks * test rule syncing in LoggedInView * strict fixes * more comments * one more comment * add error variant for caption component * tests for new error message * tweak styles * noImplicitAny * revert out of date prettier changes to unrelated files * limit inline message to radios only, tests * strict fix --- .../components/views/typography/_Caption.pcss | 4 + res/css/views/settings/_Notifications.pcss | 8 + .../views/settings/Notifications.tsx | 65 ++++--- src/components/views/typography/Caption.tsx | 11 +- src/i18n/strings/en_EN.json | 1 + .../views/settings/Notifications-test.tsx | 159 ++++++++++++++++-- .../views/typography/Caption-test.tsx | 5 + .../__snapshots__/Caption-test.tsx.snap | 13 ++ 8 files changed, 233 insertions(+), 33 deletions(-) diff --git a/res/css/components/views/typography/_Caption.pcss b/res/css/components/views/typography/_Caption.pcss index 3af270c4a5..f51276d9f9 100644 --- a/res/css/components/views/typography/_Caption.pcss +++ b/res/css/components/views/typography/_Caption.pcss @@ -17,4 +17,8 @@ limitations under the License. .mx_Caption { font-size: $font-12px; color: $secondary-content; + + &.mx_Caption_error { + color: $alert; + } } diff --git a/res/css/views/settings/_Notifications.pcss b/res/css/views/settings/_Notifications.pcss index 7357ee1c68..45a5b4529d 100644 --- a/res/css/views/settings/_Notifications.pcss +++ b/res/css/views/settings/_Notifications.pcss @@ -61,6 +61,14 @@ limitations under the License. font-size: $font-12px; font-weight: $font-semi-bold; } +.mx_UserNotifSettings_gridRowError { + // occupy full row + grid-column: 1/-1; + justify-self: start; + padding-right: 30%; + // collapse half of the grid-gap + margin-top: -$spacing-4; +} .mx_UserNotifSettings { color: $primary-content; /* override from default settings page styles */ diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 3aa4edf8a4..6aa7dca132 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -47,6 +47,7 @@ import { updateExistingPushRulesWithActions, updatePushRuleActions, } from "../../../utils/pushRules/updatePushRuleActions"; +import { Caption } from "../typography/Caption"; // TODO: this "view" component still has far too much application logic in it, // which should be factored out to other files. @@ -55,7 +56,10 @@ enum Phase { Loading = "loading", Ready = "ready", Persisting = "persisting", // technically a meta-state for Ready, but whatever + // unrecoverable error - eg can't load push rules Error = "error", + // error saving individual rule + SavingError = "savingError", } enum RuleClass { @@ -121,6 +125,8 @@ interface IState { audioNotifications: boolean; clearingNotifications: boolean; + + ruleIdsWithError: Record; } const findInDefaultRules = ( ruleId: RuleId | string, @@ -194,6 +200,7 @@ export default class Notifications extends React.PureComponent { desktopShowBody: SettingsStore.getValue("notificationBodyEnabled"), audioNotifications: SettingsStore.getValue("audioNotificationsEnabled"), clearingNotifications: false, + ruleIdsWithError: {}, }; this.settingWatchers = [ @@ -243,13 +250,9 @@ export default class Notifications extends React.PureComponent { ).reduce((p, c) => Object.assign(c, p), {}); this.setState< - keyof Omit< + keyof Pick< IState, - | "deviceNotificationsEnabled" - | "desktopNotifications" - | "desktopShowBody" - | "audioNotifications" - | "clearingNotifications" + "phase" | "vectorKeywordRuleInfo" | "vectorPushRules" | "pushers" | "threepids" | "masterPushRule" > >({ ...newState, @@ -393,8 +396,8 @@ export default class Notifications extends React.PureComponent { private onMasterRuleChanged = async (checked: boolean): Promise => { this.setState({ phase: Phase.Persisting }); + const masterRule = this.state.masterPushRule!; try { - const masterRule = this.state.masterPushRule!; await MatrixClientPeg.get().setPushRuleEnabled("global", masterRule.kind, masterRule.rule_id, !checked); await this.refreshFromServer(); } catch (e) { @@ -404,6 +407,13 @@ export default class Notifications extends React.PureComponent { } }; + private setSavingError = (ruleId: RuleId | string): void => { + this.setState(({ ruleIdsWithError }) => ({ + phase: Phase.SavingError, + ruleIdsWithError: { ...ruleIdsWithError, [ruleId]: true }, + })); + }; + private updateDeviceNotifications = async (checked: boolean): Promise => { await SettingsStore.setValue("deviceNotificationsEnabled", null, SettingLevel.DEVICE, checked); }; @@ -455,11 +465,18 @@ export default class Notifications extends React.PureComponent { }; private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState): Promise => { - this.setState({ phase: Phase.Persisting }); + this.setState(({ ruleIdsWithError }) => ({ + phase: Phase.Persisting, + ruleIdsWithError: { ...ruleIdsWithError, [rule.ruleId]: false }, + })); try { const cli = MatrixClientPeg.get(); if (rule.ruleId === KEYWORD_RULE_ID) { + // should not encounter this + if (!this.state.vectorKeywordRuleInfo) { + throw new Error("Notification data is incomplete."); + } // Update all the keywords for (const rule of this.state.vectorKeywordRuleInfo.rules) { let enabled: boolean | undefined; @@ -505,9 +522,8 @@ export default class Notifications extends React.PureComponent { await this.refreshFromServer(); } catch (e) { - this.setState({ phase: Phase.Error }); + this.setSavingError(rule.ruleId); logger.error("Error updating push rule:", e); - this.showSaveError(); } }; @@ -618,14 +634,16 @@ export default class Notifications extends React.PureComponent { private renderTopSection(): JSX.Element { const masterSwitch = ( - + <> + + ); // If all the rules are inhibited, don't show anything. @@ -639,7 +657,7 @@ export default class Notifications extends React.PureComponent { p.kind === "email" && p.pushkey === e.address)} + value={!!this.state.pushers?.some((p) => p.kind === "email" && p.pushkey === e.address)} label={_t("Enable email notifications for %(email)s", { email: e.address })} onChange={this.onEmailNotificationsChanged.bind(this, e.address)} disabled={this.state.phase === Phase.Persisting} @@ -768,6 +786,15 @@ export default class Notifications extends React.PureComponent { {makeRadio(r, VectorState.Off)} {makeRadio(r, VectorState.On)} {makeRadio(r, VectorState.Loud)} + {this.state.ruleIdsWithError[r.ruleId] && ( +
+ + {_t( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + )} + +
+ )} )); diff --git a/src/components/views/typography/Caption.tsx b/src/components/views/typography/Caption.tsx index 4ec7c20152..69e7714b22 100644 --- a/src/components/views/typography/Caption.tsx +++ b/src/components/views/typography/Caption.tsx @@ -14,15 +14,22 @@ See the License for the specific language governing permissions and limitations under the License. */ +import classNames from "classnames"; import React, { HTMLAttributes } from "react"; interface Props extends Omit, "className"> { children: React.ReactNode; + isError?: boolean; } -export const Caption: React.FC = ({ children, ...rest }) => { +export const Caption: React.FC = ({ children, isError, ...rest }) => { return ( - + {children} ); diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 7fd40936e9..fb34c24501 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1443,6 +1443,7 @@ "On": "On", "Off": "Off", "Noisy": "Noisy", + "An error occurred when updating your notification preferences. Please try to toggle your option again.": "An error occurred when updating your notification preferences. Please try to toggle your option again.", "Global": "Global", "Mentions & keywords": "Mentions & keywords", "Notification targets": "Notification targets", diff --git a/test/components/views/settings/Notifications-test.tsx b/test/components/views/settings/Notifications-test.tsx index 2aa4c93a3b..5084dfb258 100644 --- a/test/components/views/settings/Notifications-test.tsx +++ b/test/components/views/settings/Notifications-test.tsx @@ -28,7 +28,7 @@ import { IPushRuleCondition, } from "matrix-js-sdk/src/matrix"; import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; -import { act, fireEvent, getByTestId, render, screen, waitFor } from "@testing-library/react"; +import { act, fireEvent, getByTestId, render, screen, waitFor, within } from "@testing-library/react"; import Notifications from "../../../../src/components/views/settings/Notifications"; import SettingsStore from "../../../../src/settings/SettingsStore"; @@ -90,6 +90,15 @@ const encryptedGroupRule: IPushRule = { default: true, enabled: true, } as IPushRule; + +const bananaRule = { + actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }], + pattern: "banana", + rule_id: "banana", + default: false, + enabled: true, +} as IPushRule; + const pushRules: IPushRules = { global: { underride: [ @@ -130,13 +139,7 @@ const pushRules: IPushRules = { }, ], content: [ - { - actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }], - pattern: "banana", - rule_id: "banana", - default: false, - enabled: true, - }, + bananaRule, { actions: [ PushRuleActionName.Notify, @@ -272,7 +275,7 @@ describe("", () => { mockClient.getPushers.mockClear().mockResolvedValue({ pushers: [] }); mockClient.getThreePids.mockClear().mockResolvedValue({ threepids: [] }); mockClient.setPusher.mockClear().mockResolvedValue({}); - mockClient.setPushRuleActions.mockClear().mockResolvedValue({}); + mockClient.setPushRuleActions.mockReset().mockResolvedValue({}); mockClient.pushRules = pushRules; }); @@ -395,6 +398,18 @@ describe("", () => { }); }); + it("toggles master switch correctly", async () => { + await getComponentAndWait(); + + // master switch is on + expect(screen.getByLabelText("Enable notifications for this account")).toBeChecked(); + fireEvent.click(screen.getByLabelText("Enable notifications for this account")); + + await flushPromises(); + + expect(mockClient.setPushRuleEnabled).toHaveBeenCalledWith("global", "override", ".m.rule.master", true); + }); + it("toggles and sets settings correctly", async () => { await getComponentAndWait(); let audioNotifsToggle!: HTMLDivElement; @@ -473,6 +488,73 @@ describe("", () => { ); }); + it("adds an error message when updating notification level fails", async () => { + await getComponentAndWait(); + const section = "vector_global"; + + const error = new Error("oups"); + mockClient.setPushRuleEnabled.mockRejectedValue(error); + + // oneToOneRule is set to 'on' + // and is kind: 'underride' + const offToggle = screen.getByTestId(section + oneToOneRule.rule_id).querySelector('input[type="radio"]')!; + fireEvent.click(offToggle); + + await flushPromises(); + + // error message attached to oneToOne rule + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + // old value still shown as selected + expect(within(oneToOneRuleElement).getByLabelText("On")).toBeChecked(); + expect( + within(oneToOneRuleElement).getByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).toBeInTheDocument(); + }); + + it("clears error message for notification rule on retry", async () => { + await getComponentAndWait(); + const section = "vector_global"; + + const error = new Error("oups"); + mockClient.setPushRuleEnabled.mockRejectedValueOnce(error).mockResolvedValue({}); + + // oneToOneRule is set to 'on' + // and is kind: 'underride' + const offToggle = screen.getByTestId(section + oneToOneRule.rule_id).querySelector('input[type="radio"]')!; + fireEvent.click(offToggle); + + await flushPromises(); + + // error message attached to oneToOne rule + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + expect( + within(oneToOneRuleElement).getByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).toBeInTheDocument(); + + // retry + fireEvent.click(offToggle); + + // error removed as soon as we start request + expect( + within(oneToOneRuleElement).queryByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).not.toBeInTheDocument(); + + await flushPromises(); + + // no error after after successful change + expect( + within(oneToOneRuleElement).queryByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).not.toBeInTheDocument(); + }); + describe("synced rules", () => { const pollStartOneToOne = { conditions: [ @@ -554,7 +636,11 @@ describe("", () => { expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(1); // no error - expect(screen.queryByTestId("error-message")).not.toBeInTheDocument(); + expect( + within(oneToOneRuleElement).queryByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).not.toBeInTheDocument(); }); it("updates synced rules when they exist for user", async () => { @@ -585,7 +671,11 @@ describe("", () => { expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(2); // no error - expect(screen.queryByTestId("error-message")).not.toBeInTheDocument(); + expect( + within(oneToOneRuleElement).queryByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).not.toBeInTheDocument(); }); it("does not update synced rules when main rule update fails", async () => { @@ -610,7 +700,11 @@ describe("", () => { // only called for parent rule expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(1); - expect(screen.queryByTestId("error-message")).toBeInTheDocument(); + expect( + within(oneToOneRuleElement).getByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).toBeInTheDocument(); }); it("sets the UI toggle to rule value when no synced rule exist for the user", async () => { @@ -664,6 +758,47 @@ describe("", () => { }); }); + describe("keywords", () => { + // keywords rule is not a real rule, but controls actions on keywords content rules + const keywordsRuleId = "_keywords"; + it("updates individual keywords content rules when keywords rule is toggled", async () => { + await getComponentAndWait(); + const section = "vector_mentions"; + + fireEvent.click(within(screen.getByTestId(section + keywordsRuleId)).getByLabelText("Off")); + + expect(mockClient.setPushRuleEnabled).toHaveBeenCalledWith("global", "content", bananaRule.rule_id, false); + + fireEvent.click(within(screen.getByTestId(section + keywordsRuleId)).getByLabelText("Noisy")); + + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "content", + bananaRule.rule_id, + StandardActions.ACTION_HIGHLIGHT_DEFAULT_SOUND, + ); + }); + + it("renders an error when updating keywords fails", async () => { + await getComponentAndWait(); + const section = "vector_mentions"; + + mockClient.setPushRuleEnabled.mockRejectedValueOnce("oups"); + + fireEvent.click(within(screen.getByTestId(section + keywordsRuleId)).getByLabelText("Off")); + + await flushPromises(); + + const rule = screen.getByTestId(section + keywordsRuleId); + + expect( + within(rule).getByText( + "An error occurred when updating your notification preferences. Please try to toggle your option again.", + ), + ).toBeInTheDocument(); + }); + }); + describe("clear all notifications", () => { it("clears all notifications", async () => { const room = new Room("room123", mockClient, "@alice:example.org"); diff --git a/test/components/views/typography/Caption-test.tsx b/test/components/views/typography/Caption-test.tsx index ca3258f725..bd684ca0f8 100644 --- a/test/components/views/typography/Caption-test.tsx +++ b/test/components/views/typography/Caption-test.tsx @@ -31,6 +31,11 @@ describe("", () => { expect({ container }).toMatchSnapshot(); }); + it("renders an error message", () => { + const { container } = render(getComponent({ isError: true })); + expect({ container }).toMatchSnapshot(); + }); + it("renders react children", () => { const children = ( <> diff --git a/test/components/views/typography/__snapshots__/Caption-test.tsx.snap b/test/components/views/typography/__snapshots__/Caption-test.tsx.snap index bb3c380a80..b17d5044b0 100644 --- a/test/components/views/typography/__snapshots__/Caption-test.tsx.snap +++ b/test/components/views/typography/__snapshots__/Caption-test.tsx.snap @@ -1,5 +1,18 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[` renders an error message 1`] = ` +{ + "container":
+ + test + +
, +} +`; + exports[` renders plain text children 1`] = ` { "container":