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

Author: Kerry

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"
-                    | "desktopNotifications"
-                    | "desktopShowBody"
-                    | "audioNotifications"
-                    | "clearingNotifications"
+                    "phase" | "vectorKeywordRuleInfo" | "vectorPushRules" | "pushers" | "threepids" | "masterPushRule"
                 >
             >({
                 ...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"
-                value={!this.isInhibited}
-                label={_t("Enable notifications for this account")}
-                caption={_t("Turn off to disable notifications on all your devices and sessions")}
-                onChange={this.onMasterRuleChanged}
-                disabled={this.state.phase === Phase.Persisting}
-            />
+            <>
+                <LabelledToggleSwitch
+                    data-testid="notif-master-switch"
+                    value={!this.isInhibited}
+                    label={_t("Enable notifications for this account")}
+                    caption={_t("Turn off to disable notifications on all your devices and sessions")}
+                    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",