Don't duplicate disco items across two collections

We now no longer have an `.items` collection on a disco entity.
Instead, add a new API method `api.disco.entities.items` should be used.

This should solve the issue of the entities disappearing when reloading
the page.

Updates #2925

CHANGES.md

@@ -2,6 +2,7 @@
 
 ## Unreleased
 
+- #2925 File upload is not always enabled
 - Add a "Add to Contacts" button in MUC occupant modals
 
 ## 10.0.0 (2022-10-30)

src/headless/plugins/disco/api.js

@@ -195,19 +195,29 @@ export default {
                 }
                 if (_converse.disco_entities === undefined) {
                     // Happens during tests when disco lookups happen asynchronously after teardown.
-                    const msg = `Tried to look up entity ${jid} but _converse.disco_entities has been torn down`;
-                    log.warn(msg);
+                    log.warn(`Tried to look up entity ${jid} but _converse.disco_entities has been torn down`);
                     return;
                 }
                 const entity = _converse.disco_entities.get(jid);
                 if (entity || !create) {
                     return entity;
                 }
-                return api.disco.entities.create(jid);
+                return api.disco.entities.create({ jid });
             },
 
             /**
-             * Create a new disco entity. It's identity and features
+             * Return any disco items advertised on this entity
+             *
+             * @method api.disco.entities.items
+             * @param {string} jid The Jabber ID of the entity for which we want to fetch items
+             * @example api.disco.entities.items(jid);
+             */
+            items (jid) {
+                return _converse.disco_entities.filter(e => e.get('parent_jids')?.includes(jid));
+            },
+
+            /**
+             * Create a new  disco entity. It's identity and features
              * will automatically be fetched from cache or from the
              * XMPP server.
              *
@@ -215,14 +225,17 @@ export default {
              * `ignore_cache: true` in the options parameter.
              *
              * @method api.disco.entities.create
-             * @param {string} jid The Jabber ID of the entity
-             * @param {object} [options] Additional options
+             * @param {object} data
+             * @param {string} data.jid - The Jabber ID of the entity
+             * @param {string} data.parent_jid - The Jabber ID of the parent entity
+             * @param {string} data.name
+             * @param {object} [options] - Additional options
              * @param {boolean} [options.ignore_cache]
              *     If true, fetch all features from the XMPP server instead of restoring them from cache
-             * @example _converse.api.disco.entities.create(jid, {'ignore_cache': true});
+             * @example _converse.api.disco.entities.create({ jid }, {'ignore_cache': true});
              */
-            create (jid, options) {
-                return _converse.disco_entities.create({'jid': jid}, options);
+            create (data, options) {
+                return _converse.disco_entities.create(data, options);
             }
         },
 
@@ -249,22 +262,56 @@ export default {
              * api.disco.features.get(Strophe.NS.MAM, _converse.bare_jid);
              */
             async get (feature, jid) {
-                if (!jid) {
-                    throw new TypeError('You need to provide an entity JID');
-                }
-                await api.waitUntil('discoInitialized');
-                let entity = await api.disco.entities.get(jid, true);
+                if (!jid) throw new TypeError('You need to provide an entity JID');
+
+                const entity = await api.disco.entities.get(jid, true);
 
                 if (_converse.disco_entities === undefined && !api.connection.connected()) {
                     // Happens during tests when disco lookups happen asynchronously after teardown.
-                    const msg = `Tried to get feature ${feature} for ${jid} but _converse.disco_entities has been torn down`;
-                    log.warn(msg);
-                    return;
+                    log.warn(`Tried to get feature ${feature} for ${jid} but _converse.disco_entities has been torn down`);
+                    return [];
                 }
-                entity = await entity.waitUntilFeaturesDiscovered;
-                const promises = [...entity.items.map(i => i.hasFeature(feature)), entity.hasFeature(feature)];
+
+                const promises = [
+                    entity.getFeature(feature),
+                    ...api.disco.entities.items(jid).map(i => i.getFeature(feature))
+                ];
                 const result = await Promise.all(promises);
                 return result.filter(isObject);
+            },
+
+            /**
+             * Returns true if an entity with the given JID, or if one of its
+             * associated items, supports a given feature.
+             *
+             * @method api.disco.features.has
+             * @param {string} feature The feature that might be
+             *     supported. In the XML stanza, this is the `var`
+             *     attribute of the `<feature>` element. For
+             *     example: `http://jabber.org/protocol/muc`
+             * @param {string} jid The JID of the entity
+             *     (and its associated items) which should be queried
+             * @returns {Promise} A promise which resolves with a boolean
+             * @example
+             *      api.disco.features.has(Strophe.NS.MAM, _converse.bare_jid);
+             */
+            async has (feature, jid) {
+                if (!jid) throw new TypeError('You need to provide an entity JID');
+
+                const entity = await api.disco.entities.get(jid, true);
+
+                if (_converse.disco_entities === undefined && !api.connection.connected()) {
+                    // Happens during tests when disco lookups happen asynchronously after teardown.
+                    log.warn(`Tried to check if ${jid} supports feature ${feature}`);
+                    return false;
+                }
+
+                if (await entity.getFeature(feature)) {
+                    return true;
+                }
+
+                const result = await Promise.all(api.disco.entities.items(jid).map(i => i.getFeature(feature)));
+                return result.map(isObject).includes(true);
             }
         },
 
@@ -286,9 +333,8 @@ export default {
          *     // The feature is not supported
          * }
          */
-        async supports (feature, jid) {
-            const features = await api.disco.features.get(feature, jid) || [];
-            return features.length > 0;
+        supports (feature, jid) {
+            return api.disco.features.has(feature, jid);
         },
 
         /**
@@ -316,7 +362,7 @@ export default {
                 entity.queryInfo();
             } else {
                 // Create it if it doesn't exist
-                entity = await api.disco.entities.create(jid, {'ignore_cache': true});
+                entity = await api.disco.entities.create({ jid }, {'ignore_cache': true});
             }
             return entity.waitUntilFeaturesDiscovered;
         },

src/headless/plugins/disco/entity.js

@@ -1,8 +1,8 @@
-import log from "@converse/headless/log.js";
-import sizzle from "sizzle";
-import { Collection } from "@converse/skeletor/src/collection";
+import log from '@converse/headless/log.js';
+import sizzle from 'sizzle';
+import { Collection } from '@converse/skeletor/src/collection';
 import { Model } from '@converse/skeletor/src/model.js';
-import { _converse, api, converse } from "@converse/headless/core.js";
+import { _converse, api, converse } from '@converse/headless/core.js';
 import { getOpenPromise } from '@converse/openpromise';
 
 const { Strophe } = converse.env;
@@ -19,7 +19,7 @@ const { Strophe } = converse.env;
 const DiscoEntity = Model.extend({
     idAttribute: 'jid',
 
-    async initialize (_, options) {
+    initialize (_, options) {
         this.waitUntilFeaturesDiscovered = getOpenPromise();
 
         this.dataforms = new Collection();
@@ -29,17 +29,12 @@ const DiscoEntity = Model.extend({
         this.features = new Collection();
         id = `converse.features-${this.get('jid')}`;
         this.features.browserStorage = _converse.createStore(id, 'session');
-        this.listenTo(this.features, 'add', this.onFeatureAdded)
+        this.listenTo(this.features, 'add', this.onFeatureAdded);
 
         this.fields = new Collection();
         id = `converse.fields-${this.get('jid')}`;
         this.fields.browserStorage = _converse.createStore(id, 'session');
-        this.listenTo(this.fields, 'add', this.onFieldAdded)
-
-        this.items = new _converse.DiscoEntities();
-        id = `converse.disco-items-${this.get('jid')}`;
-        this.items.browserStorage = _converse.createStore(id, 'session');
-        await new Promise(f => this.items.fetch({'success': f, 'error': f}));
+        this.listenTo(this.fields, 'add', this.onFieldAdded);
 
         this.identities = new Collection();
         id = `converse.identities-${this.get('jid')}`;
@@ -59,7 +54,7 @@ const DiscoEntity = Model.extend({
         await this.waitUntilFeaturesDiscovered;
         return this.identities.findWhere({
             'category': category,
-            'type': type
+            'type': type,
         });
     },
 
@@ -67,12 +62,12 @@ const DiscoEntity = Model.extend({
      * Returns a Promise which resolves with a map indicating
      * whether a given feature is supported.
      * @private
-     * @method _converse.DiscoEntity#hasFeature
+     * @method _converse.DiscoEntity#getFeature
      * @param { String } feature - The feature that might be supported.
      */
-    async hasFeature (feature) {
-        await this.waitUntilFeaturesDiscovered
-        if (this.features.findWhere({'var': feature})) {
+    async getFeature (feature) {
+        await this.waitUntilFeaturesDiscovered;
+        if (this.features.findWhere({ 'var': feature })) {
             return this;
         }
     },
@@ -106,7 +101,7 @@ const DiscoEntity = Model.extend({
         } else {
             const store_id = this.features.browserStorage.name;
             const result = await this.features.browserStorage.store.getItem(store_id);
-            if (result && result.length === 0 || result === null) {
+            if ((result && result.length === 0) || result === null) {
                 this.queryInfo();
             } else {
                 this.features.fetch({
@@ -114,9 +109,9 @@ const DiscoEntity = Model.extend({
                     success: () => {
                         this.waitUntilFeaturesDiscovered.resolve(this);
                         this.trigger('featuresDiscovered');
-                    }
+                    },
                 });
-                this.identities.fetch({add: true});
+                this.identities.fetch({ add: true });
             }
         }
     },
@@ -135,22 +130,27 @@ const DiscoEntity = Model.extend({
 
     onDiscoItems (stanza) {
         sizzle(`query[xmlns="${Strophe.NS.DISCO_ITEMS}"] item`, stanza).forEach(item => {
-            if (item.getAttribute("node")) {
+            if (item.getAttribute('node')) {
                 // XXX: Ignore nodes for now.
                 // See: https://xmpp.org/extensions/xep-0030.html#items-nodes
                 return;
             }
             const jid = item.getAttribute('jid');
-            if (this.items.get(jid) === undefined) {
-                const entities = _converse.disco_entities;
-                const entity = entities.get(jid) || entities.create({ jid, name: item.getAttribute('name') });
-                this.items.create(entity);
+            const entity = _converse.disco_entities.get(jid);
+            if (entity) {
+                entity.set({ parent_jids: [this.get('jid')] });
+            } else {
+                api.disco.entities.create({
+                    jid,
+                    'parent_jids': [this.get('jid')],
+                    'name': item.getAttribute('name'),
+                });
             }
         });
     },
 
     async queryForItems () {
-        if (this.identities.where({'category': 'server'}).length === 0) {
+        if (this.identities.where({ 'category': 'server' }).length === 0) {
             // Don't fetch features and items if this is not a
             // server or a conference component.
             return;
@@ -164,7 +164,7 @@ const DiscoEntity = Model.extend({
             this.identities.create({
                 'category': identity.getAttribute('category'),
                 'type': identity.getAttribute('type'),
-                'name': identity.getAttribute('name')
+                'name': identity.getAttribute('name'),
             });
         });
 
@@ -173,7 +173,7 @@ const DiscoEntity = Model.extend({
             sizzle('field', form).forEach(field => {
                 data[field.getAttribute('var')] = {
                     'value': field.querySelector('value')?.textContent,
-                    'type': field.getAttribute('type')
+                    'type': field.getAttribute('type'),
                 };
             });
             this.dataforms.create(data);
@@ -185,7 +185,7 @@ const DiscoEntity = Model.extend({
         Array.from(stanza.querySelectorAll('feature')).forEach(feature => {
             this.features.create({
                 'var': feature.getAttribute('var'),
-                'from': stanza.getAttribute('from')
+                'from': stanza.getAttribute('from'),
             });
         });
 
@@ -194,13 +194,13 @@ const DiscoEntity = Model.extend({
             this.fields.create({
                 'var': field.getAttribute('var'),
                 'value': field.querySelector('value')?.textContent,
-                'from': stanza.getAttribute('from')
+                'from': stanza.getAttribute('from'),
             });
         });
 
         this.waitUntilFeaturesDiscovered.resolve(this);
         this.trigger('featuresDiscovered');
-    }
+    },
 });
 
 export default DiscoEntity;

src/headless/plugins/disco/tests/disco.js

@@ -4,7 +4,7 @@ describe("Service Discovery", function () {
 
     describe("Whenever a server is queried for its features", function () {
 
-        it("stores the features it receives",
+        fit("stores the features it receives",
             mock.initConverse(
                 ['discoInitialized'], {},
                 async function (_converse) {
@@ -160,11 +160,14 @@ describe("Service Discovery", function () {
                 'plays.shakespeare.lit',
                 'words.shakespeare.lit'
             ]);
+            const { api, domain } = _converse;
             let entity = entities.get(_converse.domain);
-            expect(entity.items.length).toBe(3);
-            expect(entity.items.pluck('jid').includes('people.shakespeare.lit')).toBeTruthy();
-            expect(entity.items.pluck('jid').includes('plays.shakespeare.lit')).toBeTruthy();
-            expect(entity.items.pluck('jid').includes('words.shakespeare.lit')).toBeTruthy();
+            expect(api.disco.entities.items(domain).length).toBe(3);
+
+            expect(api.disco.entities.items(domain).map(e => e.get('jid'))).toEqual(
+                ['people.shakespeare.lit', 'plays.shakespeare.lit', 'words.shakespeare.lit']
+            )
+
             expect(entity.identities.where({'category': 'conference'}).length).toBe(1);
             expect(entity.identities.where({'category': 'directory'}).length).toBe(1);
 

src/headless/plugins/disco/utils.js

@@ -69,7 +69,7 @@ export async function initializeDisco () {
     const collection = await _converse.disco_entities.fetchEntities();
     if (collection.length === 0 || !collection.get(_converse.domain)) {
         // If we don't have an entity for our own XMPP server, create one.
-        _converse.disco_entities.create({'jid': _converse.domain});
+        api.disco.entities.create({'jid': _converse.domain}, {'ignore_cache': true});
     }
     /**
      * Triggered once the `converse-disco` plugin has been initialized and the

src/headless/plugins/smacks/tests/smacks.js

@@ -49,14 +49,13 @@ describe("XEP-0198 Stream Management", function () {
             `<iq id="${IQ_stanzas[1].getAttribute('id')}" type="get" xmlns="jabber:client"><query xmlns="jabber:iq:roster"/></iq>`);
         await mock.waitForRoster(_converse, 'current', 1);
 
-        const omemo_iq = IQ_stanzas[2];
-        expect(Strophe.serialize(omemo_iq)).toBe(
-            `<iq from="romeo@montague.lit" id="${omemo_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>`);
+        expect(Strophe.serialize(IQ_stanzas[2])).toBe(
+            `<iq from="romeo@montague.lit/orchard" id="${IQ_stanzas[2].getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
+                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
 
         expect(Strophe.serialize(IQ_stanzas[3])).toBe(
-            `<iq from="romeo@montague.lit/orchard" id="${IQ_stanzas[3].getAttribute('id')}" to="romeo@montague.lit" type="get" xmlns="jabber:client">`+
-                `<query xmlns="http://jabber.org/protocol/disco#info"/></iq>`);
+            `<iq from="romeo@montague.lit" id="${IQ_stanzas[3].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>`);
 
         expect(Strophe.serialize(IQ_stanzas[4])).toBe(
             `<iq from="romeo@montague.lit/orchard" id="${IQ_stanzas[4].getAttribute('id')}" type="set" xmlns="jabber:client"><enable xmlns="urn:xmpp:carbons:2"/></iq>`);
@@ -137,13 +136,13 @@ describe("XEP-0198 Stream Management", function () {
 
         iq = IQ_stanzas.pop();
         expect(Strophe.serialize(iq)).toBe(
-            `<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 from="romeo@montague.lit" id="${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.pop();
         expect(Strophe.serialize(iq)).toBe(
-            `<iq from="romeo@montague.lit" id="${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 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>`);
 
         expect(IQ_stanzas.filter(iq => sizzle('query[xmlns="jabber:iq:roster"]', iq).pop()).length).toBe(0);
     }));

src/plugins/chatview/tests/http-file-upload.js

@@ -9,6 +9,7 @@ describe("XEP-0363: HTTP File Upload", function () {
     describe("Discovering support", function () {
 
         it("is done automatically", mock.initConverse(['chatBoxesFetched'], {}, async function (_converse) {
+            const { api } = _converse;
             const IQ_stanzas = _converse.connection.IQ_stanzas;
             await mock.waitUntilDiscoConfirmed(_converse, _converse.bare_jid, [], []);
             let selector = 'iq[to="montague.lit"] query[xmlns="http://jabber.org/protocol/disco#info"]';
@@ -70,17 +71,17 @@ describe("XEP-0363: HTTP File Upload", function () {
 
             _converse.connection._dataRecv(mock.createRequest(stanza));
 
-            let entities = await _converse.api.disco.entities.get();
+            let entities = await api.disco.entities.get();
             expect(entities.length).toBe(3);
             expect(entities.pluck('jid')).toEqual(['montague.lit', 'romeo@montague.lit', 'upload.montague.lit']);
 
             expect(entities.get(_converse.domain).features.length).toBe(2);
             expect(entities.get(_converse.domain).identities.length).toBe(1);
 
-            _converse.api.disco.entities.get().then(entities => {
+            api.disco.entities.get().then(entities => {
                 expect(entities.length).toBe(3);
                 expect(entities.pluck('jid')).toEqual(['montague.lit', 'romeo@montague.lit', 'upload.montague.lit']);
-                expect(entities.get('montague.lit').items.length).toBe(1);
+                expect(api.disco.entities.items('montague.lit').length).toBe(1);
                 // Converse.js sees that the entity has a disco#info feature, so it will make a query for it.
                 const selector = 'iq[to="upload.montague.lit"] query[xmlns="http://jabber.org/protocol/disco#info"]';
                 return u.waitUntil(() => IQ_stanzas.filter(iq => iq.querySelector(selector)).length > 0);
@@ -126,7 +127,9 @@ describe("XEP-0363: HTTP File Upload", function () {
             _converse.connection._dataRecv(mock.createRequest(stanza));
 
             entities = await _converse.api.disco.entities.get();
-            expect(entities.get('montague.lit').items.get('upload.montague.lit').identities.where({'category': 'store'}).length).toBe(1);
+            const entity = await api.disco.entities.get('upload.montague.lit');
+            expect(entity.get('parent_jids')).toEqual(['montague.lit']);
+            expect(entity.identities.where({'category': 'store'}).length).toBe(1);
             const supported = await _converse.api.disco.supports(Strophe.NS.HTTPUPLOAD, _converse.domain);
             expect(supported).toBe(true);
             const features = await _converse.api.disco.features.get(Strophe.NS.HTTPUPLOAD, _converse.domain);
@@ -282,6 +285,7 @@ describe("XEP-0363: HTTP File Upload", function () {
                 it("shows an error message if the file is too large",
                         mock.initConverse(['chatBoxesFetched'], {}, async function (_converse) {
 
+                    const { api } = _converse;
                     const IQ_stanzas = _converse.connection.IQ_stanzas;
                     const IQ_ids =  _converse.connection.IQ_ids;
 
@@ -290,10 +294,9 @@ describe("XEP-0363: HTTP File Upload", function () {
                         iq => iq.querySelector('iq[to="montague.lit"] query[xmlns="http://jabber.org/protocol/disco#info"]')).length
                     );
 
-                    let stanza = IQ_stanzas.find(function (iq) {
-                        return iq.querySelector(
-                            'iq[to="montague.lit"] query[xmlns="http://jabber.org/protocol/disco#info"]');
-                    });
+                    let stanza = IQ_stanzas.find((iq) =>
+                        iq.querySelector('iq[to="montague.lit"] query[xmlns="http://jabber.org/protocol/disco#info"]'));
+
                     const info_IQ_id = IQ_ids[IQ_stanzas.indexOf(stanza)];
                     stanza = $iq({
                         'type': 'result',
@@ -340,7 +343,7 @@ describe("XEP-0363: HTTP File Upload", function () {
                     expect(entities.get(_converse.domain).features.length).toBe(2);
                     expect(entities.get(_converse.domain).identities.length).toBe(1);
                     expect(entities.pluck('jid')).toEqual(['montague.lit', 'romeo@montague.lit', 'upload.montague.lit']);
-                    expect(entities.get('montague.lit').items.length).toBe(1);
+                    expect(api.disco.entities.items('montague.lit').length).toBe(1);
                     await u.waitUntil(function () {
                         // Converse.js sees that the entity has a disco#info feature,
                         // so it will make a query for it.
@@ -368,7 +371,9 @@ describe("XEP-0363: HTTP File Upload", function () {
                                     .c('value').t('5242880');
                     _converse.connection._dataRecv(mock.createRequest(stanza));
                     entities = await _converse.api.disco.entities.get();
-                    expect(entities.get('montague.lit').items.get('upload.montague.lit').identities.where({'category': 'store'}).length).toBe(1);
+                    const entity = await api.disco.entities.get('upload.montague.lit');
+                    expect(entity.get('parent_jids')).toEqual(['montague.lit']);
+                    expect(entity.identities.where({'category': 'store'}).length).toBe(1);
                     await _converse.api.disco.supports(Strophe.NS.HTTPUPLOAD, _converse.domain);
                     await mock.waitForRoster(_converse, 'current');
 

src/plugins/muc-views/tests/mentions.js

@@ -507,7 +507,7 @@ describe("A sent groupchat message", function () {
             message_form.onKeyDown(enter_event);
             await u.waitUntil(() => view.querySelectorAll('.chat-msg__text').length);
 
-            const msg = _converse.connection.send.calls.all()[0].args[0];
+            const msg = _converse.connection.send.calls.all()[1].args[0];
             expect(Strophe.serialize(msg))
                 .toBe(`<message from="romeo@montague.lit/orchard" id="${msg.getAttribute("id")}" `+
                         `to="lounge@montague.lit" type="groupchat" `+

src/plugins/muc-views/tests/unfurls.js

@@ -428,7 +428,7 @@ describe("A Groupchat Message", function () {
         expect(view.querySelector('.chat-msg__text').textContent)
             .toBe(unfurl_url);
 
-        let msg = _converse.connection.send.calls.all()[0].args[0];
+        let msg = _converse.connection.send.calls.all()[1].args[0];
         expect(Strophe.serialize(msg))
         .toBe(
             `<message from="${_converse.jid}" id="${msg.getAttribute('id')}" to="${muc_jid}" type="groupchat" xmlns="jabber:client">`+