Fix roster caching and versioning issue.

- Always try to first get local cache before requesting the roster.
- Rename `roster_fetched` with `roster_cached`

spec/smacks.js

@@ -44,6 +44,13 @@
                 `<iq from="romeo@montague.lit/orchard" id="${iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
                     `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
 
+            iq = IQ_stanzas[IQ_stanzas.length-1];
+            expect(Strophe.serialize(iq)).toBe(
+                `<iq id="${iq.getAttribute('id')}" type="get" xmlns="jabber:client"><query xmlns="jabber:iq:roster"/></iq>`);
+
+            await test_utils.waitForRoster(_converse, 'current', 1);
+
+            iq = IQ_stanzas.pop();
             iq = IQ_stanzas.pop();
             expect(Strophe.serialize(iq)).toBe(
                 `<iq from="romeo@montague.lit/orchard" id="${iq.getAttribute('id')}" to="montague.lit" type="get" xmlns="jabber:client">`+
@@ -54,19 +61,13 @@
                 `<iq from="romeo@montague.lit" id="${disco_iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
                     `<pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="eu.siacs.conversations.axolotl.devicelist"/></pubsub></iq>`);
 
-            iq = IQ_stanzas[IQ_stanzas.length-1];
-            expect(Strophe.serialize(iq)).toBe(
-                `<iq id="${iq.getAttribute('id')}" type="get" xmlns="jabber:client"><query xmlns="jabber:iq:roster"/></iq>`);
-
-            await test_utils.waitForRoster(_converse, 'current', 1);
-
             expect(sent_stanzas.filter(s => (s.nodeName === 'r')).length).toBe(2);
             expect(_converse.session.get('unacked_stanzas').length).toBe(5);
 
             // test handling of acks
-            let ack = u.toStanza(`<a xmlns="urn:xmpp:sm:3" h="1"/>`);
+            let ack = u.toStanza(`<a xmlns="urn:xmpp:sm:3" h="2"/>`);
             _converse.connection._dataRecv(test_utils.createRequest(ack));
-            expect(_converse.session.get('unacked_stanzas').length).toBe(4);
+            expect(_converse.session.get('unacked_stanzas').length).toBe(3);
 
             // test handling of ack requests
             let r = u.toStanza(`<r xmlns="urn:xmpp:sm:3"/>`);
@@ -90,9 +91,9 @@
                 .c('feature', {'var': 'http://jabber.org/protocol/disco#items'});
             _converse.connection._dataRecv(test_utils.createRequest(disco_result));
 
-            ack = u.toStanza(`<a xmlns="urn:xmpp:sm:3" h="2"/>`);
+            ack = u.toStanza(`<a xmlns="urn:xmpp:sm:3" h="3"/>`);
             _converse.connection._dataRecv(test_utils.createRequest(ack));
-            expect(_converse.session.get('unacked_stanzas').length).toBe(3);
+            expect(_converse.session.get('unacked_stanzas').length).toBe(2);
 
             r = u.toStanza(`<r xmlns="urn:xmpp:sm:3"/>`);
             _converse.connection._dataRecv(test_utils.createRequest(r));
@@ -104,6 +105,7 @@
             IQ_stanzas = _converse.connection.IQ_stanzas;
             _converse.api.connection.reconnect();
             stanza = await u.waitUntil(() => sent_stanzas.filter(s => (s.tagName === 'resume')).pop());
+
             expect(Strophe.serialize(stanza)).toEqual('<resume h="2" previd="some-long-sm-id" xmlns="urn:xmpp:sm:3"/>');
 
             result = u.toStanza(`<resumed xmlns="urn:xmpp:sm:3" h="another-sequence-number" previd="some-long-sm-id"/>`);
@@ -113,7 +115,7 @@
             expect(sent_stanzas.filter(s => (s.tagName === 'enable')).length).toBe(1);
             expect(_converse.session.get('smacks_enabled')).toBe(true);
 
-            await u.waitUntil(() => IQ_stanzas.length === 2);
+            await u.waitUntil(() => IQ_stanzas.length === 1);
 
             // Test that unacked stanzas get resent out
             iq = IQ_stanzas.pop();
@@ -121,8 +123,6 @@
                 `<iq from="romeo@montague.lit/orchard" id="${iq.getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
                     `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
 
-            // We don't fetch the roster again because it's cached.
-            expect(_converse.session.get('roster_fetched')).toBeTruthy();
             expect(IQ_stanzas.filter(iq => sizzle('query[xmlns="jabber:iq:roster"]', iq).pop()).length).toBe(0);
 
             await _converse.api.waitUntil('statusInitialized');
@@ -163,20 +163,23 @@
                 `</failed>`);
             _converse.connection._dataRecv(test_utils.createRequest(result));
 
+            _converse.connection.IQ_stanzas = [];
+            const IQ_stanzas = _converse.connection.IQ_stanzas;
+
             // Session data gets reset
             expect(_converse.session.get('smacks_enabled')).toBe(false);
             expect(_converse.session.get('num_stanzas_handled')).toBe(0);
             expect(_converse.session.get('num_stanzas_handled_by_server')).toBe(0);
             expect(_converse.session.get('num_stanzas_since_last_ack')).toBe(0);
             expect(_converse.session.get('unacked_stanzas').length).toBe(0);
-            expect(_converse.session.get('roster_fetched')).toBeFalsy();
+            expect(_converse.session.get('roster_cached')).toBeFalsy();
+
 
             await u.waitUntil(() => sent_stanzas.filter(s => (s.tagName === 'enable')).length === 2);
             stanza = sent_stanzas.filter(s => (s.tagName === 'enable')).pop();
             expect(Strophe.serialize(stanza)).toEqual('<enable resume="true" xmlns="urn:xmpp:sm:3"/>');
 
             result = u.toStanza(`<enabled xmlns="urn:xmpp:sm:3" id="another-long-sm-id" resume="true"/>`);
-
             _converse.connection._dataRecv(test_utils.createRequest(result));
             expect(_converse.session.get('smacks_enabled')).toBe(true);
 

src/headless/converse-roster.js

@@ -103,39 +103,24 @@ converse.plugins.add('converse-roster', {
         _converse.populateRoster = async function (ignore_cache=false) {
             if (ignore_cache) {
                 _converse.send_initial_presence = true;
-                try {
-                    await _converse.roster.fetchFromServer();
-                    /**
-                     * Triggered once roster contacts have been fetched. Used by the
-                     * `converse-rosterview.js` plugin to know when it can start to show the roster.
-                     * @event _converse#rosterContactsFetched
-                     * @example _converse.api.listen.on('rosterContactsFetched', () => { ... });
-                     */
-                    _converse.api.trigger('rosterContactsFetched');
-                } catch (reason) {
-                    _converse.log(reason, Strophe.LogLevel.ERROR);
-                } finally {
-                    _converse.sendInitialPresence();
-                }
-            } else {
-                try {
-                    await _converse.rostergroups.fetchRosterGroups();
-                    /**
-                     * Triggered once roster groups have been fetched. Used by the
-                     * `converse-rosterview.js` plugin to know when it can start alphabetically
-                     * position roster groups.
-                     * @event _converse#rosterGroupsFetched
-                     * @example _converse.api.listen.on('rosterGroupsFetched', () => { ... });
-                     * @example _converse.api.waitUntil('rosterGroupsFetched').then(() => { ... });
-                     */
-                    _converse.api.trigger('rosterGroupsFetched');
-                    await _converse.roster.fetchRosterContacts();
-                    _converse.api.trigger('rosterContactsFetched');
-                } catch (reason) {
-                    _converse.log(reason, Strophe.LogLevel.ERROR);
-                } finally {
-                    _converse.sendInitialPresence();
-                }
+            }
+            try {
+                await _converse.rostergroups.fetchRosterGroups();
+                /**
+                 * Triggered once roster groups have been fetched. Used by the
+                 * `converse-rosterview.js` plugin to know when it can start alphabetically
+                 * position roster groups.
+                 * @event _converse#rosterGroupsFetched
+                 * @example _converse.api.listen.on('rosterGroupsFetched', () => { ... });
+                 * @example _converse.api.waitUntil('rosterGroupsFetched').then(() => { ... });
+                 */
+                _converse.api.trigger('rosterGroupsFetched');
+                await _converse.roster.fetchRosterContacts();
+                _converse.api.trigger('rosterContactsFetched');
+            } catch (reason) {
+                _converse.log(reason, Strophe.LogLevel.ERROR);
+            } finally {
+                _converse.sendInitialPresence();
             }
         };
 
@@ -438,33 +423,34 @@ converse.plugins.add('converse-roster', {
              * @returns {promise} Promise which resolves once the contacts have been fetched.
              */
             async fetchRosterContacts () {
-                if (_converse.session.get('roster_fetched')) {
-                    const result = await new Promise((resolve, reject) => {
-                        this.fetch({
-                            'add': true,
-                            'silent': true,
-                            'success': resolve,
-                            'error': (c, e) => reject(e)
-                        });
+                const result = await new Promise((resolve, reject) => {
+                    this.fetch({
+                        'add': true,
+                        'silent': true,
+                        'success': resolve,
+                        'error': (c, e) => reject(e)
                     });
-                    if (u.isErrorObject(result)) {
-                        _converse.log(result, Strophe.LogLevel.ERROR);
-                        _converse.session.set('roster_fetched', false)
-                    } else {
-                        /**
-                         * The contacts roster has been retrieved from the local cache (`sessionStorage`).
-                         * @event _converse#cachedRoster
-                         * @type { _converse.RosterContacts }
-                         * @example _converse.api.listen.on('cachedRoster', (items) => { ... });
-                         * @example _converse.api.waitUntil('cachedRoster').then(items => { ... });
-                         */
-                        _converse.api.trigger('cachedRoster', result);
-                        return;
-                    }
+                });
+                if (u.isErrorObject(result)) {
+                    _converse.log(result, Strophe.LogLevel.ERROR);
+                    // Force a full roster refresh
+                    _converse.session.set('roster_cached', false)
+                    this.data.save('version', undefined);
                 }
 
-                _converse.send_initial_presence = true;
-                return _converse.roster.fetchFromServer();
+                if (_converse.session.get('roster_cached')) {
+                    /**
+                     * The contacts roster has been retrieved from the local cache (`sessionStorage`).
+                     * @event _converse#cachedRoster
+                     * @type { _converse.RosterContacts }
+                     * @example _converse.api.listen.on('cachedRoster', (items) => { ... });
+                     * @example _converse.api.waitUntil('cachedRoster').then(items => { ... });
+                     */
+                    _converse.api.trigger('cachedRoster', result);
+                } else {
+                    _converse.send_initial_presence = true;
+                    return _converse.roster.fetchFromServer();
+                }
             },
 
             subscribeToSuggestedItems (msg) {
@@ -623,7 +609,7 @@ converse.plugins.add('converse-roster', {
             },
 
             rosterVersioningSupported () {
-                return !!(_converse.api.disco.stream.getFeature('ver', 'urn:xmpp:features:rosterver') && this.data.get('version'));
+                return _converse.api.disco.stream.getFeature('ver', 'urn:xmpp:features:rosterver') && this.data.get('version');
             },
 
             /**
@@ -653,7 +639,7 @@ converse.plugins.add('converse-roster', {
                     _converse.log(iq, Strophe.LogLevel.ERROR);
                     return _converse.log("Error while trying to fetch roster from the server", Strophe.LogLevel.ERROR);
                 }
-                _converse.session.save('roster_fetched', true);
+                _converse.session.save('roster_cached', true);
                 /**
                  * When the roster has been received from the XMPP server.
                  * See also the `cachedRoster` event further up, which gets called instead of
@@ -848,6 +834,11 @@ converse.plugins.add('converse-roster', {
         });
 
 
+        /**
+         * @class
+         * @namespace _converse.RosterGroups
+         * @memberOf _converse
+         */
         _converse.RosterGroups = _converse.Collection.extend({
             model: _converse.RosterGroup,
 
@@ -868,12 +859,13 @@ converse.plugins.add('converse-roster', {
                 }
             },
 
+            /**
+             * Fetches all the roster groups from sessionStorage.
+             * @private
+             * @method _converse.RosterGroups#fetchRosterGroups
+             * @returns { Promise } - A promise which resolves once the groups have been fetched.
+             */
             fetchRosterGroups () {
-                /* Fetches all the roster groups from sessionStorage.
-                *
-                * Returns a promise which resolves once the groups have been
-                * returned.
-                */
                 return new Promise(success => {
                     this.fetch({
                         success,
@@ -936,7 +928,7 @@ converse.plugins.add('converse-roster', {
             }
         }
 
-        _converse.api.listen.on('streamResumptionFailed', () => _converse.session.set('roster_fetched', false));
+        _converse.api.listen.on('streamResumptionFailed', () => _converse.session.set('roster_cached', false));
 
         _converse.api.listen.on('clearSession', () => {
             clearPresences();
@@ -996,7 +988,7 @@ converse.plugins.add('converse-roster', {
             }
             _converse.roster.onConnected();
             _converse.registerPresenceHandler();
-            _converse.populateRoster(reconnecting);
+            _converse.populateRoster(!_converse.connection.restored);
         });