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

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;
+    }
 }

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 */

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<RuleId | string, boolean>;
 }
 const findInDefaultRules = (
     ruleId: RuleId | string,
@@ -194,6 +200,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
             desktopShowBody: SettingsStore.getValue("notificationBodyEnabled"),
             audioNotifications: SettingsStore.getValue("audioNotificationsEnabled"),
             clearingNotifications: false,
+            ruleIdsWithError: {},
         };
 
         this.settingWatchers = [
@@ -243,13 +250,9 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
             ).reduce((p, c) => Object.assign(c, p), {});
 
             this.setState<
-                keyof Omit<
+                keyof Pick<
                     IState,
-                    | "deviceNotificationsEnabled"
+                    "phase" | "vectorKeywordRuleInfo" | "vectorPushRules" | "pushers" | "threepids" | "masterPushRule"
-                    | "desktopNotifications"
-                    | "desktopShowBody"
-                    | "audioNotifications"
-                    | "clearingNotifications"
                 >
             >({
                 ...newState,
@@ -393,8 +396,8 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
     private onMasterRuleChanged = async (checked: boolean): Promise<void> => {
         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<IProps, IState> {
         }
     };
 
+    private setSavingError = (ruleId: RuleId | string): void => {
+        this.setState(({ ruleIdsWithError }) => ({
+            phase: Phase.SavingError,
+            ruleIdsWithError: { ...ruleIdsWithError, [ruleId]: true },
+        }));
+    };
+
     private updateDeviceNotifications = async (checked: boolean): Promise<void> => {
         await SettingsStore.setValue("deviceNotificationsEnabled", null, SettingLevel.DEVICE, checked);
     };
@@ -455,11 +465,18 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
     };
 
     private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState): Promise<void> => {
-        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<IProps, IState> {
 
             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<IProps, IState> {
 
     private renderTopSection(): JSX.Element {
         const masterSwitch = (
-            <LabelledToggleSwitch
+            <>
-                data-testid="notif-master-switch"
+                <LabelledToggleSwitch
-                value={!this.isInhibited}
+                    data-testid="notif-master-switch"
-                label={_t("Enable notifications for this account")}
+                    value={!this.isInhibited}
-                caption={_t("Turn off to disable notifications on all your devices and sessions")}
+                    label={_t("Enable notifications for this account")}
-                onChange={this.onMasterRuleChanged}
+                    caption={_t("Turn off to disable notifications on all your devices and sessions")}
-                disabled={this.state.phase === Phase.Persisting}
+                    onChange={this.onMasterRuleChanged}
-            />
+                    disabled={this.state.phase === Phase.Persisting}
+                />
+            </>
         );
 
         // If all the rules are inhibited, don't show anything.
@@ -639,7 +657,7 @@ export default class Notifications extends React.PureComponent<IProps, IState> {
                 <LabelledToggleSwitch
                     data-testid="notif-email-switch"
                     key={e.address}
-                    value={this.state.pushers.some((p) => 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<IProps, IState> {
                 {makeRadio(r, VectorState.Off)}
                 {makeRadio(r, VectorState.On)}
                 {makeRadio(r, VectorState.Loud)}
+                {this.state.ruleIdsWithError[r.ruleId] && (
+                    <div className="mx_UserNotifSettings_gridRowError">
+                        <Caption isError>
+                            {_t(
+                                "An error occurred when updating your notification preferences. Please try to toggle your option again.",
+                            )}
+                        </Caption>
+                    </div>
+                )}
             </fieldset>
         ));
 

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<HTMLAttributes<HTMLSpanElement>, "className"> {
     children: React.ReactNode;
+    isError?: boolean;
 }
 
-export const Caption: React.FC<Props> = ({ children, ...rest }) => {
+export const Caption: React.FC<Props> = ({ children, isError, ...rest }) => {
     return (
-        <span className="mx_Caption" {...rest}>
+        <span
+            className={classNames("mx_Caption", {
+                mx_Caption_error: isError,
+            })}
+            {...rest}
+        >
             {children}
         </span>
     );

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",

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: [
-            {
+            bananaRule,
-                actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }],
-                pattern: "banana",
-                rule_id: "banana",
-                default: false,
-                enabled: true,
-            },
             {
                 actions: [
                     PushRuleActionName.Notify,
@@ -272,7 +275,7 @@ describe("<Notifications />", () => {
         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("<Notifications />", () => {
             });
         });
 
+        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("<Notifications />", () => {
             );
         });
 
+        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("<Notifications />", () => {
                 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("<Notifications />", () => {
                 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("<Notifications />", () => {
                 // 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("<Notifications />", () => {
         });
     });
 
+    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");

test/components/views/typography/Caption-test.tsx

@@ -31,6 +31,11 @@ describe("<Caption />", () => {
         expect({ container }).toMatchSnapshot();
     });
 
+    it("renders an error message", () => {
+        const { container } = render(getComponent({ isError: true }));
+        expect({ container }).toMatchSnapshot();
+    });
+
     it("renders react children", () => {
         const children = (
             <>

test/components/views/typography/__snapshots__/Caption-test.tsx.snap

@@ -1,5 +1,18 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`<Caption /> renders an error message 1`] = `
+{
+  "container": <div>
+    <span
+      class="mx_Caption mx_Caption_error"
+      data-testid="test test id"
+    >
+      test
+    </span>
+  </div>,
+}
+`;
+
 exports[`<Caption /> renders plain text children 1`] = `
 {
   "container": <div>