muc: refactor MUC joining

We now distinguish more clearly between joining and re-joining a MUC.

The `enterRoom` method has been refactored away.

Instead, `join` or `rejoin` should be used, depending on the circumstances.

Don't fetch cached occupants in the `initialize` function, instead, only fetch
them if we're restoring a MUC that we're still connected to.

If we're not restoring a still-connected MUC, then we clear the
occupants cache, and the messages cache if `clear_messages_on_reconnection` is `true`.

updates #1807
updates #1808

spec/bookmarks.js

@@ -191,17 +191,18 @@
                     ['rosterGroupsFetched'], {}, async function (done, _converse) {
 
                 await test_utils.waitUntilBookmarksReturned(_converse);
-                const room_jid = 'coven@chat.shakespeare.lit';
+                const muc_jid = 'coven@chat.shakespeare.lit';
                 _converse.bookmarks.create({
-                    'jid': room_jid,
+                    'jid': muc_jid,
                     'autojoin': false,
                     'name':  'The Play',
                     'nick': 'Othello'
                 });
-                const room = await _converse.api.rooms.open(room_jid);
-                spyOn(room, 'join').and.callThrough();
-                await test_utils.getRoomFeatures(_converse, 'coven', 'chat.shakespeare.lit');
-                await u.waitUntil(() => room.join.calls.count());
+                spyOn(_converse.ChatRoom.prototype, 'getAndPersistNickname').and.callThrough();
+                const room_creation_promise = _converse.api.rooms.open(muc_jid);
+                await test_utils.getRoomFeatures(_converse, muc_jid);
+                const room = await room_creation_promise;
+                await u.waitUntil(() => room.getAndPersistNickname.calls.count());
                 expect(room.get('nick')).toBe('Othello');
                 done();
             }));

spec/muc.js

@@ -22,7 +22,11 @@
                     async function (done, _converse) {
 
                 await test_utils.openAndEnterChatRoom(_converse, 'lounge@montague.lit', 'romeo');
+
+                _converse.connection.IQ_stanzas = [];
                 await test_utils.openAndEnterChatRoom(_converse, 'leisure@montague.lit', 'romeo');
+
+                _converse.connection.IQ_stanzas = [];
                 await test_utils.openAndEnterChatRoom(_converse, 'news@montague.lit', 'romeo');
                 expect(u.isVisible(_converse.chatboxviews.get('lounge@montague.lit').el)).toBeTruthy();
                 expect(u.isVisible(_converse.chatboxviews.get('leisure@montague.lit').el)).toBeTruthy();
@@ -108,6 +112,7 @@
                 await test_utils.openControlBox(_converse);
                 await test_utils.waitForRoster(_converse, 'current');
                 await u.waitUntil(() => _converse.rosterview.el.querySelectorAll('.roster-group .group-toggle').length);
+
                 let room = await _converse.api.rooms.open(jid);
                 // Test on groupchat that's not yet open
                 expect(room instanceof Backbone.Model).toBeTruthy();
@@ -267,7 +272,7 @@
                     ['rosterGroupsFetched', 'chatBoxesFetched', 'emojisInitialized'], {},
                     async function (done, _converse) {
 
-                const IQ_stanzas = _converse.connection.IQ_stanzas;
+                let IQ_stanzas = _converse.connection.IQ_stanzas;
                 const muc_jid = 'lounge@montague.lit';
                 await test_utils.openChatRoom(_converse, 'lounge', 'montague.lit', 'romeo');
 
@@ -291,6 +296,8 @@
                 input.value = 'nicky';
                 view.el.querySelector('input[type=submit]').click();
                 expect(view.model.join).toHaveBeenCalled();
+                _converse.connection.IQ_stanzas = [];
+                await test_utils.getRoomFeatures(_converse, muc_jid);
                 await u.waitUntil(() => view.model.get('connection_status') === converse.ROOMSTATUS.CONNECTING);
 
                 // The user has just entered the room (because join was called)
@@ -334,7 +341,9 @@
                  *   <query xmlns="http://jabber.org/protocol/muc#owner"><x xmlns="jabber:x:data" type="submit"/></query>
                  * </iq>
                  */
-                const iq = IQ_stanzas.filter(s => s.querySelector(`query[xmlns="${Strophe.NS.MUC_OWNER}"]`)).pop();
+                const selector = `query[xmlns="${Strophe.NS.MUC_OWNER}"]`;
+                IQ_stanzas = _converse.connection.IQ_stanzas;
+                const iq = await u.waitUntil(() => IQ_stanzas.filter(s => s.querySelector(selector)).pop());
                 expect(Strophe.serialize(iq)).toBe(
                     `<iq id="${iq.getAttribute('id')}" to="lounge@montague.lit" type="set" xmlns="jabber:client">`+
                         `<query xmlns="http://jabber.org/protocol/muc#owner"><x type="submit" xmlns="jabber:x:data"/>`+
@@ -405,10 +414,8 @@
                             Strophe.NS.SID
                         ];
                         const nick = 'romeo';
-                        const room = Strophe.getNodeFromJid(muc_jid);
-                        const server = Strophe.getDomainFromJid(muc_jid);
                         await _converse.api.rooms.open(muc_jid);
-                        await test_utils.getRoomFeatures(_converse, room, server, features);
+                        await test_utils.getRoomFeatures(_converse, muc_jid, features);
                         await test_utils.waitForReservedNick(_converse, muc_jid, nick);
                         test_utils.receiveOwnMUCPresence(_converse, muc_jid, nick);
                         const view = _converse.chatboxviews.get(muc_jid);
@@ -532,11 +539,9 @@
                     async function (done, _converse) {
 
                 const muc_jid = 'coven@chat.shakespeare.lit';
-                const room = Strophe.getNodeFromJid(muc_jid);
-                const server = Strophe.getDomainFromJid(muc_jid);
                 const nick = 'romeo';
                 await _converse.api.rooms.open(muc_jid);
-                await test_utils.getRoomFeatures(_converse, room, server);
+                await test_utils.getRoomFeatures(_converse, muc_jid);
                 await test_utils.waitForReservedNick(_converse, muc_jid, nick);
 
                 const view = _converse.chatboxviews.get(muc_jid);
@@ -577,8 +582,9 @@
                     ['rosterGroupsFetched', 'chatBoxesFetched', 'emojisInitialized'], {},
                     async function (done, _converse) {
 
+                const muc_jid = 'coven@chat.shakespeare.lit';
                 await test_utils.openChatRoom(_converse, "coven", 'chat.shakespeare.lit', 'some1');
-                await test_utils.getRoomFeatures(_converse, 'coven', 'chat.shakespeare.lit');
+                await test_utils.getRoomFeatures(_converse, muc_jid);
 
                 const view = _converse.chatboxviews.get('coven@chat.shakespeare.lit');
                 const chat_content = view.el.querySelector('.chat-content');
@@ -1633,13 +1639,8 @@
                 expect(view.model.occupants.length).toBe(9);
                 expect(view.model.occupants.filter(o => o.isMember()).length).toBe(8);
 
-
+                view.model.rejoin();
                 // Test that members aren't removed when we reconnect
-                // See example 21 https://xmpp.org/extensions/xep-0045.html#enter-pres
-                spyOn(view.model, 'removeNonMembers').and.callThrough();
-                view.model.save('connection_status', converse.ROOMSTATUS.DISCONNECTED);
-                view.model.enterRoom();
-                expect(view.model.removeNonMembers).toHaveBeenCalled();
                 expect(view.model.occupants.length).toBe(8);
                 expect(occupants.querySelectorAll('li').length).toBe(8);
                 done();
@@ -1856,8 +1857,6 @@
                         .c('item-not-found', {'xmlns': "urn:ietf:params:xml:ns:xmpp-stanzas"});
                 _converse.connection._dataRecv(test_utils.createRequest(features_stanza));
 
-                const view = _converse.chatboxviews.get('lounge@montague.lit');
-                spyOn(view.model, 'join').and.callThrough();
 
                 /* <iq from='hag66@shakespeare.lit/pda'
                  *     id='getnick1'
@@ -1890,6 +1889,7 @@
                  *     </query>
                  * </iq>
                  */
+                const view = _converse.chatboxviews.get('lounge@montague.lit');
                 stanza = $iq({
                     'type': 'result',
                     'id': iq.getAttribute('id'),
@@ -1899,8 +1899,6 @@
                 .c('identity', {'category': 'conference', 'name': 'thirdwitch', 'type': 'text'});
                 _converse.connection._dataRecv(test_utils.createRequest(stanza));
 
-                expect(view.model.join).toHaveBeenCalled();
-
                 // The user has just entered the groupchat (because join was called)
                 // and receives their own presence from the server.
                 // See example 24:
@@ -2203,7 +2201,7 @@
                     async function (done, _converse) {
 
                 const muc_jid = 'coven@chat.shakespeare.lit';
-                await test_utils.openAndEnterChatRoom(_converse, 'coven@chat.shakespeare.lit', 'romeo');
+                await test_utils.openAndEnterChatRoom(_converse, muc_jid, 'romeo');
                 const view = _converse.chatboxviews.get(muc_jid);
                 expect(view.model.get('connection_status')).toBe(converse.ROOMSTATUS.ENTERED);
                 await test_utils.sendMessage(view, 'hello world');
@@ -2238,8 +2236,9 @@
                 sent_stanzas = _converse.connection.sent_stanzas;
                 const index = sent_stanzas.length -1;
 
+                _converse.connection.IQ_stanzas = [];
                 _converse.connection._dataRecv(test_utils.createRequest(result));
-                await test_utils.getRoomFeatures(_converse, 'coven', 'chat.shakespeare.lit');
+                await test_utils.getRoomFeatures(_converse, muc_jid);
 
                 const pres = await u.waitUntil(
                     () => sent_stanzas.slice(index).filter(s => s.nodeName === 'presence').pop());
@@ -4320,9 +4319,9 @@
                 const sent_IQs = _converse.connection.IQ_stanzas;
                 const muc_jid = 'coven@chat.shakespeare.lit';
 
-                await _converse.api.rooms.open(muc_jid, {'nick': 'romeo'});
+                const room_creation_promise = _converse.api.rooms.open(muc_jid, {'nick': 'romeo'});
 
-                // Check that the groupchat queried for the feautures.
+                // Check that the groupchat queried for the features.
                 let stanza = await u.waitUntil(() => sent_IQs.filter(iq => iq.querySelector(`iq[to="${muc_jid}"] query[xmlns="http://jabber.org/protocol/disco#info"]`)).pop());
                 expect(Strophe.serialize(stanza)).toBe(
                     `<iq from="romeo@montague.lit/orchard" id="${stanza.getAttribute("id")}" to="${muc_jid}" type="get" xmlns="jabber:client">`+
@@ -4351,6 +4350,8 @@
                 await u.waitUntil(() => (view.model.get('connection_status') === converse.ROOMSTATUS.CONNECTING));
                 expect(view.model.features.get('membersonly')).toBeTruthy();
 
+                await room_creation_promise;
+
                 await test_utils.createContacts(_converse, 'current');
 
                 let sent_stanza, sent_id;

spec/roomslist.js

@@ -279,7 +279,7 @@
             await test_utils.openControlBox(_converse);
             const room_jid = 'kitchen@conference.shakespeare.lit';
             await u.waitUntil(() => _converse.rooms_list_view !== undefined, 500);
-            await test_utils.openAndEnterChatRoom(_converse, 'kitchen@conference.shakespeare.lit', 'romeo');
+            await test_utils.openAndEnterChatRoom(_converse, room_jid, 'romeo');
             const view = _converse.chatboxviews.get(room_jid);
             view.model.set({'minimized': true});
             const nick = mock.chatroom_names[0];

src/converse-muc-views.js

@@ -1649,12 +1649,11 @@ converse.plugins.add('converse-muc-views', {
                 });
                 const switch_el = container.querySelector('a.switch-chat');
                 if (switch_el) {
-                    switch_el.addEventListener('click', ev => {
+                    switch_el.addEventListener('click', async ev => {
                         ev.preventDefault();
-                        this.model.save('jid', moved_jid);
-                        container.innerHTML = '';
-                        this.showSpinner();
-                        this.model.enterRoom();
+                        const room = await _converse.api.rooms.get(moved_jid, null, true);
+                        room.maybeShow(true);
+                        this.model.destroy();
                     });
                 }
                 u.showElement(container);

src/headless/converse-muc.js

@@ -387,10 +387,13 @@ converse.plugins.add('converse-muc', {
                 this.on('change:connection_status', this.onConnectionStatusChanged, this);
 
                 this.initMessages();
+                this.initOccupants();
                 this.registerHandlers();
 
-                await this.initOccupants();
-                this.enterRoom();
+                const restored = await this.restoreFromCache()
+                if (!restored) {
+                    this.join();
+                }
                 this.initialized.resolve();
             },
 
@@ -402,25 +405,85 @@ converse.plugins.add('converse-muc', {
                 }
             },
 
-            async enterRoom () {
-                const conn_status = this.get('connection_status');
-                if (conn_status !==  converse.ROOMSTATUS.ENTERED) {
-                    // We're not restoring a room from cache, so let's clear the potentially stale cache.
-                    this.removeNonMembers();
-                    await this.refreshRoomFeatures();
-                    if (_converse.muc_show_logs_before_join) {
-                        await this.fetchMessages();
-                    } else if (_converse.clear_messages_on_reconnection) {
-                        await this.clearMessages();
-                    }
-                    this.join();
-                } else if (!(await this.rejoinIfNecessary())) {
+            /**
+             * Checks whether we're still joined and if so, restores the MUC state from cache.
+             * @private
+             * @method _converse.ChatRoom#restoreFromCache
+             * @returns { Boolean } Returns `true` if we're still joined, otherwise returns `false`.
+             */
+            async restoreFromCache () {
+                if (this.get('connection_status') === converse.ROOMSTATUS.ENTERED && await this.isJoined()) {
                     // We've restored the room from cache and we're still joined.
                     await new Promise(resolve => this.features.fetch({'success': resolve, 'error': resolve}));
+                    await this.fetchOccupants();
                     await this.fetchMessages();
+                    return true;
+                } else {
+                    await this.clearCache();
+                    return false;
                 }
             },
 
+            /**
+             * Join the MUC
+             * @private
+             * @method _converse.ChatRoom#join
+             * @param { String } nick - The user's nickname
+             * @param { String } [password] - Optional password, if required by the groupchat.
+             */
+            async join (nick, password) {
+                if (this.get('connection_status') === converse.ROOMSTATUS.ENTERED) {
+                    // We have restored a groupchat from session storage,
+                    // so we don't send out a presence stanza again.
+                    return this;
+                }
+                await this.refreshRoomFeatures();
+                nick = await this.getAndPersistNickname(nick);
+                if (!nick) {
+                    u.safeSave(this, {'connection_status': converse.ROOMSTATUS.NICKNAME_REQUIRED});
+                    if (_converse.muc_show_logs_before_join) {
+                        await this.fetchMessages();
+                    }
+                    return this;
+                }
+                const stanza = $pres({
+                    'from': _converse.connection.jid,
+                    'to': this.getRoomJIDAndNick()
+                }).c("x", {'xmlns': Strophe.NS.MUC})
+                  .c("history", {'maxstanzas': this.features.get('mam_enabled') ? 0 : _converse.muc_history_max_stanzas}).up();
+
+                if (password) {
+                    stanza.cnode(Strophe.xmlElement("password", [], password));
+                }
+                this.save('connection_status', converse.ROOMSTATUS.CONNECTING);
+                _converse.api.send(stanza);
+                return this;
+            },
+
+            async clearCache () {
+                this.save('connection_status', converse.ROOMSTATUS.DISCONNECTED);
+                if (this.occupants.length) {
+                    // Remove non-members when reconnecting
+                    this.occupants.filter(o => !o.isMember()).forEach(o => o.destroy());
+                } else {
+                    // Looks like we haven't restored occupants from cache, so we clear it.
+                    this.occupants.clearSession();
+                }
+                if (_converse.clear_messages_on_reconnection) {
+                    await this.clearMessages();
+                }
+            },
+
+            /**
+             * Clear stale cache and re-join a MUC we've been in before.
+             * @private
+             * @method _converse.ChatRoom#rejoin
+             */
+            rejoin () {
+                this.clearCache();
+                return this.join();
+            },
+
             async onConnectionStatusChanged () {
                 if (this.get('connection_status') === converse.ROOMSTATUS.ENTERED) {
                     if (_converse.muc_fetch_members) {
@@ -442,17 +505,9 @@ converse.plugins.add('converse-muc', {
                 }
             },
 
-            removeNonMembers () {
-                const non_members = this.occupants.filter(o => !o.isMember());
-                if (non_members.length) {
-                    non_members.forEach(o => o.destroy());
-                }
-            },
-
             async onReconnection () {
-                this.save('connection_status', converse.ROOMSTATUS.DISCONNECTED);
                 this.registerHandlers();
-                await this.enterRoom();
+                await this.rejoin();
                 this.announceReconnection();
             },
 
@@ -468,7 +523,10 @@ converse.plugins.add('converse-muc', {
                 this.occupants = new _converse.ChatRoomOccupants();
                 const id = `converse.occupants-${_converse.bare_jid}${this.get('jid')}`;
                 this.occupants.browserStorage = _converse.createStore(id, 'session');
-                this.occupants.chatroom  = this;
+                this.occupants.chatroom = this;
+            },
+
+            fetchOccupants () {
                 this.occupants.fetched = new Promise(resolve => {
                     this.occupants.fetch({
                         'add': true,
@@ -564,38 +622,6 @@ converse.plugins.add('converse-muc', {
                 }
             },
 
-            /**
-             * Join the groupchat.
-             * @private
-             * @method _converse.ChatRoom#join
-             * @param { String } nick - The user's nickname
-             * @param { String } [password] - Optional password, if required by the groupchat.
-             */
-            async join (nick, password) {
-                if (this.get('connection_status') === converse.ROOMSTATUS.ENTERED) {
-                    // We have restored a groupchat from session storage,
-                    // so we don't send out a presence stanza again.
-                    return this;
-                }
-                nick = await this.getAndPersistNickname(nick);
-                if (!nick) {
-                    u.safeSave(this, {'connection_status': converse.ROOMSTATUS.NICKNAME_REQUIRED});
-                    return this;
-                }
-                const stanza = $pres({
-                    'from': _converse.connection.jid,
-                    'to': this.getRoomJIDAndNick()
-                }).c("x", {'xmlns': Strophe.NS.MUC})
-                  .c("history", {'maxstanzas': this.features.get('mam_enabled') ? 0 : _converse.muc_history_max_stanzas}).up();
-
-                if (password) {
-                    stanza.cnode(Strophe.xmlElement("password", [], password));
-                }
-                this.save('connection_status', converse.ROOMSTATUS.CONNECTING);
-                _converse.api.send(stanza);
-                return this;
-            },
-
             /**
              * Sends a message stanza to the XMPP server and expects a reflection
              * or error message within a specific timeout period.
@@ -1262,7 +1288,7 @@ converse.plugins.add('converse-muc', {
                     _converse.getDefaultMUCNickname();
 
                 if (nick) {
-                    this.save({'nick': nick}, {'silent': true});
+                    this.save({nick}, {'silent': true});
                 }
                 return nick;
             },
@@ -1581,10 +1607,8 @@ converse.plugins.add('converse-muc', {
              * @method _converse.ChatRoom#rejoinIfNecessary
              */
             async rejoinIfNecessary () {
-                const is_joined = await this.isJoined();
-                if (!is_joined) {
-                    this.save('connection_status', converse.ROOMSTATUS.DISCONNECTED);
-                    this.enterRoom();
+                if (! await this.isJoined()) {
+                    this.rejoin();
                     return true;
                 }
             },
@@ -2231,7 +2255,7 @@ converse.plugins.add('converse-muc', {
             if (result === true) {
                 const chatroom = await openChatRoom(room_jid, {'password': x_el.getAttribute('password') });
                 if (chatroom.get('connection_status') === converse.ROOMSTATUS.DISCONNECTED) {
-                    _converse.chatboxes.get(room_jid).join();
+                    _converse.chatboxes.get(room_jid).rejoin();
                 }
             }
         };

tests/utils.js

@@ -145,18 +145,15 @@
         return model;
     };
 
-    utils.getRoomFeatures = async function (_converse, room, server, features=[]) {
-        const muc_jid = `${room}@${server}`.toLowerCase();
+    utils.getRoomFeatures = async function (_converse, muc_jid, features=[]) {
+        const room = Strophe.getNodeFromJid(muc_jid);
+        muc_jid = muc_jid.toLowerCase();
         const stanzas = _converse.connection.IQ_stanzas;
-        // XXX How necessary is this?
-        const index = stanzas.length-2;
-        const stanza = await u.waitUntil(() => _.filter(
-            stanzas.slice(index),
+        const stanza = await u.waitUntil(() => stanzas.filter(
             iq => iq.querySelector(
                 `iq[to="${muc_jid}"] query[xmlns="http://jabber.org/protocol/disco#info"]`
-            )).pop());
-
-
+            )).pop()
+        );
         const features_stanza = $iq({
             'from': muc_jid,
             'id': stanza.getAttribute('id'),
@@ -183,7 +180,6 @@
 
 
     utils.waitForReservedNick = async function (_converse, muc_jid, nick) {
-        const view = _converse.chatboxviews.get(muc_jid);
         const stanzas = _converse.connection.IQ_stanzas;
         const selector = `iq[to="${muc_jid.toLowerCase()}"] query[node="x-roomuser-item"]`;
         const iq = await u.waitUntil(() => stanzas.filter(s => sizzle(selector, s).length).pop());
@@ -196,7 +192,7 @@
         const stanza = $iq({
             'type': 'result',
             'id': IQ_id,
-            'from': view.model.get('jid'),
+            'from': muc_jid,
             'to': _converse.connection.jid
         }).c('query', {'xmlns': 'http://jabber.org/protocol/disco#info', 'node': 'x-roomuser-item'});
         if (nick) {
@@ -204,7 +200,7 @@
         }
         _converse.connection._dataRecv(utils.createRequest(stanza));
         if (nick) {
-            return u.waitUntil(() => view.model.get('nick'));
+            return u.waitUntil(() => nick);
         }
     };
 
@@ -296,15 +292,15 @@
 
     utils.openAndEnterChatRoom = async function (_converse, muc_jid, nick, features=[], members=[]) {
         muc_jid = muc_jid.toLowerCase();
-        const room = Strophe.getNodeFromJid(muc_jid);
-        const server = Strophe.getDomainFromJid(muc_jid);
-        await _converse.api.rooms.open(muc_jid);
-        await utils.getRoomFeatures(_converse, room, server, features);
+        const room_creation_promise = _converse.api.rooms.open(muc_jid);
+        await utils.getRoomFeatures(_converse, muc_jid, features);
         await utils.waitForReservedNick(_converse, muc_jid, nick);
         // The user has just entered the room (because join was called)
         // and receives their own presence from the server.
         // See example 24: https://xmpp.org/extensions/xep-0045.html#enter-pres
         utils.receiveOwnMUCPresence(_converse, muc_jid, nick);
+
+        await room_creation_promise;
         const view = _converse.chatboxviews.get(muc_jid);
         await u.waitUntil(() => (view.model.get('connection_status') === converse.ROOMSTATUS.ENTERED));
         if (_converse.muc_fetch_members) {