From f353fe8611d98a3ca54fb727b3b5f0c451f5db2e Mon Sep 17 00:00:00 2001 From: JC Brand Date: Mon, 28 Mar 2016 10:49:52 +0000 Subject: [PATCH] Bugfix for headline messages. Couldn't handle messages with no "from" attribute. Some refactoring to add code that checks if a messages is a headline to the utils module. Updated tests. Add sinon so that we can test returned value of spy. --- bower.json | 3 ++- jshintrc | 1 + spec/chatbox.js | 39 ++++++++++++++++++++++++++++++------ spec/headline.js | 12 ++++++----- src/converse-core.js | 31 +++++++++++++++++++++------- src/converse-headline.js | 9 +++------ src/converse-notification.js | 15 ++++++-------- src/utils.js | 15 ++++++++++++++ tests/main.js | 7 +++++-- 9 files changed, 96 insertions(+), 36 deletions(-) diff --git a/bower.json b/bower.json index 4efe66f31..389b0006a 100644 --- a/bower.json +++ b/bower.json @@ -30,7 +30,8 @@ "skeleton-sass": "~2.0.3", "strophejs": "1.2.4", "strophejs-plugins": "https://github.com/strophe/strophejs-plugins.git#amd", - "bourbon": "~4.2.3" + "bourbon": "~4.2.3", + "sinon": "^1.17.3" }, "resolutions": { "backbone": "1.1.2" diff --git a/jshintrc b/jshintrc index d446704dd..c92b809fa 100644 --- a/jshintrc +++ b/jshintrc @@ -20,6 +20,7 @@ "global", "it", "jasmine", + "sinon", "module", "require", "runs", diff --git a/spec/chatbox.js b/spec/chatbox.js index 7dffb5808..7bbd67c82 100644 --- a/spec/chatbox.js +++ b/spec/chatbox.js @@ -3,13 +3,11 @@ define([ "jquery", "underscore", + "utils", "mock", "test_utils" - ], function ($, _, mock, test_utils) { - return factory($, _, mock, test_utils); - } - ); -} (this, function ($, _, mock, test_utils) { + ], factory); +} (this, function ($, _, utils, mock, test_utils) { var $msg = converse_api.env.$msg; var Strophe = converse_api.env.Strophe; var moment = converse_api.env.moment; @@ -461,6 +459,7 @@ it("is ignored if it's intended for a different resource", function () { // Send a message from a different resource spyOn(converse, 'log'); + spyOn(converse.chatboxes, 'getChatBox'); var sender_jid = mock.cur_names[0].replace(/ /g,'.').toLowerCase() + '@localhost'; var msg = $msg({ from: sender_jid, @@ -471,7 +470,35 @@ .c('active', {'xmlns': 'http://jabber.org/protocol/chatstates'}).tree(); converse.chatboxes.onMessage(msg); expect(converse.log).toHaveBeenCalledWith( - "Ignore incoming message intended for a different resource: dummy@localhost/some-other-resource", "info"); + "onMessage: Ignoring incoming message intended for a different resource: dummy@localhost/some-other-resource", "info"); + expect(converse.chatboxes.getChatBox).not.toHaveBeenCalled(); + }); + + it("is ignored if it's a malformed headline message", function () { + /* Ideally we wouldn't have to filter out headline + * messages, but Prosody gives them the wrong 'type' :( + */ + sinon.spy(converse, 'log'); + sinon.spy(converse.chatboxes, 'getChatBox'); + sinon.spy(utils, 'isHeadlineMessage'); + var msg = $msg({ + from: 'localhost', + to: converse.bare_jid, + type: 'chat', + id: (new Date()).getTime() + }).c('body').t("This headline message will not be shown").tree(); + converse.chatboxes.onMessage(msg); + expect(converse.log.calledWith( + "onMessage: Ignoring incoming headline message sent with type 'chat' from JID: localhost", + "info" + )).toBeTruthy(); + expect(utils.isHeadlineMessage.called).toBeTruthy(); + expect(utils.isHeadlineMessage.returned(true)).toBeTruthy(); + expect(converse.chatboxes.getChatBox.called).toBeFalsy(); + // Remove sinon spies + converse.log.restore(); + converse.chatboxes.getChatBox.restore(); + utils.isHeadlineMessage.restore(); }); it("can be a carbon message, as defined in XEP-0280", function () { diff --git a/spec/headline.js b/spec/headline.js index a7703a301..c937f6d1b 100644 --- a/spec/headline.js +++ b/spec/headline.js @@ -2,13 +2,11 @@ (function (root, factory) { define([ "jquery", + "utils", "mock", "test_utils" - ], function ($, mock, test_utils) { - return factory($, mock, test_utils); - } - ); -} (this, function ($, mock, test_utils) { + ], factory); +} (this, function ($, utils, mock, test_utils) { "use strict"; var $msg = converse_api.env.$msg, _ = converse_api.env._; @@ -30,6 +28,7 @@ * * */ + sinon.spy(utils, 'isHeadlineMessage'); runs(function () { var stanza = $msg({ 'type': 'headline', @@ -50,6 +49,9 @@ converse.chatboxviews.keys(), 'notify.example.com') ).toBeTruthy(); + expect(utils.isHeadlineMessage.called).toBeTruthy(); + expect(utils.isHeadlineMessage.returned(true)).toBeTruthy(); + utils.isHeadlineMessage.restore(); // unwraps }); }); }); diff --git a/src/converse-core.js b/src/converse-core.js index bef68abd3..224da4dc5 100755 --- a/src/converse-core.js +++ b/src/converse-core.js @@ -1308,22 +1308,39 @@ }, onMessage: function (message) { - /* Handler method for all incoming single-user chat "message" stanzas. + /* Handler method for all incoming single-user chat "message" + * stanzas. */ var $message = $(message), - contact_jid, $forwarded, $delay, from_bare_jid, from_resource, is_me, msgid, + contact_jid, $forwarded, $delay, from_bare_jid, + from_resource, is_me, msgid, chatbox, resource, from_jid = $message.attr('from'), to_jid = $message.attr('to'), to_resource = Strophe.getResourceFromJid(to_jid); if (to_resource && to_resource !== converse.resource) { - converse.log('Ignore incoming message intended for a different resource: '+to_jid, 'info'); + converse.log( + 'onMessage: Ignoring incoming message intended for a different resource: '+to_jid, + 'info' + ); return true; - } - if (from_jid === converse.connection.jid) { - // FIXME: Forwarded messages should be sent to specific resources, not broadcasted - converse.log("Ignore incoming message sent from this client's JID: "+from_jid, 'info'); + } else if (from_jid === converse.connection.jid) { + // FIXME: Forwarded messages should be sent to specific + // resources, not broadcasted + converse.log( + "onMessage: Ignoring incoming message sent from this client's JID: "+from_jid, + 'info' + ); + return true; + } else if (utils.isHeadlineMessage(message)) { + // XXX: Ideally we wouldn't have to check for headline + // messages, but Prosody sends headline messages with the + // wrong type ('chat'), so we need to filter them out here. + converse.log( + "onMessage: Ignoring incoming headline message sent with type 'chat' from JID: "+from_jid, + 'info' + ); return true; } $forwarded = $message.find('forwarded'); diff --git a/src/converse-headline.js b/src/converse-headline.js index 92e2baaae..d83a7437f 100644 --- a/src/converse-headline.js +++ b/src/converse-headline.js @@ -22,12 +22,9 @@ var onHeadlineMessage = function (message) { /* Handler method for all incoming messages of type "headline". */ - var $message = $(message), from_jid = $message.attr('from'); - if ($message.attr('type') === 'headline' || from_jid.indexOf('@') === -1) { - // Some servers (I'm looking at you Prosody) don't set the message - // type to "headline" when sending server messages. For now we - // check if an @ signal is included, and if not, we assume it's - // a headline message. + var $message = $(message), + from_jid = $message.attr('from'); + if (utils.isHeadlineMessage(message)) { converse.chatboxes.create({ 'id': from_jid, 'jid': from_jid, diff --git a/src/converse-notification.js b/src/converse-notification.js index 6e0d6a3a9..935e8e7a2 100644 --- a/src/converse-notification.js +++ b/src/converse-notification.js @@ -68,19 +68,16 @@ return true; }; - converse.shouldNotifyOfMessage = function ($message) { + converse.shouldNotifyOfMessage = function (message) { /* Is this a message worthy of notification? */ - var $forwarded = $message.find('forwarded'); + var $message = $(message), + $forwarded = $message.find('forwarded'); if ($forwarded.length) { return false; - } - if ($message.attr('type') === 'groupchat') { + } else if ($message.attr('type') === 'groupchat') { return converse.shouldNotifyOfGroupMessage($message); - } - if ($message.attr('type') === 'headline' || $message.attr('from').indexOf('@') === -1) { - // XXX: 2nd check is workaround for Prosody which doesn't give type "headline" - + } else if (utils.isHeadlineMessage(message)) { // We want to show notifications for headline messages. return true; } @@ -198,7 +195,7 @@ * to play sounds and show HTML5 notifications. */ var $message = $(message); - if (!converse.shouldNotifyOfMessage($message)) { + if (!converse.shouldNotifyOfMessage(message)) { return false; } converse.playSoundNotification($message); diff --git a/src/utils.js b/src/utils.js index 84c4eecee..5be8bc165 100755 --- a/src/utils.js +++ b/src/utils.js @@ -125,6 +125,21 @@ return str; }, + isHeadlineMessage: function (message) { + var $message = $(message), + from_jid = $message.attr('from'); + if ($message.attr('type') === 'headline' || + // Some servers (I'm looking at you Prosody) don't set the message + // type to "headline" when sending server messages. For now we + // check if an @ signal is included, and if not, we assume it's + // a headline message. + (typeof from_jid !== 'undefined' && from_jid.indexOf('@') === -1) + ) { + return true; + } + return false; + }, + refreshWebkit: function () { /* This works around a webkit bug. Refreshes the browser's viewport, * otherwise chatboxes are not moved along when one is closed. diff --git a/tests/main.js b/tests/main.js index 8a5c42905..2db608599 100644 --- a/tests/main.js +++ b/tests/main.js @@ -1,6 +1,7 @@ // Extra test dependencies config.paths.mock = "tests/mock"; config.paths.test_utils = "tests/utils"; +config.paths.sinon = "components/sinon/lib/sinon"; config.paths.jasmine = "components/jasmine/lib/jasmine-core/jasmine"; config.paths["jasmine-html"] = "components/jasmine/lib/jasmine-core/jasmine-html"; config.paths["console-runner"] = "node_modules/phantom-jasmine/lib/console-runner"; @@ -34,9 +35,11 @@ require([ "jquery", "converse", "mock", - "jasmine-html" - ], function($, converse, mock, jasmine) { + "jasmine-html", + "sinon" + ], function($, converse, mock, jasmine, sinon) { // Set up converse.js + window.sinon = sinon; window.converse_api = converse; window.localStorage.clear(); window.sessionStorage.clear();