From eab206c3bd30e824757c1b1cececfec2f25e3166 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 15 Apr 2016 12:42:03 +0100 Subject: [PATCH] Improve handling of notification rules we can't parse * An absent rule is the same as a rule with 'enabled == false', and is not necessarily 'OFF' (particularly in the case of the bot rule, which is inverted). * If we don't understand the rule, then don't tick any of the radio buttons, and instead show it in the 'external' section. --- .../views/settings/Notifications.js | 49 ++++---------- .../VectorPushRulesDefinitions.js | 64 +++++++++++++++---- 2 files changed, 65 insertions(+), 48 deletions(-) diff --git a/src/components/views/settings/Notifications.js b/src/components/views/settings/Notifications.js index 01d7e70b5e..e0cfd159d0 100644 --- a/src/components/views/settings/Notifications.js +++ b/src/components/views/settings/Notifications.js @@ -27,6 +27,9 @@ var notifications = require('../../../notifications'); // TODO: this "view" component still has far to much application logic in it, // which should be factored out to other files. +// TODO: this component also does a lot of direct poking into this.state, which +// is VERY NAUGHTY. + var NotificationUtils = notifications.NotificationUtils; var VectorPushRulesDefinitions = notifications.VectorPushRulesDefinitions; var PushRuleVectorState = notifications.PushRuleVectorState; @@ -408,6 +411,7 @@ module.exports = React.createClass({ _refreshFromServer: function() { var self = this; var pushRulesPromise = MatrixClientPeg.get().getPushRules().then(self._portRulesToNewAPI).done(function(rulesets) { + /// XXX seriously? wtf is this? MatrixClientPeg.get().pushRules = rulesets; // Get homeserver default rules and triage them by categories @@ -466,6 +470,7 @@ module.exports = React.createClass({ // Build the rules displayed in the Vector UI matrix table self.state.vectorPushRules = []; + self.state.externalPushRules = []; var vectorRuleIds = [ '.m.rule.contains_display_name', @@ -479,7 +484,6 @@ module.exports = React.createClass({ ]; for (var i in vectorRuleIds) { var vectorRuleId = vectorRuleIds[i]; - var ruleDefinition = VectorPushRulesDefinitions[vectorRuleId]; if (vectorRuleId === '_keywords') { // keywords needs a special handling @@ -492,42 +496,10 @@ module.exports = React.createClass({ }); } else { + var ruleDefinition = VectorPushRulesDefinitions[vectorRuleId]; var rule = defaultRules.vector[vectorRuleId]; - // Translate the rule actions and its enabled value into vector state - var vectorState; - if (rule) { - for (var stateKey in PushRuleVectorState.states) { - var state = PushRuleVectorState.states[stateKey]; - var vectorStateToActions = ruleDefinition.vectorStateToActions[state]; - - if (!vectorStateToActions) { - // No defined actions means that this vector state expects a disabled default hs rule - if (rule.enabled === false) { - vectorState = state; - break; - } - } - else { - // The actions must match to the ones expected by vector state - if (JSON.stringify(rule.actions) === JSON.stringify(vectorStateToActions)) { - // And the rule must be enabled. - if (rule.enabled === true) { - vectorState = state; - break; - } - } - } - } - - if (!vectorState) { - console.error("Cannot translate rule actions into Vector rule state. Rule: " + rule); - vectorState = PushRuleVectorState.OFF; - } - } - else { - vectorState = PushRuleVectorState.OFF; - } + var vectorState = ruleDefinition.ruleToVectorState(rule); self.state.vectorPushRules.push({ "vectorRuleId": vectorRuleId, @@ -535,6 +507,12 @@ module.exports = React.createClass({ "rule": rule, "vectorState": vectorState, }); + + // if there was a rule which we couldn't parse, add it to the external list + if (rule && !vectorState) { + rule.description = ruleDefinition.description; + self.state.externalPushRules.push(rule); + } } } @@ -544,7 +522,6 @@ module.exports = React.createClass({ '.m.rule.fallback': "Notify me for anything else" }; - self.state.externalPushRules = []; for (var i in defaultRules.others) { var rule = defaultRules.others[i]; var ruleDescription = otherRulesDescriptions[rule.rule_id]; diff --git a/src/notifications/VectorPushRulesDefinitions.js b/src/notifications/VectorPushRulesDefinitions.js index cae28a0fd5..dfbc06c083 100644 --- a/src/notifications/VectorPushRulesDefinitions.js +++ b/src/notifications/VectorPushRulesDefinitions.js @@ -17,6 +17,46 @@ limitations under the License. 'use strict'; var StandardActions = require('./StandardActions'); +var PushRuleVectorState = require('./PushRuleVectorState'); + +class VectorPushRuleDefinition { + constructor(opts) { + this.kind = opts.kind; + this.description = opts.description; + this.vectorStateToActions = opts.vectorStateToActions; + } + + // Translate the rule actions and its enabled value into vector state + ruleToVectorState(rule) { + var enabled = false; + var actions = null; + if (rule) { + enabled = rule.enabled; + actions = rule.actions; + } + + for (var stateKey in PushRuleVectorState.states) { + var state = PushRuleVectorState.states[stateKey]; + var vectorStateToActions = this.vectorStateToActions[state]; + + if (!vectorStateToActions) { + // No defined actions means that this vector state expects a disabled (or absent) rule + if (!enabled) { + return state; + } + } else { + // The actions must match to the ones expected by vector state + if (enabled && JSON.stringify(rule.actions) === JSON.stringify(vectorStateToActions)) { + return state; + } + } + } + + console.error("Cannot translate rule actions into Vector rule state. Rule: " + + JSON.stringify(rule)); + return undefined; + } +}; /** * The descriptions of rules managed by the Vector UI. @@ -24,7 +64,7 @@ var StandardActions = require('./StandardActions'); module.exports = { // Messages containing user's display name // (skip contains_user_name which is too geeky) - ".m.rule.contains_display_name": { + ".m.rule.contains_display_name": new VectorPushRuleDefinition({ kind: "underride", description: "Messages containing my name", vectorStateToActions: { // The actions for each vector state, or null to disable the rule. @@ -32,10 +72,10 @@ module.exports = { loud: StandardActions.ACTION_HIGHLIGHT_DEFAULT_SOUND, off: StandardActions.ACTION_DISABLED } - }, + }), // Messages just sent to the user in a 1:1 room - ".m.rule.room_one_to_one": { + ".m.rule.room_one_to_one": new VectorPushRuleDefinition({ kind: "underride", description: "Messages in one-to-one chats", vectorStateToActions: { @@ -43,12 +83,12 @@ module.exports = { loud: StandardActions.ACTION_NOTIFY_DEFAULT_SOUND, off: StandardActions.ACTION_DONT_NOTIFY } - }, + }), // Messages just sent to a group chat room // 1:1 room messages are catched by the .m.rule.room_one_to_one rule if any defined // By opposition, all other room messages are from group chat rooms. - ".m.rule.message": { + ".m.rule.message": new VectorPushRuleDefinition({ kind: "underride", description: "Messages in group chats", vectorStateToActions: { @@ -56,10 +96,10 @@ module.exports = { loud: StandardActions.ACTION_NOTIFY_DEFAULT_SOUND, off: StandardActions.ACTION_DONT_NOTIFY } - }, + }), // Invitation for the user - ".m.rule.invite_for_me": { + ".m.rule.invite_for_me": new VectorPushRuleDefinition({ kind: "underride", description: "When I'm invited to a room", vectorStateToActions: { @@ -67,10 +107,10 @@ module.exports = { loud: StandardActions.ACTION_NOTIFY_DEFAULT_SOUND, off: StandardActions.ACTION_DISABLED } - }, + }), // Incoming call - ".m.rule.call": { + ".m.rule.call": new VectorPushRuleDefinition({ kind: "underride", description: "Call invitation", vectorStateToActions: { @@ -78,10 +118,10 @@ module.exports = { loud: StandardActions.ACTION_NOTIFY_RING_SOUND, off: StandardActions.ACTION_DISABLED } - }, + }), // Notifications from bots - ".m.rule.suppress_notices": { + ".m.rule.suppress_notices": new VectorPushRuleDefinition({ kind: "override", description: "Messages sent by bot", vectorStateToActions: { @@ -90,5 +130,5 @@ module.exports = { loud: StandardActions.ACTION_NOTIFY_DEFAULT_SOUND, off: StandardActions.ACTION_DONT_NOTIFY, } - } + }), };