Improve parsing of keyword notification rules

For notification rules, the absence of a value on a 'highlight' tweak means
that the highlight should happen; this was previously confusing us.

Use the action parser from NotificationUtils to simplify the code.

Fixes https://github.com/vector-im/vector-web/issues/1330

src/notifications/PushRuleVectorState.js

@@ -17,6 +17,7 @@ limitations under the License.
 'use strict';
 
 var StandardActions = require('./StandardActions');
+var NotificationUtils = require('./NotificationUtils');
 
 var states = {
     /** The push rule is disabled */
@@ -58,20 +59,24 @@ module.exports = {
      *
      * Determines whether a content rule is in the PushRuleVectorState.ON
      * category or in PushRuleVectorState.LOUD, regardless of its enabled
-     * state. Returns undefined if it does not match these categories.
+     * state. Returns null if it does not match these categories.
      */
     contentRuleVectorStateKind: function(rule) {
-        var stateKind;
+        var decoded = NotificationUtils.decodeActions(rule.actions);
+
+        if (!decoded) {
+            return null;
+        }
 
         // Count tweaks to determine if it is a ON or LOUD rule
         var tweaks = 0;
-        for (var j in rule.actions) {
+        if (decoded.sound) {
-            var action = rule.actions[j];
+            tweaks++;
-            if (action.set_tweak === 'sound' ||
-                (action.set_tweak === 'highlight' && action.value)) {
-                tweaks++;
-            }
         }
+        if (decoded.highlight) {
+            tweaks++;
+        }
+        var stateKind = null;
         switch (tweaks) {
             case 0:
                 stateKind = this.ON;

test/unit-tests/notifications/ContentRules-test.js

@@ -0,0 +1,117 @@
+/*
+Copyright 2016 OpenMarket Ltd
+
+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.
+*/
+
+var notifications = require('notifications');
+
+var ContentRules = notifications.ContentRules;
+var PushRuleVectorState = notifications.PushRuleVectorState;
+
+var expect = require('expect');
+var test_utils = require('../../test-utils');
+
+var NORMAL_RULE = {
+    actions: [
+        "notify",
+        { set_tweak: "highlight", value: false },
+    ],
+    enabled: true,
+    pattern: "vdh2",
+    rule_id: "vdh2",
+};
+
+var LOUD_RULE = {
+    actions: [
+        "notify",
+        { set_tweak: "highlight" },
+        { set_tweak: "sound", value: "default" },
+    ],
+    enabled: true,
+    pattern: "vdh2",
+    rule_id: "vdh2",
+};
+
+var USERNAME_RULE = {
+    actions: [
+        "notify",
+        { set_tweak: "sound", value: "default" },
+        { set_tweak: "highlight" },
+    ],
+    default: true,
+    enabled: true,
+    pattern: "richvdh",
+    rule_id: ".m.rule.contains_user_name",
+};
+
+
+
+describe("ContentRules", function() {
+    beforeEach(function() {
+        test_utils.beforeEach(this);
+    });
+
+    describe("parseContentRules", function() {
+        it("should handle there being no keyword rules", function() {
+            var rules = { 'global': { 'content': [
+                    USERNAME_RULE,
+            ]}};
+            var parsed = ContentRules.parseContentRules(rules);
+            expect(parsed.rules).toEqual([]);
+            expect(parsed.vectorState).toEqual(PushRuleVectorState.ON);
+            expect(parsed.externalRules).toEqual([]);
+        });
+
+        it("should parse regular keyword notifications", function() {
+            var rules = { 'global': { 'content': [
+                NORMAL_RULE,
+                USERNAME_RULE,
+            ]}};
+
+            var parsed = ContentRules.parseContentRules(rules);
+            expect(parsed.rules.length).toEqual(1);
+            expect(parsed.rules[0]).toEqual(NORMAL_RULE);
+            expect(parsed.vectorState).toEqual(PushRuleVectorState.ON);
+            expect(parsed.externalRules).toEqual([]);
+        });
+
+        it("should parse loud keyword notifications", function() {
+            var rules = { 'global': { 'content': [
+                LOUD_RULE,
+                USERNAME_RULE,
+            ]}};
+
+            var parsed = ContentRules.parseContentRules(rules);
+            expect(parsed.rules.length).toEqual(1);
+            expect(parsed.rules[0]).toEqual(LOUD_RULE);
+            expect(parsed.vectorState).toEqual(PushRuleVectorState.LOUD);
+            expect(parsed.externalRules).toEqual([]);
+        });
+
+        it("should parse mixed keyword notifications", function() {
+            var rules = { 'global': { 'content': [
+                LOUD_RULE,
+                NORMAL_RULE,
+                USERNAME_RULE,
+            ]}};
+
+            var parsed = ContentRules.parseContentRules(rules);
+            expect(parsed.rules.length).toEqual(1);
+            expect(parsed.rules[0]).toEqual(LOUD_RULE);
+            expect(parsed.vectorState).toEqual(PushRuleVectorState.LOUD);
+            expect(parsed.externalRules.length).toEqual(1);
+            expect(parsed.externalRules[0]).toEqual(NORMAL_RULE);
+        });
+    });
+});

test/unit-tests/notifications/PushRuleVectorState-test.js

@@ -23,8 +23,40 @@ var expect = require('expect');
 describe("PushRuleVectorState", function() {
     describe("contentRuleVectorStateKind", function() {
         it("should understand normal notifications", function () {
-            expect(prvs.contentRuleVectorStateKind(["notify"])).
+            var rule = {
+                actions: [
+                    "notify",
+                ],
+            };
+
+            expect(prvs.contentRuleVectorStateKind(rule)).
                 toEqual(prvs.ON);
         });
+
+        it("should handle loud notifications", function () {
+            var rule = {
+                actions: [
+                    "notify",
+                    { set_tweak: "highlight", value: true },
+                    { set_tweak: "sound", value: "default" },
+                ]
+            };
+
+            expect(prvs.contentRuleVectorStateKind(rule)).
+                toEqual(prvs.LOUD);
+        });
+
+        it("should understand missing highlight.value", function () {
+            var rule = {
+                actions: [
+                    "notify",
+                    { set_tweak: "highlight" },
+                    { set_tweak: "sound", value: "default" },
+                ]
+            };
+
+            expect(prvs.contentRuleVectorStateKind(rule)).
+                toEqual(prvs.LOUD);
+        });
     });
 });