Fix merging of default push rules

spec/unit/pushprocessor.spec.ts

@@ -1,6 +1,16 @@
 import * as utils from "../test-utils/test-utils";
 import { IActionsObject, PushProcessor } from "../../src/pushprocessor";
-import { ConditionKind, EventType, IContent, MatrixClient, MatrixEvent, PushRuleActionName, RuleId } from "../../src";
+import {
+    ConditionKind,
+    EventType,
+    IContent,
+    IPushRule,
+    MatrixClient,
+    MatrixEvent,
+    PushRuleActionName,
+    RuleId,
+    TweakName,
+} from "../../src";
 import { mockClientMethodsUser } from "../test-utils/client";
 
 describe("NotificationService", function () {
@@ -12,21 +22,21 @@ describe("NotificationService", function () {
 
     let pushProcessor: PushProcessor;
 
-    const msc3914RoomCallRule = {
+    const msc3914RoomCallRule: IPushRule = {
         rule_id: ".org.matrix.msc3914.rule.room.call",
         default: true,
         enabled: true,
         conditions: [
             {
-                kind: "event_match",
+                kind: ConditionKind.EventMatch,
                 key: "type",
                 pattern: "org.matrix.msc3401.call",
             },
             {
-                kind: "call_started",
+                kind: ConditionKind.CallStarted,
             },
         ],
-        actions: ["notify", { set_tweak: "sound", value: "default" }],
+        actions: [PushRuleActionName.Notify, { set_tweak: TweakName.Sound, value: "default" }],
     };
 
     let matrixClient: MatrixClient;
@@ -188,23 +198,139 @@ describe("NotificationService", function () {
     });
 
     it("should add default rules in the correct order", () => {
+        const pushRules = PushProcessor.rewriteDefaultRules({
+            device: {},
+            global: {
+                content: [],
+                override: [
+                    // Include user-defined push rules inbetween .m.rule.master and other default rules to assert they are maintained in-order.
+                    {
+                        rule_id: ".m.rule.master",
+                        default: true,
+                        enabled: false,
+                        conditions: [],
+                        actions: [],
+                    },
+                    {
+                        actions: [
+                            PushRuleActionName.Notify,
+                            {
+                                set_tweak: TweakName.Sound,
+                                value: "default",
+                            },
+                            {
+                                set_tweak: TweakName.Highlight,
+                            },
+                        ],
+                        enabled: true,
+                        pattern: "coffee",
+                        rule_id: "coffee",
+                        default: false,
+                    },
+                    {
+                        actions: [
+                            PushRuleActionName.Notify,
+                            {
+                                set_tweak: TweakName.Sound,
+                                value: "default",
+                            },
+                            {
+                                set_tweak: TweakName.Highlight,
+                            },
+                        ],
+                        conditions: [
+                            {
+                                kind: ConditionKind.ContainsDisplayName,
+                            },
+                        ],
+                        enabled: true,
+                        default: true,
+                        rule_id: ".m.rule.contains_display_name",
+                    },
+                    {
+                        actions: [
+                            PushRuleActionName.Notify,
+                            {
+                                set_tweak: TweakName.Sound,
+                                value: "default",
+                            },
+                        ],
+                        conditions: [
+                            {
+                                is: "2",
+                                kind: ConditionKind.RoomMemberCount,
+                            },
+                        ],
+                        enabled: true,
+                        rule_id: ".m.rule.room_one_to_one",
+                        default: true,
+                    },
+                ],
+                room: [],
+                sender: [],
+                underride: [
+                    {
+                        actions: [
+                            PushRuleActionName.Notify,
+                            {
+                                set_tweak: TweakName.Highlight,
+                                value: false,
+                            },
+                        ],
+                        conditions: [],
+                        enabled: true,
+                        rule_id: "user-defined",
+                        default: false,
+                    },
+                    msc3914RoomCallRule,
+                    {
+                        actions: [
+                            PushRuleActionName.Notify,
+                            {
+                                set_tweak: TweakName.Highlight,
+                                value: false,
+                            },
+                        ],
+                        conditions: [],
+                        enabled: true,
+                        rule_id: ".m.rule.fallback",
+                        default: true,
+                    },
+                ],
+            },
+        });
+
         // By the time we get here, we expect the PushProcessor to have merged the new .m.rule.is_room_mention rule into the existing list of rules.
         // Check that has happened, and that it is in the right place.
-        const containsDisplayNameRuleIdx = matrixClient.pushRules?.global.override?.findIndex(
+        const containsDisplayNameRuleIdx = pushRules.global.override?.findIndex(
             (rule) => rule.rule_id === RuleId.ContainsDisplayName,
         );
         expect(containsDisplayNameRuleIdx).toBeGreaterThan(-1);
-        const isRoomMentionRuleIdx = matrixClient.pushRules?.global.override?.findIndex(
+        const isRoomMentionRuleIdx = pushRules.global.override?.findIndex(
             (rule) => rule.rule_id === RuleId.IsRoomMention,
         );
         expect(isRoomMentionRuleIdx).toBeGreaterThan(-1);
-        const mReactionRuleIdx = matrixClient.pushRules?.global.override?.findIndex(
-            (rule) => rule.rule_id === ".m.rule.reaction",
-        );
+        const mReactionRuleIdx = pushRules.global.override?.findIndex((rule) => rule.rule_id === ".m.rule.reaction");
         expect(mReactionRuleIdx).toBeGreaterThan(-1);
 
         expect(containsDisplayNameRuleIdx).toBeLessThan(isRoomMentionRuleIdx!);
         expect(isRoomMentionRuleIdx).toBeLessThan(mReactionRuleIdx!);
+
+        expect(pushRules.global.override?.map((r) => r.rule_id)).toEqual([
+            ".m.rule.master",
+            "coffee",
+            ".m.rule.contains_display_name",
+            ".m.rule.room_one_to_one",
+            ".m.rule.is_room_mention",
+            ".m.rule.reaction",
+            ".org.matrix.msc3786.rule.room.server_acl",
+        ]);
+        expect(pushRules.global.underride?.map((r) => r.rule_id)).toEqual([
+            "user-defined",
+            ".org.matrix.msc3914.rule.room.call",
+            // Assert that unknown default rules are maintained
+            ".m.rule.fallback",
+        ]);
     });
 
     // User IDs

src/pushprocessor.ts

@@ -25,8 +25,8 @@ import {
     ICallStartedPrefixCondition,
     IContainsDisplayNameCondition,
     IEventMatchCondition,
-    IEventPropertyIsCondition,
     IEventPropertyContainsCondition,
+    IEventPropertyIsCondition,
     IPushRule,
     IPushRules,
     IRoomMemberCountCondition,
@@ -115,8 +115,14 @@ const DEFAULT_OVERRIDE_RULES: Record<string, IPushRule> = {
     },
 };
 
-const EXPECTED_DEFAULT_OVERRIDE_RULE_IDS = [
+// A special rule id for `EXPECTED_DEFAULT_OVERRIDE_RULE_IDS` and friends.
+const UserDefinedRules = Symbol("UserDefinedRules");
+
+type OrderedRules = Array<string | typeof UserDefinedRules>;
+
+const EXPECTED_DEFAULT_OVERRIDE_RULE_IDS: OrderedRules = [
     RuleId.Master,
+    UserDefinedRules,
     RuleId.SuppressNotices,
     RuleId.InviteToSelf,
     RuleId.MemberEvent,
@@ -151,7 +157,8 @@ const DEFAULT_UNDERRIDE_RULES: Record<string, IPushRule> = {
     },
 };
 
-const EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS = [
+const EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS: OrderedRules = [
+    UserDefinedRules,
     RuleId.IncomingCall,
     RuleId.EncryptedDM,
     RuleId.DM,
@@ -162,24 +169,31 @@ const EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS = [
 /**
  * Make sure that each of the rules listed in `defaultRuleIds` is listed in the given set of push rules.
  *
+ * @param kind - the kind of push rule set being merged.
  * @param incomingRules - the existing set of known push rules for the user.
  * @param defaultRules - a lookup table for the default definitions of push rules.
- * @param defaultRuleIds - the IDs of the expected default push rules, in order.
+ * @param defaultRuleIds - the IDs of the expected push rules, in order.
  *
  * @returns A copy of `incomingRules`, with any missing default rules inserted in the right place.
  */
 function mergeRulesWithDefaults(
+    kind: PushRuleKind,
     incomingRules: IPushRule[],
     defaultRules: Record<string, IPushRule>,
-    defaultRuleIds: string[],
+    defaultRuleIds: OrderedRules,
 ): IPushRule[] {
-    // Calculate the index after the last default rule in `incomingRules`
-    // to allow us to split the incomingRules into defaults and custom
-    let firstCustomRuleIndex = incomingRules.findIndex((r) => !r.default);
-    if (firstCustomRuleIndex < 0) firstCustomRuleIndex = incomingRules.length;
-
-    function insertDefaultPushRule(ruleId: string): void {
-        if (ruleId in defaultRules) {
+    // Find the indices of the edges of the user-defined rules in the incoming rules
+    const mappedIncomingRules = incomingRules.map((rule) => rule.default);
+    const userDefinedRulesRange: [number, number] = [
+        mappedIncomingRules.indexOf(false),
+        mappedIncomingRules.lastIndexOf(false),
+    ];
+
+    function insertDefaultPushRule(ruleId: OrderedRules[number]): void {
+        if (ruleId === UserDefinedRules) {
+            // Re-insert any user-defined rules that were in `incomingRules`
+            newRules.push(...incomingRules.slice(...userDefinedRulesRange));
+        } else if (ruleId in defaultRules) {
             logger.warn(`Adding default global push rule ${ruleId}`);
             newRules.push(defaultRules[ruleId]);
         } else {
@@ -189,7 +203,11 @@ function mergeRulesWithDefaults(
 
     let nextExpectedRuleIdIndex = 0;
     const newRules: IPushRule[] = [];
-    for (const rule of incomingRules.slice(0, firstCustomRuleIndex)) {
+    // Merge our expected rules (including the incoming custom rules) into the incoming default rules.
+    for (const rule of [
+        ...incomingRules.slice(0, userDefinedRulesRange[0]),
+        ...incomingRules.slice(userDefinedRulesRange[1]),
+    ]) {
         const ruleIndex = defaultRuleIds.indexOf(rule.rule_id);
         if (ruleIndex === -1) {
             // an unrecognised rule; copy it over
@@ -212,8 +230,6 @@ function mergeRulesWithDefaults(
         insertDefaultPushRule(ruleId);
     }
 
-    // Finally any non-default rules that were in `incomingRules`
-    newRules.push(...incomingRules.slice(firstCustomRuleIndex));
     return newRules;
 }
 
@@ -281,12 +297,14 @@ export class PushProcessor {
 
         // Merge the client-level defaults with the ones from the server
         newRules.global.override = mergeRulesWithDefaults(
+            PushRuleKind.Override,
             newRules.global.override,
             DEFAULT_OVERRIDE_RULES,
             EXPECTED_DEFAULT_OVERRIDE_RULE_IDS,
         );
 
         newRules.global.underride = mergeRulesWithDefaults(
+            PushRuleKind.Underride,
             newRules.global.underride,
             DEFAULT_UNDERRIDE_RULES,
             EXPECTED_DEFAULT_UNDERRIDE_RULE_IDS,