Don't override initialization settings via api.settings.update

This is a problem that could occur when calling converse.initialize twice

spec/converse.js

@@ -2,17 +2,34 @@
 
 describe("Converse", function() {
 
+    describe("Settings", function () {
+
+        it("extended via settings.update don't override settings passed in via converse.initialize",
+                mock.initConverse([], {'emoji_categories': {"travel": ":rocket:"}}, (done, _converse) => {
+
+            expect(_converse.api.settings.get('emoji_categories')?.travel).toBe(':rocket:');
+
+            // Test that the update command doesn't override user-provided site
+            // settings (i.e. settings passed in via converse.initialize).
+            _converse.api.settings.update({'emoji_categories': {"travel": ":motorcycle:", "food": ":burger:"}});
+
+            expect(_converse.api.settings.get('emoji_categories')?.travel).toBe(':rocket:');
+            expect(_converse.api.settings.get('emoji_categories')?.food).toBe(undefined);
+            done();
+        }));
+    });
+
     describe("Authentication", function () {
 
         it("needs either a bosh_service_url a websocket_url or both", mock.initConverse(async (done, _converse) => {
             const url = _converse.bosh_service_url;
             const connection = _converse.connection;
-            delete _converse.bosh_service_url;
+            _converse.api.settings.set('bosh_service_url', undefined);
             delete _converse.connection;
             try {
                 await _converse.initConnection();
             } catch (e) {
-                _converse.bosh_service_url = url;
+                _converse.api.settings.set('bosh_service_url', url);
                 _converse.connection = connection;
                 expect(e.message).toBe("initConnection: you must supply a value for either the bosh_service_url or websocket_url or both.");
                 done();

spec/utils.js

@@ -1,58 +0,0 @@
-describe("Converse.js Utilities", function() {
-
-    it("applySiteSettings: recursively applies user settings", function () {
-        const context = {};
-        const settings = {
-            show_toolbar: true,
-            chatview_avatar_width: 32,
-            chatview_avatar_height: 32,
-            auto_join_rooms: [],
-            visible_toolbar_buttons: {
-                'emojis': true,
-                'call': false,
-                'clear': true,
-                'toggle_occupants': true
-            }
-        };
-        Object.assign(context, settings);
-
-        let user_settings = {
-            something_else: 'xxx',
-            show_toolbar: false,
-            chatview_avatar_width: 32,
-            chatview_avatar_height: 48,
-            auto_join_rooms: [
-                'anonymous@conference.nomnom.im',
-            ],
-            visible_toolbar_buttons: {
-                'emojis': false,
-                'call': false,
-                'toggle_occupants':false,
-                'invalid': false
-            }
-        };
-        const utils = converse.env.utils;
-        utils.applySiteSettings(context, settings, user_settings);
-
-        expect(context.something_else).toBeUndefined();
-        expect(context.show_toolbar).toBeFalsy();
-        expect(context.chatview_avatar_width).toBe(32);
-        expect(context.chatview_avatar_height).toBe(48);
-        expect(Object.keys(context.visible_toolbar_buttons)).toEqual(Object.keys(settings.visible_toolbar_buttons));
-        expect(context.visible_toolbar_buttons.emojis).toBeFalsy();
-        expect(context.visible_toolbar_buttons.call).toBeFalsy();
-        expect(context.visible_toolbar_buttons.toggle_occupants).toBeFalsy();
-        expect(context.visible_toolbar_buttons.invalid).toBeFalsy();
-        expect(context.auto_join_rooms.length).toBe(1);
-        expect(context.auto_join_rooms[0]).toBe('anonymous@conference.nomnom.im');
-
-        user_settings = {
-            visible_toolbar_buttons: {
-                'toggle_occupants': true
-            }
-        };
-        utils.applySiteSettings(context, settings, user_settings);
-        expect(Object.keys(context.visible_toolbar_buttons)).toEqual(Object.keys(settings.visible_toolbar_buttons));
-        expect(context.visible_toolbar_buttons.toggle_occupants).toBeTruthy();
-    });
-});

src/headless/converse-core.js

@@ -273,8 +273,19 @@ Object.assign(_converse, Events);
 pluggable.enable(_converse, '_converse', 'pluggable');
 
 
-// Populated via the _converse.api.users.settings API
-let user_settings;
+let user_settings; // User settings, populated via api.users.settings
+let initialization_settings = {}; // Container for settings passed in via converse.initialize
+
+
+function initSettings (settings) {
+    _converse.settings = {};
+    initialization_settings = settings;
+    // Allow only whitelisted settings to be overwritten via converse.initialize
+    const allowed_settings = pick(settings, Object.keys(DEFAULT_SETTINGS));
+    assignIn(_converse.settings, DEFAULT_SETTINGS, allowed_settings);
+    assignIn(_converse, DEFAULT_SETTINGS, allowed_settings); // FIXME: remove
+}
+
 
 function initUserSettings () {
     if (!_converse.bare_jid) {
@@ -641,8 +652,13 @@ export const api = _converse.api = {
          */
         update (settings) {
             u.merge(DEFAULT_SETTINGS, settings);
-            u.merge(_converse, settings);
-            u.applySiteSettings(_converse, settings, site_settings);
+            // When updating the settings, we need to avoid overwriting the
+            // initialization_settings (i.e. the settings passed in via converse.initialize).
+            const allowed_keys = Object.keys(DEFAULT_SETTINGS);
+            const allowed_site_settings = pick(initialization_settings, allowed_keys);
+            const updated_settings = assignIn(pick(settings, allowed_keys), allowed_site_settings);
+            u.merge(_converse.settings, updated_settings);
+            u.merge(_converse, updated_settings); // FIXME: remove
         },
 
         /**
@@ -1708,18 +1724,6 @@ async function initLocale () {
 }
 
 
-let site_settings;
-
-function initSettings (settings) {
-    _converse.settings = {};
-    assignIn(_converse.settings, DEFAULT_SETTINGS);
-    // Allow only whitelisted configuration attributes to be overwritten
-    assignIn(_converse.settings, pick(settings, Object.keys(DEFAULT_SETTINGS)));
-    assignIn(_converse, _converse.settings);
-    site_settings = settings;
-}
-
-
 function setUnloadEvent () {
     if ('onpagehide' in window) {
         // Pagehide gets thrown in more cases than unload. Specifically it

src/headless/utils/core.js

@@ -207,22 +207,6 @@ u.merge = function merge (first, second) {
     }
 };
 
-u.applySiteSettings = function applySiteSettings (context, settings, user_settings) {
-    /* Configuration settings might be nested objects. We only want to
-     * add settings which are whitelisted.
-     */
-    for (var k in settings) {
-        if (user_settings[k] === undefined) {
-            continue;
-        }
-        if (isObject(settings[k]) && !Array.isArray(settings[k])) {
-            applySiteSettings(context[k], settings[k], user_settings[k]);
-        } else {
-            context[k] = user_settings[k];
-        }
-    }
-};
-
 /**
  * Converts an HTML string into a DOM Node.
  * Expects that the HTML string has only one top-level element,