Fixes #3362: Don't create empty nick element in bookmarks

CHANGES.md

@@ -10,6 +10,7 @@
 - #3305: New config option [muc_search_service](https://conversejs.org/docs/html/configuration.html#muc-search-service)
 - #3307: Fix inconsistency between browsers on textarea outlines
 - #3337: Correctly display multiline nested quotes
+- #3362: Don't create empty nick element in bookmarks
 - Fix: MUC occupant list does not sort itself on nicknames or roles changes
 - Fix: refresh the MUC sidebar when participants collection is sorted
 - Fix: room information not correctly refreshed when modifications are made by other users

karma.conf.js

@@ -24,6 +24,7 @@ module.exports = function(config) {
       },
       { pattern: "src/shared/tests/mock.js", type: 'module' },
 
+      { pattern: "src/headless/plugins/bookmarks/tests/bookmarks.js", type: 'module' },
       { pattern: "src/headless/plugins/caps/tests/caps.js", type: 'module' },
       { pattern: "src/headless/plugins/chat/tests/api.js", type: 'module' },
       { pattern: "src/headless/plugins/disco/tests/disco.js", type: 'module' },

src/headless/plugins/bookmarks/collection.js

@@ -1,11 +1,14 @@
+/**
+ * @typedef {import('@converse/headless').MUC} MUC
+ */
+import { Collection } from "@converse/skeletor";
+import { getOpenPromise } from '@converse/openpromise';
 import "../../plugins/muc/index.js";
 import Bookmark from './model.js';
 import _converse from '../../shared/_converse.js';
 import api from '../../shared/api/index.js';
 import converse from '../../shared/api/public.js';
 import log from "../../log.js";
-import { Collection } from "@converse/skeletor";
-import { getOpenPromise } from '@converse/openpromise';
 import { initStorage } from '../../utils/storage.js';
 
 const { Strophe, $iq, sizzle } = converse.env;
@@ -33,7 +36,7 @@ class Bookmarks extends Collection {
          * Triggered once the _converse.Bookmarks collection
          * has been created and cached bookmarks have been fetched.
          * @event _converse#bookmarksInitialized
-         * @type { Bookmarks }
+         * @type {Bookmarks}
          * @example _converse.api.listen.on('bookmarksInitialized', (bookmarks) => { ... });
          */
         api.trigger('bookmarksInitialized', this);
@@ -98,12 +101,19 @@ class Bookmarks extends Collection {
                 .c('publish', {'node': Strophe.NS.BOOKMARKS})
                     .c('item', {'id': 'current'})
                         .c('storage', {'xmlns': Strophe.NS.BOOKMARKS});
-        this.forEach(model => {
+
+        this.forEach(/** @param {MUC} model */(model) => {
             stanza.c('conference', {
                 'name': model.get('name'),
                 'autojoin': model.get('autojoin'),
                 'jid': model.get('jid'),
-            }).c('nick').t(model.get('nick')).up().up();
+            });
+            const nick = model.get('nick');
+            if (nick) {
+                stanza.c('nick').t(nick).up().up();
+            } else {
+                stanza.up();
+            }
         });
         stanza.up().up().up();
         stanza.c('publish-options')

src/headless/plugins/bookmarks/tests/bookmarks.js

@@ -0,0 +1,116 @@
+/* global mock, converse */
+
+describe("A chat room", function () {
+
+    describe("when autojoin is set", function () {
+
+        it("will be be opened and joined automatically upon login", mock.initConverse(
+                [], {}, async function (_converse) {
+
+            await mock.waitForRoster(_converse, 'current', 0);
+            await mock.waitUntilBookmarksReturned(_converse);
+            spyOn(_converse.api.rooms, 'create').and.callThrough();
+            const jid = 'theplay@conference.shakespeare.lit';
+            const model = _converse.bookmarks.create({
+                'jid': jid,
+                'autojoin': false,
+                'name':  'The Play',
+                'nick': ''
+            });
+            expect(_converse.api.rooms.create).not.toHaveBeenCalled();
+            _converse.bookmarks.remove(model);
+            _converse.bookmarks.create({
+                'jid': jid,
+                'autojoin': true,
+                'name':  'Hamlet',
+                'nick': ''
+            });
+            expect(_converse.api.rooms.create).toHaveBeenCalled();
+        }));
+    });
+});
+
+
+describe("A bookmark", function () {
+
+    it("can be created and sends out a stanza", mock.initConverse(
+            ['connected', 'chatBoxesFetched'], {}, async function (_converse) {
+
+        await mock.waitForRoster(_converse, 'current', 0);
+        await mock.waitUntilBookmarksReturned(_converse);
+
+        const jid = _converse.session.get('jid');
+        const muc1_jid = 'theplay@conference.shakespeare.lit';
+        const { Strophe, sizzle, u } = converse.env;
+
+        _converse.bookmarks.createBookmark({
+            jid: muc1_jid,
+            autojoin: true,
+            name:  'Hamlet',
+            nick: ''
+        });
+
+        const IQ_stanzas = _converse.api.connection.get().IQ_stanzas;
+        let sent_stanza = await u.waitUntil(
+            () => IQ_stanzas.filter(s => sizzle('item[id="current"]', s).length).pop());
+
+        expect(Strophe.serialize(sent_stanza)).toBe(
+            `<iq from="${jid}" id="${sent_stanza.getAttribute('id')}" type="set" xmlns="jabber:client">`+
+                '<pubsub xmlns="http://jabber.org/protocol/pubsub">'+
+                    '<publish node="storage:bookmarks">'+
+                        '<item id="current">'+
+                            '<storage xmlns="storage:bookmarks">'+
+                                `<conference autojoin="true" jid="${muc1_jid}" name="Hamlet"/>`+
+                            '</storage>'+
+                        '</item>'+
+                    '</publish>'+
+                    '<publish-options>'+
+                        '<x type="submit" xmlns="jabber:x:data">'+
+                            '<field type="hidden" var="FORM_TYPE">'+
+                                '<value>http://jabber.org/protocol/pubsub#publish-options</value>'+
+                            '</field>'+
+                            '<field var="pubsub#persist_items"><value>true</value></field>'+
+                            '<field var="pubsub#access_model"><value>whitelist</value></field>'+
+                        '</x>'+
+                    '</publish-options>'+
+                '</pubsub>'+
+            '</iq>');
+
+
+        const muc2_jid = 'balcony@conference.shakespeare.lit';
+        _converse.bookmarks.createBookmark({
+            jid: muc2_jid,
+            autojoin: true,
+            name:  'Balcony',
+            nick: 'romeo'
+        });
+
+        sent_stanza = await u.waitUntil(
+            () => IQ_stanzas.filter(s => sizzle('item[id="current"] conference[name="Balcony"]', s).length).pop());
+
+        expect(Strophe.serialize(sent_stanza)).toBe(
+            `<iq from="${jid}" id="${sent_stanza.getAttribute('id')}" type="set" xmlns="jabber:client">`+
+                '<pubsub xmlns="http://jabber.org/protocol/pubsub">'+
+                    '<publish node="storage:bookmarks">'+
+                        '<item id="current">'+
+                            '<storage xmlns="storage:bookmarks">'+
+                                `<conference autojoin="true" jid="${muc2_jid}" name="Balcony">`+
+                                    '<nick>romeo</nick>'+
+                                '</conference>'+
+                                `<conference autojoin="true" jid="${muc1_jid}" name="Hamlet"/>`+
+                            '</storage>'+
+                        '</item>'+
+                    '</publish>'+
+                    '<publish-options>'+
+                        '<x type="submit" xmlns="jabber:x:data">'+
+                            '<field type="hidden" var="FORM_TYPE">'+
+                                '<value>http://jabber.org/protocol/pubsub#publish-options</value>'+
+                            '</field>'+
+                            '<field var="pubsub#persist_items"><value>true</value></field>'+
+                            '<field var="pubsub#access_model"><value>whitelist</value></field>'+
+                        '</x>'+
+                    '</publish-options>'+
+                '</pubsub>'+
+            '</iq>');
+    }));
+});

src/plugins/bookmark-views/tests/bookmarks.js

@@ -187,7 +187,8 @@ describe("A chat room", function () {
             expect(room.get('nick')).toBe('Othello');
         }));
 
-        it("displays that it's bookmarked through its bookmark icon", mock.initConverse([], {}, async function (_converse) {
+        it("displays that it's bookmarked through its bookmark icon",
+                mock.initConverse([], {}, async function (_converse) {
 
             const { u } = converse.env;
             await mock.waitForRoster(_converse, 'current', 0);
@@ -293,33 +294,6 @@ describe("A chat room", function () {
             );
         }));
     });
-
-    describe("and when autojoin is set", function () {
-
-        it("will be be opened and joined automatically upon login", mock.initConverse(
-                [], {}, async function (_converse) {
-
-            await mock.waitForRoster(_converse, 'current', 0);
-            await mock.waitUntilBookmarksReturned(_converse);
-            spyOn(_converse.api.rooms, 'create').and.callThrough();
-            const jid = 'theplay@conference.shakespeare.lit';
-            const model = _converse.bookmarks.create({
-                'jid': jid,
-                'autojoin': false,
-                'name':  'The Play',
-                'nick': ''
-            });
-            expect(_converse.api.rooms.create).not.toHaveBeenCalled();
-            _converse.bookmarks.remove(model);
-            _converse.bookmarks.create({
-                'jid': jid,
-                'autojoin': true,
-                'name':  'Hamlet',
-                'nick': ''
-            });
-            expect(_converse.api.rooms.create).toHaveBeenCalled();
-        }));
-    });
 });
 
 describe("Bookmarks", function () {
@@ -407,7 +381,9 @@ describe("Bookmarks", function () {
         _converse.api.connection.get()._dataRecv(mock.createRequest(stanza));
 
         await u.waitUntil(() => _converse.bookmarks.length === 3);
-        expect(_converse.bookmarks.map(b => b.get('name'))).toEqual(['Second bookmark', 'The Play's the Thing', 'Yet another bookmark']);
+        expect(_converse.bookmarks.map(b => b.get('name'))).toEqual(
+            ['Second bookmark', 'The Play'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);
     }));
@@ -468,7 +444,7 @@ describe("Bookmarks", function () {
         expect(_converse.bookmarks.models.length).toBe(0);
 
         spyOn(_converse.bookmarks, 'onBookmarksReceived').and.callThrough();
-        var stanza = $iq({'to': _converse.api.connection.get().jid, 'type':'result', 'id':sent_stanza.getAttribute('id')})
+        const stanza = $iq({'to': _converse.api.connection.get().jid, 'type':'result', 'id':sent_stanza.getAttribute('id')})
             .c('pubsub', {'xmlns': Strophe.NS.PUBSUB})
                 .c('items', {'node': 'storage:bookmarks'})
                     .c('item', {'id': 'current'})