From 121fe3418010a441da4a8a7f45625a654194bf75 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 14 Apr 2016 22:45:00 +0100 Subject: [PATCH] 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 | 21 ++-- .../notifications/ContentRules-test.js | 117 ++++++++++++++++++ .../notifications/PushRuleVectorState-test.js | 34 ++++- 3 files changed, 163 insertions(+), 9 deletions(-) create mode 100644 test/unit-tests/notifications/ContentRules-test.js diff --git a/src/notifications/PushRuleVectorState.js b/src/notifications/PushRuleVectorState.js index 67f9a3c0..c838aa20 100644 --- a/src/notifications/PushRuleVectorState.js +++ b/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) { - var action = rule.actions[j]; - if (action.set_tweak === 'sound' || - (action.set_tweak === 'highlight' && action.value)) { - tweaks++; - } + if (decoded.sound) { + tweaks++; } + if (decoded.highlight) { + tweaks++; + } + var stateKind = null; switch (tweaks) { case 0: stateKind = this.ON; diff --git a/test/unit-tests/notifications/ContentRules-test.js b/test/unit-tests/notifications/ContentRules-test.js new file mode 100644 index 00000000..e7928147 --- /dev/null +++ b/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); + }); + }); +}); diff --git a/test/unit-tests/notifications/PushRuleVectorState-test.js b/test/unit-tests/notifications/PushRuleVectorState-test.js index 48084f08..6b0f81c6 100644 --- a/test/unit-tests/notifications/PushRuleVectorState-test.js +++ b/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); + }); }); });