Implement MSC3987, fix setting Element Android notifications (#3242)

Should fix #3183, since Element Android already implements [MSC3987](element-hq/element-android#8530) This is also part of #3225
matrix-org · Oct 24, 2023 · 1b124fe · 1b124fe
1 parent c1d6b9a
commit 1b124fe
Show file tree

Hide file tree

Showing 9 changed files with 18 additions and 29 deletions.
diff --git a/clientapi/clientapi_test.go b/clientapi/clientapi_test.go
@@ -1418,7 +1418,7 @@ func TestPushRules(t *testing.T) {
 				validateFunc: func(t *testing.T, respBody *bytes.Buffer) {
 					actions := gjson.GetBytes(respBody.Bytes(), "actions").Array()
 					// only a basic check
-					assert.Equal(t, 1, len(actions))
+					assert.Equal(t, 0, len(actions))
 				},
 			},
 			{

diff --git a/internal/pushrules/action_test.go b/internal/pushrules/action_test.go
@@ -13,8 +13,6 @@ func TestActionJSON(t *testing.T) {
 		Want Action
 	}{
 		{Action{Kind: NotifyAction}},
-		{Action{Kind: DontNotifyAction}},
-		{Action{Kind: CoalesceAction}},
 		{Action{Kind: SetTweakAction}},
 
 		{Action{Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"}},

diff --git a/internal/pushrules/default_override.go b/internal/pushrules/default_override.go
@@ -10,6 +10,7 @@ func defaultOverrideRules(userID string) []*Rule {
 		&mRuleRoomNotifDefinition,
 		&mRuleTombstoneDefinition,
 		&mRuleReactionDefinition,
+		&mRuleACLsDefinition,
 	}
 }
 
@@ -30,7 +31,7 @@ var (
 		RuleID:  MRuleMaster,
 		Default: true,
 		Enabled: false,
-		Actions: []*Action{{Kind: DontNotifyAction}},
+		Actions: []*Action{},
 	}
 	mRuleSuppressNoticesDefinition = Rule{
 		RuleID:  MRuleSuppressNotices,
@@ -43,7 +44,7 @@ var (
 				Pattern: pointer("m.notice"),
 			},
 		},
-		Actions: []*Action{{Kind: DontNotifyAction}},
+		Actions: []*Action{},
 	}
 	mRuleMemberEventDefinition = Rule{
 		RuleID:  MRuleMemberEvent,
@@ -56,7 +57,7 @@ var (
 				Pattern: pointer("m.room.member"),
 			},
 		},
-		Actions: []*Action{{Kind: DontNotifyAction}},
+		Actions: []*Action{},
 	}
 	mRuleContainsDisplayNameDefinition = Rule{
 		RuleID:     MRuleContainsDisplayName,
@@ -152,9 +153,7 @@ var (
 				Pattern: pointer("m.reaction"),
 			},
 		},
-		Actions: []*Action{
-			{Kind: DontNotifyAction},
-		},
+		Actions: []*Action{},
 	}
 )
 

diff --git a/internal/pushrules/default_pushrules_test.go b/internal/pushrules/default_pushrules_test.go
@@ -21,12 +21,12 @@ func TestDefaultRules(t *testing.T) {
 		// Default override rules
 		{
 			name:       ".m.rule.master",
-			inputBytes: []byte(`{"rule_id":".m.rule.master","default":true,"enabled":false,"actions":["dont_notify"]}`),
+			inputBytes: []byte(`{"rule_id":".m.rule.master","default":true,"enabled":false,"actions":[]}`),
 			want:       mRuleMasterDefinition,
 		},
 		{
 			name:       ".m.rule.suppress_notices",
-			inputBytes: []byte(`{"rule_id":".m.rule.suppress_notices","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"content.msgtype","pattern":"m.notice"}],"actions":["dont_notify"]}`),
+			inputBytes: []byte(`{"rule_id":".m.rule.suppress_notices","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"content.msgtype","pattern":"m.notice"}],"actions":[]}`),
 			want:       mRuleSuppressNoticesDefinition,
 		},
 		{
@@ -36,7 +36,7 @@ func TestDefaultRules(t *testing.T) {
 		},
 		{
 			name:       ".m.rule.member_event",
-			inputBytes: []byte(`{"rule_id":".m.rule.member_event","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"type","pattern":"m.room.member"}],"actions":["dont_notify"]}`),
+			inputBytes: []byte(`{"rule_id":".m.rule.member_event","default":true,"enabled":true,"conditions":[{"kind":"event_match","key":"type","pattern":"m.room.member"}],"actions":[]}`),
 			want:       mRuleMemberEventDefinition,
 		},
 		{

diff --git a/internal/pushrules/util.go b/internal/pushrules/util.go
@@ -16,10 +16,7 @@ func ActionsToTweaks(as []*Action) (ActionKind, map[string]interface{}, error) {
 
 	for _, a := range as {
 		switch a.Kind {
-		case DontNotifyAction:
-			// Don't bother processing any further
-			return DontNotifyAction, nil, nil
-
+		case DontNotifyAction: // Ignored
 		case SetTweakAction:
 			if tweaks == nil {
 				tweaks = map[string]interface{}{}

diff --git a/internal/pushrules/util_test.go b/internal/pushrules/util_test.go
@@ -17,17 +17,16 @@ func TestActionsToTweaks(t *testing.T) {
 		{"empty", nil, UnknownAction, nil},
 		{"zero", []*Action{{}}, UnknownAction, nil},
 		{"onlyPrimary", []*Action{{Kind: NotifyAction}}, NotifyAction, nil},
-		{"onlyPrimaryDontNotify", []*Action{{Kind: DontNotifyAction}}, DontNotifyAction, nil},
+		{"onlyPrimaryDontNotify", []*Action{}, UnknownAction, nil},
 		{"onlyTweak", []*Action{{Kind: SetTweakAction, Tweak: HighlightTweak}}, UnknownAction, map[string]interface{}{"highlight": nil}},
 		{"onlyTweakWithValue", []*Action{{Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"}}, UnknownAction, map[string]interface{}{"sound": "default"}},
 		{
 			"all",
 			[]*Action{
-				{Kind: CoalesceAction},
 				{Kind: SetTweakAction, Tweak: HighlightTweak},
 				{Kind: SetTweakAction, Tweak: SoundTweak, Value: "default"},
 			},
-			CoalesceAction,
+			UnknownAction,
 			map[string]interface{}{"highlight": nil, "sound": "default"},
 		},
 	}

diff --git a/internal/pushrules/validate.go b/internal/pushrules/validate.go
@@ -18,7 +18,7 @@ func ValidateRule(kind Kind, rule *Rule) []error {
 		errs = append(errs, fmt.Errorf("invalid rule ID: %s", rule.RuleID))
 	}
 
-	if len(rule.Actions) == 0 {
+	if rule.Actions == nil {
 		errs = append(errs, fmt.Errorf("missing actions"))
 	}
 	for _, action := range rule.Actions {

diff --git a/userapi/consumers/roomserver.go b/userapi/consumers/roomserver.go
@@ -538,8 +538,8 @@ func (s *OutputRoomEventConsumer) notifyLocal(ctx context.Context, event *rstype
 	if err != nil {
 		return fmt.Errorf("pushrules.ActionsToTweaks: %w", err)
 	}
-	// TODO: support coalescing.
-	if a != pushrules.NotifyAction && a != pushrules.CoalesceAction {
+
+	if a != pushrules.NotifyAction {
 		log.WithFields(log.Fields{
 			"event_id":  event.EventID(),
 			"room_id":   event.RoomID().String(),

diff --git a/userapi/consumers/roomserver_test.go b/userapi/consumers/roomserver_test.go
@@ -81,12 +81,8 @@ func Test_evaluatePushRules(t *testing.T) {
 			{
 				name:         "m.reaction doesn't notify",
 				eventContent: `{"type":"m.reaction","room_id":"!room:example.com"}`,
-				wantAction:   pushrules.DontNotifyAction,
-				wantActions: []*pushrules.Action{
-					{
-						Kind: pushrules.DontNotifyAction,
-					},
-				},
+				wantAction:   pushrules.UnknownAction,
+				wantActions:  []*pushrules.Action{},
 			},
 			{
 				name:         "m.room.message notifies",
@@ -136,7 +132,7 @@ func Test_evaluatePushRules(t *testing.T) {
 					t.Fatalf("expected action to be '%s', got '%s'", tc.wantAction, gotAction)
 				}
 				// this is taken from `notifyLocal`
-				if tc.wantNotify && gotAction != pushrules.NotifyAction && gotAction != pushrules.CoalesceAction {
+				if tc.wantNotify && gotAction != pushrules.NotifyAction {
 					t.Fatalf("expected to notify but didn't")
 				}
 			})