Fixes #2302 Bookmarks get duplicated on server push

package-lock.json

@@ -991,6 +991,34 @@
 				}
 			}
 		},
+		"@babel/helper-skip-transparent-expression-wrappers": {
+			"version": "7.12.1",
+			"resolved": "https://registry.npmjs.org/@babel/helper-skip-transparent-expression-wrappers/-/helper-skip-transparent-expression-wrappers-7.12.1.tgz",
+			"integrity": "sha512-Mf5AUuhG1/OCChOJ/HcADmvcHM42WJockombn8ATJG3OnyiSxBK/Mm5x78BQWvmtXZKHgbjdGL2kin/HOLlZGA==",
+			"dev": true,
+			"requires": {
+				"@babel/types": "^7.12.1"
+			},
+			"dependencies": {
+				"@babel/helper-validator-identifier": {
+					"version": "7.10.4",
+					"resolved": "https://registry.npmjs.org/@babel/helper-validator-identifier/-/helper-validator-identifier-7.10.4.tgz",
+					"integrity": "sha512-3U9y+43hz7ZM+rzG24Qe2mufW5KhvFg/NhnNph+i9mgCtdTCtMJuI1TMkrIUiK7Ix4PYlRF9I5dhqaLYA/ADXw==",
+					"dev": true
+				},
+				"@babel/types": {
+					"version": "7.12.1",
+					"resolved": "https://registry.npmjs.org/@babel/types/-/types-7.12.1.tgz",
+					"integrity": "sha512-BzSY3NJBKM4kyatSOWh3D/JJ2O3CVzBybHWxtgxnggaxEuaSTTDqeiSb/xk9lrkw2Tbqyivw5ZU4rT+EfznQsA==",
+					"dev": true,
+					"requires": {
+						"@babel/helper-validator-identifier": "^7.10.4",
+						"lodash": "^4.17.19",
+						"to-fast-properties": "^2.0.0"
+					}
+				}
+			}
+		},
 		"@babel/helper-split-export-declaration": {
 			"version": "7.10.4",
 			"resolved": "https://registry.npmjs.org/@babel/helper-split-export-declaration/-/helper-split-export-declaration-7.10.4.tgz",
@@ -1285,12 +1313,13 @@
 			}
 		},
 		"@babel/plugin-proposal-optional-chaining": {
-			"version": "7.10.4",
-			"resolved": "https://registry.npmjs.org/@babel/plugin-proposal-optional-chaining/-/plugin-proposal-optional-chaining-7.10.4.tgz",
-			"integrity": "sha512-ZIhQIEeavTgouyMSdZRap4VPPHqJJ3NEs2cuHs5p0erH+iz6khB0qfgU8g7UuJkG88+fBMy23ZiU+nuHvekJeQ==",
+			"version": "7.12.1",
+			"resolved": "https://registry.npmjs.org/@babel/plugin-proposal-optional-chaining/-/plugin-proposal-optional-chaining-7.12.1.tgz",
+			"integrity": "sha512-c2uRpY6WzaVDzynVY9liyykS+kVU+WRZPMPYpkelXH8KBt1oXoI89kPbZKKG/jDT5UK92FTW2fZkZaJhdiBabw==",
 			"dev": true,
 			"requires": {
 				"@babel/helper-plugin-utils": "^7.10.4",
+				"@babel/helper-skip-transparent-expression-wrappers": "^7.12.1",
 				"@babel/plugin-syntax-optional-chaining": "^7.8.0"
 			},
 			"dependencies": {

package.json

@@ -59,7 +59,7 @@
     "@babel/cli": "^7.10.3",
     "@babel/core": "^7.10.5",
     "@babel/plugin-proposal-nullish-coalescing-operator": "^7.10.1",
-    "@babel/plugin-proposal-optional-chaining": "^7.10.4",
+    "@babel/plugin-proposal-optional-chaining": "^7.12.1",
     "@babel/plugin-syntax-dynamic-import": "^7.2.0",
     "@babel/preset-env": "^7.10.2",
     "@converse/headless": "file:src/headless",

spec/bookmarks.js

@@ -318,52 +318,90 @@ describe("A chat room", function () {
 describe("Bookmarks", function () {
 
     it("can be pushed from the XMPP server", mock.initConverse(
-            ['rosterGroupsFetched', 'connected'], {}, async function (done, _converse) {
+            ['connected', 'rosterGroupsFetched', 'chatBoxesFetched'], {}, async function (done, _converse) {
 
         const { $msg, u } = converse.env;
         await mock.waitUntilBookmarksReturned(_converse);
 
         /* The stored data is automatically pushed to all of the user's
-            * connected resources.
-            *
-            * Publisher receives event notification
-            * -------------------------------------
-            * <message from='juliet@capulet.lit'
-            *         to='juliet@capulet.lit/balcony'
-            *         type='headline'
-            *         id='rnfoo1'>
-            * <event xmlns='http://jabber.org/protocol/pubsub#event'>
-            *     <items node='storage:bookmarks'>
-            *     <item id='current'>
-            *         <storage xmlns='storage:bookmarks'>
-            *         <conference name='The Play&apos;s the Thing'
-            *                     autojoin='true'
-            *                     jid='theplay@conference.shakespeare.lit'>
-            *             <nick>JC</nick>
-            *         </conference>
-            *         </storage>
-            *     </item>
-            *     </items>
-            * </event>
-            * </message>
-            */
-        const stanza = $msg({
+         * connected resources.
+         *
+         * Publisher receives event notification
+         * -------------------------------------
+         * <message from='juliet@capulet.lit'
+         *         to='juliet@capulet.lit/balcony'
+         *         type='headline'
+         *         id='rnfoo1'>
+         * <event xmlns='http://jabber.org/protocol/pubsub#event'>
+         *     <items node='storage:bookmarks'>
+         *     <item id='current'>
+         *         <storage xmlns='storage:bookmarks'>
+         *         <conference name='The Play&apos;s the Thing'
+         *                     autojoin='true'
+         *                     jid='theplay@conference.shakespeare.lit'>
+         *             <nick>JC</nick>
+         *         </conference>
+         *         </storage>
+         *     </item>
+         *     </items>
+         * </event>
+         * </message>
+         */
+        let stanza = $msg({
             'from': 'romeo@montague.lit',
-            'to': 'romeo@montague.lit/orchard',
+            'to': _converse.jid,
             'type': 'headline',
-            'id': 'rnfoo1'
+            'id': u.getUniqueId()
         }).c('event', {'xmlns': 'http://jabber.org/protocol/pubsub#event'})
             .c('items', {'node': 'storage:bookmarks'})
                 .c('item', {'id': 'current'})
                     .c('storage', {'xmlns': 'storage:bookmarks'})
-                        .c('conference', {'name': 'The Play&apos;s the Thing',
-                                        'autojoin': 'true',
-                                        'jid':'theplay@conference.shakespeare.lit'})
-                            .c('nick').t('JC');
+                        .c('conference', {
+                            'name': 'The Play&apos;s the Thing',
+                            'autojoin': 'true',
+                            'jid':'theplay@conference.shakespeare.lit'
+                        }).c('nick').t('JC').up().up()
+                        .c('conference', {
+                            'name': 'Another bookmark',
+                            'autojoin': 'false',
+                            'jid':'another@conference.shakespeare.lit'
+                        }).c('nick').t('JC');
         _converse.connection._dataRecv(mock.createRequest(stanza));
         await u.waitUntil(() => _converse.bookmarks.length);
-        expect(_converse.bookmarks.length).toBe(1);
+        expect(_converse.bookmarks.length).toBe(2);
+        expect(_converse.bookmarks.map(b => b.get('name'))).toEqual(['Another bookmark', 'The Play&apos;s the Thing']);
+        expect(_converse.chatboxviews.get('theplay@conference.shakespeare.lit')).not.toBeUndefined();
+
+        stanza = $msg({
+            'from': 'romeo@montague.lit',
+            'to': _converse.jid,
+            'type': 'headline',
+            'id': u.getUniqueId()
+        }).c('event', {'xmlns': 'http://jabber.org/protocol/pubsub#event'})
+            .c('items', {'node': 'storage:bookmarks'})
+                .c('item', {'id': 'current'})
+                    .c('storage', {'xmlns': 'storage:bookmarks'})
+                        .c('conference', {
+                            'name': 'The Play&apos;s the Thing',
+                            'autojoin': 'true',
+                            'jid':'theplay@conference.shakespeare.lit'
+                        }).c('nick').t('JC').up().up()
+                        .c('conference', {
+                            'name': 'Second bookmark',
+                            'autojoin': 'false',
+                            'jid':'another@conference.shakespeare.lit'
+                        }).c('nick').t('JC').up().up()
+                        .c('conference', {
+                            'name': 'Yet another bookmark',
+                            'autojoin': 'false',
+                            'jid':'yab@conference.shakespeare.lit'
+                        }).c('nick').t('JC');
+        _converse.connection._dataRecv(mock.createRequest(stanza));
+
+        await u.waitUntil(() => _converse.bookmarks.length === 3);
+        expect(_converse.bookmarks.map(b => b.get('name'))).toEqual(['Second bookmark', 'The Play&apos;s the Thing', 'Yet another bookmark']);
         expect(_converse.chatboxviews.get('theplay@conference.shakespeare.lit')).not.toBeUndefined();
+        expect(Object.keys(_converse.chatboxviews.getAll()).length).toBe(2);
         done();
     }));
 

src/headless/converse-bookmarks.js

@@ -16,6 +16,17 @@ const u = converse.env.utils;
 
 Strophe.addNamespace('BOOKMARKS', 'storage:bookmarks');
 
+
+function handleBookmarksPush (message) {
+    if (sizzle(`event[xmlns="${Strophe.NS.PUBSUB}#event"] items[node="${Strophe.NS.BOOKMARKS}"]`, message).length) {
+        api.waitUntil('bookmarksInitialized')
+            .then(() => _converse.bookmarks.createBookmarksFromStanza(message))
+            .catch(e => log.fatal(e));
+    }
+    return true;
+}
+
+
 converse.plugins.add('converse-bookmarks', {
 
     /* Plugin dependencies are other plugins which might be
@@ -92,6 +103,7 @@ converse.plugins.add('converse-bookmarks', {
         }
 
         _converse.Bookmark = Model.extend({
+            idAttribute: 'jid',
             getDisplayName () {
                 return Strophe.xmlunescape(this.get('name'));
             }
@@ -150,9 +162,9 @@ converse.plugins.add('converse-bookmarks', {
                         'from': _converse.connection.jid,
                     })
                     .c('pubsub', {'xmlns': Strophe.NS.PUBSUB})
-                        .c('publish', {'node': 'storage:bookmarks'})
+                        .c('publish', {'node': Strophe.NS.BOOKMARKS})
                             .c('item', {'id': 'current'})
-                                .c('storage', {'xmlns':'storage:bookmarks'});
+                                .c('storage', {'xmlns': Strophe.NS.BOOKMARKS});
                 this.forEach(model => {
                     stanza.c('conference', {
                         'name': model.get('name'),
@@ -186,7 +198,7 @@ converse.plugins.add('converse-bookmarks', {
                     'from': _converse.connection.jid,
                     'type': 'get',
                 }).c('pubsub', {'xmlns': Strophe.NS.PUBSUB})
-                    .c('items', {'node': 'storage:bookmarks'});
+                    .c('items', {'node': Strophe.NS.BOOKMARKS});
                 api.sendIQ(stanza)
                     .then(iq => this.onBookmarksReceived(deferred, iq))
                     .catch(iq => this.onBookmarksReceivedError(deferred, iq)
@@ -208,18 +220,18 @@ converse.plugins.add('converse-bookmarks', {
             },
 
             createBookmarksFromStanza (stanza) {
-                const bookmarks = sizzle(
-                    `items[node="storage:bookmarks"] item storage[xmlns="storage:bookmarks"] conference`,
-                    stanza
-                );
-                bookmarks.forEach(bookmark => {
-                    const jid = bookmark.getAttribute('jid');
-                    this.create({
+                const xmlns = Strophe.NS.BOOKMARKS;
+                const sel = `items[node="${xmlns}"] item storage[xmlns="${xmlns}"] conference`;
+                sizzle(sel, stanza).forEach(el => {
+                    const jid = el.getAttribute('jid');
+                    const bookmark = this.get(jid);
+                    const attrs = {
                         'jid': jid,
-                        'name': bookmark.getAttribute('name') || jid,
-                        'autojoin': bookmark.getAttribute('autojoin') === 'true',
-                        'nick': bookmark.querySelector('nick')?.textContent || ''
-                    });
+                        'name': el.getAttribute('name') || jid,
+                        'autojoin': el.getAttribute('autojoin') === 'true',
+                        'nick': el.querySelector('nick')?.textContent || ''
+                    }
+                    bookmark ? bookmark.save(attrs) : this.create(attrs);
                 });
             },
 
@@ -291,7 +303,7 @@ converse.plugins.add('converse-bookmarks', {
             }
         }
 
-        api.listen.on('addClientFeatures', () => { 
+        api.listen.on('addClientFeatures', () => {
             if (api.settings.get('allow_bookmarks')) {
                 api.disco.own.features.add(Strophe.NS.BOOKMARKS + '+notify')
             }
@@ -309,14 +321,8 @@ converse.plugins.add('converse-bookmarks', {
 
         api.listen.on('connected', async () =>  {
             // Add a handler for bookmarks pushed from other connected clients
-            _converse.connection.addHandler(message => {
-                if (sizzle('event[xmlns="'+Strophe.NS.PUBSUB+'#event"] items[node="storage:bookmarks"]', message).length) {
-                    api.waitUntil('bookmarksInitialized')
-                        .then(() => _converse.bookmarks.createBookmarksFromStanza(message))
-                        .catch(e => log.fatal(e));
-                }
-            }, null, 'message', 'headline', null, _converse.bare_jid);
-
+            const { connection } = _converse;
+            connection.addHandler(handleBookmarksPush, null, 'message', 'headline', null, _converse.bare_jid);
             await Promise.all([api.waitUntil('chatBoxesFetched')]);
             initBookmarks();
         });