Fix merging of default push rules

[t3chguy]

Fixes element-hq/element-web#27173
Regressed by #4100

[richvdh]

A few thoughts

[richvdh]

we're actually doing quite a lot of iterating and copying of incomingRules. I'd instead start just by partitioning them:

// Split the incomingRules into defaults and custom
const incomingDefaultRules = incomingRules.filter((rule) => rule.default);
const incomingCustomRules = incomingRules.filter((rule) => !rule.default);

... and I think everything else can then be more easily written in terms of those, instead of incomingRules.slice.

Not a blocker, just a thought.

[t3chguy]

Splitting them as per your example would mean you lose track of the location of the user-defined rules. Right now only .m.rule.master precedes them but the server could send other default rules as they are added in that position. This codepath is only hit once for each incoming m.push_rules event down /sync, those should be quite rare

[richvdh]

I don't really follow. We insert the custom rules into newRules just before copying RuleId.SuppressNotices either way?

This codepath is only hit once for each incoming m.push_rules event down /sync, those should be quite rare

Yeah, I'm arguing this rather more from the PoV of clarity than performance. Just saying: it's not like doing the indexOfs here rather than copying them is saving you much.

[t3chguy]

I don't really follow. We insert the custom rules into newRules just before copying RuleId.SuppressNotices either way?

incomingRules is in the order of:

some default rules (currently just master)
user defined rules
other default rules

Your proposed code would merge the default rules into one list and the default orders wouldn't account for any rules in the first bucket other than master so the order of any additional rules specified by the server there in later spec versions go into the wrong spot

// Split the incomingRules into defaults and custom
const incomingDefaultRules = incomingRules.filter((rule) => rule.default);
const incomingCustomRules = incomingRules.filter((rule) => !rule.default);

[richvdh]

Your proposed code would merge the default rules into one list and the default orders wouldn't account for any rules in the first bucket other than master so the order of any additional rules specified by the server there in later spec versions go into the wrong spot

I don't think so. In such a scenario, the new rule would end up as the second entry in incomingDefaultRules (after master, and before suppress_notices). Assuming we change the loop below to iterate over incomingDefaultRules instead of the current pair of slices of incomingRules:

• We'll see master; it will match the expectation, and we will copy it over.
• We'll see the hypothesized new rule; it is unrecognized, and so we will copy it over.
• We'll see suppress_notices. Its index in orderedRuleIds is higher than the expectation, so first we copy over the custom rules. Then we carry on.

[t3chguy]

Making the proposed change the test fails as follows:

[t3chguy]

Turns out this was due to missing ".org.matrix.msc3914.rule.room.call" in the expected defaults in the last PR

[richvdh]

ftr I am generally deeply ambivalent about using symbolic constants in test code: using the literal gives us a chance to check that the constant is doing what we expect.

(This brought to you on the back of a Synapse bug where we had a test which used what we thought was a constant but turned out not to be.)

YMMV though, not a particular request to change this.

[t3chguy]

ftr I am generally deeply ambivalent about using symbolic constants in test code: using the literal gives us a chance to check that the constant is doing what we expect.

Its preferable to @ts-ignore.

[richvdh]

I mean I'd argue your problem here is forcing msc3914RoomCallRule to be an IPushRule, but whatever

[t3chguy]

Even if I remove that type cast there, then I'd get the failure in the call to rewritePushRules

The only reason it was fine before this PR is because we passed matrixClient.pushRules! to rewritePushRules and had
to make TS happy with the types of pushRules within the mock matrixClient

[t3chguy]

[richvdh]

I still think you should do #4135 (comment), but I'm not going to block this.

[dbkr]

Force merging because of e2e flake

[RiotRobot]

The backport to staging failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-staging staging
# Navigate to the new working tree
cd .worktrees/backport-staging
# Create a new branch
git switch --create backport-4135-to-staging
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 78a225795bb99d44f7ae04f408a9c0213d7c2c47
# Push it to GitHub
git push --set-upstream origin backport-4135-to-staging
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-staging

Then, create a pull request where the base branch is staging and the compare/head branch is backport-4135-to-staging.