From 6f31ac013989d1823a3917175fa7150023c4f9a9 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Tue, 28 Feb 2023 18:33:33 +1300 Subject: [PATCH 01/26] basic sync setup --- .../views/settings/Notifications.tsx | 64 +++++++++++++++++-- .../VectorPushRulesDefinitions.ts | 16 ++++- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 3fb902ba5e9..f16dd990cc1 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -15,7 +15,14 @@ limitations under the License. */ import React, { ReactNode } from "react"; -import { IAnnotatedPushRule, IPusher, PushRuleAction, PushRuleKind, RuleId } from "matrix-js-sdk/src/@types/PushRules"; +import { + IAnnotatedPushRule, + IPusher, + PushRuleAction, + IPushRule, + PushRuleKind, + RuleId, +} from "matrix-js-sdk/src/@types/PushRules"; import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { LocalNotificationSettings } from "matrix-js-sdk/src/@types/local_notifications"; @@ -43,6 +50,7 @@ import TagComposer from "../elements/TagComposer"; import { objectClone } from "../../../utils/objects"; import { arrayDiff } from "../../../utils/arrays"; import { clearAllNotifications, getLocalNotificationAccountDataEventType } from "../../../utils/notifications"; +import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; // TODO: this "view" component still has far too much application logic in it, // which should be factored out to other files. @@ -118,6 +126,7 @@ interface IState { export default class Notifications extends React.PureComponent { private settingWatchers: string[]; + private pushProcessor: PushProcessor; public constructor(props: IProps) { super(props); @@ -145,6 +154,8 @@ export default class Notifications extends React.PureComponent { this.setState({ audioNotifications: value as boolean }), ), ]; + + this.pushProcessor = new PushProcessor(MatrixClientPeg.get()); } private get isInhibited(): boolean { @@ -306,6 +317,8 @@ export default class Notifications extends React.PureComponent { } } + console.log("hhh", { preparedNewState, ruleSets }, MatrixClientPeg.get().pushRules); + return preparedNewState; } @@ -388,6 +401,47 @@ export default class Notifications extends React.PureComponent { await SettingsStore.setValue("audioNotificationsEnabled", null, SettingLevel.DEVICE, checked); }; + private setPushRuleActions = async ( + ruleId: IPushRule["rule_id"], + kind: PushRuleKind, + actions?: PushRuleAction[], + ): Promise => { + const cli = MatrixClientPeg.get(); + if (!actions) { + await cli.setPushRuleEnabled("global", kind, ruleId, false); + } else { + await cli.setPushRuleActions("global", kind, ruleId, actions); + await cli.setPushRuleEnabled("global", kind, ruleId, true); + } + }; + + /** + * Updated syncedRuleIds from rule definition + * If a rule does not exist it is ignored + * Synced rules are updated sequentially + * and stop silently at first error + */ + private updateSyncedRules = async ( + syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], + rule: IVectorPushRule, + actions?: PushRuleAction[], + ) => { + console.log("hhh updateSyncedRules", { syncedRuleIds, actions }); + const syncedRules = syncedRuleIds?.map((ruleId) => this.pushProcessor.getPushRuleByKindAndId(ruleId, rule.rule.kind)).filter(Boolean); + console.log("hhh syncedRules", { syncedRuleIds, syncedRules }); + if (!syncedRules?.length) { + return; + } + try { + for (let syncedRule of syncedRules) { + console.log("hhh", { syncedRule }); + await this.setPushRuleActions(syncedRule.rule_id, rule.rule.kind, actions); + } + } catch (error) { + logger.error(`Failed to update all synced rules for ${rule.ruleId}`, error); + } + }; + private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState): Promise => { this.setState({ phase: Phase.Persisting }); @@ -428,12 +482,8 @@ export default class Notifications extends React.PureComponent { } else { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; - if (!actions) { - await cli.setPushRuleEnabled("global", rule.rule.kind, rule.rule.rule_id, false); - } else { - await cli.setPushRuleActions("global", rule.rule.kind, rule.rule.rule_id, actions); - await cli.setPushRuleEnabled("global", rule.rule.kind, rule.rule.rule_id, true); - } + await this.setPushRuleActions(rule.rule.rule_id, rule.rule.kind, actions); + await this.updateSyncedRules(definition.syncedRuleIds, rule, actions); } await this.refreshFromServer(); diff --git a/src/notifications/VectorPushRulesDefinitions.ts b/src/notifications/VectorPushRulesDefinitions.ts index 81a9c84a4e1..59d57b6e461 100644 --- a/src/notifications/VectorPushRulesDefinitions.ts +++ b/src/notifications/VectorPushRulesDefinitions.ts @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { IAnnotatedPushRule, PushRuleAction } from "matrix-js-sdk/src/@types/PushRules"; +import { IAnnotatedPushRule, PushRuleAction, RuleId } from "matrix-js-sdk/src/@types/PushRules"; import { logger } from "matrix-js-sdk/src/logger"; import { _td } from "../languageHandler"; @@ -29,15 +29,22 @@ type StateToActionsMap = { interface IVectorPushRuleDefinition { description: string; vectorStateToActions: StateToActionsMap; + /** + * Rules that should be updated to be kept in sync + * when this rule changes + */ + syncedRuleIds?: (RuleId | string)[]; } class VectorPushRuleDefinition { public readonly description: string; public readonly vectorStateToActions: StateToActionsMap; + public readonly syncedRuleIds: (RuleId | string)[]; public constructor(opts: IVectorPushRuleDefinition) { this.description = opts.description; this.vectorStateToActions = opts.vectorStateToActions; + this.syncedRuleIds = opts.syncedRuleIds || []; } // Translate the rule actions and its enabled value into vector state @@ -125,6 +132,12 @@ export const VectorPushRulesDefinitions: Record Date: Wed, 1 Mar 2023 10:52:29 +1300 Subject: [PATCH 02/26] formatting --- .../views/settings/Notifications.tsx | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index f16dd990cc1..a200629b00f 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -419,7 +419,7 @@ export default class Notifications extends React.PureComponent { * Updated syncedRuleIds from rule definition * If a rule does not exist it is ignored * Synced rules are updated sequentially - * and stop silently at first error + * and stop at first error */ private updateSyncedRules = async ( syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], @@ -427,19 +427,20 @@ export default class Notifications extends React.PureComponent { actions?: PushRuleAction[], ) => { console.log("hhh updateSyncedRules", { syncedRuleIds, actions }); - const syncedRules = syncedRuleIds?.map((ruleId) => this.pushProcessor.getPushRuleByKindAndId(ruleId, rule.rule.kind)).filter(Boolean); + const syncedRules: ReturnType[] = syncedRuleIds + ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)) + .filter(Boolean); console.log("hhh syncedRules", { syncedRuleIds, syncedRules }); if (!syncedRules?.length) { return; } - try { - for (let syncedRule of syncedRules) { - console.log("hhh", { syncedRule }); - await this.setPushRuleActions(syncedRule.rule_id, rule.rule.kind, actions); - } - } catch (error) { - logger.error(`Failed to update all synced rules for ${rule.ruleId}`, error); + // try { + for (let { kind, rule: syncedRule } of syncedRules) { + await this.setPushRuleActions(syncedRule.rule_id, kind, actions); } + // } catch (error) { + // logger.error(`Failed to update all synced rules for ${rule.ruleId}`, error); + // } }; private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState): Promise => { From cfac7f70ddf5fb2ed1dd02d629fe9e3ae18ca3b9 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Wed, 1 Mar 2023 15:29:46 +1300 Subject: [PATCH 03/26] get loudest value for synced rules --- .../views/settings/Notifications.tsx | 62 ++++++++++++++++--- .../VectorPushRulesDefinitions.ts | 4 +- 2 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index a200629b00f..1fa5a9d36bc 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -26,6 +26,7 @@ import { import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { LocalNotificationSettings } from "matrix-js-sdk/src/@types/local_notifications"; +import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; import Spinner from "../elements/Spinner"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; @@ -50,7 +51,6 @@ import TagComposer from "../elements/TagComposer"; import { objectClone } from "../../../utils/objects"; import { arrayDiff } from "../../../utils/arrays"; import { clearAllNotifications, getLocalNotificationAccountDataEventType } from "../../../utils/notifications"; -import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; // TODO: this "view" component still has far too much application logic in it, // which should be factored out to other files. @@ -100,6 +100,9 @@ interface IVectorPushRule { rule?: IAnnotatedPushRule; description: TranslatedString | string; vectorState: VectorState; + // loudest vectorState of a rule and its synced rules + // undefined when rule has no synced rules + syncedVectorState?: VectorState; } interface IProps {} @@ -123,6 +126,48 @@ interface IState { clearingNotifications: boolean; } +const findInDefaultRules = (ruleId: RuleId | string, defaultRules): IAnnotatedPushRule | undefined => { + for (const category in defaultRules) { + const rule = defaultRules[category].find((rule) => rule.rule_id === ruleId); + if (rule) { + return rule; + } + } +}; +const OrderedVectorStates = [VectorState.Off, VectorState.On, VectorState.Loud]; +/** + * Find the 'loudest' vector state assigned to a rule + * and it's synced rules + * If rules have fallen out of sync, + * the loudest rule can determine the display value + * @param defaultRules + * @param rule - parent rule + * @param definition - definition of parent rule + * @returns VectorState - the maximum/loudest state for the parent and synced rules + */ +const maximumVectorState = (defaultRules, rule, definition: VectorPushRuleDefinition): VectorState | undefined => { + if (!definition.syncedRuleIds?.length) { + return undefined; + } + const vectorState = definition.syncedRuleIds.reduce((maxVectorState, ruleId) => { + // already set to maximum + if (maxVectorState === VectorState.Loud) { + return maxVectorState; + } + const syncedRule = findInDefaultRules(ruleId, defaultRules); + if (syncedRule) { + const syncedRuleVectorState = definition.ruleToVectorState(syncedRule); + // if syncedRule is 'louder' than current maximum + // set maximum to louder vectorState + if (OrderedVectorStates.indexOf(syncedRuleVectorState) > OrderedVectorStates.indexOf(maxVectorState)) { + return syncedRuleVectorState; + } + } + return maxVectorState; + }, definition.ruleToVectorState(rule)); + + return vectorState; +}; export default class Notifications extends React.PureComponent { private settingWatchers: string[]; @@ -292,6 +337,7 @@ export default class Notifications extends React.PureComponent { ruleId: rule.rule_id, rule, vectorState, + syncedVectorState: maximumVectorState(defaultRules, rule, definition), description: _t(definition.description), }); } @@ -425,22 +471,18 @@ export default class Notifications extends React.PureComponent { syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], rule: IVectorPushRule, actions?: PushRuleAction[], - ) => { - console.log("hhh updateSyncedRules", { syncedRuleIds, actions }); + ): Promise => { + // get synced rules that exist for user const syncedRules: ReturnType[] = syncedRuleIds ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)) .filter(Boolean); - console.log("hhh syncedRules", { syncedRuleIds, syncedRules }); + if (!syncedRules?.length) { return; } - // try { - for (let { kind, rule: syncedRule } of syncedRules) { + for (const { kind, rule: syncedRule } of syncedRules) { await this.setPushRuleActions(syncedRule.rule_id, kind, actions); } - // } catch (error) { - // logger.error(`Failed to update all synced rules for ${rule.ruleId}`, error); - // } }; private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState): Promise => { @@ -735,7 +777,7 @@ export default class Notifications extends React.PureComponent { Date: Wed, 1 Mar 2023 15:41:32 +1300 Subject: [PATCH 04/26] more types --- .../views/settings/Notifications.tsx | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 1fa5a9d36bc..f1ec34fa422 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -126,7 +126,12 @@ interface IState { clearingNotifications: boolean; } -const findInDefaultRules = (ruleId: RuleId | string, defaultRules): IAnnotatedPushRule | undefined => { +const findInDefaultRules = ( + ruleId: RuleId | string, + defaultRules: { + [k in RuleClass]: IAnnotatedPushRule[]; + }, +): IAnnotatedPushRule | undefined => { for (const category in defaultRules) { const rule = defaultRules[category].find((rule) => rule.rule_id === ruleId); if (rule) { @@ -145,7 +150,13 @@ const OrderedVectorStates = [VectorState.Off, VectorState.On, VectorState.Loud]; * @param definition - definition of parent rule * @returns VectorState - the maximum/loudest state for the parent and synced rules */ -const maximumVectorState = (defaultRules, rule, definition: VectorPushRuleDefinition): VectorState | undefined => { +const maximumVectorState = ( + defaultRules: { + [k in RuleClass]: IAnnotatedPushRule[]; + }, + rule: IAnnotatedPushRule, + definition: VectorPushRuleDefinition, +): VectorState | undefined => { if (!definition.syncedRuleIds?.length) { return undefined; } @@ -469,7 +480,6 @@ export default class Notifications extends React.PureComponent { */ private updateSyncedRules = async ( syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], - rule: IVectorPushRule, actions?: PushRuleAction[], ): Promise => { // get synced rules that exist for user @@ -526,7 +536,7 @@ export default class Notifications extends React.PureComponent { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; await this.setPushRuleActions(rule.rule.rule_id, rule.rule.kind, actions); - await this.updateSyncedRules(definition.syncedRuleIds, rule, actions); + await this.updateSyncedRules(definition.syncedRuleIds, actions); } await this.refreshFromServer(); From 19dbc9564f109eb23ac15103097e6d5e9f39482b Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 2 Mar 2023 14:10:23 +1300 Subject: [PATCH 05/26] test synced rules in notifications settings --- .../views/settings/Notifications.tsx | 5 +- .../views/settings/Notifications-test.tsx | 187 +++++++++++++++++- 2 files changed, 184 insertions(+), 8 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index f1ec34fa422..bcd2182fac1 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -258,6 +258,7 @@ export default class Notifications extends React.PureComponent { phase: Phase.Ready, }); } catch (e) { + console.log(e); logger.error("Error setting up notifications for settings: ", e); this.setState({ phase: Phase.Error }); } @@ -374,7 +375,7 @@ export default class Notifications extends React.PureComponent { } } - console.log("hhh", { preparedNewState, ruleSets }, MatrixClientPeg.get().pushRules); + console.log("hhh", { preparedNewState, ruleSets }, MatrixClientPeg.get().pushRules.global.underride); return preparedNewState; } @@ -482,6 +483,8 @@ export default class Notifications extends React.PureComponent { syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], actions?: PushRuleAction[], ): Promise => { + console.log('updatesynedce', syncedRuleIds, syncedRuleIds + ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId))) // get synced rules that exist for user const syncedRules: ReturnType[] = syncedRuleIds ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)) diff --git a/test/components/views/settings/Notifications-test.tsx b/test/components/views/settings/Notifications-test.tsx index a30eb1b8434..be87290f6c3 100644 --- a/test/components/views/settings/Notifications-test.tsx +++ b/test/components/views/settings/Notifications-test.tsx @@ -65,6 +65,13 @@ const encryptedOneToOneRule: IPushRule = { default: true, enabled: true, } as IPushRule; +const groupRule = { + conditions: [{ kind: "event_match", key: "type", pattern: "m.room.message" }], + actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight", value: false }], + rule_id: ".m.rule.message", + default: true, + enabled: true, +}; const encryptedGroupRule: IPushRule = { conditions: [{ kind: "event_match", key: "type", pattern: "m.room.encrypted" }], actions: ["dont_notify"], @@ -84,13 +91,7 @@ const pushRules: IPushRules = { }, oneToOneRule, encryptedOneToOneRule, - { - conditions: [{ kind: "event_match", key: "type", pattern: "m.room.message" }], - actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight", value: false }], - rule_id: ".m.rule.message", - default: true, - enabled: true, - }, + groupRule, encryptedGroupRule, { conditions: [ @@ -231,6 +232,8 @@ describe("", () => { mockClient.getPushers.mockClear().mockResolvedValue({ pushers: [] }); mockClient.getThreePids.mockClear().mockResolvedValue({ threepids: [] }); mockClient.setPusher.mockClear().mockResolvedValue({}); + mockClient.setPushRuleActions.mockClear().mockResolvedValue({}); + mockClient.pushRules = pushRules; }); it("renders spinner while loading", async () => { @@ -429,6 +432,176 @@ describe("", () => { StandardActions.ACTION_DONT_NOTIFY, ); }); + + describe("synced rules", () => { + const pollStartOneToOne = { + conditions: [ + { + kind: "room_member_count", + is: "2", + }, + { + kind: "event_match", + key: "type", + pattern: "org.matrix.msc3381.poll.start", + }, + ], + actions: [PushRuleActionName.DontNotify], + rule_id: ".org.matrix.msc3930.rule.poll_start_one_to_one", + default: true, + enabled: true, + }; + const pollStartGroup = { + conditions: [ + { + kind: "event_match", + key: "type", + pattern: "org.matrix.msc3381.poll.start", + }, + ], + actions: ["notify"], + rule_id: ".org.matrix.msc3930.rule.poll_start", + default: true, + enabled: true, + }; + const pollEndOneToOne = { + conditions: [ + { + kind: "room_member_count", + is: "2", + }, + { + kind: "event_match", + key: "type", + pattern: "org.matrix.msc3381.poll.end", + }, + ], + actions: [PushRuleActionName.Notify, + { set_tweak: "highlight", value: false }, + { set_tweak: "sound", value: "default" } + ], + rule_id: ".org.matrix.msc3930.rule.poll_end_one_to_one", + default: true, + enabled: true, + }; + + const setPushRuleMock = (rules: IPushRule[] = []): void => { + const combinedRules = { + ...pushRules, + global: { + ...pushRules.global, + underride: [...pushRules.global.underride!, ...rules], + }, + }; + mockClient.getPushRules.mockClear().mockResolvedValue(combinedRules); + mockClient.pushRules = combinedRules; + }; + + // ".m.rule.room_one_to_one" and ".m.rule.message" have synced rules + it("succeeds when no synced rules exist for user", async () => { + await getComponentAndWait(); + const section = "vector_global"; + + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + + const offToggle = oneToOneRuleElement.querySelector('input[type="radio"]')!; + fireEvent.click(offToggle); + + await flushPromises(); + + // didnt attempt to update any non-existant rules + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(1); + + // no error + expect(screen.queryByTestId("error-message")).not.toBeInTheDocument(); + }); + + it("updates synced rules when they exist for user", async () => { + setPushRuleMock([pollStartOneToOne, pollStartGroup]); + await getComponentAndWait(); + const section = "vector_global"; + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + + const offToggle = oneToOneRuleElement.querySelector('input[type="radio"]')!; + fireEvent.click(offToggle); + + await flushPromises(); + + // updated synced rule + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + 'global', 'underride', oneToOneRule.rule_id, [PushRuleActionName.DontNotify] + ); + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + 'global', 'underride', pollStartOneToOne.rule_id, [PushRuleActionName.DontNotify] + ); + // only called for parent rule and one existing synced rule + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(2); + + // no error + expect(screen.queryByTestId("error-message")).not.toBeInTheDocument(); + }); + + it("does not update synced rules when main rule update fails", async () => { + setPushRuleMock([pollStartOneToOne]); + await getComponentAndWait(); + const section = "vector_global"; + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + // have main rule update fail + mockClient.setPushRuleActions.mockRejectedValue('oups') + + const offToggle = oneToOneRuleElement.querySelector('input[type="radio"]')!; + fireEvent.click(offToggle); + + await flushPromises(); + + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + 'global', 'underride', oneToOneRule.rule_id, [PushRuleActionName.DontNotify] + ); + // only called for parent rule + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(1); + + expect(screen.queryByTestId("error-message")).toBeInTheDocument(); + }); + + it("sets the UI toggle to rule value when no synced rule exist for the user", async () => { + setPushRuleMock([]); + await getComponentAndWait(); + const section = "vector_global"; + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + + // loudest state of synced rules should be the toggle value + expect(oneToOneRuleElement.querySelector('input[aria-label="On"]')).toBeChecked(); + }); + + it("sets the UI toggle to the loudest synced rule value", async () => { + // oneToOneRule is set to 'On' + // pollEndOneToOne is set to 'Loud' + setPushRuleMock([pollStartOneToOne, pollEndOneToOne]); + await getComponentAndWait(); + const section = "vector_global"; + const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); + + // loudest state of synced rules should be the toggle value + expect(oneToOneRuleElement.querySelector('input[aria-label="Noisy"]')).toBeChecked(); + + console.log('ONONONONONONON') + + const onToggle = oneToOneRuleElement.querySelector('input[aria-label="On"]')!; + fireEvent.click(onToggle); + + await flushPromises(); + + // called for all 3 rules + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(3); + const expectedActions = [ + PushRuleActionName.Notify, { set_tweak: "highlight", value: false } + ] + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith("global", "underride", oneToOneRule.rule_id, expectedActions); + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith("global", "underride", pollStartOneToOne.rule_id, expectedActions); + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith("global", "underride", pollEndOneToOne.rule_id, expectedActions); + }); + + }); }); describe("clear all notifications", () => { From e69c65ae491dcd986e9192357f3a58088ec2e460 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 2 Mar 2023 14:36:03 +1300 Subject: [PATCH 06/26] type fixes --- .../views/settings/Notifications.tsx | 7 +- .../views/settings/Notifications-test.tsx | 188 ++++++++++++------ 2 files changed, 129 insertions(+), 66 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index bcd2182fac1..42ab33f8c41 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -483,8 +483,11 @@ export default class Notifications extends React.PureComponent { syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], actions?: PushRuleAction[], ): Promise => { - console.log('updatesynedce', syncedRuleIds, syncedRuleIds - ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId))) + console.log( + "updatesynedce", + syncedRuleIds, + syncedRuleIds?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)), + ); // get synced rules that exist for user const syncedRules: ReturnType[] = syncedRuleIds ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)) diff --git a/test/components/views/settings/Notifications-test.tsx b/test/components/views/settings/Notifications-test.tsx index be87290f6c3..2aa4c93a3b7 100644 --- a/test/components/views/settings/Notifications-test.tsx +++ b/test/components/views/settings/Notifications-test.tsx @@ -23,6 +23,9 @@ import { Room, NotificationCountType, PushRuleActionName, + TweakName, + ConditionKind, + 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"; @@ -47,34 +50,42 @@ const masterRule: IPushRule = { }; const oneToOneRule: IPushRule = { conditions: [ - { kind: "room_member_count", is: "2" }, - { kind: "event_match", key: "type", pattern: "m.room.message" }, + { kind: ConditionKind.RoomMemberCount, is: "2" }, + { kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.message" }, ], - actions: ["notify", { set_tweak: "highlight", value: false }], + actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }], rule_id: ".m.rule.room_one_to_one", default: true, enabled: true, } as IPushRule; const encryptedOneToOneRule: IPushRule = { conditions: [ - { kind: "room_member_count", is: "2" }, - { kind: "event_match", key: "type", pattern: "m.room.encrypted" }, + { kind: ConditionKind.RoomMemberCount, is: "2" }, + { kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.encrypted" }, + ], + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Sound, value: "default" }, + { set_tweak: TweakName.Highlight, value: false }, ], - actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight", value: false }], rule_id: ".m.rule.encrypted_room_one_to_one", default: true, enabled: true, } as IPushRule; const groupRule = { - conditions: [{ kind: "event_match", key: "type", pattern: "m.room.message" }], - actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight", value: false }], + conditions: [{ kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.message" }], + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Sound, value: "default" }, + { set_tweak: TweakName.Highlight, value: false }, + ], rule_id: ".m.rule.message", default: true, enabled: true, }; const encryptedGroupRule: IPushRule = { - conditions: [{ kind: "event_match", key: "type", pattern: "m.room.encrypted" }], - actions: ["dont_notify"], + conditions: [{ kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.encrypted" }], + actions: [PushRuleActionName.DontNotify], rule_id: ".m.rule.encrypted", default: true, enabled: true, @@ -83,8 +94,12 @@ const pushRules: IPushRules = { global: { underride: [ { - conditions: [{ kind: "event_match", key: "type", pattern: "m.call.invite" }], - actions: ["notify", { set_tweak: "sound", value: "ring" }, { set_tweak: "highlight", value: false }], + conditions: [{ kind: ConditionKind.EventMatch, key: "type", pattern: "m.call.invite" }], + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Sound, value: "ring" }, + { set_tweak: TweakName.Highlight, value: false }, + ], rule_id: ".m.rule.call", default: true, enabled: true, @@ -95,28 +110,39 @@ const pushRules: IPushRules = { encryptedGroupRule, { conditions: [ - { kind: "event_match", key: "type", pattern: "im.vector.modular.widgets" }, - { kind: "event_match", key: "content.type", pattern: "jitsi" }, - { kind: "event_match", key: "state_key", pattern: "*" }, + { kind: ConditionKind.EventMatch, key: "type", pattern: "im.vector.modular.widgets" }, + { kind: ConditionKind.EventMatch, key: "content.type", pattern: "jitsi" }, + { kind: ConditionKind.EventMatch, key: "state_key", pattern: "*" }, ], - actions: ["notify", { set_tweak: "highlight", value: false }], + actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }], rule_id: ".im.vector.jitsi", default: true, enabled: true, }, ], sender: [], - room: [{ actions: ["dont_notify"], rule_id: "!zJPyWqpMorfCcWObge:matrix.org", default: false, enabled: true }], + room: [ + { + actions: [PushRuleActionName.DontNotify], + rule_id: "!zJPyWqpMorfCcWObge:matrix.org", + default: false, + enabled: true, + }, + ], content: [ { - actions: ["notify", { set_tweak: "highlight", value: false }], + actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }], pattern: "banana", rule_id: "banana", default: false, enabled: true, }, { - actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight" }], + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Sound, value: "default" }, + { set_tweak: TweakName.Highlight }, + ], pattern: "kadev1", rule_id: ".m.rule.contains_user_name", default: true, @@ -124,62 +150,76 @@ const pushRules: IPushRules = { }, ], override: [ - { conditions: [], actions: ["dont_notify"], rule_id: ".m.rule.master", default: true, enabled: false }, { - conditions: [{ kind: "event_match", key: "content.msgtype", pattern: "m.notice" }], - actions: ["dont_notify"], + conditions: [], + actions: [PushRuleActionName.DontNotify], + rule_id: ".m.rule.master", + default: true, + enabled: false, + }, + { + conditions: [{ kind: ConditionKind.EventMatch, key: "content.msgtype", pattern: "m.notice" }], + actions: [PushRuleActionName.DontNotify], rule_id: ".m.rule.suppress_notices", default: true, enabled: true, }, { conditions: [ - { kind: "event_match", key: "type", pattern: "m.room.member" }, - { kind: "event_match", key: "content.membership", pattern: "invite" }, - { kind: "event_match", key: "state_key", pattern: "@kadev1:matrix.org" }, + { kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.member" }, + { kind: ConditionKind.EventMatch, key: "content.membership", pattern: "invite" }, + { kind: ConditionKind.EventMatch, key: "state_key", pattern: "@kadev1:matrix.org" }, + ], + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Sound, value: "default" }, + { set_tweak: TweakName.Highlight, value: false }, ], - actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight", value: false }], rule_id: ".m.rule.invite_for_me", default: true, enabled: true, }, { - conditions: [{ kind: "event_match", key: "type", pattern: "m.room.member" }], - actions: ["dont_notify"], + conditions: [{ kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.member" }], + actions: [PushRuleActionName.DontNotify], rule_id: ".m.rule.member_event", default: true, enabled: true, }, { conditions: [{ kind: "contains_display_name" }], - actions: ["notify", { set_tweak: "sound", value: "default" }, { set_tweak: "highlight" }], + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Sound, value: "default" }, + { set_tweak: TweakName.Highlight }, + ], rule_id: ".m.rule.contains_display_name", default: true, enabled: true, }, { conditions: [ - { kind: "event_match", key: "content.body", pattern: "@room" }, + { kind: ConditionKind.EventMatch, key: "content.body", pattern: "@room" }, { kind: "sender_notification_permission", key: "room" }, ], - actions: ["notify", { set_tweak: "highlight", value: true }], + actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: true }], rule_id: ".m.rule.roomnotif", default: true, enabled: true, }, { conditions: [ - { kind: "event_match", key: "type", pattern: "m.room.tombstone" }, - { kind: "event_match", key: "state_key", pattern: "" }, + { kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.tombstone" }, + { kind: ConditionKind.EventMatch, key: "state_key", pattern: "" }, ], - actions: ["notify", { set_tweak: "highlight", value: true }], + actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: true }], rule_id: ".m.rule.tombstone", default: true, enabled: true, }, { - conditions: [{ kind: "event_match", key: "type", pattern: "m.reaction" }], - actions: ["dont_notify"], + conditions: [{ kind: ConditionKind.EventMatch, key: "type", pattern: "m.reaction" }], + actions: [PushRuleActionName.DontNotify], rule_id: ".m.rule.reaction", default: true, enabled: true, @@ -437,53 +477,54 @@ describe("", () => { const pollStartOneToOne = { conditions: [ { - kind: "room_member_count", + kind: ConditionKind.RoomMemberCount, is: "2", - }, + } as IPushRuleCondition, { - kind: "event_match", + kind: ConditionKind.EventMatch, key: "type", pattern: "org.matrix.msc3381.poll.start", - }, + } as IPushRuleCondition, ], actions: [PushRuleActionName.DontNotify], rule_id: ".org.matrix.msc3930.rule.poll_start_one_to_one", default: true, enabled: true, - }; + } as IPushRule; const pollStartGroup = { conditions: [ { - kind: "event_match", + kind: ConditionKind.EventMatch, key: "type", pattern: "org.matrix.msc3381.poll.start", }, ], - actions: ["notify"], + actions: [PushRuleActionName.Notify], rule_id: ".org.matrix.msc3930.rule.poll_start", default: true, enabled: true, - }; + } as IPushRule; const pollEndOneToOne = { conditions: [ { - kind: "room_member_count", + kind: ConditionKind.RoomMemberCount, is: "2", }, { - kind: "event_match", + kind: ConditionKind.EventMatch, key: "type", pattern: "org.matrix.msc3381.poll.end", }, ], - actions: [PushRuleActionName.Notify, - { set_tweak: "highlight", value: false }, - { set_tweak: "sound", value: "default" } + actions: [ + PushRuleActionName.Notify, + { set_tweak: TweakName.Highlight, value: false }, + { set_tweak: TweakName.Sound, value: "default" }, ], rule_id: ".org.matrix.msc3930.rule.poll_end_one_to_one", default: true, enabled: true, - }; + } as IPushRule; const setPushRuleMock = (rules: IPushRule[] = []): void => { const combinedRules = { @@ -529,10 +570,16 @@ describe("", () => { // updated synced rule expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( - 'global', 'underride', oneToOneRule.rule_id, [PushRuleActionName.DontNotify] + "global", + "underride", + oneToOneRule.rule_id, + [PushRuleActionName.DontNotify], ); expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( - 'global', 'underride', pollStartOneToOne.rule_id, [PushRuleActionName.DontNotify] + "global", + "underride", + pollStartOneToOne.rule_id, + [PushRuleActionName.DontNotify], ); // only called for parent rule and one existing synced rule expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(2); @@ -547,7 +594,7 @@ describe("", () => { const section = "vector_global"; const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); // have main rule update fail - mockClient.setPushRuleActions.mockRejectedValue('oups') + mockClient.setPushRuleActions.mockRejectedValue("oups"); const offToggle = oneToOneRuleElement.querySelector('input[type="radio"]')!; fireEvent.click(offToggle); @@ -555,7 +602,10 @@ describe("", () => { await flushPromises(); expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( - 'global', 'underride', oneToOneRule.rule_id, [PushRuleActionName.DontNotify] + "global", + "underride", + oneToOneRule.rule_id, + [PushRuleActionName.DontNotify], ); // only called for parent rule expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(1); @@ -568,7 +618,7 @@ describe("", () => { await getComponentAndWait(); const section = "vector_global"; const oneToOneRuleElement = screen.getByTestId(section + oneToOneRule.rule_id); - + // loudest state of synced rules should be the toggle value expect(oneToOneRuleElement.querySelector('input[aria-label="On"]')).toBeChecked(); }); @@ -584,8 +634,6 @@ describe("", () => { // loudest state of synced rules should be the toggle value expect(oneToOneRuleElement.querySelector('input[aria-label="Noisy"]')).toBeChecked(); - console.log('ONONONONONONON') - const onToggle = oneToOneRuleElement.querySelector('input[aria-label="On"]')!; fireEvent.click(onToggle); @@ -593,14 +641,26 @@ describe("", () => { // called for all 3 rules expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(3); - const expectedActions = [ - PushRuleActionName.Notify, { set_tweak: "highlight", value: false } - ] - expect(mockClient.setPushRuleActions).toHaveBeenCalledWith("global", "underride", oneToOneRule.rule_id, expectedActions); - expect(mockClient.setPushRuleActions).toHaveBeenCalledWith("global", "underride", pollStartOneToOne.rule_id, expectedActions); - expect(mockClient.setPushRuleActions).toHaveBeenCalledWith("global", "underride", pollEndOneToOne.rule_id, expectedActions); + const expectedActions = [PushRuleActionName.Notify, { set_tweak: TweakName.Highlight, value: false }]; + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + oneToOneRule.rule_id, + expectedActions, + ); + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollStartOneToOne.rule_id, + expectedActions, + ); + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollEndOneToOne.rule_id, + expectedActions, + ); }); - }); }); From 8c58724714d4aaec56258c9456be1f207b241e77 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 2 Mar 2023 15:02:41 +1300 Subject: [PATCH 07/26] noimplicitany fixes --- src/components/views/settings/Notifications.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 42ab33f8c41..d2ee050689e 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -133,7 +133,9 @@ const findInDefaultRules = ( }, ): IAnnotatedPushRule | undefined => { for (const category in defaultRules) { - const rule = defaultRules[category].find((rule) => rule.rule_id === ruleId); + const rule: IAnnotatedPushRule | undefined = defaultRules[category as RuleClass].find( + (rule) => rule.rule_id === ruleId, + ); if (rule) { return rule; } From 6c8994fcbe7ffcd9c0a3b10c6cd104a34d32df91 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 2 Mar 2023 15:56:49 +1300 Subject: [PATCH 08/26] remove debug --- src/components/views/settings/Notifications.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index d2ee050689e..f686ef65188 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -377,8 +377,6 @@ export default class Notifications extends React.PureComponent { } } - console.log("hhh", { preparedNewState, ruleSets }, MatrixClientPeg.get().pushRules.global.underride); - return preparedNewState; } From 4188cb078ddb88472585eb4380bdb6f494700f8f Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 2 Mar 2023 18:09:17 +1300 Subject: [PATCH 09/26] tidying --- src/components/views/settings/Notifications.tsx | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index f686ef65188..35c7fa3d49b 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -141,7 +141,10 @@ const findInDefaultRules = ( } } }; + +// Vector notification states ordered by loudness in ascending order const OrderedVectorStates = [VectorState.Off, VectorState.On, VectorState.Loud]; + /** * Find the 'loudest' vector state assigned to a rule * and it's synced rules @@ -260,7 +263,6 @@ export default class Notifications extends React.PureComponent { phase: Phase.Ready, }); } catch (e) { - console.log(e); logger.error("Error setting up notifications for settings: ", e); this.setState({ phase: Phase.Error }); } @@ -483,11 +485,6 @@ export default class Notifications extends React.PureComponent { syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], actions?: PushRuleAction[], ): Promise => { - console.log( - "updatesynedce", - syncedRuleIds, - syncedRuleIds?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)), - ); // get synced rules that exist for user const syncedRules: ReturnType[] = syncedRuleIds ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)) From 7d5f80047509cc1f52f26b1745c9ee23ad9a8b91 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 3 Mar 2023 14:27:52 +1300 Subject: [PATCH 10/26] extract updatePushRuleActions fn to utils --- .../views/settings/Notifications.tsx | 20 ++-------- src/utils/pushRules/monitorSyncedPushRules.ts | 5 +++ src/utils/pushRules/updatePushRuleActions.ts | 40 +++++++++++++++++++ 3 files changed, 49 insertions(+), 16 deletions(-) create mode 100644 src/utils/pushRules/monitorSyncedPushRules.ts create mode 100644 src/utils/pushRules/updatePushRuleActions.ts diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 35c7fa3d49b..7a0972fbeb0 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -51,6 +51,7 @@ import TagComposer from "../elements/TagComposer"; import { objectClone } from "../../../utils/objects"; import { arrayDiff } from "../../../utils/arrays"; import { clearAllNotifications, getLocalNotificationAccountDataEventType } from "../../../utils/notifications"; +import { updatePushRuleActions } from "../../../utils/pushRules/updatePushRuleActions"; // TODO: this "view" component still has far too much application logic in it, // which should be factored out to other files. @@ -461,20 +462,6 @@ export default class Notifications extends React.PureComponent { await SettingsStore.setValue("audioNotificationsEnabled", null, SettingLevel.DEVICE, checked); }; - private setPushRuleActions = async ( - ruleId: IPushRule["rule_id"], - kind: PushRuleKind, - actions?: PushRuleAction[], - ): Promise => { - const cli = MatrixClientPeg.get(); - if (!actions) { - await cli.setPushRuleEnabled("global", kind, ruleId, false); - } else { - await cli.setPushRuleActions("global", kind, ruleId, actions); - await cli.setPushRuleEnabled("global", kind, ruleId, true); - } - }; - /** * Updated syncedRuleIds from rule definition * If a rule does not exist it is ignored @@ -493,8 +480,9 @@ export default class Notifications extends React.PureComponent { if (!syncedRules?.length) { return; } + const cli = MatrixClientPeg.get(); for (const { kind, rule: syncedRule } of syncedRules) { - await this.setPushRuleActions(syncedRule.rule_id, kind, actions); + await updatePushRuleActions(cli, syncedRule.rule_id, kind, actions); } }; @@ -538,7 +526,7 @@ export default class Notifications extends React.PureComponent { } else { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; - await this.setPushRuleActions(rule.rule.rule_id, rule.rule.kind, actions); + await updatePushRuleActions(cli, rule.rule.rule_id, rule.rule.kind, actions); await this.updateSyncedRules(definition.syncedRuleIds, actions); } diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts new file mode 100644 index 00000000000..8389e74524a --- /dev/null +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -0,0 +1,5 @@ +import { MatrixEvent } from "matrix-js-sdk/src/matrix"; + +export const monitorSyncedPushRules = async(accountDataEvent: MatrixEvent, matrixClient: MatrixClient): Promise => { + console.log('hhh monitorSyncedPushRules', accountDataEvent) +} \ No newline at end of file diff --git a/src/utils/pushRules/updatePushRuleActions.ts b/src/utils/pushRules/updatePushRuleActions.ts new file mode 100644 index 00000000000..a1597758624 --- /dev/null +++ b/src/utils/pushRules/updatePushRuleActions.ts @@ -0,0 +1,40 @@ +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { MatrixClient } from "matrix-js-sdk/src/client"; +import { IPushRule, PushRuleAction, PushRuleKind } from "matrix-js-sdk/src/matrix"; + +/** + * Sets the actions for a given push rule id and kind + * Where there are no actions, disables the rule + * @param matrixClient - cli + * @param ruleId - rule id to update + * @param kind - PushRuleKind + * @param actions - push rule actions to set for rule + */ +export const updatePushRuleActions = async ( + matrixClient: MatrixClient, + ruleId: IPushRule["rule_id"], + kind: PushRuleKind, + actions?: PushRuleAction[], +): Promise => { + if (!actions) { + await matrixClient.setPushRuleEnabled("global", kind, ruleId, false); + } else { + await matrixClient.setPushRuleActions("global", kind, ruleId, actions); + await matrixClient.setPushRuleEnabled("global", kind, ruleId, true); + } +}; \ No newline at end of file From 5e752d01ab15e39977f54e808bd5089afbe5ec90 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 3 Mar 2023 14:49:11 +1300 Subject: [PATCH 11/26] extract update synced rules --- .../views/settings/Notifications.tsx | 29 ++--------------- src/utils/pushRules/updatePushRuleActions.ts | 32 +++++++++++++++++-- 2 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 7a0972fbeb0..daaddbe95ac 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -19,7 +19,6 @@ import { IAnnotatedPushRule, IPusher, PushRuleAction, - IPushRule, PushRuleKind, RuleId, } from "matrix-js-sdk/src/@types/PushRules"; @@ -51,7 +50,7 @@ import TagComposer from "../elements/TagComposer"; import { objectClone } from "../../../utils/objects"; import { arrayDiff } from "../../../utils/arrays"; import { clearAllNotifications, getLocalNotificationAccountDataEventType } from "../../../utils/notifications"; -import { updatePushRuleActions } from "../../../utils/pushRules/updatePushRuleActions"; +import { updateExistingPushRulesWithActions, updatePushRuleActions } from "../../../utils/pushRules/updatePushRuleActions"; // TODO: this "view" component still has far too much application logic in it, // which should be factored out to other files. @@ -462,30 +461,6 @@ export default class Notifications extends React.PureComponent { await SettingsStore.setValue("audioNotificationsEnabled", null, SettingLevel.DEVICE, checked); }; - /** - * Updated syncedRuleIds from rule definition - * If a rule does not exist it is ignored - * Synced rules are updated sequentially - * and stop at first error - */ - private updateSyncedRules = async ( - syncedRuleIds: VectorPushRuleDefinition["syncedRuleIds"], - actions?: PushRuleAction[], - ): Promise => { - // get synced rules that exist for user - const syncedRules: ReturnType[] = syncedRuleIds - ?.map((ruleId) => this.pushProcessor.getPushRuleAndKindById(ruleId)) - .filter(Boolean); - - if (!syncedRules?.length) { - return; - } - const cli = MatrixClientPeg.get(); - for (const { kind, rule: syncedRule } of syncedRules) { - await updatePushRuleActions(cli, syncedRule.rule_id, kind, actions); - } - }; - private onRadioChecked = async (rule: IVectorPushRule, checkedState: VectorState): Promise => { this.setState({ phase: Phase.Persisting }); @@ -527,7 +502,7 @@ export default class Notifications extends React.PureComponent { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; await updatePushRuleActions(cli, rule.rule.rule_id, rule.rule.kind, actions); - await this.updateSyncedRules(definition.syncedRuleIds, actions); + await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); } await this.refreshFromServer(); diff --git a/src/utils/pushRules/updatePushRuleActions.ts b/src/utils/pushRules/updatePushRuleActions.ts index a1597758624..28e2a3635fa 100644 --- a/src/utils/pushRules/updatePushRuleActions.ts +++ b/src/utils/pushRules/updatePushRuleActions.ts @@ -16,10 +16,11 @@ limitations under the License. import { MatrixClient } from "matrix-js-sdk/src/client"; import { IPushRule, PushRuleAction, PushRuleKind } from "matrix-js-sdk/src/matrix"; +import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; /** * Sets the actions for a given push rule id and kind - * Where there are no actions, disables the rule + * When actions are falsy, disables the rule * @param matrixClient - cli * @param ruleId - rule id to update * @param kind - PushRuleKind @@ -37,4 +38,31 @@ export const updatePushRuleActions = async ( await matrixClient.setPushRuleActions("global", kind, ruleId, actions); await matrixClient.setPushRuleEnabled("global", kind, ruleId, true); } -}; \ No newline at end of file +}; + +/** + * Update push rules with given actions + * Where they already exist for current user + * Rules are updated sequentially and stop at first error + * @param matrixClient - cli + * @param ruleIds - RuleIds of push rules to attempt to set actions for + * @param actions - push rule actions to set for rule + */ +export const updateExistingPushRulesWithActions = async ( + matrixClient: MatrixClient, + ruleIds: IPushRule['rule_id'][], + actions?: PushRuleAction[], +): Promise => { + const pushProcessor = new PushProcessor(matrixClient); + + const rules: ReturnType[] = ruleIds + ?.map((ruleId) => pushProcessor.getPushRuleAndKindById(ruleId)) + .filter(Boolean); + + if (!rules?.length) { + return; + } + for (const { kind, rule } of rules) { + await updatePushRuleActions(matrixClient, rule.rule_id, kind, actions); + } +} \ No newline at end of file From 8d3955751fa51f16e5f3c169cb5a58cda54ff3e5 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 3 Mar 2023 17:44:38 +1300 Subject: [PATCH 12/26] just synchronise in one place? --- src/components/structures/LoggedInView.tsx | 4 + src/components/structures/MatrixChat.tsx | 2 + .../views/settings/Notifications.tsx | 6 +- src/utils/pushRules/monitorSyncedPushRules.ts | 79 ++++++++++++++++++- 4 files changed, 84 insertions(+), 7 deletions(-) diff --git a/src/components/structures/LoggedInView.tsx b/src/components/structures/LoggedInView.tsx index 7b01f1e4050..992a1fb7a5e 100644 --- a/src/components/structures/LoggedInView.tsx +++ b/src/components/structures/LoggedInView.tsx @@ -71,6 +71,7 @@ import { IConfigOptions } from "../../IConfigOptions"; import LeftPanelLiveShareWarning from "../views/beacon/LeftPanelLiveShareWarning"; import { UserOnboardingPage } from "../views/user-onboarding/UserOnboardingPage"; import { PipContainer } from "./PipContainer"; +import { monitorSyncedPushRules } from "../../utils/pushRules/monitorSyncedPushRules"; // We need to fetch each pinned message individually (if we don't already have it) // so each pinned message may trigger a request. Limit the number per room for sanity. @@ -166,6 +167,8 @@ class LoggedInView extends React.Component { this.updateServerNoticeEvents(); this._matrixClient.on(ClientEvent.AccountData, this.onAccountData); + monitorSyncedPushRules(this._matrixClient.getAccountData('m.push_rules'), this._matrixClient); + console.log('hhh componentDidMount LoggedInView'); this._matrixClient.on(ClientEvent.Sync, this.onSync); // Call `onSync` with the current state as well this.onSync(this._matrixClient.getSyncState(), null, this._matrixClient.getSyncStateData()); @@ -280,6 +283,7 @@ class LoggedInView extends React.Component { if (event.getType() === "m.ignored_user_list") { dis.dispatch({ action: "ignore_state_changed" }); } + monitorSyncedPushRules(event, this._matrixClient) }; private onCompactLayoutChanged = (): void => { diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 18c61240c9e..592b1703ede 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -1276,6 +1276,8 @@ export default class MatrixChat extends React.PureComponent { this.themeWatcher.recheck(); StorageManager.tryPersistStorage(); + console.log('hhh onLoggedIN') + if (MatrixClientPeg.currentUserIsJustRegistered() && SettingsStore.getValue("FTUE.useCaseSelection") === null) { this.setStateForNewView({ view: Views.USE_CASE_SELECTION }); diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index daaddbe95ac..d842dd0f8b2 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -25,7 +25,6 @@ import { import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { LocalNotificationSettings } from "matrix-js-sdk/src/@types/local_notifications"; -import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; import Spinner from "../elements/Spinner"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; @@ -187,7 +186,6 @@ const maximumVectorState = ( export default class Notifications extends React.PureComponent { private settingWatchers: string[]; - private pushProcessor: PushProcessor; public constructor(props: IProps) { super(props); @@ -215,8 +213,6 @@ export default class Notifications extends React.PureComponent { this.setState({ audioNotifications: value as boolean }), ), ]; - - this.pushProcessor = new PushProcessor(MatrixClientPeg.get()); } private get isInhibited(): boolean { @@ -502,7 +498,7 @@ export default class Notifications extends React.PureComponent { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; await updatePushRuleActions(cli, rule.rule.rule_id, rule.rule.kind, actions); - await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); + // await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); } await this.refreshFromServer(); diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index 8389e74524a..becc17ed5c0 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -1,5 +1,80 @@ -import { MatrixEvent } from "matrix-js-sdk/src/matrix"; +/* +Copyright 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { MatrixClient } from "matrix-js-sdk/src/client"; +import { MatrixEvent, EventType } from "matrix-js-sdk/src/matrix"; +import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; +import { + RuleId, + IAnnotatedPushRule, +} from "matrix-js-sdk/src/@types/PushRules"; +import { logger } from "matrix-js-sdk/src/logger"; + +import { VectorPushRulesDefinitions, VectorPushRuleDefinition } from "../../notifications"; +import { updateExistingPushRulesWithActions } from "./updatePushRuleActions"; + +const pushRuleAndKindToAnnotated = (ruleAndKind: ReturnType): IAnnotatedPushRule | undefined => ruleAndKind ? { + ...ruleAndKind.rule, + kind: ruleAndKind.kind, +} : undefined; + +const monitorSyncedRule = async (matrixClient: MatrixClient, pushProcessor: PushProcessor, ruleId: RuleId | string, definition: VectorPushRuleDefinition): Promise => { + const primaryRule = pushRuleAndKindToAnnotated(pushProcessor.getPushRuleAndKindById(ruleId)); + + if (!primaryRule) { + return; + } + const syncedRules: IAnnotatedPushRule[] = definition.syncedRuleIds + ?.map((ruleId) => + pushRuleAndKindToAnnotated(pushProcessor.getPushRuleAndKindById(ruleId)) + ) + .filter(Boolean); + + // no synced rules to manage + if (!syncedRules?.length) { + return; + } + + const primaryRuleVectorState = definition.ruleToVectorState(primaryRule); + + const outOfSyncRules = syncedRules.filter(syncedRule => definition.ruleToVectorState(syncedRule) !== primaryRuleVectorState); + + console.log('hhh outOfSync', ruleId, outOfSyncRules, { primaryRuleVectorState, syncedRules}); + + if (outOfSyncRules.length) { + await updateExistingPushRulesWithActions(matrixClient, outOfSyncRules.map(({ rule_id }) => rule_id), primaryRule.actions); + } +} export const monitorSyncedPushRules = async(accountDataEvent: MatrixEvent, matrixClient: MatrixClient): Promise => { - console.log('hhh monitorSyncedPushRules', accountDataEvent) + console.log('hhh monitorSyncedPushRules', accountDataEvent, accountDataEvent?.getType()); + if (accountDataEvent?.getType() !== EventType.PushRules) { + return; + } + + const pushProcessor = new PushProcessor(matrixClient); + + Object.entries(VectorPushRulesDefinitions).map(([ruleId, definition]) => { + try { + monitorSyncedRule(matrixClient, pushProcessor, ruleId, definition); + } catch (error) { + console.error('hhh KA TODO', error); + logger.error(`Failed to fully synchronise push rules for ${ruleId}`, error) + } + }) + + VectorPushRulesDefinitions } \ No newline at end of file From 4602ed45e846365e6f4fa0a9cef8a77598f069cf Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 6 Mar 2023 09:49:37 +1300 Subject: [PATCH 13/26] monitor account data changes AND trigger changes sync in notifications form --- src/components/structures/LoggedInView.tsx | 6 +- src/components/structures/MatrixChat.tsx | 2 - .../views/settings/Notifications.tsx | 15 ++--- src/utils/pushRules/monitorSyncedPushRules.ts | 63 ++++++++++--------- src/utils/pushRules/updatePushRuleActions.ts | 4 +- 5 files changed, 46 insertions(+), 44 deletions(-) diff --git a/src/components/structures/LoggedInView.tsx b/src/components/structures/LoggedInView.tsx index 992a1fb7a5e..356257ec330 100644 --- a/src/components/structures/LoggedInView.tsx +++ b/src/components/structures/LoggedInView.tsx @@ -167,8 +167,8 @@ class LoggedInView extends React.Component { this.updateServerNoticeEvents(); this._matrixClient.on(ClientEvent.AccountData, this.onAccountData); - monitorSyncedPushRules(this._matrixClient.getAccountData('m.push_rules'), this._matrixClient); - console.log('hhh componentDidMount LoggedInView'); + // check push rules on start up as well + monitorSyncedPushRules(this._matrixClient.getAccountData("m.push_rules"), this._matrixClient); this._matrixClient.on(ClientEvent.Sync, this.onSync); // Call `onSync` with the current state as well this.onSync(this._matrixClient.getSyncState(), null, this._matrixClient.getSyncStateData()); @@ -283,7 +283,7 @@ class LoggedInView extends React.Component { if (event.getType() === "m.ignored_user_list") { dis.dispatch({ action: "ignore_state_changed" }); } - monitorSyncedPushRules(event, this._matrixClient) + monitorSyncedPushRules(event, this._matrixClient); }; private onCompactLayoutChanged = (): void => { diff --git a/src/components/structures/MatrixChat.tsx b/src/components/structures/MatrixChat.tsx index 592b1703ede..18c61240c9e 100644 --- a/src/components/structures/MatrixChat.tsx +++ b/src/components/structures/MatrixChat.tsx @@ -1276,8 +1276,6 @@ export default class MatrixChat extends React.PureComponent { this.themeWatcher.recheck(); StorageManager.tryPersistStorage(); - console.log('hhh onLoggedIN') - if (MatrixClientPeg.currentUserIsJustRegistered() && SettingsStore.getValue("FTUE.useCaseSelection") === null) { this.setStateForNewView({ view: Views.USE_CASE_SELECTION }); diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index d842dd0f8b2..639af302e69 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -15,13 +15,7 @@ limitations under the License. */ import React, { ReactNode } from "react"; -import { - IAnnotatedPushRule, - IPusher, - PushRuleAction, - PushRuleKind, - RuleId, -} from "matrix-js-sdk/src/@types/PushRules"; +import { IAnnotatedPushRule, IPusher, PushRuleAction, PushRuleKind, RuleId } from "matrix-js-sdk/src/@types/PushRules"; import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { LocalNotificationSettings } from "matrix-js-sdk/src/@types/local_notifications"; @@ -49,7 +43,10 @@ import TagComposer from "../elements/TagComposer"; import { objectClone } from "../../../utils/objects"; import { arrayDiff } from "../../../utils/arrays"; import { clearAllNotifications, getLocalNotificationAccountDataEventType } from "../../../utils/notifications"; -import { updateExistingPushRulesWithActions, updatePushRuleActions } from "../../../utils/pushRules/updatePushRuleActions"; +import { + updateExistingPushRulesWithActions, + updatePushRuleActions, +} from "../../../utils/pushRules/updatePushRuleActions"; // TODO: this "view" component still has far too much application logic in it, // which should be factored out to other files. @@ -498,7 +495,7 @@ export default class Notifications extends React.PureComponent { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; await updatePushRuleActions(cli, rule.rule.rule_id, rule.rule.kind, actions); - // await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); + await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); } await this.refreshFromServer(); diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index becc17ed5c0..231e670bf13 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -17,32 +17,37 @@ limitations under the License. import { MatrixClient } from "matrix-js-sdk/src/client"; import { MatrixEvent, EventType } from "matrix-js-sdk/src/matrix"; import { PushProcessor } from "matrix-js-sdk/src/pushprocessor"; -import { - RuleId, - IAnnotatedPushRule, -} from "matrix-js-sdk/src/@types/PushRules"; +import { RuleId, IAnnotatedPushRule } from "matrix-js-sdk/src/@types/PushRules"; import { logger } from "matrix-js-sdk/src/logger"; import { VectorPushRulesDefinitions, VectorPushRuleDefinition } from "../../notifications"; import { updateExistingPushRulesWithActions } from "./updatePushRuleActions"; -const pushRuleAndKindToAnnotated = (ruleAndKind: ReturnType): IAnnotatedPushRule | undefined => ruleAndKind ? { - ...ruleAndKind.rule, - kind: ruleAndKind.kind, -} : undefined; - -const monitorSyncedRule = async (matrixClient: MatrixClient, pushProcessor: PushProcessor, ruleId: RuleId | string, definition: VectorPushRuleDefinition): Promise => { +const pushRuleAndKindToAnnotated = ( + ruleAndKind: ReturnType, +): IAnnotatedPushRule | undefined => + ruleAndKind + ? { + ...ruleAndKind.rule, + kind: ruleAndKind.kind, + } + : undefined; + +const monitorSyncedRule = async ( + matrixClient: MatrixClient, + pushProcessor: PushProcessor, + ruleId: RuleId | string, + definition: VectorPushRuleDefinition, +): Promise => { const primaryRule = pushRuleAndKindToAnnotated(pushProcessor.getPushRuleAndKindById(ruleId)); if (!primaryRule) { return; } const syncedRules: IAnnotatedPushRule[] = definition.syncedRuleIds - ?.map((ruleId) => - pushRuleAndKindToAnnotated(pushProcessor.getPushRuleAndKindById(ruleId)) - ) + ?.map((ruleId) => pushRuleAndKindToAnnotated(pushProcessor.getPushRuleAndKindById(ruleId))) .filter(Boolean); - + // no synced rules to manage if (!syncedRules?.length) { return; @@ -50,31 +55,33 @@ const monitorSyncedRule = async (matrixClient: MatrixClient, pushProcessor: Push const primaryRuleVectorState = definition.ruleToVectorState(primaryRule); - const outOfSyncRules = syncedRules.filter(syncedRule => definition.ruleToVectorState(syncedRule) !== primaryRuleVectorState); - - console.log('hhh outOfSync', ruleId, outOfSyncRules, { primaryRuleVectorState, syncedRules}); + const outOfSyncRules = syncedRules.filter( + (syncedRule) => definition.ruleToVectorState(syncedRule) !== primaryRuleVectorState, + ); if (outOfSyncRules.length) { - await updateExistingPushRulesWithActions(matrixClient, outOfSyncRules.map(({ rule_id }) => rule_id), primaryRule.actions); + await updateExistingPushRulesWithActions( + matrixClient, + outOfSyncRules.map(({ rule_id }) => rule_id), + primaryRule.actions, + ); } -} +}; -export const monitorSyncedPushRules = async(accountDataEvent: MatrixEvent, matrixClient: MatrixClient): Promise => { - console.log('hhh monitorSyncedPushRules', accountDataEvent, accountDataEvent?.getType()); +export const monitorSyncedPushRules = async ( + accountDataEvent: MatrixEvent, + matrixClient: MatrixClient, +): Promise => { if (accountDataEvent?.getType() !== EventType.PushRules) { return; } - const pushProcessor = new PushProcessor(matrixClient); Object.entries(VectorPushRulesDefinitions).map(([ruleId, definition]) => { try { monitorSyncedRule(matrixClient, pushProcessor, ruleId, definition); } catch (error) { - console.error('hhh KA TODO', error); - logger.error(`Failed to fully synchronise push rules for ${ruleId}`, error) + logger.error(`Failed to fully synchronise push rules for ${ruleId}`, error); } - }) - - VectorPushRulesDefinitions -} \ No newline at end of file + }); +}; diff --git a/src/utils/pushRules/updatePushRuleActions.ts b/src/utils/pushRules/updatePushRuleActions.ts index 28e2a3635fa..e955d80344b 100644 --- a/src/utils/pushRules/updatePushRuleActions.ts +++ b/src/utils/pushRules/updatePushRuleActions.ts @@ -50,7 +50,7 @@ export const updatePushRuleActions = async ( */ export const updateExistingPushRulesWithActions = async ( matrixClient: MatrixClient, - ruleIds: IPushRule['rule_id'][], + ruleIds: IPushRule["rule_id"][], actions?: PushRuleAction[], ): Promise => { const pushProcessor = new PushProcessor(matrixClient); @@ -65,4 +65,4 @@ export const updateExistingPushRulesWithActions = async ( for (const { kind, rule } of rules) { await updatePushRuleActions(matrixClient, rule.rule_id, kind, actions); } -} \ No newline at end of file +}; From 1a8b003728faba42eb6d2e5a44aab8314e76b5c2 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 6 Mar 2023 12:10:04 +1300 Subject: [PATCH 14/26] lint --- src/components/views/settings/Notifications.tsx | 8 +------- src/utils/pushRules/monitorSyncedPushRules.ts | 1 + 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 6ffe0ca7a61..639af302e69 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -15,13 +15,7 @@ limitations under the License. */ import React, { ReactNode } from "react"; -import { - IAnnotatedPushRule, - IPusher, - PushRuleAction, - PushRuleKind, - RuleId, -} from "matrix-js-sdk/src/@types/PushRules"; +import { IAnnotatedPushRule, IPusher, PushRuleAction, PushRuleKind, RuleId } from "matrix-js-sdk/src/@types/PushRules"; import { IThreepid, ThreepidMedium } from "matrix-js-sdk/src/@types/threepids"; import { logger } from "matrix-js-sdk/src/logger"; import { LocalNotificationSettings } from "matrix-js-sdk/src/@types/local_notifications"; diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index 231e670bf13..c6fbb84c69a 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -62,6 +62,7 @@ const monitorSyncedRule = async ( if (outOfSyncRules.length) { await updateExistingPushRulesWithActions( matrixClient, + // eslint-disable-next-line camelcase, @typescript-eslint/naming-convention outOfSyncRules.map(({ rule_id }) => rule_id), primaryRule.actions, ); From 0e2eb9e3d4771c61bb6a4c54d682329294f3e969 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 6 Mar 2023 12:30:01 +1300 Subject: [PATCH 15/26] setup LoggedInView test with enough mocks --- .../structures/LoggedInView-test.tsx | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 test/components/structures/LoggedInView-test.tsx diff --git a/test/components/structures/LoggedInView-test.tsx b/test/components/structures/LoggedInView-test.tsx new file mode 100644 index 00000000000..667e2c45e6d --- /dev/null +++ b/test/components/structures/LoggedInView-test.tsx @@ -0,0 +1,68 @@ +/* +Copyright 2015 - 2023 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { render, RenderResult } from "@testing-library/react"; +import { MediaHandler } from "matrix-js-sdk/src/webrtc/mediaHandler"; +import React, { ReactElement } from "react"; +import LoggedInView from "../../../src/components/structures/LoggedInView"; +import { SDKContext } from "../../../src/contexts/SDKContext"; +import ResizeNotifier from "../../../src/utils/ResizeNotifier"; +import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../../test-utils"; +import { TestSdkContext } from "../../TestSdkContext"; + +describe('', () => { + const userId = '@alice:domain.org'; + const mockClient = getMockClientWithEventEmitter({ + ...mockClientMethodsUser(userId), + getAccountData: jest.fn(), + getRoom: jest.fn(), + getSyncState: jest.fn().mockReturnValue(null), + getSyncStateData: jest.fn().mockReturnValue(null), + getMediaHandler: jest.fn() + }); + const mediaHandler = new MediaHandler(mockClient); + const mockSdkContext = new TestSdkContext(); + + const defaultProps = { + matrixClient: mockClient, + onRegistered: jest.fn(), + resizeNotifier: new ResizeNotifier(), + collapseLhs: false, + hideToSRUsers: false, + config: { + brand: 'Test', + element_call: {}, + }, + currentRoomId: '', + } + + const getComponent = (props = {}): RenderResult => render( + , + {wrapper: ({ children }) => ( + {children} + ),} + ); + + beforeEach(() => { + mockClient.getMediaHandler.mockReturnValue(mediaHandler); + }) + + it('renders', () => { + const { container } = getComponent(); + + expect(container).toMatchSnapshot(); + }); +}); \ No newline at end of file From 55bb32330a87f1f479abf43b6ba63af99af04486 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 6 Mar 2023 15:36:05 +1300 Subject: [PATCH 16/26] test rule syncing in LoggedInView --- src/utils/pushRules/monitorSyncedPushRules.ts | 4 +- .../structures/LoggedInView-test.tsx | 296 ++++++++++++++++-- 2 files changed, 279 insertions(+), 21 deletions(-) diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index c6fbb84c69a..b19e82aa63d 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -78,9 +78,9 @@ export const monitorSyncedPushRules = async ( } const pushProcessor = new PushProcessor(matrixClient); - Object.entries(VectorPushRulesDefinitions).map(([ruleId, definition]) => { + Object.entries(VectorPushRulesDefinitions).forEach(async ([ruleId, definition]) => { try { - monitorSyncedRule(matrixClient, pushProcessor, ruleId, definition); + await monitorSyncedRule(matrixClient, pushProcessor, ruleId, definition); } catch (error) { logger.error(`Failed to fully synchronise push rules for ${ruleId}`, error); } diff --git a/test/components/structures/LoggedInView-test.tsx b/test/components/structures/LoggedInView-test.tsx index 667e2c45e6d..331766a2ad0 100644 --- a/test/components/structures/LoggedInView-test.tsx +++ b/test/components/structures/LoggedInView-test.tsx @@ -14,24 +14,31 @@ See the License for the specific language governing permissions and limitations under the License. */ +import React from "react"; import { render, RenderResult } from "@testing-library/react"; +import { ConditionKind, EventType, IPushRule, MatrixEvent } from "matrix-js-sdk/src/matrix"; +import { ClientEvent } from "matrix-js-sdk/src/client"; import { MediaHandler } from "matrix-js-sdk/src/webrtc/mediaHandler"; -import React, { ReactElement } from "react"; +import { logger } from "matrix-js-sdk/src/logger"; + import LoggedInView from "../../../src/components/structures/LoggedInView"; import { SDKContext } from "../../../src/contexts/SDKContext"; +import { StandardActions } from "../../../src/notifications/StandardActions"; import ResizeNotifier from "../../../src/utils/ResizeNotifier"; -import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../../test-utils"; +import { flushPromises, getMockClientWithEventEmitter, mockClientMethodsUser } from "../../test-utils"; import { TestSdkContext } from "../../TestSdkContext"; -describe('', () => { - const userId = '@alice:domain.org'; +describe("", () => { + const userId = "@alice:domain.org"; const mockClient = getMockClientWithEventEmitter({ ...mockClientMethodsUser(userId), getAccountData: jest.fn(), getRoom: jest.fn(), getSyncState: jest.fn().mockReturnValue(null), getSyncStateData: jest.fn().mockReturnValue(null), - getMediaHandler: jest.fn() + getMediaHandler: jest.fn(), + setPushRuleEnabled: jest.fn(), + setPushRuleActions: jest.fn(), }); const mediaHandler = new MediaHandler(mockClient); const mockSdkContext = new TestSdkContext(); @@ -43,26 +50,277 @@ describe('', () => { collapseLhs: false, hideToSRUsers: false, config: { - brand: 'Test', + brand: "Test", element_call: {}, }, - currentRoomId: '', - } + currentRoomId: "", + }; - const getComponent = (props = {}): RenderResult => render( - , - {wrapper: ({ children }) => ( - {children} - ),} - ); + const getComponent = (props = {}): RenderResult => + render(, { + wrapper: ({ children }) => {children}, + }); beforeEach(() => { + jest.clearAllMocks(); mockClient.getMediaHandler.mockReturnValue(mediaHandler); - }) + mockClient.setPushRuleActions.mockReset().mockResolvedValue({}); + }); + + describe("synced push rules", () => { + const pushRulesEvent = new MatrixEvent({ type: EventType.PushRules }); + + const oneToOneRule = { + conditions: [ + { kind: ConditionKind.RoomMemberCount, is: "2" }, + { kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.message" }, + ], + actions: StandardActions.ACTION_NOTIFY, + rule_id: ".m.rule.room_one_to_one", + default: true, + enabled: true, + } as IPushRule; + + const groupRule = { + conditions: [{ kind: ConditionKind.EventMatch, key: "type", pattern: "m.room.message" }], + actions: StandardActions.ACTION_NOTIFY, + rule_id: ".m.rule.message", + default: true, + enabled: true, + } as IPushRule; + + const pollStartOneToOne = { + conditions: [ + { + kind: ConditionKind.RoomMemberCount, + is: "2", + }, + { + kind: ConditionKind.EventMatch, + key: "type", + pattern: "org.matrix.msc3381.poll.start", + }, + ], + actions: StandardActions.ACTION_NOTIFY, + rule_id: ".org.matrix.msc3930.rule.poll_start_one_to_one", + default: true, + enabled: true, + } as IPushRule; + + const pollEndOneToOne = { + conditions: [ + { + kind: ConditionKind.RoomMemberCount, + is: "2", + }, + { + kind: ConditionKind.EventMatch, + key: "type", + pattern: "org.matrix.msc3381.poll.end", + }, + ], + actions: StandardActions.ACTION_HIGHLIGHT_DEFAULT_SOUND, + rule_id: ".org.matrix.msc3930.rule.poll_end_one_to_one", + default: true, + enabled: true, + } as IPushRule; + + const pollStartGroup = { + conditions: [ + { + kind: ConditionKind.EventMatch, + key: "type", + pattern: "org.matrix.msc3381.poll.start", + }, + ], + actions: StandardActions.ACTION_HIGHLIGHT_DEFAULT_SOUND, + rule_id: ".org.matrix.msc3930.rule.poll_start", + default: true, + enabled: true, + } as IPushRule; + + beforeEach(() => { + mockClient.getAccountData.mockImplementation((eventType: string) => + eventType === EventType.PushRules ? pushRulesEvent : undefined, + ); + setPushRules([]); + // stub out error logger to avoid littering console + jest.spyOn(logger, "error") + .mockClear() + .mockImplementation(() => {}); + + mockClient.setPushRuleActions.mockClear(); + mockClient.setPushRuleEnabled.mockClear(); + }); + + const setPushRules = (rules: IPushRule[] = []): void => { + const pushRules = { + global: { + underride: [...rules], + }, + }; + + mockClient.pushRules = pushRules; + }; + + describe("on mount", () => { + it("handles when user has no push rules event in account data", () => { + mockClient.getAccountData.mockReturnValue(undefined); + getComponent(); + + expect(mockClient.getAccountData).toHaveBeenCalledWith(EventType.PushRules); + expect(logger.error).not.toHaveBeenCalled(); + }); + + it("handles when user doesnt have a push rule defined in vector definitions", () => { + // synced push rules uses VectorPushRulesDefinitions + // rules defined there may not exist in m.push_rules + // mock push rules with group rule, but missing oneToOne rule + setPushRules([pollStartOneToOne, groupRule, pollStartGroup]); + + getComponent(); + + // just called once for one-to-one + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(1); + // set to match primary rule (groupRule) + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollStartGroup.rule_id, + StandardActions.ACTION_NOTIFY, + ); + }); + + it("updates all mismatched rules from synced rules", () => { + setPushRules([ + // poll 1-1 rules are synced with oneToOneRule + oneToOneRule, // on + pollStartOneToOne, // on + pollEndOneToOne, // loud + // poll group rules are synced with groupRule + groupRule, // on + pollStartGroup, // loud + ]); + + getComponent(); + + // only called for rules not in sync with their primary rule + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(2); + // set to match primary rule + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollStartGroup.rule_id, + StandardActions.ACTION_NOTIFY, + ); + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollEndOneToOne.rule_id, + StandardActions.ACTION_NOTIFY, + ); + }); + + it("catches and logs errors while updating a rule", async () => { + mockClient.setPushRuleActions.mockRejectedValueOnce("oups").mockResolvedValueOnce({}); + + setPushRules([ + // poll 1-1 rules are synced with oneToOneRule + oneToOneRule, // on + pollStartOneToOne, // on + pollEndOneToOne, // loud + // poll group rules are synced with groupRule + groupRule, // on + pollStartGroup, // loud + ]); + + getComponent(); + await flushPromises(); + + expect(mockClient.setPushRuleActions).toHaveBeenCalledTimes(2); + // both calls made + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollStartGroup.rule_id, + StandardActions.ACTION_NOTIFY, + ); + // second primary rule still updated after first rule failed + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollEndOneToOne.rule_id, + StandardActions.ACTION_NOTIFY, + ); + expect(logger.error).toHaveBeenCalledWith( + "Failed to fully synchronise push rules for .m.rule.room_one_to_one", + "oups", + ); + }); + }); + + describe("on changes to account_data", () => { + it("ignores other account data events", () => { + // setup a push rule state with mismatched rules + setPushRules([ + // poll 1-1 rules are synced with oneToOneRule + oneToOneRule, // on + pollEndOneToOne, // loud + ]); + + getComponent(); + + mockClient.setPushRuleActions.mockClear(); + + const someOtherAccountData = new MatrixEvent({ type: "my-test-account-data " }); + mockClient.emit(ClientEvent.AccountData, someOtherAccountData); + + // didnt check rule sync + expect(mockClient.setPushRuleActions).not.toHaveBeenCalled(); + }); + + it("updates all mismatched rules from synced rules on a change to push rules account data", () => { + // setup a push rule state with mismatched rules + setPushRules([ + // poll 1-1 rules are synced with oneToOneRule + oneToOneRule, // on + pollEndOneToOne, // loud + ]); + + getComponent(); + + mockClient.setPushRuleActions.mockClear(); + + mockClient.emit(ClientEvent.AccountData, pushRulesEvent); + + // set to match primary rule + expect(mockClient.setPushRuleActions).toHaveBeenCalledWith( + "global", + "underride", + pollEndOneToOne.rule_id, + StandardActions.ACTION_NOTIFY, + ); + }); + + it("stops listening to account data events on unmount", () => { + // setup a push rule state with mismatched rules + setPushRules([ + // poll 1-1 rules are synced with oneToOneRule + oneToOneRule, // on + pollEndOneToOne, // loud + ]); + + const { unmount } = getComponent(); + + mockClient.setPushRuleActions.mockClear(); + + unmount(); - it('renders', () => { - const { container } = getComponent(); + mockClient.emit(ClientEvent.AccountData, pushRulesEvent); - expect(container).toMatchSnapshot(); + // set to match primary rule + expect(mockClient.setPushRuleActions).not.toHaveBeenCalled(); + }); + }); }); -}); \ No newline at end of file +}); From 2443dd467eee95016a10f74534b9265ec159a7f0 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 6 Mar 2023 15:59:34 +1300 Subject: [PATCH 17/26] strict fixes --- src/components/views/settings/Notifications.tsx | 4 ++++ src/utils/pushRules/monitorSyncedPushRules.ts | 6 +++--- src/utils/pushRules/updatePushRuleActions.ts | 11 ++++++++--- test/components/structures/LoggedInView-test.tsx | 2 +- 4 files changed, 16 insertions(+), 7 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 639af302e69..2e9ea6084b2 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -494,6 +494,10 @@ export default class Notifications extends React.PureComponent { } else { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; + // we should not encounter this + if (!rule.rule) { + throw new Error("Unable to update rule without rule_id"); + } await updatePushRuleActions(cli, rule.rule.rule_id, rule.rule.kind, actions); await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); } diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index b19e82aa63d..bcce50d6ada 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -44,9 +44,9 @@ const monitorSyncedRule = async ( if (!primaryRule) { return; } - const syncedRules: IAnnotatedPushRule[] = definition.syncedRuleIds + const syncedRules: IAnnotatedPushRule[] | undefined = definition.syncedRuleIds ?.map((ruleId) => pushRuleAndKindToAnnotated(pushProcessor.getPushRuleAndKindById(ruleId))) - .filter(Boolean); + .filter((n?: IAnnotatedPushRule): n is IAnnotatedPushRule => Boolean(n)); // no synced rules to manage if (!syncedRules?.length) { @@ -70,7 +70,7 @@ const monitorSyncedRule = async ( }; export const monitorSyncedPushRules = async ( - accountDataEvent: MatrixEvent, + accountDataEvent: MatrixEvent | undefined, matrixClient: MatrixClient, ): Promise => { if (accountDataEvent?.getType() !== EventType.PushRules) { diff --git a/src/utils/pushRules/updatePushRuleActions.ts b/src/utils/pushRules/updatePushRuleActions.ts index e955d80344b..b2a11db3fd1 100644 --- a/src/utils/pushRules/updatePushRuleActions.ts +++ b/src/utils/pushRules/updatePushRuleActions.ts @@ -40,6 +40,11 @@ export const updatePushRuleActions = async ( } }; +interface PushRuleAndKind { + rule: IPushRule; + kind: PushRuleKind; +} + /** * Update push rules with given actions * Where they already exist for current user @@ -50,14 +55,14 @@ export const updatePushRuleActions = async ( */ export const updateExistingPushRulesWithActions = async ( matrixClient: MatrixClient, - ruleIds: IPushRule["rule_id"][], + ruleIds?: IPushRule["rule_id"][], actions?: PushRuleAction[], ): Promise => { const pushProcessor = new PushProcessor(matrixClient); - const rules: ReturnType[] = ruleIds + const rules: PushRuleAndKind[] | undefined = ruleIds ?.map((ruleId) => pushProcessor.getPushRuleAndKindById(ruleId)) - .filter(Boolean); + .filter((n: PushRuleAndKind | null): n is PushRuleAndKind => Boolean(n)); if (!rules?.length) { return; diff --git a/test/components/structures/LoggedInView-test.tsx b/test/components/structures/LoggedInView-test.tsx index 331766a2ad0..5e389d65dea 100644 --- a/test/components/structures/LoggedInView-test.tsx +++ b/test/components/structures/LoggedInView-test.tsx @@ -318,7 +318,7 @@ describe("", () => { mockClient.emit(ClientEvent.AccountData, pushRulesEvent); - // set to match primary rule + // not called expect(mockClient.setPushRuleActions).not.toHaveBeenCalled(); }); }); From c9949727b5506286437f10d06878f2beb99590b0 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 10:26:37 +1300 Subject: [PATCH 18/26] more comments --- src/components/views/settings/Notifications.tsx | 3 ++- src/utils/pushRules/monitorSyncedPushRules.ts | 11 +++++++++++ src/utils/pushRules/updatePushRuleActions.ts | 2 ++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 2e9ea6084b2..3aa4edf8a43 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -495,8 +495,9 @@ export default class Notifications extends React.PureComponent { const definition: VectorPushRuleDefinition = VectorPushRulesDefinitions[rule.ruleId]; const actions = definition.vectorStateToActions[checkedState]; // we should not encounter this + // satisfies types if (!rule.rule) { - throw new Error("Unable to update rule without rule_id"); + throw new Error("Cannot update rule: push rule data is incomplete."); } await updatePushRuleActions(cli, rule.rule.rule_id, rule.rule.kind, actions); await updateExistingPushRulesWithActions(cli, definition.syncedRuleIds, actions); diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index bcce50d6ada..a90f662e215 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -69,6 +69,17 @@ const monitorSyncedRule = async ( } }; +/** + * On changes to m.push_rules account data, + * check that synced push rules are in sync with their primary rule, + * and update any out of sync rules. + * synced rules are defined in VectorPushRulesDefinitions + * If updating a rule fails for any reason, + * the error is caught and handled silently + * @param accountDataEvent - MatrixEvent + * @param matrixClient - cli + * @returns Resolves when updates are complete + */ export const monitorSyncedPushRules = async ( accountDataEvent: MatrixEvent | undefined, matrixClient: MatrixClient, diff --git a/src/utils/pushRules/updatePushRuleActions.ts b/src/utils/pushRules/updatePushRuleActions.ts index b2a11db3fd1..e4ea970c39e 100644 --- a/src/utils/pushRules/updatePushRuleActions.ts +++ b/src/utils/pushRules/updatePushRuleActions.ts @@ -52,6 +52,8 @@ interface PushRuleAndKind { * @param matrixClient - cli * @param ruleIds - RuleIds of push rules to attempt to set actions for * @param actions - push rule actions to set for rule + * @returns resolves when all rules have been updated + * @returns rejects when a rule update fails */ export const updateExistingPushRulesWithActions = async ( matrixClient: MatrixClient, From 5afce3ff0bfe094e765812d364fdda5cdfd11d1e Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 10:30:52 +1300 Subject: [PATCH 19/26] one more comment --- src/utils/pushRules/monitorSyncedPushRules.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/utils/pushRules/monitorSyncedPushRules.ts b/src/utils/pushRules/monitorSyncedPushRules.ts index a90f662e215..9061d74a8f4 100644 --- a/src/utils/pushRules/monitorSyncedPushRules.ts +++ b/src/utils/pushRules/monitorSyncedPushRules.ts @@ -33,6 +33,15 @@ const pushRuleAndKindToAnnotated = ( } : undefined; +/** + * Checks that any synced rules that exist a given rule are in sync + * And updates any that are out of sync + * Ignores ruleIds that do not exist for the user + * @param matrixClient - cli + * @param pushProcessor - processor used to retrieve current state of rules + * @param ruleId - primary rule + * @param definition - VectorPushRuleDefinition of the primary rule + */ const monitorSyncedRule = async ( matrixClient: MatrixClient, pushProcessor: PushProcessor, From 7d553dc4f326ba1b03ebcc62231338a87300e6cf Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Mon, 6 Mar 2023 17:34:00 +1300 Subject: [PATCH 20/26] add error variant for caption component --- res/css/components/views/typography/_Caption.pcss | 4 ++++ src/components/views/typography/Caption.tsx | 11 +++++++++-- test/components/views/typography/Caption-test.tsx | 5 +++++ .../typography/__snapshots__/Caption-test.tsx.snap | 13 +++++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/res/css/components/views/typography/_Caption.pcss b/res/css/components/views/typography/_Caption.pcss index 3af270c4a5e..f51276d9f96 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/src/components/views/typography/Caption.tsx b/src/components/views/typography/Caption.tsx index 4ec7c20152d..69e7714b223 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/test/components/views/typography/Caption-test.tsx b/test/components/views/typography/Caption-test.tsx index ca3258f725f..bd684ca0f80 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 bb3c380a80c..b17d5044b08 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":
From 30a3595d29b6d1f0deded2345ab07b34bc664fa1 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 13:05:27 +1300 Subject: [PATCH 21/26] tests for new error message --- res/css/views/settings/_Notifications.pcss | 6 ++ .../views/settings/Notifications.tsx | 47 +++++++--- src/i18n/strings/en_EN.json | 1 + .../views/settings/Notifications-test.tsx | 89 +++++++++++++++++-- 4 files changed, 127 insertions(+), 16 deletions(-) diff --git a/res/css/views/settings/_Notifications.pcss b/res/css/views/settings/_Notifications.pcss index 7357ee1c68b..ef6257c704e 100644 --- a/res/css/views/settings/_Notifications.pcss +++ b/res/css/views/settings/_Notifications.pcss @@ -61,6 +61,12 @@ 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%; +} .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 3aa4edf8a43..3f901daeee7 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,17 +396,28 @@ 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) { - this.setState({ phase: Phase.Error }); + if (masterRule?.rule_id) { + this.setSavingError(masterRule.rule_id); + } else { + this.setState({ phase: Phase.Error }); + } logger.error("Error updating master push rule:", e); this.showSaveError(); } }; + 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,7 +469,10 @@ 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(); @@ -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(); } }; @@ -768,6 +784,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/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index d5aa83660cc..0dc0c4dcdcf 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1441,6 +1441,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 2aa4c93a3b7..f352106db32 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"; @@ -272,7 +272,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; }); @@ -473,6 +473,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 +621,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 +656,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 +685,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 () => { From 2eec3bbf3ef3878d0f10b28f92b162b93e9245bc Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 13:16:33 +1300 Subject: [PATCH 22/26] tweak styles --- res/css/views/settings/_Notifications.pcss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/res/css/views/settings/_Notifications.pcss b/res/css/views/settings/_Notifications.pcss index ef6257c704e..45a5b4529d8 100644 --- a/res/css/views/settings/_Notifications.pcss +++ b/res/css/views/settings/_Notifications.pcss @@ -66,6 +66,8 @@ limitations under the License. grid-column: 1/-1; justify-self: start; padding-right: 30%; + // collapse half of the grid-gap + margin-top: -$spacing-4; } .mx_UserNotifSettings { From cc750fc80f69eb3c23298c7b6869a69b470b74b1 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 15:09:49 +1300 Subject: [PATCH 23/26] noImplicitAny --- src/components/views/settings/Notifications.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 3f901daeee7..18b504882ad 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -477,6 +477,10 @@ export default class Notifications extends React.PureComponent { 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; From caa05c5031d9952916e2bb993baa0ea63fc3f694 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 17:57:30 +1300 Subject: [PATCH 24/26] revert out of date prettier changes to unrelated files --- src/components/views/dialogs/ChangelogDialog.tsx | 4 ++-- src/theme.ts | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/views/dialogs/ChangelogDialog.tsx b/src/components/views/dialogs/ChangelogDialog.tsx index 85af8203b74..8321cc40f49 100644 --- a/src/components/views/dialogs/ChangelogDialog.tsx +++ b/src/components/views/dialogs/ChangelogDialog.tsx @@ -27,7 +27,7 @@ interface IProps { onFinished: (success: boolean) => void; } -type State = Partial>; +type State = Partial>; interface Commit { sha: string; @@ -46,7 +46,7 @@ export default class ChangelogDialog extends React.Component { this.state = {}; } - private async fetchChanges(repo: typeof REPOS[number], oldVersion: string, newVersion: string): Promise { + private async fetchChanges(repo: (typeof REPOS)[number], oldVersion: string, newVersion: string): Promise { const url = `https://riot.im/github/repos/${repo}/compare/${oldVersion}...${newVersion}`; try { diff --git a/src/theme.ts b/src/theme.ts index 8089102eb52..e85460e9baa 100644 --- a/src/theme.ts +++ b/src/theme.ts @@ -26,7 +26,7 @@ const HIGH_CONTRAST_THEMES: Record = { light: "light-high-contrast", }; -interface IFontFaces extends Omit, "src"> { +interface IFontFaces extends Omit, "src"> { src: { format: string; url: string; @@ -145,9 +145,9 @@ function generateCustomFontFaceCSS(faces: IFontFaces[]): string { return ""; }) .join(", "); - const props = Object.keys(face).filter((prop: typeof allowedFontFaceProps[number]) => + const props = Object.keys(face).filter((prop: (typeof allowedFontFaceProps)[number]) => allowedFontFaceProps.includes(prop), - ) as Array; + ) as Array<(typeof allowedFontFaceProps)[number]>; const body = props .map((prop) => { let value: string; From c634f4e539c26ea38de90f86a04a3350984e1617 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Thu, 9 Mar 2023 18:42:28 +1300 Subject: [PATCH 25/26] limit inline message to radios only, tests --- .../views/settings/Notifications.tsx | 24 +++---- .../views/settings/Notifications-test.tsx | 70 +++++++++++++++++-- 2 files changed, 74 insertions(+), 20 deletions(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index 18b504882ad..db92f36b504 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -401,11 +401,7 @@ export default class Notifications extends React.PureComponent { await MatrixClientPeg.get().setPushRuleEnabled("global", masterRule.kind, masterRule.rule_id, !checked); await this.refreshFromServer(); } catch (e) { - if (masterRule?.rule_id) { - this.setSavingError(masterRule.rule_id); - } else { - this.setState({ phase: Phase.Error }); - } + this.setState({ phase: Phase.Error }); logger.error("Error updating master push rule:", e); this.showSaveError(); } @@ -638,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. diff --git a/test/components/views/settings/Notifications-test.tsx b/test/components/views/settings/Notifications-test.tsx index f352106db32..5084dfb258a 100644 --- a/test/components/views/settings/Notifications-test.tsx +++ b/test/components/views/settings/Notifications-test.tsx @@ -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, @@ -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; @@ -743,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"); From 287aa2a754bc2451b19c045a5bc35acadec0c2c7 Mon Sep 17 00:00:00 2001 From: Kerry Archibald Date: Fri, 10 Mar 2023 10:04:28 +1300 Subject: [PATCH 26/26] strict fix --- src/components/views/settings/Notifications.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/views/settings/Notifications.tsx b/src/components/views/settings/Notifications.tsx index db92f36b504..6aa7dca132d 100644 --- a/src/components/views/settings/Notifications.tsx +++ b/src/components/views/settings/Notifications.tsx @@ -657,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}