🐛 Fixed GA labs flags not appearing enabled in settings API (#13681)

no issue

The way GA flags were introduced means that they stop existing in the `'labs'` setting in the db and are instead forced to always return `true` when checking the flag in the labs service. However, Admin which uses the flags fetches them via the `/settings/` API endpoint which was only returning the raw labs setting db value meaning GA flags appeared to be disabled unless the flag had previously been enabled and no settings save had occured.

- updated the settings bread service to replace the labs setting value with the JSON stringified output of `labs.getAll()` which is the ultimate source-of-truth for a feature being enabled/disabled
  - extracted `browse()` behaviour to an internal `_formatBrowse()` method so we can apply the same filtering/modification for output of `browse()` and `edit()`

Co-authored-by: Fabien O'Carroll <fabien@allou.is>

core/server/api/canary/utils/serializers/output/settings.js

@@ -55,9 +55,8 @@ module.exports = {
         this.browse(...arguments);
     },
 
-    edit(models, apiConfig, frame) {
-        const settingsKeyedJSON = _.keyBy(_.invokeMap(models, 'toJSON'), 'key');
-        this.browse(settingsKeyedJSON, apiConfig, frame);
+    edit() {
+        this.browse(...arguments);
     },
 
     download(bytes, apiConfig, frame) {

core/server/api/v3/utils/serializers/output/settings.js

@@ -55,9 +55,8 @@ module.exports = {
         this.browse(...arguments);
     },
 
-    edit(models, apiConfig, frame) {
-        const settingsKeyedJSON = _.keyBy(_.invokeMap(models, 'toJSON'), 'key');
-        this.browse(settingsKeyedJSON, apiConfig, frame);
+    edit() {
+        this.browse(...arguments);
     },
 
     download(bytes, apiConfig, frame) {

core/server/services/settings/index.js

@@ -4,6 +4,7 @@
  */
 const events = require('../../lib/common/events');
 const models = require('../../models');
+const labs = require('../../../shared/labs');
 const SettingsCache = require('../../../shared/settings-cache');
 const SettingsBREADService = require('./settings-bread-service');
 const {obfuscatedSetting, isSecretSetting, hideValueIfSecret} = require('./settings-utils');
@@ -14,7 +15,8 @@ const {obfuscatedSetting, isSecretSetting, hideValueIfSecret} = require('./setti
 const getSettingsBREADServiceInstance = () => {
     return new SettingsBREADService({
         SettingsModel: models.Settings,
-        settingsCache: SettingsCache
+        settingsCache: SettingsCache,
+        labsService: labs
     });
 };
 

core/server/services/settings/settings-bread-service.js

@@ -14,10 +14,12 @@ class SettingsBREADService {
      * @param {Object} options
      * @param {Object} options.SettingsModel
      * @param {Object} options.settingsCache - SettingsCache instance
+     * @param {Object} options.labsService - labs service instance
      */
-    constructor({SettingsModel, settingsCache}) {
+    constructor({SettingsModel, settingsCache, labsService}) {
         this.SettingsModel = SettingsModel;
         this.settingsCache = settingsCache;
+        this.labs = labsService;
     }
 
     /**
@@ -28,24 +30,7 @@ class SettingsBREADService {
     browse(context) {
         let settings = this.settingsCache.getAll();
 
-        // CASE: no context passed (functional call)
-        if (!context) {
-            return Promise.resolve(settings.filter((setting) => {
-                return setting.group === 'site';
-            }));
-        }
-
-        if (!context.internal) {
-            // CASE: omit core settings unless internal request
-            settings = _.filter(settings, (setting) => {
-                const isCore = setting.group === 'core';
-                return !isCore;
-            });
-            // CASE: omit secret settings unless internal request
-            settings = settings.map(hideValueIfSecret);
-        }
-
-        return settings;
+        return this._formatBrowse(settings, context);
     }
 
     /**
@@ -86,6 +71,12 @@ class SettingsBREADService {
             }));
         }
 
+        // NOTE: Labs flags can exist outside of the DB when they are forced on/off
+        //       so we grab them from the labs service instead as that's source-of-truth
+        if (setting.key === 'labs') {
+            setting.value = JSON.stringify(this.labs.getAll());
+        }
+
         setting = hideValueIfSecret(setting);
 
         return {
@@ -161,7 +152,9 @@ class SettingsBREADService {
             });
         }
 
-        return this.SettingsModel.edit(filteredSettings, options);
+        return this.SettingsModel.edit(filteredSettings, options).then((result) => {
+            return this._formatBrowse(_.keyBy(_.invokeMap(result, 'toJSON'), 'key'), options.context);
+        });
     }
 
     /**
@@ -183,6 +176,35 @@ class SettingsBREADService {
             }
         }
     }
+
+    _formatBrowse(inputSettings, context) {
+        let settings = _.values(inputSettings);
+        // CASE: no context passed (functional call)
+        if (!context) {
+            return Promise.resolve(settings.filter((setting) => {
+                return setting.group === 'site';
+            }));
+        }
+
+        if (!context.internal) {
+            // CASE: omit core settings unless internal request
+            settings = _.filter(settings, (setting) => {
+                const isCore = setting.group === 'core';
+                return !isCore;
+            });
+            // CASE: omit secret settings unless internal request
+            settings = settings.map(hideValueIfSecret);
+        }
+
+        // NOTE: Labs flags can exist outside of the DB when they are forced on/off
+        //       so we grab them from the labs service instead as that's source-of-truth
+        const labsSetting = settings.find(setting => setting.key === 'labs');
+        if (labsSetting) {
+            labsSetting.value = JSON.stringify(this.labs.getAll());
+        }
+
+        return settings;
+    }
 }
 
 module.exports = SettingsBREADService;

test/e2e-api/admin/settings.test.js

@@ -6,6 +6,7 @@ const fs = require('fs-extra');
 const config = require('../../../core/shared/config');
 const testUtils = require('../../utils');
 const localUtils = require('./utils');
+const labs = require('../../../core/shared/labs');
 
 describe('Settings API', function () {
     let request;
@@ -221,7 +222,7 @@ describe('Settings API', function () {
         should.equal(putBody.settings[13].value, 'ua');
 
         putBody.settings[14].key.should.eql('labs');
-        should.equal(putBody.settings[14].value, JSON.stringify({}));
+        should.equal(putBody.settings[14].value, JSON.stringify(labs.getAll()));
 
         putBody.settings[15].key.should.eql('timezone');
         should.equal(putBody.settings[15].value, 'Pacific/Auckland');

test/regression/api/canary/admin/settings.test.js

@@ -1028,9 +1028,9 @@ describe('Settings API (canary)', function () {
             testUtils.API.checkResponseValue(jsonResponse.settings[0], ['id', 'group', 'key', 'value', 'type', 'flags', 'created_at', 'updated_at']);
             jsonResponse.settings[0].key.should.eql('labs');
 
-            jsonResponse.settings[0].value.should.eql(JSON.stringify({
-                activitypub: true
-            }));
+            const responseObj = JSON.parse(jsonResponse.settings[0].value);
+
+            should.not.exist(responseObj.gibberish);
         });
 
         it('Can\'t edit non existent setting', function () {