Skip to content

Commit

Permalink
Bugfix for headline messages.
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jcbrand
jcbrand committed Mar 28, 2016
1 parent b3e9a17 commit f353fe8
Show file tree
Hide file tree
Showing 9 changed files with 96 additions and 36 deletions.
3 changes: 2 additions & 1 deletion bower.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
1 change: 1 addition & 0 deletions jshintrc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"global",
"it",
"jasmine",
"sinon",
"module",
"require",
"runs",
Expand Down
39 changes: 33 additions & 6 deletions spec/chatbox.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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 () {
Expand Down
12 changes: 7 additions & 5 deletions spec/headline.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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._;
Expand All @@ -30,6 +28,7 @@
* </x>
* </message>
*/
sinon.spy(utils, 'isHeadlineMessage');
runs(function () {
var stanza = $msg({
'type': 'headline',
Expand All @@ -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
});
});
});
Expand Down
31 changes: 24 additions & 7 deletions src/converse-core.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
9 changes: 3 additions & 6 deletions src/converse-headline.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
15 changes: 6 additions & 9 deletions src/converse-notification.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 15 additions & 0 deletions src/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 5 additions & 2 deletions tests/main.js
View file
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit f353fe8

Please sign in to comment.